Dear Jagannadha Sutradharudu Teki,
In message <10597224-d520-4a3f-8185-5de018ee5...@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 <jaga...@xilinx.com>
> Tested-by: Jagannadha Sutradharudu Teki <jaga...@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: w...@denx.de
Irony is the gaiety of reflection and the joy of wisdom.
- Anatole France
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
