[PATCH v4 0/2] Add support for PCIe RP PERST#

[PATCH v4 0/2] Add support for PCIe RP PERST#

[Sai Krishna Musham]

Add support for PCIe Root Port PERST# signal.

Add `reset-gpios` property to the Versal CPM PCIe controller binding.
Add CPM clock and reset control register base for handling Versal CPM PCIe IP reset.

Sai Krishna Musham (2):
  dt-bindings: PCI: xilinx-cpm: Add reset-gpios for PCIe RP PERST#
  PCI: xilinx-cpm: Add support for PCIe RP PERST# signal

 .../bindings/pci/xilinx-versal-cpm.yaml       | 21 ++++--
 drivers/pci/controller/pcie-xilinx-cpm.c      | 66 ++++++++++++++++++-
 2 files changed, 81 insertions(+), 6 deletions(-)

-- 
2.44.1

[PATCH v4 1/2] dt-bindings: PCI: xilinx-cpm: Add reset-gpios for PCIe RP PERST#

[Sai Krishna Musham]

Introduce `reset-gpios` property to enable GPIO-based control of
the PCIe RP PERST# signal, generating assert and deassert signals.

Traditionally, the reset was managed in hardware and enabled during
initialization. With this patch set, the reset will be handled by the
driver. Consequently, the `reset-gpios` property must be explicitly
provided to ensure proper functionality.

Add CPM clock and reset control registers base to handle PCIe IP
reset along with PCIe RP PERST# to avoid Link Training errors.

Signed-off-by: Sai Krishna Musham <sai.krishna.musham@amd.com>
---
Changes for v4:
- Add CPM clock and reset control registers base to handle PCIe IP
  reset.
- Update commit message.

Changes for v3:
- None

Changes for v2:
- Add define from include/dt-bindings/gpio/gpio.h for PERST# polarity
- Update commit message
---
 .../bindings/pci/xilinx-versal-cpm.yaml       | 21 ++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml b/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
index d674a24c8ccc..904594138af2 100644
--- a/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
+++ b/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
@@ -24,15 +24,20 @@ properties:
     items:
       - description: CPM system level control and status registers.
       - description: Configuration space region and bridge registers.
+      - description: CPM clock and reset control registers.
       - description: CPM5 control and status registers.
-    minItems: 2
+    minItems: 3
 
   reg-names:
     items:
       - const: cpm_slcr
       - const: cfg
+      - const: cpm_crx
       - const: cpm_csr
-    minItems: 2
+    minItems: 3
+
+  reset-gpios:
+    description: GPIO used as PERST# signal
 
   interrupts:
     maxItems: 1
@@ -64,6 +69,7 @@ properties:
 required:
   - reg
   - reg-names
+  - reset-gpios
   - "#interrupt-cells"
   - interrupts
   - interrupt-map
@@ -76,6 +82,7 @@ unevaluatedProperties: false
 
 examples:
   - |
