| Message ID | 20211115070809.15529-2-sergio.paracuellos@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | PCI: mt7621: remove specific MIPS code from driver | expand |
[+cc Thierry] In subject, PCI: Let pcibios_root_bridge_prepare() access bridge->windows On Mon, Nov 15, 2021 at 08:08:05AM +0100, Sergio Paracuellos wrote: > When function 'pci_register_host_bridge()' is called, 'bridge->windows' are > already available. However this windows are being moved temporarily from > there. To let 'pcibios_root_bridge_prepare()' to have access to this windows > move this windows movement after call this function. This is interesting for > MIPS ralink mt7621 platform to be able to properly set I/O coherence units > with this information and avoid custom MIPs code in generic PCIe controller > drivers. > > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com> > --- > drivers/pci/probe.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > index 087d3658f75c..372a70efccc6 100644 > --- a/drivers/pci/probe.c > +++ b/drivers/pci/probe.c > @@ -898,8 +898,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); Arnd added this with 37d6a0a6f470 ("PCI: Add pci_register_host_bridge() interface") [1]. I can't remember why this was done, but we did go to some trouble to move things around, so there must have been a good reason. Arnd or Thierry, do you remember? > bus->sysdata = bridge->sysdata; > bus->ops = bridge->ops; > bus->number = bus->busn_res.start = bridge->busnr; > @@ -925,6 +923,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); > -- > 2.33.0 > [1] https://git.kernel.org/linus/37d6a0a6f470
On Fri, Nov 19, 2021 at 05:20:17PM -0600, Bjorn Helgaas wrote: > [+cc Thierry] > > In subject, > > PCI: Let pcibios_root_bridge_prepare() access bridge->windows > > On Mon, Nov 15, 2021 at 08:08:05AM +0100, Sergio Paracuellos wrote: > > When function 'pci_register_host_bridge()' is called, 'bridge->windows' are > > already available. However this windows are being moved temporarily from > > there. To let 'pcibios_root_bridge_prepare()' to have access to this windows > > move this windows movement after call this function. This is interesting for > > MIPS ralink mt7621 platform to be able to properly set I/O coherence units > > with this information and avoid custom MIPs code in generic PCIe controller > > drivers. > > > > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com> > > --- > > drivers/pci/probe.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > > index 087d3658f75c..372a70efccc6 100644 > > --- a/drivers/pci/probe.c > > +++ b/drivers/pci/probe.c > > @@ -898,8 +898,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); > > Arnd added this with 37d6a0a6f470 ("PCI: Add > pci_register_host_bridge() interface") [1]. > > I can't remember why this was done, but we did go to some trouble to > move things around, so there must have been a good reason. > > Arnd or Thierry, do you remember? Nobody seems to remember, so I think we should go ahead and make this change after the usual due diligence (audit the code between the old site and the new site to look for any uses of bridge->windows). I think this would be material for v5.17. > > bus->sysdata = bridge->sysdata; > > bus->ops = bridge->ops; > > bus->number = bus->busn_res.start = bridge->busnr; > > @@ -925,6 +923,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); > > -- > > 2.33.0 > > > > [1] https://git.kernel.org/linus/37d6a0a6f470
On Mon, Nov 15, 2021 at 08:08:05AM +0100, Sergio Paracuellos wrote: > When function 'pci_register_host_bridge()' is called, 'bridge->windows' are > already available. However this windows are being moved temporarily from > there. To let 'pcibios_root_bridge_prepare()' to have access to this windows > move this windows movement after call this function. This is interesting for > MIPS ralink mt7621 platform to be able to properly set I/O coherence units > with this information and avoid custom MIPs code in generic PCIe controller > drivers. Oops, forgot to mention: s/this windows/these windows/ s/MIPs/MIPS/ You can drop the single quote around function names, too; the "()" is enough of a hint. And s/PCI: let/PCI: Let/ in the subject.
On Wed, Dec 1, 2021 at 9:24 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > On Fri, Nov 19, 2021 at 05:20:17PM -0600, Bjorn Helgaas wrote: > > > > Arnd added this with 37d6a0a6f470 ("PCI: Add > > pci_register_host_bridge() interface") [1]. > > > > I can't remember why this was done, but we did go to some trouble to > > move things around, so there must have been a good reason. > > > > Arnd or Thierry, do you remember? > > Nobody seems to remember, so I think we should go ahead and make this > change after the usual due diligence (audit the code between the old > site and the new site to look for any uses of bridge->windows). > > I think this would be material for v5.17. Sorry I forgot to reply to your earlier mail. I think this is fine, as far as I remember, the only reason the bridge windows are moved from the list and added back one at a time was to preserve the exact orderorks. We could probably even skip that step entirely and iterate throughing that was there originally, to keep the behavior after a series of reworks. We could probably even skip that step entirely and iterate through bridge->windows instead of the local list to simplify this. For Sergio's patch: Acked-by: Arnd Bergmann <arnd@arndb.de>
On Wed, Dec 1, 2021 at 9:24 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Fri, Nov 19, 2021 at 05:20:17PM -0600, Bjorn Helgaas wrote: > > [+cc Thierry] > > > > In subject, > > > > PCI: Let pcibios_root_bridge_prepare() access bridge->windows > > > > On Mon, Nov 15, 2021 at 08:08:05AM +0100, Sergio Paracuellos wrote: > > > When function 'pci_register_host_bridge()' is called, 'bridge->windows' are > > > already available. However this windows are being moved temporarily from > > > there. To let 'pcibios_root_bridge_prepare()' to have access to this windows > > > move this windows movement after call this function. This is interesting for > > > MIPS ralink mt7621 platform to be able to properly set I/O coherence units > > > with this information and avoid custom MIPs code in generic PCIe controller > > > drivers. > > > > > > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com> > > > --- > > > drivers/pci/probe.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > > > index 087d3658f75c..372a70efccc6 100644 > > > --- a/drivers/pci/probe.c > > > +++ b/drivers/pci/probe.c > > > @@ -898,8 +898,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); > > > > Arnd added this with 37d6a0a6f470 ("PCI: Add > > pci_register_host_bridge() interface") [1]. > > > > I can't remember why this was done, but we did go to some trouble to > > move things around, so there must have been a good reason. > > > > Arnd or Thierry, do you remember? > > Nobody seems to remember, so I think we should go ahead and make this > change after the usual due diligence (audit the code between the old > site and the new site to look for any uses of bridge->windows). It seems any user of the pcibios_root_bridge_prepare() does nothing with 'bridge->windows'. But I don't get the point of passing around a complete bridge pointer if windows are temporarily removed from there. That is an incomplete bridge and after parsing windows from dts are supposed to be there... What do you mean with 'audit the code between the old and new site'? > > I think this would be material for v5.17. Do you prefer me to parse dts again inside pcibios_root_bridge_prepare() for ralink mt7621?. Not real sense since 'windows' should be already there, but it would be a way to get this patchset added for v5.16. Something like (not tested yet but it should work): int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge) { resource_size_t mask; struct device_node *np; struct of_range range; struct of_range_parser parser; struct resource res; np = of_find_compatible_node(NULL, NULL, "mediatek,mt7621-pci"); if (!np) { pr_err("Cannot find pci node\n"); return -ENODEV; } if (of_range_parser_init(&parser, np)) { pr_err("Error parsing resources\n"); of_node_put(np); return -EINVAL; } for_each_of_range(&parser, &range) { switch (range.flags & IORESOURCE_TYPE_BITS) { case IORESOURCE_MEM: res.start = range.cpu_addr; res.end = range.cpu_addr + range.size - 1; break; } } if (mips_cps_numiocu(0)) { mask = ~(res.end - res.start); write_gcr_reg1_base(res.start); write_gcr_reg1_mask(mask | CM_GCR_REGn_MASK_CMTGT_IOCU0); pr_info("PCI coherence region base: 0x%08llx, mask/settings: 0x%08llx\n", (unsigned long long)read_gcr_reg1_base(), (unsigned long long)read_gcr_reg1_mask()); } return 0; } We can change this for v5.17 with the change in PCI core. I have just seen Arnd's Acked-by for PATCH 1 while I was writing this, so I am not sure if we can consider now this patchset as it is with proposed changes for v5.16. Thanks in advance for clarification. Best regards, Sergio Paracuellos > > > > bus->sysdata = bridge->sysdata; > > > bus->ops = bridge->ops; > > > bus->number = bus->busn_res.start = bridge->busnr; > > > @@ -925,6 +923,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); > > > -- > > > 2.33.0 > > > > > > > [1] https://git.kernel.org/linus/37d6a0a6f470
[+cc Guenter from other thread: https://lore.kernel.org/r/20211129015909.GA921717@roeck-us.net] On Wed, Dec 01, 2021 at 09:56:22PM +0100, Sergio Paracuellos wrote: > On Wed, Dec 1, 2021 at 9:24 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Fri, Nov 19, 2021 at 05:20:17PM -0600, Bjorn Helgaas wrote: > > > [+cc Thierry] > > > > > > In subject, > > > > > > PCI: Let pcibios_root_bridge_prepare() access bridge->windows > > > > > > On Mon, Nov 15, 2021 at 08:08:05AM +0100, Sergio Paracuellos wrote: > > > > When function 'pci_register_host_bridge()' is called, 'bridge->windows' are > > > > already available. However this windows are being moved temporarily from > > > > there. To let 'pcibios_root_bridge_prepare()' to have access to this windows > > > > move this windows movement after call this function. This is interesting for > > > > MIPS ralink mt7621 platform to be able to properly set I/O coherence units > > > > with this information and avoid custom MIPs code in generic PCIe controller > > > > drivers. > > > > > > > > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com> > > > > --- > > > > drivers/pci/probe.c | 4 ++-- > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > > > > index 087d3658f75c..372a70efccc6 100644 > > > > --- a/drivers/pci/probe.c > > > > +++ b/drivers/pci/probe.c > > > > @@ -898,8 +898,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); > > > > > > Arnd added this with 37d6a0a6f470 ("PCI: Add > > > pci_register_host_bridge() interface") [1]. > > > > > > I can't remember why this was done, but we did go to some trouble to > > > move things around, so there must have been a good reason. > > > > > > Arnd or Thierry, do you remember? > > > > Nobody seems to remember, so I think we should go ahead and make this > > change after the usual due diligence (audit the code between the old > > site and the new site to look for any uses of bridge->windows). > > It seems any user of the pcibios_root_bridge_prepare() does nothing > with 'bridge->windows'. But I don't get the point of passing around a > complete bridge pointer if windows are temporarily removed from there. > That is an incomplete bridge and after parsing windows from dts are > supposed to be there... What do you mean with 'audit the code between > the old and new site'? I mean "look at all the code that is run between the old site and the new site to make sure that none of that code depends on bridge->windows being temporarily emptied." > > I think this would be material for v5.17. > > Do you prefer me to parse dts again inside > pcibios_root_bridge_prepare() for ralink mt7621?. Not real sense since > 'windows' should be already there, but it would be a way to get this > patchset added for v5.16. Something like (not tested yet but it should > work): This is definitely too big for v5.16, regardless of which way you go. For v5.16, the only thing that's practical is to avoid building as a module. It'd be *nice* if it could be built as a module, but it is not a requirement. Bjorn
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 087d3658f75c..372a70efccc6 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -898,8 +898,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->ops = bridge->ops; bus->number = bus->busn_res.start = bridge->busnr; @@ -925,6 +923,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);
When function 'pci_register_host_bridge()' is called, 'bridge->windows' are already available. However this windows are being moved temporarily from there. To let 'pcibios_root_bridge_prepare()' to have access to this windows move this windows movement after call this function. This is interesting for MIPS ralink mt7621 platform to be able to properly set I/O coherence units with this information and avoid custom MIPs code in generic PCIe controller drivers. Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com> --- drivers/pci/probe.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)