Re: [PATCH 8/8] memtest: add rewritten memtest command

[Sascha Hauer <s.hauer@pengutronix.de>]

On Tue, Jan 15, 2013 at 02:48:50PM +0100, Alexander Aring wrote:
> Rewrite memtest command:
> 	- Skip barebox sdram regions.
> 	- Uncache unused mem regions while testing.
> 	- Add iteration parameter.
> 	- Add parameter to do only bus testing.
> 	- Check start and end addresses.
> 	- Testing all banks if no start and end
> 	  address are given.
> 
> 	- Add sha changes (thanks):
> 	- fix calculation of regions to test. When we use PAGE_ALIGN on
> 	  size, size can be to high.
> 	  - start address has to be aligned up
> 	  - end address has to be aligned down
> 	  - then size can be calculated as end - start + 1
> 	- Add ctrlc() to the longer loops
> 	- Add a progress bar to give some visual feedback that something
> 	  issues
> 	  still going on.
> 
> 	- Change to use end element instead of size in region struct.
> 	- Fix some newline issues.
> 
> 	- Add '-c' parameter if CONFIG_MMU enabled
> 	  to test with enabled cache.
> 	- Fix some size calculation.
> 	- set start address to 0xffffffff
> 
> Signed-off-by: Alexander Aring <alex.aring@gmail.com>
> ---
>  commands/Kconfig   |   9 +
>  commands/Makefile  |   1 +
>  commands/memtest.c | 698 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 708 insertions(+)
>  create mode 100644 commands/memtest.c
> 
> diff --git a/commands/Kconfig b/commands/Kconfig
> index fc0e448..f027a7e 100644
> --- a/commands/Kconfig
> +++ b/commands/Kconfig
> @@ -496,6 +496,15 @@ config CMD_NANDTEST
>  	select PARTITION_NEED_MTD
>  	prompt "nandtest"
>  
> +config CMD_MEMTEST
> +    tristate
> +    prompt "memtest"
> +	help
> +	  This command enables a memtest to test installed memory.
> +	  During this test allocated iomem regions will be skipped.
> +	  If tested architecture has MMU with PTE flags support,
> +	  caching can be set enabled or disabled.
> +
>  endmenu
>  
>  menu "video command"
> diff --git a/commands/Makefile b/commands/Makefile
> index 3145685..6b4d9cb 100644
> --- a/commands/Makefile
> +++ b/commands/Makefile
> @@ -7,6 +7,7 @@ obj-$(CONFIG_CMD_LOADY)		+= loadxy.o
>  obj-$(CONFIG_CMD_LOADS)		+= loads.o
>  obj-$(CONFIG_CMD_ECHO)		+= echo.o
>  obj-$(CONFIG_CMD_MEMORY)	+= mem.o
> +obj-$(CONFIG_CMD_MEMTEST)   += memtest.o
>  obj-$(CONFIG_CMD_EDIT)		+= edit.o
>  obj-$(CONFIG_CMD_EXEC)		+= exec.o
>  obj-$(CONFIG_CMD_SLEEP)		+= sleep.o
> diff --git a/commands/memtest.c b/commands/memtest.c
> new file mode 100644
> index 0000000..c31e553
> --- /dev/null
> +++ b/commands/memtest.c
> @@ -0,0 +1,698 @@
> +/*
> + * memtest - Perform a memory test
> + *
> + * (C) Copyright 2013
> + * Alexander Aring <a.aring@gmail.com>
> + *
> + * (C) Copyright 2000
> + * Wolfgang Denk, DENX Software Engineering, wd@denx.de.
> + *
> + * See file CREDITS for list of people who contributed to this
> + * project.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> + * MA 02111-1307 USA
> + */
> +
> +#include <common.h>
> +#include <command.h>
> +#include <types.h>
> +#include <getopt.h>
> +#include <memory.h>
> +#include <errno.h>
> +#include <progress.h>
> +#include <asm/mmu.h>
> +
> +static const vu_long bitpattern[] = {
> +	0x00000001,	/* single bit */
> +	0x00000003,	/* two adjacent bits */
> +	0x00000007,	/* three adjacent bits */
> +	0x0000000F,	/* four adjacent bits */
> +	0x00000005,	/* two non-adjacent bits */
> +	0x00000015,	/* three non-adjacent bits */
> +	0x00000055,	/* four non-adjacent bits */
> +	0xAAAAAAAA,	/* alternating 1/0 */
> +};
> +
> +/*
> + * In CONFIG_MMU we have a special c flag.
> + */
> +#ifdef CONFIG_MMU
> +static char optstr[] = "s:e:i:cb";
> +
> +/*
> + * PTE flags variables to set cached and
> + * uncached regions.
> + */
> +static uint32_t PTE_FLAGS_CACHED;
> +static uint32_t PTE_FLAGS_UNCACHED;

Please no uppercase letters for variable names.

> +#else
> +static char optstr[] = "s:e:i:b";
> +#endif
> +
> +/*
> + * Perform a memory test. The complete test
> + * loops until interrupted by ctrl-c.
> + */
> +static int mem_test(vu_long _start, vu_long _end,
> +		int bus_only)

It would be good to move this function to common/memory_test.c. This way
it could be called from C. Especially testing memory might be called
from some early small (no shell) development binaries which are running from some
internal SRAM.

> +{
> +	vu_long *start = (vu_long *)_start;
> +	/* Point the dummy to start[1] */
> +	vu_long *dummy = start+1;
> +
> +	vu_long val;
> +	vu_long readback;
> +	vu_long offset;
> +	vu_long pattern;
> +	vu_long test_offset;
> +	vu_long temp;
> +	vu_long anti_pattern;
> +	vu_long num_words;
> +
> +	int i;
> +	int ret;
> +
> +	if (!IS_ALIGNED(_end - _start + 1, sizeof(vu_long))) {
> +		printf("Testing memarea size (0x%08lx) not a multiple"
> +				"of size 0x%x, please change start or end address.",
> +				_end - _start + 1, sizeof(vu_long));
> +		return -1;
> +	}

I think this is both too restrictive and not restitrictive enough. How
about quietly aligning up the start to a multiple-of-4 boundary and the
end down to a multiple-of-4 boundary?

The above requires me to enter a address containing a lot of 'f's and it
will happily crash my system if I pass 0xa0000001 as start address.

> +
> +	num_words = (_end - _start + 1)/sizeof(vu_long);
> +
> +	printf("Starting data line test.\n");
> +
> +	/*
> +	 * Data line test: write a pattern to the first
> +	 * location, write the 1's complement to a 'parking'
> +	 * address (changes the state of the data bus so a
> +	 * floating bus doen't give a false OK), and then
> +	 * read the value back. Note that we read it back
> +	 * into a variable because the next time we read it,
> +	 * it might be right (been there, tough to explain to
> +	 * the quality guys why it prints a failure when the
> +	 * "is" and "should be" are obviously the same in the
> +	 * error message).
> +	 *
> +	 * Rather than exhaustively testing, we test some
> +	 * patterns by shifting '1' bits through a field of
> +	 * '0's and '0' bits through a field of '1's (i.e.
> +	 * pattern and ~pattern).
> +	 */
> +	for (i = 0; i < sizeof(bitpattern)/
> +			sizeof(bitpattern[0]); i++) {
> +		ret = address_in_sdram_regions((vu_long)start);

Can't you just call request_sdram_region at the beginning of this
function? Then you can be sure that you exclusivly own the region and do
not have to test for it on and on again here.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox