[PATCH 1/3] cxl/pci: Rename cxl_setup_parent_dport() and cxl_dport_map_regs()

[PATCH 1/3] cxl/pci: Rename cxl_setup_parent_dport() and cxl_dport_map_regs()

[Li Ming]

The name of cxl_setup_parent_dport() function is not clear, the function
is used to initialize AER and RAS capabilities on a dport, therefore,
rename the function to cxl_dport_init_aer(), it is easier for user to
understand what the function does. Besides, adjust the order of the
function parameters, the subject of cxl_dport_init_aer() is a cxl port,
so a struct cxl_dport as the first parameter of the function should be
better.

cxl_dport_map_regs() is used to map CXL RAS capability on a cxl dport,
using cxl_dport_map_ras() as the function name.

Signed-off-by: Li Ming <ming4.li@intel.com>
---
 drivers/cxl/core/pci.c        | 13 +++++++++----
 drivers/cxl/cxl.h             |  6 +++---
 drivers/cxl/mem.c             |  2 +-
 tools/testing/cxl/Kbuild      |  2 +-
 tools/testing/cxl/test/mock.c |  6 +++---
 5 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index 51132a575b27..9761178372b6 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -787,7 +787,7 @@ static void cxl_dport_map_rch_aer(struct cxl_dport *dport)
 	dport->regs.dport_aer = dport_aer;
 }
 
-static void cxl_dport_map_regs(struct cxl_dport *dport)
+static void cxl_dport_map_ras(struct cxl_dport *dport)
 {
 	struct cxl_register_map *map = &dport->reg_map;
 	struct device *dev = dport->dport_dev;
@@ -831,7 +831,12 @@ static void cxl_disable_rch_root_ints(struct cxl_dport *dport)
 	}
 }
 
-void cxl_setup_parent_dport(struct device *host, struct cxl_dport *dport)
+/**
+ * cxl_dport_init_aer - Setup AER on this dport
+ * @dport: the cxl_dport that needs to be initialized
+ * @host: host device for devm operations
+ */
+void cxl_dport_init_aer(struct cxl_dport *dport, struct device *host)
 {
 	struct device *dport_dev = dport->dport_dev;
 
@@ -843,12 +848,12 @@ void cxl_setup_parent_dport(struct device *host, struct cxl_dport *dport)
 	}
 
 	dport->reg_map.host = host;
-	cxl_dport_map_regs(dport);
+	cxl_dport_map_ras(dport);
 
 	if (dport->rch)
 		cxl_disable_rch_root_ints(dport);
 }
-EXPORT_SYMBOL_NS_GPL(cxl_setup_parent_dport, CXL);
+EXPORT_SYMBOL_NS_GPL(cxl_dport_init_aer, CXL);
 
 static void cxl_handle_rdport_cor_ras(struct cxl_dev_state *cxlds,
 					  struct cxl_dport *dport)
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 9afb407d438f..c4b7a0d6bb22 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -761,10 +761,10 @@ struct cxl_dport *devm_cxl_add_rch_dport(struct cxl_port *port,
 					 resource_size_t rcrb);
 
 #ifdef CONFIG_PCIEAER_CXL
-void cxl_setup_parent_dport(struct device *host, struct cxl_dport *dport);
+void cxl_dport_init_aer(struct cxl_dport *dport, struct device *host);
 #else
-static inline void cxl_setup_parent_dport(struct device *host,
-					  struct cxl_dport *dport) { }
+static inline void cxl_dport_init_aer(struct cxl_dport *dport,
+				      struct device *host) { }
 #endif
 
 struct cxl_decoder *to_cxl_decoder(struct device *dev);
diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
index 7de232eaeb17..e464ec3681bb 100644
--- a/drivers/cxl/mem.c
+++ b/drivers/cxl/mem.c
@@ -166,7 +166,7 @@ static int cxl_mem_probe(struct device *dev)
 	else
 		endpoint_parent = &parent_port->dev;
 
