[Skiboot] [PATCH] npu2: Use same compatible string for NVLink and OpenCAPI link nodes in device tree

Frederic Barrat fbarrat at linux.ibm.com
Thu Jun 28 23:08:03 AEST 2018



Le 27/06/2018 à 11:30, 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>
> ---

This looks good.
Just one minor comment below

>   doc/device-tree/opencapi.rst | 10 ++++++----
>   hdata/spira.c                |  1 +
>   hw/npu2-common.c             | 13 +++++++++++++
>   hw/npu2-opencapi.c           | 18 +++++++++++++-----
>   hw/npu2.c                    | 20 ++++++++++++++------
>   include/npu2.h               |  2 ++
>   platforms/astbmc/zaius.c     |  3 ++-
>   7 files changed, 51 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..325e56b5843c 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_NVLINK) {
> +			prlog(PR_DEBUG,
> +			      "OCAPI: NPU%d: NVLink link found, skipping\n",
> +			      index);
> +			return;
> +		}

Couldn't we exit if the type != opencapi? It would avoid having to do 
similar checks in the 2 loops below. It's highly unexpected and we 
probably want to stay away from such a NPU if it happens anyway.

   Fred



>   	}
> 
>   	path = dt_get_path(dn);
> @@ -1728,7 +1731,10 @@ 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") {
> +		if (npu2_dt_link_dev_type(link) != NPU2_DEV_TYPE_OPENCAPI)
> +			continue;
> +
>   		dev_index = dt_prop_get_u32(link, "ibm,npu-link-index");
>   		prlog(PR_INFO, "OCAPI: Configuring link index %lld\n",
>   		      dev_index);
> @@ -1748,7 +1754,9 @@ 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") {
> +		if (npu2_dt_link_dev_type(link) != NPU2_DEV_TYPE_OPENCAPI)
> +			continue;
>   		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