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

13 Nov 2013

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/armv7/Makefile b/arch/arm/cpu/armv7/Makefile
index ee4b021..aea4e8b 100644
--- a/arch/arm/cpu/armv7/Makefile
+++ b/arch/arm/cpu/armv7/Makefile
@@ -16,7 +16,7 @@ COBJS	+= cache_v7.o
 COBJS	+= cpu.o
 COBJS	+= syslib.o
-ifneq ($(CONFIG_AM43XX)$(CONFIG_AM33XX)$(CONFIG_OMAP44XX)$(CONFIG_OMAP54XX)$(CONFIG_TEGRA)$(CONFIG_MX6)$(CONFIG_TI81XX),)
+ifneq ($(CONFIG_AM43XX)$(CONFIG_AM33XX)$(CONFIG_OMAP44XX)$(CONFIG_OMAP54XX)$(CONFIG_TEGRA)$(CONFIG_MX6)$(CONFIG_TI81XX)$(CONFIG_AT91FAMILY),)
 SOBJS	+= lowlevel_init.o
 endif
diff --git a/arch/arm/cpu/at91-common/Makefile b/arch/arm/cpu/at91-common/Makefile
index 6f1a9e6..4377a0b 100644
--- a/arch/arm/cpu/at91-common/Makefile
+++ b/arch/arm/cpu/at91-common/Makefile
@@ -13,6 +13,7 @@ include $(TOPDIR)/config.mk
 LIB	= $(obj)libat91-common.o
COBJS-$(CONFIG_SPL_BUILD) += mpddrc.o
+COBJS-$(CONFIG_SPL_BUILD) += spl.o
SRCS    := $(SOBJS-y:.o=.S) $(COBJS-y:.o=.c)
 OBJS    := $(addprefix $(obj),$(SOBJS-y) $(COBJS-y))
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.
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.
...
+{

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

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

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

return BOOT_DEVICE_NONE;

+}



+u32 spl_boot_mode(void)
+{

switch (spl_boot_device()) {

+#ifdef CONFIG_SYS_USE_MMC

case BOOT_DEVICE_MMC1:
return MMCSD_MODE_FAT;


break;



+#endif

case BOOT_DEVICE_NONE:
default:
hang();


}

+}



+void s_init(void)
+{

/* disable watchdog */
at91_disable_wdt();

/* PMC configuration */
at91_pmc_init();

at91_clock_init(CONFIG_SYS_AT91_MAIN_CLOCK);

timer_init();

board_early_init_f();

preloader_console_init();

mem_init();

+}
diff --git a/arch/arm/cpu/at91-common/u-boot-spl.lds b/arch/arm/cpu/at91-common/u-boot-spl.lds
new file mode 100644
index 0000000..038335d
--- /dev/null
+++ b/arch/arm/cpu/at91-common/u-boot-spl.lds
@@ -0,0 +1,50 @@
+/*


(C) Copyright 2002



Gary Jennejohn, DENX Software Engineering, garyj@denx.de







(C) Copyright 2010



Texas Instruments, <www.ti.com>



Aneesh V aneesh@ti.com







(C) 2013 Atmel Corporation



   Bo Shen <voice.shen@atmel.com>









SPDX-License-Identifier:	GPL-2.0+


*/


+MEMORY { .sram : ORIGIN = CONFIG_SPL_TEXT_BASE, \

LENGTH = CONFIG_SPL_MAX_SIZE }



+MEMORY { .sdram : ORIGIN = CONFIG_SPL_BSS_START_ADDR, \

LENGTH = CONFIG_SPL_BSS_MAX_SIZE }




+OUTPUT_FORMAT("elf32-littlearm", "elf32-littlearm", "elf32-littlearm")
+OUTPUT_ARCH(arm)
+ENTRY(_start)
+SECTIONS
+{

.text      :
{
__start = .;


arch/arm/cpu/armv7/start.o	(.text*)


*(.text*)


} >.sram

. = ALIGN(4);
.rodata : { *(SORT_BY_ALIGNMENT(.rodata*)) } >.sram

. = ALIGN(4);
.data : { *(SORT_BY_ALIGNMENT(.data*)) } >.sram

. = ALIGN(4);
__image_copy_end = .;
_end = .;

.bss :
{
. = ALIGN(4);


__bss_start = .;


*(.bss*)


. = ALIGN(4);


__bss_end = .;


} >.sdram

+}
diff --git a/arch/arm/include/asm/arch-at91/at91_common.h b/arch/arm/include/asm/arch-at91/at91_common.h
index abcb97d..3ca4d5b 100644
--- a/arch/arm/include/asm/arch-at91/at91_common.h
+++ b/arch/arm/include/asm/arch-at91/at91_common.h
@@ -22,5 +22,9 @@ void at91_spi1_hw_init(unsigned long cs_mask);
 void at91_udp_hw_init(void);
 void at91_uhp_hw_init(void);
 void at91_lcd_hw_init(void);
