[PATCH v1 0/1] Add CXL Reset Support for CXL Devices

[PATCH v1 0/1] Add CXL Reset Support for CXL Devices

[Srirangan Madhavan]

This patch introduces support for the CXL Reset method
for CXL devices, implementing the reset procedure outlined in
CXL Spec [1] v3.1, Sections 9.6 and 9.7.

RFC -> v1:
- Added cover letter and dropped the RFC.

The RFC patche can be found here [2]

Motivation:
-----------
This change is broadly useful for reasons including but not limited to the
following:
- As support for Type 2 devices [3] is being introduced, more devices will
  require finer-grained reset mechanisms beyond bus-wide reset methods.
- FLR does not affect CXL.cache or CXL.mem protocols, making CXL Reset
  the preferred method in some cases.
- The CXL spec (Sections 7.2.3 Binding and Unbinding, 9.5 FLR) highlights use
  cases like function rebinding and error recovery, where CXL Reset is
  explicitly mentioned.

Change Description:
-------------------
The patch implements the required steps for CXL reset and introduces a new reset
method in the pci_reset_fn_methods. It also defines the necessary CXL DVSEC
register macros. The actual steps for rest are broadly: disable cache lines and
asserts WB+I, wait for cache and memory clear signals, initial reset, wait for
complete and return status.

Command line to test the CXL reset on a capable device:
    echo cxl_reset > /sys/bus/pci/devices/<pci_device>/reset_method
    echo 1 > /sys/bus/pci/devices/<pci_device>/reset

[1] https://computeexpresslink.org/wp-content/uploads/2024/02/CXL-3.1-Specification.pdf
[2] https://lore.kernel.org/all/20241213074143.374-1-smadhavan@nvidia.com/
[3] https://lore.kernel.org/linux-cxl/20241216161042.42108-1-alejandro.lucero-palau@amd.com/

Srirangan Madhavan (1):
  cxl: add support for cxl reset

 drivers/pci/pci.c             | 183 ++++++++++++++++++++++++++++++++++
 include/linux/pci.h           |   2 +-
 include/uapi/linux/pci_regs.h |  25 +++++
 3 files changed, 209 insertions(+), 1 deletion(-)

-- 
2.25.1

[PATCH v1 1/1] cxl: add support for cxl reset

[Srirangan Madhavan]

This change adds the support and implements the CXL reset
steps as laid out by the CXL Spec v3.1 Sections 9.6 & 9.7.

With support for Type 2 devices being introduced, more devices will
require finer-grained reset mechanisms beyond bus-wide reset methods.

This change defines the necessary CXL DVSEC register macros.
For devices that support CXL Reset, cache lines are disabled, WB+I is
asserted, wait for cache invalid status, Mem Clr bit is asserted and
finally reset is initiated.

Signed-off-by: Srirangan Madhavan <smadhavan@nvidia.com>
---
 drivers/pci/pci.c             | 183 ++++++++++++++++++++++++++++++++++
 include/linux/pci.h           |   2 +-
 include/uapi/linux/pci_regs.h |  25 +++++
 3 files changed, 209 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 869d204a70a3..cf6009f5bd6c 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -5026,6 +5026,12 @@ static int pci_dev_reset_slot_function(struct pci_dev *dev, bool probe)
 	return pci_reset_hotplug_slot(dev->slot->hotplug, probe);
 }
 
