Dear Remco Poelstra,
In message <49c10b37.1070...@duran-audio.com> you wrote:
> This patch includes support for the LPC2468 processor from NXP.
>
> Signed-off-by: Remco Poelstra <remco.poelstra+u-b...@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: w...@denx.de
To understand a program you must become both the machine and the
program.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
