[U-Boot] [PATCH 1/9] dm: allow 4GB of DRAM on 32bit systems

[Philipp Tomsich]

> On 14.09.2018, at 12:53, Simon Glass <sjg at chromium.org> wrote:
> 
> Hi,
> 
> On 3 September 2018 at 16:29, Lokesh Vutla <lokeshvutla at ti.com> wrote:
>> 
>> 
>> On Sunday 02 September 2018 11:19 PM, Alexander Graf wrote:
>>> 
>>> 
>>>> Am 02.09.2018 um 18:04 schrieb Vagrant Cascadian <vagrant at debian.org>:
>>>> 
>>>>> On 2018-09-02, Alexander Graf <agraf at suse.de> wrote:
>>>>>> On 02.08.18 23:31, Dr. Philipp Tomsich wrote:
>>>>>> 
>>>>>>> On 2 Aug 2018, at 22:36, Simon Glass <sjg at chromium.org> wrote:
>>>>>>> 
>>>>>>> On 26 July 2018 at 07:59, Philipp Tomsich
>>>>>>> <philipp.tomsich at theobroma-systems.com <mailto:philipp.tomsich at theobroma-systems.com>> wrote:
>>>>>>>> Even on 32bit systems a full 4GB of DRAM may be installed and reported
>>>>>>>> by the DRAM controller.  Whether these 4GB are larger available
>>>>>>>> depends on the size/configuration of address decoding windows and
>>>>>>>> architectural features (e.g. consider a hypothetical architecture that
>>>>>>>> uses dedicated instructions to access the 'memory-mapped' peripheral
>>>>>>>> IO ranges).  In U-Boot, the available DRAM, as reported by the
>>>>>>>> device-model is independent of the accessible DRAM (i.e. adjusted top
>>>>>>>> and effective size).
>>>>>>>> 
>>>>>>>> This was first reported against the RK3288, which may have 4GB DRAM
>>>>>>>> attached, but will have a small (e.g. 128MB) window not accessible due
>>>>>>>> to address decoding limitations.
>>>>>>>> 
>>>>>>>> The solution is to increase the types used for storing the ram_size to
>>>>>>>> have at least 33 bits even on 32bit systems... i.e. we need to use a
>>>>>>>> u64 for these always (this was previously only the case for
>>>>>>>> PHYS_64BIT, which will have unwanted side-effects for 32bit systems
>>>>>>>> and would require more intrusive changes).
>>>>>>>> 
>>>>>>>> This commit changes the size-field in 'struct ram' and the ram_size in
>>>>>>>> 'gd' to be a u64.
>>>>>>>> 
>>>>>>>> Reported-by: Marty E. Plummer <hanetzer at startmail.com>
>>>>>>>> Signed-off-by: Philipp Tomsich <philipp.tomsich at theobroma-systems.com>
>>>>>>>> ---
>>>>>>>> 
>>>>>>>> include/asm-generic/global_data.h | 2 +-
>>>>>>>> include/ram.h                     | 9 ++++++++-
>>>>>>>> 2 files changed, 9 insertions(+), 2 deletions(-)
>>>>>>>> 
>>>>>>> 
>>>>>>> Reviewed-by: Simon Glass <sjg at chromium.org <mailto:sjg at chromium.org>>
>>>>>>> 
>>>>>>>> diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h
>>>>>>>> index 0fd4900..f3d687c 100644
>>>>>>>> --- a/include/asm-generic/global_data.h
>>>>>>>> +++ b/include/asm-generic/global_data.h
>>>>>>>> @@ -55,7 +55,7 @@ typedef struct global_data {
>>>>>>>>      unsigned long ram_base;         /* Base address of RAM used by U-Boot */
>>>>>>>>      unsigned long ram_top;          /* Top address of RAM used by U-Boot */
>>>>>>>>      unsigned long relocaddr;        /* Start address of U-Boot in RAM */
>>>>>>>> -       phys_size_t ram_size;           /* RAM size */
>>>>>>>> +       u64 ram_size;                   /* RAM size */
>>>>>>>>      unsigned long mon_len;          /* monitor len */
>>>>>>>>      unsigned long irq_sp;           /* irq stack pointer */
>>>>>>>>      unsigned long start_addr_sp;    /* start_addr_stackpointer */
>>>>>>>> diff --git a/include/ram.h b/include/ram.h
>>>>>>>> index 67e22d7..1fe024f 100644
>>>>>>>> --- a/include/ram.h
>>>>>>>> +++ b/include/ram.h
>>>>>>>> @@ -9,7 +9,14 @@
>>>>>>>> 
>>>>>>>> struct ram_info {
>>>>>>>>      phys_addr_t base;
>>>>>>>> -       size_t size;
>>>>>>>> +       /*
>>>>>>>> +        * We use a 64bit type for the size to correctly handle 32bit
>>>>>>>> +        * systems with 4GB of DRAM (which would overflow a 32bit type
>>>>>>>> +        * and read back as 0).  For 64bit systems, we assume (for now)
>>>>>>> 
>>>>>>> forever :-)
>>>>>>> 
>>>>>>>> +        * that they will always have less than 2^65 byte of DRAM
>>>>>>>> +        * installed. -- prt
>>>>>>> 
>>>>>>> Is the prt your signature? I suggest dropping it.
>>>>>> 
>>>>>> In fact it is. But I’ll need to rewrite the entire comment anyway for the next
>>>>>> version of this series as there’s even more functions and places that the
>>>>>> memory size is stored in...
>>>>>> 
>>>>>>> 
>>>>>>>> +        */
>>>>>>>> +       u64 size;
>>>>> 
>>>>> With LPAE available in all modern ARM cores, shouldn't phys_addr_t just
>>>>> be u64? And then we'd probably want to use that throughout the code, right?
>>>> 
>>>> Quite a few currently supported boards do not support LPAE, e.g. imx6.
>>> 
>>> What I'm trying to say is that we probably want to make phys_addr_t be u64 when CONFIG_LPAE is set.
>> 
>> That's right. Enabling PHYS_64BIT should be sufficient. Based on this
>> phys_addr_t should be set to u32 or u64. arm already does that[1].
>> 
>> [1]
>> http://git.denx.de/?p=u-boot.git;a=blob;f=arch/arm/include/asm/types.h;h=9af7353f0866f05dbe298a603d52d90e9c8e6d28;hb=HEAD
>> 
> 
> Yes I agree. So will this patch be changed?

This patch-set needs to be revised for other reasons… 
However, PHYS_64BIT is not a valid option for some of the affected chips (e.g. if 
the address space is indeed 32bit and the address-decode is not straightforward).

E.g. Marty Plummer already reported that the RK3288 breaks when enabling PHYS_64BIT.

Thanks,
Philipp.