fix compilation problem for mpc8349itx CFG_RAMBOOT

PATCH New Nikita V. Youshchenko 7 participants 26 messages May 23, 2007 – Jun 1, 2007

Share & tools

Nikita V. Youshchenko

author

2007-05-23 08:45 UTC #0 lore raw

PATCH 1/1 fix compilation problem for mpc8349itx CFG_RAMBOOT

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 <[email protected]>

include/configs/MPC8349ITX.h +9 -1

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

Timur Tabi

2007-05-23 15:09 UTC #1 lore raw

PATCH 1/1 fix compilation problem for mpc8349itx CFG_RAMBOOT

Quoted 3 lines · 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.

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:

include/configs/MPC8349ITX.h boards, we say we have two, but don't di · line 446

Quoted 7 lines

include/configs/MPC8349ITX.h +6 -1

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

Signature · 2 lines

Timur Tabi
Linux Kernel Developer @ Freescale

Nikita V. Youshchenko

author

2007-05-23 15:37 UTC #2 lore raw

PATCH 1/1 fix compilation problem for mpc8349itx CFG_RAMBOOT

Quoted 7 lines

Quoted 4 lines · 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.

Quoted 5 lines

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.

Quoted 10 lines

include/configs/MPC8349ITX.h boards, we say we have two, but don't di · line 446
Quoted 7 lines
include/configs/MPC8349ITX.h +6 -1
-#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

Timur Tabi

2007-05-23 15:55 UTC #3 lore raw

PATCH 1/1 fix compilation problem for mpc8349itx CFG_RAMBOOT

Quoted 2 lines · Nikita V. Youshchenko

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?

Quoted 3 lines

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.

Quoted 5 lines

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.

Signature · 2 lines

Timur Tabi
Linux Kernel Developer @ Freescale

Wolfgang Denk

2007-05-23 16:30 UTC #4 lore raw

PATCH 1/1 fix compilation problem for mpc8349itx CFG_RAMBOOT

Quoted 5 lines · In message <[email protected]> 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

Signature · 7 lines

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

Timur Tabi

2007-05-23 16:50 UTC #5 lore raw

PATCH 1/1 fix compilation problem for mpc8349itx CFG_RAMBOOT

Quoted 2 lines · Wolfgang Denk

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.

Signature · 2 lines

Timur Tabi
Linux Kernel Developer @ Freescale

Timur Tabi

2007-05-23 16:52 UTC #6 lore raw

PATCH 1/1 fix compilation problem for mpc8349itx CFG_RAMBOOT

In reply to Nikita V. Youshchenko

Quoted 3 lines · 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.

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

Signature · 2 lines

Timur Tabi
Linux Kernel Developer @ Freescale

Nikita V. Youshchenko

author

2007-05-23 18:59 UTC #7 lore raw

PATCH 1/1 fix compilation problem for mpc8349itx CFG_RAMBOOT

Quoted 8 lines

Quoted 4 lines · 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.
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

Timur Tabi

2007-05-23 19:08 UTC #8 lore raw

PATCH 1/1 fix compilation problem for mpc8349itx CFG_RAMBOOT

Quoted 2 lines · Nikita V. Youshchenko

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.

Signature · 2 lines

Timur Tabi
Linux Kernel Developer @ Freescale

Timur Tabi

2007-05-23 17:48 UTC #9 lore raw

PATCH 1/1 fix compilation problem for mpc8349itx CFG_RAMBOOT

In reply to Nikita V. Youshchenko

Quoted 16 lines · 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 <[email protected]>

include/configs/MPC8349ITX.h boards, we say we have two, but don't di · line 412

Quoted 4 lines

include/configs/MPC8349ITX.h +1 -0

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

include/configs/MPC8349ITX.h boards, we say we have two, but don't di · line 451

Quoted 12 lines

include/configs/MPC8349ITX.h +8 -1

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

Signature · 2 lines