+static u16 cxl_device_dvsec(struct pci_dev *dev)
+{
+	return pci_find_dvsec_capability(dev, PCI_VENDOR_ID_CXL,
+					 PCI_DVSEC_CXL_DEV);
+}
+
 static u16 cxl_port_dvsec(struct pci_dev *dev)
 {
 	return pci_find_dvsec_capability(dev, PCI_VENDOR_ID_CXL,
@@ -5116,6 +5122,182 @@ static int cxl_reset_bus_function(struct pci_dev *dev, bool probe)
 	return rc;
 }
 
+static int cxl_reset_prepare(struct pci_dev *dev, u16 dvsec)
+{
+	u16 reg, val, cap;
+	int rc;
+	u32 timeout_us = 100, timeout_tot_us = 10000;
+
+	/*
+	 * Wait for any pending transactions.
+	 * Assuming this does cxl.io stuff.
+	 */
+	if (!pci_wait_for_pending_transaction(dev))
+		pci_err(dev, "timed out waiting for pending transaction; performing cxl reset anyway\n");
+
+	/*
+	 * Disable caching and then write back and invalidate lines.
+	 */
+	rc = pci_read_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCAP,
+				  &cap);
+	if (rc)
+		return rc;
+
+	if (!(cap & PCI_DVSEC_CXL_DEVCAP_CACHE_CAPABLE))
+		return 0;
+
+	/*
+	 * Disable cache.
+	 * WB and invalidate cahce if capability is advertised.
+	 */
+	rc = pci_read_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCTL2,
+				  &reg);
+	if (rc)
+		return rc;
+	val = reg | PCI_DVSEC_CXL_DEVCTL2_DISABLE_CACHING;
+
+	if (cap & PCI_DVSEC_CXL_DEVCAP_CACHE_WB_INVALIDATE)
+		val = reg | PCI_DVSEC_CXL_DEVCTL2_INIT_CACHE_WB_INVALIDATE;
+	pci_write_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCTL2,
+			      val);
+
+	/*
+	 * From Section 9.6: "Software may leverage the cache size reported in
+	 * the DVSEC CXL Capability2 register to compute a suitable timeout
+	 * value".
+	 * Given there is no conversion factor for cache size -> timeout,
+	 * setting timer for default 10ms.
+	 */
+	do {
+		if (timeout_tot_us < 0)
+			return -ETIMEDOUT;
+		usleep_range(timeout_us, timeout_us+1);
+		timeout_tot_us -= timeout_us;
+		rc = pci_read_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCTL2,
+					  &reg);
+		if (rc)
+			return rc;
+	} while (!(reg & PCI_DVSEC_CXL_DEVSTATUS2_CACHE_INVALID));
+
+	return 0;
+}
+
+/**
+ * cxl_reset_init - initiate a cxl reset
+ * @dev: device to reset
+ *
+ * Initiate a cxl reset.
+ */
+static int cxl_reset_init(struct pci_dev *dev, u16 dvsec)
+{
+	u16 reg, val;
+	u32 timeout_ms;
+	int rc;
+	u32 reset_timeouts_ms[] = {10, 100, 1000, 10000, 100000};
+
+	/*
+	 * Check if CXL Reset MEM CLR is supported.
+	 */
+	rc = pci_read_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCAP,
+				  &reg);
+	if (rc)
+		return rc;
+
+	if (reg & PCI_DVSEC_CXL_DEVCAP_CXL_RST_MEM_CLR) {
+		rc = pci_read_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCTL2,
+					  &reg);
+		if (rc)
+			return rc;
+
+		val = reg | PCI_DVSEC_CXL_DEVCTL2_CXL_RST_MEM_CLR_ENABLE;
+		pci_write_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCTL2,
+				      val);
+	}
+
+	/*
+	 * Read timeout value
+	 */
+	rc = pci_read_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCAP,
+				  &reg);
+	if (rc)
+		return rc;
+	timeout_ms = reset_timeouts_ms[FIELD_GET(PCI_DVSEC_CXL_DEVCAP_CXL_RST_TIMEOUT_MASK, reg)];
+
+	/*
+	 * Write reset config
+	 */
+	rc = pci_read_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCTL2,
+				  &reg);
+	if (rc)
+		return rc;
+
+	val = reg | PCI_DVSEC_CXL_DEVCTL2_CXL_INIT_RST;
+	pci_write_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCTL2,
+			      val);
+
+	/*
+	 *  Wait till timeout and then check reset status is complete.
+	 */
+	msleep(timeout_ms);
+	rc = pci_read_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVSTATUS2,
+				  &reg);
+	if (rc)
+		return rc;
+	if (reg & PCI_DVSEC_CXL_DEVSTATUS2_RST_ERR ||
+	    ~reg & PCI_DVSEC_CXL_DEVSTATUS2_RST_COMPLETE)
+		return -ETIMEDOUT;
+
+	/*
+	 * Revert cashing disable.
+	 */
+	rc = pci_read_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCTL2,
+				  &reg);
+	if (rc)
+		return rc;
+	val = (reg & (~PCI_DVSEC_CXL_DEVCTL2_DISABLE_CACHING));
+	pci_write_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCTL2,
+			      val);
+
+	return 0;
+}
+
+/**
+ * cxl_reset - initiate a cxl reset
+ * @dev: device to reset
+ * @probe: if true, return 0 if device can be reset this way
+ *
+ * Initiate a cxl reset on @dev.
+ */
+static int cxl_reset(struct pci_dev *dev, bool probe)
+{
+	u16 dvsec, reg;
+	int rc;
+
+	dvsec = cxl_device_dvsec(dev);
+	if (!dvsec)
+		return -ENOTTY;
+
+	/*
+	 * Check if CXL Reset is supported.
+	 */
+	rc = pci_read_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCAP,
+				  &reg);
+	if (rc)
+		return -ENOTTY;
+
+	if (~(reg & PCI_DVSEC_CXL_DEVCAP_CXL_RST))
+		return -ENOTTY;
+
+	if (probe)
+		return 0;
+
+	rc = cxl_reset_prepare(dev, dvsec);
+	if (rc)
+		return rc;
+
+	return cxl_reset_init(dev, dvsec);
+}
+
 void pci_dev_lock(struct pci_dev *dev)
 {
 	/* block PM suspend, driver probe, etc. */
@@ -5202,6 +5384,7 @@ const struct pci_reset_fn_method pci_reset_fn_methods[] = {
 	{ pci_dev_acpi_reset, .name = "acpi" },
 	{ pcie_reset_flr, .name = "flr" },
 	{ pci_af_flr, .name = "af_flr" },
+	{ cxl_reset, .name = "cxl_reset" },
 	{ pci_pm_reset, .name = "pm" },
 	{ pci_reset_bus_function, .name = "bus" },
 	{ cxl_reset_bus_function, .name = "cxl_bus" },
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 47b31ad724fa..efcb06598f26 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -51,7 +51,7 @@
 			       PCI_STATUS_PARITY)
 
 /* Number of reset methods used in pci_reset_fn_methods array in pci.c */
-#define PCI_NUM_RESET_METHODS 8
+#define PCI_NUM_RESET_METHODS 9
 
 #define PCI_RESET_PROBE		true
 #define PCI_RESET_DO_RESET	false
diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index 3445c4970e4d..52618c5b095d 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -1209,6 +1209,31 @@
 #define PCI_DOE_DATA_OBJECT_DISC_RSP_3_NEXT_INDEX	0xff000000
 
 /* Compute Express Link (CXL r3.1, sec 8.1.5) */
+#define PCI_DVSEC_CXL_DEV				0
+#define PCI_DVSEC_CXL_DEVCAP				0x0a
+#define PCI_DVSEC_CXL_DEVCAP_CACHE_CAPABLE		0x00000001
+#define PCI_DVSEC_CXL_DEVCAP_CACHE_WB_INVALIDATE	0x00000040
+#define PCI_DVSEC_CXL_DEVCAP_CXL_RST			0x00000080
+#define PCI_DVSEC_CXL_DEVCAP_CXL_RST_TIMEOUT_IND	0x8
+#define PCI_DVSEC_CXL_DEVCAP_CXL_RST_TIMEOUT_MASK	0x00000700
+#define PCI_DVSEC_CXL_DEVCAP_CXL_RST_MEM_CLR		0x00000800
+#define PCI_DVSEC_CXL_DEVCTL				0x0c
+#define PCI_DVSEC_CXL_DEVCTL2				0x10
+#define PCI_DVSEC_CXL_DEVCTL2_DISABLE_CACHING		0x1
+#define PCI_DVSEC_CXL_DEVCTL2_INIT_CACHE_WB_INVALIDATE	0x2
+#define PCI_DVSEC_CXL_DEVCTL2_CXL_INIT_RST		0x4
+#define PCI_DVSEC_CXL_DEVCTL2_CXL_RST_MEM_CLR_ENABLE	0x8
+#define PCI_DVSEC_CXL_DEVSTATUS2			0x12
+#define PCI_DVSEC_CXL_DEVSTATUS2_CACHE_INVALID		0x1
+#define PCI_DVSEC_CXL_DEVSTATUS2_RST_COMPLETE		0x2
+#define PCI_DVSEC_CXL_DEVSTATUS2_RST_ERR		0x4
+#define PCI_DVSEC_CXL_DEVCAP2				0x16
+#define PCI_DVSEC_CXL_DEVCAP2_CACHE_SIZE_UNIT		0x0000000F
+#define PCI_DVSEC_CXL_DEVCAP2_CACHE_SIZE_UNIT_0		0x0
+#define PCI_DVSEC_CXL_DEVCAP2_CACHE_SIZE_UNIT_1		0x40
+#define PCI_DVSEC_CXL_DEVCAP2_CACHE_SIZE_UNIT_2		0x400
+#define PCI_DVSEC_CXL_DEVCAP2_CACHE_SIZE(x)		(((x) & 0x0000FF00) >> 8)
+
 #define PCI_DVSEC_CXL_PORT				3
 #define PCI_DVSEC_CXL_PORT_CTL				0x0c
 #define PCI_DVSEC_CXL_PORT_CTL_UNMASK_SBR		0x00000001
-- 
2.25.1

Re: [PATCH v1 1/1] cxl: add support for cxl reset

[Dave Jiang]

On 2/7/25 2:03 AM, Srirangan Madhavan wrote:
> This change adds the support and implements the CXL reset
> steps as laid out by the CXL Spec v3.1 Sections 9.6 & 9.7.
> 
> With support for Type 2 devices being introduced, more devices will
> require finer-grained reset mechanisms beyond bus-wide reset methods.
> 
> This change defines the necessary CXL DVSEC register macros.
> For devices that support CXL Reset, cache lines are disabled, WB+I is
> asserted, wait for cache invalid status, Mem Clr bit is asserted and
> finally reset is initiated.
> 
> Signed-off-by: Srirangan Madhavan <smadhavan@nvidia.com>

Hi Srirangan,
Given this patch touches PCI code, it needs to also cc Bjorn and linux-pci. The expectation is it would go through the pci subsystem for upstream with review tags from the CXL maintainers. Thanks!

DJ

> ---
>  drivers/pci/pci.c             | 183 ++++++++++++++++++++++++++++++++++
>  include/linux/pci.h           |   2 +-
>  include/uapi/linux/pci_regs.h |  25 +++++
>  3 files changed, 209 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 869d204a70a3..cf6009f5bd6c 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -5026,6 +5026,12 @@ static int pci_dev_reset_slot_function(struct pci_dev *dev, bool probe)
>  	return pci_reset_hotplug_slot(dev->slot->hotplug, probe);
>  }
>  
> +static u16 cxl_device_dvsec(struct pci_dev *dev)
> +{
> +	return pci_find_dvsec_capability(dev, PCI_VENDOR_ID_CXL,
> +					 PCI_DVSEC_CXL_DEV);
> +}
> +
>  static u16 cxl_port_dvsec(struct pci_dev *dev)
>  {
>  	return pci_find_dvsec_capability(dev, PCI_VENDOR_ID_CXL,
> @@ -5116,6 +5122,182 @@ static int cxl_reset_bus_function(struct pci_dev *dev, bool probe)
>  	return rc;
>  }
>  
> +static int cxl_reset_prepare(struct pci_dev *dev, u16 dvsec)
> +{
> +	u16 reg, val, cap;
> +	int rc;
> +	u32 timeout_us = 100, timeout_tot_us = 10000;
> +
> +	/*
> +	 * Wait for any pending transactions.
> +	 * Assuming this does cxl.io stuff.
> +	 */
> +	if (!pci_wait_for_pending_transaction(dev))
> +		pci_err(dev, "timed out waiting for pending transaction; performing cxl reset anyway\n");
> +
> +	/*
> +	 * Disable caching and then write back and invalidate lines.
> +	 */
> +	rc = pci_read_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCAP,
> +				  &cap);
> +	if (rc)
> +		return rc;
> +
> +	if (!(cap & PCI_DVSEC_CXL_DEVCAP_CACHE_CAPABLE))
> +		return 0;
> +
> +	/*
> +	 * Disable cache.
> +	 * WB and invalidate cahce if capability is advertised.
> +	 */
> +	rc = pci_read_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCTL2,
> +				  &reg);
> +	if (rc)
> +		return rc;
> +	val = reg | PCI_DVSEC_CXL_DEVCTL2_DISABLE_CACHING;
> +
> +	if (cap & PCI_DVSEC_CXL_DEVCAP_CACHE_WB_INVALIDATE)
> +		val = reg | PCI_DVSEC_CXL_DEVCTL2_INIT_CACHE_WB_INVALIDATE;
> +	pci_write_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCTL2,
> +			      val);
> +
> +	/*
> +	 * From Section 9.6: "Software may leverage the cache size reported in
> +	 * the DVSEC CXL Capability2 register to compute a suitable timeout
> +	 * value".
> +	 * Given there is no conversion factor for cache size -> timeout,
> +	 * setting timer for default 10ms.
> +	 */
> +	do {
> +		if (timeout_tot_us < 0)
> +			return -ETIMEDOUT;
> +		usleep_range(timeout_us, timeout_us+1);
> +		timeout_tot_us -= timeout_us;
> +		rc = pci_read_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCTL2,
> +					  &reg);
> +		if (rc)
> +			return rc;
> +	} while (!(reg & PCI_DVSEC_CXL_DEVSTATUS2_CACHE_INVALID));
> +
> +	return 0;
> +}
> +
> +/**
> + * cxl_reset_init - initiate a cxl reset
> + * @dev: device to reset
> + *
> + * Initiate a cxl reset.
> + */
> +static int cxl_reset_init(struct pci_dev *dev, u16 dvsec)
> +{
> +	u16 reg, val;
> +	u32 timeout_ms;
> +	int rc;
> +	u32 reset_timeouts_ms[] = {10, 100, 1000, 10000, 100000};
> +
> +	/*
> +	 * Check if CXL Reset MEM CLR is supported.
> +	 */
> +	rc = pci_read_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCAP,
> +				  &reg);
> +	if (rc)
> +		return rc;
> +
> +	if (reg & PCI_DVSEC_CXL_DEVCAP_CXL_RST_MEM_CLR) {
> +		rc = pci_read_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCTL2,
> +					  &reg);
> +		if (rc)
> +			return rc;
> +
> +		val = reg | PCI_DVSEC_CXL_DEVCTL2_CXL_RST_MEM_CLR_ENABLE;
> +		pci_write_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCTL2,
> +				      val);
> +	}
> +
> +	/*
> +	 * Read timeout value
> +	 */
> +	rc = pci_read_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCAP,
> +				  &reg);
> +	if (rc)
> +		return rc;
> +	timeout_ms = reset_timeouts_ms[FIELD_GET(PCI_DVSEC_CXL_DEVCAP_CXL_RST_TIMEOUT_MASK, reg)];
> +
> +	/*
> +	 * Write reset config
> +	 */
> +	rc = pci_read_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCTL2,
> +				  &reg);
> +	if (rc)
> +		return rc;
> +
> +	val = reg | PCI_DVSEC_CXL_DEVCTL2_CXL_INIT_RST;
> +	pci_write_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCTL2,
> +			      val);
> +
> +	/*
> +	 *  Wait till timeout and then check reset status is complete.
> +	 */
> +	msleep(timeout_ms);
> +	rc = pci_read_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVSTATUS2,
> +				  &reg);
> +	if (rc)
> +		return rc;
> +	if (reg & PCI_DVSEC_CXL_DEVSTATUS2_RST_ERR ||
> +	    ~reg & PCI_DVSEC_CXL_DEVSTATUS2_RST_COMPLETE)
> +		return -ETIMEDOUT;
> +
> +	/*
> +	 * Revert cashing disable.
> +	 */
> +	rc = pci_read_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCTL2,
> +				  &reg);
> +	if (rc)
> +		return rc;
> +	val = (reg & (~PCI_DVSEC_CXL_DEVCTL2_DISABLE_CACHING));
> +	pci_write_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCTL2,
> +			      val);
> +
> +	return 0;
> +}
> +
> +/**
> + * cxl_reset - initiate a cxl reset
> + * @dev: device to reset
> + * @probe: if true, return 0 if device can be reset this way
> + *
> + * Initiate a cxl reset on @dev.
> + */
> +static int cxl_reset(struct pci_dev *dev, bool probe)
> +{
> +	u16 dvsec, reg;
> +	int rc;
> +
> +	dvsec = cxl_device_dvsec(dev);
> +	if (!dvsec)
> +		return -ENOTTY;
> +
> +	/*
> +	 * Check if CXL Reset is supported.
> +	 */
> +	rc = pci_read_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCAP,
> +				  &reg);
> +	if (rc)
> +		return -ENOTTY;
> +
> +	if (~(reg & PCI_DVSEC_CXL_DEVCAP_CXL_RST))
> +		return -ENOTTY;
> +
> +	if (probe)
> +		return 0;
> +
> +	rc = cxl_reset_prepare(dev, dvsec);
> +	if (rc)
> +		return rc;
> +
> +	return cxl_reset_init(dev, dvsec);
> +}
> +
>  void pci_dev_lock(struct pci_dev *dev)
>  {
>  	/* block PM suspend, driver probe, etc. */
> @@ -5202,6 +5384,7 @@ const struct pci_reset_fn_method pci_reset_fn_methods[] = {
>  	{ pci_dev_acpi_reset, .name = "acpi" },
>  	{ pcie_reset_flr, .name = "flr" },
>  	{ pci_af_flr, .name = "af_flr" },
> +	{ cxl_reset, .name = "cxl_reset" },
>  	{ pci_pm_reset, .name = "pm" },
>  	{ pci_reset_bus_function, .name = "bus" },
>  	{ cxl_reset_bus_function, .name = "cxl_bus" },
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 47b31ad724fa..efcb06598f26 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -51,7 +51,7 @@
>  			       PCI_STATUS_PARITY)
>  
>  /* Number of reset methods used in pci_reset_fn_methods array in pci.c */
> -#define PCI_NUM_RESET_METHODS 8
> +#define PCI_NUM_RESET_METHODS 9
>  
>  #define PCI_RESET_PROBE		true
>  #define PCI_RESET_DO_RESET	false
> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> index 3445c4970e4d..52618c5b095d 100644
> --- a/include/uapi/linux/pci_regs.h
> +++ b/include/uapi/linux/pci_regs.h
> @@ -1209,6 +1209,31 @@
>  #define PCI_DOE_DATA_OBJECT_DISC_RSP_3_NEXT_INDEX	0xff000000
>  
>  /* Compute Express Link (CXL r3.1, sec 8.1.5) */
> +#define PCI_DVSEC_CXL_DEV				0
> +#define PCI_DVSEC_CXL_DEVCAP				0x0a
> +#define PCI_DVSEC_CXL_DEVCAP_CACHE_CAPABLE		0x00000001
> +#define PCI_DVSEC_CXL_DEVCAP_CACHE_WB_INVALIDATE	0x00000040
> +#define PCI_DVSEC_CXL_DEVCAP_CXL_RST			0x00000080
> +#define PCI_DVSEC_CXL_DEVCAP_CXL_RST_TIMEOUT_IND	0x8
> +#define PCI_DVSEC_CXL_DEVCAP_CXL_RST_TIMEOUT_MASK	0x00000700
> +#define PCI_DVSEC_CXL_DEVCAP_CXL_RST_MEM_CLR		0x00000800
> +#define PCI_DVSEC_CXL_DEVCTL				0x0c
> +#define PCI_DVSEC_CXL_DEVCTL2				0x10
> +#define PCI_DVSEC_CXL_DEVCTL2_DISABLE_CACHING		0x1
> +#define PCI_DVSEC_CXL_DEVCTL2_INIT_CACHE_WB_INVALIDATE	0x2
> +#define PCI_DVSEC_CXL_DEVCTL2_CXL_INIT_RST		0x4
> +#define PCI_DVSEC_CXL_DEVCTL2_CXL_RST_MEM_CLR_ENABLE	0x8
> +#define PCI_DVSEC_CXL_DEVSTATUS2			0x12
> +#define PCI_DVSEC_CXL_DEVSTATUS2_CACHE_INVALID		0x1
> +#define PCI_DVSEC_CXL_DEVSTATUS2_RST_COMPLETE		0x2
> +#define PCI_DVSEC_CXL_DEVSTATUS2_RST_ERR		0x4
> +#define PCI_DVSEC_CXL_DEVCAP2				0x16
> +#define PCI_DVSEC_CXL_DEVCAP2_CACHE_SIZE_UNIT		0x0000000F
> +#define PCI_DVSEC_CXL_DEVCAP2_CACHE_SIZE_UNIT_0		0x0
> +#define PCI_DVSEC_CXL_DEVCAP2_CACHE_SIZE_UNIT_1		0x40
> +#define PCI_DVSEC_CXL_DEVCAP2_CACHE_SIZE_UNIT_2		0x400
> +#define PCI_DVSEC_CXL_DEVCAP2_CACHE_SIZE(x)		(((x) & 0x0000FF00) >> 8)
> +
>  #define PCI_DVSEC_CXL_PORT				3
>  #define PCI_DVSEC_CXL_PORT_CTL				0x0c
>  #define PCI_DVSEC_CXL_PORT_CTL_UNMASK_SBR		0x00000001

Re: [PATCH v1 1/1] cxl: add support for cxl reset

[Ira Weiny]

Srirangan Madhavan wrote:

Generally this looks good.  Some minor issues below.

> This change adds the support and implements the CXL reset
> steps as laid out by the CXL Spec v3.1 Sections 9.6 & 9.7.
> 
> With support for Type 2 devices being introduced, more devices will
> require finer-grained reset mechanisms beyond bus-wide reset methods.
> 
> This change defines the necessary CXL DVSEC register macros.
> For devices that support CXL Reset, cache lines are disabled, WB+I is
> asserted, wait for cache invalid status, Mem Clr bit is asserted and
> finally reset is initiated.

This commit message should be in the imperative.  For example:

"Type 2 devices are being introduced and will require finer-grained
reset.

Add support for CXL reset per CXL v3.1 Section 9.6/9.7."

The rest of the details are given in the code itself.  BTW why not just
update to 3.2?

> 
> Signed-off-by: Srirangan Madhavan <smadhavan@nvidia.com>
> ---
>  drivers/pci/pci.c             | 183 ++++++++++++++++++++++++++++++++++
>  include/linux/pci.h           |   2 +-
>  include/uapi/linux/pci_regs.h |  25 +++++
>  3 files changed, 209 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 869d204a70a3..cf6009f5bd6c 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -5026,6 +5026,12 @@ static int pci_dev_reset_slot_function(struct pci_dev *dev, bool probe)
>  	return pci_reset_hotplug_slot(dev->slot->hotplug, probe);
>  }
>  
> +static u16 cxl_device_dvsec(struct pci_dev *dev)
> +{
> +	return pci_find_dvsec_capability(dev, PCI_VENDOR_ID_CXL,
> +					 PCI_DVSEC_CXL_DEV);
> +}

