| Message ID | 1390321323-1855-12-git-send-email-drjones@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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, ®); > + 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? > + > +#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,
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, ®); > > + 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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Feb 03, 2014 at 04:31:24PM +0100, Andrew Jones wrote: > 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: [...] > > > + > > > +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. > well, your function is called dt_get_reg and is generally exported from lib/devicetree.c, so there's nothing here suggesting this can only be used for a reg property for a certain type of devices. cpus is an example of a node that has #size-cells == 0 and has a perfectly valid reg property. The fact that you don't plan on using this function to iterate CPUs doesn't mean that somebody else will not, or we will add another binding that will use same convention, and now this funciton has to be refactored. If this function was virtio_get_reg, it would be a different story... > > > > 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! > > [...] Thanks, -Christoffer -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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) +{ + u64 temp64; + + if (!reg || !addr) + return -EINVAL; + + /* + * 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]; + + /* + * If we're 32-bit, then the upper word of a two word + * address better be zero. + */ + 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; + + *size = (size_t)temp64; + } + + 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; + + 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); + + } 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; + 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; + + memset(reg, 0, sizeof(struct dt_reg)); + + /* + * We assume #size-cells == 0 means translation is impossible, + * 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; + + 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) +{ + 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); + +#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]; +}; + +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 + */ + 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 + */ + void *private; +}; + +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 + */ +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); + +/* + * 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
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. 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