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

Wolfgang Denk
2012-04-14T06:22:23Z

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.

> --- 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.

> 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>

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

Best regards,

Wolfgang Denk

--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"Wish not to seem, but to be, the best." - Aeschylus