[U-Boot] [PATCH] sunxi: Fix buggy sun6i/sun8i DRAM size detection logic
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 ---
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.
It might be a good idea to backup/restore the data in RAM when doing this check in the code.
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. But since I don't have any Allwinner A23/A31 devboard for testing/debugging, don't expect me to do this work any time soon, or ever at all.
arch/arm/cpu/armv7/sunxi/dram_sun6i.c | 1 - arch/arm/cpu/armv7/sunxi/dram_sun8i.c | 1 - arch/arm/include/asm/arch-sunxi/dram.h | 22 ++++++---------------- 3 files changed, 6 insertions(+), 18 deletions(-)
diff --git a/arch/arm/cpu/armv7/sunxi/dram_sun6i.c b/arch/arm/cpu/armv7/sunxi/dram_sun6i.c index 4675c48..4518b80 100644 --- a/arch/arm/cpu/armv7/sunxi/dram_sun6i.c +++ b/arch/arm/cpu/armv7/sunxi/dram_sun6i.c @@ -377,7 +377,6 @@ unsigned long sunxi_dram_init(void) MCTL_CR_BANK(1) | MCTL_CR_RANK(1));
/* Detect and set page size */ - mctl_mem_fill(); for (columns = 7; columns < 20; columns++) { if (mctl_mem_matches(1 << columns)) break; diff --git a/arch/arm/cpu/armv7/sunxi/dram_sun8i.c b/arch/arm/cpu/armv7/sunxi/dram_sun8i.c index df9ff1f..ebba53b 100644 --- a/arch/arm/cpu/armv7/sunxi/dram_sun8i.c +++ b/arch/arm/cpu/armv7/sunxi/dram_sun8i.c @@ -289,7 +289,6 @@ unsigned long sunxi_dram_init(void) writel(0x000310f4 | MCTL_CR_PAGE_SIZE(page_size), &mctl_com->cr); setbits_le32(&mctl_com->swonr, 0x0003ffff); - mctl_mem_fill(); for (rows = 11; rows < 16; rows++) { offset = 1 << (rows + columns + bus); if (mctl_mem_matches(offset)) diff --git a/arch/arm/include/asm/arch-sunxi/dram.h b/arch/arm/include/asm/arch-sunxi/dram.h index 8d78029..7ff43e6 100644 --- a/arch/arm/include/asm/arch-sunxi/dram.h +++ b/arch/arm/include/asm/arch-sunxi/dram.h @@ -24,8 +24,6 @@ #include <asm/arch/dram_sun4i.h> #endif
-#define MCTL_MEM_FILL_MATCH_COUNT 64 - unsigned long sunxi_dram_init(void);
/* @@ -42,24 +40,16 @@ static inline void mctl_await_completion(u32 *reg, u32 mask, u32 val) }
/* - * Fill beginning of DRAM with "random" data for mctl_mem_matches() - */ -static inline void mctl_mem_fill(void) -{ - int i; - - for (i = 0; i < MCTL_MEM_FILL_MATCH_COUNT; i++) - writel(0xaa55aa55 + i, CONFIG_SYS_SDRAM_BASE + i * 4); -} - -/* * Test if memory at offset offset matches memory at begin of DRAM */ static inline bool mctl_mem_matches(u32 offset) { - return memcmp((u32 *)CONFIG_SYS_SDRAM_BASE, - (u32 *)(CONFIG_SYS_SDRAM_BASE + offset), - MCTL_MEM_FILL_MATCH_COUNT * 4) == 0; + /* Try to write different values to RAM at two addresses */ + writel(0, CONFIG_SYS_SDRAM_BASE); + writel(0xaa55aa55, CONFIG_SYS_SDRAM_BASE + offset); + /* Check if the same value is actually observed when reading back */ + return readl(CONFIG_SYS_SDRAM_BASE) == + readl(CONFIG_SYS_SDRAM_BASE + offset); }
#endif /* _SUNXI_DRAM_H */
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
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.
It might be a good idea to backup/restore the data in RAM when doing this check in the code.
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.
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
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 {
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 {
On Thu, 25 Dec 2014 11:14:23 +0100 Hans de Goede hdegoede@redhat.com wrote:
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.
Thanks.
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.
HDMI works too. Too bad that neither LCD nor HDMI can handle input. But at least the UART console can. I was hoping to just get WLAN working as the next step in order to use it instead of the UART console for communicating with the device.
The MSI Primo81 is quite a nice tablet: * has a HDMI connector (can run a real desktop Linux system on a HDMI monitor, that is if we get USB OTG working) * has access to the hardware FEL button (this makes the device truly unbrickable) * has a sturdy slim metal frame, but a downside is that it may be more difficult/dangerous to disassemble * has an IPS LCD display with good colors and viewing angles but unfortunately relatively low resolution (1024x768) * The current price seems to be around or under 100 EUR on amazon.de and other places.
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.
If you really insist on doing this yourself, then you can find the fex file in the sunxi-boards git repository: https://github.com/linux-sunxi/sunxi-boards/blob/master/sys_config/a31s/msi_...
In any case, this tablet is not going to be really usable until the mainline kernel gets USB OTG support. Which gives us some weeks/months to take care of the necessary dts/defconfig files.
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 ?
Everything works fine after the "sunxi: Fix buggy sun6i/sun8i DRAM size detection logic" patch is applied. Sorry for not stating this clearly enough.
Without the patch and after running the test program, I just see that the system fails to boot. Typically with the HDMI not coming up at all. The UART console allows to see why it fails and makes debugging possible.
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
We are not getting anything that behaves obviously wrong, so I would postpone this check for a later date.
I had root on this tablet for a short while (just for extracting fex), but not anymore. Gaining root in Android is a pain on this tablet. And one rooting package that works is flagged as a trojan on virustotal.com (maybe a bogus report, but I don't want to take any risks). Since I'm actually using this tablet as an Android tablet, trying to root it again only for running mmio-dump would cause way too much disruption (backing up and restoring everything).
But I'm glad that you are actually using mmio dumping yourself. You did not sound like a big fan of it earlier, this had me seriously worried: http://lists.denx.de/pipermail/u-boot/2014-December/199429.html
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.
OK.
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.
Agreed here.
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 {
Hi,
On 25-12-14 17:13, Siarhei Siamashka wrote:
On Thu, 25 Dec 2014 11:14:23 +0100 Hans de Goede hdegoede@redhat.com wrote:
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.
Thanks.
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.
HDMI works too. Too bad that neither LCD nor HDMI can handle input. But at least the UART console can. I was hoping to just get WLAN working as the next step in order to use it instead of the UART console for communicating with the device.
The MSI Primo81 is quite a nice tablet:
- has a HDMI connector (can run a real desktop Linux system on a HDMI monitor, that is if we get USB OTG working)
- has access to the hardware FEL button (this makes the device truly unbrickable)
- has a sturdy slim metal frame, but a downside is that it may be more difficult/dangerous to disassemble
- has an IPS LCD display with good colors and viewing angles but unfortunately relatively low resolution (1024x768)
- The current price seems to be around or under 100 EUR on amazon.de and other places.
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.
If you really insist on doing this yourself, then you can find the fex file in the sunxi-boards git repository: https://github.com/linux-sunxi/sunxi-boards/blob/master/sys_config/a31s/msi_...
In any case, this tablet is not going to be really usable until the mainline kernel gets USB OTG support. Which gives us some weeks/months to take care of the necessary dts/defconfig files.
Ack, I offered to create the defconfig since the alternative was to write a short howto on the VIDEO_LCD_MODE Kconfig, but since I've done that now anyways I'll happily wait for you to provide a defconfig.
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 ?
Everything works fine after the "sunxi: Fix buggy sun6i/sun8i DRAM size detection logic" patch is applied. Sorry for not stating this clearly enough.
Ah, ok, that is good to know.
Without the patch and after running the test program, I just see that the system fails to boot. Typically with the HDMI not coming up at all. The UART console allows to see why it fails and makes debugging possible.
Ack.
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
We are not getting anything that behaves obviously wrong, so I would postpone this check for a later date.
I had root on this tablet for a short while (just for extracting fex), but not anymore. Gaining root in Android is a pain on this tablet. And one rooting package that works is flagged as a trojan on virustotal.com (maybe a bogus report, but I don't want to take any risks). Since I'm actually using this tablet as an Android tablet, trying to root it again only for running mmio-dump would cause way too much disruption (backing up and restoring everything).
But I'm glad that you are actually using mmio dumping yourself. You did not sound like a big fan of it earlier, this had me seriously worried: http://lists.denx.de/pipermail/u-boot/2014-December/199429.html
Oh, no I do use mmio dumping frequently to check various things, esp. to compare the dram setup between what boot0 / u-boot does. I just do not see much use in a meminfo like tool for a31 / a23 for end users, since for double checking all register matter, and for a simple bringup the info in the fex is enough.
Regards,
Hans
Hi,
On 24-12-14 17:58, 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 <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. I've run various tests and it works well, queued up for merging in u-boot-sunxi/next .
Regards,
Hans
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.
It might be a good idea to backup/restore the data in RAM when doing this check in the code.
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. But since I don't have any Allwinner A23/A31 devboard for testing/debugging, don't expect me to do this work any time soon, or ever at all.
arch/arm/cpu/armv7/sunxi/dram_sun6i.c | 1 - arch/arm/cpu/armv7/sunxi/dram_sun8i.c | 1 - arch/arm/include/asm/arch-sunxi/dram.h | 22 ++++++---------------- 3 files changed, 6 insertions(+), 18 deletions(-)
diff --git a/arch/arm/cpu/armv7/sunxi/dram_sun6i.c b/arch/arm/cpu/armv7/sunxi/dram_sun6i.c index 4675c48..4518b80 100644 --- a/arch/arm/cpu/armv7/sunxi/dram_sun6i.c +++ b/arch/arm/cpu/armv7/sunxi/dram_sun6i.c @@ -377,7 +377,6 @@ unsigned long sunxi_dram_init(void) MCTL_CR_BANK(1) | MCTL_CR_RANK(1));
/* Detect and set page size */
- mctl_mem_fill(); for (columns = 7; columns < 20; columns++) { if (mctl_mem_matches(1 << columns)) break;
diff --git a/arch/arm/cpu/armv7/sunxi/dram_sun8i.c b/arch/arm/cpu/armv7/sunxi/dram_sun8i.c index df9ff1f..ebba53b 100644 --- a/arch/arm/cpu/armv7/sunxi/dram_sun8i.c +++ b/arch/arm/cpu/armv7/sunxi/dram_sun8i.c @@ -289,7 +289,6 @@ unsigned long sunxi_dram_init(void) writel(0x000310f4 | MCTL_CR_PAGE_SIZE(page_size), &mctl_com->cr); setbits_le32(&mctl_com->swonr, 0x0003ffff);
mctl_mem_fill();diff --git a/arch/arm/include/asm/arch-sunxi/dram.h b/arch/arm/include/asm/arch-sunxi/dram.h index 8d78029..7ff43e6 100644 --- a/arch/arm/include/asm/arch-sunxi/dram.h +++ b/arch/arm/include/asm/arch-sunxi/dram.h @@ -24,8 +24,6 @@ #include <asm/arch/dram_sun4i.h> #endif
-#define MCTL_MEM_FILL_MATCH_COUNT 64
unsigned long sunxi_dram_init(void);
/*
@@ -42,24 +40,16 @@ static inline void mctl_await_completion(u32 *reg, u32 mask, u32 val) }
/*
- Fill beginning of DRAM with "random" data for mctl_mem_matches()
- */
-static inline void mctl_mem_fill(void) -{
- int i;
- for (i = 0; i < MCTL_MEM_FILL_MATCH_COUNT; i++)
writel(0xaa55aa55 + i, CONFIG_SYS_SDRAM_BASE + i * 4);-}
-/*
- Test if memory at offset offset matches memory at begin of DRAM
*/ static inline bool mctl_mem_matches(u32 offset) {
- return memcmp((u32 *)CONFIG_SYS_SDRAM_BASE,
(u32 *)(CONFIG_SYS_SDRAM_BASE + offset),MCTL_MEM_FILL_MATCH_COUNT * 4) == 0;
/* Try to write different values to RAM at two addresses */
writel(0, CONFIG_SYS_SDRAM_BASE);
writel(0xaa55aa55, CONFIG_SYS_SDRAM_BASE + offset);
/* Check if the same value is actually observed when reading back */
return readl(CONFIG_SYS_SDRAM_BASE) ==
readl(CONFIG_SYS_SDRAM_BASE + offset);}
#endif /* _SUNXI_DRAM_H */
participants (2)
-
Hans de Goede -
Siarhei Siamashka