[Skiboot] [PATCH v2 01/15] hw/npu2: Simplify BAR structures

Andrew Donnellan andrew.donnellan at au1.ibm.com
Fri Jan 11 12:09:21 AEDT 2019


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>
Reviewed-by: Frederic Barrat <fbarrat at linux.ibm.com>

---

v1->v2:
- rebase on top of Reza's cleanups
---
 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 8075d4e536d4..c10cf9863dda 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 d6b634d75f0c..79b87f196b3a 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;
 }
@@ -266,8 +260,8 @@ static int64_t npu2_cfg_write_bar(struct npu2_dev *dev,
 				  uint32_t offset, uint32_t size,
 				  uint32_t data)
 {
-	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;
 
 	if ((size != 4) ||
 	    (offset != pcrf->start && offset != pcrf->start + 4))
@@ -275,34 +269,34 @@ 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);
 
-		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 */
@@ -1408,13 +1402,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,
@@ -1670,7 +1664,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 */
@@ -1692,9 +1686,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,
@@ -1703,16 +1697,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,
@@ -1802,7 +1796,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 ?
@@ -1812,7 +1806,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 f51a6a2a8ffb..fcaeb55f67dc 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 {
@@ -121,7 +116,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;
-- 
git-series 0.9.1



More information about the Skiboot mailing list