+void at91_plla_init(u32 pllar);
+void at91_mck_init(u32 mckr);
+void at91_pmc_init(void);
+void mem_init(void);
#endif /* AT91_COMMON_H */
diff --git a/arch/arm/include/asm/arch-at91/spl.h b/arch/arm/include/asm/arch-at91/spl.h
new file mode 100644
index 0000000..68c5349
--- /dev/null
+++ b/arch/arm/include/asm/arch-at91/spl.h
@@ -0,0 +1,20 @@
+/*


Copyright (C) 2013 Atmel Corporation



      Bo Shen <voice.shen@atmel.com>









SPDX-License-Identifier:	GPL-2.0+


*/


+#ifndef	_ASM_ARCH_SPL_H_
+#define	_ASM_ARCH_SPL_H_



+enum {

BOOT_DEVICE_NONE,

+#ifdef CONFIG_SYS_USE_MMC

BOOT_DEVICE_MMC1,
BOOT_DEVICE_MMC2,
BOOT_DEVICE_MMC2_2,

+#endif
+};



+#endif
diff --git a/board/atmel/sama5d3xek/sama5d3xek.c b/board/atmel/sama5d3xek/sama5d3xek.c
index f245f98..846a1b8 100644
--- a/board/atmel/sama5d3xek/sama5d3xek.c
+++ b/board/atmel/sama5d3xek/sama5d3xek.c
@@ -20,6 +20,9 @@
 #include <micrel.h>
 #include <net.h>
 #include <netdev.h>
+#include <spl.h>
+#include <asm/arch/atmel_mpddrc.h>
+#include <asm/arch/at91_wdt.h>
#ifdef CONFIG_USB_GADGET_ATMEL_USBA
 #include <asm/arch/atmel_usba_udc.h>
@@ -296,3 +299,82 @@ void spi_cs_deactivate(struct spi_slave *slave)
   }
 }
 #endif /* CONFIG_ATMEL_SPI */



+/* SPL */
+#ifdef CONFIG_SPL_BUILD
+void spl_board_init(void)
+{
+#ifdef CONFIG_SYS_USE_MMC

sama5d3xek_mci_hw_init();

+#endif
+}



+void ddr2_conf(struct atmel_mpddr *ddr2)
+{

ddr2->mdr = (ATMEL_MPDDRC_MDR_DBW_32BITS | ATMEL_MPDDRC_MDR_DDR2_SDRAM);

ddr2->cr = (ATMEL_MPDDRC_CR_NC_COL_10 |
    ATMEL_MPDDRC_CR_NR_ROW_14 |


    ATMEL_MPDDRC_CR_CAS_3 |


    ATMEL_MPDDRC_CR_EN_ENRDM |


    ATMEL_MPDDRC_CR_NB_8BANKS |


    ATMEL_MPDDRC_CR_DIS_NDQS |


    ATMEL_MPDDRC_CR_DECOD_INTERLEAVED |


    ATMEL_MPDDRC_CR_UNAL_SUPPORTED);



ddr2->rtr = 0x411;

Please document magic 0x411.
...


ddr2->tp0r = (6 << ATMEL_MPDDRC_TP0R_TRAS_OFFSET |
      2 << ATMEL_MPDDRC_TP0R_TRCD_OFFSET |


      2 << ATMEL_MPDDRC_TP0R_TWR_OFFSET |


      8 << ATMEL_MPDDRC_TP0R_TRC_OFFSET |


      2 << ATMEL_MPDDRC_TP0R_TRP_OFFSET |


      2 << ATMEL_MPDDRC_TP0R_TRRD_OFFSET |


      2 << ATMEL_MPDDRC_TP0R_TWTR_OFFSET |


      2 << ATMEL_MPDDRC_TP0R_TMRD_OFFSET);



ddr2->tp1r = (2 << ATMEL_MPDDRC_TP1R_TXP_OFFSET |
      200 << ATMEL_MPDDRC_TP1R_TXSRD_OFFSET |


      28 << ATMEL_MPDDRC_TP1R_TXSNR_OFFSET |


      26 << ATMEL_MPDDRC_TP1R_TRFC_OFFSET);



ddr2->tp2r = (7 << ATMEL_MPDDRC_TP2R_TFAW_OFFSET |
      2 << ATMEL_MPDDRC_TP2R_TRTP_OFFSET |


      2 << ATMEL_MPDDRC_TP2R_TRPA_OFFSET |


      7 << ATMEL_MPDDRC_TP2R_TXARDS_OFFSET |


      8 << ATMEL_MPDDRC_TP2R_TXARD_OFFSET);



+}
NAK, that is just copying data from ro section to bss section, please
setup a static struct atmel_mpddr with static initialization. I can't
see use cases where this won't work. You could have just more than one
register setup and choose the correct one. Or modify a single instance
before feeding it into ddr2_init().
...



