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

Lorenzo Pieralisi
2022-08-25T15:15:58Z

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
>