Fix fdt_chosen() to call ft_board_setup(), clean up long lines.

The fdt_chosen() function was adding/seting some properties ad-hoc
improperly and duplicated (poorly) what was done in ft_board_setup()

Clean up long lines (setting properties, printing errors).

Signed-off-by Gerald Van Baren <[email protected]>

---
Kim:

Can I have an ack/nack on this?

Thanks,
gvb

 common/fdt_support.c |  113 ++++++++++++++++++++++++++++++++-----------------
 1 files changed, 74 insertions(+), 39 deletions(-)

common/fdt_support.c +7 -4

index efa63f0..d12c751 100644
@@ -32,6 +32,10 @@
 #include <libfdt.h>
 #include <fdt_support.h>
 
+#ifdef CONFIG_OF_BOARD_SETUP
+void ft_board_setup(void *blob, bd_t *bd);
+#endif
+
 /*
  * Global data (for the gd->bd)
  */

@@ -42,7 +46,6 @@ DECLARE_GLOBAL_DATA_PTR;
  */
 struct fdt_header *fdt;
 
-
 /********************************************************************/
 
 int fdt_chosen(void *fdt, ulong initrd_start, ulong initrd_end, int force)

@@ -50,9 +53,8 @@ int fdt_chosen(void *fdt, ulong initrd_start, ulong initrd_end, int force)
 	bd_t *bd = gd->bd;
 	int   nodeoffset;
 	int   err;
-	u32   tmp;			/* used to set 32 bit integer properties */
-	char  *str;			/* used to set string properties */
-	ulong clock;
+	u32   tmp;		/* used to set 32 bit integer properties */
+	char  *str;		/* used to set string properties */
 
 	err = fdt_check_header(fdt);
 	if (err < 0) {

@@ -60,6 +62,17 @@ int fdt_chosen(void *fdt, ulong initrd_start, ulong initrd_end, int force)
 		return err;
 	}
 
+#ifdef CONFIG_OF_BOARD_SETUP

common/fdt_support.c +39 -29

 	if (initrd_start && initrd_end) {
 		struct fdt_reserve_entry re;
 		int  used;

@@ -72,7 +85,8 @@ int fdt_chosen(void *fdt, ulong initrd_start, ulong initrd_end, int force)
 			return err;
 		}
 		if (used >= total) {
-			printf("WARNING fdt_chosen: no room in the reserved map (%d of %d)\n",
+			printf("WARNING fdt_chosen: "
+				"no room in the reserved map (%d of %d)\n",
 				used, total);
 			return -1;
 		}

@@ -115,7 +129,10 @@ int fdt_chosen(void *fdt, ulong initrd_start, ulong initrd_end, int force)
 		 */
 		nodeoffset = fdt_add_subnode(fdt, 0, "chosen");
 		if (nodeoffset < 0) {
-			printf("WARNING fdt_chosen: could not create the \"/chosen node\" (libfdt error %s).\n", fdt_strerror(nodeoffset));
+			printf("WARNING fdt_chosen: "
+				"could not create the \"/chosen node\" "
+				"(libfdt error %s).\n",
+				fdt_strerror(nodeoffset));
 			return nodeoffset;
 		}
 	}

