diff mbox series

[v2,14/15] hw/npu2-common: Allow mixed mode NPU setups

Message ID 2ffb70b58cfe4a2b14f891305db83d84f23ba435.1547168645.git-series.andrew.donnellan@au1.ibm.com
State Changes Requested
Headers show
Series Support OpenCAPI and NVLink devices on same NPU on Witherspoon | expand

Checks

Context Check Description
snowpatch_ozlabs/apply_patch warning master/apply_patch Patch failed to apply
snowpatch_ozlabs/apply_patch fail Failed to apply to any branch

Commit Message

Andrew Donnellan Jan. 11, 2019, 1:09 a.m. UTC 
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@au1.ibm.com>
Reviewed-by: Frederic Barrat <fbarrat@linux.ibm.com>
---
 hw/npu2-common.c   | 28 ++++++----------------------
 hw/npu2-opencapi.c |  5 -----
 2 files changed, 6 insertions(+), 27 deletions(-)

Comments

Alexey Kardashevskiy Jan. 22, 2019, 1:05 a.m. UTC | #1 
On 11/01/2019 12:09, Andrew Donnellan wrote:
> 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.

setup_devices() did set up devices, what inaccurate was about that?

This is my problem with this patchset - functional changes are
intermixed with refactoring, it is quite hard to follow what individual
change is what, it does not have to be that complex...


> 
> Signed-off-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com>
> Reviewed-by: Frederic Barrat <fbarrat@linux.ibm.com>
> ---
>  hw/npu2-common.c   | 28 ++++++----------------------
>  hw/npu2-opencapi.c |  5 -----
>  2 files changed, 6 insertions(+), 27 deletions(-)
> 
> diff --git a/hw/npu2-common.c b/hw/npu2-common.c
> index e323e71cb8d5..9e1b4bcc250e 100644
> --- a/hw/npu2-common.c
> +++ b/hw/npu2-common.c
> @@ -620,26 +620,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");
> @@ -652,19 +645,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;
> -	}
> -
> -	assign_bars(npu);
> -	setup_irqs(npu);
> -
> -	if (nvlink_detected)
> -		npu2_nvlink_init_npu(npu);
> -	else if (ocapi_detected)
> -		npu2_opencapi_init_npu(npu);
>  }
>  
>  void probe_npu2(void)
> @@ -700,7 +680,11 @@ void probe_npu2(void)
>  		if (!npu)
>  			continue;
>  		platform.npu2_device_detect(npu);
> +		add_link_type_properties(npu);
>  		set_brick_config(npu);
> -		setup_devices(npu);
> +		assign_bars(npu);
> +		setup_irqs(npu);
> +		npu2_nvlink_init_npu(npu);
> +		npu2_opencapi_init_npu(npu);
>  	}
>  }
> diff --git a/hw/npu2-opencapi.c b/hw/npu2-opencapi.c
> index 0c7d4f9ff148..7acf3837fd40 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
>
Andrew Donnellan Jan. 31, 2019, 12:40 a.m. UTC | #2 
On 22/1/19 12:05 pm, Alexey Kardashevskiy wrote:
> 
> 
> On 11/01/2019 12:09, Andrew Donnellan wrote:
>> 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.
> 
> setup_devices() did set up devices, what inaccurate was about that?
> 
> This is my problem with this patchset - functional changes are
> intermixed with refactoring, it is quite hard to follow what individual
> change is what, it does not have to be that complex...

I'll change the commit message to explain it more clearly

> 
> 
>>
>> Signed-off-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com>
>> Reviewed-by: Frederic Barrat <fbarrat@linux.ibm.com>
>> ---
>>   hw/npu2-common.c   | 28 ++++++----------------------
>>   hw/npu2-opencapi.c |  5 -----
>>   2 files changed, 6 insertions(+), 27 deletions(-)
>>
>> diff --git a/hw/npu2-common.c b/hw/npu2-common.c
>> index e323e71cb8d5..9e1b4bcc250e 100644
>> --- a/hw/npu2-common.c
>> +++ b/hw/npu2-common.c
>> @@ -620,26 +620,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");
>> @@ -652,19 +645,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;
>> -	}
>> -
>> -	assign_bars(npu);
>> -	setup_irqs(npu);
>> -
>> -	if (nvlink_detected)
>> -		npu2_nvlink_init_npu(npu);
>> -	else if (ocapi_detected)
>> -		npu2_opencapi_init_npu(npu);
>>   }
>>   
>>   void probe_npu2(void)
>> @@ -700,7 +680,11 @@ void probe_npu2(void)
>>   		if (!npu)
>>   			continue;
>>   		platform.npu2_device_detect(npu);
>> +		add_link_type_properties(npu);
>>   		set_brick_config(npu);
>> -		setup_devices(npu);
>> +		assign_bars(npu);
>> +		setup_irqs(npu);
>> +		npu2_nvlink_init_npu(npu);
>> +		npu2_opencapi_init_npu(npu);
>>   	}
>>   }
>> diff --git a/hw/npu2-opencapi.c b/hw/npu2-opencapi.c
>> index 0c7d4f9ff148..7acf3837fd40 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
>>
>
diff mbox series

Patch

diff --git a/hw/npu2-common.c b/hw/npu2-common.c
index e323e71cb8d5..9e1b4bcc250e 100644
--- a/hw/npu2-common.c
+++ b/hw/npu2-common.c
@@ -620,26 +620,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");
@@ -652,19 +645,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;
-	}
-
-	assign_bars(npu);
-	setup_irqs(npu);
-
-	if (nvlink_detected)
-		npu2_nvlink_init_npu(npu);
-	else if (ocapi_detected)
-		npu2_opencapi_init_npu(npu);
 }
 
 void probe_npu2(void)
@@ -700,7 +680,11 @@  void probe_npu2(void)
 		if (!npu)
 			continue;
 		platform.npu2_device_detect(npu);
+		add_link_type_properties(npu);
 		set_brick_config(npu);
-		setup_devices(npu);
+		assign_bars(npu);
+		setup_irqs(npu);
+		npu2_nvlink_init_npu(npu);
+		npu2_opencapi_init_npu(npu);
 	}
 }
diff --git a/hw/npu2-opencapi.c b/hw/npu2-opencapi.c
index 0c7d4f9ff148..7acf3837fd40 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