Hello!
On Tuesday 19 July 2022 10:05:28 Arnd Bergmann wrote:
> On Mon, Jul 18, 2022 at 10:28 PM Mauri Sandberg <maukka@xxxxxxxxxxxx> 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
> > 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@xxxxxxxxxxxx>
> > Cc: Pali Rohár <pali@xxxxxxxxxx>
>
> Nice job, glad you managed to figure this out!
>
> > 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,
>
> If the idea is to have the PCI_MVEBU driver only used for the DT based orion5x
> machines, but not the legacy board files, I suspect this breaks the legacy
> pci driver, unless you move the mbus configuration into the pcie_setup()
> function.
>
> > +/* Relevant only for Orion-1/Orion-NAS */
> > +#define ORION5X_PCIE_WA_PHYS_BASE 0xf0000000
> > +#define ORION5X_PCIE_WA_VIRT_BASE IOMEM(0xfd000000)
>
> You should not need to hardcode these here. The ORION5X_PCIE_WA_PHYS_BASE
> should already be part of the DT binding.
Of course! But the issue is that we do not know how to do this DT
binding. I have already wrote email with asking for help in which
property and which format should be this config range defined, but no
answer yet: https://lore.kernel.org/linux-pci/20220710225108.bgedria6igtqpz5l@pali/
> There is little practical difference
> here, but I see no value in taking the shortcut here either.
>
> For the ORION5X_PCIE_WA_VIRT_BASE, you rely on this to match the
> definition in arch/arm/mach-orion5x/common.c, and this is rather fragile.
>
> Instead, please use ioremap() to create a mapping at runtime. The ioremap()
> implementation on ARM is smart enough to reuse the address from the static
> mapping in common.c, but will also keep working if that should go away.
I'm planning to work with Mauri on this, but current blocker is DT.
> > +#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.)
> > + */
> > + 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);
> > +}
> > +
>
> This is probably good enough here, though I think you could also use
> the trick from drivers/pci/ecam.c and map each bus at a time.
>
> Arnd
Yes, there are also helper functions like map bus and etc. which could
simplify this code. I'm planning to do cleanups once we have fully
working driver for Orion.
