[U-Boot-Users] [PATCH] fdt, mpc5xxx: Adapt MPC5xxx to use libfdt in ft_cpu_setup()

[U-Boot-Users] [PATCH] fdt, mpc5xxx: Adapt MPC5xxx to use libfdt in ft_cpu_setup()

[Bartlomiej Sieka]

Signed-off-by: Grzegorz Bernacki <gjb@semihalf.com>
---
Not sure where this should be merged, it was applied to u-boot-fdt#fdt (as of
01f771763ed822145b54819abb9c4516c8216d48) merged with the master (as of
cc3023b9f95d7ac959a764471a65001062aecf41). Tested on two MPC5200B-based
boards, one using CONFIG_OF_LIBFDT, the other using CONFIG_OF_FLAT_TREE.

diff --git a/cpu/mpc5xxx/cpu.c b/cpu/mpc5xxx/cpu.c
index 1eac2bb..c4484ef 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,125 @@ unsigned long get_tbclk (void)
 	return (tbclk);
 }
 
+#if defined(CONFIG_OF_LIBFDT)
+/*
+ * "Setter" functions used to add/modify FDT entries.
+ */
+static int fdt_set_tbfreq(void *fdt, int nodeoffset, const char *name, bd_t *bd)
+{
+	u32 tmp;
+	/*
+	 * Create or update the property.
+	 */
+	tmp = cpu_to_be32(OF_TBCLK);
+	return fdt_setprop(fdt, nodeoffset, name, &tmp, sizeof(tmp));
+}
+
+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));
+}
+
+static int fdt_set_clockfreq(void *fdt, int nodeoffset, const char *name,
+				bd_t *bd)
+{
+	u32 tmp;
+	/*
+	 * Create or update the property.
+	 */
+	tmp = cpu_to_be32(bd->bi_intfreq);
+	return fdt_setprop(fdt, nodeoffset, name, &tmp, sizeof(tmp));
+}
+
+static int fdt_set_ipbusfreq(void *fdt, int nodeoffset, const char *name,
+				bd_t *bd)
+{
+	u32 tmp;
+	/*
+	 * Create or update the property.
+	 */
+	tmp = cpu_to_be32(bd->bi_ipbfreq);
+	return fdt_setprop(fdt, nodeoffset, name, &tmp, sizeof(tmp));
+}
+
+static int fdt_set_macaddress(void *fdt, int nodeoffset, const char *name,
+				bd_t *bd)
+{
+	/*
+	 * Fix it up if it exists, don't create it if it doesn't exist.
+	 */
+	if (fdt_get_property(fdt, nodeoffset, name, 0)) {
+		return fdt_setprop(fdt, nodeoffset, name, bd->bi_enetaddr, 6);
+	}
+	return 0;
+}
+
 /* ------------------------------------------------------------------------- */
+/*
+ * Fixups to the fdt.
+ */
+static const struct {
+	char *node;
+	char *prop;
+	int (*set_fn)(void *fdt, int nodeoffset, const char *name, bd_t *bd);
+} fixup_props[] = {
+	{	"/cpus/" OF_CPU,
+		"timebase-frequency",
+		fdt_set_tbfreq
+	},
+	{	"/cpus/" OF_CPU,
+		"bus-frequency",
+		fdt_set_busfreq
+	},
+	{	"/cpus/" OF_CPU,
+		"clock-frequency",
+		fdt_set_clockfreq
+	},
+	{	"/" OF_SOC,
+		"bus-frequency",
+		fdt_set_ipbusfreq
+	},
+	{	"/" OF_SOC "/ethernet at 3000",
+		"mac-address",
+		fdt_set_macaddress
+	},
+	{	"/" OF_SOC "/ethernet at 3000",
+		"local-mac-address",
+		fdt_set_macaddress
+	}
+};
 
-#ifdef CONFIG_OF_FLAT_TREE
+void
+ft_cpu_setup(void *blob, bd_t *bd)
+{
+	int nodeoffset;
+	int err;
+	int j;
+	
+	for (j = 0; j < (sizeof(fixup_props) / sizeof(fixup_props[0])); j++) {
+		nodeoffset = fdt_find_node_by_path(blob, fixup_props[j].node);
+		if (nodeoffset >= 0) {
+			err = fixup_props[j].set_fn(blob, nodeoffset,
+						fixup_props[j].prop, bd);
+			if (err < 0)
+				printf("Problem setting %s = %s: %s\n",
+					fixup_props[j].node,
+					fixup_props[j].prop,
+					fdt_strerror(err));
+		} else {
+			printf("Couldn't find %s: %s\n",
+				fixup_props[j].node,
+				fdt_strerror(nodeoffset));
+		}
+	}
+}
+#elif defined(CONFIG_OF_FLAT_TREE)
 void
 ft_cpu_setup(void *blob, bd_t *bd)
 {
@@ -132,8 +251,9 @@ ft_cpu_setup(void *blob, bd_t *bd)
 	if (p != NULL)
 		memcpy(p, bd->bi_enetaddr, 6);
 
-	p = ft_get_prop(blob, "/" OF_SOC "/ethernet at 3000/local-mac-address", &len);
+	p = ft_get_prop(blob, "/" OF_SOC "/ethernet at 3000/local-mac-address",
+			 &len);
 	if (p != NULL)
 		memcpy(p, bd->bi_enetaddr, 6);
 }
-#endif
+#endif /* defined(CONFIG_OF_LIBFDT) */

[U-Boot-Users] [PATCH CORRECTION] fdt, mpc5xxx: Adapt MPC5xxx to use libfdt in ft_cpu_setup().

[Bartlomiej Sieka]

Add necessary functions to allow MPC5XXX machines to use libfdt. Code was
derived from analogous MPC83xx implementation. Also, add a small coding style
fix while in the area.

Signed-off-by: Grzegorz Bernacki <gjb@semihalf.com>
---
I have sent a wrong patch with wrong comments before, please disregard my
previous email on the same topic; and sorry for the noise.

The baseline used to generate the patch was the master (as of
cc3023b9f95d7ac959a764471a65001062aecf41) with pulled u-boot-fdt#fdt branch
(as of 01f771763ed822145b54819abb9c4516c8216d48).

The patch has been tested on two MPC5200B-based boards, one using
CONFIG_OF_LIBFDT, the other using CONFIG_OF_FLAT_TREE.

diff --git a/cpu/mpc5xxx/cpu.c b/cpu/mpc5xxx/cpu.c
index 1eac2bb..2be5caa 100644
--- a/cpu/mpc5xxx/cpu.c
+++ b/cpu/mpc5xxx/cpu.c
@@ -19,6 +19,8 @@
  * along with this program; if not, write to the Free Software
  * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
  * MA 02111-1307 USA
