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

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 25-12-14 03:07, Siarhei Siamashka wrote:
> 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>

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 {
>