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

[Heiko Schocher <hs@denx.de>]

Hello Andreas,

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>
>>
>> ---
>> Changes in v3:
>>    - Move plla and mck configure to spl.c file
>>
>> Changes in v2:
>>    - Move spl related code to at91-common folder
>>
>>   arch/arm/cpu/armv7/Makefile                  |    2 +-
>>   arch/arm/cpu/at91-common/Makefile            |    1 +
>>   arch/arm/cpu/at91-common/spl.c               |   90 ++++++++++++++++++++++++++
>>   arch/arm/cpu/at91-common/u-boot-spl.lds      |   50 ++++++++++++++
>>   arch/arm/include/asm/arch-at91/at91_common.h |    4 ++
>>   arch/arm/include/asm/arch-at91/spl.h         |   20 ++++++
>>   board/atmel/sama5d3xek/sama5d3xek.c          |   82 +++++++++++++++++++++++
>>   include/configs/sama5d3xek.h                 |   34 ++++++++++
>>   8 files changed, 282 insertions(+), 1 deletion(-)
>>   create mode 100644 arch/arm/cpu/at91-common/spl.c
>>   create mode 100644 arch/arm/cpu/at91-common/u-boot-spl.lds
>>   create mode 100644 arch/arm/include/asm/arch-at91/spl.h
[...]
>> diff --git a/arch/arm/cpu/at91-common/spl.c b/arch/arm/cpu/at91-common/spl.c
>> new file mode 100644
>> index 0000000..37c0cc4
>> --- /dev/null
>> +++ b/arch/arm/cpu/at91-common/spl.c
>> @@ -0,0 +1,90 @@
>> +/*
>> + * Copyright (C) 2013 Atmel Corporation
>> + *		      Bo Shen<voice.shen@atmel.com>
>> + *
>> + * SPDX-License-Identifier:	GPL-2.0+
>> + */
>> +
>> +#include<common.h>
>> +#include<asm/io.h>
>> +#include<asm/arch/at91_common.h>
>> +#include<asm/arch/at91_pmc.h>
>> +#include<asm/arch/at91_wdt.h>
>> +#include<asm/arch/clk.h>
>> +#include<spl.h>
>> +
>> +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...

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

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

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

>> +	writel(tmp,&pmc->mckr);
>> +
>> +	while (!(readl(&pmc->sr)&  AT91_PMC_MCKRDY))
>> +		;
>> +}
>> +
>> +
>> +u32 spl_boot_device(void)
>> +{
>> +#ifdef CONFIG_SYS_USE_MMC
>> +	return BOOT_DEVICE_MMC1;
>> +#endif
>
> Isn't there some way to detect the boot source by asking for example the
> ROM code? Is there some register that holds that information?

Yeah, that would be nice if we have such an information. Just got the
information, that on the taurus board is also a SPI dataflash, from
which it can boot. So I have the problem, that I have two boot sources,
and if I want only one spl image, I have to detect from where SPL code
got loaded ...

[...]

bye,
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany