[U-Boot] [PATCH 03/23] fdt: Add resource parsing functions

Thierry Reding
2014-08-19T11:35:50Z

On Mon, Aug 18, 2014 at 12:06:26PM -0600, Simon Glass wrote:
> Hi Thierry,
>
> On 18 August 2014 01:16, Thierry Reding <thierry.reding@gmail.com> wrote:
> > From: Thierry Reding <treding@nvidia.com>
> >
> > Add the fdt_get_resource() and fdt_get_named_resource() functions which
> > can be used to parse resources (memory regions) from an FDT. A helper to
> > compute the size of a region is also provided.
> >
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> > include/fdtdec.h | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
> > lib/fdtdec.c | 43 +++++++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 91 insertions(+)
> >
> > diff --git a/include/fdtdec.h b/include/fdtdec.h
> > index 856e6cf766de..e9091eee6bae 100644
> > --- a/include/fdtdec.h
> > +++ b/include/fdtdec.h
> > @@ -40,6 +40,23 @@ struct fdt_memory {
> > fdt_addr_t end;
> > };
> >
> > +/* information about a resource */
>
> Please add comments, e.g. that end is inclusive.

I've added this comment:

/*
* Information about a resource. start is the first address of the resource
* and end is the last address (inclusive). The length of the resource will
* be equal to: end - start + 1.
*/

Is that enough or do you want me to be more verbose?

> > +/**
> > + * Obtain a named resource from a device property.
> > + *
> > + * Look up the index of the name in a list of strings and return the resource
> > + * at that index.
> > + *
> > + * @param fdt FDT blob
> > + * @param node node to examine
> > + * @param property name of the property to parse
> > + * @param names name of the property to obtain the match the name to
> > + * @param name the name of the entry to look up
>
> These two parameters are confusing. Perhaps rename names to something better?

How about "prop_names" so that it indicates it is the name of a
property? I've also adjusted the comment a bit:

* @param prop_names name of the property containing the list of names

Hopefully that will make it more obvious.

> > +int fdt_get_resource(const void *fdt, int node, const char *property,
> > + unsigned int index, struct fdt_resource *res)
>
> s/index/find_index/

In my opinion the find_ prefix is redundant. After all the function name
already indicates that we're looking for a resource inside a property.
And index would be the offset in that property.

> > + if (!ptr)
> > + return len;
> > +
> > + end = ptr + len / 4;
>
> sizeof(*ptr) might be better than 4.

Device tree explicitly specifies that cells are 32-bit, so this can't
ever be anything other than 4. But I'll change it anyway if you prefer.

> > +
> > + while (ptr + na + ns <= end) {
> > + if (i == index) {
> > + res->start = fdt_addr_to_cpu(*ptr);
>
> This doesn't deal with 64-bit addresses. There is a half-hearted
> attempt with fdt_addr_t, but I wonder if we need a helper function
> like fdt_get_addr(ptr, num_cells)?

The Linux kernel handles this by wrapping it in an of_read_number()
helper and always returning u64, like this:

static inline u64 of_read_number(const __be32 *cell, int size)
{
u64 r = 0;

while (size--)
r = (r << 32) | be32_to_cpu(*(cell++));

return r;
}

It obviously only works for size in { 0, 1, 2 }, but I can add a helper
like that if you want.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20140819/92553822/attachment.pgp>