| Message ID | 1372069376-30640-2-git-send-email-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
On 24.06.2013, at 12:22, Peter Maydell 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() function which allows the number > of cells used by each element to be specified. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> This looks pretty complicated for something actually quite simple: All you want is to pass in a number of 64bit values, rather than 32bit ones, right? There's got to be an easier way to implement that. Alex > --- > device_tree.c | 48 ++++++++++++++++++++++++++++++++++++++++++ > include/sysemu/device_tree.h | 29 +++++++++++++++++++++++++ > 2 files changed, 77 insertions(+) > > diff --git a/device_tree.c b/device_tree.c > index 69be9da..15a081c 100644 > --- a/device_tree.c > +++ b/device_tree.c > @@ -319,3 +319,51 @@ void qemu_devtree_dumpdtb(void *fdt, int size) > } > > } > + > +int qemu_devtree_setprop_sized_cells(void *fdt, const char *node_path, > + const char *property, ...) > +{ > + uint32_t *propcells; > + uint32_t propsize = 0; > + va_list ap; > + uint32_t ncells; > + uint64_t value; > + int i, argnum; > + > + va_start(ap, property); > + for (;;) { > + ncells = va_arg(ap, uint32_t); > + if (!ncells) { > + break; > + } > + assert(ncells < 3); > + propsize += ncells; > + value = va_arg(ap, uint64_t); > + } > + va_end(ap); > + > + propcells = g_new0(uint32_t, propsize); > + > + i = 0; > + va_start(ap, property); > + for (argnum = 1; ; argnum++) { > + uint32_t hival; > + > + ncells = va_arg(ap, uint32_t); > + if (!ncells) { > + break; > + } > + value = va_arg(ap, uint64_t); > + hival = cpu_to_be32(value >> 32); > + if (ncells > 1) { > + propcells[i++] = hival; > + } else if (hival != 0) { > + return argnum; > + } > + propcells[i++] = cpu_to_be32(value); > + } > + va_end(ap); > + > + return qemu_devtree_setprop(fdt, node_path, property, propcells, > + propsize * sizeof(uint32_t)); > +} > diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h > index f0b3f35..d7691f9 100644 > --- a/include/sysemu/device_tree.h > +++ b/include/sysemu/device_tree.h > @@ -51,4 +51,33 @@ int qemu_devtree_add_subnode(void *fdt, const char *name); > > void qemu_devtree_dumpdtb(void *fdt, int size); > > +/** > + * qemu_devtree_setprop_sized_cells: > + * @fdt: device tree blob > + * @node_path: node to set property on > + * @property: property to set > + * @...: 0-terminated list of uint32_t number-of-cells, uint64_t 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 varargs list, which alternates between "number of cells used by > + * this value" and "value" (terminated when number-of-cells is zero). > + * 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(). > + * > + * 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(void *fdt, const char *node_path, > + const char *property, ...); > + > #endif /* __DEVICE_TREE_H__ */ > -- > 1.7.9.5 >
On 24 June 2013 11:56, Alexander Graf <agraf@suse.de> wrote: > On 24.06.2013, at 12:22, Peter Maydell 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() function which allows the number >> of cells used by each element to be specified. >> >> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > > This looks pretty complicated for something actually quite > simple: All you want is to pass in a number of 64bit values, > rather than 32bit ones, right? Nope. If the device tree blob says #address-cells is 1 and #size-cells is 1, then I want to pass in values to go in 32 bit cells. If it says #address-cells is 2 but #size-cells is still 1, then I want to pass in a value for a 64 bit cell then one for a 32 bit cell. If they're both 2 then I want to pass in values for two 64 bit cells. It's pretty complicated because the device tree spec is pretty complicated (if it had just mandated 64 bit addr/size everywhere then this would be easy). -- PMM
On Mon, Jun 24, 2013 at 12:02:39PM +0100, Peter Maydell wrote: > On 24 June 2013 11:56, Alexander Graf <agraf@suse.de> wrote: > > On 24.06.2013, at 12:22, Peter Maydell 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() function which allows the number > >> of cells used by each element to be specified. > >> > >> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > > > > This looks pretty complicated for something actually quite > > simple: All you want is to pass in a number of 64bit values, > > rather than 32bit ones, right? > > Nope. If the device tree blob says #address-cells is 1 > and #size-cells is 1, then I want to pass in values to > go in 32 bit cells. If it says #address-cells is 2 but > #size-cells is still 1, then I want to pass in a value > for a 64 bit cell then one for a 32 bit cell. If they're > both 2 then I want to pass in values for two 64 bit > cells. Hmm.. the property certainly needs to be constructed that way. But I think Alex's point is that you could make the arguments all 64-bit, and then truncate them in the generated property. There's a bigger problem, though, that exists with both versions. In general people expect integer arguments like this not to care too much about the exact integer type, because it will be promoted to the right thing. Except with varargs it won't. So if you ever have a notionally 64-bit address that happens to fit in a 32-bit variable and you pass that it, this function will be broken. And the worst of it is, it'll work most of the time, until you happen to hit the wrong ABI and parameter combination :(. > It's pretty complicated because the device tree > spec is pretty complicated (if it had just mandated > 64 bit addr/size everywhere then this would be easy). Actually, no, because #address-cells and #size-cells cover things other than "memory-like" addresses. e.g. #address-cells is 3 for PCI, which wouldn't be covered by 64-bit addressing.
On 26 June 2013 00:38, David Gibson <david@gibson.dropbear.id.au> wrote: > On Mon, Jun 24, 2013 at 12:02:39PM +0100, Peter Maydell wrote: >> On 24 June 2013 11:56, Alexander Graf <agraf@suse.de> wrote: >> > This looks pretty complicated for something actually quite >> > simple: All you want is to pass in a number of 64bit values, >> > rather than 32bit ones, right? >> >> Nope. If the device tree blob says #address-cells is 1 >> and #size-cells is 1, then I want to pass in values to >> go in 32 bit cells. If it says #address-cells is 2 but >> #size-cells is still 1, then I want to pass in a value >> for a 64 bit cell then one for a 32 bit cell. If they're >> both 2 then I want to pass in values for two 64 bit >> cells. > > Hmm.. the property certainly needs to be constructed that way. But I > think Alex's point is that you could make the arguments all 64-bit, > and then truncate them in the generated property. Er, the arguments *are* all 64 bits and truncated in the generated property: + * @...: 0-terminated list of uint32_t number-of-cells, uint64_t value pairs > There's a bigger problem, though, that exists with both versions. In > general people expect integer arguments like this not to care too much > about the exact integer type, because it will be promoted to the right > thing. Except with varargs it won't. So if you ever have a > notionally 64-bit address that happens to fit in a 32-bit variable and > you pass that it, this function will be broken. And the worst of it > is, it'll work most of the time, until you happen to hit the wrong ABI > and parameter combination :(. Do you have a suggested improvement to the API to avoid this? thanks -- PMM
On 26.06.2013, at 10:49, Peter Maydell wrote: > On 26 June 2013 00:38, David Gibson <david@gibson.dropbear.id.au> wrote: >> On Mon, Jun 24, 2013 at 12:02:39PM +0100, Peter Maydell wrote: >>> On 24 June 2013 11:56, Alexander Graf <agraf@suse.de> wrote: >>>> This looks pretty complicated for something actually quite >>>> simple: All you want is to pass in a number of 64bit values, >>>> rather than 32bit ones, right? >>> >>> Nope. If the device tree blob says #address-cells is 1 >>> and #size-cells is 1, then I want to pass in values to >>> go in 32 bit cells. If it says #address-cells is 2 but >>> #size-cells is still 1, then I want to pass in a value >>> for a 64 bit cell then one for a 32 bit cell. If they're >>> both 2 then I want to pass in values for two 64 bit >>> cells. >> >> Hmm.. the property certainly needs to be constructed that way. But I >> think Alex's point is that you could make the arguments all 64-bit, >> and then truncate them in the generated property. > > Er, the arguments *are* all 64 bits and truncated > in the generated property: > + * @...: 0-terminated list of uint32_t number-of-cells, uint64_t value pairs > >> There's a bigger problem, though, that exists with both versions. In >> general people expect integer arguments like this not to care too much >> about the exact integer type, because it will be promoted to the right >> thing. Except with varargs it won't. So if you ever have a >> notionally 64-bit address that happens to fit in a 32-bit variable and >> you pass that it, this function will be broken. And the worst of it >> is, it'll work most of the time, until you happen to hit the wrong ABI >> and parameter combination :(. > > Do you have a suggested improvement to the API to avoid this? I think it makes sense to make this API special-purpose for "reg". We currently have a generic "put any number of 32bit values into the property" function (qemu_devtree_setprop_cells). Can't we also just add a qemu_devtree_setprop_reg() that walks the tree downwards in search for #address-cells and #size-cells and assembles the correct reg property from a list of 64bit arguments? qemu_devtree_setprop_reg(fdt, "/foo/bar", region[0].offset, region[0].size, region[1].offset, region[1].size); Maybe we could even make it more generic to also cover "ranges". Alex
On 26 June 2013 11:31, Alexander Graf <agraf@suse.de> wrote: > I think it makes sense to make this API special-purpose for "reg". > We currently have a generic "put any number of 32bit values into the > property" function (qemu_devtree_setprop_cells). Yes, but that doesn't work for things that aren't simple arrays of 32 bit values, so I think that a generic way to deal with those too would be useful. If you wanted to write a "ranges" property you'd need this too, so it doesn't just apply to "reg". I think we could avoid the "varargs doesn't promote" problem by using a varargs-macro wrapper: #define qemu_devtree_setprop_sized_cells(fdt, node, prop, ...) \ do { \ uint64_t args[] = { __VA_ARGS__ }; \ do_qemu_devtree_setprop_sized_cells(fdt, node, prop, \ args, sizeof(args)); } while (0) which will promote everything (including the size arguments, harmlessly) to uint64_t, and avoids having a varargs function. > Can't we also just add a qemu_devtree_setprop_reg() that walks > the tree downwards in search for #address-cells and #size-cells > and assembles the correct reg property from a list of 64bit > arguments? Do we have an actual use for this? It seems pretty complicated. I would expect that in practice there are two major use cases: (a) create your own fdt from scratch (in which case you can just make everything 64 bits and in any case will know when creating nodes what the #address-cells etc are) (b) modify an existing fdt, in which case you definitely don't want to go poking around too deeply in the tree; anything more than just "put an extra node in the root" is starting to get pretty chancy. thanks -- PMM
On 26.06.2013, at 12:50, Peter Maydell wrote: > On 26 June 2013 11:31, Alexander Graf <agraf@suse.de> wrote: >> I think it makes sense to make this API special-purpose for "reg". >> We currently have a generic "put any number of 32bit values into the >> property" function (qemu_devtree_setprop_cells). > > Yes, but that doesn't work for things that aren't simple arrays > of 32 bit values, so I think that a generic way to deal > with those too would be useful. If you wanted to write a > "ranges" property you'd need this too, so it doesn't just > apply to "reg". > > I think we could avoid the "varargs doesn't promote" problem > by using a varargs-macro wrapper: > > #define qemu_devtree_setprop_sized_cells(fdt, node, prop, ...) \ > do { \ > uint64_t args[] = { __VA_ARGS__ }; \ > do_qemu_devtree_setprop_sized_cells(fdt, node, prop, \ > args, sizeof(args)); > } while (0) > > which will promote everything (including the size arguments, > harmlessly) to uint64_t, and avoids having a varargs function. That would work, yes :). > >> Can't we also just add a qemu_devtree_setprop_reg() that walks >> the tree downwards in search for #address-cells and #size-cells >> and assembles the correct reg property from a list of 64bit >> arguments? > > Do we have an actual use for this? It seems pretty complicated. > I would expect that in practice there are two major use cases: > (a) create your own fdt from scratch (in which case you can > just make everything 64 bits and in any case will know > when creating nodes what the #address-cells etc are) > (b) modify an existing fdt, in which case you definitely don't > want to go poking around too deeply in the tree; anything > more than just "put an extra node in the root" is starting > to get pretty chancy. Well, though I do agree it would mimic exactly what the interpreter will do when reading those values, ensuring consistency, no? Alex
Hi, On Wed, Jun 26, 2013 at 8:50 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 26 June 2013 11:31, Alexander Graf <agraf@suse.de> wrote: >> I think it makes sense to make this API special-purpose for "reg". >> We currently have a generic "put any number of 32bit values into the >> property" function (qemu_devtree_setprop_cells). > > Yes, but that doesn't work for things that aren't simple arrays > of 32 bit values, so I think that a generic way to deal > with those too would be useful. If you wanted to write a > "ranges" property you'd need this too, so it doesn't just > apply to "reg". > +1. And wouldn't an implementation of such a reg-specific function back onto Peter's new function quite nicely anyway? > I think we could avoid the "varargs doesn't promote" problem > by using a varargs-macro wrapper: > > #define qemu_devtree_setprop_sized_cells(fdt, node, prop, ...) \ > do { \ > uint64_t args[] = { __VA_ARGS__ }; \ > do_qemu_devtree_setprop_sized_cells(fdt, node, prop, \ > args, sizeof(args)); > } while (0) > Are statement expressions sanctioned? Or do we need to give up the nice caller accessible return codes? And can we factor out common functionality (mainly the error checking) with existing set_prop_cells to make the interfaces consistent? (we need to get rid of those aborts sooner or later) > which will promote everything (including the size arguments, > harmlessly) to uint64_t, and avoids having a varargs function. > >> Can't we also just add a qemu_devtree_setprop_reg() that walks >> the tree downwards in search for #address-cells and #size-cells >> and assembles the correct reg property from a list of 64bit >> arguments? > I have a patch in my tree that generalises qemu_devtree_getprop* to allow you walk parents for properties (as per the #foo-cells semantic). I use it for interrupt cells however, which kinda suggests that this wish for parent traversal is more generic than just populating reg. I think that Peters patch, along with a parent search friendly property search will be enough to be able to do whatever you want in only a handful of lines. > Do we have an actual use for this? It seems pretty complicated. > I would expect that in practice there are two major use cases: > (a) create your own fdt from scratch (in which case you can > just make everything 64 bits and in any case will know > when creating nodes what the #address-cells etc are) > (b) modify an existing fdt, in which case you definitely don't > want to go poking around too deeply in the tree; anything > more than just "put an extra node in the root" is starting > to get pretty chancy. > Looking to the future, what about -device adding a periph then having qemu add it to the dtb before boot? Regards, Peter > thanks > -- PMM
On 06/26/2013 02:38 PM, Peter Crosthwaite wrote: > Hi, > > On Wed, Jun 26, 2013 at 8:50 PM, Peter Maydell<peter.maydell@linaro.org> wrote: >> On 26 June 2013 11:31, Alexander Graf<agraf@suse.de> wrote: >>> I think it makes sense to make this API special-purpose for "reg". >>> We currently have a generic "put any number of 32bit values into the >>> property" function (qemu_devtree_setprop_cells). >> Yes, but that doesn't work for things that aren't simple arrays >> of 32 bit values, so I think that a generic way to deal >> with those too would be useful. If you wanted to write a >> "ranges" property you'd need this too, so it doesn't just >> apply to "reg". >> > +1. And wouldn't an implementation of such a reg-specific function > back onto Peter's new function quite nicely anyway? > >> I think we could avoid the "varargs doesn't promote" problem >> by using a varargs-macro wrapper: >> >> #define qemu_devtree_setprop_sized_cells(fdt, node, prop, ...) \ >> do { \ >> uint64_t args[] = { __VA_ARGS__ }; \ >> do_qemu_devtree_setprop_sized_cells(fdt, node, prop, \ >> args, sizeof(args)); >> } while (0) >> > Are statement expressions sanctioned? Or do we need to give up the > nice caller accessible return codes? > > And can we factor out common functionality (mainly the error checking) > with existing set_prop_cells to make the interfaces consistent? (we > need to get rid of those aborts sooner or later) > >> which will promote everything (including the size arguments, >> harmlessly) to uint64_t, and avoids having a varargs function. >> >>> Can't we also just add a qemu_devtree_setprop_reg() that walks >>> the tree downwards in search for #address-cells and #size-cells >>> and assembles the correct reg property from a list of 64bit >>> arguments? > I have a patch in my tree that generalises qemu_devtree_getprop* to > allow you walk parents for properties (as per the #foo-cells > semantic). I use it for interrupt cells however, which kinda suggests > that this wish for parent traversal is more generic than just > populating reg. I think that Peters patch, along with a parent search > friendly property search will be enough to be able to do whatever you > want in only a handful of lines. > >> Do we have an actual use for this? It seems pretty complicated. >> I would expect that in practice there are two major use cases: >> (a) create your own fdt from scratch (in which case you can >> just make everything 64 bits and in any case will know >> when creating nodes what the #address-cells etc are) >> (b) modify an existing fdt, in which case you definitely don't >> want to go poking around too deeply in the tree; anything >> more than just "put an extra node in the root" is starting >> to get pretty chancy. >> > Looking to the future, what about -device adding a periph then having > qemu add it to the dtb before boot? I've had a lengthy discussion about that with Anthony a while ago. His take was that this is perfectly reasonable, as long as the device tree generation code stays within the machine model. The machine would just traverse the QOM hierachy and generate device tree nodes for everything it knows. Alex
On 26 June 2013 13:38, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: > On Wed, Jun 26, 2013 at 8:50 PM, Peter Maydell <peter.maydell@linaro.org> wrote: >> I think we could avoid the "varargs doesn't promote" problem >> by using a varargs-macro wrapper: >> >> #define qemu_devtree_setprop_sized_cells(fdt, node, prop, ...) \ >> do { \ >> uint64_t args[] = { __VA_ARGS__ }; \ >> do_qemu_devtree_setprop_sized_cells(fdt, node, prop, \ >> args, sizeof(args)); >> } while (0) >> > > Are statement expressions sanctioned? Or do we need to give up the > nice caller accessible return codes? I just borrowed the syntax from the existing qemu_devtree_setprop_cells macro to illustrate how the varargs bit would work; I'll tweak it to actually get the return code right for the patch proper. > And can we factor out common functionality (mainly the error checking) > with existing set_prop_cells to make the interfaces consistent? (we > need to get rid of those aborts sooner or later) Maybe we should at some point switch to using Error** for error reporting in our wrapper APIs here? It is a bit odd that half our device tree functions die on error and the other half report a failure code. >> which will promote everything (including the size arguments, >> harmlessly) to uint64_t, and avoids having a varargs function. >> >>> Can't we also just add a qemu_devtree_setprop_reg() that walks >>> the tree downwards in search for #address-cells and #size-cells >>> and assembles the correct reg property from a list of 64bit >>> arguments? >> > > I have a patch in my tree that generalises qemu_devtree_getprop* to > allow you walk parents for properties (as per the #foo-cells > semantic). I use it for interrupt cells however, which kinda suggests > that this wish for parent traversal is more generic than just > populating reg. I think that Peters patch, along with a parent search > friendly property search will be enough to be able to do whatever you > want in only a handful of lines. Sounds good to me. -- PMM
On 26 June 2013 13:38, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> Are statement expressions sanctioned?
..and to answer this specific question which I missed first time
round, we already use them (eg container_of, MAKE_TCGV_PTR).
We seem to mark some but not all of them with __extension__;
I think on balance that's probably unnecessary given we aren't
going to be trying to compile with -pedantic any time soon.
-- PMM
On Wed, Jun 26, 2013 at 09:49:51AM +0100, Peter Maydell wrote: > On 26 June 2013 00:38, David Gibson <david@gibson.dropbear.id.au> wrote: > > On Mon, Jun 24, 2013 at 12:02:39PM +0100, Peter Maydell wrote: > >> On 24 June 2013 11:56, Alexander Graf <agraf@suse.de> wrote: > >> > This looks pretty complicated for something actually quite > >> > simple: All you want is to pass in a number of 64bit values, > >> > rather than 32bit ones, right? > >> > >> Nope. If the device tree blob says #address-cells is 1 > >> and #size-cells is 1, then I want to pass in values to > >> go in 32 bit cells. If it says #address-cells is 2 but > >> #size-cells is still 1, then I want to pass in a value > >> for a 64 bit cell then one for a 32 bit cell. If they're > >> both 2 then I want to pass in values for two 64 bit > >> cells. > > > > Hmm.. the property certainly needs to be constructed that way. But I > > think Alex's point is that you could make the arguments all 64-bit, > > and then truncate them in the generated property. > > Er, the arguments *are* all 64 bits and truncated > in the generated property: > + * @...: 0-terminated list of uint32_t number-of-cells, uint64_t value pairs Duh, sorry, misread. That's even worse for the point below. uint32_t / uint64_t pairs, which will sometimes work if you mess that up, until you get the wrong platform / parameter combination. And the uint32_ts are things that could naturally be in just a plain old int or long, which might be 64-bit by default on some ABIs, and the uint64_ts could be addresses in 32-bit space which would naturally be stored in a 32-bit variable, but *must* be upcast to 64-bit or again this interface will break subtly on certain platforms. This is hair-tearing frustration waiting to happen. > > There's a bigger problem, though, that exists with both versions. In > > general people expect integer arguments like this not to care too much > > about the exact integer type, because it will be promoted to the right > > thing. Except with varargs it won't. So if you ever have a > > notionally 64-bit address that happens to fit in a 32-bit variable and > > you pass that it, this function will be broken. And the worst of it > > is, it'll work most of the time, until you happen to hit the wrong ABI > > and parameter combination :(. > > Do you have a suggested improvement to the API to avoid this? After some thought, yes, though it will need gcc extensions: /* Big fat comment saying not to use this function directly */ int __qemu_fdt_pack_ints(void *fdt, int n, uint64_t[]) { /* Error if n is not even */ /* take the u64s from the array in pairs, packing as the previous version */ } #define qemu_fdt_pack_ints(fdt, ...) \ ({ \ uint64_t _tmp[] = { __VA_ARGS__ }; \ __qemu_fdt_pack_ints((fdt), ARRAY_SIZE(_tmp), _tmp); \ })
On Wed, Jun 26, 2013 at 11:50:26AM +0100, Peter Maydell wrote: > On 26 June 2013 11:31, Alexander Graf <agraf@suse.de> wrote: > > I think it makes sense to make this API special-purpose for "reg". > > We currently have a generic "put any number of 32bit values into the > > property" function (qemu_devtree_setprop_cells). > > Yes, but that doesn't work for things that aren't simple arrays > of 32 bit values, so I think that a generic way to deal > with those too would be useful. If you wanted to write a > "ranges" property you'd need this too, so it doesn't just > apply to "reg". > > I think we could avoid the "varargs doesn't promote" problem > by using a varargs-macro wrapper: > > #define qemu_devtree_setprop_sized_cells(fdt, node, prop, ...) \ > do { \ > uint64_t args[] = { __VA_ARGS__ }; \ > do_qemu_devtree_setprop_sized_cells(fdt, node, prop, \ > args, sizeof(args)); > } while (0) > > which will promote everything (including the size arguments, > harmlessly) to uint64_t, and avoids having a varargs function. Heh. Oops, didn't read this before I suggested the same thing. Although a statement expression will let you actually get any fdt error codes out. > > Can't we also just add a qemu_devtree_setprop_reg() that walks > > the tree downwards in search for #address-cells and #size-cells > > and assembles the correct reg property from a list of 64bit > > arguments? > > Do we have an actual use for this? It seems pretty complicated. > I would expect that in practice there are two major use cases: > (a) create your own fdt from scratch (in which case you can > just make everything 64 bits and in any case will know > when creating nodes what the #address-cells etc are) > (b) modify an existing fdt, in which case you definitely don't > want to go poking around too deeply in the tree; anything > more than just "put an extra node in the root" is starting > to get pretty chancy. So, I tend to think that "reg" and "ranges" specific helpers would be nicer than having to add the size for every parameter. But I was thinking of passing in the address-cells and size-cells values rather than having the function dig around in the tree to get them. The other way to avoid varargs would be to have a helper that just adds one entry at a time, using fdt_appendprop().
On Wed, Jun 26, 2013 at 02:44:17PM +0200, Alexander Graf wrote: > On 06/26/2013 02:38 PM, Peter Crosthwaite wrote: > >Hi, > > > >On Wed, Jun 26, 2013 at 8:50 PM, Peter Maydell<peter.maydell@linaro.org> wrote: > >>On 26 June 2013 11:31, Alexander Graf<agraf@suse.de> wrote: > >>>I think it makes sense to make this API special-purpose for "reg". > >>>We currently have a generic "put any number of 32bit values into the > >>>property" function (qemu_devtree_setprop_cells). > >>Yes, but that doesn't work for things that aren't simple arrays > >>of 32 bit values, so I think that a generic way to deal > >>with those too would be useful. If you wanted to write a > >>"ranges" property you'd need this too, so it doesn't just > >>apply to "reg". > >> > >+1. And wouldn't an implementation of such a reg-specific function > >back onto Peter's new function quite nicely anyway? > > > >>I think we could avoid the "varargs doesn't promote" problem > >>by using a varargs-macro wrapper: > >> > >>#define qemu_devtree_setprop_sized_cells(fdt, node, prop, ...) \ > >> do { \ > >> uint64_t args[] = { __VA_ARGS__ }; \ > >> do_qemu_devtree_setprop_sized_cells(fdt, node, prop, \ > >> args, sizeof(args)); > >> } while (0) > >> > >Are statement expressions sanctioned? Or do we need to give up the > >nice caller accessible return codes? > > > >And can we factor out common functionality (mainly the error checking) > >with existing set_prop_cells to make the interfaces consistent? (we > >need to get rid of those aborts sooner or later) > > > >>which will promote everything (including the size arguments, > >>harmlessly) to uint64_t, and avoids having a varargs function. > >> > >>>Can't we also just add a qemu_devtree_setprop_reg() that walks > >>>the tree downwards in search for #address-cells and #size-cells > >>>and assembles the correct reg property from a list of 64bit > >>>arguments? > >I have a patch in my tree that generalises qemu_devtree_getprop* to > >allow you walk parents for properties (as per the #foo-cells > >semantic). I use it for interrupt cells however, which kinda suggests > >that this wish for parent traversal is more generic than just > >populating reg. I think that Peters patch, along with a parent search > >friendly property search will be enough to be able to do whatever you > >want in only a handful of lines. > > > >>Do we have an actual use for this? It seems pretty complicated. > >>I would expect that in practice there are two major use cases: > >> (a) create your own fdt from scratch (in which case you can > >> just make everything 64 bits and in any case will know > >> when creating nodes what the #address-cells etc are) > >> (b) modify an existing fdt, in which case you definitely don't > >> want to go poking around too deeply in the tree; anything > >> more than just "put an extra node in the root" is starting > >> to get pretty chancy. > >> > >Looking to the future, what about -device adding a periph then having > >qemu add it to the dtb before boot? > > I've had a lengthy discussion about that with Anthony a while ago. > His take was that this is perfectly reasonable, as long as the > device tree generation code stays within the machine model. The > machine would just traverse the QOM hierachy and generate device > tree nodes for everything it knows. I also talked with Anthony about this. Although he's insistent on the fdt generation staying within the machine, I think it would make sense to have some shared helpers for this between the fdt platforms. Note that spapr already contains a half-arsed implementation of this.
On Wed, Jun 26, 2013 at 7:17 PM, David Gibson <david@gibson.dropbear.id.au> wrote: > On Wed, Jun 26, 2013 at 02:44:17PM +0200, Alexander Graf wrote: >> I've had a lengthy discussion about that with Anthony a while ago. >> His take was that this is perfectly reasonable, as long as the >> device tree generation code stays within the machine model. The >> machine would just traverse the QOM hierachy and generate device >> tree nodes for everything it knows. > > I also talked with Anthony about this. Although he's insistent on the > fdt generation staying within the machine, I think it would make sense > to have some shared helpers for this between the fdt platforms. FDT generation should stay in the machine but having helpers for platforms to share makes all the sense in the world. What I don't think makes sense is to move FDT generation logic to qdev*.c or anything like that. But if 99% of ARM/PPC just call fdt_build_generic_table() and then customize from there, that's fine by me. Regards, Anthony Liguori > Note that spapr already contains a half-arsed implementation of this. > > -- > David Gibson | I'll have my music baroque, and my code > david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ > | _way_ _around_! > http://www.ozlabs.org/~dgibson
diff --git a/device_tree.c b/device_tree.c index 69be9da..15a081c 100644 --- a/device_tree.c +++ b/device_tree.c @@ -319,3 +319,51 @@ void qemu_devtree_dumpdtb(void *fdt, int size) } } + +int qemu_devtree_setprop_sized_cells(void *fdt, const char *node_path, + const char *property, ...) +{ + uint32_t *propcells; + uint32_t propsize = 0; + va_list ap; + uint32_t ncells; + uint64_t value; + int i, argnum; + + va_start(ap, property); + for (;;) { + ncells = va_arg(ap, uint32_t); + if (!ncells) { + break; + } + assert(ncells < 3); + propsize += ncells; + value = va_arg(ap, uint64_t); + } + va_end(ap); + + propcells = g_new0(uint32_t, propsize); + + i = 0; + va_start(ap, property); + for (argnum = 1; ; argnum++) { + uint32_t hival; + + ncells = va_arg(ap, uint32_t); + if (!ncells) { + break; + } + value = va_arg(ap, uint64_t); + hival = cpu_to_be32(value >> 32); + if (ncells > 1) { + propcells[i++] = hival; + } else if (hival != 0) { + return argnum; + } + propcells[i++] = cpu_to_be32(value); + } + va_end(ap); + + return qemu_devtree_setprop(fdt, node_path, property, propcells, + propsize * sizeof(uint32_t)); +} diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h index f0b3f35..d7691f9 100644 --- a/include/sysemu/device_tree.h +++ b/include/sysemu/device_tree.h @@ -51,4 +51,33 @@ int qemu_devtree_add_subnode(void *fdt, const char *name); void qemu_devtree_dumpdtb(void *fdt, int size); +/** + * qemu_devtree_setprop_sized_cells: + * @fdt: device tree blob + * @node_path: node to set property on + * @property: property to set + * @...: 0-terminated list of uint32_t number-of-cells, uint64_t 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 varargs list, which alternates between "number of cells used by + * this value" and "value" (terminated when number-of-cells is zero). + * 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(). + * + * 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(void *fdt, const char *node_path, + const char *property, ...); + #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() function which allows the number of cells used by each element to be specified. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- device_tree.c | 48 ++++++++++++++++++++++++++++++++++++++++++ include/sysemu/device_tree.h | 29 +++++++++++++++++++++++++ 2 files changed, 77 insertions(+)