diff mbox

[U-Boot,23/30] km/common: replace env var checkboardidlist by function

Message ID e341fcb668eb67e1eb44ea638e96f9bb514c8bfb.1302272395.git.valentin.longchamp@keymile.com
State Changes Requested
Headers show

Commit Message

Valentin Longchamp April 8, 2011, 2:24 p.m. UTC
From: Thomas Herzmann <thomas.herzmann@keymile.com>

The environment variable (defining a checkboardidlist function)
has been replaced by a u-boot function call. This call is much
faster and the environment is a leaner.

Signed-off-by: Thomas Herzmann <thomas.herzmann@keymile.com>
Signed-off-by: Holger Brunck <holger.brunck@keymile.com>
Acked-by: Heiko Schocher <hs@denx.de>
cc: Wolfgang Denk <wd@denx.de>
cc: Detlev Zundel <dzu@denx.de>
cc: Valentin Longchamp <valentin.longchamp@keymile.com>
Signed-off-by: Valentin Longchamp <valentin.longchamp@keymile.com>
---
 board/keymile/common/common.c    |  112 ++++++++++++++++++++++++++++++++++++++
 include/configs/keymile-common.h |   30 +----------
 2 files changed, 113 insertions(+), 29 deletions(-)

Comments

Wolfgang Denk April 30, 2011, 8:29 a.m. UTC | #1
Dear Valentin Longchamp,

In message <e341fcb668eb67e1eb44ea638e96f9bb514c8bfb.1302272395.git.valentin.longchamp@keymile.com> you wrote:
> From: Thomas Herzmann <thomas.herzmann@keymile.com>
> 
> The environment variable (defining a checkboardidlist function)
> has been replaced by a u-boot function call. This call is much
> faster and the environment is a leaner.
...
> +	p = get_local_var("IVM_BoardId");
> +	strict_strtoul(p, 16, &ivmbid);
> +	p = get_local_var("IVM_HWKey");
> +	strict_strtoul(p, 16, &ivmhwkey);

Error handling missing.

