Re: [PATCH 01/13] powerpc/powernv: Introduce new PHB type for opencapi links

[Andrew Donnellan]

On 19/12/17 02:21, Frederic Barrat wrote:
> The NPU was already abstracted by opal as a virtual PHB for nvlink,
> but it helps to be able to differentiate between a nvlink or opencapi
> PHB, as it's not completely transparent to linux. In particular, PE
> assignment differs and we'll also need the information in later
> patches.
>
> So rename existing PNV_PHB_NPU type to PNV_PHB_NPU_NVLINK and add a
> new type PNV_PHB_NPU_OCAPI.
>
> Signed-off-by: Frederic Barrat <fbarrat@xxxxxxxxxxxxxxxxxx>
> Signed-off-by: Andrew Donnellan <andrew.donnellan@xxxxxxxxxxx>
> ---
>   arch/powerpc/platforms/powernv/npu-dma.c  |  2 +-
>   arch/powerpc/platforms/powernv/pci-ioda.c | 46 +++++++++++++++++++++++--------
>   arch/powerpc/platforms/powernv/pci.c      |  4 +++
>   arch/powerpc/platforms/powernv/pci.h      |  8 ++++--
>   4 files changed, 45 insertions(+), 15 deletions(-)
>
> diff --git a/arch/powerpc/platforms/powernv/npu-dma.c b/arch/powerpc/platforms/powernv/npu-dma.c
> index f6cbc1a71472..c5899c107d59 100644
> --- a/arch/powerpc/platforms/powernv/npu-dma.c
> +++ b/arch/powerpc/platforms/powernv/npu-dma.c
> @@ -277,7 +277,7 @@ static int pnv_npu_dma_set_bypass(struct pnv_ioda_pe *npe)
>   	int64_t rc = 0;
>   	phys_addr_t top = memblock_end_of_DRAM();
>
> -	if (phb->type != PNV_PHB_NPU || !npe->pdev)
> +	if (phb->type != PNV_PHB_NPU_NVLINK || !npe->pdev)
>   		return -EINVAL;
>
>   	rc = pnv_npu_unset_window(npe, 0);
> diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
> index 749055553064..c37b5d288f9c 100644
> --- a/arch/powerpc/platforms/powernv/pci-ioda.c
> +++ b/arch/powerpc/platforms/powernv/pci-ioda.c
> @@ -54,7 +54,8 @@
>   #define POWERNV_IOMMU_DEFAULT_LEVELS	1
>   #define POWERNV_IOMMU_MAX_LEVELS	5
>
> -static const char * const pnv_phb_names[] = { "IODA1", "IODA2", "NPU" };
> +static const char * const pnv_phb_names[] = { "IODA1", "IODA2", "NPU_NVLINK",
> +					      "NPU_OCAPI" };
>   static void pnv_pci_ioda2_table_free_pages(struct iommu_table *tbl);
>
>   void pe_level_printk(const struct pnv_ioda_pe *pe, const char *level,
> @@ -924,7 +925,7 @@ static int pnv_ioda_configure_pe(struct pnv_phb *phb, struct pnv_ioda_pe *pe)
>   	 * Configure PELTV. NPUs don't have a PELTV table so skip
>   	 * configuration on them.
>   	 */
> -	if (phb->type != PNV_PHB_NPU)
> +	if (phb->type != PNV_PHB_NPU_NVLINK && phb->type != PNV_PHB_NPU_OCAPI)
>   		pnv_ioda_set_peltv(phb, pe, true);
>
>   	/* Setup reverse map */
> @@ -1260,12 +1261,13 @@ static struct pnv_ioda_pe *pnv_ioda_setup_npu_PE(struct pci_dev *npu_pdev)
>   		return pe;
>   }
>
> -static void pnv_ioda_setup_npu_PEs(struct pci_bus *bus)
> +static void pnv_ioda_setup_npu_PEs(struct pci_bus *bus,
> +			    struct pnv_ioda_pe *fn(struct pci_dev *npu_pdev))
>   {
>   	struct pci_dev *pdev;
>
>   	list_for_each_entry(pdev, &bus->devices, bus_list)
> -		pnv_ioda_setup_npu_PE(pdev);
> +		fn(pdev);
>   }

I think adding a function pointer here is rather ugly, at this point you 
might as well just do this directly in pnv_pci_ioda_setup_PEs()

>   static void pnv_pci_ioda_setup_PEs(void)
> @@ -1275,13 +1277,18 @@ static void pnv_pci_ioda_setup_PEs(void)
>
>   	list_for_each_entry_safe(hose, tmp, &hose_list, list_node) {
>   		phb = hose->private_data;
> -		if (phb->type == PNV_PHB_NPU) {
> +		if (phb->type == PNV_PHB_NPU_NVLINK) {
>   			/* PE#0 is needed for error reporting */
>   			pnv_ioda_reserve_pe(phb, 0);
> -			pnv_ioda_setup_npu_PEs(hose->bus);
> +			pnv_ioda_setup_npu_PEs(hose->bus,
> +					pnv_ioda_setup_npu_PE);
>   			if (phb->model == PNV_PHB_MODEL_NPU2)
>   				pnv_npu2_init(phb);
>   		}
> +		if (phb->type == PNV_PHB_NPU_OCAPI) {
> +			pnv_ioda_setup_npu_PEs(hose->bus,
> +					pnv_ioda_setup_dev_PE);
> +		}
>   	}
>   }

--
Andrew Donnellan              OzLabs, ADL Canberra
andrew.donnellan@xxxxxxxxxxx  IBM Australia Limited