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

[Philipp Tomsich]

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

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
>