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

[Andreas Bießmann]

Hi Bo,

On 11/14/2013 07:53 AM, Bo Shen wrote:
> On 11/14/2013 02:28 PM, Andreas Bießmann wrote:
>> 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 ...
> 
> The watchdog is one time configurable. If configurabled, can not change
> anymore, so we disable it by default.

But we should give an easy way to enable it. I swear industrial users
require this, so we should at least add a possibility to enable it
easily. Take it as a comment, no need to change it for that patch.

>>>>> +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(?)

For now yes ... we need to think about a better solution in the future.

Best regards

Andreas Bießmann