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

Jerry Van Baren
2007-04-06T22:39:03Z

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 :-/).