Re: [PATCH 1/4] of/pci: Provide support for parsing PCI DT rangesproperty

[Andrew Murray]

On Wed, Feb 13, 2013 at 10:53:11PM +0000, Grant Likely wrote:
> On Mon, 11 Feb 2013 09:22:17 +0100, Thierry Reding <thierry.reding@xxxxxxxxxxxxxxxxx> wrote:
> > From: Andrew Murray <andrew.murray@xxxxxxx>
> > 
> > DT bindings for PCI host bridges often use the ranges property to describe
> > memory and IO ranges - this binding tends to be the same across architectures
> > yet several parsing implementations exist, e.g. arch/mips/pci/pci.c,
> > arch/powerpc/kernel/pci-common.c, arch/sparc/kernel/pci.c and
> > arch/microblaze/pci/pci-common.c (clone of PPC). Some of these duplicate
> > functionality provided by drivers/of/address.c.
> 
> Hi Thierry,
> 
> Following from my comments on not merging these patches, here are my
> comments on this patch...
> 
> > This patch provides a common iterator-based parser for the ranges property, it
> > is hoped this will reduce DT representation differences between architectures
> > and that architectures will migrate in part to this new parser.
> > 
> > It is also hoped (and the motativation for the patch) that this patch will
> > reduce duplication of code when writing host bridge drivers that are supported
> > by multiple architectures.
> > 
> > This patch provides struct resources from a device tree node, e.g.:
> > 
> > 	u32 *last = NULL;
> > 	struct resource res;
> > 	while ((last = of_pci_process_ranges(np, res, last))) {
> > 		//do something with res
> > 	}
> 
> The approach seems reasonable, but it isn't optimal. For one, the setup
> code ends up getting run every time of_pci_process_ranges() gets called.
> It would also be more user-friendly to wrap it up in a
> "for_each_of_pci_range()" macro.
> 
> > Platforms with quirks can then do what they like with the resource or migrate
> > common quirk handling to the parser. In an ideal world drivers can just request
> > the obtained resources and pass them on (e.g. pci_add_resource_offset).
> > 
> > Signed-off-by: Andrew Murray <Andrew.Murray@xxxxxxx>
> > Signed-off-by: Liviu Dudau <Liviu.Dudau@xxxxxxx>
> > Signed-off-by: Thierry Reding <thierry.reding@xxxxxxxxxxxxxxxxx>
> > ---
> >  drivers/of/address.c       | 63 ++++++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/of_address.h |  9 +++++++
> >  2 files changed, 72 insertions(+)
> > 
> > diff --git a/drivers/of/address.c b/drivers/of/address.c
> > index 04da786..f607008 100644
> > --- a/drivers/of/address.c
> > +++ b/drivers/of/address.c
> > @@ -13,6 +13,7 @@
> >  #define OF_CHECK_COUNTS(na, ns)	(OF_CHECK_ADDR_COUNT(na) && (ns) > 0)
> >  
> >  static struct of_bus *of_match_bus(struct device_node *np);
> > +static struct of_bus *of_find_bus(const char *name);
> >  static int __of_address_to_resource(struct device_node *dev,
> >  		const __be32 *addrp, u64 size, unsigned int flags,
> >  		const char *name, struct resource *r);
> > @@ -227,6 +228,57 @@ 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);
> > +
> > +const __be32 *of_pci_process_ranges(struct device_node *node,
> > +				    struct resource *res, const __be32 *from)
> > +{
> > +	const __be32 *start, *end;
> > +	int na, ns, np, pna;
> > +	int rlen;
> > +	struct of_bus *bus;
> > +
> > +	WARN_ON(!res);
> > +
> > +	bus = of_find_bus("pci");
> > +	bus->count_cells(node, &na, &ns);
> > +	if (!OF_CHECK_COUNTS(na, ns)) {
> > +		pr_err("Bad cell count for %s\n", node->full_name);
> > +		return NULL;
> > +	}
> 
> Looking up the pci of_bus structure isn't really warrented here. This
> function will only ever be used on PCI busses, and na/ns for PCI is
> always 3/2. Just use those numbers here. You could however validate that
> the node has the correct values in #address-cells and #size-cells
> 
> > +
> > +	pna = of_n_addr_cells(node);
> > +	np = pna + na + ns;
> > +
> > +	start = of_get_property(node, "ranges", &rlen);
> > +	if (start == NULL)
> > +		return NULL;
> > +
> > +	end = start + rlen / sizeof(__be32);
> 
> The above structure means that the ranges property has to be looked up
> each and every time this function is called. It would be better to have
> a state structure that can look it up once and then keep track of the
> iteration.
> 
> > +
> > +	if (!from)
> > +		from = start;
> > +
> > +	while (from + np <= end) {
> > +		u64 cpu_addr, size;
> > +
> > +		cpu_addr = of_translate_address(node, from + na);
> > +		size = of_read_number(from + na + pna, ns);
> > +		res->flags = bus->get_flags(from);
> > +		from += np;
> > +
> > +		if (cpu_addr == OF_BAD_ADDR || size == 0)
> > +			continue;
> > +
> > +		res->name = node->full_name;
> > +		res->start = cpu_addr;
> > +		res->end = res->start + size - 1;
> > +		res->parent = res->child = res->sibling = NULL;
> > +		return from;
> > +	}
> > +
> > +	return NULL;
> > +}
> > +EXPORT_SYMBOL_GPL(of_pci_process_ranges);
> 
> All of the above can be directly factored out of the PCI implementation.
> You don't even need to touch most of the powerpc code. Create your
> iterator helper functions in the same patch that makes PowerPC and
> Microblaze use them, and then improve/modify the behaviour in seperate
> patches. The delta to ppc/microblaze really will be very small with this
> approach.  Here are the relevant PPC lines:
> 
> 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;
> 
> 	printk(KERN_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)
> 		return;
> 
> 	/* Parse it */
> 	while ((rlen -= np * 4) >= 0) {
> 		/* 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);
> 		ranges += np;
> 
> 		/* 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)
> 			continue;
> 
> 		/* 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);
> 		}
> 
> After factoring out the bits you want to use it will probably look like
> this:
> 
> void pci_process_bridge_OF_ranges(struct pci_controller *hose,
> 				  struct device_node *dev, int primary)
> {
> 	const u32 *ranges;
> 	int memno = 0, isa_hole = -1;
> 	unsigned long long isa_mb = 0;
> 	struct resource *res;
> 	struct of_pci_range_iter iter;
> 
> 	printk(KERN_INFO "PCI host bridge %s %s ranges:\n",
> 	       dev->full_name, primary ? "(primary)" : "");
> 
> 	for_each_of_pci_range(iter, np) {
> 		/* Read next ranges element */
> 		u32 pci_space = iter->pci_space;
> 		unsigned long long pci_addr = iter->pci_addr;
> 		unsigned long long cpu_addr = iter->cpu_addr;
> 		unsigned long long size = iter->size;
> 		int pna = iter->pna;
> 		/* or the remainder of the body of this function could
> 		 * have 'iter->' prefixed to each reference, which is
> 		 * better, but also a bit more invasive */
> 
> here really shouldn't be any further changes the the rest of the
> function. I don't think that is unreasonable to ask, and I can help with
> putting this together if you need it. Plus it will /reduce/ the number
> of lines in the kernel instead of adding to them. That is something I
> always want more of. :-)
> 
> Actually, a lot of the powerpc behaviour should still be
> relevant to all architectures, but I haven't dug that deep yet.

Thierry,

If you don't have much bandwidth I'd be quite happy to take this on - this
would be beneficial for my eventual patchset. I can start by refactoring common
implementations of pci_process_bridge_OF_ranges or similar across the
architectures as per Grant's suggestion? I didn't do this when I first posted
the patch as I was concerned about the testing effort.

Andrew Murray

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/