Message-ID:
 <DM4PR12MB6158AD426CB1E5A7A0101917CD7CA@DM4PR12MB6158.namprd12.prod.outlook.com>
Date: Fri, 20 Jun 2025 02:52:33 +0000
From: "Musham, Sai Krishna" <sai.krishna.musham@....com>
To: Bjorn Helgaas <helgaas@...nel.org>
CC: "bhelgaas@...gle.com" <bhelgaas@...gle.com>, "lpieralisi@...nel.org"
	<lpieralisi@...nel.org>, "kw@...ux.com" <kw@...ux.com>, Manivannan Sadhasivam
	<mani@...nel.org>, "robh@...nel.org" <robh@...nel.org>, "krzk+dt@...nel.org"
	<krzk+dt@...nel.org>, "conor+dt@...nel.org" <conor+dt@...nel.org>,
	"cassel@...nel.org" <cassel@...nel.org>, "linux-pci@...r.kernel.org"
	<linux-pci@...r.kernel.org>, "devicetree@...r.kernel.org"
	<devicetree@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "Simek, Michal" <michal.simek@....com>,
	"Gogada, Bharat Kumar" <bharat.kumar.gogada@....com>, "Havalige, Thippeswamy"
	<thippeswamy.havalige@....com>
Subject: RE: [RESEND PATCH v7 2/2] PCI: xilinx-cpm: Add support for PCIe RP
 PERST# signal

[AMD Official Use Only - AMD Internal Distribution Only]

Hi Bjorn,

Thanks for the review.

> -----Original Message-----
> From: Bjorn Helgaas <helgaas@...nel.org>
> Sent: Friday, June 13, 2025 2:04 AM
> To: Musham, Sai Krishna <sai.krishna.musham@....com>
> Cc: bhelgaas@...gle.com; lpieralisi@...nel.org; kw@...ux.com;
> manivannan.sadhasivam@...aro.org; robh@...nel.org; krzk+dt@...nel.org;
> conor+dt@...nel.org; cassel@...nel.org; linux-pci@...r.kernel.org;
> devicetree@...r.kernel.org; linux-kernel@...r.kernel.org; Simek, Michal
> <michal.simek@....com>; Gogada, Bharat Kumar
> <bharat.kumar.gogada@....com>; Havalige, Thippeswamy
> <thippeswamy.havalige@....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.
>
> Please say something specific here about what this does.  I *think* it
> asserts both the PCIe IP reset (which I assume resets the host
> controller) and PERST# (which resets any devices connected to the Root
> Port), but only for devices that implement the CPM Clock and Reset
> Control Registers AND describe the address of those registers via
> DT "cpm_crx" AND describe a GPIO connected to PERST# via DT "reset".
>

Yes, in Hardware logic both PCIe IP reset and PERST# are reset for CPM
devices. I will include this in commit message.

