[U-Boot-Users] [PATCH] fix compilation problem for mpc8349itx CFG_RAMBOOT

[U-Boot-Users] [PATCH] fix compilation problem for mpc8349itx CFG_RAMBOOT

[Nikita V. Youshchenko]

Current include/configs/MPC8349ITX.h does contain some support for building
image that will be started from memory (without putting in into flash).
It could be triggered by building with TEXT_BASE set to a low value.

However, this support is incomplete: using of low TEXT_BASE causes
defining configuration macros in inconsistent way, which later leads
to compilation errors. In particular. flash support is being disabled,
but then flash structures get referenced.

This patch fixes this, making it possible to build with low TEXT_BASE.

Signed-Off-By: Nikita Youshchenko <yoush@debian.org>

--- a/include/configs/MPC8349ITX.h
+++ b/include/configs/MPC8349ITX.h
@@ -408,6 +409,7 @@ boards, we say we have two, but don't di
   #define CFG_ENV_SIZE         0x2000
 #else
   #define CFG_NO_FLASH         /* Flash is not usable now */
+  #undef CFG_FLASH_CFI_DRIVER
   #define CFG_ENV_IS_NOWHERE   /* Store ENV in memory only */
   #define CFG_ENV_ADDR         (CFG_MONITOR_BASE - 0x1000)
   #define CFG_ENV_SIZE         0x2000
@@ -436,7 +438,14 @@ boards, we say we have two, but don't di
 #define CONFIG_COMMANDS_I2C    0
 #endif

-#define CONFIG_COMMANDS                (CONFIG_CMD_DFL | \
+#ifdef CFG_NO_FLASH
+#define CONFIG_COMMANDS_DEFAULT (CONFIG_CMD_DFL & ~(CFG_CMD_FLASH | \
+                                                   CFG_CMD_IMLS))
+#else
+#define CONFIG_COMMANDS_DEFAULT CONFIG_CMD_DFL
+#endif
+
+#define CONFIG_COMMANDS                (CONFIG_COMMANDS_DEFAULT | \
                                CONFIG_COMMANDS_CF      | \
                                CFG_CMD_NET     | \
                                CFG_CMD_PING    | \

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
Url : http://lists.denx.de/pipermail/u-boot/attachments/20070523/fa9f208f/attachment.pgp 

[U-Boot-Users] [PATCH] fix compilation problem for mpc8349itx CFG_RAMBOOT

[Timur Tabi]

Nikita V. Youshchenko wrote:
> Current include/configs/MPC8349ITX.h does contain some support for building
> image that will be started from memory (without putting in into flash).
> It could be triggered by building with TEXT_BASE set to a low value.

Well that's ironic.  I was just about to remove support for ramboot altogether from all 
8xxx boards.

I guess I won't be doing that.  However, I have to NACK your patch for one reason:

> -#define CONFIG_COMMANDS                (CONFIG_CMD_DFL | \
> +#ifdef CFG_NO_FLASH
> +#define CONFIG_COMMANDS_DEFAULT (CONFIG_CMD_DFL & ~(CFG_CMD_FLASH | \
> +                                                   CFG_CMD_IMLS))
> +#else
> +#define CONFIG_COMMANDS_DEFAULT CONFIG_CMD_DFL
> +#endif

Please don't put CONFIG_COMMANDS inside an #ifdef block.  Instead, please follow the 
example of CONFIG_COMMANDS_CF.

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

[U-Boot-Users] [PATCH] fix compilation problem for mpc8349itx CFG_RAMBOOT

[Nikita V. Youshchenko]

> Nikita V. Youshchenko wrote:
> > Current include/configs/MPC8349ITX.h does contain some support for
> > building image that will be started from memory (without putting in
> > into flash). It could be triggered by building with TEXT_BASE set to a
> > low value.
>
> Well that's ironic.  I was just about to remove support for ramboot
> altogether from all 8xxx boards.
>
> I guess I won't be doing that.  However, I have to NACK your patch for
> one reason: 

In my particular scenario I needed ramboot because I work with the board 
remotely and don't have jtag access; so if something will go whong while 
reflashing, the board will be dead.

However, I need updated u-boot to boot kernels that require flat device 
tree.

I guess I'm not alone in such a situation. So please don't remove ramboot 
support.


