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

[RESEND PATCH v7 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 example
node.
Add CPM clock and reset control register base, and CPM5NC firewall
attribute base.

Sai Krishna Musham (2):
  dt-bindings: PCI: xilinx-cpm: Add `cpm_crx` and `cpm5nc_fw_attr`
    properties
  PCI: xilinx-cpm: Add support for PCIe RP PERST# signal

 .../bindings/pci/xilinx-versal-cpm.yaml       | 129 +++++++++++++++---
 drivers/pci/controller/pcie-xilinx-cpm.c      |  97 ++++++++++++-
 2 files changed, 203 insertions(+), 23 deletions(-)

-- 
2.44.1

[RESEND PATCH v7 1/2] dt-bindings: PCI: xilinx-cpm: Add `cpm_crx` and `cpm5nc_fw_attr` properties

[Sai Krishna Musham]

Add the `cpm_crx` property to manage the PCIe IP reset, and
`cpm5nc_fw_attr` property to clear firewall after link reset, while
maintaining backward compatibility with existing device trees.

Also, incorporate `reset-gpios` in example for GPIO-based handling of
the PCIe Root Port (RP) PERST# signal for enabling assert and deassert
control.

The `reset-gpios` and `cpm_crx` properties must be provided for CPM,
CPM5 and CPM5_HOST1. For CPM5NC, all three properties - `reset-gpios`,
`cpm_crx` and `cpm5nc_fw_attr` must be explicitly defined to ensure
proper functionality.

Include an example DTS node and complete the binding documentation for
CPM5NC. Also, fix the bridge register address size in the example for
CPM5.

Signed-off-by: Sai Krishna Musham <sai.krishna.musham@amd.com>
---
Changes for v7:
- Update CPM5NC device tree binding.
- Add CPM5NC device tree example node.
- Update commit message.

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       | 129 +++++++++++++++---
 1 file changed, 109 insertions(+), 20 deletions(-)

diff --git a/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml b/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
index d674a24c8ccc..ed07896f763e 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
+    maxItems: 4
 
   reg-names:
-    items:
-      - const: cpm_slcr
-      - const: cfg
-      - const: cpm_csr
     minItems: 2
+    maxItems: 4
 
   interrupts:
     maxItems: 1
@@ -64,18 +55,94 @@ properties:
 required:
   - reg
   - reg-names
-  - "#interrupt-cells"
-  - interrupts
-  - interrupt-map
-  - interrupt-map-mask
   - bus-range
   - 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.
+          minItems: 2
+        reg-names:
+          items:
+            - const: cpm_slcr
+            - const: cfg
+            - const: cpm_crx
+          minItems: 2
+      required:
+        - "#interrupt-cells"
+        - interrupts
+        - interrupt-map
+        - interrupt-map-mask
+        - interrupt-controller
+  - 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.
+          minItems: 3
+        reg-names:
+          items:
+            - const: cpm_slcr
+            - const: cfg
+            - const: cpm_csr
+            - const: cpm_crx
+          minItems: 3
+      required:
+        - "#interrupt-cells"
+        - interrupts
+        - interrupt-map
+        - interrupt-map-mask
+        - interrupt-controller
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - xlnx,versal-cpm5nc-host
+    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.
+            - description: CPM5NC Firewall attribute register.
+          minItems: 2
+        reg-names:
+          items:
+            - const: cpm_slcr
+            - const: cfg
+            - const: cpm_crx
+            - const: cpm5nc_fw_attr
+          minItems: 2
 
 unevaluatedProperties: false
 
 examples:
   - |
+    #include <dt-bindings/gpio/gpio.h>
 
     versal {
                #address-cells = <2>;
@@ -98,8 +165,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 +195,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 0x10000>,
+                             <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>;
@@ -136,4 +207,22 @@ examples:
                        };
                };
 
+               cpm5nc_pcie: pcie@e4a10000 {
+                       compatible = "xlnx,versal-cpm5nc-host";
+                       device_type = "pci";
+                       #address-cells = <3>;
+                       #size-cells = <2>;
+                       interrupt-parent = <&gic>;
+                       bus-range = <0x00 0xff>;
+                       ranges = <0x2000000 0x00 0xa8000000 0x00 0xa8000000 0x00 0x8000000>,
+                                <0x43000000 0x1010 0x00 0x1010 0x00 0x08 0x00>;
+                       msi-map = <0x0 &its_gic 0x40000 0x10000>;
+                       reg = <0x00 0xe4a10000 0x00 0x10000>,
+                             <0x00 0xa0000000 0x00 0x8000000>,
+                             <0x00 0xe4a00000 0x00 0x10000>,
+                             <0x00 0xe4301000 0x00 0x10000>;
+                       reg-names = "cpm_slcr", "cfg", "cpm_crx", "cpm5nc_fw_attr";
+                       reset-gpios = <&gpio0 22 GPIO_ACTIVE_LOW>;
+               };
+
     };
-- 
2.44.1

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

[Sai Krishna Musham]

Add support for handling the PCIe Root Port (RP) PERST# signal using
the GPIO framework, along with the PCIe IP reset. This reset is
managed by the driver and occurs after the Initial Power Up sequence
(PCIe CEM r6.0, 2.2.1) is handled in hardware before the driver's probe
function is called.

This reset mechanism is particularly useful in warm reset scenarios,
where the power rails remain stable and only PERST# signal is toggled
through the driver. Applying both the PCIe IP reset and the PERST#
improves the reliability of the reset process by ensuring that both
the Root Port controller and the Endpoint are reset synchronously
and avoid lane errors.

Adapt the implementation to use the GPIO framework for reset signal
handling and make this reset handling optional, along with the
`cpm_crx` property, to maintain backward compatibility with existing
device tree binaries (DTBs).

Additionally, clear Firewall after the link reset for CPM5NC to allow
further PCIe transactions.

Signed-off-by: Sai Krishna Musham <sai.krishna.musham@amd.com>
---
Changes for v7:
- Use platform_get_resource_byname() to make cpm_crx and cpm5nc_fw_attr
  optional
- Use 100us delay T_PERST as per PCIe spec before PERST# deassert.

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 | 97 +++++++++++++++++++++++-
 1 file changed, 94 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/controller/pcie-xilinx-cpm.c b/drivers/pci/controller/pcie-xilinx-cpm.c
index 13ca493d22bd..c46642417d52 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	0x00000140
+
 #define XILINX_CPM_PCIE_REG_IDR		0x00000E10
 #define XILINX_CPM_PCIE_REG_IMR		0x00000E14
 #define XILINX_CPM_PCIE_REG_PSCR	0x00000E1C
@@ -93,12 +102,16 @@ enum xilinx_cpm_version {
  * @ir_status: Offset for the error interrupt status register
  * @ir_enable: Offset for the CPM5 local error interrupt enable register
  * @ir_misc_value: A bitmask for the miscellaneous interrupt status
+ * @cpm_pcie_rst: Offset for the PCIe IP reset
+ * @cpm5nc_fw_rst: Offset for the CPM5NC Firewall
  */
 struct xilinx_cpm_variant {
 	enum xilinx_cpm_version version;
 	u32 ir_status;
 	u32 ir_enable;
 	u32 ir_misc_value;
+	u32 cpm_pcie_rst;
+	u32 cpm5nc_fw_rst;
 };
 
 /**
@@ -106,6 +119,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_fw_base: CPM5NC Firewall Attribute Base
  * @intx_domain: Legacy IRQ domain pointer
  * @cpm_domain: CPM IRQ domain pointer
  * @cfg: Holds mappings of config space window
@@ -118,6 +133,8 @@ struct xilinx_cpm_pcie {
 	struct device			*dev;
 	void __iomem			*reg_base;
 	void __iomem			*cpm_base;
+	void __iomem			*crx_base;
+	void __iomem			*cpm5nc_fw_base;
 	struct irq_domain		*intx_domain;
 	struct irq_domain		*cpm_domain;
 	struct pci_config_window	*cfg;
@@ -475,12 +492,57 @@ 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;
+	bool do_reset = false;
+
+	if (port->crx_base && (variant->version < CPM5NC_HOST ||
+			       (variant->version == CPM5NC_HOST &&
+				port->cpm5nc_fw_base))) {
+		/* Request the GPIO for PCIe reset signal and assert */
+		reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
+		if (IS_ERR(reset_gpio))
+			return dev_err_probe(dev, PTR_ERR(reset_gpio),
+					     "Failed to request reset GPIO\n");
+		if (reset_gpio)
+			do_reset = true;
+	}
+
+	if (do_reset) {
+		/* Assert the PCIe IP reset */
+		writel_relaxed(0x1, port->crx_base + variant->cpm_pcie_rst);
+
+		/*
+		 * "PERST# active time", as per Table 2-10: Power Sequencing
+		 * and Reset Signal Timings of the PCIe Electromechanical
+		 * Specification, Revision 6.0, symbol "T_PERST".
+		 */
+		udelay(100);
+
+		/* 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_fw_base) {
+			/* Clear Firewall */
+			writel_relaxed(0x00, port->cpm5nc_fw_base +
+				       variant->cpm5nc_fw_rst);
+			writel_relaxed(0x01, port->cpm5nc_fw_base +
+				       variant->cpm5nc_fw_rst);
+			writel_relaxed(0x00, port->cpm5nc_fw_base +
+				       variant->cpm5nc_fw_rst);
+		}
+	}
 
 	if (variant->version == CPM5NC_HOST)
