Hi Andre, On Thu 09 Apr 26, 23:34, Andre Przywara wrote: > On Thu, 9 Apr 2026 17:10:19 +0200 > Paul Kocialkowski 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 > > > > > > > > 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 > > > > [adjusted commit message, update default value] > > > > Signed-off-by: Philippe Simons > > > > --- > > > > 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.