> > -#define CONFIG_COMMANDS                (CONFIG_CMD_DFL | \
> > +#ifdef CFG_NO_FLASH
> > +#define CONFIG_COMMANDS_DEFAULT (CONFIG_CMD_DFL & ~(CFG_CMD_FLASH | \
> > +                                                   CFG_CMD_IMLS))
> > +#else
> > +#define CONFIG_COMMANDS_DEFAULT CONFIG_CMD_DFL
> > +#endif
>
> Please don't put CONFIG_COMMANDS inside an #ifdef block.  Instead,
> please follow the example of CONFIG_COMMANDS_CF.

Could you please explain what you mean?

I don't put CONFIG_COMMANDS under #ifdef.
I put another macro, CONFIG_COMMANDS_DEFAULT, under ifdef.
And then use it in unconditional CONFIG_COMMANDS definition.
Looks similar to CONFIG_COMMANDS_CF and others.
The difference from CONFIG_COMMANDS_CF is that I have to exclude bits from 
CONFIG_CMD_DFL, not add more commands.

Nikita

[U-Boot-Users] [PATCH] fix compilation problem for mpc8349itx CFG_RAMBOOT

[Timur Tabi]

Nikita V. Youshchenko wrote:

> I guess I'm not alone in such a situation. So please don't remove ramboot 
> support.

Ok, I'll keep it.  But I think you need to expand your patch to fix this problem:

    #define CFG_ENV_ADDR		(CFG_MONITOR_BASE - 0x1000)
    #define CFG_ENV_SIZE		0x2000

The environment is located 0x1000 bytes before the start of U-Boot, but 0x2000 bytes have 
been reserved.

Your patch should probably change the above lines to:

    #define CFG_ENV_SIZE		0x2000
    #define CFG_ENV_ADDR		(CFG_MONITOR_BASE - CFG_ENV_SIZE)

However, Wolfgang says this is still wrong, but he won't explain why.  What do you think?

> Could you please explain what you mean?
> 
> I don't put CONFIG_COMMANDS under #ifdef.

Sorry, I misread your patch.  I have a crappy monitor.

> I put another macro, CONFIG_COMMANDS_DEFAULT, under ifdef.
> And then use it in unconditional CONFIG_COMMANDS definition.
> Looks similar to CONFIG_COMMANDS_CF and others.
> The difference from CONFIG_COMMANDS_CF is that I have to exclude bits from 
> CONFIG_CMD_DFL, not add more commands.

Ok, I understand now.  This part is fine.

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

[U-Boot-Users] [PATCH] fix compilation problem for mpc8349itx CFG_RAMBOOT

[Wolfgang Denk]

In message <465463E1.6050000@freescale.com> you wrote:
> 
>     #define CFG_ENV_SIZE		0x2000
>     #define CFG_ENV_ADDR		(CFG_MONITOR_BASE - CFG_ENV_SIZE)
> 
> However, Wolfgang says this is still wrong, but he won't explain why.  What do you think?

Timur, did you really check the code?

See for example:

"include/configs/MPC8349ITX.h":
    404 #ifndef CFG_RAMBOOT
    ...
    409 #else
    410   #define CFG_NO_FLASH          /* Flash is not usable now */
==> 411   #define CFG_ENV_IS_NOWHERE    /* Store ENV in memory only */
    412   #define CFG_ENV_ADDR          (CFG_MONITOR_BASE - 0x1000)
    413   #define CFG_ENV_SIZE          0x2000
    414 #endif

and