+ *
+ * Libfdt related routines derived from the MPC83xx.
  */
 
 /*
@@ -33,6 +35,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 +114,125 @@ unsigned long get_tbclk (void)
 	return (tbclk);
 }
 
+#if defined(CONFIG_OF_LIBFDT)
+/*
+ * "Setter" functions used to add/modify FDT entries.
+ */
+static int fdt_set_tbfreq(void *fdt, int nodeoffset, const char *name, bd_t *bd)
+{
+	u32 tmp;
+	/*
+	 * Create or update the property.
+	 */
+	tmp = cpu_to_be32(OF_TBCLK);
+	return fdt_setprop(fdt, nodeoffset, name, &tmp, sizeof(tmp));
+}
+
+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));
+}
+
+static int fdt_set_clockfreq(void *fdt, int nodeoffset, const char *name,
+				bd_t *bd)
+{
+	u32 tmp;
+	/*
+	 * Create or update the property.
+	 */
+	tmp = cpu_to_be32(bd->bi_intfreq);
+	return fdt_setprop(fdt, nodeoffset, name, &tmp, sizeof(tmp));
+}
+
+static int fdt_set_ipbusfreq(void *fdt, int nodeoffset, const char *name,
+				bd_t *bd)
+{
+	u32 tmp;
+	/*
+	 * Create or update the property.
+	 */
+	tmp = cpu_to_be32(bd->bi_ipbfreq);
+	return fdt_setprop(fdt, nodeoffset, name, &tmp, sizeof(tmp));
+}
+
+static int fdt_set_macaddress(void *fdt, int nodeoffset, const char *name,
+				bd_t *bd)
+{
+	/*
+	 * Fix it up if it exists, don't create it if it doesn't exist.
+	 */
+	if (fdt_get_property(fdt, nodeoffset, name, 0)) {
+		return fdt_setprop(fdt, nodeoffset, name, bd->bi_enetaddr, 6);
+	}
+	return 0;
+}
+
 /* ------------------------------------------------------------------------- */
+/*
+ * Fixups to the fdt.
+ */
+static const struct {
+	char *node;
+	char *prop;
+	int (*set_fn)(void *fdt, int nodeoffset, const char *name, bd_t *bd);
+} fixup_props[] = {
+	{	"/cpus/" OF_CPU,
+		"timebase-frequency",
+		fdt_set_tbfreq
+	},
+	{	"/cpus/" OF_CPU,
+		"bus-frequency",
+		fdt_set_busfreq
+	},
+	{	"/cpus/" OF_CPU,
+		"clock-frequency",
+		fdt_set_clockfreq
+	},
+	{	"/" OF_SOC,
+		"bus-frequency",
+		fdt_set_ipbusfreq
+	},
+	{	"/" OF_SOC "/ethernet at 3000",
+		"mac-address",
+		fdt_set_macaddress
+	},
+	{	"/" OF_SOC "/ethernet at 3000",
+		"local-mac-address",
+		fdt_set_macaddress
+	}
+};
 
-#ifdef CONFIG_OF_FLAT_TREE
+void
+ft_cpu_setup(void *blob, bd_t *bd)
+{
+	int nodeoffset;
+	int err;
+	int j;
+	
+	for (j = 0; j < (sizeof(fixup_props) / sizeof(fixup_props[0])); j++) {
+		nodeoffset = fdt_find_node_by_path(blob, fixup_props[j].node);
+		if (nodeoffset >= 0) {
+			err = fixup_props[j].set_fn(blob, nodeoffset,
+						fixup_props[j].prop, bd);
+			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));
+		}
+	}
+}
+#elif defined(CONFIG_OF_FLAT_TREE)
 void
 ft_cpu_setup(void *blob, bd_t *bd)
 {
@@ -132,8 +253,9 @@ ft_cpu_setup(void *blob, bd_t *bd)
 	if (p != NULL)
 		memcpy(p, bd->bi_enetaddr, 6);
 
-	p = ft_get_prop(blob, "/" OF_SOC "/ethernet at 3000/local-mac-address", &len);
+	p = ft_get_prop(blob, "/" OF_SOC "/ethernet at 3000/local-mac-address",
+			 &len);
 	if (p != NULL)
 		memcpy(p, bd->bi_enetaddr, 6);
 }
-#endif
+#endif /* defined(CONFIG_OF_FLAT_TREE) */

[U-Boot-Users] [PATCH CORRECTION] fdt, mpc5xxx: Adapt MPC5xxx to use libfdt in ft_cpu_setup().

[Grant Likely]

On 8/3/07, Bartlomiej Sieka <tur@semihalf.com> wrote:
> Add necessary functions to allow MPC5XXX machines to use libfdt. Code was
> derived from analogous MPC83xx implementation. Also, add a small coding style
> fix while in the area.
>
> Signed-off-by: Grzegorz Bernacki <gjb@semihalf.com>

Comments below
g.

> ---
> I have sent a wrong patch with wrong comments before, please disregard my
> previous email on the same topic; and sorry for the noise.
>
> The baseline used to generate the patch was the master (as of
> cc3023b9f95d7ac959a764471a65001062aecf41) with pulled u-boot-fdt#fdt branch
> (as of 01f771763ed822145b54819abb9c4516c8216d48).
>
> The patch has been tested on two MPC5200B-based boards, one using
> CONFIG_OF_LIBFDT, the other using CONFIG_OF_FLAT_TREE.

Isn't OF_LIBFDT supposed to replace OF_FLAT_TREE?  There aren't many
5xxx boards using OF_FLAT_TREE.  You should craft your patch to update
all the OF_FLAT_TREE users to OF_LIBFDT.

