* Re: [PATCH v1 1/1] cxl: add support for cxl reset
2025-02-07 9:03 ` [PATCH v1 1/1] cxl: add support for cxl reset Srirangan Madhavan
@ 2025-02-07 15:19 ` Dave Jiang
2025-02-07 17:15 ` Ira Weiny
` (2 subsequent siblings)
3 siblings, 0 replies; 8+ messages in thread
From: Dave Jiang @ 2025-02-07 15:19 UTC (permalink / raw)
To: Srirangan Madhavan, Davidlohr Bueso, Jonathan Cameron,
Alison Schofield, Vishal Verma, Ira Weiny, Dan Williams
Cc: Zhi Wang, Vishal Aslot, Shanker Donthineni, linux-cxl
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,
> + ®);
> + 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,
> + ®);
> + 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,
> + ®);
> + 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,
> + ®);
> + 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,
> + ®);
> + 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,
> + ®);
> + 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,
> + ®);
> + 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,
> + ®);
> + 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,
> + ®);
> + 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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v1 1/1] cxl: add support for cxl reset
2025-02-07 9:03 ` [PATCH v1 1/1] cxl: add support for cxl reset Srirangan Madhavan
2025-02-07 15:19 ` Dave Jiang
@ 2025-02-07 17:15 ` Ira Weiny
2025-02-13 7:35 ` Srirangan Madhavan
2025-02-08 11:48 ` kernel test robot
2025-02-14 16:57 ` Jonathan Cameron
3 siblings, 1 reply; 8+ messages in thread
From: Ira Weiny @ 2025-02-07 17:15 UTC (permalink / raw)
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
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,
> + ®);
> + 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,
> + ®);
> + 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,
> + ®);
> + 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,
> + ®);
> + 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,
> + ®);
> + 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,
> + ®);
> + 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,
> + ®);
> + 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,
> + ®);
> + 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,
> + ®);
> + 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]
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v1 1/1] cxl: add support for cxl reset
2025-02-07 17:15 ` Ira Weiny
@ 2025-02-13 7:35 ` Srirangan Madhavan
0 siblings, 0 replies; 8+ messages in thread
From: Srirangan Madhavan @ 2025-02-13 7:35 UTC (permalink / raw)
To: Ira Weiny, Davidlohr Bueso, Jonathan Cameron, Dave Jiang,
Alison Schofield, Vishal Verma, Dan Williams, Alejandro Lucero
Cc: Zhi Wang, Vishal Aslot, Shanker Donthineni,
linux-cxl@vger.kernel.org
@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,
> + ®);
> + 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,
> + ®);
> + 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,
> + ®);
> + 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,
> + ®);
> + 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,
> + ®);
> + 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,
> + ®);
> + 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,
> + ®);
> + 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,
> + ®);
> + 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,
> + ®);
> + 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]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v1 1/1] cxl: add support for cxl reset
2025-02-07 9:03 ` [PATCH v1 1/1] cxl: add support for cxl reset Srirangan Madhavan
2025-02-07 15:19 ` Dave Jiang
2025-02-07 17:15 ` Ira Weiny
@ 2025-02-08 11:48 ` kernel test robot
2025-02-14 16:57 ` Jonathan Cameron
3 siblings, 0 replies; 8+ messages in thread
From: kernel test robot @ 2025-02-08 11:48 UTC (permalink / raw)
To: Srirangan Madhavan, Davidlohr Bueso, Jonathan Cameron, Dave Jiang,
Alison Schofield, Vishal Verma, Ira Weiny, Dan Williams
Cc: oe-kbuild-all, Zhi Wang, Vishal Aslot, Shanker Donthineni,
linux-cxl
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 ®);
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 ®);
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v1 1/1] cxl: add support for cxl reset
2025-02-07 9:03 ` [PATCH v1 1/1] cxl: add support for cxl reset Srirangan Madhavan
` (2 preceding siblings ...)
2025-02-08 11:48 ` kernel test robot
@ 2025-02-14 16:57 ` Jonathan Cameron
2025-02-21 5:15 ` Srirangan Madhavan
3 siblings, 1 reply; 8+ messages in thread
From: Jonathan Cameron @ 2025-02-14 16:57 UTC (permalink / raw)
To: Srirangan Madhavan
Cc: Davidlohr Bueso, Dave Jiang, Alison Schofield, Vishal Verma,
Ira Weiny, Dan Williams, Zhi Wang, Vishal Aslot,
Shanker Donthineni, linux-cxl
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,
> + ®);
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,
> + ®);
> + 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,
> + ®);
> + 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,
> + ®);
> + 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,
> + ®);
> + 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,
> + ®);
> + 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,
> + ®);
> + 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,
> + ®);
> + 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,
> + ®);
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v1 1/1] cxl: add support for cxl reset
2025-02-14 16:57 ` Jonathan Cameron
@ 2025-02-21 5:15 ` Srirangan Madhavan
0 siblings, 0 replies; 8+ messages in thread
From: Srirangan Madhavan @ 2025-02-21 5:15 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Davidlohr Bueso, Dave Jiang, Alison Schofield, Vishal Verma,
Ira Weiny, Dan Williams, Zhi Wang, Vishal Aslot,
Shanker Donthineni, linux-cxl@vger.kernel.org
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,
> + ®);
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,
> + ®);
> + 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,
> + ®);
> + 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,
> + ®);
> + 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,
> + ®);
> + 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,
> + ®);
> + 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,
> + ®);
> + 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,
> + ®);
> + 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,
> + ®);
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
^ permalink raw reply [flat|nested] 8+ messages in thread