* [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