Re: [U-Boot] [RFC PATCH 1/4] sunxi: add DDR2 support to H3-like DRAM controller

5 Jan 2017

On Thu, Dec 29, 2016 at 03:00:58AM +0800, Icenowy Zheng wrote:
...
H3-like DRAM controller needs some special code to operate a DDR2 DRAM
chip. Add the logic to probe such a chip.
As there's no commercial boards available now with H3 and DDR2 DRAM, the
patch is developed and tested on a V3s chip, which has in-package DDR2
DRAM.
Signed-off-by: Icenowy Zheng icenowy@aosc.xyz
It would have been great if your previous patch renaming the H3 symbol
was part of that serie.
...

arch/arm/mach-sunxi/dram_sun8i_h3.c | 114 ++++++++++++++++++++++++++++++++++--
 board/sunxi/Kconfig                 |  11 ++++
 2 files changed, 120 insertions(+), 5 deletions(-)

diff --git a/arch/arm/mach-sunxi/dram_sun8i_h3.c b/arch/arm/mach-sunxi/dram_sun8i_h3.c
index 8e2527dee1..a48320e01c 100644
--- a/arch/arm/mach-sunxi/dram_sun8i_h3.c
+++ b/arch/arm/mach-sunxi/dram_sun8i_h3.c
@@ -22,6 +22,9 @@ struct dram_para {
   u8 bus_width;
   u8 dual_rank;
   u8 row_bits;
+#ifdef CONFIG_SUNXI_H3_DRAM_DDR2

u8 bank_bits;

+#endif
 };
static inline int ns_to_t(int nanoseconds)
@@ -136,36 +139,77 @@ static void mctl_set_timing_params(struct dram_para *para)
   struct sunxi_mctl_ctl_reg * const mctl_ctl =
   		(struct sunxi_mctl_ctl_reg *)SUNXI_DRAM_CTL0_BASE;
+#ifndef CONFIG_SUNXI_H3_DRAM_DDR2
   u8 tccd		= 2;
+#else

u8 tccd		= 1;

+#endif
   u8 tfaw		= ns_to_t(50);
+#ifndef CONFIG_SUNXI_H3_DRAM_DDR2
   u8 trrd		= max(ns_to_t(10), 4);
   u8 trcd		= ns_to_t(15);
   u8 trc		= ns_to_t(53);
   u8 txp		= max(ns_to_t(8), 3);
   u8 twtr		= max(ns_to_t(8), 4);
   u8 trtp		= max(ns_to_t(8), 4);
+#else

u8 trrd		= max(ns_to_t(10), 2);
u8 trcd		= ns_to_t(20);
u8 trc		= ns_to_t(65);
u8 txp		= 2;
u8 twtr		= max(ns_to_t(8), 2);
u8 trtp		= max(ns_to_t(8), 2);

+#endif
   u8 twr		= max(ns_to_t(15), 3);
   u8 trp		= ns_to_t(15);
+#ifndef CONFIG_SUNXI_H3_DRAM_DDR2
   u8 tras		= ns_to_t(38);
+#else

u8 tras		= ns_to_t(45);

+#endif
   u16 trefi	= ns_to_t(7800) / 32;
+#ifndef CONFIG_SUNXI_H3_DRAM_DDR2
   u16 trfc	= ns_to_t(350);
+#else

u16 trfc	= ns_to_t(328);

+#endif
u8 tmrw		= 0;
+#ifndef CONFIG_SUNXI_H3_DRAM_DDR2
   u8 tmrd		= 4;
+#else

u8 tmrd		= 2;

+#endif
   u8 tmod		= 12;
   u8 tcke		= 3;
   u8 tcksrx	= 5;
   u8 tcksre	= 5;
   u8 tckesr	= 4;
+#ifndef CONFIG_SUNXI_H3_DRAM_DDR2
   u8 trasmax	= 24;
+#else

u8 trasmax	= 27;