NIT: probably not worth having a whole function but...

> +
>  static u16 cxl_port_dvsec(struct pci_dev *dev)
>  {
>  	return pci_find_dvsec_capability(dev, PCI_VENDOR_ID_CXL,
> @@ -5116,6 +5122,182 @@ static int cxl_reset_bus_function(struct pci_dev *dev, bool probe)
>  	return rc;
>  }
>  
> +static int cxl_reset_prepare(struct pci_dev *dev, u16 dvsec)
> +{
> +	u16 reg, val, cap;
> +	int rc;
> +	u32 timeout_us = 100, timeout_tot_us = 10000;
> +
> +	/*
> +	 * Wait for any pending transactions.
> +	 * Assuming this does cxl.io stuff.
> +	 */

The comment here is generally not needed.  In particular the first
sentence is the same as the function name being called.

> +	if (!pci_wait_for_pending_transaction(dev))
> +		pci_err(dev, "timed out waiting for pending transaction; performing cxl reset anyway\n");
> +
> +	/*
> +	 * Disable caching and then write back and invalidate lines.
> +	 */

I think this comment is wrong...

> +	rc = pci_read_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCAP,
> +				  &cap);
> +	if (rc)
> +		return rc;
> +
> +	if (!(cap & PCI_DVSEC_CXL_DEVCAP_CACHE_CAPABLE))
> +		return 0;

Isn't this checking for a cache capable device?

> +
> +	/*
> +	 * Disable cache.
> +	 * WB and invalidate cahce if capability is advertised.
> +	 */

Then disabling and invalidating the cache here.

> +	rc = pci_read_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCTL2,
> +				  &reg);
> +	if (rc)
> +		return rc;
> +	val = reg | PCI_DVSEC_CXL_DEVCTL2_DISABLE_CACHING;
> +
> +	if (cap & PCI_DVSEC_CXL_DEVCAP_CACHE_WB_INVALIDATE)
> +		val = reg | PCI_DVSEC_CXL_DEVCTL2_INIT_CACHE_WB_INVALIDATE;
> +	pci_write_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCTL2,
> +			      val);
> +
> +	/*
> +	 * From Section 9.6: "Software may leverage the cache size reported in
> +	 * the DVSEC CXL Capability2 register to compute a suitable timeout
> +	 * value".
> +	 * Given there is no conversion factor for cache size -> timeout,
> +	 * setting timer for default 10ms.
> +	 */
> +	do {
> +		if (timeout_tot_us < 0)
> +			return -ETIMEDOUT;
> +		usleep_range(timeout_us, timeout_us+1);
> +		timeout_tot_us -= timeout_us;
> +		rc = pci_read_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCTL2,
> +					  &reg);
> +		if (rc)
> +			return rc;
> +	} while (!(reg & PCI_DVSEC_CXL_DEVSTATUS2_CACHE_INVALID));
> +
> +	return 0;
> +}
> +
> +/**
> + * cxl_reset_init - initiate a cxl reset
> + * @dev: device to reset

@dvsec?

> + *
> + * Initiate a cxl reset.
> + */

