Subject: [PATCH v2 2/3] PCI: host: new PCI host controller driver for Aardvark

From: [email-masked] (Bjorn Helgaas)
Date: Wed, 22 Jun 2016 12:25:02 -0500

On Fri, Jun 10, 2016 at 05:54:15PM +0200, Thomas Petazzoni wrote:
> This commit adds a new driver for a PCIe controller called Aardvark,
> which is used on the Marvell Armada 3700 ARM64 SoC.
> 
> This patch is based on work done by Hezi Shahmoon
> <[email-masked]> and Marcin Wojtas <[email-masked]>.
> 
> Signed-off-by: Thomas Petazzoni <[email-masked]>
> ---
>  MAINTAINERS                     |    7 +
>  drivers/pci/host/Kconfig        |    9 +
>  drivers/pci/host/Makefile       |    1 +
>  drivers/pci/host/pci-aardvark.c | 1023 +++++++++++++++++++++++++++++++++++++++
>  4 files changed, 1040 insertions(+)
>  create mode 100644 drivers/pci/host/pci-aardvark.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 7304d2e..373215c 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -8740,6 +8740,13 @@ L:	linux-arm-kernel at lists.infradead.org (moderated for non-subscribers)
>  S:	Maintained
>  F:	drivers/pci/host/*mvebu*
>  
> +PCI DRIVER FOR AARDVARK (Marvell Armada 3700)
> +M:	Thomas Petazzoni <[email-masked]>
> +L:	linux-pci at vger.kernel.org
> +L:	linux-arm-kernel at lists.infradead.org (moderated for non-subscribers)
> +S:	Maintained
> +F:	drivers/pci/host/pci-aardvark.c
> +
>  PCI DRIVER FOR NVIDIA TEGRA
>  M:	Thierry Reding <[email-masked]>
>  L:	linux-tegra at vger.kernel.org
> diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
> index 5d2374e..5f085fd 100644
> --- a/drivers/pci/host/Kconfig
> +++ b/drivers/pci/host/Kconfig
> @@ -16,6 +16,15 @@ config PCI_MVEBU
>  	depends on ARM
>  	depends on OF
>  
> +config PCI_AARDVARK
> +	bool "Aardvark PCIe controller"
> +	depends on ARCH_MVEBU && ARM64
> +	depends on OF
> +	select PCI_MSI

I'm guessing this should be "depends on PCI_MSI_IRQ_DOMAIN" per Arnd's
recent patch: http://lkml.kernel.org/r/8531046.3ceRqUA835 at wuerfel

I have merged Arnd's patch, so it will be in v4.8.

> +	help
> +	 Add support for Aardvark 64bit PCIe Host Controller. This
> +	 controller is part of the South Bridge of the Marvel Armada
> +	 3700 SoC.
>  
>  config PCIE_XILINX_NWL
>  	bool "NWL PCIe Core"
> diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
> index 9c8698e..66f45b6 100644
> --- a/drivers/pci/host/Makefile
> +++ b/drivers/pci/host/Makefile
> @@ -5,6 +5,7 @@ obj-$(CONFIG_PCI_EXYNOS) += pci-exynos.o
>  obj-$(CONFIG_PCI_IMX6) += pci-imx6.o
>  obj-$(CONFIG_PCI_HYPERV) += pci-hyperv.o
>  obj-$(CONFIG_PCI_MVEBU) += pci-mvebu.o
> +obj-$(CONFIG_PCI_AARDVARK) += pci-aardvark.o
>  obj-$(CONFIG_PCI_TEGRA) += pci-tegra.o
>  obj-$(CONFIG_PCI_RCAR_GEN2) += pci-rcar-gen2.o
>  obj-$(CONFIG_PCIE_RCAR) += pcie-rcar.o
> diff --git a/drivers/pci/host/pci-aardvark.c b/drivers/pci/host/pci-aardvark.c
> new file mode 100644
> index 0000000..15d66a7
> --- /dev/null
> +++ b/drivers/pci/host/pci-aardvark.c
> @@ -0,0 +1,1023 @@
> +/*
> + * Driver for the Aardvark PCIe controller, used on Marvell Armada
> + * 3700.
> + *
> + * Copyright (C) 2016 Marvell
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2.  This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/kernel.h>
> +#include <linux/pci.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/of_address.h>
> +#include <linux/of_pci.h>
> +
> +/* PCIe core registers */
> +#define PCIE_CORE_CMD_STATUS_REG				0x4
> +#define     PCIE_CORE_CMD_IO_ACCESS_EN				BIT(0)
> +#define     PCIE_CORE_CMD_MEM_ACCESS_EN				BIT(1)
> +#define     PCIE_CORE_CMD_MEM_IO_REQ_EN				BIT(2)
> +#define PCIE_CORE_DEV_CTRL_STATS_REG				0xC8

Please use either upper- or lower-case in hex constants consistently.
Most of the ones below are lower-case.

