| Message ID | 20190809130534.9152-1-fbarrat@linux.ibm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,1/4] npu2: Rework phb-index assignments for virtual PHBs | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch master (0e1db80c70477d89a73c7f2a1a7e19c7d8292c5f) |
| snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot | success | Test snowpatch/job/snowpatch-skiboot on branch master |
| snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot-dco | success | Signed-off-by present |
On Fri, Aug 09, 2019 at 03:05:31PM +0200, Frederic Barrat wrote: >Until now, opencapi PHBs were not using the 'ibm,phb-index' property, >as it was thought unnecessary. For nvlink, a phb-index was associated >to the npu when parsing hdat data, and the nvlink PHB was reusing the >same value. > >It turns out it helps to have the 'ibm,phb-index' property for >opencapi PHBs after all. Otherwise it can lead to wrong results on >platforms like mihawk when trying to match entries in the slot >table. We end up with an opencapi device inheriting wrong properties >in the device tree, because match_slot_phb_entry() default to >phb-index 0 if it cannot find the property. Though it doesn't seem to >cause any harm, it's wrong and a future patch is expected to start >using the slot table for opencapi, so it needs fixing. > >The twist is that with opencapi, we can have multiple virtual PHBs for >a single NPU on P9. There's one PHB per (opencapi) brick. Therefore >there's no 1-to-1 mapping between the NPU and PHB index and it no >longer makes sense to associate a phb-index to a npu. > >With this patch, opencapi PHBs created under a NPU use a fixed mapping >for their phb-index, based on the brick index. The range of possible >values is 7 to 12. Because there can only be one nvlink PHB per NPU, >it is always using a phb-index of 7. > >A side effect is that 2 virtual PHBs on 2 different chips can have the >same phb-index, which is similar to what happens for 'real' PCI PHBs, >but is different from what was happening on a nvlink-only witherspoon >so far. > >Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com> Reviewed-by: Reza Arbab <arbab@linux.ibm.com>
On 9/8/19 3:05 pm, Frederic Barrat wrote: > Until now, opencapi PHBs were not using the 'ibm,phb-index' property, > as it was thought unnecessary. For nvlink, a phb-index was associated > to the npu when parsing hdat data, and the nvlink PHB was reusing the > same value. > > It turns out it helps to have the 'ibm,phb-index' property for > opencapi PHBs after all. Otherwise it can lead to wrong results on > platforms like mihawk when trying to match entries in the slot > table. We end up with an opencapi device inheriting wrong properties > in the device tree, because match_slot_phb_entry() default to > phb-index 0 if it cannot find the property. Though it doesn't seem to > cause any harm, it's wrong and a future patch is expected to start > using the slot table for opencapi, so it needs fixing. > > The twist is that with opencapi, we can have multiple virtual PHBs for > a single NPU on P9. There's one PHB per (opencapi) brick. Therefore > there's no 1-to-1 mapping between the NPU and PHB index and it no > longer makes sense to associate a phb-index to a npu. > > With this patch, opencapi PHBs created under a NPU use a fixed mapping > for their phb-index, based on the brick index. The range of possible > values is 7 to 12. Because there can only be one nvlink PHB per NPU, > it is always using a phb-index of 7. > > A side effect is that 2 virtual PHBs on 2 different chips can have the > same phb-index, which is similar to what happens for 'real' PCI PHBs, > but is different from what was happening on a nvlink-only witherspoon > so far. > > Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com> > --- Comment is good. Reviewed-by: Andrew Donnellan <ajd@linux.ibm.com> > Changelog: > v2: > - add comment summarizing the commit msg in the code (Andrew) > - move the axone changes to separate patch > > > hw/npu2-common.c | 1 - > hw/npu2-opencapi.c | 2 ++ > hw/npu2.c | 2 +- > include/npu2.h | 20 +++++++++++++++++++- > 4 files changed, 22 insertions(+), 3 deletions(-) > > diff --git a/hw/npu2-common.c b/hw/npu2-common.c > index 6d5c35af..7326e165 100644 > --- a/hw/npu2-common.c > +++ b/hw/npu2-common.c > @@ -530,7 +530,6 @@ static struct npu2 *setup_npu(struct dt_node *dn) > npu->index = dt_prop_get_u32(dn, "ibm,npu-index"); > npu->chip_id = gcid; > npu->xscom_base = dt_get_address(dn, 0, NULL); > - npu->phb_index = dt_prop_get_u32(dn, "ibm,phb-index"); > > init_lock(&npu->i2c_lock); > npu->i2c_pin_mode = ~0; // input mode by default > diff --git a/hw/npu2-opencapi.c b/hw/npu2-opencapi.c > index 9a391bb0..c8c95631 100644 > --- a/hw/npu2-opencapi.c > +++ b/hw/npu2-opencapi.c > @@ -1665,6 +1665,8 @@ static void setup_device(struct npu2_dev *dev) > dt_add_property_strings(dn_phb, "device_type", "pciex"); > dt_add_property(dn_phb, "reg", mm_win, sizeof(mm_win)); > dt_add_property_cells(dn_phb, "ibm,npu-index", dev->npu->index); > + dt_add_property_cells(dn_phb, "ibm,phb-index", > + npu2_get_phb_index(dev->brick_index)); > dt_add_property_cells(dn_phb, "ibm,chip-id", dev->npu->chip_id); > dt_add_property_cells(dn_phb, "ibm,xscom-base", dev->npu->xscom_base); > dt_add_property_cells(dn_phb, "ibm,npcq", dev->npu->dt_node->phandle); > diff --git a/hw/npu2.c b/hw/npu2.c > index 29c998f6..324d492f 100644 > --- a/hw/npu2.c > +++ b/hw/npu2.c > @@ -1478,7 +1478,7 @@ int npu2_nvlink_init_npu(struct npu2 *npu) > "ibm,ioda2-npu2-phb"); > dt_add_property_strings(np, "device_type", "pciex"); > dt_add_property(np, "reg", reg, sizeof(reg)); > - dt_add_property_cells(np, "ibm,phb-index", npu->phb_index); > + dt_add_property_cells(np, "ibm,phb-index", npu2_get_phb_index(0)); > dt_add_property_cells(np, "ibm,npu-index", npu->index); > dt_add_property_cells(np, "ibm,chip-id", npu->chip_id); > dt_add_property_cells(np, "ibm,xscom-base", npu->xscom_base); > diff --git a/include/npu2.h b/include/npu2.h > index 92b58988..5a8dc32b 100644 > --- a/include/npu2.h > +++ b/include/npu2.h > @@ -175,7 +175,6 @@ struct npu2 { > > /* NVLink */ > struct phb phb_nvlink; > - uint32_t phb_index; > > /* OCAPI */ > uint64_t i2c_port_id_ocapi; > @@ -251,4 +250,23 @@ int64_t npu2_map_lpar(struct phb *phb, uint64_t bdf, uint64_t lparid, > int64_t npu2_set_relaxed_order(struct phb *phb, uint32_t gcid, int pec, > bool enable); > > +#define NPU2_PHB_INDEX_BASE 7 > +/* to avoid conflicts with PCI and for historical reasons */ > + > +static inline int npu2_get_phb_index(unsigned int brick_index) > +{ > + /* > + * There's one virtual PHB per brick with opencapi, so we no > + * longer have a 1-to-1 mapping between a NPU and a virtual > + * PHB. And we want a static phb-index, as it is needed to use > + * a slot table on some platforms. So we associate a per-chip > + * phb-index based on the brick index. > + * > + * nvlink only creates one virtual PHB per chip, so it is > + * treated as if using brick 0, which is never used by > + * opencapi. > + */ > + return NPU2_PHB_INDEX_BASE + brick_index; > +} > + > #endif /* __NPU2_H */ >
diff --git a/hw/npu2-common.c b/hw/npu2-common.c index 6d5c35af..7326e165 100644 --- a/hw/npu2-common.c +++ b/hw/npu2-common.c @@ -530,7 +530,6 @@ static struct npu2 *setup_npu(struct dt_node *dn) npu->index = dt_prop_get_u32(dn, "ibm,npu-index"); npu->chip_id = gcid; npu->xscom_base = dt_get_address(dn, 0, NULL); - npu->phb_index = dt_prop_get_u32(dn, "ibm,phb-index"); init_lock(&npu->i2c_lock); npu->i2c_pin_mode = ~0; // input mode by default diff --git a/hw/npu2-opencapi.c b/hw/npu2-opencapi.c index 9a391bb0..c8c95631 100644 --- a/hw/npu2-opencapi.c +++ b/hw/npu2-opencapi.c @@ -1665,6 +1665,8 @@ static void setup_device(struct npu2_dev *dev) dt_add_property_strings(dn_phb, "device_type", "pciex"); dt_add_property(dn_phb, "reg", mm_win, sizeof(mm_win)); dt_add_property_cells(dn_phb, "ibm,npu-index", dev->npu->index); + dt_add_property_cells(dn_phb, "ibm,phb-index", + npu2_get_phb_index(dev->brick_index)); dt_add_property_cells(dn_phb, "ibm,chip-id", dev->npu->chip_id); dt_add_property_cells(dn_phb, "ibm,xscom-base", dev->npu->xscom_base); dt_add_property_cells(dn_phb, "ibm,npcq", dev->npu->dt_node->phandle); diff --git a/hw/npu2.c b/hw/npu2.c index 29c998f6..324d492f 100644 --- a/hw/npu2.c +++ b/hw/npu2.c @@ -1478,7 +1478,7 @@ int npu2_nvlink_init_npu(struct npu2 *npu) "ibm,ioda2-npu2-phb"); dt_add_property_strings(np, "device_type", "pciex"); dt_add_property(np, "reg", reg, sizeof(reg)); - dt_add_property_cells(np, "ibm,phb-index", npu->phb_index); + dt_add_property_cells(np, "ibm,phb-index", npu2_get_phb_index(0)); dt_add_property_cells(np, "ibm,npu-index", npu->index); dt_add_property_cells(np, "ibm,chip-id", npu->chip_id); dt_add_property_cells(np, "ibm,xscom-base", npu->xscom_base); diff --git a/include/npu2.h b/include/npu2.h index 92b58988..5a8dc32b 100644 --- a/include/npu2.h +++ b/include/npu2.h @@ -175,7 +175,6 @@ struct npu2 { /* NVLink */ struct phb phb_nvlink; - uint32_t phb_index; /* OCAPI */ uint64_t i2c_port_id_ocapi; @@ -251,4 +250,23 @@ int64_t npu2_map_lpar(struct phb *phb, uint64_t bdf, uint64_t lparid, int64_t npu2_set_relaxed_order(struct phb *phb, uint32_t gcid, int pec, bool enable); +#define NPU2_PHB_INDEX_BASE 7 +/* to avoid conflicts with PCI and for historical reasons */ + +static inline int npu2_get_phb_index(unsigned int brick_index) +{ + /* + * There's one virtual PHB per brick with opencapi, so we no + * longer have a 1-to-1 mapping between a NPU and a virtual + * PHB. And we want a static phb-index, as it is needed to use + * a slot table on some platforms. So we associate a per-chip + * phb-index based on the brick index. + * + * nvlink only creates one virtual PHB per chip, so it is + * treated as if using brick 0, which is never used by + * opencapi. + */ + return NPU2_PHB_INDEX_BASE + brick_index; +} + #endif /* __NPU2_H */
Until now, opencapi PHBs were not using the 'ibm,phb-index' property, as it was thought unnecessary. For nvlink, a phb-index was associated to the npu when parsing hdat data, and the nvlink PHB was reusing the same value. It turns out it helps to have the 'ibm,phb-index' property for opencapi PHBs after all. Otherwise it can lead to wrong results on platforms like mihawk when trying to match entries in the slot table. We end up with an opencapi device inheriting wrong properties in the device tree, because match_slot_phb_entry() default to phb-index 0 if it cannot find the property. Though it doesn't seem to cause any harm, it's wrong and a future patch is expected to start using the slot table for opencapi, so it needs fixing. The twist is that with opencapi, we can have multiple virtual PHBs for a single NPU on P9. There's one PHB per (opencapi) brick. Therefore there's no 1-to-1 mapping between the NPU and PHB index and it no longer makes sense to associate a phb-index to a npu. With this patch, opencapi PHBs created under a NPU use a fixed mapping for their phb-index, based on the brick index. The range of possible values is 7 to 12. Because there can only be one nvlink PHB per NPU, it is always using a phb-index of 7. A side effect is that 2 virtual PHBs on 2 different chips can have the same phb-index, which is similar to what happens for 'real' PCI PHBs, but is different from what was happening on a nvlink-only witherspoon so far. Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com> --- Changelog: v2: - add comment summarizing the commit msg in the code (Andrew) - move the axone changes to separate patch hw/npu2-common.c | 1 - hw/npu2-opencapi.c | 2 ++ hw/npu2.c | 2 +- include/npu2.h | 20 +++++++++++++++++++- 4 files changed, 22 insertions(+), 3 deletions(-)