| Message ID | 1373661422-23606-2-git-send-email-peter.maydell@linaro.org |
|---|---|
| State | New |
| Headers | show |
Hi Peter, On Sat, Jul 13, 2013 at 6:36 AM, Peter Maydell <peter.maydell@linaro.org> wrote: > We already have a qemu_devtree_setprop_cells() which sets a dtb > property to an array of cells whose values are specified by varargs. > However for the fairly common case of setting a property to a list > of addresses or of address,size pairs the number of cells used by > each element in the list depends on the parent's #address-cells > and #size-cells properties. To make this easier we provide an analogous > qemu_devtree_setprop_sized_cells() macro which allows the number > of cells used by each element to be specified. This is implemented > using an underlying qemu_devtree_setprop_sized_cells_from_array() > function which takes the values and sizes as an array; this may > also be directly useful for cases where the cell contents are > constructed programmatically. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > device_tree.c | 31 +++++++++++++++++++++ > include/sysemu/device_tree.h | 62 ++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 93 insertions(+) > > diff --git a/device_tree.c b/device_tree.c > index 10cf3d0..308205c 100644 > --- a/device_tree.c > +++ b/device_tree.c > @@ -308,3 +308,34 @@ void qemu_devtree_dumpdtb(void *fdt, int size) > exit(g_file_set_contents(dumpdtb, fdt, size, NULL) ? 0 : 1); > } > } > + > +int qemu_devtree_setprop_sized_cells_from_array(void *fdt, > + const char *node_path, > + const char *property, > + int numvalues, > + uint64_t *values) > +{ > + uint32_t *propcells; > + uint64_t value; > + int cellnum, vnum, ncells; > + uint32_t hival; > + > + propcells = g_new0(uint32_t, numvalues * 2); > + > + cellnum = 0; > + for (vnum = 0; vnum < numvalues; vnum++) { > + ncells = values[vnum * 2]; > + assert(ncells == 1 || ncells == 2); > + value = values[vnum * 2 + 1]; > + hival = cpu_to_be32(value >> 32); > + if (ncells > 1) { > + propcells[cellnum++] = hival; > + } else if (hival != 0) { > + return vnum + 1; > + } > + propcells[cellnum++] = cpu_to_be32(value); > + } > + > + return qemu_devtree_setprop(fdt, node_path, property, propcells, > + cellnum * sizeof(uint32_t)); > +} > diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h > index f0b3f35..8189ebc 100644 > --- a/include/sysemu/device_tree.h > +++ b/include/sysemu/device_tree.h > @@ -51,4 +51,66 @@ int qemu_devtree_add_subnode(void *fdt, const char *name); > > void qemu_devtree_dumpdtb(void *fdt, int size); > > +/** > + * qemu_devtree_setprop_sized_cells_from_array: > + * @fdt: device tree blob > + * @node_path: node to set property on > + * @property: property to set > + * @numvalues: number of values > + * @values: array of number-of-cells, value pairs > + * > + * Set the specified property on the specified node in the device tree > + * to be an array of cells. The values of the cells are specified via > + * the values list, which alternates between "number of cells used by > + * this value" and "value". > + * number-of-cells must be either 1 or 2 (other values will assert()). > + * > + * This function is useful because device tree nodes often have cell arrays > + * which are either lists of addresses or lists of address,size tuples, but > + * the number of cells used for each element vary depending on the > + * #address-cells and #size-cells properties of their parent node. > + * If you know all your cell elements are one cell wide you can use the > + * simpler qemu_devtree_setprop_cells(). If you're not setting up the > + * array programmatically, qemu_devtree_setprop_sized_cells may be more > + * convenient. > + * > + * Return value: 0 on success, <0 if setting the property failed, > + * n (for n>0) if value n wouldn't fit in the required number of cells. > + * (This slightly odd convention is for the benefit of callers who might > + * wish to print different error messages depending on which value > + * was too large to fit, since values might be user-specified.) I have patches on list converting the other "setprop" APIs to Error ** based reporting (as per your suggestion). Once that change happens we will have two separate error schemes for the one fn. For sanity checking user specified values (e.g. command line args) cant it just be done explicitly as required? It seems to me that this will be the exception not the rule, as for DTB generation from machine model a fail here will be a fatal anyway (better handled by Error **). if (acells < 2 && addr >= (1ull << 32)) { fprintf("your address is too big for this dtb"); } LOC looks to be similar and this API gets a little cleaner. Regards, Peter
On 15 July 2013 01:30, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: > I have patches on list converting the other "setprop" APIs to Error ** based > reporting (as per your suggestion). Once that change happens we will have > two separate error schemes for the one fn. > > For sanity checking user specified values (e.g. command line args) cant it > just be done explicitly as required? It seems to me that this will be the > exception not the rule, as for DTB generation from machine model a fail > here will be a fatal anyway (better handled by Error **). > > if (acells < 2 && addr >= (1ull << 32)) { > fprintf("your address is too big for this dtb"); > } > > LOC looks to be similar and this API gets a little cleaner. It's trying to allow the arm/boot code to continue to distinguish the two error cases it does currently: * RAM start address doesn't fit [a board model error] * RAM size doesn't fit [a user error for which a specific error message is more friendly and helpful] It's a fatal error in both cases, but the text is different. I guess I could put a special purpose "if scells is 1 and ram_size > 4GB then fail early" check in boot.c... I wanted to catch "user is using a DTB with only 32 bit addresses but tried a big fat RAM size", because some earlier versions of vexpress DTBs did only use 32 bit cells. -- PMM
Hi Peter, On Mon, Jul 15, 2013 at 7:44 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 15 July 2013 01:30, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: >> I have patches on list converting the other "setprop" APIs to Error ** based >> reporting (as per your suggestion). Once that change happens we will have >> two separate error schemes for the one fn. >> >> For sanity checking user specified values (e.g. command line args) cant it >> just be done explicitly as required? It seems to me that this will be the >> exception not the rule, as for DTB generation from machine model a fail >> here will be a fatal anyway (better handled by Error **). >> >> if (acells < 2 && addr >= (1ull << 32)) { >> fprintf("your address is too big for this dtb"); >> } >> >> LOC looks to be similar and this API gets a little cleaner. > > It's trying to allow the arm/boot code to continue to distinguish > the two error cases it does currently: > * RAM start address doesn't fit [a board model error] > * RAM size doesn't fit [a user error for which a specific > error message is more friendly and helpful] > > It's a fatal error in both cases, but the text is different. > > I guess I could put a special purpose "if scells is 1 and > ram_size > 4GB then fail early" check in boot.c... > That was what I thinking with the above snippet. It goes with the caller in this case arm/boot.c. You still need to switch on the return code in the caller for your multiple error messages so I dont think you save too much save over just sanity checking args beforehand in boot.c Regards, Peter > I wanted to catch "user is using a DTB with only 32 bit > addresses but tried a big fat RAM size", because some earlier > versions of vexpress DTBs did only use 32 bit cells. > > -- PMM >
On 15 July 2013 11:24, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: > On Mon, Jul 15, 2013 at 7:44 PM, Peter Maydell <peter.maydell@linaro.org> wrote: >> It's trying to allow the arm/boot code to continue to distinguish >> the two error cases it does currently: >> * RAM start address doesn't fit [a board model error] >> * RAM size doesn't fit [a user error for which a specific >> error message is more friendly and helpful] >> >> It's a fatal error in both cases, but the text is different. >> >> I guess I could put a special purpose "if scells is 1 and >> ram_size > 4GB then fail early" check in boot.c... > That was what I thinking with the above snippet. It goes with the > caller in this case arm/boot.c. You still need to switch on the return > code in the caller for your multiple error messages so I dont think > you save too much save over just sanity checking args beforehand in > boot.c Yeah, you've convinced me -- it's not worth complicating the heck out of the API for the generic function; I'll put an explicit check in boot.c -- PMM
diff --git a/device_tree.c b/device_tree.c index 10cf3d0..308205c 100644 --- a/device_tree.c +++ b/device_tree.c @@ -308,3 +308,34 @@ void qemu_devtree_dumpdtb(void *fdt, int size) exit(g_file_set_contents(dumpdtb, fdt, size, NULL) ? 0 : 1); } } + +int qemu_devtree_setprop_sized_cells_from_array(void *fdt, + const char *node_path, + const char *property, + int numvalues, + uint64_t *values) +{ + uint32_t *propcells; + uint64_t value; + int cellnum, vnum, ncells; + uint32_t hival; + + propcells = g_new0(uint32_t, numvalues * 2); + + cellnum = 0; + for (vnum = 0; vnum < numvalues; vnum++) { + ncells = values[vnum * 2]; + assert(ncells == 1 || ncells == 2); + value = values[vnum * 2 + 1]; + hival = cpu_to_be32(value >> 32); + if (ncells > 1) { + propcells[cellnum++] = hival; + } else if (hival != 0) { + return vnum + 1; + } + propcells[cellnum++] = cpu_to_be32(value); + } + + return qemu_devtree_setprop(fdt, node_path, property, propcells, + cellnum * sizeof(uint32_t)); +} diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h index f0b3f35..8189ebc 100644 --- a/include/sysemu/device_tree.h +++ b/include/sysemu/device_tree.h @@ -51,4 +51,66 @@ int qemu_devtree_add_subnode(void *fdt, const char *name); void qemu_devtree_dumpdtb(void *fdt, int size); +/** + * qemu_devtree_setprop_sized_cells_from_array: + * @fdt: device tree blob + * @node_path: node to set property on + * @property: property to set + * @numvalues: number of values + * @values: array of number-of-cells, value pairs + * + * Set the specified property on the specified node in the device tree + * to be an array of cells. The values of the cells are specified via + * the values list, which alternates between "number of cells used by + * this value" and "value". + * number-of-cells must be either 1 or 2 (other values will assert()). + * + * This function is useful because device tree nodes often have cell arrays + * which are either lists of addresses or lists of address,size tuples, but + * the number of cells used for each element vary depending on the + * #address-cells and #size-cells properties of their parent node. + * If you know all your cell elements are one cell wide you can use the + * simpler qemu_devtree_setprop_cells(). If you're not setting up the + * array programmatically, qemu_devtree_setprop_sized_cells may be more + * convenient. + * + * Return value: 0 on success, <0 if setting the property failed, + * n (for n>0) if value n wouldn't fit in the required number of cells. + * (This slightly odd convention is for the benefit of callers who might + * wish to print different error messages depending on which value + * was too large to fit, since values might be user-specified.) + */ +int qemu_devtree_setprop_sized_cells_from_array(void *fdt, + const char *node_path, + const char *property, + int numvalues, + uint64_t *values); + +/** + * qemu_devtree_setprop_sized_cells: + * @fdt: device tree blob + * @node_path: node to set property on + * @property: property to set + * @...: list of number-of-cells, value pairs + * + * Set the specified property on the specified node in the device tree + * to be an array of cells. The values of the cells are specified via + * the variable arguments, which alternates between "number of cells + * used by this value" and "value". + * + * This is a convenience wrapper for the function + * qemu_devtree_setprop_sized_cells_from_array(). + * + * Return value: 0 on success, <0 if setting the property failed, + * n (for n>0) if value n wouldn't fit in the required number of cells. + */ +#define qemu_devtree_setprop_sized_cells(fdt, node_path, property, ...) \ + ({ \ + uint64_t qdt_tmp[] = { __VA_ARGS__ }; \ + qemu_devtree_setprop_sized_cells_from_array(fdt, node_path, \ + property, \ + ARRAY_SIZE(qdt_tmp) / 2, \ + qdt_tmp); \ + }) + #endif /* __DEVICE_TREE_H__ */
We already have a qemu_devtree_setprop_cells() which sets a dtb property to an array of cells whose values are specified by varargs. However for the fairly common case of setting a property to a list of addresses or of address,size pairs the number of cells used by each element in the list depends on the parent's #address-cells and #size-cells properties. To make this easier we provide an analogous qemu_devtree_setprop_sized_cells() macro which allows the number of cells used by each element to be specified. This is implemented using an underlying qemu_devtree_setprop_sized_cells_from_array() function which takes the values and sizes as an array; this may also be directly useful for cases where the cell contents are constructed programmatically. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- device_tree.c | 31 +++++++++++++++++++++ include/sysemu/device_tree.h | 62 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 93 insertions(+)