RE: [PATCH v6 2/2] PCI: xilinx-cpm: Add support for PCIe RP PERST# signal

From: Musham, Sai Krishna
Date: Fri Apr 04 2025 - 02:59:39 EST

Next message: syzbot: "[syzbot] [perf?] WARNING in __free_event"
Previous message: Erick Karanja: "Re: [PATCH v2 1/3] staging: rtl8723bs: Modify struct rx_pkt_attrib attribute bdecrypted"
Next in thread: Musham, Sai Krishna: "RE: [PATCH v6 2/2] PCI: xilinx-cpm: Add support for PCIe RP PERST# signal"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

[AMD Official Use Only - AMD Internal Distribution Only]

Hi Manivannan,

Thanks for the review.

> -----Original Message-----
> From: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
> Sent: Thursday, March 27, 2025 10:56 PM
> To: Musham, Sai Krishna <sai.krishna.musham@xxxxxxx>
> Cc: bhelgaas@xxxxxxxxxx; lpieralisi@xxxxxxxxxx; kw@xxxxxxxxx; 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: [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@xxxxxxx>
> > ---
> > 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.

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.

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.
Thanks.
>
> - Mani
>
> --
> மணிவண்ணன் சதாசிவம்

Regards,
Sai Krishna

Next message: syzbot: "[syzbot] [perf?] WARNING in __free_event"
Previous message: Erick Karanja: "Re: [PATCH v2 1/3] staging: rtl8723bs: Modify struct rx_pkt_attrib attribute bdecrypted"
Next in thread: Musham, Sai Krishna: "RE: [PATCH v6 2/2] PCI: xilinx-cpm: Add support for PCIe RP PERST# signal"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]