+    #include <dt-bindings/gpio/gpio.h>
 
     versal {
                #address-cells = <2>;
@@ -98,8 +105,10 @@ examples:
                                 <0x43000000 0x80 0x00000000 0x80 0x00000000 0x0 0x80000000>;
                        msi-map = <0x0 &its_gic 0x0 0x10000>;
                        reg = <0x0 0xfca10000 0x0 0x1000>,
-                             <0x6 0x00000000 0x0 0x10000000>;
-                       reg-names = "cpm_slcr", "cfg";
+                             <0x6 0x00000000 0x0 0x10000000>,
+                             <0x0 0xfca00000 0x0 10000>;
+                       reg-names = "cpm_slcr", "cfg", "cpm_crx";
+                       reset-gpios = <&gpio1 38 GPIO_ACTIVE_LOW>;
                        pcie_intc_0: interrupt-controller {
                                #address-cells = <0>;
                                #interrupt-cells = <1>;
@@ -126,8 +135,10 @@ examples:
                        msi-map = <0x0 &its_gic 0x0 0x10000>;
                        reg = <0x00 0xfcdd0000 0x00 0x1000>,
                              <0x06 0x00000000 0x00 0x1000000>,
+                             <0x00 0xfcdc0000 0x00 0x10000>,
                              <0x00 0xfce20000 0x00 0x1000000>;
-                       reg-names = "cpm_slcr", "cfg", "cpm_csr";
+                       reg-names = "cpm_slcr", "cfg", "cpm_crx", "cpm_csr";
+                       reset-gpios = <&gpio1 38 GPIO_ACTIVE_LOW>;
 
                        pcie_intc_1: interrupt-controller {
                                #address-cells = <0>;
-- 
2.44.1

[PATCH v4 2/2] PCI: xilinx-cpm: Add support for PCIe RP PERST# signal

[Sai Krishna Musham]

Add PCIe IP reset along with GPIO-based control for the PCIe Root
Port PERST# signal. Synchronizing the PCIe IP reset with the PERST#
signal's assertion and deassertion avoids Link Training failures.

Add clear firewall after Link reset for CPM5NC.

Adapt to use GPIO framework and make reset optional to maintain
backward compatibility with existing DTBs.

Signed-off-by: Sai Krishna Musham <sai.krishna.musham@amd.com>
---
Changes for v4:
- Add PCIe PERST# support for CPM5NC.
- Add PCIe IP reset along with PERST# to avoid Link Training Errors.
- Remove PCIE_T_PVPERL_MS define and PCIE_T_RRS_READY_MS after
  PERST# deassert.
- Move PCIe PERST# assert and deassert logic to
  xilinx_cpm_pcie_init_port() before cpm_pcie_link_up(), since
  Interrupts enable and PCIe RP bridge enable should be done after
  Link up.
- Update commit message.

Changes for v3:
- Use PCIE_T_PVPERL_MS define.

Changes for v2:
- Make the request GPIO optional.
- Correct the reset sequence as per PERST#
- Update commit message
---
 drivers/pci/controller/pcie-xilinx-cpm.c | 66 +++++++++++++++++++++++-
 1 file changed, 65 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/pcie-xilinx-cpm.c b/drivers/pci/controller/pcie-xilinx-cpm.c
index d0ab187d917f..fd1fee2f614b 100644
--- a/drivers/pci/controller/pcie-xilinx-cpm.c
+++ b/drivers/pci/controller/pcie-xilinx-cpm.c
@@ -6,6 +6,8 @@
  */
 
 #include <linux/bitfield.h>
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
 #include <linux/interrupt.h>
 #include <linux/irq.h>
 #include <linux/irqchip.h>
@@ -21,6 +23,13 @@
 #include "pcie-xilinx-common.h"
 
 /* Register definitions */
+#define XILINX_CPM_PCIE0_RST		0x00000308
+#define XILINX_CPM5_PCIE0_RST		0x00000318
+#define XILINX_CPM5_PCIE1_RST		0x0000031C
+#define XILINX_CPM5NC_PCIE0_RST		0x00000324
+
+#define XILINX_CPM5NC_PCIE0_FW		0x00001140
+
 #define XILINX_CPM_PCIE_REG_IDR		0x00000E10
 #define XILINX_CPM_PCIE_REG_IMR		0x00000E14
 #define XILINX_CPM_PCIE_REG_PSCR	0x00000E1C
@@ -99,6 +108,7 @@ struct xilinx_cpm_variant {
 	u32 ir_status;
 	u32 ir_enable;
 	u32 ir_misc_value;
+	u32 cpm_pcie_rst;
 };
 
 /**
@@ -106,6 +116,8 @@ struct xilinx_cpm_variant {
  * @dev: Device pointer
  * @reg_base: Bridge Register Base
  * @cpm_base: CPM System Level Control and Status Register(SLCR) Base
+ * @crx_base: CPM Clock and Reset Control Registers Base
+ * @cpm5nc_base: CPM5NC Control and Status Registers Base
  * @intx_domain: Legacy IRQ domain pointer
  * @cpm_domain: CPM IRQ domain pointer
  * @cfg: Holds mappings of config space window
@@ -118,6 +130,8 @@ struct xilinx_cpm_pcie {
 	struct device			*dev;
 	void __iomem			*reg_base;
 	void __iomem			*cpm_base;
+	void __iomem			*crx_base;
+	void __iomem			*cpm5nc_base;
 	struct irq_domain		*intx_domain;
 	struct irq_domain		*cpm_domain;
 	struct pci_config_window	*cfg;
@@ -478,9 +492,42 @@ static int xilinx_cpm_setup_irq(struct xilinx_cpm_pcie *port)
 static void xilinx_cpm_pcie_init_port(struct xilinx_cpm_pcie *port)
 {
 	const struct xilinx_cpm_variant *variant = port->variant;
+	struct device *dev = port->dev;
+	struct gpio_desc *reset_gpio;
+
+	/* Request the GPIO for PCIe reset signal */
+	reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(reset_gpio)) {
+		dev_err(dev, "Failed to request reset GPIO\n");
+		return;
+	}
+
+	/* Assert the reset signal */
+	gpiod_set_value(reset_gpio, 1);
 
-	if (variant->version == CPM5NC_HOST)
+	/* Assert the PCIe IP reset */
+	writel_relaxed(0x1, port->crx_base + variant->cpm_pcie_rst);
+
+	/* Controller specific delay */
+	udelay(50);
+
+	/* Deassert the PCIe IP reset */
+	writel_relaxed(0x0, port->crx_base + variant->cpm_pcie_rst);
+
+	/* Deassert the reset signal */
+	gpiod_set_value(reset_gpio, 0);
+	mdelay(PCIE_T_RRS_READY_MS);
+
+	if (variant->version == CPM5NC_HOST) {
+		/* Clear Firewall */
+		writel_relaxed(0x00, port->cpm5nc_base +
+			       XILINX_CPM5NC_PCIE0_FW);
+		writel_relaxed(0x01, port->cpm5nc_base +
+			       XILINX_CPM5NC_PCIE0_FW);
+		writel_relaxed(0x00, port->cpm5nc_base +
+			       XILINX_CPM5NC_PCIE0_FW);
 		return;
+	}
 
 	if (cpm_pcie_link_up(port))
 		dev_info(port->dev, "PCIe Link is UP\n");
@@ -551,6 +598,19 @@ static int xilinx_cpm_pcie_parse_dt(struct xilinx_cpm_pcie *port,
 		port->reg_base = port->cfg->win;
 	}
 
+	port->crx_base = devm_platform_ioremap_resource_byname(pdev,
+							       "cpm_crx");
+	if (IS_ERR(port->crx_base))
+		return PTR_ERR(port->crx_base);
+
+	if (port->variant->version == CPM5NC_HOST) {
+		port->cpm5nc_base =
+			devm_platform_ioremap_resource_byname(pdev,
+							      "cpm5nc_csr");
+		if (IS_ERR(port->cpm5nc_base))
+			return PTR_ERR(port->cpm5nc_base);
+	}
+
 	return 0;
 }
 
@@ -635,6 +695,7 @@ static int xilinx_cpm_pcie_probe(struct platform_device *pdev)
 static const struct xilinx_cpm_variant cpm_host = {
 	.version = CPM,
 	.ir_misc_value = XILINX_CPM_PCIE0_MISC_IR_LOCAL,
+	.cpm_pcie_rst = XILINX_CPM_PCIE0_RST,
 };
 
 static const struct xilinx_cpm_variant cpm5_host = {
@@ -642,6 +703,7 @@ static const struct xilinx_cpm_variant cpm5_host = {
 	.ir_misc_value = XILINX_CPM_PCIE0_MISC_IR_LOCAL,
 	.ir_status = XILINX_CPM_PCIE0_IR_STATUS,
 	.ir_enable = XILINX_CPM_PCIE0_IR_ENABLE,
+	.cpm_pcie_rst = XILINX_CPM5_PCIE0_RST,
 };
 
 static const struct xilinx_cpm_variant cpm5_host1 = {
@@ -649,10 +711,12 @@ static const struct xilinx_cpm_variant cpm5_host1 = {
 	.ir_misc_value = XILINX_CPM_PCIE1_MISC_IR_LOCAL,
 	.ir_status = XILINX_CPM_PCIE1_IR_STATUS,
 	.ir_enable = XILINX_CPM_PCIE1_IR_ENABLE,
+	.cpm_pcie_rst = XILINX_CPM5_PCIE1_RST,
 };
 
 static const struct xilinx_cpm_variant cpm5n_host = {
 	.version = CPM5NC_HOST,
+	.cpm_pcie_rst = XILINX_CPM5NC_PCIE0_RST,
 };
 
 static const struct of_device_id xilinx_cpm_pcie_of_match[] = {
-- 
2.44.1

Re: [PATCH v4 1/2] dt-bindings: PCI: xilinx-cpm: Add reset-gpios for PCIe RP PERST#

[Krzysztof Kozlowski]

On 18/03/2025 10:26, Sai Krishna Musham wrote:
> Changes for v2:
> - Add define from include/dt-bindings/gpio/gpio.h for PERST# polarity
> - Update commit message
> ---
>  .../bindings/pci/xilinx-versal-cpm.yaml       | 21 ++++++++++++++-----
>  1 file changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml b/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
> index d674a24c8ccc..904594138af2 100644
> --- a/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
> +++ b/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
> @@ -24,15 +24,20 @@ properties:
>      items:
>        - description: CPM system level control and status registers.
>        - description: Configuration space region and bridge registers.
> +      - description: CPM clock and reset control registers.
>        - description: CPM5 control and status registers.

You cannot add items to the middle, that's an ABI break. Adding required
properties is also an ABI break. Why you cannot add it to the end of the
list?

Or at least explain ABI break impact in commit msg?


> -    minItems: 2
> +    minItems: 3
>  
>    reg-names:
>      items:
>        - const: cpm_slcr
>        - const: cfg
> +      - const: cpm_crx
>        - const: cpm_csr
> -    minItems: 2
> +    minItems: 3
> +
> +  reset-gpios:
> +    description: GPIO used as PERST# signal

Isn't this already in pci-bus-common.yaml?


Best regards,
Krzysztof

Re: [PATCH v4 2/2] PCI: xilinx-cpm: Add support for PCIe RP PERST# signal

[Krzysztof Kozlowski]

On 18/03/2025 10:26, Sai Krishna Musham wrote:
>  	const struct xilinx_cpm_variant *variant = port->variant;
> +	struct device *dev = port->dev;
> +	struct gpio_desc *reset_gpio;
> +
> +	/* Request the GPIO for PCIe reset signal */
> +	reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(reset_gpio)) {
> +		dev_err(dev, "Failed to request reset GPIO\n");

Isn't this probe path? If not, then why? How are you going to handle
deferrer probe?

> +		return;> +	}
> +
> +	/* Assert the reset signal */
> +	gpiod_set_value(reset_gpio, 1);

It was already asserted.

>  
> -	if (variant->version == CPM5NC_HOST)
> +	/* Assert the PCIe IP reset */
> +	writel_relaxed(0x1, port->crx_base + variant->cpm_pcie_rst);
> +
> +	/* Controller specific delay */
> +	udelay(50);
> +
> +	/* Deassert the PCIe IP reset */
> +	writel_relaxed(0x0, port->crx_base + variant->cpm_pcie_rst);
> +
> +	/* Deassert the reset signal */
> +	gpiod_set_value(reset_gpio, 0);
> +	mdelay(PCIE_T_RRS_READY_MS);
> +
> +	if (variant->version == CPM5NC_HOST) {
> +		/* Clear Firewall */
> +		writel_relaxed(0x00, port->cpm5nc_base +
> +			       XILINX_CPM5NC_PCIE0_FW);
> +		writel_relaxed(0x01, port->cpm5nc_base +
> +			       XILINX_CPM5NC_PCIE0_FW);
> +		writel_relaxed(0x00, port->cpm5nc_base +
> +			       XILINX_CPM5NC_PCIE0_FW);
>  		return;
> +	}
>  
>  	if (cpm_pcie_link_up(port))
>  		dev_info(port->dev, "PCIe Link is UP\n");
> @@ -551,6 +598,19 @@ static int xilinx_cpm_pcie_parse_dt(struct xilinx_cpm_pcie *port,
>  		port->reg_base = port->cfg->win;
>  	}
>  
> +	port->crx_base = devm_platform_ioremap_resource_byname(pdev,
> +							       "cpm_crx");

And here is the actual ABI break.

> +	if (IS_ERR(port->crx_base))
> +		return PTR_ERR(port->crx_base);



Best regards,
Krzysztof

RE: [PATCH v4 1/2] dt-bindings: PCI: xilinx-cpm: Add reset-gpios for PCIe RP PERST#

[Musham, Sai Krishna]

[AMD Official Use Only - AMD Internal Distribution Only]

Hi Krzysztof,

> -----Original Message-----
> From: Krzysztof Kozlowski <krzk@kernel.org>
> Sent: Tuesday, March 18, 2025 3:23 PM
> To: Musham, Sai Krishna <sai.krishna.musham@amd.com>;
> bhelgaas@google.com; lpieralisi@kernel.org; kw@linux.com;
> manivannan.sadhasivam@linaro.org; robh@kernel.org; krzk+dt@kernel.org;
> conor+dt@kernel.org; cassel@kernel.org
> Cc: linux-pci@vger.kernel.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org; Simek, Michal <michal.simek@amd.com>; Gogada, Bharat
> Kumar <bharat.kumar.gogada@amd.com>; Havalige, Thippeswamy
> <thippeswamy.havalige@amd.com>
> Subject: Re: [PATCH v4 1/2] dt-bindings: PCI: xilinx-cpm: Add reset-gpios for PCIe
> RP PERST#
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> On 18/03/2025 10:26, Sai Krishna Musham wrote:
> > Changes for v2:
> > - Add define from include/dt-bindings/gpio/gpio.h for PERST# polarity
> > - Update commit message
> > ---
> >  .../bindings/pci/xilinx-versal-cpm.yaml       | 21 ++++++++++++++-----
> >  1 file changed, 16 insertions(+), 5 deletions(-)
> >
> > diff --git
> > a/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
> > b/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
> > index d674a24c8ccc..904594138af2 100644
> > --- a/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
> > +++ b/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
> > @@ -24,15 +24,20 @@ properties:
> >      items:
> >        - description: CPM system level control and status registers.
> >        - description: Configuration space region and bridge registers.
> > +      - description: CPM clock and reset control registers.
> >        - description: CPM5 control and status registers.
>
> You cannot add items to the middle, that's an ABI break. Adding required properties
> is also an ABI break. Why you cannot add it to the end of the list?
>
> Or at least explain ABI break impact in commit msg?
>
When I add property at the end, I'm observing failure during dt_binding_check.
$ make DT_CHECKER_FLAGS=-m dt_binding_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
Documentation/devicetree/bindings/pci/xilinx-versal-cpm.example.dtb: pcie@fca10000: reg-names:2: 'cpm_csr' was expected
        from schema $id: http://devicetree.org/schemas/pci/xilinx-versal-cpm.yaml#
>
> > -    minItems: 2
> > +    minItems: 3
> >
> >    reg-names:
> >      items:
> >        - const: cpm_slcr
> >        - const: cfg
> > +      - const: cpm_crx
> >        - const: cpm_csr
> > -    minItems: 2
> > +    minItems: 3
> > +
> > +  reset-gpios:
> > +    description: GPIO used as PERST# signal
>
> Isn't this already in pci-bus-common.yaml?
>
>
> Best regards,
> Krzysztof

Re: [PATCH v4 2/2] PCI: xilinx-cpm: Add support for PCIe RP PERST# signal

[Manivannan Sadhasivam]

On Tue, Mar 18, 2025 at 02:56:48PM +0530, Sai Krishna Musham wrote:
> Add PCIe IP reset along with GPIO-based control for the PCIe Root
> Port PERST# signal. Synchronizing the PCIe IP reset with the PERST#
> signal's assertion and deassertion avoids Link Training failures.
> 
> Add clear firewall after Link reset for CPM5NC.
> 
> Adapt to use GPIO framework and make reset optional to maintain
> backward compatibility with existing DTBs.
> 
> Signed-off-by: Sai Krishna Musham <sai.krishna.musham@amd.com>
> ---
> Changes for v4:
> - Add PCIe PERST# support for CPM5NC.
> - Add PCIe IP reset along with PERST# to avoid Link Training Errors.
> - Remove PCIE_T_PVPERL_MS define and PCIE_T_RRS_READY_MS after
>   PERST# deassert.
> - Move PCIe PERST# assert and deassert logic to
>   xilinx_cpm_pcie_init_port() before cpm_pcie_link_up(), since
>   Interrupts enable and PCIe RP bridge enable should be done after
>   Link up.
> - Update commit message.
> 
> Changes for v3:
> - Use PCIE_T_PVPERL_MS define.
> 
> Changes for v2:
> - Make the request GPIO optional.
> - Correct the reset sequence as per PERST#
> - Update commit message
> ---
>  drivers/pci/controller/pcie-xilinx-cpm.c | 66 +++++++++++++++++++++++-
>  1 file changed, 65 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/pcie-xilinx-cpm.c b/drivers/pci/controller/pcie-xilinx-cpm.c
> index d0ab187d917f..fd1fee2f614b 100644
> --- a/drivers/pci/controller/pcie-xilinx-cpm.c
> +++ b/drivers/pci/controller/pcie-xilinx-cpm.c
> @@ -6,6 +6,8 @@
>   */
>  
>  #include <linux/bitfield.h>
> +#include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
>  #include <linux/interrupt.h>
>  #include <linux/irq.h>
>  #include <linux/irqchip.h>
> @@ -21,6 +23,13 @@
>  #include "pcie-xilinx-common.h"
>  
>  /* Register definitions */
> +#define XILINX_CPM_PCIE0_RST		0x00000308
> +#define XILINX_CPM5_PCIE0_RST		0x00000318
> +#define XILINX_CPM5_PCIE1_RST		0x0000031C
> +#define XILINX_CPM5NC_PCIE0_RST		0x00000324
> +
> +#define XILINX_CPM5NC_PCIE0_FW		0x00001140
> +
>  #define XILINX_CPM_PCIE_REG_IDR		0x00000E10
>  #define XILINX_CPM_PCIE_REG_IMR		0x00000E14
>  #define XILINX_CPM_PCIE_REG_PSCR	0x00000E1C
> @@ -99,6 +108,7 @@ struct xilinx_cpm_variant {
>  	u32 ir_status;
>  	u32 ir_enable;
>  	u32 ir_misc_value;
> +	u32 cpm_pcie_rst;
>  };
>  
>  /**
> @@ -106,6 +116,8 @@ struct xilinx_cpm_variant {
>   * @dev: Device pointer
>   * @reg_base: Bridge Register Base
>   * @cpm_base: CPM System Level Control and Status Register(SLCR) Base
> + * @crx_base: CPM Clock and Reset Control Registers Base
> + * @cpm5nc_base: CPM5NC Control and Status Registers Base
>   * @intx_domain: Legacy IRQ domain pointer
>   * @cpm_domain: CPM IRQ domain pointer
>   * @cfg: Holds mappings of config space window
> @@ -118,6 +130,8 @@ struct xilinx_cpm_pcie {
>  	struct device			*dev;
>  	void __iomem			*reg_base;
>  	void __iomem			*cpm_base;
> +	void __iomem			*crx_base;
> +	void __iomem			*cpm5nc_base;
>  	struct irq_domain		*intx_domain;
>  	struct irq_domain		*cpm_domain;
>  	struct pci_config_window	*cfg;
> @@ -478,9 +492,42 @@ static int xilinx_cpm_setup_irq(struct xilinx_cpm_pcie *port)
>  static void xilinx_cpm_pcie_init_port(struct xilinx_cpm_pcie *port)
>  {
>  	const struct xilinx_cpm_variant *variant = port->variant;
> +	struct device *dev = port->dev;
> +	struct gpio_desc *reset_gpio;
> +
> +	/* Request the GPIO for PCIe reset signal */
> +	reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(reset_gpio)) {
> +		dev_err(dev, "Failed to request reset GPIO\n");
> +		return;
> +	}
> +
> +	/* Assert the reset signal */
> +	gpiod_set_value(reset_gpio, 1);
>  
> -	if (variant->version == CPM5NC_HOST)
> +	/* Assert the PCIe IP reset */
> +	writel_relaxed(0x1, port->crx_base + variant->cpm_pcie_rst);
> +
> +	/* Controller specific delay */
> +	udelay(50);
> +
> +	/* Deassert the PCIe IP reset */
> +	writel_relaxed(0x0, port->crx_base + variant->cpm_pcie_rst);
> +
> +	/* Deassert the reset signal */
> +	gpiod_set_value(reset_gpio, 0);
> +	mdelay(PCIE_T_RRS_READY_MS);
> +
> +	if (variant->version == CPM5NC_HOST) {
> +		/* Clear Firewall */

On top of Krzk's review:

What does this 'firewall' mean? Clearly, not something defined in the PCIe spec.
Also, you made it independent of PERST# line. So is it really needed for
platforms not supporting PERST#?

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v4 1/2] dt-bindings: PCI: xilinx-cpm: Add reset-gpios for PCIe RP PERST#

[Krzysztof Kozlowski]

On 21/03/2025 10:42, Musham, Sai Krishna wrote:
> [AMD Official Use Only - AMD Internal Distribution Only]
> 
> Hi Krzysztof,
> 
>> -----Original Message-----
>> From: Krzysztof Kozlowski <krzk@kernel.org>
>> Sent: Tuesday, March 18, 2025 3:23 PM
>> To: Musham, Sai Krishna <sai.krishna.musham@amd.com>;
>> bhelgaas@google.com; lpieralisi@kernel.org; kw@linux.com;
>> manivannan.sadhasivam@linaro.org; robh@kernel.org; krzk+dt@kernel.org;
>> conor+dt@kernel.org; cassel@kernel.org
>> Cc: linux-pci@vger.kernel.org; devicetree@vger.kernel.org; linux-
>> kernel@vger.kernel.org; Simek, Michal <michal.simek@amd.com>; Gogada, Bharat
>> Kumar <bharat.kumar.gogada@amd.com>; Havalige, Thippeswamy
>> <thippeswamy.havalige@amd.com>
>> Subject: Re: [PATCH v4 1/2] dt-bindings: PCI: xilinx-cpm: Add reset-gpios for PCIe
>> RP PERST#
>>
>> Caution: This message originated from an External Source. Use proper caution
>> when opening attachments, clicking links, or responding.
>>
>>
>> On 18/03/2025 10:26, Sai Krishna Musham wrote:
>>> Changes for v2:
>>> - Add define from include/dt-bindings/gpio/gpio.h for PERST# polarity
>>> - Update commit message
>>> ---
>>>  .../bindings/pci/xilinx-versal-cpm.yaml       | 21 ++++++++++++++-----
>>>  1 file changed, 16 insertions(+), 5 deletions(-)
>>>
>>> diff --git
>>> a/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
>>> b/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
>>> index d674a24c8ccc..904594138af2 100644
>>> --- a/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
>>> +++ b/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
>>> @@ -24,15 +24,20 @@ properties:
>>>      items:
>>>        - description: CPM system level control and status registers.
>>>        - description: Configuration space region and bridge registers.
>>> +      - description: CPM clock and reset control registers.
>>>        - description: CPM5 control and status registers.
>>
>> You cannot add items to the middle, that's an ABI break. Adding required properties
>> is also an ABI break. Why you cannot add it to the end of the list?
>>
>> Or at least explain ABI break impact in commit msg?
>>
> When I add property at the end, I'm observing failure during dt_binding_check.
> $ make DT_CHECKER_FLAGS=-m dt_binding_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
> Documentation/devicetree/bindings/pci/xilinx-versal-cpm.example.dtb: pcie@fca10000: reg-names:2: 'cpm_csr' was expected
>         from schema $id: http://devicetree.org/schemas/pci/xilinx-versal-cpm.yaml#
Maybe for a good reason.

Best regards,
Krzysztof

RE: [PATCH v4 2/2] PCI: xilinx-cpm: Add support for PCIe RP PERST# signal

[Musham, Sai Krishna]

[AMD Official Use Only - AMD Internal Distribution Only]

Hi Mani,

> -----Original Message-----
> From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Sent: Friday, March 21, 2025 6:22 PM
> To: Musham, Sai Krishna <sai.krishna.musham@amd.com>
> Cc: bhelgaas@google.com; lpieralisi@kernel.org; kw@linux.com; robh@kernel.org;
> krzk+dt@kernel.org; conor+dt@kernel.org; cassel@kernel.org; linux-
> pci@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> Simek, Michal <michal.simek@amd.com>; Gogada, Bharat Kumar
> <bharat.kumar.gogada@amd.com>; Havalige, Thippeswamy
> <thippeswamy.havalige@amd.com>
> Subject: Re: [PATCH v4 2/2] PCI: xilinx-cpm: Add support for PCIe RP PERST#
> signal
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> On Tue, Mar 18, 2025 at 02:56:48PM +0530, Sai Krishna Musham wrote:
> > Add PCIe IP reset along with GPIO-based control for the PCIe Root Port
> > PERST# signal. Synchronizing the PCIe IP reset with the PERST#
> > signal's assertion and deassertion avoids Link Training failures.
> >
> > Add clear firewall after Link reset for CPM5NC.
> >
> > Adapt to use GPIO framework and make reset optional to maintain
> > backward compatibility with existing DTBs.
> >
> > Signed-off-by: Sai Krishna Musham <sai.krishna.musham@amd.com>
> > ---
> > Changes for v4:
> > - Add PCIe PERST# support for CPM5NC.
> > - Add PCIe IP reset along with PERST# to avoid Link Training Errors.
> > - Remove PCIE_T_PVPERL_MS define and PCIE_T_RRS_READY_MS after
> >   PERST# deassert.
> > - Move PCIe PERST# assert and deassert logic to
> >   xilinx_cpm_pcie_init_port() before cpm_pcie_link_up(), since
> >   Interrupts enable and PCIe RP bridge enable should be done after
> >   Link up.
> > - Update commit message.
> >
> > Changes for v3:
> > - Use PCIE_T_PVPERL_MS define.
> >
> > Changes for v2:
> > - Make the request GPIO optional.
> > - Correct the reset sequence as per PERST#
> > - Update commit message
> > ---
> >  drivers/pci/controller/pcie-xilinx-cpm.c | 66
> > +++++++++++++++++++++++-
> >  1 file changed, 65 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pci/controller/pcie-xilinx-cpm.c
> > b/drivers/pci/controller/pcie-xilinx-cpm.c
> > index d0ab187d917f..fd1fee2f614b 100644
> > --- a/drivers/pci/controller/pcie-xilinx-cpm.c
> > +++ b/drivers/pci/controller/pcie-xilinx-cpm.c
> > @@ -6,6 +6,8 @@
> >   */
> >
> >  #include <linux/bitfield.h>
> > +#include <linux/delay.h>
> > +#include <linux/gpio/consumer.h>
> >  #include <linux/interrupt.h>
> >  #include <linux/irq.h>
> >  #include <linux/irqchip.h>
> > @@ -21,6 +23,13 @@
> >  #include "pcie-xilinx-common.h"
> >
> >  /* Register definitions */
> > +#define XILINX_CPM_PCIE0_RST         0x00000308
> > +#define XILINX_CPM5_PCIE0_RST                0x00000318
> > +#define XILINX_CPM5_PCIE1_RST                0x0000031C
> > +#define XILINX_CPM5NC_PCIE0_RST              0x00000324
> > +
> > +#define XILINX_CPM5NC_PCIE0_FW               0x00001140
> > +
> >  #define XILINX_CPM_PCIE_REG_IDR              0x00000E10
> >  #define XILINX_CPM_PCIE_REG_IMR              0x00000E14
> >  #define XILINX_CPM_PCIE_REG_PSCR     0x00000E1C
> > @@ -99,6 +108,7 @@ struct xilinx_cpm_variant {
> >       u32 ir_status;
> >       u32 ir_enable;
> >       u32 ir_misc_value;
> > +     u32 cpm_pcie_rst;
> >  };
> >
> >  /**
> > @@ -106,6 +116,8 @@ struct xilinx_cpm_variant {
> >   * @dev: Device pointer
> >   * @reg_base: Bridge Register Base
> >   * @cpm_base: CPM System Level Control and Status Register(SLCR) Base
> > + * @crx_base: CPM Clock and Reset Control Registers Base
> > + * @cpm5nc_base: CPM5NC Control and Status Registers Base
> >   * @intx_domain: Legacy IRQ domain pointer
> >   * @cpm_domain: CPM IRQ domain pointer
> >   * @cfg: Holds mappings of config space window @@ -118,6 +130,8 @@
> > struct xilinx_cpm_pcie {
> >       struct device                   *dev;
> >       void __iomem                    *reg_base;
> >       void __iomem                    *cpm_base;
> > +     void __iomem                    *crx_base;
> > +     void __iomem                    *cpm5nc_base;
> >       struct irq_domain               *intx_domain;
> >       struct irq_domain               *cpm_domain;
> >       struct pci_config_window        *cfg;
> > @@ -478,9 +492,42 @@ static int xilinx_cpm_setup_irq(struct
> > xilinx_cpm_pcie *port)  static void xilinx_cpm_pcie_init_port(struct
> > xilinx_cpm_pcie *port)  {
> >       const struct xilinx_cpm_variant *variant = port->variant;
> > +     struct device *dev = port->dev;
> > +     struct gpio_desc *reset_gpio;
> > +
> > +     /* Request the GPIO for PCIe reset signal */
> > +     reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> > +     if (IS_ERR(reset_gpio)) {
> > +             dev_err(dev, "Failed to request reset GPIO\n");
> > +             return;
> > +     }
> > +
> > +     /* Assert the reset signal */
> > +     gpiod_set_value(reset_gpio, 1);
> >
> > -     if (variant->version == CPM5NC_HOST)
> > +     /* Assert the PCIe IP reset */
> > +     writel_relaxed(0x1, port->crx_base + variant->cpm_pcie_rst);
> > +
> > +     /* Controller specific delay */
> > +     udelay(50);
> > +
> > +     /* Deassert the PCIe IP reset */
> > +     writel_relaxed(0x0, port->crx_base + variant->cpm_pcie_rst);
> > +
> > +     /* Deassert the reset signal */
> > +     gpiod_set_value(reset_gpio, 0);
> > +     mdelay(PCIE_T_RRS_READY_MS);
> > +
> > +     if (variant->version == CPM5NC_HOST) {
> > +             /* Clear Firewall */
>
> On top of Krzk's review:
>
> What does this 'firewall' mean? Clearly, not something defined in the PCIe spec.
> Also, you made it independent of PERST# line. So is it really needed for platforms
> not supporting PERST#?
>
> - Mani
>
> --
> மணிவண்ணன் சதாசிவம்

Firewall is internal to CPM5NC IP, it is asserted when the PCIe Link goes DOWN and then will not allow further PCIe transactions. So, firewall mode should be cleared after the PERST# sequence.
- Sai Krishna

RE: [PATCH v4 1/2] dt-bindings: PCI: xilinx-cpm: Add reset-gpios for PCIe RP PERST#

[Musham, Sai Krishna]

[AMD Official Use Only - AMD Internal Distribution Only]

Hi Krzysztof,

Thanks, I have resolved the ABI break and added cpm_crx at the end.
I will send in v6 series.

> -----Original Message-----
> From: Krzysztof Kozlowski <krzk@kernel.org>
> Sent: Monday, March 24, 2025 12:50 PM
> To: Musham, Sai Krishna <sai.krishna.musham@amd.com>;
> bhelgaas@google.com; lpieralisi@kernel.org; kw@linux.com;
> manivannan.sadhasivam@linaro.org; robh@kernel.org; krzk+dt@kernel.org;
> conor+dt@kernel.org; cassel@kernel.org
> Cc: linux-pci@vger.kernel.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org; Simek, Michal <michal.simek@amd.com>; Gogada, Bharat
> Kumar <bharat.kumar.gogada@amd.com>; Havalige, Thippeswamy
> <thippeswamy.havalige@amd.com>
> Subject: Re: [PATCH v4 1/2] dt-bindings: PCI: xilinx-cpm: Add reset-gpios for PCIe
> RP PERST#
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> On 21/03/2025 10:42, Musham, Sai Krishna wrote:
> > [AMD Official Use Only - AMD Internal Distribution Only]
> >
> > Hi Krzysztof,
> >
> >> -----Original Message-----
> >> From: Krzysztof Kozlowski <krzk@kernel.org>
> >> Sent: Tuesday, March 18, 2025 3:23 PM
> >> To: Musham, Sai Krishna <sai.krishna.musham@amd.com>;
> >> bhelgaas@google.com; lpieralisi@kernel.org; kw@linux.com;
> >> manivannan.sadhasivam@linaro.org; robh@kernel.org;
> >> krzk+dt@kernel.org;
> >> conor+dt@kernel.org; cassel@kernel.org
> >> Cc: linux-pci@vger.kernel.org; devicetree@vger.kernel.org; linux-
> >> kernel@vger.kernel.org; Simek, Michal <michal.simek@amd.com>; Gogada,
> >> Bharat Kumar <bharat.kumar.gogada@amd.com>; Havalige, Thippeswamy
> >> <thippeswamy.havalige@amd.com>
> >> Subject: Re: [PATCH v4 1/2] dt-bindings: PCI: xilinx-cpm: Add
> >> reset-gpios for PCIe RP PERST#
> >>
> >> Caution: This message originated from an External Source. Use proper
> >> caution when opening attachments, clicking links, or responding.
> >>
> >>
> >> On 18/03/2025 10:26, Sai Krishna Musham wrote:
> >>> Changes for v2:
> >>> - Add define from include/dt-bindings/gpio/gpio.h for PERST#
> >>> polarity
> >>> - Update commit message
> >>> ---
> >>>  .../bindings/pci/xilinx-versal-cpm.yaml       | 21 ++++++++++++++-----
> >>>  1 file changed, 16 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git
> >>> a/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
> >>> b/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
> >>> index d674a24c8ccc..904594138af2 100644
> >>> --- a/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
> >>> +++ b/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
> >>> @@ -24,15 +24,20 @@ properties:
> >>>      items:
> >>>        - description: CPM system level control and status registers.
> >>>        - description: Configuration space region and bridge registers.
> >>> +      - description: CPM clock and reset control registers.
> >>>        - description: CPM5 control and status registers.
> >>
> >> You cannot add items to the middle, that's an ABI break. Adding
> >> required properties is also an ABI break. Why you cannot add it to the end of the
> list?
> >>
> >> Or at least explain ABI break impact in commit msg?
> >>
> > When I add property at the end, I'm observing failure during dt_binding_check.
> > $ make DT_CHECKER_FLAGS=-m dt_binding_check
> > DT_SCHEMA_FILES=Documentation/devicetree/bindings/pci/xilinx-versal-cp
> > m.yaml
> > Documentation/devicetree/bindings/pci/xilinx-versal-cpm.example.dtb:
> pcie@fca10000: reg-names:2: 'cpm_csr' was expected
> >         from schema $id:
> > http://devicetree.org/schemas/pci/xilinx-versal-cpm.yaml#
> Maybe for a good reason.
>
> Best regards,
> Krzysztof