[U-Boot] [PATCH] cmd_mem: Decode the mtest start and end values from fdt

[U-Boot] [PATCH] cmd_mem: Decode the mtest start and end values from fdt

[Jagannadha Sutradharudu Teki]

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(-)

diff --git a/common/cmd_mem.c b/common/cmd_mem.c
index d44aa1d..b41e182 100644
--- a/common/cmd_mem.c
+++ b/common/cmd_mem.c
@@ -642,12 +642,14 @@ static int do_mem_mtest(cmd_tbl_t *cmdtp, int flag, int argc,
 	if (argc > 1)
 		start = (ulong *)simple_strtoul(argv[1], NULL, 16);
 	else
-		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);
 
 	if (argc > 3)
 		pattern = (ulong)simple_strtoul(argv[3], NULL, 16);
diff --git a/common/main.c b/common/main.c
index e2d2e09..d6613aa 100644
--- a/common/main.c
+++ b/common/main.c
@@ -345,6 +345,21 @@ static void process_fdt_options(const void *blob)
 	if (addr)
 		setenv_addr("rootaddr", (void *)(CONFIG_SYS_TEXT_BASE + addr));
 }
+
+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);
+}
 #endif /* CONFIG_OF_CONTROL */
 
 
@@ -475,6 +490,9 @@ void main_loop (void)
 	if (fdtdec_get_config_int(gd->fdt_blob, "bootsecure", 0))
 		secure_boot_cmd(env);
 
+	/* Get the mtest start and end from fdt */
+	process_fdt_mem_options(gd->fdt_blob);
+
 #endif /* CONFIG_OF_CONTROL */
 
 	debug ("### main_loop: bootcmd=\"%s\"\n", s ? s : "<UNDEFINED>");
-- 
1.7.4

[U-Boot] [PATCH] cmd_mem: Decode the mtest start and end values from fdt

[Wolfgang Denk]

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

[U-Boot] [PATCH] cmd_mem: Decode the mtest start and end values from fdt

[Tom Rini]

On Fri, Mar 01, 2013 at 05:06:48PM +0100, Wolfgang Denk wrote:
> 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).

In addition, if we're going to start whacking around in here, can we
just make this function bail if not passed a start and end address?  I'm
a little torn since on the one hand this is a known function, but on the
other it's not something one would expect to be used outside of testing
environments.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20130301/634aaf4d/attachment.pgp>

[U-Boot] [PATCH] cmd_mem: Decode the mtest start and end values from fdt

[Simon Glass]

Hi,

On Fri, Mar 1, 2013 at 11:15 AM, Tom Rini <trini@ti.com> wrote:
> On Fri, Mar 01, 2013 at 05:06:48PM +0100, Wolfgang Denk wrote:
>> 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).
>
> In addition, if we're going to start whacking around in here, can we
> just make this function bail if not passed a start and end address?  I'm
> a little torn since on the one hand this is a known function, but on the
> other it's not something one would expect to be used outside of testing
> environments.

I am not sure that storing the memory addresses in an env variable is
all that useful, but OK. If there is an env variable feature then it
should override the FDT. So perhaps you could create a function which
returns the mtest start and end address, something like this perhaps:

/* Should this return an error? */
int memtest_get_region(ulong *start, ulong *end)
{
   *start = CONFIG_SYS_MEMTEST_START;
   *end = CONFIG_SYS_MEMTEST_END;
#ifdef CONFIG_OF_CONTROL
   fdtdec_get_config_int() with default
   (actually fdt_get_addr() might be better if enhanced to support a default)
#endif
   /* maybe read from environment var here, if CONFIG enabled for that */
   return 0;
}

Regards,
Simon

[U-Boot] [PATCH] cmd_mem: Decode the mtest start and end values from fdt

[Wolfgang Denk]

Dear Jagannadha Sutradharudu Teki,

In message <ff1257d2-b332-477f-915d-0851e5ef944f@CO9EHSMHS030.ehs.local> you wrote:
>
> I think the entire change are not good as it touches the environment concept,
> Can you suggest any better way we could do or it may not be possible/require the mem test values
> alter though devicetree.

May I ask _why_ you would want to do this in the first place?

It does not seem to make sense to me.

Please also note that "mtest" is basicly useless.  Totally useless,
to be honest.  It performs plain read / write cycles, and unless you
have very basic memory issues this will always work - if you have
such, these get detected when running get_ram_size().

"mtest" was once added as an explicit requirement of a customer to
meet some formal specification requests. It is a ZERO practical use.

If you want to do memory testing, then use the memory test functions
of the POST framework.  OK, these will not exercise any burst mode
accesses either, but they are at least a bit more useful.

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 evolution of the human race will not be accomplished in  the  ten
thousand  years  of  tame  animals,  but in the million years of wild
animals, because man is and will always be a wild animal.
                                              - Charles Galton Darwin

[U-Boot] [PATCH] cmd_mem: Decode the mtest start and end values from fdt

[Wolfgang Denk]

Dear Jagannadha Sutradharudu Teki,

In message <51f05678-4118-4be8-bb94-ad295c407602@DB3EHSMHS015.ehs.local> you wrote:
>
> I think the entire logic to add mtest address support through devicetree with the
> help of environment variables is a mess as there are some side effects (as you mentioned already).

