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

[Wolfgang Denk]

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?


>  #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");
	}



May I ask what is the size impact of this change (i. e. all the
additional parameter passing) ?

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
The amount of time between slipping on the peel and  landing  on  the
pavement is precisely 1 bananosecond.