[Skiboot] [PATCH] npu2-opencapi: Allow platforms to identify physical slots

[Andrew Donnellan]

On 5/2/20 1:54 am, Frederic Barrat wrote:
> This patch lets each platform define the name of the opencapi
> slots. It makes it easier to identify which physical card is
> generating errors or messages in the linux or skiboot log files.
> 
> The patch provides slot names for mihawk and witherspoon. If the
> platform doesn't define any, then we default to 'OPENCAPI-xxxx'
> 
> There are various ways to find out about the slot names:
>    skiboot log
>    lspci command (if the PCI hotplug driver pnv-php is loaded)
>    lshw
>    checking the device tree
>    and probably others....
> 
> Signed-off-by: Frederic Barrat <fbarrat at linux.ibm.com>

Nitpick below

Otherwise

Reviewed-by: Andrew Donnellan <ajd at linux.ibm.com>

> ---
>   hw/npu2-opencapi.c             | 16 +++++++++++++---
>   include/platform.h             |  1 +
>   platforms/astbmc/mihawk.c      | 19 +++++++++++++++++++
>   platforms/astbmc/witherspoon.c | 20 ++++++++++++++++++++
>   4 files changed, 53 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/npu2-opencapi.c b/hw/npu2-opencapi.c
> index f855cdfc..5a3cd837 100644
> --- a/hw/npu2-opencapi.c
> +++ b/hw/npu2-opencapi.c
> @@ -1308,7 +1308,9 @@ static int64_t npu2_opencapi_hreset(struct pci_slot *slot __unused)
>   
>   static void make_slot_hotpluggable(struct pci_slot *slot, struct phb *phb)
>   {
> -	char label[40];
> +	struct npu2_dev *dev = phb_to_npu2_dev_ocapi(phb);
> +	char name[40];
> +	const char *label = NULL;
>   
>   	/*
>   	 * Add a few definitions to the DT so that the linux PCI
> @@ -1319,8 +1321,16 @@ static void make_slot_hotpluggable(struct pci_slot *slot, struct phb *phb)
>   	 */
>   	slot->pluggable = 1;
>   	pci_slot_add_dt_properties(slot, phb->dt_node);
> -	snprintf(label, sizeof(label), "OPENCAPI-%04x",
> -		 (int)PCI_SLOT_PHB_INDEX(slot->id));
> +
> +	if (platform.ocapi->ocapi_slot_label)
> +		label = platform.ocapi->ocapi_slot_label(dev->npu->chip_id,
> +							 dev->brick_index);
> +
> +	if (!label) {
> +		snprintf(name, sizeof(name), "OPENCAPI-%04x",
> +			 (int)PCI_SLOT_PHB_INDEX(slot->id));
> +		label = name;
> +	}
>   	dt_add_property_string(phb->dt_node, "ibm,slot-label", label);
>   }
>   
> diff --git a/include/platform.h b/include/platform.h
> index 6ecdbe47..6909a6a4 100644
> --- a/include/platform.h
> +++ b/include/platform.h
> @@ -63,6 +63,7 @@ struct platform_ocapi {
>   	uint8_t i2c_presence_brick5;	/* I2C pin to read for presence on brick 5 */
>   	bool odl_phy_swap;		/* Swap ODL1 to use brick 2 rather than
>   					 * brick 1 lanes */
> +	const char *(*ocapi_slot_label)(uint32_t chip_id, uint32_t brick_index);
>   };
>   
>   struct dt_node;
> diff --git a/platforms/astbmc/mihawk.c b/platforms/astbmc/mihawk.c
> index 6816accd..476d324e 100644
> --- a/platforms/astbmc/mihawk.c
> +++ b/platforms/astbmc/mihawk.c
> @@ -55,6 +55,24 @@ static void mihawk_get_slot_info(struct phb *phb, struct pci_device *pd)
>   		slot_table_get_slot_info(phb, pd);
>   }
>   
> +static const char *mihawk_ocapi_slot_label(uint32_t chip_id,
> +					   uint32_t brick_index)
> +{
> +	const char *name = NULL;
> +
> +	if (chip_id == 0)
> +		if (brick_index == 2)
> +			name = "JP90NVB1";
> +		else
> +			name = "JP90NVT1";
> +	else
> +		if (brick_index == 2)
> +			name = "JP91NVB1";
> +		else
> +			name = "JP91NVT1";

Can we have consistent uses of braces and consistent use of "else" vs 
"else if" for everything across both platforms?

> +	return name;
> +}
> +
>   static const struct platform_ocapi mihawk_ocapi = {
>           .i2c_engine          = 1,
>           .i2c_port            = 4,
> @@ -69,6 +87,7 @@ static const struct platform_ocapi mihawk_ocapi = {
>           .i2c_presence_brick4 = 0, /* unused */
>           .i2c_presence_brick5 = 0, /* unused */
>           .odl_phy_swap        = true,
> +	.ocapi_slot_label    = mihawk_ocapi_slot_label,
>   };
>   
>   static const struct slot_table_entry P1E1A_x8_PLX8748_down[] = {
> diff --git a/platforms/astbmc/witherspoon.c b/platforms/astbmc/witherspoon.c
> index 08199668..0d8d33cd 100644
> --- a/platforms/astbmc/witherspoon.c
> +++ b/platforms/astbmc/witherspoon.c
> @@ -325,6 +325,25 @@ i2c_failed:
>   	return;
>   }
>   
> +static const char *witherspoon_ocapi_slot_label(uint32_t chip_id,
> +						uint32_t brick_index)
> +{
> +	const char *name = NULL;
> +
> +	if (chip_id == 0) {
> +		if (brick_index == 3)
> +			name = "OPENCAPI-GPU0";
> +		else if (brick_index == 4)
> +			name = "OPENCAPI-GPU1";
> +	} else {
> +		if (brick_index == 3)
> +			name = "OPENCAPI-GPU3";
> +		else if (brick_index == 4)
> +			name = "OPENCAPI-GPU4";
> +	}
> +	return name;
> +}
> +
>   static const struct platform_ocapi witherspoon_ocapi = {
>          .i2c_engine          = 1,
>          .i2c_port            = 4,
> @@ -348,6 +367,7 @@ static const struct platform_ocapi witherspoon_ocapi = {
>          .i2c_presence_brick3 = 0,
>          .i2c_presence_brick4 = 0,
>          .i2c_presence_brick5 = 0,
> +       .ocapi_slot_label    = witherspoon_ocapi_slot_label,
>   };
>   
>   static int gpu_slot_to_num(const char *slot)
> 

-- 
Andrew Donnellan              OzLabs, ADL Canberra
ajd at linux.ibm.com             IBM Australia Limited