Agreed.

> The main intention to add these address on devicetree is to make memory address configured at
> runtime instead of CONFIG_ macros.

Then just use command line arguments!

The defaults set in the CONFIG_ veriables are intended as safe
margins; they should be set such that they exclude just the exception
vector area (supposed to be located at the lower end of memory) and
the U-Boot code and data (supposed to be located at the upper end of
memory) from testing, but exclude nearly all otherwise available RAM.

> As in most of the boards CONFIG_SYS_MEMTEST_START  is depends on CONFIG_SYS_SDRAM_BASE.
> I am trying to remove the dependency instead use the devicetree nodes.

This makes no sense to me.

If CONFIG_SYS_SDRAM_BASE and the device tree settings don;t agree, you
have a serious problem anyway, and need to fix this first.

> Ok, maybe I will add my logic for decoding these address from devicetree on to common/cmd_mem.c
> instead of common/main.c, does it make sense? Help me I f am wrong.

I have to admit that I don't see a use case where this might be
needed, or even beneficial.



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
Time is a drug. Too much of it kills you.
                                      - Terry Pratchett, _Small Gods_

[U-Boot] [PATCH] Feature Removal: disable "mtest" command by default

[Wolfgang Denk]

The "mtest" command is of little practical use (if any), and
experience has shown that a large number of board configurations
define useless or even dangerous start and end addresses.  If not even
the board maintainers are able to figure out which memory range can be
reliably tested, how can we expect such from the end users?  As this
problem comes up repeatedly, we rather do not enable this command by
default, so only people who know what they are doing will be
confronted with it.

As this changes the user interface, we allow for a grace period
before this change takes effect. For now, we make "mtest"
configurable through the CONFIG_CMD_MEMTEST variable, which is defined
in include/config_cmd_default.h;  we also add an entry to
doc/feature-removal-schedule.txt which announces the removal of this
default setting in two releases from now, i. e. with v2013.07.

Signed-off-by: Wolfgang Denk <wd@denx.de>
Cc: Tom Rini <trini@ti.com>
---
 README                           |  3 +-
 common/cmd_mem.c                 |  5 +-
 doc/README.memory-test           | 98 ++++++++++++++++++++++++++++++++++++++++
 doc/feature-removal-schedule.txt | 17 +++++++
 include/config_cmd_all.h         |  3 +-
 include/config_cmd_default.h     |  3 +-
 6 files changed, 125 insertions(+), 4 deletions(-)
 create mode 100644 doc/README.memory-test

diff --git a/README b/README
index 42544ce..869694f 100644
--- a/README
+++ b/README
@@ -860,7 +860,8 @@ The following options need to be configured:
 					  (requires CONFIG_CMD_MEMORY and CONFIG_MD5)
 		CONFIG_CMD_MEMINFO	* Display detailed memory information
 		CONFIG_CMD_MEMORY	  md, mm, nm, mw, cp, cmp, crc, base,
-					  loop, loopw, mtest
+					  loop, loopw
+		CONFIG_CMD_MEMTEST	  mtest
 		CONFIG_CMD_MISC		  Misc functions like sleep etc
 		CONFIG_CMD_MMC		* MMC memory mapped support
 		CONFIG_CMD_MII		* MII utility commands
diff --git a/common/cmd_mem.c b/common/cmd_mem.c
index 042c994..53572f7 100644
--- a/common/cmd_mem.c
+++ b/common/cmd_mem.c
@@ -910,6 +910,7 @@ static ulong mem_test_quick(vu_long *buf, ulong start_addr, ulong end_addr,
 	return 0;
 }
 
+#ifdef CONFIG_CMD_MEMTEST
 /*
  * Perform a memory test. A more complete alternative test can be
  * configured using CONFIG_SYS_ALT_MEMTEST. The complete test loops until
@@ -1002,7 +1003,7 @@ static int do_mem_mtest(cmd_tbl_t *cmdtp, int flag, int argc,
 
 	return ret;	/* not reached */
 }
-
+#endif	/* CONFIG_CMD_MEMTEST */
 
 /* Modify memory.
  *
@@ -1240,11 +1241,13 @@ U_BOOT_CMD(
 );
 #endif /* CONFIG_LOOPW */
 
+#ifdef CONFIG_CMD_MEMTEST
 U_BOOT_CMD(
 	mtest,	5,	1,	do_mem_mtest,
 	"simple RAM read/write test",
 	"[start [end [pattern [iterations]]]]"
 );
