[PATCH v2 2/3] MIPS: cm/cpc: export some missing symbols to be able to use them from driver code

Tue Nov 9 22:00:09 PST 2021

On Tue, Nov 9, 2021 at 11:41 PM Bjorn Helgaas <helgaas at kernel.org> wrote:
>
> [+cc Arnd]
>
> On Sun, Nov 07, 2021 at 08:00:56AM +0100, Sergio Paracuellos wrote:
> > On Sat, Oct 30, 2021 at 7:38 AM Sergio Paracuellos
> > <sergio.paracuellos at gmail.com> wrote:
> > > On Sat, Oct 30, 2021 at 7:21 AM Sergio Paracuellos
> > > <sergio.paracuellos at gmail.com> wrote:
> > > > On Fri, Oct 29, 2021 at 10:27 PM Sergio Paracuellos
> > > > <sergio.paracuellos at gmail.com> wrote:
> > > > > On Fri, Oct 29, 2021 at 9:47 PM Bjorn Helgaas <helgaas at kernel.org> wrote:
> > > > > > On Fri, Oct 29, 2021 at 09:37:53PM +0200, Sergio Paracuellos wrote:
> > > > > > > On Fri, Oct 29, 2021 at 8:49 PM Bjorn Helgaas <helgaas at kernel.org> wrote:
>
> > > > > > > > One way might be to implement a
> > > > > > > > pcibios_root_bridge_prepare() for mips and put the
> > > > > > > > setup_cm_memory_region() stuff in there.  It's not
> > > > > > > > *ideal* because that's a strong/weak function
> > > > > > > > arrangement that doesn't allow for multiple host
> > > > > > > > bridges, but that's probably not an issue here.
> > > > > > > >
> > > > > > > > If we can't do that, I think making it bool is probably
> > > > > > > > the right answer, but it would be worth a brief comment
> > > > > > > > in the commit log to explain the issue.
> > > > > > >
> > > > > > > Do you mean to implement 'pcibios_root_bridge_prepare()'
> > > > > > > for MIPS ralink? I guess this means to parse device tree
> > > > > > > and so on only to get memory range addresses to be added
> > > > > > > to the MIPS I/O coherence regions to make things work and
> > > > > > > then re-parse it again in the driver to do the proper PCI
> > > > > > > setup... We end up in an arch generic driver but at the
> > > > > > > end this controller is only present in ralink MIPS, so I
> > > > > > > am not sure that implementing
> > > > > > > 'pcibios_root_bridge_prepare()' is worthy here... I can
> > > > > > > explore and try to implement it if you think that it
> > > > > > > really makes sense... but, IMHO if this is the case, just
> > > > > > > making it bool looks like the correct thing to do.
> > > > > >
> > > > > > It should be trivial to put the contents of
> > > > > > setup_cm_memory_region() into a ralink function called
> > > > > > pcibios_root_bridge_prepare().
> > > > > >
> > > > > > pcibios_root_bridge_prepare() is called with the same
> > > > > > "struct pci_host_bridge *" argument as
> > > > > > setup_cm_memory_region(), and it's called slightly later, so
> > > > > > the window resources are already set up, so no DT parsing is
> > > > > > required.  It looks like a simple move and rename to me.
> > > > >
> > > > > I see. Thanks Bjorn. I will try the approach during the
> > > > > weekend and report if it works.
> > > >
> > > > I have tested the change from 'setup_cm_memory_region()' code
> > > > into 'pcibios_root_bridge_prepare()' just by moving and renaming
> > > > it from the PCIe controller code. The function is properly being
> > > > called.  However, it looks like at that point, windows are not
> > > > setup yet (no windows present at all in bridge->windows) so the
> > > > system is not able to get the IORESOURCE_MEM resource to set up
> > > > the IO coherency unit and the PCI failed to start:
> > > >
> > > > [   16.785359] mt7621-pci 1e140000.pcie: host bridge /pcie at 1e140000 ranges:
> > > > [   16.798719] mt7621-pci 1e140000.pcie:   No bus range found for
> > > > /pcie at 1e140000, using [bus 00-ff]
> > > > [   16.816248] mt7621-pci 1e140000.pcie:      MEM
> > > > 0x0060000000..0x006fffffff -> 0x0060000000
> > > > [   16.861310] mt7621-pci 1e140000.pcie:       IO
> > > > 0x001e160000..0x001e16ffff -> 0x0000000000
> > > > [   17.179230] mt7621-pci 1e140000.pcie: PCIE0 enabled
> > > > [   17.188954] mt7621-pci 1e140000.pcie: PCIE1 enabled
> > > > [   17.198678] mt7621-pci 1e140000.pcie: PCIE2 enabled
> > > > [   17.208415] Cannot get memory resource
> > > > [   17.215884] mt7621-pci 1e140000.pcie: Scanning root bridge failed
> > > > [   17.228454] mt7621-pci: probe of 1e140000.pcie failed with error -22
> > > >
> > > > FWIW, when the function is called, I have also tried to set up
> > > > hardcoded addresses. Doing that the IO coherency unit was
> > > > properly set up and PCI properly worked (expected). So, using
> > > > this 'pcibios_root_bridge_prepare()' funcion looks like a
> > > > possible way to go but we need the addresses properly being
> > > > passed into the function.  I've also tried to list
> > > > 'bridge->dma_ranges' and get resources from there instead of
> > > > using the not already setup 'bridge->windows'. There is nothing
> > > > inside that list also. 'bridge->bus->resources' is also empty...
> > > > Am I missing something? I was expecting the bridge passed around
> > > > to be the same that was in PCIe controller code, and it seems it
> > > > is (I printed the bridge pointer itself in driver code before
> > > > calling 'mt7621_pcie_register_host()' and in
> > > > 'pcibios_root_bridge_prepare()' at the begging of the function
> > > > and the pointer is the same) but windows and other stuff are not
> > > > already present there...
> > >
> > > Looking into [0] it looks like resources are temporarily removed
> > > from the list just before call 'pcibios_root_bridge_prepare()'.
> > > Hence the behaviour I am seeing when trying to get them...
> > >
> > > [0]:
> > > https://elixir.bootlin.com/linux/latest/source/drivers/pci/probe.c#L915
> >
> > Can you explain to me, why are resources temporarily removed from
> > the 'bridge->windows' list?
> >
> > Would moving that list split to be done after
> > 'pcibios_root_bridge_prepare()' is called a possibility?
>
> I don't know why the windows are managed that way.  That was added by
> 37d6a0a6f470 ("PCI: Add pci_register_host_bridge() interface").   I
> cc'd Arnd just in case he remembers, but that was a long time ago.

