[U-Boot] [PATCH 13/14] sun8i: Add dram initialization support

[Siarhei Siamashka]

On Fri, 19 Dec 2014 18:05:49 +0100
Hans de Goede <hdegoede at redhat.com> wrote:

> Hi,
> 
> On 19-12-14 11:20, Siarhei Siamashka wrote:
> > On Tue, 16 Dec 2014 21:31:38 +0100
> > Hans de Goede <hdegoede at redhat.com> wrote:
> >
> >> Based on the register / dram_para headers from the Allwinner u-boot / linux
> >> sources + the init sequences from boot0.
> >
> > Can the commit message have more detailed information about the precise
> > location of the original Allwinner code, which was used as the
> > reference?
> 
> No, it cannot because no code was referenced, I traced the boot0 binary to
> figure out what was needed to get dram going on the A23.

Thanks for explaining. And good job extracting all the information
from there.

In this case, can we have a clear reference to this binary in the
commit message? It might be useful to have a look at objdump logs
when reviewing your patch.

[...]

> >> +++ b/arch/arm/cpu/armv7/sunxi/dram_sun8i.c
> >> @@ -0,0 +1,340 @@
> >> +/*
> >> + * Sun8i platform dram controller init.
> >> + *
> >> + * (C) Copyright 2014 Hans de Goede <hdegoede at redhat.com>
> >
> > Is Allwinner copyright really not necessary here?
> >
> >> + * SPDX-License-Identifier:	GPL-2.0+
> >> + */
> >> +
> >> +/*
> >> + * Note this code uses a lot of magic hex values, that is because this code
> >> + * simply replays the init sequence as done by the Allwinner boot0 code, so
> >> + * we do not know what these values mean. There are no symbolic constants for
> >> + * these magic values, since we do not know how to name them and making up
> >> + * names for them is not useful.
> >> + */
> >
> > You are well aware of this documentation since a long time ago, right?
> >     http://www.ti.com/lit/ug/spruhn7a/spruhn7a.pdf
> 
> I'm aware you've used various sources to figure out more about the A10
> dram controller, since this DRAM controller seems significantly different
> I assumed your sources would not apply, it is good to hear that they do.

I wonder how could you have ended up with this strange assumption?
When I clearly told you that "It looks like the Allwinner A31 DRAM
controller registers are very similar to what is used in RK3288" and
provided the links to the relevant sources and documentation back
in September:
    http://lists.denx.de/pipermail/u-boot/2014-September/189199.html

Then reminded yet again about this RK3288 similarity and used the
mctl_phy->ptr0 register as an example:
    http://lists.denx.de/pipermail/u-boot/2014-December/198582.html

When we were discussing the A31 DRAM controller, of course you should
have expected the information about A31.

Yes, A23 was not a part of the picture at that time. However comparing
its registers with the information from spruhn7a.pdf and various
Rockchip source code drops is a very natural thing to try.

So far the best matching pairs (almost 100% identical PHY) are:
   Allwinner A10 (sun4i) - RK29xx
   Allwinner A31 (sun6i) - RK3288
   Allwinner A23 (sun8i) - TI Keystone2

It's all the same family of DRAM controllers, very likely just
different revisions/generations from the same supplier. I don't
expect A80 or other Allwinner chips to be any different.

[...]

> >> +	else
> >> +		writel(0x1000040b, &mctl_phy->dcr);
> >
> > The bit 28 is marked as reserved and "Reads return zeros" in the
> > TI Keystone2 documentation. It's a nice chance to test whether
> > this write has any effect and whether it does modify the register.
> 
> It modifies the register, so that means that at least that bit does
> not match the TI Keystone2 documentation.

OK.

-- 
Best regards,
Siarhei Siamashka