[U-Boot] [PATCH 1/7] powerpc/mpc8xxx: Enable entering DDR debugging by key press

[U-Boot] [PATCH 1/7] powerpc/mpc8xxx: Enable entering DDR debugging by key press

[York Sun]

Using environmental variable "ddr_interactive" to activate interactive DDR
debugging seomtiems is not enough. For example, after updating SPD with a
valid but wrong image, u-boot won't come up due to wrong DDR configuration.
By enabling key press method, we can enter debug mode to have a chance to
boot without using other tools to recover the board.

CONFIG_FSL_DDR_INTERACTIVE needs to be defined in header file. To enter the
debug mode by key press, press key 'd' shortly after reset, like one would
do to abort auto booting. It is fixed to lower case 'd' at this moment.

Signed-off-by: York Sun <yorksun@freescale.com>
---
 arch/powerpc/cpu/mpc8xxx/ddr/main.c |    6 ++++--
 doc/README.fsl-ddr                  |    7 +++++++
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/cpu/mpc8xxx/ddr/main.c b/arch/powerpc/cpu/mpc8xxx/ddr/main.c
index d6b73c7..a33c9e2 100644
--- a/arch/powerpc/cpu/mpc8xxx/ddr/main.c
+++ b/arch/powerpc/cpu/mpc8xxx/ddr/main.c
@@ -532,9 +532,11 @@ phys_size_t fsl_ddr_sdram(void)
 
 	/* Compute it once normally. */
 #ifdef CONFIG_FSL_DDR_INTERACTIVE
-	if (getenv("ddr_interactive"))
+	if (getenv("ddr_interactive")) {
 		total_memory = fsl_ddr_interactive(&info);
-	else
+	} else if (tstc() && (getc() == 'd')) {	/* we got a key press of 'd' */
+		total_memory = fsl_ddr_interactive(&info);
+	} else
 #endif
 		total_memory = fsl_ddr_compute(&info, STEP_GET_SPD, 0);
 
diff --git a/doc/README.fsl-ddr b/doc/README.fsl-ddr
index 3992640..59583b3 100644
--- a/doc/README.fsl-ddr
+++ b/doc/README.fsl-ddr
@@ -268,6 +268,13 @@ be activated by saving an environment variable "ddr_interactive". The value
 doesn't matter. Once activated, U-boot prompts "FSL DDR>" before enabling DDR
 controller. The available commands can be seen by typing "help".
 
+Another way to enter debug mode without using environment variable is to send
+a key press during boot, like one would do to abort auto boot. To save booting
+time, no additioal delay is added so the window to send the key press is very
+short. For example, user can send the key press using reset command followed by
+hitting enter key twice. In case of power on reset, user can keep hitting any
+key while applying the power.
+
 The example flow of using interactive debugging is
 type command "compute" to calculate the parameters from the default
 type command "print" with arguments to show SPD, options, registers
-- 
1.7.9.5

[U-Boot] [PATCH 2/7] Move DDR command parsing to separate function

[York Sun]

From: James Yang <James.Yang@freescale.com>

Move the FSL DDR prompt command parsing to a separate function
so that it can be reused.

Signed-off-by: James Yang <James.Yang@freescale.com>
---
 arch/powerpc/cpu/mpc8xxx/ddr/interactive.c |  153 ++++++++++++++--------------
 1 file changed, 74 insertions(+), 79 deletions(-)

