Re: [PATCH v7 2/3] of/pci: Provide support for parsing PCI DT ranges property
On Thu, Apr 18, 2013 at 02:44:01PM +0100, Grant Likely wrote:
On Tue, 16 Apr 2013 11:18:27 +0100, Andrew Murray andrew.mur...@arm.com
wrote:
Acked-by: Grant Likely grant.lik...@secretlab.ca
But comments below...
I've updated the patchset (now v8) to reflect your feedback, after a closer
look...
-
- pr_debug(pci_space: 0x%08x pci_addr:0x%016llx ,
- pci_space, pci_addr);
- pr_debug(cpu_addr:0x%016llx size:0x%016llx\n,
- cpu_addr, size);
-
- ranges += np;
+ pr_debug(pci_space: 0x%08x pci_addr: 0x%016llx ,
+ range.pci_space, range.pci_addr);
+ pr_debug(cpu_addr: 0x%016llx size: 0x%016llx\n,
+ range.cpu_addr, range.size);
Nit: the patch changed whitespace on the pr_debug() statements, so even
though the first line of each is identical, they look different in the
patch.
Actually the first line isn't identical, the original file was inconsistent
with its use of spaces between ':' and '0x%0' - my patch ensured that there
was always a space. I guess this could have been done as a separate patch.
/* If we failed translation or got a zero-sized region
* (some FW try to feed us with non sensical zero sized regions
* such as power3 which look like some kind of attempt
* at exposing the VGA memory hole)
*/
- if (cpu_addr == OF_BAD_ADDR || size == 0)
+ if (range.cpu_addr == OF_BAD_ADDR || range.size == 0)
continue;
Can this also be rolled into the parsing iterator?
I decided not to do this. Mainly because ARM drivers use the parser directly
(instead of pci_process_bridge_OF_ranges function) and it seemed perfectly
valid for the parser to return a range of size 0 if that is what was present in
the DT.
- /* Now consume following elements while they are contiguous */
- for (; rlen = np * sizeof(u32);
-ranges += np, rlen -= np * 4) {
- if (ranges[0] != pci_space)
- break;
- pci_next = of_read_number(ranges + 1, 2);
- cpu_next = of_translate_address(dev, ranges + 3);
- if (pci_next != pci_addr + size ||
- cpu_next != cpu_addr + size)
- break;
- size += of_read_number(ranges + pna + 3, 2);
- }
-
/* Act based on address space type */
res = NULL;
- switch ((pci_space 24) 0x3) {
- case 1: /* PCI IO space */
+ res_type = range.flags IORESOURCE_TYPE_BITS;
+ if (res_type == IORESOURCE_IO) {
Why the change from switch() to an if/else if sequence?
I think this was an artifact of the patches evolution, I've reverted back to
the switch.
But those are mostly nitpicks. If this is deferred to v3.10 then I would
suggest fixing them up and posting for another round of review.
Andrew Murray
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v7 2/3] of/pci: Provide support for parsing PCI DT ranges property
On Tue, 16 Apr 2013 11:18:27 +0100, Andrew Murray andrew.mur...@arm.com wrote:
This patch factors out common implementation patterns to reduce overall kernel
code and provide a means for host bridge drivers to directly obtain struct
resources from the DT's ranges property without relying on architecture
specific
DT handling. This will make it easier to write archiecture independent host
bridge
drivers and mitigate against further duplication of DT parsing code.
This patch can be used in the following way:
struct of_pci_range_parser parser;
struct of_pci_range range;
if (of_pci_range_parser(parser, np))
; //no ranges property
for_each_of_pci_range(parser, range) {
/*
directly access properties of the address range, e.g.:
range.pci_space, range.pci_addr, range.cpu_addr,
range.size, range.flags
alternatively obtain a struct resource, e.g.:
struct resource res;
of_pci_range_to_resource(range, np, res);
*/
}
Additionally the implementation takes care of adjacent ranges and merges them
into a single range (as was the case with powerpc and microblaze).
Signed-off-by: Andrew Murray andrew.mur...@arm.com
Signed-off-by: Liviu Dudau liviu.du...@arm.com
Signed-off-by: Thomas Petazzoni thomas.petazz...@free-electrons.com
Reviewed-by: Rob Herring rob.herr...@calxeda.com
Tested-by: Thomas Petazzoni thomas.petazz...@free-electrons.com
Tested-by: Linus Walleij linus.wall...@linaro.org
Acked-by: Grant Likely grant.lik...@secretlab.ca
But comments below...
---
drivers/of/address.c | 67 ++
drivers/of/of_pci.c| 113
include/linux/of_address.h | 46 ++
3 files changed, 154 insertions(+), 72 deletions(-)
diff --git a/drivers/of/address.c b/drivers/of/address.c
index 04da786..6eec70c 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -227,6 +227,73 @@ int of_pci_address_to_resource(struct device_node *dev,
int bar,
return __of_address_to_resource(dev, addrp, size, flags, NULL, r);
}
EXPORT_SYMBOL_GPL(of_pci_address_to_resource);
+
+int of_pci_range_parser(struct of_pci_range_parser *parser,
+ struct device_node *node)
+{
+ const int na = 3, ns = 2;
+ int rlen;
+
+ parser-node = node;
+ parser-pna = of_n_addr_cells(node);
+ parser-np = parser-pna + na + ns;
+
+ parser-range = of_get_property(node, ranges, rlen);
+ if (parser-range == NULL)
+ return -ENOENT;
+
+ parser-end = parser-range + rlen / sizeof(__be32);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(of_pci_range_parser);
of_pci_range_parser_init would be a clearer name
+struct of_pci_range *of_pci_process_ranges(struct of_pci_range_parser
*parser,
+ struct of_pci_range *range)
Similarly, of_pci_range_parser_one would be more consistent.
+{
+ const int na = 3, ns = 2;
+
+ if (!range)
+ return NULL;
+
+ if (!parser-range || parser-range + parser-np parser-end)
+ return NULL;
+
+ range-pci_space = parser-range[0];
+ range-flags = of_bus_pci_get_flags(parser-range);
+ range-pci_addr = of_read_number(parser-range + 1, ns);
+ range-cpu_addr = of_translate_address(parser-node,
+ parser-range + na);
+ range-size = of_read_number(parser-range + parser-pna + na, ns);
+
+ parser-range += parser-np;
+
+ /* Now consume following elements while they are contiguous */
+ while (parser-range + parser-np = parser-end) {
+ u32 flags, pci_space;
+ u64 pci_addr, cpu_addr, size;
+
+ pci_space = be32_to_cpup(parser-range);
+ flags = of_bus_pci_get_flags(parser-range);
+ pci_addr = of_read_number(parser-range + 1, ns);
+ cpu_addr = of_translate_address(parser-node,
+ parser-range + na);
+ size = of_read_number(parser-range + parser-pna + na, ns);
+
+ if (flags != range-flags)
+ break;
+ if (pci_addr != range-pci_addr + range-size ||
+ cpu_addr != range-cpu_addr + range-size)
+ break;
+
+ range-size += size;
+ parser-range += parser-np;
+ }
+
+ return range;
+}
+EXPORT_SYMBOL_GPL(of_pci_process_ranges);
+
#endif /* CONFIG_PCI */
/*
diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c
index 1626172..e5ab604 100644
--- a/drivers/of/of_pci.c
+++ b/drivers/of/of_pci.c
@@ -2,6 +2,7 @@
#include linux/export.h
#include linux/of.h
#include linux/of_pci.h
+#include linux/of_address.h
#include asm/prom.h
Re: [PATCH v7 2/3] of/pci: Provide support for parsing PCI DT ranges property
On Thu, Apr 18, 2013 at 02:44:01PM +0100, Grant Likely wrote:
On Tue, 16 Apr 2013 11:18:27 +0100, Andrew Murray andrew.mur...@arm.com
wrote:
This patch factors out common implementation patterns to reduce overall
kernel
code and provide a means for host bridge drivers to directly obtain struct
resources from the DT's ranges property without relying on architecture
specific
DT handling. This will make it easier to write archiecture independent host
bridge
drivers and mitigate against further duplication of DT parsing code.
This patch can be used in the following way:
struct of_pci_range_parser parser;
struct of_pci_range range;
if (of_pci_range_parser(parser, np))
; //no ranges property
for_each_of_pci_range(parser, range) {
/*
directly access properties of the address range, e.g.:
range.pci_space, range.pci_addr, range.cpu_addr,
range.size, range.flags
alternatively obtain a struct resource, e.g.:
struct resource res;
of_pci_range_to_resource(range, np, res);
*/
}
Additionally the implementation takes care of adjacent ranges and merges
them
into a single range (as was the case with powerpc and microblaze).
Signed-off-by: Andrew Murray andrew.mur...@arm.com
Signed-off-by: Liviu Dudau liviu.du...@arm.com
Signed-off-by: Thomas Petazzoni thomas.petazz...@free-electrons.com
Reviewed-by: Rob Herring rob.herr...@calxeda.com
Tested-by: Thomas Petazzoni thomas.petazz...@free-electrons.com
Tested-by: Linus Walleij linus.wall...@linaro.org
Acked-by: Grant Likely grant.lik...@secretlab.ca
But comments below...
---
drivers/of/address.c | 67 ++
drivers/of/of_pci.c| 113
include/linux/of_address.h | 46 ++
3 files changed, 154 insertions(+), 72 deletions(-)
diff --git a/drivers/of/address.c b/drivers/of/address.c
index 04da786..6eec70c 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -227,6 +227,73 @@ int of_pci_address_to_resource(struct device_node
*dev, int bar,
return __of_address_to_resource(dev, addrp, size, flags, NULL, r);
}
EXPORT_SYMBOL_GPL(of_pci_address_to_resource);
+
+int of_pci_range_parser(struct of_pci_range_parser *parser,
+ struct device_node *node)
+{
+ const int na = 3, ns = 2;
+ int rlen;
+
+ parser-node = node;
+ parser-pna = of_n_addr_cells(node);
+ parser-np = parser-pna + na + ns;
+
+ parser-range = of_get_property(node, ranges, rlen);
+ if (parser-range == NULL)
+ return -ENOENT;
+
+ parser-end = parser-range + rlen / sizeof(__be32);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(of_pci_range_parser);
of_pci_range_parser_init would be a clearer name
+struct of_pci_range *of_pci_process_ranges(struct of_pci_range_parser
*parser,
+ struct of_pci_range *range)
Similarly, of_pci_range_parser_one would be more consistent.
+{
+ const int na = 3, ns = 2;
+
+ if (!range)
+ return NULL;
+
+ if (!parser-range || parser-range + parser-np parser-end)
+ return NULL;
+
+ range-pci_space = parser-range[0];
+ range-flags = of_bus_pci_get_flags(parser-range);
+ range-pci_addr = of_read_number(parser-range + 1, ns);
+ range-cpu_addr = of_translate_address(parser-node,
+ parser-range + na);
+ range-size = of_read_number(parser-range + parser-pna + na, ns);
+
+ parser-range += parser-np;
+
+ /* Now consume following elements while they are contiguous */
+ while (parser-range + parser-np = parser-end) {
+ u32 flags, pci_space;
+ u64 pci_addr, cpu_addr, size;
+
+ pci_space = be32_to_cpup(parser-range);
+ flags = of_bus_pci_get_flags(parser-range);
+ pci_addr = of_read_number(parser-range + 1, ns);
+ cpu_addr = of_translate_address(parser-node,
+ parser-range + na);
+ size = of_read_number(parser-range + parser-pna + na, ns);
+
+ if (flags != range-flags)
+ break;
+ if (pci_addr != range-pci_addr + range-size ||
+ cpu_addr != range-cpu_addr + range-size)
+ break;
+
+ range-size += size;
+ parser-range += parser-np;
+ }
+
+ return range;
+}
+EXPORT_SYMBOL_GPL(of_pci_process_ranges);
+
#endif /* CONFIG_PCI */
/*
diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c
index 1626172..e5ab604 100644
--- a/drivers/of/of_pci.c
+++ b/drivers/of/of_pci.c
@@ -2,6 +2,7 @@
#include linux/export.h
Re: [PATCH v7 2/3] of/pci: Provide support for parsing PCI DT ranges property
On Thu, Apr 18, 2013 at 3:24 PM, Andrew Murray andrew.mur...@arm.com wrote:
On Thu, Apr 18, 2013 at 02:44:01PM +0100, Grant Likely wrote:
On Tue, 16 Apr 2013 11:18:27 +0100, Andrew Murray andrew.mur...@arm.com
wrote:
/* Act based on address space type */
res = NULL;
- switch ((pci_space 24) 0x3) {
- case 1: /* PCI IO space */
+ res_type = range.flags IORESOURCE_TYPE_BITS;
+ if (res_type == IORESOURCE_IO) {
Why the change from switch() to an if/else if sequence?
Russell King suggested this change - see
https://patchwork.kernel.org/patch/2323941/
Umm, that isn't what that link shows. That link shows the patch
already changing to an if/else if construct, and rmk is commenting on
that.
g.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v7 2/3] of/pci: Provide support for parsing PCI DT ranges property
On Thu, Apr 18, 2013 at 04:29:54PM +0100, Grant Likely wrote:
On Thu, Apr 18, 2013 at 3:24 PM, Andrew Murray andrew.mur...@arm.com wrote:
On Thu, Apr 18, 2013 at 02:44:01PM +0100, Grant Likely wrote:
On Tue, 16 Apr 2013 11:18:27 +0100, Andrew Murray andrew.mur...@arm.com
wrote:
/* Act based on address space type */
res = NULL;
- switch ((pci_space 24) 0x3) {
- case 1: /* PCI IO space */
+ res_type = range.flags IORESOURCE_TYPE_BITS;
+ if (res_type == IORESOURCE_IO) {
Why the change from switch() to an if/else if sequence?
Russell King suggested this change - see
https://patchwork.kernel.org/patch/2323941/
Umm, that isn't what that link shows. That link shows the patch
already changing to an if/else if construct, and rmk is commenting on
that.
Arh yes sorry about that. I can't find or remember any good reason why I
changed it from a switch to an if/else :\
Andrew Murray
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH v7 2/3] of/pci: Provide support for parsing PCI DT ranges property
This patch factors out common implementation patterns to reduce overall kernel
code and provide a means for host bridge drivers to directly obtain struct
resources from the DT's ranges property without relying on architecture specific
DT handling. This will make it easier to write archiecture independent host
bridge
drivers and mitigate against further duplication of DT parsing code.
This patch can be used in the following way:
struct of_pci_range_parser parser;
struct of_pci_range range;
if (of_pci_range_parser(parser, np))
; //no ranges property
for_each_of_pci_range(parser, range) {
/*
directly access properties of the address range, e.g.:
range.pci_space, range.pci_addr, range.cpu_addr,
range.size, range.flags
alternatively obtain a struct resource, e.g.:
struct resource res;
of_pci_range_to_resource(range, np, res);
*/
}
Additionally the implementation takes care of adjacent ranges and merges them
into a single range (as was the case with powerpc and microblaze).
Signed-off-by: Andrew Murray andrew.mur...@arm.com
Signed-off-by: Liviu Dudau liviu.du...@arm.com
Signed-off-by: Thomas Petazzoni thomas.petazz...@free-electrons.com
Reviewed-by: Rob Herring rob.herr...@calxeda.com
Tested-by: Thomas Petazzoni thomas.petazz...@free-electrons.com
Tested-by: Linus Walleij linus.wall...@linaro.org
---
drivers/of/address.c | 67 ++
drivers/of/of_pci.c| 113
include/linux/of_address.h | 46 ++
3 files changed, 154 insertions(+), 72 deletions(-)
diff --git a/drivers/of/address.c b/drivers/of/address.c
index 04da786..6eec70c 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -227,6 +227,73 @@ int of_pci_address_to_resource(struct device_node *dev,
int bar,
return __of_address_to_resource(dev, addrp, size, flags, NULL, r);
}
EXPORT_SYMBOL_GPL(of_pci_address_to_resource);
+
+int of_pci_range_parser(struct of_pci_range_parser *parser,
+ struct device_node *node)
+{
+ const int na = 3, ns = 2;
+ int rlen;
+
+ parser-node = node;
+ parser-pna = of_n_addr_cells(node);
+ parser-np = parser-pna + na + ns;
+
+ parser-range = of_get_property(node, ranges, rlen);
+ if (parser-range == NULL)
+ return -ENOENT;
+
+ parser-end = parser-range + rlen / sizeof(__be32);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(of_pci_range_parser);
+
+struct of_pci_range *of_pci_process_ranges(struct of_pci_range_parser *parser,
+ struct of_pci_range *range)
+{
+ const int na = 3, ns = 2;
+
+ if (!range)
+ return NULL;
+
+ if (!parser-range || parser-range + parser-np parser-end)
+ return NULL;
+
+ range-pci_space = parser-range[0];
+ range-flags = of_bus_pci_get_flags(parser-range);
+ range-pci_addr = of_read_number(parser-range + 1, ns);
+ range-cpu_addr = of_translate_address(parser-node,
+ parser-range + na);
+ range-size = of_read_number(parser-range + parser-pna + na, ns);
+
+ parser-range += parser-np;
+
+ /* Now consume following elements while they are contiguous */
+ while (parser-range + parser-np = parser-end) {
+ u32 flags, pci_space;
+ u64 pci_addr, cpu_addr, size;
+
+ pci_space = be32_to_cpup(parser-range);
+ flags = of_bus_pci_get_flags(parser-range);
+ pci_addr = of_read_number(parser-range + 1, ns);
+ cpu_addr = of_translate_address(parser-node,
+ parser-range + na);
+ size = of_read_number(parser-range + parser-pna + na, ns);
+
+ if (flags != range-flags)
+ break;
+ if (pci_addr != range-pci_addr + range-size ||
+ cpu_addr != range-cpu_addr + range-size)
+ break;
+
+ range-size += size;
+ parser-range += parser-np;
+ }
+
+ return range;
+}
+EXPORT_SYMBOL_GPL(of_pci_process_ranges);
+
#endif /* CONFIG_PCI */
/*
diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c
index 1626172..e5ab604 100644
--- a/drivers/of/of_pci.c
+++ b/drivers/of/of_pci.c
@@ -2,6 +2,7 @@
#include linux/export.h
#include linux/of.h
#include linux/of_pci.h
+#include linux/of_address.h
#include asm/prom.h
#if defined(CONFIG_PPC32) || defined(CONFIG_PPC64) ||
defined(CONFIG_MICROBLAZE)
@@ -82,67 +83,43 @@ EXPORT_SYMBOL_GPL(of_pci_find_child_device);
void pci_process_bridge_OF_ranges(struct pci_controller *hose,
struct device_node *dev, int primary)
