[PATCH 0/3] sunxi: assorted fixes to DRAM and clock init

[PATCH 0/3] sunxi: assorted fixes to DRAM and clock init

[Andre Przywara]

Some assorted fixes for bugs I found while debugging and researching
the boot and DRAM init process on some chips.
Patch 1 and 2 fix bugs hidden by the fact that the code in question
would ever be compiled for arm64 only, but for an experiment I compiled
the A133 SPL for 32-bit, which revealed those issues.
Patch 3 fixes the mode register setup for the H616 boards with LPDDR3,
this was found when wrapping the mode register accesses into some
functions, for a later cleanup patch.

Please have a look!

Cheers,
Andre

Andre Przywara (3):
  sunxi: a133: dram: fix data type for address variable
  sunxi: spl: initialise timer before clocks
  sunxi: H616: dram: fix LPDDR3 mode register settings

 arch/arm/mach-sunxi/board.c            |  2 +-
 arch/arm/mach-sunxi/dram_sun50i_a133.c |  2 +-
 arch/arm/mach-sunxi/dram_sun50i_h616.c | 12 ++++++------
 3 files changed, 8 insertions(+), 8 deletions(-)

-- 
2.46.3

[PATCH 1/3] sunxi: a133: dram: fix data type for address variable

[Andre Przywara]

Variables holding addresses are typically using the "long" C type in
U-Boot, to be easily compatible with both 32-bit and 64-bit builds.

The A133 DRAM driver is typically compiled for AArch64, so u64 is the
same type as unsigned long, but that breaks when compiling the DRAM
driver in AArch32 (for some experiments).

