[PATCH 0/7] Enumerate HDM Decoder registers

[PATCH 0/7] Enumerate HDM Decoder registers

[Ben Widawsky]

The arbiter of a Host-managed Device Memory (HDM) region is an "HDM Decoder".
An HDM decoder is defined in the Compute eXpress Link (CXL) specification as
hardware that handles the routing of memory ranges controlled by CXL devices in
a linear, or interleaved fashion.

HDM decoders are not specific to CXL devices and therefore live in a different
part of the MMIO space for CXL *components*. The CXL component registers are
found and mapped in a similar way to the device registers and so a lot of this
series is refactoring to try to easily add the new register block type.

Management of HDM decoders is not handled in this series. This work is the
foundation for the eventual integration with libnvdimm. That integration will
enable CXL persistent and volatile capacities to be managed by Linux and gain
use of all the related existing infrastructure.

These patches go on top of the reworks and improvements recently submitted by
Dan [1].

[1]: https://lore.kernel.org/linux-cxl/161728744224.2474040.12854720917440712854.stgit@dwillia2-desk3.amr.corp.intel.com/T/#t

Ben Widawsky (7):
  cxl/mem: Use dev instead of pdev->dev
  cxl/mem: Split creation from mapping in probe
  cxl/mem: Move register locator logic into reg setup
  cxl/mem: Get rid of @cxlm.base
  cxl/mem: Move device register setup
  cxl/mem: Create a helper to setup device regs
  cxl: Add HDM decoder capbilities

 drivers/cxl/core.c |  73 +++++++++++++++
 drivers/cxl/cxl.h  |  48 ++++++++++
 drivers/cxl/mem.c  | 225 +++++++++++++++++++++++++++------------------
 drivers/cxl/mem.h  |   2 -
 drivers/cxl/pci.h  |   1 +
 5 files changed, 260 insertions(+), 89 deletions(-)


base-commit: 24ca2d6ff18cc59c14c0aff65025b0cc11a72722
-- 
2.31.1

[PATCH 1/7] cxl/mem: Use dev instead of pdev->dev

[Ben Widawsky]

Trivial cleanup.

Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
---
 drivers/cxl/mem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
