[U-Boot-Users] New features for Microblaze-Part1

Tue May 23 22:27:44 CEST 2006

In message <005b01c67e90$36e44dc0$0200a8c0 at 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 at 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