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 <s...@denx.de> wrote:
On 11.12.18 15:53, Marek Behún wrote:
On Tue, 11 Dec 2018 15:28:11 +0100
Stefan Roese <s...@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 <s...@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.be...@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
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: s...@denx.de
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
