[U-Boot-Users] [PATCH] Generic Support for Motorola i.MX architecture

[U-Boot-Users] [PATCH] Generic Support for Motorola i.MX architecture

[Robert Schwebel]

On Sun, Jun 27, 2004 at 08:18:31PM +0200, Robert Schwebel wrote:
> I'll post our patch in five minutes...

Well, six minutes ;)

I've temporarily put the patch here:

	http://www.pengutronix.de/software/u-boot/u-boot-imx1.diff

Sidenote: it is difficult to audit this patch for trailing whitespace as
the patch context contains that much of it that you don't see the needle
in the haystack =8-)

Changelog entry:

* Patch by Sascha Hauer, 27 Jun 2004:
  Split mx1ads stuff into generic imx and platform parts
  Add support for Synertronixx scb9328, Viasys FlowScreen 2

Wolfgang, do you prefer the code being split into separate pieces for
the single tasks?

Robert
-- 
 Dipl.-Ing. Robert Schwebel | http://www.pengutronix.de
 Pengutronix - Linux Solutions for Science and Industry
   Handelsregister:  Amtsgericht Hildesheim, HRA 2686
     Hornemannstra?e 12,  31137 Hildesheim, Germany
    Phone: +49-5121-28619-0 |  Fax: +49-5121-28619-4

[U-Boot-Users] [PATCH] Generic Support for Motorola i.MX architecture

[Wolfgang Denk]

In message <20040627192709.GK21651@pengutronix.de> you wrote:
> 
> Sidenote: it is difficult to audit this patch for trailing whitespace as
> the patch context contains that much of it that you don't see the needle
> in the haystack =8-)

No, this is actually trivial. Use the toolbox:

	bash$ egrep '^\+.*[^I ]$' u-boot-imx1.diff  | wc -l
	     21

There are 21 cases where you insert trailing whitespace.

There are 39 cases where you use space instead of TAB for indentation.

There are 9 cases where you use C++ comments in non-C++ files.

There are 5 cases where you use DOS '\r\n' line feeds

There is one case where you add more than 2 empty lines to a source file

There are 4 cases where you add trailing empty lines to source files

Please cleanup and resubmit.

> * Patch by Sascha Hauer, 27 Jun 2004:
>   Split mx1ads stuff into generic imx and platform parts
>   Add support for Synertronixx scb9328, Viasys FlowScreen 2

It seems the patch does more than just this. For example, I see  that
it modifies the code for interrupt handling for all ARM9 systems?

Please make sure the description is complete.

> Wolfgang, do you prefer the code being split into separate pieces for
> the single tasks?

If the patch is clean this is not necessary.



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
Microsoft Compatibility:
     your old Windows 3.11 application crash exactly as the new ones.

[U-Boot-Users] [PATCH] Generic Support for Motorola i.MX architecture

[Sascha Hauer]

Hi Wolfgang,

I removed the trailing white spaces, dos line endings, etc from the
patch. Ming-Len Wu sent me a patch for mx1ads boards, which I included
into the patch. I can't test mx1ads support, but he says it works.

You can download the patch here:

http://www.pengutronix.de/software/u-boot/u-boot-imx1-20040628-1.diff

Best regards,

  Sascha Hauer

[U-Boot-Users] [PATCH] Generic Support for Motorola i.MX architecture

[Wolfgang Denk]

Dear Sasha,

in message <20040628120334.GA27672@herry.saufen> you wrote:
> 
> I removed the trailing white spaces, dos line endings, etc from the
> patch. Ming-Len Wu sent me a patch for mx1ads boards, which I included
> into the patch. I can't test mx1ads support, but he says it works.
> 
> You can download the patch here:
> 
> http://www.pengutronix.de/software/u-boot/u-boot-imx1-20040628-1.diff

Added, thanks.

I understand that this patch obsoletes all your previous patches,
i. e. the following messages:

06/17 Sascha Hauer       [U-Boot-Users] [PATCH] Motorola i.MX support (1/2)<<--HlL+5n6rz5pIUxb
06/17 Sascha Hauer       [U-Boot-Users] [PATCH] Motorola i.MX support (2/2)<<--cmJC7u66zC7hs+8
06/18 Sascha Hauer       Re: [U-Boot-Users] [PATCH] Motorola i.MX support (1/2)<<I've just had
06/19 Robert Schwebel    Re: [U-Boot-Users] [PATCH] Motorola i.MX support (1/2)<<Hi, On Fri, J
06/19 Sascha Hauer       Re: [U-Boot-Users] [PATCH] Motorola i.MX support (1/2)<<On Sat, Jun 1
06/21 Robert Schwebel    Re: [U-Boot-Users] [PATCH] Motorola i.MX support (1/2)<<--SO98HVl1bnM
06/21 Steven Scholz      Re: [U-Boot-Users] [PATCH] Motorola i.MX support (1/2)<<Robert Schweb
06/21 Robert Schwebel    Re: [U-Boot-Users] [PATCH] Motorola i.MX support (1/2)<<On Mon, Jun 2

