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

["Musham, Sai Krishna" <sai.krishna.musham@amd.com>]

[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>;