"common/cmd_nvedit.c":

    541 #if defined(CFG_ENV_IS_IN_NVRAM) || defined(CFG_ENV_IS_IN_EEPROM) || \
    542     ((CONFIG_COMMANDS & (CFG_CMD_ENV|CFG_CMD_FLASH)) == \
    543       (CFG_CMD_ENV|CFG_CMD_FLASH)) || \
    544     ((CONFIG_COMMANDS & (CFG_CMD_ENV|CFG_CMD_NAND)) == \
    545       (CFG_CMD_ENV|CFG_CMD_NAND))
    546 int do_saveenv (cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
    547 {
    ...
    553 }
    554
    555
    556 #endif

In other words: in your configuration there is not even an  implemen-
tation of the saveenv() function, so why bother?

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
You go slow, be gentle. It's no one-way street -- you  know  how  you
feel and that's all. It's how the girl feels too. Don't press. If the
girl feels anything for you at all, you'll know.
	-- Kirk, "Charlie X", stardate 1535.8

[U-Boot-Users] [PATCH] fix compilation problem for mpc8349itx CFG_RAMBOOT

[Timur Tabi]

Wolfgang Denk wrote:

> In other words: in your configuration there is not even an  implemen-
> tation of the saveenv() function, so why bother?

Ah, I see now.  So there's no point in defining CFG_ENV_ADDR is CFG_ENV_IS_NOWHERE is also 
defined.

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

[U-Boot-Users] [PATCH] fix compilation problem for mpc8349itx CFG_RAMBOOT

[Timur Tabi]

Nikita V. Youshchenko wrote:
> Current include/configs/MPC8349ITX.h does contain some support for building
> image that will be started from memory (without putting in into flash).
> It could be triggered by building with TEXT_BASE set to a low value.

Question: how exactly do you specify a new value for TEXT_BASE?

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

[U-Boot-Users] [PATCH] fix compilation problem for mpc8349itx CFG_RAMBOOT

[Nikita V. Youshchenko]

> Nikita V. Youshchenko wrote:
> > Current include/configs/MPC8349ITX.h does contain some support for
> > building image that will be started from memory (without putting in
> > into flash). It could be triggered by building with TEXT_BASE set to a
> > low value.
>
> Question: how exactly do you specify a new value for TEXT_BASE?

make TEXT_BASE=0x100000

The resulting u-boot.bin I load using 'tftpboot' command of u-boot that is 
currently on the board, and then start using 'g 100110'. This is the 
_start_warm address. This works.

I've also tried 'g 100100' (_start_cold address), but it results into 
exception. I have not checked why.

Nikita
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
Url : http://lists.denx.de/pipermail/u-boot/attachments/20070523/d9006019/attachment.pgp 

[U-Boot-Users] [PATCH] fix compilation problem for mpc8349itx CFG_RAMBOOT

[Timur Tabi]

Nikita V. Youshchenko wrote:

> I've also tried 'g 100100' (_start_cold address), but it results into 
> exception. I have not checked why.

Probably because the IMMR has been moved from FF400000 to E0000000.  On cold boot, U-Boot 
expects the IMMR to be at FF400000, but instead it's at E0000000.  Address 100100 is the 
"cold boot" address, which assumes a power-on.

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

[U-Boot-Users] [PATCH] fix compilation problem for mpc8349itx CFG_RAMBOOT

[Timur Tabi]

Nikita V. Youshchenko wrote:
> Current include/configs/MPC8349ITX.h does contain some support for building
> image that will be started from memory (without putting in into flash).
> It could be triggered by building with TEXT_BASE set to a low value.
> 
> However, this support is incomplete: using of low TEXT_BASE causes
> defining configuration macros in inconsistent way, which later leads
> to compilation errors. In particular. flash support is being disabled,
> but then flash structures get referenced.
> 
> This patch fixes this, making it possible to build with low TEXT_BASE.
> 
> Signed-Off-By: Nikita Youshchenko <yoush@debian.org>
> 
> --- a/include/configs/MPC8349ITX.h
> +++ b/include/configs/MPC8349ITX.h
> @@ -408,6 +409,7 @@ boards, we say we have two, but don't di
>    #define CFG_ENV_SIZE         0x2000
>  #else
>    #define CFG_NO_FLASH         /* Flash is not usable now */
> +  #undef CFG_FLASH_CFI_DRIVER

I'm not crazy about this.  Basically, we define CFG_FLASH_CFI_DRIVER (and a bunch of other 
flash stuff) at the top of the file, and then here we undefine only one of those macros to 
get the code to compile.

The better approach would be to move this code to the top, and define CFG_FLASH_CFI_DRIVER 
only if CFG_NO_FLASH is not defined.

> -#define CONFIG_COMMANDS                (CONFIG_CMD_DFL | \
> +#ifdef CFG_NO_FLASH
> +#define CONFIG_COMMANDS_DEFAULT (CONFIG_CMD_DFL & ~(CFG_CMD_FLASH | \
> +                                                   CFG_CMD_IMLS))
> +#else
> +#define CONFIG_COMMANDS_DEFAULT CONFIG_CMD_DFL
> +#endif
> +
> +#define CONFIG_COMMANDS                (CONFIG_COMMANDS_DEFAULT | \
>                                 CONFIG_COMMANDS_CF      | \
>                                 CFG_CMD_NET     | \
>                                 CFG_CMD_PING    | \

