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

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

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...

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.

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

defensive programming?

> +
> +	/*
> +	 * 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.

> +	}
> +
> +	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.

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.

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?

> +
> +#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.

> +
> +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;

> +	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.

> +};
> +
> +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).

> +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?

> +
> +/*
> + * 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