[Skiboot] [PATCH 01/13] hw/npu2: Simplify BAR structures
Frederic Barrat
fbarrat at linux.ibm.com
Fri Dec 14 00:11:28 AEDT 2018
Le 12/12/2018 à 07:58, Andrew Donnellan a écrit :
> At the moment, we have struct npu2_bar to represent an NPU BAR, and struct
> npu2_pcie_bar which wraps an npu2_bar and does nothing other than adding an
> additional flags field, which is exposed through the PCI virtual device.
>
> In npu2_bar, we have another flags field which is used to store state
> about the BAR, but it's only used internally, which means we use a lot of
> bitwise manipulations with no benefit other than saving a couple of bytes.
>
> Get rid of npu2_pcie_bar, convert the flags in npu2_bar::flags into
> individual bools, and fix references accordingly.
>
> Signed-off-by: Andrew Donnellan <andrew.donnellan at au1.ibm.com>
> ---
This looks simpler and better and I didn't see any problem in the
conversions.
Reviewed-by: Frederic Barrat <fbarrat at linux.ibm.com>
> hw/npu2-opencapi.c | 12 ++---
> hw/npu2.c | 104 +++++++++++++++++++++-------------------------
> include/npu2.h | 27 ++++--------
> 3 files changed, 66 insertions(+), 77 deletions(-)
>
> diff --git a/hw/npu2-opencapi.c b/hw/npu2-opencapi.c
> index 65f623c731c2..b160a41741b9 100644
> --- a/hw/npu2-opencapi.c
> +++ b/hw/npu2-opencapi.c
> @@ -807,8 +807,8 @@ static void setup_afu_mmio_bars(uint32_t gcid, uint32_t scom_base,
> prlog(PR_DEBUG, "OCAPI: AFU MMIO set to %llx, size %llx\n", addr, size);
> write_bar(gcid, scom_base, NPU2_REG_OFFSET(stack, 0, offset), addr,
> size);
> - dev->bars[0].npu2_bar.base = addr;
> - dev->bars[0].npu2_bar.size = size;
> + dev->bars[0].base = addr;
> + dev->bars[0].size = size;
>
> reg = SETFIELD(NPU2_CQ_CTL_MISC_MMIOPA_ADDR, 0ull, addr >> 16);
> reg = SETFIELD(NPU2_CQ_CTL_MISC_MMIOPA_SIZE, reg, ilog2(size >> 16));
> @@ -832,8 +832,8 @@ static void setup_afu_config_bars(uint32_t gcid, uint32_t scom_base,
> prlog(PR_DEBUG, "OCAPI: Assigning GENID BAR: %016llx\n", addr);
> write_bar(gcid, scom_base, NPU2_REG_OFFSET(stack, 0, NPU2_GENID_BAR),
> addr, size);
> - dev->bars[1].npu2_bar.base = addr;
> - dev->bars[1].npu2_bar.size = size;
> + dev->bars[1].base = addr;
> + dev->bars[1].size = size;
> }
>
> static void otl_enabletx(uint32_t gcid, uint32_t scom_base,
> @@ -1293,7 +1293,7 @@ static int64_t npu2_opencapi_pcicfg_read(struct phb *phb, uint32_t bdfn,
> if (rc)
> return rc;
>
> - genid_base = dev->bars[1].npu2_bar.base +
> + genid_base = dev->bars[1].base +
> (index_to_block(dev->brick_index) == NPU2_BLOCK_OTL1 ? 256 : 0);
>
> cfg_addr = NPU2_CQ_CTL_CONFIG_ADDR_ENABLE;
> @@ -1351,7 +1351,7 @@ static int64_t npu2_opencapi_pcicfg_write(struct phb *phb, uint32_t bdfn,
> if (rc)
> return rc;
>
> - genid_base = dev->bars[1].npu2_bar.base +
> + genid_base = dev->bars[1].base +
> (index_to_block(dev->brick_index) == NPU2_BLOCK_OTL1 ? 256 : 0);
>
> cfg_addr = NPU2_CQ_CTL_CONFIG_ADDR_ENABLE;
> diff --git a/hw/npu2.c b/hw/npu2.c
> index 4e75eeb764ce..ba5575cf3097 100644
> --- a/hw/npu2.c
> +++ b/hw/npu2.c
> @@ -114,7 +114,6 @@ static inline void npu2_get_bar(uint32_t gcid, struct npu2_bar *bar)
> static void npu2_read_bar(struct npu2 *p, struct npu2_bar *bar)
> {
> uint64_t reg, val;
> - int enabled;
>
> reg = NPU2_REG_OFFSET(0, NPU2_BLOCK_SM_0, bar->reg);
> val = npu2_read(p, reg);
> @@ -122,7 +121,7 @@ static void npu2_read_bar(struct npu2 *p, struct npu2_bar *bar)
> switch (NPU2_REG(bar->reg)) {
> case NPU2_PHY_BAR:
> bar->base = GETFIELD(NPU2_PHY_BAR_ADDR, val) << 21;
> - enabled = GETFIELD(NPU2_PHY_BAR_ENABLE, val);
> + bar->enabled = GETFIELD(NPU2_PHY_BAR_ENABLE, val);
>
> if (NPU2_REG_STACK(reg) == NPU2_STACK_STCK_2)
> /* This is the global MMIO BAR */
> @@ -133,22 +132,20 @@ static void npu2_read_bar(struct npu2 *p, struct npu2_bar *bar)
> case NPU2_NTL0_BAR:
> case NPU2_NTL1_BAR:
> bar->base = GETFIELD(NPU2_NTL_BAR_ADDR, val) << 16;
> - enabled = GETFIELD(NPU2_NTL_BAR_ENABLE, val);
> + bar->enabled = GETFIELD(NPU2_NTL_BAR_ENABLE, val);
> bar->size = 0x10000 << GETFIELD(NPU2_NTL_BAR_SIZE, val);
> break;
> case NPU2_GENID_BAR:
> bar->base = GETFIELD(NPU2_GENID_BAR_ADDR, val) << 16;
> - enabled = GETFIELD(NPU2_GENID_BAR_ENABLE, val);
> + bar->enabled = GETFIELD(NPU2_GENID_BAR_ENABLE, val);
> bar->size = 0x20000;
> break;
> default:
> bar->base = 0ul;
> - enabled = 0;
> + bar->enabled = false;
> bar->size = 0;
> break;
> }
> -
> - bar->flags = SETFIELD(NPU2_BAR_FLAG_ENABLED, bar->flags, enabled);
> }
>
> static void npu2_write_bar(struct npu2 *p,
> @@ -156,23 +153,23 @@ static void npu2_write_bar(struct npu2 *p,
> uint32_t gcid,
> uint32_t scom)
> {
> - uint64_t reg, val, enable = !!(bar->flags & NPU2_BAR_FLAG_ENABLED);
> + uint64_t reg, val;
> int block;
>
> switch (NPU2_REG(bar->reg)) {
> case NPU2_PHY_BAR:
> val = SETFIELD(NPU2_PHY_BAR_ADDR, 0ul, bar->base >> 21);
> - val = SETFIELD(NPU2_PHY_BAR_ENABLE, val, enable);
> + val = SETFIELD(NPU2_PHY_BAR_ENABLE, val, bar->enabled);
> break;
> case NPU2_NTL0_BAR:
> case NPU2_NTL1_BAR:
> val = SETFIELD(NPU2_NTL_BAR_ADDR, 0ul, bar->base >> 16);
> - val = SETFIELD(NPU2_NTL_BAR_ENABLE, val, enable);
> + val = SETFIELD(NPU2_NTL_BAR_ENABLE, val, bar->enabled);
> val = SETFIELD(NPU2_NTL_BAR_SIZE, val, 1);
> break;
> case NPU2_GENID_BAR:
> val = SETFIELD(NPU2_GENID_BAR_ADDR, 0ul, bar->base >> 16);
> - val = SETFIELD(NPU2_GENID_BAR_ENABLE, val, enable);
> + val = SETFIELD(NPU2_GENID_BAR_ENABLE, val, bar->enabled);
> break;
> default:
> val = 0ul;
> @@ -211,10 +208,10 @@ static int64_t npu2_cfg_write_cmd(void *dev,
> * one GENID BAR, which is exposed via the first brick.
> */
> enabled = !!(*data & PCI_CFG_CMD_MEM_EN);
> - ntl_npu_bar = &ndev->bars[0].npu2_bar;
> - genid_npu_bar = &ndev->bars[1].npu2_bar;
> + ntl_npu_bar = &ndev->bars[0];
> + genid_npu_bar = &ndev->bars[1];
>
> - ntl_npu_bar->flags = SETFIELD(NPU2_BAR_FLAG_ENABLED, ntl_npu_bar->flags, enabled);
> + ntl_npu_bar->enabled = enabled;
> npu2_write_bar(ndev->npu, ntl_npu_bar, 0, 0);
>
> /*
> @@ -223,16 +220,12 @@ static int64_t npu2_cfg_write_cmd(void *dev,
> * track the enables separately.
> */
> if (NPU2DEV_BRICK(ndev))
> - genid_npu_bar->flags = SETFIELD(NPU2_BAR_FLAG_ENABLED1, genid_npu_bar->flags,
> - enabled);
> + genid_npu_bar->enabled1 = enabled;
> else
> - genid_npu_bar->flags = SETFIELD(NPU2_BAR_FLAG_ENABLED0, genid_npu_bar->flags,
> - enabled);
> + genid_npu_bar->enabled0 = enabled;
>
> /* Enable the BAR if either device requests it enabled, otherwise disable it */
> - genid_npu_bar->flags = SETFIELD(NPU2_BAR_FLAG_ENABLED, genid_npu_bar->flags,
> - !!(genid_npu_bar->flags & (NPU2_BAR_FLAG_ENABLED0 |
> - NPU2_BAR_FLAG_ENABLED1)));
> + genid_npu_bar->enabled = genid_npu_bar->enabled0 || genid_npu_bar->enabled1;
> npu2_write_bar(ndev->npu, genid_npu_bar, 0, 0);
>
> return OPAL_PARTIAL;
> @@ -243,20 +236,21 @@ static int64_t npu2_cfg_read_bar(struct npu2_dev *dev __unused,
> uint32_t offset, uint32_t size,
> uint32_t *data)
> {
> - struct npu2_pcie_bar *bar = (struct npu2_pcie_bar *) pcrf->data;
> + struct npu2_bar *bar = (struct npu2_bar *) pcrf->data;
>
> - if (!(bar->flags & NPU2_PCIE_BAR_FLAG_TRAPPED))
> + if (!(bar->trapped))
> return OPAL_PARTIAL;
>
> if ((size != 4) ||
> (offset != pcrf->start && offset != pcrf->start + 4))
> return OPAL_PARAMETER;
>
> - if (bar->flags & NPU2_PCIE_BAR_FLAG_SIZE_HI)
> - *data = bar->npu2_bar.size >> 32;
> + if (bar->size_hi)
> + *data = bar->size >> 32;
> else
> - *data = bar->npu2_bar.size;
> - bar->flags &= ~(NPU2_PCIE_BAR_FLAG_TRAPPED | NPU2_PCIE_BAR_FLAG_SIZE_HI);
> + *data = bar->size;
> + bar->trapped = false;
> + bar->size_hi = false;
>
> return OPAL_SUCCESS;
> }
> @@ -267,8 +261,8 @@ static int64_t npu2_cfg_write_bar(struct npu2_dev *dev,
> uint32_t data)
> {
> struct pci_virt_device *pvd = dev->nvlink.pvd;
> - struct npu2_pcie_bar *bar = (struct npu2_pcie_bar *) pcrf->data;
> - struct npu2_bar old_bar, *npu2_bar = &bar->npu2_bar;
> + struct npu2_bar *bar = (struct npu2_bar *) pcrf->data;
> + struct npu2_bar old_bar;
> uint32_t pci_cmd;
>
> if ((size != 4) ||
> @@ -277,36 +271,36 @@ static int64_t npu2_cfg_write_bar(struct npu2_dev *dev,
>
> /* Return BAR size on next read */
> if (data == 0xffffffff) {
> - bar->flags |= NPU2_PCIE_BAR_FLAG_TRAPPED;
> + bar->trapped = true;
> if (offset == pcrf->start + 4)
> - bar->flags |= NPU2_PCIE_BAR_FLAG_SIZE_HI;
> + bar->size_hi = true;
>
> return OPAL_SUCCESS;
> }
>
> if (offset == pcrf->start) {
> - npu2_bar->base &= 0xffffffff00000000;
> - npu2_bar->base |= (data & 0xfffffff0);
> + bar->base &= 0xffffffff00000000;
> + bar->base |= (data & 0xfffffff0);
> } else {
> - npu2_bar->base &= 0x00000000ffffffff;
> - npu2_bar->base |= ((uint64_t)data << 32);
> + bar->base &= 0x00000000ffffffff;
> + bar->base |= ((uint64_t)data << 32);
>
> PCI_VIRT_CFG_NORMAL_RD(pvd, PCI_CFG_CMD, 4, &pci_cmd);
>
> - if (NPU2_REG(npu2_bar->reg) == NPU2_GENID_BAR && NPU2DEV_BRICK(dev))
> - npu2_bar->base -= 0x10000;
> + if (NPU2_REG(bar->reg) == NPU2_GENID_BAR && NPU2DEV_BRICK(dev))
> + bar->base -= 0x10000;
>
> - old_bar.reg = npu2_bar->reg;
> + old_bar.reg = bar->reg;
> npu2_read_bar(dev->npu, &old_bar);
>
> /* Only allow changing the base address if the BAR is not enabled */
> - if ((npu2_bar->flags & NPU2_BAR_FLAG_ENABLED) &&
> - (npu2_bar->base != old_bar.base)) {
> - npu2_bar->base = old_bar.base;
> + if (bar->enabled &&
> + (bar->base != old_bar.base)) {
> + bar->base = old_bar.base;
> return OPAL_HARDWARE;
> }
>
> - npu2_write_bar(dev->npu, &bar->npu2_bar, 0, 0);
> + npu2_write_bar(dev->npu, bar, 0, 0);
> }
>
> /* To update the config cache */
> @@ -1452,13 +1446,13 @@ static void assign_mmio_bars(uint64_t gcid, uint32_t scom, uint64_t reg[2], uint
> /* 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),
> - .flags = NPU2_BAR_FLAG_ENABLED },
> + .enabled = true },
> { .type = NPU_PHY, .index = 0,
> .reg = NPU2_REG_OFFSET(NPU2_STACK_STCK_1, 0, NPU2_PHY_BAR),
> - .flags = NPU2_BAR_FLAG_ENABLED },
> + .enabled = true },
> { .type = NPU_PHY, .index = 1,
> .reg = NPU2_REG_OFFSET(NPU2_STACK_STCK_2, 0, NPU2_PHY_BAR),
> - .flags = NPU2_BAR_FLAG_ENABLED },
> + .enabled = true },
> { .type = NPU_NTL, .index = 0,
> .reg = NPU2_REG_OFFSET(NPU2_STACK_STCK_0, 0, NPU2_NTL0_BAR) },
> { .type = NPU_NTL, .index = 1,
> @@ -1715,7 +1709,7 @@ static uint32_t npu2_populate_vendor_cap(struct npu2_dev *dev,
> static void npu2_populate_cfg(struct npu2_dev *dev)
> {
> struct pci_virt_device *pvd = dev->nvlink.pvd;
> - struct npu2_pcie_bar *bar;
> + struct npu2_bar *bar;
> uint32_t pos;
>
> /* 0x00 - Vendor/Device ID */
> @@ -1737,9 +1731,9 @@ static void npu2_populate_cfg(struct npu2_dev *dev)
> /* 0x10/14 - BAR#0, NTL BAR */
> bar = &dev->bars[0];
> PCI_VIRT_CFG_INIT(pvd, PCI_CFG_BAR0, 4,
> - (bar->npu2_bar.base & 0xfffffff0) | (bar->flags & 0xF),
> + (bar->base & 0xfffffff0) | bar->flags,
> 0x0000000f, 0x00000000);
> - PCI_VIRT_CFG_INIT(pvd, PCI_CFG_BAR1, 4, (bar->npu2_bar.base >> 32),
> + PCI_VIRT_CFG_INIT(pvd, PCI_CFG_BAR1, 4, (bar->base >> 32),
> 0x00000000, 0x00000000);
> pci_virt_add_filter(pvd, PCI_CFG_BAR0, 8,
> PCI_REG_FLAG_READ | PCI_REG_FLAG_WRITE,
> @@ -1748,16 +1742,16 @@ static void npu2_populate_cfg(struct npu2_dev *dev)
> /* 0x18/1c - BAR#1, GENID BAR */
> bar = &dev->bars[1];
> if (NPU2DEV_BRICK(dev) == 0)
> - PCI_VIRT_CFG_INIT(pvd, PCI_CFG_BAR2, 4, (bar->npu2_bar.base & 0xfffffff0) |
> - (bar->flags & 0xF),
> + PCI_VIRT_CFG_INIT(pvd, PCI_CFG_BAR2, 4, (bar->base & 0xfffffff0) |
> + bar->flags,
> 0x0000000f, 0x00000000);
> else
> /* Brick 1 gets the upper portion of the generation id register */
> - PCI_VIRT_CFG_INIT(pvd, PCI_CFG_BAR2, 4, ((bar->npu2_bar.base + 0x10000) & 0xfffffff0) |
> - (bar->flags & 0xF),
> + PCI_VIRT_CFG_INIT(pvd, PCI_CFG_BAR2, 4, ((bar->base + 0x10000) & 0xfffffff0) |
> + bar->flags,
> 0x0000000f, 0x00000000);
>
> - PCI_VIRT_CFG_INIT(pvd, PCI_CFG_BAR3, 4, (bar->npu2_bar.base >> 32), 0x00000000,
> + PCI_VIRT_CFG_INIT(pvd, PCI_CFG_BAR3, 4, (bar->base >> 32), 0x00000000,
> 0x00000000);
> pci_virt_add_filter(pvd, PCI_CFG_BAR2, 8,
> PCI_REG_FLAG_READ | PCI_REG_FLAG_WRITE,
> @@ -1847,7 +1841,7 @@ static void npu2_populate_devices(struct npu2 *p,
>
> /* Populate BARs. BAR0/1 is the NTL bar. */
> stack = NPU2_STACK_STCK_0 + NPU2DEV_STACK(dev);
> - npu2_bar = &dev->bars[0].npu2_bar;
> + npu2_bar = &dev->bars[0];
> npu2_bar->type = NPU_NTL;
> npu2_bar->index = dev->brick_index;
> npu2_bar->reg = NPU2_REG_OFFSET(stack, 0, NPU2DEV_BRICK(dev) == 0 ?
> @@ -1857,7 +1851,7 @@ static void npu2_populate_devices(struct npu2 *p,
> dev->bars[0].flags = PCI_CFG_BAR_TYPE_MEM | PCI_CFG_BAR_MEM64;
>
> /* BAR2/3 is the GENID bar. */
> - npu2_bar = &dev->bars[1].npu2_bar;
> + npu2_bar = &dev->bars[1];
> npu2_bar->type = NPU_GENID;
> npu2_bar->index = NPU2DEV_STACK(dev);
> npu2_bar->reg = NPU2_REG_OFFSET(stack, 0, NPU2_GENID_BAR);
> diff --git a/include/npu2.h b/include/npu2.h
> index 1de963d19928..da5a5597feb0 100644
> --- a/include/npu2.h
> +++ b/include/npu2.h
> @@ -67,26 +67,21 @@
> struct npu2_bar {
> enum phys_map_type type;
> int index;
> -#define NPU2_BAR_FLAG_ENABLED 0x0010
> + uint32_t flags; /* NVLink: exported to PCI config space */
> + uint64_t base;
> + uint64_t size;
> + uint64_t reg;
> +
> + bool enabled;
>
> /* Generation ID's are a single space in the hardware but we split
> * them in two for the emulated PCIe devices so we need to keep track
> * of which one has been enabled/disabled. */
> -#define NPU2_BAR_FLAG_ENABLED0 0x0080
> -#define NPU2_BAR_FLAG_ENABLED1 0x0100
> - uint32_t flags;
> - uint64_t base;
> - uint64_t size;
> - uint64_t reg;
> -};
> + bool enabled0;
> + bool enabled1;
>
> -/* Rpresents a BAR that is exposed via the PCIe emulated
> - * devices */
> -struct npu2_pcie_bar {
> -#define NPU2_PCIE_BAR_FLAG_SIZE_HI 0x0020
> -#define NPU2_PCIE_BAR_FLAG_TRAPPED 0x0040
> - uint32_t flags;
> - struct npu2_bar npu2_bar;
> + bool size_hi;
> + bool trapped;
> };
>
> enum npu2_dev_type {
> @@ -124,7 +119,7 @@ struct npu2_dev {
> uint32_t brick_index;
> uint64_t pl_xscom_base;
> struct dt_node *dt_node;
> - struct npu2_pcie_bar bars[2];
> + struct npu2_bar bars[2];
> struct npu2 *npu;
>
> uint32_t bdfn;
>
More information about the Skiboot
mailing list