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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CAL_JsqJvRFR1Su8ajWOiAKaz2n1JH9RK0RojhhPKsiKsGeGm4Q@mail.gmail.com>
Date: Tue, 15 Apr 2025 10:14:20 -0500
From: Rob Herring <robh@...nel.org>
To: Sai Krishna Musham <sai.krishna.musham@....com>
Cc: bhelgaas@...gle.com, lpieralisi@...nel.org, kw@...ux.com, 
	manivannan.sadhasivam@...aro.org, krzk+dt@...nel.org, conor+dt@...nel.org, 
	cassel@...nel.org, linux-pci@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org, michal.simek@....com, 
	bharat.kumar.gogada@....com, thippeswamy.havalige@....com
Subject: Re: [RESEND PATCH v7 1/2] dt-bindings: PCI: xilinx-cpm: Add `cpm_crx`
 and `cpm5nc_fw_attr` properties

On Sun, Apr 13, 2025 at 10:23 PM Sai Krishna Musham
<sai.krishna.musham@....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@....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@....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>;