>
> diff --git a/cpu/mpc5xxx/cpu.c b/cpu/mpc5xxx/cpu.c
> index 1eac2bb..2be5caa 100644
> --- a/cpu/mpc5xxx/cpu.c
> +++ b/cpu/mpc5xxx/cpu.c
> @@ -19,6 +19,8 @@
>   * along with this program; if not, write to the Free Software
>   * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
>   * MA 02111-1307 USA
> + *
> + * Libfdt related routines derived from the MPC83xx.
>   */
>
>  /*
> @@ -33,6 +35,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 +114,125 @@ unsigned long get_tbclk (void)
>         return (tbclk);
>  }
>
> +#if defined(CONFIG_OF_LIBFDT)
> +/*
> + * "Setter" functions used to add/modify FDT entries.
> + */
> +static int fdt_set_tbfreq(void *fdt, int nodeoffset, const char *name, bd_t *bd)
> +{
> +       u32 tmp;
> +       /*
> +        * Create or update the property.
> +        */
> +       tmp = cpu_to_be32(OF_TBCLK);
> +       return fdt_setprop(fdt, nodeoffset, name, &tmp, sizeof(tmp));
> +}
> +
> +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));
> +}
> +
> +static int fdt_set_clockfreq(void *fdt, int nodeoffset, const char *name,
> +                               bd_t *bd)
> +{
> +       u32 tmp;
> +       /*
> +        * Create or update the property.
> +        */
> +       tmp = cpu_to_be32(bd->bi_intfreq);
> +       return fdt_setprop(fdt, nodeoffset, name, &tmp, sizeof(tmp));
> +}
> +
> +static int fdt_set_ipbusfreq(void *fdt, int nodeoffset, const char *name,
> +                               bd_t *bd)
> +{
> +       u32 tmp;
> +       /*
> +        * Create or update the property.
> +        */
> +       tmp = cpu_to_be32(bd->bi_ipbfreq);
> +       return fdt_setprop(fdt, nodeoffset, name, &tmp, sizeof(tmp));
> +}

These 4 functions are pretty close to identical (except for the
parameter to cpu_to_be32()).  Surely there is a more compact way to do
this.  In addition, these function don't really contain anything that
screams out "5xxx only!".  Can this be common support code usable by
all boards?

Alternately, this is a very verbose block of code.  If it cannot be
consolidated and simplified, then it should all just be rolled into
ft_cpu_setup.  (Table driven works well when there is a lot of entries
using common functions/data, which is not currently the case).

> +
> +static int fdt_set_macaddress(void *fdt, int nodeoffset, const char *name,
> +                               bd_t *bd)
> +{
> +       /*
> +        * Fix it up if it exists, don't create it if it doesn't exist.
> +        */
> +       if (fdt_get_property(fdt, nodeoffset, name, 0)) {
> +               return fdt_setprop(fdt, nodeoffset, name, bd->bi_enetaddr, 6);
> +       }
> +       return 0;
> +}
> +
>  /* ------------------------------------------------------------------------- */
> +/*
> + * Fixups to the fdt.
> + */
> +static const struct {
> +       char *node;
> +       char *prop;
> +       int (*set_fn)(void *fdt, int nodeoffset, const char *name, bd_t *bd);
> +} fixup_props[] = {
> +       {       "/cpus/" OF_CPU,
> +               "timebase-frequency",
> +               fdt_set_tbfreq
> +       },
> +       {       "/cpus/" OF_CPU,
> +               "bus-frequency",
> +               fdt_set_busfreq
> +       },
> +       {       "/cpus/" OF_CPU,
> +               "clock-frequency",
> +               fdt_set_clockfreq
> +       },
> +       {       "/" OF_SOC,
> +               "bus-frequency",
> +               fdt_set_ipbusfreq
> +       },
> +       {       "/" OF_SOC "/ethernet at 3000",
> +               "mac-address",
> +               fdt_set_macaddress
> +       },
> +       {       "/" OF_SOC "/ethernet at 3000",
> +               "local-mac-address",
> +               fdt_set_macaddress
> +       }
> +};
>
> -#ifdef CONFIG_OF_FLAT_TREE
> +void
> +ft_cpu_setup(void *blob, bd_t *bd)
> +{
> +       int nodeoffset;
> +       int err;
> +       int j;
> +
> +       for (j = 0; j < (sizeof(fixup_props) / sizeof(fixup_props[0])); j++) {
> +               nodeoffset = fdt_find_node_by_path(blob, fixup_props[j].node);
> +               if (nodeoffset >= 0) {
> +                       err = fixup_props[j].set_fn(blob, nodeoffset,
> +                                               fixup_props[j].prop, bd);
> +                       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));
> +               }
> +       }
> +}
> +#elif defined(CONFIG_OF_FLAT_TREE)
>  void
>  ft_cpu_setup(void *blob, bd_t *bd)
>  {
> @@ -132,8 +253,9 @@ ft_cpu_setup(void *blob, bd_t *bd)
>         if (p != NULL)
>                 memcpy(p, bd->bi_enetaddr, 6);
>
> -       p = ft_get_prop(blob, "/" OF_SOC "/ethernet at 3000/local-mac-address", &len);
> +       p = ft_get_prop(blob, "/" OF_SOC "/ethernet at 3000/local-mac-address",
> +                        &len);
>         if (p != NULL)
>                 memcpy(p, bd->bi_enetaddr, 6);
>  }
> -#endif
> +#endif /* defined(CONFIG_OF_FLAT_TREE) */
>
> -------------------------------------------------------------------------
> This SF.net email is sponsored by: Splunk Inc.
> Still grepping through log files to find problems?  Stop.
> Now Search log events and configuration files using AJAX and a browser.
> Download your FREE copy of Splunk now >>  http://get.splunk.com/
> _______________________________________________
> U-Boot-Users mailing list
> U-Boot-Users at lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/u-boot-users
>


-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
grant.likely at secretlab.ca
(403) 399-0195

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

[Bartlomiej Sieka]

On Fri, Aug 03, 2007 at 08:25:56AM -0600, Grant Likely wrote:
[...]
> > +static int fdt_set_tbfreq(void *fdt, int nodeoffset, const char *name, bd_t *bd)
> > +{
> > +       u32 tmp;
> > +       /*
> > +        * Create or update the property.
> > +        */
> > +       tmp = cpu_to_be32(OF_TBCLK);
> > +       return fdt_setprop(fdt, nodeoffset, name, &tmp, sizeof(tmp));
> > +}
> > +
> > +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));
> > +}
> > +
> > +static int fdt_set_clockfreq(void *fdt, int nodeoffset, const char *name,
> > +                               bd_t *bd)
> > +{
> > +       u32 tmp;
> > +       /*
> > +        * Create or update the property.
> > +        */
> > +       tmp = cpu_to_be32(bd->bi_intfreq);
> > +       return fdt_setprop(fdt, nodeoffset, name, &tmp, sizeof(tmp));
> > +}
> > +
> > +static int fdt_set_ipbusfreq(void *fdt, int nodeoffset, const char *name,
> > +                               bd_t *bd)
> > +{
> > +       u32 tmp;
> > +       /*
> > +        * Create or update the property.
> > +        */
> > +       tmp = cpu_to_be32(bd->bi_ipbfreq);
> > +       return fdt_setprop(fdt, nodeoffset, name, &tmp, sizeof(tmp));
> > +}
> 
> These 4 functions are pretty close to identical (except for the
> parameter to cpu_to_be32()).  Surely there is a more compact way to do
> this.  In addition, these function don't really contain anything that
> screams out "5xxx only!".  Can this be common support code usable by
> all boards?

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
+		},
+		{	"/cpus/" OF_CPU,
+			"bus-frequency",
+			NULL,	/* to be set manually */
+			0,	/* to be set manually */
+			1
+		},
+		{	"/cpus/" OF_CPU,
+			"clock-frequency",
+			NULL,	/* to be set manually */
+			0,	/* to be set manually */
+			1
+		},
+		{	"/" OF_SOC,
+			"bus-frequency",
+			NULL,	/* to be set manually */
+			0,	/* to be set manually */
+			1
+		},
+		{	"/" 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);
+
+
+	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 */

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

