[U-Boot-Users] Warning for mpc8360emds users: fdt-cmd from u-boot-fdt.git

[Jerry Van Baren <gvb.uboot@gmail.com>]

Timur Tabi wrote:
> Jerry Van Baren wrote:
> 
>> This was sloppy on my part and I apologize in advance for any confusion 
>> this may cause.  Please take this opportunity to generate improvement 
>> patches, rather than invectives toward myself. ;-)
> 
> I finally had a chance to look at this code, and, well, I'm not crazy about it.
> 
> In short, I have a real problem with this:
> 
> #define	FT_BUSFREQ	0x00000002		/* source is bd->bi_busfreq */
> #define	FT_ENETADDR	0x00000004		/* source is bd->bi_enetaddr */
> 
> Using flags to determine the SOURCE of the property data is bad, IMHO.  It's just not 
> flexible enough.  Not only that, but you already have a bug in the definition of 
> fixup_props[]:
> 
> #ifdef CONFIG_MPC83XX_TSEC2
> 	{	FT_UPDATE | FT_ENETADDR,
> 		"/" OF_SOC "/ethernet at 25000,
> 		"mac-address",
> 	},
> 	{	FT_UPDATE | FT_ENETADDR,
> 		"/" OF_SOC "/ethernet at 25000,
> 		"local-mac-address",
> 	},
> #endif
> 
> FT_ENETADDR is the MAC address of eth0, but here it's being programmed into eth1!  The 
> obvious solution is to introduce:
> 
> #define	FT_ENETADDR1	0x00000008		/* source is bd->bi_enetaddr1 */
> 
> But then you need definitions for eth2 and eth3
> 
> #define	FT_ENETADDR2	0x00000010		/* source is bd->bi_enetaddr2 */
> #define	FT_ENETADDR3	0x00000020		/* source is bd->bi_enetaddr3 */
> 
> As you can see, you're already bloating the code.
> 
> A better solution would be to provide a function pointer for a function that can be called 
> to fill in the property.  Something like:
> 
> int ft_set_eth0(void *fdt, int nodeoffset, const char *name, bd_t *bd)
> {
> 	return fdt_setprop(fdt, nodeoffset, name, bd->bi_enetaddr, 6);
> }
> 
> or something like that.  Then define fixup_props[] like this:
> 
> static const struct {
> 	int  createflags;
> 	char *node;
> 	char *prop;
> 	int (*set_function)(void *fdt, int nodeoffset, const char *name, bd_t *bd);
> } fixup_props[] = {
> 
> What do you think about that?  If you like it, I can make the change to mpc83xx/cpu.c

Hi Timur,

Thanks for the code review.

Oops, yes, the MAC address programming was bad. :-(  I thought I was 
being clever but all I was was very incorrect.

I'm not all that wild about having a bunch of one-liner functions with a 
table of fn pointers, but I cannot think of a better way (the original 
code is/was a sequence of hardcoded calls).

I'll take you up on your offer with a proposed tweak: remove the 
"createflags" too - the only remaining purpose is to indicate "create" 
vs. "update only" (i.e. must previously exist).  This logic can go into 
each "setter" function.  Half the properties are "create or force set" 
so the fdt_get_property() call isn't needed, just set it.  The MAC 
setters that care can do the fdt_get_property() in the "setter" function.

static const struct {
	char *node;
	char *prop;
	int (*set_function)(void *fdt, int nodeoffset, const char *name, bd_t *bd);
} fixup_props[] = {

Does that make sense?

Thanks,
gvb

P.S. I've pushed a new fdt-cmd branch to the u-boot-fdt repo that does 
some clean up and separates out some fdt_support.c functions in 
preparation for improving the bootm automagic logic.  There should be no 
collisions with Timur's proposed changes (if there are, I'll bear the 
responsibility of merging them :-/).