Re: [PATCH 05/11] PCI: versatile: Use pci_parse_request_of_pci_ranges()

[Andrew Murray <andrew.murray@arm.com>]

On Thu, Sep 26, 2019 at 04:44:31PM -0500, Rob Herring wrote:
> On Wed, Sep 25, 2019 at 5:37 AM Andrew Murray <andrew.murray@arm.com> wrote:
> >
> > On Tue, Sep 24, 2019 at 04:46:24PM -0500, Rob Herring wrote:
> > > Convert ARM Versatile host bridge to use the common
> > > pci_parse_request_of_pci_ranges().
> > >
> > > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > > Signed-off-by: Rob Herring <robh@kernel.org>
> > > ---
> > >  drivers/pci/controller/pci-versatile.c | 62 +++++---------------------
> > >  1 file changed, 11 insertions(+), 51 deletions(-)
> > >
> > > diff --git a/drivers/pci/controller/pci-versatile.c b/drivers/pci/controller/pci-versatile.c
> > > index f59ad2728c0b..237b1abb26f2 100644
> > > --- a/drivers/pci/controller/pci-versatile.c
> > > +++ b/drivers/pci/controller/pci-versatile.c
> > > @@ -62,60 +62,12 @@ static struct pci_ops pci_versatile_ops = {
> > >       .write  = pci_generic_config_write,
> > >  };
> > >
> > > -static int versatile_pci_parse_request_of_pci_ranges(struct device *dev,
> > > -                                                  struct list_head *res)
> > > -{
> > > -     int err, mem = 1, res_valid = 0;
> > > -     resource_size_t iobase;
> > > -     struct resource_entry *win, *tmp;
> > > -
> > > -     err = devm_of_pci_get_host_bridge_resources(dev, 0, 0xff, res, &iobase);
> > > -     if (err)
> > > -             return err;
> > > -
> > > -     err = devm_request_pci_bus_resources(dev, res);
> > > -     if (err)
> > > -             goto out_release_res;
> > > -
> > > -     resource_list_for_each_entry_safe(win, tmp, res) {
> > > -             struct resource *res = win->res;
> > > -
> > > -             switch (resource_type(res)) {
> > > -             case IORESOURCE_IO:
> > > -                     err = devm_pci_remap_iospace(dev, res, iobase);
> > > -                     if (err) {
> > > -                             dev_warn(dev, "error %d: failed to map resource %pR\n",
> > > -                                      err, res);
> > > -                             resource_list_destroy_entry(win);
> > > -                     }
> > > -                     break;
> > > -             case IORESOURCE_MEM:
> > > -                     res_valid |= !(res->flags & IORESOURCE_PREFETCH);
> > > -
> > > -                     writel(res->start >> 28, PCI_IMAP(mem));
> > > -                     writel(PHYS_OFFSET >> 28, PCI_SMAP(mem));
> > > -                     mem++;
> > > -
> > > -                     break;
> > > -             }
> > > -     }
> > > -
> > > -     if (res_valid)
> > > -             return 0;
> > > -
> > > -     dev_err(dev, "non-prefetchable memory resource required\n");
> > > -     err = -EINVAL;
> > > -
> > > -out_release_res:
> > > -     pci_free_resource_list(res);
> > > -     return err;
> > > -}
> > > -
> > >  static int versatile_pci_probe(struct platform_device *pdev)
> > >  {
> > >       struct device *dev = &pdev->dev;
> > >       struct resource *res;
> > > -     int ret, i, myslot = -1;
> > > +     struct resource_entry *entry;
> > > +     int ret, i, myslot = -1, mem = 0;
> >
> > I think 'mem' should be initialised to 1, at least that's what the original
> > code did. However I'm not sure why it should start from 1.
> 
> The original code I moved from arch/arm had 32MB @ 0x0c000000 called
> "PCI unused" which was requested with request_resource(), but never
> provided to the PCI core. Otherwise, I kept the setup the same. No one
> has complained in 4 years, though I'm not sure anyone would have
> noticed if I just deleted PCI support...

OK, well it would be good to see a tested-by tag from someone for this.

Reviewed-by: Andrew Murray <andrew.murray@arm.com>

Thanks,

Andrew Murray

> 
> Rob