[Skiboot] [PATCH 1/2] npu2: Rework phb-index assignments for virtual PHBs

[Andrew Donnellan]

On 1/8/19 6:54 pm, Frederic Barrat wrote:
> Until now, opencapi PHBs were not using the 'ibm,phb-index' property,
> as it was thought unnecessary. For nvlink, a phb-index was associated
> to the npu when parsing hdat data, and the nvlink PHB was reusing the
> same value.
> 
> It turns out it helps to have the 'ibm,phb-index' property for
> opencapi PHBs after all. Otherwise it can lead to wrong results on
> platforms like mihawk when trying to match entries in the slot
> table. We end up with an opencapi device inheriting wrong properties
> in the device tree, because match_slot_phb_entry() default to
> phb-index 0 if it cannot find the property. Though it doesn't seem to
> cause any harm, it's wrong and a future patch is expected to start
> using the slot table for opencapi, so it needs fixing.
> 
> The twist is that with opencapi, we can have multiple virtual PHBs for
> a single NPU, at least on P9. Therefore there's no 1-to-1 mapping
> between the NPU and PHB index. It doesn't really make sense to
> associate a phb-index to a npu.
> 
> With this patch, opencapi PHBs created under a NPU use a fixed mapping
> for their phb-index, based on the brick index. The range of possible
> values is 7 to 12. Because there can only be one nvlink PHB per NPU,
> it is always using a phb-index of 7.
> 
> A side effect is that 2 virtual PHBs on 2 different chips can have the
> same phb-index, which is similar to what happens for 'real' PCI PHBs,
> but is different from what was happening on a nvlink-only witherspoon
> so far.
> 
> This doesn't really affect Axone, where we have a 1-to-1 mapping
> between NPU and PHB for nvlink (and it will remain true with
> opencapi), but to be consistent, we define the phb-index when creating
> the PHB like on P9.
> 
> Signed-off-by: Frederic Barrat <fbarrat at linux.ibm.com>

Maybe include a comment explaining this in the code?

Otherwise

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

