Re: [PATCH] PCI: zynqmp: Add ZynqMP NWL PCIe root port driver

Hello! I have looked at this change and below are my comments.

On Tuesday 23 May 2023 14:57:38 Stefan Roese wrote:

This patch adds the PCIe controller driver for the Xilinx / AMD ZynqMP NWL PCIe Bridge as root port. The driver source is partly copied from the Linux PCI driver and modified to enable usage in U-Boot (e.g. simplified and interrupt support removed).

Signed-off-by: Stefan Roese sr@denx.de Cc: Simon Glass sjg@chromium.org Cc: Pali Rohár pali@kernel.org Cc: Bin Meng bmeng.cn@gmail.com Cc: Michal Simek michal.simek@amd.com

---

MAINTAINERS | 1 + drivers/pci/Kconfig | 7 + drivers/pci/Makefile | 1 + drivers/pci/pcie-xilinx-nwl.c | 374 ++++++++++++++++++++++++++++++++++ 4 files changed, 383 insertions(+) create mode 100644 drivers/pci/pcie-xilinx-nwl.c

diff --git a/MAINTAINERS b/MAINTAINERS index c8f72e9ec6a2..2bc19d9daa75 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -343,6 +343,7 @@ F: drivers/rtc/armada38x.c F: drivers/spi/kirkwood_spi.c F: drivers/spi/mvebu_a3700_spi.c F: drivers/pci/pcie_dw_mvebu.c +F: drivers/pci/pcie-xilinx-nwl.c F: drivers/watchdog/armada-37xx-wdt.c F: drivers/watchdog/orion_wdt.c F: include/configs/mv-common.h diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig index ef328d26525b..60d98d14640d 100644 --- a/drivers/pci/Kconfig +++ b/drivers/pci/Kconfig @@ -374,4 +374,11 @@ config PCIE_UNIPHIER Say Y here if you want to enable PCIe controller support on UniPhier SoCs.

+config PCIE_XILINX_NWL

• bool "Xilinx NWL PCIe controller"
• depends on ARCH_ZYNQMP
• help

• Say 'Y' here if you want support for Xilinx / AMD NWL PCIe
  

• controller as Root Port.
  

endif diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile index 49506e7ba59b..11f60c6991d9 100644 --- a/drivers/pci/Makefile +++ b/drivers/pci/Makefile @@ -49,3 +49,4 @@ obj-$(CONFIG_PCI_OCTEONTX) += pci_octeontx.o obj-$(CONFIG_PCIE_OCTEON) += pcie_octeon.o obj-$(CONFIG_PCIE_DW_SIFIVE) += pcie_dw_sifive.o obj-$(CONFIG_PCIE_UNIPHIER) += pcie_uniphier.o +obj-$(CONFIG_PCIE_XILINX_NWL) += pcie-xilinx-nwl.o diff --git a/drivers/pci/pcie-xilinx-nwl.c b/drivers/pci/pcie-xilinx-nwl.c new file mode 100644 index 000000000000..3d64a0e3b961 --- /dev/null +++ b/drivers/pci/pcie-xilinx-nwl.c @@ -0,0 +1,374 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • PCIe host bridge driver for Xilinx / AMD ZynqMP NWL PCIe Bridge
• • Based on the Linux driver which is:
• • (C) Copyright 2014 - 2015, Xilinx, Inc.
• • Author: Stefan Roese sr@denx.de
• */

+#include <clk.h> +#include <dm.h> +#include <dm/device_compat.h> +#include <dm/devres.h> +#include <mapmem.h> +#include <pci.h> +#include <linux/delay.h> +#include <linux/io.h> +#include <linux/ioport.h>

+/* Bridge core config registers */ +#define BRCFG_PCIE_RX0 0x00000000 +#define BRCFG_PCIE_RX1 0x00000004 +#define BRCFG_INTERRUPT 0x00000010 +#define BRCFG_PCIE_RX_MSG_FILTER 0x00000020

+/* Egress - Bridge translation registers */ +#define E_BREG_CAPABILITIES 0x00000200 +#define E_BREG_CONTROL 0x00000208 +#define E_BREG_BASE_LO 0x00000210 +#define E_BREG_BASE_HI 0x00000214 +#define E_ECAM_CAPABILITIES 0x00000220 +#define E_ECAM_CONTROL 0x00000228 +#define E_ECAM_BASE_LO 0x00000230 +#define E_ECAM_BASE_HI 0x00000234

