Re: [PATCH v2 2/2] PCI: mvebu: add support for orion5x

From: Lorenzo Pieralisi <lpieralisi@kernel.org>
To: Mauri Sandberg <maukka@ext.kapsi.fi>
Cc: linux-pci@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org, bhelgaas@google.com,
	robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org,
	andrew@lunn.ch, sebastian.hesselbarth@gmail.com,
	gregory.clement@bootlin.com, linux@armlinux.org.uk, kw@linux.com,
	thomas.petazzoni@bootlin.com, pali@kernel.org
Subject: Re: [PATCH v2 2/2] PCI: mvebu: add support for orion5x
Date: Thu, 25 Aug 2022 17:15:36 +0200	[thread overview]
Message-ID: <YweSGDN7rei2Ugwp@lpieralisi> (raw)
In-Reply-To: <20220802173423.47230-3-maukka@ext.kapsi.fi>

On Tue, Aug 02, 2022 at 08:34:23PM +0300, Mauri Sandberg wrote:
> Add support for orion5x PCIe controller.
> 
> There is Orion-specific errata that config space via CF8/CFC registers
> is broken. Workaround documented in errata documented (linked from above
> documentation) does not work when DMA is used and instead other

Linked to which documentation ?

> undocumented workaround is needed which maps config space to memory
> (and therefore avoids usage of broken CF8/CFC memory mapped registers).
> 
> Signed-off-by: Mauri Sandberg <maukka@ext.kapsi.fi>
> Cc: Pali Rohár <pali@kernel.org>
> ---
> v1 -> v2:
>  - do pcie related mvebu windows and remaps in pcie_setup()
> ---
>  arch/arm/mach-orion5x/common.c     | 13 -------
>  arch/arm/mach-orion5x/pci.c        | 14 +++++++
>  drivers/pci/controller/Kconfig     |  2 +-
>  drivers/pci/controller/pci-mvebu.c | 59 ++++++++++++++++++++++++++++++
>  4 files changed, 74 insertions(+), 14 deletions(-)
> 
> diff --git a/arch/arm/mach-orion5x/common.c b/arch/arm/mach-orion5x/common.c
> index 7bcb41137bbf..9d8be5ce1266 100644
> --- a/arch/arm/mach-orion5x/common.c
> +++ b/arch/arm/mach-orion5x/common.c
> @@ -231,19 +231,6 @@ void __init orion5x_init_early(void)
>  
>  void orion5x_setup_wins(void)
>  {
> -	/*
> -	 * The PCIe windows will no longer be statically allocated
> -	 * here once Orion5x is migrated to the pci-mvebu driver.
> -	 */
> -	mvebu_mbus_add_window_remap_by_id(ORION_MBUS_PCIE_IO_TARGET,
> -					  ORION_MBUS_PCIE_IO_ATTR,
> -					  ORION5X_PCIE_IO_PHYS_BASE,
> -					  ORION5X_PCIE_IO_SIZE,
> -					  ORION5X_PCIE_IO_BUS_BASE);
> -	mvebu_mbus_add_window_by_id(ORION_MBUS_PCIE_MEM_TARGET,
> -				    ORION_MBUS_PCIE_MEM_ATTR,
> -				    ORION5X_PCIE_MEM_PHYS_BASE,
> -				    ORION5X_PCIE_MEM_SIZE);
>  	mvebu_mbus_add_window_remap_by_id(ORION_MBUS_PCI_IO_TARGET,
>  					  ORION_MBUS_PCI_IO_ATTR,
>  					  ORION5X_PCI_IO_PHYS_BASE,
> diff --git a/arch/arm/mach-orion5x/pci.c b/arch/arm/mach-orion5x/pci.c
> index 9574c73f3c03..7c4e2f355cc7 100644
> --- a/arch/arm/mach-orion5x/pci.c
> +++ b/arch/arm/mach-orion5x/pci.c
> @@ -150,6 +150,20 @@ static int __init pcie_setup(struct pci_sys_data *sys)
>  	 */
>  	orion_pcie_setup(PCIE_BASE);
>  
> +	/*
> +	 * The PCIe windows will no longer be statically allocated
> +	 * here once Orion5x is migrated to the pci-mvebu driver.
> +	 */

Is this comment still relevant ? And more importantly, may I ask
you why this code move in this hunk ? I think, whatever the reason
is, that deserves a comment.

> +	mvebu_mbus_add_window_remap_by_id(ORION_MBUS_PCIE_IO_TARGET,
> +					  ORION_MBUS_PCIE_IO_ATTR,
> +					  ORION5X_PCIE_IO_PHYS_BASE,
> +					  ORION5X_PCIE_IO_SIZE,
> +					  ORION5X_PCIE_IO_BUS_BASE);
> +	mvebu_mbus_add_window_by_id(ORION_MBUS_PCIE_MEM_TARGET,
> +				    ORION_MBUS_PCIE_MEM_ATTR,
> +				    ORION5X_PCIE_MEM_PHYS_BASE,
> +				    ORION5X_PCIE_MEM_SIZE);
> +
>  	/*
>  	 * Check whether to apply Orion-1/Orion-NAS PCIe config
>  	 * read transaction workaround.
> diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
> index b8d96d38064d..a249375837f0 100644
> --- a/drivers/pci/controller/Kconfig
> +++ b/drivers/pci/controller/Kconfig
> @@ -5,7 +5,7 @@ menu "PCI controller drivers"
>  
>  config PCI_MVEBU
>  	tristate "Marvell EBU PCIe controller"
> -	depends on ARCH_MVEBU || ARCH_DOVE || COMPILE_TEST
> +	depends on ARCH_MVEBU || ARCH_DOVE || ARCH_ORION5X || COMPILE_TEST
>  	depends on MVEBU_MBUS
>  	depends on ARM
>  	depends on OF
> diff --git a/drivers/pci/controller/pci-mvebu.c b/drivers/pci/controller/pci-mvebu.c
> index c1ffdb06c971..1d3052aa7e49 100644
> --- a/drivers/pci/controller/pci-mvebu.c
> +++ b/drivers/pci/controller/pci-mvebu.c
> @@ -1487,6 +1487,54 @@ static int mvebu_pcie_parse_request_resources(struct mvebu_pcie *pcie)
>  	return 0;
>  }
>  
> +static int orion_pcie_rd_conf_wa(void __iomem *wa_base, struct pci_bus *bus,
> +			  u32 devfn, int where, int size, u32 *val)
> +{
> +	*val = readl(wa_base + (PCIE_CONF_BUS(bus->number) |
> +				PCIE_CONF_DEV(PCI_SLOT(devfn)) |
> +				PCIE_CONF_FUNC(PCI_FUNC(devfn)) |
> +				PCIE_CONF_REG(where)));
> +
> +	if (size == 1)
> +		*val = (*val >> (8 * (where & 3))) & 0xff;
> +	else if (size == 2)
> +		*val = (*val >> (8 * (where & 3))) & 0xffff;
> +
> +	return PCIBIOS_SUCCESSFUL;
> +}
> +
> +/* Relevant only for Orion-1/Orion-NAS */
> +#define ORION5X_PCIE_WA_PHYS_BASE	0xf0000000
> +#define ORION5X_PCIE_WA_VIRT_BASE	IOMEM(0xfd000000)
> +#define ORION5X_PCIE_WA_SIZE		SZ_16M
> +#define ORION_MBUS_PCIE_WA_TARGET	0x04
> +#define ORION_MBUS_PCIE_WA_ATTR		0x79
> +
> +static int mvebu_pcie_child_rd_conf_wa(struct pci_bus *bus, u32 devfn, int where, int size, u32 *val)
> +{
> +	struct mvebu_pcie *pcie = bus->sysdata;
> +	struct mvebu_pcie_port *port;
> +
> +	port = mvebu_pcie_find_port(pcie, bus, devfn);
> +	if (!port)
> +		return PCIBIOS_DEVICE_NOT_FOUND;
> +
> +	if (!mvebu_pcie_link_up(port))
> +		return PCIBIOS_DEVICE_NOT_FOUND;
> +
> +	/*
> +	 * We only support access to the non-extended configuration
> +	 * space when using the WA access method (or we would have to
> +	 * sacrifice 256M of CPU virtual address space.)

Please expand the comment - future reviewers and developers may need
this information to understand this choice, me included.

Lorenzo

> +	 */
> +	if (where >= 0x100) {
> +		*val = 0xffffffff;
> +		return PCIBIOS_DEVICE_NOT_FOUND;
> +	}
> +
> +	return orion_pcie_rd_conf_wa(ORION5X_PCIE_WA_VIRT_BASE, bus, devfn, where, size, val);
> +}
> +
>  static int mvebu_pcie_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> @@ -1663,6 +1711,16 @@ static int mvebu_pcie_probe(struct platform_device *pdev)
>  	bridge->align_resource = mvebu_pcie_align_resource;
>  	bridge->map_irq = mvebu_pcie_map_irq;
>  
> +	if (of_machine_is_compatible("marvell,orion5x-88f5181")) {
> +		dev_info(dev, "Applying Orion-1/Orion-NAS PCIe config read transaction workaround\n");
> +
> +		mvebu_pcie_child_ops.read = mvebu_pcie_child_rd_conf_wa;
> +		mvebu_mbus_add_window_by_id(ORION_MBUS_PCIE_WA_TARGET,
> +					    ORION_MBUS_PCIE_WA_ATTR,
> +					    ORION5X_PCIE_WA_PHYS_BASE,
> +					    ORION5X_PCIE_WA_SIZE);
> +	}
> +
>  	return pci_host_probe(bridge);
>  }
>  
> @@ -1733,6 +1791,7 @@ static const struct of_device_id mvebu_pcie_of_match_table[] = {
>  	{ .compatible = "marvell,armada-370-pcie", },
>  	{ .compatible = "marvell,dove-pcie", },
>  	{ .compatible = "marvell,kirkwood-pcie", },
> +	{ .compatible = "marvell,orion5x-pcie", },
>  	{},
>  };
>  
> -- 
> 2.25.1
> 