From mboxrd@z Thu Jan  1 00:00:00 1970
From: Simon Glass <sjg@chromium.org>
Date: Tue, 19 Aug 2014 15:28:09 -0600
Subject: [U-Boot] [PATCH 03/23] fdt: Add resource parsing functions
In-Reply-To: <20140819131201.GB1586@ulmo>
References: <1408346196-30419-1-git-send-email-thierry.reding@gmail.com>
	<1408346196-30419-4-git-send-email-thierry.reding@gmail.com>
	<CAPnjgZ2X6q29ZwjpsohXoNVvu71YOg_PEPpciHLg87jK58s3nQ@mail.gmail.com>
	<20140819113549.GC19515@ulmo>
	<CAPnjgZ1VgDCAq9hmMw50fmcFZSXLFoaZ_-Dzh5JWAoFLf+_2vg@mail.gmail.com>
	<20140819131201.GB1586@ulmo>
Message-ID: <CAPnjgZ1PTAavxbdP0ycXWtFoy3UeHdCW+DCgaNcK1R4AtsahgA@mail.gmail.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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