[Jerry Van Baren]

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

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

[Kim Phillips]

On Wed, 22 Aug 2007 09:18:20 -0400
Jerry Van Baren <gerald.vanbaren@smiths-aerospace.com> wrote:

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

?  Bartlomiej's first patch followed the 83xx 'methodology', which Grant
commented on, which is what Bartlomiej tried to fix here.  Maybe I
missed something, but anyway, now that the window has closed, we can
arrange to fix libfdt support for all powerpc boards for the next
release.

What about having a list of functions to call?  It could be called
something like fdt_update_sequence, and be similar in implementation to
lib_ppc/board.c's init_sequence.  It could also reside in lib_ppc,
esp. since, e.g. all powerpc's have a timebase, and to help naturally
enforce a certain level of consistency among powerpc board code.  The
updater/fixup/"setter" functions would only need be passed the pointer
to the base of the blob to update.

The update functions need to be rewritten to be independent of the
OF_CPU, OF_SOC, OF_QE, OF_STDOUT_PATH defines.  Those should all be
removed (OF_TBCLK might be a bit harder to get rid of though --
implement TBCLK_DIVISOR instead?).  btw, Linux is changing the name of
the soc node from, e.g. "soc8360", to simply "soc", and u-boot needs to
handle transitions like these better.  Instead of adding support for
both name types, the update functions can handle these situations by
using some of the higher level functions available in libfdt, such as
fdt_find_node_by_type or even fdt_find_compatible_node.

Kim

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

[Jerry Van Baren]

Kim Phillips wrote:
> On Wed, 22 Aug 2007 09:18:20 -0400
> Jerry Van Baren <gerald.vanbaren@smiths-aerospace.com> wrote:
> 
>> IMO your proposal is not acceptable.  Follow Grant's advice and the 
>> current cpu/mpc83xx/cpu.c methodology.
> 
> ?  Bartlomiej's first patch followed the 83xx 'methodology', which Grant
> commented on, which is what Bartlomiej tried to fix here.  Maybe I
> missed something, but anyway, now that the window has closed, we can
> arrange to fix libfdt support for all powerpc boards for the next
> release.

Well, perhaps Grant can speak to the issue better than me, but I read 
Grant's reply as a request to coelesce the four "setter" functions into 
one, not to remove all setter functions and change it back to the 
original table mechanism.  IMHO, using hardcoded [0] [1]... indexes is 
MUCH MORE EVIL than having four similar setter functions.

While I agree that extra setter functions are undesirable, I don't know 
if Grant's suggestion can be achieved in a way better than the current 
way (that is a challenge ;-).

> What about having a list of functions to call?  It could be called
> something like fdt_update_sequence, and be similar in implementation to
> lib_ppc/board.c's init_sequence.  It could also reside in lib_ppc,
> esp. since, e.g. all powerpc's have a timebase, and to help naturally
> enforce a certain level of consistency among powerpc board code.  The
> updater/fixup/"setter" functions would only need be passed the pointer
> to the base of the blob to update.

We _have_ "a list of functions to call".  That is what is done with the 
83xx which is what I advocated and what Bartlomiej had before rewriting 
it back into a table without functions.

If the "setter" functions can be segregated into general PPC ones and 
CPU-specific ones, that would be great.  If the number of "setter" 
functions can be reduced without degrading reliability etc, that would 
be greate.

> The update functions need to be rewritten to be independent of the
> OF_CPU, OF_SOC, OF_QE, OF_STDOUT_PATH defines.  Those should all be
> removed (OF_TBCLK might be a bit harder to get rid of though --
> implement TBCLK_DIVISOR instead?).  btw, Linux is changing the name of
> the soc node from, e.g. "soc8360", to simply "soc", and u-boot needs to
> handle transitions like these better.  Instead of adding support for
> both name types, the update functions can handle these situations by
> using some of the higher level functions available in libfdt, such as
> fdt_find_node_by_type or even fdt_find_compatible_node.

Yes, that needs to go on the list.  Done.
<http://www.denx.de/wiki/view/UBoot/UBootFdtInfo#ToDo>

Best regards,
gvb

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

[Kim Phillips]

On Wed, 22 Aug 2007 19:16:16 -0400
Jerry Van Baren <vanbargw@gmail.com> wrote:

> Kim Phillips wrote:
> > On Wed, 22 Aug 2007 09:18:20 -0400
> > Jerry Van Baren <gerald.vanbaren@smiths-aerospace.com> wrote:
> > 
> >> IMO your proposal is not acceptable.  Follow Grant's advice and the 
> >> current cpu/mpc83xx/cpu.c methodology.
> > 
> > ?  Bartlomiej's first patch followed the 83xx 'methodology', which Grant
> > commented on, which is what Bartlomiej tried to fix here.  Maybe I
> > missed something, but anyway, now that the window has closed, we can
> > arrange to fix libfdt support for all powerpc boards for the next
> > release.
> 
> Well, perhaps Grant can speak to the issue better than me, but I read 
> Grant's reply as a request to coelesce the four "setter" functions into 
> one, not to remove all setter functions and change it back to the 
> original table mechanism.  IMHO, using hardcoded [0] [1]... indexes is 

sounds good.  Thanks for clarifying this.

> > What about having a list of functions to call?  It could be called
> > something like fdt_update_sequence, and be similar in implementation to
> > lib_ppc/board.c's init_sequence.  It could also reside in lib_ppc,
> > esp. since, e.g. all powerpc's have a timebase, and to help naturally
> > enforce a certain level of consistency among powerpc board code.  The
> > updater/fixup/"setter" functions would only need be passed the pointer
> > to the base of the blob to update.
> 
> We _have_ "a list of functions to call".  That is what is done with the 
> 83xx which is what I advocated and what Bartlomiej had before rewriting 
> it back into a table without functions.