> +#define     PCIE_CORE_DEV_CTRL_STATS_RELAX_ORDER_DISABLE	(0 << 4)
> +#define     PCIE_CORE_DEV_CTRL_STATS_MAX_PAYLOAD_SZ_SHIFT	5
> +#define     PCIE_CORE_DEV_CTRL_STATS_SNOOP_DISABLE		(0 << 11)
> +#define     PCIE_CORE_DEV_CTRL_STATS_MAX_RD_REQ_SIZE_SHIFT	12
> +#define PCIE_CORE_LINK_CTRL_STAT_REG				0xD0
> +#define     PCIE_CORE_LINK_L0S_ENTRY				BIT(0)
> +#define     PCIE_CORE_LINK_TRAINING				BIT(5)
> +#define     PCIE_CORE_LINK_WIDTH_SHIFT				20
> +#define PCIE_CORE_ERR_CAPCTL_REG				0x118
> +#define     PCIE_CORE_ERR_CAPCTL_ECRC_CHK_TX			BIT(5)
> +#define     PCIE_CORE_ERR_CAPCTL_ECRC_CHK_TX_EN			BIT(6)
> +#define     PCIE_CORE_ERR_CAPCTL_ECRC_CHCK			BIT(7)
> +#define     PCIE_CORE_ERR_CAPCTL_ECRC_CHCK_RCV			BIT(8)
> +
> +/* PIO registers base address and register offsets */
> +#define PIO_BASE_ADDR				0x4000
> +#define PIO_CTRL				(PIO_BASE_ADDR + 0x0)
> +#define PIO_STAT				(PIO_BASE_ADDR + 0x4)
> +#define   PIO_COMPLETION_STATUS_SHIFT		7
> +#define   PIO_COMPLETION_STATUS_MASK		GENMASK(9, 7)
> +#define   PIO_COMPLETION_STATUS_OK		0
> +#define   PIO_COMPLETION_STATUS_UR		1
> +#define   PIO_COMPLETION_STATUS_CRS		2
> +#define   PIO_COMPLETION_STATUS_CA		4
> +#define   PIO_NON_POSTED_REQ			BIT(0)
> +#define PIO_ADDR_LS				(PIO_BASE_ADDR + 0x8)
> +#define PIO_ADDR_MS				(PIO_BASE_ADDR + 0xc)
> +#define PIO_WR_DATA				(PIO_BASE_ADDR + 0x10)
> +#define PIO_WR_DATA_STRB			(PIO_BASE_ADDR + 0x14)
> +#define PIO_RD_DATA				(PIO_BASE_ADDR + 0x18)
> +#define PIO_START				(PIO_BASE_ADDR + 0x1c)
> +#define PIO_ISR					(PIO_BASE_ADDR + 0x20)
> +#define PIO_ISRM				(PIO_BASE_ADDR + 0x24)
> +
> +/* Aardvark Control registers */
> +#define CONTROL_BASE_ADDR			0x4800
> +#define PCIE_CORE_CTRL0_REG			(CONTROL_BASE_ADDR + 0x0)
> +#define     PCIE_GEN_SEL_MSK			0x3
> +#define     PCIE_GEN_SEL_SHIFT			0x0
> +#define     SPEED_GEN_1				0
> +#define     SPEED_GEN_2				1
> +#define     SPEED_GEN_3				2
> +#define     IS_RC_MSK				1
> +#define     IS_RC_SHIFT				2
> +#define     LANE_CNT_MSK			0x18
> +#define     LANE_CNT_SHIFT			0x3
> +#define     LANE_COUNT_1			(0 << LANE_CNT_SHIFT)
> +#define     LANE_COUNT_2			(1 << LANE_CNT_SHIFT)
> +#define     LANE_COUNT_4			(2 << LANE_CNT_SHIFT)
> +#define     LANE_COUNT_8			(3 << LANE_CNT_SHIFT)
> +#define     LINK_TRAINING_EN			BIT(6)
> +#define     LEGACY_INTA				BIT(28)
> +#define     LEGACY_INTB				BIT(29)
> +#define     LEGACY_INTC				BIT(30)
> +#define     LEGACY_INTD				BIT(31)
> +#define PCIE_CORE_CTRL1_REG			(CONTROL_BASE_ADDR + 0x4)
> +#define     HOT_RESET_GEN			BIT(0)
> +#define PCIE_CORE_CTRL2_REG			(CONTROL_BASE_ADDR + 0x8)
> +#define     PCIE_CORE_CTRL2_RESERVED		0x7
> +#define     PCIE_CORE_CTRL2_TD_ENABLE		BIT(4)
> +#define     PCIE_CORE_CTRL2_STRICT_ORDER_ENABLE	BIT(5)
> +#define     PCIE_CORE_CTRL2_OB_WIN_ENABLE	BIT(6)
> +#define     PCIE_CORE_CTRL2_MSI_ENABLE		BIT(10)
> +#define PCIE_ISR0_REG				(CONTROL_BASE_ADDR + 0x40)
> +#define PCIE_ISR0_MASK_REG			(CONTROL_BASE_ADDR + 0x44)
> +#define     PCIE_ISR0_FLR_INT			BIT(26)
> +#define     PCIE_ISR0_MSG_LTR			BIT(25)
> +#define     PCIE_ISR0_MSI_INT_PENDING		BIT(24)
> +#define     PCIE_ISR0_INTD_DEASSERT		BIT(23)
> +#define     PCIE_ISR0_INTC_DEASSERT		BIT(22)
> +#define     PCIE_ISR0_INTB_DEASSERT		BIT(21)
> +#define     PCIE_ISR0_INTA_DEASSERT		BIT(20)
> +#define     PCIE_ISR0_INTD_ASSERT		BIT(19)
> +#define     PCIE_ISR0_INTC_ASSERT		BIT(18)
> +#define     PCIE_ISR0_INTB_ASSERT		BIT(17)
> +#define     PCIE_ISR0_INTA_ASSERT		BIT(16)
> +#define     PCIE_ISR0_FAT_ERR			BIT(13)
> +#define     PCIE_ISR0_NFAT_ERR			BIT(12)
> +#define     PCIE_ISR0_CORR_ERR			BIT(11)
> +#define     PCIE_ISR0_LMI_LOCAL_INT		BIT(10)
> +#define     PCIE_ISR0_LEGACY_INT_SENT		BIT(9)
> +#define     PCIE_ISR0_MSG_PM_ACTIVE_STATE_NAK	BIT(8)
> +#define     PCIE_ISR0_MSG_PM_PME		BIT(7)
> +#define     PCIE_ISR0_MSG_PM_TURN_OFF		BIT(6)
> +#define     PCIE_ISR0_MSG_PME_TO_ACK		BIT(5)
> +#define     PCIE_ISR0_INB_DP_FERR_PERR_IRQ	BIT(4)
> +#define     PCIE_ISR0_OUTB_DP_FERR_PERR_IRQ	BIT(3)
> +#define     PCIE_ISR0_INBOUND_MSG		BIT(2)
> +#define     PCIE_ISR0_LINK_DOWN			BIT(1)
> +#define     PCIE_ISR0_HOT_RESET			BIT(0)
> +#define     PCIE_ISR0_INTX_ASSERT(val)		BIT(16 + (val))
> +#define     PCIE_ISR0_INTX_DEASSERT(val)	BIT(20 + (val))
> +#define	    PCIE_ISR0_ALL_MASK			GENMASK(26, 0)
> +#define PCIE_ISR1_REG				(CONTROL_BASE_ADDR + 0x48)
> +#define PCIE_ISR1_MASK_REG			(CONTROL_BASE_ADDR + 0x4C)
> +#define     PCIE_ISR1_POWER_STATE_CHANGE	BIT(4)
> +#define     PCIE_ISR1_FLUSH			BIT(5)
> +#define     PCIE_ISR1_ALL_MASK			GENMASK(5, 4)
> +#define PCIE_MSI_ADDR_LOW_REG			(CONTROL_BASE_ADDR + 0x50)
> +#define PCIE_MSI_ADDR_HIGH_REG			(CONTROL_BASE_ADDR + 0x54)
> +#define PCIE_MSI_STATUS_REG			(CONTROL_BASE_ADDR + 0x58)
> +#define PCIE_MSI_MASK_REG			(CONTROL_BASE_ADDR + 0x5C)
> +#define PCIE_MSI_PAYLOAD_REG			(CONTROL_BASE_ADDR + 0x9C)
> +
> +/* PCIe window configuration */
> +#define OB_WIN_BASE_ADDR			0x4c00
> +#define OB_WIN_BLOCK_SIZE			0x20
> +#define OB_WIN_REG_ADDR(win, offset)		(OB_WIN_BASE_ADDR + \
> +						 OB_WIN_BLOCK_SIZE * (win) + \
> +						 (offset))
> +#define OB_WIN_MATCH_LS(win)			OB_WIN_REG_ADDR(win, 0x00)
> +#define OB_WIN_MATCH_MS(win)			OB_WIN_REG_ADDR(win, 0x04)
> +#define OB_WIN_REMAP_LS(win)			OB_WIN_REG_ADDR(win, 0x08)
> +#define OB_WIN_REMAP_MS(win)			OB_WIN_REG_ADDR(win, 0x0c)
> +#define OB_WIN_MASK_LS(win)			OB_WIN_REG_ADDR(win, 0x10)
> +#define OB_WIN_MASK_MS(win)			OB_WIN_REG_ADDR(win, 0x14)
> +#define OB_WIN_ACTIONS(win)			OB_WIN_REG_ADDR(win, 0x18)
> +
> +/* PCIe window types */
> +#define     OB_PCIE_MEM				0x0
> +#define     OB_PCIE_IO				0x4

