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

[Krzysztof Kozlowski]

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

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

> 
> Traditionally, the reset was managed in hardware and enabled during
> initialization. With this patch set, the reset will be handled by the
> driver. Consequently, the `reset-gpios` property must be explicitly
> provided to ensure proper functionality.
> 
> Add CPM clock and reset control registers base (`cpm_crx`) to handle
> PCIe IP reset along with PCIe RP PERST# to avoid Link Training errors.

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

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

That's an ABI break.

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

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

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

Same

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

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

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

Best regards,
Krzysztof