index b6fe4e81d38a..99534260034e 100644
--- a/drivers/cxl/mem.c
+++ b/drivers/cxl/mem.c
@@ -935,7 +935,7 @@ static struct cxl_mem *cxl_mem_create(struct pci_dev *pdev, u32 reg_lo,
 	u8 bar;
 	int rc;
 
-	cxlm = devm_kzalloc(&pdev->dev, sizeof(*cxlm), GFP_KERNEL);
+	cxlm = devm_kzalloc(dev, sizeof(*cxlm), GFP_KERNEL);
 	if (!cxlm) {
 		dev_err(dev, "No memory available\n");
 		return NULL;
-- 
2.31.1

Re: [PATCH 1/7] cxl/mem: Use dev instead of pdev->dev

[Jonathan Cameron]

On Wed, 7 Apr 2021 15:26:19 -0700
Ben Widawsky <ben.widawsky@intel.com> wrote:

> Trivial cleanup.

Obviously correct :)

> 
> Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
FWIW
Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/cxl/mem.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index b6fe4e81d38a..99534260034e 100644
> --- a/drivers/cxl/mem.c
> +++ b/drivers/cxl/mem.c
> @@ -935,7 +935,7 @@ static struct cxl_mem *cxl_mem_create(struct pci_dev *pdev, u32 reg_lo,
>  	u8 bar;
>  	int rc;
>  
> -	cxlm = devm_kzalloc(&pdev->dev, sizeof(*cxlm), GFP_KERNEL);
> +	cxlm = devm_kzalloc(dev, sizeof(*cxlm), GFP_KERNEL);
>  	if (!cxlm) {
>  		dev_err(dev, "No memory available\n");
>  		return NULL;

[PATCH 2/7] cxl/mem: Split creation from mapping in probe

[Ben Widawsky]

Add a new function specifically for mapping the register blocks and
offsets within. The new function can be used more generically for other
register block identifiers.

No functional change is meant to be introduced in this patch with the
exception of a dev_err printed when the device register block isn't
found.

Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
---
 drivers/cxl/mem.c | 64 +++++++++++++++++++++++++++++------------------
 1 file changed, 40 insertions(+), 24 deletions(-)

diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
index 99534260034e..520edaf233d4 100644
--- a/drivers/cxl/mem.c
+++ b/drivers/cxl/mem.c
@@ -925,22 +925,40 @@ static int cxl_mem_setup_mailbox(struct cxl_mem *cxlm)
 	return 0;
 }
 
-static struct cxl_mem *cxl_mem_create(struct pci_dev *pdev, u32 reg_lo,
-				      u32 reg_hi)
+static struct cxl_mem *cxl_mem_create(struct pci_dev *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct cxl_mem *cxlm;
-	void __iomem *regs;
-	u64 offset;
-	u8 bar;
-	int rc;
 
 	cxlm = devm_kzalloc(dev, sizeof(*cxlm), GFP_KERNEL);
 	if (!cxlm) {
 		dev_err(dev, "No memory available\n");
-		return NULL;
+		return ERR_PTR(-ENOMEM);
+	}
+
+	mutex_init(&cxlm->mbox_mutex);
+	cxlm->pdev = pdev;
+	cxlm->enabled_cmds =
+		devm_kmalloc_array(dev, BITS_TO_LONGS(cxl_cmd_count),
+				   sizeof(unsigned long),
+				   GFP_KERNEL | __GFP_ZERO);
+	if (!cxlm->enabled_cmds) {
+		dev_err(dev, "No memory available for bitmap\n");
+		return ERR_PTR(-ENOMEM);
 	}
 
+	return cxlm;
+}
+
+static int cxl_mem_map_regblock(struct cxl_mem *cxlm, u32 reg_lo, u32 reg_hi)
+{
+	struct pci_dev *pdev = cxlm->pdev;
+	struct device *dev = &pdev->dev;
+	void __iomem *regs;
+	u64 offset;
+	u8 bar;
+	int rc;
+
 	offset = ((u64)reg_hi << 32) | FIELD_GET(CXL_REGLOC_ADDR_MASK, reg_lo);
 	bar = FIELD_GET(CXL_REGLOC_BIR_MASK, reg_lo);
 
@@ -948,30 +966,20 @@ static struct cxl_mem *cxl_mem_create(struct pci_dev *pdev, u32 reg_lo,
 	if (pci_resource_len(pdev, bar) < offset) {
 		dev_err(dev, "BAR%d: %pr: too small (offset: %#llx)\n", bar,
 			&pdev->resource[bar], (unsigned long long)offset);
-		return NULL;
+		return -ENXIO;
 	}
 
 	rc = pcim_iomap_regions(pdev, BIT(bar), pci_name(pdev));
 	if (rc) {
 		dev_err(dev, "failed to map registers\n");
-		return NULL;
+		return rc;
 	}
 	regs = pcim_iomap_table(pdev)[bar];
 
-	mutex_init(&cxlm->mbox_mutex);
-	cxlm->pdev = pdev;
 	cxlm->base = regs + offset;
-	cxlm->enabled_cmds =
-		devm_kmalloc_array(dev, BITS_TO_LONGS(cxl_cmd_count),
-				   sizeof(unsigned long),
-				   GFP_KERNEL | __GFP_ZERO);
-	if (!cxlm->enabled_cmds) {
-		dev_err(dev, "No memory available for bitmap\n");
-		return NULL;
-	}
 
 	dev_dbg(dev, "Mapped CXL Memory Device resource\n");
-	return cxlm;
+	return 0;
 }
 
 static int cxl_mem_dvsec(struct pci_dev *pdev, int dvsec)
@@ -1403,14 +1411,18 @@ static int cxl_mem_identify(struct cxl_mem *cxlm)
 static int cxl_mem_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 {
 	struct device *dev = &pdev->dev;
-	struct cxl_mem *cxlm = NULL;
 	u32 regloc_size, regblocks;
+	struct cxl_mem *cxlm;
 	int rc, regloc, i;
 
 	rc = pcim_enable_device(pdev);
 	if (rc)
 		return rc;
 
+	cxlm = cxl_mem_create(pdev);
+	if (IS_ERR(cxlm))
+		return PTR_ERR(cxlm);
+
 	regloc = cxl_mem_dvsec(pdev, PCI_DVSEC_ID_CXL_REGLOC_OFFSET);
 	if (!regloc) {
 		dev_err(dev, "register location dvsec not found\n");
@@ -1435,13 +1447,17 @@ static int cxl_mem_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 		reg_type = FIELD_GET(CXL_REGLOC_RBI_MASK, reg_lo);
 
 		if (reg_type == CXL_REGLOC_RBI_MEMDEV) {
-			cxlm = cxl_mem_create(pdev, reg_lo, reg_hi);
+			rc = cxl_mem_map_regblock(cxlm, reg_lo, reg_hi);
+			if (rc)
+				return rc;
 			break;
 		}
 	}
 
-	if (!cxlm)
-		return -ENODEV;
+	if (i == regblocks) {
+		dev_err(dev, "Missing register locator for device registers\n");
+		return -ENXIO;
+	}
 
 	rc = cxl_mem_setup_regs(cxlm);
 	if (rc)
-- 
2.31.1

Re: [PATCH 2/7] cxl/mem: Split creation from mapping in probe

[Jonathan Cameron]

On Wed, 7 Apr 2021 15:26:20 -0700
Ben Widawsky <ben.widawsky@intel.com> wrote:

> Add a new function specifically for mapping the register blocks and
> offsets within. The new function can be used more generically for other
> register block identifiers.
> 
> No functional change is meant to be introduced in this patch with the
> exception of a dev_err printed when the device register block isn't
> found.
> 
> Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
Agreed, this seems to be a noop refactor to me.

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

> ---
>  drivers/cxl/mem.c | 64 +++++++++++++++++++++++++++++------------------
>  1 file changed, 40 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index 99534260034e..520edaf233d4 100644
> --- a/drivers/cxl/mem.c
> +++ b/drivers/cxl/mem.c
> @@ -925,22 +925,40 @@ static int cxl_mem_setup_mailbox(struct cxl_mem *cxlm)
>  	return 0;
>  }
>  
> -static struct cxl_mem *cxl_mem_create(struct pci_dev *pdev, u32 reg_lo,
> -				      u32 reg_hi)
> +static struct cxl_mem *cxl_mem_create(struct pci_dev *pdev)
>  {
>  	struct device *dev = &pdev->dev;
>  	struct cxl_mem *cxlm;
> -	void __iomem *regs;
> -	u64 offset;
> -	u8 bar;
> -	int rc;
>  
>  	cxlm = devm_kzalloc(dev, sizeof(*cxlm), GFP_KERNEL);
>  	if (!cxlm) {
>  		dev_err(dev, "No memory available\n");
> -		return NULL;
> +		return ERR_PTR(-ENOMEM);
> +	}
> +
> +	mutex_init(&cxlm->mbox_mutex);
> +	cxlm->pdev = pdev;
> +	cxlm->enabled_cmds =
> +		devm_kmalloc_array(dev, BITS_TO_LONGS(cxl_cmd_count),
> +				   sizeof(unsigned long),
> +				   GFP_KERNEL | __GFP_ZERO);
> +	if (!cxlm->enabled_cmds) {
> +		dev_err(dev, "No memory available for bitmap\n");
> +		return ERR_PTR(-ENOMEM);
>  	}
>  
> +	return cxlm;
> +}
> +
> +static int cxl_mem_map_regblock(struct cxl_mem *cxlm, u32 reg_lo, u32 reg_hi)
> +{
> +	struct pci_dev *pdev = cxlm->pdev;
> +	struct device *dev = &pdev->dev;
> +	void __iomem *regs;
> +	u64 offset;
> +	u8 bar;
> +	int rc;
> +
>  	offset = ((u64)reg_hi << 32) | FIELD_GET(CXL_REGLOC_ADDR_MASK, reg_lo);
>  	bar = FIELD_GET(CXL_REGLOC_BIR_MASK, reg_lo);
>  
> @@ -948,30 +966,20 @@ static struct cxl_mem *cxl_mem_create(struct pci_dev *pdev, u32 reg_lo,
>  	if (pci_resource_len(pdev, bar) < offset) {
>  		dev_err(dev, "BAR%d: %pr: too small (offset: %#llx)\n", bar,
>  			&pdev->resource[bar], (unsigned long long)offset);
> -		return NULL;
> +		return -ENXIO;
>  	}
>  
>  	rc = pcim_iomap_regions(pdev, BIT(bar), pci_name(pdev));
>  	if (rc) {
>  		dev_err(dev, "failed to map registers\n");
> -		return NULL;
> +		return rc;
>  	}
>  	regs = pcim_iomap_table(pdev)[bar];
>  
> -	mutex_init(&cxlm->mbox_mutex);
> -	cxlm->pdev = pdev;
>  	cxlm->base = regs + offset;
> -	cxlm->enabled_cmds =
> -		devm_kmalloc_array(dev, BITS_TO_LONGS(cxl_cmd_count),
> -				   sizeof(unsigned long),
> -				   GFP_KERNEL | __GFP_ZERO);
> -	if (!cxlm->enabled_cmds) {
> -		dev_err(dev, "No memory available for bitmap\n");
> -		return NULL;
> -	}
>  
>  	dev_dbg(dev, "Mapped CXL Memory Device resource\n");
> -	return cxlm;
> +	return 0;
>  }
>  
>  static int cxl_mem_dvsec(struct pci_dev *pdev, int dvsec)
> @@ -1403,14 +1411,18 @@ static int cxl_mem_identify(struct cxl_mem *cxlm)
>  static int cxl_mem_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  {
>  	struct device *dev = &pdev->dev;
> -	struct cxl_mem *cxlm = NULL;
>  	u32 regloc_size, regblocks;
> +	struct cxl_mem *cxlm;
>  	int rc, regloc, i;
>  
>  	rc = pcim_enable_device(pdev);
>  	if (rc)
>  		return rc;
>  
> +	cxlm = cxl_mem_create(pdev);
> +	if (IS_ERR(cxlm))
> +		return PTR_ERR(cxlm);
> +
>  	regloc = cxl_mem_dvsec(pdev, PCI_DVSEC_ID_CXL_REGLOC_OFFSET);
>  	if (!regloc) {
>  		dev_err(dev, "register location dvsec not found\n");
> @@ -1435,13 +1447,17 @@ static int cxl_mem_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  		reg_type = FIELD_GET(CXL_REGLOC_RBI_MASK, reg_lo);
>  
>  		if (reg_type == CXL_REGLOC_RBI_MEMDEV) {
> -			cxlm = cxl_mem_create(pdev, reg_lo, reg_hi);
> +			rc = cxl_mem_map_regblock(cxlm, reg_lo, reg_hi);
> +			if (rc)
> +				return rc;
>  			break;
>  		}
>  	}
>  
> -	if (!cxlm)
> -		return -ENODEV;
> +	if (i == regblocks) {
> +		dev_err(dev, "Missing register locator for device registers\n");
> +		return -ENXIO;
> +	}
>  
>  	rc = cxl_mem_setup_regs(cxlm);
>  	if (rc)

[PATCH 3/7] cxl/mem: Move register locator logic into reg setup

[Ben Widawsky]

Start moving code around to ultimately get rid of @cxlm.base. The
@cxlm.base member serves no purpose other than intermediate storage of
the offset found in cxl_mem_map_regblock() later used by
cxl_mem_setup_regs(). Aside from wanting to get rid of this useless
member, it will help later when adding new register block identifiers.

While @cxlm.base still exists, it will become trivial to remove it in a
future patch.

No functional change is meant to be introduced in this patch.

Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
---
 drivers/cxl/mem.c | 135 +++++++++++++++++++++++-----------------------
 1 file changed, 68 insertions(+), 67 deletions(-)

diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
index 520edaf233d4..04b4f7445083 100644
--- a/drivers/cxl/mem.c
+++ b/drivers/cxl/mem.c
@@ -870,34 +870,6 @@ static int cxl_mem_mbox_send_cmd(struct cxl_mem *cxlm, u16 opcode,
 	return 0;
 }
 
-/**
- * cxl_mem_setup_regs() - Setup necessary MMIO.
- * @cxlm: The CXL memory device to communicate with.
- *
- * Return: 0 if all necessary registers mapped.
- *
- * A memory device is required by spec to implement a certain set of MMIO
- * regions. The purpose of this function is to enumerate and map those
- * registers.
- */
-static int cxl_mem_setup_regs(struct cxl_mem *cxlm)
-{
-	struct device *dev = &cxlm->pdev->dev;
-	struct cxl_regs *regs = &cxlm->regs;
-
-	cxl_setup_device_regs(dev, cxlm->base, &regs->device_regs);
-
-	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;
-	}
-
-	return 0;
-}
-
 static int cxl_mem_setup_mailbox(struct cxl_mem *cxlm)
 {
 	const int cap = readl(cxlm->regs.mbox + CXLDEV_MBOX_CAPS_OFFSET);
@@ -1005,6 +977,73 @@ static int cxl_mem_dvsec(struct pci_dev *pdev, int dvsec)
 	return 0;
 }
 
+/**
+ * cxl_mem_setup_regs() - Setup necessary MMIO.
+ * @cxlm: The CXL memory device to communicate with.
+ *
+ * Return: 0 if all necessary registers mapped.
+ *
+ * A memory device is required by spec to implement a certain set of MMIO
+ * regions. The purpose of this function is to enumerate and map those
+ * registers.
+ */
+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;
+	int rc, regloc, i;
+
+	regloc = cxl_mem_dvsec(pdev, PCI_DVSEC_ID_CXL_REGLOC_OFFSET);
+	if (!regloc) {
+		dev_err(dev, "register location dvsec not found\n");
+		return -ENXIO;
+	}
+
+	/* 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);
+
+	regloc += PCI_DVSEC_ID_CXL_REGLOC_BLOCK1_OFFSET;
+	regblocks = (regloc_size - PCI_DVSEC_ID_CXL_REGLOC_BLOCK1_OFFSET) / 8;
+
+	for (i = 0; i < regblocks; i++, regloc += 8) {
+		u32 reg_lo, reg_hi;
+		u8 reg_type;
+
+		/* "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);
+
+		if (reg_type == CXL_REGLOC_RBI_MEMDEV) {
+			rc = cxl_mem_map_regblock(cxlm, reg_lo, reg_hi);
+			if (rc)
+				return rc;
+			break;
+		}
+	}
+
+	if (i == regblocks) {
+		dev_err(dev, "Missing register locator for device registers\n");
+		return -ENXIO;
+	}
+
+	cxl_setup_device_regs(dev, cxlm->base, &regs->device_regs);
+
+	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;
+	}
+
+	return 0;
+}
+
 static struct cxl_memdev *to_cxl_memdev(struct device *dev)
 {
 	return container_of(dev, struct cxl_memdev, dev);
@@ -1410,10 +1449,8 @@ static int cxl_mem_identify(struct cxl_mem *cxlm)
 
 static int cxl_mem_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 {
-	struct device *dev = &pdev->dev;
-	u32 regloc_size, regblocks;
 	struct cxl_mem *cxlm;
-	int rc, regloc, i;
+	int rc;
 
 	rc = pcim_enable_device(pdev);
 	if (rc)
@@ -1423,42 +1460,6 @@ static int cxl_mem_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	if (IS_ERR(cxlm))
 		return PTR_ERR(cxlm);
 
-	regloc = cxl_mem_dvsec(pdev, PCI_DVSEC_ID_CXL_REGLOC_OFFSET);
-	if (!regloc) {
-		dev_err(dev, "register location dvsec not found\n");
-		return -ENXIO;
-	}
-
-	/* 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);
-
-	regloc += PCI_DVSEC_ID_CXL_REGLOC_BLOCK1_OFFSET;
-	regblocks = (regloc_size - PCI_DVSEC_ID_CXL_REGLOC_BLOCK1_OFFSET) / 8;
-
-	for (i = 0; i < regblocks; i++, regloc += 8) {
-		u32 reg_lo, reg_hi;
-		u8 reg_type;
-
-		/* "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);
-
-		if (reg_type == CXL_REGLOC_RBI_MEMDEV) {
-			rc = cxl_mem_map_regblock(cxlm, reg_lo, reg_hi);
-			if (rc)
-				return rc;
-			break;
-		}
-	}
-
-	if (i == regblocks) {
-		dev_err(dev, "Missing register locator for device registers\n");
-		return -ENXIO;
-	}
-
 	rc = cxl_mem_setup_regs(cxlm);
 	if (rc)
 		return rc;
-- 
2.31.1

Re: [PATCH 3/7] cxl/mem: Move register locator logic into reg setup

[Jonathan Cameron]

On Wed, 7 Apr 2021 15:26:21 -0700
Ben Widawsky <ben.widawsky@intel.com> wrote:

> Start moving code around to ultimately get rid of @cxlm.base. The
> @cxlm.base member serves no purpose other than intermediate storage of
> the offset found in cxl_mem_map_regblock() later used by
> cxl_mem_setup_regs(). Aside from wanting to get rid of this useless
> member, it will help later when adding new register block identifiers.
> 
> While @cxlm.base still exists, it will become trivial to remove it in a
> future patch.
> 
> No functional change is meant to be introduced in this patch.
> 
> Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>

Seems like a noop refactor to me as you say.

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

> ---
>  drivers/cxl/mem.c | 135 +++++++++++++++++++++++-----------------------
>  1 file changed, 68 insertions(+), 67 deletions(-)
> 
> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index 520edaf233d4..04b4f7445083 100644
> --- a/drivers/cxl/mem.c
> +++ b/drivers/cxl/mem.c
> @@ -870,34 +870,6 @@ static int cxl_mem_mbox_send_cmd(struct cxl_mem *cxlm, u16 opcode,
>  	return 0;
>  }
>  
> -/**
> - * cxl_mem_setup_regs() - Setup necessary MMIO.
> - * @cxlm: The CXL memory device to communicate with.
> - *
> - * Return: 0 if all necessary registers mapped.
> - *
> - * A memory device is required by spec to implement a certain set of MMIO
> - * regions. The purpose of this function is to enumerate and map those
> - * registers.
> - */
> -static int cxl_mem_setup_regs(struct cxl_mem *cxlm)
> -{
> -	struct device *dev = &cxlm->pdev->dev;
> -	struct cxl_regs *regs = &cxlm->regs;
> -
> -	cxl_setup_device_regs(dev, cxlm->base, &regs->device_regs);
> -
> -	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;
> -	}
> -
> -	return 0;
> -}
> -
>  static int cxl_mem_setup_mailbox(struct cxl_mem *cxlm)
>  {
>  	const int cap = readl(cxlm->regs.mbox + CXLDEV_MBOX_CAPS_OFFSET);
> @@ -1005,6 +977,73 @@ static int cxl_mem_dvsec(struct pci_dev *pdev, int dvsec)
>  	return 0;
>  }
>  
> +/**
> + * cxl_mem_setup_regs() - Setup necessary MMIO.
> + * @cxlm: The CXL memory device to communicate with.
> + *
> + * Return: 0 if all necessary registers mapped.
> + *
> + * A memory device is required by spec to implement a certain set of MMIO
> + * regions. The purpose of this function is to enumerate and map those
> + * registers.
> + */
> +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;
> +	int rc, regloc, i;
> +
> +	regloc = cxl_mem_dvsec(pdev, PCI_DVSEC_ID_CXL_REGLOC_OFFSET);
> +	if (!regloc) {
> +		dev_err(dev, "register location dvsec not found\n");
> +		return -ENXIO;
> +	}
> +
> +	/* 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);
> +
> +	regloc += PCI_DVSEC_ID_CXL_REGLOC_BLOCK1_OFFSET;
> +	regblocks = (regloc_size - PCI_DVSEC_ID_CXL_REGLOC_BLOCK1_OFFSET) / 8;
> +
> +	for (i = 0; i < regblocks; i++, regloc += 8) {
> +		u32 reg_lo, reg_hi;
> +		u8 reg_type;
> +
> +		/* "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);
> +
> +		if (reg_type == CXL_REGLOC_RBI_MEMDEV) {
> +			rc = cxl_mem_map_regblock(cxlm, reg_lo, reg_hi);
> +			if (rc)
> +				return rc;
> +			break;
> +		}
> +	}
> +
> +	if (i == regblocks) {
> +		dev_err(dev, "Missing register locator for device registers\n");
> +		return -ENXIO;
> +	}
> +
> +	cxl_setup_device_regs(dev, cxlm->base, &regs->device_regs);
> +
> +	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;
> +	}
> +
> +	return 0;
> +}
> +
>  static struct cxl_memdev *to_cxl_memdev(struct device *dev)
>  {
>  	return container_of(dev, struct cxl_memdev, dev);
> @@ -1410,10 +1449,8 @@ static int cxl_mem_identify(struct cxl_mem *cxlm)
>  
>  static int cxl_mem_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  {
> -	struct device *dev = &pdev->dev;
> -	u32 regloc_size, regblocks;
>  	struct cxl_mem *cxlm;
> -	int rc, regloc, i;
> +	int rc;
>  
>  	rc = pcim_enable_device(pdev);
>  	if (rc)
> @@ -1423,42 +1460,6 @@ static int cxl_mem_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	if (IS_ERR(cxlm))
>  		return PTR_ERR(cxlm);
>  
> -	regloc = cxl_mem_dvsec(pdev, PCI_DVSEC_ID_CXL_REGLOC_OFFSET);
> -	if (!regloc) {
> -		dev_err(dev, "register location dvsec not found\n");
> -		return -ENXIO;
> -	}
> -
> -	/* 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);
> -
> -	regloc += PCI_DVSEC_ID_CXL_REGLOC_BLOCK1_OFFSET;
> -	regblocks = (regloc_size - PCI_DVSEC_ID_CXL_REGLOC_BLOCK1_OFFSET) / 8;
> -
> -	for (i = 0; i < regblocks; i++, regloc += 8) {
> -		u32 reg_lo, reg_hi;
> -		u8 reg_type;
> -
> -		/* "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);
> -
> -		if (reg_type == CXL_REGLOC_RBI_MEMDEV) {
> -			rc = cxl_mem_map_regblock(cxlm, reg_lo, reg_hi);
> -			if (rc)
> -				return rc;
> -			break;
> -		}
> -	}
> -
> -	if (i == regblocks) {
> -		dev_err(dev, "Missing register locator for device registers\n");
> -		return -ENXIO;
> -	}
> -
>  	rc = cxl_mem_setup_regs(cxlm);
>  	if (rc)
>  		return rc;

[PATCH 4/7] cxl/mem: Get rid of @cxlm.base

[Ben Widawsky]

@cxlm.base only existed to support holding the base found in the
register block mapping code, and pass it along to the register setup
code. Now that the register setup function has all logic around managing
the registers, from DVSEC to iomapping up to populating our CXL specific
information, it is easy to turn the @base values into local variables
and remove them from our device driver state.

Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
---
 drivers/cxl/mem.c | 24 +++++++++++-------------
 drivers/cxl/mem.h |  2 --
 2 files changed, 11 insertions(+), 15 deletions(-)

diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
index 04b4f7445083..60b95c524c3e 100644
--- a/drivers/cxl/mem.c
+++ b/drivers/cxl/mem.c
@@ -922,11 +922,10 @@ static struct cxl_mem *cxl_mem_create(struct pci_dev *pdev)
 	return cxlm;
 }
 
-static int 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, u32 reg_lo, u32 reg_hi)
 {
 	struct pci_dev *pdev = cxlm->pdev;
 	struct device *dev = &pdev->dev;
-	void __iomem *regs;
 	u64 offset;
 	u8 bar;
 	int rc;
@@ -938,20 +937,18 @@ static int cxl_mem_map_regblock(struct cxl_mem *cxlm, u32 reg_lo, u32 reg_hi)
 	if (pci_resource_len(pdev, bar) < offset) {
 		dev_err(dev, "BAR%d: %pr: too small (offset: %#llx)\n", bar,
 			&pdev->resource[bar], (unsigned long long)offset);
-		return -ENXIO;
+		return (void __iomem *)ERR_PTR(-ENXIO);
 	}
 
 	rc = pcim_iomap_regions(pdev, BIT(bar), pci_name(pdev));
 	if (rc) {
 		dev_err(dev, "failed to map registers\n");
-		return rc;
+		return (void __iomem *)ERR_PTR(rc);
 	}
-	regs = pcim_iomap_table(pdev)[bar];
-
-	cxlm->base = regs + offset;
 
 	dev_dbg(dev, "Mapped CXL Memory Device resource\n");
-	return 0;
+
+	return pcim_iomap_table(pdev)[bar] + offset;
 }
 
 static int cxl_mem_dvsec(struct pci_dev *pdev, int dvsec)
@@ -993,7 +990,8 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm)
 	struct pci_dev *pdev = cxlm->pdev;
 	struct device *dev = &pdev->dev;
 	u32 regloc_size, regblocks;
-	int rc, regloc, i;
+	void __iomem *base;
+	int regloc, i;
 
 	regloc = cxl_mem_dvsec(pdev, PCI_DVSEC_ID_CXL_REGLOC_OFFSET);
 	if (!regloc) {
@@ -1019,9 +1017,9 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm)
 		reg_type = FIELD_GET(CXL_REGLOC_RBI_MASK, reg_lo);
 
 		if (reg_type == CXL_REGLOC_RBI_MEMDEV) {
-			rc = cxl_mem_map_regblock(cxlm, reg_lo, reg_hi);
-			if (rc)
-				return rc;
+			base = cxl_mem_map_regblock(cxlm, reg_lo, reg_hi);
+			if (IS_ERR(base))
+				return PTR_ERR(base);
 			break;
 		}
 	}
@@ -1031,7 +1029,7 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm)
 		return -ENXIO;
 	}
 
-	cxl_setup_device_regs(dev, cxlm->base, &regs->device_regs);
+	cxl_setup_device_regs(dev, base, &regs->device_regs);
 
 	if (!regs->status || !regs->mbox || !regs->memdev) {
 		dev_err(dev, "registers not found: %s%s%s\n",
diff --git a/drivers/cxl/mem.h b/drivers/cxl/mem.h
index 8bad7166adba..bfcfef461b16 100644
--- a/drivers/cxl/mem.h
+++ b/drivers/cxl/mem.h
@@ -49,7 +49,6 @@ struct cxl_memdev {
 /**
  * struct cxl_mem - A CXL memory device
  * @pdev: The PCI device associated with this CXL device.
- * @base: IO mappings to the device's MMIO
  * @cxlmd: Logical memory device chardev / interface
  * @regs: Parsed register blocks
  * @payload_size: Size of space for payload
@@ -62,7 +61,6 @@ struct cxl_memdev {
  */
 struct cxl_mem {
 	struct pci_dev *pdev;
-	void __iomem *base;
 	struct cxl_memdev *cxlmd;
 
 	struct cxl_regs regs;
-- 
2.31.1

Re: [PATCH 4/7] cxl/mem: Get rid of @cxlm.base

[Jonathan Cameron]

On Wed, 7 Apr 2021 15:26:22 -0700
Ben Widawsky <ben.widawsky@intel.com> wrote:

> @cxlm.base only existed to support holding the base found in the
> register block mapping code, and pass it along to the register setup
> code. Now that the register setup function has all logic around managing
> the registers, from DVSEC to iomapping up to populating our CXL specific
> information, it is easy to turn the @base values into local variables
> and remove them from our device driver state.
> 
> Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>

Patch is basically fine, but I do wonder if you could avoid the
nasty casting in and out of __iomem in the error paths.

It's a common enough idiom though so I'm not htat fussed.

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

> ---
>  drivers/cxl/mem.c | 24 +++++++++++-------------
>  drivers/cxl/mem.h |  2 --
>  2 files changed, 11 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index 04b4f7445083..60b95c524c3e 100644
> --- a/drivers/cxl/mem.c
> +++ b/drivers/cxl/mem.c
> @@ -922,11 +922,10 @@ static struct cxl_mem *cxl_mem_create(struct pci_dev *pdev)
>  	return cxlm;
>  }
>  
> -static int 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, u32 reg_lo, u32 reg_hi)
>  {
>  	struct pci_dev *pdev = cxlm->pdev;
>  	struct device *dev = &pdev->dev;
> -	void __iomem *regs;
>  	u64 offset;
>  	u8 bar;
>  	int rc;
> @@ -938,20 +937,18 @@ static int cxl_mem_map_regblock(struct cxl_mem *cxlm, u32 reg_lo, u32 reg_hi)
>  	if (pci_resource_len(pdev, bar) < offset) {
>  		dev_err(dev, "BAR%d: %pr: too small (offset: %#llx)\n", bar,
>  			&pdev->resource[bar], (unsigned long long)offset);
> -		return -ENXIO;
> +		return (void __iomem *)ERR_PTR(-ENXIO);
>  	}
>  
>  	rc = pcim_iomap_regions(pdev, BIT(bar), pci_name(pdev));
>  	if (rc) {
>  		dev_err(dev, "failed to map registers\n");
> -		return rc;
> +		return (void __iomem *)ERR_PTR(rc);

The casting is fairly horrible, perhaps just pass in
a void __iomem ** and pass base back through that?

>  	}
> -	regs = pcim_iomap_table(pdev)[bar];
> -
> -	cxlm->base = regs + offset;
>  
>  	dev_dbg(dev, "Mapped CXL Memory Device resource\n");
> -	return 0;
> +
> +	return pcim_iomap_table(pdev)[bar] + offset;
>  }
>  
>  static int cxl_mem_dvsec(struct pci_dev *pdev, int dvsec)
> @@ -993,7 +990,8 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm)
>  	struct pci_dev *pdev = cxlm->pdev;
>  	struct device *dev = &pdev->dev;
>  	u32 regloc_size, regblocks;
> -	int rc, regloc, i;
> +	void __iomem *base;
> +	int regloc, i;
>  
>  	regloc = cxl_mem_dvsec(pdev, PCI_DVSEC_ID_CXL_REGLOC_OFFSET);
>  	if (!regloc) {
> @@ -1019,9 +1017,9 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm)
>  		reg_type = FIELD_GET(CXL_REGLOC_RBI_MASK, reg_lo);
>  
>  		if (reg_type == CXL_REGLOC_RBI_MEMDEV) {
> -			rc = cxl_mem_map_regblock(cxlm, reg_lo, reg_hi);
> -			if (rc)
> -				return rc;
> +			base = cxl_mem_map_regblock(cxlm, reg_lo, reg_hi);
> +			if (IS_ERR(base))
> +				return PTR_ERR(base);
>  			break;
>  		}
>  	}
> @@ -1031,7 +1029,7 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm)
>  		return -ENXIO;
>  	}
>  
> -	cxl_setup_device_regs(dev, cxlm->base, &regs->device_regs);
> +	cxl_setup_device_regs(dev, base, &regs->device_regs);
>  
>  	if (!regs->status || !regs->mbox || !regs->memdev) {
>  		dev_err(dev, "registers not found: %s%s%s\n",
> diff --git a/drivers/cxl/mem.h b/drivers/cxl/mem.h
> index 8bad7166adba..bfcfef461b16 100644
> --- a/drivers/cxl/mem.h
> +++ b/drivers/cxl/mem.h
> @@ -49,7 +49,6 @@ struct cxl_memdev {
>  /**
>   * struct cxl_mem - A CXL memory device
>   * @pdev: The PCI device associated with this CXL device.
> - * @base: IO mappings to the device's MMIO
>   * @cxlmd: Logical memory device chardev / interface
>   * @regs: Parsed register blocks
>   * @payload_size: Size of space for payload
> @@ -62,7 +61,6 @@ struct cxl_memdev {
>   */
>  struct cxl_mem {
>  	struct pci_dev *pdev;
> -	void __iomem *base;
>  	struct cxl_memdev *cxlmd;
>  
>  	struct cxl_regs regs;

Re: [PATCH 4/7] cxl/mem: Get rid of @cxlm.base

[Ben Widawsky]

On 21-04-08 18:26:35, Jonathan Cameron wrote:
> On Wed, 7 Apr 2021 15:26:22 -0700
> Ben Widawsky <ben.widawsky@intel.com> wrote:
> 
> > @cxlm.base only existed to support holding the base found in the
> > register block mapping code, and pass it along to the register setup
> > code. Now that the register setup function has all logic around managing
> > the registers, from DVSEC to iomapping up to populating our CXL specific
> > information, it is easy to turn the @base values into local variables
> > and remove them from our device driver state.
> > 
> > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> 
> Patch is basically fine, but I do wonder if you could avoid the
> nasty casting in and out of __iomem in the error paths.
> 
> It's a common enough idiom though so I'm not htat fussed.
> 
> Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> > ---
> >  drivers/cxl/mem.c | 24 +++++++++++-------------
> >  drivers/cxl/mem.h |  2 --
> >  2 files changed, 11 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> > index 04b4f7445083..60b95c524c3e 100644
> > --- a/drivers/cxl/mem.c
> > +++ b/drivers/cxl/mem.c
> > @@ -922,11 +922,10 @@ static struct cxl_mem *cxl_mem_create(struct pci_dev *pdev)
> >  	return cxlm;
> >  }
> >  
> > -static int 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, u32 reg_lo, u32 reg_hi)
> >  {
> >  	struct pci_dev *pdev = cxlm->pdev;
> >  	struct device *dev = &pdev->dev;
> > -	void __iomem *regs;
> >  	u64 offset;
> >  	u8 bar;
> >  	int rc;
> > @@ -938,20 +937,18 @@ static int cxl_mem_map_regblock(struct cxl_mem *cxlm, u32 reg_lo, u32 reg_hi)
> >  	if (pci_resource_len(pdev, bar) < offset) {
> >  		dev_err(dev, "BAR%d: %pr: too small (offset: %#llx)\n", bar,
> >  			&pdev->resource[bar], (unsigned long long)offset);
> > -		return -ENXIO;
> > +		return (void __iomem *)ERR_PTR(-ENXIO);
> >  	}
> >  
> >  	rc = pcim_iomap_regions(pdev, BIT(bar), pci_name(pdev));
> >  	if (rc) {
> >  		dev_err(dev, "failed to map registers\n");
> > -		return rc;
> > +		return (void __iomem *)ERR_PTR(rc);
> 
> The casting is fairly horrible, perhaps just pass in
> a void __iomem ** and pass base back through that?
> 

TIL: IOMEM_ERR_PTR. Would that suffice?

> >  	}
> > -	regs = pcim_iomap_table(pdev)[bar];
> > -
> > -	cxlm->base = regs + offset;
> >  
> >  	dev_dbg(dev, "Mapped CXL Memory Device resource\n");
> > -	return 0;
> > +
> > +	return pcim_iomap_table(pdev)[bar] + offset;
> >  }
> >  
> >  static int cxl_mem_dvsec(struct pci_dev *pdev, int dvsec)
> > @@ -993,7 +990,8 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm)
> >  	struct pci_dev *pdev = cxlm->pdev;
> >  	struct device *dev = &pdev->dev;
> >  	u32 regloc_size, regblocks;
> > -	int rc, regloc, i;
> > +	void __iomem *base;
> > +	int regloc, i;
> >  
> >  	regloc = cxl_mem_dvsec(pdev, PCI_DVSEC_ID_CXL_REGLOC_OFFSET);
> >  	if (!regloc) {
> > @@ -1019,9 +1017,9 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm)
> >  		reg_type = FIELD_GET(CXL_REGLOC_RBI_MASK, reg_lo);
> >  
> >  		if (reg_type == CXL_REGLOC_RBI_MEMDEV) {
> > -			rc = cxl_mem_map_regblock(cxlm, reg_lo, reg_hi);
> > -			if (rc)
> > -				return rc;
> > +			base = cxl_mem_map_regblock(cxlm, reg_lo, reg_hi);
> > +			if (IS_ERR(base))
> > +				return PTR_ERR(base);
> >  			break;
> >  		}
> >  	}
> > @@ -1031,7 +1029,7 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm)
> >  		return -ENXIO;
> >  	}
> >  
> > -	cxl_setup_device_regs(dev, cxlm->base, &regs->device_regs);
> > +	cxl_setup_device_regs(dev, base, &regs->device_regs);
> >  
> >  	if (!regs->status || !regs->mbox || !regs->memdev) {
> >  		dev_err(dev, "registers not found: %s%s%s\n",
> > diff --git a/drivers/cxl/mem.h b/drivers/cxl/mem.h
> > index 8bad7166adba..bfcfef461b16 100644
> > --- a/drivers/cxl/mem.h
> > +++ b/drivers/cxl/mem.h
> > @@ -49,7 +49,6 @@ struct cxl_memdev {
> >  /**
> >   * struct cxl_mem - A CXL memory device
> >   * @pdev: The PCI device associated with this CXL device.
> > - * @base: IO mappings to the device's MMIO
> >   * @cxlmd: Logical memory device chardev / interface
> >   * @regs: Parsed register blocks
> >   * @payload_size: Size of space for payload
> > @@ -62,7 +61,6 @@ struct cxl_memdev {
> >   */
> >  struct cxl_mem {
> >  	struct pci_dev *pdev;
> > -	void __iomem *base;
> >  	struct cxl_memdev *cxlmd;
> >  
> >  	struct cxl_regs regs;
> 

Re: [PATCH 4/7] cxl/mem: Get rid of @cxlm.base

[Jonathan Cameron]

On Tue, 13 Apr 2021 09:17:26 -0700
Ben Widawsky <ben.widawsky@intel.com> wrote:

> On 21-04-08 18:26:35, Jonathan Cameron wrote:
> > On Wed, 7 Apr 2021 15:26:22 -0700
> > Ben Widawsky <ben.widawsky@intel.com> wrote:
> >   
> > > @cxlm.base only existed to support holding the base found in the
> > > register block mapping code, and pass it along to the register setup
> > > code. Now that the register setup function has all logic around managing
> > > the registers, from DVSEC to iomapping up to populating our CXL specific
> > > information, it is easy to turn the @base values into local variables
> > > and remove them from our device driver state.
> > > 
> > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>  
> > 
> > Patch is basically fine, but I do wonder if you could avoid the
> > nasty casting in and out of __iomem in the error paths.
> > 
> > It's a common enough idiom though so I'm not htat fussed.
> > 
> > Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> >   
> > > ---
> > >  drivers/cxl/mem.c | 24 +++++++++++-------------
> > >  drivers/cxl/mem.h |  2 --
> > >  2 files changed, 11 insertions(+), 15 deletions(-)
> > > 
> > > diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> > > index 04b4f7445083..60b95c524c3e 100644
> > > --- a/drivers/cxl/mem.c
> > > +++ b/drivers/cxl/mem.c
> > > @@ -922,11 +922,10 @@ static struct cxl_mem *cxl_mem_create(struct pci_dev *pdev)
> > >  	return cxlm;
> > >  }
> > >  
> > > -static int 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, u32 reg_lo, u32 reg_hi)
> > >  {
> > >  	struct pci_dev *pdev = cxlm->pdev;
> > >  	struct device *dev = &pdev->dev;
> > > -	void __iomem *regs;
> > >  	u64 offset;
> > >  	u8 bar;
> > >  	int rc;
> > > @@ -938,20 +937,18 @@ static int cxl_mem_map_regblock(struct cxl_mem *cxlm, u32 reg_lo, u32 reg_hi)
> > >  	if (pci_resource_len(pdev, bar) < offset) {
> > >  		dev_err(dev, "BAR%d: %pr: too small (offset: %#llx)\n", bar,
> > >  			&pdev->resource[bar], (unsigned long long)offset);
> > > -		return -ENXIO;
> > > +		return (void __iomem *)ERR_PTR(-ENXIO);
> > >  	}
> > >  
> > >  	rc = pcim_iomap_regions(pdev, BIT(bar), pci_name(pdev));
> > >  	if (rc) {
> > >  		dev_err(dev, "failed to map registers\n");
> > > -		return rc;
> > > +		return (void __iomem *)ERR_PTR(rc);  
> > 
> > The casting is fairly horrible, perhaps just pass in
> > a void __iomem ** and pass base back through that?
> >   
> 
> TIL: IOMEM_ERR_PTR. Would that suffice?

Definitely.  Didn't know about that!

Jonathan

[PATCH v2 4/7] cxl/mem: Get rid of @cxlm.base

[Ben Widawsky]

@cxlm.base only existed to support holding the base found in the
register block mapping code, and pass it along to the register setup
code. Now that the register setup function has all logic around managing
the registers, from DVSEC to iomapping up to populating our CXL specific
information, it is easy to turn the @base values into local variables
and remove them from our device driver state.

Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
---
 drivers/cxl/mem.h |  2 --
 drivers/cxl/pci.c | 24 +++++++++++-------------
 2 files changed, 11 insertions(+), 15 deletions(-)

diff --git a/drivers/cxl/mem.h b/drivers/cxl/mem.h
index 23fc40dde27e..13868ff7cadf 100644
--- a/drivers/cxl/mem.h
+++ b/drivers/cxl/mem.h
@@ -49,7 +49,6 @@ struct cxl_memdev {
 /**
  * struct cxl_mem - A CXL memory device
  * @pdev: The PCI device associated with this CXL device.
- * @base: IO mappings to the device's MMIO
  * @cxlmd: Logical memory device chardev / interface
  * @regs: Parsed register blocks
  * @payload_size: Size of space for payload
@@ -64,7 +63,6 @@ struct cxl_memdev {
  */
 struct cxl_mem {
 	struct pci_dev *pdev;
-	void __iomem *base;
 	struct cxl_memdev *cxlmd;
 
 	struct cxl_regs regs;
diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 285a898a0867..d47258e51563 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -941,11 +941,10 @@ static struct cxl_mem *cxl_mem_create(struct pci_dev *pdev)
 	return cxlm;
 }
 
-static int 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, u32 reg_lo, u32 reg_hi)
 {
 	struct pci_dev *pdev = cxlm->pdev;
 	struct device *dev = &pdev->dev;
-	void __iomem *regs;
 	u64 offset;
 	u8 bar;
 	int rc;
@@ -957,20 +956,18 @@ static int cxl_mem_map_regblock(struct cxl_mem *cxlm, u32 reg_lo, u32 reg_hi)
 	if (pci_resource_len(pdev, bar) < offset) {
 		dev_err(dev, "BAR%d: %pr: too small (offset: %#llx)\n", bar,
 			&pdev->resource[bar], (unsigned long long)offset);
-		return -ENXIO;
+		return IOMEM_ERR_PTR(-ENXIO);
 	}
 
 	rc = pcim_iomap_regions(pdev, BIT(bar), pci_name(pdev));
 	if (rc) {
 		dev_err(dev, "failed to map registers\n");
-		return rc;
+		return IOMEM_ERR_PTR(rc);
 	}
-	regs = pcim_iomap_table(pdev)[bar];
-
-	cxlm->base = regs + offset;
 
 	dev_dbg(dev, "Mapped CXL Memory Device resource\n");
-	return 0;
+
+	return pcim_iomap_table(pdev)[bar] + offset;
 }
 
 static int cxl_mem_dvsec(struct pci_dev *pdev, int dvsec)
@@ -1012,7 +1009,8 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm)
 	struct pci_dev *pdev = cxlm->pdev;
 	struct device *dev = &pdev->dev;
 	u32 regloc_size, regblocks;
-	int rc, regloc, i;
+	void __iomem *base;
+	int regloc, i;
 
 	regloc = cxl_mem_dvsec(pdev, PCI_DVSEC_ID_CXL_REGLOC_OFFSET);
 	if (!regloc) {
@@ -1038,9 +1036,9 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm)
 		reg_type = FIELD_GET(CXL_REGLOC_RBI_MASK, reg_lo);
 
 		if (reg_type == CXL_REGLOC_RBI_MEMDEV) {
-			rc = cxl_mem_map_regblock(cxlm, reg_lo, reg_hi);
-			if (rc)
-				return rc;
+			base = cxl_mem_map_regblock(cxlm, reg_lo, reg_hi);
+			if (IS_ERR(base))
+				return PTR_ERR(base);
 			break;
 		}
 	}
@@ -1050,7 +1048,7 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm)
 		return -ENXIO;
 	}
 