+#endif
Can't that be moved into a structure that would have different
declaration, this is barely readable.
...
+#ifndef CONFIG_SUNXI_H3_DRAM_DDR2
   u8 tcl		= 6; /* CL 12 */
   u8 tcwl		= 4; /* CWL 8 */
   u8 t_rdata_en	= 4;
   u8 wr_latency	= 2;



+#else

u8 tcl		= 3; /* CL 12 */
u8 tcwl		= 3; /* CWL 8 */

Aren't the comments supposed to change?
...

u8 t_rdata_en	= 1;
u8 wr_latency	= 1;

+#endif



+#ifndef CONFIG_SUNXI_H3_DRAM_DDR2
   u32 tdinit0	= (500 * CONFIG_DRAM_CLK) + 1;		/* 500us */
   u32 tdinit1	= (360 * CONFIG_DRAM_CLK) / 1000 + 1;	/* 360ns */
+#else

u32 tdinit0	= (400 * CONFIG_DRAM_CLK) + 1;		/* 400us */
u32 tdinit1	= (500 * CONFIG_DRAM_CLK) / 1000 + 1;	/* 500ns */

+#endif
   u32 tdinit2	= (200 * CONFIG_DRAM_CLK) + 1;		/* 200us */
   u32 tdinit3	= (1 * CONFIG_DRAM_CLK) + 1;		/* 1us */
@@ -174,9 +218,15 @@ static void mctl_set_timing_params(struct dram_para *para)
   u8 trd2wr	= tcl + 2 + 1 - tcwl;	/* RL + BL / 2 + 2 - WL */
/* set mode register */
+#ifndef CONFIG_SUNXI_H3_DRAM_DDR2
   writel(0x1c70, &mctl_ctl->mr[0]);	/* CL=11, WR=12 */
   writel(0x40, &mctl_ctl->mr[1]);
   writel(0x18, &mctl_ctl->mr[2]);		/* CWL=8 */
+#else

writel(0x263, &mctl_ctl->mr[0]);	/* CL=11, WR=12 */
writel(0x4, &mctl_ctl->mr[1]);
writel(0x0, &mctl_ctl->mr[2]);		/* CWL=8 */

Ditto
...
+#endif
   writel(0x0, &mctl_ctl->mr[3]);
/* set DRAM timing */
@@ -244,7 +294,12 @@ static void mctl_zq_calibration(struct dram_para *para)
writel(0x0a0a0a0a, &mctl_ctl->zqdr[2]);



