Skip to contentSkip to conversation

rockchip: rk3288: correct sdram setting

PATCH New Chris Zhong 2 participants 10 messages Feb 29, 2016 – Mar 10, 2016

Subsystems (3)
Arm (32-bit) Arm sub-architectures Arm Rockchip
lore
Share & tools
lore

Chris Zhong

author
#0 lore raw

PATCH 1/1 rockchip: rk3288: correct sdram setting

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 <[email protected]>

---

 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(-)
arch/arm/include/asm/arch-rockchip/ddr_rk3288.h +21 -21
index fccabcd..f2e3130 100644
@@ -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
arch/arm/mach-rockchip/rk3288/sdram_rk3288.c +27 -34

77 lines not on this page · raw message

Signature · 1 line
2.6.3
#1 lore raw

PATCH 1/1 rockchip: rk3288: correct sdram setting

Hi Chris,

Quoted 57 lines · Chris Zhong

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 <[email protected]>

---

 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(-)
arch/arm/include/asm/arch-rockchip/ddr_rk3288.h +11 -21
index fccabcd..f2e3130 100644
@@ -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) \

arch/arm/include/asm/arch-rockchip/ddr_rk3288.h enum { · line 482

Quoted 10 lines
arch/arm/include/asm/arch-rockchip/ddr_rk3288.h +10 -0
+                                       << (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.

Quoted 34 lines
arch/arm/include/asm/arch-rockchip/ddr_rk3288.h +0 -0
 #endif
arch/arm/mach-rockchip/rk3288/sdram_rk3288.c +4 -17
index e9e2211..4db39ec 100644
@@ -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);

arch/arm/mach-rockchip/rk3288/sdram_rk3288.c static void dram_cfg_rbc(const struct chan_info *chan, u32 chnum, · line 570

Quoted 10 lines
arch/arm/mach-rockchip/rk3288/sdram_rk3288.c +10 -0
+                       &(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?

Quoted 42 lines
arch/arm/mach-rockchip/rk3288/sdram_rk3288.c +13 -18
+
+               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));
 
--
Quoted 2 lines

2.6.3

Regards,
Simon

author
#2 lore raw

PATCH 1/1 rockchip: rk3288: correct sdram setting

Hi Simon

Quoted 65 lines · Simon Glass

Hi Chris,

Quoted 62 lines · Chris Zhong

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 <[email protected]>

---

  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(-)
arch/arm/include/asm/arch-rockchip/ddr_rk3288.h +16 -21
index fccabcd..f2e3130 100644
@@ -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)))
Quoted 8 lines

arch/arm/include/asm/arch-rockchip/ddr_rk3288.h enum { · line 482

Quoted 5 lines
arch/arm/include/asm/arch-rockchip/ddr_rk3288.h +5 -0
+#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

Quoted 95 lines
Quoted 34 lines
arch/arm/include/asm/arch-rockchip/ddr_rk3288.h +0 -0
  #endif
arch/arm/mach-rockchip/rk3288/sdram_rk3288.c +4 -17
index e9e2211..4db39ec 100644
@@ -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);

arch/arm/mach-rockchip/rk3288/sdram_rk3288.c static void dram_cfg_rbc(const struct chan_info *chan, u32 chnum, · line 570

Quoted 10 lines
arch/arm/mach-rockchip/rk3288/sdram_rk3288.c +10 -0
+                       &(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?

Quoted 42 lines
arch/arm/mach-rockchip/rk3288/sdram_rk3288.c +13 -18
+
+               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));
 
--
Quoted 2 lines

2.6.3

Regards,
Simon

#3 lore raw

PATCH 1/1 rockchip: rk3288: correct sdram setting

Hi Chris,

Quoted 91 lines · Chris Zhong

Hi Simon

Quoted 78 lines · Simon Glass

Hi Chris,

Quoted 74 lines · Chris Zhong

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 <[email protected]>

---

  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(-)
arch/arm/include/asm/arch-rockchip/ddr_rk3288.h +10 -21
index fccabcd..f2e3130 100644
@@ -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))

arch/arm/include/asm/arch-rockchip/ddr_rk3288.h +3 -0
+#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))

arch/arm/include/asm/arch-rockchip/ddr_rk3288.h +2 -0
+#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))

arch/arm/include/asm/arch-rockchip/ddr_rk3288.h +2 -0
+#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))

arch/arm/include/asm/arch-rockchip/ddr_rk3288.h +2 -0
+#define SYS_REG_ENC_BW(n, ch)          ((2 >> (n)) << (2 + ((ch) * 16)))
+#define SYS_REG_DEC_BW(n, ch)          (2 >> ((n >> (2 + 16 * ch)) &
Quoted 8 lines
Quoted 4 lines

0x3))

arch/arm/include/asm/arch-rockchip/ddr_rk3288.h +2 -0
+#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.

Quoted 2 lines

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.

Quoted 114 lines

[0]https://chromium.googlesource.com/chromiumos/third_party/coreboot/+/ firmware-veyron-6588.B/src/soc/rockchip/rk3288/sdram.c

Quoted 108 lines
Quoted 42 lines
  #endif
arch/arm/mach-rockchip/rk3288/sdram_rk3288.c +4 -17
index e9e2211..4db39ec 100644
@@ -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);

