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

["Pali Rohár" <pali@kernel.org>]

Hello!

On Tuesday 19 July 2022 10:05:28 Arnd Bergmann wrote:
> On Mon, Jul 18, 2022 at 10:28 PM Mauri Sandberg <maukka@ext.kapsi.fi> 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@ext.kapsi.fi>
> > Cc: Pali Rohár <pali@kernel.org>
> 
> 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.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel