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

[Simon Schwarz <simonschwarzcor@googlemail.com>]

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