+#define I_ISUB_CONTROL 0x000003E8 +#define SET_ISUB_CONTROL BIT(0) +/* Rxed msg fifo - Interrupt status registers */ +#define MSGF_MISC_STATUS 0x00000400 +#define MSGF_MISC_MASK 0x00000404 +#define MSGF_LEG_STATUS 0x00000420 +#define MSGF_LEG_MASK 0x00000424 +#define MSGF_MSI_STATUS_LO 0x00000440 +#define MSGF_MSI_STATUS_HI 0x00000444 +#define MSGF_MSI_MASK_LO 0x00000448 +#define MSGF_MSI_MASK_HI 0x0000044C

+/* Msg filter mask bits */ +#define CFG_ENABLE_PM_MSG_FWD BIT(1) +#define CFG_ENABLE_INT_MSG_FWD BIT(2) +#define CFG_ENABLE_ERR_MSG_FWD BIT(3) +#define CFG_ENABLE_MSG_FILTER_MASK (CFG_ENABLE_PM_MSG_FWD | \

• 			 CFG_ENABLE_INT_MSG_FWD |	\
  

• 			 CFG_ENABLE_ERR_MSG_FWD)
  

+/* Misc interrupt status mask bits */ +#define MSGF_MISC_SR_RXMSG_AVAIL BIT(0) +#define MSGF_MISC_SR_RXMSG_OVER BIT(1) +#define MSGF_MISC_SR_SLAVE_ERR BIT(4) +#define MSGF_MISC_SR_MASTER_ERR BIT(5) +#define MSGF_MISC_SR_I_ADDR_ERR BIT(6) +#define MSGF_MISC_SR_E_ADDR_ERR BIT(7) +#define MSGF_MISC_SR_FATAL_AER BIT(16) +#define MSGF_MISC_SR_NON_FATAL_AER BIT(17) +#define MSGF_MISC_SR_CORR_AER BIT(18) +#define MSGF_MISC_SR_UR_DETECT BIT(20) +#define MSGF_MISC_SR_NON_FATAL_DEV BIT(22) +#define MSGF_MISC_SR_FATAL_DEV BIT(23) +#define MSGF_MISC_SR_LINK_DOWN BIT(24) +#define MSGF_MSIC_SR_LINK_AUTO_BWIDTH BIT(25) +#define MSGF_MSIC_SR_LINK_BWIDTH BIT(26)

+#define MSGF_MISC_SR_MASKALL (MSGF_MISC_SR_RXMSG_AVAIL | \

• 			 MSGF_MISC_SR_RXMSG_OVER |	\
  

• 			 MSGF_MISC_SR_SLAVE_ERR |	\
  

• 			 MSGF_MISC_SR_MASTER_ERR |	\
  

• 			 MSGF_MISC_SR_I_ADDR_ERR |	\
  

• 			 MSGF_MISC_SR_E_ADDR_ERR |	\
  

• 			 MSGF_MISC_SR_FATAL_AER |	\
  

• 			 MSGF_MISC_SR_NON_FATAL_AER |	\
  

• 			 MSGF_MISC_SR_CORR_AER |	\
  

• 			 MSGF_MISC_SR_UR_DETECT |	\
  

• 			 MSGF_MISC_SR_NON_FATAL_DEV |	\
  

• 			 MSGF_MISC_SR_FATAL_DEV |	\
  

• 			 MSGF_MISC_SR_LINK_DOWN |	\
  

• 			 MSGF_MSIC_SR_LINK_AUTO_BWIDTH | \
  

• 			 MSGF_MSIC_SR_LINK_BWIDTH)
  

+/* Legacy interrupt status mask bits */ +#define MSGF_LEG_SR_INTA BIT(0) +#define MSGF_LEG_SR_INTB BIT(1) +#define MSGF_LEG_SR_INTC BIT(2) +#define MSGF_LEG_SR_INTD BIT(3) +#define MSGF_LEG_SR_MASKALL (MSGF_LEG_SR_INTA | MSGF_LEG_SR_INTB | \

