RE: [PATCH v6 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,

Thanks for the review.

> -----Original Message-----
> From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Sent: Wednesday, April 9, 2025 12:25 PM
> To: Musham, Sai Krishna <sai.krishna.musham@amd.com>
> Cc: bhelgaas@google.com; lpieralisi@kernel.org; kw@linux.com; 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: [PATCH v6 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 Fri, Apr 04, 2025 at 06:59:23AM +0000, Musham, Sai Krishna wrote:
> > [AMD Official Use Only - AMD Internal Distribution Only]
> >
> > Hi Manivannan,
> >
> > Thanks for the review.
> >
> > > -----Original Message-----
> > > From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > Sent: Thursday, March 27, 2025 10:56 PM
> > > To: Musham, Sai Krishna <sai.krishna.musham@amd.com>
> > > Cc: bhelgaas@google.com; lpieralisi@kernel.org; kw@linux.com;
> > > 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: [PATCH v6 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 Wed, Mar 26, 2025 at 07:58:11AM +0530, Sai Krishna Musham wrote:
> > > > Add PCIe IP reset along with GPIO-based control for the PCIe Root
> > > > Port PERST# signal. Synchronizing the PCIe IP reset with the
> > > > PERST# signal's assertion and deassertion avoids Link Training failures.
> > > >
> > > > Adapt to use GPIO framework and make reset optional to maintain
> > > > backward compatibility with existing DTBs.
> > > >
> > > > Add clear firewall after Link reset for CPM5NC.
> > > >
> > > > Signed-off-by: Sai Krishna Musham <sai.krishna.musham@amd.com>
> > > > ---
> > > > 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 | 86
> > > > ++++++++++++++++++++++--
> > > >  1 file changed, 82 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/pci/controller/pcie-xilinx-cpm.c
> > > > b/drivers/pci/controller/pcie-xilinx-
> > > cpm.c
> > > > index d0ab187d917f..b10c0752a94f 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   0x00001140
> > > > +
> > > >  #define XILINX_CPM_PCIE_REG_IDR              0x00000E10
> > > >  #define XILINX_CPM_PCIE_REG_IMR              0x00000E14
> > > >  #define XILINX_CPM_PCIE_REG_PSCR     0x00000E1C
> > > > @@ -99,6 +108,7 @@ struct xilinx_cpm_variant {
> > > >       u32 ir_status;
> > > >       u32 ir_enable;
> > > >       u32 ir_misc_value;
> > > > +     u32 cpm_pcie_rst;
> > > >  };
> > > >
> > > >  /**
> > > > @@ -106,6 +116,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_attr_base: CPM5NC Control and Status Registers Base
> > > >   * @intx_domain: Legacy IRQ domain pointer
> > > >   * @cpm_domain: CPM IRQ domain pointer
> > > >   * @cfg: Holds mappings of config space window @@ -118,6 +130,8
> > > > @@ struct xilinx_cpm_pcie {
> > > >       struct device                   *dev;
> > > >       void __iomem                    *reg_base;
> > > >       void __iomem                    *cpm_base;
> > > > +     void __iomem                    *crx_base;
> > > > +     void __iomem                    *cpm5nc_attr_base;
> > > >       struct irq_domain               *intx_domain;
> > > >       struct irq_domain               *cpm_domain;
> > > >       struct pci_config_window        *cfg;
> > > > @@ -475,12 +489,45 @@ 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;
> > > > +
> > > > +     /* Request the GPIO for PCIe reset signal */
> > > > +     reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> > > > +     if (IS_ERR(reset_gpio)) {
> > > > +             if (PTR_ERR(reset_gpio) != -EPROBE_DEFER)
> > > > +                     dev_err(dev, "Failed to request reset GPIO\n");
> > > > +             return PTR_ERR(reset_gpio);
> > > > +     }
> > > >
> > > > -     if (variant->version == CPM5NC_HOST)
> > > > -             return;
> > > > +     if (reset_gpio && port->crx_base) {
> > > > +             /* Assert the PCIe IP reset */
> > > > +             writel_relaxed(0x1, port->crx_base +
> > > > + variant->cpm_pcie_rst);
> > > > +
> > > > +             /* Controller specific delay */
> > > > +             udelay(50);
> > > > +
> > >
> > > There should be atleast 100ms delay before PERST# deassert as per
> > > the spec. So use PCIE_T_PVPERL_MS. I know that you had it before,
> > > but removed in v4. I don't see a valid reason for that.
> >
> > For CPM/CPM5/CPM5NC, the "Power Up" sequence mentioned in section
> > 2.2.1 of PCIe Electromechanical Spec is handled in the design. The
> > PERST# we are using here is applied after the Power Up sequence and
> > will be used for warm reset, where power of the system is already stable.
> >
>
> I don't quite understand what you mean by 'warm reset' here. Even if the power was
> already stable, what is the guarantee that the 100ms time is elapsed before
> deasserting the PERST#? Does the hardware logic ensure 100ms time is elapsed
> before the driver is probed?
>

The Initial Power Up sequence is handled in hardware logic, and 100ms
(T_PVPERL) delay is provided after the power becomes stable. Yes, this part
is handled before the driver is probed.

By "warm reset" here, I'm referring to a reset that does not involve power
cycling the device, as per PCIe spec section 6.6.1. The power rails remain
stable, and only PERST# is toggled through the driver.

As per the PCIe Spec replaced 50us with 100us (T_PERST) before PERST#
deassert in driver, will send it in next patch.

> > So, we changed the delay after PERST# and IP reset assertion to 50us
> > controller specific delay, similar to TPERST(PERST# active time 100us)
> > delay in "Power sequencing and Reset Signal Timings" of PCIe
> > Electromechanical Spec. After deassertion of PERST# signal and IP
> > reset, a delay of PCIE_T_RRS_READY_MS is required before checking the Link.
> Please let me know if you have further queries.
> >
>
> This part is fine.
>
> > Thanks, I will update this information in commit message.
> > >
> > > > +             /* Deassert the PCIe IP reset */
> > > > +             writel_relaxed(0x0, port->crx_base +
> > > > + variant->cpm_pcie_rst);
> > > > +
> > > > +             /* Deassert the reset signal */
> > > > +             gpiod_set_value(reset_gpio, 0);
> > > > +             mdelay(PCIE_T_RRS_READY_MS);
> > > > +
> > > > +             if (variant->version == CPM5NC_HOST && port-
> >cpm5nc_attr_base) {
> > > > +                     /* Clear Firewall */
> > > > +                     writel_relaxed(0x00, port->cpm5nc_attr_base +
> > > > +                                     XILINX_CPM5NC_PCIE0_FRWALL);
> > > > +                     writel_relaxed(0x01, port->cpm5nc_attr_base +
> > > > +                                     XILINX_CPM5NC_PCIE0_FRWALL);
> > > > +                     writel_relaxed(0x00, port->cpm5nc_attr_base +
> > > > +                                     XILINX_CPM5NC_PCIE0_FRWALL);
> > > > +                     return 0;
> > > > +             }
> > > > +     }
> > > >
> > > >       if (cpm_pcie_link_up(port))
> > > >               dev_info(port->dev, "PCIe Link is UP\n"); @@ -512,6
> > > > +559,8 @@ static void xilinx_cpm_pcie_init_port(struct
> > > xilinx_cpm_pcie *port)
> > > >       pcie_write(port, pcie_read(port, XILINX_CPM_PCIE_REG_RPSC) |
> > > >                  XILINX_CPM_PCIE_REG_RPSC_BEN,
> > > >                  XILINX_CPM_PCIE_REG_RPSC);
> > > > +
> > > > +     return 0;
> > > >  }
> > > >
> > > >  /**
> > > > @@ -551,6 +600,27 @@ static int xilinx_cpm_pcie_parse_dt(struct
> > > xilinx_cpm_pcie *port,
> > > >               port->reg_base = port->cfg->win;
> > > >       }
> > > >
> > > > +     port->crx_base = devm_platform_ioremap_resource_byname(pdev,
> > > > +                                                            "cpm_crx");
> > > > +     if (IS_ERR(port->crx_base)) {
> > > > +             if (PTR_ERR(port->crx_base) == -EINVAL)
> > > > +                     port->crx_base = NULL;
> > > > +             else
> > > > +                     return PTR_ERR(port->crx_base);
> > > > +     }
> > > > +
> > > > +     if (port->variant->version == CPM5NC_HOST) {
> > > > +             port->cpm5nc_attr_base =
> > > > +                     devm_platform_ioremap_resource_byname(pdev,
> > > > +
> > > > + "cpm5nc_attr");
> > >
> > > Where is this resource defined in the binding?
> >
> > This patch is tested for mentioned CPM versions, I apologize that I
> > missed adding the cpm5nc_attr resource in DT binding. I will not
> > repeat this again. I will add the resource in the next patch.
> > Thanks for your understanding.
> > >
> > > > +             if (IS_ERR(port->cpm5nc_attr_base)) {
> > > > +                     if (PTR_ERR(port->cpm5nc_attr_base) ==
> > > > + -EINVAL)
> > >
> > > Why?
> >
> > This condition check is added to make cpm5nc_attr_base optional, once
> > I add missing resource in DT this condition will be applicable.
>
> Why are you checking for -EINVAL? What does it correspond to?
>
> If your intention is to make the resource_get optional, you should use
> platform_get_resource_byname() first. If it returns NULL, then it means the resource
> is not defined in DT.
>

Yes, my intention is to make resource_get optional. Sure, I will use
platform_get_resource_byname(). Thanks.

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

Thanks,
Sai Krishna