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

[PATCH v6 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       | 72 ++++++++++++----
 drivers/pci/controller/pcie-xilinx-cpm.c      | 86 ++++++++++++++++++-
 2 files changed, 137 insertions(+), 21 deletions(-)

-- 
2.44.1

[PATCH v6 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 (`cpm_crx`) 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 v6:
- Resolve ABI break.
- Update commit message.

Changes for v5:
- Remove `reset-gpios` property from required as it is already present
  in pci-bus-common.yaml
- Update commit message

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       | 72 ++++++++++++++-----
 1 file changed, 55 insertions(+), 17 deletions(-)

diff --git a/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml b/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
index d674a24c8ccc..26e9cea41889 100644
--- a/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
+++ b/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
@@ -9,9 +9,6 @@ title: CPM Host Controller device tree for Xilinx Versal SoCs
 maintainers:
   - Bharat Kumar Gogada <bharat.kumar.gogada@amd.com>
 
-allOf:
-  - $ref: /schemas/pci/pci-host-bridge.yaml#
-
 properties:
   compatible:
     enum:
@@ -21,18 +18,12 @@ properties:
       - xlnx,versal-cpm5nc-host
 
   reg:
-    items:
-      - description: CPM system level control and status registers.
-      - description: Configuration space region and bridge registers.
-      - description: CPM5 control and status registers.
-    minItems: 2
+    minItems: 3
+    maxItems: 4
 
   reg-names:
-    items:
-      - const: cpm_slcr
-      - const: cfg
-      - const: cpm_csr
-    minItems: 2
+    minItems: 3
+    maxItems: 4
 
   interrupts:
     maxItems: 1
@@ -72,10 +63,53 @@ required:
   - msi-map
   - interrupt-controller
 
+allOf:
+  - $ref: /schemas/pci/pci-host-bridge.yaml#
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - xlnx,versal-cpm-host-1.00
+    then:
+      properties:
+        reg:
+          items:
+            - description: CPM system level control and status registers.
+            - description: Configuration space region and bridge registers.
+            - description: CPM clock and reset control registers.
+        reg-names:
+          items:
+            - const: cpm_slcr
+            - const: cfg
+            - const: cpm_crx
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - xlnx,versal-cpm5-host
+              - xlnx,versal-cpm5-host1
+    then:
+      properties:
+        reg:
+          items:
+            - description: CPM system level control and status registers.
+            - description: Configuration space region and bridge registers.
+            - description: CPM5 control and status registers.
+            - description: CPM clock and reset control registers.
+        reg-names:
+          items:
+            - const: cpm_slcr
+            - const: cfg
+            - const: cpm_csr
+            - const: cpm_crx
+
 unevaluatedProperties: false
 
 examples:
   - |
+    #include <dt-bindings/gpio/gpio.h>
 
     versal {
                #address-cells = <2>;
@@ -98,8 +132,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 +162,10 @@ examples:
                        msi-map = <0x0 &its_gic 0x0 0x10000>;
                        reg = <0x00 0xfcdd0000 0x00 0x1000>,
                              <0x06 0x00000000 0x00 0x1000000>,
-                             <0x00 0xfce20000 0x00 0x1000000>;
-                       reg-names = "cpm_slcr", "cfg", "cpm_csr";
+                             <0x00 0xfce20000 0x00 0x1000000>,
+                             <0x00 0xfcdc0000 0x00 0x10000>;
+                       reg-names = "cpm_slcr", "cfg", "cpm_csr", "cpm_crx";
+                       reset-gpios = <&gpio1 38 GPIO_ACTIVE_LOW>;
 
                        pcie_intc_1: interrupt-controller {
                                #address-cells = <0>;
-- 
2.44.1

[PATCH v6 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.

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

Add clear firewall after Link reset for CPM5NC.

Signed-off-by: Sai Krishna Musham <sai.krishna.musham@amd.com>
---
Changes for v6:
- Correct version check condition of CPM5NC_HOST.

Changes for v5:
- Handle probe defer for reset_gpio.
- Resolve ABI break.

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 | 86 ++++++++++++++++++++++--
 1 file changed, 82 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/controller/pcie-xilinx-cpm.c b/drivers/pci/controller/pcie-xilinx-cpm.c
index d0ab187d917f..b10c0752a94f 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_FRWALL	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_attr_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_attr_base;
 	struct irq_domain		*intx_domain;
 	struct irq_domain		*cpm_domain;
 	struct pci_config_window	*cfg;
@@ -475,12 +489,45 @@ static int xilinx_cpm_setup_irq(struct xilinx_cpm_pcie *port)
  * xilinx_cpm_pcie_init_port - Initialize hardware
  * @port: PCIe port information
  */
-static void xilinx_cpm_pcie_init_port(struct xilinx_cpm_pcie *port)
+static int 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)) {
+		if (PTR_ERR(reset_gpio) != -EPROBE_DEFER)
+			dev_err(dev, "Failed to request reset GPIO\n");
+		return PTR_ERR(reset_gpio);
+	}
 
-	if (variant->version == CPM5NC_HOST)
-		return;
+	if (reset_gpio && port->crx_base) {
+		/* 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 && port->cpm5nc_attr_base) {
+			/* Clear Firewall */
+			writel_relaxed(0x00, port->cpm5nc_attr_base +
+					XILINX_CPM5NC_PCIE0_FRWALL);
+			writel_relaxed(0x01, port->cpm5nc_attr_base +
+					XILINX_CPM5NC_PCIE0_FRWALL);
+			writel_relaxed(0x00, port->cpm5nc_attr_base +
+					XILINX_CPM5NC_PCIE0_FRWALL);
+			return 0;
+		}
+	}
 
 	if (cpm_pcie_link_up(port))
 		dev_info(port->dev, "PCIe Link is UP\n");
@@ -512,6 +559,8 @@ static void xilinx_cpm_pcie_init_port(struct xilinx_cpm_pcie *port)
 	pcie_write(port, pcie_read(port, XILINX_CPM_PCIE_REG_RPSC) |
 		   XILINX_CPM_PCIE_REG_RPSC_BEN,
 		   XILINX_CPM_PCIE_REG_RPSC);
+
+	return 0;
 }
 
 /**
@@ -551,6 +600,27 @@ 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)) {
+		if (PTR_ERR(port->crx_base) == -EINVAL)
+			port->crx_base = NULL;
+		else
+			return PTR_ERR(port->crx_base);
+	}
+
+	if (port->variant->version == CPM5NC_HOST) {
+		port->cpm5nc_attr_base =
+			devm_platform_ioremap_resource_byname(pdev,
+							      "cpm5nc_attr");
+		if (IS_ERR(port->cpm5nc_attr_base)) {
+			if (PTR_ERR(port->cpm5nc_attr_base) == -EINVAL)
+				port->cpm5nc_attr_base = NULL;
+			else
+				return PTR_ERR(port->cpm5nc_attr_base);
+		}
+	}
+
 	return 0;
 }
 
@@ -602,7 +672,11 @@ static int xilinx_cpm_pcie_probe(struct platform_device *pdev)
 		goto err_free_irq_domains;
 	}
 
-	xilinx_cpm_pcie_init_port(port);
+	err = xilinx_cpm_pcie_init_port(port);
+	if (err) {
+		dev_err(dev, "Init port failed\n");
+		goto err_setup_irq;
+	}
 
 	if (port->variant->version != CPM5NC_HOST) {
 		err = xilinx_cpm_setup_irq(port);
@@ -635,6 +709,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 +717,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 +725,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 v6 1/2] dt-bindings: PCI: xilinx-cpm: Add reset-gpios for PCIe RP PERST#

[Krzysztof Kozlowski]

On Wed, Mar 26, 2025 at 07:58:10AM +0530, Sai Krishna Musham wrote:
> Introduce `reset-gpios` property to enable GPIO-based control of
> the PCIe RP PERST# signal, generating assert and deassert signals.

I think it was removed, so this is not necessary. The property was there
all the time.

> 
> 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 (`cpm_crx`) to handle
> PCIe IP reset along with PCIe RP PERST# to avoid Link Training errors.

So does it mean it was not working before at all?

> 
> Signed-off-by: Sai Krishna Musham <sai.krishna.musham@amd.com>
> ---
> Changes for v6:
> - Resolve ABI break.
> - Update commit message.
> 
> Changes for v5:
> - Remove `reset-gpios` property from required as it is already present
>   in pci-bus-common.yaml
> - Update commit message
> 
> 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       | 72 ++++++++++++++-----
>  1 file changed, 55 insertions(+), 17 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml b/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
> index d674a24c8ccc..26e9cea41889 100644
> --- a/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
> +++ b/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
> @@ -9,9 +9,6 @@ title: CPM Host Controller device tree for Xilinx Versal SoCs
>  maintainers:
>    - Bharat Kumar Gogada <bharat.kumar.gogada@amd.com>
>  
> -allOf:
> -  - $ref: /schemas/pci/pci-host-bridge.yaml#
> -
>  properties:
>    compatible:
>      enum:
> @@ -21,18 +18,12 @@ properties:
>        - xlnx,versal-cpm5nc-host
>  
>    reg:
> -    items:
> -      - description: CPM system level control and status registers.
> -      - description: Configuration space region and bridge registers.
> -      - description: CPM5 control and status registers.
> -    minItems: 2
> +    minItems: 3

That's an ABI break.

> +    maxItems: 4
>  
>    reg-names:
> -    items:
> -      - const: cpm_slcr
> -      - const: cfg
> -      - const: cpm_csr
> -    minItems: 2
> +    minItems: 3
> +    maxItems: 4
>  
>    interrupts:
>      maxItems: 1
> @@ -72,10 +63,53 @@ required:
>    - msi-map
>    - interrupt-controller
>  
> +allOf:
> +  - $ref: /schemas/pci/pci-host-bridge.yaml#
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - xlnx,versal-cpm-host-1.00
> +    then:
> +      properties:
> +        reg:
> +          items:
> +            - description: CPM system level control and status registers.
> +            - description: Configuration space region and bridge registers.
> +            - description: CPM clock and reset control registers.

Before two items, now min 3, so another ABI break. Missing minItems.

> +        reg-names:
> +          items:
> +            - const: cpm_slcr
> +            - const: cfg
> +            - const: cpm_crx

Same

> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - xlnx,versal-cpm5-host
> +              - xlnx,versal-cpm5-host1
> +    then:
> +      properties:
> +        reg:
> +          items:
> +            - description: CPM system level control and status registers.
> +            - description: Configuration space region and bridge registers.
> +            - description: CPM5 control and status registers.
> +            - description: CPM clock and reset control registers.

This makes no sense, you still add the entry in the middle. This patch
fixed nothing from issues previously pointed out.

It's the third or fourth try and you keep repeating the same mistake,
which means you do not understand the problem. The problem is: you
cannot change the order. If you change it, it's an ABI break and nothing
in commit msg explained that.

Best regards,
Krzysztof

RE: [PATCH v6 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: Wednesday, March 26, 2025 1:16 PM
> To: Musham, Sai Krishna <sai.krishna.musham@amd.com>
> Cc: 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; 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 v6 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 Wed, Mar 26, 2025 at 07:58:10AM +0530, Sai Krishna Musham wrote:
> > Introduce `reset-gpios` property to enable GPIO-based control of the
> > PCIe RP PERST# signal, generating assert and deassert signals.
>
> I think it was removed, so this is not necessary. The property was there all the time.
Thank you for reviewing, I will update this in next patch.
>
> >
> > 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 (`cpm_crx`) to handle
> > PCIe IP reset along with PCIe RP PERST# to avoid Link Training errors.
>
> So does it mean it was not working before at all?
Thank you for reviewing, it was working earlier also. Currently boards are designed to handle
PERST# and IP reset from software driver, and IP reset is being removed from design.
I will update the commit message in next patch.
>
> >
> > Signed-off-by: Sai Krishna Musham <sai.krishna.musham@amd.com>
> > ---
> > Changes for v6:
> > - Resolve ABI break.
> > - Update commit message.
> >
> > Changes for v5:
> > - Remove `reset-gpios` property from required as it is already present
> >   in pci-bus-common.yaml
> > - Update commit message
> >
> > 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       | 72 ++++++++++++++-----
> >  1 file changed, 55 insertions(+), 17 deletions(-)
> >
> > diff --git
> > a/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
> > b/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
> > index d674a24c8ccc..26e9cea41889 100644
> > --- a/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
> > +++ b/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
> > @@ -9,9 +9,6 @@ title: CPM Host Controller device tree for Xilinx
> > Versal SoCs
> >  maintainers:
> >    - Bharat Kumar Gogada <bharat.kumar.gogada@amd.com>
> >
> > -allOf:
> > -  - $ref: /schemas/pci/pci-host-bridge.yaml#
> > -
> >  properties:
> >    compatible:
> >      enum:
> > @@ -21,18 +18,12 @@ properties:
> >        - xlnx,versal-cpm5nc-host
> >
> >    reg:
> > -    items:
> > -      - description: CPM system level control and status registers.
> > -      - description: Configuration space region and bridge registers.
> > -      - description: CPM5 control and status registers.
> > -    minItems: 2
> > +    minItems: 3
>
> That's an ABI break.
Thanks for reviewing, I will update minItems to 2, so it will not be an ABI break.
>
> > +    maxItems: 4
> >
> >    reg-names:
> > -    items:
> > -      - const: cpm_slcr
> > -      - const: cfg
> > -      - const: cpm_csr
> > -    minItems: 2
> > +    minItems: 3
> > +    maxItems: 4
> >
> >    interrupts:
> >      maxItems: 1
> > @@ -72,10 +63,53 @@ required:
> >    - msi-map
> >    - interrupt-controller
> >
> > +allOf:
> > +  - $ref: /schemas/pci/pci-host-bridge.yaml#
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            enum:
> > +              - xlnx,versal-cpm-host-1.00
> > +    then:
> > +      properties:
> > +        reg:
> > +          items:
> > +            - description: CPM system level control and status registers.
> > +            - description: Configuration space region and bridge registers.
> > +            - description: CPM clock and reset control registers.
>
> Before two items, now min 3, so another ABI break. Missing minItems.
Thanks for reviewing, I will update minItems to 2, so it will not be an ABI break.
>
> > +        reg-names:
> > +          items:
> > +            - const: cpm_slcr
> > +            - const: cfg
> > +            - const: cpm_crx
>
> Same
Thanks for reviewing, I will update minItems to 2, so it will not be an ABI break.
>
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            enum:
> > +              - xlnx,versal-cpm5-host
> > +              - xlnx,versal-cpm5-host1
> > +    then:
> > +      properties:
> > +        reg:
> > +          items:
> > +            - description: CPM system level control and status registers.
> > +            - description: Configuration space region and bridge registers.
> > +            - description: CPM5 control and status registers.
> > +            - description: CPM clock and reset control registers.
>
> This makes no sense, you still add the entry in the middle. This patch fixed nothing
> from issues previously pointed out.
>
> It's the third or fourth try and you keep repeating the same mistake, which means
> you do not understand the problem. The problem is: you cannot change the order. If
> you change it, it's an ABI break and nothing in commit msg explained that.
Thanks for reviewing, I will update minItems to 3, so it will not be an ABI break.
And currently in this patch I have a cpm_crx property at the end.
This is the description for cpm_crx.
            - description: CPM clock and reset control registers.

Below is the updated version of this patch. Please let me know if you have any further comments.
       - xlnx,versal-cpm5nc-host

   reg:
-    minItems: 3
+    minItems: 2
     maxItems: 4

   reg-names:
-    minItems: 3
+    minItems: 2
     maxItems: 4

   interrupts:
@@ -78,11 +78,13 @@ allOf:
             - description: CPM system level control and status registers.
             - description: Configuration space region and bridge registers.
             - description: CPM clock and reset control registers.
+          minItems: 2
         reg-names:
           items:
             - const: cpm_slcr
             - const: cfg
             - const: cpm_crx
+          minItems: 2
   - if:
       properties:
         compatible:
@@ -98,12 +100,14 @@ allOf:
             - description: Configuration space region and bridge registers.
             - description: CPM5 control and status registers.
             - description: CPM clock and reset control registers.
+          minItems: 3
         reg-names:
           items:
             - const: cpm_slcr
             - const: cfg
             - const: cpm_csr
             - const: cpm_crx
+          minItems: 3

 unevaluatedProperties: false
>
> Best regards,
> Krzysztof

Regards,
Sai Krishna

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

[Manivannan Sadhasivam]

On Wed, Mar 26, 2025 at 07:58:11AM +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.
> 
> Adapt to use GPIO framework and make reset optional to maintain
> backward compatibility with existing DTBs.
> 
> Add clear firewall after Link reset for CPM5NC.
> 
> Signed-off-by: Sai Krishna Musham <sai.krishna.musham@amd.com>
> ---
> Changes for v6:
> - Correct version check condition of CPM5NC_HOST.
> 
> Changes for v5:
> - Handle probe defer for reset_gpio.
> - Resolve ABI break.
> 
> 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 | 86 ++++++++++++++++++++++--
>  1 file changed, 82 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-xilinx-cpm.c b/drivers/pci/controller/pcie-xilinx-cpm.c
> index d0ab187d917f..b10c0752a94f 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_FRWALL	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_attr_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_attr_base;
>  	struct irq_domain		*intx_domain;
>  	struct irq_domain		*cpm_domain;
>  	struct pci_config_window	*cfg;
> @@ -475,12 +489,45 @@ static int xilinx_cpm_setup_irq(struct xilinx_cpm_pcie *port)
>   * xilinx_cpm_pcie_init_port - Initialize hardware
>   * @port: PCIe port information
>   */
> -static void xilinx_cpm_pcie_init_port(struct xilinx_cpm_pcie *port)
> +static int 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)) {
> +		if (PTR_ERR(reset_gpio) != -EPROBE_DEFER)
> +			dev_err(dev, "Failed to request reset GPIO\n");
> +		return PTR_ERR(reset_gpio);
> +	}
>  
> -	if (variant->version == CPM5NC_HOST)
> -		return;
> +	if (reset_gpio && port->crx_base) {
> +		/* Assert the PCIe IP reset */
> +		writel_relaxed(0x1, port->crx_base + variant->cpm_pcie_rst);
> +
> +		/* Controller specific delay */
> +		udelay(50);
> +

There should be atleast 100ms delay before PERST# deassert as per the spec. So
use PCIE_T_PVPERL_MS. I know that you had it before, but removed in v4. I don't
see a valid reason for that.

> +		/* 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 && port->cpm5nc_attr_base) {
> +			/* Clear Firewall */
> +			writel_relaxed(0x00, port->cpm5nc_attr_base +
> +					XILINX_CPM5NC_PCIE0_FRWALL);
> +			writel_relaxed(0x01, port->cpm5nc_attr_base +
> +					XILINX_CPM5NC_PCIE0_FRWALL);
> +			writel_relaxed(0x00, port->cpm5nc_attr_base +
> +					XILINX_CPM5NC_PCIE0_FRWALL);
> +			return 0;
> +		}
> +	}
>  
>  	if (cpm_pcie_link_up(port))
>  		dev_info(port->dev, "PCIe Link is UP\n");
> @@ -512,6 +559,8 @@ static void xilinx_cpm_pcie_init_port(struct xilinx_cpm_pcie *port)
>  	pcie_write(port, pcie_read(port, XILINX_CPM_PCIE_REG_RPSC) |
>  		   XILINX_CPM_PCIE_REG_RPSC_BEN,
>  		   XILINX_CPM_PCIE_REG_RPSC);
> +
> +	return 0;
>  }
>  
>  /**
> @@ -551,6 +600,27 @@ 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)) {
> +		if (PTR_ERR(port->crx_base) == -EINVAL)
> +			port->crx_base = NULL;
> +		else
> +			return PTR_ERR(port->crx_base);
> +	}
> +
> +	if (port->variant->version == CPM5NC_HOST) {
> +		port->cpm5nc_attr_base =
> +			devm_platform_ioremap_resource_byname(pdev,
> +							      "cpm5nc_attr");

Where is this resource defined in the binding?

> +		if (IS_ERR(port->cpm5nc_attr_base)) {
> +			if (PTR_ERR(port->cpm5nc_attr_base) == -EINVAL)

Why?

- Mani

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

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

[Krzysztof Kozlowski]

On 27/03/2025 18:25, Manivannan Sadhasivam wrote:
>>  /**
>> @@ -551,6 +600,27 @@ 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)) {
>> +		if (PTR_ERR(port->crx_base) == -EINVAL)
>> +			port->crx_base = NULL;
>> +		else
>> +			return PTR_ERR(port->crx_base);
>> +	}
>> +
>> +	if (port->variant->version == CPM5NC_HOST) {
>> +		port->cpm5nc_attr_base =
>> +			devm_platform_ioremap_resource_byname(pdev,
>> +							      "cpm5nc_attr");
> 
> Where is this resource defined in the binding?
> 

So this is v6 and still not tested.

Where is the DTS using this binding and driver, so we can verify that
AMD is not sending us such totally bogus, downstream code?

Best regards,
Krzysztof

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

[Musham, Sai Krishna]

[AMD Official Use Only - AMD Internal Distribution Only]

Hi Manivannan,

Thanks for the review.

> -----Original Message-----
> From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Sent: Thursday, March 27, 2025 10:56 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 v6 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 Wed, Mar 26, 2025 at 07:58:11AM +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.
> >
> > Adapt to use GPIO framework and make reset optional to maintain
> > backward compatibility with existing DTBs.
> >
> > Add clear firewall after Link reset for CPM5NC.
> >
> > Signed-off-by: Sai Krishna Musham <sai.krishna.musham@amd.com>
> > ---
> > Changes for v6:
> > - Correct version check condition of CPM5NC_HOST.
> >
> > Changes for v5:
> > - Handle probe defer for reset_gpio.
> > - Resolve ABI break.
> >
> > 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 | 86 ++++++++++++++++++++++--
> >  1 file changed, 82 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/pci/controller/pcie-xilinx-cpm.c b/drivers/pci/controller/pcie-xilinx-
> cpm.c
> > index d0ab187d917f..b10c0752a94f 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_FRWALL   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_attr_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_attr_base;
> >       struct irq_domain               *intx_domain;
> >       struct irq_domain               *cpm_domain;
> >       struct pci_config_window        *cfg;
> > @@ -475,12 +489,45 @@ static int xilinx_cpm_setup_irq(struct xilinx_cpm_pcie
> *port)
> >   * xilinx_cpm_pcie_init_port - Initialize hardware
> >   * @port: PCIe port information
> >   */
> > -static void xilinx_cpm_pcie_init_port(struct xilinx_cpm_pcie *port)
> > +static int 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)) {
> > +             if (PTR_ERR(reset_gpio) != -EPROBE_DEFER)
> > +                     dev_err(dev, "Failed to request reset GPIO\n");
> > +             return PTR_ERR(reset_gpio);
> > +     }
> >
> > -     if (variant->version == CPM5NC_HOST)
> > -             return;
> > +     if (reset_gpio && port->crx_base) {
> > +             /* Assert the PCIe IP reset */
> > +             writel_relaxed(0x1, port->crx_base + variant->cpm_pcie_rst);
> > +
> > +             /* Controller specific delay */
> > +             udelay(50);
> > +
>
> There should be atleast 100ms delay before PERST# deassert as per the spec. So
> use PCIE_T_PVPERL_MS. I know that you had it before, but removed in v4. I don't
> see a valid reason for that.

For CPM/CPM5/CPM5NC, the "Power Up" sequence mentioned in section 2.2.1
of PCIe Electromechanical Spec is handled in the design. The PERST# we are
using here is applied after the Power Up sequence and will be used for warm reset,
where power of the system is already stable.

So, we changed the delay after PERST# and IP reset assertion to 50us controller
specific delay, similar to TPERST(PERST# active time 100us) delay in "Power
sequencing and Reset Signal Timings" of PCIe Electromechanical Spec. After
deassertion of PERST# signal and IP reset, a delay of PCIE_T_RRS_READY_MS
is required before checking the Link. Please let me know if you have further queries.

Thanks, I will update this information in commit message.
>
> > +             /* 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 && port->cpm5nc_attr_base) {
> > +                     /* Clear Firewall */
> > +                     writel_relaxed(0x00, port->cpm5nc_attr_base +
> > +                                     XILINX_CPM5NC_PCIE0_FRWALL);
> > +                     writel_relaxed(0x01, port->cpm5nc_attr_base +
> > +                                     XILINX_CPM5NC_PCIE0_FRWALL);
> > +                     writel_relaxed(0x00, port->cpm5nc_attr_base +
> > +                                     XILINX_CPM5NC_PCIE0_FRWALL);
> > +                     return 0;
> > +             }
> > +     }
> >
> >       if (cpm_pcie_link_up(port))
> >               dev_info(port->dev, "PCIe Link is UP\n");
> > @@ -512,6 +559,8 @@ static void xilinx_cpm_pcie_init_port(struct
> xilinx_cpm_pcie *port)
> >       pcie_write(port, pcie_read(port, XILINX_CPM_PCIE_REG_RPSC) |
> >                  XILINX_CPM_PCIE_REG_RPSC_BEN,
> >                  XILINX_CPM_PCIE_REG_RPSC);
> > +
> > +     return 0;
> >  }
> >
> >  /**
> > @@ -551,6 +600,27 @@ 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)) {
> > +             if (PTR_ERR(port->crx_base) == -EINVAL)
> > +                     port->crx_base = NULL;
> > +             else
> > +                     return PTR_ERR(port->crx_base);
> > +     }
> > +
> > +     if (port->variant->version == CPM5NC_HOST) {
> > +             port->cpm5nc_attr_base =
> > +                     devm_platform_ioremap_resource_byname(pdev,
> > +                                                           "cpm5nc_attr");
>
> Where is this resource defined in the binding?

This patch is tested for mentioned CPM versions, I apologize that
I missed adding the cpm5nc_attr resource in DT binding. I will not
repeat this again. I will add the resource in the next patch.
Thanks for your understanding.
>
> > +             if (IS_ERR(port->cpm5nc_attr_base)) {
> > +                     if (PTR_ERR(port->cpm5nc_attr_base) == -EINVAL)
>
> Why?

This condition check is added to make cpm5nc_attr_base optional,
once I add missing resource in DT this condition will be applicable.
Thanks.
>
> - Mani
>
> --
> மணிவண்ணன் சதாசிவம்

Regards,
Sai Krishna

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

[Musham, Sai Krishna]

[AMD Official Use Only - AMD Internal Distribution Only]

Hi Krzysztof,

Thank you for reviewing.

> -----Original Message-----
> From: Krzysztof Kozlowski <krzk@kernel.org>
> Sent: Thursday, March 27, 2025 11:38 PM
> To: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>; 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 v6 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 27/03/2025 18:25, Manivannan Sadhasivam wrote:
> >>  /**
> >> @@ -551,6 +600,27 @@ 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)) {
> >> +            if (PTR_ERR(port->crx_base) == -EINVAL)
> >> +                    port->crx_base = NULL;
> >> +            else
> >> +                    return PTR_ERR(port->crx_base);
> >> +    }
> >> +
> >> +    if (port->variant->version == CPM5NC_HOST) {
> >> +            port->cpm5nc_attr_base =
> >> +                    devm_platform_ioremap_resource_byname(pdev,
> >> +
> >> + "cpm5nc_attr");
> >
> > Where is this resource defined in the binding?
> >
>
> So this is v6 and still not tested.
>
> Where is the DTS using this binding and driver, so we can verify that AMD is not
> sending us such totally bogus, downstream code?
>

This patch is tested for mentioned CPM versions, I apologize that
I missed adding the cpm5nc_attr resource in DT binding. I will not
repeat this again. I will add the resource in the next patch.
Thanks for your understanding.

> Best regards,
> Krzysztof

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

[Krzysztof Kozlowski]

On 04/04/2025 09:03, Musham, Sai Krishna wrote:
> [AMD Official Use Only - AMD Internal Distribution Only]
> 
> Hi Krzysztof,
> 
> Thank you for reviewing.
> 
>> -----Original Message-----
>> From: Krzysztof Kozlowski <krzk@kernel.org>
>> Sent: Thursday, March 27, 2025 11:38 PM
>> To: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>; 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 v6 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 27/03/2025 18:25, Manivannan Sadhasivam wrote:
>>>>  /**
>>>> @@ -551,6 +600,27 @@ 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)) {
>>>> +            if (PTR_ERR(port->crx_base) == -EINVAL)
>>>> +                    port->crx_base = NULL;
>>>> +            else
>>>> +                    return PTR_ERR(port->crx_base);
>>>> +    }
>>>> +
>>>> +    if (port->variant->version == CPM5NC_HOST) {
>>>> +            port->cpm5nc_attr_base =
>>>> +                    devm_platform_ioremap_resource_byname(pdev,
>>>> +
>>>> + "cpm5nc_attr");
>>>
>>> Where is this resource defined in the binding?
>>>
>>
>> So this is v6 and still not tested.
>>
>> Where is the DTS using this binding and driver, so we can verify that AMD is not
>> sending us such totally bogus, downstream code?
>>
> 
> This patch is tested for mentioned CPM versions, I apologize that

No, it wasn't. Testing would point that out.

> I missed adding the cpm5nc_attr resource in DT binding. I will not
> repeat this again. I will add the resource in the next patch.
> Thanks for your understanding.

Again, where is the DTS?

Best regards,
Krzysztof

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

[Manivannan Sadhasivam]

On Fri, Apr 04, 2025 at 06:59:23AM +0000, Musham, Sai Krishna wrote:
> [AMD Official Use Only - AMD Internal Distribution Only]
> 
> Hi Manivannan,
> 
> Thanks for the review.
> 
> > -----Original Message-----
> > From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > Sent: Thursday, March 27, 2025 10:56 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 v6 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 Wed, Mar 26, 2025 at 07:58:11AM +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.
> > >
> > > Adapt to use GPIO framework and make reset optional to maintain
> > > backward compatibility with existing DTBs.
> > >
> > > Add clear firewall after Link reset for CPM5NC.
> > >
> > > Signed-off-by: Sai Krishna Musham <sai.krishna.musham@amd.com>
> > > ---
> > > Changes for v6:
> > > - Correct version check condition of CPM5NC_HOST.
> > >
> > > Changes for v5:
> > > - Handle probe defer for reset_gpio.
> > > - Resolve ABI break.
> > >
> > > 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 | 86 ++++++++++++++++++++++--
> > >  1 file changed, 82 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/pci/controller/pcie-xilinx-cpm.c b/drivers/pci/controller/pcie-xilinx-
> > cpm.c
> > > index d0ab187d917f..b10c0752a94f 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_FRWALL   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_attr_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_attr_base;
> > >       struct irq_domain               *intx_domain;
> > >       struct irq_domain               *cpm_domain;
> > >       struct pci_config_window        *cfg;
> > > @@ -475,12 +489,45 @@ static int xilinx_cpm_setup_irq(struct xilinx_cpm_pcie
> > *port)
> > >   * xilinx_cpm_pcie_init_port - Initialize hardware
> > >   * @port: PCIe port information
> > >   */
> > > -static void xilinx_cpm_pcie_init_port(struct xilinx_cpm_pcie *port)
> > > +static int 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)) {
> > > +             if (PTR_ERR(reset_gpio) != -EPROBE_DEFER)
> > > +                     dev_err(dev, "Failed to request reset GPIO\n");
> > > +             return PTR_ERR(reset_gpio);
> > > +     }
> > >
> > > -     if (variant->version == CPM5NC_HOST)
> > > -             return;
> > > +     if (reset_gpio && port->crx_base) {
> > > +             /* Assert the PCIe IP reset */
> > > +             writel_relaxed(0x1, port->crx_base + variant->cpm_pcie_rst);
> > > +
> > > +             /* Controller specific delay */
> > > +             udelay(50);
> > > +
> >
> > There should be atleast 100ms delay before PERST# deassert as per the spec. So
> > use PCIE_T_PVPERL_MS. I know that you had it before, but removed in v4. I don't
> > see a valid reason for that.
> 
> For CPM/CPM5/CPM5NC, the "Power Up" sequence mentioned in section 2.2.1
> of PCIe Electromechanical Spec is handled in the design. The PERST# we are
> using here is applied after the Power Up sequence and will be used for warm reset,
> where power of the system is already stable.
> 

I don't quite understand what you mean by 'warm reset' here. Even if the power
was already stable, what is the guarantee that the 100ms time is elapsed before
deasserting the PERST#? Does the hardware logic ensure 100ms time is elapsed
before the driver is probed?

> So, we changed the delay after PERST# and IP reset assertion to 50us controller
> specific delay, similar to TPERST(PERST# active time 100us) delay in "Power
> sequencing and Reset Signal Timings" of PCIe Electromechanical Spec. After
> deassertion of PERST# signal and IP reset, a delay of PCIE_T_RRS_READY_MS
> is required before checking the Link. Please let me know if you have further queries.
> 

This part is fine.

> Thanks, I will update this information in commit message.
> >
> > > +             /* 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 && port->cpm5nc_attr_base) {
> > > +                     /* Clear Firewall */
> > > +                     writel_relaxed(0x00, port->cpm5nc_attr_base +
> > > +                                     XILINX_CPM5NC_PCIE0_FRWALL);
> > > +                     writel_relaxed(0x01, port->cpm5nc_attr_base +
> > > +                                     XILINX_CPM5NC_PCIE0_FRWALL);
> > > +                     writel_relaxed(0x00, port->cpm5nc_attr_base +
> > > +                                     XILINX_CPM5NC_PCIE0_FRWALL);
> > > +                     return 0;
> > > +             }
> > > +     }
> > >
> > >       if (cpm_pcie_link_up(port))
> > >               dev_info(port->dev, "PCIe Link is UP\n");
> > > @@ -512,6 +559,8 @@ static void xilinx_cpm_pcie_init_port(struct
> > xilinx_cpm_pcie *port)
> > >       pcie_write(port, pcie_read(port, XILINX_CPM_PCIE_REG_RPSC) |
> > >                  XILINX_CPM_PCIE_REG_RPSC_BEN,
> > >                  XILINX_CPM_PCIE_REG_RPSC);
> > > +
> > > +     return 0;
> > >  }
> > >
> > >  /**
> > > @@ -551,6 +600,27 @@ 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)) {
> > > +             if (PTR_ERR(port->crx_base) == -EINVAL)
> > > +                     port->crx_base = NULL;
> > > +             else
> > > +                     return PTR_ERR(port->crx_base);
> > > +     }
> > > +
> > > +     if (port->variant->version == CPM5NC_HOST) {
> > > +             port->cpm5nc_attr_base =
> > > +                     devm_platform_ioremap_resource_byname(pdev,
> > > +                                                           "cpm5nc_attr");
> >
> > Where is this resource defined in the binding?
> 
> This patch is tested for mentioned CPM versions, I apologize that
> I missed adding the cpm5nc_attr resource in DT binding. I will not
> repeat this again. I will add the resource in the next patch.
> Thanks for your understanding.
> >
> > > +             if (IS_ERR(port->cpm5nc_attr_base)) {
> > > +                     if (PTR_ERR(port->cpm5nc_attr_base) == -EINVAL)
> >
> > Why?
> 
> This condition check is added to make cpm5nc_attr_base optional,
> once I add missing resource in DT this condition will be applicable.

Why are you checking for -EINVAL? What does it correspond to?

If your intention is to make the resource_get optional, you should use
platform_get_resource_byname() first. If it returns NULL, then it means the
resource is not defined in DT.

- Mani

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

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

[Musham, Sai Krishna]

[AMD Official Use Only - AMD Internal Distribution Only]

Hi Krzysztof,

> -----Original Message-----
> From: Krzysztof Kozlowski <krzk@kernel.org>
> Sent: Friday, April 4, 2025 12:42 PM
> To: Musham, Sai Krishna <sai.krishna.musham@amd.com>; Manivannan
> Sadhasivam <manivannan.sadhasivam@linaro.org>
> 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 v6 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 04/04/2025 09:03, Musham, Sai Krishna wrote:
> > [AMD Official Use Only - AMD Internal Distribution Only]
> >
> > Hi Krzysztof,
> >
> > Thank you for reviewing.
> >
> >> -----Original Message-----
> >> From: Krzysztof Kozlowski <krzk@kernel.org>
> >> Sent: Thursday, March 27, 2025 11:38 PM
> >> To: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>; 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 v6 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 27/03/2025 18:25, Manivannan Sadhasivam wrote:
> >>>>  /**
> >>>> @@ -551,6 +600,27 @@ 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)) {
> >>>> +            if (PTR_ERR(port->crx_base) == -EINVAL)
> >>>> +                    port->crx_base = NULL;
> >>>> +            else
> >>>> +                    return PTR_ERR(port->crx_base);
> >>>> +    }
> >>>> +
> >>>> +    if (port->variant->version == CPM5NC_HOST) {
> >>>> +            port->cpm5nc_attr_base =
> >>>> +                    devm_platform_ioremap_resource_byname(pdev,
> >>>> +
> >>>> + "cpm5nc_attr");
> >>>
> >>> Where is this resource defined in the binding?
> >>>
> >>
> >> So this is v6 and still not tested.
> >>
> >> Where is the DTS using this binding and driver, so we can verify that
> >> AMD is not sending us such totally bogus, downstream code?
> >>
> >
> > This patch is tested for mentioned CPM versions, I apologize that
>
> No, it wasn't. Testing would point that out.
>
> > I missed adding the cpm5nc_attr resource in DT binding. I will not
> > repeat this again. I will add the resource in the next patch.
> > Thanks for your understanding.
>
> Again, where is the DTS?

I will update DTS and send the patch.

>
> Best regards,
> Krzysztof


Thanks,
Sai Krishna

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

[Musham, Sai Krishna]

[AMD Official Use Only - AMD Internal Distribution Only]

Hi Manivannan,

Thanks for the review.

> -----Original Message-----
> From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Sent: Wednesday, April 9, 2025 12:25 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 v6 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 Fri, Apr 04, 2025 at 06:59:23AM +0000, Musham, Sai Krishna wrote:
> > [AMD Official Use Only - AMD Internal Distribution Only]
> >
> > Hi Manivannan,
> >
> > Thanks for the review.
> >
> > > -----Original Message-----
> > > From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > Sent: Thursday, March 27, 2025 10:56 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 v6 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 Wed, Mar 26, 2025 at 07:58:11AM +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.
> > > >
> > > > Adapt to use GPIO framework and make reset optional to maintain
> > > > backward compatibility with existing DTBs.
> > > >
> > > > Add clear firewall after Link reset for CPM5NC.
> > > >
> > > > Signed-off-by: Sai Krishna Musham <sai.krishna.musham@amd.com>
> > > > ---
> > > > Changes for v6:
> > > > - Correct version check condition of CPM5NC_HOST.
> > > >
> > > > Changes for v5:
> > > > - Handle probe defer for reset_gpio.
> > > > - Resolve ABI break.
> > > >
> > > > 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 | 86
> > > > ++++++++++++++++++++++--
> > > >  1 file changed, 82 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/pci/controller/pcie-xilinx-cpm.c
> > > > b/drivers/pci/controller/pcie-xilinx-
> > > cpm.c
> > > > index d0ab187d917f..b10c0752a94f 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_FRWALL   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_attr_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_attr_base;
> > > >       struct irq_domain               *intx_domain;
> > > >       struct irq_domain               *cpm_domain;
> > > >       struct pci_config_window        *cfg;
> > > > @@ -475,12 +489,45 @@ static int xilinx_cpm_setup_irq(struct
> > > > xilinx_cpm_pcie
> > > *port)
> > > >   * xilinx_cpm_pcie_init_port - Initialize hardware
> > > >   * @port: PCIe port information
> > > >   */
> > > > -static void xilinx_cpm_pcie_init_port(struct xilinx_cpm_pcie
> > > > *port)
> > > > +static int 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)) {
> > > > +             if (PTR_ERR(reset_gpio) != -EPROBE_DEFER)
> > > > +                     dev_err(dev, "Failed to request reset GPIO\n");
> > > > +             return PTR_ERR(reset_gpio);
> > > > +     }
> > > >
> > > > -     if (variant->version == CPM5NC_HOST)
> > > > -             return;
> > > > +     if (reset_gpio && port->crx_base) {
> > > > +             /* Assert the PCIe IP reset */
> > > > +             writel_relaxed(0x1, port->crx_base +
> > > > + variant->cpm_pcie_rst);
> > > > +
> > > > +             /* Controller specific delay */
> > > > +             udelay(50);
> > > > +
> > >
> > > There should be atleast 100ms delay before PERST# deassert as per
> > > the spec. So use PCIE_T_PVPERL_MS. I know that you had it before,
> > > but removed in v4. I don't see a valid reason for that.
> >
> > For CPM/CPM5/CPM5NC, the "Power Up" sequence mentioned in section
> > 2.2.1 of PCIe Electromechanical Spec is handled in the design. The
> > PERST# we are using here is applied after the Power Up sequence and
> > will be used for warm reset, where power of the system is already stable.
> >
>
> I don't quite understand what you mean by 'warm reset' here. Even if the power was
> already stable, what is the guarantee that the 100ms time is elapsed before
> deasserting the PERST#? Does the hardware logic ensure 100ms time is elapsed
> before the driver is probed?
>

The Initial Power Up sequence is handled in hardware logic, and 100ms
(T_PVPERL) delay is provided after the power becomes stable. Yes, this part
is handled before the driver is probed.

By "warm reset" here, I'm referring to a reset that does not involve power
cycling the device, as per PCIe spec section 6.6.1. The power rails remain
stable, and only PERST# is toggled through the driver.

As per the PCIe Spec replaced 50us with 100us (T_PERST) before PERST#
deassert in driver, will send it in next patch.

> > So, we changed the delay after PERST# and IP reset assertion to 50us
> > controller specific delay, similar to TPERST(PERST# active time 100us)
> > delay in "Power sequencing and Reset Signal Timings" of PCIe
> > Electromechanical Spec. After deassertion of PERST# signal and IP
> > reset, a delay of PCIE_T_RRS_READY_MS is required before checking the Link.
> Please let me know if you have further queries.
> >
>
> This part is fine.
>
> > Thanks, I will update this information in commit message.
> > >
> > > > +             /* 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 && port-
> >cpm5nc_attr_base) {
> > > > +                     /* Clear Firewall */
> > > > +                     writel_relaxed(0x00, port->cpm5nc_attr_base +
> > > > +                                     XILINX_CPM5NC_PCIE0_FRWALL);
> > > > +                     writel_relaxed(0x01, port->cpm5nc_attr_base +
> > > > +                                     XILINX_CPM5NC_PCIE0_FRWALL);
> > > > +                     writel_relaxed(0x00, port->cpm5nc_attr_base +
> > > > +                                     XILINX_CPM5NC_PCIE0_FRWALL);
> > > > +                     return 0;
> > > > +             }
> > > > +     }
> > > >
> > > >       if (cpm_pcie_link_up(port))
> > > >               dev_info(port->dev, "PCIe Link is UP\n"); @@ -512,6
> > > > +559,8 @@ static void xilinx_cpm_pcie_init_port(struct
> > > xilinx_cpm_pcie *port)
> > > >       pcie_write(port, pcie_read(port, XILINX_CPM_PCIE_REG_RPSC) |
> > > >                  XILINX_CPM_PCIE_REG_RPSC_BEN,
> > > >                  XILINX_CPM_PCIE_REG_RPSC);
> > > > +
> > > > +     return 0;
> > > >  }
> > > >
> > > >  /**
> > > > @@ -551,6 +600,27 @@ 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)) {
> > > > +             if (PTR_ERR(port->crx_base) == -EINVAL)
> > > > +                     port->crx_base = NULL;
> > > > +             else
> > > > +                     return PTR_ERR(port->crx_base);
> > > > +     }
> > > > +
> > > > +     if (port->variant->version == CPM5NC_HOST) {
> > > > +             port->cpm5nc_attr_base =
> > > > +                     devm_platform_ioremap_resource_byname(pdev,
> > > > +
> > > > + "cpm5nc_attr");
> > >
> > > Where is this resource defined in the binding?
> >
> > This patch is tested for mentioned CPM versions, I apologize that I
> > missed adding the cpm5nc_attr resource in DT binding. I will not
> > repeat this again. I will add the resource in the next patch.
> > Thanks for your understanding.
> > >
> > > > +             if (IS_ERR(port->cpm5nc_attr_base)) {
> > > > +                     if (PTR_ERR(port->cpm5nc_attr_base) ==
> > > > + -EINVAL)
> > >
> > > Why?
> >
> > This condition check is added to make cpm5nc_attr_base optional, once
> > I add missing resource in DT this condition will be applicable.
>
> Why are you checking for -EINVAL? What does it correspond to?
>
> If your intention is to make the resource_get optional, you should use
> platform_get_resource_byname() first. If it returns NULL, then it means the resource
> is not defined in DT.
>

Yes, my intention is to make resource_get optional. Sure, I will use
platform_get_resource_byname(). Thanks.

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

Thanks,
Sai Krishna

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

[Manivannan Sadhasivam]

On Sun, Apr 13, 2025 at 04:28:55AM +0000, Musham, Sai Krishna wrote:
> [AMD Official Use Only - AMD Internal Distribution Only]
> 
> Hi Manivannan,
> 
> Thanks for the review.
> 
> > -----Original Message-----
> > From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > Sent: Wednesday, April 9, 2025 12:25 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 v6 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 Fri, Apr 04, 2025 at 06:59:23AM +0000, Musham, Sai Krishna wrote:
> > > [AMD Official Use Only - AMD Internal Distribution Only]
> > >
> > > Hi Manivannan,
> > >
> > > Thanks for the review.
> > >
> > > > -----Original Message-----
> > > > From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > > Sent: Thursday, March 27, 2025 10:56 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 v6 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 Wed, Mar 26, 2025 at 07:58:11AM +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.
> > > > >
> > > > > Adapt to use GPIO framework and make reset optional to maintain
> > > > > backward compatibility with existing DTBs.
> > > > >
> > > > > Add clear firewall after Link reset for CPM5NC.
> > > > >
> > > > > Signed-off-by: Sai Krishna Musham <sai.krishna.musham@amd.com>
> > > > > ---
> > > > > Changes for v6:
> > > > > - Correct version check condition of CPM5NC_HOST.
> > > > >
> > > > > Changes for v5:
> > > > > - Handle probe defer for reset_gpio.
> > > > > - Resolve ABI break.
> > > > >
> > > > > 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 | 86
> > > > > ++++++++++++++++++++++--
> > > > >  1 file changed, 82 insertions(+), 4 deletions(-)
> > > > >
> > > > > diff --git a/drivers/pci/controller/pcie-xilinx-cpm.c
> > > > > b/drivers/pci/controller/pcie-xilinx-
> > > > cpm.c
> > > > > index d0ab187d917f..b10c0752a94f 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_FRWALL   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_attr_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_attr_base;
> > > > >       struct irq_domain               *intx_domain;
> > > > >       struct irq_domain               *cpm_domain;
> > > > >       struct pci_config_window        *cfg;
> > > > > @@ -475,12 +489,45 @@ static int xilinx_cpm_setup_irq(struct
> > > > > xilinx_cpm_pcie
> > > > *port)
> > > > >   * xilinx_cpm_pcie_init_port - Initialize hardware
> > > > >   * @port: PCIe port information
> > > > >   */
> > > > > -static void xilinx_cpm_pcie_init_port(struct xilinx_cpm_pcie
> > > > > *port)
> > > > > +static int 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)) {
> > > > > +             if (PTR_ERR(reset_gpio) != -EPROBE_DEFER)
> > > > > +                     dev_err(dev, "Failed to request reset GPIO\n");
> > > > > +             return PTR_ERR(reset_gpio);
> > > > > +     }
> > > > >
> > > > > -     if (variant->version == CPM5NC_HOST)
> > > > > -             return;
> > > > > +     if (reset_gpio && port->crx_base) {
> > > > > +             /* Assert the PCIe IP reset */
> > > > > +             writel_relaxed(0x1, port->crx_base +
> > > > > + variant->cpm_pcie_rst);
> > > > > +
> > > > > +             /* Controller specific delay */
> > > > > +             udelay(50);
> > > > > +
> > > >
> > > > There should be atleast 100ms delay before PERST# deassert as per
> > > > the spec. So use PCIE_T_PVPERL_MS. I know that you had it before,
> > > > but removed in v4. I don't see a valid reason for that.
> > >
> > > For CPM/CPM5/CPM5NC, the "Power Up" sequence mentioned in section
> > > 2.2.1 of PCIe Electromechanical Spec is handled in the design. The
> > > PERST# we are using here is applied after the Power Up sequence and
> > > will be used for warm reset, where power of the system is already stable.
> > >
> >
> > I don't quite understand what you mean by 'warm reset' here. Even if the power was
> > already stable, what is the guarantee that the 100ms time is elapsed before
> > deasserting the PERST#? Does the hardware logic ensure 100ms time is elapsed
> > before the driver is probed?
> >
> 
> The Initial Power Up sequence is handled in hardware logic, and 100ms
> (T_PVPERL) delay is provided after the power becomes stable. Yes, this part
> is handled before the driver is probed.
> 

Ok, in that case, please mention it in comments before deasserting PERST#.
Otherwise, no one except you will know.

> By "warm reset" here, I'm referring to a reset that does not involve power
> cycling the device, as per PCIe spec section 6.6.1. The power rails remain
> stable, and only PERST# is toggled through the driver.
> 
> As per the PCIe Spec replaced 50us with 100us (T_PERST) before PERST#
> deassert in driver, will send it in next patch.
> 

You mean T_PERST-CLK? I don't think you need to wait for this. IIUC, this delay
is already part of in T_PVPERL. This requirement is to make sure that the refclk
becomes active atleast T_PERST-CLK time before deasserting PERST#. I don't think
you can guarantee that in software by introducing a delay without controlling
refclk.

- Mani

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

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

[Musham, Sai Krishna]

[AMD Official Use Only - AMD Internal Distribution Only]

Hi Manivannan,


> -----Original Message-----
> From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Sent: Tuesday, April 15, 2025 12:44 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 v6 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 Sun, Apr 13, 2025 at 04:28:55AM +0000, Musham, Sai Krishna wrote:
> > [AMD Official Use Only - AMD Internal Distribution Only]
> >
> > Hi Manivannan,
> >
> > Thanks for the review.
> >
> > > -----Original Message-----
> > > From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > Sent: Wednesday, April 9, 2025 12:25 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 v6 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 Fri, Apr 04, 2025 at 06:59:23AM +0000, Musham, Sai Krishna wrote:
> > > > [AMD Official Use Only - AMD Internal Distribution Only]
> > > >
> > > > Hi Manivannan,
> > > >
> > > > Thanks for the review.
> > > >
> > > > > -----Original Message-----
> > > > > From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > > > Sent: Thursday, March 27, 2025 10:56 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;
> > > > > krzk+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 v6 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 Wed, Mar 26, 2025 at 07:58:11AM +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.
> > > > > >
> > > > > > Adapt to use GPIO framework and make reset optional to
> > > > > > maintain backward compatibility with existing DTBs.
> > > > > >
> > > > > > Add clear firewall after Link reset for CPM5NC.
> > > > > >
> > > > > > Signed-off-by: Sai Krishna Musham <sai.krishna.musham@amd.com>
> > > > > > ---
> > > > > > Changes for v6:
> > > > > > - Correct version check condition of CPM5NC_HOST.
> > > > > >
> > > > > > Changes for v5:
> > > > > > - Handle probe defer for reset_gpio.
> > > > > > - Resolve ABI break.
> > > > > >
> > > > > > 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 | 86
> > > > > > ++++++++++++++++++++++--
> > > > > >  1 file changed, 82 insertions(+), 4 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/pci/controller/pcie-xilinx-cpm.c
> > > > > > b/drivers/pci/controller/pcie-xilinx-
> > > > > cpm.c
> > > > > > index d0ab187d917f..b10c0752a94f 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_FRWALL   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_attr_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_attr_base;
> > > > > >       struct irq_domain               *intx_domain;
> > > > > >       struct irq_domain               *cpm_domain;
> > > > > >       struct pci_config_window        *cfg;
> > > > > > @@ -475,12 +489,45 @@ static int xilinx_cpm_setup_irq(struct
> > > > > > xilinx_cpm_pcie
> > > > > *port)
> > > > > >   * xilinx_cpm_pcie_init_port - Initialize hardware
> > > > > >   * @port: PCIe port information
> > > > > >   */
> > > > > > -static void xilinx_cpm_pcie_init_port(struct xilinx_cpm_pcie
> > > > > > *port)
> > > > > > +static int 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)) {
> > > > > > +             if (PTR_ERR(reset_gpio) != -EPROBE_DEFER)
> > > > > > +                     dev_err(dev, "Failed to request reset GPIO\n");
> > > > > > +             return PTR_ERR(reset_gpio);
> > > > > > +     }
> > > > > >
> > > > > > -     if (variant->version == CPM5NC_HOST)
> > > > > > -             return;
> > > > > > +     if (reset_gpio && port->crx_base) {
> > > > > > +             /* Assert the PCIe IP reset */
> > > > > > +             writel_relaxed(0x1, port->crx_base +
> > > > > > + variant->cpm_pcie_rst);
> > > > > > +
> > > > > > +             /* Controller specific delay */
> > > > > > +             udelay(50);
> > > > > > +
> > > > >
> > > > > There should be atleast 100ms delay before PERST# deassert as
> > > > > per the spec. So use PCIE_T_PVPERL_MS. I know that you had it
> > > > > before, but removed in v4. I don't see a valid reason for that.
> > > >
> > > > For CPM/CPM5/CPM5NC, the "Power Up" sequence mentioned in section
> > > > 2.2.1 of PCIe Electromechanical Spec is handled in the design. The
> > > > PERST# we are using here is applied after the Power Up sequence
> > > > and will be used for warm reset, where power of the system is already stable.
> > > >
> > >
> > > I don't quite understand what you mean by 'warm reset' here. Even if
> > > the power was already stable, what is the guarantee that the 100ms
> > > time is elapsed before deasserting the PERST#? Does the hardware
> > > logic ensure 100ms time is elapsed before the driver is probed?
> > >
> >
> > The Initial Power Up sequence is handled in hardware logic, and 100ms
> > (T_PVPERL) delay is provided after the power becomes stable. Yes, this
> > part is handled before the driver is probed.
> >
>
> Ok, in that case, please mention it in comments before deasserting PERST#.
> Otherwise, no one except you will know.
>

During the power-up sequence, the hardware logic deasserts the PERST# signal.
By the time the driver is probed, the PERST# signal will already be in
deasserted state.

Later, the driver asserts and then deasserts the PERST# signal. Following your
suggestion and adhering to the specifications, I am changing the delay before
deasserting the PERST# signal in the driver to 100ms. I will send with this change
in next patch. Thanks.

> > By "warm reset" here, I'm referring to a reset that does not involve
> > power cycling the device, as per PCIe spec section 6.6.1. The power
> > rails remain stable, and only PERST# is toggled through the driver.
> >
> > As per the PCIe Spec replaced 50us with 100us (T_PERST) before PERST#
> > deassert in driver, will send it in next patch.
> >
>
> You mean T_PERST-CLK? I don't think you need to wait for this. IIUC, this delay is
> already part of in T_PVPERL. This requirement is to make sure that the refclk
> becomes active atleast T_PERST-CLK time before deasserting PERST#. I don't
> think you can guarantee that in software by introducing a delay without controlling
> refclk.
>
> - Mani
>
> --
> மணிவண்ணன் சதாசிவம்

Thanks,
Sai Krishna