-	cxl_setup_device_regs(dev, cxlm->base, &regs->device_regs);
+	cxl_setup_device_regs(dev, base, &regs->device_regs);
 
 	if (!regs->status || !regs->mbox || !regs->memdev) {
 		dev_err(dev, "registers not found: %s%s%s\n",
-- 
2.31.1

[PATCH 5/7] cxl/mem: Move device register setup

[Ben Widawsky]

Support expansion of register block types that the driver will attempt
to recognize by pulling the code up into the register block scanning
loop. Subsequent code can easily add in new register block types with
this.

Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
---
 drivers/cxl/mem.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
index 60b95c524c3e..49f651694cb0 100644
--- a/drivers/cxl/mem.c
+++ b/drivers/cxl/mem.c
@@ -1020,6 +1020,15 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm)
 			base = cxl_mem_map_regblock(cxlm, reg_lo, reg_hi);
 			if (IS_ERR(base))
 				return PTR_ERR(base);
+
+			cxl_setup_device_regs(dev, base, &regs->device_regs);
+			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;
+			}
 			break;
 		}
 	}
@@ -1029,16 +1038,6 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm)
 		return -ENXIO;
 	}
 
-	cxl_setup_device_regs(dev, base, &regs->device_regs);
-
-	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;
-	}
-
 	return 0;
 }
 
