diff mbox

[3/3] rockchip: fix incorrect detection of ram size

Message ID 20180506142513.19911-4-hanetzer@startmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Marty E. Plummer May 6, 2018, 2:25 p.m. UTC 
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(-)

Comments

Philipp Tomsich May 6, 2018, 10:19 p.m. UTC | #1 
> On 6 May 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.

Could you elaborate what the change is and what root-cause this addresses (4GB
reporting as 0 sounds a bit like a 32bit type overflowing)?
It’s really hard to tell from the patch below (which seems to have everything simply
reformatted to a different indentation)...

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

Given that there’s a “size_mb = 0” just above it, this will always evaluate
to true… 

> 
> -	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
> +		 */
> +		size_mb = min(size_mb, 0xff000000/SZ_1M);
> 	}
> 
> 	return (size_t)size_mb << 20;
> -- 
> 2.17.0
> 
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> https://lists.denx.de/listinfo/u-boot
Marty E. Plummer May 7, 2018, 12:25 a.m. UTC | #2 
On Mon, May 07, 2018 at 12:19:11AM +0200, Dr. Philipp Tomsich wrote:
> 
> > On 6 May 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.
> 
> Could you elaborate what the change is and what root-cause this addresses (4GB
> reporting as 0 sounds a bit like a 32bit type overflowing)?
> It's really hard to tell from the patch below (which seems to have everything simply
> reformatted to a different indentation)...
> 
if (!size_mb) {} wrapping, plus the min code near the end. However,
actual testing on hardware shows this if guard to be unneeded, so I'll
be dropping it. I was just taking what was different from coreboot's
implementation (which I knew to work), but not all was needed it seems.
> > 
> > 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>
> > 
> > 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) {
> 
> Given that there's a "size_mb = 0" just above it, this will always evaluate
> to true... 
> 
Very true, next patch revision will do away with this if guard, as its
unneeded according to hardware retesting.
> > 
> > -	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
> > +		 */
> > +		size_mb = min(size_mb, 0xff000000/SZ_1M);
> > 	}
> > 
> > 	return (size_t)size_mb << 20;
> > -- 
> > 2.17.0
> > 
> > _______________________________________________
> > U-Boot mailing list
> > U-Boot@lists.denx.de
> > https://lists.denx.de/listinfo/u-boot
>
Kever Yang May 7, 2018, 2:20 a.m. UTC | #3 
Hi Marty,


On 05/06/2018 10:25 PM, Marty E. Plummer 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.

I know the root cause for this issue, and I have a local patch for it.
The rk3288 is 32bit, and 4GB size is just out of range, so we need to before
the max size before return with '<<20'. Sorry for forgot to send it out.

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

I don't understand this and follow up changes, we don't really need it,
isn't it?
>  
> -	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);
> +		}
> +

I think don't need the changes before here.
> +		/*
> +		 * we use the 0x00000000~0xfeffffff space
> +		 * since 0xff000000~0xffffffff is soc register space
> +		 * so we reserve it
> +		 */
> +		size_mb = min(size_mb, 0xff000000/SZ_1M);

This is what we really need, as Klaus point out, we need to use
SDRAM_MAX_SIZE
instead of hard code.

Thanks,
- Kever
>  	}
>  
>  	return (size_t)size_mb << 20;
Marty E. Plummer May 7, 2018, 2:34 a.m. UTC | #4 
On Mon, May 07, 2018 at 10:20:55AM +0800, Kever Yang wrote:
> Hi Marty,
> 
> 
> On 05/06/2018 10:25 PM, Marty E. Plummer 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.
> 
> I know the root cause for this issue, and I have a local patch for it.
> The rk3288 is 32bit, and 4GB size is just out of range, so we need to before
> the max size before return with '<<20'. Sorry for forgot to send it out.
> 
> >
> > 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>
> >  
> >  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) {
> 
> I don't understand this and follow up changes, we don't really need it,
> isn't it?
> I think don't need the changes before here.
Yeah, that was just another level of indentation for the if (!size_mb)
guard, but I've reworked the patch to not do that as it was pointed out
that since size_mb is initialized to 0 prior.
> > +		/*
> > +		 * we use the 0x00000000~0xfeffffff space
> > +		 * since 0xff000000~0xffffffff is soc register space
> > +		 * so we reserve it
> > +		 */
> > +		size_mb = min(size_mb, 0xff000000/SZ_1M);
> 
> This is what we really need, as Klaus point out, we need to use
> SDRAM_MAX_SIZE
> instead of hard code.
Yeah, I've got a rework on that which uses SDRAM_MAX_SIZE as instructed,
build and boot tested on my hardware.
> 
> Thanks,
> - Kever
> >  	}
> >  
> >  	return (size_t)size_mb << 20;
> 
>
diff mbox

Patch

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>
 
 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) {
 
-	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
+		 */
+		size_mb = min(size_mb, 0xff000000/SZ_1M);
 	}
 
 	return (size_t)size_mb << 20;