-	cxl_setup_parent_dport(dev, dport);
+	cxl_dport_init_aer(dport, dev);
 
 	device_lock(endpoint_parent);
 	if (!endpoint_parent->driver) {
diff --git a/tools/testing/cxl/Kbuild b/tools/testing/cxl/Kbuild
index 3d1ca9e38b1f..f9eb12eb3f1b 100644
--- a/tools/testing/cxl/Kbuild
+++ b/tools/testing/cxl/Kbuild
@@ -14,7 +14,7 @@ ldflags-y += --wrap=cxl_dvsec_rr_decode
 ldflags-y += --wrap=devm_cxl_add_rch_dport
 ldflags-y += --wrap=cxl_rcd_component_reg_phys
 ldflags-y += --wrap=cxl_endpoint_parse_cdat
-ldflags-y += --wrap=cxl_setup_parent_dport
+ldflags-y += --wrap=cxl_dport_init_aer
 
 DRIVERS := ../../../drivers
 CXL_SRC := $(DRIVERS)/cxl
diff --git a/tools/testing/cxl/test/mock.c b/tools/testing/cxl/test/mock.c
index d619672faa49..6cf89e16fa44 100644
--- a/tools/testing/cxl/test/mock.c
+++ b/tools/testing/cxl/test/mock.c
@@ -299,17 +299,17 @@ void __wrap_cxl_endpoint_parse_cdat(struct cxl_port *port)
 }
 EXPORT_SYMBOL_NS_GPL(__wrap_cxl_endpoint_parse_cdat, CXL);
 
-void __wrap_cxl_setup_parent_dport(struct device *host, struct cxl_dport *dport)
+void __wrap_cxl_dport_init_aer(struct cxl_dport *dport, struct device *host)
 {
 	int index;
 	struct cxl_mock_ops *ops = get_cxl_mock_ops(&index);
 
 	if (!ops || !ops->is_mock_port(dport->dport_dev))
-		cxl_setup_parent_dport(host, dport);
+		cxl_dport_init_aer(dport, host);
 
 	put_cxl_mock_ops(index);
 }
-EXPORT_SYMBOL_NS_GPL(__wrap_cxl_setup_parent_dport, CXL);
+EXPORT_SYMBOL_NS_GPL(__wrap_cxl_dport_init_aer, CXL);
 
 MODULE_LICENSE("GPL v2");
 MODULE_IMPORT_NS(ACPI);
-- 
2.40.1

[PATCH 2/3] cxl/pci: cxl_dport_map_rch_aer() cleanup

[Li Ming]

cxl_dport_map_ras() is used to map CXL RAS capability, the RCH AER
capability should not be mapped in the function but should mapped in
cxl_dport_init_aer(). Moving cxl_dport_map_ras() out of
cxl_dport_map_ras() and into cxl_dport_init_aer().

In cxl_dport_init_aer(), the AER capability position in RCRB will be
located but the position is only used in cxl_dport_map_rch_aer(),
getting the position in cxl_dport_map_rch_aer() rather than
cxl_dport_init_aer() is more reasonable and makes the code clearer.

Besides, some local variables in cxl_dport_map_rch_aer() are
unnecessary, remove them to make the function more concise.

Signed-off-by: Li Ming <ming4.li@intel.com>
---
 drivers/cxl/core/pci.c | 33 +++++++++++++--------------------
 1 file changed, 13 insertions(+), 20 deletions(-)

diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index 9761178372b6..118db6a577d7 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -772,19 +772,17 @@ static bool cxl_handle_endpoint_ras(struct cxl_dev_state *cxlds)
 
 static void cxl_dport_map_rch_aer(struct cxl_dport *dport)
 {
-	struct cxl_rcrb_info *ri = &dport->rcrb;
-	void __iomem *dport_aer = NULL;
 	resource_size_t aer_phys;
 	struct device *host;
+	u16 aer_cap;
 
-	if (dport->rch && ri->aer_cap) {
+	aer_cap = cxl_rcrb_to_aer(dport->dport_dev, dport->rcrb.base);
+	if (aer_cap) {
 		host = dport->reg_map.host;
-		aer_phys = ri->aer_cap + ri->base;
-		dport_aer = devm_cxl_iomap_block(host, aer_phys,
-				sizeof(struct aer_capability_regs));
+		aer_phys = aer_cap + dport->rcrb.base;
+		dport->regs.dport_aer = devm_cxl_iomap_block(host, aer_phys,
+						sizeof(struct aer_capability_regs));
 	}
-
-	dport->regs.dport_aer = dport_aer;
 }
 
 static void cxl_dport_map_ras(struct cxl_dport *dport)
@@ -797,9 +795,6 @@ static void cxl_dport_map_ras(struct cxl_dport *dport)
 	else if (cxl_map_component_regs(map, &dport->regs.component,
 					BIT(CXL_CM_CAP_CAP_ID_RAS)))
 		dev_dbg(dev, "Failed to map RAS capability.\n");
-
-	if (dport->rch)
-		cxl_dport_map_rch_aer(dport);
 }
 
 static void cxl_disable_rch_root_ints(struct cxl_dport *dport)
