| Message ID | 1341507652-22155-5-git-send-email-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
On Fri, Jul 6, 2012 at 3:00 AM, Peter Maydell <peter.maydell@linaro.org> wrote: > Add support for reading device tree properties (both generic > and single-cell ones) to QEMU's convenience wrapper layer. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > device_tree.c | 30 ++++++++++++++++++++++++++++++ > device_tree.h | 4 ++++ > 2 files changed, 34 insertions(+), 0 deletions(-) > > diff --git a/device_tree.c b/device_tree.c > index b366fdd..d7a9b6b 100644 > --- a/device_tree.c > +++ b/device_tree.c > @@ -178,6 +178,36 @@ int qemu_devtree_setprop_string(void *fdt, const char *node_path, > return r; > } > > +const void *qemu_devtree_getprop(void *fdt, const char *node_path, > + const char *property, int *lenp) > +{ > + int len; > + const void *r; > + if (!lenp) { > + lenp = &len; > + } > + r = fdt_getprop(fdt, findnode_nofail(fdt, node_path), property, lenp); > + if (!r) { > + fprintf(stderr, "%s: Couldn't get %s/%s: %s\n", __func__, > + node_path, property, fdt_strerror(*lenp)); > + exit(1); > + } > + return r; > +} > + > +uint32_t qemu_devtree_getprop_cell(void *fdt, const char *node_path, > + const char *property) Hi Peter, Can we generalise and get functionality for reading cells with offsets as well? Your function assumes (and asserts) that the property is a single cell, but can we add a index parameter for reading a non-0th property out of a multi-cell prop? Needed for reading things like ranges, regs and interrupt properties. Regards, Peter > +{ > + int len; > + const uint32_t *p = qemu_devtree_getprop(fdt, node_path, property, &len); > + if (len != 4) { > + fprintf(stderr, "%s: %s/%s not 4 bytes long (not a cell?)\n", > + __func__, node_path, property); > + exit(1); > + } > + return be32_to_cpu(*p); > +} > + > uint32_t qemu_devtree_get_phandle(void *fdt, const char *path) > { > uint32_t r; > diff --git a/device_tree.h b/device_tree.h > index 2244270..f7a3e6c 100644 > --- a/device_tree.h > +++ b/device_tree.h > @@ -28,6 +28,10 @@ int qemu_devtree_setprop_string(void *fdt, const char *node_path, > int qemu_devtree_setprop_phandle(void *fdt, const char *node_path, > const char *property, > const char *target_node_path); > +const void *qemu_devtree_getprop(void *fdt, const char *node_path, > + const char *property, int *lenp); > +uint32_t qemu_devtree_getprop_cell(void *fdt, const char *node_path, > + const char *property); > uint32_t qemu_devtree_get_phandle(void *fdt, const char *path); > uint32_t qemu_devtree_alloc_phandle(void *fdt); > int qemu_devtree_nop_node(void *fdt, const char *node_path); > -- > 1.7.1 >
On 6 July 2012 02:56, Peter Crosthwaite <peter.crosthwaite@petalogix.com> wrote: > On Fri, Jul 6, 2012 at 3:00 AM, Peter Maydell <peter.maydell@linaro.org> wrote: >> +uint32_t qemu_devtree_getprop_cell(void *fdt, const char *node_path, >> + const char *property) > > Hi Peter, > > Can we generalise and get functionality for reading cells with offsets > as well? Your function assumes (and asserts) that the property is a > single cell, but can we add a index parameter for reading a non-0th > property out of a multi-cell prop? Needed for reading things like > ranges, regs and interrupt properties. We could, but in this instance I was just following the pattern of the existing qemu_devtree_setprop_cell(), which also works only on single-celled properties. Maybe 'set one cell in a multicell prop' should be qemu_devtree_setprop_one_cell() ? -- PMM
On 6 July 2012 02:56, Peter Crosthwaite <peter.crosthwaite@petalogix.com> wrote: > Can we generalise and get functionality for reading cells with offsets > as well? Your function assumes (and asserts) that the property is a > single cell, but can we add a index parameter for reading a non-0th > property out of a multi-cell prop? Needed for reading things like > ranges, regs and interrupt properties. I was playing about with this and I'm really not sure that we should be providing a "read a single u32 from a u32 array property" at the device_tree.c layer. For example, for handling the ranges property what you really want to do is treat it as a list of tuples (including doing something sensible if it doesn't have the right length to be a complete list), so the code that knows the structure of the ranges property is better off calling qemu_devtree_getprop to get a uint32_t* for the whole array. Then it has the whole thing as a straightforward C array which will be much easier and more efficient to handle than constantly bouncing back into the fdt layer to read each uint32_t. I've also just realised that I'm assuming that the pointer returned by fdt_getprop() is naturally aligned for a 32 bit integer if the property is a 32 bit integer -- is that valid? -- PMM
On 6 July 2012 16:34, Peter Maydell <peter.maydell@linaro.org> wrote: > I've also just realised that I'm assuming that the pointer returned > by fdt_getprop() is naturally aligned for a 32 bit integer if the > property is a 32 bit integer -- is that valid? To answer my own question here, the dtb format mandates that property data starts at a 4-aligned address, so casting the pointer to a uint32_t* is safe. -- PMM
On Sat, Jul 7, 2012 at 1:34 AM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 6 July 2012 02:56, Peter Crosthwaite <peter.crosthwaite@petalogix.com> wrote: >> Can we generalise and get functionality for reading cells with offsets >> as well? Your function assumes (and asserts) that the property is a >> single cell, but can we add a index parameter for reading a non-0th >> property out of a multi-cell prop? Needed for reading things like >> ranges, regs and interrupt properties. > > I was playing about with this and I'm really not sure that we should > be providing a "read a single u32 from a u32 array property" at the > device_tree.c layer. For example, for handling the ranges property > what you really want to do is treat it as a list of tuples The tuples concept layers on top of the cells concept. The meaning of cells will differ from property to property but cells are cells. The setter API manages cells but not tuples, so the same should be true of the getter API. So if you are dealing with tuples, the device tree layer should provide access to the cells (which is essentially reading u32 from u32 array) then you can interpret them as tuples if you wish. I guess what i'm trying to say is that tuples and cells should be managed quite separately, the former in the client and the latter in the device tree API. (including > doing something sensible if it doesn't have the right length to be > a complete list), so the code that knows the structure of the ranges > property is better off calling qemu_devtree_getprop to get a uint32_t* > for the whole array. The logic for the byte reversal should still be in device tree however. You have to be_to_cpu each read value after reading that array which is tedious and an easy omission to make. I think if we are going to wrap for single cell props then it makes sense for multi cell as well. Then it has the whole thing as a straightforward > C array which will be much easier and more efficient to handle than Efficient yes, but easier no because of the endian issue. For my few use cases out of tree I only ever get the one property at a time. I never parse an entire cell array. > constantly bouncing back into the fdt layer to read each uint32_t. > Constantly bouncing back is safer however. If you hang on to an in-place pointer into the FDT (as returned by get_prop) and someone comes along and set_props() then your pointer is corrupted. Ive been snagged before by doing exactly this and eventually came to the brute-force approach of just requerying the DTB every touch rather than try to work with pointers to arrays. duping the property could work, but its a bit of a mess trying to free the returned copies. (As an aside this is one of the reasons why my chicken-and-egg machine model uses coroutines instead of threads. I need the get_prop() followed by its immediate usage to be atomic). If user care about efficiency over safety, then your get_prop + cast + endian_swap approach will always be available to them. I just think a single idx arg at the end creates more options for users. We could vararg it and if its omitted it just takes the 0th element to keep the call sites for the 90% case a little more concise. Regards, Peter > I've also just realised that I'm assuming that the pointer returned > by fdt_getprop() is naturally aligned for a 32 bit integer if the > property is a 32 bit integer -- is that valid? > > -- PMM
On 10 July 2012 07:54, Peter Crosthwaite <peter.crosthwaite@petalogix.com> wrote: > If user care about efficiency over safety, then your get_prop + cast + > endian_swap approach will always be available to them. I just think a > single idx arg at the end creates more options for users. We could > vararg it and if its omitted it just takes the 0th element to keep the > call sites for the 90% case a little more concise. Hmm, OK, I'll have another go and see how the patch comes out. -- PMM
On 10 July 2012 07:54, Peter Crosthwaite <peter.crosthwaite@petalogix.com> wrote: > Constantly bouncing back is safer however. If you hang on to an > in-place pointer into the FDT (as returned by get_prop) and someone > comes along and set_props() then your pointer is corrupted. Ive been > snagged before by doing exactly this and eventually came to the > brute-force approach of just requerying the DTB every touch rather > than try to work with pointers to arrays. duping the property could > work, but its a bit of a mess trying to free the returned copies. Incidentally, if you have two separate bits of code both accessing the DTB in parallel then this sounds like a really weird corner case use. I would expect that the standard thing would be "at startup we read the DTB, modify it slightly and after that ignore it", all of which should be straightforward single threaded code with no particular control flow/threading/coroutine issues. -- PMM
On Fri, Jul 6, 2012 at 3:00 AM, Peter Maydell <peter.maydell@linaro.org> wrote: > Add support for reading device tree properties (both generic > and single-cell ones) to QEMU's convenience wrapper layer. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com> > --- > device_tree.c | 30 ++++++++++++++++++++++++++++++ > device_tree.h | 4 ++++ > 2 files changed, 34 insertions(+), 0 deletions(-) > > diff --git a/device_tree.c b/device_tree.c > index b366fdd..d7a9b6b 100644 > --- a/device_tree.c > +++ b/device_tree.c > @@ -178,6 +178,36 @@ int qemu_devtree_setprop_string(void *fdt, const char *node_path, > return r; > } > > +const void *qemu_devtree_getprop(void *fdt, const char *node_path, > + const char *property, int *lenp) > +{ > + int len; > + const void *r; > + if (!lenp) { > + lenp = &len; > + } > + r = fdt_getprop(fdt, findnode_nofail(fdt, node_path), property, lenp); > + if (!r) { > + fprintf(stderr, "%s: Couldn't get %s/%s: %s\n", __func__, > + node_path, property, fdt_strerror(*lenp)); > + exit(1); > + } > + return r; > +} > + > +uint32_t qemu_devtree_getprop_cell(void *fdt, const char *node_path, > + const char *property) > +{ > + int len; > + const uint32_t *p = qemu_devtree_getprop(fdt, node_path, property, &len); > + if (len != 4) { > + fprintf(stderr, "%s: %s/%s not 4 bytes long (not a cell?)\n", > + __func__, node_path, property); > + exit(1); > + } > + return be32_to_cpu(*p); > +} > + > uint32_t qemu_devtree_get_phandle(void *fdt, const char *path) > { > uint32_t r; > diff --git a/device_tree.h b/device_tree.h > index 2244270..f7a3e6c 100644 > --- a/device_tree.h > +++ b/device_tree.h > @@ -28,6 +28,10 @@ int qemu_devtree_setprop_string(void *fdt, const char *node_path, > int qemu_devtree_setprop_phandle(void *fdt, const char *node_path, > const char *property, > const char *target_node_path); > +const void *qemu_devtree_getprop(void *fdt, const char *node_path, > + const char *property, int *lenp); > +uint32_t qemu_devtree_getprop_cell(void *fdt, const char *node_path, > + const char *property); > uint32_t qemu_devtree_get_phandle(void *fdt, const char *path); > uint32_t qemu_devtree_alloc_phandle(void *fdt); > int qemu_devtree_nop_node(void *fdt, const char *node_path); > -- > 1.7.1 >
diff --git a/device_tree.c b/device_tree.c index b366fdd..d7a9b6b 100644 --- a/device_tree.c +++ b/device_tree.c @@ -178,6 +178,36 @@ int qemu_devtree_setprop_string(void *fdt, const char *node_path, return r; } +const void *qemu_devtree_getprop(void *fdt, const char *node_path, + const char *property, int *lenp) +{ + int len; + const void *r; + if (!lenp) { + lenp = &len; + } + r = fdt_getprop(fdt, findnode_nofail(fdt, node_path), property, lenp); + if (!r) { + fprintf(stderr, "%s: Couldn't get %s/%s: %s\n", __func__, + node_path, property, fdt_strerror(*lenp)); + exit(1); + } + return r; +} + +uint32_t qemu_devtree_getprop_cell(void *fdt, const char *node_path, + const char *property) +{ + int len; + const uint32_t *p = qemu_devtree_getprop(fdt, node_path, property, &len); + if (len != 4) { + fprintf(stderr, "%s: %s/%s not 4 bytes long (not a cell?)\n", + __func__, node_path, property); + exit(1); + } + return be32_to_cpu(*p); +} + uint32_t qemu_devtree_get_phandle(void *fdt, const char *path) { uint32_t r; diff --git a/device_tree.h b/device_tree.h index 2244270..f7a3e6c 100644 --- a/device_tree.h +++ b/device_tree.h @@ -28,6 +28,10 @@ int qemu_devtree_setprop_string(void *fdt, const char *node_path, int qemu_devtree_setprop_phandle(void *fdt, const char *node_path, const char *property, const char *target_node_path); +const void *qemu_devtree_getprop(void *fdt, const char *node_path, + const char *property, int *lenp); +uint32_t qemu_devtree_getprop_cell(void *fdt, const char *node_path, + const char *property); uint32_t qemu_devtree_get_phandle(void *fdt, const char *path); uint32_t qemu_devtree_alloc_phandle(void *fdt); int qemu_devtree_nop_node(void *fdt, const char *node_path);
Add support for reading device tree properties (both generic and single-cell ones) to QEMU's convenience wrapper layer. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- device_tree.c | 30 ++++++++++++++++++++++++++++++ device_tree.h | 4 ++++ 2 files changed, 34 insertions(+), 0 deletions(-)