| Message ID | 20180629025709.11210-1-andrew.donnellan@au1.ibm.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v2] npu2: Use same compatible string for NVLink and OpenCAPI link nodes in device tree | expand |
Le 29/06/2018 à 04:57, Andrew Donnellan a écrit : > Currently, we distinguish between NPU links for NVLink devices and OpenCAPI > devices through the use of two different compatible strings - ibm,npu-link > and ibm,npu-link-opencapi. > > As we move towards supporting configurations with both NVLink and OpenCAPI > devices behind a single NPU, we need to detect the device type as part of > presence detection, which can't happen until well after the point where the > HDAT or platform code has created the NPU device tree nodes. Changing a > node's compatible string after it's been created is a bit ugly, so instead > we should move the device type to a new property which we can add to the > node later on. > > Get rid of the ibm,npu-link-opencapi compatible string, add a new > ibm,npu-link-type property, and a helper function to check the link type. > Add an "unknown" device type in preparation for later patches to detect > device type dynamically. > > These device tree bindings are entirely internal to skiboot and are not > consumed directly by Linux, so this shouldn't break anything (other than > internal BML lab environments). > > Signed-off-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com> > > --- Looks ok to me. Reviewed-by: Frederic Barrat <fbarrat@linux.vnet.ibm.com> > > v1->v2: > > - Clean up the non-OpenCAPI link checks in npu2_opencapi_probe() (thanks > Fred) > --- > doc/device-tree/opencapi.rst | 10 ++++++---- > hdata/spira.c | 1 + > hw/npu2-common.c | 13 +++++++++++++ > hw/npu2-opencapi.c | 13 ++++++++----- > hw/npu2.c | 20 ++++++++++++++------ > include/npu2.h | 2 ++ > platforms/astbmc/zaius.c | 3 ++- > 7 files changed, 46 insertions(+), 16 deletions(-) > > diff --git a/doc/device-tree/opencapi.rst b/doc/device-tree/opencapi.rst > index b24b29cf8cc7..80ff996311f9 100644 > --- a/doc/device-tree/opencapi.rst > +++ b/doc/device-tree/opencapi.rst > @@ -10,8 +10,8 @@ NPU bindings > The NPU nodes are similar to those in :doc:`nvlink`. > > We distinguish between OpenCAPI and NVLink links using the > -`ibm.npu-link-opencapi` compatible string. NPUs with a mixture of > -OpenCAPI and NVLink links are currently unsupported. > +`ibm.npu-link-type` property. NPUs with a mixture of OpenCAPI and > +NVLink links are currently unsupported. > > .. code-block:: dts > > @@ -25,7 +25,8 @@ OpenCAPI and NVLink links are currently unsupported. > ibm,npu-links = <0x2>; /* Number of links wired up to this npu. */ > > link@2 { > - compatible = "ibm,npu-link-opencapi"; > + compatible = "ibm,npu-link"; > + ibm,npu-link-type = "opencapi"; > ibm,npu-group-id = <0x1>; > ibm,npu-lane-mask = <0xf1e000>; /* Mask specifying which IBM PHY lanes > * are used for this link. 24-bit, > @@ -38,7 +39,8 @@ OpenCAPI and NVLink links are currently unsupported. > }; > > link@3 { > - compatible = "ibm,npu-link-opencapi"; > + compatible = "ibm,npu-link"; > + ibm,npu-link-type = "opencapi"; > ibm,npu-group-id = <0x2>; > ibm,npu-lane-mask = <0x78f>; > ibm,npu-phy = <0x80000000 0x9010c3f>; > diff --git a/hdata/spira.c b/hdata/spira.c > index d459df7c8cec..e3045c5f1543 100644 > --- a/hdata/spira.c > +++ b/hdata/spira.c > @@ -1488,6 +1488,7 @@ static void add_npu(struct dt_node *xscom, const struct HDIF_array_hdr *links, > } > > dt_add_property_string(node, "compatible", "ibm,npu-link"); > + dt_add_property_string(node, "ibm,npu-link-type", "nvlink"); > dt_add_property_cells(node, "reg", link_count); > dt_add_property_cells(node, "ibm,npu-link-index", link_count); > dt_add_property_cells(node, "ibm,workbook-link-id", link_id); > diff --git a/hw/npu2-common.c b/hw/npu2-common.c > index 71440f619e6a..d076b4906fcc 100644 > --- a/hw/npu2-common.c > +++ b/hw/npu2-common.c > @@ -21,6 +21,19 @@ > #include <npu2-regs.h> > #include <bitutils.h> > > +enum npu2_dev_type npu2_dt_link_dev_type(struct dt_node *link) > +{ > + const char *link_type = dt_prop_get(link, "ibm,npu-link-type") ?: > + "unknown"; > + if (streq(link_type, "nvlink")) { > + return NPU2_DEV_TYPE_NVLINK; > + } else if (streq(link_type, "opencapi")) { > + return NPU2_DEV_TYPE_OPENCAPI; > + } else { > + return NPU2_DEV_TYPE_UNKNOWN; > + } > +} > + > /* > * We use the indirect method because it uses the same addresses as > * the MMIO offsets (NPU RING) > diff --git a/hw/npu2-opencapi.c b/hw/npu2-opencapi.c > index d085cd1564f8..f82e6562c59e 100644 > --- a/hw/npu2-opencapi.c > +++ b/hw/npu2-opencapi.c > @@ -1701,9 +1701,12 @@ static void npu2_opencapi_probe(struct dt_node *dn) > > /* Don't try to init when we have an NVLink link */ > dt_for_each_compatible(dn, link, "ibm,npu-link") { > - prlog(PR_DEBUG, "OCAPI: NPU%d: NVLink link found, skipping\n", > - index); > - return; > + if (npu2_dt_link_dev_type(link) != NPU2_DEV_TYPE_OPENCAPI) { > + prlog(PR_DEBUG, > + "OCAPI: NPU%d: Non-OpenCAPI link found, skipping OpenCAPI init\n", > + index); > + return; > + } > } > > path = dt_get_path(dn); > @@ -1728,7 +1731,7 @@ static void npu2_opencapi_probe(struct dt_node *dn) > n->regs = (void *)reg[0]; > n->dt_node = dn; > > - dt_for_each_compatible(dn, link, "ibm,npu-link-opencapi") { > + dt_for_each_compatible(dn, link, "ibm,npu-link") { > dev_index = dt_prop_get_u32(link, "ibm,npu-link-index"); > prlog(PR_INFO, "OCAPI: Configuring link index %lld\n", > dev_index); > @@ -1748,7 +1751,7 @@ static void npu2_opencapi_probe(struct dt_node *dn) > if (rc) > goto failed; > > - dt_for_each_compatible(dn, link, "ibm,npu-link-opencapi") { > + dt_for_each_compatible(dn, link, "ibm,npu-link") { > npu2_opencapi_setup_device(link, n, &n->devices[i]); > i++; > } > diff --git a/hw/npu2.c b/hw/npu2.c > index c351404ac285..8e2f6fe94552 100644 > --- a/hw/npu2.c > +++ b/hw/npu2.c > @@ -1345,17 +1345,25 @@ static void assign_mmio_bars(uint64_t gcid, uint32_t scom, uint64_t reg[2], uint > static void npu2_probe_phb(struct dt_node *dn) > { > struct proc_chip *proc_chip; > - struct dt_node *np; > + struct dt_node *np, *link; > + bool ocapi_detected = false, nvlink_detected = false; > uint32_t gcid, scom, index, phb_index, links; > uint64_t reg[2], mm_win[2], val; > char *path; > > /* Abort if any OpenCAPI links detected */ > - if (dt_find_compatible_node(dn, NULL, "ibm,npu-link-opencapi")) { > - /* Die if there's also an NVLink link */ > - assert(!dt_find_compatible_node(dn, NULL, "ibm,npu-link")); > - prlog(PR_INFO, "NPU: OpenCAPI link configuration detected, " > - "not initialising NVLink\n"); > + dt_for_each_compatible(dn, link, "ibm,npu-link") { > + if (npu2_dt_link_dev_type(link) == NPU2_DEV_TYPE_OPENCAPI) > + ocapi_detected = true; > + else > + nvlink_detected = true; > + } > + > + if (ocapi_detected && nvlink_detected) { > + prlog(PR_ERR, "NPU: NVLink and OpenCAPI devices on same chip not supported\n"); > + assert(false); > + } else if (ocapi_detected) { > + prlog(PR_INFO, "NPU: OpenCAPI link configuration detected, not initialising NVLink\n"); > return; > } > > diff --git a/include/npu2.h b/include/npu2.h > index eb4af5e6c319..4c2e20e0e2f7 100644 > --- a/include/npu2.h > +++ b/include/npu2.h > @@ -81,6 +81,7 @@ struct npu2_pcie_bar { > }; > > enum npu2_dev_type { > + NPU2_DEV_TYPE_UNKNOWN, > NPU2_DEV_TYPE_NVLINK, > NPU2_DEV_TYPE_OPENCAPI, > }; > @@ -191,6 +192,7 @@ static inline struct phb *npu2_dev_to_phb(struct npu2_dev *ndev) > } > } > > +enum npu2_dev_type npu2_dt_link_dev_type(struct dt_node *link); > void npu2_write_4b(struct npu2 *p, uint64_t reg, uint32_t val); > uint32_t npu2_read_4b(struct npu2 *p, uint64_t reg); > void npu2_write(struct npu2 *p, uint64_t reg, uint64_t val); > diff --git a/platforms/astbmc/zaius.c b/platforms/astbmc/zaius.c > index 36e66faf0913..2f296c3679ac 100644 > --- a/platforms/astbmc/zaius.c > +++ b/platforms/astbmc/zaius.c > @@ -50,7 +50,8 @@ static void create_link(struct dt_node *npu, int group, int index) > snprintf(namebuf, sizeof(namebuf), "link@%x", index); > link = dt_new(npu, namebuf); > > - dt_add_property_string(link, "compatible", "ibm,npu-link-opencapi"); > + dt_add_property_string(link, "compatible", "ibm,npu-link"); > + dt_add_property_string(link, "ibm,npu-link-type", "opencapi"); > dt_add_property_cells(link, "ibm,npu-link-index", index); > > switch (index) { >
Andrew Donnellan <andrew.donnellan@au1.ibm.com> writes: > Currently, we distinguish between NPU links for NVLink devices and OpenCAPI > devices through the use of two different compatible strings - ibm,npu-link > and ibm,npu-link-opencapi. > > As we move towards supporting configurations with both NVLink and OpenCAPI > devices behind a single NPU, we need to detect the device type as part of > presence detection, which can't happen until well after the point where the > HDAT or platform code has created the NPU device tree nodes. Changing a > node's compatible string after it's been created is a bit ugly, so instead > we should move the device type to a new property which we can add to the > node later on. > > Get rid of the ibm,npu-link-opencapi compatible string, add a new > ibm,npu-link-type property, and a helper function to check the link type. > Add an "unknown" device type in preparation for later patches to detect > device type dynamically. > > These device tree bindings are entirely internal to skiboot and are not > consumed directly by Linux, so this shouldn't break anything (other than > internal BML lab environments). > > Signed-off-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com> Thanks, merged to master as of a36b40799055d0ff070f87376a486caed5c3a475
diff --git a/doc/device-tree/opencapi.rst b/doc/device-tree/opencapi.rst index b24b29cf8cc7..80ff996311f9 100644 --- a/doc/device-tree/opencapi.rst +++ b/doc/device-tree/opencapi.rst @@ -10,8 +10,8 @@ NPU bindings The NPU nodes are similar to those in :doc:`nvlink`. We distinguish between OpenCAPI and NVLink links using the -`ibm.npu-link-opencapi` compatible string. NPUs with a mixture of -OpenCAPI and NVLink links are currently unsupported. +`ibm.npu-link-type` property. NPUs with a mixture of OpenCAPI and +NVLink links are currently unsupported. .. code-block:: dts @@ -25,7 +25,8 @@ OpenCAPI and NVLink links are currently unsupported. ibm,npu-links = <0x2>; /* Number of links wired up to this npu. */ link@2 { - compatible = "ibm,npu-link-opencapi"; + compatible = "ibm,npu-link"; + ibm,npu-link-type = "opencapi"; ibm,npu-group-id = <0x1>; ibm,npu-lane-mask = <0xf1e000>; /* Mask specifying which IBM PHY lanes * are used for this link. 24-bit, @@ -38,7 +39,8 @@ OpenCAPI and NVLink links are currently unsupported. }; link@3 { - compatible = "ibm,npu-link-opencapi"; + compatible = "ibm,npu-link"; + ibm,npu-link-type = "opencapi"; ibm,npu-group-id = <0x2>; ibm,npu-lane-mask = <0x78f>; ibm,npu-phy = <0x80000000 0x9010c3f>; diff --git a/hdata/spira.c b/hdata/spira.c index d459df7c8cec..e3045c5f1543 100644 --- a/hdata/spira.c +++ b/hdata/spira.c @@ -1488,6 +1488,7 @@ static void add_npu(struct dt_node *xscom, const struct HDIF_array_hdr *links, } dt_add_property_string(node, "compatible", "ibm,npu-link"); + dt_add_property_string(node, "ibm,npu-link-type", "nvlink"); dt_add_property_cells(node, "reg", link_count); dt_add_property_cells(node, "ibm,npu-link-index", link_count); dt_add_property_cells(node, "ibm,workbook-link-id", link_id); diff --git a/hw/npu2-common.c b/hw/npu2-common.c index 71440f619e6a..d076b4906fcc 100644 --- a/hw/npu2-common.c +++ b/hw/npu2-common.c @@ -21,6 +21,19 @@ #include <npu2-regs.h> #include <bitutils.h> +enum npu2_dev_type npu2_dt_link_dev_type(struct dt_node *link) +{ + const char *link_type = dt_prop_get(link, "ibm,npu-link-type") ?: + "unknown"; + if (streq(link_type, "nvlink")) { + return NPU2_DEV_TYPE_NVLINK; + } else if (streq(link_type, "opencapi")) { + return NPU2_DEV_TYPE_OPENCAPI; + } else { + return NPU2_DEV_TYPE_UNKNOWN; + } +} + /* * We use the indirect method because it uses the same addresses as * the MMIO offsets (NPU RING) diff --git a/hw/npu2-opencapi.c b/hw/npu2-opencapi.c index d085cd1564f8..f82e6562c59e 100644 --- a/hw/npu2-opencapi.c +++ b/hw/npu2-opencapi.c @@ -1701,9 +1701,12 @@ static void npu2_opencapi_probe(struct dt_node *dn) /* Don't try to init when we have an NVLink link */ dt_for_each_compatible(dn, link, "ibm,npu-link") { - prlog(PR_DEBUG, "OCAPI: NPU%d: NVLink link found, skipping\n", - index); - return; + if (npu2_dt_link_dev_type(link) != NPU2_DEV_TYPE_OPENCAPI) { + prlog(PR_DEBUG, + "OCAPI: NPU%d: Non-OpenCAPI link found, skipping OpenCAPI init\n", + index); + return; + } } path = dt_get_path(dn); @@ -1728,7 +1731,7 @@ static void npu2_opencapi_probe(struct dt_node *dn) n->regs = (void *)reg[0]; n->dt_node = dn; - dt_for_each_compatible(dn, link, "ibm,npu-link-opencapi") { + dt_for_each_compatible(dn, link, "ibm,npu-link") { dev_index = dt_prop_get_u32(link, "ibm,npu-link-index"); prlog(PR_INFO, "OCAPI: Configuring link index %lld\n", dev_index); @@ -1748,7 +1751,7 @@ static void npu2_opencapi_probe(struct dt_node *dn) if (rc) goto failed; - dt_for_each_compatible(dn, link, "ibm,npu-link-opencapi") { + dt_for_each_compatible(dn, link, "ibm,npu-link") { npu2_opencapi_setup_device(link, n, &n->devices[i]); i++; } diff --git a/hw/npu2.c b/hw/npu2.c index c351404ac285..8e2f6fe94552 100644 --- a/hw/npu2.c +++ b/hw/npu2.c @@ -1345,17 +1345,25 @@ static void assign_mmio_bars(uint64_t gcid, uint32_t scom, uint64_t reg[2], uint static void npu2_probe_phb(struct dt_node *dn) { struct proc_chip *proc_chip; - struct dt_node *np; + struct dt_node *np, *link; + bool ocapi_detected = false, nvlink_detected = false; uint32_t gcid, scom, index, phb_index, links; uint64_t reg[2], mm_win[2], val; char *path; /* Abort if any OpenCAPI links detected */ - if (dt_find_compatible_node(dn, NULL, "ibm,npu-link-opencapi")) { - /* Die if there's also an NVLink link */ - assert(!dt_find_compatible_node(dn, NULL, "ibm,npu-link")); - prlog(PR_INFO, "NPU: OpenCAPI link configuration detected, " - "not initialising NVLink\n"); + dt_for_each_compatible(dn, link, "ibm,npu-link") { + if (npu2_dt_link_dev_type(link) == NPU2_DEV_TYPE_OPENCAPI) + ocapi_detected = true; + else + nvlink_detected = true; + } + + if (ocapi_detected && nvlink_detected) { + prlog(PR_ERR, "NPU: NVLink and OpenCAPI devices on same chip not supported\n"); + assert(false); + } else if (ocapi_detected) { + prlog(PR_INFO, "NPU: OpenCAPI link configuration detected, not initialising NVLink\n"); return; } diff --git a/include/npu2.h b/include/npu2.h index eb4af5e6c319..4c2e20e0e2f7 100644 --- a/include/npu2.h +++ b/include/npu2.h @@ -81,6 +81,7 @@ struct npu2_pcie_bar { }; enum npu2_dev_type { + NPU2_DEV_TYPE_UNKNOWN, NPU2_DEV_TYPE_NVLINK, NPU2_DEV_TYPE_OPENCAPI, }; @@ -191,6 +192,7 @@ static inline struct phb *npu2_dev_to_phb(struct npu2_dev *ndev) } } +enum npu2_dev_type npu2_dt_link_dev_type(struct dt_node *link); void npu2_write_4b(struct npu2 *p, uint64_t reg, uint32_t val); uint32_t npu2_read_4b(struct npu2 *p, uint64_t reg); void npu2_write(struct npu2 *p, uint64_t reg, uint64_t val); diff --git a/platforms/astbmc/zaius.c b/platforms/astbmc/zaius.c index 36e66faf0913..2f296c3679ac 100644 --- a/platforms/astbmc/zaius.c +++ b/platforms/astbmc/zaius.c @@ -50,7 +50,8 @@ static void create_link(struct dt_node *npu, int group, int index) snprintf(namebuf, sizeof(namebuf), "link@%x", index); link = dt_new(npu, namebuf); - dt_add_property_string(link, "compatible", "ibm,npu-link-opencapi"); + dt_add_property_string(link, "compatible", "ibm,npu-link"); + dt_add_property_string(link, "ibm,npu-link-type", "opencapi"); dt_add_property_cells(link, "ibm,npu-link-index", index); switch (index) {
Currently, we distinguish between NPU links for NVLink devices and OpenCAPI devices through the use of two different compatible strings - ibm,npu-link and ibm,npu-link-opencapi. As we move towards supporting configurations with both NVLink and OpenCAPI devices behind a single NPU, we need to detect the device type as part of presence detection, which can't happen until well after the point where the HDAT or platform code has created the NPU device tree nodes. Changing a node's compatible string after it's been created is a bit ugly, so instead we should move the device type to a new property which we can add to the node later on. Get rid of the ibm,npu-link-opencapi compatible string, add a new ibm,npu-link-type property, and a helper function to check the link type. Add an "unknown" device type in preparation for later patches to detect device type dynamically. These device tree bindings are entirely internal to skiboot and are not consumed directly by Linux, so this shouldn't break anything (other than internal BML lab environments). Signed-off-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com> --- v1->v2: - Clean up the non-OpenCAPI link checks in npu2_opencapi_probe() (thanks Fred) --- doc/device-tree/opencapi.rst | 10 ++++++---- hdata/spira.c | 1 + hw/npu2-common.c | 13 +++++++++++++ hw/npu2-opencapi.c | 13 ++++++++----- hw/npu2.c | 20 ++++++++++++++------ include/npu2.h | 2 ++ platforms/astbmc/zaius.c | 3 ++- 7 files changed, 46 insertions(+), 16 deletions(-)