On Wed, 24 Dec 2014 19:39:33 +0200 Siarhei Siamashka siarhei.siamashka@gmail.com wrote:
On Wed, 24 Dec 2014 18:58:17 +0200 Siarhei Siamashka siarhei.siamashka@gmail.com 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@gmail.com
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.
Got a serial console on my tablet via a MicroSD breakout board. So I'm retracting this statement :-)
And indeed, the DRAM parameters get incorrectly detected after running the test program (the system fails to boot later on):
U-Boot 2015.01-rc3-02809-g02f4a69-dirty (Dec 25 2014 - 03:05:03) Allwinner Technology
CPU: Allwinner A31s (SUN6I) I2C: ready DRAM: 248 MiB Using default environment
It might be a good idea to backup/restore the data in RAM when doing this check in the code.
BTW, I only mentioned this because the 'get_ram_size' function restores memory to the original state after it has done the job. But if being non-destructive is not a requirement for the 'mctl_mem_matches' function, then there is no need to care.
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.
Just noticed that there is actually '__ilog2' function in u-boot. This makes it easier to switch the sun6i/sun8i dram code to using the standard 'get_ram_size' function.
With the use of "__ilog2(get_ram_size(...))", the DRAM parameters detection may look like the piece of code below. But not sure if this is actually any better than the use of 'mctl_mem_matches' at least on sun6i hardware. Still on sun8i it fits quite fine.
diff --git a/arch/arm/cpu/armv7/sunxi/dram_sun6i.c b/arch/arm/cpu/armv7/sunxi/dram_sun6i.c index 4675c48..139944d 100644 --- a/arch/arm/cpu/armv7/sunxi/dram_sun6i.c +++ b/arch/arm/cpu/armv7/sunxi/dram_sun6i.c @@ -332,6 +332,7 @@ unsigned long sunxi_dram_init(void) (struct sunxi_mctl_com_reg *)SUNXI_DRAM_COM_BASE; u32 offset; int bank, bus, columns; + int n_col, n_row, n_bnk;
/* Set initial parameters, these get modified by the autodetect code */ struct dram_sun6i_para para = { @@ -384,6 +385,10 @@ unsigned long sunxi_dram_init(void) } bus = (para.bus_width == 32) ? 2 : 1; columns -= bus; + + n_col = __ilog2(get_ram_size((long *)PHYS_SDRAM_0, PHYS_SDRAM_0_SIZE)) - + bus; + para.page_size = (1 << columns) * (bus << 1); clrsetbits_le32(&mctl_com->cr, MCTL_CR_PAGE_SIZE_MASK, MCTL_CR_PAGE_SIZE(para.page_size)); @@ -394,6 +399,10 @@ unsigned long sunxi_dram_init(void) if (mctl_mem_matches(offset)) break; } + + n_row = __ilog2(get_ram_size((long *)PHYS_SDRAM_0, PHYS_SDRAM_0_SIZE)) - + (columns + bus); + clrsetbits_le32(&mctl_com->cr, MCTL_CR_ROW_MASK, MCTL_CR_ROW(para.rows));
@@ -401,6 +410,12 @@ unsigned long sunxi_dram_init(void) offset = 1 << (para.rows + columns + bus + 2); bank = mctl_mem_matches(offset) ? 0 : 1;
+ n_bnk = __ilog2(get_ram_size((long *)PHYS_SDRAM_0, PHYS_SDRAM_0_SIZE)) - + (para.rows + columns + bus + 2); + + printf("old: columns=%d, rows=%d, bank=%d\n", columns, para.rows, bank); + printf("new: columns=%d, rows=%d, bank=%d\n", n_col, n_row, n_bnk); + /* Restore interleave, chan and rank values, set bank size */ clrsetbits_le32(&mctl_com->cr, MCTL_CR_CHANNEL_MASK | MCTL_CR_SEQUENCE | diff --git a/arch/arm/cpu/armv7/sunxi/dram_sun8i.c b/arch/arm/cpu/armv7/sunxi/dram_sun8i.c index df9ff1f..416ef8a 100644 --- a/arch/arm/cpu/armv7/sunxi/dram_sun8i.c +++ b/arch/arm/cpu/armv7/sunxi/dram_sun8i.c @@ -272,6 +272,7 @@ unsigned long sunxi_dram_init(void) (struct sunxi_mctl_com_reg *)SUNXI_DRAM_COM_BASE; const u32 columns = 13; u32 bus, bus_width, offset, page_size, rows; + u32 n_row;
mctl_sys_init(); mctl_init(&bus_width); @@ -295,6 +296,14 @@ unsigned long sunxi_dram_init(void) if (mctl_mem_matches(offset)) break; } + + n_row = __ilog2(get_ram_size((long *)PHYS_SDRAM_0, + PHYS_SDRAM_0_SIZE)) - + (columns + bus); + + printf("old: rows=%d\n", rows); + printf("new: rows=%d\n", n_row); + clrsetbits_le32(&mctl_com->cr, MCTL_CR_ROW_MASK, MCTL_CR_ROW(rows)); } else {