Re: [Qemu-devel] [PATCH 1/2] device_tree: Add qemu_devtree_setprop_sized_cells() utility function

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3926 bytes --]

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.

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

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]