From mboxrd@z Thu Jan 1 00:00:00 1970 From: Hans de Goede Date: Sat, 27 Dec 2014 18:05:25 +0100 Subject: [U-Boot] [PATCH] sunxi: Fix buggy sun6i/sun8i DRAM size detection logic In-Reply-To: <1419440297-29503-1-git-send-email-siarhei.siamashka@gmail.com> References: <1419440297-29503-1-git-send-email-siarhei.siamashka@gmail.com> Message-ID: <549EE6D5.9020203@redhat.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 > #include > #include > > 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 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 > #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 */ >