-		return;
+		return 0;
 
 	if (cpm_pcie_link_up(port))
 		dev_info(port->dev, "PCIe Link is UP\n");
@@ -512,6 +574,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;
 }
 
 /**
@@ -552,6 +616,24 @@ static int xilinx_cpm_pcie_parse_dt(struct xilinx_cpm_pcie *port,
 		port->reg_base = port->cfg->win;
 	}
 
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "cpm_crx");
+	if (res) {
+		port->crx_base = devm_ioremap_resource(dev, res);
+		if (IS_ERR(port->crx_base))
+			return PTR_ERR(port->crx_base);
+	}
+
+	if (port->variant->version == CPM5NC_HOST) {
+		res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
+						   "cpm5nc_fw_attr");
+		if (res) {
+			port->cpm5nc_fw_base =
+				devm_ioremap_resource(dev, res);
+			if (IS_ERR(port->cpm5nc_fw_base))
+				return PTR_ERR(port->cpm5nc_fw_base);
+		}
+	}
+
 	return 0;
 }
 
@@ -603,7 +685,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);
@@ -636,6 +722,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 = {
@@ -643,6 +730,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 = {
@@ -650,10 +738,13 @@ 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,
+	.cpm5nc_fw_rst = XILINX_CPM5NC_PCIE0_FRWALL,
 };
 
 static const struct of_device_id xilinx_cpm_pcie_of_match[] = {
-- 
2.44.1

Re: [RESEND PATCH v7 1/2] dt-bindings: PCI: xilinx-cpm: Add `cpm_crx` and `cpm5nc_fw_attr` properties

[Krzysztof Kozlowski]

On Mon, Apr 14, 2025 at 08:53:03AM GMT, Sai Krishna Musham wrote:
> Add the `cpm_crx` property to manage the PCIe IP reset, and
> `cpm5nc_fw_attr` property to clear firewall after link reset, while
> maintaining backward compatibility with existing device trees.
> 
> Also, incorporate `reset-gpios` in example for GPIO-based handling of
> the PCIe Root Port (RP) PERST# signal for enabling assert and deassert
> control.
> 
> The `reset-gpios` and `cpm_crx` properties must be provided for CPM,
> CPM5 and CPM5_HOST1. For CPM5NC, all three properties - `reset-gpios`,
> `cpm_crx` and `cpm5nc_fw_attr` must be explicitly defined to ensure

This we see from the diff, but why they must be defined?

> proper functionality.

What functionality?

> 
> Include an example DTS node and complete the binding documentation for
> CPM5NC. Also, fix the bridge register address size in the example for
> CPM5.
> 
> Signed-off-by: Sai Krishna Musham <sai.krishna.musham@amd.com>
> ---
> Changes for v7:
> - Update CPM5NC device tree binding.
> - Add CPM5NC device tree example node.
> - Update commit message.
> 
> Changes for v6:
> - Resolve ABI break.
> - Update commit message.
> 

...

> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - xlnx,versal-cpm5nc-host
> +    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.
> +            - description: CPM5NC Firewall attribute register.
> +          minItems: 2
> +        reg-names:
> +          items:
> +            - const: cpm_slcr
> +            - const: cfg
> +            - const: cpm_crx
> +            - const: cpm5nc_fw_attr
> +          minItems: 2

Why interrupts are not required for this variant? Why isn't this an
interrupt controller?

>  
>  unevaluatedProperties: false
>  
>  examples:
>    - |
> +    #include <dt-bindings/gpio/gpio.h>
>  
>      versal {
>                 #address-cells = <2>;
> @@ -98,8 +165,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 +195,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 0x10000>,
> +                             <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>;
> @@ -136,4 +207,22 @@ examples:
>                         };
>                 };
>  
> +               cpm5nc_pcie: pcie@e4a10000 {
> +                       compatible = "xlnx,versal-cpm5nc-host";
> +                       device_type = "pci";
> +                       #address-cells = <3>;
> +                       #size-cells = <2>;
> +                       interrupt-parent = <&gic>;
> +                       bus-range = <0x00 0xff>;
> +                       ranges = <0x2000000 0x00 0xa8000000 0x00 0xa8000000 0x00 0x8000000>,
> +                                <0x43000000 0x1010 0x00 0x1010 0x00 0x08 0x00>;
> +                       msi-map = <0x0 &its_gic 0x40000 0x10000>;
> +                       reg = <0x00 0xe4a10000 0x00 0x10000>,
> +                             <0x00 0xa0000000 0x00 0x8000000>,
> +                             <0x00 0xe4a00000 0x00 0x10000>,
> +                             <0x00 0xe4301000 0x00 0x10000>;

Follow DTS coding style. Or just drop this example... it also has
incorrect indentation. :/

> +                       reg-names = "cpm_slcr", "cfg", "cpm_crx", "cpm5nc_fw_attr";
> +                       reset-gpios = <&gpio0 22 GPIO_ACTIVE_LOW>;
> +               };
> +
>      };
> -- 
> 2.44.1
> 

RE: [RESEND PATCH v7 1/2] dt-bindings: PCI: xilinx-cpm: Add `cpm_crx` and `cpm5nc_fw_attr` properties

[Musham, Sai Krishna]

[AMD Official Use Only - AMD Internal Distribution Only]

Hi Krzysztof,

Thanks for the review.

> -----Original Message-----
> From: Krzysztof Kozlowski <krzk@kernel.org>
> Sent: Monday, April 14, 2025 12:32 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: [RESEND PATCH v7 1/2] dt-bindings: PCI: xilinx-cpm: Add `cpm_crx`
> and `cpm5nc_fw_attr` properties
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> On Mon, Apr 14, 2025 at 08:53:03AM GMT, Sai Krishna Musham wrote:
> > Add the `cpm_crx` property to manage the PCIe IP reset, and
> > `cpm5nc_fw_attr` property to clear firewall after link reset, while
> > maintaining backward compatibility with existing device trees.
> >
> > Also, incorporate `reset-gpios` in example for GPIO-based handling of
> > the PCIe Root Port (RP) PERST# signal for enabling assert and deassert
> > control.
> >
> > The `reset-gpios` and `cpm_crx` properties must be provided for CPM,
> > CPM5 and CPM5_HOST1. For CPM5NC, all three properties - `reset-gpios`,
> > `cpm_crx` and `cpm5nc_fw_attr` must be explicitly defined to ensure
>
> This we see from the diff, but why they must be defined?
>
> > proper functionality.
>
> What functionality?
>

For our controller, if cpm_crx is not provided lane errors will be observed.
Specifically for CPM5NC, if cpm5nc_fw_attr property is not provided, the firewall
is not cleared after reset and further PCIe transactions will not be allowed.
Therefore, these properties must be defined.

> >
> > Include an example DTS node and complete the binding documentation for
> > CPM5NC. Also, fix the bridge register address size in the example for
> > CPM5.
> >
> > Signed-off-by: Sai Krishna Musham <sai.krishna.musham@amd.com>
> > ---
> > Changes for v7:
> > - Update CPM5NC device tree binding.
> > - Add CPM5NC device tree example node.
> > - Update commit message.
> >
> > Changes for v6:
> > - Resolve ABI break.
> > - Update commit message.
> >
>
> ...
>
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            enum:
> > +              - xlnx,versal-cpm5nc-host
> > +    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.
> > +            - description: CPM5NC Firewall attribute register.
> > +          minItems: 2
> > +        reg-names:
> > +          items:
> > +            - const: cpm_slcr
> > +            - const: cfg
> > +            - const: cpm_crx
> > +            - const: cpm5nc_fw_attr
> > +          minItems: 2
>
> Why interrupts are not required for this variant? Why isn't this an
> interrupt controller?
>

MSI and MSI-X interrupts are handled via GIC, so msi-map property is
required for interrupt handling.
Legacy interrupt support is not available, and Error interrupt support will be
added in future, once it is added corresponding DT changes will be added.

> >
> >  unevaluatedProperties: false
> >
> >  examples:
> >    - |
> > +    #include <dt-bindings/gpio/gpio.h>
> >
> >      versal {
> >                 #address-cells = <2>;
> > @@ -98,8 +165,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 +195,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 0x10000>,
> > +                             <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>;
> > @@ -136,4 +207,22 @@ examples:
> >                         };
> >                 };
> >
> > +               cpm5nc_pcie: pcie@e4a10000 {
> > +                       compatible = "xlnx,versal-cpm5nc-host";
> > +                       device_type = "pci";
> > +                       #address-cells = <3>;
> > +                       #size-cells = <2>;
> > +                       interrupt-parent = <&gic>;
> > +                       bus-range = <0x00 0xff>;
> > +                       ranges = <0x2000000 0x00 0xa8000000 0x00 0xa8000000 0x00
> 0x8000000>,
> > +                                <0x43000000 0x1010 0x00 0x1010 0x00 0x08 0x00>;
> > +                       msi-map = <0x0 &its_gic 0x40000 0x10000>;
> > +                       reg = <0x00 0xe4a10000 0x00 0x10000>,
> > +                             <0x00 0xa0000000 0x00 0x8000000>,
> > +                             <0x00 0xe4a00000 0x00 0x10000>,
> > +                             <0x00 0xe4301000 0x00 0x10000>;
>
> Follow DTS coding style. Or just drop this example... it also has
> incorrect indentation. :/

Ok, I will drop this example.

>
> > +                       reg-names = "cpm_slcr", "cfg", "cpm_crx", "cpm5nc_fw_attr";
> > +                       reset-gpios = <&gpio0 22 GPIO_ACTIVE_LOW>;
> > +               };
> > +
> >      };
> > --
> > 2.44.1
> >

Thanks,
Sai Krishna

Re: [RESEND PATCH v7 1/2] dt-bindings: PCI: xilinx-cpm: Add `cpm_crx` and `cpm5nc_fw_attr` properties

[Krzysztof Kozlowski]

On 14/04/2025 14:23, Musham, Sai Krishna wrote:
> [AMD Official Use Only - AMD Internal Distribution Only]
> 
> Hi Krzysztof,
> 
> Thanks for the review.
> 
>> -----Original Message-----
>> From: Krzysztof Kozlowski <krzk@kernel.org>
>> Sent: Monday, April 14, 2025 12:32 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: [RESEND PATCH v7 1/2] dt-bindings: PCI: xilinx-cpm: Add `cpm_crx`
>> and `cpm5nc_fw_attr` properties
>>
>> Caution: This message originated from an External Source. Use proper caution
>> when opening attachments, clicking links, or responding.
>>
>>
>> On Mon, Apr 14, 2025 at 08:53:03AM GMT, Sai Krishna Musham wrote:
>>> Add the `cpm_crx` property to manage the PCIe IP reset, and
>>> `cpm5nc_fw_attr` property to clear firewall after link reset, while
>>> maintaining backward compatibility with existing device trees.
>>>
>>> Also, incorporate `reset-gpios` in example for GPIO-based handling of
>>> the PCIe Root Port (RP) PERST# signal for enabling assert and deassert
>>> control.
>>>
>>> The `reset-gpios` and `cpm_crx` properties must be provided for CPM,
>>> CPM5 and CPM5_HOST1. For CPM5NC, all three properties - `reset-gpios`,
>>> `cpm_crx` and `cpm5nc_fw_attr` must be explicitly defined to ensure
>>
>> This we see from the diff, but why they must be defined?
>>
>>> proper functionality.
>>
>> What functionality?
>>
> 
> For our controller, if cpm_crx is not provided lane errors will be observed.
> Specifically for CPM5NC, if cpm5nc_fw_attr property is not provided, the firewall
> is not cleared after reset and further PCIe transactions will not be allowed.
> Therefore, these properties must be defined.

This must be in the commit msg.

> 
>>>
>>> Include an example DTS node and complete the binding documentation for
>>> CPM5NC. Also, fix the bridge register address size in the example for
>>> CPM5.
>>>
>>> Signed-off-by: Sai Krishna Musham <sai.krishna.musham@amd.com>
>>> ---
>>> Changes for v7:
>>> - Update CPM5NC device tree binding.
>>> - Add CPM5NC device tree example node.
>>> - Update commit message.
>>>
>>> Changes for v6:
>>> - Resolve ABI break.
>>> - Update commit message.
>>>
>>
>> ...
>>
>>> +  - if:
>>> +      properties:
>>> +        compatible:
>>> +          contains:
>>> +            enum:
>>> +              - xlnx,versal-cpm5nc-host
>>> +    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.
>>> +            - description: CPM5NC Firewall attribute register.
>>> +          minItems: 2
>>> +        reg-names:
>>> +          items:
>>> +            - const: cpm_slcr
>>> +            - const: cfg
>>> +            - const: cpm_crx
>>> +            - const: cpm5nc_fw_attr
>>> +          minItems: 2
>>
>> Why interrupts are not required for this variant? Why isn't this an
>> interrupt controller?
>>
> 
> MSI and MSI-X interrupts are handled via GIC, so msi-map property is
> required for interrupt handling.
> Legacy interrupt support is not available, and Error interrupt support will be
> added in future, once it is added corresponding DT changes will be added.

I don't think commit msg explained this.

> 



Best regards,
Krzysztof

Re: [RESEND PATCH v7 1/2] dt-bindings: PCI: xilinx-cpm: Add `cpm_crx` and `cpm5nc_fw_attr` properties

[Rob Herring]

On Sun, Apr 13, 2025 at 10:23 PM Sai Krishna Musham
<sai.krishna.musham@amd.com> wrote:
>
> Add the `cpm_crx` property to manage the PCIe IP reset, and
> `cpm5nc_fw_attr` property to clear firewall after link reset, while
> maintaining backward compatibility with existing device trees.

You aren't adding properties. You are adding entries to 'reg'.

But the real problem here is you are adding reset and firewall
controls, but not using the respective bindings. It looks like you
need to use the 'resets' and possibly the 'access-controllers'
bindings. Unless these controls are really part of the PCIe bridge
(and only for it).

>
> Also, incorporate `reset-gpios` in example for GPIO-based handling of
> the PCIe Root Port (RP) PERST# signal for enabling assert and deassert
> control.

"Also" is a red flag for that change should be a separate commit.

>
> The `reset-gpios` and `cpm_crx` properties must be provided for CPM,
> CPM5 and CPM5_HOST1. For CPM5NC, all three properties - `reset-gpios`,
> `cpm_crx` and `cpm5nc_fw_attr` must be explicitly defined to ensure
> proper functionality.
>
> Include an example DTS node and complete the binding documentation for
> CPM5NC. Also, fix the bridge register address size in the example for
> CPM5.
>
> Signed-off-by: Sai Krishna Musham <sai.krishna.musham@amd.com>
> ---
> Changes for v7:
> - Update CPM5NC device tree binding.
> - Add CPM5NC device tree example node.
> - Update commit message.
>
> 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       | 129 +++++++++++++++---
>  1 file changed, 109 insertions(+), 20 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml b/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
> index d674a24c8ccc..ed07896f763e 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
> +    maxItems: 4
>
>    reg-names:
> -    items:
> -      - const: cpm_slcr
> -      - const: cfg
> -      - const: cpm_csr
>      minItems: 2
> +    maxItems: 4
>
>    interrupts:
>      maxItems: 1
> @@ -64,18 +55,94 @@ properties:
>  required:
>    - reg
>    - reg-names
> -  - "#interrupt-cells"
> -  - interrupts
> -  - interrupt-map
> -  - interrupt-map-mask
>    - bus-range
>    - 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.
> +          minItems: 2
> +        reg-names:
> +          items:
> +            - const: cpm_slcr
> +            - const: cfg
> +            - const: cpm_crx

The xlnx,versal-cpm-host-1.00 no longer has cpm_csr registers? Where
did they go? This is an ABI issue.

> +          minItems: 2
> +      required:
> +        - "#interrupt-cells"
> +        - interrupts
> +        - interrupt-map
> +        - interrupt-map-mask
> +        - interrupt-controller
> +  - 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.
> +          minItems: 3
> +        reg-names:
> +          items:
> +            - const: cpm_slcr
> +            - const: cfg
> +            - const: cpm_csr
> +            - const: cpm_crx
> +          minItems: 3
> +      required:
> +        - "#interrupt-cells"
> +        - interrupts
> +        - interrupt-map
> +        - interrupt-map-mask
> +        - interrupt-controller
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - xlnx,versal-cpm5nc-host
> +    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.
> +            - description: CPM5NC Firewall attribute register.

Just 1 register?

> +          minItems: 2
> +        reg-names:
> +          items:
> +            - const: cpm_slcr
> +            - const: cfg
> +            - const: cpm_crx
> +            - const: cpm5nc_fw_attr

The block name in the entry is redundant. Drop 'cpm5nc_'.

> +          minItems: 2
>
>  unevaluatedProperties: false
>
>  examples:
>    - |
> +    #include <dt-bindings/gpio/gpio.h>
>
>      versal {
>                 #address-cells = <2>;

RE: [RESEND PATCH v7 1/2] dt-bindings: PCI: xilinx-cpm: Add `cpm_crx` and `cpm5nc_fw_attr` properties

[Musham, Sai Krishna]

[AMD Official Use Only - AMD Internal Distribution Only]

Hi Krzysztof,

> -----Original Message-----
> From: Krzysztof Kozlowski <krzk@kernel.org>
> Sent: Tuesday, April 15, 2025 11:04 AM
> 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: [RESEND PATCH v7 1/2] dt-bindings: PCI: xilinx-cpm: Add `cpm_crx`
> and `cpm5nc_fw_attr` properties
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> On 14/04/2025 14:23, Musham, Sai Krishna wrote:
> > [AMD Official Use Only - AMD Internal Distribution Only]
> >
> > Hi Krzysztof,
> >
> > Thanks for the review.
> >
> >> -----Original Message-----
> >> From: Krzysztof Kozlowski <krzk@kernel.org>
> >> Sent: Monday, April 14, 2025 12:32 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: [RESEND PATCH v7 1/2] dt-bindings: PCI: xilinx-cpm: Add
> >> `cpm_crx` and `cpm5nc_fw_attr` properties
> >>
> >> Caution: This message originated from an External Source. Use proper
> >> caution when opening attachments, clicking links, or responding.
> >>
> >>
> >> On Mon, Apr 14, 2025 at 08:53:03AM GMT, Sai Krishna Musham wrote:
> >>> Add the `cpm_crx` property to manage the PCIe IP reset, and
> >>> `cpm5nc_fw_attr` property to clear firewall after link reset, while
> >>> maintaining backward compatibility with existing device trees.
> >>>
> >>> Also, incorporate `reset-gpios` in example for GPIO-based handling
> >>> of the PCIe Root Port (RP) PERST# signal for enabling assert and
> >>> deassert control.
> >>>
> >>> The `reset-gpios` and `cpm_crx` properties must be provided for CPM,
> >>> CPM5 and CPM5_HOST1. For CPM5NC, all three properties -
> >>> `reset-gpios`, `cpm_crx` and `cpm5nc_fw_attr` must be explicitly
> >>> defined to ensure
> >>
> >> This we see from the diff, but why they must be defined?
> >>
> >>> proper functionality.
> >>
> >> What functionality?
> >>
> >
> > For our controller, if cpm_crx is not provided lane errors will be observed.
> > Specifically for CPM5NC, if cpm5nc_fw_attr property is not provided,
> > the firewall is not cleared after reset and further PCIe transactions will not be
> allowed.
> > Therefore, these properties must be defined.
>
> This must be in the commit msg.
>

Sure, I will add this in commit message. Thanks.

> >
> >>>
> >>> Include an example DTS node and complete the binding documentation
> >>> for CPM5NC. Also, fix the bridge register address size in the
> >>> example for CPM5.
> >>>
> >>> Signed-off-by: Sai Krishna Musham <sai.krishna.musham@amd.com>
> >>> ---
> >>> Changes for v7:
> >>> - Update CPM5NC device tree binding.
> >>> - Add CPM5NC device tree example node.
> >>> - Update commit message.
> >>>
> >>> Changes for v6:
> >>> - Resolve ABI break.
> >>> - Update commit message.
> >>>
> >>
> >> ...
> >>
> >>> +  - if:
> >>> +      properties:
> >>> +        compatible:
> >>> +          contains:
> >>> +            enum:
> >>> +              - xlnx,versal-cpm5nc-host
> >>> +    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.
> >>> +            - description: CPM5NC Firewall attribute register.
> >>> +          minItems: 2
> >>> +        reg-names:
> >>> +          items:
> >>> +            - const: cpm_slcr
> >>> +            - const: cfg
> >>> +            - const: cpm_crx
> >>> +            - const: cpm5nc_fw_attr
> >>> +          minItems: 2
> >>
> >> Why interrupts are not required for this variant? Why isn't this an
> >> interrupt controller?
> >>
> >
> > MSI and MSI-X interrupts are handled via GIC, so msi-map property is
> > required for interrupt handling.
> > Legacy interrupt support is not available, and Error interrupt support
> > will be added in future, once it is added corresponding DT changes will be added.
>
> I don't think commit msg explained this.
>

Sure, I will mention this also in commit message and send in next patch.

> >
>
>
>
> Best regards,
> Krzysztof

Thanks,
Sai Krishna

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

[Manivannan Sadhasivam]

On Mon, Apr 14, 2025 at 08:53:04AM +0530, Sai Krishna Musham wrote:
> Add support for handling the PCIe Root Port (RP) PERST# signal using
> the GPIO framework, along with the PCIe IP reset. This reset is
> managed by the driver and occurs after the Initial Power Up sequence
> (PCIe CEM r6.0, 2.2.1) is handled in hardware before the driver's probe
> function is called.
> 
> This reset mechanism is particularly useful in warm reset scenarios,
> where the power rails remain stable and only PERST# signal is toggled
> through the driver. Applying both the PCIe IP reset and the PERST#
> improves the reliability of the reset process by ensuring that both
> the Root Port controller and the Endpoint are reset synchronously
> and avoid lane errors.
> 
> Adapt the implementation to use the GPIO framework for reset signal
> handling and make this reset handling optional, along with the
> `cpm_crx` property, to maintain backward compatibility with existing
> device tree binaries (DTBs).
> 
> Additionally, clear Firewall after the link reset for CPM5NC to allow
> further PCIe transactions.
> 
> Signed-off-by: Sai Krishna Musham <sai.krishna.musham@amd.com>
> ---
> Changes for v7:
> - Use platform_get_resource_byname() to make cpm_crx and cpm5nc_fw_attr
>   optional
> - Use 100us delay T_PERST as per PCIe spec before PERST# deassert.
> 
> 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 | 97 +++++++++++++++++++++++-
>  1 file changed, 94 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-xilinx-cpm.c b/drivers/pci/controller/pcie-xilinx-cpm.c
> index 13ca493d22bd..c46642417d52 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	0x00000140
> +
>  #define XILINX_CPM_PCIE_REG_IDR		0x00000E10
>  #define XILINX_CPM_PCIE_REG_IMR		0x00000E14
>  #define XILINX_CPM_PCIE_REG_PSCR	0x00000E1C
> @@ -93,12 +102,16 @@ enum xilinx_cpm_version {
>   * @ir_status: Offset for the error interrupt status register
>   * @ir_enable: Offset for the CPM5 local error interrupt enable register
>   * @ir_misc_value: A bitmask for the miscellaneous interrupt status
> + * @cpm_pcie_rst: Offset for the PCIe IP reset
> + * @cpm5nc_fw_rst: Offset for the CPM5NC Firewall
>   */
>  struct xilinx_cpm_variant {
>  	enum xilinx_cpm_version version;
>  	u32 ir_status;
>  	u32 ir_enable;
>  	u32 ir_misc_value;
> +	u32 cpm_pcie_rst;
> +	u32 cpm5nc_fw_rst;
>  };
>  
>  /**
> @@ -106,6 +119,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_fw_base: CPM5NC Firewall Attribute Base
>   * @intx_domain: Legacy IRQ domain pointer
>   * @cpm_domain: CPM IRQ domain pointer
>   * @cfg: Holds mappings of config space window
> @@ -118,6 +133,8 @@ struct xilinx_cpm_pcie {
>  	struct device			*dev;
>  	void __iomem			*reg_base;
>  	void __iomem			*cpm_base;
> +	void __iomem			*crx_base;
> +	void __iomem			*cpm5nc_fw_base;
>  	struct irq_domain		*intx_domain;
>  	struct irq_domain		*cpm_domain;
>  	struct pci_config_window	*cfg;
> @@ -475,12 +492,57 @@ 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;
> +	bool do_reset = false;
> +
> +	if (port->crx_base && (variant->version < CPM5NC_HOST ||
> +			       (variant->version == CPM5NC_HOST &&
> +				port->cpm5nc_fw_base))) {
> +		/* Request the GPIO for PCIe reset signal and assert */
> +		reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> +		if (IS_ERR(reset_gpio))
> +			return dev_err_probe(dev, PTR_ERR(reset_gpio),
> +					     "Failed to request reset GPIO\n");
> +		if (reset_gpio)
> +			do_reset = true;
> +	}
> +
> +	if (do_reset) {
> +		/* Assert the PCIe IP reset */
> +		writel_relaxed(0x1, port->crx_base + variant->cpm_pcie_rst);
> +
> +		/*
> +		 * "PERST# active time", as per Table 2-10: Power Sequencing
> +		 * and Reset Signal Timings of the PCIe Electromechanical
> +		 * Specification, Revision 6.0, symbol "T_PERST".
> +		 */
> +		udelay(100);
> +

Are you sure that you need T_PERST here and not T_PVPERL? T_PERST is only valid
while resuming from D3Cold i.e., after power up, while T_PVPERL is valid during
the power up, which is usually the case when a controller driver probes. Is your
driver relying on power being enabled by the bootloader and the driver just
toggling PERST# to perform conventional reset of the endpoint?

- Mani

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

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

[Bjorn Helgaas]

On Mon, Apr 14, 2025 at 08:53:04AM +0530, Sai Krishna Musham wrote:
> Add support for handling the PCIe Root Port (RP) PERST# signal using
> the GPIO framework, along with the PCIe IP reset. This reset is
> managed by the driver and occurs after the Initial Power Up sequence
> (PCIe CEM r6.0, 2.2.1) is handled in hardware before the driver's probe
> function is called.

Please say something specific here about what this does.  I *think* it
asserts both the PCIe IP reset (which I assume resets the host
controller) and PERST# (which resets any devices connected to the Root
Port), but only for devices that implement the CPM Clock and Reset
Control Registers AND describe the address of those registers via
DT "cpm_crx" AND describe a GPIO connected to PERST# via DT "reset".