@@ -125,42 +142,42 @@ int fdt_chosen(void *fdt, ulong initrd_start, ulong initrd_end, int force)
 	 */
 	str = getenv("bootargs");
 	if (str != NULL) {
-		err = fdt_setprop(fdt, nodeoffset, "bootargs", str, strlen(str)+1);
+		err = fdt_setprop(fdt, nodeoffset,
+			"bootargs", str, strlen(str)+1);
 		if (err < 0)
-			printf("WARNING fdt_chosen: could not set \"bootargs\" (libfdt error %s).\n", fdt_strerror(err));
+			printf("WARNING fdt_chosen: "
+				"could not set \"bootargs\" "
+				"(libfdt error %s).\n",
+				fdt_strerror(err));
 	}
 	if (initrd_start && initrd_end) {
 		tmp = __cpu_to_be32(initrd_start);
-		err = fdt_setprop(fdt, nodeoffset, "linux,initrd-start", &tmp, sizeof(tmp));
+		err = fdt_setprop(fdt, nodeoffset,
+			 "linux,initrd-start", &tmp, sizeof(tmp));
 		if (err < 0)
-			printf("WARNING fdt_chosen: could not set \"linux,initrd-start\" (libfdt error %s).\n", fdt_strerror(err));
+			printf("WARNING fdt_chosen: "
+				"could not set \"linux,initrd-start\" "
+				"(libfdt error %s).\n",
+				fdt_strerror(err));
 		tmp = __cpu_to_be32(initrd_end);
-		err = fdt_setprop(fdt, nodeoffset, "linux,initrd-end", &tmp, sizeof(tmp));
+		err = fdt_setprop(fdt, nodeoffset,
+			"linux,initrd-end", &tmp, sizeof(tmp));
 		if (err < 0)
-			printf("WARNING fdt_chosen: could not set \"linux,initrd-end\" (libfdt error %s).\n", fdt_strerror(err));
+			printf("WARNING fdt_chosen: "
+				"could not set \"linux,initrd-end\" "
+				"(libfdt error %s).\n",
+				fdt_strerror(err));
 	}
 #ifdef OF_STDOUT_PATH
-	err = fdt_setprop(fdt, nodeoffset, "linux,stdout-path", OF_STDOUT_PATH, strlen(OF_STDOUT_PATH)+1);
+	err = fdt_setprop(fdt, nodeoffset,
+		"linux,stdout-path", OF_STDOUT_PATH, strlen(OF_STDOUT_PATH)+1);
 	if (err < 0)
-		printf("WARNING fdt_chosen: could not set \"linux,stdout-path\" (libfdt error %s).\n", fdt_strerror(err));
+		printf("WARNING fdt_chosen: "
+			"could not set \"linux,stdout-path\" "
+			"(libfdt error %s).\n",
+			fdt_strerror(err));
 #endif
 
-	nodeoffset = fdt_find_node_by_path (fdt, "/cpus");
-	if (nodeoffset >= 0) {
-		clock = cpu_to_be32(bd->bi_intfreq);
-		err = fdt_setprop(fdt, nodeoffset, "clock-frequency", &clock, 4);
-		if (err < 0)
-			printf("WARNING fdt_chosen: could not set \"clock-frequency\" (libfdt error %s).\n", fdt_strerror(err));
-	}
-#ifdef OF_TBCLK
-	nodeoffset = fdt_find_node_by_path (fdt, "/cpus/" OF_CPU "/timebase-frequency");
-	if (nodeoffset >= 0) {
-		clock = cpu_to_be32(OF_TBCLK);
-		err = fdt_setprop(fdt, nodeoffset, "clock-frequency", &clock, 4);
-		if (err < 0)
-			printf("WARNING fdt_chosen: could not set \"clock-frequency\" (libfdt error %s).\n", fdt_strerror(err));
-	}
-#endif
 	return err;
 }
 

@@ -203,7 +220,10 @@ int fdt_env(void *fdt)
 	 */
 	nodeoffset = fdt_add_subnode(fdt, 0, "u-boot-env");
 	if (nodeoffset < 0) {
-		printf("WARNING fdt_env: could not create the \"/u-boot-env node\" (libfdt error %s).\n", fdt_strerror(nodeoffset));
+		printf("WARNING fdt_env: "
+			"could not create the \"/u-boot-env node\" "
+			"(libfdt error %s).\n",
+			fdt_strerror(nodeoffset));
 		return nodeoffset;
 	}
 

@@ -231,7 +251,10 @@ int fdt_env(void *fdt)
 			continue;
 		err = fdt_setprop(fdt, nodeoffset, lval, rval, strlen(rval)+1);
 		if (err < 0) {
-			printf("WARNING fdt_env: could not set \"%s\" (libfdt error %s).\n", lval, fdt_strerror(err));
+			printf("WARNING fdt_env: "
+				"could not set \"%s\" "
+				"(libfdt error %s).\n",
+				lval, fdt_strerror(err));
 			return err;
 		}
 	}

@@ -297,7 +320,7 @@ int fdt_bd_t(void *fdt)
 	bd_t *bd = gd->bd;
 	int   nodeoffset;
 	int   err;
