From mboxrd@z Thu Jan  1 00:00:00 1970
From: Wolfgang Denk <wd@denx.de>
Date: Fri, 01 Mar 2013 17:06:48 +0100
Subject: [U-Boot] [PATCH] cmd_mem: Decode the mtest start and end values
	from fdt
In-Reply-To: <10597224-d520-4a3f-8185-5de018ee5046@TX2EHSMHS026.ehs.local>
References: <10597224-d520-4a3f-8185-5de018ee5046@TX2EHSMHS026.ehs.local>
Message-ID: <20130301160648.CD661200527@gemini.denx.de>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Dear Jagannadha Sutradharudu Teki,

In message <10597224-d520-4a3f-8185-5de018ee5046@TX2EHSMHS026.ehs.local> you wrote:
> This patch provides a support to decode the mtest start
> and end values from fdt config node.
> 
> Signed-off-by: Jagannadha Sutradharudu Teki <jaganna@xilinx.com>
> Tested-by: Jagannadha Sutradharudu Teki <jaganna@xilinx.com>
> ---
>  common/cmd_mem.c |    6 ++++--
>  common/main.c    |   18 ++++++++++++++++++
>  2 files changed, 22 insertions(+), 2 deletions(-)

You are adding code here which may bot be user and/or wanted by the
majority of boards, so please make it configureable (and document the
new config option).

Also, your patchj is actually doing more than you describe in the
subject:


> -		start = (ulong *)CONFIG_SYS_MEMTEST_START;
> +		start = (ulong *)getenv_ulong("mteststart", 16,
> +				CONFIG_SYS_MEMTEST_START);
>  
>  	if (argc > 2)
>  		end = (ulong *)simple_strtoul(argv[2], NULL, 16);
>  	else
> -		end = (ulong *)(CONFIG_SYS_MEMTEST_END);
> +		end = (ulong *)getenv_ulong("mtestend", 16,
> +				CONFIG_SYS_MEMTEST_END);

Here you are switching to use environment variablkes, which is NOT
mentioned in the commit message.  Actually this should be in a
separate patch, and it also should be configurable.

> +static void process_fdt_mem_options(const void *blob)
> +{
> +	ulong addr;
> +
> +	/* Add an env variable to point to a mtest start, if available */
> +	addr = fdtdec_get_config_int(gd->fdt_blob, "mtest-start", 0);
> +	if (addr)
> +		setenv_addr("mteststart", (void *)addr);
> +
> +	/* Add an env variable to point to a mtest end, if available */
> +	addr = fdtdec_get_config_int(gd->fdt_blob, "mtest-end", 0);
> +	if (addr)
> +		setenv_addr("mtestend", (void *)addr);
> +}

NAK.  It is always wrong to mandatorily overwrite environment variable
that may have been set by the user.

If you really want to use enviuronment variables here (which I don't
think is a clever thing to do), then you need to be careful about
priorities: any user settings always have highest priority.

Note that you will shoot yourself in the foot because after a
"saveenv" any new, different values from a new device tree would be
ignored unless you manually delete these settings first.

As mentioned, this is not a good idea.

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
Irony is the gaiety of reflection and the joy of wisdom.
                                                     - Anatole France