* [U-Boot-Users] New features for Microblaze-Part1
@ 2006-05-23 17:42 Michal Simek
2006-05-23 20:27 ` Wolfgang Denk
0 siblings, 1 reply; 2+ messages in thread
From: Michal Simek @ 2006-05-23 17:42 UTC (permalink / raw)
To: u-boot
Hi,
I added some new features for Microblaze and support for Xilinx ML401 board.
Interrupt handler, describe exception, cache, booting linux kernels, timer, ethernet, etc.
Michal Simek
monstr at seznam.cz
Czech Technical University
Czech Republic
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.denx.de/pipermail/u-boot/attachments/20060523/7c38c8ca/attachment.htm
-------------- next part --------------
A non-text attachment was scrubbed...
Name: part1
Type: application/octet-stream
Size: 35744 bytes
Desc: not available
Url : http://lists.denx.de/pipermail/u-boot/attachments/20060523/7c38c8ca/attachment.obj
^ permalink raw reply [flat|nested] 2+ messages in thread
* [U-Boot-Users] New features for Microblaze-Part1
2006-05-23 17:42 [U-Boot-Users] New features for Microblaze-Part1 Michal Simek
@ 2006-05-23 20:27 ` Wolfgang Denk
0 siblings, 0 replies; 2+ messages in thread
From: Wolfgang Denk @ 2006-05-23 20:27 UTC (permalink / raw)
To: u-boot
In message <005b01c67e90$36e44dc0$0200a8c0@monstrone> you wrote:
>
> I added some new features for Microblaze and support for Xilinx ML401 board.
> Interrupt handler, describe exception, cache, booting linux kernels, timer, ethernet, etc.
There are some problems with your list of patches:
* There is no description what is what, i. e. if these are independ-
ent patches or if they have to be applied in sequence, or why there
is a "1b" patch, etc.
* CHANGELOG entry missing
> ------=_NextPart_001_0058_01C67EA0.FA6136E0
> Content-Type: text/html;
> charset="iso-8859-2"
> Content-Transfer-Encoding: quoted-printable
>
> <!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.0 Transitional//EN">
> <HTML><HEAD>
* NEVER post HTML to this list! Never!
* Coding Style problems:
- no // comments allowed
- no trailing white space allowed
- no more than 2 consequtive empty lines allowed
- no trailing empty lines allowed
- indentation must be done by 8 columns using TABs
- K&R brace style is mandatory
* Legal issues:
- the licensing of the following files is unclear:
board/xilinx/ml401/auto-config.h
cpu/microblaze/microblaze_disable_interrupts.S
cpu/microblaze/microblaze_enable_interrupts.S
include/asm-microblaze/microblaze_intc.h
include/asm-microblaze/microblaze_timer.h
- the licensing of the following files is not compatible with the
GPL:
board/xilinx/ml401/xparameters.h
* common/cmd_bootm.c:
-#elif defined(__microblaze__)
+#elif defined(__microblaze__) || defined(microblaze)
This should not be necessary. Either we use "__microblaze__" *or*
"microblaze", but please don't pollute the code with a mix of both
forms.
* common/cmd_bdinfo.c:
+#elif defined(MICROBLAZE) /* Microblaze */
...
#else /* ! PPC, which leaves MIPS */
Comment is wrong.
* General:
- I don't like the idea of having auto-config.h in the U-Boot
source tree; try to avoid this, please.
- lib_microblaze/board.c - do not put board specific code intot his
file. Especially not in a way that is broken for most (N-2 of N)
boards.
- lib_microblaze/microblaze_linux.c: do_bootm_linux() looks a bit
too simplistic to me ?
- lib_microblaze/time.c: Code like this needs to be cleaned up:
+#ifndef CONFIG_SUZAKU
+ int i,j;
+// for (j=0;j<usec;j++);
+// for (i=0;i<8000000;i++){};
+// j=(usec/150)+1;
+// j=(usec/(1000/6))+1;
+
+// j=(usec/(10000000000/CFG_HZ))+1;
+ j=(usec/150)+1;
+ i=get_timer(0);
+ while((get_timer(0)-i)<j);
+#endif
- Above code indicates that your timer tick is not 1 millisecond;
please fix this.
* Nitpicking:
- Use ligthweight functions where possible; for example, there is
no reason to invoke a complex function like printf() when all you
want to do is printing constant strings.
- Avoid useless function calls.
Example: cpu/microblaze/interrupts.c
+ printf ("\nInterrupt-Information:\n\n");
+ printf ("Nr Routine Arg Count\n");
+ printf ("-----------------------------\n");
Instead, you could use:
puts ("\nInterrupt-Information:\n\n"
"Nr Routine Arg Count\n"
"-----------------------------\n"
);
But this example shows two more issues:
- be terse. Don't print prosa. Shorten the output.
- never print nenlines at the begin of any output.
Sorry, this was just a cusory review of your patches. There are
probably more issues, but I think you got a feeling what you have to
check. Please clean up and resubmit. Patches rejected.
Best regards,
Wolfgang Denk
--
Software Engineering: Embedded and Realtime Systems, Embedded Linux
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
A man either lives life as it happens to him, meets it head-on and
licks it, or he turns his back on it and starts to wither away.
-- Dr. Boyce, "The Menagerie" ("The Cage"), stardate unknown
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2006-05-23 20:27 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2006-05-23 17:42 [U-Boot-Users] New features for Microblaze-Part1 Michal Simek
2006-05-23 20:27 ` Wolfgang Denk
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.