yeah, I meant /just/ a list of functions to call, without all the extra
fluff in both the existing 83xx and Bartlomiej's implementations.

Kim

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

[Jerry Van Baren]

Kim Phillips wrote:
> On Wed, 22 Aug 2007 19:16:16 -0400
> Jerry Van Baren <vanbargw@gmail.com> wrote:
>> Kim Phillips wrote:

[snip]

>>> What about having a list of functions to call?  It could be called
>>> something like fdt_update_sequence, and be similar in implementation to
>>> lib_ppc/board.c's init_sequence.  It could also reside in lib_ppc,
>>> esp. since, e.g. all powerpc's have a timebase, and to help naturally
>>> enforce a certain level of consistency among powerpc board code.  The
>>> updater/fixup/"setter" functions would only need be passed the pointer
>>> to the base of the blob to update.
>> We _have_ "a list of functions to call".  That is what is done with the 
>> 83xx which is what I advocated and what Bartlomiej had before rewriting 
>> it back into a table without functions.
> 
> yeah, I meant /just/ a list of functions to call, without all the extra
> fluff in both the existing 83xx and Bartlomiej's implementations.
> 
> Kim

Well, the table has the node, the property, and the "setter" routine. 
This allows us to re-use the "setter" routine where possible.  Where 
there are multiple setter routines is where, for instance, the property 
is based on a different clock or a different MAC address.  In those 
cases the setters are identical except for one line, but that one line 
is *why* they are different.

The first half of Grant's comment was:
 > These 4 functions are pretty close to identical (except for the
 > parameter to cpu_to_be32()).  Surely there is a more compact way to do
 > this.

My answer is "no, the whole reason there is one line different is 
because it *has to be*."  As the French say about the "Y" chromosome 
"...and viva la difference."  My first attempt, and Bartlomiej's 
proposed patch, tried (tries) to put the value in the table, but it is a 
*WORSE* solution because you have the problem with byteswapping or not 
and different sizes of values.  The scorecard for putting the values in 
the tables and also having it maintainable is 0 for 2.

Note that the original code is a repetitive in-line coding of what is 
inside the "setter" functions, with 100% duplication of code.  My change 
to a table driven method with "setter" functions drops the 100% 
duplication of code to less, probably 80%.

I would lay down a challenge to make the code both more compact *and* 
not lose maintainability.

"Make everything as simple as possible, but not simpler."
   -- Albert Einstein <http://www.quotedb.com/quotes/1360>

The second half of Grant's comment was:
 > In addition, these function don't really contain anything that
 > screams out "5xxx only!". Can this be common support code usable by
 > all boards?

To this I simply say "yup."  To date, this has been 83xx only (mostly?). 
  With more PPC CPU types comes the opportunity to identify and abstract 
the common "setter" functions.

*This* is where Bartlomiej should start IMHO.  It is where the payback is.

Best regards,
gvb

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

[Grant Likely]

On 8/23/07, Jerry Van Baren <gerald.vanbaren@smiths-aerospace.com> wrote:
> The first half of Grant's comment was:
>  > These 4 functions are pretty close to identical (except for the
>  > parameter to cpu_to_be32()).  Surely there is a more compact way to do
>  > this.
>
> My answer is "no, the whole reason there is one line different is
> because it *has to be*."  As the French say about the "Y" chromosome
> "...and viva la difference."  My first attempt, and Bartlomiej's
> proposed patch, tried (tries) to put the value in the table, but it is a
> *WORSE* solution because you have the problem with byteswapping or not
> and different sizes of values.  The scorecard for putting the values in
> the tables and also having it maintainable is 0 for 2.

Yes, this is the icky bit; but I still  argue that there is a more
compact way to do it.  Table driven is nice when it works; but (as I
also mentioned in my original comment) whie it doesn't its probably
better to just fall back to something programatic

ie (totally untested):

static int fixup_prop(void *fdt, char *node, char *prop, void *val, int size)
{
        int nodeoffset;
        int rc;

        nodeoffset = fdt_find_node_by_path(fdt, node);
        if (nodeoffset < 0) {
                debug("Couldn't find %s: %s\n", node, fdt_strerror(nodeoffset));
                return nodeoffset;
        }

        rc = fdt_get_property(fdt, nodeoffset, prop, 0)
        if (rc)
                goto err;

        rc = fdt_setprop(fdt, nodeoffset, prop, val, size);
        if (rc)
                goto err;

err:
        debug("Problem setting %s = %s: %s\n", node, prop, fdt_strerror(rc));
        return rc;
}

static int fixup_int_prop(void *fdt, char *node, char *prop, u32 val)
{
        val = cpu_to_be32(val);
        return ft_fixup_int_prop(fdt, node, prop, &val, sizeof(val));
}

void
ft_cpu_setup(void *blob, bd_t *bd)
{
        fixup_int_prop(blob, "/cpus/" OF_CPU, "timebase-frequency", OF_TBCLK);
        fixup_int_prop(blob, "/cpus/" OF_CPU, "bus-frequency", bd->bi_busfreq);
        fixup_int_prop(blob, "/cpus/" OF_CPU, "clock-frequency",
bd->bi_intfreq);
        fixup_int_prop(blob, "/cpus/" OF_CPU, "bus-frequency", bd->ipbfreq);
        fixup_prop(blob, "/" OF_SOC "/ethernet at 3000", "mac-address",
                   bd->bi_enetaddr, 6);
        fixup_prop(blob, "/" OF_SOC "/ethernet at 3000", "local-mac-address",
                   bd->bi_enetaddr, 6);
}

This adds 2 helper functions instead of 5, and one of them is the same
path for all of the fixups.  Drops the table approach entirely due to
the problems with extracting values out of bd.  (Which is what I meant
in the third part of my original comment)

The 2 new helpers could also be generalized for use by all boards.

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
grant.likely at secretlab.ca
(403) 399-0195

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

[Jerry Van Baren]

