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 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.
> >
> > This reset mechanism is particularly useful in warm reset scenarios,
> > where the power rails remain stable and only PERST# signal is toggled
> > through the driver. Applying both the PCIe IP reset and the PERST#
> > improves the reliability of the reset process by ensuring that both
> > the Root Port controller and the Endpoint are reset synchronously
> > and avoid lane errors.
> >
> > Adapt the implementation to use the GPIO framework for reset signal
> > handling and make this reset handling optional, along with the
> > `cpm_crx` property, to maintain backward compatibility with existing
> > device tree binaries (DTBs).
> >
> > Additionally, clear Firewall after the link reset for CPM5NC to allow
> > further PCIe transactions.
> >
> > Signed-off-by: Sai Krishna Musham <sai.krishna.musham@amd.com>
> > ---
> > Changes for v7:
> > - Use platform_get_resource_byname() to make cpm_crx and cpm5nc_fw_attr
> >   optional
> > - Use 100us delay T_PERST as per PCIe spec before PERST# deassert.
> >
> > Changes for v6:
> > - Correct version check condition of CPM5NC_HOST.
> >
> > Changes for v5:
> > - Handle probe defer for reset_gpio.
> > - Resolve ABI break.
> >
> > Changes for v4:
> > - Add PCIe PERST# support for CPM5NC.
> > - Add PCIe IP reset along with PERST# to avoid Link Training Errors.
> > - Remove PCIE_T_PVPERL_MS define and PCIE_T_RRS_READY_MS after
> >   PERST# deassert.
> > - Move PCIe PERST# assert and deassert logic to
> >   xilinx_cpm_pcie_init_port() before cpm_pcie_link_up(), since
> >   Interrupts enable and PCIe RP bridge enable should be done after
> >   Link up.
> > - Update commit message.
> >
> > Changes for v3:
> > - Use PCIE_T_PVPERL_MS define.
> >
> > Changes for v2:
> > - Make the request GPIO optional.
> > - Correct the reset sequence as per PERST#
> > - Update commit message
> > ---
> >  drivers/pci/controller/pcie-xilinx-cpm.c | 97 +++++++++++++++++++++++-
> >  1 file changed, 94 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/pci/controller/pcie-xilinx-cpm.c b/drivers/pci/controller/pcie-xilinx-
> cpm.c
> > index 13ca493d22bd..c46642417d52 100644
> > --- a/drivers/pci/controller/pcie-xilinx-cpm.c
> > +++ b/drivers/pci/controller/pcie-xilinx-cpm.c
> > @@ -6,6 +6,8 @@
> >   */
> >
> >  #include <linux/bitfield.h>
> > +#include <linux/delay.h>
> > +#include <linux/gpio/consumer.h>
> >  #include <linux/interrupt.h>
> >  #include <linux/irq.h>
> >  #include <linux/irqchip.h>
> > @@ -21,6 +23,13 @@
> >  #include "pcie-xilinx-common.h"
> >
> >  /* Register definitions */
> > +#define XILINX_CPM_PCIE0_RST         0x00000308
> > +#define XILINX_CPM5_PCIE0_RST                0x00000318
> > +#define XILINX_CPM5_PCIE1_RST                0x0000031C
> > +#define XILINX_CPM5NC_PCIE0_RST              0x00000324
> > +
> > +#define XILINX_CPM5NC_PCIE0_FRWALL   0x00000140
> > +
> >  #define XILINX_CPM_PCIE_REG_IDR              0x00000E10
> >  #define XILINX_CPM_PCIE_REG_IMR              0x00000E14
> >  #define XILINX_CPM_PCIE_REG_PSCR     0x00000E1C
> > @@ -93,12 +102,16 @@ enum xilinx_cpm_version {
> >   * @ir_status: Offset for the error interrupt status register
> >   * @ir_enable: Offset for the CPM5 local error interrupt enable register
> >   * @ir_misc_value: A bitmask for the miscellaneous interrupt status
> > + * @cpm_pcie_rst: Offset for the PCIe IP reset
> > + * @cpm5nc_fw_rst: Offset for the CPM5NC Firewall
> >   */
> >  struct xilinx_cpm_variant {
> >       enum xilinx_cpm_version version;
> >       u32 ir_status;
> >       u32 ir_enable;
> >       u32 ir_misc_value;
> > +     u32 cpm_pcie_rst;
> > +     u32 cpm5nc_fw_rst;
> >  };
> >
> >  /**
> > @@ -106,6 +119,8 @@ struct xilinx_cpm_variant {
> >   * @dev: Device pointer
> >   * @reg_base: Bridge Register Base
> >   * @cpm_base: CPM System Level Control and Status Register(SLCR) Base
> > + * @crx_base: CPM Clock and Reset Control Registers Base
> > + * @cpm5nc_fw_base: CPM5NC Firewall Attribute Base
> >   * @intx_domain: Legacy IRQ domain pointer
> >   * @cpm_domain: CPM IRQ domain pointer
> >   * @cfg: Holds mappings of config space window
> > @@ -118,6 +133,8 @@ struct xilinx_cpm_pcie {
> >       struct device                   *dev;
> >       void __iomem                    *reg_base;
> >       void __iomem                    *cpm_base;
> > +     void __iomem                    *crx_base;
> > +     void __iomem                    *cpm5nc_fw_base;
> >       struct irq_domain               *intx_domain;
> >       struct irq_domain               *cpm_domain;
> >       struct pci_config_window        *cfg;
> > @@ -475,12 +492,57 @@ static int xilinx_cpm_setup_irq(struct xilinx_cpm_pcie
> *port)
> >   * xilinx_cpm_pcie_init_port - Initialize hardware
> >   * @port: PCIe port information
> >   */
> > -static void xilinx_cpm_pcie_init_port(struct xilinx_cpm_pcie *port)
> > +static int xilinx_cpm_pcie_init_port(struct xilinx_cpm_pcie *port)
> >  {
> >       const struct xilinx_cpm_variant *variant = port->variant;
> > +     struct device *dev = port->dev;
> > +     struct gpio_desc *reset_gpio;
> > +     bool do_reset = false;
> > +
> > +     if (port->crx_base && (variant->version < CPM5NC_HOST ||
> > +                            (variant->version == CPM5NC_HOST &&
> > +                             port->cpm5nc_fw_base))) {
> > +             /* Request the GPIO for PCIe reset signal and assert */
> > +             reset_gpio = devm_gpiod_get_optional(dev, "reset",
> GPIOD_OUT_HIGH);
> > +             if (IS_ERR(reset_gpio))
> > +                     return dev_err_probe(dev, PTR_ERR(reset_gpio),
> > +                                          "Failed to request reset GPIO\n");
> > +             if (reset_gpio)
> > +                     do_reset = true;
> > +     }
> > +
> > +     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.

> - Mani
>
> --
> மணிவண்ணன் சதாசிவம்

Thanks,
Sai Krishna