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

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

[Philippe Simons]

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
 	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;
 		break;
 	case SUNXI_DRAM_TYPE_LPDDR4:
 		val = para->tpr6 >> 24 & 0xff;
-- 
2.53.0

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

[Mikhail Kalashnikov]

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/

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

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).
>   	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;
>   		break;
>   	case SUNXI_DRAM_TYPE_LPDDR4:
>   		val = para->tpr6 >> 24 & 0xff;

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

[Paul Kocialkowski]

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

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.

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

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.

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

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

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

[Andre Przywara]

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

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?

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?

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

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

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

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

[Paul Kocialkowski]

[-- 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 --]