Re: [PATCH 11/11] PCI: rcar: Use inbound resources for setup

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

On Thu, Sep 26, 2019 at 07:53:20AM -0500, Rob Herring wrote:
> On Thu, Sep 26, 2019 at 3:47 AM Andrew Murray <andrew.murray@arm.com> wrote:
> >
> > On Tue, Sep 24, 2019 at 04:46:30PM -0500, Rob Herring wrote:
> > > Now that the helpers provide the inbound resources in the host bridge
> > > 'dma_ranges' resource list, convert Renesas R-Car PCIe host bridge to
> > > use the resource list to setup the inbound addresses.
> > >
> > > Cc: Simon Horman <horms@verge.net.au>
> > > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > > Signed-off-by: Rob Herring <robh@kernel.org>
> > > ---
> > >  drivers/pci/controller/pcie-rcar.c | 45 +++++++++++-------------------
> > >  1 file changed, 16 insertions(+), 29 deletions(-)
> > >
> > > diff --git a/drivers/pci/controller/pcie-rcar.c b/drivers/pci/controller/pcie-rcar.c
> > > index b8d6e86a5539..453c931aaf77 100644
> > > --- a/drivers/pci/controller/pcie-rcar.c
> > > +++ b/drivers/pci/controller/pcie-rcar.c
> > > @@ -1014,16 +1014,16 @@ static int rcar_pcie_get_resources(struct rcar_pcie *pcie)
> > >  }
> > >
> > >  static int rcar_pcie_inbound_ranges(struct rcar_pcie *pcie,
> > > -                                 struct of_pci_range *range,
> > > +                                 struct resource_entry *entry,
> > >                                   int *index)
> > >  {
> > > -     u64 restype = range->flags;
> > > -     u64 cpu_addr = range->cpu_addr;
> > > -     u64 cpu_end = range->cpu_addr + range->size;
> > > -     u64 pci_addr = range->pci_addr;
> > > +     u64 restype = entry->res->flags;
> > > +     u64 cpu_addr = entry->res->start;
> > > +     u64 cpu_end = entry->res->end;
> > > +     u64 pci_addr = entry->res->start - entry->offset;
> > >       u32 flags = LAM_64BIT | LAR_ENABLE;
> > >       u64 mask;
> > > -     u64 size;
> > > +     u64 size = resource_size(entry->res);
> > >       int idx = *index;
> > >
> > >       if (restype & IORESOURCE_PREFETCH)
> > > @@ -1037,9 +1037,7 @@ static int rcar_pcie_inbound_ranges(struct rcar_pcie *pcie,
> > >               unsigned long nr_zeros = __ffs64(cpu_addr);
> > >               u64 alignment = 1ULL << nr_zeros;
> > >
> > > -             size = min(range->size, alignment);
> > > -     } else {
> > > -             size = range->size;
> > > +             size = min(size, alignment);
> > >       }
> >
> > AFAICT the (if cpu_addr > 0) is here because the result of __ffs64 is undefined
> > if no bits are set (according to the comment). However by removing the else
> > statement we no longer guarantee that nr_zeros is defined.
> 
> You might want to read this again...
> 
> The 'if (cpu_addr > 0) {' is still there and nr_zeros is only under
> that condition. We just init 'size' instead of setting it in the else
> clause.

Ah yes, apologies for the noise, thanks for your patience.

Andrew Murray

> 
> Rob