Message ID | e9147aeafce7ab8ab901f5d7f04b4a5d7ba299e9.1547168645.git-series.andrew.donnellan@au1.ibm.com |
---|---|
State | Changes Requested |
Headers | show |
Series | Support OpenCAPI and NVLink devices on same NPU on Witherspoon | expand |
Context | Check | Description |
---|---|---|
snowpatch_ozlabs/apply_patch | warning | master/apply_patch Patch failed to apply |
snowpatch_ozlabs/apply_patch | fail | Failed to apply to any branch |
On 11/01/2019 12:09, Andrew Donnellan wrote: > In 68415d5e38ef ("hw/npu2: Common NPU2 init routine between NVLink and > OpenCAPI") we refactored a large chunk of the NPU init path into common > code, including walking the device tree to setup npu2_devs based on the > ibm,npu-link device tree nodes. > > We didn't actually remove the code that does that in > npu2_populate_devices(), so currently we populate the devices in the common > setup path Please add here "in setup_npu()". For a firmware there is way too many callbacks to follow the initialization path :( >, then repopulate them incorrectly in the NVLink setup path. > > Currently this is fine, because we don't support having both NVLink and > OpenCAPI devices on the same NPU, but when we do, this will be a problem. > > Fix npu2_populate_devices() to only populate additional fields as required > for NVLink devices. Rename it to npu2_configure_devices() to better reflect > what it now does. > > Signed-off-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com> > Reviewed-by: Frederic Barrat <fbarrat@linux.ibm.com> > > --- > v1->v2: > - remove unneeded scan_map assignment, it's zalloced anyway (Alexey) > - rebase on Reza's cleanups > --- > hw/npu2.c | 42 ++++++++++-------------------------------- > 1 file changed, 10 insertions(+), 32 deletions(-) > > diff --git a/hw/npu2.c b/hw/npu2.c > index 1c7af14958e8..106b32150994 100644 > --- a/hw/npu2.c > +++ b/hw/npu2.c > @@ -1599,12 +1599,12 @@ static void npu2_populate_cfg(struct npu2_dev *dev) > PCI_VIRT_CFG_INIT_RO(pvd, pos + 1, 1, 0); > } > > -static uint32_t npu_allocate_bdfn(struct npu2 *p, uint32_t group) > +static uint32_t npu_allocate_bdfn(struct npu2 *p, uint32_t group, int size) > { > int i; > int bdfn = (group << 3); > > - for (i = 0; i < p->total_devices; i++) { > + for (i = 0; i < size; i++) { > if ((p->devices[i].bdfn & 0xf8) == (bdfn & 0xf8)) > bdfn++; > } > @@ -1612,51 +1612,29 @@ static uint32_t npu_allocate_bdfn(struct npu2 *p, uint32_t group) > return bdfn; > } > > -static void npu2_populate_devices(struct npu2 *p, > - struct dt_node *dn) > +static void npu2_configure_devices(struct npu2 *p) > { > struct npu2_dev *dev; > - struct dt_node *npu2_dn, *link; > - uint32_t npu_phandle, index = 0; > - > - /* > - * Get the npu node which has the links which we expand here > - * into pci like devices attached to our emulated phb. > - */ > - npu_phandle = dt_prop_get_u32(dn, "ibm,npcq"); > - npu2_dn = dt_find_by_phandle(dt_root, npu_phandle); > - assert(npu2_dn); > + uint32_t index = 0; Do not need to initialize index here. > > - /* Walk the link@x nodes to initialize devices */ > - p->total_devices = 0; > - p->phb_nvlink.scan_map = 0; > - dt_for_each_compatible(npu2_dn, link, "ibm,npu-link") { > + for (index = 0; index < p->total_devices; index++) { > uint32_t group_id; > > dev = &p->devices[index]; > - dev->type = NPU2_DEV_TYPE_NVLINK; > - dev->npu = p; > - dev->dt_node = link; > - dev->link_index = dt_prop_get_u32(link, "ibm,npu-link-index"); > - dev->brick_index = dev->link_index; > + if (dev->type != NPU2_DEV_TYPE_NVLINK) > + continue; Took me some time to realize where the type is set (platforms/astbmc/witherspoon.c set_link_details()). Ok, safe. > > - group_id = dt_prop_get_u32(link, "ibm,npu-group-id"); > - dev->bdfn = npu_allocate_bdfn(p, group_id); > + group_id = dt_prop_get_u32(dev->dt_node, "ibm,npu-group-id"); > + dev->bdfn = npu_allocate_bdfn(p, group_id, index); > > /* This must be done after calling > * npu_allocate_bdfn() */ The comment is obsolete now. > - p->total_devices++; > p->phb_nvlink.scan_map |= 0x1 << ((dev->bdfn & 0xf8) >> 3); > > - dev->pl_xscom_base = dt_prop_get_u64(link, "ibm,npu-phy"); > - dev->lane_mask = dt_prop_get_u32(link, "ibm,npu-lane-mask"); > - > /* Initialize PCI virtual device */ > dev->nvlink.pvd = pci_virt_add_device(&p->phb_nvlink, dev->bdfn, 0x100, dev); > if (dev->nvlink.pvd) > npu2_populate_cfg(dev); > - > - index++; > } > } > > @@ -1857,7 +1835,7 @@ void npu2_nvlink_create_phb(struct npu2 *npu, struct dt_node *dn) > list_head_init(&npu->phb_nvlink.virt_devices); > > npu2_setup_irqs(npu); > - npu2_populate_devices(npu, dn); > + npu2_configure_devices(npu); > npu2_add_interrupt_map(npu, dn); > npu2_add_phb_properties(npu); > >
On 17/1/19 5:02 pm, Alexey Kardashevskiy wrote: > > > On 11/01/2019 12:09, Andrew Donnellan wrote: >> In 68415d5e38ef ("hw/npu2: Common NPU2 init routine between NVLink and >> OpenCAPI") we refactored a large chunk of the NPU init path into common >> code, including walking the device tree to setup npu2_devs based on the >> ibm,npu-link device tree nodes. >> >> We didn't actually remove the code that does that in >> npu2_populate_devices(), so currently we populate the devices in the common >> setup path > > Please add here "in setup_npu()". For a firmware there is way too many > callbacks to follow the initialization path :( OK. > > >> , then repopulate them incorrectly in the NVLink setup path. >> >> Currently this is fine, because we don't support having both NVLink and >> OpenCAPI devices on the same NPU, but when we do, this will be a problem. >> >> Fix npu2_populate_devices() to only populate additional fields as required >> for NVLink devices. Rename it to npu2_configure_devices() to better reflect >> what it now does. >> >> Signed-off-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com> >> Reviewed-by: Frederic Barrat <fbarrat@linux.ibm.com> >> >> --- >> v1->v2: >> - remove unneeded scan_map assignment, it's zalloced anyway (Alexey) >> - rebase on Reza's cleanups >> --- >> hw/npu2.c | 42 ++++++++++-------------------------------- >> 1 file changed, 10 insertions(+), 32 deletions(-) >> >> diff --git a/hw/npu2.c b/hw/npu2.c >> index 1c7af14958e8..106b32150994 100644 >> --- a/hw/npu2.c >> +++ b/hw/npu2.c >> @@ -1599,12 +1599,12 @@ static void npu2_populate_cfg(struct npu2_dev *dev) >> PCI_VIRT_CFG_INIT_RO(pvd, pos + 1, 1, 0); >> } >> >> -static uint32_t npu_allocate_bdfn(struct npu2 *p, uint32_t group) >> +static uint32_t npu_allocate_bdfn(struct npu2 *p, uint32_t group, int size) >> { >> int i; >> int bdfn = (group << 3); >> >> - for (i = 0; i < p->total_devices; i++) { >> + for (i = 0; i < size; i++) { >> if ((p->devices[i].bdfn & 0xf8) == (bdfn & 0xf8)) >> bdfn++; >> } >> @@ -1612,51 +1612,29 @@ static uint32_t npu_allocate_bdfn(struct npu2 *p, uint32_t group) >> return bdfn; >> } >> >> -static void npu2_populate_devices(struct npu2 *p, >> - struct dt_node *dn) >> +static void npu2_configure_devices(struct npu2 *p) >> { >> struct npu2_dev *dev; >> - struct dt_node *npu2_dn, *link; >> - uint32_t npu_phandle, index = 0; >> - >> - /* >> - * Get the npu node which has the links which we expand here >> - * into pci like devices attached to our emulated phb. >> - */ >> - npu_phandle = dt_prop_get_u32(dn, "ibm,npcq"); >> - npu2_dn = dt_find_by_phandle(dt_root, npu_phandle); >> - assert(npu2_dn); >> + uint32_t index = 0; > > Do not need to initialize index here. ack > > >> >> - /* Walk the link@x nodes to initialize devices */ >> - p->total_devices = 0; >> - p->phb_nvlink.scan_map = 0; >> - dt_for_each_compatible(npu2_dn, link, "ibm,npu-link") { >> + for (index = 0; index < p->total_devices; index++) { >> uint32_t group_id; >> >> dev = &p->devices[index]; >> - dev->type = NPU2_DEV_TYPE_NVLINK; >> - dev->npu = p; >> - dev->dt_node = link; >> - dev->link_index = dt_prop_get_u32(link, "ibm,npu-link-index"); >> - dev->brick_index = dev->link_index; >> + if (dev->type != NPU2_DEV_TYPE_NVLINK) >> + continue; > > Took me some time to realize where the type is set > (platforms/astbmc/witherspoon.c set_link_details()). Ok, safe. > > >> >> - group_id = dt_prop_get_u32(link, "ibm,npu-group-id"); >> - dev->bdfn = npu_allocate_bdfn(p, group_id); >> + group_id = dt_prop_get_u32(dev->dt_node, "ibm,npu-group-id"); >> + dev->bdfn = npu_allocate_bdfn(p, group_id, index); >> >> /* This must be done after calling >> * npu_allocate_bdfn() */ > > The comment is obsolete now. ack > > >> - p->total_devices++; >> p->phb_nvlink.scan_map |= 0x1 << ((dev->bdfn & 0xf8) >> 3); >> >> - dev->pl_xscom_base = dt_prop_get_u64(link, "ibm,npu-phy"); >> - dev->lane_mask = dt_prop_get_u32(link, "ibm,npu-lane-mask"); >> - >> /* Initialize PCI virtual device */ >> dev->nvlink.pvd = pci_virt_add_device(&p->phb_nvlink, dev->bdfn, 0x100, dev); >> if (dev->nvlink.pvd) >> npu2_populate_cfg(dev); >> - >> - index++; >> } >> } >> >> @@ -1857,7 +1835,7 @@ void npu2_nvlink_create_phb(struct npu2 *npu, struct dt_node *dn) >> list_head_init(&npu->phb_nvlink.virt_devices); >> >> npu2_setup_irqs(npu); >> - npu2_populate_devices(npu, dn); >> + npu2_configure_devices(npu); >> npu2_add_interrupt_map(npu, dn); >> npu2_add_phb_properties(npu); >> >> >
diff --git a/hw/npu2.c b/hw/npu2.c index 1c7af14958e8..106b32150994 100644 --- a/hw/npu2.c +++ b/hw/npu2.c @@ -1599,12 +1599,12 @@ static void npu2_populate_cfg(struct npu2_dev *dev) PCI_VIRT_CFG_INIT_RO(pvd, pos + 1, 1, 0); } -static uint32_t npu_allocate_bdfn(struct npu2 *p, uint32_t group) +static uint32_t npu_allocate_bdfn(struct npu2 *p, uint32_t group, int size) { int i; int bdfn = (group << 3); - for (i = 0; i < p->total_devices; i++) { + for (i = 0; i < size; i++) { if ((p->devices[i].bdfn & 0xf8) == (bdfn & 0xf8)) bdfn++; } @@ -1612,51 +1612,29 @@ static uint32_t npu_allocate_bdfn(struct npu2 *p, uint32_t group) return bdfn; } -static void npu2_populate_devices(struct npu2 *p, - struct dt_node *dn) +static void npu2_configure_devices(struct npu2 *p) { struct npu2_dev *dev; - struct dt_node *npu2_dn, *link; - uint32_t npu_phandle, index = 0; - - /* - * Get the npu node which has the links which we expand here - * into pci like devices attached to our emulated phb. - */ - npu_phandle = dt_prop_get_u32(dn, "ibm,npcq"); - npu2_dn = dt_find_by_phandle(dt_root, npu_phandle); - assert(npu2_dn); + uint32_t index = 0; - /* Walk the link@x nodes to initialize devices */ - p->total_devices = 0; - p->phb_nvlink.scan_map = 0; - dt_for_each_compatible(npu2_dn, link, "ibm,npu-link") { + for (index = 0; index < p->total_devices; index++) { uint32_t group_id; dev = &p->devices[index]; - dev->type = NPU2_DEV_TYPE_NVLINK; - dev->npu = p; - dev->dt_node = link; - dev->link_index = dt_prop_get_u32(link, "ibm,npu-link-index"); - dev->brick_index = dev->link_index; + if (dev->type != NPU2_DEV_TYPE_NVLINK) + continue; - group_id = dt_prop_get_u32(link, "ibm,npu-group-id"); - dev->bdfn = npu_allocate_bdfn(p, group_id); + group_id = dt_prop_get_u32(dev->dt_node, "ibm,npu-group-id"); + dev->bdfn = npu_allocate_bdfn(p, group_id, index); /* This must be done after calling * npu_allocate_bdfn() */ - p->total_devices++; p->phb_nvlink.scan_map |= 0x1 << ((dev->bdfn & 0xf8) >> 3); - dev->pl_xscom_base = dt_prop_get_u64(link, "ibm,npu-phy"); - dev->lane_mask = dt_prop_get_u32(link, "ibm,npu-lane-mask"); - /* Initialize PCI virtual device */ dev->nvlink.pvd = pci_virt_add_device(&p->phb_nvlink, dev->bdfn, 0x100, dev); if (dev->nvlink.pvd) npu2_populate_cfg(dev); - - index++; } } @@ -1857,7 +1835,7 @@ void npu2_nvlink_create_phb(struct npu2 *npu, struct dt_node *dn) list_head_init(&npu->phb_nvlink.virt_devices); npu2_setup_irqs(npu); - npu2_populate_devices(npu, dn); + npu2_configure_devices(npu); npu2_add_interrupt_map(npu, dn); npu2_add_phb_properties(npu);