Hi Amarendra,
On Thu, Dec 20, 2012 at 5:53 AM, Amarendra Reddy amar.lavanuru@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@chromium.org wrote:
Hi Amar,
On Mon, Dec 17, 2012 at 3:19 AM, Amar amarendra.xt@samsung.com wrote:
This patch adds support for eMMC booting on SMDK5250
Signed-off-by: Amar amarendra.xt@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@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Regards, Simon