+#endif	/* CONFIG_CMD_MEMTEST */
 
 #ifdef CONFIG_MX_CYCLIC
 U_BOOT_CMD(
diff --git a/doc/README.memory-test b/doc/README.memory-test
new file mode 100644
index 0000000..1e9520b
--- /dev/null
+++ b/doc/README.memory-test
@@ -0,0 +1,98 @@
+The most frequent cause of problems when porting U-Boot to new
+hardware, or when using a sloppy port on some board, is memory errors.
+In most cases these are not caused by failing hardware, but by
+incorrect initialization of the memory controller.  So it appears to
+be a good idea to always test if the memory is working correctly,
+before looking for any other potential causes of any problems.
+
+U-Boot implements 3 different approaches to perform memory tests:
+
+1. The get_ram_size() function (see "common/memsize.c").
+
+   This function is supposed to be used in each and every U-Boot port
+   determine the presence and actual size of each of the potential
+   memory banks on this piece of hardware.  The code is supposed to be
+   very fast, so running it for each reboot does not hurt.  It is a
+   little known and generally underrated fact that this code will also
+   catch 99% of hardware related (i. e. reliably reproducable) memory
+   errors.  It is strongly recommended to always use this function, in
+   each and every port of U-Boot.
+
+2. The "mtest" command.
+
+   This is probably the best known memory test utility in U-Boot.
+   Unfortunately, it is also the most problematic, and the most
+   useless one.
+
+   There are a number of serious problems with this command:
+
+   - It is terribly slow.  Running "mtest" on the whole system RAM
+     takes a _long_ time before there is any significance in the fact
+     that no errors have been found so far.
+
+   - It is difficult to configure, and to use.  And any errors here
+     will reliably crash or hang your system.  "mtest" is dump and has
+     no knowledge about memory ranges that may be in use for other
+     purposes, like exception code, U-Boot code and data, stack,
+     malloc arena, video buffer, log buffer, etc.  If you let it, it
+     will happily "test" all such areas, which of corse will cause
+     some problems.
+
+   - It is not easy to configure and use, and a large number of
+     systems are seriously misconfigured.  The original idea was to
+     test basically the whole system RAM, with only excempting the
+     areas used by U-Bot itself - on most systems these are the areas
+     used for the exception vectors (usually at the very lower end of
+     system memory) and for U-Boot (code, data, etc. - see above;
+     these are usually at the very upper end of system memory).  But
+     experience has shown that a very large number of ports use
+     pretty much bogus settings of CONFIG_SYS_MEMTEST_START and
+     CONFIG_SYS_MEMTEST_END; this results in useless tests (because
+     the ranges is too small and/or badly located) or in critical
+     failures (system crashes).
+
+   Because of these issues, the "mtest" command is considered depre-
+   cated.  It should not be enabled in most normal ports of U-Boot,
+   especially not in production.  If you really need a memory test,
+   then see 1. and 3. above resp. below.
+
+3. The most thorough memory test facility is available as part of the
+   POST (Power-On Self Test) sub-system, see "post/drivers/memory.c".
+
+   If you areally need to perform memory tests (for example, because
+   it is mandatory part of your requirement specification), then
+   enable this test which is generic and should work on all archi-
+   tectures.
+
+WARNING:
+
+It should pointed out that _all_ these memory tests have one
+fundamental, unfixable design flaw:  they are based on the assumption
+that memory errors can be found by writing to and reading from memory.
+Unfortunaltely, this is only true for the relatively harmless, usually
+static errors like shorts between data or address lines, unconnected
+pins, etc.  All the really nasty errors which will first turn your
+hair grey, only to make you tear it out later, are dynamical errors,
+which usually happen not with simple read or write cycles on the bus,
+but when performing back-to-back data transfers in burst mode.  Such
+accesses usually happen only for certain DMA operations, or for heavy
+cache use (instruction fetching, cache flushing).  So far I am not
+aware of any freely available code that implements a generic, and
+efficient, memory test like that.  The best known test case to stress
+a system like that is to boot Linux with root file system mounted over
+NFS, and then build some larger software package natively (say,
+compile a Linux kernel on the system) - this will cause enough context
+switches, network traffic (and thus DMA transfers from the network
+controller), varying RAM use, etc. to trigger any weak spots in this
+area.
+
+Note: An attempt was made once to implement such a test to catch
+memory problems on a specific board.  The code is pretty much board
+specific (for example, it includes setting specific GPIO signals to
+provide triggers for an attached logic analyzer), but you can get an
+idea how it works: see "examples/standalone/test_burst*".
+
+Note 2: Ironically enough, the "test_burst" did not catch any RAM
+errors, not a single one ever.  The problems this code was supposed
+to catch did not happen when accessing the RAM, but when reading from
+NOR flash.
diff --git a/doc/feature-removal-schedule.txt b/doc/feature-removal-schedule.txt
index e04ba2d..d9a0cc2 100644
--- a/doc/feature-removal-schedule.txt
+++ b/doc/feature-removal-schedule.txt
@@ -7,6 +7,23 @@ file.
 
 ---------------------------
 
+What:	Remove CONFIG_CMD_MEMTEST from default list
+When:	Release v2013.07
+
+Why:	The "mtest" command is of little practical use (if any), and
+	experience has shown that a large number of board configu-
+	rations define useless or even dangerous start and end
+	addresses.  If not even the board maintainers are able to
+	figure out which memory range can be reliably tested, how can
+	we expect such from the end users?  As this problem comes up
+	repeatedly, we rather do not enable this command by default,
+	so only people who know what they are doing will be confronted
+	with it.
+
+Who:	Wolfgang Denk <wd@denx.de>
+
+---------------------------
+
 What:	Users of the legacy miiphy_* code
 When:	undetermined
 
diff --git a/include/config_cmd_all.h b/include/config_cmd_all.h
index 0930781..53a2f05 100644
--- a/include/config_cmd_all.h
+++ b/include/config_cmd_all.h
@@ -57,7 +57,8 @@
 #define CONFIG_CMD_LOADB	/* loadb			*/
 #define CONFIG_CMD_LOADS	/* loads			*/
 #define CONFIG_CMD_MEMINFO	/* meminfo			*/
-#define CONFIG_CMD_MEMORY	/* md mm nm mw cp cmp crc base loop mtest */
+#define CONFIG_CMD_MEMORY	/* md mm nm mw cp cmp crc base loop */
+#define CONFIG_CMD_MEMTEST	/* mtest			*/
 #define CONFIG_CMD_MFSL		/* FSL support for Microblaze	*/
 #define CONFIG_CMD_MII		/* MII support			*/
 #define CONFIG_CMD_MISC		/* Misc functions like sleep etc*/
diff --git a/include/config_cmd_default.h b/include/config_cmd_default.h
index 6e3903c..a521103 100644
--- a/include/config_cmd_default.h
+++ b/include/config_cmd_default.h
@@ -30,7 +30,8 @@
 #endif
 #define CONFIG_CMD_LOADB	/* loadb			*/
 #define CONFIG_CMD_LOADS	/* loads			*/
-#define CONFIG_CMD_MEMORY	/* md mm nm mw cp cmp crc base loop mtest */
+#define CONFIG_CMD_MEMORY	/* md mm nm mw cp cmp crc base loop */
+#define CONFIG_CMD_MEMTEST	/* mtest			*/
 #define CONFIG_CMD_MISC		/* Misc functions like sleep etc*/
 #define CONFIG_CMD_NET		/* bootp, tftpboot, rarpboot	*/
 #define CONFIG_CMD_NFS		/* NFS support			*/
-- 
1.7.11.7

[U-Boot] [PATCH] Feature Removal: disable "mtest" command by default

[Ira W. Snyder]

On Fri, Mar 08, 2013 at 08:08:24PM +0100, Wolfgang Denk wrote:
> The "mtest" command is of little practical use (if any), and
> experience has shown that a large number of board configurations
> define useless or even dangerous start and end addresses.  If not even
> the board maintainers are able to figure out which memory range can be
> reliably tested, how can we expect such from the end users?  As this
> problem comes up repeatedly, we rather do not enable this command by
> default, so only people who know what they are doing will be
> confronted with it.
> 
> As this changes the user interface, we allow for a grace period
> before this change takes effect. For now, we make "mtest"
> configurable through the CONFIG_CMD_MEMTEST variable, which is defined
> in include/config_cmd_default.h;  we also add an entry to
> doc/feature-removal-schedule.txt which announces the removal of this
> default setting in two releases from now, i. e. with v2013.07.
> 
> Signed-off-by: Wolfgang Denk <wd@denx.de>
> Cc: Tom Rini <trini@ti.com>

Hi Wolfgang Denk,

I noticed a couple of small typos, and thought I'd point them out.

Hope it helps,
Ira

> ---
>  README                           |  3 +-
>  common/cmd_mem.c                 |  5 +-
>  doc/README.memory-test           | 98 ++++++++++++++++++++++++++++++++++++++++
>  doc/feature-removal-schedule.txt | 17 +++++++
>  include/config_cmd_all.h         |  3 +-
>  include/config_cmd_default.h     |  3 +-
>  6 files changed, 125 insertions(+), 4 deletions(-)
>  create mode 100644 doc/README.memory-test
> 
> diff --git a/README b/README
> index 42544ce..869694f 100644
> --- a/README
> +++ b/README
> @@ -860,7 +860,8 @@ The following options need to be configured:
>  					  (requires CONFIG_CMD_MEMORY and CONFIG_MD5)
>  		CONFIG_CMD_MEMINFO	* Display detailed memory information
>  		CONFIG_CMD_MEMORY	  md, mm, nm, mw, cp, cmp, crc, base,
> -					  loop, loopw, mtest
> +					  loop, loopw
> +		CONFIG_CMD_MEMTEST	  mtest
>  		CONFIG_CMD_MISC		  Misc functions like sleep etc
>  		CONFIG_CMD_MMC		* MMC memory mapped support
>  		CONFIG_CMD_MII		* MII utility commands
> diff --git a/common/cmd_mem.c b/common/cmd_mem.c
> index 042c994..53572f7 100644
> --- a/common/cmd_mem.c
> +++ b/common/cmd_mem.c
> @@ -910,6 +910,7 @@ static ulong mem_test_quick(vu_long *buf, ulong start_addr, ulong end_addr,
>  	return 0;
>  }
>  
> +#ifdef CONFIG_CMD_MEMTEST
>  /*
>   * Perform a memory test. A more complete alternative test can be
>   * configured using CONFIG_SYS_ALT_MEMTEST. The complete test loops until
> @@ -1002,7 +1003,7 @@ static int do_mem_mtest(cmd_tbl_t *cmdtp, int flag, int argc,
>  
>  	return ret;	/* not reached */
>  }
> -
> +#endif	/* CONFIG_CMD_MEMTEST */
>  
>  /* Modify memory.
>   *
> @@ -1240,11 +1241,13 @@ U_BOOT_CMD(
>  );
>  #endif /* CONFIG_LOOPW */
>  
> +#ifdef CONFIG_CMD_MEMTEST
>  U_BOOT_CMD(
>  	mtest,	5,	1,	do_mem_mtest,
>  	"simple RAM read/write test",
>  	"[start [end [pattern [iterations]]]]"
>  );
> +#endif	/* CONFIG_CMD_MEMTEST */
>  
>  #ifdef CONFIG_MX_CYCLIC
>  U_BOOT_CMD(
> diff --git a/doc/README.memory-test b/doc/README.memory-test
> new file mode 100644
> index 0000000..1e9520b
> --- /dev/null
> +++ b/doc/README.memory-test
> @@ -0,0 +1,98 @@
> +The most frequent cause of problems when porting U-Boot to new
> +hardware, or when using a sloppy port on some board, is memory errors.
> +In most cases these are not caused by failing hardware, but by
> +incorrect initialization of the memory controller.  So it appears to
> +be a good idea to always test if the memory is working correctly,
> +before looking for any other potential causes of any problems.
> +
> +U-Boot implements 3 different approaches to perform memory tests:
> +
> +1. The get_ram_size() function (see "common/memsize.c").
> +
> +   This function is supposed to be used in each and every U-Boot port
> +   determine the presence and actual size of each of the potential
> +   memory banks on this piece of hardware.  The code is supposed to be
> +   very fast, so running it for each reboot does not hurt.  It is a
> +   little known and generally underrated fact that this code will also
> +   catch 99% of hardware related (i. e. reliably reproducable) memory
> +   errors.  It is strongly recommended to always use this function, in
> +   each and every port of U-Boot.
> +
> +2. The "mtest" command.
> +
> +   This is probably the best known memory test utility in U-Boot.
> +   Unfortunately, it is also the most problematic, and the most
> +   useless one.
> +
> +   There are a number of serious problems with this command:
> +
> +   - It is terribly slow.  Running "mtest" on the whole system RAM
> +     takes a _long_ time before there is any significance in the fact
> +     that no errors have been found so far.
> +
> +   - It is difficult to configure, and to use.  And any errors here
> +     will reliably crash or hang your system.  "mtest" is dump and has
                                                             ^dumb

> +     no knowledge about memory ranges that may be in use for other
> +     purposes, like exception code, U-Boot code and data, stack,
> +     malloc arena, video buffer, log buffer, etc.  If you let it, it
> +     will happily "test" all such areas, which of corse will cause
                                                     ^course

> +     some problems.
> +
> +   - It is not easy to configure and use, and a large number of
> +     systems are seriously misconfigured.  The original idea was to
> +     test basically the whole system RAM, with only excempting the
                                                       ^exempting

> +     areas used by U-Bot itself - on most systems these are the areas
                      ^U-Boot

> +     used for the exception vectors (usually at the very lower end of
> +     system memory) and for U-Boot (code, data, etc. - see above;
> +     these are usually at the very upper end of system memory).  But
> +     experience has shown that a very large number of ports use
> +     pretty much bogus settings of CONFIG_SYS_MEMTEST_START and
> +     CONFIG_SYS_MEMTEST_END; this results in useless tests (because
> +     the ranges is too small and/or badly located) or in critical
> +     failures (system crashes).
> +
> +   Because of these issues, the "mtest" command is considered depre-
> +   cated.  It should not be enabled in most normal ports of U-Boot,
> +   especially not in production.  If you really need a memory test,
> +   then see 1. and 3. above resp. below.
> +
> +3. The most thorough memory test facility is available as part of the
> +   POST (Power-On Self Test) sub-system, see "post/drivers/memory.c".
> +
> +   If you areally need to perform memory tests (for example, because
             ^really

> +   it is mandatory part of your requirement specification), then
> +   enable this test which is generic and should work on all archi-
> +   tectures.
> +
> +WARNING:
> +
> +It should pointed out that _all_ these memory tests have one
> +fundamental, unfixable design flaw:  they are based on the assumption
> +that memory errors can be found by writing to and reading from memory.
> +Unfortunaltely, this is only true for the relatively harmless, usually
   ^Unfortunately

> +static errors like shorts between data or address lines, unconnected
> +pins, etc.  All the really nasty errors which will first turn your
> +hair grey, only to make you tear it out later, are dynamical errors,
> +which usually happen not with simple read or write cycles on the bus,
> +but when performing back-to-back data transfers in burst mode.  Such
> +accesses usually happen only for certain DMA operations, or for heavy
> +cache use (instruction fetching, cache flushing).  So far I am not
> +aware of any freely available code that implements a generic, and
> +efficient, memory test like that.  The best known test case to stress
> +a system like that is to boot Linux with root file system mounted over
> +NFS, and then build some larger software package natively (say,
> +compile a Linux kernel on the system) - this will cause enough context
> +switches, network traffic (and thus DMA transfers from the network
> +controller), varying RAM use, etc. to trigger any weak spots in this
> +area.
> +
> +Note: An attempt was made once to implement such a test to catch
> +memory problems on a specific board.  The code is pretty much board
> +specific (for example, it includes setting specific GPIO signals to
> +provide triggers for an attached logic analyzer), but you can get an
> +idea how it works: see "examples/standalone/test_burst*".
> +
> +Note 2: Ironically enough, the "test_burst" did not catch any RAM
> +errors, not a single one ever.  The problems this code was supposed
> +to catch did not happen when accessing the RAM, but when reading from
> +NOR flash.
> diff --git a/doc/feature-removal-schedule.txt b/doc/feature-removal-schedule.txt
> index e04ba2d..d9a0cc2 100644
> --- a/doc/feature-removal-schedule.txt
> +++ b/doc/feature-removal-schedule.txt
> @@ -7,6 +7,23 @@ file.
>  
>  ---------------------------
>  
> +What:	Remove CONFIG_CMD_MEMTEST from default list
> +When:	Release v2013.07
> +
> +Why:	The "mtest" command is of little practical use (if any), and
> +	experience has shown that a large number of board configu-
> +	rations define useless or even dangerous start and end
> +	addresses.  If not even the board maintainers are able to
> +	figure out which memory range can be reliably tested, how can
> +	we expect such from the end users?  As this problem comes up
> +	repeatedly, we rather do not enable this command by default,
> +	so only people who know what they are doing will be confronted
> +	with it.
> +
> +Who:	Wolfgang Denk <wd@denx.de>
> +
> +---------------------------
> +
>  What:	Users of the legacy miiphy_* code
>  When:	undetermined
>  
> diff --git a/include/config_cmd_all.h b/include/config_cmd_all.h
> index 0930781..53a2f05 100644
> --- a/include/config_cmd_all.h
> +++ b/include/config_cmd_all.h
> @@ -57,7 +57,8 @@
>  #define CONFIG_CMD_LOADB	/* loadb			*/
>  #define CONFIG_CMD_LOADS	/* loads			*/
>  #define CONFIG_CMD_MEMINFO	/* meminfo			*/
> -#define CONFIG_CMD_MEMORY	/* md mm nm mw cp cmp crc base loop mtest */
> +#define CONFIG_CMD_MEMORY	/* md mm nm mw cp cmp crc base loop */
> +#define CONFIG_CMD_MEMTEST	/* mtest			*/
>  #define CONFIG_CMD_MFSL		/* FSL support for Microblaze	*/
>  #define CONFIG_CMD_MII		/* MII support			*/
>  #define CONFIG_CMD_MISC		/* Misc functions like sleep etc*/
> diff --git a/include/config_cmd_default.h b/include/config_cmd_default.h
> index 6e3903c..a521103 100644
> --- a/include/config_cmd_default.h
> +++ b/include/config_cmd_default.h
> @@ -30,7 +30,8 @@
>  #endif
>  #define CONFIG_CMD_LOADB	/* loadb			*/
>  #define CONFIG_CMD_LOADS	/* loads			*/
> -#define CONFIG_CMD_MEMORY	/* md mm nm mw cp cmp crc base loop mtest */
> +#define CONFIG_CMD_MEMORY	/* md mm nm mw cp cmp crc base loop */
> +#define CONFIG_CMD_MEMTEST	/* mtest			*/
>  #define CONFIG_CMD_MISC		/* Misc functions like sleep etc*/
>  #define CONFIG_CMD_NET		/* bootp, tftpboot, rarpboot	*/
>  #define CONFIG_CMD_NFS		/* NFS support			*/
> -- 
> 1.7.11.7
> 
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH] Feature Removal: disable "mtest" command by default

