Patchwork [4/6] device_tree: Add support for reading device tree properties

login
register
mail settings
Submitter Peter Maydell
Date July 5, 2012, 5 p.m.
Message ID <1341507652-22155-5-git-send-email-peter.maydell@linaro.org>
Download mbox | patch
Permalink /patch/169231/
State New
Headers show

Comments

Peter Maydell - July 5, 2012, 5 p.m.
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(-)
Peter A. G. Crosthwaite - July 6, 2012, 1:56 a.m.
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
>
Peter Maydell - July 6, 2012, 7:18 a.m.
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
Peter Maydell - July 6, 2012, 3:34 p.m.
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
Peter Maydell - July 6, 2012, 3:44 p.m.
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
Peter A. G. Crosthwaite - July 10, 2012, 6:54 a.m.
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
Peter Maydell - July 10, 2012, 7:52 a.m.
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
Peter Maydell - July 10, 2012, 1:03 p.m.
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
Peter A. G. Crosthwaite - July 13, 2012, 1:24 a.m.
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
>

Patch

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