-- 
2.31.1

Re: [PATCH 5/7] cxl/mem: Move device register setup

[Jonathan Cameron]

On Wed, 7 Apr 2021 15:26:23 -0700
Ben Widawsky <ben.widawsky@intel.com> wrote:

> Support expansion of register block types that the driver will attempt
> to recognize by pulling the code up into the register block scanning
> loop. Subsequent code can easily add in new register block types with
> this.
> 
> Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>

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

> ---
>  drivers/cxl/mem.c | 19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index 60b95c524c3e..49f651694cb0 100644
> --- a/drivers/cxl/mem.c
> +++ b/drivers/cxl/mem.c
> @@ -1020,6 +1020,15 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm)
>  			base = cxl_mem_map_regblock(cxlm, reg_lo, reg_hi);
>  			if (IS_ERR(base))
>  				return PTR_ERR(base);
> +
> +			cxl_setup_device_regs(dev, base, &regs->device_regs);
> +			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;
> +			}
>  			break;
>  		}
>  	}
> @@ -1029,16 +1038,6 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm)
>  		return -ENXIO;
>  	}
>  
> -	cxl_setup_device_regs(dev, base, &regs->device_regs);
> -
> -	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;
> -	}
> -
>  	return 0;
>  }
>  

[PATCH 6/7] cxl/mem: Create a helper to setup device regs

