[U-Boot-Users] [PATCH] FreeScale MC68360 support

[Wolfgang Denk]

Dear Ilya,

in message <8496f91a0704080420m1e72900cn70116c3dc7973b2b at mail.gmail.com> you wrote:
> 
> This patch adds support for MC68360 based board for u-boot-1.1.6. Mail me if
> you have any questions :)

Please correct me if I'm wrong, but MC68360 is  not  the  name  of  a
board,  but  of the Freescale (resp. ex Motorola) QICC CPU. There are
many different boards using this processor, so you will have to chose
a more descriptive board name.

Also, U-Boot 1.1.6 is outdated. Please submit your  patch  against  a
recent  version of U-Boot (i. e. top of tree in git repository, or at
least release 1.2.0).

Then, there are some coding style issues with your patch (indentation
not by TAB, indentation not by multiple of 8 columns, trailing  white
space,  C++  comments,  too long lines, etc.) as well as other formal
issues (missing Copyright entries, missing license  headers,  missing
Signed-off-by: line).

It seems you use a private flash driver for a device that looks as if
it was CFI conformant. Please explain why you cannot use the CFI
driver instead.

I  also  wonder  if  you  really   need   a   new   ethernet   driver
(cpu/mc68360/enet.c) and a new serial driver (cpu/mc68360/serial.c) -
IIRC  the  CPM  on  the 360 is mostly compatible (with very few minor
deviations that can be handled easily) to the CPM on the PowerQICC  I
(=  MPC8xx) processors. I think most of this could could (and should)
be shared? The same holds for  some  other  files  like  for  example
include/asm-m68k/arch-mc68360/commproc.h  which  is  highly redundand
with the MPC8xx commproc.h


Some of your files contain version ID stuff like here:

include/asm-m68k/arch-mc68360/mc68360_enet.h:

	***********************************
	* $Id: m68360_enet.h,v 1.1.1.1 2001/05/18 17:10:11 hamilton Exp $
	***********************************

Please remove this.


I see some "#if 0" blocks in your code; please remove these.

I see:

	#define CONFIG_BOOTFILE                        vmlinuz

This looks broken to me. U-Boot does not boot a vmlinuz file, but
U-Boot images created by mkimage.


#define CFG_BAUDRATE_TABLE             { 19200 }

This is *very* restrictive. I recommend to support all commonly  used
baudrates  instead  (i.  e.  at  least  the  range  from 9600 through
115200).


#define CFG_HZ                         (unsigned long int)32768000

This is broken. CFG_HZ is required to be 1000 (i. e. millisecond
ticks).


#define CFG_SDRAM_SIZE                 0x00800000

Please don't do that. U-Boot style is to allow auto-adjustment to the
actual RAM size.


#define CFG_MALLOC_LEN                 (5*1024*1024)

You have 8 MB of RAM and reserve 5 MB for malloc()? This seems broken
to me.


You comment out relevant parts of lib_m68k/board.c - you must not do
that!


lib_m68k/time.c - your "implementation" of udelay() is broken. Please
replace by a working version.




Please clean up these issues and resubmit.

Thanks.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH, HRB 165235 Munich, CEO: Wolfgang Denk
Office:  Kirchenstr. 5,       D-82194 Groebenzell,            Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Include the success of others in your dreams for your own success.