> +#define     OB_PCIE_CONFIG0			0x8
> +#define     OB_PCIE_CONFIG1			0x9
> +#define     OB_PCIE_MSG				0xc
> +#define     OB_PCIE_MSG_VENDOR			0xd

Some of these definitions (ISR bits above, these window types,
probably others) are not actually used.  I usually omit unused ones on
the theory that they add bulk and opportunities for transcription
errors.  But if they're useful to you, I'm OK with keeping them.

> +/* LMI registers base address and register offsets */
> +#define LMI_BASE_ADDR				0x6000
> +#define CFG_REG					(LMI_BASE_ADDR + 0x0)
> +#define     LTSSM_SHIFT				24
> +#define     LTSSM_MASK				0x3f
> +#define     LTSSM_L0				0x10
> +#define     RC_BAR_CONFIG			0x300
> +
> +/* PCIe core controller registers */
> +#define CTRL_CORE_BASE_ADDR			0x18000
> +#define CTRL_CONFIG_REG				(CTRL_CORE_BASE_ADDR + 0x0)
> +#define     CTRL_MODE_SHIFT			0x0
> +#define     CTRL_MODE_MASK			0x1
> +#define     PCIE_CORE_MODE_DIRECT		0x0
> +#define     PCIE_CORE_MODE_COMMAND		0x1
> +
> +/* PCIe Central Interrupts Registers */
> +#define CENTRAL_INT_BASE_ADDR			0x1b000
> +#define HOST_CTRL_INT_STATUS_REG		(CENTRAL_INT_BASE_ADDR + 0x0)
> +#define HOST_CTRL_INT_MASK_REG			(CENTRAL_INT_BASE_ADDR + 0x4)
> +#define     PCIE_IRQ_CMDQ_INT			BIT(0)
> +#define     PCIE_IRQ_MSI_STATUS_INT		BIT(1)
> +#define     PCIE_IRQ_CMD_SENT_DONE		BIT(3)
> +#define     PCIE_IRQ_DMA_INT			BIT(4)
> +#define     PCIE_IRQ_IB_DXFERDONE		BIT(5)
> +#define     PCIE_IRQ_OB_DXFERDONE		BIT(6)
> +#define     PCIE_IRQ_OB_RXFERDONE		BIT(7)
> +#define     PCIE_IRQ_COMPQ_INT			BIT(12)
> +#define     PCIE_IRQ_DIR_RD_DDR_DET		BIT(13)
> +#define     PCIE_IRQ_DIR_WR_DDR_DET		BIT(14)
> +#define     PCIE_IRQ_CORE_INT			BIT(16)
> +#define     PCIE_IRQ_CORE_INT_PIO		BIT(17)
> +#define     PCIE_IRQ_DPMU_INT			BIT(18)
> +#define     PCIE_IRQ_PCIE_MIS_INT		BIT(19)
> +#define     PCIE_IRQ_MSI_INT1_DET		BIT(20)
> +#define     PCIE_IRQ_MSI_INT2_DET		BIT(21)
> +#define     PCIE_IRQ_RC_DBELL_DET		BIT(22)
> +#define     PCIE_IRQ_EP_STATUS			BIT(23)
> +#define     PCIE_IRQ_ALL_MASK			0xfff0fb
> +#define     PCIE_IRQ_ENABLE_INTS_MASK		PCIE_IRQ_CORE_INT
> +
> +/* Transaction types */
> +#define PCIE_CONFIG_RD_TYPE0			0x8
> +#define PCIE_CONFIG_RD_TYPE1			0x9
> +#define PCIE_CONFIG_WR_TYPE0			0xa
> +#define PCIE_CONFIG_WR_TYPE1			0xb
> +
> +/* PCI_BDF shifts 8bit, so we need extra 4bit shift */
> +#define PCIE_BDF(dev)				(dev << 4)
> +#define PCIE_CONF_BUS(bus)			(((bus) & 0xff) << 20)
> +#define PCIE_CONF_DEV(dev)			(((dev) & 0x1f) << 15)
> +#define PCIE_CONF_FUNC(fun)			(((fun) & 0x7)	<< 12)
> +#define PCIE_CONF_REG(reg)			((reg) & 0xffc)
> +#define PCIE_CONF_ADDR(bus, devfn, where)	\
> +	(PCIE_CONF_BUS(bus) | PCIE_CONF_DEV(PCI_SLOT(devfn))	| \
> +	 PCIE_CONF_FUNC(PCI_FUNC(devfn)) | PCIE_CONF_REG(where))
> +
> +#define PIO_TIMEOUT_MS			(1)
> +#define PCIE_LINKUP_TIMEOUT_MS		(10)

Bare numbers do not require parentheses.