[Ben Widawsky]

Memory devices have a list of required register regions within the
register block, but this isn't required of all CXL components or
devices. To make things more tidy, and allow for easily setting up other
block types in this loop, the helper is introduced.

Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
---
 drivers/cxl/mem.c | 38 +++++++++++++++++++++++---------------
 1 file changed, 23 insertions(+), 15 deletions(-)

diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
index 49f651694cb0..b7342aaf38c4 100644
--- a/drivers/cxl/mem.c
+++ b/drivers/cxl/mem.c
@@ -974,6 +974,24 @@ static int cxl_mem_dvsec(struct pci_dev *pdev, int dvsec)
 	return 0;
 }
 
+static int __cxl_setup_device_regs(struct cxl_mem *cxlm, void __iomem *base)
+{
+	struct cxl_regs *regs = &cxlm->regs;
+	struct pci_dev *pdev = cxlm->pdev;
+	struct device *dev = &pdev->dev;
+
+	cxl_setup_device_regs(dev, base, &regs->device_regs);
+	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;
+	}
+
+	return 0;
+}
+
 /**
  * cxl_mem_setup_regs() - Setup necessary MMIO.
  * @cxlm: The CXL memory device to communicate with.
@@ -986,12 +1004,11 @@ static int cxl_mem_dvsec(struct pci_dev *pdev, int dvsec)
  */
 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;
+	int regloc, i, rc;
 
 	regloc = cxl_mem_dvsec(pdev, PCI_DVSEC_ID_CXL_REGLOC_OFFSET);
 	if (!regloc) {
@@ -1021,23 +1038,14 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm)
 			if (IS_ERR(base))
 				return PTR_ERR(base);
 
-			cxl_setup_device_regs(dev, base, &regs->device_regs);
-			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;
-			}
+			rc = __cxl_setup_device_regs(cxlm, base);
+			if (rc)
+				return rc;
+
 			break;
 		}
 	}
 
-	if (i == regblocks) {
-		dev_err(dev, "Missing register locator for device registers\n");
-		return -ENXIO;
-	}
-
 	return 0;
 }
 
-- 
2.31.1

Re: [PATCH 6/7] cxl/mem: Create a helper to setup device regs

[Jonathan Cameron]

On Wed, 7 Apr 2021 15:26:24 -0700
Ben Widawsky <ben.widawsky@intel.com> wrote:

> Memory devices have a list of required register regions within the
> register block, but this isn't required of all CXL components or
> devices. To make things more tidy, and allow for easily setting up other
> block types in this loop, the helper is introduced.

Two things in here, the move and allowing it to be missing.
I would call that out explicitly in the patch description.

> 
> Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> ---
>  drivers/cxl/mem.c | 38 +++++++++++++++++++++++---------------
>  1 file changed, 23 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index 49f651694cb0..b7342aaf38c4 100644
> --- a/drivers/cxl/mem.c
> +++ b/drivers/cxl/mem.c
> @@ -974,6 +974,24 @@ static int cxl_mem_dvsec(struct pci_dev *pdev, int dvsec)
>  	return 0;
>  }
>  
> +static int __cxl_setup_device_regs(struct cxl_mem *cxlm, void __iomem *base)

Naming is a little unusual.   Normally __ would imply unlocked or something
like that, whereas here it's mostly implying more error checks.

I don't immediately have a good idea for a name however...

> +{
> +	struct cxl_regs *regs = &cxlm->regs;
> +	struct pci_dev *pdev = cxlm->pdev;
> +	struct device *dev = &pdev->dev;
> +
> +	cxl_setup_device_regs(dev, base, &regs->device_regs);
> +	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;
> +	}
> +
> +	return 0;
> +}
> +
>  /**
>   * cxl_mem_setup_regs() - Setup necessary MMIO.
>   * @cxlm: The CXL memory device to communicate with.
> @@ -986,12 +1004,11 @@ static int cxl_mem_dvsec(struct pci_dev *pdev, int dvsec)
>   */
>  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;
> +	int regloc, i, rc;
>  
>  	regloc = cxl_mem_dvsec(pdev, PCI_DVSEC_ID_CXL_REGLOC_OFFSET);
>  	if (!regloc) {
> @@ -1021,23 +1038,14 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm)
>  			if (IS_ERR(base))
>  				return PTR_ERR(base);
>  
> -			cxl_setup_device_regs(dev, base, &regs->device_regs);
> -			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;
> -			}
> +			rc = __cxl_setup_device_regs(cxlm, base);
> +			if (rc)
> +				return rc;
> +
>  			break;
>  		}
>  	}
>  
> -	if (i == regblocks) {
> -		dev_err(dev, "Missing register locator for device registers\n");
> -		return -ENXIO;
> -	}
> -
>  	return 0;
>  }
>  

Re: [PATCH 6/7] cxl/mem: Create a helper to setup device regs

[Dan Williams]

On Wed, Apr 7, 2021 at 3:26 PM Ben Widawsky <ben.widawsky@intel.com> wrote:
>
> Memory devices have a list of required register regions within the
> register block, but this isn't required of all CXL components or
> devices. To make things more tidy, and allow for easily setting up other
> block types in this loop, the helper is introduced.

I don't understand the point of this or the organization choice.
cxl_setup_device_regs() *is* the core implementation so there
shouldn't be a "__" prefixed version wrapping it. I agree that some
users will not care that some of the device registers are not found,
but that won't be cxl_mem_setup_regs() making that call.

[PATCH 7/7] cxl: Add HDM decoder capbilities

[Ben Widawsky]

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.

Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
---
 drivers/cxl/core.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++
 drivers/cxl/cxl.h  | 48 ++++++++++++++++++++++++++++++
 drivers/cxl/mem.c  | 37 ++++++++++++++++++++---
 drivers/cxl/pci.h  |  1 +
 4 files changed, 155 insertions(+), 4 deletions(-)

diff --git a/drivers/cxl/core.c b/drivers/cxl/core.c
index 65cd704581bc..db6a83eed0a2 100644
--- a/drivers/cxl/core.c
+++ b/drivers/cxl/core.c
@@ -479,6 +479,79 @@ struct cxl_port *devm_cxl_add_port(struct device *host,
 }
 EXPORT_SYMBOL_GPL(devm_cxl_add_port);
 
+void cxl_setup_component_regs(struct device *dev, void __iomem *base,
+			      struct cxl_component_regs *regs)
+{
+	int cap, cap_count;
+	u64 cap_array;
+
+	*regs = (struct cxl_component_regs) { 0 };
+
+	/*
+	 * CXL.cache and CXL.mem registers are at offset 0x1000 as defined in
+	 * CXL 2.0 8.2.4 Table 141.
+	 *
+	 * TODO: Map other registers as needed.
+	 */
+	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 */
+#define CAPID_VERSION_CHECK(data, mask, expected, capability_msg)              \
+	do {                                                                   \
+		if (FIELD_GET(mask, data) < expected) {                        \
+			dev_err(dev,                                           \
+				capability_msg                                 \
+				" version %ld is below expected %d",           \
+				FIELD_GET(mask, data), expected);              \
+			return;                                                \
+		}                                                              \
+	} while (0)
+
+	CAPID_VERSION_CHECK(cap_array, CXL_CM_CAP_HDR_VERSION_MASK,
+			    CM_CAP_HDR_CAP_VERSION, "Capability array header");
+	CAPID_VERSION_CHECK(cap_array, CXL_CM_CAP_HDR_CACHE_MEM_VERSION_MASK,
+			    CM_CAP_HDR_CACHE_MEM_VERSION,
+			    "Capability array header CXL.cache CXL.mem");
+
+	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;
+		u16 cap_id, offset;
+
+		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:
+			CAPID_VERSION_CHECK(hdr, CXL_CM_CAP_HDR_VERSION_MASK,
+					    CXL_CM_CAP_CAP_HDM_VERSION,
+					    "HDM decoder capability");
+			dev_dbg(dev, "found HDM decoder capability (0x%x)\n",
+				offset);
+			regs->hdm_decoder = register_block;
+			break;
+		default:
+			dev_dbg(dev, "Unknown CM cap ID: %d (0x%x)\n", cap_id,
+				offset);
+			break;
+		}
+	}
+}
+EXPORT_SYMBOL_GPL(cxl_setup_component_regs);
+
 /*
  * cxl_setup_device_regs() - Detect CXL Device register blocks
  * @dev: Host device of the @base mapping
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 0211f44c95a2..a4ad1176dc5a 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
@@ -35,11 +60,26 @@
 #define CXLDEV_MBOX_PAYLOAD_OFFSET 0x20
 
 /* See note for 'struct cxl_regs' for the rationale of this organization */
+#define CXL_COMPONENT_REGS() \
+	void __iomem *hdm_decoder
+
 #define CXL_DEVICE_REGS() \
 	void __iomem *status; \
 	void __iomem *mbox; \
 	void __iomem *memdev
 