Given this is a static internal call it seems best to leave out the kdoc
altogether rather than add the dvsec parameter to the doc.

> +static int cxl_reset_init(struct pci_dev *dev, u16 dvsec)
> +{
> +	u16 reg, val;
> +	u32 timeout_ms;
> +	int rc;
> +	u32 reset_timeouts_ms[] = {10, 100, 1000, 10000, 100000};

NIT: Maybe...  give the spec reference for the timeout values.

> +
> +	/*
> +	 * Check if CXL Reset MEM CLR is supported.
> +	 */
> +	rc = pci_read_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCAP,
> +				  &reg);
> +	if (rc)
> +		return rc;
> +
> +	if (reg & PCI_DVSEC_CXL_DEVCAP_CXL_RST_MEM_CLR) {
> +		rc = pci_read_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCTL2,
> +					  &reg);
> +		if (rc)
> +			return rc;
> +
> +		val = reg | PCI_DVSEC_CXL_DEVCTL2_CXL_RST_MEM_CLR_ENABLE;
> +		pci_write_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCTL2,
> +				      val);
> +	}
> +
> +	/*
> +	 * Read timeout value
> +	 */
> +	rc = pci_read_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCAP,
> +				  &reg);
> +	if (rc)
> +		return rc;
> +	timeout_ms = reset_timeouts_ms[FIELD_GET(PCI_DVSEC_CXL_DEVCAP_CXL_RST_TIMEOUT_MASK, reg)];
> +
> +	/*
> +	 * Write reset config
> +	 */
> +	rc = pci_read_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCTL2,
> +				  &reg);
> +	if (rc)
> +		return rc;
> +
> +	val = reg | PCI_DVSEC_CXL_DEVCTL2_CXL_INIT_RST;
> +	pci_write_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCTL2,
> +			      val);
> +
> +	/*
> +	 *  Wait till timeout and then check reset status is complete.
> +	 */
> +	msleep(timeout_ms);
> +	rc = pci_read_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVSTATUS2,
> +				  &reg);
> +	if (rc)
> +		return rc;
> +	if (reg & PCI_DVSEC_CXL_DEVSTATUS2_RST_ERR ||
> +	    ~reg & PCI_DVSEC_CXL_DEVSTATUS2_RST_COMPLETE)
> +		return -ETIMEDOUT;
> +
> +	/*
> +	 * Revert cashing disable.

NIT caching

Probably best to remove the comment.

> +	 */
> +	rc = pci_read_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCTL2,
> +				  &reg);
> +	if (rc)
> +		return rc;
> +	val = (reg & (~PCI_DVSEC_CXL_DEVCTL2_DISABLE_CACHING));
> +	pci_write_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCTL2,
> +			      val);
> +
> +	return 0;
> +}
> +
> +/**
> + * cxl_reset - initiate a cxl reset
> + * @dev: device to reset
> + * @probe: if true, return 0 if device can be reset this way
> + *
> + * Initiate a cxl reset on @dev.
> + */
> +static int cxl_reset(struct pci_dev *dev, bool probe)
> +{
> +	u16 dvsec, reg;
> +	int rc;
> +
> +	dvsec = cxl_device_dvsec(dev);
> +	if (!dvsec)
> +		return -ENOTTY;
> +
> +	/*
> +	 * Check if CXL Reset is supported.
> +	 */
> +	rc = pci_read_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCAP,
> +				  &reg);
> +	if (rc)
> +		return -ENOTTY;
> +
> +	if (~(reg & PCI_DVSEC_CXL_DEVCAP_CXL_RST))
> +		return -ENOTTY;
> +
> +	if (probe)
> +		return 0;
> +
> +	rc = cxl_reset_prepare(dev, dvsec);
> +	if (rc)
> +		return rc;
> +
> +	return cxl_reset_init(dev, dvsec);
> +}
> +
>  void pci_dev_lock(struct pci_dev *dev)
>  {
>  	/* block PM suspend, driver probe, etc. */
> @@ -5202,6 +5384,7 @@ const struct pci_reset_fn_method pci_reset_fn_methods[] = {
>  	{ pci_dev_acpi_reset, .name = "acpi" },
>  	{ pcie_reset_flr, .name = "flr" },
>  	{ pci_af_flr, .name = "af_flr" },
> +	{ cxl_reset, .name = "cxl_reset" },
>  	{ pci_pm_reset, .name = "pm" },
>  	{ pci_reset_bus_function, .name = "bus" },
>  	{ cxl_reset_bus_function, .name = "cxl_bus" },
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 47b31ad724fa..efcb06598f26 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -51,7 +51,7 @@
>  			       PCI_STATUS_PARITY)
>  
>  /* Number of reset methods used in pci_reset_fn_methods array in pci.c */
> -#define PCI_NUM_RESET_METHODS 8
> +#define PCI_NUM_RESET_METHODS 9
>  
>  #define PCI_RESET_PROBE		true
>  #define PCI_RESET_DO_RESET	false
> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> index 3445c4970e4d..52618c5b095d 100644
> --- a/include/uapi/linux/pci_regs.h
> +++ b/include/uapi/linux/pci_regs.h
> @@ -1209,6 +1209,31 @@
>  #define PCI_DOE_DATA_OBJECT_DISC_RSP_3_NEXT_INDEX	0xff000000
>  
>  /* Compute Express Link (CXL r3.1, sec 8.1.5) */

v3.2

Ira

[snip]

Re: [PATCH v1 1/1] cxl: add support for cxl reset

[kernel test robot]

Hi Srirangan,

kernel test robot noticed the following build warnings:

[auto build test WARNING on pci/next]
[also build test WARNING on pci/for-linus linus/master v6.14-rc1 next-20250207]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Srirangan-Madhavan/cxl-add-support-for-cxl-reset/20250207-170511
base:   https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git next
patch link:    https://lore.kernel.org/r/20250207090327.172478-2-smadhavan%40nvidia.com
patch subject: [PATCH v1 1/1] cxl: add support for cxl reset
config: x86_64-randconfig-161-20250208 (https://download.01.org/0day-ci/archive/20250208/202502081954.MzqpYilc-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-12) 11.3.0

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202502081954.MzqpYilc-lkp@intel.com/

smatch warnings:
drivers/pci/pci.c:5172 cxl_reset_prepare() warn: unsigned 'timeout_tot_us' is never less than zero.

vim +/timeout_tot_us +5172 drivers/pci/pci.c

  5124	
  5125	static int cxl_reset_prepare(struct pci_dev *dev, u16 dvsec)
  5126	{
  5127		u16 reg, val, cap;
  5128		int rc;
  5129		u32 timeout_us = 100, timeout_tot_us = 10000;
  5130	
  5131		/*
  5132		 * Wait for any pending transactions.
  5133		 * Assuming this does cxl.io stuff.
  5134		 */
  5135		if (!pci_wait_for_pending_transaction(dev))
  5136			pci_err(dev, "timed out waiting for pending transaction; performing cxl reset anyway\n");
  5137	
  5138		/*
  5139		 * Disable caching and then write back and invalidate lines.
  5140		 */
  5141		rc = pci_read_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCAP,
  5142					  &cap);
  5143		if (rc)
  5144			return rc;
  5145	
  5146		if (!(cap & PCI_DVSEC_CXL_DEVCAP_CACHE_CAPABLE))
  5147			return 0;
  5148	
  5149		/*
  5150		 * Disable cache.
  5151		 * WB and invalidate cahce if capability is advertised.
  5152		 */
  5153		rc = pci_read_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCTL2,
  5154					  &reg);
  5155		if (rc)
  5156			return rc;
  5157		val = reg | PCI_DVSEC_CXL_DEVCTL2_DISABLE_CACHING;
  5158	
  5159		if (cap & PCI_DVSEC_CXL_DEVCAP_CACHE_WB_INVALIDATE)
  5160			val = reg | PCI_DVSEC_CXL_DEVCTL2_INIT_CACHE_WB_INVALIDATE;
  5161		pci_write_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCTL2,
  5162				      val);
  5163	
  5164		/*
  5165		 * From Section 9.6: "Software may leverage the cache size reported in
  5166		 * the DVSEC CXL Capability2 register to compute a suitable timeout
  5167		 * value".
  5168		 * Given there is no conversion factor for cache size -> timeout,
  5169		 * setting timer for default 10ms.
  5170		 */
  5171		do {
> 5172			if (timeout_tot_us < 0)
  5173				return -ETIMEDOUT;
  5174			usleep_range(timeout_us, timeout_us+1);
  5175			timeout_tot_us -= timeout_us;
  5176			rc = pci_read_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCTL2,
  5177						  &reg);
  5178			if (rc)
  5179				return rc;
  5180		} while (!(reg & PCI_DVSEC_CXL_DEVSTATUS2_CACHE_INVALID));
  5181	
  5182		return 0;
  5183	}
  5184	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v1 1/1] cxl: add support for cxl reset