Grant Likely wrote:
> On 8/23/07, Jerry Van Baren <gerald.vanbaren@smiths-aerospace.com> wrote:
>> The first half of Grant's comment was:
>>  > These 4 functions are pretty close to identical (except for the
>>  > parameter to cpu_to_be32()).  Surely there is a more compact way to do
>>  > this.
>>
>> My answer is "no, the whole reason there is one line different is
>> because it *has to be*."  As the French say about the "Y" chromosome
>> "...and viva la difference."  My first attempt, and Bartlomiej's
>> proposed patch, tried (tries) to put the value in the table, but it is a
>> *WORSE* solution because you have the problem with byteswapping or not
>> and different sizes of values.  The scorecard for putting the values in
>> the tables and also having it maintainable is 0 for 2.
> 
> Yes, this is the icky bit; but I still  argue that there is a more
> compact way to do it.  Table driven is nice when it works; but (as I
> also mentioned in my original comment) whie it doesn't its probably
> better to just fall back to something programatic
> 
> ie (totally untested):
> 
> static int fixup_prop(void *fdt, char *node, char *prop, void *val, int size)
> {
>         int nodeoffset;
>         int rc;
> 
>         nodeoffset = fdt_find_node_by_path(fdt, node);
>         if (nodeoffset < 0) {
>                 debug("Couldn't find %s: %s\n", node, fdt_strerror(nodeoffset));
>                 return nodeoffset;
>         }
> 
>         rc = fdt_get_property(fdt, nodeoffset, prop, 0)
>         if (rc)
>                 goto err;
> 
>         rc = fdt_setprop(fdt, nodeoffset, prop, val, size);
>         if (rc)
>                 goto err;
	goto ret;
> err:
>         debug("Problem setting %s = %s: %s\n", node, prop, fdt_strerror(rc));
ret:		// hey, we *CAN* program FORTRAN in C  ;-)
>         return rc;
> }

This is too simplistic for the known cases.
* You pass in a size, but can only handle integers (rc is int).  If the 
size is 6 bytes (MAC), Bad Things[tm] will happen.  If it is 1 or 2 
bytes, it probably will work, but is unclean type-wise.

* The rules are different for some properties: some are set-always and 
some are set only if it already exists (e.g. local-mac-address).

By the time you write enough "general" functions to cover all the cases 
currently covered, I suspect you will have the same number of functions 
as the current implementation.  If not, we have too many "setter" 
functions in the current implementation and they should be coalesced.  :-)

It is possible that I _did_ write more "setter" functions than 
necessary.  I plan to look at it, but have been busy. :-/  Adding LIBFDT 
support to other CPUs and factoring out the common routines would help 
with a review.

> static int fixup_int_prop(void *fdt, char *node, char *prop, u32 val)
> {
>         val = cpu_to_be32(val);
>         return ft_fixup_int_prop(fdt, node, prop, &val, sizeof(val));
> }
> 
> void
> ft_cpu_setup(void *blob, bd_t *bd)
> {
>         fixup_int_prop(blob, "/cpus/" OF_CPU, "timebase-frequency", OF_TBCLK);
>         fixup_int_prop(blob, "/cpus/" OF_CPU, "bus-frequency", bd->bi_busfreq);
>         fixup_int_prop(blob, "/cpus/" OF_CPU, "clock-frequency",
> bd->bi_intfreq);
>         fixup_int_prop(blob, "/cpus/" OF_CPU, "bus-frequency", bd->ipbfreq);
>         fixup_prop(blob, "/" OF_SOC "/ethernet at 3000", "mac-address",
>                    bd->bi_enetaddr, 6);
>         fixup_prop(blob, "/" OF_SOC "/ethernet at 3000", "local-mac-address",
>                    bd->bi_enetaddr, 6);
> }
> 
> This adds 2 helper functions instead of 5, and one of them is the same
> path for all of the fixups.  Drops the table approach entirely due to
> the problems with extracting values out of bd.  (Which is what I meant
> in the third part of my original comment)

I think 2 is too optimistic, but it is still possible this approach 
would be better than the current 5.  The above looks better, but I claim 
only because it is oversimplified - I contend the 2 helper functions 
will expand to 5 functions that basically are the current "setter" 
functions.  On the other hand, maybe not.  It is worth trying.

I thought a table approach would be more obvious than the #ifdef 
spaghetti that was in the original code, but I'm not all that wild about 
the resulting code.  In my implementation, with the table and the 
"setter" functions separated, it still results in obfuscation, 
especially where table entries and corresponding "setter" functions are 
#ifdefed. :-(  Note that the above proposal doesn't help a lot because 
the routine calling the new helper function and the helper functions are 
still separated.  :-/

Part of what I'm unhappy about is that I ended up with two sets of 
#ifdefs where there is conditional code: one in the table and one for 
the corresponding "setter" function.  The above proposal may help, but 
only if the "helper" functions are truly generalized and I am somewhat 
skeptical about 100% success.

> The 2 new helpers could also be generalized for use by all boards.

s/2/n/ (where n is probably 5) and I would agree 100%. :-/

> Cheers,
> g.

Best regards,
gvb

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

[Jerry Van Baren]

Jerry Van Baren wrote:
> Grant Likely wrote:
>> On 8/23/07, Jerry Van Baren <gerald.vanbaren@smiths-aerospace.com> wrote:
>>> The first half of Grant's comment was:
>>>  > These 4 functions are pretty close to identical (except for the
>>>  > parameter to cpu_to_be32()).  Surely there is a more compact way to do
>>>  > this.
>>>
>>> My answer is "no, the whole reason there is one line different is
>>> because it *has to be*."  As the French say about the "Y" chromosome
>>> "...and viva la difference."  My first attempt, and Bartlomiej's
>>> proposed patch, tried (tries) to put the value in the table, but it is a
>>> *WORSE* solution because you have the problem with byteswapping or not
>>> and different sizes of values.  The scorecard for putting the values in
>>> the tables and also having it maintainable is 0 for 2.
>> Yes, this is the icky bit; but I still  argue that there is a more
>> compact way to do it.  Table driven is nice when it works; but (as I
>> also mentioned in my original comment) whie it doesn't its probably
>> better to just fall back to something programatic

[snip pseudo code any my reply]

