[U-Boot] [PATCH] sunxi: Fix buggy sun6i/sun8i DRAM size detection logic

[Hans de Goede]

Hi,

On 24-12-14 17:58, Siarhei Siamashka wrote:
> After reboot, reset or even short power off, DRAM typically retains
> the old stale data for some period of time (for this type of memory,
> the bits of data are stored in slowly discharging capacitors).
>
> The current sun6i/sun8i DRAM size detection logic, which is
> inherited from the Allwinner code, relies on using a large magic
> signature with the hope that it is unique enough and unlikely to
> ever accidentally match this leftover garbage data in RAM. But
> this approach is inherently unsafe, as can be demonstrated using
> the following test program:
>
> /***** A testcase for reproducing the problem ******/
> #include <stdlib.h>
> #include <stdio.h>
> #include <stdint.h>
>
> void main(int argc, char *argv[])
> {
>      size_t size, i;
>      uint32_t *buf;
>      /* Allocate the buffer */
>      if (argc < 2 || !(size = (size_t)atoi(argv[1]) * 1048576) ||
>                      !(buf = malloc(size))) {
>          printf("Need buffer size in MiB as a cmdline argument\n");
>          exit(1);
>      }
>      /* Fill it with the Allwinner DRAM "magic" values */
>      for (i = 0; i < size / 4; i++)
>          buf[i] = 0xaa55aa55 + ((uintptr_t)&buf[i] / 4) % 64;
>      /* Try to reboot */
>      system("reboot");
>      /* And wait */
>      for (;;) {}
> }
> /***************************************************/
>
> If this test program is run on the device (giving it a large
> chunk of memory), then the DRAM size detection logic in u-boot
> gets confused after reboot and fails to initialize DRAM properly.
>
> A better approach is not to rely on luck and abstain from making
> any assumptions about the properties of the leftover garbage
> data in RAM. Instead just use a more reliable code for testing
> whether two different addresses refer to the same memory location.
>
> Signed-off-by: Siarhei Siamashka <siarhei.siamashka at gmail.com>

Thanks for the patch. I've run various tests and it works well,
queued up for merging in u-boot-sunxi/next .

Regards,

Hans

> ---
>
> Done only very limited testing on MSI Primo81 tablet (Allwinner A31s),
> which is currently a rather impractical device for doing any sun6i code
> development due to having no access to the serial console, USB or other
> convenient ways to interact with the device.
>
> It might be a good idea to backup/restore the data in RAM when doing
> this check in the code.
>
> Using the standard u-boot 'get_ram_size' function could be also an
> option to replace the loops and simplify the sun6i/sun8i dram code
> in the future. The only inconvenience is that 'get_ram_size' returns
> 'size' instead of 'log2(size)'. This could be probably resolved by
> introducing a new 'get_ram_size_log2' common function. But since
> I don't have any Allwinner A23/A31 devboard for testing/debugging,
> don't expect me to do this work any time soon, or ever at all.
>
>   arch/arm/cpu/armv7/sunxi/dram_sun6i.c  |  1 -
>   arch/arm/cpu/armv7/sunxi/dram_sun8i.c  |  1 -
>   arch/arm/include/asm/arch-sunxi/dram.h | 22 ++++++----------------
>   3 files changed, 6 insertions(+), 18 deletions(-)
>
> diff --git a/arch/arm/cpu/armv7/sunxi/dram_sun6i.c b/arch/arm/cpu/armv7/sunxi/dram_sun6i.c
> index 4675c48..4518b80 100644
> --- a/arch/arm/cpu/armv7/sunxi/dram_sun6i.c
> +++ b/arch/arm/cpu/armv7/sunxi/dram_sun6i.c
> @@ -377,7 +377,6 @@ unsigned long sunxi_dram_init(void)
>   		    MCTL_CR_BANK(1) | MCTL_CR_RANK(1));
>
>   	/* Detect and set page size */
> -	mctl_mem_fill();
>   	for (columns = 7; columns < 20; columns++) {
>   		if (mctl_mem_matches(1 << columns))
>   			break;
> diff --git a/arch/arm/cpu/armv7/sunxi/dram_sun8i.c b/arch/arm/cpu/armv7/sunxi/dram_sun8i.c
> index df9ff1f..ebba53b 100644
> --- a/arch/arm/cpu/armv7/sunxi/dram_sun8i.c
> +++ b/arch/arm/cpu/armv7/sunxi/dram_sun8i.c
> @@ -289,7 +289,6 @@ unsigned long sunxi_dram_init(void)
>   		writel(0x000310f4 | MCTL_CR_PAGE_SIZE(page_size),
>   		       &mctl_com->cr);
>   		setbits_le32(&mctl_com->swonr, 0x0003ffff);
> -		mctl_mem_fill();
>   		for (rows = 11; rows < 16; rows++) {
>   			offset = 1 << (rows + columns + bus);
>   			if (mctl_mem_matches(offset))
> diff --git a/arch/arm/include/asm/arch-sunxi/dram.h b/arch/arm/include/asm/arch-sunxi/dram.h
> index 8d78029..7ff43e6 100644
> --- a/arch/arm/include/asm/arch-sunxi/dram.h
> +++ b/arch/arm/include/asm/arch-sunxi/dram.h
> @@ -24,8 +24,6 @@
>   #include <asm/arch/dram_sun4i.h>
>   #endif
>
> -#define MCTL_MEM_FILL_MATCH_COUNT 64
> -
>   unsigned long sunxi_dram_init(void);
>
>   /*
> @@ -42,24 +40,16 @@ static inline void mctl_await_completion(u32 *reg, u32 mask, u32 val)
>   }
>
>   /*
> - * Fill beginning of DRAM with "random" data for mctl_mem_matches()
> - */
> -static inline void mctl_mem_fill(void)
> -{
> -	int i;
> -
> -	for (i = 0; i < MCTL_MEM_FILL_MATCH_COUNT; i++)
> -		writel(0xaa55aa55 + i, CONFIG_SYS_SDRAM_BASE + i * 4);
> -}
> -
> -/*
>    * Test if memory at offset offset matches memory at begin of DRAM
>    */
>   static inline bool mctl_mem_matches(u32 offset)
>   {
> -	return memcmp((u32 *)CONFIG_SYS_SDRAM_BASE,
> -		      (u32 *)(CONFIG_SYS_SDRAM_BASE + offset),
> -		      MCTL_MEM_FILL_MATCH_COUNT * 4) == 0;
> +	/* Try to write different values to RAM at two addresses */
> +	writel(0, CONFIG_SYS_SDRAM_BASE);
> +	writel(0xaa55aa55, CONFIG_SYS_SDRAM_BASE + offset);
> +	/* Check if the same value is actually observed when reading back */
> +	return readl(CONFIG_SYS_SDRAM_BASE) ==
> +	       readl(CONFIG_SYS_SDRAM_BASE + offset);
>   }
>
>   #endif /* _SUNXI_DRAM_H */
>