[AMD Official Use Only - AMD Internal Distribution Only]
Hi Manivannan,
> -----Original Message-----
> From: Manivannan Sadhasivam <mani@xxxxxxxxxx>
> Sent: Thursday, June 12, 2025 10:49 PM
> 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.
> >
> > 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@xxxxxxx>
> > ---
> > 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
