| Message ID | 57019d682bbb2e700baa7900f0d564c8409ec68c.1544597914.git-series.andrew.donnellan@au1.ibm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Support OpenCAPI and NVLink devices on same NPU on Witherspoon | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | master/apply_patch Successfully applied |
| snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot | success | Test snowpatch/job/snowpatch-skiboot on branch master |
On 12/12/2018 17:58, Andrew Donnellan wrote: > Rework npu2_add_interrupt_map() so it only includes NVLink devices. Use the > existing struct npu2_devs rather than accessing the device tree. > > Signed-off-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com> > > --- > > I don't really know how to test this. > --- > hw/npu2.c | 36 ++++++++++++++++++++---------------- > 1 file changed, 20 insertions(+), 16 deletions(-) > > diff --git a/hw/npu2.c b/hw/npu2.c > index 07213f9f75e1..9e2c7d5fdda4 100644 > --- a/hw/npu2.c > +++ b/hw/npu2.c > @@ -1686,11 +1686,11 @@ static void npu2_configure_devices(struct npu2 *p) > } > } > > -static void npu2_add_interrupt_map(struct npu2 *p, > - struct dt_node *dn) > +static void npu2_add_interrupt_map(struct npu2 *p) > { > - struct dt_node *npu2_dn, *link, *phb_dn; > - uint32_t npu2_phandle, index = 0, i; > + struct dt_node *phb_dn; > + struct npu2_dev *dev; > + int index, i = 0, nv_devices = 0; > uint32_t icsp = get_ics_phandle(); > uint32_t *map; > size_t map_size; > @@ -1699,23 +1699,27 @@ static void npu2_add_interrupt_map(struct npu2 *p, > assert(p->phb_nvlink.dt_node); > phb_dn = p->phb_nvlink.dt_node; > > - npu2_phandle = dt_prop_get_u32(dn, "ibm,npcq"); > - npu2_dn = dt_find_by_phandle(dt_root, npu2_phandle); > - assert(npu2_dn); > - map_size = 7 * sizeof(*map) * p->total_devices; > + for (index = 0; index < p->total_devices; index++) { > + if (p->devices[index].type == NPU2_DEV_TYPE_NVLINK) > + nv_devices++; > + } > + > + map_size = 7 * sizeof(*map) * nv_devices; > map = malloc(map_size); > - index = 0; > - dt_for_each_compatible(npu2_dn, link, "ibm,npu-link") { > - i = index * 7; > - map[i + 0] = (p->devices[index].bdfn << 8); > + > + for (index = 0; index < p->total_devices; index++) { > + dev = &p->devices[index]; > + if (dev->type != NPU2_DEV_TYPE_NVLINK) > + continue; > + > + map[i + 0] = (dev->bdfn << 8); > map[i + 1] = 0; > map[i + 2] = 0; > - > map[i + 3] = 1; /* INT A */ > map[i + 4] = icsp; /* interrupt-parent */ > - map[i + 5] = p->base_lsi + (index * 2) + 1; /* NDL No-Stall Event */ > + map[i + 5] = p->base_lsi + (dev->brick_index * 2) + 1; /* NDL No-Stall Event */ The comment in hw/npu2-common.c says "May be overridden by platform presence detection" about dev->brick_index initialization. And brick_index is initialized from link_index which is (as I figured out recently) not exactly the index into npu2::devices[]. So probably switching from npu2::devices[]'s index to dev->brick_index is the right move but please make it a separate patch for bisectability. > map[i + 6] = 0; /* 0 = EDGE, 1 = LEVEL. */ > - index++; > + i += 7; > } > dt_add_property(phb_dn, "interrupt-map", map, map_size); > free(map); > @@ -1885,7 +1889,7 @@ void npu2_nvlink_create_phb(struct npu2 *npu, struct dt_node *dn) > > npu2_setup_irqs(npu); > npu2_configure_devices(npu); > - npu2_add_interrupt_map(npu, dn); > + npu2_add_interrupt_map(npu); > npu2_add_phb_properties(npu); > > slot = npu2_slot_create(&npu->phb_nvlink); >
On 14/12/18 4:24 pm, Alexey Kardashevskiy wrote: >> map[i + 4] = icsp; /* interrupt-parent */ >> - map[i + 5] = p->base_lsi + (index * 2) + 1; /* NDL No-Stall Event */ >> + map[i + 5] = p->base_lsi + (dev->brick_index * 2) + 1; /* NDL No-Stall Event */ > > > The comment in hw/npu2-common.c says "May be overridden by platform > presence detection" about dev->brick_index initialization. And > brick_index is initialized from link_index which is (as I figured out > recently) not exactly the index into npu2::devices[]. So probably > switching from npu2::devices[]'s index to dev->brick_index is the right > move but please make it a separate patch for bisectability. If you really want it in a separate patch I can do that but it's a very minor change, I will at least document it in the commit
On 14/12/2018 16:30, Andrew Donnellan wrote: > On 14/12/18 4:24 pm, Alexey Kardashevskiy wrote: >>> map[i + 4] = icsp; /* interrupt-parent */ >>> - map[i + 5] = p->base_lsi + (index * 2) + 1; /* NDL No-Stall >>> Event */ >>> + map[i + 5] = p->base_lsi + (dev->brick_index * 2) + 1; /* >>> NDL No-Stall Event */ >> >> >> The comment in hw/npu2-common.c says "May be overridden by platform >> presence detection" about dev->brick_index initialization. And >> brick_index is initialized from link_index which is (as I figured out >> recently) not exactly the index into npu2::devices[]. So probably >> switching from npu2::devices[]'s index to dev->brick_index is the right >> move but please make it a separate patch for bisectability. > > If you really want it in a separate patch I can do that but it's a very > minor change, I will at least document it in the commit I'd like to know if this was tested (but you say it was not) or see it clear in the code and the commit log that what you are doing is correct. I can only see that brick_index==index most of the times but not always and nothing really puts any light on when/why it is !=, explaining this in the commit log of this patch would be useful. I'd even suggest making it a comment in the chunk above.
diff --git a/hw/npu2.c b/hw/npu2.c index 07213f9f75e1..9e2c7d5fdda4 100644 --- a/hw/npu2.c +++ b/hw/npu2.c @@ -1686,11 +1686,11 @@ static void npu2_configure_devices(struct npu2 *p) } } -static void npu2_add_interrupt_map(struct npu2 *p, - struct dt_node *dn) +static void npu2_add_interrupt_map(struct npu2 *p) { - struct dt_node *npu2_dn, *link, *phb_dn; - uint32_t npu2_phandle, index = 0, i; + struct dt_node *phb_dn; + struct npu2_dev *dev; + int index, i = 0, nv_devices = 0; uint32_t icsp = get_ics_phandle(); uint32_t *map; size_t map_size; @@ -1699,23 +1699,27 @@ static void npu2_add_interrupt_map(struct npu2 *p, assert(p->phb_nvlink.dt_node); phb_dn = p->phb_nvlink.dt_node; - npu2_phandle = dt_prop_get_u32(dn, "ibm,npcq"); - npu2_dn = dt_find_by_phandle(dt_root, npu2_phandle); - assert(npu2_dn); - map_size = 7 * sizeof(*map) * p->total_devices; + for (index = 0; index < p->total_devices; index++) { + if (p->devices[index].type == NPU2_DEV_TYPE_NVLINK) + nv_devices++; + } + + map_size = 7 * sizeof(*map) * nv_devices; map = malloc(map_size); - index = 0; - dt_for_each_compatible(npu2_dn, link, "ibm,npu-link") { - i = index * 7; - map[i + 0] = (p->devices[index].bdfn << 8); + + for (index = 0; index < p->total_devices; index++) { + dev = &p->devices[index]; + if (dev->type != NPU2_DEV_TYPE_NVLINK) + continue; + + map[i + 0] = (dev->bdfn << 8); map[i + 1] = 0; map[i + 2] = 0; - map[i + 3] = 1; /* INT A */ map[i + 4] = icsp; /* interrupt-parent */ - map[i + 5] = p->base_lsi + (index * 2) + 1; /* NDL No-Stall Event */ + map[i + 5] = p->base_lsi + (dev->brick_index * 2) + 1; /* NDL No-Stall Event */ map[i + 6] = 0; /* 0 = EDGE, 1 = LEVEL. */ - index++; + i += 7; } dt_add_property(phb_dn, "interrupt-map", map, map_size); free(map); @@ -1885,7 +1889,7 @@ void npu2_nvlink_create_phb(struct npu2 *npu, struct dt_node *dn) npu2_setup_irqs(npu); npu2_configure_devices(npu); - npu2_add_interrupt_map(npu, dn); + npu2_add_interrupt_map(npu); npu2_add_phb_properties(npu); slot = npu2_slot_create(&npu->phb_nvlink);
Rework npu2_add_interrupt_map() so it only includes NVLink devices. Use the existing struct npu2_devs rather than accessing the device tree. Signed-off-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com> --- I don't really know how to test this. --- hw/npu2.c | 36 ++++++++++++++++++++---------------- 1 file changed, 20 insertions(+), 16 deletions(-)