On 04/13/2012 11:22 PM, Wolfgang Denk wrote:
Dear Tom Rini,

In message<1334355606-16491-9-git-send-email-tr...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Reply via email to