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

[Arnd Bergmann <arnd@arndb.de>]

On Tue, Jul 19, 2022 at 11:46 AM Pali Rohár <pali@kernel.org> wrote:
> On Tuesday 19 July 2022 10:05:28 Arnd Bergmann wrote:
> > > +/* 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/

Ah, I had not seen that email. Quoting from there:

> So my question is: How to properly define config space range in device
> tree file? In which device tree property and in which format? Please
> note that this memory range of config space is PCIe root port specific
> and it requires its own MBUS_ID() like memory range of PCIe MEM and PCIe
> IO mapping. Please look e.g. at armada-385.dtsi how are MBUS_ID() used:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/armada-385.dtsi

This is probably a question for Rob as the mvebu driver is a rather special
case. Normally this would just be a 'reg' property of the host bridge,
but I think
in your case the root device is imaginary, and the ports under it are the
actual hardware devices, so you'll probably have to do the same thing as
the armada-385, translating the mbus ranges for the config space in the
"ranges" property of the parent, and then referring to them by PCI
MMIO addresses using the assigned-addresses property to pass the
config-space registers as a second set of registers in addition to the
first set.

> > 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.

Ok. It should not be hard to do this first, as you just need to pass the
same physical address that you pass in the mbus setup, but I agree
it's easier to do this afterwards to avoid having to rewrite it again.

> > 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.
> >
> 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.

Ok. This is probably not worth the effort if the old driver doesn't already
do provide access to the high registers.

      Arnd