Hi Stefan,
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?
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
>
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
