[PATCH v2 0/5] Map register blocks individually

[PATCH v2 0/5] Map register blocks individually

[ira.weiny]

From: Ira Weiny <ira.weiny@intel.com>

Changes for v2:
	Incorporate feedback from Dan
	Ensure memory blocks are individually reserved as well as mapped
	Remove pci device management in favor of lower level device management
	Drop version checking
	Reorder patches
	Update commit messages

Some hardware implementations mix component and device registers into the same
BAR and the driver stack is going to have independent mapping implementations
for those 2 cases.  Furthermore, it will be nice to have finer grained mappings
should user space want to map some register blocks.

Unfortunately, the information for the register blocks is contained inside the
BARs themselves.  Which means the BAR must be mapped, probed, and unmapped
prior to the registers being mapped individually.

The series starts by introducing the helper function
cxl_decode_register_block().  Then breaks out region reservation and register
mapping.  Separates mapping the registers into a probe stage and mapping stage.
The probe stage creates list of register blocks which is then iterated to map
the individual register blocks.

Once mapping is performed in 2 steps the pci device management is removed and
the resource reservation can be done per register block as well.

Finally, the mapping the HDM decoder register block is added.


Ben Widawsky (1):
  cxl: Add HDM decoder capbilities

Ira Weiny (4):
  cxl/mem: Introduce cxl_decode_register_block()
  cxl/mem: Reserve all device regions at once
  cxl/mem: Map registers based on capabilities
  cxl/mem: Reserve individual register block regions

 drivers/cxl/core.c | 182 +++++++++++++++++++++++++++++++++++++++++----
 drivers/cxl/cxl.h  |  98 +++++++++++++++++++++---
 drivers/cxl/pci.c  | 168 ++++++++++++++++++++++++++++++++---------
 drivers/cxl/pci.h  |   1 +
 4 files changed, 388 insertions(+), 61 deletions(-)

-- 
2.28.0.rc0.12.gb6a658bd00c9

[PATCH v2 1/5] cxl/mem: Introduce cxl_decode_register_block()

[ira.weiny]

From: Ira Weiny <ira.weiny@intel.com>

Each register block located in the DVSEC needs to be decoded from 2
words, 'register offset high' and 'register offset low'.

Create a function, cxl_decode_register_block() to perform this decode
and return the bar, offset, and register type of the register block.

Then use the values decoded in cxl_mem_map_regblock() instead of passing
the raw registers.

Signed-off-by: Ira Weiny <ira.weiny@intel.com>

---
Changes for V2:
	Push this to the start of the series
---
 drivers/cxl/pci.c | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 8bdae74d7d78..b2f978954daa 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -922,17 +922,13 @@ static struct cxl_mem *cxl_mem_create(struct pci_dev *pdev)
 	return cxlm;
 }
 