[Wolfgang Denk]

Dear Ira,

In message <20130308194131.GE23323@ovro.caltech.edu> you wrote:
>
> I noticed a couple of small typos, and thought I'd point them out.

Thanks a lot, highly appreciated!

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
Madness takes its toll. Please have exact change.

[U-Boot] [PATCH v2] Feature Removal: disable "mtest" command by default

[Wolfgang Denk]

The "mtest" command is of little practical use (if any), and
experience has shown that a large number of board configurations
define useless or even dangerous start and end addresses.  If not even
the board maintainers are able to figure out which memory range can be
reliably tested, how can we expect such from the end users?  As this
problem comes up repeatedly, we rather do not enable this command by
default, so only people who know what they are doing will be
confronted with it.

As this changes the user interface, we allow for a grace period
before this change takes effect. For now, we make "mtest"
configurable through the CONFIG_CMD_MEMTEST variable, which is defined
in include/config_cmd_default.h;  we also add an entry to
doc/feature-removal-schedule.txt which announces the removal of this
default setting in two releases from now, i. e. with v2013.07.

Signed-off-by: Wolfgang Denk <wd@denx.de>
Cc: Tom Rini <trini@ti.com>
---
v2: - Fix spelling errors in doc/README.memory-test;
      kudos to Ira W. Snyder

 README                           |  3 +-
 common/cmd_mem.c                 |  5 +-
 doc/README.memory-test           | 98 ++++++++++++++++++++++++++++++++++++++++
 doc/feature-removal-schedule.txt | 17 +++++++
 include/config_cmd_all.h         |  3 +-
 include/config_cmd_default.h     |  3 +-
 6 files changed, 125 insertions(+), 4 deletions(-)
 create mode 100644 doc/README.memory-test

