Re: [PATCH v2] sunxi: H616: dram: fix LPDDR3 TRP6 parsing

[Paul Kocialkowski <paulk@sys-base.io>]

[-- Attachment #1: Type: text/plain, Size: 9768 bytes --]

Hi Andre,

On Thu 09 Apr 26, 23:34, Andre Przywara wrote:
> On Thu, 9 Apr 2026 17:10:19 +0200
> Paul Kocialkowski <contact@paulk.fr> wrote:
> 
> Hi Paul, Mikhail,
> 
> thanks for the reply!
> 
> > Hi,
> > 
> > On Thu 09 Apr 26, 13:48, Mikhail Kalashnikov wrote:
> > > On 4/8/26 00:47, Philippe Simons wrote:  
> > > > From: Jernej Skrabec <jernej.skrabec@gmail.com>
> > > > 
> > > > Allwinner's BSP DRAM code uses parameter TPR6, presumably containing
> > > > some "Vref" parameter, to encode the values for *all* four supported DRAM
> > > > types. The code selects one byte based on the DRAM type used at runtime.
> > > > To allow copying DRAM parameters from vendor firmware, we used this value
> > > > and its encoding, but wrongly: the proper order of bytes is DDR3, DDR4,
> > > > LPDDR3, LPDDR4, from LSB to MSB, cf. the A523 and A133 DRAM code.
> > > > 
> > > > Correct the masking for LPDDR3 to fix DRAM operation on some boards
> > > > using this DRAM type.
> > > > 
> > > > With LPDDR3 TRP6 parsing fixed, adapt default DRAM_SUNXI_TPR6 value.
> > > > Also change LPDDR4 default value to 0x38 used by A523 boards.
> > > > 
> > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
> > > > [adjusted commit message, update default value]
> > > > Signed-off-by: Philippe Simons <simons.philippe@gmail.com>
> > > > ---
> > > >   arch/arm/mach-sunxi/Kconfig            | 2 +-
> > > >   arch/arm/mach-sunxi/dram_sun50i_h616.c | 2 +-
> > > >   2 files changed, 2 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/arch/arm/mach-sunxi/Kconfig b/arch/arm/mach-sunxi/Kconfig
> > > > index e979ee4a2cc..a1ddc6a1fc8 100644
> > > > --- a/arch/arm/mach-sunxi/Kconfig
> > > > +++ b/arch/arm/mach-sunxi/Kconfig
> > > > @@ -144,7 +144,7 @@ config DRAM_SUNXI_TPR3
> > > >   config DRAM_SUNXI_TPR6
> > > >   	hex "DRAM TPR6 parameter"
> > > > -	default 0x3300c080
> > > > +	default 0x38c00080  
> > > I don't think this will be a good solution. The changes in this series
> > > were originally made to improve compatibility with the vendor driver,
> > > but now you are introducing an additional inconsistency by closing the
> > > previous one. The default values are not just arbitrary values — they
> > > are part of the vendor code that was obtained through binary analysis.
> > > https://lore.kernel.org/all/20231018172109.085cce24@donnerap.manchester.arm.com/
> >
> > I tend to agree with you, it feels like the default value was taken from the
> > reference code while the field parsing was just an issue that was introduced
> > with the h616 code. So it should not affect the defaut value, which was likely
> > mistakenly parsed before.
> 
> Yes, that Tanix TX1 uses a wrong value because of this, but that's
> about the only effect, I'd say.
> 
> It seems like at this point the "default" value is just a convenience
> value we introduced to avoid naming the same repeated TPR6 value in
> every defconfig, as per my email. But it's a pure mainline U-Boot
> technicality at this point, and we could easily just provide an
> explicit value in each defconfig. I came up with this table, for the
> currently mainline supported boards:
> 
> board name		SoC	TPR6 value	type	Vref value
> ------------------------------------------------------------------
> liontron-h-a133l	A133	0x48000000	LPDDR4  48xxxxxx
> anbernic_rg35xx_h700	H616	0x40808080      LPDDR4  40xxxxxx
> orangepi_zero2		H616	0x3300c080 d	DDR3    xxxxxx80
> orangepi_zero2w		H616	0x48808080	LPDDR4  48xxxxxx
> orangepi_zero3		H616	0x44000000	LPDDR4  44xxxxxx
> tanix_tx1		H616	0x2fb08080	LPDDR3  xxb0xxxx
> transpeed-8k618-t	H616	0x3300c080 d	DDR3    xxxxxx80
> x96_mate		H616	0x3300c080 d	DDR3    xxxxxx80
> x96q			H616	0x3300c080 d	DDR3    xxxxxx80
> yuzukihd-chameleon	H616	0x33808080	DDR3    xxxxxx80
> avaota-a1		A523	0x38000000	LPDDR4  38xxxxxx
> orangepi_4a		A523	0x38000000	LPDDR4  38xxxxxx
> radxa-cubie-a5e		A523	0x38000000	LPDDR4  38xxxxxx
> x96q_pro_plus		A523	0x3380807e	DDR3    xxxxxx7e

A few more entries for board I could get running (not submitted yet):

board name		SoC	TPR6 value	type	Vref value
------------------------------------------------------------------
baijie-helperboard-a133	A133	0x48000000	LPDDR4	48xxxxxx
logicom-la-tab-129	A133	0x2fb88080	LPDDR3	xxb8xxxx
kickpi-k5c		A133	0x33808080	DDR3	xxxxxx80
dshanpi-rosx		R818	0x48000000	LPDDR4	48xxxxxx

> So the default value is just used for the old H616 DDR3 boards, where
> only the LSB matters.
> 
> So what about dropping the default completely, since now it spans three
> SoCs already, and finding some reasonable default for all of them will
> become more complicated?

Yes honestly this is the only DRAM TPR value that has a non-zero default in
Kconfig and it feels a bit weird. I'm in favor of explicitly defining all the
board-specific parameters in the configs and using zero as Kconfig default.

> And maybe you can confirm: this should not affect compatibility with
> vendor parameters at all, I think they always contain an explicit TPR6
> value, and "no TPR6 value provided" is not a thing?

Yes I've always seen an explicit value defined, and since it's part of the
dram_para array, there has to be something. However the A133 code has fallback
values when it gets a zero from dram_para:
https://source.denx.de/u-boot/custodians/u-boot-sunxi/-/blob/master/arch/arm/mach-sunxi/dram_sun50i_a133.c?ref_type=heads#L598

This doesn't seem to be the case in the H616 and A523 code. The default is 0x80
for DDR3/DDR4/LPDDR3 and 0x33 for LPDDR4. I'm not sure this is really a thing
that boards might actually rely on though. I would rather be in favor of
removing it too to avoid confusion.

> > > There are several options here:
> > > 1. If our goal is to improve compatibility, then we should not change
> > > the default values for LPDDR4, and only keep the changes for LPDDR3.
> 
> Fair, I don't really care about the MSB, the new value I suggested
> seemed just like the most common value (see the table above). But it's
> pointless, since those boards provide an explicit value, and don't use
> the default.
>
> > > 2. Add the changes from option 2, and additionally introduce the default
> > > values for the A523 platform that I made during the initial binary
> > > analysis but for some reason did not get merged into mainline.
> > > (https://github.com/iuncuim/u-boot/blob/b17c5c8911a4fce328b01e6332632a9ccd88ebc6/arch/arm/mach-sunxi/Kconfig#L102)
> 
> Sorry, but this value doesn't seem right: The LPDDR4 boards use 0x38,
> not 0x33, and the DDR3 board uses 0x7e, not 0x80. I don't know about
> other boards, but this value don't seem to help - and it's unneeded
> anyway, as the A523 board already provide an explicit TPR6 - and new
> boards could do as well.

I think we should just drop the default value instead of trying to make sense
of what that default should be for different platforms.

> > It would be good to have different defaults for different platforms yes.
> > Note that the a133 code has some fallback with defaults when the extracted
> > field in tpr6 is zero. Maybe they should be used as default value for a133.
> > We probably still need to keep these fallbacks in the code in case some
> > board-specific tpr6 value does have zeroes and relies on this behavior.
> > 
> > > 3. If we accept the incompatibility between the vendor driver and
> > > the mainline driver, then this patch series is unnecessary because
> > > they introduce additional inconsistency with the vendor driver.  
> > 
> > We definitely want to be able to take the dram parameter values straight from
> > the BSP and put them directly into our Kconfig. Changing the order of fields
> > would make porting new boards very painful so it should be avoided.
> 
> Yes, I agree, we should take the DRAM parameters values verbatim. But
> as explained above, I think the Kconfig default is a different matter.
> For simplicity, I'd say we drop it and add TPR6 values into the
> affected defconfigs. Providing separate defaults for different SoC just
> makes things more complicated - for no reason.

Agreed.

All the best,

Paul

> > 
> > > I think option two is the most correct, but option one is also suitable,
> > > since the DRAM driver for the A523 only supports DDR3 and LPDDR4,
> > > and the default values in this case are the same as for the H616
> > > (A523 is 0x33808080, H616 is 0x33c00080).  
> > 
> > I suggest we keep the current default (without moving the lpddr3 bits) for h616
> > and add new defaults in Kconfig for a523 and a133.
> > 
> > > >   	help
> > > >   	  TPR6 value from vendor DRAM settings.
> > > > diff --git a/arch/arm/mach-sunxi/dram_sun50i_h616.c b/arch/arm/mach-sunxi/dram_sun50i_h616.c
> > > > index 3345c9b8e82..42a0550e015 100644
> > > > --- a/arch/arm/mach-sunxi/dram_sun50i_h616.c
> > > > +++ b/arch/arm/mach-sunxi/dram_sun50i_h616.c
> > > > @@ -975,7 +975,7 @@ static bool mctl_phy_init(const struct dram_para *para,
> > > >   		val = para->tpr6 & 0xff;
> > > >   		break;
> > > >   	case SUNXI_DRAM_TYPE_LPDDR3:
> > > > -		val = para->tpr6 >> 8 & 0xff;
> > > > +		val = para->tpr6 >> 16 & 0xff;  
> > 
> > This change is definitely necessary though.
> 
> Yes, absolutely.
> 
> 
> Cheers,
> Andre
> 
> > 
> > > >   		break;
> > > >   	case SUNXI_DRAM_TYPE_LPDDR4:
> > > >   		val = para->tpr6 >> 24 & 0xff;  
> > 
> > All the best,
> > 
> > Paul
> > 
> 

-- 
Paul Kocialkowski,

Free software developer - https://www.paulk.fr/
Independent contractor - sys-base - https://www.sys-base.io/

Contributor to fully free software support for selected hardware.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]