Message-ID: <CAMhs-H-NkL4=hcVfBvT0ZeBOfg8bmPYv1urJ1JVWtcA2tbtfjA@mail.gmail.com>
Date:   Wed, 1 Dec 2021 21:56:22 +0100
From:   Sergio Paracuellos <sergio.paracuellos@...il.com>
To:     Bjorn Helgaas <helgaas@...nel.org>
Cc:     linux-pci <linux-pci@...r.kernel.org>,
        "open list:MIPS" <linux-mips@...r.kernel.org>,
        Thomas Bogendoerfer <tsbogend@...ha.franken.de>,
        John Crispin <john@...ozen.org>,
        Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        Arnd Bergmann <arnd@...db.de>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Thierry Reding <thierry.reding@...il.com>
Subject: Re: [PATCH 1/5] PCI: let 'pcibios_root_bridge_prepare()' access to 'bridge->windows'

On Wed, Dec 1, 2021 at 9:24 PM Bjorn Helgaas <helgaas@...nel.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@...il.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

Powered by blists - more mailing lists