[U-Boot] [PATCH 8/9] SMDK5250: Enable eMMC booting
Amarendra Reddy
amar.lavanuru at gmail.com
Thu Dec 27 04:55:29 CET 2012
Hi Simon,
Thanks for review comments.
Please find my responses below.
Thanks & Regards
Amarendra
On 27 December 2012 01:29, Simon Glass <sjg at chromium.org> wrote:
> 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.
> Ok.
> >
> >
> >>
> >> > +}
> >> > +
> >> >
> >> > /*
> >> > * 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.
> Ok.
> >
> > 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.
> Yes, I have used enum in actual implementation. The above was an example.
>
>
> >
> >>
> >> > +
> >> > + 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
>
More information about the U-Boot
mailing list