-	u32   tmp;			/* used to set 32 bit integer properties */
+	u32   tmp;		/* used to set 32 bit integer properties */
 	int i;
 
 	err = fdt_check_header(fdt);

common/fdt_support.c +6 -3

 		return nodeoffset;
 	}

@@ -332,20 +358,29 @@ int fdt_bd_t(void *fdt)
 	 */
 	for (i = 0; i < sizeof(bd_map)/sizeof(bd_map[0]); i++) {
 		tmp = cpu_to_be32(getenv("bootargs"));
-		err = fdt_setprop(fdt, nodeoffset, bd_map[i].name, &tmp, sizeof(tmp));
+		err = fdt_setprop(fdt, nodeoffset,
+			bd_map[i].name, &tmp, sizeof(tmp));
 		if (err < 0)
-			printf("WARNING fdt_bd_t: could not set \"%s\" (libfdt error %s).\n", bd_map[i].name, fdt_strerror(err));
+			printf("WARNING fdt_bd_t: "
+				"could not set \"%s\" "
+				"(libfdt error %s).\n",
+				bd_map[i].name, fdt_strerror(err));
 	}
 	/*
 	 * Add a couple of oddball entries...
 	 */
 	err = fdt_setprop(fdt, nodeoffset, "enetaddr", &bd->bi_enetaddr, 6);
 	if (err < 0)
-		printf("WARNING fdt_bd_t: could not set \"enetaddr\" (libfdt error %s).\n", fdt_strerror(err));

Quoted 56 lines · On Sat, 26 May 2007 08:52:31 -0400 Jerry Van Baren <[email protected]> wrote:

The fdt_chosen() function was adding/seting some properties ad-hoc
improperly and duplicated (poorly) what was done in ft_board_setup()

Clean up long lines (setting properties, printing errors).

Signed-off-by Gerald Van Baren <[email protected]>

---
Kim:

Can I have an ack/nack on this?

Thanks,
gvb

 common/fdt_support.c |  113 ++++++++++++++++++++++++++++++++-----------------
 1 files changed, 74 insertions(+), 39 deletions(-)

common/fdt_support.c +6 -4

index efa63f0..d12c751 100644
@@ -32,6 +32,10 @@
 #include <libfdt.h>
 #include <fdt_support.h>
 
+#ifdef CONFIG_OF_BOARD_SETUP
+void ft_board_setup(void *blob, bd_t *bd);
+#endif
+
 /*
  * Global data (for the gd->bd)
  */

@@ -42,7 +46,6 @@ DECLARE_GLOBAL_DATA_PTR;
  */
 struct fdt_header *fdt;
 
-
 /********************************************************************/
 
 int fdt_chosen(void *fdt, ulong initrd_start, ulong initrd_end, int force)

@@ -50,9 +53,8 @@ int fdt_chosen(void *fdt, ulong initrd_start, ulong initrd_end, int force)
 	bd_t *bd = gd->bd;
 	int   nodeoffset;
 	int   err;
-	u32   tmp;			/* used to set 32 bit integer properties */
-	char  *str;			/* used to set string properties */
-	ulong clock;
+	u32   tmp;		/* used to set 32 bit integer properties */
+	char  *str;		/* used to set string properties */
 
 	err = fdt_check_header(fdt);
 	if (err < 0) {

@@ -60,6 +62,17 @@ int fdt_chosen(void *fdt, ulong initrd_start, ulong initrd_end, int force)
 		return err;
 	}
 

common/fdt_support.c int fdt_chosen(void *fdt, ulong initrd_start, ulong initrd_end, int force) · line 75

Quoted 11 lines

common/fdt_support.c +11 -0

+#ifdef CONFIG_OF_BOARD_SETUP
+	/*
+	 * ft_board_setup() sets various board-specific properties to
+	 * the proper values.
+	 *
+	 * STRICTLY SPEAKING, this is out of place, but it isn't clear
+	 * where a better place would be.
+	 */
+	ft_board_setup(fdt, bd);
+#endif
+

I think it's fine here, btw. What are your reservations?

Quoted 124 lines

