[U-Boot-Users] [PATCH] fdt, mpc5xxx: Adapt MPC5xxx to use libfdt in ft_cpu_setup()
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@3000", + "mac-address", + fdt_set_macaddress + }, + { "/" OF_SOC "/ethernet@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@3000/local-mac-address", &len); + p = ft_get_prop(blob, "/" OF_SOC "/ethernet@3000/local-mac-address", + &len); if (p != NULL) memcpy(p, bd->bi_enetaddr, 6); } -#endif +#endif /* defined(CONFIG_OF_LIBFDT) */
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@3000", + "mac-address", + fdt_set_macaddress + }, + { "/" OF_SOC "/ethernet@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@3000/local-mac-address", &len); + p = ft_get_prop(blob, "/" OF_SOC "/ethernet@3000/local-mac-address", + &len); if (p != NULL) memcpy(p, bd->bi_enetaddr, 6); } -#endif +#endif /* defined(CONFIG_OF_FLAT_TREE) */
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@3000","mac-address",fdt_set_macaddress},{ "/" OF_SOC "/ethernet@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@3000/local-mac-address", &len);
p = ft_get_prop(blob, "/" OF_SOC "/ethernet@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@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/u-boot-users
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@3000", + "mac-address", + bd->bi_enetaddr, + 6, + 0 + }, + { "/" OF_SOC "/ethernet@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 */
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@3000","mac-address",bd->bi_enetaddr,6,0},{ "/" OF_SOC "/ethernet@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);elseerr = 0;if (err < 0)debug("Problem setting %s = %s: %s\n",fixup_props[j].node,fixup_props[j].prop,fdt_strerror(err));} elsedebug("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
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
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
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
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
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@3000", "mac-address", bd->bi_enetaddr, 6); fixup_prop(blob, "/" OF_SOC "/ethernet@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 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@3000", "mac-address", bd->bi_enetaddr, 6); fixup_prop(blob, "/" OF_SOC "/ethernet@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
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@3000", "mac-address", bd->bi_enetaddr, 6);
fixup_mac_prop(blob, "/" OF_SOC "/ethernet@3000", "mac-address", bd->bi_enetaddr, 1);
fixup_prop(blob, "/" OF_SOC "/ethernet@3000", "local-mac-address", bd->bi_enetaddr, 6);
fixup_mac_prop(blob, "/" OF_SOC "/ethernet@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
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@3000", "mac-address", bd->bi_enetaddr, 6);
fixup_mac_prop(blob, "/" OF_SOC "/ethernet@3000", "mac-address", bd->bi_enetaddr, 1);
fixup_prop(blob, "/" OF_SOC "/ethernet@3000", "local-mac-address", bd->bi_enetaddr, 6);fixup_mac_prop(blob, "/" OF_SOC "/ethernet@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@3000", "mac-address", bd->bi_enetaddr, 6, 0); fixup_str_prop(blob, "/" OF_SOC "/ethernet@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
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
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
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
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
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
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.
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
participants (8)
-
Bartlomiej Sieka -
Bartlomiej Sieka -
Grant Likely -
Jerry Van Baren -
Jerry Van Baren -
Jerry Van Baren -
Kim Phillips -
Wolfgang Denk