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

[Siarhei Siamashka]

On Wed, 24 Dec 2014 19:39:33 +0200
Siarhei Siamashka <siarhei.siamashka at gmail.com> wrote:

> On Wed, 24 Dec 2014 18:58:17 +0200
> Siarhei Siamashka <siarhei.siamashka at 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 at 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 {