Hi Andreas,
On 27 October 2016 at 05:44, Andreas Färber afaerber@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@suse.de wrote:
On a Raspberry Pi 2 disagreements on cell endianness can be observed:
U-Boot> fdt print /soc/gpio@7e200000 phandle phandle = <0x0000000d> U-Boot> fdt get value myvar /soc/gpio@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@ni.com Cc: Gerald Van Baren gvb@unssw.com Signed-off-by: Andreas Färber afaerber@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