How about this approach instead:

#ifdef CFG_NO_FLASH
#define CONFIG_COMMANDS_FLASH	~(CFG_CMD_FLASH | CFG_CMD_IMLS)
#else
#define CONFIG_COMMANDS_FLASH	~0
#endif

#define CONFIG_COMMANDS		(CONFIG_CMD_DFL | \
				CONFIG_COMMANDS_CF	| \
				CFG_CMD_NET	| \
				CFG_CMD_PING	| \
				CONFIG_COMMANDS_I2C	| \
				CONFIG_COMMANDS_PCI	| \
				CFG_CMD_SDRAM	| \
				CFG_CMD_DATE	| \
				CFG_CMD_CACHE	| \
				CFG_CMD_IRQ)	& \
				CONFIG_COMMANDS_FLASH

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

[U-Boot-Users] [PATCH] fix compilation problem for mpc8349itx CFG_RAMBOOT

[Wolfgang Denk]

In message <46547E55.3020608@freescale.com> you wrote:
>
> How about this approach instead:
> 
> #ifdef CFG_NO_FLASH
> #define CONFIG_COMMANDS_FLASH	~(CFG_CMD_FLASH | CFG_CMD_IMLS)
> #else
> #define CONFIG_COMMANDS_FLASH	~0
> #endif
> 
> #define CONFIG_COMMANDS		(CONFIG_CMD_DFL | \
> 				CONFIG_COMMANDS_CF	| \
> 				CFG_CMD_NET	| \
> 				CFG_CMD_PING	| \
> 				CONFIG_COMMANDS_I2C	| \
> 				CONFIG_COMMANDS_PCI	| \
> 				CFG_CMD_SDRAM	| \
> 				CFG_CMD_DATE	| \
> 				CFG_CMD_CACHE	| \
> 				CFG_CMD_IRQ)	& \
> 				CONFIG_COMMANDS_FLASH

This is extremely inconsistent. Some CONFIG_COMMANDS_* (PCI, I2C) are
used to add features, while others (FLASH) are used to remove
features. I consinder this very error prone.

Also, you might consider using  better  (i.  e.  local)  preprocessor
variable   names   for   such   functions.  Note  that  CONFIG_*  and
CONFIG_COMMANDS* is kind of reserved by U-Boot, in the sense that  it
has  a  pre-defined  meaning.  You  should avoid using such names for
local variables; you might run into unpleasant  faiulures  sooner  or
later.  In  general,  it  is  always  a good idea to avoid name space
pollution.

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
When some people discover the truth, they just can't  understand  why
everybody isn't eager to hear it.

[U-Boot-Users] [PATCH] fix compilation problem for mpc8349itx CFG_RAMBOOT

[Timur Tabi]

Wolfgang Denk wrote:
> In message <46547E55.3020608@freescale.com> you wrote:
>> How about this approach instead:
>>
>> #ifdef CFG_NO_FLASH
>> #define CONFIG_COMMANDS_FLASH	~(CFG_CMD_FLASH | CFG_CMD_IMLS)
>> #else
>> #define CONFIG_COMMANDS_FLASH	~0
>> #endif
>>
>> #define CONFIG_COMMANDS		(CONFIG_CMD_DFL | \
>> 				CONFIG_COMMANDS_CF	| \
>> 				CFG_CMD_NET	| \
>> 				CFG_CMD_PING	| \
>> 				CONFIG_COMMANDS_I2C	| \
>> 				CONFIG_COMMANDS_PCI	| \
>> 				CFG_CMD_SDRAM	| \
>> 				CFG_CMD_DATE	| \
>> 				CFG_CMD_CACHE	| \
>> 				CFG_CMD_IRQ)	& \
>> 				CONFIG_COMMANDS_FLASH
> 
> This is extremely inconsistent. Some CONFIG_COMMANDS_* (PCI, I2C) are
> used to add features, while others (FLASH) are used to remove
> features. I consinder this very error prone.

Well, it's supposed to be an alternative to what Nikita was proposing.  I wanted to keep 
the concept of CONFIG_CMD_DFL + changes, rather than define CONFIG_CMD_DEFAULT and then 
make changes.

Please take a look at my MPC8349ITX.h to see the full extent of what I was trying to do. 
Nikita is just expanding on the code that's already there, and I'm just refining what 
Nikita is doing.  Basically, I want to avoid putting #define CONFIG_COMMANDS inside an 
ifdef.  As the number of configurable CFG_CMD_xxx options increase, the number of 
variations on CONFIG_COMMANDS grew exponentially.

