[U-Boot] [PATCH V2 1/3] sunxi: Use clrsetbits_le32 instead of multiple instruction

[U-Boot] [PATCH V2 1/3] sunxi: Use clrsetbits_le32 instead of multiple instruction

[Michael Trimarchi]

Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
---

V1->V2: none

---
 arch/arm/mach-sunxi/dram_sun8i_a33.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/arch/arm/mach-sunxi/dram_sun8i_a33.c b/arch/arm/mach-sunxi/dram_sun8i_a33.c
index 1da2727f98..83212aaddf 100644
--- a/arch/arm/mach-sunxi/dram_sun8i_a33.c
+++ b/arch/arm/mach-sunxi/dram_sun8i_a33.c
@@ -148,12 +148,8 @@ static void auto_set_timing_para(struct dram_para *para)
 	reg_val = (tcksrx << 24) | (tcksre << 16) | (tckesr << 8) | (tcke << 0);
 	writel(reg_val, &mctl_ctl->dramtmg5);
 	/* Set two rank timing and exit self-refresh timing */
-	reg_val = readl(&mctl_ctl->dramtmg8);
-	reg_val &= ~(0xff << 8);
-	reg_val &= ~(0xff << 0);
-	reg_val |= (0x33 << 8);
-	reg_val |= (0x8 << 0);
-	writel(reg_val, &mctl_ctl->dramtmg8);
+	clrsetbits_le32(&mctl_ctl->dramtmg8, (0xff << 8) | (0xff << 0),
+			0x33 << 8 | (0x8 << 0));
 	/* Set phy interface time */
 	reg_val = (0x2 << 24) | (t_rdata_en << 16) | (0x1 << 8)
 			| (wr_latency << 0);
-- 
2.17.1

[U-Boot] [PATCH V2 2/3] sunxi: Don't change the rank in dram size detection in A33

[Michael Trimarchi]

Change the size create a glitch in the clken signal on second
bank. According to the ddr manual the clken need to be sent
accros the reset signal coming the cpu. The rank is calculated
just before this function is called and the mctl_set_cr should
not change this value anymore

Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
---

V1->V2: Adjust commit description

---
 arch/arm/mach-sunxi/dram_sun8i_a33.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/arch/arm/mach-sunxi/dram_sun8i_a33.c b/arch/arm/mach-sunxi/dram_sun8i_a33.c
index 83212aaddf..d73a93a132 100644
--- a/arch/arm/mach-sunxi/dram_sun8i_a33.c
+++ b/arch/arm/mach-sunxi/dram_sun8i_a33.c
@@ -45,14 +45,12 @@ static void mctl_set_cr(struct dram_para *para)
 
 static void auto_detect_dram_size(struct dram_para *para)
 {
-	u8 orig_rank = para->rank;
 	int rows, columns;
 
 	/* Row detect */
 	para->page_size = 512;
 	para->seq = 1;
 	para->rows = 16;
-	para->rank = 1;
 	mctl_set_cr(para);
 	for (rows = 11 ; rows < 16 ; rows++) {
 		if (mctl_mem_matches(1 << (rows + 9))) /* row-column */
@@ -69,7 +67,6 @@ static void auto_detect_dram_size(struct dram_para *para)
 	}
 
 	para->seq = 0;
-	para->rank = orig_rank;
 	para->rows = rows;
 	para->page_size = 1 << columns;
 	mctl_set_cr(para);
-- 
2.17.1

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

[Michael Trimarchi]

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@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));
 	/* Set phy interface time */
 	reg_val = (0x2 << 24) | (t_rdata_en << 16) | (0x1 << 8)
 			| (wr_latency << 0);
-- 
2.17.1

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

[Philipp Tomsich]

> On 14.02.2019, at 16:58, Michael Trimarchi <michael@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@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@“<< 0”?

Are you configuring the IOs to 1.5V with this write (as the commit message
would suggest)?

