Hello Sughosh, I reviewed your patch and I agree with Heiko's comments. I am looking forward to v3 :-) Regards, Christian
On Wed, Jan 11, 2012 at 8:53 AM, Sughosh Ganu <urwithsugh...@gmail.com> wrote: > hi Heiko, > > On Wed Jan 11, 2012 at 07:52:02AM +0100, Heiko Schocher wrote: >> Hello Sughosh, >> >> Sughosh Ganu wrote: >> > This patch moves hawkboard to the new spl infrastructure from the >> > older nand_spl one. >> > >> > Removed the hawkboard_nand_config build option -- The spl code now >> > gets compiled with hawkboard_config, after building the main u-boot >> > image, using the CONFIG_SPL_TEXT_BASE. Modified the README.hawkboard >> > to reflect the same. >> > >> > Signed-off-by: Sughosh Ganu <urwithsugh...@gmail.com> >> > Cc: Heiko Schocher <h...@denx.de> >> > Cc: Christian Riesch <christian.rie...@omicron.at> >> > Cc: Sudhakar Rajashekhara <sudhakar....@ti.com> >> > Cc: Tom Rini <tr...@ti.com> >> > --- >> > >> [...] >> > diff --git a/arch/arm/cpu/arm926ejs/davinci/da850_lowlevel.c >> > b/arch/arm/cpu/arm926ejs/davinci/da850_lowlevel.c >> > index a532f8a..a4778b8 100644 >> > --- a/arch/arm/cpu/arm926ejs/davinci/da850_lowlevel.c >> > +++ b/arch/arm/cpu/arm926ejs/davinci/da850_lowlevel.c >> > @@ -32,6 +32,7 @@ >> > #include <asm/arch/emif_defs.h> >> > #include <asm/arch/pll_defs.h> >> > >> > +#if !defined(CONFIG_MACH_DAVINCI_HAWK) >> >> Please no board specific defines. > > Ok, will replace with the arch specific define that you suggest. > >> >> > void da850_waitloop(unsigned long loopcnt) >> > { >> > unsigned long i; >> > @@ -235,6 +236,7 @@ int da850_ddr_setup(void) >> > >> > return 0; >> > } >> > +#endif /* CONFIG_MACH_DAVINCI_HAWK */ >> > >> > __attribute__((weak)) >> > void board_gpio_init(void) >> > @@ -242,10 +244,6 @@ void board_gpio_init(void) >> > return; >> > } >> > >> > -/* pinmux_resource[] vector is defined in the board specific file */ >> > -extern const struct pinmux_resource pinmuxes[]; >> > -extern const int pinmuxes_size; >> > - >> > int arch_cpu_init(void) >> > { >> > /* Unlock kick registers */ >> > @@ -259,6 +257,7 @@ int arch_cpu_init(void) >> > if (davinci_configure_pin_mux_items(pinmuxes, pinmuxes_size)) >> > return 1; >> > >> > +#if defined(CONFIG_MACH_DAVINCI_DA850_EVM) >> >> here too. I propose here a CONFIG_SYS_DA850_PLL_INIT ... > > Ok. > >> >> > /* PLL setup */ >> > da850_pll_init(davinci_pllc0_regs, CONFIG_SYS_DA850_PLL0_PLLM); >> > da850_pll_init(davinci_pllc1_regs, CONFIG_SYS_DA850_PLL1_PLLM); >> > @@ -275,6 +274,12 @@ int arch_cpu_init(void) >> > #endif >> > >> > lpsc_on(CONFIG_SYS_DA850_LPSC_UART); >> > + >> > + da850_ddr_setup(); >> >> and around the ddr_setup a CONFIG_SYS_DA850_DDR_INIT ... >> >> > +#elif defined(CONFIG_MACH_DAVINCI_HAWK) >> > + da8xx_configure_lpsc_items(lpsc, lpsc_size); >> > +#endif >> >> and we should use da8xx_configure_lpsc_items() for all da850 boards. > > Ok. > >> >> This patch breaks current enbw_cmc board compile >> >> [hs@pollux u-boot]$ ./MAKEALL enbw_cmc >> Configuring for enbw_cmc board... >> enbw_cmc.c:52:35: error: static declaration of 'lpsc' follows non-static >> declaration >> /work/hs/u-boot/include/asm/arch/da850_lowlevel.h:33:35: note: previous >> declaration of 'lpsc' was here >> make[1]: *** [enbw_cmc.o] Fehler 1 >> make: *** [board/enbw/enbw_cmc/libenbw_cmc.o] Fehler 2 >> arm-linux-gnueabi-size: './u-boot': No such file > > Oops, sorry about that. > > <snip> > >> diff --git a/include/configs/enbw_cmc.h b/include/configs/enbw_cmc.h >> index c427dc7..804846d 100644 >> --- a/include/configs/enbw_cmc.h >> +++ b/include/configs/enbw_cmc.h >> @@ -48,6 +48,8 @@ >> #define CONFIG_SKIP_LOWLEVEL_INIT >> #define CONFIG_DA850_LOWLEVEL >> #define CONFIG_ARCH_CPU_INIT >> +#define CONFIG_SYS_DA850_PLL_INIT >> +#define CONFIG_SYS_DA850_DDR_INIT > > I think we'll need to add this for da850evm.h too. > >> #define CONFIG_DA8XX_GPIO >> #define CONFIG_HOSTNAME enbw_cmc >> #define CONFIG_DISPLAY_CPUINFO >> -- >> 1.7.7.4 >> >> [...] >> > diff --git a/include/configs/cam_enc_4xx.h b/include/configs/cam_enc_4xx.h >> > index a21d448..8e3a4d2 100644 >> > --- a/include/configs/cam_enc_4xx.h >> > +++ b/include/configs/cam_enc_4xx.h >> > @@ -205,6 +205,7 @@ >> > >> > /* Defines for SPL */ >> > #define CONFIG_SPL >> > +#define CONFIG_DM365_SPL >> >> Why we need this define? > > Yeah, missed out cleaning these defines that i had used in V1. Will > clean up in next version for all the cases. > > -sughosh > _______________________________________________ > 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