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@xxxxxxxxxx>
> > ---
> > 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, ®);
> > + if (ret < 0)
> > + return ret;
> > +
> > + return bus->translate(bus, ®, 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
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm
