[U-Boot] [PATCH v3 6/6] arm: atmel: sama5d3: spl boot from fat fs SD card

[Andreas Bießmann]

Hello Heiko,

On 14.11.13 06:52, Heiko Schocher wrote:
> Am 13.11.2013 14:34, schrieb Andreas Bießmann:
>> Hi Bo,
>>
>> On 11/06/2013 06:29 AM, Bo Shen wrote:
>>> Enable Atmel sama5d3xek boart spl boot support, which can load u-boot
>>> from SD card with FAT file system.
>>>
>>> Signed-off-by: Bo Shen<voice.shen at atmel.com>
>>>
>>> ---

<snip>

>>> +static void at91_disable_wdt(void)
>>
>> Why should we disable the WDT in SPL? I think it would be better to
>> configure a working timer value than just disable it.
> 
> This is currently done in the at91bootstrap code too...

I know ...
> 
>> Well it's easy and works, but for the future I think it would be good to
>> let it run while in SPL and u-boot.
> 
> We should have the option to enable/disable it ...

I just like to mention that this should be changed in near future.

>>> +{
>>> +    struct at91_wdt *wdt = (struct at91_wdt *)ATMEL_BASE_WDT;
>>> +
>>> +    writel(AT91_WDT_MR_WDDIS,&wdt->mr);
>>> +}
>>> +
>>> +void at91_plla_init(u32 pllar)
>>> +{
>>> +    struct at91_pmc *pmc = (struct at91_pmc *)ATMEL_BASE_PMC;
>>> +
>>
>> We should ensure bit 29 to be '1' here.
> 
> What is bit 29 ?

'ONE' ;) Spec says it must be written to '1'

>>> +    writel(pllar,&pmc->pllar);
>>> +    while (!(readl(&pmc->sr)&  (AT91_PMC_LOCKA | AT91_PMC_MCKRDY)))
>>> +        ;
>>
>> Especially for doing such things it would be best handled by the WDT on
>> error.
>>
>>> +}
>>> +
>>> +void at91_mck_init(u32 mckr)
>>> +{
>>> +    struct at91_pmc *pmc = (struct at91_pmc *)ATMEL_BASE_PMC;
>>> +    u32 tmp;
>>> +
>>> +    tmp = readl(&pmc->mckr);
>>> +    tmp&= ~(AT91_PMC_MCKR_PRES_MASK |
>>> +         AT91_PMC_MCKR_MDIV_MASK |
>>> +         AT91_PMC_MCKR_PLLADIV_2);
>>> +    tmp |= mckr&  (AT91_PMC_MCKR_PRES_MASK |
>>> +               AT91_PMC_MCKR_MDIV_MASK |
>>> +               AT91_PMC_MCKR_PLLADIV_2);
>>
>> Why gets the at91_mck_init() just some parts of the MCK register (some
>> fields are preserved here) while the at91_plla_init() just rewrites the
>> PLLA register?

Don't you see the error in my sentence here? ;) I thought some parts of
the register content where preserved. The other way round is true, we
need to clean up relevant parts (PRES, MDIV, and PLLADIV Bit(s)). Sorry,
my fault here.

>> I think it is not much more than hiding the writel() register access. I
>> think a better API would be to request some specific frequency and we
>> calculate the register values with that input.
>> Please let us discuss this.
> 
> Such a function would be nice, indeed. But I would accept this function,
> to get SPL code into mainline... (I have the same function ;-)

Yea, no problem with this function. I just wonder why the
at91_plla_init() is a plain writel() (plus waiting for lock) and this
one does a bit more (preserve CSS... We could also declare the whole
register content when calling at91_mckr_init(), don't we?

This is no change request in general, but please lets discuss why we not
just write the given register content.

Best regards

Andreas Bießmann