From mboxrd@z Thu Jan  1 00:00:00 1970
From: Timur Tabi <timur@freescale.com>
Date: Fri, 06 Apr 2007 16:57:11 -0500
Subject: [U-Boot-Users] Warning for mpc8360emds users: fdt-cmd
	from	u-boot-fdt.git
In-Reply-To: <46139872.5020707@smiths-aerospace.com>
References: <20070403235037.B677E352676@atlas.denx.de>	<46137B17.4050104@gmail.com>
	<46139872.5020707@smiths-aerospace.com>
Message-ID: <4616C237.3020301@freescale.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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