[U-Boot-Users] [PATCH] Generic Support for Motorola i.MX architecture
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
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
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
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
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?
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
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" sinceNo.
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?
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
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!
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/
participants (4)
-
Robert Schwebel -
Sascha Hauer -
Steven Scholz -
Wolfgang Denk