Hi Simon, Thanks for review comments. Please find my responses below. Thanks & Regards Amarendra
On 27 December 2012 01:29, Simon Glass <s...@chromium.org> wrote: > Hi Amarendra, > > On Thu, Dec 20, 2012 at 5:53 AM, Amarendra Reddy > <amar.lavan...@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 <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. > > 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@lists.denx.de > >> http://lists.denx.de/mailman/listinfo/u-boot > >> > > > > Regards, > Simon >
_______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot