Hi Marek,
On 13.12.18 04:53, Marek Behun wrote:
it turned out that what I found out was not causing the bug.
get_ram_size reported 1 GiB of ram because I tried it when dcache was
already enabled. If I call get_ram_size in dram_init, it returns the
correct size on both 512 MiB and 1 GiB board.
In the next patch I shall define dram_init and dram_init_banksize in
arm64-common.c as __weak, and the definition in turris_mox.c shall call
get_ram_size. Is this acceptable?
Okay, please prepare the patch and I'll review it then.
Thanks,
Stefan
Marek
On Wed, 12 Dec 2018 10:44:15 +0100
Stefan Roese <s...@denx.de> wrote:
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
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
