Re: [PATCH v11 04/10] PCI: OF: Fix the conversion of IO ranges into IO resources.
On Mon, Sep 22, 2014 at 10:32 AM, Liviu Dudau wrote:
> On Sat, Sep 20, 2014 at 06:33:11PM +0100, Rob Herring wrote:
>> On 09/17/2014 08:30 PM, Liviu Dudau wrote:
>> > The ranges property for a host bridge controller in DT describes
>> > the mapping between the PCI bus address and the CPU physical address.
>> > The resources framework however expects that the IO resources start
>> > at a pseudo "port" address 0 (zero) and have a maximum size of
>> > IO_SPACE_LIMIT.
>> > The conversion from pci ranges to resources failed to take that into
>> > account.
[...]
>> > if ((range.flags & IORESOURCE_MEM) &&
>> > (range.flags & IORESOURCE_PREFETCH)) {
>> > pre_mem_pci = range.pci_addr;
>> > pre_mem_pci_sz = range.size;
>> > - of_pci_range_to_resource(&range, np, &pre_mem);
>> > + ret = of_pci_range_to_resource(&range, np, &pre_mem);
>> > pre_mem.name = "PCIv3 prefetched mem";
>> > }
>> > - }
>> >
>> > - if (!conf_mem.start || !io_mem.start ||
>> > - !non_mem.start || !pre_mem.start) {
>> > - dev_err(&pdev->dev, "missing ranges in device node\n");
>> > - return -EINVAL;
>> > + if (ret < 0) {
>> > + dev_err(&pdev->dev, "missing ranges in device
>> > node\n");
>> > + return -EINVAL;
>>
>> You should return ret rather than potentially changing the return value.
>
> I was trying to keep the existing behaviour, which was to return -EINVAL if
> the
> parsing has failed. But I can return ret and propagate the original error
> code.
A valid concern, but I checked and I believe the return from
of_pci_range_to_resource is currently -EINVAL.
>> > +int of_pci_range_to_resource(struct of_pci_range *range,
>> > + struct device_node *np, struct resource *res)
>> > +{
>> > + int err;
>> > + res->flags = range->flags;
>> > + res->parent = res->child = res->sibling = NULL;
>> > + res->name = np->full_name;
>> > +
>> > + if (res->flags & IORESOURCE_IO) {
>> > + unsigned long port = -1;
>>
>> Assigning a signed value to unsigned...
>
> Ooops, sorry about that. Removed the initialisation now.
>
>>
>> Does port need to be 64-bit on 64-bit hosts?
>
> I'm following existing APIs. Basically my function is a variant of
> __of_address_to_resource()
Okay.
Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v11 04/10] PCI: OF: Fix the conversion of IO ranges into IO resources.
On Sat, Sep 20, 2014 at 06:33:11PM +0100, Rob Herring wrote:
> On 09/17/2014 08:30 PM, Liviu Dudau wrote:
> > The ranges property for a host bridge controller in DT describes
> > the mapping between the PCI bus address and the CPU physical address.
> > The resources framework however expects that the IO resources start
> > at a pseudo "port" address 0 (zero) and have a maximum size of
> > IO_SPACE_LIMIT.
> > The conversion from pci ranges to resources failed to take that into
> > account.
> >
> > In the process move the function into drivers/of/address.c as it now
> > depends on pci_address_to_pio() code and make it return an error code.
> > Also fix all the drivers that depend on the old behaviour by fetching
> > the CPU physical address based on the port number.
> >
> > Cc: Grant Likely
> > Cc: Rob Herring
> > Cc: Arnd Bergmann
> > Cc: Linus Walleij
> > Cc: Thierry Reding
> > Cc: Simon Horman
> > Cc: Catalin Marinas
> > Signed-off-by: Liviu Dudau
>
> A few minor things below.
>
> > ---
> > arch/arm/mach-integrator/pci_v3.c | 23 ++--
> > drivers/of/address.c | 46
> > +++
> > drivers/pci/host/pci-tegra.c | 10 ++---
> > drivers/pci/host/pcie-rcar.c | 21 +-
> > include/linux/of_address.h| 13 ++-
> > 5 files changed, 82 insertions(+), 31 deletions(-)
> >
> > diff --git a/arch/arm/mach-integrator/pci_v3.c
> > b/arch/arm/mach-integrator/pci_v3.c
> > index 05e1f73..3321e1b 100644
> > --- a/arch/arm/mach-integrator/pci_v3.c
> > +++ b/arch/arm/mach-integrator/pci_v3.c
> > @@ -660,6 +660,7 @@ static void __init pci_v3_preinit(void)
> > {
> > unsigned long flags;
> > unsigned int temp;
> > + phys_addr_t io_address = pci_pio_to_address(io_mem.start);
> >
> > pcibios_min_mem = 0x0010;
> >
> > @@ -701,7 +702,7 @@ static void __init pci_v3_preinit(void)
> > /*
> >* Setup window 2 - PCI IO
> >*/
> > - v3_writel(V3_LB_BASE2, v3_addr_to_lb_base2(io_mem.start) |
> > + v3_writel(V3_LB_BASE2, v3_addr_to_lb_base2(io_address) |
> > V3_LB_BASE_ENABLE);
> > v3_writew(V3_LB_MAP2, v3_addr_to_lb_map2(0));
> >
> > @@ -742,6 +743,7 @@ static void __init pci_v3_preinit(void)
> > static void __init pci_v3_postinit(void)
> > {
> > unsigned int pci_cmd;
> > + phys_addr_t io_address = pci_pio_to_address(io_mem.start);
> >
> > pci_cmd = PCI_COMMAND_MEMORY |
> > PCI_COMMAND_MASTER | PCI_COMMAND_INVALIDATE;
> > @@ -758,7 +760,7 @@ static void __init pci_v3_postinit(void)
> > "interrupt: %d\n", ret);
> > #endif
> >
> > - register_isa_ports(non_mem.start, io_mem.start, 0);
> > + register_isa_ports(non_mem.start, io_address, 0);
> > }
> >
> > /*
> > @@ -867,33 +869,32 @@ static int __init pci_v3_probe(struct platform_device
> > *pdev)
> >
> > for_each_of_pci_range(&parser, &range) {
> > if (!range.flags) {
> > - of_pci_range_to_resource(&range, np, &conf_mem);
> > + ret = of_pci_range_to_resource(&range, np, &conf_mem);
> > conf_mem.name = "PCIv3 config";
> > }
> > if (range.flags & IORESOURCE_IO) {
> > - of_pci_range_to_resource(&range, np, &io_mem);
> > + ret = of_pci_range_to_resource(&range, np, &io_mem);
> > io_mem.name = "PCIv3 I/O";
> > }
> > if ((range.flags & IORESOURCE_MEM) &&
> > !(range.flags & IORESOURCE_PREFETCH)) {
> > non_mem_pci = range.pci_addr;
> > non_mem_pci_sz = range.size;
> > - of_pci_range_to_resource(&range, np, &non_mem);
> > + ret = of_pci_range_to_resource(&range, np, &non_mem);
> > non_mem.name = "PCIv3 non-prefetched mem";
> > }
> > if ((range.flags & IORESOURCE_MEM) &&
> > (range.flags & IORESOURCE_PREFETCH)) {
> > pre_mem_pci = range.pci_addr;
> > pre_mem_pci_sz = range.size;
> > - of_pci_range_to_resource(&range, np, &pre_mem);
> > + ret = of_pci_range_to_resource(&range, np, &pre_mem);
> > pre_mem.name = "PCIv3 prefetched mem";
> > }
> > - }
> >
> > - if (!conf_mem.start || !io_mem.start ||
> > - !non_mem.start || !pre_mem.start) {
> > - dev_err(&pdev->dev, "missing ranges in device node\n");
> > - return -EINVAL;
> > + if (ret < 0) {
> > + dev_err(&pdev->dev, "missing ranges in device
> > node\n");
> > + return -EINVAL;
>
> You should return ret rather than potentially changing the return value.
I was trying to keep the existing behaviour, which was t
Re: [PATCH v11 04/10] PCI: OF: Fix the conversion of IO ranges into IO resources.
On 09/17/2014 08:30 PM, Liviu Dudau wrote:
> The ranges property for a host bridge controller in DT describes
> the mapping between the PCI bus address and the CPU physical address.
> The resources framework however expects that the IO resources start
> at a pseudo "port" address 0 (zero) and have a maximum size of IO_SPACE_LIMIT.
> The conversion from pci ranges to resources failed to take that into account.
>
> In the process move the function into drivers/of/address.c as it now
> depends on pci_address_to_pio() code and make it return an error code.
> Also fix all the drivers that depend on the old behaviour by fetching
> the CPU physical address based on the port number.
>
> Cc: Grant Likely
> Cc: Rob Herring
> Cc: Arnd Bergmann
> Cc: Linus Walleij
> Cc: Thierry Reding
> Cc: Simon Horman
> Cc: Catalin Marinas
> Signed-off-by: Liviu Dudau
A few minor things below.
> ---
> arch/arm/mach-integrator/pci_v3.c | 23 ++--
> drivers/of/address.c | 46
> +++
> drivers/pci/host/pci-tegra.c | 10 ++---
> drivers/pci/host/pcie-rcar.c | 21 +-
> include/linux/of_address.h| 13 ++-
> 5 files changed, 82 insertions(+), 31 deletions(-)
>
> diff --git a/arch/arm/mach-integrator/pci_v3.c
> b/arch/arm/mach-integrator/pci_v3.c
> index 05e1f73..3321e1b 100644
> --- a/arch/arm/mach-integrator/pci_v3.c
> +++ b/arch/arm/mach-integrator/pci_v3.c
> @@ -660,6 +660,7 @@ static void __init pci_v3_preinit(void)
> {
> unsigned long flags;
> unsigned int temp;
> + phys_addr_t io_address = pci_pio_to_address(io_mem.start);
>
> pcibios_min_mem = 0x0010;
>
> @@ -701,7 +702,7 @@ static void __init pci_v3_preinit(void)
> /*
>* Setup window 2 - PCI IO
>*/
> - v3_writel(V3_LB_BASE2, v3_addr_to_lb_base2(io_mem.start) |
> + v3_writel(V3_LB_BASE2, v3_addr_to_lb_base2(io_address) |
> V3_LB_BASE_ENABLE);
> v3_writew(V3_LB_MAP2, v3_addr_to_lb_map2(0));
>
> @@ -742,6 +743,7 @@ static void __init pci_v3_preinit(void)
> static void __init pci_v3_postinit(void)
> {
> unsigned int pci_cmd;
> + phys_addr_t io_address = pci_pio_to_address(io_mem.start);
>
> pci_cmd = PCI_COMMAND_MEMORY |
> PCI_COMMAND_MASTER | PCI_COMMAND_INVALIDATE;
> @@ -758,7 +760,7 @@ static void __init pci_v3_postinit(void)
> "interrupt: %d\n", ret);
> #endif
>
> - register_isa_ports(non_mem.start, io_mem.start, 0);
> + register_isa_ports(non_mem.start, io_address, 0);
> }
>
> /*
> @@ -867,33 +869,32 @@ static int __init pci_v3_probe(struct platform_device
> *pdev)
>
> for_each_of_pci_range(&parser, &range) {
> if (!range.flags) {
> - of_pci_range_to_resource(&range, np, &conf_mem);
> + ret = of_pci_range_to_resource(&range, np, &conf_mem);
> conf_mem.name = "PCIv3 config";
> }
> if (range.flags & IORESOURCE_IO) {
> - of_pci_range_to_resource(&range, np, &io_mem);
> + ret = of_pci_range_to_resource(&range, np, &io_mem);
> io_mem.name = "PCIv3 I/O";
> }
> if ((range.flags & IORESOURCE_MEM) &&
> !(range.flags & IORESOURCE_PREFETCH)) {
> non_mem_pci = range.pci_addr;
> non_mem_pci_sz = range.size;
> - of_pci_range_to_resource(&range, np, &non_mem);
> + ret = of_pci_range_to_resource(&range, np, &non_mem);
> non_mem.name = "PCIv3 non-prefetched mem";
> }
> if ((range.flags & IORESOURCE_MEM) &&
> (range.flags & IORESOURCE_PREFETCH)) {
> pre_mem_pci = range.pci_addr;
> pre_mem_pci_sz = range.size;
> - of_pci_range_to_resource(&range, np, &pre_mem);
> + ret = of_pci_range_to_resource(&range, np, &pre_mem);
> pre_mem.name = "PCIv3 prefetched mem";
> }
> - }
>
> - if (!conf_mem.start || !io_mem.start ||
> - !non_mem.start || !pre_mem.start) {
> - dev_err(&pdev->dev, "missing ranges in device node\n");
> - return -EINVAL;
> + if (ret < 0) {
> + dev_err(&pdev->dev, "missing ranges in device node\n");
> + return -EINVAL;
You should return ret rather than potentially changing the return value.
> + }
> }
>
> pci_v3.map_irq = of_irq_parse_and_map_pci;
> diff --git a/drivers/of/address.c b/drivers/of/address.c
> index 2373a92..ff10b64 100644
> --- a/drivers/of/address.c
> +++ b/drivers/of/address.c
> @@ -947,3 +947,49 @@ bool of_dma_is_coherent(struct device_node *np)
> return false;
> }
> EXPO
Re: [PATCH v11 04/10] PCI: OF: Fix the conversion of IO ranges into IO resources.
On Thu, Sep 18, 2014 at 02:30:19AM +0100, Liviu Dudau wrote: > The ranges property for a host bridge controller in DT describes > the mapping between the PCI bus address and the CPU physical address. > The resources framework however expects that the IO resources start > at a pseudo "port" address 0 (zero) and have a maximum size of IO_SPACE_LIMIT. > The conversion from pci ranges to resources failed to take that into account. > > In the process move the function into drivers/of/address.c as it now > depends on pci_address_to_pio() code and make it return an error code. I think you're talking about of_pci_range_to_resource(). Can you split this into one patch that moves it from of_address.h to of/address.c without changing its functionality, and a second one that does the actual change? Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v11 04/10] PCI: OF: Fix the conversion of IO ranges into IO resources.
The ranges property for a host bridge controller in DT describes
the mapping between the PCI bus address and the CPU physical address.
The resources framework however expects that the IO resources start
at a pseudo "port" address 0 (zero) and have a maximum size of IO_SPACE_LIMIT.
The conversion from pci ranges to resources failed to take that into account.
In the process move the function into drivers/of/address.c as it now
depends on pci_address_to_pio() code and make it return an error code.
Also fix all the drivers that depend on the old behaviour by fetching
the CPU physical address based on the port number.
Cc: Grant Likely
Cc: Rob Herring
Cc: Arnd Bergmann
Cc: Linus Walleij
Cc: Thierry Reding
Cc: Simon Horman
Cc: Catalin Marinas
Signed-off-by: Liviu Dudau
---
arch/arm/mach-integrator/pci_v3.c | 23 ++--
drivers/of/address.c | 46 +++
drivers/pci/host/pci-tegra.c | 10 ++---
drivers/pci/host/pcie-rcar.c | 21 +-
include/linux/of_address.h| 13 ++-
5 files changed, 82 insertions(+), 31 deletions(-)
diff --git a/arch/arm/mach-integrator/pci_v3.c
b/arch/arm/mach-integrator/pci_v3.c
index 05e1f73..3321e1b 100644
--- a/arch/arm/mach-integrator/pci_v3.c
+++ b/arch/arm/mach-integrator/pci_v3.c
@@ -660,6 +660,7 @@ static void __init pci_v3_preinit(void)
{
unsigned long flags;
unsigned int temp;
+ phys_addr_t io_address = pci_pio_to_address(io_mem.start);
pcibios_min_mem = 0x0010;
@@ -701,7 +702,7 @@ static void __init pci_v3_preinit(void)
/*
* Setup window 2 - PCI IO
*/
- v3_writel(V3_LB_BASE2, v3_addr_to_lb_base2(io_mem.start) |
+ v3_writel(V3_LB_BASE2, v3_addr_to_lb_base2(io_address) |
V3_LB_BASE_ENABLE);
v3_writew(V3_LB_MAP2, v3_addr_to_lb_map2(0));
@@ -742,6 +743,7 @@ static void __init pci_v3_preinit(void)
static void __init pci_v3_postinit(void)
{
unsigned int pci_cmd;
+ phys_addr_t io_address = pci_pio_to_address(io_mem.start);
pci_cmd = PCI_COMMAND_MEMORY |
PCI_COMMAND_MASTER | PCI_COMMAND_INVALIDATE;
@@ -758,7 +760,7 @@ static void __init pci_v3_postinit(void)
"interrupt: %d\n", ret);
#endif
- register_isa_ports(non_mem.start, io_mem.start, 0);
+ register_isa_ports(non_mem.start, io_address, 0);
}
/*
@@ -867,33 +869,32 @@ static int __init pci_v3_probe(struct platform_device
*pdev)
for_each_of_pci_range(&parser, &range) {
if (!range.flags) {
- of_pci_range_to_resource(&range, np, &conf_mem);
+ ret = of_pci_range_to_resource(&range, np, &conf_mem);
conf_mem.name = "PCIv3 config";
}
if (range.flags & IORESOURCE_IO) {
- of_pci_range_to_resource(&range, np, &io_mem);
+ ret = of_pci_range_to_resource(&range, np, &io_mem);
io_mem.name = "PCIv3 I/O";
}
if ((range.flags & IORESOURCE_MEM) &&
!(range.flags & IORESOURCE_PREFETCH)) {
non_mem_pci = range.pci_addr;
non_mem_pci_sz = range.size;
- of_pci_range_to_resource(&range, np, &non_mem);
+ ret = of_pci_range_to_resource(&range, np, &non_mem);
non_mem.name = "PCIv3 non-prefetched mem";
}
if ((range.flags & IORESOURCE_MEM) &&
(range.flags & IORESOURCE_PREFETCH)) {
pre_mem_pci = range.pci_addr;
pre_mem_pci_sz = range.size;
- of_pci_range_to_resource(&range, np, &pre_mem);
+ ret = of_pci_range_to_resource(&range, np, &pre_mem);
pre_mem.name = "PCIv3 prefetched mem";
}
- }
- if (!conf_mem.start || !io_mem.start ||
- !non_mem.start || !pre_mem.start) {
- dev_err(&pdev->dev, "missing ranges in device node\n");
- return -EINVAL;
+ if (ret < 0) {
+ dev_err(&pdev->dev, "missing ranges in device node\n");
+ return -EINVAL;
+ }
}
pci_v3.map_irq = of_irq_parse_and_map_pci;
diff --git a/drivers/of/address.c b/drivers/of/address.c
index 2373a92..ff10b64 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -947,3 +947,49 @@ bool of_dma_is_coherent(struct device_node *np)
return false;
}
EXPORT_SYMBOL_GPL(of_dma_is_coherent);
+
+/*
+ * of_pci_range_to_resource - Create a resource from an of_pci_range
+ * @range: the PCI range that describes the resource
+ * @np:device node where the range belongs to
+ * @res: pointer to a valid re