• 			 MSGF_LEG_SR_INTC | MSGF_LEG_SR_INTD)
  

+/* MSI interrupt status mask bits */ +#define MSGF_MSI_SR_LO_MASK GENMASK(31, 0) +#define MSGF_MSI_SR_HI_MASK GENMASK(31, 0)

+/* Bridge config interrupt mask */ +#define BRCFG_INTERRUPT_MASK BIT(0) +#define BREG_PRESENT BIT(0) +#define BREG_ENABLE BIT(0) +#define BREG_ENABLE_FORCE BIT(1)

+/* E_ECAM status mask bits */ +#define E_ECAM_PRESENT BIT(0) +#define E_ECAM_CR_ENABLE BIT(0) +#define E_ECAM_SIZE_LOC GENMASK(20, 16) +#define E_ECAM_SIZE_SHIFT 16 +#define NWL_ECAM_VALUE_DEFAULT 12

+#define CFG_DMA_REG_BAR GENMASK(2, 0) +#define CFG_PCIE_CACHE GENMASK(7, 0)

+/* Readin the PS_LINKUP */ +#define PS_LINKUP_OFFSET 0x00000238 +#define PCIE_PHY_LINKUP_BIT BIT(0) +#define PHY_RDY_LINKUP_BIT BIT(1)

+/* Parameters for the waiting for link up routine */ +#define LINK_WAIT_MAX_RETRIES 10 +#define LINK_WAIT_USLEEP_MIN 90000 +#define LINK_WAIT_USLEEP_MAX 100000

+struct nwl_pcie {

• struct udevice *dev;
• void __iomem *breg_base;
• void __iomem *pcireg_base;
• void __iomem *ecam_base;
• phys_addr_t phys_breg_base; /* Physical Bridge Register Base */
• phys_addr_t phys_pcie_reg_base; /* Physical PCIe Controller Base */

Unused member

• phys_addr_t phys_ecam_base; /* Physical Configuration Base */
• u32 breg_size;

Unused member

• u32 pcie_reg_size;

Unused member

• u32 ecam_size;

Unused member

• u32 ecam_value;
• u8 last_busno;

Unused member

+};

+static int nwl_pcie_config_address(const struct udevice *bus,

• 		   pci_dev_t bdf, uint offset,
  

• 		   void **paddress)
  

+{

• struct nwl_pcie *pcie = dev_get_priv(bus);
• void *addr;
• addr = pcie->ecam_base;
• addr += PCIE_ECAM_OFFSET(PCI_BUS(bdf), PCI_DEV(bdf),

• 		 PCI_FUNC(bdf), offset);
  

As pci bus number you should use: PCI_BUS(bdf)-dev_seq(bus) This is because U-Boot PCI bus numbers are relative to the bus number on which U-Boot puts PCIe Root Port; and because U-Boot does that bus remapping. Normally first controller has dev seq 0, so this issue is not visible on system without multiple PCIe controllers. I guess this is here in U-Boot because U-Boot does not have PCI domains.

• *paddress = addr;
• return 0;

+}

+static int nwl_pcie_read_config(const struct udevice *bus, pci_dev_t bdf,

• 		uint offset, ulong *valuep,
  

• 		enum pci_size_t size)
  

+{

• return pci_generic_mmap_read_config(bus, nwl_pcie_config_address,

• 			    bdf, offset, valuep, size);
  

+}

+static int nwl_pcie_write_config(struct udevice *bus, pci_dev_t bdf,

• 		 uint offset, ulong value,
  

• 		 enum pci_size_t size)
  

+{

• return pci_generic_mmap_write_config(bus, nwl_pcie_config_address,

• 			     bdf, offset, value, size);
  

+}

+static const struct dm_pci_ops nwl_pcie_ops = {

• .read_config = nwl_pcie_read_config,
• .write_config = nwl_pcie_write_config,

+};

+static inline u32 nwl_bridge_readl(struct nwl_pcie *pcie, u32 off) +{

• return readl(pcie->breg_base + off);

+}

+static inline void nwl_bridge_writel(struct nwl_pcie *pcie, u32 val, u32 off) +{

• writel(val, pcie->breg_base + off);

+}