> +			while (!found) {
> +				/* loop over each bid/hwkey pair in the list */
> +				unsigned long bid   = 0;
> +				unsigned long hwkey = 0;
> +				while (*rest && !isxdigit(*rest))
> +					rest++;
> +				bid = simple_strtoul(rest, &endp, 16);
> +				if (*endp == '_') {
> +					rest  = endp + 1;
> +					hwkey =	simple_strtoul(rest, &endp, 16);
> +					rest  = endp;
> +					while (*rest && !isxdigit(*rest))
> +						rest++;
> +				}
> +				if ((!bid) || (!hwkey)) {
> +					/* end of list */
> +					break;
> +				}
> +				if (verbose) {
> +					printf("trying bid=0x%lX, hwkey=%ld\n",
> +						bid, hwkey);
> +				}
> +				if ((bid == ivmbid) && (hwkey == ivmhwkey)) {
> +					char buf[10];
> +
> +					found = 1;
> +					envbid   = bid;
> +					envhwkey = hwkey;
> +					sprintf(buf, "%lx", bid);
> +					setenv("boardid", buf);
> +					sprintf(buf, "%lx", hwkey);
> +					setenv("hwkey", buf);
> +					saveenv();
> +				}

How well has this code been tested?  I may not understand it correctly
(which might be an indication that some comments are missing that
actually explain what you are doing), or there might be some unhandled
error cases.

Best regards,

Wolfgang Denk
Holger Brunck May 2, 2011, 8:30 a.m. UTC | #2
Hello,

On 04/30/2011 10:29 AM, Wolfgang Denk wrote:
> Dear Valentin Longchamp,
> 
> In message <e341fcb668eb67e1eb44ea638e96f9bb514c8bfb.1302272395.git.valentin.longchamp@keymile.com> you wrote:
>> From: Thomas Herzmann <thomas.herzmann@keymile.com>
>>
>> The environment variable (defining a checkboardidlist function)
>> has been replaced by a u-boot function call. This call is much
>> faster and the environment is a leaner.
> ...
>> +	p = get_local_var("IVM_BoardId");
>> +	strict_strtoul(p, 16, &ivmbid);
>> +	p = get_local_var("IVM_HWKey");
>> +	strict_strtoul(p, 16, &ivmhwkey);
> 
> Error handling missing.
> 

Ok.

>> +			while (!found) {
>> +				/* loop over each bid/hwkey pair in the list */
>> +				unsigned long bid   = 0;
>> +				unsigned long hwkey = 0;
>> +				while (*rest && !isxdigit(*rest))
>> +					rest++;
>> +				bid = simple_strtoul(rest, &endp, 16);
>> +				if (*endp == '_') {
>> +					rest  = endp + 1;
>> +					hwkey =	simple_strtoul(rest, &endp, 16);
>> +					rest  = endp;
>> +					while (*rest && !isxdigit(*rest))
>> +						rest++;
>> +				}
>> +				if ((!bid) || (!hwkey)) {
>> +					/* end of list */
>> +					break;
>> +				}
>> +				if (verbose) {
>> +					printf("trying bid=0x%lX, hwkey=%ld\n",
>> +						bid, hwkey);
>> +				}
>> +				if ((bid == ivmbid) && (hwkey == ivmhwkey)) {
>> +					char buf[10];
>> +
>> +					found = 1;
>> +					envbid   = bid;
>> +					envhwkey = hwkey;
>> +					sprintf(buf, "%lx", bid);
>> +					setenv("boardid", buf);
>> +					sprintf(buf, "%lx", hwkey);
>> +					setenv("hwkey", buf);
>> +					saveenv();
>> +				}
> 
> How well has this code been tested?  I may not understand it correctly
> (which might be an indication that some comments are missing that
> actually explain what you are doing), or there might be some unhandled
> error cases.
> 

It is tested on our boards. But indeed the commit message should be better to be
able to understand the functionality. Additionaly we check if additionaly some
error cases are not handled.

Best regards
Holger Brunck
Wolfgang Denk May 2, 2011, 8:43 a.m. UTC | #3
Dear Holger Brunck,

In message <4DBE6B91.7090008@keymile.com> you wrote:
> 
> > How well has this code been tested?  I may not understand it correctly
> > (which might be an indication that some comments are missing that
> > actually explain what you are doing), or there might be some unhandled
> > error cases.
> 
> It is tested on our boards. But indeed the commit message should be better to be
> able to understand the functionality. Additionaly we check if additionaly some
> error cases are not handled.

And please comment the _code_!

Best regards,

Wolfgang Denk
diff mbox

Patch

diff --git a/board/keymile/common/common.c b/board/keymile/common/common.c
index 5704b7f..72278a0 100644
--- a/board/keymile/common/common.c
+++ b/board/keymile/common/common.c
@@ -32,6 +32,7 @@ 
 #include <net.h>
 #include <netdev.h>
 #include <asm/io.h>
+#include <linux/ctype.h>
 
 #if defined(CONFIG_OF_BOARD_SETUP) && defined(CONFIG_OF_LIBFDT)
 #include <libfdt.h>
@@ -714,3 +715,114 @@  static int do_setboardid(cmd_tbl_t *cmdtp, int flag, int argc,
 U_BOOT_CMD(km_setboardid, 1, 0, do_setboardid, "setboardid", "read out bid and "
 				 "hwkey from IVM and set in environment");
 
+/*
+ * command km_checkboardidlist
+ *	if "boardid" and "hwkey" are not already set in the environment, do:
+ *		if a "boardIdListHex" exists in the environment:
+ *			- read ivm data for boardid and hwkey
+ *			- compare each entry of the boardIdListHex with the
+ *				IVM data:
+ *			if match:
+ *				set environment variables boardid, boardId,
+ *				hwkey, hwKey to	the found values
+ *				both (boardid and boardId) are set because
+ *				they might be used differently in the
+ *				application and in the init scripts (?)
+ *	return 0 in case of match, 1 if not match or error
+ */
+int do_checkboardidlist(cmd_tbl_t *cmdtp, int flag, int argc,
+			char *const argv[])
+{
+	unsigned long ivmbid = 0, ivmhwkey = 0;
+	unsigned long envbid = 0, envhwkey = 0;
+	char *p;
+	int verbose = argc > 1 && *argv[1] == 'v';
+	int rc = 1;	/* default: no match */
+
+	p = get_local_var("IVM_BoardId");
+	strict_strtoul(p, 16, &ivmbid);
+	p = get_local_var("IVM_HWKey");
+	strict_strtoul(p, 16, &ivmhwkey);
+
+	if (!ivmbid || !ivmhwkey) {
+		printf("Error: IVM_BoardId and/or IVM_HWKey not set!\n");
+		return rc;
+	}
+
+	/* try to read values from environment */
+	p = getenv("boardid");
+	if (p != NULL)
+		strict_strtoul(p, 16, &envbid);
+	p = getenv("hwkey");
+	if (p != NULL)
+		strict_strtoul(p, 16, &envhwkey);
+
+	if (!envbid || !envhwkey) {
+		/* must set environment first */
+		char *bidhwklist = getenv("boardIdListHex");
+		if (bidhwklist) {
+			int found = 0;
+			char *rest = bidhwklist;
+			char *endp;
+			if (verbose) {
+				printf("IVM_BoardId: %ld, IVM_HWKey=%ld\n",
+					ivmbid, ivmhwkey);
+				printf("boardIdHwKeyList: %s\n",
+					bidhwklist);
+			}
+			while (!found) {
+				/* loop over each bid/hwkey pair in the list */
+				unsigned long bid   = 0;
+				unsigned long hwkey = 0;
+				while (*rest && !isxdigit(*rest))
+					rest++;
+				bid = simple_strtoul(rest, &endp, 16);
+				if (*endp == '_') {
+					rest  = endp + 1;
+					hwkey =	simple_strtoul(rest, &endp, 16);
+					rest  = endp;
+					while (*rest && !isxdigit(*rest))
+						rest++;
+				}
+				if ((!bid) || (!hwkey)) {
+					/* end of list */
+					break;
+				}
+				if (verbose) {
+					printf("trying bid=0x%lX, hwkey=%ld\n",
+						bid, hwkey);
+				}
+				if ((bid == ivmbid) && (hwkey == ivmhwkey)) {
+					char buf[10];
+
+					found = 1;
+					envbid   = bid;
+					envhwkey = hwkey;
+					sprintf(buf, "%lx", bid);
+					setenv("boardid", buf);
+					sprintf(buf, "%lx", hwkey);
+					setenv("hwkey", buf);
+					saveenv();
+				}
+			} /* end while( ! found ) */
+		}
+	}
+
+	/* compare now the values */
+	if ((ivmbid == envbid) && (ivmhwkey == envhwkey)) {
+		printf("boardid=0x%3lX, hwkey=%ld\n", envbid, envhwkey);
+		rc = 0;
+	} else {
+		printf("Error: env bId=0x%3lX, hwKey=%ld\n", envbid, envhwkey);
+		printf("       IVM bId=0x%3lX, hwKey=%ld\n", ivmbid, ivmhwkey);
+	}
+	return rc;
+}
+
+U_BOOT_CMD(km_checkbidhwk, 2, 0, do_checkboardidlist,
+		"check boardid and hwkey",
+		"[v]\n  - check environment parameter "\
+		"\"boardIdListHex\" against stored boardid and hwkey "\
+		"from the IVM\n    v: verbose output"
+);
+
diff --git a/include/configs/keymile-common.h b/include/configs/keymile-common.h
index 5c32023..a6d3bcf 100644
--- a/include/configs/keymile-common.h
+++ b/include/configs/keymile-common.h
@@ -240,7 +240,6 @@ 
 	"release="							\
 		"setenv actual_bank ${initial_boot_bank} && "		\
 		"setenv subbootcmds \""					\
-		"checkboardidlist "					\
 		"checkboardid "						\
 		"ubiattach ubicopy "					\
 		"cramfsloadfdt cramfsloadkernel "			\
@@ -391,36 +390,9 @@ 
 	"default="							\
 		"setenv default 'run newenv; reset' &&  "		\
 		"run release && saveenv; reset\0"			\
-	"checkboardidlist="						\
-		"if test \"x${boardIdListHex}\" != \"x\"; then "	\
-		"IVMbidhwk=${IVM_BoardId}_${IVM_HWKey}; "		\
-		"found=0; "						\
-		"for bidhwk in \"${boardIdListHex}\"; do "		\
-		"echo trying $bidhwk ...; "				\
-		"if test \"x$bidhwk\" = \"x$IVMbidhwk\"; then "		\
-		"found=1; "						\
-		"echo match found for $bidhwk; "			\
-		"if test \"x$bidhwk\" != \"x${boardId}_${hwKey}\";then "\
-			"setenv boardid ${IVM_BoardId}; "		\
-			"setenv boardId ${IVM_BoardId}; "		\
-			"setenv hwkey ${IVM_HWKey}; "			\
-			"setenv hwKey ${IVM_HWKey}; "			\
-			"echo \"boardId set to ${boardId}\"; "		\
-			"echo \"hwKey   set to ${hwKey}\"; "		\
-			"saveenv; "					\
-		"fi; "							\
-		"fi; "							\
-		"done; "						\
-		"else "							\
-			"echo \"boardIdListHex not set, not checked\"; "\
-			"found=1; "					\
-		"fi; "							\
-		"test \"$found\" = 1 \0"				\
-	"checkboardid="							\
-		"test \"x${boardId}\" = \"x${IVM_BoardId}\" && "	\
-		"test \"x${hwKey}\" = \"x${IVM_HWKey}\"\0"		\
 	"printbootargs=print bootargs\0"				\
 	"rootfsfile="xstr(CONFIG_HOSTNAME) "/rootfsImage\0"		\
+	"checkboardid=km_checkbidhwk\0"					\
 	""
 
 #ifndef CONFIG_KM_DEF_ENV