From mboxrd@z Thu Jan 1 00:00:00 1970 From: klaus.goger at theobroma-systems.com Date: Sun, 6 May 2018 20:39:23 +0200 Subject: [U-Boot] [PATCH 3/3] rockchip: fix incorrect detection of ram size In-Reply-To: <20180506142513.19911-4-hanetzer@startmail.com> References: <20180506142513.19911-1-hanetzer@startmail.com> <20180506142513.19911-4-hanetzer@startmail.com> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit To: u-boot@lists.denx.de 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 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 > --- > 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 > #include > #include > +#include > +#include Is adding these headers really necessary? common.h already includes kernel.h and sizes.h (with some redirections of config.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) { 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. > - 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. > return (size_t)size_mb << 20; > -- > 2.17.0 Regards, Klaus