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

[Arnd Bergmann <arnd@arndb.de>]

On Wed, Jul 20, 2022 at 6:13 PM Pali Rohár <pali@kernel.org> wrote:
>
> On Tuesday 19 July 2022 12:16:34 Arnd Bergmann wrote:
> > On Tue, Jul 19, 2022 at 11:46 AM Pali Rohár <pali@kernel.org> wrote:
> > 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
>
> Problem is that "ranges" in PCIe are used for specifying MEM and IO
> mappings and kernel PCI code does not allow any other type.
>
> > 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.
>
> It is more complicated. PCIe MEM and IO memory ranges are defined in
> "soc" node in "pcie-mem-aperture" and "pcie-io-aperture" properties.
> These ranges are shared across all PCIe controllers and assigning slices
> of these ranges to specific devices is done later by dynamic allocation.
> "soc" node is bind to mbus driver (which parse these properties) and
> provides API for other kernel drivers for dynamic allocation of memory
> from pcie aperture. In pcie node is just indirect reference to PCIe MEM
> and IO via MBUS_ID() macro and it is pci-mvebu.c driver who ask mbus
> driver for PCIe MEM and IO dynamic allocation.
>
> So because PCIe config space is not of type PCIe MEM nor PCIe IO
> (obviously) it cannot use "ranges" property. Because DT pcie nodes use
> "reg" property for specifying BDF address, we cannot use neither "reg"
> property for specifying memory range of PCIe config space.
>
> And here I'm lost.
>
> My guess is that proper way is to define "pcie-cfg-aperture" in "soc"
> node where would be defined physical address range without any binding
> to controller, then extend mbus driver to export API also for PCIe CFG
> and add code which dynamically assign slice of this range to some
> controller. And then use this new API by pci-mvebu.c to access config
> space. But pci-mvebu.c needs to know MBUS_ID() attributes which needs to
> be defined somewhere in pcie DT node...

I think you can define a "ranges" property in the parent node that
converts the MBUS address into a fake PCI MEM space address,
and this is what the Armada 385 example does with
<0x82000000 0 0x80000 MBUS_ID(0xf0, 0x01) 0x80000 0 0x00002000>
In the child you can refer to this using the "assigned-addresses"
property and then use the "ranges" to ensure that only the actual
IO and MEM space addresses are translated.

The alternative would be to do away with the indirection and make
the orion5x variant be more like a single hostbridge, and getting
rid of the fake parent. This is closer to what orion5x does at the
moment with the old driver, but might be harder to integrate into
the new one.

> > > > 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
>
> If we have free 256MB in physical address space, then we can implement
> it easily. It is just changing _size_ argument. I'm not sure how much
> DDR RAM has Orion, but if only 2GB then we should be fine (remaining 2GB
> should be enough for all peripherals + 256MB for PCIe config space).

I recently checked the sizes as I was researching which boards are actually
still usable, and I can confirm what Andrew also said. The only one with
256MB is TS409, everything else has 32MB to 128MB.

As I said before, I also see no problem with leaving the smaller mapping,
as we know that nobody has ever used the extended config space on
orion5x with mainline kernels, and we are not trying to support new
hardware.

> So we can only set "size" of the physical config space mapping and if we
> choose smaller size then we cannot access upper registers. I do not see
> any option how to specify "offset" for physical config space to allow
> mapping just one PCI bus.
>
> What we have under full control is virtual address space mapping, so we
> can just map only one PCI bus to virtual address space from large 256MB
> physical config address space.

Right.

       Arnd