common/fdt_support.c +39 -29

 	if (initrd_start && initrd_end) {
 		struct fdt_reserve_entry re;
 		int  used;

@@ -72,7 +85,8 @@ int fdt_chosen(void *fdt, ulong initrd_start, ulong initrd_end, int force)
 			return err;
 		}
 		if (used >= total) {
-			printf("WARNING fdt_chosen: no room in the reserved map (%d of %d)\n",
+			printf("WARNING fdt_chosen: "
+				"no room in the reserved map (%d of %d)\n",
 				used, total);
 			return -1;
 		}

@@ -115,7 +129,10 @@ int fdt_chosen(void *fdt, ulong initrd_start, ulong initrd_end, int force)
 		 */
 		nodeoffset = fdt_add_subnode(fdt, 0, "chosen");
 		if (nodeoffset < 0) {
-			printf("WARNING fdt_chosen: could not create the \"/chosen node\" (libfdt error %s).\n", fdt_strerror(nodeoffset));
+			printf("WARNING fdt_chosen: "
+				"could not create the \"/chosen node\" "
+				"(libfdt error %s).\n",
+				fdt_strerror(nodeoffset));
 			return nodeoffset;
 		}
 	}

@@ -125,42 +142,42 @@ int fdt_chosen(void *fdt, ulong initrd_start, ulong initrd_end, int force)
 	 */
 	str = getenv("bootargs");
 	if (str != NULL) {
-		err = fdt_setprop(fdt, nodeoffset, "bootargs", str, strlen(str)+1);
+		err = fdt_setprop(fdt, nodeoffset,
+			"bootargs", str, strlen(str)+1);
 		if (err < 0)
-			printf("WARNING fdt_chosen: could not set \"bootargs\" (libfdt error %s).\n", fdt_strerror(err));
+			printf("WARNING fdt_chosen: "
+				"could not set \"bootargs\" "
+				"(libfdt error %s).\n",
+				fdt_strerror(err));
 	}
 	if (initrd_start && initrd_end) {
 		tmp = __cpu_to_be32(initrd_start);
-		err = fdt_setprop(fdt, nodeoffset, "linux,initrd-start", &tmp, sizeof(tmp));
+		err = fdt_setprop(fdt, nodeoffset,
+			 "linux,initrd-start", &tmp, sizeof(tmp));
 		if (err < 0)
-			printf("WARNING fdt_chosen: could not set \"linux,initrd-start\" (libfdt error %s).\n", fdt_strerror(err));
+			printf("WARNING fdt_chosen: "
+				"could not set \"linux,initrd-start\" "
+				"(libfdt error %s).\n",
+				fdt_strerror(err));
 		tmp = __cpu_to_be32(initrd_end);
-		err = fdt_setprop(fdt, nodeoffset, "linux,initrd-end", &tmp, sizeof(tmp));
+		err = fdt_setprop(fdt, nodeoffset,
+			"linux,initrd-end", &tmp, sizeof(tmp));
 		if (err < 0)
-			printf("WARNING fdt_chosen: could not set \"linux,initrd-end\" (libfdt error %s).\n", fdt_strerror(err));
+			printf("WARNING fdt_chosen: "
+				"could not set \"linux,initrd-end\" "
+				"(libfdt error %s).\n",
+				fdt_strerror(err));
 	}
 #ifdef OF_STDOUT_PATH
-	err = fdt_setprop(fdt, nodeoffset, "linux,stdout-path", OF_STDOUT_PATH, strlen(OF_STDOUT_PATH)+1);
+	err = fdt_setprop(fdt, nodeoffset,
+		"linux,stdout-path", OF_STDOUT_PATH, strlen(OF_STDOUT_PATH)+1);
 	if (err < 0)
-		printf("WARNING fdt_chosen: could not set \"linux,stdout-path\" (libfdt error %s).\n", fdt_strerror(err));
+		printf("WARNING fdt_chosen: "
+			"could not set \"linux,stdout-path\" "
+			"(libfdt error %s).\n",
+			fdt_strerror(err));
 #endif
 