> Also, you might consider using  better  (i.  e.  local)  preprocessor
> variable   names   for   such   functions.  Note  that  CONFIG_*  and
> CONFIG_COMMANDS* is kind of reserved by U-Boot, in the sense that  it
> has  a  pre-defined  meaning.  You  should avoid using such names for
> local variables; you might run into unpleasant  faiulures  sooner  or
> later.  In  general,  it  is  always  a good idea to avoid name space
> pollution.

Should they be renamed to CFG_COMMANDS_xxx?

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

[U-Boot-Users] [PATCH] fix compilation problem for mpc8349itx CFG_RAMBOOT

[Wolfgang Denk]

In message <46548B5B.4060607@freescale.com> you wrote:
>
> > This is extremely inconsistent. Some CONFIG_COMMANDS_* (PCI, I2C) are
> > used to add features, while others (FLASH) are used to remove
> > features. I consinder this very error prone.
> 
> Well, it's supposed to be an alternative to what Nikita was proposing.  I wanted to keep 

It's bad. Use something like __CMD_ADD_I2C or  __CMD_REMOVE_FLASH  or
so, but don't use the same name for different functions.

> Please take a look at my MPC8349ITX.h to see the full extent of what I was trying to do. 

Yes, I know what it does. Frankly, it's ugly.

> Should they be renamed to CFG_COMMANDS_xxx?

No, as  CFG  also  has  a  specific  meaning.  Here,  you  just  need
temporary,  local  variables.  Please avoid name space pollution. You
can and should even #undef those after use.

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
Committee, n.:  A group of men who individually can do nothing but as
a group decide that nothing can be done.                 - Fred Allen

[U-Boot-Users] [PATCH] fix compilation problem for mpc8349itx CFG_RAMBOOT

[Nikita V. Youshchenko]

>
> > Should they be renamed to CFG_COMMANDS_xxx?
>
> No, as  CFG  also  has  a  specific  meaning.  Here,  you  just  need
> temporary,  local  variables.  Please avoid name space pollution. You
> can and should even #undef those after use.

Hmm...

nikita at blacky:~> cpp
#define A aaa
#define B A
#undef A
B
<Ctrl-D>
# 1 "<stdin>"
# 1 "<built-in>"
# 1 "<command line>"
# 1 "<stdin>"



A

This example clearly shows that macros used to define other macros can't be 
undefined.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
Url : http://lists.denx.de/pipermail/u-boot/attachments/20070524/8a8153f6/attachment.pgp 

[U-Boot-Users] [PATCH] fix compilation problem for mpc8349itx CFG_RAMBOOT

[Nikita V. Youshchenko]

> > Please take a look at my MPC8349ITX.h to see the full extent of what I
> > was trying to do.
>
> Yes, I know what it does. Frankly, it's ugly.

I think that the original reason of such 'ugly' solutions is the usage of 
singe macro (CONFIG_COMMANDS) with bit-fields. This is causing problems, 
because:
- it is nos scalable to manually define CONFIG_COMMANDS as a bitwise-or of 
all needed CFG_CMD_* in each file under include/configs - because adding 
new 'arch-independent' command in this case will require mo9dification of 
all those files;
- so things as CONFIG_CMD_DFL become needed;
- but C preprocessor is has poor redefining capabilities - we can't just 
redefine a macro using it's previous definition - so usage of 
CONFIG_CMD_DFL causes issues like one being discussed.

A much better solution could be to have a macro-per-command.
In this case:
- command xxx could be compiled in, if CONFIG_CMD_XXX is defined, and not 
compiled in if not defined;
- it could be easy to define/undefine such macros in config

But I don't think that such a change could be possible at the current stage 
of u-boot development.

Nikita
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
Url : http://lists.denx.de/pipermail/u-boot/attachments/20070524/32f49331/attachment.pgp 

[U-Boot-Users] [PATCH] fix compilation problem for mpc8349itx CFG_RAMBOOT

[Wolfgang Denk]

In message <200705240013.48844@sercond.localdomain> you wrote:
>
> A much better solution could be to have a macro-per-command.

I'm not convinced that this is "much better". Believe me, we
considered this before.

