[U-Boot] [PATCH u-boot-marvell v3 09/10] board: turris_mox: Support 1 GB version of Turris Mox
Marek Behun
marek.behun at nic.cz
Wed Dec 12 02:23:28 UTC 2018
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 at 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 at 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 at 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 at 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
>
More information about the U-Boot
mailing list