* [U-Boot] [PATCH] rockchip: rk3288: correct sdram setting
@ 2016-02-29 12:16 Chris Zhong
2016-03-01 2:04 ` Simon Glass
0 siblings, 1 reply; 10+ messages in thread
From: Chris Zhong @ 2016-02-29 12:16 UTC (permalink / raw)
To: u-boot
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))
#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);
+
+ 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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [U-Boot] [PATCH] rockchip: rk3288: correct sdram setting
2016-02-29 12:16 [U-Boot] [PATCH] rockchip: rk3288: correct sdram setting Chris Zhong
@ 2016-03-01 2:04 ` Simon Glass
2016-03-01 2:29 ` Chris Zhong
0 siblings, 1 reply; 10+ messages in thread
From: Simon Glass @ 2016-03-01 2:04 UTC (permalink / raw)
To: u-boot
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.
>
> #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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [U-Boot] [PATCH] rockchip: rk3288: correct sdram setting
2016-03-01 2:04 ` Simon Glass
@ 2016-03-01 2:29 ` Chris Zhong
2016-03-07 2:39 ` Simon Glass
0 siblings, 1 reply; 10+ messages in thread
From: Chris Zhong @ 2016-03-01 2:29 UTC (permalink / raw)
To: u-boot
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?
[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
>
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [U-Boot] [PATCH] rockchip: rk3288: correct sdram setting
2016-03-01 2:29 ` Chris Zhong
@ 2016-03-07 2:39 ` Simon Glass
2016-03-07 2:57 ` Chris Zhong
2016-03-07 6:51 ` [U-Boot] [PATCH v2] " Chris Zhong
0 siblings, 2 replies; 10+ messages in thread
From: Simon Glass @ 2016-03-07 2:39 UTC (permalink / raw)
To: u-boot
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@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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [U-Boot] [PATCH] rockchip: rk3288: correct sdram setting
2016-03-07 2:39 ` Simon Glass
@ 2016-03-07 2:57 ` Chris Zhong
2016-03-07 6:51 ` [U-Boot] [PATCH v2] " Chris Zhong
1 sibling, 0 replies; 10+ messages in thread
From: Chris Zhong @ 2016-03-07 2:57 UTC (permalink / raw)
To: u-boot
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
>
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [U-Boot] [PATCH v2] rockchip: rk3288: correct sdram setting
2016-03-07 2:39 ` Simon Glass
2016-03-07 2:57 ` Chris Zhong
@ 2016-03-07 6:51 ` Chris Zhong
2016-03-07 18:30 ` Simon Glass
2016-03-10 15:20 ` Simon Glass
1 sibling, 2 replies; 10+ messages in thread
From: Chris Zhong @ 2016-03-07 6:51 UTC (permalink / raw)
To: u-boot
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>
---
Changes in v2:
Modified into a more readable code style(Simon Glass)
arch/arm/mach-rockchip/rk3288/sdram_rk3288.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/arch/arm/mach-rockchip/rk3288/sdram_rk3288.c b/arch/arm/mach-rockchip/rk3288/sdram_rk3288.c
index e9e2211..17daeca 100644
--- a/arch/arm/mach-rockchip/rk3288/sdram_rk3288.c
+++ b/arch/arm/mach-rockchip/rk3288/sdram_rk3288.c
@@ -561,14 +561,14 @@ static void dram_all_config(const struct dram_info *dram,
&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 |= 1 << 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->bk == 3 ? 0 : 1 << SYS_REG_BK_SHIFT(chan);
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);
+ sys_reg |= (2 >> info->bw) << SYS_REG_BW_SHIFT(chan);
+ sys_reg |= (2 >>info->dbw) << SYS_REG_DBW_SHIFT(chan);
dram_cfg_rbc(&dram->chan[chan], chan, sdram_params);
}
@@ -720,13 +720,13 @@ size_t sdram_size_mb(struct rk3288_pmu *pmu)
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;
+ 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 = (sys_reg >> SYS_REG_BW_SHIFT(ch)) &
- SYS_REG_BW_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;
--
1.9.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [U-Boot] [PATCH v2] rockchip: rk3288: correct sdram setting
2016-03-07 6:51 ` [U-Boot] [PATCH v2] " Chris Zhong
@ 2016-03-07 18:30 ` Simon Glass
2016-03-08 0:48 ` Chris Zhong
2016-03-10 15:20 ` Simon Glass
1 sibling, 1 reply; 10+ messages in thread
From: Simon Glass @ 2016-03-07 18:30 UTC (permalink / raw)
To: u-boot
Hi Chris,
On 6 March 2016 at 23:51, 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>
> ---
>
> Changes in v2:
> Modified into a more readable code style(Simon Glass)
>
> arch/arm/mach-rockchip/rk3288/sdram_rk3288.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
Acked-by: Simon Glass <sjg@chromium.org>
Longer term, might it be possible to write this info into the device
tree and pass it to Linux that way?
>
> diff --git a/arch/arm/mach-rockchip/rk3288/sdram_rk3288.c b/arch/arm/mach-rockchip/rk3288/sdram_rk3288.c
> index e9e2211..17daeca 100644
> --- a/arch/arm/mach-rockchip/rk3288/sdram_rk3288.c
> +++ b/arch/arm/mach-rockchip/rk3288/sdram_rk3288.c
> @@ -561,14 +561,14 @@ static void dram_all_config(const struct dram_info *dram,
> &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 |= 1 << 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->bk == 3 ? 0 : 1 << SYS_REG_BK_SHIFT(chan);
> 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);
> + sys_reg |= (2 >> info->bw) << SYS_REG_BW_SHIFT(chan);
> + sys_reg |= (2 >>info->dbw) << SYS_REG_DBW_SHIFT(chan);
>
> dram_cfg_rbc(&dram->chan[chan], chan, sdram_params);
> }
> @@ -720,13 +720,13 @@ size_t sdram_size_mb(struct rk3288_pmu *pmu)
> 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;
> + 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 = (sys_reg >> SYS_REG_BW_SHIFT(ch)) &
> - SYS_REG_BW_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;
>
> --
> 1.9.1
>
Regards,
Simon
^ permalink raw reply [flat|nested] 10+ messages in thread
* [U-Boot] [PATCH v2] rockchip: rk3288: correct sdram setting
2016-03-07 18:30 ` Simon Glass
@ 2016-03-08 0:48 ` Chris Zhong
0 siblings, 0 replies; 10+ messages in thread
From: Chris Zhong @ 2016-03-08 0:48 UTC (permalink / raw)
To: u-boot
On 2016?03?08? 02:30, Simon Glass wrote:
> Hi Chris,
>
> On 6 March 2016 at 23:51, 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>
>> ---
>>
>> Changes in v2:
>> Modified into a more readable code style(Simon Glass)
>>
>> arch/arm/mach-rockchip/rk3288/sdram_rk3288.c | 14 +++++++-------
>> 1 file changed, 7 insertions(+), 7 deletions(-)
> Acked-by: Simon Glass <sjg@chromium.org>
>
> Longer term, might it be possible to write this info into the device
> tree and pass it to Linux that way?
Yes, agree, that would be better. At least not need so many complicated
calculations.
>
>> diff --git a/arch/arm/mach-rockchip/rk3288/sdram_rk3288.c b/arch/arm/mach-rockchip/rk3288/sdram_rk3288.c
>> index e9e2211..17daeca 100644
>> --- a/arch/arm/mach-rockchip/rk3288/sdram_rk3288.c
>> +++ b/arch/arm/mach-rockchip/rk3288/sdram_rk3288.c
>> @@ -561,14 +561,14 @@ static void dram_all_config(const struct dram_info *dram,
>> &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 |= 1 << 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->bk == 3 ? 0 : 1 << SYS_REG_BK_SHIFT(chan);
>> 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);
>> + sys_reg |= (2 >> info->bw) << SYS_REG_BW_SHIFT(chan);
>> + sys_reg |= (2 >>info->dbw) << SYS_REG_DBW_SHIFT(chan);
>>
>> dram_cfg_rbc(&dram->chan[chan], chan, sdram_params);
>> }
>> @@ -720,13 +720,13 @@ size_t sdram_size_mb(struct rk3288_pmu *pmu)
>> 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;
>> + 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 = (sys_reg >> SYS_REG_BW_SHIFT(ch)) &
>> - SYS_REG_BW_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;
>>
>> --
>> 1.9.1
>>
> Regards,
> Simon
>
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [U-Boot] [PATCH v2] rockchip: rk3288: correct sdram setting
2016-03-07 6:51 ` [U-Boot] [PATCH v2] " Chris Zhong
2016-03-07 18:30 ` Simon Glass
@ 2016-03-10 15:20 ` Simon Glass
2016-03-10 15:40 ` Simon Glass
1 sibling, 1 reply; 10+ messages in thread
From: Simon Glass @ 2016-03-10 15:20 UTC (permalink / raw)
To: u-boot
On 6 March 2016 at 23:51, 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>
> ---
>
> Changes in v2:
> Modified into a more readable code style(Simon Glass)
>
> arch/arm/mach-rockchip/rk3288/sdram_rk3288.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
Acked-by: Simon Glass <sjg@chromium.org>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [U-Boot] [PATCH v2] rockchip: rk3288: correct sdram setting
2016-03-10 15:20 ` Simon Glass
@ 2016-03-10 15:40 ` Simon Glass
0 siblings, 0 replies; 10+ messages in thread
From: Simon Glass @ 2016-03-10 15:40 UTC (permalink / raw)
To: u-boot
On 10 March 2016 at 08:20, Simon Glass <sjg@chromium.org> wrote:
> On 6 March 2016 at 23:51, 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>
>> ---
>>
>> Changes in v2:
>> Modified into a more readable code style(Simon Glass)
>>
>> arch/arm/mach-rockchip/rk3288/sdram_rk3288.c | 14 +++++++-------
>> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> Acked-by: Simon Glass <sjg@chromium.org>
Applied to u-boot-rockchip, thanks!
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2016-03-10 15:40 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-02-29 12:16 [U-Boot] [PATCH] rockchip: rk3288: correct sdram setting Chris Zhong
2016-03-01 2:04 ` Simon Glass
2016-03-01 2:29 ` Chris Zhong
2016-03-07 2:39 ` Simon Glass
2016-03-07 2:57 ` Chris Zhong
2016-03-07 6:51 ` [U-Boot] [PATCH v2] " Chris Zhong
2016-03-07 18:30 ` Simon Glass
2016-03-08 0:48 ` Chris Zhong
2016-03-10 15:20 ` Simon Glass
2016-03-10 15:40 ` Simon Glass
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.