[U-Boot] [PATCH 3/3] rockchip: fix incorrect detection of ram size

[Marty E. Plummer <hanetzer@startmail.com>]

On Sun, May 06, 2018 at 08:39:23PM +0200, klaus.goger at theobroma-systems.com wrote:
> CC Philipp and Simon due maintainership (you may want to use get_maintainer.pl in the future)
> and Kever as the original author of the file.
> 
> > On 06.05.2018, at 16:25, Marty E. Plummer <hanetzer@startmail.com> wrote:
> > 
> > Taken from coreboot's src/soc/rockchip/rk3288/sdram.c
> > 
> > Without this change, my u-boot build for the asus c201 chromebook (4GiB)
> > is incorrectly detected as 0 Bytes of ram.
> > 
> > Signed-off-by: Marty E. Plummer <hanetzer@startmail.com>
> > ---
> > arch/arm/mach-rockchip/sdram_common.c | 62 ++++++++++++++++-----------
> > 1 file changed, 37 insertions(+), 25 deletions(-)
> > 
> > diff --git a/arch/arm/mach-rockchip/sdram_common.c b/arch/arm/mach-rockchip/sdram_common.c
> > index 76dbdc8715..a9c9f970a4 100644
> > --- a/arch/arm/mach-rockchip/sdram_common.c
> > +++ b/arch/arm/mach-rockchip/sdram_common.c
> > @@ -10,6 +10,8 @@
> > #include <asm/io.h>
> > #include <asm/arch/sdram_common.h>
> > #include <dm/uclass-internal.h>
> > +#include <linux/kernel.h>
> > +#include <linux/sizes.h>
> 
> Is adding these headers really necessary?
> common.h already includes kernel.h and sizes.h (with some redirections of config.h)
> 
If it it does get that in eventually, I suppose its not required.
I didn't know that chain of inclusion existed, so I grabbed what I
needed.
> > DECLARE_GLOBAL_DATA_PTR;
> > size_t rockchip_sdram_size(phys_addr_t reg)
> > @@ -19,34 +21,44 @@ size_t rockchip_sdram_size(phys_addr_t reg)
> > 	size_t size_mb = 0;
> > 	u32 ch;
> > 
> > -	u32 sys_reg = readl(reg);
> > -	u32 ch_num = 1 + ((sys_reg >> SYS_REG_NUM_CH_SHIFT)
> > -		       & SYS_REG_NUM_CH_MASK);
> > +	if (!size_mb) {
> 
> Is this really required? I don't see a way that size_mb will already be set at this point.
> But it blows up your diff that it takes quite a while to see that your only real change is 
> the size_mb = min(...) part at the end.
> 
It may be unneeded. I just knew that ram init worked on coreboot and not
on u-boot, and the only difference between the codepaths on both was the
if and min()
> > -	debug("%s %x %x\n", __func__, (u32)reg, sys_reg);
> > -	for (ch = 0; ch < ch_num; ch++) {
> > -		rank = 1 + (sys_reg >> SYS_REG_RANK_SHIFT(ch) &
> > -			SYS_REG_RANK_MASK);
> > -		col = 9 + (sys_reg >> SYS_REG_COL_SHIFT(ch) & SYS_REG_COL_MASK);
> > -		bk = 3 - ((sys_reg >> SYS_REG_BK_SHIFT(ch)) & SYS_REG_BK_MASK);
> > -		cs0_row = 13 + (sys_reg >> SYS_REG_CS0_ROW_SHIFT(ch) &
> > -				SYS_REG_CS0_ROW_MASK);
> > -		cs1_row = 13 + (sys_reg >> SYS_REG_CS1_ROW_SHIFT(ch) &
> > -				SYS_REG_CS1_ROW_MASK);
> > -		bw = (2 >> ((sys_reg >> SYS_REG_BW_SHIFT(ch)) &
> > -			SYS_REG_BW_MASK));
> > -		row_3_4 = sys_reg >> SYS_REG_ROW_3_4_SHIFT(ch) &
> > -			SYS_REG_ROW_3_4_MASK;
> > +		u32 sys_reg = readl(reg);
> > +		u32 ch_num = 1 + ((sys_reg >> SYS_REG_NUM_CH_SHIFT)
> > +			       & SYS_REG_NUM_CH_MASK);
> > 
> > -		chipsize_mb = (1 << (cs0_row + col + bk + bw - 20));
> > +		debug("%s %x %x\n", __func__, (u32)reg, sys_reg);
> > +		for (ch = 0; ch < ch_num; ch++) {
> > +			rank = 1 + (sys_reg >> SYS_REG_RANK_SHIFT(ch) &
> > +				SYS_REG_RANK_MASK);
> > +			col = 9 + (sys_reg >> SYS_REG_COL_SHIFT(ch) & SYS_REG_COL_MASK);
> > +			bk = 3 - ((sys_reg >> SYS_REG_BK_SHIFT(ch)) & SYS_REG_BK_MASK);
> > +			cs0_row = 13 + (sys_reg >> SYS_REG_CS0_ROW_SHIFT(ch) &
> > +					SYS_REG_CS0_ROW_MASK);
> > +			cs1_row = 13 + (sys_reg >> SYS_REG_CS1_ROW_SHIFT(ch) &
> > +					SYS_REG_CS1_ROW_MASK);
> > +			bw = (2 >> ((sys_reg >> SYS_REG_BW_SHIFT(ch)) &
> > +				SYS_REG_BW_MASK));
> > +			row_3_4 = sys_reg >> SYS_REG_ROW_3_4_SHIFT(ch) &
> > +				SYS_REG_ROW_3_4_MASK;
> > 
> > -		if (rank > 1)
> > -			chipsize_mb += chipsize_mb >> (cs0_row - cs1_row);
> > -		if (row_3_4)
> > -			chipsize_mb = chipsize_mb * 3 / 4;
> > -		size_mb += chipsize_mb;
> > -		debug("rank %d col %d bk %d cs0_row %d bw %d row_3_4 %d\n",
> > -		      rank, col, bk, cs0_row, bw, row_3_4);
> > +			chipsize_mb = (1 << (cs0_row + col + bk + bw - 20));
> > +
> > +			if (rank > 1)
> > +				chipsize_mb += chipsize_mb >> (cs0_row - cs1_row);
> > +			if (row_3_4)
> > +				chipsize_mb = chipsize_mb * 3 / 4;
> > +			size_mb += chipsize_mb;
> > +			debug("rank %d col %d bk %d cs0_row %d bw %d row_3_4 %d\n",
> > +			      rank, col, bk, cs0_row, bw, row_3_4);
> > +		}
> > +
> > +		/*
> > +		 * we use the 0x00000000~0xfeffffff space
> > +		 * since 0xff000000~0xffffffff is soc register space
> > +		 * so we reserve it 
> > +		 */
> 
> That's not true for all Rockchip SoCs. On the RK3399 for example the upper limit 
> is 0xf8000000. Even on the RK3288 DRAM address range is actually
>  0x00000000-0xfe00000
>
> > +		size_mb = min(size_mb, 0xff000000/SZ_1M);
> > 	}
> 
> Should be 0xfe000000 (4G-32MB) for RK3288.
> But there is already a define for that, SDRAM_MAX_SIZE is defined in
> rkxxx_common.h. Using that one would help to avoid possible breakage
> of other Rockchip SoCs.
> 
Oh, does that get #defined properly for each SoC? if so, I'll sub that
in for better compat.
> > 	return (size_t)size_mb << 20;
> > -- 
> > 2.17.0
> 
> Regards,
> Klaus
>