[U-Boot] [PATCH] cmd/fdt: Make fdt get value endian-safe for single-cell properties

[Simon Glass]

Hi Andreas,

On 27 October 2016 at 05:44, Andreas Färber <afaerber at suse.de> wrote:
> Hi Simon,
>
> Am 26.10.2016 um 21:19 schrieb Simon Glass:
>> On 26 October 2016 at 09:02, Andreas Färber <afaerber at suse.de> wrote:
>>> On a Raspberry Pi 2 disagreements on cell endianness can be observed:
>>>
>>>   U-Boot> fdt print /soc/gpio at 7e200000 phandle
>>>   phandle = <0x0000000d>
>>>   U-Boot> fdt get value myvar /soc/gpio at 7e200000 phandle; printenv myvar
>>>   myvar=0x0D000000
>>>
>>> Fix this by always treating the pointer as __be32 and converting it in
>>> fdt_value_setenv(), like its counterpart fdt_parse_prop() already does.
>>>
>>> Fixes: bc80295 ("fdt: Add get commands to fdt")
>>> Cc: Joe Hershberger <joe.hershberger at ni.com>
>>> Cc: Gerald Van Baren <gvb at unssw.com>
>>> Signed-off-by: Andreas Färber <afaerber at suse.de>
>>> ---
>>>  cmd/fdt.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> The patch looks good, but please can you add a function comment to
>> fdt_value_setenv() ?
>
> Where and what should it say? There is a general comment above the
> function already, and by choosing __be32* instead of uint32_t* I thought
> my code was fairly self-documenting.

It should explain what the function does and, importantly, what the
arguments and return value are.

>
>> You might even consider changing nodep to a
>> __be32.
>
> Hm, it's const void* so we're casting it everywhere, but __be32* doesn't
> feel correct when we're also using the variable to access strings or arrays.

OK fair enough, let's leave it as it is.

>
> I am not aware of having a test case for any sha1 hashes, so not sure if
> further endianness fixes may be needed in this function? I do assume it
> breaks extending multi-cell pinctrl-0 properties and the like: If we
> store them as one hex sequence then we might use them as [] array but
> can't extend them <> style with a native hex number read from a phandle.
>
>
> BTW why is the overlay support (fdt apply) not enabled by default? Seems
> to build okay, but because it's defaulting to disabled for all boards
> it's unavailable in our distro and led me to play with these low-level
> operations. Considering that the Raspberry Pi boards use a FAT
> filesystem rather than some memory-constrained partition and that they
> have a number of Hats available, shouldn't we enable it at least in the
> rpi* defconfigs? That would give the feature some more build- and
> possibly functional testing.

It is probably just for code-size reasons.

Regards,
Simon