[Skiboot] [PATCH 03/13] hw/npu2: Move PHY/NTL/GENID BAR assignment to common code

[Frederic Barrat]

Le 12/12/2018 à 07:58, Andrew Donnellan a écrit :
> Assignment of PHY/NTL/GENID BARs is currently duplicated between NVLink
> and OpenCAPI. This is going to cause us particular issues later on when we
> implement support for mixed-mode setups with NVLink and OpenCAPI on the
> same NPU.
> 
> Centralise the assignment of PHY/NTL/GENID BARs in common code.
> 
> Signed-off-by: Andrew Donnellan <andrew.donnellan at au1.ibm.com>
> ---

That one gave me a headache, mostly because I was not that familiar with 
the bar setup on the nvlink side of things (and some bars are written 
twice!). Hopefully we'll get another pair of eyes with a nvlink focus on 
it. A few questions below.



>   hw/npu2-common.c   |  76 ++++++++++++++++++++++++++++++-
>   hw/npu2-opencapi.c |  80 ++++----------------------------
>   hw/npu2.c          | 116 +++++-----------------------------------------
>   include/npu2.h     |   4 +-
>   4 files changed, 104 insertions(+), 172 deletions(-)
> 
> diff --git a/hw/npu2-common.c b/hw/npu2-common.c
> index 3446acb45bea..b140e9ffd064 100644
> --- a/hw/npu2-common.c
> +++ b/hw/npu2-common.c
> @@ -17,6 +17,7 @@
>   #include <skiboot.h>
>   #include <xscom.h>
>   #include <pci.h>
> +#include <pci-cfg.h>
>   #include <npu2.h>
>   #include <npu2-regs.h>
>   #include <bitutils.h>
> @@ -174,6 +175,79 @@ void npu2_write_bar(struct npu2 *p, struct npu2_bar *bar, uint32_t gcid,
>   	}
>   }
> 
> +static void assign_bars(struct npu2 *npu)
> +{
> +	uint32_t i;
> +	struct npu2_bar *bar;
> +	struct npu2_dev *dev;
> +	struct npu2_bar phy_bars[] = {
> +		/* NPU_REGS must be first in this list */
> +		{ .type = NPU_REGS, .index = 0,
> +		  .reg = NPU2_REG_OFFSET(NPU2_STACK_STCK_0, 0, NPU2_PHY_BAR),
> +		  .enabled = true },
> +		{ .type = NPU_PHY, .index = 0,
> +		  .reg = NPU2_REG_OFFSET(NPU2_STACK_STCK_1, 0, NPU2_PHY_BAR),
> +		  .enabled = true },
> +		{ .type = NPU_PHY, .index = 1,
> +		  .reg = NPU2_REG_OFFSET(NPU2_STACK_STCK_2, 0, NPU2_PHY_BAR),
> +		  .enabled = true },
> +	};
> +	struct npu2_bar last_bar =
> +		{ .type = NPU_GENID, .index = 2,
> +		  .reg = NPU2_REG_OFFSET(NPU2_STACK_STCK_2, 0, NPU2_GENID_BAR) };
> +
> +	/* Set common PHY BARs */
> +	for (i = 0; i < ARRAY_SIZE(phy_bars); i++) {
> +		bar = &phy_bars[i];
> +		npu2_get_bar(npu->chip_id, bar);
> +		npu2_write_bar(npu, bar, npu->chip_id, npu->xscom_base);
> +	}
> +
> +	/* Device BARs */
> +	for (i = 0; i < npu->total_devices; i++) {
> +		dev = &npu->devices[i];
> +		if (dev->type == NPU2_DEV_TYPE_UNKNOWN)
> +			continue;
> +
> +		/* NTL / OCAPI_MMIO BAR */
> +		bar = &dev->ntl_bar;
> +		if (dev->type == NPU2_DEV_TYPE_NVLINK)
> +			bar->type = NPU_NTL;
> +		else if (dev->type == NPU2_DEV_TYPE_OPENCAPI)
> +			bar->type = NPU_OCAPI_MMIO;
> +		bar->index = dev->brick_index;
> +		bar->reg = NPU2_REG_OFFSET(NPU2_STACK_STCK_0 + NPU2DEV_STACK(dev),
> +					   0, NPU2DEV_BRICK(dev) == 0 ?
> +					   NPU2_NTL0_BAR : NPU2_NTL1_BAR);
> +		bar->flags = PCI_CFG_BAR_TYPE_MEM | PCI_CFG_BAR_MEM64;
> +		npu2_get_bar(npu->chip_id, bar);
> +		npu2_write_bar(npu, bar, npu->chip_id, npu->xscom_base);
> +
> +		/* GENID BAR */
> +		bar = &dev->genid_bar;
> +		bar->type = NPU_GENID;
> +		bar->index = NPU2DEV_STACK(dev);
> +		bar->reg = NPU2_REG_OFFSET(NPU2_STACK_STCK_0 + NPU2DEV_STACK(dev),
> +					   0, NPU2_GENID_BAR);
> +		bar->flags = PCI_CFG_BAR_TYPE_MEM | PCI_CFG_BAR_MEM64;
> +		npu2_get_bar(npu->chip_id, bar);
> +		bar->size = 0x10000;
> +		if (NPU2DEV_BRICK(dev))
> +			bar->base += 0x10000;
> +		npu2_write_bar(npu, bar, npu->chip_id, npu->xscom_base);

The GENID reg is per stack (same address for the 2 devices). For 2 
devices on the same stack, isn't the 2nd one overwriting the 1st, with 
only half the size available? I don't understand why we're doing it. And 
it seems different from what we were having for opencapi. I'm guessing 
it may not make much of a difference, since we're only addressing 4k of 
config space.

It matches what nvlink was doing though, but the same comment applies.



> diff --git a/hw/npu2-opencapi.c b/hw/npu2-opencapi.c
> index 88e4eb1974a2..b374a1035ac9 100644
> --- a/hw/npu2-opencapi.c
> +++ b/hw/npu2-opencapi.c

> @@ -1582,8 +1530,8 @@ static void setup_device(struct npu2_dev *dev)
>   	uint64_t mm_win[2];
> 
>   	/* Populate PHB device node */
> -	phys_map_get(dev->npu->chip_id, NPU_OCAPI_MMIO, dev->brick_index, &mm_win[0],
> -		     &mm_win[1]);
> +	mm_win[0] = dev->ntl_bar.base;
> +	mm_win[1] = dev->ntl_bar.size;
>   	prlog(PR_DEBUG, "OCAPI: Setting MMIO window to %016llx + %016llx\n",
>   	      mm_win[0], mm_win[1]);
>   	dn_phb = dt_new_addr(dt_root, "pciex", mm_win[0]);
> @@ -1629,7 +1577,8 @@ static void setup_device(struct npu2_dev *dev)
>   	/* Procedure 13.1.3.8 - AFU MMIO Range BARs */
>   	setup_afu_mmio_bars(dev->npu->chip_id, dev->npu->xscom_base, dev);
>   	/* Procedure 13.1.3.9 - AFU Config BARs */
> -	setup_afu_config_bars(dev->npu->chip_id, dev->npu->xscom_base, dev);
> +	dev->genid_bar.enabled = true;
> +	npu2_write_bar(dev->npu, &dev->genid_bar, dev->npu->chip_id, dev->npu->xscom_base);

For readability, I wouldn't mind keeping a separate 
setup_afu_config_bars() function with the (now minimal) genid setup.