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

Simon Glass
2014-08-19T21:28:09Z

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