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

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

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