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

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