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

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(+)

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.

> +struct fdt_resource {
> +       fdt_addr_t start;
> +       fdt_addr_t end;
> +};
> +
> +/**
> + * Compute the size of a resource.
> + *
> + * @param res  the resource to operate on
> + * @return the size of the resource
> + */
> +static inline fdt_size_t fdt_resource_size(const struct fdt_resource *res)
> +{
> +       return res->end - res->start + 1;
> +}
> +
>  /**
>   * Compat types that we know about and for which we might have drivers.
>   * Each is named COMPAT_<dir>_<filename> where <dir> is the directory
> @@ -583,4 +600,35 @@ struct fmap_entry {
>   */
>  int fdtdec_read_fmap_entry(const void *blob, int node, const char *name,
>                            struct fmap_entry *entry);
> +
> +/**
> + * Obtain an indexed resource from a device property.
> + *
> + * @param fdt          FDT blob
> + * @param node         node to examine
> + * @param property     name of the property to parse
> + * @param index                index of the resource to retrieve
> + * @param res          returns the resource
> + * @return 0 if ok, negative on error
> + */
> +int fdt_get_resource(const void *fdt, int node, const char *property,
> +                    unsigned int index, struct fdt_resource *res);
> +
> +/**
> + * 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?

> + * @param res          returns the resource
> + */
> +int fdt_get_named_resource(const void *fdt, int node, const char *property,
> +                          const char *names, const char *name,
> +                          struct fdt_resource *res);
> +
>  #endif
> diff --git a/lib/fdtdec.c b/lib/fdtdec.c
> index eb5aa20526fd..fbfae4a7cbaf 100644
> --- a/lib/fdtdec.c
> +++ b/lib/fdtdec.c
> @@ -691,4 +691,47 @@ int fdtdec_read_fmap_entry(const void *blob, int node, const char *name,
>
>         return 0;
>  }
> +
> +int fdt_get_resource(const void *fdt, int node, const char *property,
> +                    unsigned int index, struct fdt_resource *res)

s/index/find_index/

> +{
> +       const fdt32_t *ptr, *end;
> +       unsigned int i = 0;
> +       int na, ns, len;
> +
> +       na = fdt_n_addr_cells(fdt, node);
> +       ns = fdt_n_size_cells(fdt, node);
> +
> +       ptr = fdt_getprop(fdt, node, property, &len);
> +       if (!ptr)
> +               return len;
> +
> +       end = ptr + len / 4;

sizeof(*ptr) might be better than 4.

> +
> +       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)?

> +                       res->end = res->start + fdt_size_to_cpu(ptr[na]) - 1;
> +                       return 0;
> +               }
> +
> +               ptr += na + ns;
> +               i++;
> +       }
> +
> +       return -FDT_ERR_NOTFOUND;
> +}
> +
> +int fdt_get_named_resource(const void *fdt, int node, const char *property,
> +                          const char *names, const char *name,
> +                          struct fdt_resource *res)
> +{
> +       int index;
> +
> +       index = fdt_get_string_index(fdt, node, names, name);
> +       if (index < 0)
> +               return index;
> +
> +       return fdt_get_resource(fdt, node, property, index, res);
> +}
>  #endif
> --
> 2.0.4
>

Regards,
Simon

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

Hi Thierry,

On 19 August 2014 05:35, Thierry Reding <thierry.reding@gmail.com> wrote:
>
> 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?


LGTM

>
>
> > > +/**
> > > + * 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.


OK

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


OK

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


I see sizeof() used in the lifdt code, so this might not go upstream as is.

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


That looks good. It works for the cases we need and it's obvious later
where the logic is if we want to extend it.

Regards,
Simon

On Tue, Aug 19, 2014 at 06:55:18AM -0600, Simon Glass wrote:
> On 19 August 2014 05:35, Thierry Reding <thierry.reding@gmail.com> wrote:
[...]
> > 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.
> 
> 
> That looks good. It works for the cases we need and it's obvious later
> where the logic is if we want to extend it.

This is what I have for U-Boot currently.

static fdt_addr_t fdtdec_get_address(const fdt32_t *ptr, unsigned int cells)
{
	fdt_addr_t addr = 0;

	while (cells--)
		/* do the shift in two steps to avoid warning on 32-bit */
		addr = (addr << 16) << 16 | fdt32_to_cpu(*ptr++);

	return addr;
}

I use the same function to parse the size field of reg entries, even
though the types aren't technically the same. But making the types
consistent would duplicate the above exactly, so I'm not sure it's worth
it.

