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

[Andrew Donnellan]

On 9/8/19 3:05 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 on P9. There's one PHB per (opencapi) brick. Therefore
> there's no 1-to-1 mapping between the NPU and PHB index and it no
> longer makes 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.
> 
> Signed-off-by: Frederic Barrat <fbarrat at linux.ibm.com>
> ---
Comment is good.

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

> Changelog:
> v2:
>   - add comment summarizing the commit msg in the code (Andrew)
>   - move the axone changes to separate patch
> 
> 
>   hw/npu2-common.c   |  1 -
>   hw/npu2-opencapi.c |  2 ++
>   hw/npu2.c          |  2 +-
>   include/npu2.h     | 20 +++++++++++++++++++-
>   4 files changed, 22 insertions(+), 3 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..c8c95631 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_get_phb_index(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..324d492f 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_get_phb_index(0));
>   	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/include/npu2.h b/include/npu2.h
> index 92b58988..5a8dc32b 100644
> --- a/include/npu2.h
> +++ b/include/npu2.h
> @@ -175,7 +175,6 @@ struct npu2 {
>   
>   	/* NVLink */
>   	struct phb	phb_nvlink;
> -	uint32_t	phb_index;
>   
>   	/* OCAPI */
>   	uint64_t	i2c_port_id_ocapi;
> @@ -251,4 +250,23 @@ int64_t npu2_map_lpar(struct phb *phb, uint64_t bdf, uint64_t lparid,
>   int64_t npu2_set_relaxed_order(struct phb *phb, uint32_t gcid, int pec,
>   			       bool enable);
>   
> +#define NPU2_PHB_INDEX_BASE 7
> +/* to avoid conflicts with PCI and for historical reasons */
> +
> +static inline int npu2_get_phb_index(unsigned int brick_index)
> +{
> +	/*
> +	 * There's one virtual PHB per brick with opencapi, so we no
> +	 * longer have a 1-to-1 mapping between a NPU and a virtual
> +	 * PHB. And we want a static phb-index, as it is needed to use
> +	 * a slot table on some platforms. So we associate a per-chip
> +	 * phb-index based on the brick index.
> +	 *
> +	 * nvlink only creates one virtual PHB per chip, so it is
> +	 * treated as if using brick 0, which is never used by
> +	 * opencapi.
> +	 */
> +	return NPU2_PHB_INDEX_BASE + brick_index;
> +}
> +
>   #endif /* __NPU2_H */
> 

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