[U-Boot] [PATCH] Fix unreliable detection of DRAM size on Orange Pi 3

[Siarhei Siamashka]

On Sun, 25 Aug 2019 18:12:22 +0200
Ondřej Jirman <megous at megous.com> wrote:

> Hello,
> 
> On Sun, Aug 25, 2019 at 05:41:55PM +0300, Siarhei Siamashka wrote:
> > On Sat, 24 Aug 2019 22:07:43 +0200
> > Ondřej Jirman <megous at megous.com> wrote:
> > 
> > > Hi Jagan,
> > > 
> > > can you please apply this patch to the sunxi tree, so that it
> > > doesn't get lost.
> > > 
> > > thank you,
> > > 	Ondrej
> > > 
> > > On Mon, Jul 29, 2019 at 01:39:42AM +0200, megous hlavni wrote:
> > > > From: Ondrej Jirman <megous at megous.com>
> > > > 
> > > > Orange Pi 3 has 2 GiB of DRAM, that sometime get misdetected
> > > > as 4 GiB, due to false negative result from mctl_mem_matches()
> > > > when detecting number of column address bits. This leads to
> > > > u-boot detecting more address bits than there are and the
> > > > boot process hangs shortly after.
> > > > 
> > > > In mctl_mem_matches() we need to wait for each write to finish,
> > > > separately. Without this, the check is not reliable for some
> > > > unknown reason, probably having to do with unpredictable memory
> > > > access ordering.
> > > > 
> > > > Patch was made with help from André Przywara, who noticed that
> > > > my original idea about detection failing due to read-back from
> > > > cache without involving DRAM was false, because data cache is
> > > > still of at the time of the DRAM size autodetection.
> > > > 
> > > > Signed-off-by: Ondrej Jirman <megous at megous.com>
> > > > Cc: André Przywara <andre.przywara at arm.com>
> > > > ---
> > > >  arch/arm/mach-sunxi/dram_helpers.c | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > > 
> > > > diff --git a/arch/arm/mach-sunxi/dram_helpers.c b/arch/arm/mach-sunxi/dram_helpers.c
> > > > index 239ab421a8..6dba448638 100644
> > > > --- a/arch/arm/mach-sunxi/dram_helpers.c
> > > > +++ b/arch/arm/mach-sunxi/dram_helpers.c
> > > > @@ -30,6 +30,7 @@ bool mctl_mem_matches(u32 offset)
> > > >  {
> > > >  	/* Try to write different values to RAM at two addresses */
> > > >  	writel(0, CONFIG_SYS_SDRAM_BASE);
> > > > +	dsb();
> > > >  	writel(0xaa55aa55, (ulong)CONFIG_SYS_SDRAM_BASE + offset);
> > > >  	dsb();
> > > >  	/* Check if the same value is actually observed when reading back */
> > > > -- 
> > 
> > Hi!
> > 
> > This looks like resurfacing of the old problem, which did not get a
> > complete fix back in 2016:
> >    https://lists.denx.de/pipermail/u-boot/2016-April/251803.html
> > 
> > Now the main question is whether the DSB instruction's barrier
> > magic is really required here or maybe it's just a timing issue.
> 
> That's certainly possible, because without this patch, the problem
> appears intermittently, and not always. I also have the same end
> result, where mctl_mem_matches returns false when it should not.

You could also try to see if the problem becomes easier or harder
to reproduce if you change the CPU or DRAM clock speed.
 
> > Could you please experiment with this problem a little bit more?
> > 
> >  1. What if you just move this second DSB instruction after the
> >     second write and have two of them there? Does this also make
> >     the problem disappear?
> > 
> >  2. What if you just replace DSB with udelay(1) / udelay(10) /
> >     udelay(100)? Does this make the problem disappear?
> > 
> > 
> > I suspect that we may be dealing with some sort of buffering in
> > the DRAM controller itself and the only reason why DSB helps is
> > that it introduces some delay because it's a rather slow
> > instruction.
> 
> Thanks for suggestions. If it's just the delay, then whatever delay
> dsb introduces seems enough. It'll probably need just a small delay,
> because most of the time it works without one.

It still would be better to estimate how long is the delay introduced
by a single DSB instruction. If it's small, then going from one DSB
instructions to two DSB instructions might be not enough to completely
eliminate the problem. Or it might fully fix the problem on your
Orange Pi 3 board, but then show up again on some other SoC with a
different CPU clock speed.

I agree that we need to ensure that the two write instructions
do not get reordered relative to each other. And also ensure
that the second write instruction and read and not reordered
relative to each other either. So the idea of your patch is
good.

My only concern is the choice of DSB instructions, because I
suspect that these memory requests coming from the CPU might
be already properly ordered with or without the use of DSB.
If it's the DRAM controller doing all the buffering or requests
reordering outside of the CPU, then we may still have a race.

Uncached DRAM read/write operations usually need time in a
ballpark of 100 ns. Inserting 1us delays between memory
accesses should be long enough to ensure their completion.

Anyway, it's up to you whether to do some additional tests
or not, in the case if you are curious about what's going on.

If your patch is just applied as-is, then the problem will be
resolved for now. And the worst thing that may happen, would
be somebody encountering the same problem again in the future
and coming here to ask "Hey, what is the right place to
insert a third DSB instruction?" ;-)

-- 
Best regards,
Siarhei Siamashka