[Srirangan Madhavan]

@Alejandro,
It would be nice to get your "Tested by" before the next re-spin.
It would be really helpful to see how the patch works with a type 2 device changes you've added.
Will that be possible?

Thank you.

Regards,
Srirangan.

________________________________________
From: Ira Weiny <ira.weiny@intel.com>
Sent: Friday, February 7, 2025 9:15 AM
To: Srirangan Madhavan; Davidlohr Bueso; Jonathan Cameron; Dave Jiang; Alison Schofield; Vishal Verma; Ira Weiny; Dan Williams
Cc: Zhi Wang; Vishal Aslot; Shanker Donthineni; linux-cxl@vger.kernel.org
Subject: Re: [PATCH v1 1/1] cxl: add support for cxl reset

External email: Use caution opening links or attachments


Srirangan Madhavan wrote:

Generally this looks good.  Some minor issues below.

> This change adds the support and implements the CXL reset
> steps as laid out by the CXL Spec v3.1 Sections 9.6 & 9.7.
>
> With support for Type 2 devices being introduced, more devices will
> require finer-grained reset mechanisms beyond bus-wide reset methods.
>
> This change defines the necessary CXL DVSEC register macros.
> For devices that support CXL Reset, cache lines are disabled, WB+I is
> asserted, wait for cache invalid status, Mem Clr bit is asserted and
> finally reset is initiated.

This commit message should be in the imperative.  For example:

"Type 2 devices are being introduced and will require finer-grained
reset.

Add support for CXL reset per CXL v3.1 Section 9.6/9.7."

The rest of the details are given in the code itself.  BTW why not just
update to 3.2?

>
> Signed-off-by: Srirangan Madhavan <smadhavan@nvidia.com>
> ---
>  drivers/pci/pci.c             | 183 ++++++++++++++++++++++++++++++++++
>  include/linux/pci.h           |   2 +-
>  include/uapi/linux/pci_regs.h |  25 +++++
>  3 files changed, 209 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 869d204a70a3..cf6009f5bd6c 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -5026,6 +5026,12 @@ static int pci_dev_reset_slot_function(struct pci_dev *dev, bool probe)
>       return pci_reset_hotplug_slot(dev->slot->hotplug, probe);
>  }
>
> +static u16 cxl_device_dvsec(struct pci_dev *dev)
> +{
> +     return pci_find_dvsec_capability(dev, PCI_VENDOR_ID_CXL,
> +                                      PCI_DVSEC_CXL_DEV);
> +}

NIT: probably not worth having a whole function but...

> +
>  static u16 cxl_port_dvsec(struct pci_dev *dev)
>  {
>       return pci_find_dvsec_capability(dev, PCI_VENDOR_ID_CXL,
> @@ -5116,6 +5122,182 @@ static int cxl_reset_bus_function(struct pci_dev *dev, bool probe)
>       return rc;
>  }
>
> +static int cxl_reset_prepare(struct pci_dev *dev, u16 dvsec)
> +{
> +     u16 reg, val, cap;
> +     int rc;
> +     u32 timeout_us = 100, timeout_tot_us = 10000;
> +
> +     /*
> +      * Wait for any pending transactions.
> +      * Assuming this does cxl.io stuff.
> +      */

The comment here is generally not needed.  In particular the first
sentence is the same as the function name being called.

> +     if (!pci_wait_for_pending_transaction(dev))
> +             pci_err(dev, "timed out waiting for pending transaction; performing cxl reset anyway\n");
> +
> +     /*
> +      * Disable caching and then write back and invalidate lines.
> +      */

I think this comment is wrong...

> +     rc = pci_read_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCAP,
> +                               &cap);
> +     if (rc)
> +             return rc;
> +
> +     if (!(cap & PCI_DVSEC_CXL_DEVCAP_CACHE_CAPABLE))
> +             return 0;

Isn't this checking for a cache capable device?

> +
> +     /*
> +      * Disable cache.
> +      * WB and invalidate cahce if capability is advertised.
> +      */

Then disabling and invalidating the cache here.

> +     rc = pci_read_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCTL2,
> +                               &reg);
> +     if (rc)
> +             return rc;
> +     val = reg | PCI_DVSEC_CXL_DEVCTL2_DISABLE_CACHING;
> +
> +     if (cap & PCI_DVSEC_CXL_DEVCAP_CACHE_WB_INVALIDATE)
> +             val = reg | PCI_DVSEC_CXL_DEVCTL2_INIT_CACHE_WB_INVALIDATE;
> +     pci_write_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCTL2,
> +                           val);
> +
> +     /*
> +      * From Section 9.6: "Software may leverage the cache size reported in
> +      * the DVSEC CXL Capability2 register to compute a suitable timeout
> +      * value".
> +      * Given there is no conversion factor for cache size -> timeout,
> +      * setting timer for default 10ms.
> +      */
> +     do {
> +             if (timeout_tot_us < 0)
> +                     return -ETIMEDOUT;
> +             usleep_range(timeout_us, timeout_us+1);
> +             timeout_tot_us -= timeout_us;
> +             rc = pci_read_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCTL2,
> +                                       &reg);
> +             if (rc)
> +                     return rc;
> +     } while (!(reg & PCI_DVSEC_CXL_DEVSTATUS2_CACHE_INVALID));
> +
> +     return 0;
> +}
> +
> +/**
> + * cxl_reset_init - initiate a cxl reset
> + * @dev: device to reset

@dvsec?

> + *
> + * Initiate a cxl reset.
> + */

Given this is a static internal call it seems best to leave out the kdoc
altogether rather than add the dvsec parameter to the doc.

