[U-Boot-Users] Start of new ARM920T target

[U-Boot-Users] Start of new ARM920T target

[Marc Singer]

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.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: u-boot-2.diff.gz
Type: application/octet-stream
Size: 19845 bytes
Desc: not available
Url : http://lists.denx.de/pipermail/u-boot/attachments/20030707/a48fdc85/attachment.obj 

[U-Boot-Users] Start of new ARM920T target

[Wolfgang Denk]

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

-- 
Software Engineering:  Embedded and Realtime Systems,  Embedded Linux
Phone: (+49)-8142-4596-87  Fax: (+49)-8142-4596-88  Email: wd at denx.de
You can observe a lot just by watchin'.                  - Yogi Berra

[U-Boot-Users] Start of new ARM920T target

[Marc Singer]

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.

[U-Boot-Users] Start of new ARM920T target

[Wolfgang Denk]

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

-- 
Software Engineering:  Embedded and Realtime Systems,  Embedded Linux
Phone: (+49)-8142-4596-87  Fax: (+49)-8142-4596-88  Email: wd at denx.de
I wrote my name at the top of the page. I wrote down  the  number  of
the  question  ``1''.  After much reflection I put a bracket round it
thus ``(1)''. But thereafter I could not think of anything  connected
with it that was either relevant or true.
                        - Sir Winston Churchill _My Early Life_ ch. 2