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