[AMD Official Use Only - AMD Internal Distribution Only]
Hi Bjorn,
Thanks for the review.
> -----Original Message-----
> From: Bjorn Helgaas <helgaas@xxxxxxxxxx>
> Sent: Friday, June 13, 2025 2:04 AM
> 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.
>
> 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;
> > }
