Re: [PATCH v4 08/19] add support for Linux device trees

[Christoffer Dall <christoffer.dall@linaro.org>]

On Mon, Jun 09, 2014 at 12:18:49PM +0200, Andrew Jones wrote:
> On Fri, Jun 06, 2014 at 08:38:35PM +0200, Christoffer Dall wrote:
> > On Thu, Apr 10, 2014 at 06:56:49PM +0200, Andrew Jones wrote:
> > > Add some device tree functions, built on libfdt, to the arch-neutral
> > > lib code in order to facilitate the extraction of boot info and device
> > > base addresses. These functions should work on device trees conforming
> > > to section III of the kernel doc
> > > Documentation/devicetree/booting-without-of.txt.
> > > 
> > > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > > ---
> > > v4: reworked everything, added lots of comments to devicetree.h
> > 
> >  => reworked everything
> >  that will make it easy to review again ;)
> > 
> > > ---
> > >  lib/devicetree.c | 289 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  lib/devicetree.h | 230 +++++++++++++++++++++++++++++++++++++++++++
> > >  lib/libcflat.h   |   2 +
> > >  3 files changed, 521 insertions(+)
> > >  create mode 100644 lib/devicetree.c
> > >  create mode 100644 lib/devicetree.h
> > > 
> > > diff --git a/lib/devicetree.c b/lib/devicetree.c
> > > new file mode 100644
> > > index 0000000000000..482028f066346
> > > --- /dev/null
> > > +++ b/lib/devicetree.c
> > > @@ -0,0 +1,289 @@
> > > +/*
> > > + * Copyright (C) 2014, Red Hat Inc, Andrew Jones <drjones@redhat.com>
> > > + *
> > > + * This work is licensed under the terms of the GNU LGPL, version 2.
> > > + */
> > > +#include "libcflat.h"
> > > +#include "libfdt/libfdt.h"
> > > +#include "devicetree.h"
> > > +
> > > +static const void *fdt;
> > > +static u32 root_nr_address_cells, root_nr_size_cells;
> > > +
> > > +const void *dt_fdt(void)
> > > +{
> > > +	return fdt;
> > > +}
> > > +
> > > +bool dt_available(void)
> > > +{
> > > +	return fdt_check_header(fdt) == 0;
> > > +}
> > > +
> > > +int dt_get_nr_cells(int fdtnode, u32 *nr_address_cells, u32 *nr_size_cells)
> > > +{
> > > +	const struct fdt_property *prop;
> > > +	u32 *nr_cells;
> > > +	int len;
> > > +
> > > +	prop = fdt_get_property(fdt, fdtnode, "#address-cells", &len);
> > > +	if (prop == NULL)
> > > +		return len;
> > > +
> > > +	nr_cells = (u32 *)prop->data;
> > > +	*nr_address_cells = fdt32_to_cpu(*nr_cells);
> > > +
> > > +	prop = fdt_get_property(fdt, fdtnode, "#size-cells", &len);
> > > +	if (prop == NULL)
> > > +		return len;
> > > +
> > > +	nr_cells = (u32 *)prop->data;
> > > +	*nr_size_cells = fdt32_to_cpu(*nr_cells);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +void dt_reg_init(struct dt_reg *reg, u32 nr_address_cells, u32 nr_size_cells)
> > > +{
> > > +	memset(reg, 0, sizeof(struct dt_reg));
> > > +	reg->nr_address_cells = nr_address_cells;
> > > +	reg->nr_size_cells = nr_size_cells;
> > > +}
> > > +
> > > +int dt_get_reg(int fdtnode, int regidx, struct dt_reg *reg)
> > > +{
> > > +	const struct fdt_property *prop;
> > > +	u32 *cells, i;
> > > +	unsigned nr_tuple_cells;
> > > +	int len;
> > > +
> > > +	prop = fdt_get_property(fdt, fdtnode, "reg", &len);
> > > +	if (prop == NULL)
> > > +		return len;
> > > +
> > > +	cells = (u32 *)prop->data;
> > > +	nr_tuple_cells = reg->nr_address_cells + reg->nr_size_cells;
> > > +	regidx *= nr_tuple_cells;
> > > +
> > > +	if (regidx + nr_tuple_cells > len/sizeof(u32))
> > 
> > Shouldn't this be >= ?
> 
> Not this time. The LHS is the total number of words needed (not an
> index).
> 

I see, you're right.

> > 
> > > +		return -FDT_ERR_NOTFOUND;
> > > +
> > > +	for (i = 0; i < reg->nr_address_cells; ++i)
> > > +		reg->address_cells[i] = fdt32_to_cpu(cells[regidx + i]);
> > > +
> > > +	regidx += reg->nr_address_cells;
> > > +	for (i = 0; i < reg->nr_size_cells; ++i)
> > > +		reg->size_cells[i] = fdt32_to_cpu(cells[regidx + i]);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +int dt_pbus_translate_node(int fdtnode, int regidx, void *reg)
> > 
> > why doesn't this function take a struct dt_pbus_reg * instead of a
> > void *?
> 
> Guess I forgot my coffee when I wrote it... I'll change that.
> 
> > 
> > > +{
> > > +	struct dt_pbus_reg *pbus_reg = (struct dt_pbus_reg *)reg;
> > > +	struct dt_reg raw_reg;
> > > +	int ret;
> > > +
> > > +	dt_reg_init(&raw_reg, root_nr_address_cells, root_nr_size_cells);
> > > +
> > > +	ret = dt_get_reg(fdtnode, regidx, &raw_reg);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	pbus_reg->addr = dt_pbus_read_cells(raw_reg.nr_address_cells,
> > > +					    raw_reg.address_cells);
> > > +	pbus_reg->size = dt_pbus_read_cells(raw_reg.nr_size_cells,
> > > +					    raw_reg.size_cells);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +int dt_pbus_translate(const struct dt_device *dev, int regidx,
> > > +		      void *reg)
> > > +{
> > > +	return dt_pbus_translate_node(dev->fdtnode, regidx, reg);
> > > +}
> > > +
> > > +int dt_pbus_get_baseaddr(const struct dt_device *dev, dt_pbus_addr_t *base)
> > > +{
> > > +	struct dt_pbus_reg reg;
> > > +	int ret;
> > > +
> > > +	ret = dt_pbus_translate(dev, 0, &reg);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	*base = reg.addr;
> > > +	return 0;
> > > +}
> > > +
> > > +int dt_bus_match_any(const struct dt_device *dev __unused, int fdtnode)
> > > +{
> > > +	/* matches any device with a valid node */
> > > +	return fdtnode < 0 ? fdtnode : 1;
> > > +}
> > > +
> > > +static const struct dt_bus dt_default_bus = {
> > > +	.match = dt_bus_match_any,
> > > +	.translate = dt_pbus_translate,
> > > +};
> > > +
> > > +void dt_bus_init_defaults(struct dt_bus *bus)
> > > +{
> > > +	memcpy(bus, &dt_default_bus, sizeof(struct dt_bus));
> > > +}
> > > +
> > > +void dt_device_init(struct dt_device *dev, const struct dt_bus *bus,
> > > +		    const void *info)
> > > +{
> > > +	memset(dev, 0, sizeof(struct dt_device));
> > > +	dev->bus = bus;
> > > +	dev->info = (void *)info;
> > > +}
> > > +
> > > +int dt_device_find_compatible(const struct dt_device *dev,
> > > +			      const char *compatible)
> > > +{
> > > +	int node, ret;
> > > +
> > > +	node = fdt_node_offset_by_compatible(fdt, -1, compatible);
> > > +	while (node >= 0) {
> > > +		ret = dev->bus->match(dev, node);
> > > +		if (ret < 0)
> > > +			return ret;
> > > +		else if (ret)
> > > +			break;
> > > +		node = fdt_node_offset_by_compatible(fdt, node, compatible);
> > > +	}
> > > +	return node;
> > > +}
> > > +
> > > +int dt_pbus_get_baseaddr_compatible(const char *compatible,
> > > +				    dt_pbus_addr_t *baseaddr)
> > > +{
> > > +	struct dt_device dev;
> > > +	int node, ret;
> > > +
> > > +	dt_device_init(&dev, &dt_default_bus, NULL);
> > > +
> > > +	node = dt_device_find_compatible(&dev, compatible);
> > > +	if (node < 0)
> > > +		return node;
> > > +
> > > +	dt_device_bind_node(&dev, node);
> > > +
> > > +	ret = dt_pbus_get_baseaddr(&dev, baseaddr);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +int dt_get_memory_params(struct dt_pbus_reg *regs, int nr_regs)
> > > +{
> > > +	const char *pn = "device_type", *pv = "memory";
> > > +	int node, ret, pl = strlen(pv) + 1, nr = 0;
> > > +	struct dt_pbus_reg reg;
> > > +
> > > +	node = fdt_node_offset_by_prop_value(fdt, -1, pn, pv, pl);
> > > +
> > > +	while (node >= 0) {
> > > +
> > > +		while (nr < nr_regs) {
> > > +			ret = dt_pbus_translate_node(node, nr, &reg);
> > > +			if (ret == -FDT_ERR_NOTFOUND)
> > > +				break;
> > > +			if (ret < 0)
> > > +				return ret;
> > > +			regs[nr].addr = reg.addr;
> > > +			regs[nr].size = reg.size;
> > > +			++nr;
> > > +		}
> > > +
> > > +		node = fdt_node_offset_by_prop_value(fdt, node, pn, pv, pl);
> > > +	}
> > > +
> > > +	return node != -FDT_ERR_NOTFOUND ? node : nr;
> > > +}
> > > +
> > > +int dt_for_each_cpu_node(void (*func)(int fdtnode, u32 regval, void *info),
> > > +			 void *info)
> > > +{
> > > +	const struct fdt_property *prop;
> > > +	int cpus, cpu, ret, len;
> > > +	struct dt_reg raw_reg;
> > > +	u32 nac, nsc;
> > > +
> > > +	cpus = fdt_path_offset(fdt, "/cpus");
> > > +	if (cpus < 0)
> > > +		return cpus;
> > > +
> > > +	ret = dt_get_nr_cells(cpus, &nac, &nsc);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	dt_reg_init(&raw_reg, nac, nsc);
> > > +
> > > +	dt_for_each_subnode(cpus, cpu) {
> > > +
> > > +		prop = fdt_get_property(fdt, cpu, "device_type", &len);
> > > +		if (prop == NULL)
> > > +			return len;
> > > +
> > > +		if (len != 4 || strcmp((char *)prop->data, "cpu"))
> > > +			continue;
> > > +
> > > +		ret = dt_get_reg(cpu, 0, &raw_reg);
> > > +		if (ret < 0)
> > > +			return ret;
> > > +
> > > +		func(cpu, raw_reg.address_cells[0], info);
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +int dt_get_bootargs(const char **bootargs)
> > > +{
> > > +	const struct fdt_property *prop;
> > > +	int node, len;
> > > +
> > > +	*bootargs = NULL;
> > > +
> > > +	node = fdt_path_offset(fdt, "/chosen");
> > > +	if (node < 0)
> > > +		return node;
> > > +
> > > +	prop = fdt_get_property(fdt, node, "bootargs", &len);
> > > +	if (prop)
> > > +		*bootargs = (char *)prop->data;
> > > +	else if (len < 0 && len != -FDT_ERR_NOTFOUND)
> > > +		return len;
> > 
> > so if you get FDT_ERR_NOTFOUND you still return success but the bootargs
> > won't be set? why?
> 
> bootargs will be NULL in that case, which is enough to know there wasn't
> any. I also wanted to be able to easily check for any other error without
> having to filter FDT_ERR_NOTFOUND at every call site. I can skip this
> special behavior though, if a more consistent interface is preferred.
> 

didn't think about the fact that it's valid not to specify any bootargs.
Not sure if it's nicer to return an empty string or NULL, but it works
either way.

> > 
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +int dt_init(const void *fdt_ptr)
> > > +{
> > > +	struct dt_bus *defbus = (struct dt_bus *)&dt_default_bus;
> > > +	int root, ret;
> > > +
> > > +	ret = fdt_check_header(fdt_ptr);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +	fdt = fdt_ptr;
> > > +
> > > +	root = fdt_path_offset(fdt, "/");
> > > +	if (root < 0)
> > > +		return root;
> > > +
> > > +	ret = dt_get_nr_cells(root, &root_nr_address_cells,
> > > +				    &root_nr_size_cells);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	defbus->nr_address_cells = root_nr_address_cells;
> > > +	defbus->nr_size_cells = root_nr_size_cells;
> > > +
> > > +	return 0;
> > > +}
> > > diff --git a/lib/devicetree.h b/lib/devicetree.h
> > > new file mode 100644
> > > index 0000000000000..032b5497f9db1
> > > --- /dev/null
> > > +++ b/lib/devicetree.h
> > > @@ -0,0 +1,230 @@
> > > +#ifndef _DEVICETREE_H_
> > > +#define _DEVICETREE_H_
> > > +/*
> > > + * devicetree builds on libfdt to implement abstractions and accessors
> > > + * for Linux required device tree content. The accessors provided are
> > > + * common across architectures. See section III of the kernel doc
> > > + * Documentation/devicetree/booting-without-of.txt
> > > + *
> > > + * Copyright (C) 2014, Red Hat Inc, Andrew Jones <drjones@redhat.com>
> > > + *
> > > + * This work is licensed under the terms of the GNU LGPL, version 2.
> > > + */
> > > +#include "libcflat.h"
> > > +#include "libfdt/libfdt.h"
> > > +
> > > +/**********************************************************************
> > > + * devicetree init and libfdt helpers
> > > + **********************************************************************/
> > > +
> > > +/* dt_init initializes devicetree with a pointer to an fdt, @fdt_ptr */
> > > +extern int dt_init(const void *fdt_ptr);
> > > +
> > > +/* get the fdt pointer that devicetree is using */
> > > +const void *dt_fdt(void);
> > 
> > nit: why no extern here?
> 
> Just forgot it. Will add it.
> 
> > 
> > > +
> > > +/* check for an initialized, valid devicetree */
> > > +extern bool dt_available(void);
> > > +
> > > +/* traverse child nodes */
> > > +#define dt_for_each_subnode(n, s)					\
> > > +	for (s = fdt_first_subnode(dt_fdt(), n);			\
> > > +	     s != -FDT_ERR_NOTFOUND;					\
> > > +	     s = fdt_next_subnode(dt_fdt(), s))
> > > +
> > > +/**********************************************************************
> > > + * Abstractions for required node types and properties
> > > + **********************************************************************/
> > > +
> > > +struct dt_device {
> > > +	int fdtnode;
> > > +	const struct dt_bus *bus;
> > > +
> > > +	/*
> > > +	 * info is a pointer to device specific data, which may be
> > > +	 * used by the bus match() and translate() functions
> > > +	 */
> > > +	void *info;
> > > +};
> > > +
> > > +struct dt_bus {
> > > +	/*
> > > +	 * match a device @dev to an fdt node @fdtnode
> > > +	 * returns
> > > +	 *  - a positive value on match
> > > +	 *  - zero on no match
> > > +	 *  - a negative FDT_ERR_* value on failure
> > > +	 */
> > > +	int (*match)(const struct dt_device *dev, int fdtnode);
> > > +
> > > +	/*
> > > +	 * translate the @regidx'th "address size" tuple of
> > > +	 * @dev's fdt node's "reg" property, and store the result
> > > +	 * in @reg, a bus specific structure
> > > +	 * returns
> > > +	 *  - zero on success
> > > +	 *  - a negative FDT_ERR_* value on failure
> > > +	 */
> > > +	int (*translate)(const struct dt_device *dev, int regidx, void *reg);
> > > +
> > > +	/* the bus #address-cells and #size-cells properties */
> > > +	u32 nr_address_cells, nr_size_cells;
> > > +};
> > > +
> > > +/* dt_bus_match_any matches any fdt node, i.e. it always returns true */
> > > +extern int dt_bus_match_any(const struct dt_device *dev, int fdtnode);
> > > +
> > > +/* the processor bus (pbus) address type and register tuple */
> > > +typedef u64 dt_pbus_addr_t;
> > > +struct dt_pbus_reg {
> > > +	dt_pbus_addr_t addr;
> > > +	dt_pbus_addr_t size;
> > > +};
> > > +
> > > +static inline dt_pbus_addr_t dt_pbus_read_cells(u32 nr_cells, u32 *cells)
> > > +{
> > > +	return nr_cells == 2 ? ((u64)cells[0] << 32) | cells[1]
> > > +		: nr_cells == 1 ? cells[0] : (~0ULL);
> > 
> > Why the tertiary operator here?  A simple if or select statement should
> > make the code much easier to read and you don't seem to handle
> 
> I'll change it.
> 
> > #address-cells == 4or #size-cells == 4, despite the fact that you define
> > MAX_ADDRESS_CELLS=4 etc.  This seems a bit weird to me.
> 
> The default translator only knows how to handle nr_cells==2 or
> nr_cells==1. DT may have nodes with nr_cells > 2 (up to 4), but users
> will have to provide their own translators for those.
> 

ok

> > 
> > > +}
> > > +
> > > +/*
> > > + * dt_pbus_translate translates device node regs for the
> > > + * processor bus using the root node's #address-cells and
> > > + * #size-cells and dt_pbus_read_cells()
> > > + * returns
> > > + *  - zero on success
> > > + *  - a negative FDT_ERR_* value on failure
> > > + */
> > > +extern int dt_pbus_translate(const struct dt_device *dev, int regidx,
> > > +			     void *reg);
> > > +
> > > +/*
> > > + * dt_pbus_translate_node is the same as dt_pbus_translate but
> > > + * operates on an fdt node instead of a dt_device
> > > + */
> > > +extern int dt_pbus_translate_node(int fdtnode, int regidx, void *reg);
> > > +
> > > +/*
> > > + * dt_pbus_get_baseaddr is a shortcut for
> > > + *     dt_pbus_translate(dev, 0, &reg);
> > > + *     *base = reg.addr;
> > > + * returns
> > > + *  - zero on success
> > > + *  - a negative FDT_ERR_* value on failure
> > > + */
> > > +extern int dt_pbus_get_baseaddr(const struct dt_device *dev,
> > > +				dt_pbus_addr_t *base);
> > > +
> > > +/*
> > > + * dt_bus_init_defaults initializes @bus with
> > > + *  match		<- dt_bus_match_any
> > > + *  translate		<- dt_pbus_translate
> > > + *  nr_address_cells	<- #address-cells of the root node
> > > + *  nr_size_cells	<- #size-cells of the root node
> > > + */
> > > +extern void dt_bus_init_defaults(struct dt_bus *bus);
> > > +
> > > +/*
> > > + * dt_device_init initializes a dt_device with the given parameters
> > > + */
> > > +extern void dt_device_init(struct dt_device *dev, const struct dt_bus *bus,
> > > +			   const void *info);
> > > +
> > > +static inline void dt_device_bind_node(struct dt_device *dev, int fdtnode)
> > > +{
> > > +	dev->fdtnode = fdtnode;
> > > +}
> > > +
> > > +/*
> > > + * dt_device_find_compatible finds a @compatible node
> > > + * returns
> > > + *  - node (>= 0) on success
> > > + *  - a negative FDT_ERR_* value on failure
> > > + */
> > > +extern int dt_device_find_compatible(const struct dt_device *dev,
> > > +				     const char *compatible);
> > > +
> > > +/*
> > > + * dt_pbus_get_baseaddr_compatible simply bundles many functions into
> > > + * one. It finds the first @compatible fdt node and then translates the
> > > + * 0th reg tuple (the base address) using the processor bus translation,
> > > + * and finally stores that address in @baseaddr.
> > > + * returns
> > > + *  - zero on success
> > > + *  - a negative FDT_ERR_* value on failure
> > > + */
> > > +extern int dt_pbus_get_baseaddr_compatible(const char *compatible,
> > > +					   dt_pbus_addr_t *baseaddr);
> > > +
> > > +/**********************************************************************
> > > + * Low-level accessors for required node types and properties
> > > + **********************************************************************/
> > > +
> > > +/*
> > > + * dt_get_nr_cells sets @nr_address_cells and @nr_size_cells to the
> > > + * #address-cells and #size-cells properties of @fdtnode
> > > + * returns
> > > + *  - zero on success
> > > + *  - a negative FDT_ERR_* value on failure
> > > + */
> > > +extern int dt_get_nr_cells(int fdtnode, u32 *nr_address_cells,
> > > +					u32 *nr_size_cells);
> > > +
> > > +/* dt_reg is a structure for "raw" reg tuples */
> > > +#define MAX_ADDRESS_CELLS	4
> > > +#define MAX_SIZE_CELLS		4
> > > +struct dt_reg {
> > > +	u32 nr_address_cells, nr_size_cells;
> > > +	u32 address_cells[MAX_ADDRESS_CELLS];
> > > +	u32 size_cells[MAX_SIZE_CELLS];
> > > +};
> > > +
> > > +/*
> > > + * dt_reg_init initialize a dt_reg struct to zero and sets
> > > + * nr_address_cells and nr_size_cells to @nr_address_cells and
> > > + * @nr_size_cells respectively.
> > > + */
> > > +extern void dt_reg_init(struct dt_reg *reg, u32 nr_address_cells,
> > > +					    u32 nr_size_cells);
> > > +
> > > +/*
> > > + * dt_get_reg gets the @regidx'th reg tuple of @fdtnode's reg property
> > > + * and stores it in @reg. @reg must be initialized.
> > > + * returns
> > > + *  - zero on success
> > > + *  - a negative FDT_ERR_* value on failure
> > > + */
> > > +extern int dt_get_reg(int fdtnode, int regidx, struct dt_reg *reg);
> > > +
> > > +/**********************************************************************
> > > + * High-level accessors for required node types and properties
> > > + **********************************************************************/
> > > +
> > > +/*
> > > + * dt_get_bootargs gets a pointer to /chosen/bootargs
> > > + * returns
> > > + *  - zero on success
> > > + *  - a negative FDT_ERR_* value on failure
> > > + */
> > > +extern int dt_get_bootargs(const char **bootargs);
> > > +
> > > +/*
> > > + * dt_get_memory_params gets the memory parameters from the /memory node(s)
> > > + * storing each memory region ("address size" tuple) in consecutive entries
> > > + * of @regs, up to @nr_regs
> > > + * returns
> > > + *  - number of memory regions found on success
> > > + *  - a negative FDT_ERR_* value on failure
> > > + */
> > > +extern int dt_get_memory_params(struct dt_pbus_reg *regs, int nr_regs);
> > > +
> > > +/*
> > > + * dt_for_each_cpu_node runs @func on each cpu node in the /cpus node
> > > + * passing it its fdt node, its reg property value, and @info
> > > + *  - zero on success
> > > + *  - a negative FDT_ERR_* value on failure
> > > + */
> > > +extern int dt_for_each_cpu_node(void (*func)(int fdtnode, u32 regval,
> > > +				void *info), void *info);
> > > +
> > > +#endif /* _DEVICETREE_H_ */
> > > diff --git a/lib/libcflat.h b/lib/libcflat.h
> > > index 76448a33cde5f..99d1cd533dd03 100644
> > > --- a/lib/libcflat.h
> > > +++ b/lib/libcflat.h
> > > @@ -22,6 +22,8 @@
> > >  
> > >  #include <stdarg.h>
> > >  
> > > +#define __unused __attribute__((__unused__))
> > > +
> > >  typedef unsigned char u8;
> > >  typedef signed char s8;
> > >  typedef unsigned short u16;
> > > -- 
> > > 1.8.1.4
> > >