Dear Remco Poelstra,
In message 49C10B37.1070506@duran-audio.com you wrote:
This patch includes support for the LPC2468 processor from NXP.
Signed-off-by: Remco Poelstra remco.poelstra+u-boot@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