> +static int cxl_reset_init(struct pci_dev *dev, u16 dvsec)
> +{
> +     u16 reg, val;
> +     u32 timeout_ms;
> +     int rc;
> +     u32 reset_timeouts_ms[] = {10, 100, 1000, 10000, 100000};

NIT: Maybe...  give the spec reference for the timeout values.

> +
> +     /*
> +      * Check if CXL Reset MEM CLR is supported.
> +      */
> +     rc = pci_read_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCAP,
> +                               &reg);
> +     if (rc)
> +             return rc;
> +
> +     if (reg & PCI_DVSEC_CXL_DEVCAP_CXL_RST_MEM_CLR) {
> +             rc = pci_read_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCTL2,
> +                                       &reg);
> +             if (rc)
> +                     return rc;
> +
> +             val = reg | PCI_DVSEC_CXL_DEVCTL2_CXL_RST_MEM_CLR_ENABLE;
> +             pci_write_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCTL2,
> +                                   val);
> +     }
> +
> +     /*
> +      * Read timeout value
> +      */
> +     rc = pci_read_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCAP,
> +                               &reg);
> +     if (rc)
> +             return rc;
> +     timeout_ms = reset_timeouts_ms[FIELD_GET(PCI_DVSEC_CXL_DEVCAP_CXL_RST_TIMEOUT_MASK, reg)];
> +
> +     /*
> +      * Write reset config
> +      */
> +     rc = pci_read_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCTL2,
> +                               &reg);
> +     if (rc)
> +             return rc;
> +
> +     val = reg | PCI_DVSEC_CXL_DEVCTL2_CXL_INIT_RST;
> +     pci_write_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCTL2,
> +                           val);
> +
> +     /*
> +      *  Wait till timeout and then check reset status is complete.
> +      */
> +     msleep(timeout_ms);
> +     rc = pci_read_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVSTATUS2,
> +                               &reg);
> +     if (rc)
> +             return rc;
> +     if (reg & PCI_DVSEC_CXL_DEVSTATUS2_RST_ERR ||
> +         ~reg & PCI_DVSEC_CXL_DEVSTATUS2_RST_COMPLETE)
> +             return -ETIMEDOUT;
> +
> +     /*
> +      * Revert cashing disable.

NIT caching

Probably best to remove the comment.

> +      */
> +     rc = pci_read_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCTL2,
> +                               &reg);
> +     if (rc)
> +             return rc;
> +     val = (reg & (~PCI_DVSEC_CXL_DEVCTL2_DISABLE_CACHING));
> +     pci_write_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCTL2,
> +                           val);
> +
> +     return 0;
> +}
> +
> +/**
> + * cxl_reset - initiate a cxl reset
> + * @dev: device to reset
> + * @probe: if true, return 0 if device can be reset this way
> + *
> + * Initiate a cxl reset on @dev.
> + */
> +static int cxl_reset(struct pci_dev *dev, bool probe)
> +{
> +     u16 dvsec, reg;
> +     int rc;
> +
> +     dvsec = cxl_device_dvsec(dev);
> +     if (!dvsec)
> +             return -ENOTTY;
> +
> +     /*
> +      * Check if CXL Reset is supported.
> +      */
> +     rc = pci_read_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCAP,
> +                               &reg);
> +     if (rc)
> +             return -ENOTTY;
> +
> +     if (~(reg & PCI_DVSEC_CXL_DEVCAP_CXL_RST))
> +             return -ENOTTY;
> +
> +     if (probe)
> +             return 0;
> +
> +     rc = cxl_reset_prepare(dev, dvsec);
> +     if (rc)
> +             return rc;
> +
> +     return cxl_reset_init(dev, dvsec);
> +}
> +
>  void pci_dev_lock(struct pci_dev *dev)
>  {
>       /* block PM suspend, driver probe, etc. */
> @@ -5202,6 +5384,7 @@ const struct pci_reset_fn_method pci_reset_fn_methods[] = {
>       { pci_dev_acpi_reset, .name = "acpi" },
>       { pcie_reset_flr, .name = "flr" },
>       { pci_af_flr, .name = "af_flr" },
> +     { cxl_reset, .name = "cxl_reset" },
>       { pci_pm_reset, .name = "pm" },
>       { pci_reset_bus_function, .name = "bus" },
>       { cxl_reset_bus_function, .name = "cxl_bus" },
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 47b31ad724fa..efcb06598f26 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -51,7 +51,7 @@
>                              PCI_STATUS_PARITY)
>
>  /* Number of reset methods used in pci_reset_fn_methods array in pci.c */
> -#define PCI_NUM_RESET_METHODS 8
> +#define PCI_NUM_RESET_METHODS 9
>
>  #define PCI_RESET_PROBE              true
>  #define PCI_RESET_DO_RESET   false
> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> index 3445c4970e4d..52618c5b095d 100644
> --- a/include/uapi/linux/pci_regs.h
> +++ b/include/uapi/linux/pci_regs.h
> @@ -1209,6 +1209,31 @@
>  #define PCI_DOE_DATA_OBJECT_DISC_RSP_3_NEXT_INDEX    0xff000000
>
>  /* Compute Express Link (CXL r3.1, sec 8.1.5) */

v3.2

Ira

[snip]

Re: [PATCH v1 1/1] cxl: add support for cxl reset

[Jonathan Cameron]

On Fri, 7 Feb 2025 01:03:27 -0800
Srirangan Madhavan <smadhavan@nvidia.com> wrote:

> This change adds the support and implements the CXL reset
> steps as laid out by the CXL Spec v3.1 Sections 9.6 & 9.7.
> 
> With support for Type 2 devices being introduced, more devices will
> require finer-grained reset mechanisms beyond bus-wide reset methods.
> 
> This change defines the necessary CXL DVSEC register macros.
> For devices that support CXL Reset, cache lines are disabled, WB+I is
> asserted, wait for cache invalid status, Mem Clr bit is asserted and
> finally reset is initiated.
> 
> Signed-off-by: Srirangan Madhavan <smadhavan@nvidia.com>
Hi Sriranagan,

Various style related comments inline.  My main suggestion is
to match local style of the file you are editing where possible.
Also look for duplication of defines etc.

I haven't checked the spec for precise sequence but will
do that on a v2 once the more superficial stuff is tidied up.

Thanks,

Jonathan


> ---
>  drivers/pci/pci.c             | 183 ++++++++++++++++++++++++++++++++++
>  include/linux/pci.h           |   2 +-
>  include/uapi/linux/pci_regs.h |  25 +++++
>  3 files changed, 209 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 869d204a70a3..cf6009f5bd6c 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -5026,6 +5026,12 @@ static int pci_dev_reset_slot_function(struct pci_dev *dev, bool probe)
>  	return pci_reset_hotplug_slot(dev->slot->hotplug, probe);
>  }
>  
> +static u16 cxl_device_dvsec(struct pci_dev *dev)
> +{
> +	return pci_find_dvsec_capability(dev, PCI_VENDOR_ID_CXL,
> +					 PCI_DVSEC_CXL_DEV);
> +}
> +
>  static u16 cxl_port_dvsec(struct pci_dev *dev)
>  {
>  	return pci_find_dvsec_capability(dev, PCI_VENDOR_ID_CXL,
> @@ -5116,6 +5122,182 @@ static int cxl_reset_bus_function(struct pci_dev *dev, bool probe)
>  	return rc;
>  }
>  
> +static int cxl_reset_prepare(struct pci_dev *dev, u16 dvsec)
> +{
> +	u16 reg, val, cap;
> +	int rc;
> +	u32 timeout_us = 100, timeout_tot_us = 10000;
File seems to at least at first glance use reverse xmas tree.
So do that here as well.

> +
> +	/*
> +	 * Wait for any pending transactions.
> +	 * Assuming this does cxl.io stuff.
> +	 */
> +	if (!pci_wait_for_pending_transaction(dev))
> +		pci_err(dev, "timed out waiting for pending transaction; performing cxl reset anyway\n");
> +
> +	/*
> +	 * Disable caching and then write back and invalidate lines.
> +	 */

Single line comment.

> +	rc = pci_read_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCAP,
> +				  &cap);
One line. It is under 80 chars anyway.

> +	if (rc)
> +		return rc;
> +
> +	if (!(cap & PCI_DVSEC_CXL_DEVCAP_CACHE_CAPABLE))
> +		return 0;
> +
> +	/*
> +	 * Disable cache.
> +	 * WB and invalidate cahce if capability is advertised.

cache

> +	 */
> +	rc = pci_read_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCTL2,
> +				  &reg);

One line. It fits I think.

> +	if (rc)
> +		return rc;
> +	val = reg | PCI_DVSEC_CXL_DEVCTL2_DISABLE_CACHING;

I'd do this in place in reg.

> +
> +	if (cap & PCI_DVSEC_CXL_DEVCAP_CACHE_WB_INVALIDATE)
> +		val = reg | PCI_DVSEC_CXL_DEVCTL2_INIT_CACHE_WB_INVALIDATE;

I think intent is keep disable caching set for this one?
If not use an else to make that clear.

> +	pci_write_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCTL2,
> +			      val);

Single line.  Check all the code for this.


> +
> +	/*
> +	 * From Section 9.6: "Software may leverage the cache size reported in
> +	 * the DVSEC CXL Capability2 register to compute a suitable timeout
> +	 * value".
> +	 * Given there is no conversion factor for cache size -> timeout,
> +	 * setting timer for default 10ms.
> +	 */
> +	do {
> +		if (timeout_tot_us < 0)
> +			return -ETIMEDOUT;
> +		usleep_range(timeout_us, timeout_us+1);

Spaces around +

> +		timeout_tot_us -= timeout_us;
> +		rc = pci_read_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCTL2,
> +					  &reg);
> +		if (rc)
> +			return rc;
> +	} while (!(reg & PCI_DVSEC_CXL_DEVSTATUS2_CACHE_INVALID));
> +
> +	return 0;
> +}
> +
> +/**
> + * cxl_reset_init - initiate a cxl reset
> + * @dev: device to reset
> + *
> + * Initiate a cxl reset.
> + */
> +static int cxl_reset_init(struct pci_dev *dev, u16 dvsec)
> +{
> +	u16 reg, val;
> +	u32 timeout_ms;
> +	int rc;
> +	u32 reset_timeouts_ms[] = {10, 100, 1000, 10000, 100000};
Local style is reverse xmas and spaces after { and before } in lines
like this (well the one I could find anyway).

> +
> +	/*
> +	 * Check if CXL Reset MEM CLR is supported.
> +	 */
Single line comment is fine.

> +	rc = pci_read_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCAP,
> +				  &reg);
> +	if (rc)
> +		return rc;
> +
> +	if (reg & PCI_DVSEC_CXL_DEVCAP_CXL_RST_MEM_CLR) {
> +		rc = pci_read_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCTL2,
> +					  &reg);
> +		if (rc)
> +			return rc;
> +
> +		val = reg | PCI_DVSEC_CXL_DEVCTL2_CXL_RST_MEM_CLR_ENABLE;
> +		pci_write_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCTL2,
> +				      val);
> +	}
> +
> +	/*
> +	 * Read timeout value
> +	 */

Single line comment.

> +	rc = pci_read_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCAP,
> +				  &reg);
> +	if (rc)
> +		return rc;
> +	timeout_ms = reset_timeouts_ms[FIELD_GET(PCI_DVSEC_CXL_DEVCAP_CXL_RST_TIMEOUT_MASK, reg)];

Rather long line. I'd be tempted to use a local variable for the FIELD_GET()

> +
> +	/*
> +	 * Write reset config
Single line comment.
> +	 */
> +	rc = pci_read_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCTL2,
> +				  &reg);
> +	if (rc)
> +		return rc;
> +
> +	val = reg | PCI_DVSEC_CXL_DEVCTL2_CXL_INIT_RST;
> +	pci_write_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCTL2,
> +			      val);
> +
> +	/*
> +	 *  Wait till timeout and then check reset status is complete.
Extra space before W and I think fits on single line anyway.
> +	 */
> +	msleep(timeout_ms);
> +	rc = pci_read_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVSTATUS2,
> +				  &reg);
> +	if (rc)
> +		return rc;
> +	if (reg & PCI_DVSEC_CXL_DEVSTATUS2_RST_ERR ||
> +	    ~reg & PCI_DVSEC_CXL_DEVSTATUS2_RST_COMPLETE)
> +		return -ETIMEDOUT;
> +
> +	/*
> +	 * Revert cashing disable.
> +	 */
> +	rc = pci_read_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCTL2,
> +				  &reg);
> +	if (rc)
> +		return rc;
> +	val = (reg & (~PCI_DVSEC_CXL_DEVCTL2_DISABLE_CACHING));
> +	pci_write_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCTL2,
> +			      val);
> +
> +	return 0;
> +}
> +
> +/**
> + * cxl_reset - initiate a cxl reset
> + * @dev: device to reset
> + * @probe: if true, return 0 if device can be reset this way
> + *
> + * Initiate a cxl reset on @dev.
> + */
> +static int cxl_reset(struct pci_dev *dev, bool probe)
> +{
> +	u16 dvsec, reg;
> +	int rc;
> +
> +	dvsec = cxl_device_dvsec(dev);
> +	if (!dvsec)
> +		return -ENOTTY;
> +
> +	/*
> +	 * Check if CXL Reset is supported.
> +	 */

	/* Check if CXL Reset is supported. */

> +	rc = pci_read_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCAP,
> +				  &reg);

