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