+/**
+ * struct cxl_component_regs - Common container of CXL component register block
+ *			       base pointers.
+ *
+ * The only component registers that we care about are the CXL.cache and CXL.mem
+ * registers which are at offset 0x1000 from the component register base (CXL
+ * 2.0 8.2.4)
+ */
+struct cxl_component_regs {
+	CXL_COMPONENT_REGS();
+};
+
 /**
  * struct cxl_device_regs - Common container of CXL Device register
  * 			    block base pointers
@@ -59,6 +99,12 @@ struct cxl_device_regs {
  * The specificity reads naturally from left-to-right.
  */
 struct cxl_regs {
+	union {
+		struct {
+			CXL_COMPONENT_REGS();
+		};
+		struct cxl_component_regs component;
+	};
 	union {
 		struct {
 			CXL_DEVICE_REGS();
@@ -67,6 +113,8 @@ struct cxl_regs {
 	};
 };
 
+void cxl_setup_component_regs(struct device *dev, void __iomem *base,
+			      struct cxl_component_regs *regs);
 void cxl_setup_device_regs(struct device *dev, void __iomem *base,
 			   struct cxl_device_regs *regs);
 
diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
index b7342aaf38c4..e915e3743b76 100644
--- a/drivers/cxl/mem.c
+++ b/drivers/cxl/mem.c
@@ -974,6 +974,21 @@ static int cxl_mem_dvsec(struct pci_dev *pdev, int dvsec)
 	return 0;
 }
 
+static int __cxl_setup_component_regs(struct cxl_mem *cxlm, void __iomem *base)
+{
+	struct cxl_regs *regs = &cxlm->regs;
+	struct pci_dev *pdev = cxlm->pdev;
+	struct device *dev = &pdev->dev;
+
+	cxl_setup_component_regs(dev, base, &regs->component);
+	if (!regs->hdm_decoder) {
+		dev_err(dev, "HDM decoder registers not found\n");
+		return -ENXIO;
+	}
+
+	return 0;
+}
+
 static int __cxl_setup_device_regs(struct cxl_mem *cxlm, void __iomem *base)
 {
 	struct cxl_regs *regs = &cxlm->regs;
@@ -1032,16 +1047,30 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm)
 		pci_read_config_dword(pdev, regloc + 4, &reg_hi);
 
 		reg_type = FIELD_GET(CXL_REGLOC_RBI_MASK, reg_lo);
+		if (reg_type == CXL_REGLOC_RBI_EMPTY ||
+		    reg_type > CXL_REGLOC_RBI_MAX)
+			continue;
+
+		base = cxl_mem_map_regblock(cxlm, reg_lo, reg_hi);
+		if (IS_ERR(base))
+			return PTR_ERR(base);
 
-		if (reg_type == CXL_REGLOC_RBI_MEMDEV) {
-			base = cxl_mem_map_regblock(cxlm, reg_lo, reg_hi);
-			if (IS_ERR(base))
-				return PTR_ERR(base);
+		switch (reg_type) {
+		case CXL_REGLOC_RBI_COMPONENT:
+			rc = __cxl_setup_component_regs(cxlm, base);
+			if (rc)
+				return rc;
 
+			dev_dbg(dev, "Set up component registers\n");
+			break;
+		case CXL_REGLOC_RBI_MEMDEV:
 			rc = __cxl_setup_device_regs(cxlm, base);
 			if (rc)
 				return rc;
 
+			dev_dbg(dev, "Set up device registers\n");
+			break;
+		default:
 			break;
 		}
 	}
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.31.1

Re: [PATCH 7/7] cxl: Add HDM decoder capbilities

[Jonathan Cameron]

On Wed, 7 Apr 2021 15:26:25 -0700
Ben Widawsky <ben.widawsky@intel.com> wrote:

> 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.
> 
> Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
Some register field naming suggestions. Otherwise looks fine to me.

> ---
>  drivers/cxl/core.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/cxl/cxl.h  | 48 ++++++++++++++++++++++++++++++
>  drivers/cxl/mem.c  | 37 ++++++++++++++++++++---
>  drivers/cxl/pci.h  |  1 +
>  4 files changed, 155 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/cxl/core.c b/drivers/cxl/core.c
> index 65cd704581bc..db6a83eed0a2 100644
> --- a/drivers/cxl/core.c
> +++ b/drivers/cxl/core.c
> @@ -479,6 +479,79 @@ struct cxl_port *devm_cxl_add_port(struct device *host,
>  }
>  EXPORT_SYMBOL_GPL(devm_cxl_add_port);
>  
> +void cxl_setup_component_regs(struct device *dev, void __iomem *base,
> +			      struct cxl_component_regs *regs)
> +{
> +	int cap, cap_count;
> +	u64 cap_array;
> +
> +	*regs = (struct cxl_component_regs) { 0 };
> +
> +	/*
> +	 * CXL.cache and CXL.mem registers are at offset 0x1000 as defined in
> +	 * CXL 2.0 8.2.4 Table 141.
> +	 *
> +	 * TODO: Map other registers as needed.
> +	 */
> +	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 */
> +#define CAPID_VERSION_CHECK(data, mask, expected, capability_msg)              \
> +	do {                                                                   \
> +		if (FIELD_GET(mask, data) < expected) {                        \
> +			dev_err(dev,                                           \
> +				capability_msg                                 \
> +				" version %ld is below expected %d",           \

/n

> +				FIELD_GET(mask, data), expected);              \
> +			return;                                                \
> +		}                                                              \
> +	} while (0)
> +
> +	CAPID_VERSION_CHECK(cap_array, CXL_CM_CAP_HDR_VERSION_MASK,
> +			    CM_CAP_HDR_CAP_VERSION, "Capability array header");
> +	CAPID_VERSION_CHECK(cap_array, CXL_CM_CAP_HDR_CACHE_MEM_VERSION_MASK,
> +			    CM_CAP_HDR_CACHE_MEM_VERSION,
> +			    "Capability array header CXL.cache CXL.mem");

Is that macro worth bothering with?  Particularly as it will make the string
harder to grep for.

	ver = FIELD_GET(CXL_CM_CAP_HDR_VERSION_MASK, cap_array);
	if (ver < CM_CAP_HDR_CAP_VERSION)) {
		dev_err(dev, "Capability array header version %ld is below expected %d./n",
			ver, CM_CAP_HDER_CAP_VERSION); 	

etc seems better to me given we only have two instances.

> +
> +	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;
> +		u16 cap_id, offset;
> +
> +		hdr = readl(base + cap * 0x4);
> +
> +		cap_id = FIELD_GET(CXL_CM_CAP_HDR_ID_MASK, hdr);
See below, but I'd suggest some name changes for defines.  Whilst
it's the same value, this is in a different type of entry to where
you use CXL_CM_CAP_HDR_ID_MASK above.  Confused me so may confuse
others :)

> +		offset = FIELD_GET(CXL_CM_CAP_PTR_MASK, hdr);
> +		register_block = base + offset;
> +
> +		switch (cap_id) {
> +		case CXL_CM_CAP_CAP_ID_HDM:
> +			CAPID_VERSION_CHECK(hdr, CXL_CM_CAP_HDR_VERSION_MASK,
> +					    CXL_CM_CAP_CAP_HDM_VERSION,
> +					    "HDM decoder capability");
> +			dev_dbg(dev, "found HDM decoder capability (0x%x)\n",
> +				offset);
> +			regs->hdm_decoder = register_block;
> +			break;
> +		default:
> +			dev_dbg(dev, "Unknown CM cap ID: %d (0x%x)\n", cap_id,
> +				offset);
> +			break;
> +		}
> +	}
> +}
> +EXPORT_SYMBOL_GPL(cxl_setup_component_regs);
> +
>  /*
>   * cxl_setup_device_regs() - Detect CXL Device register blocks
>   * @dev: Host device of the @base mapping
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 0211f44c95a2..a4ad1176dc5a 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)

I'd separate the field naming for those in CXL Capability Header Register
and those for the individual headers. Naming tricky though.
If you don't do that it looks like you have overlapping fields because
of the reuse above..
 
e.g also have
#define CXL_CM_CAP_X_HDR_ID_MASK GENMASK(15, 0)
#define CXL_CM_CAP_X_HDR_CAP_MASK GENMASK(19, 16)
and rename to
#define CXL_CM_CAP_X_HDR_POINTER_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
> @@ -35,11 +60,26 @@
>  #define CXLDEV_MBOX_PAYLOAD_OFFSET 0x20
>  
>  /* See note for 'struct cxl_regs' for the rationale of this organization */
> +#define CXL_COMPONENT_REGS() \
> +	void __iomem *hdm_decoder
> +
>  #define CXL_DEVICE_REGS() \
>  	void __iomem *status; \
>  	void __iomem *mbox; \
>  	void __iomem *memdev
>  
> +/**
> + * struct cxl_component_regs - Common container of CXL component register block
> + *			       base pointers.

kernel-doc script won't like this.  Your best bet may be to just drop
the /** to /* and have it ignored.

> + *
> + * The only component registers that we care about are the CXL.cache and CXL.mem
> + * registers which are at offset 0x1000 from the component register base (CXL
> + * 2.0 8.2.4)
> + */
> +struct cxl_component_regs {
> +	CXL_COMPONENT_REGS();
> +};
> +
>  /**
>   * struct cxl_device_regs - Common container of CXL Device register
>   * 			    block base pointers
> @@ -59,6 +99,12 @@ struct cxl_device_regs {
>   * The specificity reads naturally from left-to-right.
>   */
>  struct cxl_regs {
> +	union {
> +		struct {
> +			CXL_COMPONENT_REGS();
> +		};
> +		struct cxl_component_regs component;
> +	};
>  	union {
>  		struct {
>  			CXL_DEVICE_REGS();
> @@ -67,6 +113,8 @@ struct cxl_regs {
>  	};
>  };
>  
> +void cxl_setup_component_regs(struct device *dev, void __iomem *base,
> +			      struct cxl_component_regs *regs);
>  void cxl_setup_device_regs(struct device *dev, void __iomem *base,
>  			   struct cxl_device_regs *regs);
>  
> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index b7342aaf38c4..e915e3743b76 100644
> --- a/drivers/cxl/mem.c
> +++ b/drivers/cxl/mem.c
> @@ -974,6 +974,21 @@ static int cxl_mem_dvsec(struct pci_dev *pdev, int dvsec)
>  	return 0;
>  }
>  
> +static int __cxl_setup_component_regs(struct cxl_mem *cxlm, void __iomem *base)
> +{
> +	struct cxl_regs *regs = &cxlm->regs;
> +	struct pci_dev *pdev = cxlm->pdev;
> +	struct device *dev = &pdev->dev;
> +
> +	cxl_setup_component_regs(dev, base, &regs->component);
> +	if (!regs->hdm_decoder) {
> +		dev_err(dev, "HDM decoder registers not found\n");
> +		return -ENXIO;
> +	}
> +
> +	return 0;
> +}
> +
>  static int __cxl_setup_device_regs(struct cxl_mem *cxlm, void __iomem *base)
>  {
>  	struct cxl_regs *regs = &cxlm->regs;
> @@ -1032,16 +1047,30 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm)
>  		pci_read_config_dword(pdev, regloc + 4, &reg_hi);
>  
>  		reg_type = FIELD_GET(CXL_REGLOC_RBI_MASK, reg_lo);
> +		if (reg_type == CXL_REGLOC_RBI_EMPTY ||
> +		    reg_type > CXL_REGLOC_RBI_MAX)
> +			continue;
> +
> +		base = cxl_mem_map_regblock(cxlm, reg_lo, reg_hi);
> +		if (IS_ERR(base))
> +			return PTR_ERR(base);
>  
> -		if (reg_type == CXL_REGLOC_RBI_MEMDEV) {
> -			base = cxl_mem_map_regblock(cxlm, reg_lo, reg_hi);
> -			if (IS_ERR(base))
> -				return PTR_ERR(base);
> +		switch (reg_type) {
> +		case CXL_REGLOC_RBI_COMPONENT:
> +			rc = __cxl_setup_component_regs(cxlm, base);
> +			if (rc)
> +				return rc;
>  
> +			dev_dbg(dev, "Set up component registers\n");
> +			break;
> +		case CXL_REGLOC_RBI_MEMDEV:
>  			rc = __cxl_setup_device_regs(cxlm, base);
>  			if (rc)
>  				return rc;
>  
> +			dev_dbg(dev, "Set up device registers\n");
> +			break;
> +		default:
>  			break;
>  		}
>  	}
> 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)
>  

Re: [PATCH 7/7] cxl: Add HDM decoder capbilities

[Dan Williams]

On Wed, Apr 7, 2021 at 3:26 PM Ben Widawsky <ben.widawsky@intel.com> wrote:
>
> 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.

It's always been possible and this patch just prepares for programming
them, but does not actually touch them yet. So let's drop this as I'm
not sure what it is saying.

> Such
> programming is expected for CXL devices with persistent memory, and hot
> plugged CXL devices that participate in CXL.mem with volatile memory.

Let's drop this as well because the state of what the OS is expected
to program is platform and topology specific. The OS needs to be
prepared for the HDMs to be unprogrammed, validly programmed in a
state the OS will leave untouched, and/or validly programmed in a way
the OS wants to redo.

