[Skiboot] [PATCH 4/4] hw/npu2-common: Allow mixed mode NPU setups

[Frederic Barrat]

Le 18/10/2018 à 03:16, Andrew Donnellan a écrit :
> Now that we have all the support in place for NPUs with both NVLink and
> OpenCAPI devices, get rid of the error that aborts NPU init when a mixed
> setup is detected.
> 
> While we're there, rename setup_devices() to more accurately reflect what
> it does, and move the calls to the NVLink/OpenCAPI setup code out of there
> into the main probe function.
> 
> Signed-off-by: Andrew Donnellan <andrew.donnellan at au1.ibm.com>
> ---
>   hw/npu2-common.c   | 23 ++++-------------------
>   hw/npu2-opencapi.c |  5 -----
>   2 files changed, 4 insertions(+), 24 deletions(-)
> 
> diff --git a/hw/npu2-common.c b/hw/npu2-common.c
> index 1b07d157b744..16533286ca79 100644
> --- a/hw/npu2-common.c
> +++ b/hw/npu2-common.c
> @@ -361,26 +361,19 @@ failed:
>   	return NULL;
>   }
> 
> -static void setup_devices(struct npu2 *npu)
> +static void add_link_type_properties(struct npu2 *npu)
>   {
> -	bool nvlink_detected = false, ocapi_detected = false;
>   	struct npu2_dev *dev;
> 
> -	/*
> -	 * TODO: In future, we'll do brick configuration here to support mixed
> -	 * setups.
> -	 */
>   	for (int i = 0; i < npu->total_devices; i++) {
>   		dev = &npu->devices[i];
>   		switch (dev->type) {
>   		case NPU2_DEV_TYPE_NVLINK:
> -			nvlink_detected = true;
>   			dt_add_property_strings(dev->dt_node,
>   						"ibm,npu-link-type",
>   						"nvlink");
>   			break;
>   		case NPU2_DEV_TYPE_OPENCAPI:
> -			ocapi_detected = true;
>   			dt_add_property_strings(dev->dt_node,
>   						"ibm,npu-link-type",
>   						"opencapi");
> @@ -393,16 +386,6 @@ static void setup_devices(struct npu2 *npu)
>   						"unknown");
>   		}
>   	}
> -
> -	if (nvlink_detected && ocapi_detected) {
> -		prlog(PR_ERR, "NPU: NVLink and OpenCAPI devices on same chip not supported, aborting NPU init\n");
> -		return;
> -	}
> -
> -	if (nvlink_detected)
> -		npu2_nvlink_init_npu(npu);
> -	else if (ocapi_detected)
> -		npu2_opencapi_init_npu(npu);
>   }
> 
>   void probe_npu2(void)
> @@ -438,7 +421,9 @@ void probe_npu2(void)
>   		if (!npu)
>   			continue;
>   		platform.npu2_device_detect(npu);
> +		add_link_type_properties(npu);
>   		set_brick_config(npu);
> -		setup_devices(npu);
> +		npu2_nvlink_init_npu(npu);
> +		npu2_opencapi_init_npu(npu);

We're always calling npu2_nvlink_init_npu() and 
npu2_opencapi_init_npu(). We may consider exiting early from those 
functions if there's no device of that type under the npu. Otherwise, 
we're creating extra PHBs for nothing (at least npu2_nvlink_init_npu() 
would).

I think we're also allocating/setting up the IRQs twice. I can imagine 
it's breaking things on the nvlink side of things (since we'll overwrite 
the NPU interrupt vector address with the opencapi one).

Don't we also have some overlap on the global mmio bar assignments 
(setup_global_mmio_bar() and assign_mmio_bars)? Though I think we end up 
overwriting the same content.

   Fred


>   	}
>   }
> diff --git a/hw/npu2-opencapi.c b/hw/npu2-opencapi.c
> index fdad7c5c1bbe..93df898cd068 100644
> --- a/hw/npu2-opencapi.c
> +++ b/hw/npu2-opencapi.c
> @@ -19,15 +19,10 @@
>    *
>    * This file provides support for OpenCAPI as implemented on POWER9.
>    *
> - * At present, we initialise the NPU separately from the NVLink code in npu2.c.
> - * As such, we don't currently support mixed NVLink and OpenCAPI configurations
> - * on the same NPU for machines such as Witherspoon.
> - *
>    * Procedure references in this file are to the POWER9 OpenCAPI NPU Workbook
>    * (IBM internal document).
>    *
>    * TODO:
> - *   - Support for mixed NVLink and OpenCAPI on the same NPU
>    *   - Support for link ganging (one AFU using multiple links)
>    *   - Link reset and error handling
>    *   - Presence detection

We can also remove presence detection.

   Fred