+static bool nwl_pcie_link_up(struct nwl_pcie *pcie) +{

• if (readl(pcie->pcireg_base + PS_LINKUP_OFFSET) & PCIE_PHY_LINKUP_BIT)

• return true;
  

• return false;

+}

+static bool nwl_phy_link_up(struct nwl_pcie *pcie) +{

• if (readl(pcie->pcireg_base + PS_LINKUP_OFFSET) & PHY_RDY_LINKUP_BIT)

• return true;
  

• return false;

+}

+static int nwl_wait_for_link(struct nwl_pcie *pcie) +{

• struct udevice *dev = pcie->dev;
• int retries;
• /* check if the link is up or not */
• for (retries = 0; retries < LINK_WAIT_MAX_RETRIES; retries++) {

• if (nwl_phy_link_up(pcie))
  

• 	return 0;
  

• udelay(LINK_WAIT_USLEEP_MIN);
  

• }
• dev_warn(dev, "PHY link never came up\n");
• return -ETIMEDOUT;

+}

+static int nwl_pcie_bridge_init(struct nwl_pcie *pcie) +{

• struct udevice *dev = pcie->dev;
• u32 breg_val, ecam_val, first_busno = 0;
• int err;
• breg_val = nwl_bridge_readl(pcie, E_BREG_CAPABILITIES) & BREG_PRESENT;
• if (!breg_val) {

• dev_err(dev, "BREG is not present\n");
  

• return breg_val;
  

• }
• /* Write bridge_off to breg base */
• nwl_bridge_writel(pcie, lower_32_bits(pcie->phys_breg_base),

• 	  E_BREG_BASE_LO);
  

• nwl_bridge_writel(pcie, upper_32_bits(pcie->phys_breg_base),

• 	  E_BREG_BASE_HI);
  

• /* Enable BREG */
• nwl_bridge_writel(pcie, ~BREG_ENABLE_FORCE & BREG_ENABLE,

• 	  E_BREG_CONTROL);
  

• /* Disable DMA channel registers */
• nwl_bridge_writel(pcie, nwl_bridge_readl(pcie, BRCFG_PCIE_RX0) |

• 	  CFG_DMA_REG_BAR, BRCFG_PCIE_RX0);
  

• /* Enable Ingress subtractive decode translation */
• nwl_bridge_writel(pcie, SET_ISUB_CONTROL, I_ISUB_CONTROL);
• /* Enable msg filtering details */
• nwl_bridge_writel(pcie, CFG_ENABLE_MSG_FILTER_MASK,

• 	  BRCFG_PCIE_RX_MSG_FILTER);
  

• err = nwl_wait_for_link(pcie);
• if (err)

• return err;
  

• ecam_val = nwl_bridge_readl(pcie, E_ECAM_CAPABILITIES) & E_ECAM_PRESENT;
• if (!ecam_val) {

• dev_err(dev, "ECAM is not present\n");
  

• return ecam_val;
  

• }
• /* Enable ECAM */
• nwl_bridge_writel(pcie, nwl_bridge_readl(pcie, E_ECAM_CONTROL) |

• 	  E_ECAM_CR_ENABLE, E_ECAM_CONTROL);
  

• nwl_bridge_writel(pcie, nwl_bridge_readl(pcie, E_ECAM_CONTROL) |

• 	  (pcie->ecam_value << E_ECAM_SIZE_SHIFT),
  

• 	  E_ECAM_CONTROL);
  

• nwl_bridge_writel(pcie, lower_32_bits(pcie->phys_ecam_base),

• 	  E_ECAM_BASE_LO);
  

• nwl_bridge_writel(pcie, upper_32_bits(pcie->phys_ecam_base),

• 	  E_ECAM_BASE_HI);
  

• /* Get bus range */
• ecam_val = nwl_bridge_readl(pcie, E_ECAM_CONTROL);
• pcie->last_busno = (ecam_val & E_ECAM_SIZE_LOC) >> E_ECAM_SIZE_SHIFT;
• /* Write primary, secondary and subordinate bus numbers */
• ecam_val = first_busno;
• ecam_val |= (first_busno + 1) << 8;
• ecam_val |= (pcie->last_busno << E_ECAM_SIZE_SHIFT);
• writel(ecam_val, (pcie->ecam_base + PCI_PRIMARY_BUS));

This writel() overwrites standard PCI Bridge registers at BDF 0:0.0, which should be PCIe Root Port. This should be really not needed for U-Boot (and neither for Linux kernel) because pci autoconf code correctly discover and configures all PCI Bridge devices.

If there is not bug in U-Boot pci code then this register is overwritten by U-Boot at later phase and therefore any change done here is lost.

Anyway, more readable expression for writing to BDF 0:0.0 at PCI_PRIMARY_BUS register could be:

writel(ecam_val, pcie->ecam_base + PCIE_ECAM_OFFSET(0, 0, 0, PCI_PRIMARY_BUS));

• if (nwl_pcie_link_up(pcie))

• dev_info(dev, "Link is UP\n");
  

• else

• dev_info(dev, "Link is DOWN\n");
  

• /* Disable all misc interrupts */
• nwl_bridge_writel(pcie, (u32)~MSGF_MISC_SR_MASKALL, MSGF_MISC_MASK);
• /* Clear pending misc interrupts */
• nwl_bridge_writel(pcie, nwl_bridge_readl(pcie, MSGF_MISC_STATUS) &

• 	  MSGF_MISC_SR_MASKALL, MSGF_MISC_STATUS);
  

• /* Disable all legacy interrupts */
• nwl_bridge_writel(pcie, (u32)~MSGF_LEG_SR_MASKALL, MSGF_LEG_MASK);
• /* Clear pending legacy interrupts */
• nwl_bridge_writel(pcie, nwl_bridge_readl(pcie, MSGF_LEG_STATUS) &

• 	  MSGF_LEG_SR_MASKALL, MSGF_LEG_STATUS);
  

• return 0;

+}

