[U-Boot] [PATCH V2 3/3] sunxi: Fix A33 memory initialization

[Philipp Tomsich]

> On 14.02.2019, at 22:24, André Przywara <andre.przywara at arm.com> wrote:
> 
> On 14/02/2019 16:36, Philipp Tomsich wrote:
>> 
>> 
>>> On 14.02.2019, at 16:58, Michael Trimarchi <michael at amarulasolutions.com> wrote:
>>> 
>>> Set two rank timing and exit self-refresh timing seems not done
>>> properly. We know use the same write that we are using
>>> on H5 silicon. Tested was done in A33 allwinner cpu, dual rank
>>> connection connected with two MT41K512M16HA-125:A memory model.
>>> Memory is configure as DDR3 1.5V
>>> 
>>> Signed-off-by: Michael Trimarchi <michael at amarulasolutions.com>
>>> ---
>>> 
>>> V1->V2: adjust commit message
>>> 
>>> ---
>>> arch/arm/mach-sunxi/dram_sun8i_a33.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>> 
>>> diff --git a/arch/arm/mach-sunxi/dram_sun8i_a33.c b/arch/arm/mach-sunxi/dram_sun8i_a33.c
>>> index d73a93a132..355fe30aba 100644
>>> --- a/arch/arm/mach-sunxi/dram_sun8i_a33.c
>>> +++ b/arch/arm/mach-sunxi/dram_sun8i_a33.c
>>> @@ -146,7 +146,7 @@ static void auto_set_timing_para(struct dram_para *para)
>>> 	writel(reg_val, &mctl_ctl->dramtmg5);
>>> 	/* Set two rank timing and exit self-refresh timing */
>>> 	clrsetbits_le32(&mctl_ctl->dramtmg8, (0xff << 8) | (0xff << 0),
>>> -			0x33 << 8 | (0x8 << 0));
>>> +			0x33 << 8 | (0x10 << 0));
>> 
>> How exactly does the change in constants match up with the commit message?
>> What is the field at “<< 0”?
> 
> From looking at the ZynqMP register guide, which is reported to be close
> to the various Allwinner DRAM controllers, bits [6:0] are tXS: Self
> refresh exit delay. Increasing that should not hurt, and if I understand
> it correctly it only affects the time after the self-refresh *exit*,
> which happens only after (re-)initialisation(?).

The “set two rank timing” comment doesn’t match our expectation that
this will be self-refresh timings, as on the ZynqMP or the A80.
Self-refresh only happens, if someone puts the DRAM into self-refresh
(i.e. “suspend to DRAM”) and then resumes it.  I don’t see a reference
to the error occurring from this.

That said, if the field indeed was the exit self-refresh timing, this would
usually be tXSDLL, encoded as tXSDLL/32. JESD79 specifies tESXR as
tXSDLL (which in turn is tDLLK(min)), which is 512 nCK always: 0x10
would be 512 and 0x08 would only be 256… then again, this should only
matter when exiting self-refresh.

Note that the ‘auto-enter self-refresh’ functionality is controlled through
BIT(3) in PWRCTL on the A33, according to Allwinner’s basic_loader.
I don’t know whether that may be turned on by the driver (i.e. didn’t
check).

OTOH, when working with an underdocumented DRAM register layout
and once one has an educated guess at what register/setting may be
affected, a DRAM protocol analyzer can provide conclusive answers
by looking at the differences in behaviour with 0x8 and 0x10 in that
bitfield…

Just my 2 cents.

> But it should be noted that this unconditionally affects all A33 boards.
> 
>> Are you configuring the IOs to 1.5V with this write (as the commit message
>> would suggest)?
> 
> I think the voltage is totally unrelated here, this is a pure timing
> register.
> 
> Cheers,
> Andre.
> 
> 
>> 
>>> 	/* Set phy interface time */
>>> 	reg_val = (0x2 << 24) | (t_rdata_en << 16) | (0x1 << 8)
>>> 			| (wr_latency << 0);
>>> -- 
>>> 2.17.1
>>> 
>>> _______________________________________________
>>> U-Boot mailing list
>>> U-Boot at lists.denx.de <mailto:U-Boot at lists.denx.de>
>>> https://lists.denx.de/listinfo/u-boot <https://lists.denx.de/listinfo/u-boot>