>> void
>> ft_cpu_setup(void *blob, bd_t *bd)
>> {
>>         fixup_int_prop(blob, "/cpus/" OF_CPU, "timebase-frequency", OF_TBCLK);
>>         fixup_int_prop(blob, "/cpus/" OF_CPU, "bus-frequency", bd->bi_busfreq);
>>         fixup_int_prop(blob, "/cpus/" OF_CPU, "clock-frequency",
>> bd->bi_intfreq);
>>         fixup_int_prop(blob, "/cpus/" OF_CPU, "bus-frequency", bd->ipbfreq);
>>         fixup_prop(blob, "/" OF_SOC "/ethernet at 3000", "mac-address",
>>                    bd->bi_enetaddr, 6);
	fixup_mac_prop(blob, "/" OF_SOC "/ethernet at 3000", "mac-address",
		bd->bi_enetaddr, 1);
>>         fixup_prop(blob, "/" OF_SOC "/ethernet at 3000", "local-mac-address",
>>                    bd->bi_enetaddr, 6);
	fixup_mac_prop(blob, "/" OF_SOC "/ethernet at 3000",
		"local-mac-address", bd->bi_enetaddr, 0);
>> }
>>
>> This adds 2 helper functions instead of 5, and one of them is the same
>> path for all of the fixups.  Drops the table approach entirely due to
>> the problems with extracting values out of bd.  (Which is what I meant
>> in the third part of my original comment)
> 
> I think 2 is too optimistic, but it is still possible this approach 
> would be better than the current 5.  The above looks better, but I claim 
> only because it is oversimplified - I contend the 2 helper functions 
> will expand to 5 functions that basically are the current "setter" 
> functions.  On the other hand, maybe not.  It is worth trying.

[more snippage]

>> The 2 new helpers could also be generalized for use by all boards.
> 
> s/2/n/ (where n is probably 5) and I would agree 100%. :-/
> 
>> Cheers,
>> g.