+void mem_init(void)
+{

struct at91_pmc *pmc = (struct at91_pmc *)ATMEL_BASE_PMC;
struct atmel_mpddr ddr2;

ddr2_conf(&ddr2);

/* enable MPDDR clock */
at91_periph_clk_enable(ATMEL_ID_MPDDRC);
writel(0x4, &pmc->scer);

/* DDRAM2 Controller initialize */
ddr2_init(ATMEL_BASE_DDRCS, &ddr2);

+}



+void at91_pmc_init(void)
+{

struct at91_pmc *pmc = (struct at91_pmc *)ATMEL_BASE_PMC;
u32 tmp;

tmp = AT91_PMC_PLLAR_29 |
     AT91_PMC_PLLXR_PLLCOUNT(0x3f) |


     AT91_PMC_PLLXR_MUL(43) |


     AT91_PMC_PLLXR_DIV(1);


at91_plla_init(tmp);

That could also be written with
writel(tmp, &pmc->pllar);
Ok, and wait for lock ... You know what I mean? As mentioned above it is
just writing the register, not really much abstraction for that
at91_plla_init() function.
But please do not remove that function. I think it worth to have it in
the current setup.
But I also think we could do it better, for example the board just calls
some function to setup 400MHz (and will then possibly use another
frequency cause the given oscillators could not reach exactely that
frequency).
...


writel(0x3 << 8, &pmc->pllicpr);

Datasheet says PLLICPR->IPLL_PLLA should be written to '0'. Why do you
write it to three? Is there something missing in the spec?
...


tmp = AT91_PMC_MCKR_MDIV_4 |
     AT91_PMC_MCKR_CSS_PLLA;


at91_mck_init(tmp);

+}
+#endif
diff --git a/include/configs/sama5d3xek.h b/include/configs/sama5d3xek.h
index 79c0068..9c80e82 100644
--- a/include/configs/sama5d3xek.h
+++ b/include/configs/sama5d3xek.h
@@ -25,7 +25,10 @@
 #define CONFIG_AT91FAMILY
 #define CONFIG_ARCH_CPU_INIT
+#ifndef CONFIG_SPL_BUILD
 #define CONFIG_SKIP_LOWLEVEL_INIT
+#endif



#define CONFIG_BOARD_EARLY_INIT_F
 #define CONFIG_DISPLAY_CPUINFO
@@ -94,8 +97,12 @@
 #define CONFIG_SYS_SDRAM_BASE           ATMEL_BASE_DDRCS
 #define CONFIG_SYS_SDRAM_SIZE		0x20000000
+#ifdef CONFIG_SPL_BUILD
+#define CONFIG_SYS_INIT_SP_ADDR		0x310000
+#else
 #define CONFIG_SYS_INIT_SP_ADDR \
   (CONFIG_SYS_SDRAM_BASE + 4 * 1024 - GENERATED_GBL_DATA_SIZE)
+#endif
/* SerialFlash */
 #define CONFIG_CMD_SF
@@ -235,4 +242,31 @@
 /* Size of malloc() pool */
 #define CONFIG_SYS_MALLOC_LEN		(1024 * 1024)
+/* SPL */
+#define CONFIG_SPL
+#define CONFIG_SPL_FRAMEWORK
+#define CONFIG_SPL_TEXT_BASE		0x300000
+#define CONFIG_SPL_MAX_SIZE		0x10000
+#define CONFIG_SPL_BSS_START_ADDR	0x20000000
+#define CONFIG_SPL_BSS_MAX_SIZE		0x80000
+#define CONFIG_SYS_SPL_MALLOC_START	0x20080000
+#define CONFIG_SYS_SPL_MALLOC_SIZE	0x80000



+#define CONFIG_SPL_LIBCOMMON_SUPPORT
+#define CONFIG_SPL_LIBGENERIC_SUPPORT
+#define CONFIG_SPL_GPIO_SUPPORT
+#define CONFIG_SPL_SERIAL_SUPPORT



+#define CONFIG_SPL_BOARD_INIT
+#ifdef CONFIG_SYS_USE_MMC
+#define CONFIG_SPL_LDSCRIPT		arch/arm/cpu/at91-common/u-boot-spl.lds
+#define CONFIG_SPL_MMC_SUPPORT
+#define CONFIG_SYS_U_BOOT_MAX_SIZE_SECTORS	0x400
+#define CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR 0x200
+#define CONFIG_SYS_MMC_SD_FAT_BOOT_PARTITION	1
+#define CONFIG_SPL_FAT_LOAD_PAYLOAD_NAME	"u-boot.img"
+#define CONFIG_SPL_FAT_SUPPORT
+#define CONFIG_SPL_LIBDISK_SUPPORT
+#endif



#endif
Looks good to me. The only thing I'd really like to change now is the
ddr setup with a function, the rest should be discussed.
Best regards
Andreas Bießmann

    