Alternatively perhaps something like this could be done:

	static u64 fdtdec_get_number(const fdt32_t *ptr, unsigned int cells)
	{
		/* the above */
	}

	static fdt_addr_t fdtdec_get_address(const fdt32_t *ptr, unsigned int cells)
	{
		return fdtdec_get_number(ptr, cells);
	}

	static fdt_size_t fdtdec_get_size(const fdt32_t *ptr, unsigned int cells)
	{
		return fdtdec_get_number(ptr, cells);
	}

Again, I'm not sure it's really worth the trouble since fdt_addr_t and
fdt_size_t are both always either u32 or u64.

Thierry

Hi Thierry,

On 19 August 2014 07:12, Thierry Reding <thierry.reding@gmail.com> wrote:
> On Tue, Aug 19, 2014 at 06:55:18AM -0600, Simon Glass wrote:
>> On 19 August 2014 05:35, Thierry Reding <thierry.reding@gmail.com> wrote:
> [...]
>> > 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.
>>
>>
>> That looks good. It works for the cases we need and it's obvious later
>> where the logic is if we want to extend it.
>
> This is what I have for U-Boot currently.
>
> static fdt_addr_t fdtdec_get_address(const fdt32_t *ptr, unsigned int cells)
> {
>         fdt_addr_t addr = 0;
>
>         while (cells--)
>                 /* do the shift in two steps to avoid warning on 32-bit */
>                 addr = (addr << 16) << 16 | fdt32_to_cpu(*ptr++);

Odd warning! Does 32UL help?

>
>         return addr;
> }
>
> I use the same function to parse the size field of reg entries, even
> though the types aren't technically the same. But making the types
> consistent would duplicate the above exactly, so I'm not sure it's worth
> it.

Seems fine.

>
> Alternatively perhaps something like this could be done:
>
>         static u64 fdtdec_get_number(const fdt32_t *ptr, unsigned int cells)
>         {
>                 /* the above */
>         }
>
>         static fdt_addr_t fdtdec_get_address(const fdt32_t *ptr, unsigned int cells)
>         {
>                 return fdtdec_get_number(ptr, cells);
>         }
>
>         static fdt_size_t fdtdec_get_size(const fdt32_t *ptr, unsigned int cells)
>         {
>                 return fdtdec_get_number(ptr, cells);
>         }
>
> Again, I'm not sure it's really worth the trouble since fdt_addr_t and
> fdt_size_t are both always either u32 or u64.

Yes, although I suppose ultimately these might be 64-bit always,
Perhaps we should merge the types?

Regards,
Simon