One line. It is under 80 chars.

> +	if (rc)
> +		return -ENOTTY;
> +
> +	if (~(reg & PCI_DVSEC_CXL_DEVCAP_CXL_RST))

Using ~ for a single bit is unusual. I'd go with 

if (reg & PCI_DVSEC_CXL_DEVCAP_CXL_RST == 0)
or !(reg & ...)



> +		return -ENOTTY;
> +
> +	if (probe)
> +		return 0;
> +
> +	rc = cxl_reset_prepare(dev, dvsec);
> +	if (rc)
> +		return rc;
> +
> +	return cxl_reset_init(dev, dvsec);
> +}

> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 47b31ad724fa..efcb06598f26 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -51,7 +51,7 @@
>  			       PCI_STATUS_PARITY)
>  
>  /* Number of reset methods used in pci_reset_fn_methods array in pci.c */
> -#define PCI_NUM_RESET_METHODS 8
> +#define PCI_NUM_RESET_METHODS 9
>  
>  #define PCI_RESET_PROBE		true
>  #define PCI_RESET_DO_RESET	false
> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> index 3445c4970e4d..52618c5b095d 100644
> --- a/include/uapi/linux/pci_regs.h
> +++ b/include/uapi/linux/pci_regs.h
> @@ -1209,6 +1209,31 @@
>  #define PCI_DOE_DATA_OBJECT_DISC_RSP_3_NEXT_INDEX	0xff000000
>  
>  /* Compute Express Link (CXL r3.1, sec 8.1.5) */
> +#define PCI_DVSEC_CXL_DEV				0

We need to deduplicate the stuff in here and drivers/cxl/cxlpci.h
as CXL_DVSEC_PCIE_DEVICE

I have not complaints about promoting this if it is going to be used
by PCI core code, but I don't want to see two lots of definitions
for the same registers. Given there are a lot of renames
etc, do that as a precursor patch.



> +#define PCI_DVSEC_CXL_DEVCAP				0x0a
> +#define PCI_DVSEC_CXL_DEVCAP_CACHE_CAPABLE		0x00000001
> +#define PCI_DVSEC_CXL_DEVCAP_CACHE_WB_INVALIDATE	0x00000040
> +#define PCI_DVSEC_CXL_DEVCAP_CXL_RST			0x00000080
> +#define PCI_DVSEC_CXL_DEVCAP_CXL_RST_TIMEOUT_IND	0x8
> +#define PCI_DVSEC_CXL_DEVCAP_CXL_RST_TIMEOUT_MASK	0x00000700
> +#define PCI_DVSEC_CXL_DEVCAP_CXL_RST_MEM_CLR		0x00000800
> +#define PCI_DVSEC_CXL_DEVCTL				0x0c
> +#define PCI_DVSEC_CXL_DEVCTL2				0x10
> +#define PCI_DVSEC_CXL_DEVCTL2_DISABLE_CACHING		0x1
> +#define PCI_DVSEC_CXL_DEVCTL2_INIT_CACHE_WB_INVALIDATE	0x2
> +#define PCI_DVSEC_CXL_DEVCTL2_CXL_INIT_RST		0x4
> +#define PCI_DVSEC_CXL_DEVCTL2_CXL_RST_MEM_CLR_ENABLE	0x8
> +#define PCI_DVSEC_CXL_DEVSTATUS2			0x12
> +#define PCI_DVSEC_CXL_DEVSTATUS2_CACHE_INVALID		0x1
> +#define PCI_DVSEC_CXL_DEVSTATUS2_RST_COMPLETE		0x2
> +#define PCI_DVSEC_CXL_DEVSTATUS2_RST_ERR		0x4
> +#define PCI_DVSEC_CXL_DEVCAP2				0x16
> +#define PCI_DVSEC_CXL_DEVCAP2_CACHE_SIZE_UNIT		0x0000000F
> +#define PCI_DVSEC_CXL_DEVCAP2_CACHE_SIZE_UNIT_0		0x0
> +#define PCI_DVSEC_CXL_DEVCAP2_CACHE_SIZE_UNIT_1		0x40
> +#define PCI_DVSEC_CXL_DEVCAP2_CACHE_SIZE_UNIT_2		0x400
> +#define PCI_DVSEC_CXL_DEVCAP2_CACHE_SIZE(x)		(((x) & 0x0000FF00) >> 8)
> +
>  #define PCI_DVSEC_CXL_PORT				3
>  #define PCI_DVSEC_CXL_PORT_CTL				0x0c
>  #define PCI_DVSEC_CXL_PORT_CTL_UNMASK_SBR		0x00000001

Re: [PATCH v1 1/1] cxl: add support for cxl reset

[Srirangan Madhavan]

Thank you Jonathan, Ira & DJ.
I've addressed the comments and sent out v2.

> We need to deduplicate the stuff in here and drivers/cxl/cxlpci.h
> as CXL_DVSEC_PCIE_DEVICE
>
> I have not complaints about promoting this if it is going to be used
> by PCI core code, but I don't want to see two lots of definitions
> for the same registers. Given there are a lot of renames
> etc, do that as a precursor patch.

I've borrowed the changes that Alejandro's T2 support patches made
and moved them to a common header to remove dupication.

Regards,
Srirangan.

________________________________________
From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Sent: Friday, February 14, 2025 8:57 AM
To: Srirangan Madhavan
Cc: Davidlohr Bueso; Dave Jiang; Alison Schofield; Vishal Verma; Ira Weiny; Dan Williams; Zhi Wang; Vishal Aslot; Shanker Donthineni; linux-cxl@vger.kernel.org
Subject: Re: [PATCH v1 1/1] cxl: add support for cxl reset

External email: Use caution opening links or attachments


On Fri, 7 Feb 2025 01:03:27 -0800
Srirangan Madhavan <smadhavan@nvidia.com> wrote:

> This change adds the support and implements the CXL reset
> steps as laid out by the CXL Spec v3.1 Sections 9.6 & 9.7.
>
> With support for Type 2 devices being introduced, more devices will
> require finer-grained reset mechanisms beyond bus-wide reset methods.
>
> This change defines the necessary CXL DVSEC register macros.
> For devices that support CXL Reset, cache lines are disabled, WB+I is
> asserted, wait for cache invalid status, Mem Clr bit is asserted and
> finally reset is initiated.
>
> Signed-off-by: Srirangan Madhavan <smadhavan@nvidia.com>
Hi Sriranagan,

Various style related comments inline.  My main suggestion is
to match local style of the file you are editing where possible.
Also look for duplication of defines etc.

I haven't checked the spec for precise sequence but will
do that on a v2 once the more superficial stuff is tidied up.

Thanks,

Jonathan


> ---
>  drivers/pci/pci.c             | 183 ++++++++++++++++++++++++++++++++++
>  include/linux/pci.h           |   2 +-
>  include/uapi/linux/pci_regs.h |  25 +++++
>  3 files changed, 209 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 869d204a70a3..cf6009f5bd6c 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -5026,6 +5026,12 @@ static int pci_dev_reset_slot_function(struct pci_dev *dev, bool probe)
>       return pci_reset_hotplug_slot(dev->slot->hotplug, probe);
>  }
>
> +static u16 cxl_device_dvsec(struct pci_dev *dev)
> +{
> +     return pci_find_dvsec_capability(dev, PCI_VENDOR_ID_CXL,
> +                                      PCI_DVSEC_CXL_DEV);
> +}
> +
>  static u16 cxl_port_dvsec(struct pci_dev *dev)
>  {
>       return pci_find_dvsec_capability(dev, PCI_VENDOR_ID_CXL,
> @@ -5116,6 +5122,182 @@ static int cxl_reset_bus_function(struct pci_dev *dev, bool probe)
>       return rc;
>  }
>
> +static int cxl_reset_prepare(struct pci_dev *dev, u16 dvsec)
> +{
> +     u16 reg, val, cap;
> +     int rc;
> +     u32 timeout_us = 100, timeout_tot_us = 10000;
File seems to at least at first glance use reverse xmas tree.
So do that here as well.

> +
> +     /*
> +      * Wait for any pending transactions.
> +      * Assuming this does cxl.io stuff.
> +      */
> +     if (!pci_wait_for_pending_transaction(dev))
> +             pci_err(dev, "timed out waiting for pending transaction; performing cxl reset anyway\n");
> +
> +     /*
> +      * Disable caching and then write back and invalidate lines.
> +      */

Single line comment.

> +     rc = pci_read_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCAP,
> +                               &cap);
One line. It is under 80 chars anyway.

> +     if (rc)
> +             return rc;
> +
> +     if (!(cap & PCI_DVSEC_CXL_DEVCAP_CACHE_CAPABLE))
> +             return 0;
> +
> +     /*
> +      * Disable cache.
> +      * WB and invalidate cahce if capability is advertised.

cache

> +      */
> +     rc = pci_read_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCTL2,
> +                               &reg);