>
> Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> ---
>  drivers/cxl/core.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/cxl/cxl.h  | 48 ++++++++++++++++++++++++++++++
>  drivers/cxl/mem.c  | 37 ++++++++++++++++++++---
>  drivers/cxl/pci.h  |  1 +
>  4 files changed, 155 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/cxl/core.c b/drivers/cxl/core.c
> index 65cd704581bc..db6a83eed0a2 100644
> --- a/drivers/cxl/core.c
> +++ b/drivers/cxl/core.c
> @@ -479,6 +479,79 @@ struct cxl_port *devm_cxl_add_port(struct device *host,
>  }
>  EXPORT_SYMBOL_GPL(devm_cxl_add_port);
>
> +void cxl_setup_component_regs(struct device *dev, void __iomem *base,
> +                             struct cxl_component_regs *regs)
> +{
> +       int cap, cap_count;
> +       u64 cap_array;
> +
> +       *regs = (struct cxl_component_regs) { 0 };
> +
> +       /*
> +        * CXL.cache and CXL.mem registers are at offset 0x1000 as defined in
> +        * CXL 2.0 8.2.4 Table 141.
> +        *
> +        * TODO: Map other registers as needed.

That TODO goes without saying.

> +        */
> +       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 */

If future versions are backwards compatible then why check? If someone
wants a driver that supports new capabilities then update the driver,
but validating that the version is non-zero seems to be a pedantic
check for no good reason to me.

> +#define CAPID_VERSION_CHECK(data, mask, expected, capability_msg)              \
> +       do {                                                                   \
> +               if (FIELD_GET(mask, data) < expected) {                        \
> +                       dev_err(dev,                                           \
> +                               capability_msg                                 \
> +                               " version %ld is below expected %d",           \
> +                               FIELD_GET(mask, data), expected);              \
> +                       return;                                                \

Ugh, "return" in a macro, please no.

> +               }                                                              \
> +       } while (0)
> +
> +       CAPID_VERSION_CHECK(cap_array, CXL_CM_CAP_HDR_VERSION_MASK,
> +                           CM_CAP_HDR_CAP_VERSION, "Capability array header");
> +       CAPID_VERSION_CHECK(cap_array, CXL_CM_CAP_HDR_CACHE_MEM_VERSION_MASK,
> +                           CM_CAP_HDR_CACHE_MEM_VERSION,
> +                           "Capability array header CXL.cache CXL.mem");
> +
> +       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;
> +               u16 cap_id, offset;
> +
> +               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:
> +                       CAPID_VERSION_CHECK(hdr, CXL_CM_CAP_HDR_VERSION_MASK,
> +                                           CXL_CM_CAP_CAP_HDM_VERSION,
> +                                           "HDM decoder capability");
> +                       dev_dbg(dev, "found HDM decoder capability (0x%x)\n",
> +                               offset);
> +                       regs->hdm_decoder = register_block;
> +                       break;
> +               default:
> +                       dev_dbg(dev, "Unknown CM cap ID: %d (0x%x)\n", cap_id,

Hex cap id? Perhaps rather than "Unknown" this can just say "Skipping"
because some of these are technically it's just the kernel does not
care.

> +                               offset);
> +                       break;
> +               }
> +       }
> +}
> +EXPORT_SYMBOL_GPL(cxl_setup_component_regs);
> +
>  /*
>   * cxl_setup_device_regs() - Detect CXL Device register blocks
>   * @dev: Host device of the @base mapping
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 0211f44c95a2..a4ad1176dc5a 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
> @@ -35,11 +60,26 @@
>  #define CXLDEV_MBOX_PAYLOAD_OFFSET 0x20
>
>  /* See note for 'struct cxl_regs' for the rationale of this organization */
> +#define CXL_COMPONENT_REGS() \
> +       void __iomem *hdm_decoder
> +
>  #define CXL_DEVICE_REGS() \
>         void __iomem *status; \
>         void __iomem *mbox; \
>         void __iomem *memdev
>
> +/**
> + * struct cxl_component_regs - Common container of CXL component register block
> + *                            base pointers.
> + *
> + * The only component registers that we care about are the CXL.cache and CXL.mem
> + * registers which are at offset 0x1000 from the component register base (CXL
> + * 2.0 8.2.4)
> + */

I don't think kernel-doc will be happy about this, so probably follow
the same fixup I did when Jonathan pointed this out for the cxl_port
patches.

