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

[Amarendra Reddy]

Hi SImon,

Thanks for the review comments.
Please find below the responses for your comments.

Thanks & Regards
Amarendra

On 20 December 2012 08:05, Simon Glass <sjg at chromium.org> wrote:

> 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/... ?
>
> OK. shall move it.
> > +
> >  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.
> OK. will make it upper case to be consistent every where.
>
> >         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.
> Ok.
>
> > +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.
> OK.
>
> > +*/
> > +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?
> This function is used by SPL, only during EMMC boot.
> Hence can be moved into board/samsung/smdk5250/clock_init.c.
>
 Please comment on this.
>


> > +}
> > +
> >
> >  /*
> >  * 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?
> OK.
>
 The signatures are different for different booting devices.
More over, SDMMC / SPI / USB booting requires only one function pointer.
Where as EMMC 4.3/4.4 requires two of those function pointers.

May be something like this can be used to create table
void (*ptr_table[])(void) = {
        (void *)0x02020030, /* iROM Function Pointer - SDMMC boot       */
        (void *)0x0202003C, /* iROM Function Pointer - EMMC 4.3 boot    */
        (void *)0x02020040, /* iROM Function Pointer - EMMC 4.3 end boot op
*/
        (void *)0x02020044, /* iROM Function Pointer - EMMC 4.4 boot    */
        (void *)0x02020048, /* iROM Function Pointer - EMMC 4.4 end boot op
*/
        (void *)0x02020050, /* iROM Function Pointer - EFNAND boot      */
        (void *)0x02020058, /* iROM Function Pointer - SPI boot         */
        (void *)0x02020070  /* iROM Function Pointer - USB boot         */
        };

Usage:
    copy_bl2_from_emmc = (void *) *(u32 *)ptr_table[3];

> end_bootop_from_emmc = (void *) *(u32 *)ptr_table[4];
>


> > +
> > +               copy_bl2_emmc(BL2_SIZE_BLOC_COUNT, CONFIG_SYS_TEXT_BASE);
> > +               end_bootop_emmc();
> > +               break;
> > +
> >         default:
> >                 break;
> >         }
> > --
> > 1.7.0.4
> >
> >
> Regards,
> Simon
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>
>