> ---
> 
> Reza, Alistair: see the blurb above about the phb-index of the nvlink
> PHBs changing from 7 and 8 on witherspoon, to 7 and 7. My
> understanding is that 1) it's not really used on witherspoon and 2)
> when it is used, the phb-index is associated to the chip ID, as it's
> needed for real PHBs anyway, but I thought I should highlight it.
> 
> 
>   hw/npu2-common.c   | 1 -
>   hw/npu2-opencapi.c | 2 ++
>   hw/npu2.c          | 2 +-
>   hw/npu3-nvlink.c   | 2 +-
>   include/npu2.h     | 2 +-
>   include/npu3.h     | 2 ++
>   6 files changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/npu2-common.c b/hw/npu2-common.c
> index 6d5c35af..7326e165 100644
> --- a/hw/npu2-common.c
> +++ b/hw/npu2-common.c
> @@ -530,7 +530,6 @@ static struct npu2 *setup_npu(struct dt_node *dn)
>   	npu->index = dt_prop_get_u32(dn, "ibm,npu-index");
>   	npu->chip_id = gcid;
>   	npu->xscom_base = dt_get_address(dn, 0, NULL);
> -	npu->phb_index = dt_prop_get_u32(dn, "ibm,phb-index");
>   
>   	init_lock(&npu->i2c_lock);
>   	npu->i2c_pin_mode = ~0; // input mode by default
> diff --git a/hw/npu2-opencapi.c b/hw/npu2-opencapi.c
> index 9a391bb0..106ce0ac 100644
> --- a/hw/npu2-opencapi.c
> +++ b/hw/npu2-opencapi.c
> @@ -1665,6 +1665,8 @@ static void setup_device(struct npu2_dev *dev)
>   	dt_add_property_strings(dn_phb, "device_type", "pciex");
>   	dt_add_property(dn_phb, "reg", mm_win, sizeof(mm_win));
>   	dt_add_property_cells(dn_phb, "ibm,npu-index", dev->npu->index);
> +	dt_add_property_cells(dn_phb, "ibm,phb-index",
> +			      NPU2_PHB_INDEX_BASE + dev->brick_index);
>   	dt_add_property_cells(dn_phb, "ibm,chip-id", dev->npu->chip_id);
>   	dt_add_property_cells(dn_phb, "ibm,xscom-base", dev->npu->xscom_base);
>   	dt_add_property_cells(dn_phb, "ibm,npcq", dev->npu->dt_node->phandle);
> diff --git a/hw/npu2.c b/hw/npu2.c
> index 29c998f6..19ca70a4 100644
> --- a/hw/npu2.c
> +++ b/hw/npu2.c
> @@ -1478,7 +1478,7 @@ int npu2_nvlink_init_npu(struct npu2 *npu)
>   				"ibm,ioda2-npu2-phb");
>   	dt_add_property_strings(np, "device_type", "pciex");
>   	dt_add_property(np, "reg", reg, sizeof(reg));
> -	dt_add_property_cells(np, "ibm,phb-index", npu->phb_index);
> +	dt_add_property_cells(np, "ibm,phb-index", NPU2_PHB_INDEX_BASE);
>   	dt_add_property_cells(np, "ibm,npu-index", npu->index);
>   	dt_add_property_cells(np, "ibm,chip-id", npu->chip_id);
>   	dt_add_property_cells(np, "ibm,xscom-base", npu->xscom_base);
> diff --git a/hw/npu3-nvlink.c b/hw/npu3-nvlink.c
> index 7e7a10e8..a4e633e6 100644
> --- a/hw/npu3-nvlink.c
> +++ b/hw/npu3-nvlink.c
> @@ -1461,7 +1461,7 @@ static void npu3_dt_add_props(struct npu3 *npu)
>   				"ibm,ioda2-npu2-phb");
>   
>   	dt_add_property_cells(dn, "ibm,phb-index",
> -			      dt_prop_get_u32(npu->dt_node, "ibm,phb-index"));
> +			      NPU3_PHB_INDEX_BASE + npu->index);
>   	dt_add_property_cells(dn, "ibm,phb-diag-data-size", 0);
>   	dt_add_property_cells(dn, "ibm,opal-num-pes", NPU3_MAX_PE_NUM);
>   	dt_add_property_cells(dn, "ibm,opal-reserved-pe", NPU3_RESERVED_PE_NUM);
> diff --git a/include/npu2.h b/include/npu2.h
> index 92b58988..47317c8c 100644
> --- a/include/npu2.h
> +++ b/include/npu2.h
> @@ -53,6 +53,7 @@
>   #define NPU2_OCAPI_PE(ndev) ((ndev)->brick_index + NPU2_MAX_PE_NUM)
>   
>   #define NPU2_LINKS_PER_CHIP 6
> +#define NPU2_PHB_INDEX_BASE 7 // enough to avoid conflicts with PCI
>   
>   /* Link flags */
>   #define NPU2_DEV_PCI_LINKED	0x1
> @@ -175,7 +176,6 @@ struct npu2 {
>   
>   	/* NVLink */
>   	struct phb	phb_nvlink;
> -	uint32_t	phb_index;
>   
>   	/* OCAPI */
>   	uint64_t	i2c_port_id_ocapi;
> diff --git a/include/npu3.h b/include/npu3.h
> index 1c657f94..417756c4 100644
> --- a/include/npu3.h
> +++ b/include/npu3.h
> @@ -78,6 +78,8 @@ struct npu3_dev {
>   	struct npu3_dev_nvlink	nvlink;
>   };
>   
> +#define NPU3_PHB_INDEX_BASE 7 // enough to avoid conflicts with PCI
> +
>   struct npu3_nvlink {
>   	struct phb		phb;
>   	uint32_t		ctx_ref[NPU3_XTS_BDF_MAP_MAX];
> 

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