Hi, I have found the bug causing this issue.
If I understand the algorithm in get_ram_size correctly, it does approximately this. Suppose A, B, C, D, E, F are different constatnts. X(i) is a value at address 1<<i (couting in longs).
save[5] <- X(5) X(5) <- F save[4] <- X(4) X(4) <- E save[3] <- X(3) X(3) <- D save[2] <- X(2) X(2) <- C save[1] <- X(1) X(1) <- B save[0] <- X(0) X(0) <- A
So the previous values are stored in array save[]. The algorithm then checks if the values written (the constants A, B, C, D, E, F) are present at those addresses. The problem is that the previous value from save[] is written during checking of address i:
Now suppose the RAM is wrapped similarily as in MOX, so that X(i+3) is the same as X(i).
After the first part, the values are as follows
X([0,1,2,3,4,5]) = [A,B,C,A,B,C] save = [D,E,F,_3,_4,_5]
Here _3, _4, _5 are the values at addresses X(3), X(4), X(5) before the algorithm.
The code that checks the values written does this:
if X(0) != A return 0 X(0) <- save[0] !!! this also writes D to X(3)
if X(1) != B return 1 X(1) <- save[1] !!! this also writes E to X(4)
if X(2) != C return 2 X(2) <- save[2] !!! this also writes F to X(F)
if X(3) != D return 3 !!! this should return, but won't X(3) <- save[3]
...
One solution would be to write the previous values from the array save[] only immediately before return from the function.
I have to confess that I do not like how this function is written at all. It does not, for example, solve correctly the case when a device has 768 MiB of RAM from two chips (512 + 256). Given 1024 MiB as argument, it would return 1024 MiB, but the system only has 768 MiB. This maybe is never an issue with devices that run u-boot, but still.
Marek
On Tue, 11 Dec 2018 16:06:42 +0100 Stefan Roese sr@denx.de wrote:
On 11.12.18 15:53, Marek Behún wrote:
On Tue, 11 Dec 2018 15:28:11 +0100 Stefan Roese sr@denx.de wrote:
Hi Marek,
On 11.12.18 14:59, Marek Behún wrote:
get_ram_size does not work correctly on Mox. On a 512 MiB board it detects 1024 MiB of RAM, because on the 512 MiB RAM chip the topmost address bit is simply ignored and the RAM wraps - on 0x20000000-0x40000000 CPU sees the same data as on 0x0-0x20000000.
That's what get_ram_size() does: It does detect such aliases when the same memory is mapped at multiple areas (power of 2). Did you give it a try with a max value of 1024 MiB? It should return 512 on such boards.
I checked it and it returned 1024 MiB. I did printf("%08x %08x\n", get_ram_size(0, 512<<20), get_ram_size(0, 1024<<20)); on a 512 MiB board and 0x20000000 0x40000000 was printed.
Very strange. Could you please debug this issue? get_ram_size() should be able to work in such situations.
Thanks, Stefan
ATF does not run RAM size determining code either, it just gets RAM size from a register, this register is written before ATF by BootROM and we have done it so that there is always 1 GB so that we could use same secure firmware image for all Moxes. I tried to change this register in secure firmware, but this lead to Synchornous Abort events in U-Boot.
Maybe we could move the dram_init funcitons from arm64-common.c to specific board files, or maybe we could declare them __weak in arm64-common.c and turris_mox can then redefine them.
Would that be OK with you?
Please fist check if get_ram_size() can't be used.
Thanks, Stefan
Marek
On Thu, 29 Nov 2018 14:07:59 +0100 Stefan Roese sr@denx.de wrote:
On 20.11.18 13:04, Marek Behún wrote:
Depending on the data in the OTP memory, differentiate between the 512 MiB and 1 GiB versions of Turris Mox and report these RAM sizes in dram_init and dram_init_banksize.
Signed-off-by: Marek Behún marek.behun@nic.cz
arch/arm/mach-mvebu/arm64-common.c | 7 ++++++- board/CZ.NIC/turris_mox/turris_mox.c | 27+++++++++++++++++++++++++++ 2 files changed, 33 insertions(+), 1 deletion(-)
diff --git a/arch/arm/mach-mvebu/arm64-common.c b/arch/arm/mach-mvebu/arm64-common.c index f47273fde9..5e6ac9fc4a 100644 --- a/arch/arm/mach-mvebu/arm64-common.c +++ b/arch/arm/mach-mvebu/arm64-common.c @@ -43,8 +43,12 @@ const struct mbus_dram_target_info *mvebu_mbus_dram_info(void) return NULL; }
-/* DRAM init code ... */ +/*
- DRAM init code ...
- Turris Mox defines this itself, depending on data in burned
eFuses
- */
+#ifndef CONFIG_TARGET_TURRIS_MOX int dram_init_banksize(void) { fdtdec_setup_memory_banksize(); @@ -59,6 +63,7 @@ int dram_init(void)
return 0; }+#endif /* !CONFIG_TARGET_TURRIS_MOX */
2 Problems with this:
a) This does not apply any more with the latest changes in mainline.
b) I really don't like #ifdef's here in this common code. Can you not get rid of this somehow? Isn't the turris_mox also using ATF and will read the RAM size from there?
U-Boot still has the good old get_ram_size() function, which can easily auto-detect 512MiB vs 1GiB when run with 1GiB as parameter.
Thanks, Stefan
int arch_cpu_init(void) {diff --git a/board/CZ.NIC/turris_mox/turris_mox.c b/board/CZ.NIC/turris_mox/turris_mox.c index 89b3cd2ce0..9aa2fc004d 100644 --- a/board/CZ.NIC/turris_mox/turris_mox.c +++ b/board/CZ.NIC/turris_mox/turris_mox.c @@ -14,6 +14,7 @@ #include <linux/string.h> #include <linux/libfdt.h> #include <fdt_support.h> +#include <environment.h>
#ifdef CONFIG_WDT_ARMADA_37XX #include <wdt.h>@@ -40,6 +41,32 @@
DECLARE_GLOBAL_DATA_PTR;+int dram_init(void) +{
- int ret, ram_size;
- gd->ram_base = 0;
- gd->ram_size = (phys_size_t)0x20000000;
- ret = mbox_sp_get_board_info(NULL, NULL, NULL, NULL,
&ram_size);
- if (ret < 0) {
puts("Cannot read RAM size from OTP, defaultingto 512 MiB");
- } else {
if (ram_size == 1024)gd->ram_size = (phys_size_t)0x40000000;- }
- return 0;
+}
+int dram_init_banksize(void) +{
- gd->bd->bi_dram[0].start = (phys_addr_t)0;
- gd->bd->bi_dram[0].size = gd->ram_size;
- return 0;
+}
- #if defined(CONFIG_OF_BOARD_FIXUP) int board_fix_fdt(void *blob) {
Viele Grüße, Stefan
Viele Grüße, Stefan
Viele Grüße, Stefan