[U-Boot] [PATCH 1/7] powerpc/mpc8xxx: Enable entering DDR debugging by key press
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
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;
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;
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
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
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')
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
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
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
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
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);
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
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
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
participants (5)
-
James Yang -
sun york-R58495 -
Timur Tabi -
Wolfgang Denk -
York Sun