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

[Mikhail Kalashnikov <iuncuim@gmail.com>]

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;