[U-Boot-Users] [PATCH] RFC: generic property fixup mechanism for LIBFDT

[Jerry Van Baren <gerald.vanbaren@smiths-aerospace.com>]

Bartlomiej Sieka wrote:
> On Fri, Aug 03, 2007 at 08:25:56AM -0600, Grant Likely wrote:
> [...]
>>> +static int fdt_set_busfreq(void *fdt, int nodeoffset, const char *name,
>>> +                               bd_t *bd)
>>> +{
>>> +       u32 tmp;
>>> +       /*
>>> +        * Create or update the property.
>>> +        */
>>> +       tmp = cpu_to_be32(bd->bi_busfreq);
>>> +       return fdt_setprop(fdt, nodeoffset, name, &tmp, sizeof(tmp));
>>> +}

[snip]

> Please see the patch below for a more generic approach to property fixups.
> The patch updates mpc5xxx-related code, but I've looked at mpc83xx fixups
> and it seems like they can be adapted to this approach as well.
> 
> I am interested in the comments people might have before proceeding further
> (i.e., getting rid of OF_FLAT_TREE in mpc5xxx).
> 
> Regards,
> Bartlomiej
> 
> 
> diff --git a/cpu/mpc5xxx/cpu.c b/cpu/mpc5xxx/cpu.c
> index 1eac2bb..1e661c2 100644
> --- a/cpu/mpc5xxx/cpu.c
> +++ b/cpu/mpc5xxx/cpu.c
> @@ -33,6 +33,9 @@
>  
>  #if defined(CONFIG_OF_FLAT_TREE)
>  #include <ft_build.h>
> +#elif defined(CONFIG_OF_LIBFDT)
> +#include <libfdt.h>
> +#include <libfdt_env.h>
>  #endif
>  
>  DECLARE_GLOBAL_DATA_PTR;
> @@ -109,9 +112,81 @@ unsigned long get_tbclk (void)
>  	return (tbclk);
>  }
>  
> +#if defined(CONFIG_OF_LIBFDT)
>  /* ------------------------------------------------------------------------- */
> -
> -#ifdef CONFIG_OF_FLAT_TREE
> +/*
> + * Fixups to the fdt.
> + */
> +void
> +ft_cpu_setup(void *blob, bd_t *bd)
> +{
> +	u32 tbfreq;
> +	u32 busfreq;
> +	u32 intfreq;
> +	u32 ipbfreq;
> +
> +	/* fixup properties */
> +	fdt_fixup_props_t fixup_props[] = {
> +		{	"/cpus/" OF_CPU,
> +			"timebase-frequency",
> +			NULL,	/* to be set manually */
> +			0,	/* to be set manually */
> +			1
> +		},

If it must be set manually, what is the point of having it in the table?

[snip more manuals]

> +		{	"/" OF_SOC "/ethernet at 3000",
> +			"mac-address",
> +			bd->bi_enetaddr,
> +			6,
> +			0
> +		},
> +		{	"/" OF_SOC "/ethernet at 3000",
> +			"local-mac-address",
> +			bd->bi_enetaddr,
> +			6,
> +			0
> +		}
> +	};
> +
> +	/* manually set members that can't be initialized in the definition */
> +	tbfreq = cpu_to_be32(OF_TBCLK);
> +	fixup_props[0].val = &tbfreq;
> +	fixup_props[0].len = sizeof(tbfreq);
> +
> +	busfreq = cpu_to_be32(bd->bi_busfreq);
> +	fixup_props[1].val = &busfreq;
> +	fixup_props[1].len = sizeof(busfreq);
> +
> +	intfreq = cpu_to_be32(bd->bi_intfreq);
> +	fixup_props[2].val = &intfreq;
> +	fixup_props[2].len = sizeof(intfreq);
> +
> +	ipbfreq = cpu_to_be32(bd->bi_ipbfreq);
> +	fixup_props[3].val = &ipbfreq;
> +	fixup_props[3].len = sizeof(ipbfreq);

Ouch, we just lost a big part of the advantage of being table driven if 
we have to rewrite the table on the fly by hand.  Now I see what 
"manual" (above) means, and it is worse than I thought originally.  :-(

What happens when someone puts another property in the table between 
fixup_props[1] and fixup_props[2]?  BAD THINGS!!!!!

> +	fdt_fixup_props(blob, fixup_props,
> +			sizeof(fixup_props) / sizeof(fixup_props[0]));
> +}
> +#elif defined(CONFIG_OF_FLAT_TREE)
>  void
>  ft_cpu_setup(void *blob, bd_t *bd)
>  {
> diff --git a/include/libfdt.h b/include/libfdt.h
> index 340e89d..065c580 100644
> --- a/include/libfdt.h
> +++ b/include/libfdt.h
> @@ -23,6 +23,17 @@
>  #include <fdt.h>
>  #include <libfdt_env.h>
>  
> +
> +/* Structure describing property to be fixed-up in cpu-specific code */
> +typedef struct {
> +        char *node;
> +        char *prop;
> +        void *val;
> +        int len;        /* sizeof(val) */
> +        char create;    /* whether to create non-existing properties */
> +} fdt_fixup_props_t;
> +
> +
>  #define FDT_FIRST_SUPPORTED_VERSION	0x10
>  #define FDT_LAST_SUPPORTED_VERSION	0x11
>  
> @@ -145,6 +156,7 @@ int fdt_add_subnode_namelen(void *fdt, int parentoffset,
>  			    const char *name, int namelen);
>  int fdt_add_subnode(void *fdt, int parentoffset, const char *name);
>  int fdt_del_node(void *fdt, int nodeoffset);
> +void fdt_fixup_props(void *blob, fdt_fixup_props_t fixup_props[], int count);
>  
>  /* Extra functions */
>  const char *fdt_strerror(int errval);
> diff --git a/libfdt/fdt_rw.c b/libfdt/fdt_rw.c
> index 693bfe4..572a4e6 100644
> --- a/libfdt/fdt_rw.c
> +++ b/libfdt/fdt_rw.c
> @@ -19,6 +19,8 @@
>  #include "config.h"
>  #if CONFIG_OF_LIBFDT
>  
> +#include <common.h>
> +
>  #include "libfdt_env.h"
>  
>  #include <fdt.h>
> @@ -295,4 +297,40 @@ int fdt_pack(void *fdt)
>  	return 0;
>  }
>  
> +
> +/*
> + * Function fixes up properties in passed 'blob'. It uses the array
> + * 'fixup_props' to take the description of properites to be fixed up, and
> + * processes 'count' elements from this array.
> + */
> +void fdt_fixup_props(void *blob, fdt_fixup_props_t fixup_props[], int count)
> +{
> +	int nodeoffset;
> +	int err;
> +	int j;
> +
> +	for (j = 0; j < count; j++) {
> +		nodeoffset = fdt_find_node_by_path(blob, fixup_props[j].node);
> +		if (nodeoffset >= 0) {
> +			if (fixup_props[j].create ||
> +				fdt_get_property(blob, nodeoffset,
> +					fixup_props[j].prop, 0))
> +				err = fdt_setprop(blob, nodeoffset,
> +							fixup_props[j].prop,
> +							fixup_props[j].val,
> +							fixup_props[j].len);
> +			else
> +				err = 0;
> +			if (err < 0)
> +				debug("Problem setting %s = %s: %s\n",
> +					fixup_props[j].node,
> +					fixup_props[j].prop,
> +					fdt_strerror(err));
> +
> +		} else
> +			debug("Couldn't find %s: %s\n",
> +				fixup_props[j].node,
> +				fdt_strerror(nodeoffset));
> +	}
> +}
>  #endif /* CONFIG_OF_LIBFDT */

This looks similar to what I originally wrote, which was table driven.
<http://www.denx.de/cgi-bin/gitweb.cgi?p=u-boot.git;a=blob;f=cpu/mpc83xx/cpu.c;hb=64dbbd40c58349b64f43fd33dbb5ca0adb67d642>

Your implementation is better than my original implementation, but the 
current cpu/mpc83xx/cpu.c implementation: simple table and "setter" 
functions (Grant's suggested approach, above) is much better.

Both my original implementation and your implementation has pretty 
severe problems when a value has to be manipulated (byteswapping is the 
primary culprit, but arithmetic scaling could also pop up).  Your table 
driven implementation deals with this problem by rewriting values in the 
table at runtime.  That is a nasty thing to do and scales horribly.  In 
addition, your "manual" fixups are all hardcoded, which totally destroys 
any benefit of having a table.

The "setter" functions have almost unlimited flexibility.  The cost is 
that there potentially needs to be quite a few setter functions.  The 
reality (so far) is that it is a reasonable number and each one is 
trivial.  In addition, some of them are used in multiple places.  On the 
"plus" side, the table becomes much simpler, which saves space and 
counterbalances the cost of the "setter" functions.

IMO your proposal is not acceptable.  Follow Grant's advice and the 
current cpu/mpc83xx/cpu.c methodology.

Sorry for the heavy handed critique,
gvb