[AMD Official Use Only - AMD Internal Distribution Only] Hi Krzysztof, > -----Original Message----- > From: Krzysztof Kozlowski <krzk@xxxxxxxxxx> > Sent: Tuesday, April 15, 2025 11:04 AM > 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 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. > Sure, I will add this in commit message. Thanks. > > > >>> > >>> 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. > Sure, I will mention this also in commit message and send in next patch. > > > > > > Best regards, > Krzysztof Thanks, Sai Krishna