> This reset mechanism is particularly useful in warm reset scenarios,
> where the power rails remain stable and only PERST# signal is toggled
> through the driver. Applying both the PCIe IP reset and the PERST#
> improves the reliability of the reset process by ensuring that both
> the Root Port controller and the Endpoint are reset synchronously
> and avoid lane errors.
> 
> Adapt the implementation to use the GPIO framework for reset signal
> handling and make this reset handling optional, along with the
> `cpm_crx` property, to maintain backward compatibility with existing
> device tree binaries (DTBs).

> Additionally, clear Firewall after the link reset for CPM5NC to allow
> further PCIe transactions.

> -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;
> +	bool do_reset = false;
> +
> +	if (port->crx_base && (variant->version < CPM5NC_HOST ||
> +			       (variant->version == CPM5NC_HOST &&
> +				port->cpm5nc_fw_base))) {

Would be nicer if you could simply test for the feature, not the
specific variants, e.g.,

  if (port->crx_base && port->perst_gpio) {
    writel_relaxed(0x1, port->crx_base + variant->cpm_pcie_rst);
    udelay(100);
    writel_relaxed(0x0, port->crx_base + variant->cpm_pcie_rst);
    gpiod_set_value(port->perst_gpio, 0);
    mdelay(PCIE_T_RRS_READY_MS);
  }

  if (port->firewall_base) {
    /* Clear Firewall */
  }

If you need to check the variants vs "cpm_crx", I think that should go
in xilinx_cpm_pcie_parse_dt().

> +		/* Request the GPIO for PCIe reset signal and assert */
> +		reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> +		if (IS_ERR(reset_gpio))
> +			return dev_err_probe(dev, PTR_ERR(reset_gpio),
> +					     "Failed to request reset GPIO\n");
> +		if (reset_gpio)
> +			do_reset = true;
> +	}

