[U-Boot] [PATCH] sunxi: Fix buggy sun6i/sun8i DRAM size detection logic
Hans de Goede
hdegoede at redhat.com
Thu Dec 25 11:14:23 CET 2014
Hi,
On 25-12-14 03:07, Siarhei Siamashka wrote:
> 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>
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 {
>
More information about the U-Boot
mailing list