+static int nwl_pcie_parse_dt(struct nwl_pcie *pcie) +{

• struct udevice *dev = pcie->dev;
• struct resource res;
• int ret;
• ret = dev_read_resource_byname(dev, "breg", &res);
• if (ret)

• return ret;
  

• pcie->breg_base = devm_ioremap(dev, res.start, resource_size(&res));
• if (IS_ERR(pcie->breg_base))

• return PTR_ERR(pcie->breg_base);
  

• pcie->phys_breg_base = res.start;
• ret = dev_read_resource_byname(dev, "pcireg", &res);
• if (ret)

• return ret;
  

• pcie->pcireg_base = devm_ioremap(dev, res.start, resource_size(&res));
• if (IS_ERR(pcie->pcireg_base))

• return PTR_ERR(pcie->pcireg_base);
  

• pcie->phys_pcie_reg_base = res.start;
• ret = dev_read_resource_byname(dev, "cfg", &res);
• if (ret)

• return ret;
  

• pcie->ecam_base = devm_ioremap(dev, res.start, resource_size(&res));
• if (IS_ERR(pcie->ecam_base))

• return PTR_ERR(pcie->ecam_base);
  

• pcie->phys_ecam_base = res.start;
• return 0;

+}

+static int nwl_pcie_probe(struct udevice *dev) +{

• struct nwl_pcie *pcie = dev_get_priv(dev);
• int err;
• pcie->dev = dev;
• pcie->ecam_value = NWL_ECAM_VALUE_DEFAULT;
• err = nwl_pcie_parse_dt(pcie);
• if (err) {

• dev_err(dev, "Parsing DT failed\n");
  

• return err;
  

• }
• err = nwl_pcie_bridge_init(pcie);
• if (err) {

• dev_err(dev, "HW Initialization failed\n");
  

• return err;
  

• }
• return 0;

+}

+static const struct udevice_id nwl_pcie_of_match[] = {

• { .compatible = "xlnx,nwl-pcie-2.11", },
• { /* sentinel */ }

+};

+U_BOOT_DRIVER(nwl_pcie) = {

• .name = "nwl-pcie",
• .id = UCLASS_PCI,
• .of_match = nwl_pcie_of_match,
• .probe = nwl_pcie_probe,
• .priv_auto = sizeof(struct nwl_pcie),
• .ops = &nwl_pcie_ops,

+};

2.40.1