[U-Boot] [PATCH v2 2/2] powerpc/8xxx: Add new hwconfig APIs to address early parsing used by DDR init
There are several users of the hwconfig APIs (8xxx DDR) before we have the environment properly setup. This causes issues because of the numerous ways the environment might be accessed because of the non-volatile memory it might be stored in. Additionally the access might be so early that memory isn't even properly setup for us.
Towards resolving these issues we provide versions of all the hwconfig APIs that can be passed in a buffer to parse and leave it to the caller to determine how to allocate and populate the buffer.
We use the _f naming convention for these new APIs even though they are perfectly useable after relocation and the environment being ready.
We also now warn if the non-f APIs are called before the environment is ready to allow users to address the issues.
Finally, we convert the 8xxx DDR code to utilize the new APIs to hopefully address the issue once and for all. We have the 8xxx DDR code create a buffer on the stack and populate it via getenv_f().
Signed-off-by: Kumar Gala galak@kernel.crashing.org --- * Add a comment about the buffer size arch/powerpc/cpu/mpc8xxx/ddr/options.c | 78 ++++++++++++++++++++++------- common/hwconfig.c | 86 +++++++++++++++++--------------- include/hwconfig.h | 68 +++++++++++++++++++------ 3 files changed, 155 insertions(+), 77 deletions(-)
diff --git a/arch/powerpc/cpu/mpc8xxx/ddr/options.c b/arch/powerpc/cpu/mpc8xxx/ddr/options.c index c641e85..0e7097b 100644 --- a/arch/powerpc/cpu/mpc8xxx/ddr/options.c +++ b/arch/powerpc/cpu/mpc8xxx/ddr/options.c @@ -13,6 +13,14 @@
#include "ddr.h"
+/* + * Use our own stack based buffer before relocation to allow accessing longer + * hwconfig strings that might be in the environment before we've relocated. + * This is pretty fragile on both the use of stack and if the buffer is big + * enough. However we will get a warning from getenv_f for the later. + */ +#define HWCONFIG_BUFFER_SIZE 128 + /* Board-specific functions defined in each board's ddr.c */ extern void fsl_ddr_board_options(memctl_options_t *popts, dimm_params_t *pdimm, @@ -24,6 +32,15 @@ unsigned int populate_memctl_options(int all_DIMMs_registered, unsigned int ctrl_num) { unsigned int i; + char buffer[HWCONFIG_BUFFER_SIZE]; + char *buf = NULL; + + /* + * Extract hwconfig from environment since we have not properly setup + * the environment but need it for ddr config params + */ + if (getenv_f("hwconfig", buffer, sizeof(buffer)) > 0) + buf = buffer;
/* Chip select options. */
@@ -221,7 +238,7 @@ unsigned int populate_memctl_options(int all_DIMMs_registered, * should be a subset of the requested configuration. */ #if (CONFIG_NUM_DDR_CONTROLLERS > 1) - if (hwconfig_sub("fsl_ddr", "ctlr_intlv")) { + if (hwconfig_sub_f("fsl_ddr", "ctlr_intlv", buf)) { if (pdimm[0].n_ranks == 0) { printf("There is no rank on CS0 for controller %d. Because only" " rank on CS0 and ranks chip-select interleaved with CS0" @@ -234,19 +251,25 @@ unsigned int populate_memctl_options(int all_DIMMs_registered, * test null first. if CONFIG_HWCONFIG is not defined * hwconfig_arg_cmp returns non-zero */ - if (hwconfig_subarg_cmp("fsl_ddr", "ctlr_intlv", "null")) { + if (hwconfig_subarg_cmp_f("fsl_ddr", "ctlr_intlv", + "null", buf)) { popts->memctl_interleaving = 0; debug("memory controller interleaving disabled.\n"); - } else if (hwconfig_subarg_cmp("fsl_ddr", "ctlr_intlv", "cacheline")) + } else if (hwconfig_subarg_cmp_f("fsl_ddr", + "ctlr_intlv", + "cacheline", buf)) popts->memctl_interleaving_mode = FSL_DDR_CACHE_LINE_INTERLEAVING; - else if (hwconfig_subarg_cmp("fsl_ddr", "ctlr_intlv", "page")) + else if (hwconfig_subarg_cmp_f("fsl_ddr", "ctlr_intlv", + "page", buf)) popts->memctl_interleaving_mode = FSL_DDR_PAGE_INTERLEAVING; - else if (hwconfig_subarg_cmp("fsl_ddr", "ctlr_intlv", "bank")) + else if (hwconfig_subarg_cmp_f("fsl_ddr", "ctlr_intlv", + "bank", buf)) popts->memctl_interleaving_mode = FSL_DDR_BANK_INTERLEAVING; - else if (hwconfig_subarg_cmp("fsl_ddr", "ctlr_intlv", "superbank")) + else if (hwconfig_subarg_cmp_f("fsl_ddr", "ctlr_intlv", + "superbank", buf)) popts->memctl_interleaving_mode = FSL_DDR_SUPERBANK_INTERLEAVING; else { @@ -256,19 +279,24 @@ unsigned int populate_memctl_options(int all_DIMMs_registered, } } #endif - if ((hwconfig_sub("fsl_ddr", "bank_intlv")) && + if ((hwconfig_sub_f("fsl_ddr", "bank_intlv", buf)) && (CONFIG_CHIP_SELECTS_PER_CTRL > 1)) { /* test null first. if CONFIG_HWCONFIG is not defined, - * hwconfig_arg_cmp returns non-zero */ - if (hwconfig_subarg_cmp("fsl_ddr", "bank_intlv", "null")) + * hwconfig_subarg_cmp_f returns non-zero */ + if (hwconfig_subarg_cmp_f("fsl_ddr", "bank_intlv", + "null", buf)) debug("bank interleaving disabled.\n"); - else if (hwconfig_subarg_cmp("fsl_ddr", "bank_intlv", "cs0_cs1")) + else if (hwconfig_subarg_cmp_f("fsl_ddr", "bank_intlv", + "cs0_cs1", buf)) popts->ba_intlv_ctl = FSL_DDR_CS0_CS1; - else if (hwconfig_subarg_cmp("fsl_ddr", "bank_intlv", "cs2_cs3")) + else if (hwconfig_subarg_cmp_f("fsl_ddr", "bank_intlv", + "cs2_cs3", buf)) popts->ba_intlv_ctl = FSL_DDR_CS2_CS3; - else if (hwconfig_subarg_cmp("fsl_ddr", "bank_intlv", "cs0_cs1_and_cs2_cs3")) + else if (hwconfig_subarg_cmp_f("fsl_ddr", "bank_intlv", + "cs0_cs1_and_cs2_cs3", buf)) popts->ba_intlv_ctl = FSL_DDR_CS0_CS1_AND_CS2_CS3; - else if (hwconfig_subarg_cmp("fsl_ddr", "bank_intlv", "cs0_cs1_cs2_cs3")) + else if (hwconfig_subarg_cmp_f("fsl_ddr", "bank_intlv", + "cs0_cs1_cs2_cs3", buf)) popts->ba_intlv_ctl = FSL_DDR_CS0_CS1_CS2_CS3; else printf("hwconfig has unrecognized parameter for bank_intlv.\n"); @@ -342,10 +370,11 @@ unsigned int populate_memctl_options(int all_DIMMs_registered, } }
- if (hwconfig_sub("fsl_ddr", "addr_hash")) { - if (hwconfig_subarg_cmp("fsl_ddr", "addr_hash", "null")) + if (hwconfig_sub_f("fsl_ddr", "addr_hash", buf)) { + if (hwconfig_subarg_cmp_f("fsl_ddr", "addr_hash", "null", buf)) popts->addr_hash = 0; - else if (hwconfig_subarg_cmp("fsl_ddr", "addr_hash", "true")) + else if (hwconfig_subarg_cmp_f("fsl_ddr", "addr_hash", + "true", buf)) popts->addr_hash = 1; }
@@ -393,11 +422,22 @@ int fsl_use_spd(void) int use_spd = 0;
#ifdef CONFIG_DDR_SPD + char buffer[HWCONFIG_BUFFER_SIZE]; + char *buf = NULL; + + /* + * Extract hwconfig from environment since we have not properly setup + * the environment but need it for ddr config params + */ + if (getenv_f("hwconfig", buffer, sizeof(buffer)) > 0) + buf = buffer; + /* if hwconfig is not enabled, or "sdram" is not defined, use spd */ - if (hwconfig_sub("fsl_ddr", "sdram")) { - if (hwconfig_subarg_cmp("fsl_ddr", "sdram", "spd")) + if (hwconfig_sub_f("fsl_ddr", "sdram", buf)) { + if (hwconfig_subarg_cmp_f("fsl_ddr", "sdram", "spd", buf)) use_spd = 1; - else if (hwconfig_subarg_cmp("fsl_ddr", "sdram", "fixed")) + else if (hwconfig_subarg_cmp_f("fsl_ddr", "sdram", + "fixed", buf)) use_spd = 0; else use_spd = 1; diff --git a/common/hwconfig.c b/common/hwconfig.c index 193863a..f909fa5 100644 --- a/common/hwconfig.c +++ b/common/hwconfig.c @@ -2,6 +2,7 @@ * An inteface for configuring a hardware via u-boot environment. * * Copyright (c) 2009 MontaVista Software, Inc. + * Copyright 2011 Freescale Semiconductor, Inc. * * Author: Anton Vorontsov avorontsov@ru.mvista.com * @@ -71,25 +72,21 @@ next: const char cpu_hwconfig[] __attribute__((weak)) = ""; const char board_hwconfig[] __attribute__((weak)) = "";
-#define HWCONFIG_PRE_RELOC_BUF_SIZE 128 - -static const char *__hwconfig(const char *opt, size_t *arglen) +static const char *__hwconfig(const char *opt, size_t *arglen, char *buf) { const char *env_hwconfig = NULL, *ret; - char buf[HWCONFIG_PRE_RELOC_BUF_SIZE];
- if (gd->flags & GD_FLG_ENV_READY) { - env_hwconfig = getenv("hwconfig"); + /* if we are passed a buffer use it, otherwise try the environment */ + if (!buf) { + if (gd->flags & GD_FLG_ENV_READY) { + env_hwconfig = getenv("hwconfig"); + } else { + printf("WARNING: Calling __hwconfig without a buffer " + "and before environment is ready\n"); + return NULL; + } } else { - /* - * Use our own on stack based buffer before relocation to allow - * accessing longer hwconfig strings that might be in the - * environment before we've relocated. This is pretty fragile - * on both the use of stack and if the buffer is big enough. - * However we will get a warning from getenv_f for the later. - */ - if ((getenv_f("hwconfig", buf, sizeof(buf))) > 0) - env_hwconfig = buf; + env_hwconfig = buf; }
if (env_hwconfig) { @@ -109,8 +106,9 @@ static const char *__hwconfig(const char *opt, size_t *arglen) }
/* - * hwconfig - query if a particular hwconfig option is specified + * hwconfig_f - query if a particular hwconfig option is specified * @opt: a string representing an option + * @buf: if non-NULL use this buffer to parse, otherwise try env * * This call can be used to find out whether U-Boot should configure * a particular hardware option. @@ -127,34 +125,36 @@ static const char *__hwconfig(const char *opt, size_t *arglen) * that the board file only calls things that are actually used, so * hwconfig() will always return true result. */ -int hwconfig(const char *opt) +int hwconfig_f(const char *opt, char *buf) { - return !!__hwconfig(opt, NULL); + return !!__hwconfig(opt, NULL, buf); }
/* - * hwconfig_arg - get hwconfig option's argument + * hwconfig_arg_f - get hwconfig option's argument * @opt: a string representing an option * @arglen: a pointer to an allocated size_t variable + * @buf: if non-NULL use this buffer to parse, otherwise try env * - * Unlike hwconfig() function, this function returns a pointer to the + * Unlike hwconfig_f() function, this function returns a pointer to the * start of the hwconfig arguments, if option is not found or it has * no specified arguments, the function returns NULL pointer. * * If CONFIG_HWCONFIG is undefined, the function returns "", and * arglen is set to 0. */ -const char *hwconfig_arg(const char *opt, size_t *arglen) +const char *hwconfig_arg_f(const char *opt, size_t *arglen, char *buf) { - return __hwconfig(opt, arglen); + return __hwconfig(opt, arglen, buf); }
/* - * hwconfig_arg_cmp - compare hwconfig option's argument + * hwconfig_arg_cmp_f - compare hwconfig option's argument * @opt: a string representing an option * @arg: a string for comparing an option's argument + * @buf: if non-NULL use this buffer to parse, otherwise try env * - * This call is similar to hwconfig_arg, but instead of returning + * This call is similar to hwconfig_arg_f, but instead of returning * hwconfig argument and its length, it is comparing it to @arg. * * Returns non-zero value if @arg matches, 0 otherwise. @@ -162,12 +162,12 @@ const char *hwconfig_arg(const char *opt, size_t *arglen) * If CONFIG_HWCONFIG is undefined, the function returns a non-zero * value, i.e. the argument matches. */ -int hwconfig_arg_cmp(const char *opt, const char *arg) +int hwconfig_arg_cmp_f(const char *opt, const char *arg, char *buf) { const char *argstr; size_t arglen;
- argstr = hwconfig_arg(opt, &arglen); + argstr = hwconfig_arg_f(opt, &arglen, buf); if (!argstr || arglen != strlen(arg)) return 0;
@@ -175,63 +175,67 @@ int hwconfig_arg_cmp(const char *opt, const char *arg) }
/* - * hwconfig_sub - query if a particular hwconfig sub-option is specified + * hwconfig_sub_f - query if a particular hwconfig sub-option is specified * @opt: a string representing an option * @subopt: a string representing a sub-option + * @buf: if non-NULL use this buffer to parse, otherwise try env * - * This call is similar to hwconfig(), except that it takes additional + * This call is similar to hwconfig_f(), except that it takes additional * argument @subopt. In this example: * "dr_usb:mode=peripheral" * "dr_usb" is an option, "mode" is a sub-option, and "peripheral" is its * argument. */ -int hwconfig_sub(const char *opt, const char *subopt) +int hwconfig_sub_f(const char *opt, const char *subopt, char *buf) { size_t arglen; const char *arg;
- arg = __hwconfig(opt, &arglen); + arg = __hwconfig(opt, &arglen, buf); if (!arg) return 0; return !!hwconfig_parse(arg, arglen, subopt, ",;", '=', NULL); }
/* - * hwconfig_subarg - get hwconfig sub-option's argument + * hwconfig_subarg_f - get hwconfig sub-option's argument * @opt: a string representing an option * @subopt: a string representing a sub-option * @subarglen: a pointer to an allocated size_t variable + * @buf: if non-NULL use this buffer to parse, otherwise try env * - * This call is similar to hwconfig_arg(), except that it takes an additional - * argument @subopt, and so works with sub-options. + * This call is similar to hwconfig_arg_f(), except that it takes an + * additional argument @subopt, and so works with sub-options. */ -const char *hwconfig_subarg(const char *opt, const char *subopt, - size_t *subarglen) +const char *hwconfig_subarg_f(const char *opt, const char *subopt, + size_t *subarglen, char *buf) { size_t arglen; const char *arg;
- arg = __hwconfig(opt, &arglen); + arg = __hwconfig(opt, &arglen, buf); if (!arg) return NULL; return hwconfig_parse(arg, arglen, subopt, ",;", '=', subarglen); }
/* - * hwconfig_arg_cmp - compare hwconfig sub-option's argument + * hwconfig_arg_cmp_f - compare hwconfig sub-option's argument * @opt: a string representing an option * @subopt: a string representing a sub-option * @subarg: a string for comparing an sub-option's argument + * @buf: if non-NULL use this buffer to parse, otherwise try env * - * This call is similar to hwconfig_arg_cmp, except that it takes an additional - * argument @subopt, and so works with sub-options. + * This call is similar to hwconfig_arg_cmp_f, except that it takes an + * additional argument @subopt, and so works with sub-options. */ -int hwconfig_subarg_cmp(const char *opt, const char *subopt, const char *subarg) +int hwconfig_subarg_cmp_f(const char *opt, const char *subopt, + const char *subarg, char *buf) { const char *argstr; size_t arglen;
- argstr = hwconfig_subarg(opt, subopt, &arglen); + argstr = hwconfig_subarg_f(opt, subopt, &arglen, buf); if (!argstr || arglen != strlen(subarg)) return 0;
diff --git a/include/hwconfig.h b/include/hwconfig.h index d517f78..a037ed8 100644 --- a/include/hwconfig.h +++ b/include/hwconfig.h @@ -2,6 +2,7 @@ * An inteface for configuring a hardware via u-boot environment. * * Copyright (c) 2009 MontaVista Software, Inc. + * Copyright 2011 Freescale Semiconductor, Inc. * * Author: Anton Vorontsov avorontsov@ru.mvista.com * @@ -19,51 +20,84 @@
#ifdef CONFIG_HWCONFIG
-extern int hwconfig(const char *opt); -extern const char *hwconfig_arg(const char *opt, size_t *arglen); -extern int hwconfig_arg_cmp(const char *opt, const char *arg); -extern int hwconfig_sub(const char *opt, const char *subopt); -extern const char *hwconfig_subarg(const char *opt, const char *subopt, - size_t *subarglen); -extern int hwconfig_subarg_cmp(const char *opt, const char *subopt, - const char *subarg); - +extern int hwconfig_f(const char *opt, char *buf); +extern const char *hwconfig_arg_f(const char *opt, size_t *arglen, char *buf); +extern int hwconfig_arg_cmp_f(const char *opt, const char *arg, char *buf); +extern int hwconfig_sub_f(const char *opt, const char *subopt, char *buf); +extern const char *hwconfig_subarg_f(const char *opt, const char *subopt, + size_t *subarglen, char *buf); +extern int hwconfig_subarg_cmp_f(const char *opt, const char *subopt, + const char *subarg, char *buf); #else
-static inline int hwconfig(const char *opt) +static inline int hwconfig_f(const char *opt, char *buf) { return -ENOSYS; }
-static inline const char *hwconfig_arg(const char *opt, size_t *arglen) +static inline const char *hwconfig_arg_f(const char *opt, size_t *arglen, + char *buf) { *arglen = 0; return ""; }
-static inline int hwconfig_arg_cmp(const char *opt, const char *arg) +static inline int hwconfig_arg_cmp_f(const char *opt, const char *arg, + char *buf) { return -ENOSYS; }
-static inline int hwconfig_sub(const char *opt, const char *subopt) +static inline int hwconfig_sub_f(const char *opt, const char *subopt, char *buf) { return -ENOSYS; }
-static inline const char *hwconfig_subarg(const char *opt, const char *subopt, - size_t *subarglen) +static inline const char *hwconfig_subarg_f(const char *opt, const char *subopt, + size_t *subarglen, char *buf) { *subarglen = 0; return ""; }
-static inline int hwconfig_subarg_cmp(const char *opt, const char *subopt, - const char *subarg) +static inline int hwconfig_subarg_cmp_f(const char *opt, const char *subopt, + const char *subarg, char *buf) { return -ENOSYS; }
#endif /* CONFIG_HWCONFIG */
+static inline int hwconfig(const char *opt) +{ + return hwconfig_f(opt, NULL); +} + +static inline const char *hwconfig_arg(const char *opt, size_t *arglen) +{ + return hwconfig_arg_f(opt, arglen, NULL); +} + +static inline int hwconfig_arg_cmp(const char *opt, const char *arg) +{ + return hwconfig_arg_cmp_f(opt, arg, NULL); +} + +static inline int hwconfig_sub(const char *opt, const char *subopt) +{ + return hwconfig_sub_f(opt, subopt, NULL); +} + +static inline const char *hwconfig_subarg(const char *opt, const char *subopt, + size_t *subarglen) +{ + return hwconfig_subarg_f(opt, subopt, subarglen, NULL); +} + +static inline int hwconfig_subarg_cmp(const char *opt, const char *subopt, + const char *subarg) +{ + return hwconfig_subarg_cmp_f(opt, subopt, subarg, NULL); +} + #endif /* _HWCONFIG_H */
On Jan 9, 2011, at 2:29 PM, Kumar Gala wrote:
There are several users of the hwconfig APIs (8xxx DDR) before we have the environment properly setup. This causes issues because of the numerous ways the environment might be accessed because of the non-volatile memory it might be stored in. Additionally the access might be so early that memory isn't even properly setup for us.
Towards resolving these issues we provide versions of all the hwconfig APIs that can be passed in a buffer to parse and leave it to the caller to determine how to allocate and populate the buffer.
We use the _f naming convention for these new APIs even though they are perfectly useable after relocation and the environment being ready.
We also now warn if the non-f APIs are called before the environment is ready to allow users to address the issues.
Finally, we convert the 8xxx DDR code to utilize the new APIs to hopefully address the issue once and for all. We have the 8xxx DDR code create a buffer on the stack and populate it via getenv_f().
Signed-off-by: Kumar Gala galak@kernel.crashing.org
- Add a comment about the buffer size
arch/powerpc/cpu/mpc8xxx/ddr/options.c | 78 ++++++++++++++++++++++------- common/hwconfig.c | 86 +++++++++++++++++--------------- include/hwconfig.h | 68 +++++++++++++++++++------ 3 files changed, 155 insertions(+), 77 deletions(-)
Wolfgang,
Reminder to take a look at this patch.
Its a precursor to York's DDR cleanup patches.
- k
Dear Kumar Gala,
In message 1294604972-24751-1-git-send-email-galak@kernel.crashing.org you wrote:
There are several users of the hwconfig APIs (8xxx DDR) before we have the environment properly setup. This causes issues because of the numerous ways the environment might be accessed because of the non-volatile memory it might be stored in. Additionally the access might be so early that memory isn't even properly setup for us.
Towards resolving these issues we provide versions of all the hwconfig APIs that can be passed in a buffer to parse and leave it to the caller to determine how to allocate and populate the buffer.
We use the _f naming convention for these new APIs even though they are perfectly useable after relocation and the environment being ready.
We also now warn if the non-f APIs are called before the environment is ready to allow users to address the issues.
Finally, we convert the 8xxx DDR code to utilize the new APIs to hopefully address the issue once and for all. We have the 8xxx DDR code create a buffer on the stack and populate it via getenv_f().
...
I would like to suggest a few changes.
@@ -24,6 +32,15 @@ unsigned int populate_memctl_options(int all_DIMMs_registered, unsigned int ctrl_num) { unsigned int i;
- char buffer[HWCONFIG_BUFFER_SIZE];
- char *buf = NULL;
- /*
* Extract hwconfig from environment since we have not properly setup* the environment but need it for ddr config params*/- if (getenv_f("hwconfig", buffer, sizeof(buffer)) > 0)
buf = buffer;
If we already know that there is no "hwconfig" setting in the environment, why do we then need to go through all the hwconfig_sub_f() and hwconfig_subarg_cmp_f() calls below?
Can we not short-cut all these?
#ifdef CONFIG_DDR_SPD
- char buffer[HWCONFIG_BUFFER_SIZE];
- char *buf = NULL;
- /*
* Extract hwconfig from environment since we have not properly setup* the environment but need it for ddr config params*/- if (getenv_f("hwconfig", buffer, sizeof(buffer)) > 0)
buf = buffer;
Ditto here.
diff --git a/common/hwconfig.c b/common/hwconfig.c index 193863a..f909fa5 100644 --- a/common/hwconfig.c +++ b/common/hwconfig.c
...
-static const char *__hwconfig(const char *opt, size_t *arglen) +static const char *__hwconfig(const char *opt, size_t *arglen, char *buf) { const char *env_hwconfig = NULL, *ret;
char buf[HWCONFIG_PRE_RELOC_BUF_SIZE];
if (gd->flags & GD_FLG_ENV_READY) {
env_hwconfig = getenv("hwconfig");
- /* if we are passed a buffer use it, otherwise try the environment */
- if (!buf) {
if (gd->flags & GD_FLG_ENV_READY) {env_hwconfig = getenv("hwconfig");} else {printf("WARNING: Calling __hwconfig without a buffer ""and before environment is ready\n");return NULL;}
/** Use our own on stack based buffer before relocation to allow* accessing longer hwconfig strings that might be in the* environment before we've relocated. This is pretty fragile* on both the use of stack and if the buffer is big enough.* However we will get a warning from getenv_f for the later.*/if ((getenv_f("hwconfig", buf, sizeof(buf))) > 0)env_hwconfig = buf;
env_hwconfig = buf;
Do we need "buf" at all?
Make this:
static const char *__hwconfig(const char *opt, size_t *arglen, const char *env_hwconfig) ... if (!env_hwconfig) { if (!(gd->flags & GD_FLG_ENV_READY)) { printf("WARNING: Calling __hwconfig without a buffer " "and before environment is ready\n"); return NULL; } env_hwconfig = getenv("hwconfig"); }
May I ask what is the size impact of this change (i. e. all the additional parameter passing) ?
Best regards,
Wolfgang Denk
On Jan 18, 2011, at 4:26 PM, Wolfgang Denk wrote:
Dear Kumar Gala,
In message 1294604972-24751-1-git-send-email-galak@kernel.crashing.org you wrote:
There are several users of the hwconfig APIs (8xxx DDR) before we have the environment properly setup. This causes issues because of the numerous ways the environment might be accessed because of the non-volatile memory it might be stored in. Additionally the access might be so early that memory isn't even properly setup for us.
Towards resolving these issues we provide versions of all the hwconfig APIs that can be passed in a buffer to parse and leave it to the caller to determine how to allocate and populate the buffer.
We use the _f naming convention for these new APIs even though they are perfectly useable after relocation and the environment being ready.
We also now warn if the non-f APIs are called before the environment is ready to allow users to address the issues.
Finally, we convert the 8xxx DDR code to utilize the new APIs to hopefully address the issue once and for all. We have the 8xxx DDR code create a buffer on the stack and populate it via getenv_f().
...
I would like to suggest a few changes.
@@ -24,6 +32,15 @@ unsigned int populate_memctl_options(int all_DIMMs_registered, unsigned int ctrl_num) { unsigned int i;
- char buffer[HWCONFIG_BUFFER_SIZE];
- char *buf = NULL;
- /*
* Extract hwconfig from environment since we have not properly setup* the environment but need it for ddr config params*/- if (getenv_f("hwconfig", buffer, sizeof(buffer)) > 0)
buf = buffer;If we already know that there is no "hwconfig" setting in the environment, why do we then need to go through all the hwconfig_sub_f() and hwconfig_subarg_cmp_f() calls below?
Can we not short-cut all these?
Short cut how?
#ifdef CONFIG_DDR_SPD
- char buffer[HWCONFIG_BUFFER_SIZE];
- char *buf = NULL;
- /*
* Extract hwconfig from environment since we have not properly setup* the environment but need it for ddr config params*/- if (getenv_f("hwconfig", buffer, sizeof(buffer)) > 0)
buf = buffer;Ditto here.
diff --git a/common/hwconfig.c b/common/hwconfig.c index 193863a..f909fa5 100644 --- a/common/hwconfig.c +++ b/common/hwconfig.c
...
-static const char *__hwconfig(const char *opt, size_t *arglen) +static const char *__hwconfig(const char *opt, size_t *arglen, char *buf) { const char *env_hwconfig = NULL, *ret;
char buf[HWCONFIG_PRE_RELOC_BUF_SIZE];
if (gd->flags & GD_FLG_ENV_READY) {
env_hwconfig = getenv("hwconfig");
- /* if we are passed a buffer use it, otherwise try the environment */
- if (!buf) {
if (gd->flags & GD_FLG_ENV_READY) {env_hwconfig = getenv("hwconfig");} else {printf("WARNING: Calling __hwconfig without a buffer ""and before environment is ready\n");return NULL;}
/** Use our own on stack based buffer before relocation to allow* accessing longer hwconfig strings that might be in the* environment before we've relocated. This is pretty fragile* on both the use of stack and if the buffer is big enough.* However we will get a warning from getenv_f for the later.*/if ((getenv_f("hwconfig", buf, sizeof(buf))) > 0)env_hwconfig = buf;
env_hwconfig = buf;Do we need "buf" at all?
Make this:
static const char *__hwconfig(const char *opt, size_t *arglen, const char *env_hwconfig) ... if (!env_hwconfig) { if (!(gd->flags & GD_FLG_ENV_READY)) { printf("WARNING: Calling __hwconfig without a buffer " "and before environment is ready\n"); return NULL; } env_hwconfig = getenv("hwconfig"); }
changed made.
May I ask what is the size impact of this change (i. e. all the additional parameter passing) ?
For MPC8572DS_config
text data bss dec hex filename 393964 49144 41752 484860 765fc u-boot [before] 394147 49152 41752 485051 766bb u-boot [after] -------------------------------------------------------- 183 8 191
- k
On Jan 18, 2011, at 4:40 PM, Kumar Gala wrote:
If we already know that there is no "hwconfig" setting in the environment, why do we then need to go through all the hwconfig_sub_f() and hwconfig_subarg_cmp_f() calls below?
Can we not short-cut all these?
Short cut how?
while we could clean this up, I'm leaving it alone in this patch as its not the goal of this patch to rework the world, just trying to fix hwconfig for use early.
- k
There are several users of the hwconfig APIs (8xxx DDR) before we have the environment properly setup. This causes issues because of the numerous ways the environment might be accessed because of the non-volatile memory it might be stored in. Additionally the access might be so early that memory isn't even properly setup for us.
Towards resolving these issues we provide versions of all the hwconfig APIs that can be passed in a buffer to parse and leave it to the caller to determine how to allocate and populate the buffer.
We use the _f naming convention for these new APIs even though they are perfectly useable after relocation and the environment being ready.
We also now warn if the non-f APIs are called before the environment is ready to allow users to address the issues.
Finally, we convert the 8xxx DDR code to utilize the new APIs to hopefully address the issue once and for all. We have the 8xxx DDR code create a buffer on the stack and populate it via getenv_f().
Signed-off-by: Kumar Gala galak@kernel.crashing.org --- * Reworked __hwconfig to not require an additional local var per WD's comments
arch/powerpc/cpu/mpc8xxx/ddr/options.c | 78 ++++++++++++++++++++++------- common/hwconfig.c | 86 ++++++++++++++++--------------- include/hwconfig.h | 68 +++++++++++++++++++------ 3 files changed, 154 insertions(+), 78 deletions(-)
diff --git a/arch/powerpc/cpu/mpc8xxx/ddr/options.c b/arch/powerpc/cpu/mpc8xxx/ddr/options.c index c641e85..0e7097b 100644 --- a/arch/powerpc/cpu/mpc8xxx/ddr/options.c +++ b/arch/powerpc/cpu/mpc8xxx/ddr/options.c @@ -13,6 +13,14 @@
#include "ddr.h"
+/* + * Use our own stack based buffer before relocation to allow accessing longer + * hwconfig strings that might be in the environment before we've relocated. + * This is pretty fragile on both the use of stack and if the buffer is big + * enough. However we will get a warning from getenv_f for the later. + */ +#define HWCONFIG_BUFFER_SIZE 128 + /* Board-specific functions defined in each board's ddr.c */ extern void fsl_ddr_board_options(memctl_options_t *popts, dimm_params_t *pdimm, @@ -24,6 +32,15 @@ unsigned int populate_memctl_options(int all_DIMMs_registered, unsigned int ctrl_num) { unsigned int i; + char buffer[HWCONFIG_BUFFER_SIZE]; + char *buf = NULL; + + /* + * Extract hwconfig from environment since we have not properly setup + * the environment but need it for ddr config params + */ + if (getenv_f("hwconfig", buffer, sizeof(buffer)) > 0) + buf = buffer;
/* Chip select options. */
@@ -221,7 +238,7 @@ unsigned int populate_memctl_options(int all_DIMMs_registered, * should be a subset of the requested configuration. */ #if (CONFIG_NUM_DDR_CONTROLLERS > 1) - if (hwconfig_sub("fsl_ddr", "ctlr_intlv")) { + if (hwconfig_sub_f("fsl_ddr", "ctlr_intlv", buf)) { if (pdimm[0].n_ranks == 0) { printf("There is no rank on CS0 for controller %d. Because only" " rank on CS0 and ranks chip-select interleaved with CS0" @@ -234,19 +251,25 @@ unsigned int populate_memctl_options(int all_DIMMs_registered, * test null first. if CONFIG_HWCONFIG is not defined * hwconfig_arg_cmp returns non-zero */ - if (hwconfig_subarg_cmp("fsl_ddr", "ctlr_intlv", "null")) { + if (hwconfig_subarg_cmp_f("fsl_ddr", "ctlr_intlv", + "null", buf)) { popts->memctl_interleaving = 0; debug("memory controller interleaving disabled.\n"); - } else if (hwconfig_subarg_cmp("fsl_ddr", "ctlr_intlv", "cacheline")) + } else if (hwconfig_subarg_cmp_f("fsl_ddr", + "ctlr_intlv", + "cacheline", buf)) popts->memctl_interleaving_mode = FSL_DDR_CACHE_LINE_INTERLEAVING; - else if (hwconfig_subarg_cmp("fsl_ddr", "ctlr_intlv", "page")) + else if (hwconfig_subarg_cmp_f("fsl_ddr", "ctlr_intlv", + "page", buf)) popts->memctl_interleaving_mode = FSL_DDR_PAGE_INTERLEAVING; - else if (hwconfig_subarg_cmp("fsl_ddr", "ctlr_intlv", "bank")) + else if (hwconfig_subarg_cmp_f("fsl_ddr", "ctlr_intlv", + "bank", buf)) popts->memctl_interleaving_mode = FSL_DDR_BANK_INTERLEAVING; - else if (hwconfig_subarg_cmp("fsl_ddr", "ctlr_intlv", "superbank")) + else if (hwconfig_subarg_cmp_f("fsl_ddr", "ctlr_intlv", + "superbank", buf)) popts->memctl_interleaving_mode = FSL_DDR_SUPERBANK_INTERLEAVING; else { @@ -256,19 +279,24 @@ unsigned int populate_memctl_options(int all_DIMMs_registered, } } #endif - if ((hwconfig_sub("fsl_ddr", "bank_intlv")) && + if ((hwconfig_sub_f("fsl_ddr", "bank_intlv", buf)) && (CONFIG_CHIP_SELECTS_PER_CTRL > 1)) { /* test null first. if CONFIG_HWCONFIG is not defined, - * hwconfig_arg_cmp returns non-zero */ - if (hwconfig_subarg_cmp("fsl_ddr", "bank_intlv", "null")) + * hwconfig_subarg_cmp_f returns non-zero */ + if (hwconfig_subarg_cmp_f("fsl_ddr", "bank_intlv", + "null", buf)) debug("bank interleaving disabled.\n"); - else if (hwconfig_subarg_cmp("fsl_ddr", "bank_intlv", "cs0_cs1")) + else if (hwconfig_subarg_cmp_f("fsl_ddr", "bank_intlv", + "cs0_cs1", buf)) popts->ba_intlv_ctl = FSL_DDR_CS0_CS1; - else if (hwconfig_subarg_cmp("fsl_ddr", "bank_intlv", "cs2_cs3")) + else if (hwconfig_subarg_cmp_f("fsl_ddr", "bank_intlv", + "cs2_cs3", buf)) popts->ba_intlv_ctl = FSL_DDR_CS2_CS3; - else if (hwconfig_subarg_cmp("fsl_ddr", "bank_intlv", "cs0_cs1_and_cs2_cs3")) + else if (hwconfig_subarg_cmp_f("fsl_ddr", "bank_intlv", + "cs0_cs1_and_cs2_cs3", buf)) popts->ba_intlv_ctl = FSL_DDR_CS0_CS1_AND_CS2_CS3; - else if (hwconfig_subarg_cmp("fsl_ddr", "bank_intlv", "cs0_cs1_cs2_cs3")) + else if (hwconfig_subarg_cmp_f("fsl_ddr", "bank_intlv", + "cs0_cs1_cs2_cs3", buf)) popts->ba_intlv_ctl = FSL_DDR_CS0_CS1_CS2_CS3; else printf("hwconfig has unrecognized parameter for bank_intlv.\n"); @@ -342,10 +370,11 @@ unsigned int populate_memctl_options(int all_DIMMs_registered, } }
- if (hwconfig_sub("fsl_ddr", "addr_hash")) { - if (hwconfig_subarg_cmp("fsl_ddr", "addr_hash", "null")) + if (hwconfig_sub_f("fsl_ddr", "addr_hash", buf)) { + if (hwconfig_subarg_cmp_f("fsl_ddr", "addr_hash", "null", buf)) popts->addr_hash = 0; - else if (hwconfig_subarg_cmp("fsl_ddr", "addr_hash", "true")) + else if (hwconfig_subarg_cmp_f("fsl_ddr", "addr_hash", + "true", buf)) popts->addr_hash = 1; }
@@ -393,11 +422,22 @@ int fsl_use_spd(void) int use_spd = 0;
#ifdef CONFIG_DDR_SPD + char buffer[HWCONFIG_BUFFER_SIZE]; + char *buf = NULL; + + /* + * Extract hwconfig from environment since we have not properly setup + * the environment but need it for ddr config params + */ + if (getenv_f("hwconfig", buffer, sizeof(buffer)) > 0) + buf = buffer; + /* if hwconfig is not enabled, or "sdram" is not defined, use spd */ - if (hwconfig_sub("fsl_ddr", "sdram")) { - if (hwconfig_subarg_cmp("fsl_ddr", "sdram", "spd")) + if (hwconfig_sub_f("fsl_ddr", "sdram", buf)) { + if (hwconfig_subarg_cmp_f("fsl_ddr", "sdram", "spd", buf)) use_spd = 1; - else if (hwconfig_subarg_cmp("fsl_ddr", "sdram", "fixed")) + else if (hwconfig_subarg_cmp_f("fsl_ddr", "sdram", + "fixed", buf)) use_spd = 0; else use_spd = 1; diff --git a/common/hwconfig.c b/common/hwconfig.c index 193863a..5150748 100644 --- a/common/hwconfig.c +++ b/common/hwconfig.c @@ -2,6 +2,7 @@ * An inteface for configuring a hardware via u-boot environment. * * Copyright (c) 2009 MontaVista Software, Inc. + * Copyright 2011 Freescale Semiconductor, Inc. * * Author: Anton Vorontsov avorontsov@ru.mvista.com * @@ -71,25 +72,19 @@ next: const char cpu_hwconfig[] __attribute__((weak)) = ""; const char board_hwconfig[] __attribute__((weak)) = "";
-#define HWCONFIG_PRE_RELOC_BUF_SIZE 128 - -static const char *__hwconfig(const char *opt, size_t *arglen) +static const char *__hwconfig(const char *opt, size_t *arglen, + const char *env_hwconfig) { - const char *env_hwconfig = NULL, *ret; - char buf[HWCONFIG_PRE_RELOC_BUF_SIZE]; + const char *ret;
- if (gd->flags & GD_FLG_ENV_READY) { + /* if we are passed a buffer use it, otherwise try the environment */ + if (!env_hwconfig) { + if (!(gd->flags & GD_FLG_ENV_READY)) { + printf("WARNING: Calling __hwconfig without a buffer " + "and before environment is ready\n"); + return NULL; + } env_hwconfig = getenv("hwconfig"); - } else { - /* - * Use our own on stack based buffer before relocation to allow - * accessing longer hwconfig strings that might be in the - * environment before we've relocated. This is pretty fragile - * on both the use of stack and if the buffer is big enough. - * However we will get a warning from getenv_f for the later. - */ - if ((getenv_f("hwconfig", buf, sizeof(buf))) > 0) - env_hwconfig = buf; }
if (env_hwconfig) { @@ -109,8 +104,9 @@ static const char *__hwconfig(const char *opt, size_t *arglen) }
/* - * hwconfig - query if a particular hwconfig option is specified + * hwconfig_f - query if a particular hwconfig option is specified * @opt: a string representing an option + * @buf: if non-NULL use this buffer to parse, otherwise try env * * This call can be used to find out whether U-Boot should configure * a particular hardware option. @@ -127,34 +123,36 @@ static const char *__hwconfig(const char *opt, size_t *arglen) * that the board file only calls things that are actually used, so * hwconfig() will always return true result. */ -int hwconfig(const char *opt) +int hwconfig_f(const char *opt, char *buf) { - return !!__hwconfig(opt, NULL); + return !!__hwconfig(opt, NULL, buf); }
/* - * hwconfig_arg - get hwconfig option's argument + * hwconfig_arg_f - get hwconfig option's argument * @opt: a string representing an option * @arglen: a pointer to an allocated size_t variable + * @buf: if non-NULL use this buffer to parse, otherwise try env * - * Unlike hwconfig() function, this function returns a pointer to the + * Unlike hwconfig_f() function, this function returns a pointer to the * start of the hwconfig arguments, if option is not found or it has * no specified arguments, the function returns NULL pointer. * * If CONFIG_HWCONFIG is undefined, the function returns "", and * arglen is set to 0. */ -const char *hwconfig_arg(const char *opt, size_t *arglen) +const char *hwconfig_arg_f(const char *opt, size_t *arglen, char *buf) { - return __hwconfig(opt, arglen); + return __hwconfig(opt, arglen, buf); }
/* - * hwconfig_arg_cmp - compare hwconfig option's argument + * hwconfig_arg_cmp_f - compare hwconfig option's argument * @opt: a string representing an option * @arg: a string for comparing an option's argument + * @buf: if non-NULL use this buffer to parse, otherwise try env * - * This call is similar to hwconfig_arg, but instead of returning + * This call is similar to hwconfig_arg_f, but instead of returning * hwconfig argument and its length, it is comparing it to @arg. * * Returns non-zero value if @arg matches, 0 otherwise. @@ -162,12 +160,12 @@ const char *hwconfig_arg(const char *opt, size_t *arglen) * If CONFIG_HWCONFIG is undefined, the function returns a non-zero * value, i.e. the argument matches. */ -int hwconfig_arg_cmp(const char *opt, const char *arg) +int hwconfig_arg_cmp_f(const char *opt, const char *arg, char *buf) { const char *argstr; size_t arglen;
- argstr = hwconfig_arg(opt, &arglen); + argstr = hwconfig_arg_f(opt, &arglen, buf); if (!argstr || arglen != strlen(arg)) return 0;
@@ -175,63 +173,67 @@ int hwconfig_arg_cmp(const char *opt, const char *arg) }
/* - * hwconfig_sub - query if a particular hwconfig sub-option is specified + * hwconfig_sub_f - query if a particular hwconfig sub-option is specified * @opt: a string representing an option * @subopt: a string representing a sub-option + * @buf: if non-NULL use this buffer to parse, otherwise try env * - * This call is similar to hwconfig(), except that it takes additional + * This call is similar to hwconfig_f(), except that it takes additional * argument @subopt. In this example: * "dr_usb:mode=peripheral" * "dr_usb" is an option, "mode" is a sub-option, and "peripheral" is its * argument. */ -int hwconfig_sub(const char *opt, const char *subopt) +int hwconfig_sub_f(const char *opt, const char *subopt, char *buf) { size_t arglen; const char *arg;
- arg = __hwconfig(opt, &arglen); + arg = __hwconfig(opt, &arglen, buf); if (!arg) return 0; return !!hwconfig_parse(arg, arglen, subopt, ",;", '=', NULL); }
/* - * hwconfig_subarg - get hwconfig sub-option's argument + * hwconfig_subarg_f - get hwconfig sub-option's argument * @opt: a string representing an option * @subopt: a string representing a sub-option * @subarglen: a pointer to an allocated size_t variable + * @buf: if non-NULL use this buffer to parse, otherwise try env * - * This call is similar to hwconfig_arg(), except that it takes an additional - * argument @subopt, and so works with sub-options. + * This call is similar to hwconfig_arg_f(), except that it takes an + * additional argument @subopt, and so works with sub-options. */ -const char *hwconfig_subarg(const char *opt, const char *subopt, - size_t *subarglen) +const char *hwconfig_subarg_f(const char *opt, const char *subopt, + size_t *subarglen, char *buf) { size_t arglen; const char *arg;
- arg = __hwconfig(opt, &arglen); + arg = __hwconfig(opt, &arglen, buf); if (!arg) return NULL; return hwconfig_parse(arg, arglen, subopt, ",;", '=', subarglen); }
/* - * hwconfig_arg_cmp - compare hwconfig sub-option's argument + * hwconfig_arg_cmp_f - compare hwconfig sub-option's argument * @opt: a string representing an option * @subopt: a string representing a sub-option * @subarg: a string for comparing an sub-option's argument + * @buf: if non-NULL use this buffer to parse, otherwise try env * - * This call is similar to hwconfig_arg_cmp, except that it takes an additional - * argument @subopt, and so works with sub-options. + * This call is similar to hwconfig_arg_cmp_f, except that it takes an + * additional argument @subopt, and so works with sub-options. */ -int hwconfig_subarg_cmp(const char *opt, const char *subopt, const char *subarg) +int hwconfig_subarg_cmp_f(const char *opt, const char *subopt, + const char *subarg, char *buf) { const char *argstr; size_t arglen;
- argstr = hwconfig_subarg(opt, subopt, &arglen); + argstr = hwconfig_subarg_f(opt, subopt, &arglen, buf); if (!argstr || arglen != strlen(subarg)) return 0;
diff --git a/include/hwconfig.h b/include/hwconfig.h index d517f78..a037ed8 100644 --- a/include/hwconfig.h +++ b/include/hwconfig.h @@ -2,6 +2,7 @@ * An inteface for configuring a hardware via u-boot environment. * * Copyright (c) 2009 MontaVista Software, Inc. + * Copyright 2011 Freescale Semiconductor, Inc. * * Author: Anton Vorontsov avorontsov@ru.mvista.com * @@ -19,51 +20,84 @@
#ifdef CONFIG_HWCONFIG
-extern int hwconfig(const char *opt); -extern const char *hwconfig_arg(const char *opt, size_t *arglen); -extern int hwconfig_arg_cmp(const char *opt, const char *arg); -extern int hwconfig_sub(const char *opt, const char *subopt); -extern const char *hwconfig_subarg(const char *opt, const char *subopt, - size_t *subarglen); -extern int hwconfig_subarg_cmp(const char *opt, const char *subopt, - const char *subarg); - +extern int hwconfig_f(const char *opt, char *buf); +extern const char *hwconfig_arg_f(const char *opt, size_t *arglen, char *buf); +extern int hwconfig_arg_cmp_f(const char *opt, const char *arg, char *buf); +extern int hwconfig_sub_f(const char *opt, const char *subopt, char *buf); +extern const char *hwconfig_subarg_f(const char *opt, const char *subopt, + size_t *subarglen, char *buf); +extern int hwconfig_subarg_cmp_f(const char *opt, const char *subopt, + const char *subarg, char *buf); #else
-static inline int hwconfig(const char *opt) +static inline int hwconfig_f(const char *opt, char *buf) { return -ENOSYS; }
-static inline const char *hwconfig_arg(const char *opt, size_t *arglen) +static inline const char *hwconfig_arg_f(const char *opt, size_t *arglen, + char *buf) { *arglen = 0; return ""; }
-static inline int hwconfig_arg_cmp(const char *opt, const char *arg) +static inline int hwconfig_arg_cmp_f(const char *opt, const char *arg, + char *buf) { return -ENOSYS; }
-static inline int hwconfig_sub(const char *opt, const char *subopt) +static inline int hwconfig_sub_f(const char *opt, const char *subopt, char *buf) { return -ENOSYS; }
-static inline const char *hwconfig_subarg(const char *opt, const char *subopt, - size_t *subarglen) +static inline const char *hwconfig_subarg_f(const char *opt, const char *subopt, + size_t *subarglen, char *buf) { *subarglen = 0; return ""; }
-static inline int hwconfig_subarg_cmp(const char *opt, const char *subopt, - const char *subarg) +static inline int hwconfig_subarg_cmp_f(const char *opt, const char *subopt, + const char *subarg, char *buf) { return -ENOSYS; }
#endif /* CONFIG_HWCONFIG */
+static inline int hwconfig(const char *opt) +{ + return hwconfig_f(opt, NULL); +} + +static inline const char *hwconfig_arg(const char *opt, size_t *arglen) +{ + return hwconfig_arg_f(opt, arglen, NULL); +} + +static inline int hwconfig_arg_cmp(const char *opt, const char *arg) +{ + return hwconfig_arg_cmp_f(opt, arg, NULL); +} + +static inline int hwconfig_sub(const char *opt, const char *subopt) +{ + return hwconfig_sub_f(opt, subopt, NULL); +} + +static inline const char *hwconfig_subarg(const char *opt, const char *subopt, + size_t *subarglen) +{ + return hwconfig_subarg_f(opt, subopt, subarglen, NULL); +} + +static inline int hwconfig_subarg_cmp(const char *opt, const char *subopt, + const char *subarg) +{ + return hwconfig_subarg_cmp_f(opt, subopt, subarg, NULL); +} + #endif /* _HWCONFIG_H */
Dear Kumar Gala,
In message 1295391047-10952-1-git-send-email-galak@kernel.crashing.org you wrote:
There are several users of the hwconfig APIs (8xxx DDR) before we have the environment properly setup. This causes issues because of the numerous ways the environment might be accessed because of the non-volatile memory it might be stored in. Additionally the access might be so early that memory isn't even properly setup for us.
Towards resolving these issues we provide versions of all the hwconfig APIs that can be passed in a buffer to parse and leave it to the caller to determine how to allocate and populate the buffer.
We use the _f naming convention for these new APIs even though they are perfectly useable after relocation and the environment being ready.
We also now warn if the non-f APIs are called before the environment is ready to allow users to address the issues.
Finally, we convert the 8xxx DDR code to utilize the new APIs to hopefully address the issue once and for all. We have the 8xxx DDR code create a buffer on the stack and populate it via getenv_f().
Signed-off-by: Kumar Gala galak@kernel.crashing.org
- Reworked __hwconfig to not require an additional local var per WD's comments
arch/powerpc/cpu/mpc8xxx/ddr/options.c | 78 ++++++++++++++++++++++------- common/hwconfig.c | 86 ++++++++++++++++--------------- include/hwconfig.h | 68 +++++++++++++++++++------ 3 files changed, 154 insertions(+), 78 deletions(-)
Acked-by: Wolfgang Denk wd@denx.de
Please pull through 8xxx repo.
Best regards,
Wolfgang Denk
On Jan 19, 2011, at 2:53 AM, Wolfgang Denk wrote:
Dear Kumar Gala,
In message 1295391047-10952-1-git-send-email-galak@kernel.crashing.org you wrote:
There are several users of the hwconfig APIs (8xxx DDR) before we have the environment properly setup. This causes issues because of the numerous ways the environment might be accessed because of the non-volatile memory it might be stored in. Additionally the access might be so early that memory isn't even properly setup for us.
Towards resolving these issues we provide versions of all the hwconfig APIs that can be passed in a buffer to parse and leave it to the caller to determine how to allocate and populate the buffer.
We use the _f naming convention for these new APIs even though they are perfectly useable after relocation and the environment being ready.
We also now warn if the non-f APIs are called before the environment is ready to allow users to address the issues.
Finally, we convert the 8xxx DDR code to utilize the new APIs to hopefully address the issue once and for all. We have the 8xxx DDR code create a buffer on the stack and populate it via getenv_f().
Signed-off-by: Kumar Gala galak@kernel.crashing.org
- Reworked __hwconfig to not require an additional local var per WD's comments
arch/powerpc/cpu/mpc8xxx/ddr/options.c | 78 ++++++++++++++++++++++------- common/hwconfig.c | 86 ++++++++++++++++--------------- include/hwconfig.h | 68 +++++++++++++++++++------ 3 files changed, 154 insertions(+), 78 deletions(-)
Acked-by: Wolfgang Denk wd@denx.de
Please pull through 8xxx repo.
Best regards,
Wolfgang Denk
applied to 85xx
- k
participants (2)
-
Kumar Gala -
Wolfgang Denk