| Message ID | 20190924214630.12817-6-robh@kernel.org |
|---|---|
| State | Superseded |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | PCI dma-ranges parsing consolidation | expand |
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. Thanks, Andrew Murray > u32 val; > void __iomem *local_pci_cfg_base; > struct pci_bus *bus, *child; > @@ -141,10 +93,18 @@ static int versatile_pci_probe(struct platform_device *pdev) > if (IS_ERR(versatile_cfg_base[1])) > return PTR_ERR(versatile_cfg_base[1]); > > - ret = versatile_pci_parse_request_of_pci_ranges(dev, &pci_res); > + ret = pci_parse_request_of_pci_ranges(dev, &pci_res, NULL); > if (ret) > return ret; > > + resource_list_for_each_entry(entry, &pci_res) { > + if (resource_type(entry->res) == IORESOURCE_MEM) { > + writel(entry->res->start >> 28, PCI_IMAP(mem)); > + writel(PHYS_OFFSET >> 28, PCI_SMAP(mem)); > + mem++; > + } > + } > + > /* > * We need to discover the PCI core first to configure itself > * before the main PCI probing is performed > -- > 2.20.1 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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... Rob
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
On Thu, 26 Sep 2019 at 22:45, Rob Herring <robh@kernel.org> 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> > > > --- > > > 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... Yes, QEMU users will notice if you drop or break PCI support :-) I don't think anybody is using real hardware PCI though. Anyway, the 'mem' indexes here matter because you're passing them to PCI_IMAP() and PCI_SMAP(), which are writing to hardware registers. If you write to PCI_IMAP0 when we were previously writing to PCI_IMAP1 then suddenly you're not configuring the behaviour for accesses to the PCI window that's at CPU physaddr 0x50000000, you're configuring the window that's at CPU physaddr 0x44000000, which is entirely different (and notably is smaller, being only 0x0c000000 in size rather than 0x10000000). If this is supposed to be a no-behaviour-change refactor then it would probably be a good test to check that we're writing exactly the same values to the hardware registers on the device as we were before the change. thanks -- PMM
On Mon, Sep 30, 2019 at 05:56:51PM +0100, Peter Maydell wrote: > On Thu, 26 Sep 2019 at 22:45, Rob Herring <robh@kernel.org> 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> > > > > --- > > > > > 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... > > Yes, QEMU users will notice if you drop or break PCI support :-) > I don't think anybody is using real hardware PCI though. > > Anyway, the 'mem' indexes here matter because you're passing > them to PCI_IMAP() and PCI_SMAP(), which are writing to > hardware registers. If you write to PCI_IMAP0 when we > were previously writing to PCI_IMAP1 then suddenly you're > not configuring the behaviour for accesses to the PCI > window that's at CPU physaddr 0x50000000, you're configuring > the window that's at CPU physaddr 0x44000000, which is > entirely different (and notably is smaller, being only > 0x0c000000 in size rather than 0x10000000). > > If this is supposed to be a no-behaviour-change refactor > then it would probably be a good test to check that we're > writing exactly the same values to the hardware registers > on the device as we were before the change. As far as I understand... According to the device tree arch/arm/boot/dts/versatile-pb.dts we describe a 1:1 mapping between CPU and PCI addresses for the IORESOURCE_MEM resources: ranges = <0x01000000 0 0x00000000 0x43000000 0 0x00010000 /* downstream I/O */ 0x02000000 0 0x50000000 0x50000000 0 0x10000000 /* non-prefetchable memory */ 0x42000000 0 0x60000000 0x60000000 0 0x10000000>; /* prefetchable memory */ The existing code achieves this by shifting the CPU address and writing 0x5 to PCI_IMAP(1) and 0x6 >> 28 to PCI_IMAP(2). This value represents the top 4 bits of the outgoing PCI address, with the remainder of the bits as written to the AHB window. The hardware has three windows at 0x44000000, 0x50000000 and 0x60000000 which relate to PCI_IMAP0, 1 and 2 respectively. Therefore the existing code creates an effective 1:1 mapping as follows: CPU 0x50000000 => PCI 0x50000000 CPU 0x60000000 => PCI 0x60000000 If we were to instead write 0x5 to PCI_IMAP(0) and 0x6 to PCI_IMAP(1), as per this patch - then we end up with an effective broken mapping of: CPU 0x50000000 => PCI 0x60000000 CPU 0x60000000 => PCI unset Therefore I'd suggest we preserve the existing numbering and change mem back to 1. More information about the hardware can be foud here: http://arminfo.emea.arm.com/help/index.jsp?topic=/com.arm.doc.dui0224i/Bbajjbce.html Thanks, Andrew Murray > > thanks > -- PMM
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; u32 val; void __iomem *local_pci_cfg_base; struct pci_bus *bus, *child; @@ -141,10 +93,18 @@ static int versatile_pci_probe(struct platform_device *pdev) if (IS_ERR(versatile_cfg_base[1])) return PTR_ERR(versatile_cfg_base[1]); - ret = versatile_pci_parse_request_of_pci_ranges(dev, &pci_res); + ret = pci_parse_request_of_pci_ranges(dev, &pci_res, NULL); if (ret) return ret; + resource_list_for_each_entry(entry, &pci_res) { + if (resource_type(entry->res) == IORESOURCE_MEM) { + writel(entry->res->start >> 28, PCI_IMAP(mem)); + writel(PHYS_OFFSET >> 28, PCI_SMAP(mem)); + mem++; + } + } + /* * We need to discover the PCI core first to configure itself * before the main PCI probing is performed
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(-)