[U-Boot] [PATCH V6 2/5] omap-common: add nand spl support

[Simon Schwarz <simonschwarzcor@googlemail.com>]

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