| Message ID | 20180809095124.23589-1-fbarrat@linux.ibm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | opal/hmi: Catch NPU2 HMIs for opencapi | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | master/apply_patch Successfully applied |
| snowpatch_ozlabs/make_check | success | Test make_check on branch master |
On 09/08/18 19:51, Frederic Barrat wrote: > HMIs for NPU2 are filtered with the 'compatible' string of the PHB, so > add opencapi to the mix. > > Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com> > --- > core/hmi.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/core/hmi.c b/core/hmi.c > index e6fed405..8f9bf289 100644 > --- a/core/hmi.c > +++ b/core/hmi.c > @@ -639,6 +639,12 @@ static void dump_scoms(int flat_chip_id, const char *unit, uint32_t *scoms, > } > } > > +static bool phb_is_npu2(struct dt_node *dn) > +{ > + return (dt_node_is_compatible(dn, "ibm,power9-npu-pciex") || > + dt_node_is_compatible(dn, "ibm,power9-npu-opencapi-pciex")); > +} > + > static void find_npu2_checkstop_reason(int flat_chip_id, > struct OpalHMIEvent *hmi_evt, > uint64_t *out_flags) > @@ -662,7 +668,7 @@ static void find_npu2_checkstop_reason(int flat_chip_id, > /* Find the NPU on the chip associated with the HMI. */ > for_each_phb(phb) { > /* NOTE: if a chip ever has >1 NPU this will need adjusting */ > - if (dt_node_is_compatible(phb->dt_node, "ibm,power9-npu-pciex") && > + if (phb_is_npu2(phb->dt_node) && > (dt_get_chip_id(phb->dt_node) == flat_chip_id)) { > p = phb_to_npu(phb); This is broken - in fact, afaict it's currently broken for NVLink, because this is using the NPU1 accessor function. You want something like this for NVLink (which might need to go to stable?): if (dt_node_is_compatible(phb->dt_node, "ibm,power9-npu-pciex") && (dt_get_chip_id(phb->dt_node) == flat_chip_id)) { p = phb_to_npu2_nvlink(phb); break; } And this for OpenCAPI: if (dt_node_is_compatible(phb->dt_node, "ibm,power9-npu-pciex-opencapi") && (dt_get_chip_id(phb->dt_node) == flat_chip_id)) { p = phb_to_npu2_dev_ocapi(phb)->npu; break; }
On 10/08/18 14:02, Andrew Donnellan wrote: >> p = phb_to_npu(phb); > > This is broken - in fact, afaict it's currently broken for NVLink, > because this is using the NPU1 accessor function. > > You want something like this for NVLink (which might need to go to > stable?): > > if (dt_node_is_compatible(phb->dt_node, "ibm,power9-npu-pciex") && > (dt_get_chip_id(phb->dt_node) == flat_chip_id)) { > p = phb_to_npu2_nvlink(phb); > break; > } > > And this for OpenCAPI: > > if (dt_node_is_compatible(phb->dt_node, "ibm,power9-npu-pciex-opencapi") && > (dt_get_chip_id(phb->dt_node) == flat_chip_id)) { > p = phb_to_npu2_dev_ocapi(phb)->npu; > break; > } Looking at this more closely - it's inconsequential as we don't reference p again after this point. It should really be just a boolean "found" or something.
Le 10/08/2018 à 06:41, Andrew Donnellan a écrit : > On 10/08/18 14:02, Andrew Donnellan wrote: >>> p = phb_to_npu(phb); >> >> This is broken - in fact, afaict it's currently broken for NVLink, >> because this is using the NPU1 accessor function. >> >> You want something like this for NVLink (which might need to go to >> stable?): >> >> if (dt_node_is_compatible(phb->dt_node, "ibm,power9-npu-pciex") && >> (dt_get_chip_id(phb->dt_node) == flat_chip_id)) { >> p = phb_to_npu2_nvlink(phb); >> break; >> } >> >> And this for OpenCAPI: >> >> if (dt_node_is_compatible(phb->dt_node, >> "ibm,power9-npu-pciex-opencapi") && >> (dt_get_chip_id(phb->dt_node) == flat_chip_id)) { >> p = phb_to_npu2_dev_ocapi(phb)->npu; >> break; >> } > > Looking at this more closely - it's inconsequential as we don't > reference p again after this point. > > It should really be just a boolean "found" or something. Yeah, it doesn't really matter, but I'll clean it up. Thanks! Fred
Le 10/08/2018 à 06:41, Andrew Donnellan a écrit : > On 10/08/18 14:02, Andrew Donnellan wrote: >>> p = phb_to_npu(phb); >> >> This is broken - in fact, afaict it's currently broken for NVLink, >> because this is using the NPU1 accessor function. >> >> You want something like this for NVLink (which might need to go to >> stable?): >> >> if (dt_node_is_compatible(phb->dt_node, "ibm,power9-npu-pciex") && >> (dt_get_chip_id(phb->dt_node) == flat_chip_id)) { >> p = phb_to_npu2_nvlink(phb); >> break; >> } >> >> And this for OpenCAPI: >> >> if (dt_node_is_compatible(phb->dt_node, >> "ibm,power9-npu-pciex-opencapi") && >> (dt_get_chip_id(phb->dt_node) == flat_chip_id)) { >> p = phb_to_npu2_dev_ocapi(phb)->npu; >> break; >> } > > Looking at this more closely - it's inconsequential as we don't > reference p again after this point. > > It should really be just a boolean "found" or something. Yeah, it doesn't really matter, but I'll clean it up. Thanks! Fred
diff --git a/core/hmi.c b/core/hmi.c index e6fed405..8f9bf289 100644 --- a/core/hmi.c +++ b/core/hmi.c @@ -639,6 +639,12 @@ static void dump_scoms(int flat_chip_id, const char *unit, uint32_t *scoms, } } +static bool phb_is_npu2(struct dt_node *dn) +{ + return (dt_node_is_compatible(dn, "ibm,power9-npu-pciex") || + dt_node_is_compatible(dn, "ibm,power9-npu-opencapi-pciex")); +} + static void find_npu2_checkstop_reason(int flat_chip_id, struct OpalHMIEvent *hmi_evt, uint64_t *out_flags) @@ -662,7 +668,7 @@ static void find_npu2_checkstop_reason(int flat_chip_id, /* Find the NPU on the chip associated with the HMI. */ for_each_phb(phb) { /* NOTE: if a chip ever has >1 NPU this will need adjusting */ - if (dt_node_is_compatible(phb->dt_node, "ibm,power9-npu-pciex") && + if (phb_is_npu2(phb->dt_node) && (dt_get_chip_id(phb->dt_node) == flat_chip_id)) { p = phb_to_npu(phb); break;
HMIs for NPU2 are filtered with the 'compatible' string of the PHB, so add opencapi to the mix. Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com> --- core/hmi.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)