[U-Boot] [PATCH 1/2] sunxi: Fix dram initialization not working on some a33 devices

[Siarhei Siamashka]

Hello Hans,

On Wed, 13 May 2015 17:09:33 +0200
Hans de Goede <hdegoede at redhat.com> wrote:

> When porting the allwinner dram init code to u-boot we missed some code
> setting an extra bit when doing auto dram config.

This "extra bit" actually has a name. Not using named constants
instead of the magic numbers makes the code rather difficult to
review. Your persistent refusal to use the nicely available DRAM
controller documentation may be interpreted as an act of disrespect
towards the reviewers of your code.

Basically, the A31, A23 and now A33 DRAM initialization code was
(and still is) a quick and dirty copy/paste hack. And you made exactly
no effort to make it reviewable, despite having been requested to do
so multiple times. Yes, it is nice that this code is at least able
to boot. But this alone can't tell us anything about its quality (after
all, the Allwinner's 3.4 kernel is able to boot a pretty functional
Android system too, but it contains a lot of rather questionable or
outright buggy code under the hood). The DRAM setup is really critical
for the system reliability and has long lasting effects. The bugs in
the vast majority of the rest of the U-Boot code are much less
important because, because they have no chance to cause any troubles
after the OS takes over. The DRAM initialization is different and the
"reckless cowboy" approach is simply very dangerous (assuming that
the end users may want to use their Allwinner based devices for
something more serious than just toying with them for fun).

> This commits add this bit, fixing dram init not working on the ga10h
> 10" a33 tablet which I'm bringing up atm.
>
> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
 
We were just lucky that this particular bug was too easy to notice
and fix. Are you sure that there are no other similar, but more subtle
problems, which are just more difficult to reproduce and may remain
unnoticed longer?

Based on a quick search in the mailing list archives, I have already
provided you with a link to the "spruhn7a.pdf" document 4 (!) times.
This is the fifth attempt and I'm seriously starting to lose patience.
No offense intended, but you are probably the most dense person that I
have ever met :-) There is even an updated C revision of this document:
    http://www.ti.com/lit/ug/spruhn7c/spruhn7c.pdf

> ---
>  arch/arm/cpu/armv7/sunxi/dram_sun8i_a33.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm/cpu/armv7/sunxi/dram_sun8i_a33.c b/arch/arm/cpu/armv7/sunxi/dram_sun8i_a33.c
> index d03f00d..979bb3c 100644
> --- a/arch/arm/cpu/armv7/sunxi/dram_sun8i_a33.c
> +++ b/arch/arm/cpu/armv7/sunxi/dram_sun8i_a33.c
> @@ -195,7 +195,7 @@ static int mctl_train_dram(struct dram_para *para)
>  		(struct sunxi_mctl_ctl_reg *)SUNXI_DRAM_CTL0_BASE;
>  
>  	mctl_data_train_cfg(para);
> -	mctl_set_pir(0x1f3);
> +	mctl_set_pir(0x5f3);
>  
>  	return ((readl(&mctl_ctl->pgsr0) >> 20) & 0xff) ? -EIO : 0;
>  }

Now some questions to Ian Campbell. Are you really happy about
the non-descriptive commit messages like "Initial u-boot port by
Vishnu Patekar, major cleanup / rewrite by Hans de Goede" in
    http://git.denx.de/?p=u-boot.git;a=commit;h=ffc0ae0c70decbe5a910 ?
And are you also happy with neglecting the documentation and keeping
all these magic constants in the code? Have you in fact done the review
of the code logic? Or at least verified that it had been transplanted
from boot0 correctly without introducing any potentially problematic
functional changes?

-- 
Best regards,
Siarhei Siamashka