From mboxrd@z Thu Jan 1 00:00:00 1970 From: Hans de Goede Date: Thu, 25 Dec 2014 11:14:23 +0100 Subject: [U-Boot] [PATCH] sunxi: Fix buggy sun6i/sun8i DRAM size detection logic In-Reply-To: <20141225040753.3c4df3b6@i7> References: <1419440297-29503-1-git-send-email-siarhei.siamashka@gmail.com> <20141224193933.49fe67d4@i7> <20141225040753.3c4df3b6@i7> Message-ID: <549BE37F.1020004@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 25-12-14 03:07, Siarhei Siamashka wrote: > On Wed, 24 Dec 2014 19:39:33 +0200 > Siarhei Siamashka wrote: > >> On Wed, 24 Dec 2014 18:58:17 +0200 >> 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, it looks good to me, I'll give it a test on my own sun6i and sun8i boards when I find some time to do so. >>> --- >>> >>> 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 :-) Nice. Note that my personal u-boot git repo has lcd support in the sunxi-wip branch, so that we can have at least u-boot output messages on tablets, but I guess that FEL + console via microsd breakout works too. If you can point me to a fex file for your board I can add a defconfig for your tablet with hopefully working LCD support. > And indeed, the DRAM parameters get incorrectly detected after running > the test program (the system fails to boot later on): Bummer, does this happen with both the old and the new memcmp functions ? I'll send you a private mail with a statically linked util I've written called mmio-dump, which can dump any hardware address from within android, assuming you've an adb shell as root on your tablet, you can then copy this to /system/bin and use it to dump the dram controller registers, compare with what u-boot selects and see where we're getting things wrong. Usage is e.g.: mmio-dump 0x01c62000 64 > 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. I don't think we care, at least not until we add support for coming out of standby / self-refresh mode, and when we do we should have the dram paras stored in SoC sram somewhere, and thus not use the detect path at all. > >>> 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. On sun8i this seems to make sense, on sun6i I'm not sure, I think using a variant of mctl_mem_matches there is better. Regards, Hans > > > 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 { >