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

Timur Tabi
2007-04-06T21:57:11Z

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