| Message ID | 20190924214630.12817-12-robh@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | PCI dma-ranges parsing consolidation | expand |
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. > /* Hardware supports max 4GiB inbound region */ > size = min(size, 1ULL << 32); > @@ -1078,30 +1076,19 @@ static int rcar_pcie_inbound_ranges(struct rcar_pcie *pcie, > return 0; > } > > -static int rcar_pcie_parse_map_dma_ranges(struct rcar_pcie *pcie, > - struct device_node *np) > +static int rcar_pcie_parse_map_dma_ranges(struct rcar_pcie *pcie) > { > - struct of_pci_range range; > - struct of_pci_range_parser parser; > - int index = 0; > - int err; > - > - if (of_pci_dma_range_parser_init(&parser, np)) > - return -EINVAL; > - > - /* Get the dma-ranges from DT */ > - for_each_of_pci_range(&parser, &range) { > - u64 end = range.cpu_addr + range.size - 1; > - > - dev_dbg(pcie->dev, "0x%08x 0x%016llx..0x%016llx -> 0x%016llx\n", > - range.flags, range.cpu_addr, end, range.pci_addr); > + struct pci_host_bridge *bridge = pci_host_bridge_from_priv(pcie); > + struct resource_entry *entry; > + int index = 0, err = 0; > > - err = rcar_pcie_inbound_ranges(pcie, &range, &index); > + resource_list_for_each_entry(entry, &bridge->dma_ranges) { > + err = rcar_pcie_inbound_ranges(pcie, entry, &index); > if (err) > - return err; > + break; > } > > - return 0; > + return err; > } > > static const struct of_device_id rcar_pcie_of_match[] = { > @@ -1162,7 +1149,7 @@ static int rcar_pcie_probe(struct platform_device *pdev) > goto err_unmap_msi_irqs; > } > > - err = rcar_pcie_parse_map_dma_ranges(pcie, dev->of_node); > + err = rcar_pcie_parse_map_dma_ranges(pcie); > if (err) > goto err_clk_disable; > > -- > 2.20.1 >
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. Rob
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
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); } /* Hardware supports max 4GiB inbound region */ size = min(size, 1ULL << 32); @@ -1078,30 +1076,19 @@ static int rcar_pcie_inbound_ranges(struct rcar_pcie *pcie, return 0; } -static int rcar_pcie_parse_map_dma_ranges(struct rcar_pcie *pcie, - struct device_node *np) +static int rcar_pcie_parse_map_dma_ranges(struct rcar_pcie *pcie) { - struct of_pci_range range; - struct of_pci_range_parser parser; - int index = 0; - int err; - - if (of_pci_dma_range_parser_init(&parser, np)) - return -EINVAL; - - /* Get the dma-ranges from DT */ - for_each_of_pci_range(&parser, &range) { - u64 end = range.cpu_addr + range.size - 1; - - dev_dbg(pcie->dev, "0x%08x 0x%016llx..0x%016llx -> 0x%016llx\n", - range.flags, range.cpu_addr, end, range.pci_addr); + struct pci_host_bridge *bridge = pci_host_bridge_from_priv(pcie); + struct resource_entry *entry; + int index = 0, err = 0; - err = rcar_pcie_inbound_ranges(pcie, &range, &index); + resource_list_for_each_entry(entry, &bridge->dma_ranges) { + err = rcar_pcie_inbound_ranges(pcie, entry, &index); if (err) - return err; + break; } - return 0; + return err; } static const struct of_device_id rcar_pcie_of_match[] = { @@ -1162,7 +1149,7 @@ static int rcar_pcie_probe(struct platform_device *pdev) goto err_unmap_msi_irqs; } - err = rcar_pcie_parse_map_dma_ranges(pcie, dev->of_node); + err = rcar_pcie_parse_map_dma_ranges(pcie); if (err) goto err_clk_disable;
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(-)