Timur Tabi
Linux Kernel Developer @ Freescale

Wolfgang Denk

2007-05-23 18:38 UTC #10 lore raw

PATCH 1/1 fix compilation problem for mpc8349itx CFG_RAMBOOT

Quoted 9 lines · In message <[email protected]> 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

Quoted 11 lines

#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

Signature · 5 lines

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.

Timur Tabi

2007-05-23 18:43 UTC #11 lore raw

PATCH 1/1 fix compilation problem for mpc8349itx CFG_RAMBOOT

Quoted 21 lines · Wolfgang Denk

Quoted 8 lines · In message <[email protected]> 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
Quoted 11 lines
#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

Quoted 3 lines

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.

Quoted 7 lines

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?

Signature · 2 lines

Timur Tabi
Linux Kernel Developer @ Freescale

Wolfgang Denk

2007-05-23 19:51 UTC #12 lore raw

PATCH 1/1 fix compilation problem for mpc8349itx CFG_RAMBOOT

Quoted 6 lines · In message <[email protected]> you wrote:

Quoted 4 lines
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.

Quoted 1 line

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.

Quoted 1 line

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

Signature · 5 lines

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

Nikita V. Youshchenko

author

2007-05-23 20:01 UTC #13 lore raw

PATCH 1/1 fix compilation problem for mpc8349itx CFG_RAMBOOT

Quoted 6 lines

Quoted 2 lines
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>"

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

Nikita V. Youshchenko

author

2007-05-23 20:13 UTC #14 lore raw

PATCH 1/1 fix compilation problem for mpc8349itx CFG_RAMBOOT

In reply to Wolfgang Denk

Quoted 4 lines

Quoted 2 lines
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.

Wolfgang Denk

2007-05-23 23:11 UTC #15 lore raw

PATCH 1/1 fix compilation problem for mpc8349itx CFG_RAMBOOT

Quoted 2 lines · In message <[email protected]> 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.

Quoted 4 lines

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

Signature · 5 lines

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

Nikita V. Youshchenko

author

2007-05-24 08:10 UTC #16 lore raw

PATCH 1/1 fix compilation problem for mpc8349itx CFG_RAMBOOT

Quoted 9 lines

Quoted 4 lines
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

Quoted 1 line

* all default commands plus foo, but without baz

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

Quoted 3 lines

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.

Wolfgang Denk

2007-05-24 12:36 UTC #17 lore raw

PATCH 1/1 fix compilation problem for mpc8349itx CFG_RAMBOOT

Dear Nikita,

Quoted 14 lines · in message <[email protected]> you wrote:

Quoted 2 lines
* all commands except foo, bar and baz
#include "define_all_cmds.h"
#undef CONFIG_CMD_FOO
#undef CONFIG_CMD_BAR
#undef CONFIG_CMD_BAZ
Quoted 1 line
* all default commands plus foo, but without baz
#include "define_default_cmds.h"
#define CONFIG_CMD_FOO
#undef CONFIG_CMD_BAZ

Quoted 5 lines

Quoted 3 lines
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

Signature · 5 lines

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

Jerry Van Baren

2007-05-24 12:49 UTC #18 lore raw

PATCH 1/1 fix compilation problem for mpc8349itx CFG_RAMBOOT

Quoted 17 lines · Wolfgang Denk

Dear Nikita,
Quoted 14 lines · in message <[email protected]> you wrote:
Quoted 1 line
* all commands except foo, bar and baz
#include "define_all_cmds.h"
#undef CONFIG_CMD_FOO
#undef CONFIG_CMD_BAR
#undef CONFIG_CMD_BAZ
Quoted 1 line
* all default commands plus foo, but without baz
#include "define_default_cmds.h"
#define CONFIG_CMD_FOO
#undef CONFIG_CMD_BAZ
Quoted 3 lines
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.

Quoted 8 lines

Quoted 1 line
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

Timur Tabi

2007-05-24 15:25 UTC #19 lore raw

