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

Tom Rini
2012-04-16T15:09:09Z

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