On Sat, 3 Aug 2024 16:17:26 +0300 Mikhail Kalashnikov <[email protected]> wrote:
Hi Mikhail, > On 02.08.2024 01:55, Chris Morgan wrote: > > From: Jernej Skrabec <[email protected]> > > > > Adjust H616 LPDDR3 DRAM settings to be in line with vendor driver. > > > > Signed-off-by: Jernej Skrabec <[email protected]> > > Tested-by: Chris Morgan <[email protected]> > > --- > > arch/arm/mach-sunxi/dram_sun50i_h616.c | 2 +- > > arch/arm/mach-sunxi/dram_timings/h616_lpddr3.c | 2 +- > > 2 files changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/arch/arm/mach-sunxi/dram_sun50i_h616.c > > b/arch/arm/mach-sunxi/dram_sun50i_h616.c > > index 37c139e0ee..a20264d8b4 100644 > > --- a/arch/arm/mach-sunxi/dram_sun50i_h616.c > > +++ b/arch/arm/mach-sunxi/dram_sun50i_h616.c > > @@ -945,7 +945,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 is the correct change to match the factory tpr6 parameters. > > I think, we also need to change the default value in > arch/arm/mach-sunxi/Kconfig: > > from: config DRAM_SUN50I_H616_TPR6 > hex "H616 DRAM TPR6 parameter" > default 0x3300c080 > to > default 0x33c00080 Can you please confirm what the vendor code does here? Does it use a value of 0xc0 for "val" above, so we need to change *both* the shift and the default value? Or does the vendor code write 0 (the original bits[23:16]) into val? I am a bit puzzled because I think we copied the TPR values verbatim from the vendor firmware, so changing DRAM_SUN50I_H616_TPR6 looks a bit odd. > > > break; > > case SUNXI_DRAM_TYPE_LPDDR4: > > val = para->tpr6 >> 24 & 0xff; > > diff --git a/arch/arm/mach-sunxi/dram_timings/h616_lpddr3.c > > b/arch/arm/mach-sunxi/dram_timings/h616_lpddr3.c > > index ce2ffa7a02..82b86084a6 100644 > > --- a/arch/arm/mach-sunxi/dram_timings/h616_lpddr3.c > > +++ b/arch/arm/mach-sunxi/dram_timings/h616_lpddr3.c > > @@ -24,8 +24,8 @@ void mctl_set_timing_params(const struct dram_para *para) > > u8 trrd = max(ns_to_t(6), 4); > > u8 trcd = ns_to_t(24); > > u8 trc = ns_to_t(70); > > - u8 txp = max(ns_to_t(8), 3); > > u8 trtp = max(ns_to_t(8), 2); > > + u8 txp = trtp; > > I think Jernejchanged this value using RE. I checked the 047fb104 > register (dramtmg[1]) > > on my t98-h2b-lp3 tvbox, it has not changed and is the same as the > factory value. So my educated guess (if that change originates from REing of binary driver code): - tXP and tRTP do not seem related, judging by JEDEC documentation. If the txp value is assigned the value of trtp, that's probably some compiler optimisation, because both variables were assigned to the same expression, in the original vendor code. - The LPDDR3 spec describes tXP as "max(7.5ns,3nCK)", that would be the original setting. Down to 500 MHz DRAM clock ns_to_t(8) results in a value of 3 or higher, so this change would only be relevant for DRAM clock speeds below 500 MHz. I doubt we have a board with LPDDR3 DRAM that slow? So I am happy to change that code, if that's closer to what the vendor driver does, but I doubt that this will impact any actual hardware. Also I would like to change the assignment to copy the *formula* that trtp is using, instead of copying the value of it. So this effectively just lowers the minimum clock cycles to 2 (instead of 3). Does that make sense? Cheers, Andre > > => md.l 0x47fb100 > 047fb100: 10141811 0004041c 04070d0d 0050500c .............PP. >
