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

[Timur Tabi <timur@freescale.com>]

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

-- 
Timur Tabi
Linux Kernel Developer @ Freescale