Hi Marek,
On 12.12.18 03:23, Marek Behun wrote:
Hi, I have found the bug causing this issue.
Good.
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 admit that I didn't fully try to understand this issue you describe above (sorry, lack of time). If you have found a bug and do have a fix for it, then please submit a patch. Please add all developers (e.g. Patrick Delaunay etc) who did some work on this code to Cc, as changes here might be critical.
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.
If you have a nice and easy implementation to also support such memory configurations, that would be perfect of course. But I really think that such non-power-of-2 memory configurations are rather uncommon for U-Boot and most likely don't need to be supported by this function. Such configuration usually are a result of using multiple DIMM's (or SODIMM's) which can be equipped with various sized memories. And here the memory size can be read from the DIMM itself. So no need to support this in get_ram_size().
Thanks, Stefan
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, defaulting > to 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
Viele Grüße, Stefan