diff --git a/arch/powerpc/cpu/mpc8xxx/ddr/interactive.c b/arch/powerpc/cpu/mpc8xxx/ddr/interactive.c
index cb71f94..4d1cf3c 100644
--- a/arch/powerpc/cpu/mpc8xxx/ddr/interactive.c
+++ b/arch/powerpc/cpu/mpc8xxx/ddr/interactive.c
@@ -1369,14 +1369,15 @@ struct data_strings {
 
 #define DATA_OPTIONS(name, step, dimm) {#name, step, dimm}
 
-unsigned long long fsl_ddr_interactive(fsl_ddr_info_t *pinfo)
-{
-	unsigned long long ddrsize;
-	const char *prompt = "FSL DDR>";
-	char buffer[CONFIG_SYS_CBSIZE];
-	char *argv[CONFIG_SYS_MAXARGS + 1];	/* NULL terminated */
-	int argc;
-	unsigned int next_step = STEP_GET_SPD;
+static unsigned int fsl_ddr_parse_interactive_cmd(
+	char **argv,
+	int argc,
+	unsigned int *pstep_mask,
+	unsigned int *pctlr_mask,
+	unsigned int *pdimm_mask,
+	unsigned int *pdimm_number_required
+	 ) {
+
 	static const struct data_strings options[] = {
 		DATA_OPTIONS(spd, STEP_GET_SPD, 1),
 		DATA_OPTIONS(dimmparms, STEP_COMPUTE_DIMM_PARMS, 1),
@@ -1386,6 +1387,56 @@ unsigned long long fsl_ddr_interactive(fsl_ddr_info_t *pinfo)
 		DATA_OPTIONS(regs, STEP_COMPUTE_REGS, 0),
 	};
 	static const unsigned int n_opts = ARRAY_SIZE(options);
+
+	unsigned int i, j;
+	unsigned int error = 0;
+	unsigned int matched = 0;
+
+	for (i = 1; i < argc; i++) {
+		for (j = 0; j < n_opts; j++) {
+			if (strcmp(options[j].data_name, argv[i]) != 0)
+				continue;
+			*pstep_mask |= options[j].step_mask;
+			*pdimm_number_required =
+				options[j].dimm_number_required;
+			matched = 1;
+			break;
+		}
+
+		if (matched)
+			continue;
+
+		if (argv[i][0] == 'c') {
+			char c = argv[i][1];
+			if (isdigit(c))
+				*pctlr_mask |= 1 << (c - '0');
+			continue;
+		}
+
+		if (argv[i][0] == 'd') {
+			char c = argv[i][1];
+			if (isdigit(c))
+				*pdimm_mask |= 1 << (c - '0');
+			continue;
+		}
+
+		printf("unknown arg %s\n", argv[i]);
+		*pstep_mask = 0;
+		error = 1;
+		break;
+	}
+
+	return error;
+}
+
+unsigned long long fsl_ddr_interactive(fsl_ddr_info_t *pinfo)
+{
+	unsigned long long ddrsize;
+	const char *prompt = "FSL DDR>";
+	char buffer[CONFIG_SYS_CBSIZE];
+	char *argv[CONFIG_SYS_MAXARGS + 1];	/* NULL terminated */
+	int argc;
+	unsigned int next_step = STEP_GET_SPD;
 	const char *usage = {
 		"commands:\n"
 		"print      print SPD and intermediate computed data\n"
@@ -1426,7 +1477,6 @@ unsigned long long fsl_ddr_interactive(fsl_ddr_info_t *pinfo)
 		}
 
 		if (strcmp(argv[0], "edit") == 0) {
-			unsigned int i, j;
 			unsigned int error = 0;
 			unsigned int step_mask = 0;
 			unsigned int ctlr_mask = 0;
@@ -1436,7 +1486,6 @@ unsigned long long fsl_ddr_interactive(fsl_ddr_info_t *pinfo)
 			unsigned int dimm_number_required = 0;
 			unsigned int ctrl_num;
 			unsigned int dimm_num;
-			unsigned int matched = 0;
 
 			if (argc == 1) {
 				/* Only the element and value must be last */
@@ -1448,41 +1497,13 @@ unsigned long long fsl_ddr_interactive(fsl_ddr_info_t *pinfo)
 				continue;
 			}
 
-			for (i = 1; i < argc - 2; i++) {
-				for (j = 0; j < n_opts; j++) {
-					if (strcmp(options[j].data_name,
-						argv[i]) != 0)
-						continue;
-					step_mask |= options[j].step_mask;
-					dimm_number_required =
-						options[j].dimm_number_required;
-					matched = 1;
-					break;
-				}
-
-				if (matched)
-					continue;
-
-				if (argv[i][0] == 'c') {
-					char c = argv[i][1];
-					if (isdigit(c))
-						ctlr_mask |= 1 << (c - '0');
-					continue;
-				}
-
-				if (argv[i][0] == 'd') {
-					char c = argv[i][1];
-					if (isdigit(c))
-						dimm_mask |= 1 << (c - '0');
-					continue;
-				}
-
-				printf("unknown arg %s\n", argv[i]);
-				step_mask = 0;
-				error = 1;
-				break;
-			}
-
+			error = fsl_ddr_parse_interactive_cmd(
+				argv, argc - 2,
+				&step_mask,
+				&ctlr_mask,
+				&dimm_mask,
+				&dimm_number_required
+			);
 
 			if (error)
 				continue;
@@ -1629,12 +1650,11 @@ unsigned long long fsl_ddr_interactive(fsl_ddr_info_t *pinfo)
 		}
 
 		if (strcmp(argv[0], "print") == 0) {
-			unsigned int i, j;
 			unsigned int error = 0;
 			unsigned int step_mask = 0;
 			unsigned int ctlr_mask = 0;
 			unsigned int dimm_mask = 0;
-			unsigned int matched = 0;
+			unsigned int dimm_number_required = 0;
 
 			if (argc == 1) {
 				printf("print [c<n>] [d<n>] [spd] [dimmparms] "
@@ -1642,38 +1662,13 @@ unsigned long long fsl_ddr_interactive(fsl_ddr_info_t *pinfo)
 				continue;
 			}
 
-			for (i = 1; i < argc; i++) {
-				for (j = 0; j < n_opts; j++) {
-					if (strcmp(options[j].data_name,
-						argv[i]) != 0)
-						continue;
-					step_mask |= options[j].step_mask;
-					matched = 1;
-					break;
-				}
-
-				if (matched)
-					continue;
-
-				if (argv[i][0] == 'c') {
-					char c = argv[i][1];
-					if (isdigit(c))
-						ctlr_mask |= 1 << (c - '0');
-					continue;
-				}
-
-				if (argv[i][0] == 'd') {
-					char c = argv[i][1];
-					if (isdigit(c))
-						dimm_mask |= 1 << (c - '0');
-					continue;
-				}
-
-				printf("unknown arg %s\n", argv[i]);
-				step_mask = 0;
-				error = 1;
-				break;
-			}
+			error = fsl_ddr_parse_interactive_cmd(
+				argv, argc,
+				&step_mask,
+				&ctlr_mask,
+				&dimm_mask,
+				&dimm_number_required
+			);
 
 			if (error)
 				continue;
-- 
1.7.9.5

[U-Boot] [PATCH 3/7] Fix data stage name matching issue

[York Sun]

From: James Yang <James.Yang@freescale.com>

This fix allows the name of the stage to be specifed after the
controler and DIMM is specified.  Prior to this fix, if the
data stage name is not the first entry on the command line,
the operation is applied to all controller and DIMMs.

Signed-off-by: James Yang <James.Yang@freescale.com>
---
 arch/powerpc/cpu/mpc8xxx/ddr/interactive.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/cpu/mpc8xxx/ddr/interactive.c b/arch/powerpc/cpu/mpc8xxx/ddr/interactive.c
index 4d1cf3c..0474acc 100644
--- a/arch/powerpc/cpu/mpc8xxx/ddr/interactive.c
+++ b/arch/powerpc/cpu/mpc8xxx/ddr/interactive.c
@@ -1390,9 +1390,10 @@ static unsigned int fsl_ddr_parse_interactive_cmd(
 
 	unsigned int i, j;
 	unsigned int error = 0;
-	unsigned int matched = 0;
 
 	for (i = 1; i < argc; i++) {
+		unsigned int matched = 0;
+
 		for (j = 0; j < n_opts; j++) {
 			if (strcmp(options[j].data_name, argv[i]) != 0)
 				continue;
-- 
1.7.9.5

[U-Boot] [PATCH 4/7] Add copy command to FSL DDR interactive

[York Sun]

From: James Yang <James.Yang@freescale.com>

Add copy command which allows copying of DIMM/controller settings.
This saves tedious retyping of parameters for each identical DIMM
or controller.

Signed-off-by: James Yang <James.Yang@freescale.com>
---
 arch/powerpc/cpu/mpc8xxx/ddr/interactive.c |  127 ++++++++++++++++++++++++++++
 doc/README.fsl-ddr                         |    5 ++
 2 files changed, 132 insertions(+)

diff --git a/arch/powerpc/cpu/mpc8xxx/ddr/interactive.c b/arch/powerpc/cpu/mpc8xxx/ddr/interactive.c
index 0474acc..e5ee775 100644
--- a/arch/powerpc/cpu/mpc8xxx/ddr/interactive.c
+++ b/arch/powerpc/cpu/mpc8xxx/ddr/interactive.c
@@ -1445,6 +1445,7 @@ unsigned long long fsl_ddr_interactive(fsl_ddr_info_t *pinfo)
 		"recompute  reload SPD and options to default and recompute regs\n"
 		"edit       modify spd, parameter, or option\n"
 		"compute    recompute registers from current next_step to end\n"
+		"copy       copy parameters\n"
 		"next_step  shows current next_step\n"
 		"help       this message\n"
 		"go         program the memory controller and continue with u-boot\n"
@@ -1477,6 +1478,132 @@ unsigned long long fsl_ddr_interactive(fsl_ddr_info_t *pinfo)
 			continue;
 		}
 
+		if (strcmp(argv[0], "copy") == 0) {
+			unsigned int error = 0;
+			unsigned int step_mask = 0;
+			unsigned int src_ctlr_mask = 0;
+			unsigned int src_dimm_mask = 0;
+			unsigned int dimm_number_required = 0;
+			unsigned int src_ctlr_num = 0;
+			unsigned int src_dimm_num = 0;
+			unsigned int dst_ctlr_num = -1;
+			unsigned int dst_dimm_num = -1;
+			unsigned int i, num_dest_parms;
+
+			if (argc == 1) {
+				printf("copy <src c#> <src d#> <spd|dimmparms|commonparms|opts|addresses|regs> <dst c#> <dst d#>\n");
+				continue;
+			}
+
+			error = fsl_ddr_parse_interactive_cmd(
+				argv, argc,
+				&step_mask,
+				&src_ctlr_mask,
+				&src_dimm_mask,
+				&dimm_number_required
+			);
+
+			/* XXX: only dimm_number_required and step_mask will
+			   be used by this function.  Parse the controller and
+			   DIMM number separately because it is easier.  */
+
+			if (error)
+				continue;
+
+			/* parse source destination controller / DIMM */
+
+			num_dest_parms = dimm_number_required ? 2 : 1;
+
+			for (i = 0; i < argc; i++) {
+				if (argv[i][0] == 'c') {
+					char c = argv[i][1];
+					if (isdigit(c)) {
+						src_ctlr_num = (c - '0');
+						break;
+					}
+				}
+			}
+
+			for (i = 0; i < argc; i++) {
+				if (argv[i][0] == 'd') {
+					char c = argv[i][1];
+					if (isdigit(c)) {
+						src_dimm_num = (c - '0');
+						break;
+					}
+				}
+			}
+
+			/* parse destination controller / DIMM */
+
+			for (i = argc - 1; i >= argc - num_dest_parms; i--) {
+				if (argv[i][0] == 'c') {
+					char c = argv[i][1];
+					if (isdigit(c)) {
+						dst_ctlr_num = (c - '0');
+						break;
+					}
+				}
+			}
+
+			for (i = argc - 1; i >= argc - num_dest_parms; i--) {
+				if (argv[i][0] == 'd') {
+					char c = argv[i][1];
+					if (isdigit(c)) {
+						dst_dimm_num = (c - '0');
+						break;
+					}
+				}
+			}
+
+			/* TODO: validate inputs */
+
+			debug("src_ctlr_num = %u, src_dimm_num = %u, dst_ctlr_num = %u, dst_dimm_num = %u, step_mask = %x\n",
+				src_ctlr_num, src_dimm_num, dst_ctlr_num, dst_dimm_num, step_mask);
+
+
+			switch (step_mask) {
+
+			case STEP_GET_SPD:
+				memcpy(&(pinfo->spd_installed_dimms[dst_ctlr_num][dst_dimm_num]),
+					&(pinfo->spd_installed_dimms[src_ctlr_num][src_dimm_num]),
+					sizeof(pinfo->spd_installed_dimms[0][0]));
+				break;
+
+			case STEP_COMPUTE_DIMM_PARMS:
+				memcpy(&(pinfo->dimm_params[dst_ctlr_num][dst_dimm_num]),
+					&(pinfo->dimm_params[src_ctlr_num][src_dimm_num]),
+					sizeof(pinfo->dimm_params[0][0]));
+				break;
+
+			case STEP_COMPUTE_COMMON_PARMS:
+				memcpy(&(pinfo->common_timing_params[dst_ctlr_num]),
+					&(pinfo->common_timing_params[src_ctlr_num]),
+					sizeof(pinfo->common_timing_params[0]));
+				break;
+
+			case STEP_GATHER_OPTS:
+				memcpy(&(pinfo->memctl_opts[dst_ctlr_num]),
+					&(pinfo->memctl_opts[src_ctlr_num]),
+					sizeof(pinfo->memctl_opts[0]));
+				break;
+
+			/* someday be able to have addresses to copy addresses... */
+
+			case STEP_COMPUTE_REGS:
+				memcpy(&(pinfo->fsl_ddr_config_reg[dst_ctlr_num]),
+					&(pinfo->fsl_ddr_config_reg[src_ctlr_num]),
+					sizeof(pinfo->memctl_opts[0]));
+				break;
+
+			default:
+				printf("unexpected step_mask value\n");
+			}
+
+			continue;
+
+		}
+
 		if (strcmp(argv[0], "edit") == 0) {
 			unsigned int error = 0;
 			unsigned int step_mask = 0;
diff --git a/doc/README.fsl-ddr b/doc/README.fsl-ddr
index 59583b3..b2a7c0f 100644
--- a/doc/README.fsl-ddr
+++ b/doc/README.fsl-ddr
@@ -279,6 +279,7 @@ The example flow of using interactive debugging is
 type command "compute" to calculate the parameters from the default
 type command "print" with arguments to show SPD, options, registers
 type command "edit" with arguments to change any if desired
+type command "copy" with arguments to copy controller/dimm settings
 type command "go" to continue calculation and enable DDR controller
 type command "reset" to reset the board
 type command "recompute" to reload SPD and start over
@@ -313,6 +314,10 @@ edit <c#> <d#> <spd|dimmparms|commonparms|opts|addresses|regs> <element> <value>
 			  byte number if the object is SPD
 	<value>		- decimal or heximal (prefixed with 0x) numbers
 
+copy <src c#> <src d#> <spd|dimmparms|commonparms|opts|addresses|regs> <dst c#> <dst d#>
+	same as for "edit" command
+	DIMM numbers ignored for commonparms, opts, and regs
+
 reset
 	no arguement	- reset the board
 
-- 
1.7.9.5

[U-Boot] [PATCH 5/7] README.fsl-ddr typos and update to reflect hotkey

[York Sun]

From: James Yang <James.Yang@freescale.com>

Documentation fix to README.fsl-ddr to fix typos and
to reflect use of 'd' hotkey to enter the FSL DDR debugger.

Signed-off-by: James Yang <James.Yang@freescale.com>
---
 doc/README.fsl-ddr |   35 +++++++++++++++++++++--------------
 1 file changed, 21 insertions(+), 14 deletions(-)

diff --git a/doc/README.fsl-ddr b/doc/README.fsl-ddr
index b2a7c0f..1243a12 100644
--- a/doc/README.fsl-ddr
+++ b/doc/README.fsl-ddr
@@ -263,17 +263,21 @@ Reference http://www.samsung.com/global/business/semiconductor/products/dram/dow
 Interactive DDR debugging
 ===========================
 
-For DDR parameter tuning up and debugging, the interactive DDR debugging can
-be activated by saving an environment variable "ddr_interactive". The value
-doesn't matter. Once activated, U-boot prompts "FSL DDR>" before enabling DDR
-controller. The available commands can be seen by typing "help".
-
-Another way to enter debug mode without using environment variable is to send
-a key press during boot, like one would do to abort auto boot. To save booting
-time, no additioal delay is added so the window to send the key press is very
-short. For example, user can send the key press using reset command followed by
-hitting enter key twice. In case of power on reset, user can keep hitting any
-key while applying the power.
+For DDR parameter tuning up and debugging, the interactive DDR debugger can
+be activated by setting the environment variable "ddr_interactive" to any
+value.  (The value of ddr_interactive may have a meaning in the future, but,
+for now, the presence of the variable will cause the debugger to run.)  Once
+activated, U-boot will show the prompt "FSL DDR>" before enabling the DDR
+controller.  The available commands are printed by typing "help".
+
+Another way to enter the interactive DDR debugger without setting the
+environment variable is to send the 'd' character early during the boot
+process.  To save booting time, no additional delay is added, so the window
+to send the key press is very short -- basically, it is the time before the
+memory controller code starts to run.  For example, when rebooting from
+within u-boot, the user must press 'd' IMMEDIATELY after hitting enter to
+initiate a 'reset' command.  In case of power on/reset, the user can hold
+down the 'd' key while applying power or hitting the board's reset button.
 
 The example flow of using interactive debugging is
 type command "compute" to calculate the parameters from the default
@@ -281,13 +285,16 @@ type command "print" with arguments to show SPD, options, registers
 type command "edit" with arguments to change any if desired
 type command "copy" with arguments to copy controller/dimm settings
 type command "go" to continue calculation and enable DDR controller
+
+Additional commands to restart the debugging are:
 type command "reset" to reset the board
 type command "recompute" to reload SPD and start over
 
 Note, check "next_step" to show the flow. For example, after edit opts, the
 next_step is STEP_ASSIGN_ADDRESSES. After editing registers, the next_step is
-STEP_PROGRAM_REGS. Upon issuing command "go", DDR controller will be enabled
-with current setting without further calculation.
+STEP_PROGRAM_REGS.  Upon issuing command "go", the debugger will program the
+DDR controller with the current setting without further calculation and then
+exit to resume the booting of the machine.
 
 The detail syntax for each commands are
 
@@ -340,7 +347,7 @@ Examples of debugging flow
 
 	FSL DDR>compute
 	Detected UDIMM UG51U6400N8SU-ACF
-	SL DDR>print
+	FSL DDR>print
 	print [c<n>] [d<n>] [spd] [dimmparms] [commonparms] [opts] [addresses] [regs]
 	FSL DDR>print dimmparms
 	DIMM parameters:  Controller=0 DIMM=0
-- 
1.7.9.5

[U-Boot] [PATCH 6/7] getenv_f() env variable exist w/o needing a buffer

[York Sun]

From: James Yang <James.Yang@freescale.com>

getenv_f() searches the environment for a variable name and copies the
value of the variable to a buffer pointed to by one of the function's
parameters.  However, this means that the buffer needs to exist and
needs to be of sufficient length (passed as another parameter to
getenv_f()) to hold the requested variable's value, even if all that is
desired is the mere detection of the existence of the variable itself.

This patch removes the requirement that the buffer needs to exist.  If
the pointer to the buffer is set to NULL and the requested variable is
found, getenv_f() returns 1, else it returns -1.  The buffer length
parameter is ignored if the pointer is set to NULL.  The original
functionality of getenv_f() is retained (return number of bytes copied
if variable is found, -1 if not), other than being able to copy the
variable's value to the address 0.

Signed-off-by: James Yang <James.Yang@freescale.com>
---
 common/cmd_nvedit.c |    9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/common/cmd_nvedit.c b/common/cmd_nvedit.c
index 7633f0c..caa8a36 100644
--- a/common/cmd_nvedit.c
+++ b/common/cmd_nvedit.c
@@ -587,6 +587,9 @@ char *getenv(const char *name)
 
 /*
  * Look up variable from environment for restricted C runtime env.
+ * If the variable is found, return the number of bytes copied.
+ * If buf is NULL, len is ignored, and, if the variable is found, return 1.
+ * If the variable is not found, return -1.
  */
 int getenv_f(const char *name, char *buf, unsigned len)
 {
@@ -604,7 +607,11 @@ int getenv_f(const char *name, char *buf, unsigned len)
 		if (val < 0)
 			continue;
 
-		/* found; copy out */
+		/* found */
+		if (!buf)
+			return 1;
+
+		/* copy out */
 		for (n = 0; n < len; ++n, ++buf) {
 			*buf = env_get_char(val++);
 			if (*buf == '\0')
-- 
1.7.9.5

[U-Boot] [PATCH 6/7] getenv_f() env variable exist w/o needing a buffer

[Wolfgang Denk]

Dear York Sun,

In message <1357323245-12455-6-git-send-email-yorksun@freescale.com> you wrote:
> From: James Yang <James.Yang@freescale.com>
> 
> getenv_f() searches the environment for a variable name and copies the
> value of the variable to a buffer pointed to by one of the function's
> parameters.  However, this means that the buffer needs to exist and
> needs to be of sufficient length (passed as another parameter to
> getenv_f()) to hold the requested variable's value, even if all that is
> desired is the mere detection of the existence of the variable itself.
> 
> This patch removes the requirement that the buffer needs to exist.  If
> the pointer to the buffer is set to NULL and the requested variable is

Hm... this adds a special case and as such increases complexity - and
what is the benefit for you? 

In your code, you use this feature exactly once, which means all you
save is a single buffer on the stack of a function that does not
appear to be critical in terms of stack size.

>  /*
>   * Look up variable from environment for restricted C runtime env.
> + * If the variable is found, return the number of bytes copied.
> + * If buf is NULL, len is ignored, and, if the variable is found, return 1.
> + * If the variable is not found, return -1.

I think your description is not quite correct, and I dislike the
inconsistent behaviour we get though your patch.  So far, this
function returns the length of the variable value, or -1 in case of
errors.  This should not change even if we implement the suggested
feature, i. e. even when passing NULL as buffer pointer the function
should still return the length, and not some unrelated result.

> +		/* found */
> +		if (!buf)
> +			return 1;

I tend to NAK this part.

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
"One lawyer can steal more than a hundred men with guns."
- The Godfather

[U-Boot] [PATCH 6/7] getenv_f() env variable exist w/o needing a buffer

[James Yang]

Hello Wolfgang,


On Fri, 4 Jan 2013, Wolfgang Denk wrote:

> Dear York Sun,
> 
> In message <1357323245-12455-6-git-send-email-yorksun@freescale.com> you wrote:
> > From: James Yang <James.Yang@freescale.com>
> > 
> > getenv_f() searches the environment for a variable name and copies the
> > value of the variable to a buffer pointed to by one of the function's
> > parameters.  However, this means that the buffer needs to exist and
> > needs to be of sufficient length (passed as another parameter to
> > getenv_f()) to hold the requested variable's value, even if all that is
> > desired is the mere detection of the existence of the variable itself.
> > 
> > This patch removes the requirement that the buffer needs to exist.  If
> > the pointer to the buffer is set to NULL and the requested variable is
> 
> Hm... this adds a special case and as such increases complexity - and
> what is the benefit for you? 

The purpose is to avoid having to allocate memory for getenv_f() to 
work.  While the unmodified getenv_f() does let me do that if I pass 
len=0, it has the side effect of printing a warning message that the 
buffer is too small.  I want to avoid this message from being printed 
as well.


> In your code, you use this feature exactly once, which means all you
> save is a single buffer on the stack of a function that does not
> appear to be critical in terms of stack size.

Part 7 of the patchset runs at a point where memory can only be 
allocated from the stack.  The stack is in cache, so any available RAM 
is precious.  The function that calls getenv_f() calls another 
function, so allocating a buffer with an unmodified getenv_f() would 
require the buffer to persist in the calling function's stack frame 
uselessly.  That buffer is of size CONFIG_SYS_CBSIZE, which is either 
256 or 1024, so I wouldn't call it non-critical.

I suppose I could create another function that only calls the 
unmodified getenv_f() and returns a boolean as to whether or not that 
variable exists so that the buffer gets deallocated as soon as the 
function returns, but it would not avoid the need to have that memory 
to be actually allocated on the stack.  Also, if the compiler inlines 
that function (this can be prevented as well), it would still make 
that memory persistent.

I imagine that with the modified getenv_f(), other pre-relocation 
features could be written to utilize the detection of environment 
variables in a similar fashion.  This patch set by itself should not 
be considered as the sole usage case.

 
> >  /*
> >   * Look up variable from environment for restricted C runtime env.
> > + * If the variable is found, return the number of bytes copied.
> > + * If buf is NULL, len is ignored, and, if the variable is found, return 1.
> > + * If the variable is not found, return -1.
> 
> I think your description is not quite correct, and I dislike the
> inconsistent behaviour we get though your patch.  So far, this
> function returns the length of the variable value, or -1 in case of
> errors.  This should not change even if we implement the suggested
> feature, i. e. even when passing NULL as buffer pointer the function
> should still return the length, and not some unrelated result.

The description was not written to be a top-down procedural 
description.  Maybe reordering like this will make it seem more 
correct?

> > + * If buf is NULL, len is ignored, and, if the variable is found, return 1.
> > + * If the variable is found, return the number of bytes copied.
> > + * If the variable is not found, return -1.


> > +		/* found */
> > +		if (!buf)
> > +			return 1;
> 
> I tend to NAK this part.

Would it be acceptable if it returns 0 instead?  The reason I chose 1 
is because all of the 100+ existing usages of getenv_f() check only 
for return value > 0.  I was trying to make it consistent with all of 
those existing usage cases.


Regards,

--James


-- 
James Yang                    Digital Networking
James.Yang at freescale.com      Freescale Semiconductor

[U-Boot] [PATCH 6/7] getenv_f() env variable exist w/o needing a buffer

[Wolfgang Denk]

Dear James Yang,

In message <alpine.LRH.2.00.1301041608190.3906@ra8135-ec1.am.freescale.net> you wrote:
> 
> > Hm... this adds a special case and as such increases complexity - and
> > what is the benefit for you? 
> 
> The purpose is to avoid having to allocate memory for getenv_f() to 

What exactly is the problem of adding a dynamic variable on the stack?
This is a way cheaper operation than adding the code here...

> work.  While the unmodified getenv_f() does let me do that if I pass 
> len=0, it has the side effect of printing a warning message that the 
> buffer is too small.  I want to avoid this message from being printed 
> as well.

Then just provide a big enough buffer.  You don't bother about a few
bytes of stack space, do you?  They cost you nothing...

> Part 7 of the patchset runs at a point where memory can only be 
> allocated from the stack.  The stack is in cache, so any available RAM 
> is precious.  The function that calls getenv_f() calls another 

This argument backfires - because if you detect that the variable is
set, then you will call fsl_ddr_interactive(), which then will alocate
a buffer (char buffer2[CONFIG_SYS_CBSIZE]) and call getenv_f() again,
now for real.

Actually you now need TWO such buffers - see this snippet from your
patch 7/7:

        unsigned long long ddrsize;
        const char *prompt = "FSL DDR>";
        char buffer[CONFIG_SYS_CBSIZE];
+       char buffer2[CONFIG_SYS_CBSIZE];
+       char *p = NULL;
        char *argv[CONFIG_SYS_MAXARGS + 1];     /* NULL terminated */
        int argc;
        unsigned int next_step = STEP_GET_SPD;

I. e. before one such buffer was sufficient, now you need two - if
you care about memory, then dumpt his patch, and leave the code as is
- both your code and your stack footprint will be smaller.

> function, so allocating a buffer with an unmodified getenv_f() would 
> require the buffer to persist in the calling function's stack frame 
> uselessly.  That buffer is of size CONFIG_SYS_CBSIZE, which is either 
> 256 or 1024, so I wouldn't call it non-critical.

But you do this anyway, just in another part of the code.  ANd there
you even need two such buffers now!

> I imagine that with the modified getenv_f(), other pre-relocation 
> features could be written to utilize the detection of environment 
> variables in a similar fashion.  This patch set by itself should not 
> be considered as the sole usage case.

Well, the use case you present shows that while the idea sounds good
initially, the results tend to be worse than the existing code.

You did not convince me that the addition is a good idea.

> The description was not written to be a top-down procedural 
> description.  Maybe reordering like this will make it seem more 
> correct?

This will not remove the inconsistent behaviour of returning a 1 in
one case, indepoendent of the actual length of the value, and the
length in another case.  And there is no need for such an
inconsistency.

> > > +		if (!buf)
> > > +			return 1;
> > 
> > I tend to NAK this part.
> 
> Would it be acceptable if it returns 0 instead?  The reason I chose 1 
> is because all of the 100+ existing usages of getenv_f() check only 
> for return value > 0.  I was trying to make it consistent with all of 
> those existing usage cases.

Why don't you implement consistent behaviour and always return the
correct length of the variable value, and -1 if the variable does not
exist?

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
A failure will not appear until a unit has passed final inspection.

[U-Boot] [u-boot-release] [PATCH 6/7] getenv_f() env variable exist w/o needing a buffer

[Timur Tabi]

York Sun wrote:
> From: James Yang <James.Yang@freescale.com>
> 
> getenv_f() searches the environment for a variable name and copies the
> value of the variable to a buffer pointed to by one of the function's
> parameters.  However, this means that the buffer needs to exist and
> needs to be of sufficient length (passed as another parameter to
> getenv_f()) to hold the requested variable's value, even if all that is
> desired is the mere detection of the existence of the variable itself.
> 
> This patch removes the requirement that the buffer needs to exist.  If
> the pointer to the buffer is set to NULL and the requested variable is
> found, getenv_f() returns 1, else it returns -1.  The buffer length
> parameter is ignored if the pointer is set to NULL.  The original
> functionality of getenv_f() is retained (return number of bytes copied
> if variable is found, -1 if not), other than being able to copy the
> variable's value to the address 0.
> 
> Signed-off-by: James Yang <James.Yang@freescale.com>

Acked-by: Timur Tabi <timur@freescale.com>

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] [PATCH 7/7] powerpc/mpc8xxx: FSL DDR debugger auto run of stored commands

[York Sun]

From: James Yang <James.Yang@freescale.com>

This patch adds the ability for the FSL DDR interactive debugger to
automatically run the sequence of commands stored in the ddr_interactive
environment variable.  Commands are separated using ';'. For example,

ddr_interactive=compute; edit c0 d0 dimmparms caslat_X 0x3FC0; go

Signed-off-by: James Yang <James.Yang@freescale.com>
---
 arch/powerpc/cpu/mpc8xxx/ddr/ddr.h         |    2 +-
 arch/powerpc/cpu/mpc8xxx/ddr/interactive.c |   37 +++++++++++++++++++++++-----
 arch/powerpc/cpu/mpc8xxx/ddr/main.c        |    8 +++---
 3 files changed, 36 insertions(+), 11 deletions(-)

diff --git a/arch/powerpc/cpu/mpc8xxx/ddr/ddr.h b/arch/powerpc/cpu/mpc8xxx/ddr/ddr.h
index c8b0f91..369eaf7 100644
--- a/arch/powerpc/cpu/mpc8xxx/ddr/ddr.h
+++ b/arch/powerpc/cpu/mpc8xxx/ddr/ddr.h
@@ -86,7 +86,7 @@ void fsl_ddr_set_lawbar(
 		unsigned int memctl_interleaved,
 		unsigned int ctrl_num);
 
-unsigned long long fsl_ddr_interactive(fsl_ddr_info_t *pinfo);
+unsigned long long fsl_ddr_interactive(fsl_ddr_info_t *pinfo, int var_is_set);
 void fsl_ddr_get_spd(generic_spd_eeprom_t *ctrl_dimms_spd,
 			   unsigned int ctrl_num);
 
diff --git a/arch/powerpc/cpu/mpc8xxx/ddr/interactive.c b/arch/powerpc/cpu/mpc8xxx/ddr/interactive.c
index e5ee775..1d9ddcc 100644
--- a/arch/powerpc/cpu/mpc8xxx/ddr/interactive.c
+++ b/arch/powerpc/cpu/mpc8xxx/ddr/interactive.c
@@ -1430,11 +1430,13 @@ static unsigned int fsl_ddr_parse_interactive_cmd(
 	return error;
 }
 
-unsigned long long fsl_ddr_interactive(fsl_ddr_info_t *pinfo)
+unsigned long long fsl_ddr_interactive(fsl_ddr_info_t *pinfo, int var_is_set)
 {
 	unsigned long long ddrsize;
 	const char *prompt = "FSL DDR>";
 	char buffer[CONFIG_SYS_CBSIZE];
+	char buffer2[CONFIG_SYS_CBSIZE];
+	char *p = NULL;
 	char *argv[CONFIG_SYS_MAXARGS + 1];	/* NULL terminated */
 	int argc;
 	unsigned int next_step = STEP_GET_SPD;
@@ -1451,16 +1453,39 @@ unsigned long long fsl_ddr_interactive(fsl_ddr_info_t *pinfo)
 		"go         program the memory controller and continue with u-boot\n"
 	};
 
+	if (var_is_set) {
+		if (getenv_f("ddr_interactive", buffer2, CONFIG_SYS_CBSIZE) > 0) {
+			p = buffer2;
+		} else {
+			var_is_set = 0;
+		}
+	}
+
 	/*
 	 * The strategy for next_step is that it points to the next
 	 * step in the computation process that needs to be done.
 	 */
 	while (1) {
-		/*
-		 * No need to worry for buffer overflow here in
-		 * this function;  readline() maxes out at CFG_CBSIZE
-		 */
-		readline_into_buffer(prompt, buffer, 0);
+		if (var_is_set) {
+			char *pend = strchr(p, ';');
+			if (pend) {
+				/* found commmand separator, copy this command and evaluate it. */
+				*pend = '\0';
+				strcpy(buffer, p);
+				p = pend + 1;
+			} else {
+				/* separator was not found, so copy the entire string */
+				strcpy(buffer, p);
+				p = NULL;
+				var_is_set = 0;
+			}
+		} else {
+			/*
+			 * No need to worry for buffer overflow here in
+			 * this function;  readline() maxes out at CFG_CBSIZE
+			 */
+			readline_into_buffer(prompt, buffer, 0);
+		}
 		argc = parse_line(buffer, argv);
 		if (argc == 0)
 			continue;
diff --git a/arch/powerpc/cpu/mpc8xxx/ddr/main.c b/arch/powerpc/cpu/mpc8xxx/ddr/main.c
index a33c9e2..741cd0f 100644
--- a/arch/powerpc/cpu/mpc8xxx/ddr/main.c
+++ b/arch/powerpc/cpu/mpc8xxx/ddr/main.c
@@ -532,10 +532,10 @@ phys_size_t fsl_ddr_sdram(void)
 
 	/* Compute it once normally. */
 #ifdef CONFIG_FSL_DDR_INTERACTIVE
-	if (getenv("ddr_interactive")) {
-		total_memory = fsl_ddr_interactive(&info);
-	} else if (tstc() && (getc() == 'd')) {	/* we got a key press of 'd' */
-		total_memory = fsl_ddr_interactive(&info);
+	if (tstc() && (getc() == 'd')) {	/* we got a key press of 'd' */
+		total_memory = fsl_ddr_interactive(&info, 0);
+	} else if (getenv_f("ddr_interactive", NULL, 0) > 0) {
+		total_memory = fsl_ddr_interactive(&info, 1);
 	} else
 #endif
 		total_memory = fsl_ddr_compute(&info, STEP_GET_SPD, 0);
-- 
1.7.9.5

[U-Boot] [PATCH 1/7] powerpc/mpc8xxx: Enable entering DDR debugging by key press

[Wolfgang Denk]

Dear York Sun,

In message <1357323245-12455-1-git-send-email-yorksun@freescale.com> you wrote:
>...
> CONFIG_FSL_DDR_INTERACTIVE needs to be defined in header file. To enter the
> debug mode by key press, press key 'd' shortly after reset, like one would
> do to abort auto booting. It is fixed to lower case 'd' at this moment.
...
> --- a/doc/README.fsl-ddr
> +++ b/doc/README.fsl-ddr
> @@ -268,6 +268,13 @@ be activated by saving an environment variable "ddr_interactive". The value
>  doesn't matter. Once activated, U-boot prompts "FSL DDR>" before enabling DDR
>  controller. The available commands can be seen by typing "help".
>  
> +Another way to enter debug mode without using environment variable is to send
> +a key press during boot, like one would do to abort auto boot. To save booting
> +time, no additioal delay is added so the window to send the key press is very
> +short. For example, user can send the key press using reset command followed by
> +hitting enter key twice. In case of power on reset, user can keep hitting any
> +key while applying the power.

The documentation here does not mention the 'd' key at all.  Guess it
should?

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
You're dead, Jim.
	-- McCoy, "Amok Time", stardate 3372.7

[U-Boot] [PATCH 1/7] powerpc/mpc8xxx: Enable entering DDR debugging by key press

[sun york-R58495]

On Jan 7, 2013, at 10:35 PM, Wolfgang Denk wrote:

> Dear York Sun,
> 
> In message <1357323245-12455-1-git-send-email-yorksun@freescale.com> you wrote:
>> ...
>> CONFIG_FSL_DDR_INTERACTIVE needs to be defined in header file. To enter the
>> debug mode by key press, press key 'd' shortly after reset, like one would
>> do to abort auto booting. It is fixed to lower case 'd' at this moment.
> ...
>> --- a/doc/README.fsl-ddr
>> +++ b/doc/README.fsl-ddr
>> @@ -268,6 +268,13 @@ be activated by saving an environment variable "ddr_interactive". The value
>> doesn't matter. Once activated, U-boot prompts "FSL DDR>" before enabling DDR
>> controller. The available commands can be seen by typing "help".
>> 
>> +Another way to enter debug mode without using environment variable is to send
>> +a key press during boot, like one would do to abort auto boot. To save booting
>> +time, no additioal delay is added so the window to send the key press is very
>> +short. For example, user can send the key press using reset command followed by
>> +hitting enter key twice. In case of power on reset, user can keep hitting any
>> +key while applying the power.
> 
> The documentation here does not mention the 'd' key at all.  Guess it
> should?
> 

Guess I might have generated the patch from a wrong branch. Will update.

Thanks,

York

[U-Boot] [PATCH 1/7] powerpc/mpc8xxx: Enable entering DDR debugging by key press

[York Sun]

On 01/07/2013 10:35 PM, Wolfgang Denk wrote:
> Dear York Sun,
> 
> In message <1357323245-12455-1-git-send-email-yorksun@freescale.com> you wrote:
>> ...
>> CONFIG_FSL_DDR_INTERACTIVE needs to be defined in header file. To enter the
>> debug mode by key press, press key 'd' shortly after reset, like one would
>> do to abort auto booting. It is fixed to lower case 'd' at this moment.
> ...
>> --- a/doc/README.fsl-ddr
>> +++ b/doc/README.fsl-ddr
>> @@ -268,6 +268,13 @@ be activated by saving an environment variable "ddr_interactive". The value
>>  doesn't matter. Once activated, U-boot prompts "FSL DDR>" before enabling DDR
>>  controller. The available commands can be seen by typing "help".
>>  
>> +Another way to enter debug mode without using environment variable is to send
>> +a key press during boot, like one would do to abort auto boot. To save booting
>> +time, no additioal delay is added so the window to send the key press is very
>> +short. For example, user can send the key press using reset command followed by
>> +hitting enter key twice. In case of power on reset, user can keep hitting any
>> +key while applying the power.
> 
> The documentation here does not mention the 'd' key at all.  Guess it
> should?
> 
>
It should. And James found my error and fixed in the patch 5/7 in this
series.

York