[U-Boot] [PATCH u-boot-marvell v3 09/10] board: turris_mox: Support 1 GB version of Turris Mox
Stefan Roese
sr at denx.de
Wed Dec 12 09:44:15 UTC 2018
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 <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
>>
>
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: sr at denx.de
More information about the U-Boot
mailing list