Maybe the devm_gpiod_get_optional() could go in
xilinx_cpm_pcie_parse_dt() along with other DT stuff, as is done in
starfive_pcie_parse_dt()/starfive_pcie_host_init()?

You'd have to save the port->reset_gpio pointer so we could use it
here, but wouldn't have to return error from
xilinx_cpm_pcie_init_port().

> +
> +	if (do_reset) {
> +		/* Assert the PCIe IP reset */
> +		writel_relaxed(0x1, port->crx_base + variant->cpm_pcie_rst);
> +
> +		/*
> +		 * "PERST# active time", as per Table 2-10: Power Sequencing
> +		 * and Reset Signal Timings of the PCIe Electromechanical
> +		 * Specification, Revision 6.0, symbol "T_PERST".
> +		 */
> +		udelay(100);

Whatever we need here, this should be a #define from drivers/pci/pci.h
instead of 100.

> +
> +		/* 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);

I think reset_gpio controls PERST#.  If so, it would be nice to have
"perst" in the name to make it less ambiguous.

> +		mdelay(PCIE_T_RRS_READY_MS);

We only wait PCIE_T_RRS_READY_MS for certain variants and only when
the optional "cpm_crx" and "reset" properties are present.

What about the other cases?  Unless there's something that guarantees
a delay after the link comes up before we call pci_host_probe(), that
sounds like a bug in the existing driver.  If it is a bug, you should
fix it in its own separate patch.

> +		if (variant->version == CPM5NC_HOST &&
> +		    port->cpm5nc_fw_base) {

Unnecessary to test both variant->version and port->cpm5nc_fw_base
here, since only CPM5NC_HOST sets cpm5nc_fw_base.

The function of the "Firewall" should be explained in the commit log,
and it seems like the sort of thing that's likely to appear in future
variants, so "cpm5nc_" seems like it might be unnecessarily specific.
Maybe consider naming these "firewall_base" and "firewall_reset" so
the test and the writes wouldn't have to change for future variants.

> +			/* Clear Firewall */
> +			writel_relaxed(0x00, port->cpm5nc_fw_base +
> +				       variant->cpm5nc_fw_rst);
> +			writel_relaxed(0x01, port->cpm5nc_fw_base +
> +				       variant->cpm5nc_fw_rst);
> +			writel_relaxed(0x00, port->cpm5nc_fw_base +
> +				       variant->cpm5nc_fw_rst);
> +		}
> +	}
>  
>  	if (variant->version == CPM5NC_HOST)

