From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tom Rini Date: Mon, 16 Apr 2012 08:09:09 -0700 Subject: [U-Boot] [PATCH 08/08] include/configs: Better utilize CONFIG_SYS_NO_FLASH In-Reply-To: <20120414062224.4190920022F@gemini.denx.de> References: <1334355606-16491-1-git-send-email-trini@ti.com> <1334355606-16491-9-git-send-email-trini@ti.com> <20120414062224.4190920022F@gemini.denx.de> Message-ID: <4F8C3615.9040401@ti.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.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 > ... >> -#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 >> -#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 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