-	nodeoffset = fdt_find_node_by_path (fdt, "/cpus");
-	if (nodeoffset >= 0) {
-		clock = cpu_to_be32(bd->bi_intfreq);
-		err = fdt_setprop(fdt, nodeoffset, "clock-frequency", &clock, 4);
-		if (err < 0)
-			printf("WARNING fdt_chosen: could not set \"clock-frequency\" (libfdt error %s).\n", fdt_strerror(err));
-	}
-#ifdef OF_TBCLK
-	nodeoffset = fdt_find_node_by_path (fdt, "/cpus/" OF_CPU "/timebase-frequency");
-	if (nodeoffset >= 0) {
-		clock = cpu_to_be32(OF_TBCLK);
-		err = fdt_setprop(fdt, nodeoffset, "clock-frequency", &clock, 4);
-		if (err < 0)
-			printf("WARNING fdt_chosen: could not set \"clock-frequency\" (libfdt error %s).\n", fdt_strerror(err));
-	}
-#endif
 	return err;
 }
 

@@ -203,7 +220,10 @@ int fdt_env(void *fdt)
 	 */
 	nodeoffset = fdt_add_subnode(fdt, 0, "u-boot-env");
 	if (nodeoffset < 0) {
-		printf("WARNING fdt_env: could not create the \"/u-boot-env node\" (libfdt error %s).\n", fdt_strerror(nodeoffset));
+		printf("WARNING fdt_env: "
+			"could not create the \"/u-boot-env node\" "
+			"(libfdt error %s).\n",
+			fdt_strerror(nodeoffset));
 		return nodeoffset;
 	}
 

@@ -231,7 +251,10 @@ int fdt_env(void *fdt)
 			continue;
 		err = fdt_setprop(fdt, nodeoffset, lval, rval, strlen(rval)+1);
 		if (err < 0) {
-			printf("WARNING fdt_env: could not set \"%s\" (libfdt error %s).\n", lval, fdt_strerror(err));
+			printf("WARNING fdt_env: "
+				"could not set \"%s\" "
+				"(libfdt error %s).\n",
+				lval, fdt_strerror(err));
 			return err;
 		}
 	}

@@ -297,7 +320,7 @@ int fdt_bd_t(void *fdt)
 	bd_t *bd = gd->bd;
 	int   nodeoffset;
 	int   err;
-	u32   tmp;			/* used to set 32 bit integer properties */
+	u32   tmp;		/* used to set 32 bit integer properties */
 	int i;
 

common/fdt_support.c int fdt_bd_t(void *fdt) · line 353

Quoted 11 lines

common/fdt_support.c +4 -1

 	err = fdt_check_header(fdt);

@@ -323,7 +346,10 @@ int fdt_bd_t(void *fdt)
 	 */
 	nodeoffset = fdt_add_subnode(fdt, 0, "bd_t");
 	if (nodeoffset < 0) {
-		printf("WARNING fdt_bd_t: could not create the \"/bd_t node\" (libfdt error %s).\n", fdt_strerror(nodeoffset));
+		printf("WARNING fdt_bd_t: "
+			"could not create the \"/bd_t node\" "
+			"(libfdt error %s).\n",
+			fdt_strerror(nodeoffset));
 		printf("libfdt: %s\n", fdt_strerror(nodeoffset));

just in case the user didn't get it the first time, eh? ;)

Quoted 22 lines

common/fdt_support.c +6 -3

 		return nodeoffset;
 	}

@@ -332,20 +358,29 @@ int fdt_bd_t(void *fdt)
 	 */
 	for (i = 0; i < sizeof(bd_map)/sizeof(bd_map[0]); i++) {
 		tmp = cpu_to_be32(getenv("bootargs"));
-		err = fdt_setprop(fdt, nodeoffset, bd_map[i].name, &tmp, sizeof(tmp));
+		err = fdt_setprop(fdt, nodeoffset,
+			bd_map[i].name, &tmp, sizeof(tmp));
 		if (err < 0)
-			printf("WARNING fdt_bd_t: could not set \"%s\" (libfdt error %s).\n", bd_map[i].name, fdt_strerror(err));
+			printf("WARNING fdt_bd_t: "
+				"could not set \"%s\" "
+				"(libfdt error %s).\n",
+				bd_map[i].name, fdt_strerror(err));
 	}
 	/*
 	 * Add a couple of oddball entries...
 	 */
 	err = fdt_setprop(fdt, nodeoffset, "enetaddr", &bd->bi_enetaddr, 6);
 	if (err < 0)