> +struct cxl_component_regs {
> +       CXL_COMPONENT_REGS();
> +};
> +
>  /**
>   * struct cxl_device_regs - Common container of CXL Device register
>   *                         block base pointers
> @@ -59,6 +99,12 @@ struct cxl_device_regs {
>   * The specificity reads naturally from left-to-right.
>   */
>  struct cxl_regs {
> +       union {
> +               struct {
> +                       CXL_COMPONENT_REGS();
> +               };
> +               struct cxl_component_regs component;
> +       };
>         union {
>                 struct {
>                         CXL_DEVICE_REGS();
> @@ -67,6 +113,8 @@ struct cxl_regs {
>         };
>  };
>
> +void cxl_setup_component_regs(struct device *dev, void __iomem *base,
> +                             struct cxl_component_regs *regs);
>  void cxl_setup_device_regs(struct device *dev, void __iomem *base,
>                            struct cxl_device_regs *regs);
>
> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index b7342aaf38c4..e915e3743b76 100644
> --- a/drivers/cxl/mem.c
> +++ b/drivers/cxl/mem.c
> @@ -974,6 +974,21 @@ static int cxl_mem_dvsec(struct pci_dev *pdev, int dvsec)
>         return 0;
>  }
>
> +static int __cxl_setup_component_regs(struct cxl_mem *cxlm, void __iomem *base)
> +{
> +       struct cxl_regs *regs = &cxlm->regs;
> +       struct pci_dev *pdev = cxlm->pdev;
> +       struct device *dev = &pdev->dev;
> +
> +       cxl_setup_component_regs(dev, base, &regs->component);
> +       if (!regs->hdm_decoder) {
> +               dev_err(dev, "HDM decoder registers not found\n");
> +               return -ENXIO;

I was hoping this patch would show the justification of the
organization proposed in patch-6, but it doesn't and I think we're
better off open coding the error check in cxl_mem_setup_regs().

All the other patches in this series I did not comment on look good to me.

Re: [PATCH 7/7] cxl: Add HDM decoder capbilities

[Ben Widawsky]

Thanks for looking. Mostly trivial agreements or disagreements which I don't
care much about, but I'd really like you to focus on the last point, please.

On 21-04-15 16:27:14, Dan Williams wrote:
> On Wed, Apr 7, 2021 at 3:26 PM Ben Widawsky <ben.widawsky@intel.com> wrote:
> >
> > 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.
> 
> It's always been possible and this patch just prepares for programming
> them, but does not actually touch them yet. So let's drop this as I'm
> not sure what it is saying.
> 
> > Such
> > programming is expected for CXL devices with persistent memory, and hot
> > plugged CXL devices that participate in CXL.mem with volatile memory.
> 
> Let's drop this as well because the state of what the OS is expected
> to program is platform and topology specific. The OS needs to be
> prepared for the HDMs to be unprogrammed, validly programmed in a
> state the OS will leave untouched, and/or validly programmed in a way
> the OS wants to redo.
> 

Okay to both.

> >
> > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> > ---
> >  drivers/cxl/core.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++
> >  drivers/cxl/cxl.h  | 48 ++++++++++++++++++++++++++++++
> >  drivers/cxl/mem.c  | 37 ++++++++++++++++++++---
> >  drivers/cxl/pci.h  |  1 +
> >  4 files changed, 155 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/cxl/core.c b/drivers/cxl/core.c
> > index 65cd704581bc..db6a83eed0a2 100644
> > --- a/drivers/cxl/core.c
> > +++ b/drivers/cxl/core.c
> > @@ -479,6 +479,79 @@ struct cxl_port *devm_cxl_add_port(struct device *host,
> >  }
> >  EXPORT_SYMBOL_GPL(devm_cxl_add_port);
> >
> > +void cxl_setup_component_regs(struct device *dev, void __iomem *base,
> > +                             struct cxl_component_regs *regs)
> > +{
> > +       int cap, cap_count;
> > +       u64 cap_array;
> > +
> > +       *regs = (struct cxl_component_regs) { 0 };
> > +
> > +       /*
> > +        * CXL.cache and CXL.mem registers are at offset 0x1000 as defined in
> > +        * CXL 2.0 8.2.4 Table 141.
> > +        *
> > +        * TODO: Map other registers as needed.
> 
> That TODO goes without saying.
> 

I don't agree with this. The device register implementation is complete as is,
and this one is not.

> > +        */
> > +       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 */
> 
> If future versions are backwards compatible then why check? If someone
> wants a driver that supports new capabilities then update the driver,
> but validating that the version is non-zero seems to be a pedantic
> check for no good reason to me.
> 

This was specifically meant as a way to prevent the driver being used for 1.1
without explicit support being added (and then dropping this check). You're
right though , it should be !=, it should have been <. It's sounding like you'd
rather I remove it entirely rather than fix it though. Is that correct?

> > +#define CAPID_VERSION_CHECK(data, mask, expected, capability_msg)              \
> > +       do {                                                                   \
> > +               if (FIELD_GET(mask, data) < expected) {                        \
> > +                       dev_err(dev,                                           \
> > +                               capability_msg                                 \
> > +                               " version %ld is below expected %d",           \
> > +                               FIELD_GET(mask, data), expected);              \
> > +                       return;                                                \
> 
> Ugh, "return" in a macro, please no.
> 

Okay. If you're okay with the macro, I'll rewrite it to bool and then return
based on that.

> > +               }                                                              \
> > +       } while (0)
> > +
> > +       CAPID_VERSION_CHECK(cap_array, CXL_CM_CAP_HDR_VERSION_MASK,
> > +                           CM_CAP_HDR_CAP_VERSION, "Capability array header");
> > +       CAPID_VERSION_CHECK(cap_array, CXL_CM_CAP_HDR_CACHE_MEM_VERSION_MASK,
> > +                           CM_CAP_HDR_CACHE_MEM_VERSION,
> > +                           "Capability array header CXL.cache CXL.mem");
> > +
> > +       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;
> > +               u16 cap_id, offset;
> > +
> > +               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:
> > +                       CAPID_VERSION_CHECK(hdr, CXL_CM_CAP_HDR_VERSION_MASK,
> > +                                           CXL_CM_CAP_CAP_HDM_VERSION,
> > +                                           "HDM decoder capability");
> > +                       dev_dbg(dev, "found HDM decoder capability (0x%x)\n",
> > +                               offset);
> > +                       regs->hdm_decoder = register_block;
> > +                       break;
> > +               default:
> > +                       dev_dbg(dev, "Unknown CM cap ID: %d (0x%x)\n", cap_id,
> 
> Hex cap id? Perhaps rather than "Unknown" this can just say "Skipping"
> because some of these are technically it's just the kernel does not
> care.
> 

I just sent some patches for devices that I'd like to include for 5.12 to this
effect. I'll fix this here too.

> > +                               offset);
> > +                       break;
> > +               }
> > +       }
> > +}
> > +EXPORT_SYMBOL_GPL(cxl_setup_component_regs);
> > +
> >  /*
> >   * cxl_setup_device_regs() - Detect CXL Device register blocks
> >   * @dev: Host device of the @base mapping
> > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> > index 0211f44c95a2..a4ad1176dc5a 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
> > @@ -35,11 +60,26 @@
> >  #define CXLDEV_MBOX_PAYLOAD_OFFSET 0x20
> >
> >  /* See note for 'struct cxl_regs' for the rationale of this organization */
> > +#define CXL_COMPONENT_REGS() \
> > +       void __iomem *hdm_decoder
> > +
> >  #define CXL_DEVICE_REGS() \
> >         void __iomem *status; \
> >         void __iomem *mbox; \
> >         void __iomem *memdev
> >
> > +/**
> > + * struct cxl_component_regs - Common container of CXL component register block
> > + *                            base pointers.
> > + *
> > + * The only component registers that we care about are the CXL.cache and CXL.mem
> > + * registers which are at offset 0x1000 from the component register base (CXL
> > + * 2.0 8.2.4)
> > + */
> 
> I don't think kernel-doc will be happy about this, so probably follow
> the same fixup I did when Jonathan pointed this out for the cxl_port
> patches.
> 

I must have missed something on this. I thought the issue with yours was the way
in which you were calling out the structure members. I skipped this completely
in mine. Is the ask here to add "@hdm" to the documentation? I don't see any
other issue here.

> > +struct cxl_component_regs {
> > +       CXL_COMPONENT_REGS();
> > +};
> > +
> >  /**
> >   * struct cxl_device_regs - Common container of CXL Device register
> >   *                         block base pointers
> > @@ -59,6 +99,12 @@ struct cxl_device_regs {
> >   * The specificity reads naturally from left-to-right.
> >   */
> >  struct cxl_regs {
> > +       union {
> > +               struct {
> > +                       CXL_COMPONENT_REGS();
> > +               };
> > +               struct cxl_component_regs component;
> > +       };
> >         union {
> >                 struct {
> >                         CXL_DEVICE_REGS();
> > @@ -67,6 +113,8 @@ struct cxl_regs {
> >         };
> >  };
> >
> > +void cxl_setup_component_regs(struct device *dev, void __iomem *base,
> > +                             struct cxl_component_regs *regs);
> >  void cxl_setup_device_regs(struct device *dev, void __iomem *base,
> >                            struct cxl_device_regs *regs);
> >
> > diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> > index b7342aaf38c4..e915e3743b76 100644
> > --- a/drivers/cxl/mem.c
> > +++ b/drivers/cxl/mem.c
> > @@ -974,6 +974,21 @@ static int cxl_mem_dvsec(struct pci_dev *pdev, int dvsec)
> >         return 0;
> >  }
> >
> > +static int __cxl_setup_component_regs(struct cxl_mem *cxlm, void __iomem *base)
> > +{
> > +       struct cxl_regs *regs = &cxlm->regs;
> > +       struct pci_dev *pdev = cxlm->pdev;
> > +       struct device *dev = &pdev->dev;
> > +
> > +       cxl_setup_component_regs(dev, base, &regs->component);
> > +       if (!regs->hdm_decoder) {
> > +               dev_err(dev, "HDM decoder registers not found\n");
> > +               return -ENXIO;
> 
> I was hoping this patch would show the justification of the
> organization proposed in patch-6, but it doesn't and I think we're
> better off open coding the error check in cxl_mem_setup_regs().

Open coding is functionally equivalent and I think much less tidy, in particular
if you consider adding more register block types in the future (and
specifically, more component register types in the blocks, in the future). I
would predict that this kind of cleanup will happen at some point, however if
you'd prefer to kick the can down the road, we can do that.

If the heartburn is over the naming specifically, it would be my preference to
fix that rather than open-coding.

Addressing the feedback from patch 6 here, since they're tied together, "__"
prefix is, I want to call the core function but I have certain expectations
about the results of that function, and thus the wrapper (sounds like you got
that). TBF, the naming is backward as I'd prefer the core function be
__cxl_setup.., and this one be something like cxl_mem_setup_component_regs(). I
just didn't want to touch the thing you'd previously named. Perhaps that naming
more appropriately construes what I'm trying to do?

> 
> All the other patches in this series I did not comment on look good to me.

Re: [PATCH 7/7] cxl: Add HDM decoder capbilities

[Dan Williams]

On Thu, Apr 15, 2021 at 4:50 PM Ben Widawsky <ben.widawsky@intel.com> wrote:
>
> Thanks for looking. Mostly trivial agreements or disagreements which I don't
> care much about, but I'd really like you to focus on the last point, please.
>
> On 21-04-15 16:27:14, Dan Williams wrote:
> > On Wed, Apr 7, 2021 at 3:26 PM Ben Widawsky <ben.widawsky@intel.com> wrote:
> > >
> > > 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.
> >
> > It's always been possible and this patch just prepares for programming
> > them, but does not actually touch them yet. So let's drop this as I'm
> > not sure what it is saying.
> >
> > > Such
> > > programming is expected for CXL devices with persistent memory, and hot
> > > plugged CXL devices that participate in CXL.mem with volatile memory.
> >
> > Let's drop this as well because the state of what the OS is expected
> > to program is platform and topology specific. The OS needs to be
> > prepared for the HDMs to be unprogrammed, validly programmed in a
> > state the OS will leave untouched, and/or validly programmed in a way
> > the OS wants to redo.
> >
>
> Okay to both.
>
> > >
> > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> > > ---
> > >  drivers/cxl/core.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++
> > >  drivers/cxl/cxl.h  | 48 ++++++++++++++++++++++++++++++
> > >  drivers/cxl/mem.c  | 37 ++++++++++++++++++++---
> > >  drivers/cxl/pci.h  |  1 +
> > >  4 files changed, 155 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/cxl/core.c b/drivers/cxl/core.c
> > > index 65cd704581bc..db6a83eed0a2 100644
> > > --- a/drivers/cxl/core.c
> > > +++ b/drivers/cxl/core.c
> > > @@ -479,6 +479,79 @@ struct cxl_port *devm_cxl_add_port(struct device *host,
> > >  }
> > >  EXPORT_SYMBOL_GPL(devm_cxl_add_port);
> > >
> > > +void cxl_setup_component_regs(struct device *dev, void __iomem *base,
> > > +                             struct cxl_component_regs *regs)
> > > +{
> > > +       int cap, cap_count;
> > > +       u64 cap_array;
> > > +
> > > +       *regs = (struct cxl_component_regs) { 0 };
> > > +
> > > +       /*
> > > +        * CXL.cache and CXL.mem registers are at offset 0x1000 as defined in
> > > +        * CXL 2.0 8.2.4 Table 141.
> > > +        *
> > > +        * TODO: Map other registers as needed.
> >
> > That TODO goes without saying.
> >
>
> I don't agree with this. The device register implementation is complete as is,
> and this one is not.

...but there's a pile of stuff in the component registers that the
kernel will never care about. So if the bar is "remove when all
capabilities are mapped" then the TODO never gets removed.

>
> > > +        */
> > > +       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 */
> >
> > If future versions are backwards compatible then why check? If someone
> > wants a driver that supports new capabilities then update the driver,
> > but validating that the version is non-zero seems to be a pedantic
> > check for no good reason to me.
> >
>
> This was specifically meant as a way to prevent the driver being used for 1.1
> without explicit support being added (and then dropping this check).

Seems premature to assume this code path would be reused for 1.1, also
CXL 1.1 also puts version1 here. The difference between 1.1 and 2.0
component registers is more potential capability ids, not a different
capability version number if I'm reading the specs correctly.

> You're
> right though , it should be !=, it should have been <. It's sounding like you'd
> rather I remove it entirely rather than fix it though. Is that correct?
>
> > > +#define CAPID_VERSION_CHECK(data, mask, expected, capability_msg)              \
> > > +       do {                                                                   \
> > > +               if (FIELD_GET(mask, data) < expected) {                        \
> > > +                       dev_err(dev,                                           \
> > > +                               capability_msg                                 \
> > > +                               " version %ld is below expected %d",           \
> > > +                               FIELD_GET(mask, data), expected);              \
> > > +                       return;                                                \
> >
> > Ugh, "return" in a macro, please no.
> >
>
> Okay. If you're okay with the macro, I'll rewrite it to bool and then return
> based on that.

No, per above, I still don't think this is adding any value.

>
> > > +               }                                                              \
> > > +       } while (0)
> > > +
> > > +       CAPID_VERSION_CHECK(cap_array, CXL_CM_CAP_HDR_VERSION_MASK,
> > > +                           CM_CAP_HDR_CAP_VERSION, "Capability array header");
> > > +       CAPID_VERSION_CHECK(cap_array, CXL_CM_CAP_HDR_CACHE_MEM_VERSION_MASK,
> > > +                           CM_CAP_HDR_CACHE_MEM_VERSION,
> > > +                           "Capability array header CXL.cache CXL.mem");
> > > +
> > > +       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;
> > > +               u16 cap_id, offset;
> > > +
> > > +               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:
> > > +                       CAPID_VERSION_CHECK(hdr, CXL_CM_CAP_HDR_VERSION_MASK,
> > > +                                           CXL_CM_CAP_CAP_HDM_VERSION,
> > > +                                           "HDM decoder capability");
> > > +                       dev_dbg(dev, "found HDM decoder capability (0x%x)\n",
> > > +                               offset);
> > > +                       regs->hdm_decoder = register_block;
> > > +                       break;
> > > +               default:
> > > +                       dev_dbg(dev, "Unknown CM cap ID: %d (0x%x)\n", cap_id,
> >
> > Hex cap id? Perhaps rather than "Unknown" this can just say "Skipping"
> > because some of these are technically it's just the kernel does not
> > care.
> >
>
> I just sent some patches for devices that I'd like to include for 5.12 to this
> effect. I'll fix this here too.
>
> > > +                               offset);
> > > +                       break;
> > > +               }
> > > +       }
> > > +}
> > > +EXPORT_SYMBOL_GPL(cxl_setup_component_regs);
> > > +
> > >  /*
> > >   * cxl_setup_device_regs() - Detect CXL Device register blocks
> > >   * @dev: Host device of the @base mapping
> > > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> > > index 0211f44c95a2..a4ad1176dc5a 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
> > > @@ -35,11 +60,26 @@
> > >  #define CXLDEV_MBOX_PAYLOAD_OFFSET 0x20
> > >
> > >  /* See note for 'struct cxl_regs' for the rationale of this organization */
> > > +#define CXL_COMPONENT_REGS() \
> > > +       void __iomem *hdm_decoder
> > > +
> > >  #define CXL_DEVICE_REGS() \
> > >         void __iomem *status; \
> > >         void __iomem *mbox; \
> > >         void __iomem *memdev
> > >
> > > +/**
> > > + * struct cxl_component_regs - Common container of CXL component register block
> > > + *                            base pointers.
> > > + *
> > > + * The only component registers that we care about are the CXL.cache and CXL.mem
> > > + * registers which are at offset 0x1000 from the component register base (CXL
> > > + * 2.0 8.2.4)
> > > + */
> >
> > I don't think kernel-doc will be happy about this, so probably follow
> > the same fixup I did when Jonathan pointed this out for the cxl_port
> > patches.
> >
>
> I must have missed something on this. I thought the issue with yours was the way
> in which you were calling out the structure members. I skipped this completely
> in mine. Is the ask here to add "@hdm" to the documentation? I don't see any
> other issue here.

The issue is that this is a struct entry with no description of the
members. I ended up doing this for device registers:

/*
 * 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_DEVICE_REGS() \
        void __iomem *status; \
        void __iomem *mbox; \
        void __iomem *memdev

/* See note for 'struct cxl_regs' for the rationale of this organization */
struct cxl_device_regs {
        CXL_DEVICE_REGS();
};

...i.e. just abandon kernel-doc for this organization, but this might
be more evidence that this arrangement is more clever than its worth.

>
> > > +struct cxl_component_regs {
> > > +       CXL_COMPONENT_REGS();
> > > +};
> > > +
> > >  /**
> > >   * struct cxl_device_regs - Common container of CXL Device register
> > >   *                         block base pointers
> > > @@ -59,6 +99,12 @@ struct cxl_device_regs {
> > >   * The specificity reads naturally from left-to-right.
> > >   */
> > >  struct cxl_regs {
> > > +       union {
> > > +               struct {
> > > +                       CXL_COMPONENT_REGS();
> > > +               };
> > > +               struct cxl_component_regs component;
> > > +       };
> > >         union {
> > >                 struct {
> > >                         CXL_DEVICE_REGS();
> > > @@ -67,6 +113,8 @@ struct cxl_regs {
> > >         };
> > >  };
> > >
> > > +void cxl_setup_component_regs(struct device *dev, void __iomem *base,
> > > +                             struct cxl_component_regs *regs);
> > >  void cxl_setup_device_regs(struct device *dev, void __iomem *base,
> > >                            struct cxl_device_regs *regs);
> > >
> > > diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> > > index b7342aaf38c4..e915e3743b76 100644
> > > --- a/drivers/cxl/mem.c
> > > +++ b/drivers/cxl/mem.c
> > > @@ -974,6 +974,21 @@ static int cxl_mem_dvsec(struct pci_dev *pdev, int dvsec)
> > >         return 0;
> > >  }
> > >
> > > +static int __cxl_setup_component_regs(struct cxl_mem *cxlm, void __iomem *base)
> > > +{
> > > +       struct cxl_regs *regs = &cxlm->regs;
> > > +       struct pci_dev *pdev = cxlm->pdev;
> > > +       struct device *dev = &pdev->dev;
> > > +
> > > +       cxl_setup_component_regs(dev, base, &regs->component);
> > > +       if (!regs->hdm_decoder) {
> > > +               dev_err(dev, "HDM decoder registers not found\n");
> > > +               return -ENXIO;
> >
> > I was hoping this patch would show the justification of the
> > organization proposed in patch-6, but it doesn't and I think we're
> > better off open coding the error check in cxl_mem_setup_regs().
>
> Open coding is functionally equivalent and I think much less tidy, in particular
> if you consider adding more register block types in the future

I'm not seeing much register block type support being added in the
near term beyond HDMs.

> (and
> specifically, more component register types in the blocks, in the future). I
> would predict that this kind of cleanup will happen at some point, however if
> you'd prefer to kick the can down the road, we can do that.
>
> If the heartburn is over the naming specifically, it would be my preference to
> fix that rather than open-coding.

It just feels like the helpers here are too tiny to provide any
significant cleanup.

>
> Addressing the feedback from patch 6 here, since they're tied together, "__"
> prefix is, I want to call the core function but I have certain expectations
> about the results of that function, and thus the wrapper (sounds like you got
> that). TBF, the naming is backward as I'd prefer the core function be
> __cxl_setup.., and this one be something like cxl_mem_setup_component_regs(). I
> just didn't want to touch the thing you'd previously named. Perhaps that naming
> more appropriately construes what I'm trying to do?

I'm open to better naming, but not __func() wrapping func().