You didn't change this test, but it would be better if you could test
for a *feature* instead of a specific variant.  Then you can avoid
changes when future chips have the same feature.

> -		return;
> +		return 0;
>  
>  	if (cpm_pcie_link_up(port))
>  		dev_info(port->dev, "PCIe Link is UP\n");
> @@ -512,6 +574,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;
>  }

RE: [RESEND PATCH v7 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 <mani@kernel.org>
> Sent: Thursday, June 12, 2025 10:49 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: [RESEND PATCH v7 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 Mon, Apr 14, 2025 at 08:53:04AM +0530, Sai Krishna Musham wrote:
> > Add support for handling the PCIe Root Port (RP) PERST# signal using
> > the GPIO framework, along with the PCIe IP reset. This reset is
> > managed by the driver and occurs after the Initial Power Up sequence
> > (PCIe CEM r6.0, 2.2.1) is handled in hardware before the driver's probe
> > function is called.
> >
> > This reset mechanism is particularly useful in warm reset scenarios,
> > where the power rails remain stable and only PERST# signal is toggled
> > through the driver. Applying both the PCIe IP reset and the PERST#
> > improves the reliability of the reset process by ensuring that both
> > the Root Port controller and the Endpoint are reset synchronously
> > and avoid lane errors.
> >
> > Adapt the implementation to use the GPIO framework for reset signal
> > handling and make this reset handling optional, along with the
> > `cpm_crx` property, to maintain backward compatibility with existing
> > device tree binaries (DTBs).
> >
> > Additionally, clear Firewall after the link reset for CPM5NC to allow
> > further PCIe transactions.
> >
> > Signed-off-by: Sai Krishna Musham <sai.krishna.musham@amd.com>
> > ---
> > Changes for v7:
> > - Use platform_get_resource_byname() to make cpm_crx and cpm5nc_fw_attr
> >   optional
> > - Use 100us delay T_PERST as per PCIe spec before PERST# deassert.
> >
> > 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 | 97 +++++++++++++++++++++++-
> >  1 file changed, 94 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/pci/controller/pcie-xilinx-cpm.c b/drivers/pci/controller/pcie-xilinx-
> cpm.c
> > index 13ca493d22bd..c46642417d52 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   0x00000140
> > +
> >  #define XILINX_CPM_PCIE_REG_IDR              0x00000E10
> >  #define XILINX_CPM_PCIE_REG_IMR              0x00000E14
> >  #define XILINX_CPM_PCIE_REG_PSCR     0x00000E1C
> > @@ -93,12 +102,16 @@ enum xilinx_cpm_version {
> >   * @ir_status: Offset for the error interrupt status register
> >   * @ir_enable: Offset for the CPM5 local error interrupt enable register
> >   * @ir_misc_value: A bitmask for the miscellaneous interrupt status
> > + * @cpm_pcie_rst: Offset for the PCIe IP reset
> > + * @cpm5nc_fw_rst: Offset for the CPM5NC Firewall
> >   */
> >  struct xilinx_cpm_variant {
> >       enum xilinx_cpm_version version;
> >       u32 ir_status;
> >       u32 ir_enable;
> >       u32 ir_misc_value;
> > +     u32 cpm_pcie_rst;
> > +     u32 cpm5nc_fw_rst;
> >  };
> >
> >  /**
> > @@ -106,6 +119,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_fw_base: CPM5NC Firewall Attribute Base
> >   * @intx_domain: Legacy IRQ domain pointer
> >   * @cpm_domain: CPM IRQ domain pointer
> >   * @cfg: Holds mappings of config space window
> > @@ -118,6 +133,8 @@ struct xilinx_cpm_pcie {
> >       struct device                   *dev;
> >       void __iomem                    *reg_base;
> >       void __iomem                    *cpm_base;
> > +     void __iomem                    *crx_base;
> > +     void __iomem                    *cpm5nc_fw_base;
> >       struct irq_domain               *intx_domain;
> >       struct irq_domain               *cpm_domain;
> >       struct pci_config_window        *cfg;
> > @@ -475,12 +492,57 @@ 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;
> > +     bool do_reset = false;
> > +
> > +     if (port->crx_base && (variant->version < CPM5NC_HOST ||
> > +                            (variant->version == CPM5NC_HOST &&
> > +                             port->cpm5nc_fw_base))) {
> > +             /* Request the GPIO for PCIe reset signal and assert */
> > +             reset_gpio = devm_gpiod_get_optional(dev, "reset",
> GPIOD_OUT_HIGH);
> > +             if (IS_ERR(reset_gpio))
> > +                     return dev_err_probe(dev, PTR_ERR(reset_gpio),
> > +                                          "Failed to request reset GPIO\n");
> > +             if (reset_gpio)
> > +                     do_reset = true;
> > +     }
> > +
> > +     if (do_reset) {
> > +             /* Assert the PCIe IP reset */
> > +             writel_relaxed(0x1, port->crx_base + variant->cpm_pcie_rst);
> > +
> > +             /*
> > +              * "PERST# active time", as per Table 2-10: Power Sequencing
> > +              * and Reset Signal Timings of the PCIe Electromechanical
> > +              * Specification, Revision 6.0, symbol "T_PERST".
> > +              */
> > +             udelay(100);
> > +
>
> Are you sure that you need T_PERST here and not T_PVPERL? T_PERST is only
> valid
> while resuming from D3Cold i.e., after power up, while T_PVPERL is valid during
> the power up, which is usually the case when a controller driver probes. Is your
> driver relying on power being enabled by the bootloader and the driver just
> toggling PERST# to perform conventional reset of the endpoint?
>

Thanks for pointing that out. Yes, the power-up sequence is handled by the hardware,
and the driver relies on power being enabled by it. We're only toggling the PERST# signal
in the driver to perform a conventional reset of the endpoint. So, I'm confident that T_PERST
is the appropriate timing reference here, not T_PVPERL.

Additionally, this delay was recommended by our hardware team, who confirmed that the
power-up sequence is managed in hardware logic, and that T_PERST is the appropriate
timing to apply in this context.

I also checked pci.h but couldn't find a predefined macro for T_PERST, so I used 100.
Please let me know if there's a preferred macro I should be using instead.

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

Thanks,
Sai Krishna

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

[Bjorn Helgaas]

On Tue, Jun 17, 2025 at 04:14:37AM +0000, Musham, Sai Krishna wrote:
> [AMD Official Use Only - AMD Internal Distribution Only]
> 
> Hi Manivannan,
> 
> > -----Original Message-----
> > From: Manivannan Sadhasivam <mani@kernel.org>
> > Sent: Thursday, June 12, 2025 10:49 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: [RESEND PATCH v7 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 Mon, Apr 14, 2025 at 08:53:04AM +0530, Sai Krishna Musham wrote:
> > > Add support for handling the PCIe Root Port (RP) PERST# signal using
> > > the GPIO framework, along with the PCIe IP reset. This reset is
> > > managed by the driver and occurs after the Initial Power Up sequence
> > > (PCIe CEM r6.0, 2.2.1) is handled in hardware before the driver's probe
> > > function is called.

> > > +     if (do_reset) {
> > > +             /* Assert the PCIe IP reset */
> > > +             writel_relaxed(0x1, port->crx_base + variant->cpm_pcie_rst);
> > > +
> > > +             /*
> > > +              * "PERST# active time", as per Table 2-10: Power Sequencing
> > > +              * and Reset Signal Timings of the PCIe Electromechanical
> > > +              * Specification, Revision 6.0, symbol "T_PERST".
> > > +              */
> > > +             udelay(100);
> >
> > Are you sure that you need T_PERST here and not T_PVPERL? T_PERST
> > is only valid while resuming from D3Cold i.e., after power up,
> > while T_PVPERL is valid during the power up, which is usually the
> > case when a controller driver probes. Is your driver relying on
> > power being enabled by the bootloader and the driver just toggling
> > PERST# to perform conventional reset of the endpoint?
> 
> Thanks for pointing that out. Yes, the power-up sequence is handled
> by the hardware, and the driver relies on power being enabled by it.
> We're only toggling the PERST# signal in the driver to perform a
> conventional reset of the endpoint. So, I'm confident that T_PERST
> is the appropriate timing reference here, not T_PVPERL.
> 
> Additionally, this delay was recommended by our hardware team, who
> confirmed that the power-up sequence is managed in hardware logic,
> and that T_PERST is the appropriate timing to apply in this context.
> 
> I also checked pci.h but couldn't find a predefined macro for
> T_PERST, so I used 100.  Please let me know if there's a preferred
> macro I should be using instead.

If we need a new macro, please add it.  Include a citation to the
relevant section of the spec ("PCIe CEM r6.0, sec 2.11.2"; table
numbers don't appear in the table of contents so they're hard to
find), and include the units ("_US", I guess) in the macro name.

Given a comment at the macro definition, you don't need to repeat it
at all the uses.

Bjorn

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

[Musham, Sai Krishna]

[AMD Official Use Only - AMD Internal Distribution Only]

Hi Bjorn,

> -----Original Message-----
> From: Bjorn Helgaas <helgaas@kernel.org>
> Sent: Tuesday, June 17, 2025 8:17 PM
> To: Musham, Sai Krishna <sai.krishna.musham@amd.com>
> Cc: Manivannan Sadhasivam <mani@kernel.org>; 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: [RESEND PATCH v7 2/2] PCI: xilinx-cpm: Add support for PCIe RP
> PERST# signal
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> On Tue, Jun 17, 2025 at 04:14:37AM +0000, Musham, Sai Krishna wrote:
> > [AMD Official Use Only - AMD Internal Distribution Only]
> >
> > Hi Manivannan,
> >
> > > -----Original Message-----
> > > From: Manivannan Sadhasivam <mani@kernel.org>
> > > Sent: Thursday, June 12, 2025 10:49 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: [RESEND PATCH v7 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 Mon, Apr 14, 2025 at 08:53:04AM +0530, Sai Krishna Musham wrote:
> > > > Add support for handling the PCIe Root Port (RP) PERST# signal using
> > > > the GPIO framework, along with the PCIe IP reset. This reset is
> > > > managed by the driver and occurs after the Initial Power Up sequence
> > > > (PCIe CEM r6.0, 2.2.1) is handled in hardware before the driver's probe
> > > > function is called.
>
> > > > +     if (do_reset) {
> > > > +             /* Assert the PCIe IP reset */
> > > > +             writel_relaxed(0x1, port->crx_base + variant->cpm_pcie_rst);
> > > > +
> > > > +             /*
> > > > +              * "PERST# active time", as per Table 2-10: Power Sequencing
> > > > +              * and Reset Signal Timings of the PCIe Electromechanical
> > > > +              * Specification, Revision 6.0, symbol "T_PERST".
> > > > +              */
> > > > +             udelay(100);
> > >
> > > Are you sure that you need T_PERST here and not T_PVPERL? T_PERST
> > > is only valid while resuming from D3Cold i.e., after power up,
> > > while T_PVPERL is valid during the power up, which is usually the
> > > case when a controller driver probes. Is your driver relying on
> > > power being enabled by the bootloader and the driver just toggling
> > > PERST# to perform conventional reset of the endpoint?
> >
> > Thanks for pointing that out. Yes, the power-up sequence is handled
> > by the hardware, and the driver relies on power being enabled by it.
> > We're only toggling the PERST# signal in the driver to perform a
> > conventional reset of the endpoint. So, I'm confident that T_PERST
> > is the appropriate timing reference here, not T_PVPERL.
> >
> > Additionally, this delay was recommended by our hardware team, who
> > confirmed that the power-up sequence is managed in hardware logic,
> > and that T_PERST is the appropriate timing to apply in this context.
> >
> > I also checked pci.h but couldn't find a predefined macro for
> > T_PERST, so I used 100.  Please let me know if there's a preferred
> > macro I should be using instead.
>
> If we need a new macro, please add it.  Include a citation to the
> relevant section of the spec ("PCIe CEM r6.0, sec 2.11.2"; table
> numbers don't appear in the table of contents so they're hard to
> find), and include the units ("_US", I guess) in the macro name.
>
> Given a comment at the macro definition, you don't need to repeat it
> at all the uses.
>

Thanks for the review, sure I will add new macro and include a citation
to the relevant section of the PCIe spec.

> Bjorn

Thanks,
Sai Krishna

RE: [RESEND PATCH v7 1/2] dt-bindings: PCI: xilinx-cpm: Add `cpm_crx` and `cpm5nc_fw_attr` properties

[Musham, Sai Krishna]

[AMD Official Use Only - AMD Internal Distribution Only]

Hi Rob,

Thanks for the review.

> -----Original Message-----
> From: Rob Herring <robh@kernel.org>
> Sent: Tuesday, April 15, 2025 8:44 PM
> To: Musham, Sai Krishna <sai.krishna.musham@amd.com>
> Cc: bhelgaas@google.com; lpieralisi@kernel.org; kw@linux.com;
> manivannan.sadhasivam@linaro.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: [RESEND PATCH v7 1/2] dt-bindings: PCI: xilinx-cpm: Add `cpm_crx`
> and `cpm5nc_fw_attr` properties
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> On Sun, Apr 13, 2025 at 10:23 PM Sai Krishna Musham
> <sai.krishna.musham@amd.com> wrote:
> >
> > Add the `cpm_crx` property to manage the PCIe IP reset, and
> > `cpm5nc_fw_attr` property to clear firewall after link reset, while
> > maintaining backward compatibility with existing device trees.
>
> You aren't adding properties. You are adding entries to 'reg'.
>

You're right — we are not adding new properties but rather extending
the reg entries, I will correct this in commit message.

> But the real problem here is you are adding reset and firewall
> controls, but not using the respective bindings. It looks like you
> need to use the 'resets' and possibly the 'access-controllers'
> bindings. Unless these controls are really part of the PCIe bridge
> (and only for it).
>

Thanks for pointing that out, sorry for the delayed response.
For the PCIe IP/Core reset we are working on it to handle
PCIe IP reset through 'resets' DT binding.

Regarding CPM5NC firewall control, we are discussing the
possibility of handling firewall in Firmware.

> >
> > Also, incorporate `reset-gpios` in example for GPIO-based handling of
> > the PCIe Root Port (RP) PERST# signal for enabling assert and deassert
> > control.
>
> "Also" is a red flag for that change should be a separate commit.
>

Sure, I will correct the commit message.

> >
> > The `reset-gpios` and `cpm_crx` properties must be provided for CPM,
> > CPM5 and CPM5_HOST1. For CPM5NC, all three properties - `reset-gpios`,
> > `cpm_crx` and `cpm5nc_fw_attr` must be explicitly defined to ensure
> > proper functionality.
> >
> > Include an example DTS node and complete the binding documentation for
> > CPM5NC. Also, fix the bridge register address size in the example for
> > CPM5.
> >
> > Signed-off-by: Sai Krishna Musham <sai.krishna.musham@amd.com>
> > ---
> > Changes for v7:
> > - Update CPM5NC device tree binding.
> > - Add CPM5NC device tree example node.
> > - Update commit message.
> >
> > 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       | 129 +++++++++++++++---
> >  1 file changed, 109 insertions(+), 20 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
> b/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
> > index d674a24c8ccc..ed07896f763e 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
> > +    maxItems: 4
> >
> >    reg-names:
> > -    items:
> > -      - const: cpm_slcr
> > -      - const: cfg
> > -      - const: cpm_csr
> >      minItems: 2
> > +    maxItems: 4
> >
> >    interrupts:
> >      maxItems: 1
> > @@ -64,18 +55,94 @@ properties:
> >  required:
> >    - reg
> >    - reg-names
> > -  - "#interrupt-cells"
> > -  - interrupts
> > -  - interrupt-map
> > -  - interrupt-map-mask
> >    - bus-range
> >    - 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.
> > +          minItems: 2
> > +        reg-names:
> > +          items:
> > +            - const: cpm_slcr
> > +            - const: cfg
> > +            - const: cpm_crx
>
> The xlnx,versal-cpm-host-1.00 no longer has cpm_csr registers? Where
> did they go? This is an ABI issue.
>

Yes, xlnx,versal-cpm-host-1.00 doesn't have cpm_csr registers.

> > +          minItems: 2
> > +      required:
> > +        - "#interrupt-cells"
> > +        - interrupts
> > +        - interrupt-map
> > +        - interrupt-map-mask
> > +        - interrupt-controller
> > +  - 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.
> > +          minItems: 3
> > +        reg-names:
> > +          items:
> > +            - const: cpm_slcr
> > +            - const: cfg
> > +            - const: cpm_csr
> > +            - const: cpm_crx
> > +          minItems: 3
> > +      required:
> > +        - "#interrupt-cells"
> > +        - interrupts
> > +        - interrupt-map
> > +        - interrupt-map-mask
> > +        - interrupt-controller
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            enum:
> > +              - xlnx,versal-cpm5nc-host
> > +    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.
> > +            - description: CPM5NC Firewall attribute register.
>
> Just 1 register?
>

No, this register base contains other peripheral registers also.

> > +          minItems: 2
> > +        reg-names:
> > +          items:
> > +            - const: cpm_slcr
> > +            - const: cfg
> > +            - const: cpm_crx
> > +            - const: cpm5nc_fw_attr
>
> The block name in the entry is redundant. Drop 'cpm5nc_'.
>
> > +          minItems: 2
> >
> >  unevaluatedProperties: false
> >
> >  examples:
> >    - |
> > +    #include <dt-bindings/gpio/gpio.h>
> >
> >      versal {
> >                 #address-cells = <2>;

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

[Musham, Sai Krishna]

[AMD Official Use Only - AMD Internal Distribution Only]

Hi Bjorn,

Thanks for the review.

> -----Original Message-----
> From: Bjorn Helgaas <helgaas@kernel.org>
> Sent: Friday, June 13, 2025 2:04 AM
> 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: [RESEND PATCH v7 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 Mon, Apr 14, 2025 at 08:53:04AM +0530, Sai Krishna Musham wrote:
> > Add support for handling the PCIe Root Port (RP) PERST# signal using
> > the GPIO framework, along with the PCIe IP reset. This reset is
> > managed by the driver and occurs after the Initial Power Up sequence
> > (PCIe CEM r6.0, 2.2.1) is handled in hardware before the driver's probe
> > function is called.
>
> Please say something specific here about what this does.  I *think* it
> asserts both the PCIe IP reset (which I assume resets the host
> controller) and PERST# (which resets any devices connected to the Root
> Port), but only for devices that implement the CPM Clock and Reset
> Control Registers AND describe the address of those registers via
> DT "cpm_crx" AND describe a GPIO connected to PERST# via DT "reset".
>

Yes, in Hardware logic both PCIe IP reset and PERST# are reset for CPM
devices. I will include this in commit message.

> > This reset mechanism is particularly useful in warm reset scenarios,
> > where the power rails remain stable and only PERST# signal is toggled
> > through the driver. Applying both the PCIe IP reset and the PERST#
> > improves the reliability of the reset process by ensuring that both
> > the Root Port controller and the Endpoint are reset synchronously
> > and avoid lane errors.
> >
> > Adapt the implementation to use the GPIO framework for reset signal
> > handling and make this reset handling optional, along with the
> > `cpm_crx` property, to maintain backward compatibility with existing
> > device tree binaries (DTBs).
>
> > Additionally, clear Firewall after the link reset for CPM5NC to allow
> > further PCIe transactions.
>
> > -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;
> > +     bool do_reset = false;
> > +
> > +     if (port->crx_base && (variant->version < CPM5NC_HOST ||
> > +                            (variant->version == CPM5NC_HOST &&
> > +                             port->cpm5nc_fw_base))) {
>
> Would be nicer if you could simply test for the feature, not the
> specific variants, e.g.,
>
>   if (port->crx_base && port->perst_gpio) {
>     writel_relaxed(0x1, port->crx_base + variant->cpm_pcie_rst);
>     udelay(100);
>     writel_relaxed(0x0, port->crx_base + variant->cpm_pcie_rst);
>     gpiod_set_value(port->perst_gpio, 0);
>     mdelay(PCIE_T_RRS_READY_MS);
>   }
>
>   if (port->firewall_base) {
>     /* Clear Firewall */
>   }
>

Thanks for the suggestion, I will change the test condition as per above.

> If you need to check the variants vs "cpm_crx", I think that should go
> in xilinx_cpm_pcie_parse_dt().
>

As per suggestion from Manivannan Sadasivam, I will be moving 'reset-gpios'
to PCIe bridge node, so test with variants will be removed. Thanks.
https://lore.kernel.org/all/ph5rby7y3jnu4fnbhiojesu6dsnre63vc4hmsjyasajrvurj6g@g6eo7lvjtuax/

> > +             /* Request the GPIO for PCIe reset signal and assert */
> > +             reset_gpio = devm_gpiod_get_optional(dev, "reset",
> GPIOD_OUT_HIGH);
> > +             if (IS_ERR(reset_gpio))
> > +                     return dev_err_probe(dev, PTR_ERR(reset_gpio),
> > +                                          "Failed to request reset GPIO\n");
> > +             if (reset_gpio)
> > +                     do_reset = true;
> > +     }
>
> Maybe the devm_gpiod_get_optional() could go in
> xilinx_cpm_pcie_parse_dt() along with other DT stuff, as is done in
> starfive_pcie_parse_dt()/starfive_pcie_host_init()?
>
> You'd have to save the port->reset_gpio pointer so we could use it
> here, but wouldn't have to return error from
> xilinx_cpm_pcie_init_port().
>

Thanks, I will move devm_gpiod_get_optional() to xilinx_cpm_pcie_parse_dt(),
save the port->reset_gpio and use it.

> > +
> > +     if (do_reset) {
> > +             /* Assert the PCIe IP reset */
> > +             writel_relaxed(0x1, port->crx_base + variant->cpm_pcie_rst);
> > +
> > +             /*
> > +              * "PERST# active time", as per Table 2-10: Power Sequencing
> > +              * and Reset Signal Timings of the PCIe Electromechanical
> > +              * Specification, Revision 6.0, symbol "T_PERST".
> > +              */
> > +             udelay(100);
>
> Whatever we need here, this should be a #define from drivers/pci/pci.h
> instead of 100.
>

Thanks, as per your suggestion, I will add new macro and include a citation
to the relevant section of the PCIe spec.

> > +
> > +             /* 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);
>
> I think reset_gpio controls PERST#.  If so, it would be nice to have
> "perst" in the name to make it less ambiguous.
>

Sure, I will rename variable to "perst_gpio".

> > +             mdelay(PCIE_T_RRS_READY_MS);
>
> We only wait PCIE_T_RRS_READY_MS for certain variants and only when
> the optional "cpm_crx" and "reset" properties are present.
>
> What about the other cases?  Unless there's something that guarantees
> a delay after the link comes up before we call pci_host_probe(), that
> sounds like a bug in the existing driver.  If it is a bug, you should
> fix it in its own separate patch.
>

The PCIe IP reset and PERST# signals are reset in the hardware logic.
In the driver, we are just toggling the PERST# and PCIe IP reset bits to assert
and deassert these resets.

In our current setup, the PCIe link comes up successfully even without the
"cpm_crx" and "reset" Device Tree properties.

This is not a bug, the reset handling in driver will be useful during warm reboot
where hardware logic will be not be reprogrammed again.

> > +             if (variant->version == CPM5NC_HOST &&
> > +                 port->cpm5nc_fw_base) {
>
> Unnecessary to test both variant->version and port->cpm5nc_fw_base
> here, since only CPM5NC_HOST sets cpm5nc_fw_base.
>
> The function of the "Firewall" should be explained in the commit log,
> and it seems like the sort of thing that's likely to appear in future
> variants, so "cpm5nc_" seems like it might be unnecessarily specific.
> Maybe consider naming these "firewall_base" and "firewall_reset" so
> the test and the writes wouldn't have to change for future variants.
>

We're currently discussing internally the possibility of handling the
CPM5NC firewall control in firmware. If that approach proves viable,
I may be able to drop Firewall from the driver-side handling altogether.

If firmware-based handling doesn't work out, I'll revise the implementation
accordingly, including renaming the fields to something more generic like
"firewall_base" and "firewall_reset", as you suggested, to better support
future variants.

> > +                     /* Clear Firewall */
> > +                     writel_relaxed(0x00, port->cpm5nc_fw_base +
> > +                                    variant->cpm5nc_fw_rst);
> > +                     writel_relaxed(0x01, port->cpm5nc_fw_base +
> > +                                    variant->cpm5nc_fw_rst);
> > +                     writel_relaxed(0x00, port->cpm5nc_fw_base +
> > +                                    variant->cpm5nc_fw_rst);
> > +             }
> > +     }
> >
> >       if (variant->version == CPM5NC_HOST)
>
> You didn't change this test, but it would be better if you could test
> for a *feature* instead of a specific variant.  Then you can avoid
> changes when future chips have the same feature.
>

At present, CPM5NC doesn't have Error interrupts and will be added in the
coming patches, so this condition check will be removed soon. Thanks.

> > -             return;
> > +             return 0;
> >
> >       if (cpm_pcie_link_up(port))
> >               dev_info(port->dev, "PCIe Link is UP\n");
> > @@ -512,6 +574,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;
> >  }