[PATCH 01/11] imx: implement get_effective_memsize

Wed Nov 16 07:03:35 CET 2022

Hi Peng,

please find one late comment from me below...

On 16.11.22 03:43, Peng Fan wrote:
> 
> +Tom
> 
> On 11/9/2022 3:40 PM, Pali Rohár wrote:
>> On Wednesday 09 November 2022 09:48:53 Peng Fan wrote:
>>> On 11/8/2022 4:03 PM, Pali Rohár wrote:
>>>> On Tuesday 08 November 2022 07:56:59 Peng Fan wrote:
>>>>>> Subject: Re: [PATCH 01/11] imx: implement get_effective_memsize
>>>>>>
>>>>>> On Tuesday 08 November 2022 09:38:01 Peng Fan wrote:
>>>>>>> On 11/7/2022 3:55 PM, Pali Rohár wrote:
>>>>>>>> On Monday 07 November 2022 16:00:06 Peng Fan (OSS) wrote:
>>>>>>>>> From: Peng Fan <peng.fan at nxp.com>
>>>>>>>>>
>>>>>>>>> To i.MX6/7 which has 2GB memory, the upper 4KB cut off, will cause
>>>>>>>>> the top 1MB not mapped as normal memory, because ARMV7-A use
>>>>>>>>> section mapping. So implement i.MX6/7 specific
>>>>>>>>> get_effective_memsize to fix the issue.
>>>>>>>>>
>>>>>>>>> Fixes: 777aaaa706bc("common/memsize.c: Fix get_effective_memsize()
>>>>>>>>> to check for overflow")
>>>>>>>>> Signed-off-by: Peng Fan <peng.fan at nxp.com>
>>>>>>>>
>>>>>>>> Should not just configuring CONFIG_MAX_MEM_MAPPED properly avoid
>>>>>> that issue?
>>>>>>>
>>>>>>> No, unless I decrease PHYS_SDRAM_SIZE.
>>>>>>
>>>>>> So, what is the issue? I just do not see what happens after 
>>>>>> 777aaaa706bc
>>>>>> that RAM size is calculated incorrectly in your case. I did not 
>>>>>> catch the
>>>>>> description from commit message. What are your gd->ram_size and
>>>>>> CONFIG_MAX_MEM_MAPPED values that current code does not work?
>>>>>
>>>>> The base is 2GB, the size is 2GB. With CONFIG_MAX_MEM_MAPPED,
>>>>> the ram_size already decreased by 4KB.
>>>>
>>>> If base is 2GB and size is 2GB then ram_top is 4GB which cannot be
>>>> represented in 32-bit phys_size_t type and hence some of other u-boot
>>>> functions use 0 as ram_top. Mentioned commit tries to fix this issue.
>>>> I guess that you have some other hidden problem and my change just
>>>> showed implication of that.
>>>
>>> The issue is with higher 4KB cut off, the MMU mapping will cut off
>>> 1MB or 2MB(section mapping), so after MMU enabled, the PC instruction 
>>> will
>>> not able to fetch instruction in the higher 1MB area because of U-Boot
>>> relocated to
>>> top of DRAM.
>>
>> But it is not possible to represent whole 4GB of RAM size in 32-bit
>> phys_size_t type. So some cut-off for this storage is required.
> 
> With your commit
> Ram size: 80000000
> Ram top: FFFFF000

Looks good to me.

> Your patch would break all i.MX6/7 boards that 2GB memory with
> base starts at 0x80000000.

Why is this the case?

> After apply this patch, It works, even if RAM top is 0.
> Ram size: 80000000
> Ram top: 00000000

Ughhh. This looks broken to me. Address 0x0000.0000 is a valid address
and points to 0! In this case the code just seems to work (as seen
below) as 0 is wrapped around since it's a 32bit value.

> full log below:
> U-Boot 2022.10-00346-gd80f7fc140a-dirty (Nov 16 2022 - 11:24:56 +0800)
> 
> initcall: 8780f549
> U-Boot code: 87800000 -> 878562BC  BSS: -> 8785D998
> initcall: 8780f43d
> initcall: 87801fe1
> CPU:   Freescale i.MX6SLL rev1.0 at 792MHz
> CPU:   Commercial temperature grade (0C to 95C)
> Reset cause: POR
> initcall: 8780fb21
> Model: Freescale i.MX6SLL EVK Board
> Board: MX6SLL EVK
> initcall: 8780f5b9
> initcall: 8780f5a9
> DRAM:  initcall: 8780356d
> initcall: 8780f87d
> Monitor len: 0005D998
> Ram size: 80000000
> Ram top: 00000000
> initcall: 8780f429
> initcall: 87801d17
> initcall: 8780f5eb
> initcall: 8780f5ef
> initcall: 8780f501
> Reserving 374k for U-Boot at: fff92000

Here we've got the wrap around.

So why does it not work with the Pali's original code leading to a
ram-top of 0xFFFFF000? Where does it fail? Could you please post
this bootlog as well?

Thanks,
Stefan

> initcall: 8780f611
> Reserving 16392k for malloc() at: fef90000
> initcall: 8780f56d
> Reserving 68 Bytes for Board Info at: fef8ffb0
> initcall: 8780f63d
> Reserving 248 Bytes for Global Data at: fef8feb0
> initcall: 8780f669
> Reserving 25600 Bytes for FDT at: fef89ab0
> initcall: 8780f5f3
> initcall: 8780f5f7
> initcall: 8780f607
> initcall: 8780f8e1
> initcall: 8780f441
> initcall: 8780f6c1
> 
> RAM Configuration:
> Bank #0: 80000000 2 GiB
> 
> DRAM:  2 GiB
> initcall: 8780f8f3
> initcall: 8780f4ed
> New Stack Pointer is: fef89a90
> initcall: 8780f45b
> initcall: 8780f5fb
> initcall: 8780f5ff
> initcall: 8780f48d
> Relocation Offset is: 78792000
> Relocating to fff92000, new gd at fef8feb0, sp at fef89a90
> initcall: 8780f60b
> initcall: 8780f5e3
> Core:  74 devices, 16 uclasses, devicetree: separate
> MMC:   FSL_SDHC: 0, FSL_SDHC: 2
> Loading Environment from MMC... MMC: no card present
> *** Warning - No block device, using default environment
> 
> In:    serial at 2020000
> Out:   serial at 2020000
> Err:   serial at 2020000
> Net:   No ethernet found.
> Hit any key to stop autoboot:  0
> 
> 
>>
>>> Regards,
>>> Peng.
>>>
>>> Could you check how is gd->ram_top
>>>> configured with and without this change?
>>
>> Could you check this?
>>
>>>>> Regards,
>>>>> Peng.
>>>>>
>>>>>>
>>>>>>> Regards,
>>>>>>> Peng.
>>>>>>>
>>>>>>>>
>>>>>>>>> ---
>>>>>>>>>     arch/arm/mach-imx/cache.c | 14 ++++++++++++++
>>>>>>>>>     1 file changed, 14 insertions(+)
>>>>>>>>>
>>>>>>>>> diff --git a/arch/arm/mach-imx/cache.c b/arch/arm/mach-imx/cache.c
>>>>>>>>> index ab9b621a2a6..69a085abee7 100644
>>>>>>>>> --- a/arch/arm/mach-imx/cache.c
>>>>>>>>> +++ b/arch/arm/mach-imx/cache.c
>>>>>>>>> @@ -7,10 +7,24 @@
>>>>>>>>>     #include <cpu_func.h>
>>>>>>>>>     #include <asm/armv7.h>
>>>>>>>>>     #include <asm/cache.h>
>>>>>>>>> +#include <asm/global_data.h>
>>>>>>>>>     #include <asm/pl310.h>
>>>>>>>>>     #include <asm/io.h>
>>>>>>>>>     #include <asm/mach-imx/sys_proto.h>
>>>>>>>>> +DECLARE_GLOBAL_DATA_PTR;
>>>>>>>>> +
>>>>>>>>> +phys_size_t get_effective_memsize(void) { #ifndef
>>>>>>>>> +CONFIG_MAX_MEM_MAPPED
>>>>>>>>> +    return gd->ram_size;
>>>>>>>>> +#else
>>>>>>>>> +    /* limit stack to what we can reasonable map */
>>>>>>>>> +    return ((gd->ram_size > CONFIG_MAX_MEM_MAPPED) ?
>>>>>>>>> +        CONFIG_MAX_MEM_MAPPED : gd->ram_size); #endif }
>>>>>>>>> +
>>>>>>>>>     void enable_ca7_smp(void)
>>>>>>>>>     {
>>>>>>>>>         u32 val;
>>>>>>>>> -- 
>>>>>>>>> 2.36.0
>>>>>>>>>

Viele Grüße,
Stefan Roese

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