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

Pali Rohár pali at kernel.org
Tue May 23 19:17:23 CEST 2023


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 at denx.de>
> Cc: Simon Glass <sjg at chromium.org>
> Cc: Pali Rohár <pali at kernel.org>
> Cc: Bin Meng <bmeng.cn at gmail.com>
> Cc: Michal Simek <michal.simek at 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 at 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
> 


More information about the U-Boot mailing list