From mboxrd@z Thu Jan 1 00:00:00 1970 Received: with ECARTIS (v1.0.0; list linux-mips); Thu, 18 Apr 2013 15:44:12 +0200 (CEST) Received: from mail-wi0-f178.google.com ([209.85.212.178]:63431 "EHLO mail-wi0-f178.google.com" rhost-flags-OK-OK-OK-OK) by eddie.linux-mips.org with ESMTP id S6822444Ab3DRNoLTptqd (ORCPT ); Thu, 18 Apr 2013 15:44:11 +0200 Received: by mail-wi0-f178.google.com with SMTP id hq17so3004307wib.5 for ; Thu, 18 Apr 2013 06:44:05 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=x-received:sender:from:subject:to:cc:in-reply-to:references:date :message-id:x-gm-message-state; bh=UHXuaYNqqwnVjC6414b/JJMo1SGeXRmy/Q1L8UGb2dY=; b=Rdwc5OYXeoB0yhBi7hdB95s2e9MEETSNLr+TRxLSvyx+FBjj5vOc0GIhw3LbgxjPeV 8Q/KBQGnSOjyaSLMVkTDW0WsucXJ/hyMYn6GdZgVL2cxPc/0IzASJ8UgtfRl61B3xPeJ TKgSQ8ZfGoaOZc1P/HjbjsikPoTl90hFE8wL/Z9BgwbWOaGuc6u60DoBHCf0nmPddhqu 6jQh5Q9o/yiYLMJGiPtzjOjMopaz4Ow83NcntvJk4ra9T3X/77hZa5plzKgQTMphUhSy h6Gjg47Q21qbGO2PLcKbRb7CccaO9K8nPdQlxfyqi4xW1rKwrhLaB4rCx7ouRGxPRtj7 hxAA== X-Received: by 10.180.88.33 with SMTP id bd1mr21372412wib.18.1366292645407; Thu, 18 Apr 2013 06:44:05 -0700 (PDT) Received: from localhost (host31-53-18-197.range31-53.btcentralplus.com. [31.53.18.197]) by mx.google.com with ESMTPS id fz3sm16269346wib.0.2013.04.18.06.44.02 (version=TLSv1.1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Thu, 18 Apr 2013 06:44:04 -0700 (PDT) Received: by localhost (Postfix, from userid 1000) id 84AEE3E1319; Thu, 18 Apr 2013 14:44:01 +0100 (BST) From: Grant Likely Subject: Re: [PATCH v7 2/3] of/pci: Provide support for parsing PCI DT ranges property To: Andrew Murray , linux-mips@linux-mips.org, linuxppc-dev@lists.ozlabs.org Cc: rob.herring@calxeda.com, jgunthorpe@obsidianresearch.com, linux@arm.linux.org.uk, siva.kallam@samsung.com, linux-pci@vger.kernel.org, devicetree-discuss@lists.ozlabs.org, jg1.han@samsung.com, Liviu.Dudau@arm.com, linux-kernel@vger.kernel.org, linux-samsung-soc@vger.kernel.org, kgene.kim@samsung.com, bhelgaas@google.com, suren.reddy@samsung.com, linux-arm-kernel@lists.infradead.org, monstr@monstr.eu, benh@kernel.crashing.org, paulus@samba.org, thomas.petazzoni@free-electrons.com, thierry.reding@avionic-design.de, thomas.abraham@linaro.org, arnd@arndb.de, linus.walleij@linaro.org, Andrew Murray In-Reply-To: <1366107508-12672-3-git-send-email-Andrew.Murray@arm.com> References: <1366107508-12672-1-git-send-email-Andrew.Murray@arm.com> <1366107508-12672-3-git-send-email-Andrew.Murray@arm.com> Date: Thu, 18 Apr 2013 14:44:01 +0100 Message-Id: <20130418134401.84AEE3E1319@localhost> X-Gm-Message-State: ALoCoQmR1herqGS2VFHfyUPDDGNrjHEpCq+ZELnVUEa4P3MP1J76W46yma12eZ9rd+xpXjLHKrHv Return-Path: X-Envelope-To: <"|/home/ecartis/ecartis -s linux-mips"> (uid 0) X-Orcpt: rfc822;linux-mips@linux-mips.org Original-Recipient: rfc822;linux-mips@linux-mips.org X-archive-position: 36262 X-ecartis-version: Ecartis v1.0.0 Sender: linux-mips-bounce@linux-mips.org Errors-to: linux-mips-bounce@linux-mips.org X-original-sender: grant.likely@secretlab.ca Precedence: bulk List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: linux-mips X-List-ID: linux-mips List-subscribe: List-owner: List-post: List-archive: X-list: linux-mips On Tue, 16 Apr 2013 11:18:27 +0100, Andrew Murray 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 > Signed-off-by: Liviu Dudau > Signed-off-by: Thomas Petazzoni > Reviewed-by: Rob Herring > Tested-by: Thomas Petazzoni > Tested-by: Linus Walleij Acked-by: Grant Likely 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 > #include > #include > +#include > #include > > #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) > { > - const u32 *ranges; > - int rlen; > - int pna = of_n_addr_cells(dev); > - int np = pna + 5; > int memno = 0, isa_hole = -1; > - u32 pci_space; > - unsigned long long pci_addr, cpu_addr, pci_next, cpu_next, size; > unsigned long long isa_mb = 0; > struct resource *res; > + struct of_pci_range range; > + struct of_pci_range_parser parser; > + u32 res_type; > > pr_info("PCI host bridge %s %s ranges:\n", > dev->full_name, primary ? "(primary)" : ""); > > - /* Get ranges property */ > - ranges = of_get_property(dev, "ranges", &rlen); > - if (ranges == NULL) > + /* Check for ranges property */ > + if (of_pci_range_parser(&parser, dev)) > return; > > - /* Parse it */ > pr_debug("Parsing ranges property...\n"); > - while ((rlen -= np * 4) >= 0) { > + for_each_of_pci_range(&parser, &range) { > /* Read next ranges element */ > - pci_space = ranges[0]; > - pci_addr = of_read_number(ranges + 1, 2); > - cpu_addr = of_translate_address(dev, ranges + 3); > - size = of_read_number(ranges + pna + 3, 2); Tip: the diff on this function would be a whole lot simpler if the above locals were kept, but updated from the ranges structure. Not at all a big problem, but it is something that makes changes like this easier to review. The removal of the locals could also be split into a separate patch to end up with the same result. > - > - 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. > > /* 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? > > - /* 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? 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. g. From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wi0-f178.google.com ([209.85.212.178]:63431 "EHLO mail-wi0-f178.google.com" rhost-flags-OK-OK-OK-OK) by eddie.linux-mips.org with ESMTP id S6822444Ab3DRNoLTptqd (ORCPT ); Thu, 18 Apr 2013 15:44:11 +0200 Received: by mail-wi0-f178.google.com with SMTP id hq17so3004307wib.5 for ; Thu, 18 Apr 2013 06:44:05 -0700 (PDT) From: Grant Likely Subject: Re: [PATCH v7 2/3] of/pci: Provide support for parsing PCI DT ranges property In-Reply-To: <1366107508-12672-3-git-send-email-Andrew.Murray@arm.com> References: <1366107508-12672-1-git-send-email-Andrew.Murray@arm.com> <1366107508-12672-3-git-send-email-Andrew.Murray@arm.com> Date: Thu, 18 Apr 2013 14:44:01 +0100 Message-ID: <20130418134401.84AEE3E1319@localhost> Return-Path: Sender: linux-mips-bounce@linux-mips.org Errors-to: linux-mips-bounce@linux-mips.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-subscribe: List-owner: List-post: List-archive: To: Andrew Murray , linux-mips@linux-mips.org, linuxppc-dev@lists.ozlabs.org Cc: rob.herring@calxeda.com, jgunthorpe@obsidianresearch.com, linux@arm.linux.org.uk, siva.kallam@samsung.com, linux-pci@vger.kernel.org, devicetree-discuss@lists.ozlabs.org, jg1.han@samsung.com, Liviu.Dudau@arm.com, linux-kernel@vger.kernel.org, linux-samsung-soc@vger.kernel.org, kgene.kim@samsung.com, bhelgaas@google.com, suren.reddy@samsung.com, linux-arm-kernel@lists.infradead.org, monstr@monstr.eu, benh@kernel.crashing.org, paulus@samba.org, thomas.petazzoni@free-electrons.com, thierry.reding@avionic-design.de, thomas.abraham@linaro.org, arnd@arndb.de, linus.walleij@linaro.org Message-ID: <20130418134401.w6HzDhbN_ozkHchsXr4iGGwDA6bS-q4e-w9LBz5Wipg@z> On Tue, 16 Apr 2013 11:18:27 +0100, Andrew Murray 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 > Signed-off-by: Liviu Dudau > Signed-off-by: Thomas Petazzoni > Reviewed-by: Rob Herring > Tested-by: Thomas Petazzoni > Tested-by: Linus Walleij Acked-by: Grant Likely 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 > #include > #include > +#include > #include > > #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) > { > - const u32 *ranges; > - int rlen; > - int pna = of_n_addr_cells(dev); > - int np = pna + 5; > int memno = 0, isa_hole = -1; > - u32 pci_space; > - unsigned long long pci_addr, cpu_addr, pci_next, cpu_next, size; > unsigned long long isa_mb = 0; > struct resource *res; > + struct of_pci_range range; > + struct of_pci_range_parser parser; > + u32 res_type; > > pr_info("PCI host bridge %s %s ranges:\n", > dev->full_name, primary ? "(primary)" : ""); > > - /* Get ranges property */ > - ranges = of_get_property(dev, "ranges", &rlen); > - if (ranges == NULL) > + /* Check for ranges property */ > + if (of_pci_range_parser(&parser, dev)) > return; > > - /* Parse it */ > pr_debug("Parsing ranges property...\n"); > - while ((rlen -= np * 4) >= 0) { > + for_each_of_pci_range(&parser, &range) { > /* Read next ranges element */ > - pci_space = ranges[0]; > - pci_addr = of_read_number(ranges + 1, 2); > - cpu_addr = of_translate_address(dev, ranges + 3); > - size = of_read_number(ranges + pna + 3, 2); Tip: the diff on this function would be a whole lot simpler if the above locals were kept, but updated from the ranges structure. Not at all a big problem, but it is something that makes changes like this easier to review. The removal of the locals could also be split into a separate patch to end up with the same result. > - > - 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. > > /* 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? > > - /* 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? 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. g. From mboxrd@z Thu Jan 1 00:00:00 1970 From: Grant Likely Subject: Re: [PATCH v7 2/3] of/pci: Provide support for parsing PCI DT ranges property Date: Thu, 18 Apr 2013 14:44:01 +0100 Message-ID: <20130418134401.84AEE3E1319@localhost> References: <1366107508-12672-1-git-send-email-Andrew.Murray@arm.com> <1366107508-12672-3-git-send-email-Andrew.Murray@arm.com> Return-path: In-Reply-To: <1366107508-12672-3-git-send-email-Andrew.Murray@arm.com> Sender: linux-pci-owner@vger.kernel.org To: linux-mips@linux-mips.org, linuxppc-dev@lists.ozlabs.org Cc: rob.herring@calxeda.com, jgunthorpe@obsidianresearch.com, linux@arm.linux.org.uk, siva.kallam@samsung.com, linux-pci@vger.kernel.org, devicetree-discuss@lists.ozlabs.org, jg1.han@samsung.com, Liviu.Dudau@arm.com, linux-kernel@vger.kernel.org, linux-samsung-soc@vger.kernel.org, kgene.kim@samsung.com, bhelgaas@google.com, suren.reddy@samsung.com, linux-arm-kernel@lists.infradead.org, monstr@monstr.eu, benh@kernel.crashing.org, paulus@samba.org, thomas.petazzoni@free-electrons.com, thierry.reding@avionic-design.de, thomas.abraham@linaro.org, arnd@arndb.de, linus.walleij@linaro.org, Andrew Murray List-Id: linux-samsung-soc@vger.kernel.org On Tue, 16 Apr 2013 11:18:27 +0100, Andrew Murray 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 > Signed-off-by: Liviu Dudau > Signed-off-by: Thomas Petazzoni > Reviewed-by: Rob Herring > Tested-by: Thomas Petazzoni > Tested-by: Linus Walleij Acked-by: Grant Likely 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 > #include > #include > +#include > #include > > #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) > { > - const u32 *ranges; > - int rlen; > - int pna = of_n_addr_cells(dev); > - int np = pna + 5; > int memno = 0, isa_hole = -1; > - u32 pci_space; > - unsigned long long pci_addr, cpu_addr, pci_next, cpu_next, size; > unsigned long long isa_mb = 0; > struct resource *res; > + struct of_pci_range range; > + struct of_pci_range_parser parser; > + u32 res_type; > > pr_info("PCI host bridge %s %s ranges:\n", > dev->full_name, primary ? "(primary)" : ""); > > - /* Get ranges property */ > - ranges = of_get_property(dev, "ranges", &rlen); > - if (ranges == NULL) > + /* Check for ranges property */ > + if (of_pci_range_parser(&parser, dev)) > return; > > - /* Parse it */ > pr_debug("Parsing ranges property...\n"); > - while ((rlen -= np * 4) >= 0) { > + for_each_of_pci_range(&parser, &range) { > /* Read next ranges element */ > - pci_space = ranges[0]; > - pci_addr = of_read_number(ranges + 1, 2); > - cpu_addr = of_translate_address(dev, ranges + 3); > - size = of_read_number(ranges + pna + 3, 2); Tip: the diff on this function would be a whole lot simpler if the above locals were kept, but updated from the ranges structure. Not at all a big problem, but it is something that makes changes like this easier to review. The removal of the locals could also be split into a separate patch to end up with the same result. > - > - 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. > > /* 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? > > - /* 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? 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. g. From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wi0-x236.google.com (mail-wi0-x236.google.com [IPv6:2a00:1450:400c:c05::236]) (using TLSv1 with cipher ECDHE-RSA-RC4-SHA (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority" (not verified)) by ozlabs.org (Postfix) with ESMTPS id B901D2C0171 for ; Thu, 18 Apr 2013 23:44:09 +1000 (EST) Received: by mail-wi0-f182.google.com with SMTP id m6so3003065wiv.15 for ; Thu, 18 Apr 2013 06:44:05 -0700 (PDT) Sender: Grant Likely From: Grant Likely Subject: Re: [PATCH v7 2/3] of/pci: Provide support for parsing PCI DT ranges property To: Andrew Murray , linux-mips@linux-mips.org, linuxppc-dev@lists.ozlabs.org In-Reply-To: <1366107508-12672-3-git-send-email-Andrew.Murray@arm.com> References: <1366107508-12672-1-git-send-email-Andrew.Murray@arm.com> <1366107508-12672-3-git-send-email-Andrew.Murray@arm.com> Date: Thu, 18 Apr 2013 14:44:01 +0100 Message-Id: <20130418134401.84AEE3E1319@localhost> Cc: siva.kallam@samsung.com, linux-pci@vger.kernel.org, linus.walleij@linaro.org, thierry.reding@avionic-design.de, Liviu.Dudau@arm.com, paulus@samba.org, linux-samsung-soc@vger.kernel.org, linux@arm.linux.org.uk, jg1.han@samsung.com, jgunthorpe@obsidianresearch.com, thomas.abraham@linaro.org, arnd@arndb.de, devicetree-discuss@lists.ozlabs.org, rob.herring@calxeda.com, kgene.kim@samsung.com, bhelgaas@google.com, linux-arm-kernel@lists.infradead.org, thomas.petazzoni@free-electrons.com, monstr@monstr.eu, linux-kernel@vger.kernel.org, suren.reddy@samsung.com, Andrew Murray List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , On Tue, 16 Apr 2013 11:18:27 +0100, Andrew Murray 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 > Signed-off-by: Liviu Dudau > Signed-off-by: Thomas Petazzoni > Reviewed-by: Rob Herring > Tested-by: Thomas Petazzoni > Tested-by: Linus Walleij Acked-by: Grant Likely 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 > #include > #include > +#include > #include > > #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) > { > - const u32 *ranges; > - int rlen; > - int pna = of_n_addr_cells(dev); > - int np = pna + 5; > int memno = 0, isa_hole = -1; > - u32 pci_space; > - unsigned long long pci_addr, cpu_addr, pci_next, cpu_next, size; > unsigned long long isa_mb = 0; > struct resource *res; > + struct of_pci_range range; > + struct of_pci_range_parser parser; > + u32 res_type; > > pr_info("PCI host bridge %s %s ranges:\n", > dev->full_name, primary ? "(primary)" : ""); > > - /* Get ranges property */ > - ranges = of_get_property(dev, "ranges", &rlen); > - if (ranges == NULL) > + /* Check for ranges property */ > + if (of_pci_range_parser(&parser, dev)) > return; > > - /* Parse it */ > pr_debug("Parsing ranges property...\n"); > - while ((rlen -= np * 4) >= 0) { > + for_each_of_pci_range(&parser, &range) { > /* Read next ranges element */ > - pci_space = ranges[0]; > - pci_addr = of_read_number(ranges + 1, 2); > - cpu_addr = of_translate_address(dev, ranges + 3); > - size = of_read_number(ranges + pna + 3, 2); Tip: the diff on this function would be a whole lot simpler if the above locals were kept, but updated from the ranges structure. Not at all a big problem, but it is something that makes changes like this easier to review. The removal of the locals could also be split into a separate patch to end up with the same result. > - > - 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. > > /* 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? > > - /* 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? 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. g. From mboxrd@z Thu Jan 1 00:00:00 1970 From: grant.likely@secretlab.ca (Grant Likely) Date: Thu, 18 Apr 2013 14:44:01 +0100 Subject: [PATCH v7 2/3] of/pci: Provide support for parsing PCI DT ranges property In-Reply-To: <1366107508-12672-3-git-send-email-Andrew.Murray@arm.com> References: <1366107508-12672-1-git-send-email-Andrew.Murray@arm.com> <1366107508-12672-3-git-send-email-Andrew.Murray@arm.com> Message-ID: <20130418134401.84AEE3E1319@localhost> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Tue, 16 Apr 2013 11:18:27 +0100, Andrew Murray 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 > Signed-off-by: Liviu Dudau > Signed-off-by: Thomas Petazzoni > Reviewed-by: Rob Herring > Tested-by: Thomas Petazzoni > Tested-by: Linus Walleij Acked-by: Grant Likely 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 > #include > #include > +#include > #include > > #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) > { > - const u32 *ranges; > - int rlen; > - int pna = of_n_addr_cells(dev); > - int np = pna + 5; > int memno = 0, isa_hole = -1; > - u32 pci_space; > - unsigned long long pci_addr, cpu_addr, pci_next, cpu_next, size; > unsigned long long isa_mb = 0; > struct resource *res; > + struct of_pci_range range; > + struct of_pci_range_parser parser; > + u32 res_type; > > pr_info("PCI host bridge %s %s ranges:\n", > dev->full_name, primary ? "(primary)" : ""); > > - /* Get ranges property */ > - ranges = of_get_property(dev, "ranges", &rlen); > - if (ranges == NULL) > + /* Check for ranges property */ > + if (of_pci_range_parser(&parser, dev)) > return; > > - /* Parse it */ > pr_debug("Parsing ranges property...\n"); > - while ((rlen -= np * 4) >= 0) { > + for_each_of_pci_range(&parser, &range) { > /* Read next ranges element */ > - pci_space = ranges[0]; > - pci_addr = of_read_number(ranges + 1, 2); > - cpu_addr = of_translate_address(dev, ranges + 3); > - size = of_read_number(ranges + pna + 3, 2); Tip: the diff on this function would be a whole lot simpler if the above locals were kept, but updated from the ranges structure. Not at all a big problem, but it is something that makes changes like this easier to review. The removal of the locals could also be split into a separate patch to end up with the same result. > - > - 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. > > /* 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? > > - /* 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? 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. g.