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

[Simon Glass]

Hi Thierry,

On 20 August 2014 00:36, Thierry Reding <thierry.reding at 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 at 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 at 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