Re: [PATCH 11/17] add support for device trees

[Andrew Jones <drjones@redhat.com>]

On Sat, Feb 01, 2014 at 06:27:15PM -0800, Christoffer Dall wrote:
> On Tue, Jan 21, 2014 at 05:21:57PM +0100, Andrew Jones wrote:
> > Add some device tree functions, built on libfdt, to the common
> > code in order to facilitate the extraction of boot info and device
> > base addresses. These functions are generic and arch-neutral. It's
> > expected that more functions will be added as more information
> > from the device tree is needed.
> 
> You're doing a bit more than that, you're introducing a couple of
> concepts such as a bus and reg structures, which are concepts above DT,
> which could use a bit of clarification.  In particular this patch is a
> bit hard to review on its own, because you cannot see how things are
> supposed to be used without looking at subsequent patches...

True, but I'm not sure how I can improve things, other than add some
better documentation to the code in this patch, which I'll do for v4.

> 
> I'm not a DT expert, but here goes the best I can do...
> 
> > 
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > ---
> >  lib/devicetree.c | 257 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  lib/devicetree.h |  95 ++++++++++++++++++++
> >  lib/libcflat.h   |   1 +
> >  3 files changed, 353 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..a9ab7ed367704
> > --- /dev/null
> > +++ b/lib/devicetree.c
> > @@ -0,0 +1,257 @@
> > +#include "libcflat.h"
> > +#include "libfdt/libfdt.h"
> > +#include "devicetree.h"
> > +
> > +static const void *fdt;
> > +
> > +const char *dt_strerror(int errval)
> > +{
> > +	if (errval == -EINVAL)
> > +		return "Invalid argument";
> > +	return fdt_strerror(errval);
> > +}
> > +
> > +int dt_set(const void *fdt_ptr)
> > +{
> > +	int ret = fdt_check_header(fdt_ptr);
> > +	if (ret == 0)
> > +		fdt = fdt_ptr;
> > +	return ret;
> > +}
> > +
> > +const void *dt_get(void)
> > +{
> > +	return fdt;
> > +}
> > +
> > +int dt_get_bootargs_ptr(char **bootargs)
> > +{
> > +	const struct fdt_property *prop;
> > +	int node, err;
> > +
> > +	node = fdt_path_offset(fdt, "/chosen");
> > +	if (node < 0)
> > +		return node;
> > +
> > +	prop = fdt_get_property(fdt, node, "bootargs", &err);
> > +	if (prop)
> > +		*bootargs = (char *)prop->data;
> > +	else if (err != -FDT_ERR_NOTFOUND)
> > +		return err;
> > +
> > +	return 0;
> > +}
> > +
> > +int dt_bus_default_match(const struct dt_bus *bus __unused,
> > +			 int nodeoffset __unused)
> > +{
> > +	/* just select first node found */
> > +	return 1;
> > +}
> > +
> > +int dt_bus_default_translate(const struct dt_bus *bus __unused,
> > +			     struct dt_reg *reg, void **addr, size_t *size)
> > +{
> 
> I don't think you want addr to be a void **, but a phys_addr_t, see note
> about ARM 32-bit LPAE below.

Oh right. I'll get this straightened out.

> 
> > +	u64 temp64;
> > +
> > +	if (!reg || !addr)
> > +		return -EINVAL;
> 
> defensive programming?

yeah, on second thought, it doesn't make sense to worry about this.
I'll rip all of it out.

> 
> > +
> > +	/*
> > +	 * default translate only understands u32 (<1> <1>) and
> > +	 * u64 (<2> <1>|<2>) addresses
> > +	 */
> > +	if (reg->nr_address_cells < 1
> > +			|| reg->nr_address_cells > 2
> > +			|| reg->nr_size_cells < 1
> > +			|| reg->nr_size_cells > 2)
> > +		return -EINVAL;
> > +
> > +	if (reg->nr_address_cells == 2)
> > +		temp64 = ((u64)reg->address_cells[0] << 32)
> > +					| reg->address_cells[1];
> > +	else
> > +		temp64 = reg->address_cells[0];
> 
> I would use braces on these statements given the multi-line first
> statement.
> 
> > +
> > +	/*
> > +	 * If we're 32-bit, then the upper word of a two word
> > +	 * address better be zero.
> > +	 */
> 
> not necessarily, on ARM 32-bit LPAE you have 40-bit physical addresses
> but 32-bit virtual address pointers...
> 
> > +	if (sizeof(void *) == sizeof(u32) && reg->nr_address_cells > 1
> > +			&& reg->address_cells[0] != 0)
> > +		return -EINVAL;
> > +
> > +	*addr = (void *)(unsigned long)temp64;
> > +
> > +	if (size) {
> > +		if (reg->nr_size_cells == 2)
> > +			temp64 = ((u64)reg->size_cells[0] << 32)
> > +						| reg->size_cells[1];
> > +		else
> > +			temp64 = reg->size_cells[0];
> > +
> > +		if (sizeof(size_t) == sizeof(u32) && reg->nr_size_cells > 1
> > +				&& reg->size_cells[0] != 0)
> > +			return -EINVAL;
> 
> same as above.
> 
> > +
> > +		*size = (size_t)temp64;
> 
> hmmm, I feel like I just read this code.  Maybe you can have a static
> little helper funciton to actually read your values.

ok

> 
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +const struct dt_bus dt_default_bus = {
> > +	.name = "default",
> > +	.match = dt_bus_default_match,
> > +	.translate = dt_bus_default_translate,
> > +};
> > +
> > +void dt_bus_init_defaults(struct dt_bus *bus, const char *name)
> > +{
> > +	*bus = dt_default_bus;
> > +	bus->name = name;
> > +}
> > +
> > +int dt_bus_find_device_compatible(const struct dt_bus *bus,
> > +				  const char *compatible)
> > +{
> > +	int node, ret;
> > +
> > +	if (!bus || !bus->match)
> > +		return -EINVAL;
> 
> see __dt_bus_translate_reg below
> 
> > +
> > +	node = fdt_node_offset_by_compatible(fdt, -1, compatible);
> > +
> > +	while (node >= 0) {
> > +		if ((ret = bus->match(bus, node)) < 0)
> > +			return ret;
> > +		else if (ret)
> > +			break;
> > +		node = fdt_node_offset_by_compatible(fdt, node, compatible);
> > +	}
> > +
> > +	return node;
> > +}
> > +
> > +static int __dt_get_num_cells(int node, u32 *address_cells, u32 *size_cells)
> > +{
> > +	const struct fdt_property *prop;
> > +	u32 *data;
> > +	int err;
> > +
> > +	prop = fdt_get_property(fdt, node, "#address-cells", &err);
> > +	if (!prop && err == -FDT_ERR_NOTFOUND) {
> > +
> > +		node = fdt_parent_offset(fdt, node);
> > +		if (node < 0)
> > +			return node;
> > +
> > +		return __dt_get_num_cells(node, address_cells, size_cells);
> 
> you're doing recursive calls with a severly limited stack space, you
> should probably at least check your depth and catch errors at a
> reasonable level.

Good point on the stack use. I'll get rid of the recursion.

> 
> It's probably cleaner to just have an iterative lookup of the node with
> #address_cells in it (in which you can also verify that is also has a
> #size-cells in it) and then pass that to the function that actually
> reads it.
> 
> > +
> > +	} else if (!prop) {
> > +		return err;
> > +	}
> > +
> > +	data = (u32 *)prop->data;
> > +	*address_cells = fdt32_to_cpu(*data);
> > +
> > +	prop = fdt_get_property(fdt, node, "#size-cells", &err);
> > +	if (!prop) {
> > +		printf("we can read #address-cells, but not #size-cells?\n");
> > +		return err;
> > +	}
> > +
> > +	data = (u32 *)prop->data;
> > +	*size_cells = fdt32_to_cpu(*data);
> > +
> > +	return 0;
> > +}
> > +
> > +int dt_get_num_cells(int nodeoffset, u32 *address_cells, u32 *size_cells)
> > +{
> > +	if (!address_cells || !size_cells)
> > +		return -EINVAL;
> 
> defensive programming?
> 
> > +	return __dt_get_num_cells(nodeoffset, address_cells, size_cells);
> > +}
> > +
> > +int dt_get_reg(int nodeoffset, int regidx, u32 *address_cells,
> > +	       u32 *size_cells, struct dt_reg *reg)
> > +{
> > +	const struct fdt_property *prop;
> > +	u32 *data, regsz, i;
> > +	int err;
> > +
> > +	if (!address_cells || !size_cells || !reg)
> > +		return -EINVAL;
> 
> defensive programming?
> 
> > +
> > +	memset(reg, 0, sizeof(struct dt_reg));
> 
> sizeof(*reg)
> 
> > +
> > +	/*
> > +	 * We assume #size-cells == 0 means translation is impossible,
> 
> hmm, this won't work for the cpus' reg properties for example.  I would
> just assume (and document) that this function always takes valid
> address_cells and size_cells (as u32 instead of u32 *).  I get that
> you're trying to do some caching (from reading later patches), but that
> should still be possible to do externally.

cpus will need their own 'dt_get_cpuids', built on a 'dt_for_each_cpu',
or some such, and wouldn't use this function. Other than cpus, I think
this is a safe convention. Although, that said, I'll look into cleaning
up the interface a bit, while maintaining ease of use and the ability to
cache.

> 
> If you really really want to, then you should use address_cells, but I'm
> not sure if it's actually valid for any bindings to have address_cells
> == 0 and size_cells >0.  If you have conventions like this, it should be
> clearly documented on the function itself!
> 
> > +	 * reserving it to indicate that we don't know what #address-cells
> > +	 * and #size-cells are yet, and thus must try to get them from the
> > +	 * parent.
> > +	 */
> > +	if (*size_cells == 0 && (err = dt_get_num_cells(nodeoffset,
> > +					address_cells, size_cells)) < 0)
> > +		return err;
> > +
> > +	prop = fdt_get_property(fdt, nodeoffset, "reg", &err);
> > +	if (prop == NULL)
> > +		return err;
> > +
> > +	regsz = (*address_cells + *size_cells) * sizeof(u32);
> > +
> > +	if ((regidx + 1) * regsz > prop->len)
> > +		return -EINVAL;
> > +
> > +	data = (u32 *)(prop->data + regidx * regsz);
> > +
> > +	for (i = 0; i < *address_cells; ++i, ++data)
> > +		reg->address_cells[i] = fdt32_to_cpu(*data);
> > +	for (i = 0; i < *size_cells; ++i, ++data)
> > +		reg->size_cells[i] = fdt32_to_cpu(*data);
> > +
> > +	reg->nr_address_cells = *address_cells;
> > +	reg->nr_size_cells = *size_cells;
> > +
> > +	return 0;
> > +}
> > +
> > +int __dt_bus_translate_reg(int nodeoffset, const struct dt_bus *bus,
> > +			   int regidx, u32 *address_cells, u32 *size_cells,
> > +			   void **addr, size_t *size)
> > +{
> > +	struct dt_reg reg;
> > +	int ret;
> > +
> > +	if (!bus || !bus->translate)
> > +		return -EINVAL;
> 
> The !bus seems overly defensive again.
> 
> The !bus->translate may be helpful here, but since we're returning a
> bunch of -EINVALs you should probably put a debug statement in the error
> catching part, so devs can easily enable debugging and figure out where
> things are going wrong.
> 
> That is, if anyone would ever call this with a bus without translate on
> there...
> 
> > +
> > +	ret = dt_get_reg(nodeoffset, regidx, address_cells, size_cells, &reg);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	return bus->translate(bus, &reg, addr, size);
> > +}
> > +
> > +int dt_bus_translate_reg(int nodeoffset, const struct dt_bus *bus,
> > +			 int regidx, void **addr, size_t *size)
> > +{
> > +	/*
> > +	 * size_cells == 0 tells dt_get_reg to get address_cells
> > +	 * and size_cells from the parent node
> > +	 */
> > +	u32 address_cells, size_cells = 0;
> > +	return __dt_bus_translate_reg(nodeoffset, bus, regidx,
> > +			&address_cells, &size_cells, addr, size);
> > +}
> > +
> > +int dt_get_memory_params(void **start, size_t *size)
> 
> again, memory can exit beyond the 32-bit mark on ARM LPAE systems.
> 
> > +{
> > +	int node = fdt_path_offset(fdt, "/memory");
> > +	if (node < 0)
> > +		return node;
> > +
> > +	return dt_bus_translate_reg(node, &dt_default_bus, 0, start, size);
> > +}
> > diff --git a/lib/devicetree.h b/lib/devicetree.h
> > new file mode 100644
> > index 0000000000000..1f2d61b46a308
> > --- /dev/null
> > +++ b/lib/devicetree.h
> > @@ -0,0 +1,95 @@
> > +#ifndef _DEVICETREE_H_
> > +#define _DEVICETREE_H_
> > +#include "libcflat.h"
> > +
> > +/*
> > + * set/get the fdt pointer
> > + */
> > +extern int dt_set(const void *fdt_ptr);
> > +extern const void *dt_get(void);
> > +
> > +/*
> > + * bootinfo accessors
> > + */
> > +extern int dt_get_bootargs_ptr(char **bootargs);
> > +extern int dt_get_memory_params(void **start, size_t *size);
> 
> There's no documentation to these functions?

I was thinking the names were self-documenting, but can add comments
above them with a couple more details.

> 
> > +
> > +#define MAX_ADDRESS_CELLS	4
> > +#define MAX_SIZE_CELLS		4
> > +struct dt_reg {
> > +	u32 nr_address_cells;
> > +	u32 nr_size_cells;
> > +	u32 address_cells[MAX_ADDRESS_CELLS];
> > +	u32 size_cells[MAX_SIZE_CELLS];
> > +};
> 
> dt_reg?  Is this for dt-bindings that specify the reg property?  A
> comment would be nice.

I would have liked this to be private to devicetree.c, but as
translate is bus specific we need a way for buses to get "raw"
regs from the dt. I'll comment this better.

> 
> > +
> > +struct dt_bus {
> > +	const char *name;
> > +	int (*match)(const struct dt_bus *bus, int nodeoffset);
> > +	/*
> > +	 * match() returns
> > +	 *  - a positive value on match
> > +	 *  - zero on no match
> > +	 *  - a negative value on error
> > +	 */
> 
> what does match do?  (I know by reading the code, but it should be
> documented)
> 
> consider using defines for the return values, so your client code can
> be:
> 
> if (bus->match(bus, node) == BUS_MATCH_SUCCESS)
> 	break;

ok

> 
> > +	int (*translate)(const struct dt_bus *bus, struct dt_reg *reg,
> > +			  void **addr, size_t *size);
> > +	/*
> > +	 * translate() returns
> > +	 *  - zero on success
> > +	 *  - a negative value on error
> > +	 */
> 
> ditto
> 
> > +	void *private;
> 
> what is this field meant to contain?  A hint about drivers passing
> information to translate() and match() would be helpful.

ok

> 
> > +};
> > +
> > +extern const struct dt_bus dt_default_bus;
> > +extern void dt_bus_init_defaults(struct dt_bus *bus, const char *name);
> > +extern int dt_bus_default_match(const struct dt_bus *bus, int nodeoffset);
> > +extern int dt_bus_default_translate(const struct dt_bus *bus,
> > +				    struct dt_reg *reg, void **addr,
> > +				    size_t *size);
> > +
> > +/*
> > + * find an fdt device node compatible with @compatible using match()
> > + * from the given bus @bus.
> > + */
> > +extern int dt_bus_find_device_compatible(const struct dt_bus *bus,
> > +					 const char *compatible);
> > +
> > +/*
> > + * translate the reg indexed by @regidx of the "reg" property of the
> > + * device node at @nodeoffset using translate() from the given bus @bus.
> > + * returns the translation in @addr and @size
> > + */
> 
> define 'translate' in the comment (into an address).

ok

> 
> > +extern int dt_bus_translate_reg(int nodeoffset, const struct dt_bus *bus,
> > +				int regidx, void **addr, size_t *size);
> > +
> > +/*
> > + * same as dt_bus_translate_reg, but uses the given @address_cells and
> > + * @size_cells rather than pulling them from the parent of @nodeoffset
> > + */
> > +extern int __dt_bus_translate_reg(int nodeoffset, const struct dt_bus *bus,
> > +				  int regidx, u32 *address_cells,
> > +				  u32 *size_cells, void **addr,
> > +				  size_t *size);
> > +
> > +/*
> > + * read the "reg" property of @nodeoffset, which is defined by @address_cells
> > + * and @size_cells, and store the reg indexed by @regidx into @reg
> > + */
> > +extern int dt_get_reg(int nodeoffset, int regidx, u32 *address_cells,
> > +		      u32 *size_cells, struct dt_reg *reg);
> > +
> > +/*
> > + * searches up the devicetree for @address_cells and @size_cells,
> > + * starting from @nodeoffset
> > + */
> > +extern int dt_find_num_cells(int nodeoffset, u32 *address_cells,
> > +			    u32 *size_cells);
> 
> not defined?

Oops, s/_find_/_get_/

> 
> > +
> > +/*
> > + * convert devicetree errors to strings
> > + */
> > +extern const char *dt_strerror(int errval);
> > +
> > +#endif
> > diff --git a/lib/libcflat.h b/lib/libcflat.h
> > index 2cde64a560956..fdaaf2a8ab31d 100644
> > --- a/lib/libcflat.h
> > +++ b/lib/libcflat.h
> > @@ -61,6 +61,7 @@ extern long atol(const char *ptr);
> >  
> >  #define offsetof(TYPE, MEMBER) __builtin_offsetof (TYPE, MEMBER)
> >  
> > +#define __unused __attribute__((__unused__))
> >  #define NULL ((void *)0UL)
> >  #include "errno.h"
> >  #endif
> > -- 
> > 1.8.1.4
> > 
> 
> Thanks,
> -- 
> Christoffer