[U-Boot] [PATCH 08/08] include/configs: Better utilize CONFIG_SYS_NO_FLASH

From: Tom Rini <trini@ti.com>
To: u-boot@lists.denx.de
Subject: [U-Boot] [PATCH 08/08] include/configs: Better utilize CONFIG_SYS_NO_FLASH
Date: Mon, 16 Apr 2012 08:09:09 -0700	[thread overview]
Message-ID: <4F8C3615.9040401@ti.com> (raw)
In-Reply-To: <20120414062224.4190920022F@gemini.denx.de>

On 04/13/2012 11:22 PM, Wolfgang Denk wrote:
> Dear Tom Rini,
>
> In message<1334355606-16491-9-git-send-email-trini@ti.com>  you wrote:
>> In config files which it is clear when CONFIG_SYS_NO_FLASH will be set
>
> s/which it is clear when/where it is clear that/ ?
>
>> (either unconditionally or based on logic that can happen early in the
>> config file), ensure that we set that _before_ we include
>> config_cmd_default.h so that the logic in that file will not enable
>> CONFIG_CMD_(FLASH|IMLS).  This saves us from having to undef it in the
>> board files.
>
> I'm not sure if I like that.  It makes the code not easier to
> understand, but actually less obvious.

So, at the end of the day it's an artifact of using cpp to make our 
configs rather than something like Kconfig.

>
>> --- a/include/configs/PN62.h
>> +++ b/include/configs/PN62.h
>> @@ -56,13 +56,12 @@
>>   /*
>>    * Command line configuration.
>>    */
>> +#define CONFIG_SYS_NO_FLASH		/* There is no FLASH memory	*/
>>   #include<config_cmd_default.h>
> ...
>> -#define CONFIG_SYS_NO_FLASH		1		/* There is no FLASH memory	*/
>> -
>>   #define CONFIG_ENV_IS_NOWHERE	1		/* Store ENV in memory only	*/
>>   #define CONFIG_ENV_OFFSET		0x00004000	/* Offset of Environment Sector */
>>   #define CONFIG_ENV_SIZE		0x00002000	/* Total Size of Environment Sector */
>
> Logically, it makes more sense to me to look for the NO_FLASH setting
> close to where we are dfinging such things - the relation to the
> "Command line configuration" is pretty obscure.

I agree.  Some boards are laid out like this already, and in my 
re-factoring of some of the TI parts config files,

>> diff --git a/include/configs/SIMPC8313.h b/include/configs/SIMPC8313.h
>> index 0976077..8b51f6c 100644
>> --- a/include/configs/SIMPC8313.h
>> +++ b/include/configs/SIMPC8313.h
>> @@ -105,8 +105,6 @@
>>   /*
>>    * FLASH on the Local Bus
>>    */
>> -#define CONFIG_SYS_NO_FLASH
>> -
>>   #if !defined(CONFIG_NAND_SPL)
>>   #define CONFIG_SYS_RAMBOOT
>>   #endif
>> @@ -347,9 +345,8 @@
>>   /*
>>    * Command line configuration.
>>    */
>> +#define CONFIG_SYS_NO_FLASH
>>   #include<config_cmd_default.h>
>> -#undef CONFIG_CMD_IMLS
>> -#undef CONFIG_CMD_FLASH
>
> That's even worse here.
>
> I actually tend to believe it is a bad idea to have the #ifdef for
> CONFIG_SYS_NO_FLASH in<config_cmd_default.h>

And perhaps we should drop IMLS/FLASH from config_cmd_default these days?

> Sorry, but I don't think this patch is a good idea.

OK, I'll drop it from the general clean-ups and re-factor the configs 
I'm focusing on to have cmds done near the end.  Thanks!

-- 
Tom