[U-Boot-Users] Start of new ARM920T target
I'm sending an initial patch for the KEV7A400 dev board in order to get some feedback about the configuration extension I'm proposing. This is *not* complete. It ought not break any of the non-ARM9 builds though it may break one of the ARM9 ones.
How it works:
1) The top-level script, ./mkconfigx, takes the configured include/config.h file and compiles a list of defines roughly of the (regex) form CONFIG_.* 2) These are transformed into CONFIG_.*=y 3) The written to ./configx.mk in a form that can be included in Makefiles. 4) The cpu/arm920t/Makefile includes this file. I believe this could be included in the ./config.mk file so that everyone benefits. 5) The cpu/arm920t/Makefile has been turned up-side down to make it more amenable to this form of configuration. The present form isn't as elegant as it should, and will, be. It's there to make the point of 'how' it might look. Notice that source files are declared instead of object files. 6) The board/kev7a400/u-boot.lds has an important change. Previously, the name of the start.o object file was hard coded. Instead, start.S has been moved to a named section, .start, and the linker script reflects this. The effect is the same as before, but the dependency is removed.
The serial_lh7a400.c file could be moved to share a home with other serial drivers.
With this sort of configuration methodology, the configuration file selects the desired driver with something like this:
#define CONFIG_SERIAL_LH7A400
or
#define CONFIG_SERIAL_NS16550
Why do I prefer this? Ifdefs tend to make code harder to read. A few are usually OK. As the code grows, it is desirable to separate the conditionally compiled code. Likewise, whole file ifdefs are not very clear. One can see the #ifdef at the top, and may not know if it matches the one at the bottom of a long source file. With this configuration method, needed files are explicitly built and linked when the appropriate configuration option is present.
Cheers.
In message 20030708024322.GA425@buici.com you wrote:
I'm sending an initial patch for the KEV7A400 dev board in order to get some feedback about the configuration extension I'm proposing. This is *not* complete. It ought not break any of the non-ARM9 builds though it may break one of the ARM9 ones.
I'll reject this patch for now, mostly for formal reasons.
* Please don't try to sneak in local stuff like your .gdbinit file.
* Please don't add lines with trailing white space.
* Please use the Linux kernel coding style. Please stick with TAB indentation, especially when editing existing code that uses it.
* Please do not use C++ comments.
* I would prefer if we would NOT add the requirement for more external tools (like Perl). But if you use these, then please use the respective recommendations for "good style" code - like using the "-w" switch and maybe "use strict;"
* You use "private" debugging clauses like "#ifdef RTC_DEBUG" - why not simply using the existing debug() macro?
Now for the technical discussion: I don't see much advantage when using your mkconfigx script. IMHO it has several problems: * It will incorrectly include files that have been commented out using C comments, "#if 0" or similar clauses. * It is based on the assumption that only CONFIG_* definitions are relevant; it would be nice if this was the case, but actually there is at least a couple of CFG_* variables, and some boards use (locally) even completely different names
You may argument that it's possible to fix the know problems, but I'm sure we will run into similar problems again later.
You modify include/asm-arm/processor.h in an unexpacted way (inserting "#undef arm"). Please don't add such code to system header files. Ther eis obviously a problem somewhere in your toolchain and/or on tyour system. Please fix the cause, not the symptoms.
May I please ask you to clean up the patch as far as the KEV7A400 dev board is concerned, and resubmit it. Please omit the mkconfigx stuff (and other "goodies" like .gdbinit).
Best regards,
Wolfgang Denk
I respect your objections. This was not intended as a *real* patch. I wanted you to look at the Makefile portion to see if you approved.
As for Perl, I think I can write the script using sh. Perl was just too darn quick and easy.
BTW, the RTC_DEBUG macro isn't mine. I'll see where it came from.
On Fri, Jul 18, 2003 at 01:34:31AM +0200, Wolfgang Denk wrote:
Now for the technical discussion: I don't see much advantage when using your mkconfigx script. IMHO it has several problems:
- It will incorrectly include files that have been commented out using C comments, "#if 0" or similar clauses.
That's an interesting objection.
- It is based on the assumption that only CONFIG_* definitions are relevant; it would be nice if this was the case, but actually there is at least a couple of CFG_* variables, and some boards use (locally) even completely different names
Though this is true, the point is to enable conditional compilation and *not* to allow every configuration option to control Makefiles.
You modify include/asm-arm/processor.h in an unexpacted way (inserting "#undef arm"). Please don't add such code to system header files. Ther eis obviously a problem somewhere in your toolchain and/or on tyour system. Please fix the cause, not the symptoms.
This is to work around a bug. Someone was #define'ing it and breaking a struture. I think that the true culprit is the code that #define's a symbol that isn't all upper case.
May I please ask you to clean up the patch as far as the KEV7A400 dev board is concerned, and resubmit it. Please omit the mkconfigx stuff (and other "goodies" like .gdbinit).
Of course. There's lots of work to do before it is useful.
-*-
Let me elaborate on mkconfigx.
First, I wrote this script to handle the new feature in order to make it easy to enable or disable it. It would be very easy to make it execute only for boards or CPUs that use this configuration feature. Or, when a particular tool was available.
I look only for CONFIG_ constants on purpose. We can have a simple rule: only CONFIG_ constants that define the value '1' will be parsed by mkconfigx.
I'm not sure I'd want to solve the #if 0 problem. Yes, someone could get something that they don't want. On the other handle, it is reasonable to change the definition to '0' from '1' when a feature isn't desired.
The strength of this kind of configuration comes from the fact that it allows the Makefiles to be the arbitrator of what code is included and what is excluded. I've been using it a lot while tweaking the ARM920 builds to accomodate the architectural differences between the Samsung parts and the Sharp parts. There are far too many #ifdefs in the existing ARM920 code making it difficult to figure out what belongs where. This configuration method lets us move the decisions about what belongs where to the Makefile.
So, instead of putting at the top of a source file:
#if defined (CPUA) | defined (CPUB) | ... /* code */ #endif
We put in the makefile
src-$(CONFIG_CPUA) +=start_type_1.S timer_type1.c ... src-$(CONFIG_CPUB) +=start_type_2.S timer_type1.c ... src-$(CONFIG_CPUC) +=start_type_1.S timer_type2.c ...
and move the code to separate files. There are many places where code shares a file for no good reason. I'm looking to put truly ARM920 code in ARM920 triggered sources, and so on.
Cheers.
Dear Marc,
sorry for the long delay.
In message 20030718003050.GB18209@buici.com you wrote:
I respect your objections. This was not intended as a *real* patch.
Um, sorry. I misunderstood this then. It looked like a real patch to me.
I wanted you to look at the Makefile portion to see if you approved.
...
Though this is true, the point is to enable conditional compilation and *not* to allow every configuration option to control Makefiles.
I understand what you intend to do, and I agree that it's a Good Thing (TM) to have.
You modify include/asm-arm/processor.h in an unexpacted way (inserting "#undef arm"). Please don't add such code to system header files. Ther eis obviously a problem somewhere in your toolchain and/or on tyour system. Please fix the cause, not the symptoms.
This is to work around a bug. Someone was #define'ing it and breaking a struture. I think that the true culprit is the code that #define's a symbol that isn't all upper case.
Symbols like "arm" or "__arm__" often get defined by the prepro- cessor. Check your tools. The ELDK's cross compiler does not define "arm" (but it does define "__arm__").
Let me elaborate on mkconfigx.
...
and move the code to separate files. There are many places where code shares a file for no good reason. I'm looking to put truly ARM920 code in ARM920 triggered sources, and so on.
We agree on the target. I see another benefit: the current way to compile all files (but #ifdef'fing unneeded stuff) causes long com- pile times; this prevents many people from running "MAKEALL", and costs a lot of time to myself. An change like the suggested one would improve this, too.
I just think that your first implementation of this idea needs some more work.
Best regards,
Wolfgang Denk
participants (2)
-
Marc Singer -
Wolfgang Denk