[U-Boot] [PATCH 1/2] LPC2468 support

[Wolfgang Denk]

Dear Remco Poelstra,

In message <49C10B37.1070506 at duran-audio.com> you wrote:
> This patch includes support for the LPC2468 processor from NXP.
> 
> Signed-off-by: Remco Poelstra <remco.poelstra+u-boot at duran-audio.com>

General comment: There are lots  of  coding  style  issues:  trailing
white  space,  incorrect  brace  style, lines too long, C++ comments,
incorrect indentation, indentation not by TAB, etc.

Please clean up.


> diff -upNr u-boot-orig/cpu/arm720t/cpu.c u-boot/cpu/arm720t/cpu.c
> --- u-boot-orig/cpu/arm720t/cpu.c	2009-03-18 00:42:12.000000000 +0100
> +++ u-boot/cpu/arm720t/cpu.c	2009-03-18 09:54:58.000000000 +0100
> @@ -78,6 +78,23 @@ int cleanup_before_linux (void)
>   	/* Nothing more needed */
>   #elif defined(CONFIG_INTEGRATOR) && defined(CONFIG_ARCH_INTEGRATOR)
>   	/* No cleanup before linux for IntegratorAP/CM720T as yet */
> +#elif defined(CONFIG_LPC2468)
> +	disable_interrupts ();
> +	{
> +	    volatile unsigned char dummy,i;

Please do not use arbitrary blocks to declare variables right in the
middle of the function.

> +	    U0IER = 0;
> +	    U1IER = 0;
> +	    for(i=0; i<16; i++)
> +	    {

Incorrect brace style, here and in many other places.
Indetation not by TAB, here and in many other places.

> +		dummy=U0RBR;
> +		dummy=U0LSR;
> +		dummy=U0IIR;
> +		dummy=U1RBR;
> +		dummy=U1LSR;
> +		dummy=U1IIR;
> +	    }

What sort of magic is this "code" supposed to perform?


> diff -upNr u-boot-orig/cpu/arm720t/serial.c u-boot/cpu/arm720t/serial.c
> --- u-boot-orig/cpu/arm720t/serial.c	2009-03-18 00:42:12.000000000 +0100
> +++ u-boot/cpu/arm720t/serial.c	2009-03-18 12:29:00.000000000 +0100
> @@ -199,4 +199,91 @@ int serial_tstc (void)
>   	return (GET8(U0LSR) & 1);
>   }
> 
> +#elif defined(CONFIG_LPC2468)
> +#include <asm/arch/hardware.h>
> +
> +int serial_init (void)
> +{
> +  unsigned long pinsel0;
> +
> +  //enable uart #0 pins in GPIO (P0.2 = TxD0, P0.3 = RxD0)
> +  pinsel0 = PINSEL0;
> +  pinsel0 &= ~(0x000000f0);
> +  pinsel0 |=   0x00000050;
> +  PINSEL0 = pinsel0;

Please use proper accessor functions to access device registers, here
and everywhere else.

> +void serial_setbrg (void)
> +{
> +  DECLARE_GLOBAL_DATA_PTR;
> +
> +  unsigned short divisor = 0;
> +  unsigned long fractional = 0;
> +
> +  switch (gd->baudrate) {
> +  case   1200:	divisor = 3000;	fractional = 0xF0; break;
> +  case   9600:	divisor =  375;	fractional = 0xF0; break;
> +//  case  19200:	divisor =  188;	fractional = 0; break;
> +  case  19200:	divisor =  175;	fractional = 0xAF; break;
> +  case  38400:	divisor =   94;	fractional = 0; break;
> +//  case  38400:	divisor =   75;	fractional = 0xC3; break;
> +//  case  57600:	divisor =   63;	fractional = 0; break;
> +  case  57600:	divisor =   50;	fractional = 0xC3; break;
> +//  case 115200:	divisor =   31;	fractional = 0; break;
...
> +//#if 0
...

No such dead code, please!

> diff -upNr u-boot-orig/include/asm-arm/arch-lpc24xx/lpc24xx_registers.h u-boot/include/asm-arm/arch-lpc24xx/lpc24xx_registers.h
> --- u-boot-orig/include/asm-arm/arch-lpc24xx/lpc24xx_registers.h	1970-01-01 01:00:00.000000000 +0100
> +++ u-boot/include/asm-arm/arch-lpc24xx/lpc24xx_registers.h	2009-03-18 15:43:41.000000000 +0100
> @@ -0,0 +1,1123 @@
> +#ifndef __LPC24XX_REGISTERS_H
> +#define __LPC24XX_REGISTERS_H
> +
> +#include <config.h>
> +
> +/* Macros for reading/writing registers */
> +//#define PUT8(reg, value) (*(volatile unsigned char*)(reg) = (value))
> +//#define PUT16(reg, value) (*(volatile unsigned short*)(reg) = (value))
> +//#define PUT32(reg, value) (*(volatile unsigned int*)(reg) = (value))
> +//#define GET8(reg) (*(volatile unsigned char*)(reg))
> +//#define GET16(reg) (*(volatile unsigned short*)(reg))
> +//#define GET32(reg) (*(volatile unsigned int*)(reg))

No dead code, here and elsewhere, please.


> +
> +/* Vectored Interrupt Controller (VIC) */
> +#define VIC_BASE_ADDR	0xFFFFF000
> +#define VICIRQStatus   (*(volatile unsigned long *)(VIC_BASE_ADDR + 0x000))
> +#define VICFIQStatus   (*(volatile unsigned long *)(VIC_BASE_ADDR + 0x004))
> +#define VICRawIntr     (*(volatile unsigned long *)(VIC_BASE_ADDR + 0x008))
> +#define VICIntSelect   (*(volatile unsigned long *)(VIC_BASE_ADDR + 0x00C))
> +#define VICIntEnable   (*(volatile unsigned long *)(VIC_BASE_ADDR + 0x010))
> +#define VICIntEnClr    (*(volatile unsigned long *)(VIC_BASE_ADDR + 0x014))
> +#define VICSoftInt     (*(volatile unsigned long *)(VIC_BASE_ADDR + 0x018))
> +#define VICSoftIntClr  (*(volatile unsigned long *)(VIC_BASE_ADDR + 0x01C))
> +#define VICProtection  (*(volatile unsigned long *)(VIC_BASE_ADDR + 0x020))
> +#define VICSWPrioMask  (*(volatile unsigned long *)(VIC_BASE_ADDR + 0x024))

Please do not use offset lists, but define a proper C data  structure
instead.  And  never  ever  access  device  regiters  through  simple
volatile pointers. Use proper accessor functions. Here and elsewhere.


The whole code needs a *major* cleanup before resubmitting.

[Note: I do not spend time on the second part this time.]

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
To understand a program you must become  both  the  machine  and  the
program.