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