[U-Boot] [PATCH v2 2/2] powerpc/8xxx: Add new hwconfig APIs to address early parsing used by DDR init

[Kumar Gala]

On Jan 18, 2011, at 4:26 PM, Wolfgang Denk wrote:

> Dear Kumar Gala,
> 
> In message <1294604972-24751-1-git-send-email-galak at 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;
>> +		}
>> 	} 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;
>> 	}
> 
> 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