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

[Andre Przywara <andre.przywara@arm.com>]

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