From mboxrd@z Thu Jan  1 00:00:00 1970
From: Stefan Roese <sr@denx.de>
Date: Thu, 13 Dec 2018 07:23:15 +0100
Subject: [U-Boot] [PATCH u-boot-marvell v3 09/10] board: turris_mox:
 Support 1 GB version of Turris Mox
In-Reply-To: <20181213045358.4c4767d2@nic.cz>
References: <20181120120409.12822-1-marek.behun@nic.cz>
 <20181120120409.12822-9-marek.behun@nic.cz>
 <2bcc7dc7-5c60-1e5d-95e5-49e6b95fa87a@denx.de>
 <20181211145922.6333d170@dellmb.labs.office.nic.cz>
 <6772ccaf-0d38-8c37-92b8-04b9d2edbf81@denx.de>
 <20181211155338.044d02bf@dellmb.labs.office.nic.cz>
 <5790e39e-07e9-94d9-829d-bc0b42aa2e03@denx.de>
 <20181212032328.577163b7@nic.cz>
 <20090d44-ce83-ec97-fb55-fa9cce0d7c90@denx.de>
 <20181213045358.4c4767d2@nic.cz>
Message-ID: <6ff90eb2-872c-b970-0e7d-1f6bc7de2dff@denx.de>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 8bit
To: u-boot@lists.denx.de

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