Fix the type to make the code more portable.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arch/arm/mach-sunxi/dram_sun50i_a133.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/mach-sunxi/dram_sun50i_a133.c b/arch/arm/mach-sunxi/dram_sun50i_a133.c
index 3a231141168..1496f99624d 100644
--- a/arch/arm/mach-sunxi/dram_sun50i_a133.c
+++ b/arch/arm/mach-sunxi/dram_sun50i_a133.c
@@ -416,7 +416,7 @@ static void mctl_com_init(const struct dram_para *para,
 static void mctl_drive_odt_config(const struct dram_para *para)
 {
 	u32 val;
-	u64 base;
+	ulong base;
 	u32 i;
 
 	/* DX drive */
-- 
2.46.3

Re: [PATCH 1/3] sunxi: a133: dram: fix data type for address variable

[Jernej Škrabec]

Dne sobota, 2. avgust 2025 ob 01:49:16 Srednjeevropski poletni čas je Andre Przywara napisal(a):
> Variables holding addresses are typically using the "long" C type in
> U-Boot, to be easily compatible with both 32-bit and 64-bit builds.
> 
> The A133 DRAM driver is typically compiled for AArch64, so u64 is the
> same type as unsigned long, but that breaks when compiling the DRAM
> driver in AArch32 (for some experiments).
> 
> Fix the type to make the code more portable.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

Reviewed-by: Jernej Skrabec <jernej.skrabec@gmail.com>

Best regards,
Jernej

> ---
>  arch/arm/mach-sunxi/dram_sun50i_a133.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm/mach-sunxi/dram_sun50i_a133.c b/arch/arm/mach-sunxi/dram_sun50i_a133.c
> index 3a231141168..1496f99624d 100644
> --- a/arch/arm/mach-sunxi/dram_sun50i_a133.c
> +++ b/arch/arm/mach-sunxi/dram_sun50i_a133.c
> @@ -416,7 +416,7 @@ static void mctl_com_init(const struct dram_para *para,
>  static void mctl_drive_odt_config(const struct dram_para *para)
>  {
>  	u32 val;
> -	u64 base;
> +	ulong base;
>  	u32 i;
>  
>  	/* DX drive */
> 

[PATCH 2/3] sunxi: spl: initialise timer before clocks

[Andre Przywara]

Recent changes in the H6 clock code added delay() calls into the SPL clock
setup routine, which requires the timers to work. When compiling for
AArch64, we are always using the Arm Generic Timer (aka. arch timer),
which does not require further setup, hence having an empty timer_init()
routine.
However for 32-bit SoCs we use the Allwinner timers, which require some
setup routine, and hence we need timer_init() to be called before
clock_init().

Swap the order of the two calls, to be more robust when compiling the H6
clock code for AArch32 or when using the Allwinner timers for whatever
reason.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arch/arm/mach-sunxi/board.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/mach-sunxi/board.c b/arch/arm/mach-sunxi/board.c
index fb4837c2082..432b1c10f92 100644
--- a/arch/arm/mach-sunxi/board.c
+++ b/arch/arm/mach-sunxi/board.c
@@ -476,8 +476,8 @@ void board_init_f(ulong dummy)
 	/* Enable non-secure access to some peripherals */
 	tzpc_init();
 
-	clock_init();
 	timer_init();
+	clock_init();
 	gpio_init();
 
 	spl_init();
-- 
2.46.3

Re: [PATCH 2/3] sunxi: spl: initialise timer before clocks

[Jernej Škrabec]

Dne sobota, 2. avgust 2025 ob 01:49:17 Srednjeevropski poletni čas je Andre Przywara napisal(a):
> Recent changes in the H6 clock code added delay() calls into the SPL clock
> setup routine, which requires the timers to work. When compiling for
> AArch64, we are always using the Arm Generic Timer (aka. arch timer),
> which does not require further setup, hence having an empty timer_init()
> routine.
> However for 32-bit SoCs we use the Allwinner timers, which require some
> setup routine, and hence we need timer_init() to be called before
> clock_init().
> 
> Swap the order of the two calls, to be more robust when compiling the H6
> clock code for AArch32 or when using the Allwinner timers for whatever
> reason.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  arch/arm/mach-sunxi/board.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm/mach-sunxi/board.c b/arch/arm/mach-sunxi/board.c
> index fb4837c2082..432b1c10f92 100644
> --- a/arch/arm/mach-sunxi/board.c
> +++ b/arch/arm/mach-sunxi/board.c
> @@ -476,8 +476,8 @@ void board_init_f(ulong dummy)
>  	/* Enable non-secure access to some peripherals */
>  	tzpc_init();
>  
> -	clock_init();
>  	timer_init();
> +	clock_init();

I contemplated similar change in past. It works fine for 64-bit architectures,
but I'm unsure for 32-bit. If you take a look at A83t clock code, it uses
sdelay() exactly because timer is not initialized at that time.

So, are you sure that this change has no unwanted side effects?

Best regards,
Jernej

>  	gpio_init();
>  
>  	spl_init();
> 

Re: [PATCH 2/3] sunxi: spl: initialise timer before clocks

[Andre Przywara]

On Mon, 11 Aug 2025 17:34:47 +0200
Jernej Škrabec <jernej.skrabec@gmail.com> wrote:

> Dne sobota, 2. avgust 2025 ob 01:49:17 Srednjeevropski poletni čas je Andre Przywara napisal(a):
> > Recent changes in the H6 clock code added delay() calls into the SPL clock
> > setup routine, which requires the timers to work. When compiling for
> > AArch64, we are always using the Arm Generic Timer (aka. arch timer),
> > which does not require further setup, hence having an empty timer_init()
> > routine.
> > However for 32-bit SoCs we use the Allwinner timers, which require some
> > setup routine, and hence we need timer_init() to be called before
> > clock_init().
> > 
> > Swap the order of the two calls, to be more robust when compiling the H6
> > clock code for AArch32 or when using the Allwinner timers for whatever
> > reason.
> > 
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > ---
> >  arch/arm/mach-sunxi/board.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm/mach-sunxi/board.c b/arch/arm/mach-sunxi/board.c
> > index fb4837c2082..432b1c10f92 100644
> > --- a/arch/arm/mach-sunxi/board.c
> > +++ b/arch/arm/mach-sunxi/board.c
> > @@ -476,8 +476,8 @@ void board_init_f(ulong dummy)
> >  	/* Enable non-secure access to some peripherals */
> >  	tzpc_init();
> >  
> > -	clock_init();
> >  	timer_init();
> > +	clock_init();  
> 
> I contemplated similar change in past. It works fine for 64-bit architectures,
> but I'm unsure for 32-bit. If you take a look at A83t clock code, it uses
> sdelay() exactly because timer is not initialized at that time.
> 
> So, are you sure that this change has no unwanted side effects?

No, I am not ;-)
I was about to test this on some more boards. But I think with the
change in that direction, the only issue would be if the timer_init() code
relies on the clocks being set up already. And since timer_init() is only
*one* rather small function, just doing MMIO accesses, I think the risk is
quite low.

Thanks for having a look anyway, I will report back how the test goes.

Cheers,
Andre

> 
> >  	gpio_init();
> >  
> >  	spl_init();
> >   
> 
> 
> 
> 

Re: [PATCH 2/3] sunxi: spl: initialise timer before clocks

[Jernej Škrabec]

Dne ponedeljek, 11. avgust 2025 ob 17:52:25 Srednjeevropski poletni čas je Andre Przywara napisal(a):
> On Mon, 11 Aug 2025 17:34:47 +0200
> Jernej Škrabec <jernej.skrabec@gmail.com> wrote:
> 
> > Dne sobota, 2. avgust 2025 ob 01:49:17 Srednjeevropski poletni čas je Andre Przywara napisal(a):
> > > Recent changes in the H6 clock code added delay() calls into the SPL clock
> > > setup routine, which requires the timers to work. When compiling for
> > > AArch64, we are always using the Arm Generic Timer (aka. arch timer),
> > > which does not require further setup, hence having an empty timer_init()
> > > routine.
> > > However for 32-bit SoCs we use the Allwinner timers, which require some
> > > setup routine, and hence we need timer_init() to be called before
> > > clock_init().
> > > 
> > > Swap the order of the two calls, to be more robust when compiling the H6
> > > clock code for AArch32 or when using the Allwinner timers for whatever
> > > reason.
> > > 
> > > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > > ---
> > >  arch/arm/mach-sunxi/board.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/arch/arm/mach-sunxi/board.c b/arch/arm/mach-sunxi/board.c
> > > index fb4837c2082..432b1c10f92 100644
> > > --- a/arch/arm/mach-sunxi/board.c
> > > +++ b/arch/arm/mach-sunxi/board.c
> > > @@ -476,8 +476,8 @@ void board_init_f(ulong dummy)
> > >  	/* Enable non-secure access to some peripherals */
> > >  	tzpc_init();
> > >  
> > > -	clock_init();
> > >  	timer_init();
> > > +	clock_init();  
> > 
> > I contemplated similar change in past. It works fine for 64-bit architectures,
> > but I'm unsure for 32-bit. If you take a look at A83t clock code, it uses
> > sdelay() exactly because timer is not initialized at that time.
> > 
> > So, are you sure that this change has no unwanted side effects?
> 
> No, I am not ;-)
> I was about to test this on some more boards. But I think with the
> change in that direction, the only issue would be if the timer_init() code
> relies on the clocks being set up already. And since timer_init() is only
> *one* rather small function, just doing MMIO accesses, I think the risk is
> quite low.

As long as clock function doesn't need to enable bus or module clock,
everything should be fine. Quick check didn't show any such code so
it is probably safe to switch.

Best regards,
Jernej

> 
> Thanks for having a look anyway, I will report back how the test goes.
> 
> Cheers,
> Andre
> 
> > 
> > >  	gpio_init();
> > >  
> > >  	spl_init();
> > >   
> > 
> > 
> > 
> > 
> 
> 

Re: [PATCH 2/3] sunxi: spl: initialise timer before clocks

[Andre Przywara]

On Mon, 11 Aug 2025 16:52:25 +0100
Andre Przywara <andre.przywara@arm.com> wrote:

Hi,

> On Mon, 11 Aug 2025 17:34:47 +0200
> Jernej Škrabec <jernej.skrabec@gmail.com> wrote:
> 
> > Dne sobota, 2. avgust 2025 ob 01:49:17 Srednjeevropski poletni čas je Andre Przywara napisal(a):  
> > > Recent changes in the H6 clock code added delay() calls into the SPL clock
> > > setup routine, which requires the timers to work. When compiling for
> > > AArch64, we are always using the Arm Generic Timer (aka. arch timer),
> > > which does not require further setup, hence having an empty timer_init()
> > > routine.
> > > However for 32-bit SoCs we use the Allwinner timers, which require some
> > > setup routine, and hence we need timer_init() to be called before
> > > clock_init().
> > > 
> > > Swap the order of the two calls, to be more robust when compiling the H6
> > > clock code for AArch32 or when using the Allwinner timers for whatever
> > > reason.
> > > 
> > > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > > ---
> > >  arch/arm/mach-sunxi/board.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/arch/arm/mach-sunxi/board.c b/arch/arm/mach-sunxi/board.c
> > > index fb4837c2082..432b1c10f92 100644
> > > --- a/arch/arm/mach-sunxi/board.c
> > > +++ b/arch/arm/mach-sunxi/board.c
> > > @@ -476,8 +476,8 @@ void board_init_f(ulong dummy)
> > >  	/* Enable non-secure access to some peripherals */
> > >  	tzpc_init();
> > >  
> > > -	clock_init();
> > >  	timer_init();
> > > +	clock_init();    
> > 
> > I contemplated similar change in past. It works fine for 64-bit architectures,
> > but I'm unsure for 32-bit. If you take a look at A83t clock code, it uses
> > sdelay() exactly because timer is not initialized at that time.
> > 
> > So, are you sure that this change has no unwanted side effects?  
> 
> No, I am not ;-)
> I was about to test this on some more boards. But I think with the
> change in that direction, the only issue would be if the timer_init() code
> relies on the clocks being set up already. And since timer_init() is only
> *one* rather small function, just doing MMIO accesses, I think the risk is
> quite low.
> 
> Thanks for having a look anyway, I will report back how the test goes.

Alright, I booted this patch on:
A10, A20, A33, V40, A80, A83T, T113s3, F1C100s, H3
(/me puts the mini-USB cables back into the shelf now)

For the A83T specifically, as a test, I replaced the sdelay() calls with
udelay() ones, and it hangs with mainline, but works with this patch.

So unless someone speaks up now, I am tempted to merge this patch.

Cheers,
Andre

Re: [PATCH 2/3] sunxi: spl: initialise timer before clocks

[Jernej Škrabec]

Dne sobota, 2. avgust 2025 ob 01:49:17 Srednjeevropski poletni čas je Andre Przywara napisal(a):
> Recent changes in the H6 clock code added delay() calls into the SPL clock
> setup routine, which requires the timers to work. When compiling for
> AArch64, we are always using the Arm Generic Timer (aka. arch timer),
> which does not require further setup, hence having an empty timer_init()
> routine.
> However for 32-bit SoCs we use the Allwinner timers, which require some
> setup routine, and hence we need timer_init() to be called before
> clock_init().
> 
> Swap the order of the two calls, to be more robust when compiling the H6
> clock code for AArch32 or when using the Allwinner timers for whatever
> reason.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

Reviewed-by: Jernej Skrabec <jernej.skrabec@gmail.com>

Best regards,
Jernej

> ---
>  arch/arm/mach-sunxi/board.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm/mach-sunxi/board.c b/arch/arm/mach-sunxi/board.c
> index fb4837c2082..432b1c10f92 100644
> --- a/arch/arm/mach-sunxi/board.c
> +++ b/arch/arm/mach-sunxi/board.c
> @@ -476,8 +476,8 @@ void board_init_f(ulong dummy)
>  	/* Enable non-secure access to some peripherals */
>  	tzpc_init();
>  
> -	clock_init();
>  	timer_init();
> +	clock_init();
>  	gpio_init();
>  
>  	spl_init();
> 

[PATCH 3/3] sunxi: H616: dram: fix LPDDR3 mode register settings

[Andre Przywara]

The JEDEC LPDDR3 spec defines mode register 0 (MR0) as being read-only,
so there is no point in trying to set its value.
Also the H616 memory controller encodes the mode register index to be
written starting from bit 8 in MRCTRL1 (for LPDDR3 and LPDDR4 chips), so
we need to OR in that number to tell the controller which MR to program.

On top of that, the mode registers between DDR3 and LPDDR3 are
completely different, so writing values crafted for DDR3 into a LPDDR3
chip is just wrong. Due to the above mentioned bugs the writes for
MR0-MR2 did not have any effect (as they were all trying to set the
read-only MR0), so the mode registers just stayed unchanged.

Looking at the LPDDR3 spec and the BSP code, let's write the proper MR
values into LPDDR3 chips, using the proper addressing mode.
Use the opportunity to document the LPDDR3 mode register bits written.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arch/arm/mach-sunxi/dram_sun50i_h616.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/arm/mach-sunxi/dram_sun50i_h616.c b/arch/arm/mach-sunxi/dram_sun50i_h616.c
index 877181016f3..3345c9b8e82 100644
--- a/arch/arm/mach-sunxi/dram_sun50i_h616.c
+++ b/arch/arm/mach-sunxi/dram_sun50i_h616.c
@@ -1078,18 +1078,18 @@ static bool mctl_phy_init(const struct dram_para *para,
 		mctl_await_completion(&mctl_ctl->mrctrl0, BIT(31), 0);
 		break;
 	case SUNXI_DRAM_TYPE_LPDDR3:
-		writel(mr0, &mctl_ctl->mrctrl1);
-		writel(0x800000f0, &mctl_ctl->mrctrl0);
-		mctl_await_completion(&mctl_ctl->mrctrl0, BIT(31), 0);
-
-		writel(4, &mctl_ctl->mrctrl1);
+		/* MR0 is read-only */
+		/* MR1: nWR=14, BL8 */
+		writel(0x183, &mctl_ctl->mrctrl1);
 		writel(0x800000f0, &mctl_ctl->mrctrl0);
 		mctl_await_completion(&mctl_ctl->mrctrl0, BIT(31), 0);
 
-		writel(mr2, &mctl_ctl->mrctrl1);
+		/* MR2: no WR leveling, WL set A, use nWR>9, nRL=14/nWL=8 */
+		writel(0x21c, &mctl_ctl->mrctrl1);
 		writel(0x800000f0, &mctl_ctl->mrctrl0);
 		mctl_await_completion(&mctl_ctl->mrctrl0, BIT(31), 0);
 
+		/* MR3: 34.3 Ohm pull-up/pull-down resistor */
 		writel(0x301, &mctl_ctl->mrctrl1);
 		writel(0x800000f0, &mctl_ctl->mrctrl0);
 		mctl_await_completion(&mctl_ctl->mrctrl0, BIT(31), 0);
-- 
2.46.3

Re: [PATCH 3/3] sunxi: H616: dram: fix LPDDR3 mode register settings

[Jernej Škrabec]

Dne sobota, 2. avgust 2025 ob 01:49:18 Srednjeevropski poletni čas je Andre Przywara napisal(a):
> The JEDEC LPDDR3 spec defines mode register 0 (MR0) as being read-only,
> so there is no point in trying to set its value.
> Also the H616 memory controller encodes the mode register index to be
> written starting from bit 8 in MRCTRL1 (for LPDDR3 and LPDDR4 chips), so
> we need to OR in that number to tell the controller which MR to program.
> 
> On top of that, the mode registers between DDR3 and LPDDR3 are
> completely different, so writing values crafted for DDR3 into a LPDDR3
> chip is just wrong. Due to the above mentioned bugs the writes for
> MR0-MR2 did not have any effect (as they were all trying to set the
> read-only MR0), so the mode registers just stayed unchanged.

Nice catch! Looking at BSP DRAM code, it only sets MR1, MR2 and MR3.

> 
> Looking at the LPDDR3 spec and the BSP code, let's write the proper MR
> values into LPDDR3 chips, using the proper addressing mode.

Please explain how you find those values. Are they always set in this way
for all boards using LPDDR3?

Best regards,
Jernej

> Use the opportunity to document the LPDDR3 mode register bits written.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  arch/arm/mach-sunxi/dram_sun50i_h616.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm/mach-sunxi/dram_sun50i_h616.c b/arch/arm/mach-sunxi/dram_sun50i_h616.c
> index 877181016f3..3345c9b8e82 100644
> --- a/arch/arm/mach-sunxi/dram_sun50i_h616.c
> +++ b/arch/arm/mach-sunxi/dram_sun50i_h616.c
> @@ -1078,18 +1078,18 @@ static bool mctl_phy_init(const struct dram_para *para,
>  		mctl_await_completion(&mctl_ctl->mrctrl0, BIT(31), 0);
>  		break;
>  	case SUNXI_DRAM_TYPE_LPDDR3:
> -		writel(mr0, &mctl_ctl->mrctrl1);
> -		writel(0x800000f0, &mctl_ctl->mrctrl0);
> -		mctl_await_completion(&mctl_ctl->mrctrl0, BIT(31), 0);
> -
> -		writel(4, &mctl_ctl->mrctrl1);
> +		/* MR0 is read-only */
> +		/* MR1: nWR=14, BL8 */
> +		writel(0x183, &mctl_ctl->mrctrl1);
>  		writel(0x800000f0, &mctl_ctl->mrctrl0);
>  		mctl_await_completion(&mctl_ctl->mrctrl0, BIT(31), 0);
>  
> -		writel(mr2, &mctl_ctl->mrctrl1);
> +		/* MR2: no WR leveling, WL set A, use nWR>9, nRL=14/nWL=8 */
> +		writel(0x21c, &mctl_ctl->mrctrl1);
>  		writel(0x800000f0, &mctl_ctl->mrctrl0);
>  		mctl_await_completion(&mctl_ctl->mrctrl0, BIT(31), 0);
>  
> +		/* MR3: 34.3 Ohm pull-up/pull-down resistor */
>  		writel(0x301, &mctl_ctl->mrctrl1);
>  		writel(0x800000f0, &mctl_ctl->mrctrl0);
>  		mctl_await_completion(&mctl_ctl->mrctrl0, BIT(31), 0);
> 

Re: [PATCH 3/3] sunxi: H616: dram: fix LPDDR3 mode register settings

[Andre Przywara]

On Mon, 11 Aug 2025 17:49:06 +0200
Jernej Škrabec <jernej.skrabec@gmail.com> wrote:

Hi Jernej,

many thanks for having a look!

> Dne sobota, 2. avgust 2025 ob 01:49:18 Srednjeevropski poletni čas je Andre Przywara napisal(a):
> > The JEDEC LPDDR3 spec defines mode register 0 (MR0) as being read-only,
> > so there is no point in trying to set its value.
> > Also the H616 memory controller encodes the mode register index to be
> > written starting from bit 8 in MRCTRL1 (for LPDDR3 and LPDDR4 chips), so
> > we need to OR in that number to tell the controller which MR to program.
> > 
> > On top of that, the mode registers between DDR3 and LPDDR3 are
> > completely different, so writing values crafted for DDR3 into a LPDDR3
> > chip is just wrong. Due to the above mentioned bugs the writes for
> > MR0-MR2 did not have any effect (as they were all trying to set the
> > read-only MR0), so the mode registers just stayed unchanged.  
> 
> Nice catch! Looking at BSP DRAM code, it only sets MR1, MR2 and MR3.
> 
> > 
> > Looking at the LPDDR3 spec and the BSP code, let's write the proper MR
> > values into LPDDR3 chips, using the proper addressing mode.  
> 
> Please explain how you find those values. Are they always set in this way
> for all boards using LPDDR3?

Yeah, that's a good question! At first, I tried using what JEDEC describes
as the default settings, but that didn't work. Then I looked into some
parameters found in some BSP image dumps, but that didn't work either.
JEDEC describes those MRs as write-only, and I haven't tried whether
reading them would work nevertheless, not sure about the exact algorithm
for reading MRs anyway.
What eventually worked was to use the BSP values written by exactly the
boot0 on the eMMC that boots an LPDDR3 system - which is actually only one
in mainline: the Tanix TX1.

So yeah, those values work for me (TM), and there is only one LPDDR3 board
supported, so fingers crossed. But actually I wonder if those MR values
really belong into the timing routine (mctl_set_timing_params(), since
parts depend on the speed bin. I see some other SoC/DRAM type combinations
doing that already (H6 DDR3/LPDDR3 and A523 LPDDR4), maybe we should
follow suit here?

Cheers,
Andre


> 
> Best regards,
> Jernej
> 
> > Use the opportunity to document the LPDDR3 mode register bits written.
> > 
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > ---
> >  arch/arm/mach-sunxi/dram_sun50i_h616.c | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> > 
> > diff --git a/arch/arm/mach-sunxi/dram_sun50i_h616.c b/arch/arm/mach-sunxi/dram_sun50i_h616.c
> > index 877181016f3..3345c9b8e82 100644
> > --- a/arch/arm/mach-sunxi/dram_sun50i_h616.c
> > +++ b/arch/arm/mach-sunxi/dram_sun50i_h616.c
> > @@ -1078,18 +1078,18 @@ static bool mctl_phy_init(const struct dram_para *para,
> >  		mctl_await_completion(&mctl_ctl->mrctrl0, BIT(31), 0);
> >  		break;
> >  	case SUNXI_DRAM_TYPE_LPDDR3:
> > -		writel(mr0, &mctl_ctl->mrctrl1);
> > -		writel(0x800000f0, &mctl_ctl->mrctrl0);
> > -		mctl_await_completion(&mctl_ctl->mrctrl0, BIT(31), 0);
> > -
> > -		writel(4, &mctl_ctl->mrctrl1);
> > +		/* MR0 is read-only */
> > +		/* MR1: nWR=14, BL8 */
> > +		writel(0x183, &mctl_ctl->mrctrl1);
> >  		writel(0x800000f0, &mctl_ctl->mrctrl0);
> >  		mctl_await_completion(&mctl_ctl->mrctrl0, BIT(31), 0);
> >  
> > -		writel(mr2, &mctl_ctl->mrctrl1);
> > +		/* MR2: no WR leveling, WL set A, use nWR>9, nRL=14/nWL=8 */
> > +		writel(0x21c, &mctl_ctl->mrctrl1);
> >  		writel(0x800000f0, &mctl_ctl->mrctrl0);
> >  		mctl_await_completion(&mctl_ctl->mrctrl0, BIT(31), 0);
> >  
> > +		/* MR3: 34.3 Ohm pull-up/pull-down resistor */
> >  		writel(0x301, &mctl_ctl->mrctrl1);
> >  		writel(0x800000f0, &mctl_ctl->mrctrl0);
> >  		mctl_await_completion(&mctl_ctl->mrctrl0, BIT(31), 0);
> >   
> 
> 
> 
> 

Re: [PATCH 3/3] sunxi: H616: dram: fix LPDDR3 mode register settings

[Jernej Škrabec]

Dne ponedeljek, 11. avgust 2025 ob 18:08:16 Srednjeevropski poletni čas je Andre Przywara napisal(a):
> On Mon, 11 Aug 2025 17:49:06 +0200
> Jernej Škrabec <jernej.skrabec@gmail.com> wrote:
> 
> Hi Jernej,
> 
> many thanks for having a look!
> 
> > Dne sobota, 2. avgust 2025 ob 01:49:18 Srednjeevropski poletni čas je Andre Przywara napisal(a):
> > > The JEDEC LPDDR3 spec defines mode register 0 (MR0) as being read-only,
> > > so there is no point in trying to set its value.
> > > Also the H616 memory controller encodes the mode register index to be
> > > written starting from bit 8 in MRCTRL1 (for LPDDR3 and LPDDR4 chips), so
> > > we need to OR in that number to tell the controller which MR to program.
> > > 
> > > On top of that, the mode registers between DDR3 and LPDDR3 are
> > > completely different, so writing values crafted for DDR3 into a LPDDR3
> > > chip is just wrong. Due to the above mentioned bugs the writes for
> > > MR0-MR2 did not have any effect (as they were all trying to set the
> > > read-only MR0), so the mode registers just stayed unchanged.  
> > 
> > Nice catch! Looking at BSP DRAM code, it only sets MR1, MR2 and MR3.
> > 
> > > 
> > > Looking at the LPDDR3 spec and the BSP code, let's write the proper MR
> > > values into LPDDR3 chips, using the proper addressing mode.  
> > 
> > Please explain how you find those values. Are they always set in this way
> > for all boards using LPDDR3?
> 
> Yeah, that's a good question! At first, I tried using what JEDEC describes
> as the default settings, but that didn't work. Then I looked into some
> parameters found in some BSP image dumps, but that didn't work either.
> JEDEC describes those MRs as write-only, and I haven't tried whether
> reading them would work nevertheless, not sure about the exact algorithm
> for reading MRs anyway.
> What eventually worked was to use the BSP values written by exactly the
> boot0 on the eMMC that boots an LPDDR3 system - which is actually only one
> in mainline: the Tanix TX1.
> 
> So yeah, those values work for me (TM), and there is only one LPDDR3 board
> supported, so fingers crossed. But actually I wonder if those MR values
> really belong into the timing routine (mctl_set_timing_params(), since
> parts depend on the speed bin. I see some other SoC/DRAM type combinations
> doing that already (H6 DDR3/LPDDR3 and A523 LPDDR4), maybe we should
> follow suit here?

Historically, I checked several boards and if MR value was the same in all
configs, I just hardcoded the value. While this is a bit naive, it works most
of the time. There are some cases where it is set dynamically in BSP code. Those
cases can be a bit annoying to find. Ideally, MR values shout be Kconfig
option with default values.

It's up to you to decide which approach to take. Since it affects just one
board and changes were tested:

Reviewed-by: Jernej Skrabec <jernej.skrabec@gmail.com>

Best regards,
Jernej

> 
> Cheers,
> Andre
> 
> 
> > 
> > Best regards,
> > Jernej
> > 
> > > Use the opportunity to document the LPDDR3 mode register bits written.
> > > 
> > > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > > ---
> > >  arch/arm/mach-sunxi/dram_sun50i_h616.c | 12 ++++++------
> > >  1 file changed, 6 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/arch/arm/mach-sunxi/dram_sun50i_h616.c b/arch/arm/mach-sunxi/dram_sun50i_h616.c
> > > index 877181016f3..3345c9b8e82 100644
> > > --- a/arch/arm/mach-sunxi/dram_sun50i_h616.c
> > > +++ b/arch/arm/mach-sunxi/dram_sun50i_h616.c
> > > @@ -1078,18 +1078,18 @@ static bool mctl_phy_init(const struct dram_para *para,
> > >  		mctl_await_completion(&mctl_ctl->mrctrl0, BIT(31), 0);
> > >  		break;
> > >  	case SUNXI_DRAM_TYPE_LPDDR3:
> > > -		writel(mr0, &mctl_ctl->mrctrl1);
> > > -		writel(0x800000f0, &mctl_ctl->mrctrl0);
> > > -		mctl_await_completion(&mctl_ctl->mrctrl0, BIT(31), 0);
> > > -
> > > -		writel(4, &mctl_ctl->mrctrl1);
> > > +		/* MR0 is read-only */
> > > +		/* MR1: nWR=14, BL8 */
> > > +		writel(0x183, &mctl_ctl->mrctrl1);
> > >  		writel(0x800000f0, &mctl_ctl->mrctrl0);
> > >  		mctl_await_completion(&mctl_ctl->mrctrl0, BIT(31), 0);
> > >  
> > > -		writel(mr2, &mctl_ctl->mrctrl1);
> > > +		/* MR2: no WR leveling, WL set A, use nWR>9, nRL=14/nWL=8 */
> > > +		writel(0x21c, &mctl_ctl->mrctrl1);
> > >  		writel(0x800000f0, &mctl_ctl->mrctrl0);
> > >  		mctl_await_completion(&mctl_ctl->mrctrl0, BIT(31), 0);
> > >  
> > > +		/* MR3: 34.3 Ohm pull-up/pull-down resistor */
> > >  		writel(0x301, &mctl_ctl->mrctrl1);
> > >  		writel(0x800000f0, &mctl_ctl->mrctrl0);
> > >  		mctl_await_completion(&mctl_ctl->mrctrl0, BIT(31), 0);
> > >   
> > 
> > 
> > 
> > 
> 
>