One line. It fits I think.

> +     if (rc)
> +             return rc;
> +     val = reg | PCI_DVSEC_CXL_DEVCTL2_DISABLE_CACHING;

I'd do this in place in reg.

> +
> +     if (cap & PCI_DVSEC_CXL_DEVCAP_CACHE_WB_INVALIDATE)
> +             val = reg | PCI_DVSEC_CXL_DEVCTL2_INIT_CACHE_WB_INVALIDATE;

I think intent is keep disable caching set for this one?
If not use an else to make that clear.

> +     pci_write_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCTL2,
> +                           val);

Single line.  Check all the code for this.


> +
> +     /*
> +      * From Section 9.6: "Software may leverage the cache size reported in
> +      * the DVSEC CXL Capability2 register to compute a suitable timeout
> +      * value".
> +      * Given there is no conversion factor for cache size -> timeout,
> +      * setting timer for default 10ms.
> +      */
> +     do {
> +             if (timeout_tot_us < 0)
> +                     return -ETIMEDOUT;
> +             usleep_range(timeout_us, timeout_us+1);

Spaces around +

> +             timeout_tot_us -= timeout_us;
> +             rc = pci_read_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCTL2,
> +                                       &reg);
> +             if (rc)
> +                     return rc;
> +     } while (!(reg & PCI_DVSEC_CXL_DEVSTATUS2_CACHE_INVALID));
> +
> +     return 0;
> +}
> +
> +/**
> + * cxl_reset_init - initiate a cxl reset
> + * @dev: device to reset
> + *
> + * Initiate a cxl reset.
> + */
> +static int cxl_reset_init(struct pci_dev *dev, u16 dvsec)
> +{
> +     u16 reg, val;
> +     u32 timeout_ms;
> +     int rc;
> +     u32 reset_timeouts_ms[] = {10, 100, 1000, 10000, 100000};
Local style is reverse xmas and spaces after { and before } in lines
like this (well the one I could find anyway).

> +
> +     /*
> +      * Check if CXL Reset MEM CLR is supported.
> +      */
Single line comment is fine.

> +     rc = pci_read_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCAP,
> +                               &reg);
> +     if (rc)
> +             return rc;
> +
> +     if (reg & PCI_DVSEC_CXL_DEVCAP_CXL_RST_MEM_CLR) {
> +             rc = pci_read_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCTL2,
> +                                       &reg);
> +             if (rc)
> +                     return rc;
> +
> +             val = reg | PCI_DVSEC_CXL_DEVCTL2_CXL_RST_MEM_CLR_ENABLE;
> +             pci_write_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCTL2,
> +                                   val);
> +     }
> +
> +     /*
> +      * Read timeout value
> +      */

Single line comment.

> +     rc = pci_read_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCAP,
> +                               &reg);
> +     if (rc)
> +             return rc;
> +     timeout_ms = reset_timeouts_ms[FIELD_GET(PCI_DVSEC_CXL_DEVCAP_CXL_RST_TIMEOUT_MASK, reg)];

Rather long line. I'd be tempted to use a local variable for the FIELD_GET()

> +
> +     /*
> +      * Write reset config
Single line comment.
> +      */
> +     rc = pci_read_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCTL2,
> +                               &reg);
> +     if (rc)
> +             return rc;
> +
> +     val = reg | PCI_DVSEC_CXL_DEVCTL2_CXL_INIT_RST;
> +     pci_write_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCTL2,
> +                           val);
> +
> +     /*
> +      *  Wait till timeout and then check reset status is complete.
Extra space before W and I think fits on single line anyway.
> +      */
> +     msleep(timeout_ms);
> +     rc = pci_read_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVSTATUS2,
> +                               &reg);
> +     if (rc)
> +             return rc;
> +     if (reg & PCI_DVSEC_CXL_DEVSTATUS2_RST_ERR ||
> +         ~reg & PCI_DVSEC_CXL_DEVSTATUS2_RST_COMPLETE)
> +             return -ETIMEDOUT;
> +
> +     /*
> +      * Revert cashing disable.
> +      */
> +     rc = pci_read_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCTL2,
> +                               &reg);
> +     if (rc)
> +             return rc;
> +     val = (reg & (~PCI_DVSEC_CXL_DEVCTL2_DISABLE_CACHING));
> +     pci_write_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCTL2,
> +                           val);
> +
> +     return 0;
> +}
> +
> +/**
> + * cxl_reset - initiate a cxl reset
> + * @dev: device to reset
> + * @probe: if true, return 0 if device can be reset this way
> + *
> + * Initiate a cxl reset on @dev.
> + */
> +static int cxl_reset(struct pci_dev *dev, bool probe)
> +{
> +     u16 dvsec, reg;
> +     int rc;
> +
> +     dvsec = cxl_device_dvsec(dev);
> +     if (!dvsec)
> +             return -ENOTTY;
> +
> +     /*
> +      * Check if CXL Reset is supported.
> +      */

        /* Check if CXL Reset is supported. */

> +     rc = pci_read_config_word(dev, dvsec + PCI_DVSEC_CXL_DEVCAP,
> +                               &reg);

One line. It is under 80 chars.

> +     if (rc)
> +             return -ENOTTY;
> +
> +     if (~(reg & PCI_DVSEC_CXL_DEVCAP_CXL_RST))

Using ~ for a single bit is unusual. I'd go with

if (reg & PCI_DVSEC_CXL_DEVCAP_CXL_RST == 0)
or !(reg & ...)



> +             return -ENOTTY;
> +
> +     if (probe)
> +             return 0;
> +
> +     rc = cxl_reset_prepare(dev, dvsec);
> +     if (rc)
> +             return rc;
> +
> +     return cxl_reset_init(dev, dvsec);
> +}

> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 47b31ad724fa..efcb06598f26 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -51,7 +51,7 @@
>                              PCI_STATUS_PARITY)
>
>  /* Number of reset methods used in pci_reset_fn_methods array in pci.c */
> -#define PCI_NUM_RESET_METHODS 8
> +#define PCI_NUM_RESET_METHODS 9
>
>  #define PCI_RESET_PROBE              true
>  #define PCI_RESET_DO_RESET   false
> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> index 3445c4970e4d..52618c5b095d 100644
> --- a/include/uapi/linux/pci_regs.h
> +++ b/include/uapi/linux/pci_regs.h
> @@ -1209,6 +1209,31 @@
>  #define PCI_DOE_DATA_OBJECT_DISC_RSP_3_NEXT_INDEX    0xff000000
>
>  /* Compute Express Link (CXL r3.1, sec 8.1.5) */
> +#define PCI_DVSEC_CXL_DEV                            0

We need to deduplicate the stuff in here and drivers/cxl/cxlpci.h
as CXL_DVSEC_PCIE_DEVICE

I have not complaints about promoting this if it is going to be used
by PCI core code, but I don't want to see two lots of definitions
for the same registers. Given there are a lot of renames
etc, do that as a precursor patch.



> +#define PCI_DVSEC_CXL_DEVCAP                         0x0a
> +#define PCI_DVSEC_CXL_DEVCAP_CACHE_CAPABLE           0x00000001
> +#define PCI_DVSEC_CXL_DEVCAP_CACHE_WB_INVALIDATE     0x00000040
> +#define PCI_DVSEC_CXL_DEVCAP_CXL_RST                 0x00000080
> +#define PCI_DVSEC_CXL_DEVCAP_CXL_RST_TIMEOUT_IND     0x8
> +#define PCI_DVSEC_CXL_DEVCAP_CXL_RST_TIMEOUT_MASK    0x00000700
> +#define PCI_DVSEC_CXL_DEVCAP_CXL_RST_MEM_CLR         0x00000800
> +#define PCI_DVSEC_CXL_DEVCTL                         0x0c
> +#define PCI_DVSEC_CXL_DEVCTL2                                0x10
> +#define PCI_DVSEC_CXL_DEVCTL2_DISABLE_CACHING                0x1
> +#define PCI_DVSEC_CXL_DEVCTL2_INIT_CACHE_WB_INVALIDATE       0x2
> +#define PCI_DVSEC_CXL_DEVCTL2_CXL_INIT_RST           0x4
> +#define PCI_DVSEC_CXL_DEVCTL2_CXL_RST_MEM_CLR_ENABLE 0x8
> +#define PCI_DVSEC_CXL_DEVSTATUS2                     0x12
> +#define PCI_DVSEC_CXL_DEVSTATUS2_CACHE_INVALID               0x1
> +#define PCI_DVSEC_CXL_DEVSTATUS2_RST_COMPLETE                0x2
> +#define PCI_DVSEC_CXL_DEVSTATUS2_RST_ERR             0x4
> +#define PCI_DVSEC_CXL_DEVCAP2                                0x16
> +#define PCI_DVSEC_CXL_DEVCAP2_CACHE_SIZE_UNIT                0x0000000F
> +#define PCI_DVSEC_CXL_DEVCAP2_CACHE_SIZE_UNIT_0              0x0
> +#define PCI_DVSEC_CXL_DEVCAP2_CACHE_SIZE_UNIT_1              0x40
> +#define PCI_DVSEC_CXL_DEVCAP2_CACHE_SIZE_UNIT_2              0x400
> +#define PCI_DVSEC_CXL_DEVCAP2_CACHE_SIZE(x)          (((x) & 0x0000FF00) >> 8)
> +
>  #define PCI_DVSEC_CXL_PORT                           3
>  #define PCI_DVSEC_CXL_PORT_CTL                               0x0c
>  #define PCI_DVSEC_CXL_PORT_CTL_UNMASK_SBR            0x00000001