[U-Boot] [PATCH] rockchip: rk3288: correct sdram setting

[Chris Zhong <zyw@rock-chips.com>]

Hi Simon

On 2016?03?07? 10:39, Simon Glass wrote:
> Hi Chris,
>
> On 29 February 2016 at 19:29, Chris Zhong <zyw@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@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@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.
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
>
>
>