Thanks, 2016 is not so far to already remember why things were done :)).

>
> I don't see any use of bridge->windows in any of the
> pcibios_root_bridge_prepare() functions.  It doesn't *look* like it
> should be used until the coalesce/add code near the end.

I checked them also, and nobody uses 'bridge->windows' there but all the
information is already there from dt parsing process when the
'pci_register_host_bridge()'
is called and we are putting temporally in a 'resources' list to add
again all of them at the end.
All the calls before 'pcibios_root_bridge_prepare()' don't do anything
related with windows
either, that's why I asked to move the split after calling it since,
at a first glance, it does not
look harmful. Let's wait for Arnd explanation about why things are
being doing in this way
and if is this a possible way to proceed.

Best regards,
     Sergio Paracuellos

>
> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > index 4289030b0fff..2132df91ad8b 100644
> > --- a/drivers/pci/probe.c
> > +++ b/drivers/pci/probe.c
> > @@ -891,8 +891,6 @@ static int pci_register_host_bridge(struct
> > pci_host_bridge *bridge)
> >
> >         bridge->bus = bus;
> >
> > -       /* Temporarily move resources off the list */
> > -       list_splice_init(&bridge->windows, &resources);
> >         bus->sysdata = bridge->sysdata;
> >         bus->msi = bridge->msi;
> >         bus->ops = bridge->ops;
> > @@ -916,6 +914,8 @@ static int pci_register_host_bridge(struct
> > pci_host_bridge *bridge)
> >         if (err)
> >                 goto free;
> >
> > +       /* Temporarily move resources off the list */
> > +       list_splice_init(&bridge->windows, &resources);
> >         err = device_add(&bridge->dev);
> >         if (err) {
> >                 put_device(&bridge->dev);
> >
> > Obviously doing this works and windows are passed into mips ralink
> > specific 'pcibios_root_bridge_prepare()' and the PCIe subsystem is
> > properly working.
> >
> > The advantages I see to this approach are that doing in this way lets us to:
> > - Remove specific mips code from the driver controller.
> > - Allow the driver to be compile tested for any architecture.
> >
> > And the changes would be the following patches:
> > 1) Small 'drivers/pci/probe.c' change.
> > 2) Move mips specific code into 'arch/mips/ralink/mt76721.c' (since
> > other mips ralink stuff haven't got IO coherency units) to be inside
> > 'pcibios_root_bridge_prepare()'.
> > 3) Add MODULE_LICENSE macro to the PCIe controller driver to avoid
> > complaints when the driver is compiled as a module .
> > 4) Update PCIe controller driver's Kconfig to avoid MIPS COMPILE_TEST
> > conditional and completely enable it for COMPILE_TEST.
> >
> > When you have time, please, let me know your thoughts about this.
> >
> > Thanks in advance for your time.
> >
> > Best regards,
> >     Sergio Paracuellos