-		printf("WARNING fdt_bd_t: could not set \"enetaddr\" (libfdt error %s).\n", fdt_strerror(err));

common/fdt_support.c int fdt_bd_t(void *fdt) · line 381

Quoted 10 lines

common/fdt_support.c +6 -2

+		printf("WARNING fdt_bd_t: "
+			"could not set \"enetaddr\" "
+			"(libfdt error %s).\n",
+			fdt_strerror(err));
 	err = fdt_setprop(fdt, nodeoffset, "ethspeed", &bd->bi_ethspeed, 4);
 	if (err < 0)
-		printf("WARNING fdt_bd_t: could not set \"ethspeed\" (libfdt error %s).\n", fdt_strerror(err));
-
+		printf("WARNING fdt_bd_t: "
+			"could not set \"ethspeed\" "

[these comments apply to all the similar hunks above:]

putting escaped quotes in user messages just makes it harder for the user to grep for error text in the code, IMO. you can achieve the same level of "message correctness" by postpending the word property.

common/fdt_support.c int fdt_bd_t(void *fdt) · line 382

Quoted 1 line

common/fdt_support.c +1 -0

+			"(libfdt error %s).\n",

s/libfdt error //g

common/fdt_support.c int fdt_bd_t(void *fdt) · line 383

Quoted 1 line

common/fdt_support.c +1 -0

+			fdt_strerror(err));

Kim

Quoted 3 lines · Kim Phillips

On Sat, 26 May 2007 08:52:31 -0400
Jerry Van Baren <[email protected]> wrote:

Quoted 5 lines

Quoted 5 lines

common/fdt_support.c +5 -0

+#ifdef CONFIG_OF_BOARD_SETUP
+	/*
+	 * ft_board_setup() sets various board-specific properties to
+	 * the proper values.
+	 *

Quoted 8 lines

common/fdt_support.c int fdt_chosen(void *fdt, ulong initrd_start, ulong initrd_end, int force) · line 75

Quoted 6 lines

common/fdt_support.c +6 -0

+	 * STRICTLY SPEAKING, this is out of place, but it isn't clear
+	 * where a better place would be.
+	 */
+	ft_board_setup(fdt, bd);
+#endif
+

I think it's fine here, btw. What are your reservations?

Not really. ft_board_setup() is not related at all to creating the /chosen node. I've been thinking about this and believe the best approach is to create another "fdt" command "fdt boardsetup" that calls ft_board_setup().

The user can then run each part of the fdt setup separately and watch what is done to the blob:

fdt boardsetup - updates the board-specific fdt values

fdt chosen     - creates or updates the /chosen node
fdt env        - creates the /u-boot-env node
fdt bd_t       - creates the /bd_t node

The cmd_bootm.c will then call all four. Currently it calls the last 3 and fdt_chosen() calls fdt_board_setup() inappropriately IMHO. That is my reservation.

Quoted 4 lines

Quoted 4 lines

common/fdt_support.c +0 -0

@@ -323,7 +346,10 @@ int fdt_bd_t(void *fdt)
 	 */
 	nodeoffset = fdt_add_subnode(fdt, 0, "bd_t");
 	if (nodeoffset < 0) {

Quoted 8 lines

common/fdt_support.c int fdt_bd_t(void *fdt) · line 353

Quoted 6 lines

common/fdt_support.c +4 -1

-		printf("WARNING fdt_bd_t: could not create the \"/bd_t node\" (libfdt error %s).\n", fdt_strerror(nodeoffset));
+		printf("WARNING fdt_bd_t: "
+			"could not create the \"/bd_t node\" "
+			"(libfdt error %s).\n",
+			fdt_strerror(nodeoffset));
 		printf("libfdt: %s\n", fdt_strerror(nodeoffset));

just in case the user didn't get it the first time, eh? ;)

No, the three routines are called separately by fdt * commands, so each could fill the blob's available space separately.

The bootm commands call the three sequentially and aborts on the first one that fails. The chosen node could be created successfully but the u-boot-env node could run out of space.

Did that cover your ;) or did I miss the point?