[U-Boot] [PATCH] rockchip: rk3288: correct sdram setting
Simon Glass
sjg at chromium.org
Mon Mar 7 03:39:26 CET 2016
Hi Chris,
On 29 February 2016 at 19:29, Chris Zhong <zyw at rock-chips.com> wrote:
> Hi Simon
>
>
> On 03/01/2016 10:04 AM, Simon Glass wrote:
>>
>> Hi Chris,
>>
>> On 29 February 2016 at 05:16, Chris Zhong <zyw at rock-chips.com> 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 <zyw at rock-chips.com>
>>> ---
>>>
>>> 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.
>
> [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
More information about the U-Boot
mailing list