[AMD Official Use Only - AMD Internal Distribution Only]
Hi Rob,
Thanks for the review.
> -----Original Message-----
> From: Rob Herring <robh@xxxxxxxxxx>
> Sent: Tuesday, April 15, 2025 8:44 PM
> To: Musham, Sai Krishna <sai.krishna.musham@xxxxxxx>
> Cc: bhelgaas@xxxxxxxxxx; lpieralisi@xxxxxxxxxx; kw@xxxxxxxxx;
> manivannan.sadhasivam@xxxxxxxxxx; krzk+dt@xxxxxxxxxx; conor+dt@xxxxxxxxxx;
> cassel@xxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; Simek, Michal <michal.simek@xxxxxxx>; Gogada, Bharat
> Kumar <bharat.kumar.gogada@xxxxxxx>; Havalige, Thippeswamy
> <thippeswamy.havalige@xxxxxxx>
> 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@xxxxxxx> 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@xxxxxxx>
> > ---
> > 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@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
> > + 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>;
