[U-Boot] [PATCH u-boot-marvell v3 09/10] board: turris_mox: Support 1 GB version of Turris Mox

[Marek Behun <marek.behun@nic.cz>]

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@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
>