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

[Simon Glass]

Hi Amar,

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

> This patch adds support for eMMC booting on SMDK5250
>
> Signed-off-by: Amar <amarendra.xt at 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