diff --git a/README b/README
index 42544ce..869694f 100644
--- a/README
+++ b/README
@@ -860,7 +860,8 @@ The following options need to be configured:
 					  (requires CONFIG_CMD_MEMORY and CONFIG_MD5)
 		CONFIG_CMD_MEMINFO	* Display detailed memory information
 		CONFIG_CMD_MEMORY	  md, mm, nm, mw, cp, cmp, crc, base,
-					  loop, loopw, mtest
+					  loop, loopw
+		CONFIG_CMD_MEMTEST	  mtest
 		CONFIG_CMD_MISC		  Misc functions like sleep etc
 		CONFIG_CMD_MMC		* MMC memory mapped support
 		CONFIG_CMD_MII		* MII utility commands
diff --git a/common/cmd_mem.c b/common/cmd_mem.c
index 042c994..53572f7 100644
--- a/common/cmd_mem.c
+++ b/common/cmd_mem.c
@@ -910,6 +910,7 @@ static ulong mem_test_quick(vu_long *buf, ulong start_addr, ulong end_addr,
 	return 0;
 }
 
+#ifdef CONFIG_CMD_MEMTEST
 /*
  * Perform a memory test. A more complete alternative test can be
  * configured using CONFIG_SYS_ALT_MEMTEST. The complete test loops until
@@ -1002,7 +1003,7 @@ static int do_mem_mtest(cmd_tbl_t *cmdtp, int flag, int argc,
 
 	return ret;	/* not reached */
 }
