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

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

[Icenowy Zheng]

2017?1?6? 06:37? Maxime Ripard <maxime.ripard@free-electrons.com>???
>
> 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? 

V3s DRAM seems to be 16-bit.

However, boot0 has this bit set, and without this bit, it cannot work.

According to Jens' guess (only guess), this may be something more like full-width and half-width.

>
> > +#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 
>
> -- 
> You received this message because you are subscribed to the Google Groups "linux-sunxi" group. 
> To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe at googlegroups.com. 
> For more options, visit https://groups.google.com/d/optout. 

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

[Maxime Ripard]

On Fri, Jan 06, 2017 at 06:55:05AM +0800, Icenowy Zheng wrote:
> > > + ?????? MCTL_CR_32BIT /* fixme, thats wrong but what boot0 does */ | 
> >
> > What's wrong about it? 
> 
> V3s DRAM seems to be 16-bit.
> 
> However, boot0 has this bit set, and without this bit, it cannot work.
> 
> According to Jens' guess (only guess), this may be something more
> like full-width and half-width.

Ok. Please put that in the comments then.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170109/f2f0cf74/attachment.sig>

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

[Andre Przywara]

Hi,

On 05/01/17 22:55, Icenowy Zheng wrote:
> 
> 2017?1?6? 06:37? Maxime Ripard <maxime.ripard@free-electrons.com>???
>>
>> 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. 

Out of curiosity, how did you came up with those values?
It would be good if this was mentioned in the commit log.

>>> 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. 

I agree to that. If I got this correctly, the existing parameters here
are actually DRAM chip specific. It just happens that we use DDR3-1333
parameters for all boards so far. Most A64 boards for instance sport
DDR3-1600 capable chips, also we will need adjustments for the LPDDR3
chips used on the SoPine and Pinebook. So it would be good to address
this properly.

Philipp has a quite elaborate rework to fix this and ease the way to
allow board specific tunings of those parameters:

https://github.com/ptomsich/u-boot/commit/4ae474c756c3208a3bfaf36ed6f1850d46b07429

as part of that branch:
https://github.com/ptomsich/u-boot/commits/a64uQ7-spl-wip

At some point in the future I wanted to clean this up and upstream it, I
am wondering if you can take a look at this now?

Cheers,
Andre.

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

[Icenowy Zheng]

2017?1?9? ??6:30? Andre Przywara <andre.przywara@arm.com>???
>
> Hi, 
>
> On 05/01/17 22:55, Icenowy Zheng wrote: 
> > 
> > 2017?1?6? 06:37? Maxime Ripard <maxime.ripard@free-electrons.com>??? 
> >> 
> >> 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. 
>
> Out of curiosity, how did you came up with those values? 
> It would be good if this was mentioned in the commit log. 
>
> >>> 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. 
>
> I agree to that. If I got this correctly, the existing parameters here 
> are actually DRAM chip specific. It just happens that we use DDR3-1333 
> parameters for all boards so far. Most A64 boards for instance sport 
> DDR3-1600 capable chips, also we will need adjustments for the LPDDR3 
> chips used on the SoPine and Pinebook. So it would be good to address 
> this properly. 
>
> Philipp has a quite elaborate rework to fix this and ease the way to 
> allow board specific tunings of those parameters: 
>
> https://github.com/ptomsich/u-boot/commit/4ae474c756c3208a3bfaf36ed6f1850d46b07429 
>
> as part of that branch: 
> https://github.com/ptomsich/u-boot/commits/a64uQ7-spl-wip 

I now think it worth to do a gaint rework for H3 DRAM code...

This patch may need to be splited, and supports of A64, H5, R40 are also worthful to be part of the refactor.

>
> At some point in the future I wanted to clean this up and upstream it, I 
> am wondering if you can take a look at this now? 
>
> Cheers, 
> Andre. 

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

[Icenowy Zheng]

09.01.2017, 18:01, "Maxime Ripard" <maxime.ripard@free-electrons.com>:
> On Fri, Jan 06, 2017 at 06:55:05AM +0800, Icenowy Zheng wrote:
>> ?> > + ?????? MCTL_CR_32BIT /* fixme, thats wrong but what boot0 does */ |
>> ?>
>> ?> What's wrong about it?
>>
>> ?V3s DRAM seems to be 16-bit.
>>
>> ?However, boot0 has this bit set, and without this bit, it cannot work.
>>
>> ?According to Jens' guess (only guess), this may be something more
>> ?like full-width and half-width.
>
> Ok. Please put that in the comments then.

Now I think it's not only guess.

See dram_sun8i_a33.h, which has MCTL_CR_BUSW8 = 0 << 12 and MCTL_CR_BUSW16 = 1 << 12.

>
> Thanks!
> Maxime
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com