> In this case:
> - command xxx could be compiled in, if CONFIG_CMD_XXX is defined, and not>  
> compiled in if not defined;
> - it could be easy to define/undefine such macros in config

But you get problems when you want to configure a board with settings
like:

* all commands except foo, bar and baz
* all default commands plus foo, but without baz

I guess then you will have to  list  up  all  commands  you  want  to
include  somewhere  -  either  in  the board onfig file or in another
header file.

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
O Staat!   Wie tief dir alle Besten fluchen!  Du bist kein Ziel.  Der
Mensch mu? weiter suchen.                     - Christian Morgenstern

[U-Boot-Users] [PATCH] fix compilation problem for mpc8349itx CFG_RAMBOOT

[Nikita V. Youshchenko]

> > In this case:
> > - command xxx could be compiled in, if CONFIG_CMD_XXX is defined, and
> > not> compiled in if not defined;
> > - it could be easy to define/undefine such macros in config
>
> But you get problems when you want to configure a board with settings
> like:
>
> * all commands except foo, bar and baz

#include "define_all_cmds.h"
#undef CONFIG_CMD_FOO
#undef CONFIG_CMD_BAR
#undef CONFIG_CMD_BAZ

> * all default commands plus foo, but without baz

#include "define_default_cmds.h"
#define CONFIG_CMD_FOO
#undef CONFIG_CMD_BAZ

> I guess then you will have to  list  up  all  commands  you  want  to
> include  somewhere  -  either  in  the board onfig file or in another
> header file.

Sure - in separate headers files. I see no problems here. It scales.

[U-Boot-Users] [PATCH] fix compilation problem for mpc8349itx CFG_RAMBOOT

[Wolfgang Denk]

Dear Nikita,

in message <200705241210.19623@zigzag.lvk.cs.msu.su> you wrote:
>
> > * all commands except foo, bar and baz
> 
> #include "define_all_cmds.h"
> #undef CONFIG_CMD_FOO
> #undef CONFIG_CMD_BAR
> #undef CONFIG_CMD_BAZ
> 
> > * all default commands plus foo, but without baz
> 
> #include "define_default_cmds.h"
> #define CONFIG_CMD_FOO
> #undef CONFIG_CMD_BAZ
> 
> > I guess then you will have to  list  up  all  commands  you  want  to
> > include  somewhere  -  either  in  the board onfig file or in another
> > header file.
> 
> Sure - in separate headers files. I see no problems here. It scales.

Good idea. Shall we go for that?

Any other opinions, ACKs or NACKs?

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
Gew?hnlich glaubt der Mensch,  wenn er nur Worte h?rt,  es m?sse sich
dabei doch auch was denken lassen.                 -- Goethe, Faust I

[U-Boot-Users] [PATCH] fix compilation problem for mpc8349itx CFG_RAMBOOT

[Jerry Van Baren]

Wolfgang Denk wrote:
> Dear Nikita,
> 
> in message <200705241210.19623@zigzag.lvk.cs.msu.su> you wrote:
>>> * all commands except foo, bar and baz
>> #include "define_all_cmds.h"
>> #undef CONFIG_CMD_FOO
>> #undef CONFIG_CMD_BAR
>> #undef CONFIG_CMD_BAZ
>>
>>> * all default commands plus foo, but without baz
>> #include "define_default_cmds.h"
>> #define CONFIG_CMD_FOO
>> #undef CONFIG_CMD_BAZ
>>
>>> I guess then you will have to  list  up  all  commands  you  want  to
>>> include  somewhere  -  either  in  the board onfig file or in another
>>> header file.
>> Sure - in separate headers files. I see no problems here. It scales.
> 
> Good idea. Shall we go for that?
> 
> Any other opinions, ACKs or NACKs?
> 
> Best regards,
> Wolfgang Denk

Just to poke the sleeping lions, :-) we've discussed going to a linux 
kconfig "script" in the past and was not pursued for various reasons, 
most of which I've forgotten.  It has some nice features
* Help text
* Conditional logic to guard against nonsensical configurations

Should we reconsider it?

I believe the above is compatible with kconfig, the missing part being 
piles and piles of configuration snippets that would have to be written. 
  Anybody with more knowledge than me?  (OK, that last was rhetorical. ;-)

Best regards,
gvb

[U-Boot-Users] [PATCH] fix compilation problem for mpc8349itx CFG_RAMBOOT

[Timur Tabi]

Wolfgang Denk wrote:

