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 |
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 |
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 >
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 --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