> +#define LEGACY_IRQ_NUM			4
> +#define MSI_IRQ_NUM			32
> +
> +struct advk_pcie {
> +	struct platform_device *pdev;
> +	void __iomem *base;
> +	struct list_head resources;
> +	struct irq_domain *irq_domain;
> +	struct irq_chip irq_chip;
> +	struct msi_controller msi;
> +	struct irq_domain *msi_domain;
> +	struct irq_chip msi_irq_chip;
> +	DECLARE_BITMAP(msi_irq_in_use, MSI_IRQ_NUM);
> +	struct mutex msi_used_lock;
> +	u16 msi_msg;
> +};
> +
> +static inline void advk_writel(struct advk_pcie *pcie, u32 val, u64 reg)
> +{
> +	writel(val, pcie->base + reg);
> +}
> +
> +static inline u32 advk_readl(struct advk_pcie *pcie, u64 reg)
> +{
> +	return readl(pcie->base + reg);
> +}
> +
> +static bool advk_pcie_link_up(struct advk_pcie *pcie)
> +{
> +	unsigned long timeout;
> +	u32 ltssm_state;
> +	u32 val;
> +
> +	timeout = jiffies + msecs_to_jiffies(PCIE_LINKUP_TIMEOUT_MS);
> +
> +	while (time_before(jiffies, timeout)) {
> +		val = advk_readl(pcie, CFG_REG);
> +		ltssm_state = (val >> LTSSM_SHIFT) & LTSSM_MASK;
> +		if (ltssm_state >= LTSSM_L0)
> +			return true;
> +	}

To try to improve consistency with other similar drivers, please make
advk_pcie_link_up() a simple function that contains one register read
and no loop.

Then make a second advk_wait_for_link() function with a loop and
timeout.  Please use the same timeout/usleep structure used in
dw_pcie_wait_for_link() and nwl_wait_for_link().

> +	return false;
> +}
> +
> +/*
> + * Set PCIe address window register which could be used for memory
> + * mapping.
> + */
> +static void advk_pcie_set_ob_win(struct advk_pcie *pcie,
> +				 u32 win_num, u32 match_ms,
> +				 u32 match_ls, u32 mask_ms,
> +				 u32 mask_ls, u32 remap_ms,
> +				 u32 remap_ls, u32 action)
> +{
> +	advk_writel(pcie, match_ls, OB_WIN_MATCH_LS(win_num));
> +	advk_writel(pcie, match_ms, OB_WIN_MATCH_MS(win_num));
> +	advk_writel(pcie, mask_ms, OB_WIN_MASK_MS(win_num));
> +	advk_writel(pcie, mask_ls, OB_WIN_MASK_LS(win_num));
> +	advk_writel(pcie, remap_ms, OB_WIN_REMAP_MS(win_num));
> +	advk_writel(pcie, remap_ls, OB_WIN_REMAP_LS(win_num));
> +	advk_writel(pcie, action, OB_WIN_ACTIONS(win_num));
> +	advk_writel(pcie, match_ls | BIT(0), OB_WIN_MATCH_LS(win_num));
> +}
> +
> +static void advk_pcie_setup_hw(struct advk_pcie *pcie)
> +{
> +	u32 reg;
> +	int i;
> +
> +	/* Point PCIe unit MBUS decode windows to DRAM space. */

Tidy up the comments here by making them all one-line (when they fit) 
and consistently omitting (or keeping) the periods.

> +	for (i = 0; i < 8; i++)
> +		advk_pcie_set_ob_win(pcie, i, 0, 0, 0, 0, 0, 0, 0);
> +
> +	/* Set to Direct mode. */
> +	reg = advk_readl(pcie, CTRL_CONFIG_REG);
> +	reg &= ~(CTRL_MODE_MASK << CTRL_MODE_SHIFT);
> +	reg |= ((PCIE_CORE_MODE_DIRECT & CTRL_MODE_MASK) << CTRL_MODE_SHIFT);
> +	advk_writel(pcie, reg, CTRL_CONFIG_REG);
> +
> +	/* Set PCI global control register to RC mode */
> +	reg = advk_readl(pcie, PCIE_CORE_CTRL0_REG);
> +	reg |= (IS_RC_MSK << IS_RC_SHIFT);
> +	advk_writel(pcie, reg, PCIE_CORE_CTRL0_REG);
> +
> +	/*
> +	 * Set Advanced Error Capabilities and Control PF0 register
> +	 */
> +	reg = PCIE_CORE_ERR_CAPCTL_ECRC_CHK_TX |
> +		PCIE_CORE_ERR_CAPCTL_ECRC_CHK_TX_EN |
> +		PCIE_CORE_ERR_CAPCTL_ECRC_CHCK |
> +		PCIE_CORE_ERR_CAPCTL_ECRC_CHCK_RCV;
> +	advk_writel(pcie, reg, PCIE_CORE_ERR_CAPCTL_REG);
> +
> +	/*
> +	 * Set PCIe Device Control and Status 1 PF0 register
> +	 */
> +	reg = PCIE_CORE_DEV_CTRL_STATS_RELAX_ORDER_DISABLE |
> +		(7 << PCIE_CORE_DEV_CTRL_STATS_MAX_PAYLOAD_SZ_SHIFT) |
> +		PCIE_CORE_DEV_CTRL_STATS_SNOOP_DISABLE |
> +		PCIE_CORE_DEV_CTRL_STATS_MAX_RD_REQ_SIZE_SHIFT;
> +	advk_writel(pcie, reg, PCIE_CORE_DEV_CTRL_STATS_REG);
> +
> +	/*
> +	 * Program PCIe Control 2 to disable strict ordering.
> +	 */
> +	reg = PCIE_CORE_CTRL2_RESERVED |
> +		PCIE_CORE_CTRL2_TD_ENABLE;
> +	advk_writel(pcie, reg, PCIE_CORE_CTRL2_REG);
> +
> +	/* Set GEN2 */
> +	reg = advk_readl(pcie, PCIE_CORE_CTRL0_REG);
> +	reg &= ~PCIE_GEN_SEL_MSK;
> +	reg |= SPEED_GEN_2;
> +	advk_writel(pcie, reg, PCIE_CORE_CTRL0_REG);
> +
> +	/* Set lane X1 */
> +	reg = advk_readl(pcie, PCIE_CORE_CTRL0_REG);
> +	reg &= ~LANE_CNT_MSK;
> +	reg |= LANE_COUNT_1;
> +	advk_writel(pcie, reg, PCIE_CORE_CTRL0_REG);
> +
> +	/* Enable link training */
> +	reg = advk_readl(pcie, PCIE_CORE_CTRL0_REG);
> +	reg |= LINK_TRAINING_EN;
> +	advk_writel(pcie, reg, PCIE_CORE_CTRL0_REG);
> +
> +	/* Enable MSI */
> +	reg = advk_readl(pcie, PCIE_CORE_CTRL2_REG);
> +	reg |= PCIE_CORE_CTRL2_MSI_ENABLE;
> +	advk_writel(pcie, reg, PCIE_CORE_CTRL2_REG);
> +
> +	/* Clear all interrupts. */
> +	advk_writel(pcie, PCIE_ISR0_ALL_MASK, PCIE_ISR0_REG);
> +	advk_writel(pcie, PCIE_ISR1_ALL_MASK, PCIE_ISR1_REG);
> +	advk_writel(pcie, PCIE_IRQ_ALL_MASK, HOST_CTRL_INT_STATUS_REG);
> +
> +	/* Disable All ISR0/1 Sources */
> +	reg = PCIE_ISR0_ALL_MASK;
> +	reg &= ~PCIE_ISR0_MSI_INT_PENDING;
> +	advk_writel(pcie, reg, PCIE_ISR0_MASK_REG);
> +
> +	advk_writel(pcie, PCIE_ISR1_ALL_MASK, PCIE_ISR1_MASK_REG);
> +
> +	/* Unmask all MSI's */
> +	advk_writel(pcie, 0, PCIE_MSI_MASK_REG);
> +
> +	/* Enable summary interrupt for GIC SPI source */
> +	reg = PCIE_IRQ_ALL_MASK & (~PCIE_IRQ_ENABLE_INTS_MASK);
> +	advk_writel(pcie, reg, HOST_CTRL_INT_MASK_REG);
> +
> +	/* Start link training */
> +	reg = advk_readl(pcie, PCIE_CORE_LINK_CTRL_STAT_REG);
> +	reg |= PCIE_CORE_LINK_TRAINING;
> +	advk_writel(pcie, reg, PCIE_CORE_LINK_CTRL_STAT_REG);
> +
> +	advk_pcie_link_up(pcie);
> +
> +	reg = PCIE_CORE_LINK_L0S_ENTRY |
> +		(1 << PCIE_CORE_LINK_WIDTH_SHIFT);
> +	advk_writel(pcie, reg, PCIE_CORE_LINK_CTRL_STAT_REG);
> +
> +	reg = advk_readl(pcie, PCIE_CORE_CMD_STATUS_REG);
> +	reg |= PCIE_CORE_CMD_MEM_ACCESS_EN |
> +		PCIE_CORE_CMD_IO_ACCESS_EN |
> +		PCIE_CORE_CMD_MEM_IO_REQ_EN;
> +	advk_writel(pcie, reg, PCIE_CORE_CMD_STATUS_REG);
> +}
> +
> +/*
> + * This routine is used to enable or disable AXI address window
> + * location generation.
> + * Disabled: No address window mapping. Use AXI user fields
> + * provided by the AXI fabric.
> + * Enabled: Enable the address window mapping. The HAL bridge
> + * generates the AXI user field locally. Use the local generated AXI user
> + * fields.
> + * It should be disabled when accessing the PCIe device by PIO.
> + * It should be enabled when accessing the PCIe device by memory
> + * access directly.
> + */
> +static void advk_pcie_enable_axi_addr_gen(struct advk_pcie *pcie, bool enable)
> +{
> +	u32 reg;
> +
> +	reg = advk_readl(pcie, PCIE_CORE_CTRL2_REG);
> +	if (enable)
> +		reg |= PCIE_CORE_CTRL2_OB_WIN_ENABLE;
> +	else
> +		reg &= ~PCIE_CORE_CTRL2_OB_WIN_ENABLE;
> +	advk_writel(pcie, reg, PCIE_CORE_CTRL2_REG);
> +}
> +
> +static void advk_pcie_check_pio_status(struct advk_pcie *pcie)
> +{
> +	u32 reg;
> +	unsigned int status;
> +	char *strcomp_status, *str_posted;
> +
> +	reg = advk_readl(pcie, PIO_STAT);
> +	status = (reg & PIO_COMPLETION_STATUS_MASK) >>
> +		PIO_COMPLETION_STATUS_SHIFT;
> +
> +	if (!status)
> +		return;
> +
> +	switch (status) {
> +	case PIO_COMPLETION_STATUS_UR:
> +		strcomp_status = "UR";
> +		break;
> +	case PIO_COMPLETION_STATUS_CRS:
> +		strcomp_status = "CRS";
> +		break;
> +	case PIO_COMPLETION_STATUS_CA:
> +		strcomp_status = "CA";
> +		break;
> +	default:
> +		strcomp_status = "Unknown";
> +		break;
> +	}
> +
> +	if (reg & PIO_NON_POSTED_REQ)
> +		str_posted = "Non-posted";
> +	else
> +		str_posted = "Posted";
> +
> +	dev_err(&pcie->pdev->dev,
> +		"%s PIO Response Status: %s, %#x @ %#x\n",
> +		str_posted, strcomp_status, reg,
> +		advk_readl(pcie, PIO_ADDR_LS));
> +}
> +
> +static int advk_pcie_wait_pio(struct advk_pcie *pcie)
> +{
> +	unsigned long timeout;
> +	u32 reg, is_done;
> +
> +	timeout = jiffies + msecs_to_jiffies(PIO_TIMEOUT_MS);
> +
> +	while (time_before(jiffies, timeout)) {
> +		reg = advk_readl(pcie, PIO_START);
> +		is_done = advk_readl(pcie, PIO_ISR);
> +		if ((!reg) && is_done)
> +			return 0;
> +	}

This busy-loop could use usleep_range() similar to what
dw_pcie_wait_for_link() does.

> +	dev_err(&pcie->pdev->dev, "config read/write timed out\n");
> +	return -ETIME;
> +}
> +
> +static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
> +			     int where, int size, u32 *val)
> +{
> +	struct advk_pcie *pcie = bus->sysdata;
> +	u32 reg;
> +	int ret;
> +
> +	if (PCI_SLOT(devfn) != 0) {
> +		*val = 0xffffffff;
> +		return PCIBIOS_DEVICE_NOT_FOUND;
> +	}
> +
> +	advk_pcie_enable_axi_addr_gen(pcie, false);

This AXI enable/disable requires a mutex to prevent another thread
from performing a simulataneous config access, so please add a comment
about which mutex you are relying on.  I think there's a PCI global
one, but I'd just like to make it explicit that we need it here,
because many config accessors don't actually require the mutex.

I assume the AXI enable/disable does not affect MMIO accesses by
drivers to areas mapped by PCIe device BARs.  If it did, that would be
a pretty serious problem because it would be hard to ensure the mutual
exclusion.

> +	/* Start PIO */
> +	advk_writel(pcie, 0, PIO_START);
> +	advk_writel(pcie, 1, PIO_ISR);
> +
> +	/* Program the control register */
> +	if (bus->number ==  0)
> +		advk_writel(pcie, PCIE_CONFIG_RD_TYPE0, PIO_CTRL);

Your DT documentation requires the bus range, and
of_pci_get_host_bridge_resources() parses it, so you probably should
save that bus range from the DT in advk_pcie and use it here instead
of assuming zero.

> +	else
> +		advk_writel(pcie, PCIE_CONFIG_RD_TYPE1, PIO_CTRL);
> +
> +	/* Program the address registers */
> +	reg = PCIE_BDF(devfn) | PCIE_CONF_REG(where);
> +	advk_writel(pcie, reg, PIO_ADDR_LS);
> +	advk_writel(pcie, 0, PIO_ADDR_MS);
> +
> +	/* Program the data strobe */
> +	advk_writel(pcie, 0xf, PIO_WR_DATA_STRB);
> +
> +	/* Start the transfer */
> +	advk_writel(pcie, 1, PIO_START);
> +
> +	ret = advk_pcie_wait_pio(pcie);
> +	if (ret < 0) {
> +		advk_pcie_enable_axi_addr_gen(pcie, true);
> +		return PCIBIOS_SET_FAILED;
> +	}
> +
> +	advk_pcie_check_pio_status(pcie);
> +
> +	/* Get the read result */
> +	*val = advk_readl(pcie, PIO_RD_DATA);
> +	if (size == 1)
> +		*val = (*val >> (8 * (where & 3))) & 0xff;
> +	else if (size == 2)
> +		*val = (*val >> (8 * (where & 3))) & 0xffff;
> +
> +	advk_pcie_enable_axi_addr_gen(pcie, true);
> +
> +	return PCIBIOS_SUCCESSFUL;
> +}
> +
> +static int advk_pcie_wr_conf(struct pci_bus *bus, u32 devfn,
> +				int where, int size, u32 val)
> +{
> +	struct advk_pcie *pcie = bus->sysdata;
> +	u32 reg;
> +	u32 data_strobe = 0x0;
> +	int offset;
> +	int ret;
> +
> +	if (PCI_SLOT(devfn) != 0)
> +		return PCIBIOS_DEVICE_NOT_FOUND;
> +
> +	advk_pcie_enable_axi_addr_gen(pcie, false);
> +
> +	/* Start PIO */
> +	advk_writel(pcie, 0, PIO_START);
> +	advk_writel(pcie, 1, PIO_ISR);
> +
> +	if (bus->number == 0)
> +		advk_writel(pcie, PCIE_CONFIG_WR_TYPE0, PIO_CTRL);
> +	else
> +		advk_writel(pcie, PCIE_CONFIG_WR_TYPE1, PIO_CTRL);
> +
> +	/* Program the address registers */
> +	reg = PCIE_CONF_ADDR(bus->number, devfn, where);
> +	advk_writel(pcie, reg, PIO_ADDR_LS);
> +	advk_writel(pcie, 0, PIO_ADDR_MS);
> +
> +	if (where % size) {
> +		advk_pcie_enable_axi_addr_gen(pcie, true);
> +		return PCIBIOS_SET_FAILED;

This could be checked earlier, before fiddling with AXI and touching
PIO_START.

> +	}
> +
> +	/* Calculate the write strobe */
> +	offset      = where & 0x3;
> +	reg         = val << (8 * offset);
> +	data_strobe = GENMASK(size - 1, 0) << offset;
> +
> +	/* Program the data register */
> +	advk_writel(pcie, reg, PIO_WR_DATA);
> +
> +	/* Program the data strobe */
> +	advk_writel(pcie, data_strobe, PIO_WR_DATA_STRB);
> +
> +	/* Start the transfer */
> +	advk_writel(pcie, 1, PIO_START);
> +
> +	ret = advk_pcie_wait_pio(pcie);
> +	if (ret < 0) {
> +		advk_pcie_enable_axi_addr_gen(pcie, true);
> +		return PCIBIOS_SET_FAILED;
> +	}
> +
> +	advk_pcie_check_pio_status(pcie);
> +
> +	advk_pcie_enable_axi_addr_gen(pcie, true);
> +
> +	return PCIBIOS_SUCCESSFUL;
> +}
> +
> +static struct pci_ops advk_pcie_ops = {
> +	.read = advk_pcie_rd_conf,
> +	.write = advk_pcie_wr_conf,
> +};
> +
> +static int advk_pcie_alloc_msi(struct advk_pcie *pcie)
> +{
> +	int hwirq;
> +
> +	mutex_lock(&pcie->msi_used_lock);
> +	hwirq = find_first_zero_bit(pcie->msi_irq_in_use, MSI_IRQ_NUM);
> +	if (hwirq >= MSI_IRQ_NUM)
> +		hwirq = -ENOSPC;
> +	else
> +		set_bit(hwirq, pcie->msi_irq_in_use);
> +	mutex_unlock(&pcie->msi_used_lock);
> +
> +	return hwirq;
> +}
> +
> +static void advk_pcie_free_msi(struct advk_pcie *pcie, int hwirq)
> +{
> +	mutex_lock(&pcie->msi_used_lock);
> +	if (!test_bit(hwirq, pcie->msi_irq_in_use))
> +		pr_err("trying to free unused MSI#%d\n", hwirq);

Use dev_err().

> +	else
> +		clear_bit(hwirq, pcie->msi_irq_in_use);
> +	mutex_unlock(&pcie->msi_used_lock);
> +}
> +
> +static int advk_pcie_setup_msi_irq(struct msi_controller *chip,
> +				   struct pci_dev *pdev,
> +				   struct msi_desc *desc)
> +{
> +	struct advk_pcie *pcie = pdev->bus->sysdata;
> +	struct msi_msg msg;
> +	int virq, hwirq;
> +	phys_addr_t msi_msg_phys;
> +
> +	/* We support MSI, but not MSI-X */
> +	if (desc->msi_attrib.is_msix)
> +		return -EINVAL;
> +
> +	hwirq = advk_pcie_alloc_msi(pcie);
> +	if (hwirq < 0)
> +		return hwirq;
> +
> +	virq = irq_create_mapping(pcie->msi_domain, hwirq);
> +	if (!virq) {
> +		advk_pcie_free_msi(pcie, hwirq);
> +		return -EINVAL;
> +	}
> +
> +	irq_set_msi_desc(virq, desc);
> +
> +	msi_msg_phys = virt_to_phys(&pcie->msi_msg);
> +
> +	msg.address_lo = lower_32_bits(msi_msg_phys);
> +	msg.address_hi = upper_32_bits(msi_msg_phys);
> +	msg.data = virq;
> +
> +	pci_write_msi_msg(virq, &msg);
> +
> +	return 0;
> +}
> +
> +static void advk_pcie_teardown_msi_irq(struct msi_controller *chip,
> +				       unsigned int irq)
> +{
> +	struct irq_data *d = irq_get_irq_data(irq);
> +	struct msi_desc *msi = irq_data_get_msi_desc(d);
> +	struct advk_pcie *pcie = msi_desc_to_pci_sysdata(msi);
> +	unsigned long hwirq = d->hwirq;
> +
> +	irq_dispose_mapping(irq);
> +	advk_pcie_free_msi(pcie, hwirq);
> +}
> +
> +static int advk_pcie_msi_map(struct irq_domain *domain,
> +			     unsigned int virq, irq_hw_number_t hw)
> +{
> +	struct advk_pcie *pcie = domain->host_data;
> +
> +	irq_set_chip_and_handler(virq, &pcie->msi_irq_chip,
> +				 handle_simple_irq);
> +
> +	return 0;
> +}
> +
> +static const struct irq_domain_ops advk_pcie_msi_irq_ops = {
> +	.map = advk_pcie_msi_map,
> +};
> +
> +static void advk_pcie_irq_mask(struct irq_data *d)
> +{
> +	struct advk_pcie *pcie = d->domain->host_data;
> +	irq_hw_number_t hwirq = irqd_to_hwirq(d);
> +	u32 mask;
> +
> +	mask = advk_readl(pcie, PCIE_ISR0_MASK_REG);
> +	mask |= PCIE_ISR0_INTX_ASSERT(hwirq);
> +	advk_writel(pcie, mask, PCIE_ISR0_MASK_REG);
> +}
> +
> +static void advk_pcie_irq_unmask(struct irq_data *d)
> +{
> +	struct advk_pcie *pcie = d->domain->host_data;
> +	irq_hw_number_t hwirq = irqd_to_hwirq(d);
> +	u32 mask;
> +
> +	mask = advk_readl(pcie, PCIE_ISR0_MASK_REG);
> +	mask &= ~PCIE_ISR0_INTX_ASSERT(hwirq);
> +	advk_writel(pcie, mask, PCIE_ISR0_MASK_REG);
> +}
> +
> +static int advk_pcie_irq_map(struct irq_domain *h,
> +			     unsigned int virq, irq_hw_number_t hwirq)
> +{
> +	struct advk_pcie *pcie = h->host_data;
> +
> +	advk_pcie_irq_mask(irq_get_irq_data(virq));
> +	irq_set_status_flags(virq, IRQ_LEVEL);
> +	irq_set_chip_and_handler(virq, &pcie->irq_chip,
> +				 handle_level_irq);
> +	irq_set_chip_data(virq, pcie);
> +
> +	return 0;
> +}
> +
> +static const struct irq_domain_ops advk_pcie_irq_domain_ops = {
> +	.map = advk_pcie_irq_map,
> +	.xlate = irq_domain_xlate_onecell,
> +};
> +
> +static int advk_pcie_init_irq(struct advk_pcie *pcie)
> +{
> +	struct device *dev = &pcie->pdev->dev;
> +	struct device_node *node = dev->of_node;
> +	struct device_node *pcie_intc_node;
> +	struct irq_chip *irq_chip, *msi_irq_chip;
> +	struct msi_controller *msi;
> +	phys_addr_t msi_msg_phys;
> +	int ret;
> +
> +	pcie_intc_node =  of_get_next_child(node, NULL);
> +	if (!pcie_intc_node) {
> +		dev_err(dev, "No PCIe Intc node found\n");
> +		return PTR_ERR(pcie_intc_node);
> +	}
> +
> +	irq_chip = &pcie->irq_chip;
> +
> +	irq_chip->name = devm_kasprintf(dev, GFP_KERNEL, "%s-irq",
> +					dev_name(dev));
> +	if (!irq_chip->name)
> +		return -ENOMEM;
> +	irq_chip->irq_mask = advk_pcie_irq_mask;
> +	irq_chip->irq_mask_ack = advk_pcie_irq_mask;
> +	irq_chip->irq_unmask = advk_pcie_irq_unmask;
> +
> +	pcie->irq_domain =
> +		irq_domain_add_linear(pcie_intc_node, LEGACY_IRQ_NUM,
> +				      &advk_pcie_irq_domain_ops, pcie);
> +	if (!pcie->irq_domain) {
> +		dev_err(dev, "Failed to get a INTx IRQ domain\n");
> +		return PTR_ERR(pcie->irq_domain);
> +	}

Can you rename this to advk_pcie_init_irq_domain() and possibly split
to advk_pcie_init_msi_irq_domain() so it looks more similar to the
other drivers, e.g., nwl_pcie_init_irq_domain() or
xilinx_pcie_init_irq_domain()?

> +	msi_irq_chip = &pcie->msi_irq_chip;
> +
> +	msi_irq_chip->name = devm_kasprintf(dev, GFP_KERNEL, "%s-msi",
> +						 dev_name(dev));
> +	if (!msi_irq_chip->name) {
> +		irq_domain_remove(pcie->irq_domain);
> +		return -ENOMEM;
> +	}
> +
> +	msi_irq_chip->irq_enable = pci_msi_unmask_irq;
> +	msi_irq_chip->irq_disable = pci_msi_mask_irq;
> +	msi_irq_chip->irq_mask = pci_msi_mask_irq;
> +	msi_irq_chip->irq_unmask = pci_msi_unmask_irq;
> +
> +	msi = &pcie->msi;
> +
> +	msi->setup_irq = advk_pcie_setup_msi_irq;
> +	msi->teardown_irq = advk_pcie_teardown_msi_irq;
> +	msi->of_node = node;
> +
> +	mutex_init(&pcie->msi_used_lock);
> +
> +	msi_msg_phys = virt_to_phys(&pcie->msi_msg);
> +
> +	advk_writel(pcie, lower_32_bits(msi_msg_phys),
> +		    PCIE_MSI_ADDR_LOW_REG);
> +	advk_writel(pcie, upper_32_bits(msi_msg_phys),
> +		    PCIE_MSI_ADDR_HIGH_REG);
> +
> +	pcie->msi_domain =
> +		irq_domain_add_linear(NULL, MSI_IRQ_NUM,
> +				      &advk_pcie_msi_irq_ops, pcie);
> +	if (!pcie->msi_domain) {
> +		irq_domain_remove(pcie->irq_domain);
> +		return -ENOMEM;
> +	}
> +
> +	ret = of_pci_msi_chip_add(msi);
> +	if (ret < 0) {
> +		irq_domain_remove(pcie->msi_domain);
> +		irq_domain_remove(pcie->irq_domain);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static void advk_pcie_handle_msi(struct advk_pcie *pcie)
> +{
> +	u32 msi_val, msi_mask, msi_status, msi_idx;
> +	u16 msi_data;
> +
> +	msi_mask = advk_readl(pcie, PCIE_MSI_MASK_REG);
> +	msi_val = advk_readl(pcie, PCIE_MSI_STATUS_REG);
> +	msi_status = msi_val & ~msi_mask;
> +
> +	for (msi_idx = 0; msi_idx < MSI_IRQ_NUM; msi_idx++) {
> +		if (!(BIT(msi_idx) & msi_status))
> +			continue;
> +
> +		advk_writel(pcie, BIT(msi_idx), PCIE_MSI_STATUS_REG);
> +		msi_data = advk_readl(pcie, PCIE_MSI_PAYLOAD_REG) & 0xFF;
> +		generic_handle_irq(msi_data);
> +	}
> +
> +	advk_writel(pcie, PCIE_ISR0_MSI_INT_PENDING,
> +		    PCIE_ISR0_REG);
> +}
> +
> +static void advk_pcie_handle_int(struct advk_pcie *pcie)
> +{
> +	u32 val, mask, status;
> +	int i, virq;
> +
> +	val = advk_readl(pcie, PCIE_ISR0_REG);
> +	mask = advk_readl(pcie, PCIE_ISR0_MASK_REG);
> +	status = val & ((~mask) & PCIE_ISR0_ALL_MASK);
> +
> +	if (!status) {
> +		advk_writel(pcie, val, PCIE_ISR0_REG);
> +		return;
> +	}
> +
> +	/* Process MSI interrupts */
> +	if (status & PCIE_ISR0_MSI_INT_PENDING)
> +		advk_pcie_handle_msi(pcie);
> +
> +	/* Process legacy interrupts */
> +	for (i = 0; i < LEGACY_IRQ_NUM; i++) {
> +		if (!(status & PCIE_ISR0_INTX_ASSERT(i)))
> +			continue;
> +
> +		advk_writel(pcie, PCIE_ISR0_INTX_ASSERT(i),
> +			    PCIE_ISR0_REG);
> +
> +		virq = irq_find_mapping(pcie->irq_domain, i);
> +		generic_handle_irq(virq);
> +	}
> +}
> +
> +static irqreturn_t advk_pcie_irq_handler(int irq, void *arg)
> +{
> +	struct advk_pcie *pcie = arg;
> +	u32 status;
> +
> +	status = advk_readl(pcie, HOST_CTRL_INT_STATUS_REG);
> +	if (!(status & PCIE_IRQ_CORE_INT))
> +		return IRQ_NONE;
> +
> +	advk_pcie_handle_int(pcie);
> +
> +	/* Clear interrupt */
> +	advk_writel(pcie, PCIE_IRQ_CORE_INT, HOST_CTRL_INT_STATUS_REG);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static void advk_pcie_release_of_pci_ranges(struct advk_pcie *pcie)
> +{
> +	pci_free_resource_list(&pcie->resources);

You can inline this call below; I know other drivers wrap it too, but
I don't know why.

> +}
> +
> +static int advk_pcie_parse_request_of_pci_ranges(struct advk_pcie *pcie)
> +{
> +	int err, res_valid = 0;
> +	struct device *dev = &pcie->pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	struct resource_entry *win;
> +	resource_size_t iobase;
> +
> +	INIT_LIST_HEAD(&pcie->resources);
> +
> +	err = of_pci_get_host_bridge_resources(np, 0, 0xff, &pcie->resources,
> +					       &iobase);
> +	if (err)
> +		return err;
> +
> +	resource_list_for_each_entry(win, &pcie->resources) {
> +		struct resource *parent, *res = win->res;
> +
> +		switch (resource_type(res)) {
> +		case IORESOURCE_IO:
> +			parent = &ioport_resource;
> +			advk_pcie_set_ob_win(pcie, 1,
> +					     upper_32_bits(res->start),
> +					     lower_32_bits(res->start),
> +					     0,	0xF8000000, 0,
> +					     lower_32_bits(res->start),
> +					     OB_PCIE_IO);
> +			err = pci_remap_iospace(res, iobase);
> +			if (err) {
> +				dev_warn(dev, "error %d: failed to map resource %pR\n",
> +					 err, res);
> +				continue;
> +			}
> +			break;
> +		case IORESOURCE_MEM:
> +			parent = &iomem_resource;
> +			advk_pcie_set_ob_win(pcie, 0,
> +					     upper_32_bits(res->start),
> +					     lower_32_bits(res->start),
> +					     0x0, 0xF8000000, 0,
> +					     lower_32_bits(res->start),
> +					     (2 << 20) | OB_PCIE_MEM);
> +			res_valid |= !(res->flags & IORESOURCE_PREFETCH);
> +			break;
> +		default:
> +			continue;
> +		}
> +
> +		err = devm_request_resource(dev, parent, res);

Thanks for including this.  I'll try to remember to update this to use
devm_request_pci_bus_resources() when I merge it (that's currently on
my pci/host-request-windows branch, not upstream yet).

> +		if (err)
> +			goto out_release_res;
> +	}
> +
> +	if (!res_valid) {
> +		dev_err(dev, "non-prefetchable memory resource required\n");
> +		err = -EINVAL;
> +		goto out_release_res;
> +	}
> +
> +	return 0;
> +
> +out_release_res:
> +	advk_pcie_release_of_pci_ranges(pcie);
> +	return err;
> +}
> +
> +static int advk_pcie_probe(struct platform_device *pdev)
> +{
> +	struct advk_pcie *pcie;
> +	struct resource *res;
> +	struct pci_bus *bus, *child;
> +	struct msi_controller *msi;
> +	struct device_node *msi_node;
> +	int ret, irq;
> +
> +	pcie = devm_kzalloc(&pdev->dev, sizeof(struct advk_pcie),
> +			    GFP_KERNEL);
> +	if (!pcie)
> +		return -ENOMEM;
> +
> +	pcie->pdev = pdev;
> +	platform_set_drvdata(pdev, pcie);
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	pcie->base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(pcie->base)) {
> +		dev_err(&pdev->dev, "failed to map registers\n");
> +		return PTR_ERR(pcie->base);
> +	}
> +
> +	irq = platform_get_irq(pdev, 0);
> +	ret = devm_request_irq(&pdev->dev, irq, advk_pcie_irq_handler,
> +			       IRQF_SHARED | IRQF_NO_THREAD, "advk-pcie",
> +			       pcie);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to register interrupt\n");
> +		return ret;
> +	}
> +
> +	ret = advk_pcie_parse_request_of_pci_ranges(pcie);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to parse resources\n");
> +		return ret;
> +	}
> +
> +	advk_pcie_setup_hw(pcie);
> +	advk_pcie_enable_axi_addr_gen(pcie, true);
> +
> +	ret = advk_pcie_init_irq(pcie);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to initialize irq\n");
> +		return ret;
> +	}
> +
> +	msi_node = of_parse_phandle(pdev->dev.of_node, "msi-parent", 0);
> +	if (msi_node)
> +		msi = of_pci_find_msi_chip_by_node(msi_node);
> +	else
> +		msi = NULL;
> +
> +	bus = pci_scan_root_bus_msi(&pdev->dev, 0, &advk_pcie_ops,
> +				    pcie, &pcie->resources, &pcie->msi);
> +	if (!bus)
> +		return -ENOMEM;
> +
> +	pci_bus_assign_resources(bus);
> +
> +	list_for_each_entry(child, &bus->children, node)
> +		pcie_bus_configure_settings(child);
> +
> +	pci_bus_add_devices(bus);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id advk_pcie_of_match_table[] = {
> +	{ .compatible = "marvell,armada-3700-pcie", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, advk_pcie_of_match_table);

Since this driver is currently bool in Kconfig, can you omit the
MODULE_*() uses?  It would be ideal to make this modular, of course,
but if it's not modular, let's make it so it doesn't *look* like a
module.

> +static struct platform_driver advk_pcie_driver = {
> +	.driver = {
> +		.name = "advk-pcie",
> +		.of_match_table = advk_pcie_of_match_table,
> +		/* Driver unloading/unbinding currently not supported */
> +		.suppress_bind_attrs = true,

I see some other drivers (mvebu, rcar, tegra, altera, xilinx) also use
suppress_bind_attrs = true, but I don't know what's special about
them.  Do you know?  Do we really need this here?  What would it take
to change this driver so we could omit it?

> +	},
> +	.probe = advk_pcie_probe,
> +};
> +module_platform_driver(advk_pcie_driver);
> +
> +MODULE_AUTHOR("Hezi Shahmoon <[email-masked]>");
> +MODULE_DESCRIPTION("Aardvark PCIe driver");
> +MODULE_LICENSE("GPL v2");
> -- 
> 2.7.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html