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

[Hans de Goede <hdegoede@redhat.com>]

Hi Sairhei,

On 18-05-15 12:30, Siarhei Siamashka wrote:
> Hello Hans,
>
> On Wed, 13 May 2015 17:09:33 +0200
> Hans de Goede <hdegoede@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@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

Last time you provided that link it was when discussing the a23 (or was it the
a31?) dram controller, and now you provide the same link for the a33, this makes
no sense since all 3 dram controllers are actually quite different.

We simply do not have reliable dram controller documentation. Yes there may
be some common ancestry between the ti dram controller you're linking to
and the allwinner ones, but allwinner has been consistently tweaking there
dram controller with each new soc generation, so even if that document is
correct for one generation it certainly will not be for other generations.

>
>> ---
>>   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

There is NO documentation. What you're doing is the same as giving
someone the manual for a sony video recorder and then telling him
to use that to program recording of his favorite show into a jvc
recorder, yes there are going to be similarities, that does not
make the manual from manufacturer a going to apply to a device
from manufacturer b.

Last time we had this discussion it was trivial to point out various
places where the documentation did not match up, and that was like 3
generations of dram controllers ago ...

With all that said you're welcome to find things which do match and
provide patches to convert magic constants to useful symbolic names.

What you're not welcome to do is trash my work and throw mud at me,
it would greatly help if you were to use a more constructive tone
in future email.

Regards,

Hans