[U-Boot] [PATCH v3 5/6] arm: atmel: add ddr2 initialization function

[Andreas Bießmann]

Hi Bo,

On 11/14/2013 07:40 AM, Bo Shen wrote:
> On 11/13/2013 09:03 PM, Andreas Bießmann wrote:
>> On 11/06/2013 06:29 AM, Bo Shen wrote:

<snip>

>>> +static void atmel_mpddr_op(int mode, u32 ram_address)
>>
>> static inline, could give the compiler a hint to optimize here.
> 
> I am not sure whether the write(0, ram_address) will be optimized.
> Because this must excute later than write mode to let it work.

well, I don't mean reordering for optimization here.
The point is the atmel_mpddr_op will get two mostly static values (it is
not fully true cause ram_address will be calculated, but didn't knew
when writing). So when using the function we need to setup two
registers, branch (and do all the stuff involved: saving to the stack
a.s.o.) and write the register content to two different addresses.

If we instruct the compiler to inline atmel_mpddr_op() he could decide
to just take the const value given into ddr2_init to be written to the
relevant address. I think that is optimization too. It could however
increase the text segment by some bytes also, that is to be checked
(cause it would be getting worse and that would be no optimization).

>>> +{
>>> +    struct atmel_mpddr *mpddr = (struct atmel_mpddr
>>> *)ATMEL_BASE_MPDDRC;
>>> +
>>> +    writel(mode, &mpddr->mr);
>>> +    writel(0, ram_address);
>>> +}
>>> +
>>> +int ddr2_init(u32 ram_address, struct atmel_mpddr *mpddr_value)
>>
>> could you please constify mpddr_value for the very same reason?
> 
> OK.

I think ram_address should also be declared const.

<snip>

>>> +#ifndef __ATMEL_MPDDRC_H__
>>> +#define __ATMEL_MPDDRC_H__
>>> +
>>> +struct atmel_mpddr {
>>> +    u32 mr;
>>> +    u32 rtr;
>>> +    u32 cr;
>>> +    u32 tp0r;
>>
>> Datasheet names them tprX
> 
> Actually, this name is Timing Parameter 0 Register,
> Timing Parameter 1 Register.

Well, I know. But my data sheet names it MPDDRC_TPR0 (the abbreviation
in table 28-13). I think we should use the names in data sheet here. I
often search the data sheet for the given name. mpddr will point to the
correct subsystem in SoC, that is good. But tp0r will find nothing. This
just consumes time when looking up the spec and is IMHO annoying.

>>> +    u32 tp1r;
>>> +    u32 tp2r;
>>> +    u32 reserved[2];
>>> +    u32 mdr;
>>
>> Datasheet names it just md.
> 
> All other registers with "r" suffix, so, add r to this register name too.

But the name in data sheet is MPDDRC_MD ...

Best regards

Andreas Bießmann