-
+#endif	/* CONFIG_CMD_MEMTEST */
 
 /* Modify memory.
  *
@@ -1240,11 +1241,13 @@ U_BOOT_CMD(
 );
 #endif /* CONFIG_LOOPW */
 
+#ifdef CONFIG_CMD_MEMTEST
 U_BOOT_CMD(
 	mtest,	5,	1,	do_mem_mtest,
 	"simple RAM read/write test",
 	"[start [end [pattern [iterations]]]]"
 );
+#endif	/* CONFIG_CMD_MEMTEST */
 
 #ifdef CONFIG_MX_CYCLIC
 U_BOOT_CMD(
diff --git a/doc/README.memory-test b/doc/README.memory-test
new file mode 100644
index 0000000..eb60e8d
--- /dev/null
+++ b/doc/README.memory-test
@@ -0,0 +1,98 @@
+The most frequent cause of problems when porting U-Boot to new
+hardware, or when using a sloppy port on some board, is memory errors.
+In most cases these are not caused by failing hardware, but by
+incorrect initialization of the memory controller.  So it appears to
+be a good idea to always test if the memory is working correctly,
+before looking for any other potential causes of any problems.
+
+U-Boot implements 3 different approaches to perform memory tests:
+
+1. The get_ram_size() function (see "common/memsize.c").
+
+   This function is supposed to be used in each and every U-Boot port
+   determine the presence and actual size of each of the potential
+   memory banks on this piece of hardware.  The code is supposed to be
+   very fast, so running it for each reboot does not hurt.  It is a
+   little known and generally underrated fact that this code will also
+   catch 99% of hardware related (i. e. reliably reproducible) memory
+   errors.  It is strongly recommended to always use this function, in
+   each and every port of U-Boot.
+
+2. The "mtest" command.
+
+   This is probably the best known memory test utility in U-Boot.
+   Unfortunately, it is also the most problematic, and the most
+   useless one.
+
+   There are a number of serious problems with this command:
+
+   - It is terribly slow.  Running "mtest" on the whole system RAM
+     takes a _long_ time before there is any significance in the fact
+     that no errors have been found so far.
+
+   - It is difficult to configure, and to use.  And any errors here
+     will reliably crash or hang your system.  "mtest" is dumb and has
+     no knowledge about memory ranges that may be in use for other
+     purposes, like exception code, U-Boot code and data, stack,
+     malloc arena, video buffer, log buffer, etc.  If you let it, it
+     will happily "test" all such areas, which of course will cause
+     some problems.
+
+   - It is not easy to configure and use, and a large number of
+     systems are seriously misconfigured.  The original idea was to
+     test basically the whole system RAM, with only exempting the
+     areas used by U-Boot itself - on most systems these are the areas
+     used for the exception vectors (usually at the very lower end of
+     system memory) and for U-Boot (code, data, etc. - see above;
+     these are usually at the very upper end of system memory).  But
+     experience has shown that a very large number of ports use
+     pretty much bogus settings of CONFIG_SYS_MEMTEST_START and
+     CONFIG_SYS_MEMTEST_END; this results in useless tests (because
+     the ranges is too small and/or badly located) or in critical
+     failures (system crashes).
+
+   Because of these issues, the "mtest" command is considered depre-
+   cated.  It should not be enabled in most normal ports of U-Boot,
+   especially not in production.  If you really need a memory test,
+   then see 1. and 3. above resp. below.
+
+3. The most thorough memory test facility is available as part of the
+   POST (Power-On Self Test) sub-system, see "post/drivers/memory.c".
+
+   If you really need to perform memory tests (for example, because
+   it is mandatory part of your requirement specification), then
+   enable this test which is generic and should work on all archi-
+   tectures.
+
+WARNING:
+
+It should pointed out that _all_ these memory tests have one
+fundamental, unfixable design flaw:  they are based on the assumption
+that memory errors can be found by writing to and reading from memory.
+Unfortunately, this is only true for the relatively harmless, usually
+static errors like shorts between data or address lines, unconnected
+pins, etc.  All the really nasty errors which will first turn your
+hair gray, only to make you tear it out later, are dynamical errors,
+which usually happen not with simple read or write cycles on the bus,
+but when performing back-to-back data transfers in burst mode.  Such
+accesses usually happen only for certain DMA operations, or for heavy
+cache use (instruction fetching, cache flushing).  So far I am not
+aware of any freely available code that implements a generic, and
+efficient, memory test like that.  The best known test case to stress
+a system like that is to boot Linux with root file system mounted over
+NFS, and then build some larger software package natively (say,
+compile a Linux kernel on the system) - this will cause enough context
+switches, network traffic (and thus DMA transfers from the network
+controller), varying RAM use, etc. to trigger any weak spots in this
+area.
+
+Note: An attempt was made once to implement such a test to catch
+memory problems on a specific board.  The code is pretty much board
+specific (for example, it includes setting specific GPIO signals to
+provide triggers for an attached logic analyzer), but you can get an
+idea how it works: see "examples/standalone/test_burst*".
+
+Note 2: Ironically enough, the "test_burst" did not catch any RAM
+errors, not a single one ever.  The problems this code was supposed
+to catch did not happen when accessing the RAM, but when reading from
+NOR flash.
diff --git a/doc/feature-removal-schedule.txt b/doc/feature-removal-schedule.txt
index e04ba2d..d9a0cc2 100644
--- a/doc/feature-removal-schedule.txt
+++ b/doc/feature-removal-schedule.txt
@@ -7,6 +7,23 @@ file.
 
 ---------------------------
 
+What:	Remove CONFIG_CMD_MEMTEST from default list
+When:	Release v2013.07
+
+Why:	The "mtest" command is of little practical use (if any), and
+	experience has shown that a large number of board configu-
+	rations define useless or even dangerous start and end
+	addresses.  If not even the board maintainers are able to
+	figure out which memory range can be reliably tested, how can
+	we expect such from the end users?  As this problem comes up
+	repeatedly, we rather do not enable this command by default,
+	so only people who know what they are doing will be confronted
+	with it.
+
+Who:	Wolfgang Denk <wd@denx.de>
+
+---------------------------
+
 What:	Users of the legacy miiphy_* code
 When:	undetermined
 
diff --git a/include/config_cmd_all.h b/include/config_cmd_all.h
index 0930781..53a2f05 100644
--- a/include/config_cmd_all.h
+++ b/include/config_cmd_all.h
@@ -57,7 +57,8 @@
 #define CONFIG_CMD_LOADB	/* loadb			*/
 #define CONFIG_CMD_LOADS	/* loads			*/
 #define CONFIG_CMD_MEMINFO	/* meminfo			*/
-#define CONFIG_CMD_MEMORY	/* md mm nm mw cp cmp crc base loop mtest */
+#define CONFIG_CMD_MEMORY	/* md mm nm mw cp cmp crc base loop */
+#define CONFIG_CMD_MEMTEST	/* mtest			*/
 #define CONFIG_CMD_MFSL		/* FSL support for Microblaze	*/
 #define CONFIG_CMD_MII		/* MII support			*/
 #define CONFIG_CMD_MISC		/* Misc functions like sleep etc*/
diff --git a/include/config_cmd_default.h b/include/config_cmd_default.h
index 6e3903c..a521103 100644
--- a/include/config_cmd_default.h
+++ b/include/config_cmd_default.h
@@ -30,7 +30,8 @@
 #endif
 #define CONFIG_CMD_LOADB	/* loadb			*/
 #define CONFIG_CMD_LOADS	/* loads			*/
-#define CONFIG_CMD_MEMORY	/* md mm nm mw cp cmp crc base loop mtest */
+#define CONFIG_CMD_MEMORY	/* md mm nm mw cp cmp crc base loop */
+#define CONFIG_CMD_MEMTEST	/* mtest			*/
 #define CONFIG_CMD_MISC		/* Misc functions like sleep etc*/
 #define CONFIG_CMD_NET		/* bootp, tftpboot, rarpboot	*/
 #define CONFIG_CMD_NFS		/* NFS support			*/
-- 
1.7.11.7

[U-Boot] [PATCH v2] Feature Removal: disable "mtest" command by default

[Tom Rini]

On Fri, Mar 08, 2013 at 09:51:32PM +0100, Wolfgang Denk wrote:

> The "mtest" command is of little practical use (if any), and
> experience has shown that a large number of board configurations
> define useless or even dangerous start and end addresses.  If not even
> the board maintainers are able to figure out which memory range can be
> reliably tested, how can we expect such from the end users?  As this
> problem comes up repeatedly, we rather do not enable this command by
> default, so only people who know what they are doing will be
> confronted with it.
> 
> As this changes the user interface, we allow for a grace period
> before this change takes effect. For now, we make "mtest"
> configurable through the CONFIG_CMD_MEMTEST variable, which is defined
> in include/config_cmd_default.h;  we also add an entry to
> doc/feature-removal-schedule.txt which announces the removal of this
> default setting in two releases from now, i. e. with v2013.07.
> 
> Signed-off-by: Wolfgang Denk <wd@denx.de>
> Cc: Tom Rini <trini@ti.com>
> ---
> v2: - Fix spelling errors in doc/README.memory-test;
>       kudos to Ira W. Snyder

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20130311/a7e6e59b/attachment.pgp>