arch/arm/mach-rockchip/rk3288/sdram_rk3288.c static void dram_cfg_rbc(const struct chan_info · line 570

Quoted 10 lines
arch/arm/mach-rockchip/rk3288/sdram_rk3288.c +10 -0
+                       &(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?

Quoted 45 lines
arch/arm/mach-rockchip/rk3288/sdram_rk3288.c +2 -0
+
+               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);
}

arch/arm/mach-rockchip/rk3288/sdram_rk3288.c +10 -17
@@ -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));
Quoted 3 lines

--
2.6.3

Regards,
Simon

Regards,
Simon

author
#4 lore raw

PATCH 1/1 rockchip: rk3288: correct sdram setting

Hi Simon

Quoted 94 lines · Simon Glass

Hi Chris,

Quoted 87 lines · Chris Zhong

Hi Simon

Quoted 75 lines · Simon Glass

Hi Chris,

Quoted 72 lines · Chris Zhong

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 <[email protected]>

---

   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(-)
arch/arm/include/asm/arch-rockchip/ddr_rk3288.h +10 -21
index fccabcd..f2e3130 100644
@@ -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))

arch/arm/include/asm/arch-rockchip/ddr_rk3288.h +3 -0
+#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))

arch/arm/include/asm/arch-rockchip/ddr_rk3288.h +2 -0
+#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))

arch/arm/include/asm/arch-rockchip/ddr_rk3288.h +2 -0
+#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)))

Quoted 8 lines
Quoted 5 lines

+#define SYS_REG_DEC_BW(n, ch) (2 >> ((n >> (2 + 16 * ch)) &
0x3))

arch/arm/include/asm/arch-rockchip/ddr_rk3288.h +2 -0
+#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.

Quoted 2 lines

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.

Quoted 8 lines

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.

Quoted 116 lines
Quoted 111 lines

[0]https://chromium.googlesource.com/chromiumos/third_party/coreboot/+/ firmware-veyron-6588.B/src/soc/rockchip/rk3288/sdram.c

Quoted 106 lines
Quoted 41 lines
   #endif
arch/arm/mach-rockchip/rk3288/sdram_rk3288.c +4 -17
index e9e2211..4db39ec 100644
@@ -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);

arch/arm/mach-rockchip/rk3288/sdram_rk3288.c static void dram_cfg_rbc(const struct chan_info · line 570

Quoted 10 lines
arch/arm/mach-rockchip/rk3288/sdram_rk3288.c +10 -0
+                       &(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?

Quoted 45 lines
arch/arm/mach-rockchip/rk3288/sdram_rk3288.c +2 -0
+
+               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);
   }
arch/arm/mach-rockchip/rk3288/sdram_rk3288.c +10 -17
@@ -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));
Quoted 3 lines

--
2.6.3

Regards,
Simon

Regards,
Simon

author
#5 lore raw

PATCH v2 1/1 rockchip: rk3288: correct sdram setting

In reply to Simon Glass

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 <[email protected]>

---

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(-)
arch/arm/mach-rockchip/rk3288/sdram_rk3288.c +7 -7
index e9e2211..17daeca 100644
@@ -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;
Signature · 1 line
1.9.1
#6 lore raw

PATCH v2 1/1 rockchip: rk3288: correct sdram setting

Hi Chris,

Quoted 11 lines · Chris Zhong

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

Quoted 8 lines
Signed-off-by Chris Zhong <[email protected]>

---

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 <[email protected]>

Longer term, might it be possible to write this info into the device tree and pass it to Linux that way?

Quoted 42 lines
arch/arm/mach-rockchip/rk3288/sdram_rk3288.c +7 -8
index e9e2211..17daeca 100644
@@ -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;
 
--
Quoted 2 lines

1.9.1

Regards,
Simon

author
#7 lore raw

PATCH v2 1/1 rockchip: rk3288: correct sdram setting

Quoted 19 lines · Simon Glass

Hi Chris,

Quoted 16 lines · Chris Zhong

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 <[email protected]>

---

Changes in v2:
Modified into a more readable code style(Simon Glass)

Quoted 6 lines
Quoted 2 lines
  arch/arm/mach-rockchip/rk3288/sdram_rk3288.c | 14 +++++++-------
  1 file changed, 7 insertions(+), 7 deletions(-)
Acked-by Simon Glass <[email protected]>

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.

Quoted 49 lines
Quoted 42 lines
arch/arm/mach-rockchip/rk3288/sdram_rk3288.c +7 -8
index e9e2211..17daeca 100644
@@ -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;
 
--
Quoted 2 lines

1.9.1

Regards,
Simon

#8 lore raw

PATCH v2 1/1 rockchip: rk3288: correct sdram setting

In reply to Chris Zhong
Quoted 10 lines · Chris Zhong

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

Quoted 8 lines
Signed-off-by Chris Zhong <[email protected]>

---

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 <[email protected]>
#9 lore raw

PATCH v2 1/1 rockchip: rk3288: correct sdram setting

Quoted 14 lines · Simon Glass
Quoted 13 lines · Chris Zhong

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 <[email protected]>

---

Quoted 7 lines
Quoted 5 lines

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 <[email protected]>

Applied to u-boot-rockchip, thanks!