On 04/13/2012 11:22 PM, Wolfgang Denk wrote:
Dear Tom Rini,
In message1334355606-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!