Re: [U-Boot] [PATCH 8/9] SMDK5250: Enable eMMC booting

Hi Amar,

On Mon, Dec 17, 2012 at 3:19 AM, Amar amarendra.xt@samsung.com wrote:

This patch adds support for eMMC booting on SMDK5250

Signed-off-by: Amar amarendra.xt@samsung.com

board/samsung/smdk5250/spl_boot.c | 38 ++++++++++++++++++++++++++++++++++++- 1 files changed, 37 insertions(+), 1 deletions(-)

diff --git a/board/samsung/smdk5250/spl_boot.c b/board/samsung/smdk5250/spl_boot.c index d8f3c1e..2648b4e 100644 --- a/board/samsung/smdk5250/spl_boot.c +++ b/board/samsung/smdk5250/spl_boot.c @@ -23,15 +23,40 @@ #include<common.h> #include<config.h>

+#include <asm/arch/clock.h> +#include <asm/arch/clk.h>

+#define FSYS1_MMC0_DIV_VAL 0x0701

Should go in arch/arm/include/... ?

enum boot_mode { BOOT_MODE_MMC = 4,

•   BOOT_MODE_eMMC = 8, /* eMMC44 */
  
  

I think should you use upper case E, although I'm not completely sure.

    BOOT_MODE_SERIAL = 20,
    /* Boot based on Operating Mode pin settings */
    BOOT_MODE_OM = 32,
    BOOT_MODE_USB,  /* Boot using USB download */

};

•   typedef u32 (*spi_copy_func_t)(u32 offset, u32 nblock, u32 dst);
  

+typedef u32 (*spi_copy_func_t)(u32 offset, u32 nblock, u32 dst);

Should avoid adding typedefs.

+static void set_emmc_clk(void);

+/* +* Set MMC0 clock divisor value. +* So that the mmc0 device operating freq is 50MHz.

Do we only support booting from mmc0? That's fine, but I suggest adding a little comment.

+*/ +static void set_emmc_clk(void) +{

•   struct exynos5_clock *clk = (struct exynos5_clock
  

*)EXYNOS5_CLOCK_BASE;

•   unsigned int addr;
  

•   unsigned int div_mmc;
  

•   addr = (unsigned int) &clk->div_fsys1;
  

•   div_mmc = readl(addr) & ~FSYS1_MMC0_DIV_MASK;
  

•   div_mmc |= FSYS1_MMC0_DIV_VAL;
  

•   writel(div_mmc, addr);
  
  

Can this function go in clock.c?

+}

/*

• Copy U-boot from mmc to RAM:

@@ -43,6 +68,8 @@ void copy_uboot_to_ram(void) spi_copy_func_t spi_copy; enum boot_mode bootmode; u32 (*copy_bl2)(u32, u32, u32);

•   u32 (*copy_bl2_emmc)(u32, u32);
  

•   void (*end_bootop_emmc)(void);
  
    bootmode = readl(EXYNOS5_POWER_BASE) & OM_STAT;
  
  

@@ -57,6 +84,15 @@ void copy_uboot_to_ram(void) copy_bl2(BL2_START_OFFSET, BL2_SIZE_BLOC_COUNT, CONFIG_SYS_TEXT_BASE); break;

•   case BOOT_MODE_eMMC:
  

•           set_emmc_clk();
  

•           end_bootop_emmc = (void *) *(u32
  

*)EMMC44_END_BOOTOP_FNPTR_ADDR;

•           copy_bl2_emmc = (void *) *(u32
  

*)EMMC44_COPY_BL2_FNPTR_ADDR;

I think these are pointers to functions in the IROM. Do they have the same signature? Is it possible to create a table of them somewhere instead of using defines?

•           copy_bl2_emmc(BL2_SIZE_BLOC_COUNT, CONFIG_SYS_TEXT_BASE);
  

•           end_bootop_emmc();
  

•           break;
  

•   default:
            break;
    }
  

-- 1.7.0.4

Regards, Simon