From patchwork Thu Dec 25 02:07:53 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Siarhei Siamashka X-Patchwork-Id: 424031 X-Patchwork-Delegate: ijc@hellion.org.uk Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from theia.denx.de (theia.denx.de [85.214.87.163]) by ozlabs.org (Postfix) with ESMTP id BF4B314007D for ; Thu, 25 Dec 2014 13:08:12 +1100 (AEDT) Received: from localhost (localhost [127.0.0.1]) by theia.denx.de (Postfix) with ESMTP id A0F494B612; Thu, 25 Dec 2014 03:08:08 +0100 (CET) Received: from theia.denx.de ([127.0.0.1]) by localhost (theia.denx.de [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id Vbnx5RA3Y8SQ; Thu, 25 Dec 2014 03:08:08 +0100 (CET) Received: from theia.denx.de (localhost [127.0.0.1]) by theia.denx.de (Postfix) with ESMTP id CA92F4B609; Thu, 25 Dec 2014 03:08:07 +0100 (CET) Received: from localhost (localhost [127.0.0.1]) by theia.denx.de (Postfix) with ESMTP id 7F7354B609 for ; Thu, 25 Dec 2014 03:08:00 +0100 (CET) Received: from theia.denx.de ([127.0.0.1]) by localhost (theia.denx.de [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id Y9Yn5PRi5odk for ; Thu, 25 Dec 2014 03:08:00 +0100 (CET) X-policyd-weight: NOT_IN_SBL_XBL_SPAMHAUS=-1.5 NOT_IN_SPAMCOP=-1.5 NOT_IN_BL_NJABL=-1.5 (only DNSBL check requested) Received: from mail-la0-f45.google.com (mail-la0-f45.google.com [209.85.215.45]) by theia.denx.de (Postfix) with ESMTPS id 402C94B608 for ; Thu, 25 Dec 2014 03:07:55 +0100 (CET) Received: by mail-la0-f45.google.com with SMTP id gq15so7658791lab.18 for ; Wed, 24 Dec 2014 18:07:55 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=date:from:to:cc:subject:message-id:in-reply-to:references :mime-version:content-type:content-transfer-encoding; bh=qT06W9O53KTCeNBTVkQpO/x72RtXCfyOdvXBc4oDVH8=; b=AuQsBC0TGwIac8OGu3N9629j2VvXNMksUIL+XrBdlENq7HiYS28eImpemVBC9lsGE0 4FUN/Afi45gdnzuToj4aThNOsWarwotukk8eWmXgQGouoCG5eGAzYRRwrumc2Gsiy3bx EQ3T2juQuPpgTRWz3HU+WK8zoD2mtAFWn/UBrGxbBz4lfwUoa/1+Axk82UDpWJ1j6L/W wv0veZaaHh/H1VTy6r/mYU6lxbWLbrPuqKKmULCf1yV/XLFE+nANeQIxSu1FcHZOvBl6 QloqZWQUTTHNEdudwsCL2oM6p3J1cWZwDqjOgt2U16K4CJV6mzGzzHvAgFZ6yM6be0ZI gNCw== X-Received: by 10.152.203.137 with SMTP id kq9mr36645324lac.51.1419473275397; Wed, 24 Dec 2014 18:07:55 -0800 (PST) Received: from i7 ([212.16.98.80]) by mx.google.com with ESMTPSA id px2sm4863403lbb.27.2014.12.24.18.07.54 (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Wed, 24 Dec 2014 18:07:54 -0800 (PST) Date: Thu, 25 Dec 2014 04:07:53 +0200 From: Siarhei Siamashka To: Siarhei Siamashka Message-ID: <20141225040753.3c4df3b6@i7> In-Reply-To: <20141224193933.49fe67d4@i7> References: <1419440297-29503-1-git-send-email-siarhei.siamashka@gmail.com> <20141224193933.49fe67d4@i7> X-Mailer: Claws Mail 3.9.0 (GTK+ 2.24.24; x86_64-pc-linux-gnu) Mime-Version: 1.0 Cc: u-boot@lists.denx.de, Ian Campbell Subject: Re: [U-Boot] [PATCH] sunxi: Fix buggy sun6i/sun8i DRAM size detection logic X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.13 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: u-boot-bounces@lists.denx.de Errors-To: u-boot-bounces@lists.denx.de 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 > > --- > > > > 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 {