RE: [RESEND PATCH v7 1/2] dt-bindings: PCI: xilinx-cpm: Add `cpm_crx` and `cpm5nc_fw_attr` properties
From: Musham, Sai Krishna
Date: Mon Apr 14 2025 - 08:23:40 EST
[AMD Official Use Only - AMD Internal Distribution Only]
Hi Krzysztof,
Thanks for the review.
> -----Original Message-----
> From: Krzysztof Kozlowski <krzk@xxxxxxxxxx>
> Sent: Monday, April 14, 2025 12:32 PM
> To: Musham, Sai Krishna <sai.krishna.musham@xxxxxxx>
> Cc: bhelgaas@xxxxxxxxxx; lpieralisi@xxxxxxxxxx; kw@xxxxxxxxx;
> manivannan.sadhasivam@xxxxxxxxxx; robh@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 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?
>
For our controller, if cpm_crx is not provided lane errors will be observed.
Specifically for CPM5NC, if cpm5nc_fw_attr property is not provided, the firewall
is not cleared after reset and further PCIe transactions will not be allowed.
Therefore, these properties must be defined.
> >
> > 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.
> >
>
> ...
>
> > + - 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?
>
MSI and MSI-X interrupts are handled via GIC, so msi-map property is
required for interrupt handling.
Legacy interrupt support is not available, and Error interrupt support will be
added in future, once it is added corresponding DT changes will be added.
> >
> > 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. :/
Ok, I will drop this example.
>
> > + reg-names = "cpm_slcr", "cfg", "cpm_crx", "cpm5nc_fw_attr";
> > + reset-gpios = <&gpio0 22 GPIO_ACTIVE_LOW>;
> > + };
> > +
> > };
> > --
> > 2.44.1
> >
Thanks,
Sai Krishna