[U-Boot] [PATCH 06/10] sunxi: Add a33 dram init code

[Hans de Goede]

Hi,

On 04/16/2015 11:09 AM, Ian Campbell wrote:
> On Thu, 2015-04-16 at 09:27 +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 15-04-15 21:56, Ian Campbell wrote:
>>> On Tue, 2015-04-14 at 18:06 +0200, Hans de Goede wrote:
>>>> From: Vishnu Patekar <vishnupatekar0510 at gmail.com>
>>>>
>>>> Based on Allwinner dram init code from the a33 bsp:
>>>> https://github.com/allwinner-zh/bootloader/blob/master/basic_loader/bsp/bsp_for_a33/init_dram/mctl_hal.c
>>>>
>>>> Initial u-boot port by Vishnu Patekar, major cleanup / rewrite by
>>>> Hans de Goede.
>>>>
>>>> Signed-off-by: Vishnu Patekar <vishnupatekar0510 at gmail.com>
>>>> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
>>>
>>>> +	/* Set dram timing */
>>>> +	reg_val = (twtp << 24) | (tfaw << 16) | (trasmax << 8) | (tras << 0);
>>>> +	writel(reg_val, &mctl_ctl->dramtmg0);
>>>> +	reg_val = (txp << 16) | (trtp << 8) | (trc << 0);
>>>> +	writel(reg_val, &mctl_ctl->dramtmg1);
>>>> +	reg_val = (tcwl << 24) | (tcl << 16) | (trd2wr << 8) | (twr2rd << 0);
>>>> +	writel(reg_val, &mctl_ctl->dramtmg2);
>>>> +	reg_val = (tmrw << 16) | (tmrd << 12) | (tmod << 0);
>>>> +	writel(reg_val, &mctl_ctl->dramtmg3);
>>>> +	reg_val = (trcd << 24) | (tccd << 16) | (trrd << 8) | (trp << 0);
>>>> +	writel(reg_val, &mctl_ctl->dramtmg4);
>>>> +	reg_val = (tcksrx << 24) | (tcksre << 16) | (tckesr << 8) | (tcke << 0);
>>>> +	writel(reg_val, &mctl_ctl->dramtmg5);
>>>
>>> There's a lot of magic numbers here (and in the following code),
>>> although in this particular context (with the named var) unless they are
>>> the same elsewhere I'm not sure #defines would improve things much, but
>>> I think some of the other stuff likely would.
>>>
>>> Assuming you have any idea what the bits are, I suppose that per usual
>>> we don't really know because -ENODOC?
>>
>> Right the problem here is -ENODOC, the magic values come from the allwinnner
>> code and in the cases where we do not have named variables (as we do in
>> the above blurb) we've no clue what we're doing really, so I think adding
>> defines there will only obfuscate things.
>>
>> Can you give a short description of the cases where you believe that
>> adding defines would be a good idea ?
>
> Anywhere where we know the meanings ;-) If that's nowhere then the magic
> numbers are fine (which is what my final paragraph was trying to say,
> but not very clearly).

Ok, so I've gone over the entire dram init code another time, and I've
not been able to find a single place where I can add a define with a
meaningful name to replace the magic values.

So no v2 for this patch, can I get an ack for v1 ?

Regards,

Hans