[PATCH 1/1] sunxi: dram: Fix incorrect ram size detection for some H6 boards

[Jernej Škrabec]

Dne ponedeljek, 02. oktober 2023 ob 13:26:26 CEST je Andre Przywara napisal(a):
> On Sun,  1 Oct 2023 21:43:32 +0530
> Gunjan Gupta <viraniac at gmail.com> wrote:
> 
> (fixing Jernej's email) 
> 
> Hi Gunjan,
> 
> thanks for sending a patch!
>  
> > On some H6 boards like Orange Pi 3 LTS, some times U-Boot fails to detect
> > ram size correctly. Instead of 2GB thats available, it detects 4GB of ram
> > and then SPL just hangs there making board not to boot further.
> > 
> > On debugging, I found that the rows value were being determined correctly,
> > but columns were sometimes off by one value. I found that adding some
> > delay after the mctl_core_init call along with making use of dsb in the
> > start of the mctl_mem_matches solves the issue.
> > 
> > Signed-off-by: Gunjan Gupta <viraniac at gmail.com>
> > ---
> > 
> >  arch/arm/mach-sunxi/dram_helpers.c   | 1 +
> >  arch/arm/mach-sunxi/dram_sun50i_h6.c | 2 ++
> >  2 files changed, 3 insertions(+)
> > 
> > diff --git a/arch/arm/mach-sunxi/dram_helpers.c b/arch/arm/mach-sunxi/dram_helpers.c
> > index cdf2750f1c..5758c58e07 100644
> > --- a/arch/arm/mach-sunxi/dram_helpers.c
> > +++ b/arch/arm/mach-sunxi/dram_helpers.c
> > @@ -32,6 +32,7 @@ void mctl_await_completion(u32 *reg, u32 mask, u32 val)
> >  #ifndef CONFIG_MACH_SUNIV
> >  bool mctl_mem_matches(u32 offset)
> >  {
> > +	dsb();
> 
> This looks a bit odd, do you have an explanation for that? And are you
> sure that is really needed?
> I understand why we need the DSB after the writel's below, but before that?
> The only thing I could think of is that we are missing a barrier in
> mctl_core_init() - which is the function called before mctl_mem_matches().
> Can you move that dsb(); into mctl_auto_detect_dram_size(), right after
> the mctl_core_init() call (where you add the udelay() below)? And I wonder
> if a dmb() would already be sufficient? I noticed recently that the
> clr/setbit_le32() functions don't have a barrier at all, maybe that should
> be fixed instead?

Looking at original BSP DRAM code, there is no data barriers that I can find.
Cache shouldn't be a thing before DRAM is initialized, right? Conversely,
I suggest adding memory barriers before each udelay(), as it is there for a
reason.

> 
> >  	/* Try to write different values to RAM at two addresses */
> >  	writel(0, CFG_SYS_SDRAM_BASE);
> >  	writel(0xaa55aa55, (ulong)CFG_SYS_SDRAM_BASE + offset);
> > diff --git a/arch/arm/mach-sunxi/dram_sun50i_h6.c b/arch/arm/mach-sunxi/dram_sun50i_h6.c
> > index bff2e42513..a031a845f5 100644
> > --- a/arch/arm/mach-sunxi/dram_sun50i_h6.c
> > +++ b/arch/arm/mach-sunxi/dram_sun50i_h6.c
> > @@ -623,6 +623,8 @@ static void mctl_auto_detect_dram_size(struct dram_para *para)
> >  	para->cols = 11;
> >  	mctl_core_init(para);
> >  
> > +	udelay(50);
> 
> The location of that udelay() looks a bit odd, any chance that belongs at
> the end of mctl_channel_init() instead? And how did you come up with that
> and the value of 50? Just pure experimentation? I think the original BSP
> DRAM code has plenty of delay calls, so we might have missed this one
> here, but I would love to see some explanation here.

I checked original driver and we have *almost* all delays. There are two
missing. Both in mctl_phy_pir_init(). One before mctl_await_completion() and
another after it. Both are 1 us long.

Maybe that solves it (in combination with data barriers)?

Best regards,
Jernej

> 
> Cheers,
> Andre
> 
> >  	for (para->cols = 8; para->cols < 11; para->cols++) {
> >  		/* 8 bits per byte and 16/32 bit width */
> >  		if (mctl_mem_matches(1 << (para->cols + 1 +
> 
>