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 <s...@chromium.org> wrote:
> Hi Amar,
>
> On Mon, Dec 17, 2012 at 3:19 AM, Amar <amarendra...@samsung.com> wrote:
>
> > This patch adds support for eMMC booting on SMDK5250
> >
> > Signed-off-by: Amar <amarendra...@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@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>
>
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