@@ -838,20 +833,18 @@ static void cxl_disable_rch_root_ints(struct cxl_dport *dport)
  */
 void cxl_dport_init_aer(struct cxl_dport *dport, struct device *host)
 {
-	struct device *dport_dev = dport->dport_dev;
+	dport->reg_map.host = host;
+	cxl_dport_map_ras(dport);
 
 	if (dport->rch) {
-		struct pci_host_bridge *host_bridge = to_pci_host_bridge(dport_dev);
-
-		if (host_bridge->native_aer)
-			dport->rcrb.aer_cap = cxl_rcrb_to_aer(dport_dev, dport->rcrb.base);
-	}
+		struct pci_host_bridge *host_bridge = to_pci_host_bridge(dport->dport_dev);
 
-	dport->reg_map.host = host;
-	cxl_dport_map_ras(dport);
+		if (!host_bridge->native_aer)
+			return;
 
-	if (dport->rch)
+		cxl_dport_map_rch_aer(dport);
 		cxl_disable_rch_root_ints(dport);
+	}
 }
 EXPORT_SYMBOL_NS_GPL(cxl_dport_init_aer, CXL);
 
-- 
2.40.1

Re: [PATCH 2/3] cxl/pci: cxl_dport_map_rch_aer() cleanup

[Jonathan Cameron]

On Tue, 27 Aug 2024 04:57:54 +0000
Li Ming <ming4.li@intel.com> wrote:

> cxl_dport_map_ras() is used to map CXL RAS capability, the RCH AER
> capability should not be mapped in the function but should mapped in
> cxl_dport_init_aer(). Moving cxl_dport_map_ras() out of
> cxl_dport_map_ras() and into cxl_dport_init_aer().
> 
> In cxl_dport_init_aer(), the AER capability position in RCRB will be
> located but the position is only used in cxl_dport_map_rch_aer(),
> getting the position in cxl_dport_map_rch_aer() rather than
> cxl_dport_init_aer() is more reasonable and makes the code clearer.
> 
> Besides, some local variables in cxl_dport_map_rch_aer() are
> unnecessary, remove them to make the function more concise.
> 
> Signed-off-by: Li Ming <ming4.li@intel.com>
Seems sensible
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

[PATCH 3/3] cxl/pci: Remove reduplicate host_bridge->native_aer checking

[Li Ming]

cxl_dport_init_aer() already checks host_bridge->native_aer before
invoking cxl_disable_rch_root_ints(), so cxl_disable_rch_root_ints()
does not need to check it again.