for (i = 0; i < 6; i++) {



+#ifndef CONFIG_SUNXI_H3_DRAM_DDR2

for (i = 0; i < 6; i++)



+#else

for (i = 0; i < 4; i++)



+#endif

{



This should also be put into that structure or a define.
...
	u8 zq = (CONFIG_DRAM_ZQ >> (i * 4)) & 0xf;

	writel((zq << 20) | (zq << 16) | (zq << 12) |

@@ -266,7 +321,9 @@ static void mctl_zq_calibration(struct dram_para *para)
writel((zq_val[1] << 16) | zq_val[0], &mctl_ctl->zqdr[0]);
writel((zq_val[3] << 16) | zq_val[2], &mctl_ctl->zqdr[1]);

+#ifndef CONFIG_SUNXI_H3_DRAM_DDR2
   	writel((zq_val[5] << 16) | zq_val[4], &mctl_ctl->zqdr[2]);
+#endif
   }
 }
@@ -275,8 +332,16 @@ static void mctl_set_cr(struct dram_para *para)
   struct sunxi_mctl_com_reg * const mctl_com =
   		(struct sunxi_mctl_com_reg *)SUNXI_DRAM_COM_BASE;

writel(MCTL_CR_BL8 | MCTL_CR_2T | MCTL_CR_DDR3 | MCTL_CR_INTERLEAVED |
      MCTL_CR_EIGHT_BANKS | MCTL_CR_BUS_WIDTH(para->bus_width) |




writel(MCTL_CR_BL8 | MCTL_CR_2T |

+#ifndef CONFIG_SUNXI_H3_DRAM_DDR2

      MCTL_CR_DDR3 | MCTL_CR_EIGHT_BANKS |


      MCTL_CR_BUS_WIDTH(para->bus_width) |



+#else

      (para->bank_bits == 3 ? MCTL_CR_EIGHT_BANKS : MCTL_CR_FOUR_BANKS) |


      MCTL_CR_DDR2 |



You can use a variable and then or is based on whether that option is
enabled or not, this will be easier to read.
...

      MCTL_CR_32BIT /* fixme, thats wrong but what boot0 does */ |



What's wrong about it?
...
+#endif

      MCTL_CR_INTERLEAVED |



...
      (para->dual_rank ? MCTL_CR_DUAL_RANK : MCTL_CR_SINGLE_RANK) |
      MCTL_CR_PAGE_SIZE(para->page_size) |
      MCTL_CR_ROW_BITS(para->row_bits), &mctl_com->cr);

@@ -380,7 +445,10 @@ static int mctl_channel_init(struct dram_para *para)
mctl_zq_calibration(para);

mctl_phy_init(PIR_PLLINIT | PIR_DCAL | PIR_PHYRST | PIR_DRAMRST |


mctl_phy_init(PIR_PLLINIT | PIR_DCAL | PIR_PHYRST |

+#ifndef CONFIG_SUNXI_H3_DRAM_DDR2

      PIR_DRAMRST |



+#endif
   	      PIR_DRAMINIT | PIR_QSGATE);
/* detect ranks and bus width */
@@ -435,12 +503,29 @@ static void mctl_auto_detect_dram_size(struct dram_para *para)
   /* detect row address bits */
   para->page_size = 512;
   para->row_bits = 16;
+#ifdef CONFIG_SUNXI_H3_DRAM_DDR2

para->bank_bits = 2;

+#endif
   mctl_set_cr(para);
for (para->row_bits = 11; para->row_bits < 16; para->row_bits++)
+#ifndef CONFIG_SUNXI_H3_DRAM_DDR2
   	if (mctl_mem_matches((1 << (para->row_bits + 3)) * para->page_size))
+#else

if (mctl_mem_matches((1 << (para->row_bits + para->bank_bits)) * para->page_size))



+#endif
Can't you use bank_bits all the time?
...
	break;


+#ifndef CONFIG_SUNXI_H3_DRAM_DDR2

/* detect bank address bits */
para->bank_bits = 3;
mctl_set_cr(para);

for (para->bank_bits = 2; para->bank_bits < 3; para->bank_bits++)
if (mctl_mem_matches((1 << para->bank_bits) * para->page_size))


	break;



+#endif

/* detect page size */
 para->page_size = 8192;
 mctl_set_cr(para);

@@ -458,11 +543,19 @@ unsigned long sunxi_dram_init(void)
   		(struct sunxi_mctl_ctl_reg *)SUNXI_DRAM_CTL0_BASE;
struct dram_para para = {
+#ifndef CONFIG_SUNXI_H3_DRAM_DDR2
   	.read_delays = 0x00007979,	/* dram_tpr12 */
   	.write_delays = 0x6aaa0000,	/* dram_tpr11 */
+#else

.read_delays = 0x00007878,	/* dram_tpr12 */


.write_delays = 0x6a440000,	/* dram_tpr11 */



+#endif
   	.dual_rank = 0,
   	.bus_width = 32,
   	.row_bits = 15,
+#ifdef CONFIG_SUNXI_H3_DRAM_DDR2

.bank_bits = 3,



+#endif
   	.page_size = 4096,
   };
@@ -477,7 +570,13 @@ unsigned long sunxi_dram_init(void)
   udelay(1);
/* odt delay */
+#ifndef CONFIG_SUNXI_H3_DRAM_DDR2
   writel(0x0c000400, &mctl_ctl->odtcfg);
+#else

writel(0x04000400, &mctl_ctl->odtcfg);

clrbits_le32(&mctl_ctl->pgcr[2], (1 << 13));

+#endif
Some defines would be great.
Thanks!
Maxime
-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

    