> Good idea. Shall we go for that?
> 
> Any other opinions, ACKs or NACKs?

ACK.  I like it.  The header files should all start with the same name, like 
config_defaults.h and config_all.h.  And config_defaults should contain a list of ALL 
options, using #define to turn on options and #undef to turn off options.  Yes, the 
#undefs are redundant, but then you can use that file as a reference.  It would also be 
nice if the documentation for these options were moved from the README to this file.  My 
text editor has the ability to display comments for any identifier if I just however the 
mouse over it.

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

[U-Boot-Users] [PATCH] fix compilation problem for mpc8349itx CFG_RAMBOOT

[Jon Loeliger]

On Thu, 2007-05-24 at 07:36, Wolfgang Denk wrote:
> Dear Nikita,
> 
> in message <200705241210.19623@zigzag.lvk.cs.msu.su> you wrote:
> >
> > > * all commands except foo, bar and baz
> > 
> > #include "define_all_cmds.h"
> > #undef CONFIG_CMD_FOO
> > #undef CONFIG_CMD_BAR
> > #undef CONFIG_CMD_BAZ
> > 
> > > * all default commands plus foo, but without baz
> > 
> > #include "define_default_cmds.h"
> > #define CONFIG_CMD_FOO
> > #undef CONFIG_CMD_BAZ
> > 
> > > I guess then you will have to  list  up  all  commands  you  want  to
> > > include  somewhere  -  either  in  the board onfig file or in another
> > > header file.
> > 
> > Sure - in separate headers files. I see no problems here. It scales.
> 
> Good idea. Shall we go for that?
> 
> Any other opinions, ACKs or NACKs?

I am, in general, in favor of the approach.

We should be a bit cautious though.
In this example:

#include "define_default_cmds.h"
#define CONFIG_CMD_FOO
#undef CONFIG_CMD_BAZ

it is likely that this should rather be:

#include "define_default_cmds.h"
#ifndef CONFIG_CMD_FOO
#define CONFIG_CMD_FOO
#endif
#undef CONFIG_CMD_BAZ

or

#include "define_default_cmds.h"
#undef CONFIG_CMD_FOO
#define CONFIG_CMD_FOO
#undef CONFIG_CMD_BAZ

just in case CONFIG_CMD_FOO is previously defined
to something non-empty like "1" or so.

jdl

[U-Boot-Users] [PATCH] fix compilation problem for mpc8349itx CFG_RAMBOOT

[Timur Tabi]

Jon Loeliger wrote:

> just in case CONFIG_CMD_FOO is previously defined
> to something non-empty like "1" or so.

Wouldn't it be easier just to have config.h include these other config options?  You can't 
do any ifdefs in U-Boot without including config.h first anyway.

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

[U-Boot-Users] [PATCH] fix compilation problem for mpc8349itx CFG_RAMBOOT

[Wolfgang Grandegger]

Jon Loeliger wrote:
> On Thu, 2007-05-24 at 07:36, Wolfgang Denk wrote:
>> Dear Nikita,
>>
>> in message <200705241210.19623@zigzag.lvk.cs.msu.su> you wrote:
>>>> * all commands except foo, bar and baz
>>> #include "define_all_cmds.h"
>>> #undef CONFIG_CMD_FOO
>>> #undef CONFIG_CMD_BAR
>>> #undef CONFIG_CMD_BAZ
>>>
>>>> * all default commands plus foo, but without baz
>>> #include "define_default_cmds.h"
>>> #define CONFIG_CMD_FOO
>>> #undef CONFIG_CMD_BAZ
>>>
>>>> I guess then you will have to  list  up  all  commands  you  want  to
>>>> include  somewhere  -  either  in  the board onfig file or in another
>>>> header file.
>>> Sure - in separate headers files. I see no problems here. It scales.
>> Good idea. Shall we go for that?
>>
>> Any other opinions, ACKs or NACKs?
> 
> I am, in general, in favor of the approach.
> 
> We should be a bit cautious though.
> In this example:
> 
> #include "define_default_cmds.h"
> #define CONFIG_CMD_FOO
> #undef CONFIG_CMD_BAZ
> 
> it is likely that this should rather be:
> 
> #include "define_default_cmds.h"
> #ifndef CONFIG_CMD_FOO
> #define CONFIG_CMD_FOO
> #endif
> #undef CONFIG_CMD_BAZ
> 
> or
> 
> #include "define_default_cmds.h"
> #undef CONFIG_CMD_FOO
> #define CONFIG_CMD_FOO
> #undef CONFIG_CMD_BAZ
> 
> just in case CONFIG_CMD_FOO is previously defined
> to something non-empty like "1" or so.

