diff mbox series

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

Message ID 20210407222625.320177-4-ben.widawsky@intel.com
State Accepted
Commit 1d5a4159074bde1b2d5e4a6f5ed34de70a83a39f
Headers show
Series Enumerate HDM Decoder registers | expand

Commit Message

Ben Widawsky April 7, 2021, 10:26 p.m. UTC 
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(-)

Comments

Jonathan Cameron April 8, 2021, 5:27 p.m. UTC | #1 
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;
diff mbox series

Patch

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;