-static void __iomem *cxl_mem_map_regblock(struct cxl_mem *cxlm, u32 reg_lo, u32 reg_hi)
+static void __iomem *cxl_mem_map_regblock(struct cxl_mem *cxlm,
+					  u8 bar, u64 offset)
 {
 	struct pci_dev *pdev = cxlm->pdev;
 	struct device *dev = &pdev->dev;
-	u64 offset;
-	u8 bar;
 	int rc;
 
-	offset = ((u64)reg_hi << 32) | (reg_lo & CXL_REGLOC_ADDR_MASK);
-	bar = FIELD_GET(CXL_REGLOC_BIR_MASK, reg_lo);
-
 	/* Basic sanity check that BAR is big enough */
 	if (pci_resource_len(pdev, bar) < offset) {
 		dev_err(dev, "BAR%d: %pr: too small (offset: %#llx)\n", bar,
@@ -974,6 +970,14 @@ static int cxl_mem_dvsec(struct pci_dev *pdev, int dvsec)
 	return 0;
 }
 
+static void cxl_decode_register_block(u32 reg_lo, u32 reg_hi,
+				      u8 *bar, u64 *offset, u8 *reg_type)
+{
+	*offset = ((u64)reg_hi << 32) | (reg_lo & CXL_REGLOC_ADDR_MASK);
+	*bar = FIELD_GET(CXL_REGLOC_BIR_MASK, reg_lo);
+	*reg_type = FIELD_GET(CXL_REGLOC_RBI_MASK, reg_lo);
+}
+
 /**
  * cxl_mem_setup_regs() - Setup necessary MMIO.
  * @cxlm: The CXL memory device to communicate with.
@@ -1009,15 +1013,21 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm)
 	for (i = 0; i < regblocks; i++, regloc += 8) {
 		u32 reg_lo, reg_hi;
 		u8 reg_type;
+		u64 offset;
+		u8 bar;
 
 		/* "register low and high" contain other bits */
 		pci_read_config_dword(pdev, regloc, &reg_lo);
 		pci_read_config_dword(pdev, regloc + 4, &reg_hi);
 
-		reg_type = FIELD_GET(CXL_REGLOC_RBI_MASK, reg_lo);
+		cxl_decode_register_block(reg_lo, reg_hi, &bar, &offset,
+					  &reg_type);
+
+		dev_dbg(dev, "Found register block in bar %u @ 0x%llx of type %u\n",
+			bar, offset, reg_type);
 
 		if (reg_type == CXL_REGLOC_RBI_MEMDEV) {
-			base = cxl_mem_map_regblock(cxlm, reg_lo, reg_hi);
+			base = cxl_mem_map_regblock(cxlm, bar, offset);
 			if (IS_ERR(base))
 				return PTR_ERR(base);
 			break;
-- 
2.28.0.rc0.12.gb6a658bd00c9

Re: [PATCH v2 1/5] cxl/mem: Introduce cxl_decode_register_block()

[Jonathan Cameron]

On Fri, 21 May 2021 17:11:50 -0700
<ira.weiny@intel.com> wrote:

> From: Ira Weiny <ira.weiny@intel.com>
> 
> Each register block located in the DVSEC needs to be decoded from 2
> words, 'register offset high' and 'register offset low'.
> 
> Create a function, cxl_decode_register_block() to perform this decode
> and return the bar, offset, and register type of the register block.
> 
> Then use the values decoded in cxl_mem_map_regblock() instead of passing
> the raw registers.
> 
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
Makes sense to factor this out.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> 
> ---
> Changes for V2:
> 	Push this to the start of the series
> ---
>  drivers/cxl/pci.c | 26 ++++++++++++++++++--------
>  1 file changed, 18 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 8bdae74d7d78..b2f978954daa 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -922,17 +922,13 @@ static struct cxl_mem *cxl_mem_create(struct pci_dev *pdev)
>  	return cxlm;
>  }
>  
> -static void __iomem *cxl_mem_map_regblock(struct cxl_mem *cxlm, u32 reg_lo, u32 reg_hi)
> +static void __iomem *cxl_mem_map_regblock(struct cxl_mem *cxlm,
> +					  u8 bar, u64 offset)
>  {
>  	struct pci_dev *pdev = cxlm->pdev;
>  	struct device *dev = &pdev->dev;
> -	u64 offset;
> -	u8 bar;
>  	int rc;
>  
> -	offset = ((u64)reg_hi << 32) | (reg_lo & CXL_REGLOC_ADDR_MASK);
> -	bar = FIELD_GET(CXL_REGLOC_BIR_MASK, reg_lo);
> -
>  	/* Basic sanity check that BAR is big enough */
>  	if (pci_resource_len(pdev, bar) < offset) {
>  		dev_err(dev, "BAR%d: %pr: too small (offset: %#llx)\n", bar,
> @@ -974,6 +970,14 @@ static int cxl_mem_dvsec(struct pci_dev *pdev, int dvsec)
>  	return 0;
>  }
>  
> +static void cxl_decode_register_block(u32 reg_lo, u32 reg_hi,
> +				      u8 *bar, u64 *offset, u8 *reg_type)
> +{
> +	*offset = ((u64)reg_hi << 32) | (reg_lo & CXL_REGLOC_ADDR_MASK);
> +	*bar = FIELD_GET(CXL_REGLOC_BIR_MASK, reg_lo);
> +	*reg_type = FIELD_GET(CXL_REGLOC_RBI_MASK, reg_lo);
> +}
> +
>  /**
>   * cxl_mem_setup_regs() - Setup necessary MMIO.
>   * @cxlm: The CXL memory device to communicate with.
> @@ -1009,15 +1013,21 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm)
>  	for (i = 0; i < regblocks; i++, regloc += 8) {
>  		u32 reg_lo, reg_hi;
>  		u8 reg_type;
> +		u64 offset;
> +		u8 bar;
>  
>  		/* "register low and high" contain other bits */
>  		pci_read_config_dword(pdev, regloc, &reg_lo);
>  		pci_read_config_dword(pdev, regloc + 4, &reg_hi);
>  
> -		reg_type = FIELD_GET(CXL_REGLOC_RBI_MASK, reg_lo);
> +		cxl_decode_register_block(reg_lo, reg_hi, &bar, &offset,
> +					  &reg_type);
> +
> +		dev_dbg(dev, "Found register block in bar %u @ 0x%llx of type %u\n",
> +			bar, offset, reg_type);
>  
>  		if (reg_type == CXL_REGLOC_RBI_MEMDEV) {
> -			base = cxl_mem_map_regblock(cxlm, reg_lo, reg_hi);
> +			base = cxl_mem_map_regblock(cxlm, bar, offset);
>  			if (IS_ERR(base))
>  				return PTR_ERR(base);
>  			break;

[PATCH v2 2/5] cxl/mem: Reserve all device regions at once

[ira.weiny]

From: Ira Weiny <ira.weiny@intel.com>

In order to remap individual register sets each bar region must be
reserved prior to mapping.  Because the details of individual register
sets are contained within the BARs themselves, the bar must be mapped 2
times, once to extract this information and a second time for each
register set.

Rather than attempt to reserve each BAR individually and track if that
bar has been reserved.  Open code pcim_iomap_regions() by first
reserving all memory regions on the device and then mapping the bars
individually as needed.

NOTE pci_request_mem_regions() does not need a corresponding
pci_release_mem_regions() because the pci device is managed via
pcim_enable_device().

Signed-off-by: Ira Weiny <ira.weiny@intel.com>

---
Changes for V2:
	Rebased on https://git.kernel.org/pub/scm/linux/kernel/git/cxl/cxl.git/commit/?h=pending
	Clarify why pci_release_mem_regions() does not need to be
	called.
	Adjust for the different return code between pcim_iomap_regions() and
	pcim_iomap()
	Change print specifier.
---
 drivers/cxl/pci.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index b2f978954daa..33fc6e1634e3 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -927,7 +927,7 @@ static void __iomem *cxl_mem_map_regblock(struct cxl_mem *cxlm,
 {
 	struct pci_dev *pdev = cxlm->pdev;
 	struct device *dev = &pdev->dev;
-	int rc;
+	void __iomem *addr;
 
 	/* Basic sanity check that BAR is big enough */
 	if (pci_resource_len(pdev, bar) < offset) {
@@ -936,13 +936,14 @@ static void __iomem *cxl_mem_map_regblock(struct cxl_mem *cxlm,
 		return IOMEM_ERR_PTR(-ENXIO);
 	}
 
-	rc = pcim_iomap_regions(pdev, BIT(bar), pci_name(pdev));
-	if (rc) {
+	addr = pcim_iomap(pdev, bar, 0);
+	if (!addr) {
 		dev_err(dev, "failed to map registers\n");
-		return IOMEM_ERR_PTR(rc);
+		return addr;
 	}
 
-	dev_dbg(dev, "Mapped CXL Memory Device resource\n");
+	dev_dbg(dev, "Mapped CXL Memory Device resource bar %u @ %#llx\n",
+		bar, offset);
 
 	return pcim_iomap_table(pdev)[bar] + offset;
 }
@@ -1003,6 +1004,9 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm)
 		return -ENXIO;
 	}
 
+	if (pci_request_mem_regions(pdev, pci_name(pdev)))
+		return -ENODEV;
+
 	/* Get the size of the Register Locator DVSEC */
 	pci_read_config_dword(pdev, regloc + PCI_DVSEC_HEADER1, &regloc_size);
 	regloc_size = FIELD_GET(PCI_DVSEC_HEADER1_LENGTH_MASK, regloc_size);
@@ -1028,8 +1032,8 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm)
 
 		if (reg_type == CXL_REGLOC_RBI_MEMDEV) {
 			base = cxl_mem_map_regblock(cxlm, bar, offset);
-			if (IS_ERR(base))
-				return PTR_ERR(base);
+			if (!base)
+				return -ENOMEM;
 			break;
 		}
 	}
-- 
2.28.0.rc0.12.gb6a658bd00c9

Re: [PATCH v2 2/5] cxl/mem: Reserve all device regions at once

[Jonathan Cameron]

On Fri, 21 May 2021 17:11:51 -0700
<ira.weiny@intel.com> wrote:

> From: Ira Weiny <ira.weiny@intel.com>
> 
> In order to remap individual register sets each bar region must be
> reserved prior to mapping.  Because the details of individual register
> sets are contained within the BARs themselves, the bar must be mapped 2
> times, once to extract this information and a second time for each
> register set.
> 
> Rather than attempt to reserve each BAR individually and track if that
> bar has been reserved.  Open code pcim_iomap_regions() by first
> reserving all memory regions on the device and then mapping the bars
> individually as needed.
> 
> NOTE pci_request_mem_regions() does not need a corresponding
> pci_release_mem_regions() because the pci device is managed via
> pcim_enable_device().
> 
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> 
> ---
> Changes for V2:
> 	Rebased on https://git.kernel.org/pub/scm/linux/kernel/git/cxl/cxl.git/commit/?h=pending
> 	Clarify why pci_release_mem_regions() does not need to be
> 	called.

Gah - I'm never keen on hidden automated cleanup.  Oh well.

> 	Adjust for the different return code between pcim_iomap_regions() and
> 	pcim_iomap()
> 	Change print specifier.
> ---
>  drivers/cxl/pci.c | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index b2f978954daa..33fc6e1634e3 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -927,7 +927,7 @@ static void __iomem *cxl_mem_map_regblock(struct cxl_mem *cxlm,
>  {
>  	struct pci_dev *pdev = cxlm->pdev;
>  	struct device *dev = &pdev->dev;
> -	int rc;
> +	void __iomem *addr;
>  
>  	/* Basic sanity check that BAR is big enough */
>  	if (pci_resource_len(pdev, bar) < offset) {
> @@ -936,13 +936,14 @@ static void __iomem *cxl_mem_map_regblock(struct cxl_mem *cxlm,
>  		return IOMEM_ERR_PTR(-ENXIO);
>  	}
>  
> -	rc = pcim_iomap_regions(pdev, BIT(bar), pci_name(pdev));
> -	if (rc) {
> +	addr = pcim_iomap(pdev, bar, 0);
> +	if (!addr) {
>  		dev_err(dev, "failed to map registers\n");
> -		return IOMEM_ERR_PTR(rc);
> +		return addr;
>  	}
>  
> -	dev_dbg(dev, "Mapped CXL Memory Device resource\n");
> +	dev_dbg(dev, "Mapped CXL Memory Device resource bar %u @ %#llx\n",
> +		bar, offset);
>  
>  	return pcim_iomap_table(pdev)[bar] + offset;
>  }
> @@ -1003,6 +1004,9 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm)
>  		return -ENXIO;
>  	}
>  
> +	if (pci_request_mem_regions(pdev, pci_name(pdev)))
> +		return -ENODEV;
> +
>  	/* Get the size of the Register Locator DVSEC */
>  	pci_read_config_dword(pdev, regloc + PCI_DVSEC_HEADER1, &regloc_size);
>  	regloc_size = FIELD_GET(PCI_DVSEC_HEADER1_LENGTH_MASK, regloc_size);
> @@ -1028,8 +1032,8 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm)
>  
>  		if (reg_type == CXL_REGLOC_RBI_MEMDEV) {
>  			base = cxl_mem_map_regblock(cxlm, bar, offset);
> -			if (IS_ERR(base))
> -				return PTR_ERR(base);
> +			if (!base)
> +				return -ENOMEM;
>  			break;
>  		}
>  	}

[PATCH v2 3/5] cxl/mem: Map registers based on capabilities

[ira.weiny]

From: Ira Weiny <ira.weiny@intel.com>

The information required to map registers based on capabilities is
contained within the bars themselves.  This means the bar must be mapped
to read the information needed and then unmapped to map the individual
parts of the BAR based on capabilities.

Change cxl_setup_device_regs() to return a new cxl_register_map, change
the name to cxl_probe_device_regs().  Allocate and place
cxl_register_maps on a list while processing all of the specified
register blocks.

After probing all the register blocks go back and map smaller registers
blocks based on their capabilities and dispose of the cxl_register_maps.

NOTE: pci_iomap() is not managed automatically via pcim_enable_device()
so be careful to call pci_iounmap() correctly.

Signed-off-by: Ira Weiny <ira.weiny@intel.com>

---
Changes for v2:
	Rebased on https://git.kernel.org/pub/scm/linux/kernel/git/cxl/cxl.git/commit/?h=pending
	Squash in length read from previous patch which was dropped
	because it was not needed in a separate patch.
	Adjust to changes from previous patches
---
 drivers/cxl/core.c |  73 +++++++++++++++++++++++------
 drivers/cxl/cxl.h  |  33 ++++++++++++--
 drivers/cxl/pci.c  | 111 ++++++++++++++++++++++++++++++++++++---------
 3 files changed, 177 insertions(+), 40 deletions(-)

diff --git a/drivers/cxl/core.c b/drivers/cxl/core.c
index 38979c97158d..add66a6ec875 100644
--- a/drivers/cxl/core.c
+++ b/drivers/cxl/core.c
@@ -3,6 +3,7 @@
 #include <linux/io-64-nonatomic-lo-hi.h>
 #include <linux/device.h>
 #include <linux/module.h>
+#include <linux/pci.h>
 #include "cxl.h"
 
 /**
@@ -12,19 +13,13 @@
  * point for cross-device interleave coordination through cxl ports.
  */
 
-/**
- * cxl_setup_device_regs() - Detect CXL Device register blocks
- * @dev: Host device of the @base mapping
- * @base: Mapping of CXL 2.0 8.2.8 CXL Device Register Interface
- * @regs: Base pointers for device register blocks (see CXL_DEVICE_REGS())
- */
-void cxl_setup_device_regs(struct device *dev, void __iomem *base,
-			   struct cxl_device_regs *regs)
+void cxl_probe_device_regs(struct device *dev, void __iomem *base,
+			   struct cxl_device_reg_map *map)
 {
 	int cap, cap_count;
 	u64 cap_array;
 
-	*regs = (struct cxl_device_regs) { 0 };
+	*map = (struct cxl_device_reg_map){ 0 };
 
 	cap_array = readq(base + CXLDEV_CAP_ARRAY_OFFSET);
 	if (FIELD_GET(CXLDEV_CAP_ARRAY_ID_MASK, cap_array) !=
@@ -35,29 +30,38 @@ void cxl_setup_device_regs(struct device *dev, void __iomem *base,
 
 	for (cap = 1; cap <= cap_count; cap++) {
 		void __iomem *register_block;
-		u32 offset;
+		u32 offset, length;
 		u16 cap_id;
 
 		cap_id = FIELD_GET(CXLDEV_CAP_HDR_CAP_ID_MASK,
 				   readl(base + cap * 0x10));
 		offset = readl(base + cap * 0x10 + 0x4);
+		length = readl(base + cap * 0x10 + 0x8);
+
 		register_block = base + offset;
 
 		switch (cap_id) {
 		case CXLDEV_CAP_CAP_ID_DEVICE_STATUS:
 			dev_dbg(dev, "found Status capability (0x%x)\n", offset);
-			regs->status = register_block;
+
+			map->status.valid = true;
+			map->status.offset = offset;
+			map->status.size = length;
 			break;
 		case CXLDEV_CAP_CAP_ID_PRIMARY_MAILBOX:
 			dev_dbg(dev, "found Mailbox capability (0x%x)\n", offset);
-			regs->mbox = register_block;
+			map->mbox.valid = true;
+			map->mbox.offset = offset;
+			map->mbox.size = length;
 			break;
 		case CXLDEV_CAP_CAP_ID_SECONDARY_MAILBOX:
 			dev_dbg(dev, "found Secondary Mailbox capability (0x%x)\n", offset);
 			break;
 		case CXLDEV_CAP_CAP_ID_MEMDEV:
 			dev_dbg(dev, "found Memory Device capability (0x%x)\n", offset);
-			regs->memdev = register_block;
+			map->memdev.valid = true;
+			map->memdev.offset = offset;
+			map->memdev.size = length;
 			break;
 		default:
 			if (cap_id >= 0x8000)
@@ -68,7 +72,48 @@ void cxl_setup_device_regs(struct device *dev, void __iomem *base,
 		}
 	}
 }
-EXPORT_SYMBOL_GPL(cxl_setup_device_regs);
+EXPORT_SYMBOL_GPL(cxl_probe_device_regs);
+
+int cxl_map_device_regs(struct pci_dev *pdev,
+			struct cxl_device_regs *regs,
+			struct cxl_register_map *map)
+{
+	struct device *dev = &pdev->dev;
+	resource_size_t phys_addr;
+
+	phys_addr = pci_resource_start(pdev, map->barno);
+	phys_addr += map->block_offset;
+
+	if (map->device_map.status.valid) {
+		resource_size_t addr;
+		resource_size_t length;
+
+		addr = phys_addr + map->device_map.status.offset;
+		length = map->device_map.status.size;
+		regs->status = devm_ioremap(dev, addr, length);
+	}
+
+	if (map->device_map.mbox.valid) {
+		resource_size_t addr;
+		resource_size_t length;
+
+		addr = phys_addr + map->device_map.mbox.offset;
+		length = map->device_map.mbox.size;
+		regs->mbox = devm_ioremap(dev, addr, length);
+	}
+
+	if (map->device_map.memdev.valid) {
+		resource_size_t addr;
+		resource_size_t length;
+
+		addr = phys_addr + map->device_map.memdev.offset;
+		length = map->device_map.memdev.size;
+		regs->memdev = devm_ioremap(dev, addr, length);
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(cxl_map_device_regs);
 
 struct bus_type cxl_bus_type = {
 	.name = "cxl",
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index d49e0cb679fa..ae4b4c96c6b5 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -53,9 +53,7 @@ struct cxl_device_regs {
 /*
  * Note, the anonymous union organization allows for per
  * register-block-type helper routines, without requiring block-type
- * agnostic code to include the prefix. I.e.
- * cxl_setup_device_regs(&cxlm->regs.dev) vs readl(cxlm->regs.mbox).
- * The specificity reads naturally from left-to-right.
+ * agnostic code to include the prefix.
  */
 struct cxl_regs {
 	union {
@@ -66,8 +64,33 @@ struct cxl_regs {
 	};
 };
 
-void cxl_setup_device_regs(struct device *dev, void __iomem *base,
-			   struct cxl_device_regs *regs);
+struct cxl_reg_map {
+	bool valid;
+	unsigned long offset;
+	unsigned long size;
+};
+
+struct cxl_device_reg_map {
+	struct cxl_reg_map status;
+	struct cxl_reg_map mbox;
+	struct cxl_reg_map memdev;
+};
+
+struct cxl_register_map {
+	struct list_head list;
+	u64 block_offset;
+	u8 reg_type;
+	u8 barno;
+	union {
+		struct cxl_device_reg_map device_map;
+	};
+};
+
+void cxl_probe_device_regs(struct device *dev, void __iomem *base,
+			   struct cxl_device_reg_map *map);
+int cxl_map_device_regs(struct pci_dev *pdev,
+			struct cxl_device_regs *regs,
+			struct cxl_register_map *map);
 
 extern struct bus_type cxl_bus_type;
 #endif /* __CXL_H__ */
diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 33fc6e1634e3..3ffd5fad74b4 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -6,6 +6,7 @@
 #include <linux/module.h>
 #include <linux/sizes.h>
 #include <linux/mutex.h>
+#include <linux/list.h>
 #include <linux/cdev.h>
 #include <linux/idr.h>
 #include <linux/pci.h>
@@ -936,7 +937,7 @@ static void __iomem *cxl_mem_map_regblock(struct cxl_mem *cxlm,
 		return IOMEM_ERR_PTR(-ENXIO);
 	}
 
-	addr = pcim_iomap(pdev, bar, 0);
+	addr = pci_iomap(pdev, bar, 0);
 	if (!addr) {
 		dev_err(dev, "failed to map registers\n");
 		return addr;
@@ -945,7 +946,12 @@ static void __iomem *cxl_mem_map_regblock(struct cxl_mem *cxlm,
 	dev_dbg(dev, "Mapped CXL Memory Device resource bar %u @ %#llx\n",
 		bar, offset);
 
-	return pcim_iomap_table(pdev)[bar] + offset;
+	return addr;
+}
+
+static void cxl_mem_unmap_regblock(struct cxl_mem *cxlm, void __iomem *base)
+{
+	pci_iounmap(cxlm->pdev, base);
 }
 
 static int cxl_mem_dvsec(struct pci_dev *pdev, int dvsec)
@@ -971,6 +977,52 @@ static int cxl_mem_dvsec(struct pci_dev *pdev, int dvsec)
 	return 0;
 }
 
+static int cxl_probe_regs(struct cxl_mem *cxlm, void __iomem *base,
+			  struct cxl_register_map *map)
+{
+	struct pci_dev *pdev = cxlm->pdev;
+	struct device *dev = &pdev->dev;
+	struct cxl_device_reg_map *dev_map;
+
+	switch (map->reg_type) {
+	case CXL_REGLOC_RBI_MEMDEV:
+		dev_map = &map->device_map;
+		cxl_probe_device_regs(dev, base, dev_map);
+		if (!dev_map->status.valid || !dev_map->mbox.valid ||
+		    !dev_map->memdev.valid) {
+			dev_err(dev, "registers not found: %s%s%s\n",
+				!dev_map->status.valid ? "status " : "",
+				!dev_map->mbox.valid ? "status " : "",
+				!dev_map->memdev.valid ? "status " : "");
+			return -ENXIO;
+		}
+
+		dev_dbg(dev, "Probing device registers...\n");
+		break;
+	default:
+		break;
+	}
+
+	return 0;
+}
+
+static int cxl_map_regs(struct cxl_mem *cxlm, struct cxl_register_map *map)
+{
+	struct pci_dev *pdev = cxlm->pdev;
+	struct device *dev = &pdev->dev;
+
+	switch (map->reg_type) {
+	case CXL_REGLOC_RBI_MEMDEV:
+		cxl_map_device_regs(pdev, &cxlm->regs.device_regs, map);
+		dev_dbg(dev, "Probing device registers...\n");
+		break;
+	default:
+		break;
+	}
+
+	return 0;
+}
+
 static void cxl_decode_register_block(u32 reg_lo, u32 reg_hi,
 				      u8 *bar, u64 *offset, u8 *reg_type)
 {
@@ -991,12 +1043,14 @@ static void cxl_decode_register_block(u32 reg_lo, u32 reg_hi,
  */
 static int cxl_mem_setup_regs(struct cxl_mem *cxlm)
 {
-	struct cxl_regs *regs = &cxlm->regs;
 	struct pci_dev *pdev = cxlm->pdev;
 	struct device *dev = &pdev->dev;
 	u32 regloc_size, regblocks;
 	void __iomem *base;
 	int regloc, i;
+	struct cxl_register_map *map, *n;
+	LIST_HEAD(register_maps);
+	int ret = 0;
 
 	regloc = cxl_mem_dvsec(pdev, PCI_DVSEC_ID_CXL_REGLOC_OFFSET);
 	if (!regloc) {
@@ -1020,7 +1074,14 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm)
 		u64 offset;
 		u8 bar;
 
-		/* "register low and high" contain other bits */
+		map = kzalloc(sizeof(*map), GFP_KERNEL);
+		if (!map) {
+			ret = -ENOMEM;
+			goto free_maps;
+		}
+
+		list_add(&map->list, &register_maps);
+
 		pci_read_config_dword(pdev, regloc, &reg_lo);
 		pci_read_config_dword(pdev, regloc + 4, &reg_hi);
 
@@ -1030,30 +1091,38 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm)
 		dev_dbg(dev, "Found register block in bar %u @ 0x%llx of type %u\n",
 			bar, offset, reg_type);
 
-		if (reg_type == CXL_REGLOC_RBI_MEMDEV) {
-			base = cxl_mem_map_regblock(cxlm, bar, offset);
-			if (!base)
-				return -ENOMEM;
-			break;
+		base = cxl_mem_map_regblock(cxlm, bar, offset);
+		if (!base) {
+			ret = -ENOMEM;
+			goto free_maps;
 		}
-	}
 
-	if (i == regblocks) {
-		dev_err(dev, "Missing register locator for device registers\n");
-		return -ENXIO;
+		map->barno = bar;
+		map->block_offset = offset;
+		map->reg_type = reg_type;
+
+		ret = cxl_probe_regs(cxlm, base + offset, map);
+
+		/* Always unmap the regblock regardless of probe success */
+		cxl_mem_unmap_regblock(cxlm, base);
+
+		if (ret)
+			goto free_maps;
 	}
 
-	cxl_setup_device_regs(dev, base, &regs->device_regs);
+	list_for_each_entry(map, &register_maps, list) {
+		ret = cxl_map_regs(cxlm, map);
+		if (ret)
+			goto free_maps;
+	}
 
-	if (!regs->status || !regs->mbox || !regs->memdev) {
-		dev_err(dev, "registers not found: %s%s%s\n",
-			!regs->status ? "status " : "",
-			!regs->mbox ? "mbox " : "",
-			!regs->memdev ? "memdev" : "");
-		return -ENXIO;
+free_maps:
+	list_for_each_entry_safe(map, n, &register_maps, list) {
+		list_del(&map->list);
+		kfree(map);
 	}
 
-	return 0;
+	return ret;
 }
 
 static struct cxl_memdev *to_cxl_memdev(struct device *dev)
-- 
2.28.0.rc0.12.gb6a658bd00c9

Re: [PATCH v2 3/5] cxl/mem: Map registers based on capabilities

[Jonathan Cameron]

On Fri, 21 May 2021 17:11:52 -0700
<ira.weiny@intel.com> wrote:

> From: Ira Weiny <ira.weiny@intel.com>
> 
> The information required to map registers based on capabilities is
> contained within the bars themselves.  This means the bar must be mapped
> to read the information needed and then unmapped to map the individual
> parts of the BAR based on capabilities.
> 
> Change cxl_setup_device_regs() to return a new cxl_register_map, change
> the name to cxl_probe_device_regs().  Allocate and place
> cxl_register_maps on a list while processing all of the specified
> register blocks.
> 
> After probing all the register blocks go back and map smaller registers
> blocks based on their capabilities and dispose of the cxl_register_maps.
> 
> NOTE: pci_iomap() is not managed automatically via pcim_enable_device()
> so be careful to call pci_iounmap() correctly.
> 
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
A couple of really minor queries inline, but otherwise looks good to me.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> 
> ---
> Changes for v2:
> 	Rebased on https://git.kernel.org/pub/scm/linux/kernel/git/cxl/cxl.git/commit/?h=pending
> 	Squash in length read from previous patch which was dropped
> 	because it was not needed in a separate patch.
> 	Adjust to changes from previous patches
> ---
>  drivers/cxl/core.c |  73 +++++++++++++++++++++++------
>  drivers/cxl/cxl.h  |  33 ++++++++++++--
>  drivers/cxl/pci.c  | 111 ++++++++++++++++++++++++++++++++++++---------
>  3 files changed, 177 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/cxl/core.c b/drivers/cxl/core.c
> index 38979c97158d..add66a6ec875 100644
> --- a/drivers/cxl/core.c
> +++ b/drivers/cxl/core.c
> @@ -3,6 +3,7 @@
>  #include <linux/io-64-nonatomic-lo-hi.h>
>  #include <linux/device.h>
>  #include <linux/module.h>
> +#include <linux/pci.h>
>  #include "cxl.h"
>  
>  /**
> @@ -12,19 +13,13 @@
>   * point for cross-device interleave coordination through cxl ports.
>   */
>  
> -/**
> - * cxl_setup_device_regs() - Detect CXL Device register blocks
> - * @dev: Host device of the @base mapping
> - * @base: Mapping of CXL 2.0 8.2.8 CXL Device Register Interface
> - * @regs: Base pointers for device register blocks (see CXL_DEVICE_REGS())
> - */

Nice to keep docs given this is an exported function.

> -void cxl_setup_device_regs(struct device *dev, void __iomem *base,
> -			   struct cxl_device_regs *regs)
> +void cxl_probe_device_regs(struct device *dev, void __iomem *base,
> +			   struct cxl_device_reg_map *map)
>  {
>  	int cap, cap_count;
>  	u64 cap_array;
>  
> -	*regs = (struct cxl_device_regs) { 0 };
> +	*map = (struct cxl_device_reg_map){ 0 };
>  
>  	cap_array = readq(base + CXLDEV_CAP_ARRAY_OFFSET);
>  	if (FIELD_GET(CXLDEV_CAP_ARRAY_ID_MASK, cap_array) !=
> @@ -35,29 +30,38 @@ void cxl_setup_device_regs(struct device *dev, void __iomem *base,
>  
>  	for (cap = 1; cap <= cap_count; cap++) {
>  		void __iomem *register_block;
> -		u32 offset;
> +		u32 offset, length;
>  		u16 cap_id;
>  
>  		cap_id = FIELD_GET(CXLDEV_CAP_HDR_CAP_ID_MASK,
>  				   readl(base + cap * 0x10));
>  		offset = readl(base + cap * 0x10 + 0x4);
> +		length = readl(base + cap * 0x10 + 0x8);
> +
>  		register_block = base + offset;
>  
>  		switch (cap_id) {
>  		case CXLDEV_CAP_CAP_ID_DEVICE_STATUS:
>  			dev_dbg(dev, "found Status capability (0x%x)\n", offset);
> -			regs->status = register_block;
> +
> +			map->status.valid = true;
> +			map->status.offset = offset;
> +			map->status.size = length;
>  			break;
>  		case CXLDEV_CAP_CAP_ID_PRIMARY_MAILBOX:
>  			dev_dbg(dev, "found Mailbox capability (0x%x)\n", offset);
> -			regs->mbox = register_block;
> +			map->mbox.valid = true;
> +			map->mbox.offset = offset;
> +			map->mbox.size = length;
>  			break;
>  		case CXLDEV_CAP_CAP_ID_SECONDARY_MAILBOX:
>  			dev_dbg(dev, "found Secondary Mailbox capability (0x%x)\n", offset);
>  			break;
>  		case CXLDEV_CAP_CAP_ID_MEMDEV:
>  			dev_dbg(dev, "found Memory Device capability (0x%x)\n", offset);
> -			regs->memdev = register_block;
> +			map->memdev.valid = true;
> +			map->memdev.offset = offset;
> +			map->memdev.size = length;
>  			break;
>  		default:
>  			if (cap_id >= 0x8000)
> @@ -68,7 +72,48 @@ void cxl_setup_device_regs(struct device *dev, void __iomem *base,
>  		}
>  	}
>  }
> -EXPORT_SYMBOL_GPL(cxl_setup_device_regs);
> +EXPORT_SYMBOL_GPL(cxl_probe_device_regs);
> +
> +int cxl_map_device_regs(struct pci_dev *pdev,
> +			struct cxl_device_regs *regs,
> +			struct cxl_register_map *map)
> +{
> +	struct device *dev = &pdev->dev;
> +	resource_size_t phys_addr;
> +
> +	phys_addr = pci_resource_start(pdev, map->barno);
> +	phys_addr += map->block_offset;
> +
> +	if (map->device_map.status.valid) {
> +		resource_size_t addr;
> +		resource_size_t length;
> +
> +		addr = phys_addr + map->device_map.status.offset;
> +		length = map->device_map.status.size;
> +		regs->status = devm_ioremap(dev, addr, length);
> +	}
> +
> +	if (map->device_map.mbox.valid) {
> +		resource_size_t addr;
> +		resource_size_t length;
> +
> +		addr = phys_addr + map->device_map.mbox.offset;
> +		length = map->device_map.mbox.size;
> +		regs->mbox = devm_ioremap(dev, addr, length);
> +	}
> +
> +	if (map->device_map.memdev.valid) {
> +		resource_size_t addr;
> +		resource_size_t length;
> +
> +		addr = phys_addr + map->device_map.memdev.offset;
> +		length = map->device_map.memdev.size;
> +		regs->memdev = devm_ioremap(dev, addr, length);
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(cxl_map_device_regs);
>  
>  struct bus_type cxl_bus_type = {
>  	.name = "cxl",
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index d49e0cb679fa..ae4b4c96c6b5 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -53,9 +53,7 @@ struct cxl_device_regs {
>  /*
>   * Note, the anonymous union organization allows for per
>   * register-block-type helper routines, without requiring block-type
> - * agnostic code to include the prefix. I.e.
> - * cxl_setup_device_regs(&cxlm->regs.dev) vs readl(cxlm->regs.mbox).
> - * The specificity reads naturally from left-to-right.
> + * agnostic code to include the prefix.
>   */
>  struct cxl_regs {
>  	union {
> @@ -66,8 +64,33 @@ struct cxl_regs {
>  	};
>  };
>  
> -void cxl_setup_device_regs(struct device *dev, void __iomem *base,
> -			   struct cxl_device_regs *regs);
> +struct cxl_reg_map {
> +	bool valid;
> +	unsigned long offset;
> +	unsigned long size;
> +};
> +
> +struct cxl_device_reg_map {
> +	struct cxl_reg_map status;
> +	struct cxl_reg_map mbox;
> +	struct cxl_reg_map memdev;
> +};
> +
> +struct cxl_register_map {
> +	struct list_head list;
> +	u64 block_offset;
> +	u8 reg_type;
> +	u8 barno;
> +	union {
> +		struct cxl_device_reg_map device_map;
> +	};
> +};
> +
> +void cxl_probe_device_regs(struct device *dev, void __iomem *base,
> +			   struct cxl_device_reg_map *map);
> +int cxl_map_device_regs(struct pci_dev *pdev,
> +			struct cxl_device_regs *regs,
> +			struct cxl_register_map *map);
>  
>  extern struct bus_type cxl_bus_type;
>  #endif /* __CXL_H__ */
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 33fc6e1634e3..3ffd5fad74b4 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -6,6 +6,7 @@
>  #include <linux/module.h>
>  #include <linux/sizes.h>
>  #include <linux/mutex.h>
> +#include <linux/list.h>
>  #include <linux/cdev.h>
>  #include <linux/idr.h>
>  #include <linux/pci.h>
> @@ -936,7 +937,7 @@ static void __iomem *cxl_mem_map_regblock(struct cxl_mem *cxlm,
>  		return IOMEM_ERR_PTR(-ENXIO);
>  	}
>  
> -	addr = pcim_iomap(pdev, bar, 0);
> +	addr = pci_iomap(pdev, bar, 0);
>  	if (!addr) {
>  		dev_err(dev, "failed to map registers\n");
>  		return addr;
> @@ -945,7 +946,12 @@ static void __iomem *cxl_mem_map_regblock(struct cxl_mem *cxlm,
>  	dev_dbg(dev, "Mapped CXL Memory Device resource bar %u @ %#llx\n",
>  		bar, offset);
>  
> -	return pcim_iomap_table(pdev)[bar] + offset;
> +	return addr;
> +}
> +
> +static void cxl_mem_unmap_regblock(struct cxl_mem *cxlm, void __iomem *base)
> +{
> +	pci_iounmap(cxlm->pdev, base);
>  }
>  
>  static int cxl_mem_dvsec(struct pci_dev *pdev, int dvsec)
> @@ -971,6 +977,52 @@ static int cxl_mem_dvsec(struct pci_dev *pdev, int dvsec)
>  	return 0;
>  }
>  
> +static int cxl_probe_regs(struct cxl_mem *cxlm, void __iomem *base,
> +			  struct cxl_register_map *map)
> +{
> +	struct pci_dev *pdev = cxlm->pdev;
> +	struct device *dev = &pdev->dev;
> +	struct cxl_device_reg_map *dev_map;
> +
> +	switch (map->reg_type) {
> +	case CXL_REGLOC_RBI_MEMDEV:
> +		dev_map = &map->device_map;
> +		cxl_probe_device_regs(dev, base, dev_map);
> +		if (!dev_map->status.valid || !dev_map->mbox.valid ||
> +		    !dev_map->memdev.valid) {
> +			dev_err(dev, "registers not found: %s%s%s\n",
> +				!dev_map->status.valid ? "status " : "",
> +				!dev_map->mbox.valid ? "status " : "",
> +				!dev_map->memdev.valid ? "status " : "");
> +			return -ENXIO;
> +		}
> +
> +		dev_dbg(dev, "Probing device registers...\n");
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +static int cxl_map_regs(struct cxl_mem *cxlm, struct cxl_register_map *map)
> +{
> +	struct pci_dev *pdev = cxlm->pdev;
> +	struct device *dev = &pdev->dev;
> +
> +	switch (map->reg_type) {
> +	case CXL_REGLOC_RBI_MEMDEV:
> +		cxl_map_device_regs(pdev, &cxlm->regs.device_regs, map);
> +		dev_dbg(dev, "Probing device registers...\n");
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
>  static void cxl_decode_register_block(u32 reg_lo, u32 reg_hi,
>  				      u8 *bar, u64 *offset, u8 *reg_type)
>  {
> @@ -991,12 +1043,14 @@ static void cxl_decode_register_block(u32 reg_lo, u32 reg_hi,
>   */
>  static int cxl_mem_setup_regs(struct cxl_mem *cxlm)
>  {
> -	struct cxl_regs *regs = &cxlm->regs;
>  	struct pci_dev *pdev = cxlm->pdev;
>  	struct device *dev = &pdev->dev;
>  	u32 regloc_size, regblocks;
>  	void __iomem *base;
>  	int regloc, i;
> +	struct cxl_register_map *map, *n;
> +	LIST_HEAD(register_maps);
> +	int ret = 0;
>  
>  	regloc = cxl_mem_dvsec(pdev, PCI_DVSEC_ID_CXL_REGLOC_OFFSET);
>  	if (!regloc) {
> @@ -1020,7 +1074,14 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm)
>  		u64 offset;
>  		u8 bar;
>  
> -		/* "register low and high" contain other bits */
> +		map = kzalloc(sizeof(*map), GFP_KERNEL);
> +		if (!map) {
> +			ret = -ENOMEM;
> +			goto free_maps;
> +		}
> +
> +		list_add(&map->list, &register_maps);
> +
>  		pci_read_config_dword(pdev, regloc, &reg_lo);
>  		pci_read_config_dword(pdev, regloc + 4, &reg_hi);
>  
> @@ -1030,30 +1091,38 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm)
>  		dev_dbg(dev, "Found register block in bar %u @ 0x%llx of type %u\n",
>  			bar, offset, reg_type);
>  
> -		if (reg_type == CXL_REGLOC_RBI_MEMDEV) {
> -			base = cxl_mem_map_regblock(cxlm, bar, offset);
> -			if (!base)
> -				return -ENOMEM;
> -			break;
> +		base = cxl_mem_map_regblock(cxlm, bar, offset);
> +		if (!base) {
> +			ret = -ENOMEM;
> +			goto free_maps;
>  		}
> -	}
>  
> -	if (i == regblocks) {
> -		dev_err(dev, "Missing register locator for device registers\n");
> -		return -ENXIO;

Do we have or need an equivalent of this check somewhere else?

> +		map->barno = bar;
> +		map->block_offset = offset;
> +		map->reg_type = reg_type;
> +
> +		ret = cxl_probe_regs(cxlm, base + offset, map);
> +
> +		/* Always unmap the regblock regardless of probe success */
> +		cxl_mem_unmap_regblock(cxlm, base);
> +
> +		if (ret)
> +			goto free_maps;
>  	}
>  
> -	cxl_setup_device_regs(dev, base, &regs->device_regs);
> +	list_for_each_entry(map, &register_maps, list) {
> +		ret = cxl_map_regs(cxlm, map);
> +		if (ret)
> +			goto free_maps;
> +	}
>  
> -	if (!regs->status || !regs->mbox || !regs->memdev) {
> -		dev_err(dev, "registers not found: %s%s%s\n",
> -			!regs->status ? "status " : "",
> -			!regs->mbox ? "mbox " : "",
> -			!regs->memdev ? "memdev" : "");
> -		return -ENXIO;
> +free_maps:
> +	list_for_each_entry_safe(map, n, &register_maps, list) {
> +		list_del(&map->list);
> +		kfree(map);
>  	}
>  
> -	return 0;
> +	return ret;
>  }
>  
>  static struct cxl_memdev *to_cxl_memdev(struct device *dev)

Re: [PATCH v2 3/5] cxl/mem: Map registers based on capabilities

[Ira Weiny]

On Tue, May 25, 2021 at 10:52:14AM +0100, Jonathan Cameron wrote:
> On Fri, 21 May 2021 17:11:52 -0700
> <ira.weiny@intel.com> wrote:
> 
> > From: Ira Weiny <ira.weiny@intel.com>
> > 
> > The information required to map registers based on capabilities is
> > contained within the bars themselves.  This means the bar must be mapped
> > to read the information needed and then unmapped to map the individual
> > parts of the BAR based on capabilities.
> > 
> > Change cxl_setup_device_regs() to return a new cxl_register_map, change
> > the name to cxl_probe_device_regs().  Allocate and place
> > cxl_register_maps on a list while processing all of the specified
> > register blocks.
> > 
> > After probing all the register blocks go back and map smaller registers
> > blocks based on their capabilities and dispose of the cxl_register_maps.
> > 
> > NOTE: pci_iomap() is not managed automatically via pcim_enable_device()
> > so be careful to call pci_iounmap() correctly.
> > 
> > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> A couple of really minor queries inline, but otherwise looks good to me.
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Thanks!

> 
> > 
> > diff --git a/drivers/cxl/core.c b/drivers/cxl/core.c
> > index 38979c97158d..add66a6ec875 100644
> > --- a/drivers/cxl/core.c
> > +++ b/drivers/cxl/core.c
> > @@ -3,6 +3,7 @@
> >  #include <linux/io-64-nonatomic-lo-hi.h>
> >  #include <linux/device.h>
> >  #include <linux/module.h>
> > +#include <linux/pci.h>
> >  #include "cxl.h"
> >  
> >  /**
> > @@ -12,19 +13,13 @@
> >   * point for cross-device interleave coordination through cxl ports.
> >   */
> >  
> > -/**
> > - * cxl_setup_device_regs() - Detect CXL Device register blocks
> > - * @dev: Host device of the @base mapping
> > - * @base: Mapping of CXL 2.0 8.2.8 CXL Device Register Interface
> > - * @regs: Base pointers for device register blocks (see CXL_DEVICE_REGS())
> > - */
> 
> Nice to keep docs given this is an exported function.

I can write something better but the above does not add much IMO.  The
parameter explanations are unnecessary IMO.

> >  
> > @@ -1030,30 +1091,38 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm)
> >  		dev_dbg(dev, "Found register block in bar %u @ 0x%llx of type %u\n",
> >  			bar, offset, reg_type);
> >  
> > -		if (reg_type == CXL_REGLOC_RBI_MEMDEV) {
> > -			base = cxl_mem_map_regblock(cxlm, bar, offset);
> > -			if (!base)
> > -				return -ENOMEM;
> > -			break;
> > +		base = cxl_mem_map_regblock(cxlm, bar, offset);
> > +		if (!base) {
> > +			ret = -ENOMEM;
> > +			goto free_maps;
> >  		}
> > -	}
> >  
> > -	if (i == regblocks) {
> > -		dev_err(dev, "Missing register locator for device registers\n");
> > -		return -ENXIO;
> 
> Do we have or need an equivalent of this check somewhere else?

Yes agreed!  I moved the check to cxl_probe_regs which returns -ENXIO if the
register sets expected are not found.  A check is also added to RBI_COMPONENT
register type in the following patch.  This was moved mainly because each
register type is going to know better what it needs for proper operation.
cxl_probe_device_regs() really can't know that after this series.

Ira

[PATCH v2 4/5] cxl/mem: Reserve individual register block regions

[ira.weiny]

From: Ira Weiny <ira.weiny@intel.com>

Now that individual register blocks are mapped; those blocks regions
should be reserved individually.

Remove general pci device management and create managed region
reservations based on the individual register blocks.

Signed-off-by: Ira Weiny <ira.weiny@intel.com>

---
Changes for V2:
	New patch
---
 drivers/cxl/core.c | 36 ++++++++++++++++++++++++++++++++----
 drivers/cxl/pci.c  |  6 ++----
 2 files changed, 34 insertions(+), 8 deletions(-)

diff --git a/drivers/cxl/core.c b/drivers/cxl/core.c
index add66a6ec875..ae38f17be1e7 100644
--- a/drivers/cxl/core.c
+++ b/drivers/cxl/core.c
@@ -74,11 +74,33 @@ void cxl_probe_device_regs(struct device *dev, void __iomem *base,
 }
 EXPORT_SYMBOL_GPL(cxl_probe_device_regs);
 
+static void __iomem *cxl_ioremap_block(struct pci_dev *pdev,
+				       resource_size_t addr,
+				       resource_size_t length)
+{
+	struct device *dev = &pdev->dev;
+	void __iomem *ret_val;
+	struct resource *res;
+
+	res = devm_request_mem_region(dev, addr, length, pci_name(pdev));
+	if (!res) {
+		dev_err(dev, "Failed to request region %#llx-%#llx\n",
+			addr, addr+length);
+		return NULL;
+	}
+
+	ret_val = devm_ioremap(dev, addr, length);
+	if (!ret_val)
+		dev_err(dev, "Failed to map region %#llx-%#llx\n",
+			addr, addr+length);
+
+	return ret_val;
+}
+
 int cxl_map_device_regs(struct pci_dev *pdev,
 			struct cxl_device_regs *regs,
 			struct cxl_register_map *map)
 {
-	struct device *dev = &pdev->dev;
 	resource_size_t phys_addr;
 
 	phys_addr = pci_resource_start(pdev, map->barno);
@@ -90,7 +112,9 @@ int cxl_map_device_regs(struct pci_dev *pdev,
 
 		addr = phys_addr + map->device_map.status.offset;
 		length = map->device_map.status.size;
-		regs->status = devm_ioremap(dev, addr, length);
+		regs->status = cxl_ioremap_block(pdev, addr, length);
+		if (!regs->status)
+			return -ENOMEM;
 	}
 
 	if (map->device_map.mbox.valid) {
@@ -99,7 +123,9 @@ int cxl_map_device_regs(struct pci_dev *pdev,
 
 		addr = phys_addr + map->device_map.mbox.offset;
 		length = map->device_map.mbox.size;
-		regs->mbox = devm_ioremap(dev, addr, length);
+		regs->mbox = cxl_ioremap_block(pdev, addr, length);
+		if (!regs->mbox)
+			return -ENOMEM;
 	}
 
 	if (map->device_map.memdev.valid) {
@@ -108,7 +134,9 @@ int cxl_map_device_regs(struct pci_dev *pdev,
 
 		addr = phys_addr + map->device_map.memdev.offset;
 		length = map->device_map.memdev.size;
-		regs->memdev = devm_ioremap(dev, addr, length);
+		regs->memdev = cxl_ioremap_block(pdev, addr, length);
+		if (!regs->memdev)
+			return -ENOMEM;
 	}
 
 	return 0;
diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 3ffd5fad74b4..776cb8e28c2d 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -1110,6 +1110,8 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm)
 			goto free_maps;
 	}
 
+	pci_release_mem_regions(pdev);
+
 	list_for_each_entry(map, &register_maps, list) {
 		ret = cxl_map_regs(cxlm, map);
 		if (ret)
@@ -1547,10 +1549,6 @@ static int cxl_mem_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	struct cxl_mem *cxlm;
 	int rc;
 
-	rc = pcim_enable_device(pdev);
-	if (rc)
-		return rc;
-
 	cxlm = cxl_mem_create(pdev);
 	if (IS_ERR(cxlm))
 		return PTR_ERR(cxlm);
-- 
2.28.0.rc0.12.gb6a658bd00c9

Re: [PATCH v2 4/5] cxl/mem: Reserve individual register block regions

[Jonathan Cameron]

On Fri, 21 May 2021 17:11:53 -0700
<ira.weiny@intel.com> wrote:

> From: Ira Weiny <ira.weiny@intel.com>
> 
> Now that individual register blocks are mapped; those blocks regions
> should be reserved individually.
> 
> Remove general pci device management and create managed region
> reservations based on the individual register blocks.

Would be good to include a bit more of the 'reason why' in the actual
patch descriptions.  Afterall, who ever goes looking for the cover letter
after patches are applied ;)

> 
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> 
> ---
> Changes for V2:
> 	New patch
> ---
>  drivers/cxl/core.c | 36 ++++++++++++++++++++++++++++++++----
>  drivers/cxl/pci.c  |  6 ++----
>  2 files changed, 34 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/cxl/core.c b/drivers/cxl/core.c
> index add66a6ec875..ae38f17be1e7 100644
> --- a/drivers/cxl/core.c
> +++ b/drivers/cxl/core.c
> @@ -74,11 +74,33 @@ void cxl_probe_device_regs(struct device *dev, void __iomem *base,
>  }
>  EXPORT_SYMBOL_GPL(cxl_probe_device_regs);
>  
> +static void __iomem *cxl_ioremap_block(struct pci_dev *pdev,
> +				       resource_size_t addr,
> +				       resource_size_t length)
> +{
> +	struct device *dev = &pdev->dev;
> +	void __iomem *ret_val;
> +	struct resource *res;
> +
> +	res = devm_request_mem_region(dev, addr, length, pci_name(pdev));
> +	if (!res) {
> +		dev_err(dev, "Failed to request region %#llx-%#llx\n",
> +			addr, addr+length);
> +		return NULL;
> +	}
> +
> +	ret_val = devm_ioremap(dev, addr, length);
> +	if (!ret_val)
> +		dev_err(dev, "Failed to map region %#llx-%#llx\n",
> +			addr, addr+length);
> +
> +	return ret_val;
> +}
> +
>  int cxl_map_device_regs(struct pci_dev *pdev,
>  			struct cxl_device_regs *regs,
>  			struct cxl_register_map *map)
>  {
> -	struct device *dev = &pdev->dev;
>  	resource_size_t phys_addr;
>  
>  	phys_addr = pci_resource_start(pdev, map->barno);
> @@ -90,7 +112,9 @@ int cxl_map_device_regs(struct pci_dev *pdev,
>  
>  		addr = phys_addr + map->device_map.status.offset;
>  		length = map->device_map.status.size;
> -		regs->status = devm_ioremap(dev, addr, length);
> +		regs->status = cxl_ioremap_block(pdev, addr, length);
> +		if (!regs->status)
> +			return -ENOMEM;
>  	}
>  
>  	if (map->device_map.mbox.valid) {
> @@ -99,7 +123,9 @@ int cxl_map_device_regs(struct pci_dev *pdev,
>  
>  		addr = phys_addr + map->device_map.mbox.offset;
>  		length = map->device_map.mbox.size;
> -		regs->mbox = devm_ioremap(dev, addr, length);
> +		regs->mbox = cxl_ioremap_block(pdev, addr, length);
> +		if (!regs->mbox)
> +			return -ENOMEM;
>  	}
>  
>  	if (map->device_map.memdev.valid) {
> @@ -108,7 +134,9 @@ int cxl_map_device_regs(struct pci_dev *pdev,
>  
>  		addr = phys_addr + map->device_map.memdev.offset;
>  		length = map->device_map.memdev.size;
> -		regs->memdev = devm_ioremap(dev, addr, length);
> +		regs->memdev = cxl_ioremap_block(pdev, addr, length);
> +		if (!regs->memdev)
> +			return -ENOMEM;
>  	}
>  
>  	return 0;
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 3ffd5fad74b4..776cb8e28c2d 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -1110,6 +1110,8 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm)
>  			goto free_maps;
>  	}
>  
> +	pci_release_mem_regions(pdev);
> +
>  	list_for_each_entry(map, &register_maps, list) {
>  		ret = cxl_map_regs(cxlm, map);
>  		if (ret)
> @@ -1547,10 +1549,6 @@ static int cxl_mem_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	struct cxl_mem *cxlm;
>  	int rc;
>  
> -	rc = pcim_enable_device(pdev);
> -	if (rc)
> -		return rc;
> -
>  	cxlm = cxl_mem_create(pdev);
>  	if (IS_ERR(cxlm))
>  		return PTR_ERR(cxlm);

[PATCH v2 5/5] cxl: Add HDM decoder capbilities

[ira.weiny]

From: Ben Widawsky <ben.widawsky@intel.com>

An HDM decoder is defined in the CXL 2.0 specification as a mechanism
that allow devices and upstream ports to claim memory address ranges and
participate in interleave sets. HDM decoder registers are within the
component register block defined in CXL 2.0 8.2.3 CXL 2.0 Component
Registers as part of the CXL.cache and CXL.mem subregion.

The Component Register Block is found via the Register Locator DVSEC
in a similar fashion to how the CXL Device Register Block is found. The
primary difference is the capability id size of the Component Register
Block is a single DWORD instead of 4 DWORDS.

It's now possible to configure a CXL type 3 device's HDM decoder. Such
programming is expected for CXL devices with persistent memory, and hot
plugged CXL devices that participate in CXL.mem with volatile memory.

Co-developed-by: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Ira Weiny <ira.weiny@intel.com>
Co-developed-by: Vishal Verma <vishal.l.verma@intel.com>
Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>

---
Version 1:
Link: https://lore.kernel.org/r/20210407222625.320177-8-ben.widawsky@intel.com
Message-Id: <20210407222625.320177-8-ben.widawsky@intel.com>

Changes for V2:
	Rebased on https://git.kernel.org/pub/scm/linux/kernel/git/cxl/cxl.git/commit/?h=pending
	From Dan
		Remove version checking
		Remove unneeded TODO/FIXME comments
---
 drivers/cxl/core.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++
 drivers/cxl/cxl.h  | 65 +++++++++++++++++++++++++++++++++----
 drivers/cxl/pci.c  | 15 +++++++++
 drivers/cxl/pci.h  |  1 +
 4 files changed, 156 insertions(+), 6 deletions(-)

diff --git a/drivers/cxl/core.c b/drivers/cxl/core.c
index ae38f17be1e7..c0ced872a48d 100644
--- a/drivers/cxl/core.c
+++ b/drivers/cxl/core.c
@@ -13,6 +13,67 @@
  * point for cross-device interleave coordination through cxl ports.
  */
 
+void cxl_probe_component_regs(struct device *dev, void __iomem *base,
+			      struct cxl_component_reg_map *map)
+{
+	int cap, cap_count;
+	u64 cap_array;
+
+	*map = (struct cxl_component_reg_map) { 0 };
+
+	/*
+	 * CXL.cache and CXL.mem registers are at offset 0x1000 as defined in
+	 * CXL 2.0 8.2.4 Table 141.
+	 */
+	base += CXL_CM_OFFSET;
+
+	cap_array = readq(base + CXL_CM_CAP_HDR_OFFSET);
+
+	if (FIELD_GET(CXL_CM_CAP_HDR_ID_MASK, cap_array) != CM_CAP_HDR_CAP_ID) {
+		dev_err(dev,
+			"Couldn't locate the CXL.cache and CXL.mem capability array header./n");
+		return;
+	}
+
+	/* It's assumed that future versions will be backward compatible */
+	cap_count = FIELD_GET(CXL_CM_CAP_HDR_ARRAY_SIZE_MASK, cap_array);
+
+	for (cap = 1; cap <= cap_count; cap++) {
+		void __iomem *register_block;
+		u32 hdr;
+		int decoder_cnt;
+		u16 cap_id, offset;
+		u32 length;
+
+		hdr = readl(base + cap * 0x4);
+
+		cap_id = FIELD_GET(CXL_CM_CAP_HDR_ID_MASK, hdr);
+		offset = FIELD_GET(CXL_CM_CAP_PTR_MASK, hdr);
+		register_block = base + offset;
+
+		switch (cap_id) {
+		case CXL_CM_CAP_CAP_ID_HDM:
+			dev_dbg(dev, "found HDM decoder capability (0x%x)\n",
+				offset);
+
+			hdr = readl(register_block);
+
+			decoder_cnt = FIELD_GET(CXL_HDM_DECODER_COUNT_MASK, hdr);
+			length = 0x20 * decoder_cnt + 0x10;
+
+			map->hdm_decoder.valid = true;
+			map->hdm_decoder.offset = offset;
+			map->hdm_decoder.size = length;
+			break;
+		default:
+			dev_dbg(dev, "Unknown CM cap ID: %d (0x%x)\n", cap_id,
+				offset);
+			break;
+		}
+	}
+}
+EXPORT_SYMBOL_GPL(cxl_probe_component_regs);
+
 void cxl_probe_device_regs(struct device *dev, void __iomem *base,
 			   struct cxl_device_reg_map *map)
 {
@@ -97,6 +158,26 @@ static void __iomem *cxl_ioremap_block(struct pci_dev *pdev,
 	return ret_val;
 }
 
+int cxl_map_component_regs(struct pci_dev *pdev,
+			   struct cxl_component_regs *regs,
+			   struct cxl_register_map *map)
+{
+	resource_size_t phys_addr;
+	resource_size_t length;
+
+	phys_addr = pci_resource_start(pdev, map->barno);
+	phys_addr += map->block_offset;
+
+	phys_addr += map->component_map.hdm_decoder.offset;
+	length = map->component_map.hdm_decoder.size;
+	regs->hdm_decoder = cxl_ioremap_block(pdev, phys_addr, length);
+	if (!regs->hdm_decoder)
+		return -ENOMEM;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(cxl_map_component_regs);
+
 int cxl_map_device_regs(struct pci_dev *pdev,
 			struct cxl_device_regs *regs,
 			struct cxl_register_map *map)
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index ae4b4c96c6b5..2c47e9cffd44 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -8,6 +8,31 @@
 #include <linux/bitops.h>
 #include <linux/io.h>
 
+/* CXL 2.0 8.2.5 CXL.cache and CXL.mem Registers*/
+#define CXL_CM_OFFSET 0x1000
+#define CXL_CM_CAP_HDR_OFFSET 0x0
+#define   CXL_CM_CAP_HDR_ID_MASK GENMASK(15, 0)
+#define     CM_CAP_HDR_CAP_ID 1
+#define   CXL_CM_CAP_HDR_VERSION_MASK GENMASK(19, 16)
+#define     CM_CAP_HDR_CAP_VERSION 1
+#define   CXL_CM_CAP_HDR_CACHE_MEM_VERSION_MASK GENMASK(23, 20)
+#define     CM_CAP_HDR_CACHE_MEM_VERSION 1
+#define   CXL_CM_CAP_HDR_ARRAY_SIZE_MASK GENMASK(31, 24)
+#define CXL_CM_CAP_PTR_MASK GENMASK(31, 20)
+
+#define   CXL_CM_CAP_CAP_ID_HDM 0x5
+#define   CXL_CM_CAP_CAP_HDM_VERSION 1
+
+/* HDM decoders CXL 2.0 8.2.5.12 CXL HDM Decoder Capability Structure */
+#define CXL_HDM_DECODER_CAP_OFFSET 0x0
+#define   CXL_HDM_DECODER_COUNT_MASK GENMASK(3, 0)
+#define   CXL_HDM_DECODER_TARGET_COUNT_MASK GENMASK(7, 4)
+#define CXL_HDM_DECODER0_BASE_LOW_OFFSET 0x10
+#define CXL_HDM_DECODER0_BASE_HIGH_OFFSET 0x14
+#define CXL_HDM_DECODER0_SIZE_LOW_OFFSET 0x18
+#define CXL_HDM_DECODER0_SIZE_HIGH_OFFSET 0x1c
+#define CXL_HDM_DECODER0_CTRL_OFFSET 0x20
+
 /* CXL 2.0 8.2.8.1 Device Capabilities Array Register */
 #define CXLDEV_CAP_ARRAY_OFFSET 0x0
 #define   CXLDEV_CAP_ARRAY_CAP_ID 0
@@ -34,18 +59,30 @@
 #define CXLDEV_MBOX_BG_CMD_STATUS_OFFSET 0x18
 #define CXLDEV_MBOX_PAYLOAD_OFFSET 0x20
 
-/*
- * CXL_DEVICE_REGS - Common set of CXL Device register block base pointers
- * @status: CXL 2.0 8.2.8.3 Device Status Registers
- * @mbox: CXL 2.0 8.2.8.4 Mailbox Registers
- * @memdev: CXL 2.0 8.2.8.5 Memory Device Registers
- */
+#define CXL_COMPONENT_REGS() \
+	void __iomem *hdm_decoder
+
 #define CXL_DEVICE_REGS() \
 	void __iomem *status; \
 	void __iomem *mbox; \
 	void __iomem *memdev
 
 /* See note for 'struct cxl_regs' for the rationale of this organization */
+/*
+ * CXL_COMPONENT_REGS - Common set of CXL Component register block base pointers
+ * @hdm_decoder: CXL 2.0 8.2.5.12 CXL HDM Decoder Capability Structure
+ */
+struct cxl_component_regs {
+	CXL_COMPONENT_REGS();
+};
+
+/* See note for 'struct cxl_regs' for the rationale of this organization */
+/*
+ * CXL_DEVICE_REGS - Common set of CXL Device register block base pointers
+ * @status: CXL 2.0 8.2.8.3 Device Status Registers
+ * @mbox: CXL 2.0 8.2.8.4 Mailbox Registers
+ * @memdev: CXL 2.0 8.2.8.5 Memory Device Registers
+ */
 struct cxl_device_regs {
 	CXL_DEVICE_REGS();
 };
@@ -56,6 +93,12 @@ struct cxl_device_regs {
  * agnostic code to include the prefix.
  */
 struct cxl_regs {
+	union {
+		struct {
+			CXL_COMPONENT_REGS();
+		};
+		struct cxl_component_regs component;
+	};
 	union {
 		struct {
 			CXL_DEVICE_REGS();
@@ -70,6 +113,10 @@ struct cxl_reg_map {
 	unsigned long size;
 };
 
+struct cxl_component_reg_map {
+	struct cxl_reg_map hdm_decoder;
+};
+
 struct cxl_device_reg_map {
 	struct cxl_reg_map status;
 	struct cxl_reg_map mbox;
@@ -82,12 +129,18 @@ struct cxl_register_map {
 	u8 reg_type;
 	u8 barno;
 	union {
+		struct cxl_component_reg_map component_map;
 		struct cxl_device_reg_map device_map;
 	};
 };
 
+void cxl_probe_component_regs(struct device *dev, void __iomem *base,
+			      struct cxl_component_reg_map *map);
 void cxl_probe_device_regs(struct device *dev, void __iomem *base,
 			   struct cxl_device_reg_map *map);
+int cxl_map_component_regs(struct pci_dev *pdev,
+			   struct cxl_component_regs *regs,
+			   struct cxl_register_map *map);
 int cxl_map_device_regs(struct pci_dev *pdev,
 			struct cxl_device_regs *regs,
 			struct cxl_register_map *map);
diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 776cb8e28c2d..bf16328c6992 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -982,9 +982,20 @@ static int cxl_probe_regs(struct cxl_mem *cxlm, void __iomem *base,
 {
 	struct pci_dev *pdev = cxlm->pdev;
 	struct device *dev = &pdev->dev;
+	struct cxl_component_reg_map *comp_map;
 	struct cxl_device_reg_map *dev_map;
 
 	switch (map->reg_type) {
+	case CXL_REGLOC_RBI_COMPONENT:
+		comp_map = &map->component_map;
+		cxl_probe_component_regs(dev, base, comp_map);
+		if (!comp_map->hdm_decoder.valid) {
+			dev_err(dev, "HDM decoder registers not found\n");
+			return -ENXIO;
+		}
+
+		dev_dbg(dev, "Set up component registers\n");
+		break;
 	case CXL_REGLOC_RBI_MEMDEV:
 		dev_map = &map->device_map;
 		cxl_probe_device_regs(dev, base, dev_map);
@@ -1012,6 +1023,10 @@ static int cxl_map_regs(struct cxl_mem *cxlm, struct cxl_register_map *map)
 	struct device *dev = &pdev->dev;
 
 	switch (map->reg_type) {
+	case CXL_REGLOC_RBI_COMPONENT:
+		cxl_map_component_regs(pdev, &cxlm->regs.component, map);
+		dev_dbg(dev, "Mapping component registers...\n");
+		break;
 	case CXL_REGLOC_RBI_MEMDEV:
 		cxl_map_device_regs(pdev, &cxlm->regs.device_regs, map);
 		dev_dbg(dev, "Probing device registers...\n");
diff --git a/drivers/cxl/pci.h b/drivers/cxl/pci.h
index af3ec078cf6c..8b8c6afbe605 100644
--- a/drivers/cxl/pci.h
+++ b/drivers/cxl/pci.h
@@ -25,6 +25,7 @@
 #define CXL_REGLOC_RBI_COMPONENT 1
 #define CXL_REGLOC_RBI_VIRT 2
 #define CXL_REGLOC_RBI_MEMDEV 3
+#define CXL_REGLOC_RBI_MAX CXL_REGLOC_RBI_MEMDEV
 
 #define CXL_REGLOC_ADDR_MASK GENMASK(31, 16)
 
-- 
2.28.0.rc0.12.gb6a658bd00c9

Re: [PATCH v2 5/5] cxl: Add HDM decoder capbilities

[Jonathan Cameron]

On Fri, 21 May 2021 17:11:54 -0700
<ira.weiny@intel.com> wrote:

> From: Ben Widawsky <ben.widawsky@intel.com>
> 
> An HDM decoder is defined in the CXL 2.0 specification as a mechanism
> that allow devices and upstream ports to claim memory address ranges and
> participate in interleave sets. HDM decoder registers are within the
> component register block defined in CXL 2.0 8.2.3 CXL 2.0 Component
> Registers as part of the CXL.cache and CXL.mem subregion.
> 
> The Component Register Block is found via the Register Locator DVSEC
> in a similar fashion to how the CXL Device Register Block is found. The
> primary difference is the capability id size of the Component Register
> Block is a single DWORD instead of 4 DWORDS.
> 
> It's now possible to configure a CXL type 3 device's HDM decoder. Such
> programming is expected for CXL devices with persistent memory, and hot
> plugged CXL devices that participate in CXL.mem with volatile memory.
> 
> Co-developed-by: Ira Weiny <ira.weiny@intel.com>
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> Co-developed-by: Vishal Verma <vishal.l.verma@intel.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>

Totally trivial comments inline. Otherwise LGTM

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> 
> ---
> Version 1:
> Link: https://lore.kernel.org/r/20210407222625.320177-8-ben.widawsky@intel.com
> Message-Id: <20210407222625.320177-8-ben.widawsky@intel.com>
> 
> Changes for V2:
> 	Rebased on https://git.kernel.org/pub/scm/linux/kernel/git/cxl/cxl.git/commit/?h=pending
> 	From Dan
> 		Remove version checking
> 		Remove unneeded TODO/FIXME comments
> ---
>  drivers/cxl/core.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/cxl/cxl.h  | 65 +++++++++++++++++++++++++++++++++----
>  drivers/cxl/pci.c  | 15 +++++++++
>  drivers/cxl/pci.h  |  1 +
>  4 files changed, 156 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/cxl/core.c b/drivers/cxl/core.c
> index ae38f17be1e7..c0ced872a48d 100644
> --- a/drivers/cxl/core.c
> +++ b/drivers/cxl/core.c
> @@ -13,6 +13,67 @@
>   * point for cross-device interleave coordination through cxl ports.
>   */
>  
> +void cxl_probe_component_regs(struct device *dev, void __iomem *base,
> +			      struct cxl_component_reg_map *map)
> +{
> +	int cap, cap_count;
> +	u64 cap_array;
> +
> +	*map = (struct cxl_component_reg_map) { 0 };
> +
> +	/*
> +	 * CXL.cache and CXL.mem registers are at offset 0x1000 as defined in
> +	 * CXL 2.0 8.2.4 Table 141.

Perhaps makes more sense to have this next to the define?

> +	 */
> +	base += CXL_CM_OFFSET;
> +
> +	cap_array = readq(base + CXL_CM_CAP_HDR_OFFSET);
> +
> +	if (FIELD_GET(CXL_CM_CAP_HDR_ID_MASK, cap_array) != CM_CAP_HDR_CAP_ID) {
> +		dev_err(dev,
> +			"Couldn't locate the CXL.cache and CXL.mem capability array header./n");
> +		return;
> +	}
> +
> +	/* It's assumed that future versions will be backward compatible */
> +	cap_count = FIELD_GET(CXL_CM_CAP_HDR_ARRAY_SIZE_MASK, cap_array);
> +
> +	for (cap = 1; cap <= cap_count; cap++) {
> +		void __iomem *register_block;
> +		u32 hdr;
> +		int decoder_cnt;
> +		u16 cap_id, offset;
> +		u32 length;
> +
> +		hdr = readl(base + cap * 0x4);
> +
> +		cap_id = FIELD_GET(CXL_CM_CAP_HDR_ID_MASK, hdr);
> +		offset = FIELD_GET(CXL_CM_CAP_PTR_MASK, hdr);
> +		register_block = base + offset;
> +
> +		switch (cap_id) {
> +		case CXL_CM_CAP_CAP_ID_HDM:
> +			dev_dbg(dev, "found HDM decoder capability (0x%x)\n",
> +				offset);
> +
> +			hdr = readl(register_block);
> +
> +			decoder_cnt = FIELD_GET(CXL_HDM_DECODER_COUNT_MASK, hdr);
> +			length = 0x20 * decoder_cnt + 0x10;
> +
> +			map->hdm_decoder.valid = true;
> +			map->hdm_decoder.offset = offset;
> +			map->hdm_decoder.size = length;
> +			break;
> +		default:
> +			dev_dbg(dev, "Unknown CM cap ID: %d (0x%x)\n", cap_id,
> +				offset);
> +			break;
> +		}
> +	}
> +}
> +EXPORT_SYMBOL_GPL(cxl_probe_component_regs);
> +
>  void cxl_probe_device_regs(struct device *dev, void __iomem *base,
>  			   struct cxl_device_reg_map *map)
>  {
> @@ -97,6 +158,26 @@ static void __iomem *cxl_ioremap_block(struct pci_dev *pdev,
>  	return ret_val;
>  }
>  
> +int cxl_map_component_regs(struct pci_dev *pdev,
> +			   struct cxl_component_regs *regs,
> +			   struct cxl_register_map *map)
> +{
> +	resource_size_t phys_addr;
> +	resource_size_t length;
> +
> +	phys_addr = pci_resource_start(pdev, map->barno);
> +	phys_addr += map->block_offset;
> +
> +	phys_addr += map->component_map.hdm_decoder.offset;
> +	length = map->component_map.hdm_decoder.size;
> +	regs->hdm_decoder = cxl_ioremap_block(pdev, phys_addr, length);
> +	if (!regs->hdm_decoder)
> +		return -ENOMEM;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(cxl_map_component_regs);
> +
>  int cxl_map_device_regs(struct pci_dev *pdev,
>  			struct cxl_device_regs *regs,
>  			struct cxl_register_map *map)
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index ae4b4c96c6b5..2c47e9cffd44 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -8,6 +8,31 @@
>  #include <linux/bitops.h>
>  #include <linux/io.h>
>  
> +/* CXL 2.0 8.2.5 CXL.cache and CXL.mem Registers*/
> +#define CXL_CM_OFFSET 0x1000
> +#define CXL_CM_CAP_HDR_OFFSET 0x0
> +#define   CXL_CM_CAP_HDR_ID_MASK GENMASK(15, 0)
> +#define     CM_CAP_HDR_CAP_ID 1
> +#define   CXL_CM_CAP_HDR_VERSION_MASK GENMASK(19, 16)
> +#define     CM_CAP_HDR_CAP_VERSION 1
> +#define   CXL_CM_CAP_HDR_CACHE_MEM_VERSION_MASK GENMASK(23, 20)
> +#define     CM_CAP_HDR_CACHE_MEM_VERSION 1
> +#define   CXL_CM_CAP_HDR_ARRAY_SIZE_MASK GENMASK(31, 24)
> +#define CXL_CM_CAP_PTR_MASK GENMASK(31, 20)
> +
> +#define   CXL_CM_CAP_CAP_ID_HDM 0x5
> +#define   CXL_CM_CAP_CAP_HDM_VERSION 1
> +
> +/* HDM decoders CXL 2.0 8.2.5.12 CXL HDM Decoder Capability Structure */
> +#define CXL_HDM_DECODER_CAP_OFFSET 0x0
> +#define   CXL_HDM_DECODER_COUNT_MASK GENMASK(3, 0)
> +#define   CXL_HDM_DECODER_TARGET_COUNT_MASK GENMASK(7, 4)
> +#define CXL_HDM_DECODER0_BASE_LOW_OFFSET 0x10
> +#define CXL_HDM_DECODER0_BASE_HIGH_OFFSET 0x14
> +#define CXL_HDM_DECODER0_SIZE_LOW_OFFSET 0x18
> +#define CXL_HDM_DECODER0_SIZE_HIGH_OFFSET 0x1c
> +#define CXL_HDM_DECODER0_CTRL_OFFSET 0x20
> +
>  /* CXL 2.0 8.2.8.1 Device Capabilities Array Register */
>  #define CXLDEV_CAP_ARRAY_OFFSET 0x0
>  #define   CXLDEV_CAP_ARRAY_CAP_ID 0
> @@ -34,18 +59,30 @@
>  #define CXLDEV_MBOX_BG_CMD_STATUS_OFFSET 0x18
>  #define CXLDEV_MBOX_PAYLOAD_OFFSET 0x20
>  
> -/*
> - * CXL_DEVICE_REGS - Common set of CXL Device register block base pointers
> - * @status: CXL 2.0 8.2.8.3 Device Status Registers
> - * @mbox: CXL 2.0 8.2.8.4 Mailbox Registers
> - * @memdev: CXL 2.0 8.2.8.5 Memory Device Registers
> - */
> +#define CXL_COMPONENT_REGS() \
> +	void __iomem *hdm_decoder
> +
>  #define CXL_DEVICE_REGS() \
>  	void __iomem *status; \
>  	void __iomem *mbox; \
>  	void __iomem *memdev
>  
>  /* See note for 'struct cxl_regs' for the rationale of this organization */
> +/*
> + * CXL_COMPONENT_REGS - Common set of CXL Component register block base pointers
> + * @hdm_decoder: CXL 2.0 8.2.5.12 CXL HDM Decoder Capability Structure
> + */
> +struct cxl_component_regs {
> +	CXL_COMPONENT_REGS();
> +};
> +
> +/* See note for 'struct cxl_regs' for the rationale of this organization */
> +/*
> + * CXL_DEVICE_REGS - Common set of CXL Device register block base pointers
> + * @status: CXL 2.0 8.2.8.3 Device Status Registers
> + * @mbox: CXL 2.0 8.2.8.4 Mailbox Registers
> + * @memdev: CXL 2.0 8.2.8.5 Memory Device Registers
> + */
>  struct cxl_device_regs {
>  	CXL_DEVICE_REGS();
>  };
> @@ -56,6 +93,12 @@ struct cxl_device_regs {
>   * agnostic code to include the prefix.
>   */
>  struct cxl_regs {
> +	union {
> +		struct {
> +			CXL_COMPONENT_REGS();
> +		};
> +		struct cxl_component_regs component;
> +	};
>  	union {
>  		struct {
>  			CXL_DEVICE_REGS();
> @@ -70,6 +113,10 @@ struct cxl_reg_map {
>  	unsigned long size;
>  };
>  
> +struct cxl_component_reg_map {
> +	struct cxl_reg_map hdm_decoder;
> +};
> +
>  struct cxl_device_reg_map {
>  	struct cxl_reg_map status;
>  	struct cxl_reg_map mbox;
> @@ -82,12 +129,18 @@ struct cxl_register_map {
>  	u8 reg_type;
>  	u8 barno;
>  	union {
> +		struct cxl_component_reg_map component_map;
>  		struct cxl_device_reg_map device_map;
>  	};
>  };
>  
> +void cxl_probe_component_regs(struct device *dev, void __iomem *base,
> +			      struct cxl_component_reg_map *map);
>  void cxl_probe_device_regs(struct device *dev, void __iomem *base,
>  			   struct cxl_device_reg_map *map);
> +int cxl_map_component_regs(struct pci_dev *pdev,
> +			   struct cxl_component_regs *regs,
> +			   struct cxl_register_map *map);
>  int cxl_map_device_regs(struct pci_dev *pdev,
>  			struct cxl_device_regs *regs,
>  			struct cxl_register_map *map);
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 776cb8e28c2d..bf16328c6992 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -982,9 +982,20 @@ static int cxl_probe_regs(struct cxl_mem *cxlm, void __iomem *base,
>  {
>  	struct pci_dev *pdev = cxlm->pdev;
>  	struct device *dev = &pdev->dev;
> +	struct cxl_component_reg_map *comp_map;
>  	struct cxl_device_reg_map *dev_map;
>  
>  	switch (map->reg_type) {
> +	case CXL_REGLOC_RBI_COMPONENT:
> +		comp_map = &map->component_map;
> +		cxl_probe_component_regs(dev, base, comp_map);
> +		if (!comp_map->hdm_decoder.valid) {
> +			dev_err(dev, "HDM decoder registers not found\n");
> +			return -ENXIO;
> +		}
> +
> +		dev_dbg(dev, "Set up component registers\n");
> +		break;
>  	case CXL_REGLOC_RBI_MEMDEV:
>  		dev_map = &map->device_map;
>  		cxl_probe_device_regs(dev, base, dev_map);
> @@ -1012,6 +1023,10 @@ static int cxl_map_regs(struct cxl_mem *cxlm, struct cxl_register_map *map)
>  	struct device *dev = &pdev->dev;
>  
>  	switch (map->reg_type) {
> +	case CXL_REGLOC_RBI_COMPONENT:
> +		cxl_map_component_regs(pdev, &cxlm->regs.component, map);
> +		dev_dbg(dev, "Mapping component registers...\n");
> +		break;
>  	case CXL_REGLOC_RBI_MEMDEV:
>  		cxl_map_device_regs(pdev, &cxlm->regs.device_regs, map);
>  		dev_dbg(dev, "Probing device registers...\n");
> diff --git a/drivers/cxl/pci.h b/drivers/cxl/pci.h
> index af3ec078cf6c..8b8c6afbe605 100644
> --- a/drivers/cxl/pci.h
> +++ b/drivers/cxl/pci.h
> @@ -25,6 +25,7 @@
>  #define CXL_REGLOC_RBI_COMPONENT 1
>  #define CXL_REGLOC_RBI_VIRT 2
>  #define CXL_REGLOC_RBI_MEMDEV 3
> +#define CXL_REGLOC_RBI_MAX CXL_REGLOC_RBI_MEMDEV

This doesn't seem to be used.

>  
>  #define CXL_REGLOC_ADDR_MASK GENMASK(31, 16)
>