[AMD Official Use Only - AMD Internal Distribution Only]
Hi Bjorn,
> -----Original Message-----
> From: Bjorn Helgaas <helgaas@xxxxxxxxxx>
> Sent: Tuesday, June 17, 2025 8:17 PM
> To: Musham, Sai Krishna <sai.krishna.musham@xxxxxxx>
> Cc: Manivannan Sadhasivam <mani@xxxxxxxxxx>; 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 2/2] PCI: xilinx-cpm: Add support for PCIe RP
> PERST# signal
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> On Tue, Jun 17, 2025 at 04:14:37AM +0000, Musham, Sai Krishna wrote:
> > [AMD Official Use Only - AMD Internal Distribution Only]
> >
> > Hi Manivannan,
> >
> > > -----Original Message-----
> > > From: Manivannan Sadhasivam <mani@xxxxxxxxxx>
> > > Sent: Thursday, June 12, 2025 10:49 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 2/2] PCI: xilinx-cpm: Add support for PCIe RP
> > > PERST# signal
> > >
> > > 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:04AM +0530, Sai Krishna Musham wrote:
> > > > Add support for handling the PCIe Root Port (RP) PERST# signal using
> > > > the GPIO framework, along with the PCIe IP reset. This reset is
> > > > managed by the driver and occurs after the Initial Power Up sequence
> > > > (PCIe CEM r6.0, 2.2.1) is handled in hardware before the driver's probe
> > > > function is called.
>
> > > > + if (do_reset) {
> > > > + /* Assert the PCIe IP reset */
> > > > + writel_relaxed(0x1, port->crx_base + variant->cpm_pcie_rst);
> > > > +
> > > > + /*
> > > > + * "PERST# active time", as per Table 2-10: Power Sequencing
> > > > + * and Reset Signal Timings of the PCIe Electromechanical
> > > > + * Specification, Revision 6.0, symbol "T_PERST".
> > > > + */
> > > > + udelay(100);
> > >
> > > Are you sure that you need T_PERST here and not T_PVPERL? T_PERST
> > > is only valid while resuming from D3Cold i.e., after power up,
> > > while T_PVPERL is valid during the power up, which is usually the
> > > case when a controller driver probes. Is your driver relying on
> > > power being enabled by the bootloader and the driver just toggling
> > > PERST# to perform conventional reset of the endpoint?
> >
> > Thanks for pointing that out. Yes, the power-up sequence is handled
> > by the hardware, and the driver relies on power being enabled by it.
> > We're only toggling the PERST# signal in the driver to perform a
> > conventional reset of the endpoint. So, I'm confident that T_PERST
> > is the appropriate timing reference here, not T_PVPERL.
> >
> > Additionally, this delay was recommended by our hardware team, who
> > confirmed that the power-up sequence is managed in hardware logic,
> > and that T_PERST is the appropriate timing to apply in this context.
> >
> > I also checked pci.h but couldn't find a predefined macro for
> > T_PERST, so I used 100. Please let me know if there's a preferred
> > macro I should be using instead.
>
> If we need a new macro, please add it. Include a citation to the
> relevant section of the spec ("PCIe CEM r6.0, sec 2.11.2"; table
> numbers don't appear in the table of contents so they're hard to
> find), and include the units ("_US", I guess) in the macro name.
>
> Given a comment at the macro definition, you don't need to repeat it
> at all the uses.
>
Thanks for the review, sure I will add new macro and include a citation
to the relevant section of the PCIe spec.
> Bjorn
Thanks,
Sai Krishna
