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

[Bo Shen]

Hi Andreas,

On 11/14/2013 03:42 PM, Andreas Bießmann wrote:
> 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).

Got you means, ok, I will test it.

>>>> +{
>>>> +    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 ...

OK, for register name, I will following up the table.
Thanks.

> Best regards
>
> Andreas Bießmann
>

Best Regards,
Bo Shen