From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chris Zhong Date: Mon, 7 Mar 2016 10:57:17 +0800 Subject: [U-Boot] [PATCH] rockchip: rk3288: correct sdram setting In-Reply-To: References: <1456748198-5635-1-git-send-email-zyw@rock-chips.com> <56D4FE74.8040109@rock-chips.com> Message-ID: <56DCEE0D.5000506@rock-chips.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Simon On 2016?03?07? 10:39, Simon Glass wrote: > Hi Chris, > > On 29 February 2016 at 19:29, Chris Zhong wrote: >> Hi Simon >> >> >> On 03/01/2016 10:04 AM, Simon Glass wrote: >>> Hi Chris, >>> >>> On 29 February 2016 at 05:16, Chris Zhong wrote: >>>> The DMC driver in v3.14 kernel[0] get the ddr setting from PMU_SYS_REG2, >>>> and it expects uboot to store the value using a same protocol. But now >>>> the ddr setting value is different with DMC, so if you enable the DMC, >>>> system would crash in kernel. Correct the sdram setting here, according >>>> to the requirements of kernel. >>>> >>>> [0] >>>> https://chromium.googlesource.com/chromiumos/third_party/kernel/+/ >>>> chromeos-3.14/drivers/clk/rockchip/clk-rk3288-dmc.c >>>> >>>> Signed-off-by: Chris Zhong >>>> --- >>>> >>>> arch/arm/include/asm/arch-rockchip/ddr_rk3288.h | 42 ++++++++--------- >>>> arch/arm/mach-rockchip/rk3288/sdram_rk3288.c | 61 >>>> +++++++++++-------------- >>>> 2 files changed, 48 insertions(+), 55 deletions(-) >>>> >>>> diff --git a/arch/arm/include/asm/arch-rockchip/ddr_rk3288.h >>>> b/arch/arm/include/asm/arch-rockchip/ddr_rk3288.h >>>> index fccabcd..f2e3130 100644 >>>> --- a/arch/arm/include/asm/arch-rockchip/ddr_rk3288.h >>>> +++ b/arch/arm/include/asm/arch-rockchip/ddr_rk3288.h >>>> @@ -459,26 +459,26 @@ enum { >>>> * [3:2] bw_ch0 >>>> * [1:0] dbw_ch0 >>>> */ >>>> -#define SYS_REG_DDRTYPE_SHIFT 13 >>>> -#define SYS_REG_DDRTYPE_MASK 7 >>>> -#define SYS_REG_NUM_CH_SHIFT 12 >>>> -#define SYS_REG_NUM_CH_MASK 1 >>>> -#define SYS_REG_ROW_3_4_SHIFT(ch) (30 + (ch)) >>>> -#define SYS_REG_ROW_3_4_MASK 1 >>>> -#define SYS_REG_CHINFO_SHIFT(ch) (28 + (ch)) >>>> -#define SYS_REG_RANK_SHIFT(ch) (11 + (ch) * 16) >>>> -#define SYS_REG_RANK_MASK 1 >>>> -#define SYS_REG_COL_SHIFT(ch) (9 + (ch) * 16) >>>> -#define SYS_REG_COL_MASK 3 >>>> -#define SYS_REG_BK_SHIFT(ch) (8 + (ch) * 16) >>>> -#define SYS_REG_BK_MASK 1 >>>> -#define SYS_REG_CS0_ROW_SHIFT(ch) (6 + (ch) * 16) >>>> -#define SYS_REG_CS0_ROW_MASK 3 >>>> -#define SYS_REG_CS1_ROW_SHIFT(ch) (4 + (ch) * 16) >>>> -#define SYS_REG_CS1_ROW_MASK 3 >>>> -#define SYS_REG_BW_SHIFT(ch) (2 + (ch) * 16) >>>> -#define SYS_REG_BW_MASK 3 >>>> -#define SYS_REG_DBW_SHIFT(ch) ((ch) * 16) >>>> -#define SYS_REG_DBW_MASK 3 >>>> +#define SYS_REG_ENC_ROW_3_4(n, ch) ((n) << (30 + (ch))) >>>> +#define SYS_REG_DEC_ROW_3_4(n, ch) ((n >> (30 + ch)) & 0x1) >>>> +#define SYS_REG_ENC_CHINFO(ch) (1 << (28 + (ch))) >>>> +#define SYS_REG_ENC_DDRTYPE(n) ((n) << 13) >>>> +#define SYS_REG_ENC_NUM_CH(n) (((n) - 1) << 12) >>>> +#define SYS_REG_DEC_NUM_CH(n) (1 + ((n >> 12) & 0x1)) >>>> +#define SYS_REG_ENC_RANK(n, ch) (((n) - 1) << (11 + ((ch) >>>> * 16))) >>>> +#define SYS_REG_DEC_RANK(n, ch) (1 + ((n >> (11 + 16 * >>>> ch)) & 0x1)) >>>> +#define SYS_REG_ENC_COL(n, ch) (((n) - 9) << (9 + ((ch) * 16))) >>>> +#define SYS_REG_DEC_COL(n, ch) (9 + ((n >> (9 + 16 * ch)) & >>>> 0x3)) >>>> +#define SYS_REG_ENC_BK(n, ch) (((n) == 3 ? 0 : 1) \ >>>> + << (8 + ((ch) * 16))) >>>> +#define SYS_REG_DEC_BK(n, ch) (3 - ((n >> (8 + 16 * ch)) & >>>> 0x1)) >>>> +#define SYS_REG_ENC_CS0_ROW(n, ch) (((n) - 13) << (6 + ((ch) * 16))) >>>> +#define SYS_REG_DEC_CS0_ROW(n, ch) (13 + ((n >> (6 + 16 * ch)) & >>>> 0x3)) >>>> +#define SYS_REG_ENC_CS1_ROW(n, ch) (((n) - 13) << (4 + ((ch) * 16))) >>>> +#define SYS_REG_DEC_CS1_ROW(n, ch) (13 + ((n >> (4 + 16 * ch)) & >>>> 0x3)) >>>> +#define SYS_REG_ENC_BW(n, ch) ((2 >> (n)) << (2 + ((ch) * 16))) >>>> +#define SYS_REG_DEC_BW(n, ch) (2 >> ((n >> (2 + 16 * ch)) & >>>> 0x3)) >>>> +#define SYS_REG_ENC_DBW(n, ch) ((2 >> (n)) << (0 + ((ch) * 16))) >>>> +#define SYS_REG_DEC_DBW(n, ch) (2 >> ((n >> (0 + 16 * ch)) & >>>> 0x3)) >>> Are the above shift/masks actually wrong? I'm not keen on this style >>> of packing and unpacking registers since it is really hard to read and >>> it's not obvious that pack and unpack work the same way. >> Actually, I copy these code from coreboot[0], can we just keep the code >> consistent with it? > I'm really not keen on that style. It is really hard to read. > > So please can you fix the bug without changing the code style? > > Actually the mask I set up for rockchip is wrong also. It should be: > > #define SYS_REG_DDRTYPE_SHIFT 13 > #define SYS_REG_DDRTYPE_MASK (7 << SYS_REG_DDRTYPE_SHIFT) > > I'll take a look at fixing these. Okay, I'll send a new patch today using a better style, read the definition is not easy. > >> [0]https://chromium.googlesource.com/chromiumos/third_party/coreboot/+/ >> firmware-veyron-6588.B/src/soc/rockchip/rk3288/sdram.c >> >> >>>> #endif >>>> diff --git a/arch/arm/mach-rockchip/rk3288/sdram_rk3288.c >>>> b/arch/arm/mach-rockchip/rk3288/sdram_rk3288.c >>>> index e9e2211..4db39ec 100644 >>>> --- a/arch/arm/mach-rockchip/rk3288/sdram_rk3288.c >>>> +++ b/arch/arm/mach-rockchip/rk3288/sdram_rk3288.c >>>> @@ -551,27 +551,27 @@ static void dram_cfg_rbc(const struct chan_info >>>> *chan, u32 chnum, >>>> static void dram_all_config(const struct dram_info *dram, >>>> const struct rk3288_sdram_params >>>> *sdram_params) >>>> { >>>> - unsigned int chan; >>>> + unsigned int channel; >>>> u32 sys_reg = 0; >>>> >>>> - sys_reg |= sdram_params->base.dramtype << SYS_REG_DDRTYPE_SHIFT; >>>> - sys_reg |= (sdram_params->num_channels - 1) << >>>> SYS_REG_NUM_CH_SHIFT; >>>> - for (chan = 0; chan < sdram_params->num_channels; chan++) { >>>> + sys_reg |= SYS_REG_ENC_DDRTYPE(sdram_params->base.dramtype); >>>> + sys_reg |= SYS_REG_ENC_NUM_CH(sdram_params->num_channels); >>>> + for (channel = 0; channel < sdram_params->num_channels; >>>> channel++) { >>>> const struct rk3288_sdram_channel *info = >>>> - &sdram_params->ch[chan]; >>>> - >>>> - sys_reg |= info->row_3_4 << SYS_REG_ROW_3_4_SHIFT(chan); >>>> - sys_reg |= chan << SYS_REG_CHINFO_SHIFT(chan); >>>> - sys_reg |= (info->rank - 1) << SYS_REG_RANK_SHIFT(chan); >>>> - sys_reg |= (info->col - 9) << SYS_REG_COL_SHIFT(chan); >>>> - sys_reg |= info->bk == 3 ? 1 << SYS_REG_BK_SHIFT(chan) : >>>> 0; >>>> - sys_reg |= (info->cs0_row - 13) << >>>> SYS_REG_CS0_ROW_SHIFT(chan); >>>> - sys_reg |= (info->cs1_row - 13) << >>>> SYS_REG_CS1_ROW_SHIFT(chan); >>>> - sys_reg |= info->bw << SYS_REG_BW_SHIFT(chan); >>>> - sys_reg |= info->dbw << SYS_REG_DBW_SHIFT(chan); >>>> - >>>> - dram_cfg_rbc(&dram->chan[chan], chan, sdram_params); >>>> + &(sdram_params->ch[channel]); >>>> + sys_reg |= SYS_REG_ENC_ROW_3_4(info->row_3_4, channel); >>>> + sys_reg |= SYS_REG_ENC_CHINFO(channel); >>>> + sys_reg |= SYS_REG_ENC_RANK(info->rank, channel); >>>> + sys_reg |= SYS_REG_ENC_COL(info->col, channel); >>>> + sys_reg |= SYS_REG_ENC_BK(info->bk, channel); >>>> + sys_reg |= SYS_REG_ENC_CS0_ROW(info->cs0_row, channel); >>>> + sys_reg |= SYS_REG_ENC_CS1_ROW(info->cs1_row, channel); >>>> + sys_reg |= SYS_REG_ENC_BW(info->bw, channel); >>>> + sys_reg |= SYS_REG_ENC_DBW(info->dbw, channel); >>> Can you fix this bug while still using the original #defines? >>> >>>> + >>>> + dram_cfg_rbc(&dram->chan[channel], channel, >>>> sdram_params); >>>> } >>>> + >>>> writel(sys_reg, &dram->pmu->sys_reg[2]); >>>> rk_clrsetreg(&dram->sgrf->soc_con2, 0x1f, >>>> sdram_params->base.stride); >>>> } >>>> @@ -712,23 +712,16 @@ size_t sdram_size_mb(struct rk3288_pmu *pmu) >>>> size_t size_mb = 0; >>>> u32 ch; >>>> u32 sys_reg = readl(&pmu->sys_reg[2]); >>>> - u32 chans; >>>> - >>>> - chans = 1 + ((sys_reg >> SYS_REG_NUM_CH_SHIFT) & >>>> SYS_REG_NUM_CH_MASK); >>>> - >>>> - for (ch = 0; ch < chans; 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 = sys_reg & (1 << SYS_REG_BK_SHIFT(ch)) ? 3 : 0; >>>> - 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 = (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 ch_num = SYS_REG_DEC_NUM_CH(sys_reg); >>>> + >>>> + for (ch = 0; ch < ch_num; ch++) { >>>> + rank = SYS_REG_DEC_RANK(sys_reg, ch); >>>> + col = SYS_REG_DEC_COL(sys_reg, ch); >>>> + bk = SYS_REG_DEC_BK(sys_reg, ch); >>>> + cs0_row = SYS_REG_DEC_CS0_ROW(sys_reg, ch); >>>> + cs1_row = SYS_REG_DEC_CS1_ROW(sys_reg, ch); >>>> + bw = SYS_REG_DEC_BW(sys_reg, ch); >>>> + row_3_4 = SYS_REG_DEC_ROW_3_4(sys_reg, ch); >>>> >>>> chipsize_mb = (1 << (cs0_row + col + bk + bw - 20)); >>>> >>>> -- >>>> 2.6.3 >>>> >>> Regards, >>> Simon >>> >>> >>> > Regards, > Simon > > >