[U-Boot] [PATCH] rockchip: sdram-common: fix size for 4GB in 32bit SoC

[Kever Yang]

On 12/27/2018 07:56 PM, Philipp Tomsich wrote:
> Kever,
>
>> On 27.12.2018, at 02:07, Kever Yang <kever.yang at rock-chips.com> wrote:
>>
>> System will get size '0' in 32bit system if the size is 4GB
>> for the address is 32bit only, return the max space available
>> instead of actual DDR size in rockchip_sdram_size().
>>
>> Signed-off-by: Kever Yang <kever.yang at rock-chips.com>
> Reviewed-by: Philipp Tomsich <philipp.tomsich at theobroma-systems.com>
>
> See below for requested change.
> I want this in this release, so let me know if you can do the v2 (otherwise,
> I’ll reimplement myself).
>
>> ---
>>
>> arch/arm/mach-rockchip/sdram_common.c | 7 +++++++
>> 1 file changed, 7 insertions(+)
>>
>> diff --git a/arch/arm/mach-rockchip/sdram_common.c b/arch/arm/mach-rockchip/sdram_common.c
>> index 650d53e4d9..297859c390 100644
>> --- a/arch/arm/mach-rockchip/sdram_common.c
>> +++ b/arch/arm/mach-rockchip/sdram_common.c
>> @@ -48,6 +48,13 @@ size_t rockchip_sdram_size(phys_addr_t reg)
>> 		      rank, col, bk, cs0_row, bw, row_3_4);
>> 	}
>>
>> +	/*
>> +	 * There is always a blob of space reserved for peripheral device near
>> +	 * 4GB, this can avoid system get 0byte for 4GB ran in 32bit system.
>> +	 */
> The comment does not make sense. The underlying problem is that we
> can’t return 0x1.000.0000 on the affected platforms as it will be truncated
> to 0x0 (i.e. a 32bit system can not have 4GB of DRAM in U-Boot unless
> PHYS_64BIT is set).

OK then, I will comment it as a workaround..
There are actually two issue here, and seems you focus on one of it:
1. size_t overflow for 0x100000000, actually soc like rk3288 support 8GB
ram,
    although we do not use the LAPE feature till now, but the hardware
supports it,
    which means we are not not use the SDRAM_MAX_SIZE correct;
2. just like my comment says, there is always a blob of space reserved,
and the
    address space in SDRAM is not able to use for CPU, then if we enable
LAPE,
    and use more then 4GB memory, there will be two bank ram area.

Thanks,
- Kever
>
> There’s other infrastructure in U-Boot to account for peripheral devices that
> may overlap our SDRAM (e.g. board_get_usable_ram_top).
>
> Reword the comment to clearly identify this as a workaround, because
> PHYS_64BIT is not set and we need to return 4GB (i.e. a 33bit value).
>
>> +	if (size_mb > (SDRAM_MAX_SIZE >> 20))
>> +		size_mb = (SDRAM_MAX_SIZE >> 20);
>> +
> Please use a ‘max’ here.
>
> While I object to the SDRAM_MAX_SIZE here (this should be what the DRAM
> controller sees and not what is usable), we may as well go with it as long as the
> comment clearly communicates why it was chosen…
>
> …the other option would be to say “make sure that we do not overflow size_t”
> and then write the code similar to the following pseudocode:
>
> 	u64  size_in_bytes = ((u64)size_mb) << 20;
> 	/* clamp to maximum representable in size_t */
> 	size_in_bytes = min(size_in_bytes, (u64)SIZE_MAX);
>
> 	return (size_t)size_in_bytes;
>
>> 	return (size_t)size_mb << 20;
>> }
>>
>> -- 
>> 2.20.1
>>
>