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

["Musham, Sai Krishna" <sai.krishna.musham@amd.com>]

[AMD Official Use Only - AMD Internal Distribution Only]

Hi Bjorn,

> -----Original Message-----
> From: Bjorn Helgaas <helgaas@kernel.org>
> Sent: Tuesday, June 17, 2025 8:17 PM
> To: Musham, Sai Krishna <sai.krishna.musham@amd.com>
> Cc: Manivannan Sadhasivam <mani@kernel.org>; bhelgaas@google.com;
> lpieralisi@kernel.org; kw@linux.com; manivannan.sadhasivam@linaro.org;
> robh@kernel.org; krzk+dt@kernel.org; conor+dt@kernel.org; cassel@kernel.org;
> linux-pci@vger.kernel.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org; Simek, Michal <michal.simek@amd.com>; Gogada, Bharat
> Kumar <bharat.kumar.gogada@amd.com>; Havalige, Thippeswamy
> <thippeswamy.havalige@amd.com>
> 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@kernel.org>
> > > Sent: Thursday, June 12, 2025 10:49 PM
> > > To: Musham, Sai Krishna <sai.krishna.musham@amd.com>
> > > Cc: bhelgaas@google.com; lpieralisi@kernel.org; kw@linux.com;
> > > manivannan.sadhasivam@linaro.org; robh@kernel.org; krzk+dt@kernel.org;
> > > conor+dt@kernel.org; cassel@kernel.org; linux-pci@vger.kernel.org;
> > > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Simek, Michal
> > > <michal.simek@amd.com>; Gogada, Bharat Kumar
> > > <bharat.kumar.gogada@amd.com>; Havalige, Thippeswamy
> > > <thippeswamy.havalige@amd.com>
> > > 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