[Skiboot] [PATCH v2] npu2: Use same compatible string for NVLink and OpenCAPI link nodes in device tree
Frederic Barrat
fbarrat at linux.ibm.com
Mon Jul 2 23:27:42 AEST 2018
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 at au1.ibm.com>
>
> ---
Looks ok to me.
Reviewed-by: Frederic Barrat <fbarrat at 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 at 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 at 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) {
>
More information about the Skiboot
mailing list