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

[Simon Glass]

Hi Amarendra,

On Thu, Dec 20, 2012 at 5:53 AM, Amarendra Reddy
<amar.lavanuru at gmail.com> wrote:
> 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.

Yes that seems reasonable. There is a lot of code in clock_init that
should eventually move to U-Boot proper (i.e. we should only init
clocks in SPL that are actually needed in SPL). This can be addressed
after Hatim (copied) has finished creating the upstream patches for
clock_init / lowlevel_init refactor.

>
>
>>
>> > +}
>> > +
>> >
>> >  /*
>> >  * 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         */
>         };

Well I suggest just having addresses in the table (ulong) instead of
pointers if you can, so there are fewer casts.

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

Seems reasonable, but please do create an enum for the available
function numbers rather than just using 3 or 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
>>
>

Regards,
Simon