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

From: Manivannan Sadhasivam
Date: Thu Mar 27 2025 - 13:25:53 EST

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.

> + /* 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?

> + if (IS_ERR(port->cpm5nc_attr_base)) {
> + if (PTR_ERR(port->cpm5nc_attr_base) == -EINVAL)

Why?

- Mani

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