| Message ID | 1311842291-24837-3-git-send-email-simonschwarzcor@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
Hi Simon, On Thursday 28 July 2011 02:08 PM, Simon Schwarz wrote: > Add NAND support for the new SPL structure. > > Signed-off-by: Simon Schwarz<simonschwarzcor@gmail.com> > --- > This patch didn't exist before V2! > > V2 changes: > ADD Some define-barriers for OMAP3 to only use NAND > ADD nand_load_image() - inits the OMAP gpmc, loads the images - parses the > header > CHG cosmetic > ADD do_reset() implementation for omap-common spl > ADD nand_copy_image to nand.h > ADD CPP barriers for mmc and nand support. The parts depending on library > support are only compiled if the respective library is included. > > V3 changes: > ADD Comment why setup_clocks_for_console() isn't called for OMAP3 > CHG cosmetic (deleted empty line) > CHG rename of NAND_MODE_HW to NAND_MODE_HW_ECC > DEL NAND_MODE_SW. Not used. > > V4 changes: > CHG cosmetic - style problems > > V5 changes: > CHG renamed nand_copy_image to nand_spl_load_image > CHG offs paramter of nand_spl_load_image is of type loff_t now > > V6 changes: > ADD call to nand_deselect after loading the images > ADD nand_deselect to nand.h > > Transition from V1 to V2 also includes that this patch is now based on > - the new SPL layout by Aneesh V and Daniel Schwierzeck > - the OMAP4 SPL patches by Aneesh V > --- > arch/arm/cpu/armv7/omap-common/spl.c | 47 ++++++++++++++++++++++++++++++++++ > arch/arm/include/asm/omap_common.h | 1 + > include/nand.h | 3 ++ > 3 files changed, 51 insertions(+), 0 deletions(-) > > diff --git a/arch/arm/cpu/armv7/omap-common/spl.c b/arch/arm/cpu/armv7/omap-common/spl.c > index d177652..7ec5c7c 100644 > --- a/arch/arm/cpu/armv7/omap-common/spl.c > +++ b/arch/arm/cpu/armv7/omap-common/spl.c > @@ -26,6 +26,7 @@ > #include<asm/u-boot.h> > #include<asm/utils.h> > #include<asm/arch/sys_proto.h> > +#include<nand.h> > #include<mmc.h> > #include<fat.h> > #include<timestamp_autogenerated.h> > @@ -107,6 +108,7 @@ static void parse_image_header(const struct image_header *header) > } > } > > +#ifdef CONFIG_SPL_MMC_SUPPORT > static void mmc_load_image_raw(struct mmc *mmc) > { > u32 image_size_sectors, err; > @@ -140,7 +142,9 @@ end: > hang(); > } > } > +#endif /* CONFIG_SPL_MMC_SUPPORT */ here.. > > +#ifdef CONFIG_SPL_MMC_SUPPORT > static void mmc_load_image_fat(struct mmc *mmc) > { > s32 err; > @@ -173,7 +177,9 @@ end: > hang(); > } > } > +#endif /* CONFIG_SPL_MMC_SUPPORT */ and here.. You start the same the #ifdef again immediately after the #endif. Why don't you club them together into just one #ifdef block. Actually, since we have garbage collection of un-used functions, I think doing the calls under #ifdef should be enough, which you have taken care in board_init_r(). That may help to avoid some #ifdef clutter. > > +#ifdef CONFIG_SPL_MMC_SUPPORT > static void mmc_load_image(void) > { > struct mmc *mmc; > @@ -206,6 +212,28 @@ static void mmc_load_image(void) > hang(); > } > } > +#endif /* CONFIG_SPL_MMC_SUPPORT */ > + > +#ifdef CONFIG_SPL_NAND_SUPPORT > +static void nand_load_image(void) > +{ > + gpmc_init(); > + nand_init(); > + nand_spl_load_image(CONFIG_SYS_NAND_U_BOOT_OFFS, > + CONFIG_SYS_NAND_U_BOOT_SIZE, > + (uchar *)CONFIG_SYS_NAND_U_BOOT_DST); > +#ifdef CONFIG_NAND_ENV_DST > + nand_spl_load_image(CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE, > + (uchar *)CONFIG_NAND_ENV_DST); > +#ifdef CONFIG_ENV_OFFSET_REDUND > + nand_spl_load_image(CONFIG_ENV_OFFSET_REDUND, CONFIG_ENV_SIZE, > + (uchar *)CONFIG_NAND_ENV_DST + CONFIG_ENV_SIZE); > +#endif > +#endif > + nand_deselect(); > + parse_image_header((struct image_header *)CONFIG_SYS_NAND_U_BOOT_DST); > +} > +#endif /* CONFIG_SPL_NAND_SUPPORT */ > > void jump_to_image_no_args(void) > { > @@ -228,10 +256,17 @@ void board_init_r(gd_t *id, ulong dummy) > boot_device = omap_boot_device(); > debug("boot device - %d\n", boot_device); > switch (boot_device) { > +#ifdef CONFIG_SPL_MMC_SUPPORT > case BOOT_DEVICE_MMC1: > case BOOT_DEVICE_MMC2: > mmc_load_image(); > break; > +#endif > +#ifdef CONFIG_SPL_NAND_SUPPORT > + case BOOT_DEVICE_NAND: > + nand_load_image(); > + break; > +#endif > default: > printf("SPL: Un-supported Boot Device - %d!!!\n", boot_device); > hang(); > @@ -259,7 +294,11 @@ void preloader_console_init(void) > gd->flags |= GD_FLG_RELOC; > gd->baudrate = CONFIG_BAUDRATE; > > +/* Console clock for OMAP3 is already initialized by per_clocks_enable() > + * called in board.c by s_init() */ > +#ifndef CONFIG_OMAP34XX > setup_clocks_for_console(); > +#endif Please do one of the solutions Andreas suggested instead of having that ifndef. br, Aneesh
Hi Simon, On Thursday 28 July 2011 02:08 PM, Simon Schwarz wrote: [snip ..] > + > +int do_reset(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) > +{ > + debug("resetting cpu..."); > + reset_cpu(0); > + > + return 0; > +} Can you explain the need of this do_reset()? I couldn't figure out where it is used in SPL. br, Aneesh
On 07/28/2011 11:42 AM, Aneesh V wrote: > Hi Simon, > > On Thursday 28 July 2011 02:08 PM, Simon Schwarz wrote: >> Add NAND support for the new SPL structure. >> >> Signed-off-by: Simon Schwarz<simonschwarzcor@gmail.com> >> --- >> This patch didn't exist before V2! >> >> V2 changes: >> ADD Some define-barriers for OMAP3 to only use NAND >> ADD nand_load_image() - inits the OMAP gpmc, loads the images - parses >> the >> header >> CHG cosmetic >> ADD do_reset() implementation for omap-common spl >> ADD nand_copy_image to nand.h >> ADD CPP barriers for mmc and nand support. The parts depending on library >> support are only compiled if the respective library is included. >> >> V3 changes: >> ADD Comment why setup_clocks_for_console() isn't called for OMAP3 >> CHG cosmetic (deleted empty line) >> CHG rename of NAND_MODE_HW to NAND_MODE_HW_ECC >> DEL NAND_MODE_SW. Not used. >> >> V4 changes: >> CHG cosmetic - style problems >> >> V5 changes: >> CHG renamed nand_copy_image to nand_spl_load_image >> CHG offs paramter of nand_spl_load_image is of type loff_t now >> >> V6 changes: >> ADD call to nand_deselect after loading the images >> ADD nand_deselect to nand.h >> >> Transition from V1 to V2 also includes that this patch is now based on >> - the new SPL layout by Aneesh V and Daniel Schwierzeck >> - the OMAP4 SPL patches by Aneesh V >> --- >> arch/arm/cpu/armv7/omap-common/spl.c | 47 >> ++++++++++++++++++++++++++++++++++ >> arch/arm/include/asm/omap_common.h | 1 + >> include/nand.h | 3 ++ >> 3 files changed, 51 insertions(+), 0 deletions(-) >> >> diff --git a/arch/arm/cpu/armv7/omap-common/spl.c >> b/arch/arm/cpu/armv7/omap-common/spl.c >> index d177652..7ec5c7c 100644 >> --- a/arch/arm/cpu/armv7/omap-common/spl.c >> +++ b/arch/arm/cpu/armv7/omap-common/spl.c >> @@ -26,6 +26,7 @@ >> #include<asm/u-boot.h> >> #include<asm/utils.h> >> #include<asm/arch/sys_proto.h> >> +#include<nand.h> >> #include<mmc.h> >> #include<fat.h> >> #include<timestamp_autogenerated.h> >> @@ -107,6 +108,7 @@ static void parse_image_header(const struct >> image_header *header) >> } >> } >> >> +#ifdef CONFIG_SPL_MMC_SUPPORT >> static void mmc_load_image_raw(struct mmc *mmc) >> { >> u32 image_size_sectors, err; >> @@ -140,7 +142,9 @@ end: >> hang(); >> } >> } >> +#endif /* CONFIG_SPL_MMC_SUPPORT */ > > here.. > >> >> +#ifdef CONFIG_SPL_MMC_SUPPORT >> static void mmc_load_image_fat(struct mmc *mmc) >> { >> s32 err; >> @@ -173,7 +177,9 @@ end: >> hang(); >> } >> } >> +#endif /* CONFIG_SPL_MMC_SUPPORT */ > > and here.. > > You start the same the #ifdef again immediately after the #endif. Why > don't you club them together into just one #ifdef block. IMHO #ifdef each function makes it more readable but... > > Actually, since we have garbage collection of un-used functions, I > think doing the calls under #ifdef should be enough, which you have > taken care in board_init_r(). That may help to avoid some #ifdef > clutter. ...I take this way :) I had to define some MMC specific stuff in the board config (copied it from OMAP4 - not tested) and added __attribute__((unused)) to mmc_load_image to prevent the warning if the Library is not used. Did this also for NAND. -> next version Maybe it is a good idea to have some dummy values for the unused libs in SPL? With a compile-time warning that is emitted when no value is set but the library is activated. >> >> +#ifdef CONFIG_SPL_MMC_SUPPORT >> static void mmc_load_image(void) >> { >> struct mmc *mmc; >> @@ -206,6 +212,28 @@ static void mmc_load_image(void) >> hang(); >> } >> } >> +#endif /* CONFIG_SPL_MMC_SUPPORT */ >> + >> +#ifdef CONFIG_SPL_NAND_SUPPORT >> +static void nand_load_image(void) >> +{ >> + gpmc_init(); >> + nand_init(); >> + nand_spl_load_image(CONFIG_SYS_NAND_U_BOOT_OFFS, >> + CONFIG_SYS_NAND_U_BOOT_SIZE, >> + (uchar *)CONFIG_SYS_NAND_U_BOOT_DST); >> +#ifdef CONFIG_NAND_ENV_DST >> + nand_spl_load_image(CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE, >> + (uchar *)CONFIG_NAND_ENV_DST); >> +#ifdef CONFIG_ENV_OFFSET_REDUND >> + nand_spl_load_image(CONFIG_ENV_OFFSET_REDUND, CONFIG_ENV_SIZE, >> + (uchar *)CONFIG_NAND_ENV_DST + CONFIG_ENV_SIZE); >> +#endif >> +#endif >> + nand_deselect(); >> + parse_image_header((struct image_header *)CONFIG_SYS_NAND_U_BOOT_DST); >> +} >> +#endif /* CONFIG_SPL_NAND_SUPPORT */ >> >> void jump_to_image_no_args(void) >> { >> @@ -228,10 +256,17 @@ void board_init_r(gd_t *id, ulong dummy) >> boot_device = omap_boot_device(); >> debug("boot device - %d\n", boot_device); >> switch (boot_device) { >> +#ifdef CONFIG_SPL_MMC_SUPPORT >> case BOOT_DEVICE_MMC1: >> case BOOT_DEVICE_MMC2: >> mmc_load_image(); >> break; >> +#endif >> +#ifdef CONFIG_SPL_NAND_SUPPORT >> + case BOOT_DEVICE_NAND: >> + nand_load_image(); >> + break; >> +#endif >> default: >> printf("SPL: Un-supported Boot Device - %d!!!\n", boot_device); >> hang(); >> @@ -259,7 +294,11 @@ void preloader_console_init(void) >> gd->flags |= GD_FLG_RELOC; >> gd->baudrate = CONFIG_BAUDRATE; >> >> +/* Console clock for OMAP3 is already initialized by per_clocks_enable() >> + * called in board.c by s_init() */ >> +#ifndef CONFIG_OMAP34XX >> setup_clocks_for_console(); >> +#endif > > Please do one of the solutions Andreas suggested instead of having that > ifndef. This is done in the next version. > > br, > Aneesh Regards & as always thanks for your review! Simon
Hi Aneesh, On 07/28/2011 11:58 AM, Aneesh V wrote: > Hi Simon, > > On Thursday 28 July 2011 02:08 PM, Simon Schwarz wrote: > [snip ..] >> + >> +int do_reset(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) >> +{ >> + debug("resetting cpu..."); >> + reset_cpu(0); >> + >> + return 0; >> +} > > Can you explain the need of this do_reset()? I couldn't figure out > where it is used in SPL. Thats odd. I added this because of vsprintf - this calls do_reset() which is not included in the SPL. This was in an early stage of SPL and OMAP4 patches. Seems this is not needed anymore: will delete it for the next version. > br, > Aneesh Regards & thx for the review! Simon
Hi Simon, On Thursday 28 July 2011 02:08 PM, Simon Schwarz wrote: > Add NAND support for the new SPL structure. > > Signed-off-by: Simon Schwarz<simonschwarzcor@gmail.com> > --- > This patch didn't exist before V2! > > V2 changes: > ADD Some define-barriers for OMAP3 to only use NAND > ADD nand_load_image() - inits the OMAP gpmc, loads the images - parses the > header > CHG cosmetic > ADD do_reset() implementation for omap-common spl > ADD nand_copy_image to nand.h > ADD CPP barriers for mmc and nand support. The parts depending on library > support are only compiled if the respective library is included. > > V3 changes: > ADD Comment why setup_clocks_for_console() isn't called for OMAP3 > CHG cosmetic (deleted empty line) > CHG rename of NAND_MODE_HW to NAND_MODE_HW_ECC > DEL NAND_MODE_SW. Not used. > > V4 changes: > CHG cosmetic - style problems > > V5 changes: > CHG renamed nand_copy_image to nand_spl_load_image > CHG offs paramter of nand_spl_load_image is of type loff_t now > > V6 changes: > ADD call to nand_deselect after loading the images > ADD nand_deselect to nand.h > > Transition from V1 to V2 also includes that this patch is now based on > - the new SPL layout by Aneesh V and Daniel Schwierzeck > - the OMAP4 SPL patches by Aneesh V > --- > arch/arm/cpu/armv7/omap-common/spl.c | 47 ++++++++++++++++++++++++++++++++++ > arch/arm/include/asm/omap_common.h | 1 + > include/nand.h | 3 ++ > 3 files changed, 51 insertions(+), 0 deletions(-) > > diff --git a/arch/arm/cpu/armv7/omap-common/spl.c b/arch/arm/cpu/armv7/omap-common/spl.c > index d177652..7ec5c7c 100644 > --- a/arch/arm/cpu/armv7/omap-common/spl.c > +++ b/arch/arm/cpu/armv7/omap-common/spl.c > @@ -26,6 +26,7 @@ > #include<asm/u-boot.h> > #include<asm/utils.h> > #include<asm/arch/sys_proto.h> > +#include<nand.h> > #include<mmc.h> > #include<fat.h> > #include<timestamp_autogenerated.h> > @@ -107,6 +108,7 @@ static void parse_image_header(const struct image_header *header) > } > } > > +#ifdef CONFIG_SPL_MMC_SUPPORT > static void mmc_load_image_raw(struct mmc *mmc) > { > u32 image_size_sectors, err; > @@ -140,7 +142,9 @@ end: > hang(); > } > } > +#endif /* CONFIG_SPL_MMC_SUPPORT */ > > +#ifdef CONFIG_SPL_MMC_SUPPORT > static void mmc_load_image_fat(struct mmc *mmc) > { > s32 err; > @@ -173,7 +177,9 @@ end: > hang(); > } > } > +#endif /* CONFIG_SPL_MMC_SUPPORT */ > > +#ifdef CONFIG_SPL_MMC_SUPPORT > static void mmc_load_image(void) > { > struct mmc *mmc; > @@ -206,6 +212,28 @@ static void mmc_load_image(void) > hang(); > } > } > +#endif /* CONFIG_SPL_MMC_SUPPORT */ > + > +#ifdef CONFIG_SPL_NAND_SUPPORT > +static void nand_load_image(void) > +{ > + gpmc_init(); > + nand_init(); > + nand_spl_load_image(CONFIG_SYS_NAND_U_BOOT_OFFS, > + CONFIG_SYS_NAND_U_BOOT_SIZE, > + (uchar *)CONFIG_SYS_NAND_U_BOOT_DST); Guess CONFIG_SYS_NAND_U_BOOT_DST is same as CONFIG_SYS_TEXT_BASE. Why define a new flag then? > +#ifdef CONFIG_NAND_ENV_DST > + nand_spl_load_image(CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE, > + (uchar *)CONFIG_NAND_ENV_DST); > +#ifdef CONFIG_ENV_OFFSET_REDUND > + nand_spl_load_image(CONFIG_ENV_OFFSET_REDUND, CONFIG_ENV_SIZE, > + (uchar *)CONFIG_NAND_ENV_DST + CONFIG_ENV_SIZE); > +#endif > +#endif > + nand_deselect(); > + parse_image_header((struct image_header *)CONFIG_SYS_NAND_U_BOOT_DST); I think you are assuming the image type and size here. Why not do this as it is done for MMC. That is, read just 1 sector, parse the buffer, find the size of the image and read only that much subsequently. You will have to then use u-boot.img instead of u-boot.bin as the payload. Maybe you should clone arch/arm/cpu/armv7/omap4/config.mk in omap3 folder to get MLO and u-boot.img targets. best regards, Aneesh
On Thursday 28 July 2011 06:14 PM, Simon Schwarz wrote: [snip ..] >>> +#endif /* CONFIG_SPL_MMC_SUPPORT */ >> >> and here.. >> >> You start the same the #ifdef again immediately after the #endif. Why >> don't you club them together into just one #ifdef block. > IMHO #ifdef each function makes it more readable but... Ok. I don't have any strong views on that. > >> >> Actually, since we have garbage collection of un-used functions, I >> think doing the calls under #ifdef should be enough, which you have >> taken care in board_init_r(). That may help to avoid some #ifdef >> clutter. > ...I take this way :) > > I had to define some MMC specific stuff in the board config (copied it Does your board have MMC support? Then it definitely makes sense to keep MMC support enabled. > from OMAP4 - not tested) and added __attribute__((unused)) to > mmc_load_image to prevent the warning if the Library is not used. Did GCC give warning without this change? I think it doesn't due to -ffunction-sections? best regards, Aneesh
Hi Aneesh, On 07/28/2011 04:24 PM, Aneesh V wrote: > On Thursday 28 July 2011 06:14 PM, Simon Schwarz wrote: > [snip ..] > >>>> +#endif /* CONFIG_SPL_MMC_SUPPORT */ >>> >>> and here.. >>> >>> You start the same the #ifdef again immediately after the #endif. Why >>> don't you club them together into just one #ifdef block. >> IMHO #ifdef each function makes it more readable but... > > Ok. I don't have any strong views on that. > >> >>> >>> Actually, since we have garbage collection of un-used functions, I >>> think doing the calls under #ifdef should be enough, which you have >>> taken care in board_init_r(). That may help to avoid some #ifdef >>> clutter. >> ...I take this way :) >> >> I had to define some MMC specific stuff in the board config (copied it > > Does your board have MMC support? Then it definitely makes sense to > keep MMC support enabled. Yes it has MMC. If NAND works and is stable enough I will go for MMC. > >> from OMAP4 - not tested) and added __attribute__((unused)) to >> mmc_load_image to prevent the warning if the Library is not used. > > Did GCC give warning without this change? I think it doesn't due to > -ffunction-sections? It does issue warnings: spl.c:178:13: warning: 'mmc_load_image' defined but not used > > best regards, > Aneesh Regards Simon
On Thu, 28 Jul 2011 19:46:25 +0530 Aneesh V <aneesh@ti.com> wrote: > On Thursday 28 July 2011 02:08 PM, Simon Schwarz wrote: > > +#ifdef CONFIG_SPL_NAND_SUPPORT > > +static void nand_load_image(void) > > +{ > > + gpmc_init(); > > + nand_init(); > > + nand_spl_load_image(CONFIG_SYS_NAND_U_BOOT_OFFS, > > + CONFIG_SYS_NAND_U_BOOT_SIZE, > > + (uchar *)CONFIG_SYS_NAND_U_BOOT_DST); > > Guess CONFIG_SYS_NAND_U_BOOT_DST is same as CONFIG_SYS_TEXT_BASE. Why > define a new flag then? History from when CONFIG_SYS_TEXT_BASE was just TEXT_BASE and there was no separate CONFIG_SYS_TEXT_BASE_SPL. Also, what if there's a middle step before the final U-Boot? This is a define that's used across all platforms. > > +#ifdef CONFIG_NAND_ENV_DST > > + nand_spl_load_image(CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE, > > + (uchar *)CONFIG_NAND_ENV_DST); > > +#ifdef CONFIG_ENV_OFFSET_REDUND > > + nand_spl_load_image(CONFIG_ENV_OFFSET_REDUND, CONFIG_ENV_SIZE, > > + (uchar *)CONFIG_NAND_ENV_DST + CONFIG_ENV_SIZE); > > +#endif > > +#endif > > + nand_deselect(); > > + parse_image_header((struct image_header *)CONFIG_SYS_NAND_U_BOOT_DST); > > I think you are assuming the image type and size here. This is how all the other NAND SPLs do it. We're building both at the same time to create a single combination image, so it's not that bad of an assumption. An image header might simplify the chained spl/tpl case, though. -Scott
Dear Scott Wood, In message <20110728135005.03c97ac0@schlenkerla.am.freescale.net> you wrote: > > This is how all the other NAND SPLs do it. We're building both at the same > time to create a single combination image, so it's not that bad of an > assumption. > > An image header might simplify the chained spl/tpl case, though. I think we should have a (simple) image header, that at least provides information about the size - but eventually we might want to know the load and entry point addresses as well, so we could as well just reuse struct image_header for this purpose, too. One use case we should keep in mind here is not to load the regular U-Boot image, but something completely different - like a Linux kernel, for example. Best regards, Wolfgang Denk
Hi Scott, Simon, On Friday 29 July 2011 12:20 AM, Scott Wood wrote: > On Thu, 28 Jul 2011 19:46:25 +0530 > Aneesh V<aneesh@ti.com> wrote: > >> On Thursday 28 July 2011 02:08 PM, Simon Schwarz wrote: >>> +#ifdef CONFIG_SPL_NAND_SUPPORT >>> +static void nand_load_image(void) >>> +{ >>> + gpmc_init(); >>> + nand_init(); >>> + nand_spl_load_image(CONFIG_SYS_NAND_U_BOOT_OFFS, >>> + CONFIG_SYS_NAND_U_BOOT_SIZE, >>> + (uchar *)CONFIG_SYS_NAND_U_BOOT_DST); >> >> Guess CONFIG_SYS_NAND_U_BOOT_DST is same as CONFIG_SYS_TEXT_BASE. Why >> define a new flag then? > > History from when CONFIG_SYS_TEXT_BASE was just TEXT_BASE and there was no > separate CONFIG_SYS_TEXT_BASE_SPL. Also, what if there's a middle step > before the final U-Boot? This is a define that's used across all > platforms. > Perhaps he should not use any hard-coded destination. Please see below. >>> +#ifdef CONFIG_NAND_ENV_DST >>> + nand_spl_load_image(CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE, >>> + (uchar *)CONFIG_NAND_ENV_DST); >>> +#ifdef CONFIG_ENV_OFFSET_REDUND >>> + nand_spl_load_image(CONFIG_ENV_OFFSET_REDUND, CONFIG_ENV_SIZE, >>> + (uchar *)CONFIG_NAND_ENV_DST + CONFIG_ENV_SIZE); >>> +#endif >>> +#endif >>> + nand_deselect(); >>> + parse_image_header((struct image_header *)CONFIG_SYS_NAND_U_BOOT_DST); >> >> I think you are assuming the image type and size here. > > This is how all the other NAND SPLs do it. We're building both at the same > time to create a single combination image, so it's not that bad of an > assumption. For OMAP, we are not building a combination image. OMAP typically allows booting from a FAT file system in an MMC card in addition to raw boot from eMMC/NAND. So, we have separate files for SPL/x-loader and u-boot. The SPL file(MLO) is picked up by ROM code and u-boot is picked up by x-loader/SPL. The first version of my MMC SPL too was also making assumptions about the image. But Wolfgang asked me to make it more generic. According to this I added the necessary code to parse a mkimage header and find the image size, load address, image type etc from the image itself and use these while loading the image and while jumping to it. This gives us more flexibility about the second image. 1. In raw boot the second image can be changed anytime by flashing a new image(let's say kernel uImage) at the pre-defined location for the second image. 2. In FAT mode just change CONFIG_SPL_FAT_LOAD_PAYLOAD_NAME and you can load a different image with mkimage header. Only the jumping part needs some customization for different types of images if parameters are passed. I would like Simon to do it the same way for NAND. Here he is calling parse_image_header() but only after loading the entire image. That helps in the jumping part. But he could make the loading part also generic. best regards, Aneesh
Hi Scott, Aneesh, to short the discussion: I already work on the implementation of using the image header. I think a new version will be released late today. Regards & thx for your reviews! Simon
Hi Aneesh, <snip> >> diff --git a/arch/arm/cpu/armv7/omap-common/spl.c >> b/arch/arm/cpu/armv7/omap-common/spl.c >> index d177652..7ec5c7c 100644 >> --- a/arch/arm/cpu/armv7/omap-common/spl.c >> +++ b/arch/arm/cpu/armv7/omap-common/spl.c >> @@ -26,6 +26,7 @@ >> #include<asm/u-boot.h> >> #include<asm/utils.h> >> #include<asm/arch/sys_proto.h> >> +#include<nand.h> >> #include<mmc.h> >> #include<fat.h> >> #include<timestamp_autogenerated.h> >> @@ -107,6 +108,7 @@ static void parse_image_header(const struct >> image_header *header) >> } >> } >> >> +#ifdef CONFIG_SPL_MMC_SUPPORT >> static void mmc_load_image_raw(struct mmc *mmc) >> { >> u32 image_size_sectors, err; >> @@ -140,7 +142,9 @@ end: >> hang(); >> } >> } >> +#endif /* CONFIG_SPL_MMC_SUPPORT */ > > here.. > >> >> +#ifdef CONFIG_SPL_MMC_SUPPORT >> static void mmc_load_image_fat(struct mmc *mmc) >> { >> s32 err; >> @@ -173,7 +177,9 @@ end: >> hang(); >> } >> } >> +#endif /* CONFIG_SPL_MMC_SUPPORT */ > > and here.. > > You start the same the #ifdef again immediately after the #endif. Why > don't you club them together into just one #ifdef block. > > Actually, since we have garbage collection of un-used functions, I > think doing the calls under #ifdef should be enough, which you have > taken care in board_init_r(). That may help to avoid some #ifdef > clutter. I have to dig out this mail again. I found that removing the ifdefs breaks OMAP4 since it lacks some NAND specific defines. So I wonder - is it ok to define them to some garbage-value in the file and emit an #error if the corresponding LIB is set in SPL? This would state in the file that these defines are used and it would avoid the need of defining them. This would look like: #ifndef CONFIG_SPL_NAND_BLA #ifdef CONFIG_SPL_NAND_SUPPORT #error CONFIG_SPL_NAND_BLA is not set although \ CONFIG_SPL_NAND_SUPPORT is active #else #define CONFIG_SPL_NAND_BLA 0x0 #endif #endif Maybe this can be simplified by some macro... I find it really annoying to set values for code I don't want to use... Regards Simon
Hi Simon, On Tuesday 02 August 2011 05:33 PM, Simon Schwarz wrote: > Hi Aneesh, > > <snip> >>> diff --git a/arch/arm/cpu/armv7/omap-common/spl.c >>> b/arch/arm/cpu/armv7/omap-common/spl.c >>> index d177652..7ec5c7c 100644 >>> --- a/arch/arm/cpu/armv7/omap-common/spl.c >>> +++ b/arch/arm/cpu/armv7/omap-common/spl.c >>> @@ -26,6 +26,7 @@ >>> #include<asm/u-boot.h> >>> #include<asm/utils.h> >>> #include<asm/arch/sys_proto.h> >>> +#include<nand.h> >>> #include<mmc.h> >>> #include<fat.h> >>> #include<timestamp_autogenerated.h> >>> @@ -107,6 +108,7 @@ static void parse_image_header(const struct >>> image_header *header) >>> } >>> } >>> >>> +#ifdef CONFIG_SPL_MMC_SUPPORT >>> static void mmc_load_image_raw(struct mmc *mmc) >>> { >>> u32 image_size_sectors, err; >>> @@ -140,7 +142,9 @@ end: >>> hang(); >>> } >>> } >>> +#endif /* CONFIG_SPL_MMC_SUPPORT */ >> >> here.. >> >>> >>> +#ifdef CONFIG_SPL_MMC_SUPPORT >>> static void mmc_load_image_fat(struct mmc *mmc) >>> { >>> s32 err; >>> @@ -173,7 +177,9 @@ end: >>> hang(); >>> } >>> } >>> +#endif /* CONFIG_SPL_MMC_SUPPORT */ >> >> and here.. >> >> You start the same the #ifdef again immediately after the #endif. Why >> don't you club them together into just one #ifdef block. >> >> Actually, since we have garbage collection of un-used functions, I >> think doing the calls under #ifdef should be enough, which you have >> taken care in board_init_r(). That may help to avoid some #ifdef >> clutter. > > I have to dig out this mail again. I found that removing the ifdefs > breaks OMAP4 since it lacks some NAND specific defines. > > So I wonder - is it ok to define them to some garbage-value in the file > and emit an #error if the corresponding LIB is set in SPL? > > This would state in the file that these defines are used and it would > avoid the need of defining them. > > This would look like: > #ifndef CONFIG_SPL_NAND_BLA > #ifdef CONFIG_SPL_NAND_SUPPORT > #error CONFIG_SPL_NAND_BLA is not set although \ > CONFIG_SPL_NAND_SUPPORT is active > #else > #define CONFIG_SPL_NAND_BLA 0x0 > #endif > #endif > > Maybe this can be simplified by some macro... > > I find it really annoying to set values for code I don't want to use... Maybe, what we could do is to split the file into two: spl_mmc.c spl_nand.c or three: spl.c spl_mmc.c spl_nand.c and then in the Makefile conditionally include them based on CONFIG_SPL_NAND_SUPPORT, CONFIG_SPL_MMC_SUPPORT etc. I think this may solve your difficulties. best regards, Aneesh
Hi Aneesh, On 08/02/2011 02:18 PM, Aneesh V wrote: > Hi Simon, > > On Tuesday 02 August 2011 05:33 PM, Simon Schwarz wrote: >> Hi Aneesh, >> >> <snip> >>>> diff --git a/arch/arm/cpu/armv7/omap-common/spl.c >>>> b/arch/arm/cpu/armv7/omap-common/spl.c >>>> index d177652..7ec5c7c 100644 >>>> --- a/arch/arm/cpu/armv7/omap-common/spl.c >>>> +++ b/arch/arm/cpu/armv7/omap-common/spl.c >>>> @@ -26,6 +26,7 @@ >>>> #include<asm/u-boot.h> >>>> #include<asm/utils.h> >>>> #include<asm/arch/sys_proto.h> >>>> +#include<nand.h> >>>> #include<mmc.h> >>>> #include<fat.h> >>>> #include<timestamp_autogenerated.h> >>>> @@ -107,6 +108,7 @@ static void parse_image_header(const struct >>>> image_header *header) >>>> } >>>> } >>>> >>>> +#ifdef CONFIG_SPL_MMC_SUPPORT >>>> static void mmc_load_image_raw(struct mmc *mmc) >>>> { >>>> u32 image_size_sectors, err; >>>> @@ -140,7 +142,9 @@ end: >>>> hang(); >>>> } >>>> } >>>> +#endif /* CONFIG_SPL_MMC_SUPPORT */ >>> >>> here.. >>> >>>> >>>> +#ifdef CONFIG_SPL_MMC_SUPPORT >>>> static void mmc_load_image_fat(struct mmc *mmc) >>>> { >>>> s32 err; >>>> @@ -173,7 +177,9 @@ end: >>>> hang(); >>>> } >>>> } >>>> +#endif /* CONFIG_SPL_MMC_SUPPORT */ >>> >>> and here.. >>> >>> You start the same the #ifdef again immediately after the #endif. Why >>> don't you club them together into just one #ifdef block. >>> >>> Actually, since we have garbage collection of un-used functions, I >>> think doing the calls under #ifdef should be enough, which you have >>> taken care in board_init_r(). That may help to avoid some #ifdef >>> clutter. >> >> I have to dig out this mail again. I found that removing the ifdefs >> breaks OMAP4 since it lacks some NAND specific defines. >> >> So I wonder - is it ok to define them to some garbage-value in the file >> and emit an #error if the corresponding LIB is set in SPL? >> >> This would state in the file that these defines are used and it would >> avoid the need of defining them. >> >> This would look like: >> #ifndef CONFIG_SPL_NAND_BLA >> #ifdef CONFIG_SPL_NAND_SUPPORT >> #error CONFIG_SPL_NAND_BLA is not set although \ >> CONFIG_SPL_NAND_SUPPORT is active >> #else >> #define CONFIG_SPL_NAND_BLA 0x0 >> #endif >> #endif >> >> Maybe this can be simplified by some macro... >> >> I find it really annoying to set values for code I don't want to use... > > Maybe, what we could do is to split the file into two: > spl_mmc.c > spl_nand.c > > or three: > spl.c > spl_mmc.c > spl_nand.c > > and then in the Makefile conditionally include them based on > CONFIG_SPL_NAND_SUPPORT, CONFIG_SPL_MMC_SUPPORT etc. I think this may > solve your difficulties. Seems to be the simplest solution -> will do. > best regards, > Aneesh Regards Simon
diff --git a/arch/arm/cpu/armv7/omap-common/spl.c b/arch/arm/cpu/armv7/omap-common/spl.c index d177652..7ec5c7c 100644 --- a/arch/arm/cpu/armv7/omap-common/spl.c +++ b/arch/arm/cpu/armv7/omap-common/spl.c @@ -26,6 +26,7 @@ #include <asm/u-boot.h> #include <asm/utils.h> #include <asm/arch/sys_proto.h> +#include <nand.h> #include <mmc.h> #include <fat.h> #include <timestamp_autogenerated.h> @@ -107,6 +108,7 @@ static void parse_image_header(const struct image_header *header) } } +#ifdef CONFIG_SPL_MMC_SUPPORT static void mmc_load_image_raw(struct mmc *mmc) { u32 image_size_sectors, err; @@ -140,7 +142,9 @@ end: hang(); } } +#endif /* CONFIG_SPL_MMC_SUPPORT */ +#ifdef CONFIG_SPL_MMC_SUPPORT static void mmc_load_image_fat(struct mmc *mmc) { s32 err; @@ -173,7 +177,9 @@ end: hang(); } } +#endif /* CONFIG_SPL_MMC_SUPPORT */ +#ifdef CONFIG_SPL_MMC_SUPPORT static void mmc_load_image(void) { struct mmc *mmc; @@ -206,6 +212,28 @@ static void mmc_load_image(void) hang(); } } +#endif /* CONFIG_SPL_MMC_SUPPORT */ + +#ifdef CONFIG_SPL_NAND_SUPPORT +static void nand_load_image(void) +{ + gpmc_init(); + nand_init(); + nand_spl_load_image(CONFIG_SYS_NAND_U_BOOT_OFFS, + CONFIG_SYS_NAND_U_BOOT_SIZE, + (uchar *)CONFIG_SYS_NAND_U_BOOT_DST); +#ifdef CONFIG_NAND_ENV_DST + nand_spl_load_image(CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE, + (uchar *)CONFIG_NAND_ENV_DST); +#ifdef CONFIG_ENV_OFFSET_REDUND + nand_spl_load_image(CONFIG_ENV_OFFSET_REDUND, CONFIG_ENV_SIZE, + (uchar *)CONFIG_NAND_ENV_DST + CONFIG_ENV_SIZE); +#endif +#endif + nand_deselect(); + parse_image_header((struct image_header *)CONFIG_SYS_NAND_U_BOOT_DST); +} +#endif /* CONFIG_SPL_NAND_SUPPORT */ void jump_to_image_no_args(void) { @@ -228,10 +256,17 @@ void board_init_r(gd_t *id, ulong dummy) boot_device = omap_boot_device(); debug("boot device - %d\n", boot_device); switch (boot_device) { +#ifdef CONFIG_SPL_MMC_SUPPORT case BOOT_DEVICE_MMC1: case BOOT_DEVICE_MMC2: mmc_load_image(); break; +#endif +#ifdef CONFIG_SPL_NAND_SUPPORT + case BOOT_DEVICE_NAND: + nand_load_image(); + break; +#endif default: printf("SPL: Un-supported Boot Device - %d!!!\n", boot_device); hang(); @@ -259,7 +294,11 @@ void preloader_console_init(void) gd->flags |= GD_FLG_RELOC; gd->baudrate = CONFIG_BAUDRATE; +/* Console clock for OMAP3 is already initialized by per_clocks_enable() + * called in board.c by s_init() */ +#ifndef CONFIG_OMAP34XX setup_clocks_for_console(); +#endif serial_init(); /* serial communications setup */ /* Avoid a second "U-Boot" coming from this string */ @@ -270,3 +309,11 @@ void preloader_console_init(void) omap_rev_string(rev_string_buffer); printf("Texas Instruments %s\n", rev_string_buffer); } + +int do_reset(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) +{ + debug("resetting cpu..."); + reset_cpu(0); + + return 0; +} diff --git a/arch/arm/include/asm/omap_common.h b/arch/arm/include/asm/omap_common.h index d3cb857..13f6884 100644 --- a/arch/arm/include/asm/omap_common.h +++ b/arch/arm/include/asm/omap_common.h @@ -49,6 +49,7 @@ void preloader_console_init(void); #define MMCSD_MODE_UNDEFINED 0 #define MMCSD_MODE_RAW 1 #define MMCSD_MODE_FAT 2 +#define NAND_MODE_HW_ECC 3 u32 omap_boot_device(void); u32 omap_boot_mode(void); diff --git a/include/nand.h b/include/nand.h index 8d94b5c..3c5ef4e 100644 --- a/include/nand.h +++ b/include/nand.h @@ -132,6 +132,9 @@ int nand_lock( nand_info_t *meminfo, int tight ); int nand_unlock( nand_info_t *meminfo, ulong start, ulong length ); int nand_get_lock_status(nand_info_t *meminfo, loff_t offset); +void nand_spl_load_image(loff_t offs, unsigned int size, uchar *dst); +void nand_deselect(void); + #ifdef CONFIG_SYS_NAND_SELECT_DEVICE void board_nand_select_device(struct nand_chip *nand, int chip); #endif
Add NAND support for the new SPL structure. Signed-off-by: Simon Schwarz <simonschwarzcor@gmail.com> --- This patch didn't exist before V2! V2 changes: ADD Some define-barriers for OMAP3 to only use NAND ADD nand_load_image() - inits the OMAP gpmc, loads the images - parses the header CHG cosmetic ADD do_reset() implementation for omap-common spl ADD nand_copy_image to nand.h ADD CPP barriers for mmc and nand support. The parts depending on library support are only compiled if the respective library is included. V3 changes: ADD Comment why setup_clocks_for_console() isn't called for OMAP3 CHG cosmetic (deleted empty line) CHG rename of NAND_MODE_HW to NAND_MODE_HW_ECC DEL NAND_MODE_SW. Not used. V4 changes: CHG cosmetic - style problems V5 changes: CHG renamed nand_copy_image to nand_spl_load_image CHG offs paramter of nand_spl_load_image is of type loff_t now V6 changes: ADD call to nand_deselect after loading the images ADD nand_deselect to nand.h Transition from V1 to V2 also includes that this patch is now based on - the new SPL layout by Aneesh V and Daniel Schwierzeck - the OMAP4 SPL patches by Aneesh V --- arch/arm/cpu/armv7/omap-common/spl.c | 47 ++++++++++++++++++++++++++++++++++ arch/arm/include/asm/omap_common.h | 1 + include/nand.h | 3 ++ 3 files changed, 51 insertions(+), 0 deletions(-)