[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