> 	/* 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
> https://lists.denx.de/listinfo/u-boot

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

[Michael Nazzareno Trimarchi]

Hi Philipp

On Thu, Feb 14, 2019 at 5:36 PM Philipp Tomsich
<philipp.tomsich@theobroma-systems.com> wrote:
>
>
>
> > On 14.02.2019, at 16:58, Michael Trimarchi <michael@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@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”?
>

I just reduce the code change. Ok, forget to mention in the commit
message that I don't
have documentation of the cpu. So my fix is based on allwinner
architecture comparison and
the fact that it fix my issue. I was having a failure rate of 7% before.

> Are you configuring the IOs to 1.5V with this write (as the commit message
> would suggest)?

No, the model of the memory is LPDDR3 but they can work as DDR3 compatible mode
if they are powered at 1.5V. A33 has not support for LPDDR3 so make no
sense to try
to let them work as LPDDR3. I will update the commit message

Michael
>
> >       /* 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
> > https://lists.denx.de/listinfo/u-boot
>


-- 
| Michael Nazzareno Trimarchi                     Amarula Solutions BV |
| COO  -  Founder                                      Cruquiuskade 47 |
| +31(0)851119172                                 Amsterdam 1018 AM NL |
|                  [`as] http://www.amarulasolutions.com               |

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

[André Przywara]

On 14/02/2019 16:36, Philipp Tomsich wrote:
> 
> 
>> On 14.02.2019, at 16:58, Michael Trimarchi <michael@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@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(?).

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
>> https://lists.denx.de/listinfo/u-boot
> 

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

[Philipp Tomsich]

> On 14.02.2019, at 22:24, André Przywara <andre.przywara@arm.com> wrote:
> 
> On 14/02/2019 16:36, Philipp Tomsich wrote:
>> 
>> 
>>> On 14.02.2019, at 16:58, Michael Trimarchi <michael@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@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@lists.denx.de>
>>> https://lists.denx.de/listinfo/u-boot <https://lists.denx.de/listinfo/u-boot>

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

[Andre Przywara]

On Fri, 15 Feb 2019 11:18:38 +0100
Philipp Tomsich <philipp.tomsich@theobroma-systems.com> wrote:

Hi Philipp,

> > On 14.02.2019, at 22:24, André Przywara <andre.przywara@arm.com> wrote:
> > 
> > On 14/02/2019 16:36, Philipp Tomsich wrote:  
> >> 
> >>   
> >>> On 14.02.2019, at 16:58, Michael Trimarchi <michael@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@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.

So I was wondering about this as well. Indeed we don't seem to *explicitly* enter or exit Self-Refresh anywhere, but maybe this is done implicitly as part of some training phase?
I might be wrong about this, but in some DDR3 documentation I see that changing the DLL state is connected to self refresh, and enabling the DLL is required as part of the initialisation? So is the DRAMC somehow triggering a self refresh exit "magically"? If I understand this correctly, our ranking test does reset the controller?

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

So I was looking at this:
https://www.xilinx.com/html_docs/registers/ug1087/ddrc___dramtmg8.html
and tXSDLL is bits [14:8], while the non-DLL tXS is bits [6:0].

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

I don't see us touching pwrctl for the A33 (but for the A23).

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

All I have is a multimeter and no A33 ;-)

Thanks for providing some background!

Cheers,
Andre.


> 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@lists.denx.de>
> >>> https://lists.denx.de/listinfo/u-boot <https://lists.denx.de/listinfo/u-boot>  

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

[Philipp Tomsich]

Dipl.-Ing. Dr.techn. Philipp Tomsich
Theobroma Systems Design und Consulting GmbH
Seestadtstrasse 27 (Aspern IQ), A-1220 Wien, Austria
Phone: +43 1 2369893-401, Fax: +43 1 2369893-9-401
Cell phone: +43 664 8346109
http://www.theobroma-systems.com <http://www.theobroma-systems.com/>




> On 15.02.2019, at 12:31, Andre Przywara <andre.przywara@arm.com> wrote:
> 
> On Fri, 15 Feb 2019 11:18:38 +0100
> Philipp Tomsich <philipp.tomsich@theobroma-systems.com> wrote:
> 
> Hi Philipp,
> 
>>> On 14.02.2019, at 22:24, André Przywara <andre.przywara@arm.com> wrote:
>>> 
>>> On 14/02/2019 16:36, Philipp Tomsich wrote:  
>>>> 
>>>> 
>>>>> On 14.02.2019, at 16:58, Michael Trimarchi <michael@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@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.
> 
> So I was wondering about this as well. Indeed we don't seem to *explicitly* enter or exit Self-Refresh anywhere, but maybe this is done implicitly as part of some training phase?
> I might be wrong about this, but in some DDR3 documentation I see that changing the DLL state is connected to self refresh, and enabling the DLL is required as part of the initialisation? So is the DRAMC somehow triggering a self refresh exit "magically"? If I understand this correctly, our ranking test does reset the controller?

I’d need to reread JESD79 (a much more relaxing read than the daily news),
to figure out whether there’s any valid reason to play with self-refresh during
the training.

>> 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.
> 
> So I was looking at this:
> https://www.xilinx.com/html_docs/registers/ug1087/ddrc___dramtmg8.html
> and tXSDLL is bits [14:8], while the non-DLL tXS is bits [6:0].
> 
>> 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).
> 
> I don't see us touching pwrctl for the A33 (but for the A23).
> 
>> 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…
> 
> All I have is a multimeter and no A33 ;-)

You are aware that was more of a philosophical rambling and not intended
as a request to you…
However, I am way more surprised that none of the parties having money
riding on their A33 designs has gone through the required effort.

Cheers,
Philipp.

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

[Michael Nazzareno Trimarchi]

Hi all

On Fri, Feb 15, 2019 at 12:40 PM Philipp Tomsich
<philipp.tomsich@theobroma-systems.com> wrote:
>
>
>
> Dipl.-Ing. Dr.techn. Philipp Tomsich
> Theobroma Systems Design und Consulting GmbH
> Seestadtstrasse 27 (Aspern IQ), A-1220 Wien, Austria
> Phone: +43 1 2369893-401, Fax: +43 1 2369893-9-401
> Cell phone: +43 664 8346109
> http://www.theobroma-systems.com
>
>
>
>
> On 15.02.2019, at 12:31, Andre Przywara <andre.przywara@arm.com> wrote:
>
> On Fri, 15 Feb 2019 11:18:38 +0100
> Philipp Tomsich <philipp.tomsich@theobroma-systems.com> wrote:
>
> Hi Philipp,
>
> On 14.02.2019, at 22:24, André Przywara <andre.przywara@arm.com> wrote:
>
> On 14/02/2019 16:36, Philipp Tomsich wrote:
>
>
>
> On 14.02.2019, at 16:58, Michael Trimarchi <michael@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
>

I will fix the commit

> Signed-off-by: Michael Trimarchi <michael@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(?).
>

Seems that affect the initialization sequence.

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

The comment is the original one

> 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.
>
>
> So I was wondering about this as well. Indeed we don't seem to *explicitly* enter or exit Self-Refresh anywhere, but maybe this is done implicitly as part of some training phase?
> I might be wrong about this, but in some DDR3 documentation I see that changing the DLL state is connected to self refresh, and enabling the DLL is required as part of the initialisation? So is the DRAMC somehow triggering a self refresh exit "magically"? If I understand this correctly, our ranking test does reset the controller?
>
>
> I’d need to reread JESD79 (a much more relaxing read than the daily news),
> to figure out whether there’s any valid reason to play with self-refresh during
> the training.
>
> 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.
>

What is the best option here? create a better commit or try to go in deep.

>
> So I was looking at this:
> https://www.xilinx.com/html_docs/registers/ug1087/ddrc___dramtmg8.html
> and tXSDLL is bits [14:8], while the non-DLL tXS is bits [6:0].
>
> 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).
>
>
> I don't see us touching pwrctl for the A33 (but for the A23).
>
> 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…

Nice to have but It's not my case. The best for me is update the commit
message and give more info there. Add the memory components and the layout of my
setup, ask to test other boards before get included

Michael

>
>
> All I have is a multimeter and no A33 ;-)
>
>
> You are aware that was more of a philosophical rambling and not intended
> as a request to you…
> However, I am way more surprised that none of the parties having money
> riding on their A33 designs has gone through the required effort.
>
> Cheers,
> Philipp.
>


-- 
| Michael Nazzareno Trimarchi                     Amarula Solutions BV |
| COO  -  Founder                                      Cruquiuskade 47 |
| +31(0)851119172                                 Amsterdam 1018 AM NL |
|                  [`as] http://www.amarulasolutions.com               |

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

[Andre Przywara]

On Fri, 15 Feb 2019 17:01:40 +0100
Michael Nazzareno Trimarchi <michael@amarulasolutions.com> wrote:

> Hi all
> 
> On Fri, Feb 15, 2019 at 12:40 PM Philipp Tomsich
> <philipp.tomsich@theobroma-systems.com> wrote:
> >
> >
> >
> > Dipl.-Ing. Dr.techn. Philipp Tomsich
> > Theobroma Systems Design und Consulting GmbH
> > Seestadtstrasse 27 (Aspern IQ), A-1220 Wien, Austria
> > Phone: +43 1 2369893-401, Fax: +43 1 2369893-9-401
> > Cell phone: +43 664 8346109
> > http://www.theobroma-systems.com
> >
> >
> >
> >
> > On 15.02.2019, at 12:31, Andre Przywara <andre.przywara@arm.com> wrote:
> >
> > On Fri, 15 Feb 2019 11:18:38 +0100
> > Philipp Tomsich <philipp.tomsich@theobroma-systems.com> wrote:
> >
> > Hi Philipp,
> >
> > On 14.02.2019, at 22:24, André Przywara <andre.przywara@arm.com> wrote:
> >
> > On 14/02/2019 16:36, Philipp Tomsich wrote:
> >
> >
> >
> > On 14.02.2019, at 16:58, Michael Trimarchi <michael@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
> >  
> 
> I will fix the commit
> 
> > Signed-off-by: Michael Trimarchi <michael@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(?).
> >  
> 
> Seems that affect the initialization sequence.
> 
> >
> > 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.  
> 
> The comment is the original one
> 
> > 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.
> >
> >
> > So I was wondering about this as well. Indeed we don't seem to *explicitly* enter or exit Self-Refresh anywhere, but maybe this is done implicitly as part of some training phase?
> > I might be wrong about this, but in some DDR3 documentation I see that changing the DLL state is connected to self refresh, and enabling the DLL is required as part of the initialisation? So is the DRAMC somehow triggering a self refresh exit "magically"? If I understand this correctly, our ranking test does reset the controller?
> >
> >
> > I’d need to reread JESD79 (a much more relaxing read than the daily news),
> > to figure out whether there’s any valid reason to play with self-refresh during
> > the training.
> >
> > 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.
> >  
> 
> What is the best option here? create a better commit or try to go in deep.

Well, we don't need to get this out of hand:
1) This change affects all A33 systems, unconditionally. We would need confirmation from owners of those other systems that it still works for them.
2) Yes, the commit message should be changed. Please state the issue you have seen, maybe roughly summarise our guesses here. Then mention that this changes the DRAM init part for all A33 users.

Cheers,
Andre.

> 
> >
> > So I was looking at this:
> > https://www.xilinx.com/html_docs/registers/ug1087/ddrc___dramtmg8.html
> > and tXSDLL is bits [14:8], while the non-DLL tXS is bits [6:0].
> >
> > 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).
> >
> >
> > I don't see us touching pwrctl for the A33 (but for the A23).
> >
> > 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…  
> 
> Nice to have but It's not my case. The best for me is update the commit
> message and give more info there. Add the memory components and the layout of my
> setup, ask to test other boards before get included
> 
> Michael
> 
> >
> >
> > All I have is a multimeter and no A33 ;-)
> >
> >
> > You are aware that was more of a philosophical rambling and not intended
> > as a request to you…
> > However, I am way more surprised that none of the parties having money
> > riding on their A33 designs has gone through the required effort.
> >
> > Cheers,
> > Philipp.
> >  
> 
> 

[U-Boot] [PATCH V2 1/3] sunxi: Use clrsetbits_le32 instead of multiple instruction

[Jagan Teki]

On Thu, Feb 14, 2019 at 9:28 PM Michael Trimarchi
<michael@amarulasolutions.com> wrote:
>
> Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
> ---
>
> V1->V2: none
>
> ---
>  arch/arm/mach-sunxi/dram_sun8i_a33.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/arch/arm/mach-sunxi/dram_sun8i_a33.c b/arch/arm/mach-sunxi/dram_sun8i_a33.c
> index 1da2727f98..83212aaddf 100644
> --- a/arch/arm/mach-sunxi/dram_sun8i_a33.c
> +++ b/arch/arm/mach-sunxi/dram_sun8i_a33.c
> @@ -148,12 +148,8 @@ static void auto_set_timing_para(struct dram_para *para)
>         reg_val = (tcksrx << 24) | (tcksre << 16) | (tckesr << 8) | (tcke << 0);
>         writel(reg_val, &mctl_ctl->dramtmg5);
>         /* Set two rank timing and exit self-refresh timing */
> -       reg_val = readl(&mctl_ctl->dramtmg8);
> -       reg_val &= ~(0xff << 8);
> -       reg_val &= ~(0xff << 0);

these are clr

> -       reg_val |= (0x33 << 8);
> -       reg_val |= (0x8 << 0);

these are set

> -       writel(reg_val, &mctl_ctl->dramtmg8);
> +       clrsetbits_le32(&mctl_ctl->dramtmg8, (0xff << 8) | (0xff << 0),
> +                       0x33 << 8 | (0x8 << 0));