Re: [RESEND PATCH v7 2/2] PCI: xilinx-cpm: Add support for PCIe RP PERST# signal

[Bjorn Helgaas]

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.

Bjorn