On 14/04/2025 14:23, Musham, Sai Krishna wrote: > [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. This must be in the commit msg. > >>> >>> 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. I don't think commit msg explained this. > Best regards, Krzysztof