Signed-off-by: Li Ming <ming4.li@intel.com>
---
 drivers/cxl/core/pci.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index 118db6a577d7..7234166df0df 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -800,14 +800,11 @@ static void cxl_dport_map_ras(struct cxl_dport *dport)
 static void cxl_disable_rch_root_ints(struct cxl_dport *dport)
 {
 	void __iomem *aer_base = dport->regs.dport_aer;
-	struct pci_host_bridge *bridge;
 	u32 aer_cmd_mask, aer_cmd;
 
 	if (!aer_base)
 		return;
 
-	bridge = to_pci_host_bridge(dport->dport_dev);
-
 	/*
 	 * Disable RCH root port command interrupts.
 	 * CXL 3.0 12.2.1.1 - RCH Downstream Port-detected Errors
@@ -816,14 +813,12 @@ static void cxl_disable_rch_root_ints(struct cxl_dport *dport)
 	 * the root cmd register's interrupts is required. But, PCI spec
 	 * shows these are disabled by default on reset.
 	 */
-	if (bridge->native_aer) {
-		aer_cmd_mask = (PCI_ERR_ROOT_CMD_COR_EN |
-				PCI_ERR_ROOT_CMD_NONFATAL_EN |
-				PCI_ERR_ROOT_CMD_FATAL_EN);
-		aer_cmd = readl(aer_base + PCI_ERR_ROOT_COMMAND);
-		aer_cmd &= ~aer_cmd_mask;
-		writel(aer_cmd, aer_base + PCI_ERR_ROOT_COMMAND);
-	}
+	aer_cmd_mask = (PCI_ERR_ROOT_CMD_COR_EN |
+			PCI_ERR_ROOT_CMD_NONFATAL_EN |
+			PCI_ERR_ROOT_CMD_FATAL_EN);
+	aer_cmd = readl(aer_base + PCI_ERR_ROOT_COMMAND);
+	aer_cmd &= ~aer_cmd_mask;
+	writel(aer_cmd, aer_base + PCI_ERR_ROOT_COMMAND);
 }
 
 /**
-- 
2.40.1

Re: [PATCH 3/3] cxl/pci: Remove reduplicate host_bridge->native_aer checking

[Jonathan Cameron]

On Tue, 27 Aug 2024 04:57:55 +0000
Li Ming <ming4.li@intel.com> wrote:

> cxl_dport_init_aer() already checks host_bridge->native_aer before
> invoking cxl_disable_rch_root_ints(), so cxl_disable_rch_root_ints()
> does not need to check it again.
> 
> Signed-off-by: Li Ming <ming4.li@intel.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Re: [PATCH 3/3] cxl/pci: Remove reduplicate host_bridge->native_aer checking

[Jonathan Cameron]

On Tue, 27 Aug 2024 04:57:55 +0000
Li Ming <ming4.li@intel.com> wrote:

For title, just duplicate (not reduplicate)


> cxl_dport_init_aer() already checks host_bridge->native_aer before
> invoking cxl_disable_rch_root_ints(), so cxl_disable_rch_root_ints()
> does not need to check it again.
> 
> Signed-off-by: Li Ming <ming4.li@intel.com>
> ---
>  drivers/cxl/core/pci.c | 17 ++++++-----------
>  1 file changed, 6 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index 118db6a577d7..7234166df0df 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -800,14 +800,11 @@ static void cxl_dport_map_ras(struct cxl_dport *dport)
>  static void cxl_disable_rch_root_ints(struct cxl_dport *dport)
>  {
>  	void __iomem *aer_base = dport->regs.dport_aer;
> -	struct pci_host_bridge *bridge;
>  	u32 aer_cmd_mask, aer_cmd;
>  
>  	if (!aer_base)
>  		return;
>  
> -	bridge = to_pci_host_bridge(dport->dport_dev);
> -
>  	/*
>  	 * Disable RCH root port command interrupts.
>  	 * CXL 3.0 12.2.1.1 - RCH Downstream Port-detected Errors
> @@ -816,14 +813,12 @@ static void cxl_disable_rch_root_ints(struct cxl_dport *dport)
>  	 * the root cmd register's interrupts is required. But, PCI spec
>  	 * shows these are disabled by default on reset.
>  	 */
> -	if (bridge->native_aer) {
> -		aer_cmd_mask = (PCI_ERR_ROOT_CMD_COR_EN |
> -				PCI_ERR_ROOT_CMD_NONFATAL_EN |
> -				PCI_ERR_ROOT_CMD_FATAL_EN);
> -		aer_cmd = readl(aer_base + PCI_ERR_ROOT_COMMAND);
> -		aer_cmd &= ~aer_cmd_mask;
> -		writel(aer_cmd, aer_base + PCI_ERR_ROOT_COMMAND);
> -	}
> +	aer_cmd_mask = (PCI_ERR_ROOT_CMD_COR_EN |
> +			PCI_ERR_ROOT_CMD_NONFATAL_EN |
> +			PCI_ERR_ROOT_CMD_FATAL_EN);
> +	aer_cmd = readl(aer_base + PCI_ERR_ROOT_COMMAND);
> +	aer_cmd &= ~aer_cmd_mask;
> +	writel(aer_cmd, aer_base + PCI_ERR_ROOT_COMMAND);
>  }
>  
>  /**

Re: [PATCH 3/3] cxl/pci: Remove reduplicate host_bridge->native_aer checking

[Li, Ming4]

On 8/28/2024 4:21 PM, Jonathan Cameron wrote:
> On Tue, 27 Aug 2024 04:57:55 +0000
> Li Ming <ming4.li@intel.com> wrote:
>
> For title, just duplicate (not reduplicate)

Got it, thanks.


>
>> cxl_dport_init_aer() already checks host_bridge->native_aer before
>> invoking cxl_disable_rch_root_ints(), so cxl_disable_rch_root_ints()
>> does not need to check it again.
>>
>> Signed-off-by: Li Ming <ming4.li@intel.com>
>> ---
>>  drivers/cxl/core/pci.c | 17 ++++++-----------
>>  1 file changed, 6 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
>> index 118db6a577d7..7234166df0df 100644
>> --- a/drivers/cxl/core/pci.c
>> +++ b/drivers/cxl/core/pci.c
>> @@ -800,14 +800,11 @@ static void cxl_dport_map_ras(struct cxl_dport *dport)
>>  static void cxl_disable_rch_root_ints(struct cxl_dport *dport)
>>  {
>>  	void __iomem *aer_base = dport->regs.dport_aer;
>> -	struct pci_host_bridge *bridge;
>>  	u32 aer_cmd_mask, aer_cmd;
>>  
>>  	if (!aer_base)
>>  		return;
>>  
>> -	bridge = to_pci_host_bridge(dport->dport_dev);
>> -
>>  	/*
>>  	 * Disable RCH root port command interrupts.
>>  	 * CXL 3.0 12.2.1.1 - RCH Downstream Port-detected Errors
>> @@ -816,14 +813,12 @@ static void cxl_disable_rch_root_ints(struct cxl_dport *dport)
>>  	 * the root cmd register's interrupts is required. But, PCI spec
>>  	 * shows these are disabled by default on reset.
>>  	 */
>> -	if (bridge->native_aer) {
>> -		aer_cmd_mask = (PCI_ERR_ROOT_CMD_COR_EN |
>> -				PCI_ERR_ROOT_CMD_NONFATAL_EN |
>> -				PCI_ERR_ROOT_CMD_FATAL_EN);
>> -		aer_cmd = readl(aer_base + PCI_ERR_ROOT_COMMAND);
>> -		aer_cmd &= ~aer_cmd_mask;
>> -		writel(aer_cmd, aer_base + PCI_ERR_ROOT_COMMAND);
>> -	}
>> +	aer_cmd_mask = (PCI_ERR_ROOT_CMD_COR_EN |
>> +			PCI_ERR_ROOT_CMD_NONFATAL_EN |
>> +			PCI_ERR_ROOT_CMD_FATAL_EN);
>> +	aer_cmd = readl(aer_base + PCI_ERR_ROOT_COMMAND);
>> +	aer_cmd &= ~aer_cmd_mask;
>> +	writel(aer_cmd, aer_base + PCI_ERR_ROOT_COMMAND);
>>  }
>>  
>>  /**
>

Re: [PATCH 1/3] cxl/pci: Rename cxl_setup_parent_dport() and cxl_dport_map_regs()

[Jonathan Cameron]

On Tue, 27 Aug 2024 04:57:53 +0000
Li Ming <ming4.li@intel.com> wrote:

> The name of cxl_setup_parent_dport() function is not clear, the function
> is used to initialize AER and RAS capabilities on a dport, therefore,
> rename the function to cxl_dport_init_aer(), it is easier for user to

> understand what the function does. Besides, adjust the order of the
> function parameters, the subject of cxl_dport_init_aer() is a cxl port,
Hmm. It's not just aer, so maybe cxl_dport_init_ras_reporting() is broader
naming that incorporates the fact this includes the CXL specific stuff.
Obvious that might be used to get more detail on an AER error report,
but it's not part of aer.

Don't want to just use _ras() because of all the ras control stuff that
isn't related to this function.

Otherwise LGTM.

Jonathan

> so a struct cxl_dport as the first parameter of the function should be
> better.
> 
> cxl_dport_map_regs() is used to map CXL RAS capability on a cxl dport,
> using cxl_dport_map_ras() as the function name.
> 
> Signed-off-by: Li Ming <ming4.li@intel.com>

Re: [PATCH 1/3] cxl/pci: Rename cxl_setup_parent_dport() and cxl_dport_map_regs()

[Li, Ming4]

On 8/27/2024 10:56 PM, Jonathan Cameron wrote:
> On Tue, 27 Aug 2024 04:57:53 +0000
> Li Ming <ming4.li@intel.com> wrote:
>
>> The name of cxl_setup_parent_dport() function is not clear, the function
>> is used to initialize AER and RAS capabilities on a dport, therefore,
>> rename the function to cxl_dport_init_aer(), it is easier for user to
>> understand what the function does. Besides, adjust the order of the
>> function parameters, the subject of cxl_dport_init_aer() is a cxl port,
> Hmm. It's not just aer, so maybe cxl_dport_init_ras_reporting() is broader
> naming that incorporates the fact this includes the CXL specific stuff.
> Obvious that might be used to get more detail on an AER error report,
> but it's not part of aer.
>
> Don't want to just use _ras() because of all the ras control stuff that
> isn't related to this function.
>
> Otherwise LGTM.
>
> Jonathan

Thank you for review, will do it in v2.


>> so a struct cxl_dport as the first parameter of the function should be
>> better.
>>
>> cxl_dport_map_regs() is used to map CXL RAS capability on a cxl dport,
>> using cxl_dport_map_ras() as the function name.
>>
>> Signed-off-by: Li Ming <ming4.li@intel.com>