Re: [PATCH v2 2/3] MIPS: cm/cpc: export some missing symbols to be able to... | linux-arm-kernel

quoted

[+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
[off-list ref] wrote:
On Sat, Oct 30, 2021 at 7:21 AM Sergio Paracuellos
[off-list ref] wrote:
On Fri, Oct 29, 2021 at 10:27 PM Sergio Paracuellos
[off-list ref] wrote:
On Fri, Oct 29, 2021 at 9:47 PM Bjorn Helgaas [off-list ref] 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 [off-list ref] 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@1e140000 ranges:
[   16.798719] mt7621-pci 1e140000.pcie:   No bus range found for
/pcie@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.

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.

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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help