On Tue, Aug 19, 2014 at 03:28:09PM -0600, Simon Glass wrote:
> Hi Thierry,
> 
> On 19 August 2014 07:12, Thierry Reding <thierry.reding@gmail.com> wrote:
> > On Tue, Aug 19, 2014 at 06:55:18AM -0600, Simon Glass wrote:
> >> On 19 August 2014 05:35, Thierry Reding <thierry.reding@gmail.com> wrote:
> > [...]
> >> > 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.
> >>
> >>
> >> That looks good. It works for the cases we need and it's obvious later
> >> where the logic is if we want to extend it.
> >
> > This is what I have for U-Boot currently.
> >
> > static fdt_addr_t fdtdec_get_address(const fdt32_t *ptr, unsigned int cells)
> > {
> >         fdt_addr_t addr = 0;
> >
> >         while (cells--)
> >                 /* do the shift in two steps to avoid warning on 32-bit */
> >                 addr = (addr << 16) << 16 | fdt32_to_cpu(*ptr++);
> 
> Odd warning! Does 32UL help?

The exact warning that I get is this:

	warning: left shift count >= width of type

So the problem is that since addr is of type fdt_addr_t which is 32-bit,
we're effectively shifting all bits out of the variable. The compiler
will generate the same assembly code whether or not I use the single 32-
bit shift or two 16-bit shifts, but using the latter the warning is
gone. That's on both 32-bit and 64-bit ARM.

> > Alternatively perhaps something like this could be done:
> >
> >         static u64 fdtdec_get_number(const fdt32_t *ptr, unsigned int cells)
> >         {
> >                 /* the above */
> >         }
> >
> >         static fdt_addr_t fdtdec_get_address(const fdt32_t *ptr, unsigned int cells)
> >         {
> >                 return fdtdec_get_number(ptr, cells);
> >         }
> >
> >         static fdt_size_t fdtdec_get_size(const fdt32_t *ptr, unsigned int cells)
> >         {
> >                 return fdtdec_get_number(ptr, cells);
> >         }
> >
> > Again, I'm not sure it's really worth the trouble since fdt_addr_t and
> > fdt_size_t are both always either u32 or u64.
> 
> Yes, although I suppose ultimately these might be 64-bit always,
> Perhaps we should merge the types?

That's one other possibility. On Linux there's one common type for
these:

	typedef phys_addr_t resource_size_t;

where phys_addr_t is defined as follows:

	#ifdef CONFIG_PHYS_ADDR_T_64BIT
	typedef u64 phys_addr_t;
	#else
	typedef u32 phys_addr_t;
	#endif

Perhaps we should simply copy that. I take it CONFIG_PHYS_64BIT is
U-Boot's equivalent of CONFIG_PHYS_ADDR_T_64BIT? It doesn't seem to be
documented anywhere but its usage would indicate that it is. I don't
think U-Boot supports things like LPAE, so there's probably no need to
control this more fine-grainedly than with a single CONFIG_PHYS_64BIT
setting.

Thierry

Hi Thierry,

On 20 August 2014 00:36, Thierry Reding <thierry.reding@gmail.com> wrote:
> On Tue, Aug 19, 2014 at 03:28:09PM -0600, Simon Glass wrote:
>> Hi Thierry,
>>
>> On 19 August 2014 07:12, Thierry Reding <thierry.reding@gmail.com> wrote:
>> > On Tue, Aug 19, 2014 at 06:55:18AM -0600, Simon Glass wrote:
>> >> On 19 August 2014 05:35, Thierry Reding <thierry.reding@gmail.com> wrote:
>> > [...]
>> >> > 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.
>> >>
>> >>
>> >> That looks good. It works for the cases we need and it's obvious later
>> >> where the logic is if we want to extend it.
>> >
>> > This is what I have for U-Boot currently.
>> >
>> > static fdt_addr_t fdtdec_get_address(const fdt32_t *ptr, unsigned int cells)
>> > {
>> >         fdt_addr_t addr = 0;
>> >
>> >         while (cells--)
>> >                 /* do the shift in two steps to avoid warning on 32-bit */
>> >                 addr = (addr << 16) << 16 | fdt32_to_cpu(*ptr++);
>>
>> Odd warning! Does 32UL help?
>
> The exact warning that I get is this:
>
>         warning: left shift count >= width of type
>
> So the problem is that since addr is of type fdt_addr_t which is 32-bit,
> we're effectively shifting all bits out of the variable. The compiler
> will generate the same assembly code whether or not I use the single 32-
> bit shift or two 16-bit shifts, but using the latter the warning is
> gone. That's on both 32-bit and 64-bit ARM.

Well if fdt_addr_t is 32-bit then you are don't going to get a better
result with your change - the warning is correct. So I think you need
to have an #ifdef for the case where CONFIG_PHYS_64BIT is not defined
and deal with it separate. Yes would could just move to 64-bit
everwhere, but that will bloat the code I suspect (as we can always do
it later anyway).

>
>> > Alternatively perhaps something like this could be done:
>> >
>> >         static u64 fdtdec_get_number(const fdt32_t *ptr, unsigned int cells)
>> >         {
>> >                 /* the above */
>> >         }
>> >
>> >         static fdt_addr_t fdtdec_get_address(const fdt32_t *ptr, unsigned int cells)
>> >         {
>> >                 return fdtdec_get_number(ptr, cells);
>> >         }
>> >
>> >         static fdt_size_t fdtdec_get_size(const fdt32_t *ptr, unsigned int cells)
>> >         {
>> >                 return fdtdec_get_number(ptr, cells);
>> >         }
>> >
>> > Again, I'm not sure it's really worth the trouble since fdt_addr_t and
>> > fdt_size_t are both always either u32 or u64.
>>
>> Yes, although I suppose ultimately these might be 64-bit always,
>> Perhaps we should merge the types?
>
> That's one other possibility. On Linux there's one common type for
> these:
>
>         typedef phys_addr_t resource_size_t;
>
> where phys_addr_t is defined as follows:
>
>         #ifdef CONFIG_PHYS_ADDR_T_64BIT
>         typedef u64 phys_addr_t;
>         #else
>         typedef u32 phys_addr_t;
>         #endif
>
> Perhaps we should simply copy that. I take it CONFIG_PHYS_64BIT is
> U-Boot's equivalent of CONFIG_PHYS_ADDR_T_64BIT? It doesn't seem to be
> documented anywhere but its usage would indicate that it is. I don't
> think U-Boot supports things like LPAE, so there's probably no need to
> control this more fine-grainedly than with a single CONFIG_PHYS_64BIT
> setting.

U-Boot does have LPAE on x86 at least. I think using resource_size_t
is a good approach.

Regards,
Simon