Thinking about it and a quick glance (I'm running remotely), I think we 
could do the job with three "setter" functions based on Grant's 
proposal, which would be a Good Thing[tm].

static int fixup_int_prop(void *fdt, char *node, char *prop, u32 val)
- Like Grant proposes, note it does a cpu_to_be32()

static int fixup_mac_prop(void *fdt, char *node, char *prop, void *val, 
int force)
- Note I replaced "size" with "force".  We know the size (6).  The 
"force" flag (better name, anyone?) would be whether the MAC addr should 
be created if it _doesn't_ exist.

Best regards from the lovely Sault Ste. Marie,
gvb

<https://webcam.crrel.usace.army.mil/soo/camera2.html>
<http://maps.google.com/maps?q=sault+ste+marie&ie=UTF8&oe=utf-8&ll=46.503429,-84.351032&spn=0.005952,0.015814&t=h&z=16&om=1>

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

[Bartlomiej Sieka]

Jerry Van Baren wrote:
> Jerry Van Baren wrote:
>> Grant Likely wrote:
[...]
>>> ft_cpu_setup(void *blob, bd_t *bd)
>>> {
>>>         fixup_int_prop(blob, "/cpus/" OF_CPU, "timebase-frequency", OF_TBCLK);
>>>         fixup_int_prop(blob, "/cpus/" OF_CPU, "bus-frequency", bd->bi_busfreq);
>>>         fixup_int_prop(blob, "/cpus/" OF_CPU, "clock-frequency",
>>> bd->bi_intfreq);
>>>         fixup_int_prop(blob, "/cpus/" OF_CPU, "bus-frequency", bd->ipbfreq);
>>>         fixup_prop(blob, "/" OF_SOC "/ethernet at 3000", "mac-address",
>>>                    bd->bi_enetaddr, 6);
> 	fixup_mac_prop(blob, "/" OF_SOC "/ethernet at 3000", "mac-address",
> 		bd->bi_enetaddr, 1);
>>>         fixup_prop(blob, "/" OF_SOC "/ethernet at 3000", "local-mac-address",
>>>                    bd->bi_enetaddr, 6);
> 	fixup_mac_prop(blob, "/" OF_SOC "/ethernet at 3000",
> 		"local-mac-address", bd->bi_enetaddr, 0);
>>> }
>>>
>>> This adds 2 helper functions instead of 5, and one of them is the same
>>> path for all of the fixups.  Drops the table approach entirely due to
>>> the problems with extracting values out of bd.  (Which is what I meant
>>> in the third part of my original comment)
>> I think 2 is too optimistic, but it is still possible this approach 
>> would be better than the current 5.  The above looks better, but I claim 
>> only because it is oversimplified - I contend the 2 helper functions 
>> will expand to 5 functions that basically are the current "setter" 
>> functions.  On the other hand, maybe not.  It is worth trying.
> 
> [more snippage]
> 
>>> The 2 new helpers could also be generalized for use by all boards.
>> s/2/n/ (where n is probably 5) and I would agree 100%. :-/
>>
>>> Cheers,
>>> g.
> 
> Thinking about it and a quick glance (I'm running remotely), I think we 
> could do the job with three "setter" functions based on Grant's 
> proposal, which would be a Good Thing[tm].
> 
> static int fixup_int_prop(void *fdt, char *node, char *prop, u32 val)
> - Like Grant proposes, note it does a cpu_to_be32()
> 
> static int fixup_mac_prop(void *fdt, char *node, char *prop, void *val, 
> int force)
> - Note I replaced "size" with "force".  We know the size (6).  The 
> "force" flag (better name, anyone?) would be whether the MAC addr should 
> be created if it _doesn't_ exist.

How about having two functions lite this:

static int fixup_int_prop(void *fdt, char *node, char *prop, u32 val, 
int create);

static int fixup_str_prop(void *fdt, char *node, char *prop, char *val, 
int len, int create)

?

The last argument ('create' i.e., Jerry's 'force') tells whether the 
property should be created if it doesn't exist. fixup_str_prop() (a 
better name?) takes a len argument just in case we ever wanted to set 
properties with length different than the MAC address' length.

The above functions are generic and can be used by all boards; the 
following specific code would go into ft_cpu_setup():

fixup_int_prop(blob, "/cpus/" OF_CPU, "timebase-frequency", OF_TBCLK, 1);
fixup_int_prop(blob, "/cpus/" OF_CPU, "bus-frequency", bd->bi_busfreq, 1);
fixup_int_prop(blob, "/cpus/" OF_CPU, "clock-frequency", bd->bi_intfreq, 1);
fixup_int_prop(blob, "/cpus/" OF_CPU, "bus-frequency", bd->ipbfreq, 1);
fixup_str_prop(blob, "/" OF_SOC "/ethernet at 3000", "mac-address", 
bd->bi_enetaddr, 6, 0);
fixup_str_prop(blob, "/" OF_SOC "/ethernet at 3000", "local-mac-address", 
bd->bi_enetaddr, 6, 0);


If in the future there's a need for setting a property other than int or 
char[], a function similar to the two ones above can be added.

Regards,
Bartlomiej

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

[Wolfgang Denk]

In message <20070822160915.5cf9e116.kim.phillips@freescale.com> you wrote:
>
> Jerry Van Baren <gerald.vanbaren@smiths-aerospace.com> wrote:
> 
> > IMO your proposal is not acceptable.  Follow Grant's advice and the 
> > current cpu/mpc83xx/cpu.c methodology.
> 
> ?  Bartlomiej's first patch followed the 83xx 'methodology', which Grant
> commented on, which is what Bartlomiej tried to fix here.  Maybe I
> missed something, but anyway, now that the window has closed, we can
> arrange to fix libfdt support for all powerpc boards for the next
> release.

We still accept patches that fix bugs and solve urgent problems,  and
if  I  understand  correctly,  Bartek's  patch attempts to fix a real
problem.


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
There is an old custom among my people. When a woman  saves  a  man's
life, he is grateful.
	-- Nona, the Kanuto which woman, "A Private Little War",
	   stardate 4211.8.

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

[Kim Phillips]

On Thu, 23 Aug 2007 01:30:29 +0200
Wolfgang Denk <wd@denx.de> wrote:

> In message <20070822160915.5cf9e116.kim.phillips@freescale.com> you wrote:
> >
> > Jerry Van Baren <gerald.vanbaren@smiths-aerospace.com> wrote:
> > 
> > > IMO your proposal is not acceptable.  Follow Grant's advice and the 
> > > current cpu/mpc83xx/cpu.c methodology.
> > 
> > ?  Bartlomiej's first patch followed the 83xx 'methodology', which Grant
> > commented on, which is what Bartlomiej tried to fix here.  Maybe I
> > missed something, but anyway, now that the window has closed, we can
> > arrange to fix libfdt support for all powerpc boards for the next
> > release.
> 
> We still accept patches that fix bugs and solve urgent problems,  and
> if  I  understand  correctly,  Bartek's  patch attempts to fix a real
> problem.

maybe I misunderstood then. I just don't want to see code being
duplicated among two subplatforms (83xx and 52xx) without some stuff
being refactored into common powerpc areas.

Kim

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

[Jerry Van Baren]

Kim Phillips wrote:
> On Thu, 23 Aug 2007 01:30:29 +0200
> Wolfgang Denk <wd@denx.de> wrote:
> 
>> In message <20070822160915.5cf9e116.kim.phillips@freescale.com> you wrote:
>>> Jerry Van Baren <gerald.vanbaren@smiths-aerospace.com> wrote:
>>>
>>>> IMO your proposal is not acceptable.  Follow Grant's advice and the 
>>>> current cpu/mpc83xx/cpu.c methodology.
>>> ?  Bartlomiej's first patch followed the 83xx 'methodology', which Grant
>>> commented on, which is what Bartlomiej tried to fix here.  Maybe I
>>> missed something, but anyway, now that the window has closed, we can
>>> arrange to fix libfdt support for all powerpc boards for the next
>>> release.
>> We still accept patches that fix bugs and solve urgent problems,  and
>> if  I  understand  correctly,  Bartek's  patch attempts to fix a real
>> problem.
> 
> maybe I misunderstood then. I just don't want to see code being
> duplicated among two subplatforms (83xx and 52xx) without some stuff
> being refactored into common powerpc areas.
> 
> Kim

I didn't jump into this one because it really is Grant's call, but my 
understanding is this is new 5xxx functionality which would imply it 
goes into the next window.  *Good* functionality, but new.

The refactoring should be an effort across 5xxx and 8[356]xx (and 
possibly 4xx, 82xx, 8xx) platforms and I would love to see it adopted in 
the next window.

gvb

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

[Wolfgang Denk]

In message <20070823114842.08b9ae7e.kim.phillips@freescale.com> you wrote:
> 
> maybe I misunderstood then. I just don't want to see code being
> duplicated among two subplatforms (83xx and 52xx) without some stuff
> being refactored into common powerpc areas.

Agreed.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Disc space - the final frontier!

[U-Boot-Users] [PATCH CORRECTION] fdt, mpc5xxx: Adapt MPC5xxx to use libfdt in ft_cpu_setup().

[Wolfgang Denk]

In message <fa686aa40708030725o3eac8147i1f12fce286b4aac@mail.gmail.com> you wrote:
> On 8/3/07, Bartlomiej Sieka <tur@semihalf.com> wrote:
> > Add necessary functions to allow MPC5XXX machines to use libfdt. Code was
> > derived from analogous MPC83xx implementation. Also, add a small coding style
> > fix while in the area.
> >
> > Signed-off-by: Grzegorz Bernacki <gjb@semihalf.com>
> 
> Comments below

Grant - how do we proceed with this?

From the following discussion [see thread "RFC: generic property fixup
mechanism for LIBFDT"] I got the impression that an improved
implementation is not available yet, so it will have to wait for the
next release.

Do you agree to include this patch (which fixes a few build problems)
into the current release and clean it up in the next one?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
In theory, there is no difference between  theory  and  practice.  In
practice, however, there is.

[U-Boot-Users] [PATCH CORRECTION] fdt, mpc5xxx: Adapt MPC5xxx to use libfdt in ft_cpu_setup().

[Grant Likely]

On 8/29/07, Wolfgang Denk <wd@denx.de> wrote:
> From the following discussion [see thread "RFC: generic property fixup
> mechanism for LIBFDT"] I got the impression that an improved
> implementation is not available yet, so it will have to wait for the
> next release.
>
> Do you agree to include this patch (which fixes a few build problems)
> into the current release and clean it up in the next one?

I'm confused.  What build problems does it solve?  As far as I can
tell, this patch only adds support for CONFIG_OF_LIBFDT, but it leaves
the old code alone.  What am I missing?

As it stands, I don't like this patch.  It seems rather schizophrenic
to me by adding the new mechanism (CONFIG_OF_LIBFDT) without removing
the old one (CONFIG_OF_FLAT_TREE).  I don't see any advantage to
keeping both mechanisms around.  I'd rather see the patch switch
entirely to the new method.

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
grant.likely at secretlab.ca
(403) 399-0195

[U-Boot-Users] [PATCH] fdt, mpc5xxx: Adapt MPC5xxx to use libfdt in ft_cpu_setup()

[Jerry Van Baren]

Bartlomiej Sieka wrote:
> Signed-off-by: Grzegorz Bernacki <gjb@semihalf.com>
> ---
> Not sure where this should be merged, it was applied to u-boot-fdt#fdt (as of
> 01f771763ed822145b54819abb9c4516c8216d48) merged with the master (as of
> cc3023b9f95d7ac959a764471a65001062aecf41). Tested on two MPC5200B-based
> boards, one using CONFIG_OF_LIBFDT, the other using CONFIG_OF_FLAT_TREE.

Hi Bartlomiej,

This should be applied via the 5xxx repo.  Since Grant is in charge of 
that repo and replied already, it looks like things are progressing...

Thanks,
gvb