Then it should be removed from the config file because it is obviously 
already defined.

Wolfgang.

[U-Boot-Users] [PATCH] fix compilation problem for mpc8349itx CFG_RAMBOOT

[Wolfgang Grandegger]

Nikita V. Youshchenko wrote:
>>> In this case:
>>> - command xxx could be compiled in, if CONFIG_CMD_XXX is defined, and
>>> not> compiled in if not defined;
>>> - it could be easy to define/undefine such macros in config
>> But you get problems when you want to configure a board with settings
>> like:
>>
>> * all commands except foo, bar and baz
> 
> #include "define_all_cmds.h"
> #undef CONFIG_CMD_FOO
> #undef CONFIG_CMD_BAR
> #undef CONFIG_CMD_BAZ
> 
>> * all default commands plus foo, but without baz
> 
> #include "define_default_cmds.h"
> #define CONFIG_CMD_FOO
> #undef CONFIG_CMD_BAZ
> 
>> I guess then you will have to  list  up  all  commands  you  want  to
>> include  somewhere  -  either  in  the board onfig file or in another
>> header file.
> 
> Sure - in separate headers files. I see no problems here. It scales.

Full ack. We need an include file for the default commands 
(cmd_default.h) and one for all other commands (cmd_not_default.h). And 
"cmd_all.h" will include both. Then you can do exactly the same as with 
the bit masks and it scales nicely.

Wolfgang.

[U-Boot-Users] [PATCH] fix compilation problem for mpc8349itx CFG_RAMBOOT

[Timur Tabi]

Wolfgang Denk wrote:
> In message <46548B5B.4060607@freescale.com> you wrote:
>>> This is extremely inconsistent. Some CONFIG_COMMANDS_* (PCI, I2C) are
>>> used to add features, while others (FLASH) are used to remove
>>> features. I consinder this very error prone.
>> Well, it's supposed to be an alternative to what Nikita was proposing.  I wanted to keep 
> 
> It's bad. Use something like __CMD_ADD_I2C or  __CMD_REMOVE_FLASH  or
> so, but don't use the same name for different functions.

Ok, I can change those names.

> 
>> Please take a look at my MPC8349ITX.h to see the full extent of what I was trying to do. 
> 
> Yes, I know what it does. Frankly, it's ugly.

As Nikita pointed out, the ugliness is just a side-effect of the way CONFIG_COMMANDS is 
defined and a limitation of C macros.  A macro can only be defined once, and you can't put 
#ifdefs inside the #define, so you have two choices:

1) Use #ifdefs around #define CONFIG_COMMANDS to have the compiler choose *which* version 
of CONFIG_COMMANDS you want to actually compile

2) Create sub-macros (e.g. with __CMD_xxx) that are defined inside #ifdefs, and then make 
CONFIG_COMMANDS a combination of those macros.

I believe that option #1 scales better than option #2, and therefore is easier to read.

>> Should they be renamed to CFG_COMMANDS_xxx?
> 
> No, as  CFG  also  has  a  specific  meaning.  Here,  you  just  need
> temporary,  local  variables.  Please avoid name space pollution. You
> can and should even #undef those after use.

As NIkita demonstrated, you can't undefine those macros.

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

[U-Boot-Users] [PATCH] fix compilation problem for mpc8349itx CFG_RAMBOOT

[Kim Phillips]

On Wed, 23 May 2007 12:45:19 +0400
"Nikita V. Youshchenko" <yoush@debian.org> wrote:

> Current include/configs/MPC8349ITX.h does contain some support for building
> image that will be started from memory (without putting in into flash).
> It could be triggered by building with TEXT_BASE set to a low value.
> 
> However, this support is incomplete: using of low TEXT_BASE causes
> defining configuration macros in inconsistent way, which later leads
> to compilation errors. In particular. flash support is being disabled,
> but then flash structures get referenced.
> 
> This patch fixes this, making it possible to build with low TEXT_BASE.
> 
> Signed-Off-By: Nikita Youshchenko <yoush@debian.org>
> 

applied, thanks.

please don't send patches in nested multipart messages in the future.

Kim