| Message ID | 2f690f27c339a44d166826f820eb4d3d25a6063f.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: > Rework npu2_add_interrupt_map() so it only includes NVLink devices. Use the > existing struct npu2_devs rather than accessing the device tree. > > Use each device's brick index to determine its interrupt number, as > interrupt levels depend on the ODL number and thus the brick index rather > than link index. > > Signed-off-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com> > > --- > > This is difficult to test as I don't know how to inject faults in NVLink. > > v1->v2: > - add more info to commit message and comment about interrupt level > number (Alexey) > --- > hw/npu2.c | 41 +++++++++++++++++++++++++---------------- > 1 file changed, 25 insertions(+), 16 deletions(-) > > diff --git a/hw/npu2.c b/hw/npu2.c > index 106b32150994..78233579d3b7 100644 > --- a/hw/npu2.c > +++ b/hw/npu2.c > @@ -1638,11 +1638,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; > @@ -1651,23 +1651,32 @@ 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++; > + } if (!nv_devices) return; ? > + > + 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++) { index = 0, i = 0 ? I mean keep both initializations together. > + 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 */ > + > + /* > + * NDL No-Stall Event - the interrupt level is determined by Oh. The "level" from the "interrupt level" is not really a level but a number but the spec uses the word "level" anyway. Noiiiiice :( The workbook also says: Note 1: NDL 3 is connected to NTL 5. This NDL/NTL pair represents brick 5. Note 2: NDL 5 is connected to NTL 3. This NDL/NTL pair represents brick 3. It would help to mention in the commit log why we do not need swap bricks 3 and 5 here. > + * the brick number, per the NPU workbook > + */ > + map[i + 5] = p->base_lsi + (dev->brick_index * 2) + 1; > map[i + 6] = 0; /* 0 = EDGE, 1 = LEVEL. */ > - index++; > + i += 7; > } > dt_add_property(phb_dn, "interrupt-map", map, map_size); > free(map); > @@ -1836,7 +1845,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 17/1/19 5:30 pm, Alexey Kardashevskiy wrote: >> -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; >> @@ -1651,23 +1651,32 @@ 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++; >> + } > > > if (!nv_devices) > return; > > ? I don't think it causes any harm to have a zero-sized interrupt map but perhaps we should return there anyway... > >> + >> + 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++) { > > index = 0, i = 0 ? I mean keep both initializations together. ok > > >> + 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 */ >> + >> + /* >> + * NDL No-Stall Event - the interrupt level is determined by > > > Oh. The "level" from the "interrupt level" is not really a level but a > number but the spec uses the word "level" anyway. Noiiiiice :( Just following the spec :) > > The workbook also says: > Note 1: NDL 3 is connected to NTL 5. This NDL/NTL pair represents brick 5. > Note 2: NDL 5 is connected to NTL 3. This NDL/NTL pair represents brick 3. > > It would help to mention in the commit log why we do not need swap > bricks 3 and 5 here. Hence the comment "the interrupt level is determined by the brick number", because the brick numbering, not the NDL numbering, is what's used to determine the order of the interrupt levels. > > >> + * the brick number, per the NPU workbook >> + */ >> + map[i + 5] = p->base_lsi + (dev->brick_index * 2) + 1; >> map[i + 6] = 0; /* 0 = EDGE, 1 = LEVEL. */ >> - index++; >> + i += 7; >> } >> dt_add_property(phb_dn, "interrupt-map", map, map_size); >> free(map); >> @@ -1836,7 +1845,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); >> >
diff --git a/hw/npu2.c b/hw/npu2.c index 106b32150994..78233579d3b7 100644 --- a/hw/npu2.c +++ b/hw/npu2.c @@ -1638,11 +1638,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; @@ -1651,23 +1651,32 @@ 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 */ + + /* + * NDL No-Stall Event - the interrupt level is determined by + * the brick number, per the NPU workbook + */ + map[i + 5] = p->base_lsi + (dev->brick_index * 2) + 1; map[i + 6] = 0; /* 0 = EDGE, 1 = LEVEL. */ - index++; + i += 7; } dt_add_property(phb_dn, "interrupt-map", map, map_size); free(map); @@ -1836,7 +1845,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. Use each device's brick index to determine its interrupt number, as interrupt levels depend on the ODL number and thus the brick index rather than link index. Signed-off-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com> --- This is difficult to test as I don't know how to inject faults in NVLink. v1->v2: - add more info to commit message and comment about interrupt level number (Alexey) --- hw/npu2.c | 41 +++++++++++++++++++++++++---------------- 1 file changed, 25 insertions(+), 16 deletions(-)