Please confirm.


Technical problems:

* In include/configs/mx1ads.h etc. you have this:

  #define        CFG_HZ                  3686400

  This is BROKEN. CFG_HZ is the number of timer ticks per second.
  You don't want to process more than 3 millions of interrupts per sec.
  Please don't mis-use CFG_HZ for things it was not intended for.


Formal problems:

* Entries for MAKEALL and MAINTAINERS missing. Please submit new patch!

* Would you please keep lists sorted, i.  e.  don't  mix  ARM  boards
  inbetween PowerPC systems, and sort the names in ascending order?

* Please use plain '#' comments in Makefiles etc. Your
	#/*
	#*
	...
	#*
	#*/
  is ugly.

* Please do not add trailing white space
  (board/mx1fs2/memsetup.S, include/configs/mx1ads.h)

* Please do not add trailing empty lines.

* Please don't modify a file if all you change  is deleting an empty line.

* Don't use 3 or more consecutive empty lines.

* Please don't include opject files multiple times (cpu.o in
  cpu/arm920t/Makefile)


Best regards,

Wolfgang Denk

-- 
Software Engineering:  Embedded and Realtime Systems,  Embedded Linux
Phone: (+49)-8142-4596-87  Fax: (+49)-8142-4596-88  Email: wd@denx.de
He who hesitates is not only lost, but miles from the next exit.

[U-Boot-Users] [PATCH] Generic Support for Motorola i.MX architecture

[Steven Scholz]

Wolfgang Denk wrote:
> Dear Sasha,
> 
> in message <20040628120334.GA27672@herry.saufen> you wrote:
> 
>>I removed the trailing white spaces, dos line endings, etc from the
>>patch. Ming-Len Wu sent me a patch for mx1ads boards, which I included
>>into the patch. I can't test mx1ads support, but he says it works.
>>
>>You can download the patch here:
>>
>>http://www.pengutronix.de/software/u-boot/u-boot-imx1-20040628-1.diff
> 
> 
> Added, thanks.

Now that this patch finally got into CVS I raise the following question again:

Does it make sense to put processor specific peripheral code into 
cpu/arm920t directory (like s3c24x0_serial.c or imx_interrupts.c or usb code)!?

Although the AT91RM9200 is based on a ARM9 it has it's own directory.

I understand that copying the same code again and again won't make sense.

A while ago I suggested to create cpu/imx, cpu/s3c24x0 etc. and put all the 
cpu specific stuff in there.

To avoid copying the arm9 generic code one could do:

1.) cpu/at91rm9200/Makefile:

     OBJS    = ../arm920t/interrupts.o ../arm920t/cpu.o \
               serial.o at91rm9200_ether.o at45.o

     start.S has to be a link "start.S -> ../arm920t/start.S" since

     START= ../arm920t/start.o

     would not work due to dependencies in the main makefile.

2.) Or creating (by Makefile) links to the generic sources:

     LINKS = start.S interrupts.c cpu.c

     $(LINKS)
         ln -s ../arm920t/$@ $@    (oder s.?.)


Comments?

-- 
Steven Scholz
imc Measurement & Control

[U-Boot-Users] [PATCH] Generic Support for Motorola i.MX architecture

[Wolfgang Denk]

In message <410F766F.2020106@imc-berlin.de> you wrote:
>
> Does it make sense to put processor specific peripheral code into 
> cpu/arm920t directory (like s3c24x0_serial.c or imx_interrupts.c or usb c
> ode)!?

Yes, it makes sense, in so far as the  cpu/<processor>  directory  is
explicitely  intended  to  hold  all  code  that  is  specific to the
<processor> CPU.

