| Message ID | 20190924214630.12817-3-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:21PM -0500, Rob Herring wrote: > Convert altera host bridge to use the common > pci_parse_request_of_pci_ranges(). > > Cc: Ley Foon Tan <lftan@altera.com> > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > Cc: Bjorn Helgaas <bhelgaas@google.com> > Cc: rfi@lists.rocketboards.org > Signed-off-by: Rob Herring <robh@kernel.org> > --- > drivers/pci/controller/pcie-altera.c | 38 ++-------------------------- > 1 file changed, 2 insertions(+), 36 deletions(-) > > diff --git a/drivers/pci/controller/pcie-altera.c b/drivers/pci/controller/pcie-altera.c > index d2497ca43828..2ed00babff5a 100644 > --- a/drivers/pci/controller/pcie-altera.c > +++ b/drivers/pci/controller/pcie-altera.c > @@ -670,39 +670,6 @@ static void altera_pcie_isr(struct irq_desc *desc) > chained_irq_exit(chip, desc); > } > > -static int altera_pcie_parse_request_of_pci_ranges(struct altera_pcie *pcie) > -{ > - int err, res_valid = 0; > - struct device *dev = &pcie->pdev->dev; > - struct resource_entry *win; > - > - err = devm_of_pci_get_host_bridge_resources(dev, 0, 0xff, > - &pcie->resources, NULL); > - if (err) > - return err; > - > - err = devm_request_pci_bus_resources(dev, &pcie->resources); > - if (err) > - goto out_release_res; > - > - resource_list_for_each_entry(win, &pcie->resources) { > - struct resource *res = win->res; > - > - if (resource_type(res) == IORESOURCE_MEM) > - res_valid |= !(res->flags & IORESOURCE_PREFETCH); > - } > - > - if (res_valid) > - return 0; > - > - dev_err(dev, "non-prefetchable memory resource required\n"); > - err = -EINVAL; > - > -out_release_res: > - pci_free_resource_list(&pcie->resources); > - return err; > -} > - > static int altera_pcie_init_irq_domain(struct altera_pcie *pcie) > { > struct device *dev = &pcie->pdev->dev; > @@ -833,9 +800,8 @@ static int altera_pcie_probe(struct platform_device *pdev) > return ret; > } > > - INIT_LIST_HEAD(&pcie->resources); > - > - ret = altera_pcie_parse_request_of_pci_ranges(pcie); > + ret = pci_parse_request_of_pci_ranges(dev, &pcie->resources, Does it matter that we now map any given IO ranges whereas we didn't previously? As far as I can tell there are no users that pass an IO range, if they did then with the existing code the probe would fail and they'd get a "I/O range found for %pOF. Please provide an io_base pointer...". However with the new code if any IO range was given (which would probably represent a misconfiguration), then we'd proceed to map the IO range. When that IO is used, who knows what would happen. I wonder if there is a better way for a host driver to indicate that it doesn't support IO? Thanks, Andrew Murray > + NULL); > if (ret) { > dev_err(dev, "Failed add resources\n"); > return ret; > -- > 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:24 AM Andrew Murray <andrew.murray@arm.com> wrote: > > On Tue, Sep 24, 2019 at 04:46:21PM -0500, Rob Herring wrote: > > Convert altera host bridge to use the common > > pci_parse_request_of_pci_ranges(). > > > > Cc: Ley Foon Tan <lftan@altera.com> > > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > > Cc: Bjorn Helgaas <bhelgaas@google.com> > > Cc: rfi@lists.rocketboards.org > > Signed-off-by: Rob Herring <robh@kernel.org> > > --- > > @@ -833,9 +800,8 @@ static int altera_pcie_probe(struct platform_device *pdev) > > return ret; > > } > > > > - INIT_LIST_HEAD(&pcie->resources); > > - > > - ret = altera_pcie_parse_request_of_pci_ranges(pcie); > > + ret = pci_parse_request_of_pci_ranges(dev, &pcie->resources, > > Does it matter that we now map any given IO ranges whereas we didn't > previously? > > As far as I can tell there are no users that pass an IO range, if they > did then with the existing code the probe would fail and they'd get > a "I/O range found for %pOF. Please provide an io_base pointer...". > However with the new code if any IO range was given (which would > probably represent a misconfiguration), then we'd proceed to map the > IO range. When that IO is used, who knows what would happen. Yeah, I'm assuming that the DT doesn't have an IO range if IO is not supported. IMO, it is not the kernel's job to validate the DT. > I wonder if there is a better way for a host driver to indicate that > it doesn't support IO? We can probably test for this in the schema. ranges: items: minItems: 7 items: - not: { const: 0x01000000 } Or "- enum: [ 0x42000000, 0x02000000 ]" Of course, in theory, the bus, dev, fn fields could be non-zero and we could use minium/maximum to handle those, but in practice I think they are rarely used for FDT. Rob
On Wed, Sep 25, 2019 at 07:33:35AM -0500, Rob Herring wrote: > On Wed, Sep 25, 2019 at 5:24 AM Andrew Murray <andrew.murray@arm.com> wrote: > > > > On Tue, Sep 24, 2019 at 04:46:21PM -0500, Rob Herring wrote: > > > Convert altera host bridge to use the common > > > pci_parse_request_of_pci_ranges(). > > > > > > Cc: Ley Foon Tan <lftan@altera.com> > > > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > > > Cc: Bjorn Helgaas <bhelgaas@google.com> > > > Cc: rfi@lists.rocketboards.org > > > Signed-off-by: Rob Herring <robh@kernel.org> > > > --- > > > > @@ -833,9 +800,8 @@ static int altera_pcie_probe(struct platform_device *pdev) > > > return ret; > > > } > > > > > > - INIT_LIST_HEAD(&pcie->resources); > > > - > > > - ret = altera_pcie_parse_request_of_pci_ranges(pcie); > > > + ret = pci_parse_request_of_pci_ranges(dev, &pcie->resources, > > > > Does it matter that we now map any given IO ranges whereas we didn't > > previously? > > > > As far as I can tell there are no users that pass an IO range, if they > > did then with the existing code the probe would fail and they'd get > > a "I/O range found for %pOF. Please provide an io_base pointer...". > > However with the new code if any IO range was given (which would > > probably represent a misconfiguration), then we'd proceed to map the > > IO range. When that IO is used, who knows what would happen. > > Yeah, I'm assuming that the DT doesn't have an IO range if IO is not > supported. IMO, it is not the kernel's job to validate the DT. Sure. Is it worth mentioning in the commit message this subtle change in behaviour? > > > I wonder if there is a better way for a host driver to indicate that > > it doesn't support IO? > > We can probably test for this in the schema. > > ranges: > items: > minItems: 7 > items: > - not: { const: 0x01000000 } > > Or "- enum: [ 0x42000000, 0x02000000 ]" > > Of course, in theory, the bus, dev, fn fields could be non-zero and we > could use minium/maximum to handle those, but in practice I think they > are rarely used for FDT. Many controllers also appear to set the top bit (relocatable), e.g. 0x82000000... At present there are no PCI bindings that use the YAML schema, if I've understood correctly. Thanks, Andrew Murray > > Rob
On Mon, Sep 30, 2019 at 10:13 AM Andrew Murray <andrew.murray@arm.com> wrote: > > On Wed, Sep 25, 2019 at 07:33:35AM -0500, Rob Herring wrote: > > On Wed, Sep 25, 2019 at 5:24 AM Andrew Murray <andrew.murray@arm.com> wrote: > > > > > > On Tue, Sep 24, 2019 at 04:46:21PM -0500, Rob Herring wrote: > > > > Convert altera host bridge to use the common > > > > pci_parse_request_of_pci_ranges(). > > > > > > > > Cc: Ley Foon Tan <lftan@altera.com> > > > > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > > > > Cc: Bjorn Helgaas <bhelgaas@google.com> > > > > Cc: rfi@lists.rocketboards.org > > > > Signed-off-by: Rob Herring <robh@kernel.org> > > > > --- > > > > > > @@ -833,9 +800,8 @@ static int altera_pcie_probe(struct platform_device *pdev) > > > > return ret; > > > > } > > > > > > > > - INIT_LIST_HEAD(&pcie->resources); > > > > - > > > > - ret = altera_pcie_parse_request_of_pci_ranges(pcie); > > > > + ret = pci_parse_request_of_pci_ranges(dev, &pcie->resources, > > > > > > Does it matter that we now map any given IO ranges whereas we didn't > > > previously? > > > > > > As far as I can tell there are no users that pass an IO range, if they > > > did then with the existing code the probe would fail and they'd get > > > a "I/O range found for %pOF. Please provide an io_base pointer...". > > > However with the new code if any IO range was given (which would > > > probably represent a misconfiguration), then we'd proceed to map the > > > IO range. When that IO is used, who knows what would happen. > > > > Yeah, I'm assuming that the DT doesn't have an IO range if IO is not > > supported. IMO, it is not the kernel's job to validate the DT. > > Sure. Is it worth mentioning in the commit message this subtle change > in behaviour? Will do. > > > I wonder if there is a better way for a host driver to indicate that > > > it doesn't support IO? > > > > We can probably test for this in the schema. > > > > ranges: > > items: > > minItems: 7 > > items: > > - not: { const: 0x01000000 } > > > > Or "- enum: [ 0x42000000, 0x02000000 ]" > > > > Of course, in theory, the bus, dev, fn fields could be non-zero and we > > could use minium/maximum to handle those, but in practice I think they > > are rarely used for FDT. > > Many controllers also appear to set the top bit (relocatable), e.g. > 0x82000000... That begs the question how many should set the relocatable bit and don't... Anyways, it's still a smallish set of possible values and worthwhile to describe which ones a controller supports. > At present there are no PCI bindings that use the YAML schema, if I've > understood correctly. Probably so, there has been at least one under review. Intel LGM IIRC. We do need a common PCI schema too. Hopefully someone beats me to it. Rob
On Mon, Sep 30, 2019 at 12:36:22PM -0500, Rob Herring wrote: > On Mon, Sep 30, 2019 at 10:13 AM Andrew Murray <andrew.murray@arm.com> wrote: > > > > On Wed, Sep 25, 2019 at 07:33:35AM -0500, Rob Herring wrote: > > > On Wed, Sep 25, 2019 at 5:24 AM Andrew Murray <andrew.murray@arm.com> wrote: > > > > > > > > On Tue, Sep 24, 2019 at 04:46:21PM -0500, Rob Herring wrote: > > > > > Convert altera host bridge to use the common > > > > > pci_parse_request_of_pci_ranges(). > > > > > > > > > > Cc: Ley Foon Tan <lftan@altera.com> > > > > > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > > > > > Cc: Bjorn Helgaas <bhelgaas@google.com> > > > > > Cc: rfi@lists.rocketboards.org > > > > > Signed-off-by: Rob Herring <robh@kernel.org> > > > > > --- > > > > > > > > @@ -833,9 +800,8 @@ static int altera_pcie_probe(struct platform_device *pdev) > > > > > return ret; > > > > > } > > > > > > > > > > - INIT_LIST_HEAD(&pcie->resources); > > > > > - > > > > > - ret = altera_pcie_parse_request_of_pci_ranges(pcie); > > > > > + ret = pci_parse_request_of_pci_ranges(dev, &pcie->resources, > > > > > > > > Does it matter that we now map any given IO ranges whereas we didn't > > > > previously? > > > > > > > > As far as I can tell there are no users that pass an IO range, if they > > > > did then with the existing code the probe would fail and they'd get > > > > a "I/O range found for %pOF. Please provide an io_base pointer...". > > > > However with the new code if any IO range was given (which would > > > > probably represent a misconfiguration), then we'd proceed to map the > > > > IO range. When that IO is used, who knows what would happen. > > > > > > Yeah, I'm assuming that the DT doesn't have an IO range if IO is not > > > supported. IMO, it is not the kernel's job to validate the DT. > > > > Sure. Is it worth mentioning in the commit message this subtle change > > in behaviour? > > Will do. Hi Rob, I would like to merge this series, are you resending it ? It does not apply to v5.4-rc1, if you rebase it please also update this patch log, as per comments above (I can do it too but if you resend it there is no point). Thanks, Lorenzo
On Tue, Oct 15, 2019 at 6:03 AM Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote: > > On Mon, Sep 30, 2019 at 12:36:22PM -0500, Rob Herring wrote: > > On Mon, Sep 30, 2019 at 10:13 AM Andrew Murray <andrew.murray@arm.com> wrote: > > > > > > On Wed, Sep 25, 2019 at 07:33:35AM -0500, Rob Herring wrote: > > > > On Wed, Sep 25, 2019 at 5:24 AM Andrew Murray <andrew.murray@arm.com> wrote: > > > > > > > > > > On Tue, Sep 24, 2019 at 04:46:21PM -0500, Rob Herring wrote: > > > > > > Convert altera host bridge to use the common > > > > > > pci_parse_request_of_pci_ranges(). > > > > > > > > > > > > Cc: Ley Foon Tan <lftan@altera.com> > > > > > > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > > > > > > Cc: Bjorn Helgaas <bhelgaas@google.com> > > > > > > Cc: rfi@lists.rocketboards.org > > > > > > Signed-off-by: Rob Herring <robh@kernel.org> > > > > > > --- > > > > > > > > > > @@ -833,9 +800,8 @@ static int altera_pcie_probe(struct platform_device *pdev) > > > > > > return ret; > > > > > > } > > > > > > > > > > > > - INIT_LIST_HEAD(&pcie->resources); > > > > > > - > > > > > > - ret = altera_pcie_parse_request_of_pci_ranges(pcie); > > > > > > + ret = pci_parse_request_of_pci_ranges(dev, &pcie->resources, > > > > > > > > > > Does it matter that we now map any given IO ranges whereas we didn't > > > > > previously? > > > > > > > > > > As far as I can tell there are no users that pass an IO range, if they > > > > > did then with the existing code the probe would fail and they'd get > > > > > a "I/O range found for %pOF. Please provide an io_base pointer...". > > > > > However with the new code if any IO range was given (which would > > > > > probably represent a misconfiguration), then we'd proceed to map the > > > > > IO range. When that IO is used, who knows what would happen. > > > > > > > > Yeah, I'm assuming that the DT doesn't have an IO range if IO is not > > > > supported. IMO, it is not the kernel's job to validate the DT. > > > > > > Sure. Is it worth mentioning in the commit message this subtle change > > > in behaviour? > > > > Will do. > > Hi Rob, > > I would like to merge this series, are you resending it ? It does not > apply to v5.4-rc1, if you rebase it please also update this patch > log, as per comments above (I can do it too but if you resend it > there is no point). Yes, I plan to resend it. Rob
diff --git a/drivers/pci/controller/pcie-altera.c b/drivers/pci/controller/pcie-altera.c index d2497ca43828..2ed00babff5a 100644 --- a/drivers/pci/controller/pcie-altera.c +++ b/drivers/pci/controller/pcie-altera.c @@ -670,39 +670,6 @@ static void altera_pcie_isr(struct irq_desc *desc) chained_irq_exit(chip, desc); } -static int altera_pcie_parse_request_of_pci_ranges(struct altera_pcie *pcie) -{ - int err, res_valid = 0; - struct device *dev = &pcie->pdev->dev; - struct resource_entry *win; - - err = devm_of_pci_get_host_bridge_resources(dev, 0, 0xff, - &pcie->resources, NULL); - if (err) - return err; - - err = devm_request_pci_bus_resources(dev, &pcie->resources); - if (err) - goto out_release_res; - - resource_list_for_each_entry(win, &pcie->resources) { - struct resource *res = win->res; - - if (resource_type(res) == IORESOURCE_MEM) - res_valid |= !(res->flags & IORESOURCE_PREFETCH); - } - - if (res_valid) - return 0; - - dev_err(dev, "non-prefetchable memory resource required\n"); - err = -EINVAL; - -out_release_res: - pci_free_resource_list(&pcie->resources); - return err; -} - static int altera_pcie_init_irq_domain(struct altera_pcie *pcie) { struct device *dev = &pcie->pdev->dev; @@ -833,9 +800,8 @@ static int altera_pcie_probe(struct platform_device *pdev) return ret; } - INIT_LIST_HEAD(&pcie->resources); - - ret = altera_pcie_parse_request_of_pci_ranges(pcie); + ret = pci_parse_request_of_pci_ranges(dev, &pcie->resources, + NULL); if (ret) { dev_err(dev, "Failed add resources\n"); return ret;
Convert altera host bridge to use the common pci_parse_request_of_pci_ranges(). Cc: Ley Foon Tan <lftan@altera.com> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> Cc: Bjorn Helgaas <bhelgaas@google.com> Cc: rfi@lists.rocketboards.org Signed-off-by: Rob Herring <robh@kernel.org> --- drivers/pci/controller/pcie-altera.c | 38 ++-------------------------- 1 file changed, 2 insertions(+), 36 deletions(-)