On Wed, Nov 10, 2021 at 7:00 AM Sergio Paracuellos <sergio.paracuellos@xxxxxxxxx> wrote: > > On Tue, Nov 9, 2021 at 11:41 PM Bjorn Helgaas <helgaas@xxxxxxxxxx> 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@xxxxxxxxx> wrote: > > > > On Sat, Oct 30, 2021 at 7:21 AM Sergio Paracuellos > > > > <sergio.paracuellos@xxxxxxxxx> wrote: > > > > > On Fri, Oct 29, 2021 at 10:27 PM Sergio Paracuellos > > > > > <sergio.paracuellos@xxxxxxxxx> wrote: > > > > > > On Fri, Oct 29, 2021 at 9:47 PM Bjorn Helgaas <helgaas@xxxxxxxxxx> 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@xxxxxxxxxx> 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. > > 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. I have sent this just to a clear image of a possible way to go: https://lkml.org/lkml/2021/11/15/48 Thanks, Sergio Paracuellos > > > > > > When you have time, please, let me know your thoughts about this. > > > > > > Thanks in advance for your time. > > > > > > Best regards, > > > Sergio Paracuellos