It might make sense to add an additionallevel of directories,  i.  e.
turn s3c24x0_* or imx_* into s3c24x0/* and imx/* resp.

> Although the AT91RM9200 is based on a ARM9 it has it's own directory.

Which is IMHO a bad thing.

> I understand that copying the same code again and again won't make sense.

Indeed.

> A while ago I suggested to create cpu/imx, cpu/s3c24x0 etc. and put all the 
> cpu specific stuff in there.

Agreed. Please submit a patch.

> To avoid copying the arm9 generic code one could do:

ARM9 generic code should stay in cpu/arm920t/

> 1.) cpu/at91rm9200/Makefile:
> 
>      OBJS    = ../arm920t/interrupts.o ../arm920t/cpu.o \
>                serial.o at91rm9200_ether.o at45.o
> 
>      start.S has to be a link "start.S -> ../arm920t/start.S" since

No.

> 2.) Or creating (by Makefile) links to the generic sources:
> 
>      LINKS = start.S interrupts.c cpu.c
> 
>      $(LINKS)
>          ln -s ../arm920t/$@ $@    (oder s.?.)

No.

> Comments?

Both methods don't look really attractive to me. If ther  eis  common
code, it shall remain in the common directory.

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
Any sufficiently advanced technology is indistinguishable from magic.
Clarke's Third Law       - _Profiles of the Future_ (1962; rev. 1973)
                  ``Hazards of Prophecy: The Failure of Imagination''

[U-Boot-Users] [PATCH] Generic Support for Motorola i.MX architecture

[Steven Scholz]

Wolfgang Denk wrote:
> In message <410F766F.2020106@imc-berlin.de> you wrote:
>>A while ago I suggested to create cpu/imx, cpu/s3c24x0 etc. and put all the 
>>cpu specific stuff in there.
> 
> 
> Agreed. Please submit a patch.
> ARM9 generic code should stay in cpu/arm920t/
> 
>>1.) cpu/at91rm9200/Makefile:
>>
>>     OBJS    = ../arm920t/interrupts.o ../arm920t/cpu.o \
>>               serial.o at91rm9200_ether.o at45.o
>>
>>     start.S has to be a link "start.S -> ../arm920t/start.S" since
> 
> 
> No.
> 
> 
>>2.) Or creating (by Makefile) links to the generic sources:
>>
>>     LINKS = start.S interrupts.c cpu.c
>>
>>     $(LINKS)
>>         ln -s ../arm920t/$@ $@    (oder s.?.)
> 
> 
> No.
> 
> 
>>Comments?
> 
> 
> Both methods don't look really attractive to me. If ther  eis  common
> code, it shall remain in the common directory.

But how else could we solve this?
If we leave the common arm920t code in cpu/arm920t and put the specific 
stuff in - let's say - cpu/s3c24x0...

The main makefile only takes one cpu type. How can we achieve that the code 
in cpu/arm920t _and_ in cpu/s3c24x0 is built?


-- 
Steven Scholz

[U-Boot-Users] [PATCH] Generic Support for Motorola i.MX architecture

[Wolfgang Denk]

In message <410F951A.6020804@imc-berlin.de> you wrote:
> 
> But how else could we solve this?

As I wrote:

| It might make sense to add an additional level of directories,  i.  e.
| turn s3c24x0_* or imx_* into s3c24x0/* and imx/* resp.

> If we leave the common arm920t code in cpu/arm920t and put the specific 
> stuff in - let's say - cpu/s3c24x0...

No. Not cpu/s3c24x0/ - cpu/arm920t/s3c24x0/

> The main makefile only takes one cpu type. How can we achieve that the code 
> in cpu/arm920t _and_ in cpu/s3c24x0 is built?

By just having a cpu/arm920t/ directory with subdirectories?

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
A failure will not appear until a unit has passed final inspection.

[U-Boot-Users] [PATCH] Generic Support for Motorola i.MX architecture

[Steven Scholz]

Wolfgang Denk wrote:
> In message <410F951A.6020804@imc-berlin.de> you wrote:
> 
>>But how else could we solve this?
> 
> 
> As I wrote:
> 
> | It might make sense to add an additional level of directories,  i.  e.
> | turn s3c24x0_* or imx_* into s3c24x0/* and imx/* resp.
> 
> 
>>If we leave the common arm920t code in cpu/arm920t and put the specific 
>>stuff in - let's say - cpu/s3c24x0...
> 
> 
> No. Not cpu/s3c24x0/ - cpu/arm920t/s3c24x0/
> 
> 
>>The main makefile only takes one cpu type. How can we achieve that the code 
>>in cpu/arm920t _and_ in cpu/s3c24x0 is built?
> 
> 
> By just having a cpu/arm920t/ directory with subdirectories?

Ah! Now I see! Thanks!

-- 
Steven Scholz

[U-Boot-Users] [PATCH] Generic Support for Motorola i.MX architecture

[Steven Scholz]

Wolfgang Denk wrote:

> In message <410F951A.6020804@imc-berlin.de> you wrote:
> 
>>But how else could we solve this?
> 
> 
> As I wrote:
> 
> | It might make sense to add an additional level of directories,  i.  e.
> | turn s3c24x0_* or imx_* into s3c24x0/* and imx/* resp.

Would you agree that we should put peripheral drivers like

	s3c24x0_serial.c
	usb_ohci.c (s3c24x0)
	at91rm9200_ether.c
	at91rm9200_serial.c

into the drivers/ directory. But stuff like

	imx_interrupts.c
	imx_speed.c
	s3c24x0_interrupts.c
	s3c24x0_speed.c
	at91rm9200/interrupts.c

belongs into the (new) processor specific subdirs under cpu/

-- 
Steven Scholz