> > 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.
>
> > -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))) {
>
> Would be nicer if you could simply test for the feature, not the
> specific variants, e.g.,
>
>   if (port->crx_base && port->perst_gpio) {
>     writel_relaxed(0x1, port->crx_base + variant->cpm_pcie_rst);
>     udelay(100);
>     writel_relaxed(0x0, port->crx_base + variant->cpm_pcie_rst);
>     gpiod_set_value(port->perst_gpio, 0);
>     mdelay(PCIE_T_RRS_READY_MS);
>   }
>
>   if (port->firewall_base) {
>     /* Clear Firewall */
>   }
>

Thanks for the suggestion, I will change the test condition as per above.

> If you need to check the variants vs "cpm_crx", I think that should go
> in xilinx_cpm_pcie_parse_dt().
>

As per suggestion from Manivannan Sadasivam, I will be moving 'reset-gpios'
to PCIe bridge node, so test with variants will be removed. Thanks.
https://lore.kernel.org/all/ph5rby7y3jnu4fnbhiojesu6dsnre63vc4hmsjyasajrvurj6g@g6eo7lvjtuax/

> > +             /* 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;
> > +     }
>
> Maybe the devm_gpiod_get_optional() could go in
> xilinx_cpm_pcie_parse_dt() along with other DT stuff, as is done in
> starfive_pcie_parse_dt()/starfive_pcie_host_init()?
>
> You'd have to save the port->reset_gpio pointer so we could use it
> here, but wouldn't have to return error from
> xilinx_cpm_pcie_init_port().
>

Thanks, I will move devm_gpiod_get_optional() to xilinx_cpm_pcie_parse_dt(),
save the port->reset_gpio and use it.

> > +
> > +     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);
>
> Whatever we need here, this should be a #define from drivers/pci/pci.h
> instead of 100.
>

Thanks, as per your suggestion, I will add new macro and include a citation
to the relevant section of the PCIe spec.

> > +
> > +             /* 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);
>
> I think reset_gpio controls PERST#.  If so, it would be nice to have
> "perst" in the name to make it less ambiguous.
>

Sure, I will rename variable to "perst_gpio".

> > +             mdelay(PCIE_T_RRS_READY_MS);
>
> We only wait PCIE_T_RRS_READY_MS for certain variants and only when
> the optional "cpm_crx" and "reset" properties are present.
>
> What about the other cases?  Unless there's something that guarantees
> a delay after the link comes up before we call pci_host_probe(), that
> sounds like a bug in the existing driver.  If it is a bug, you should
> fix it in its own separate patch.
>

The PCIe IP reset and PERST# signals are reset in the hardware logic.
In the driver, we are just toggling the PERST# and PCIe IP reset bits to assert
and deassert these resets.

In our current setup, the PCIe link comes up successfully even without the
"cpm_crx" and "reset" Device Tree properties.

This is not a bug, the reset handling in driver will be useful during warm reboot
where hardware logic will be not be reprogrammed again.

> > +             if (variant->version == CPM5NC_HOST &&
> > +                 port->cpm5nc_fw_base) {
>
> Unnecessary to test both variant->version and port->cpm5nc_fw_base
> here, since only CPM5NC_HOST sets cpm5nc_fw_base.
>
> The function of the "Firewall" should be explained in the commit log,
> and it seems like the sort of thing that's likely to appear in future
> variants, so "cpm5nc_" seems like it might be unnecessarily specific.
> Maybe consider naming these "firewall_base" and "firewall_reset" so
> the test and the writes wouldn't have to change for future variants.
>

We're currently discussing internally the possibility of handling the
CPM5NC firewall control in firmware. If that approach proves viable,
I may be able to drop Firewall from the driver-side handling altogether.

If firmware-based handling doesn't work out, I'll revise the implementation
accordingly, including renaming the fields to something more generic like
"firewall_base" and "firewall_reset", as you suggested, to better support
future variants.

> > +                     /* Clear Firewall */
> > +                     writel_relaxed(0x00, port->cpm5nc_fw_base +
> > +                                    variant->cpm5nc_fw_rst);
> > +                     writel_relaxed(0x01, port->cpm5nc_fw_base +
> > +                                    variant->cpm5nc_fw_rst);
> > +                     writel_relaxed(0x00, port->cpm5nc_fw_base +
> > +                                    variant->cpm5nc_fw_rst);
> > +             }
> > +     }
> >
> >       if (variant->version == CPM5NC_HOST)
>
> You didn't change this test, but it would be better if you could test
> for a *feature* instead of a specific variant.  Then you can avoid
> changes when future chips have the same feature.
>

At present, CPM5NC doesn't have Error interrupts and will be added in the
coming patches, so this condition check will be removed soon. Thanks.

> > -             return;
> > +             return 0;
> >
> >       if (cpm_pcie_link_up(port))
> >               dev_info(port->dev, "PCIe Link is UP\n");
> > @@ -512,6 +574,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;
> >  }

Powered by blists - more mailing lists