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

[Bo Shen <bard.shen@gmail.com>]

Hi Andreas,

On 11/14/2013 02:28 PM, Andreas Bie?mann wrote:
> 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@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 ...

The watchdog is one time configurable. If configurabled, can not change 
anymore, so we disable it by default.

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

OK, I will add a check here.

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

As watchdog is disabled, we can not use it here.

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

So, keep it as is(?)

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

at91_plla_init()'s main purpose is to wait the lock.

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

This make we focus on what we need to configure.

Best Regards,
Bo Shen