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

[Krzysztof Kozlowski <krzk@kernel.org>]

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
>