PATCH 1/1 fix compilation problem for mpc8349itx CFG_RAMBOOT

In reply to Wolfgang Denk

Quoted 3 lines · Wolfgang Denk

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.

Signature · 2 lines

Timur Tabi
Linux Kernel Developer @ Freescale

Jon Loeliger

2007-05-24 18:36 UTC #20 lore raw

PATCH 1/1 fix compilation problem for mpc8349itx CFG_RAMBOOT

In reply to Wolfgang Denk

Quoted 21 lines · Wolfgang Denk

Dear Nikita,
Quoted 18 lines · in message <[email protected]> you wrote:
Quoted 2 lines
* all commands except foo, bar and baz
#include "define_all_cmds.h"
#undef CONFIG_CMD_FOO
#undef CONFIG_CMD_BAR
#undef CONFIG_CMD_BAZ
Quoted 1 line
* all default commands plus foo, but without baz
#include "define_default_cmds.h"
#define CONFIG_CMD_FOO
#undef CONFIG_CMD_BAZ
Quoted 3 lines
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.

Quoted 5 lines

Quoted 1 line
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

#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

Timur Tabi

2007-05-24 18:38 UTC #21 lore raw

PATCH 1/1 fix compilation problem for mpc8349itx CFG_RAMBOOT

Quoted 2 lines · Jon Loeliger

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.

Signature · 2 lines

Timur Tabi
Linux Kernel Developer @ Freescale

Wolfgang Grandegger

2007-05-24 19:43 UTC #22 lore raw

PATCH 1/1 fix compilation problem for mpc8349itx CFG_RAMBOOT

In reply to Jon Loeliger

Quoted 42 lines · Jon Loeliger

Quoted 14 lines · Wolfgang Denk
Dear Nikita,
Quoted 11 lines · in message <[email protected]> you wrote:
Quoted 1 line
* all commands except foo, bar and baz
#include "define_all_cmds.h"
#undef CONFIG_CMD_FOO
#undef CONFIG_CMD_BAR
#undef CONFIG_CMD_BAZ
Quoted 1 line
* all default commands plus foo, but without baz
#include "define_default_cmds.h"
#define CONFIG_CMD_FOO
#undef CONFIG_CMD_BAZ
Quoted 7 lines
Quoted 4 lines
Quoted 3 lines
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

Quoted 7 lines

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

Wolfgang Grandegger

2007-05-24 12:39 UTC #23 lore raw

PATCH 1/1 fix compilation problem for mpc8349itx CFG_RAMBOOT

In reply to Nikita V. Youshchenko

Quoted 20 lines · Nikita V. Youshchenko

Quoted 8 lines
Quoted 4 lines
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
Quoted 1 line
* all default commands plus foo, but without baz
#include "define_default_cmds.h"
#define CONFIG_CMD_FOO
#undef CONFIG_CMD_BAZ

Quoted 5 lines

Quoted 3 lines
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.

Timur Tabi

2007-05-23 20:29 UTC #24 lore raw

PATCH 1/1 fix compilation problem for mpc8349itx CFG_RAMBOOT

In reply to Wolfgang Denk

Quoted 8 lines · Wolfgang Denk

Quoted 4 lines · In message <[email protected]> you wrote:
Quoted 3 lines
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.

Quoted 4 lines

Quoted 2 lines
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.

Quoted 5 lines

Quoted 1 line
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.

Signature · 2 lines

Timur Tabi
Linux Kernel Developer @ Freescale

Kim Phillips

Applied

2007-06-01 20:18 UTC #25 lore raw

PATCH 1/1 fix compilation problem for mpc8349itx CFG_RAMBOOT

In reply to Nikita V. Youshchenko

Quoted 4 lines · On Wed, 23 May 2007 12:45:19 +0400 "Nikita V. Youshchenko" <[email protected]>…

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.

Quoted 9 lines

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 <[email protected]>

applied, thanks.

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

Kim