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