Wolfgang Denk schreef: > Dear Remco Poelstra, > > In message <49c8be7a.10...@duran-audio.com> you wrote: >> This patch includes support for the LPC2468 processor from NXP. >> >> The example board will follow when this patch is OK. > > Such a comment does not belong into the commit message. Please mode it > below the "---" line.
I see, I thought nothing else was allowed below the "---" >> Signed-off-by: Remco Poelstra <remco.poelstra+u-b...@duran-audio.com> >> --- >> diff -upNr u-boot-orig/cpu/arm720t/interrupts.c >> u-boot-cleanup/cpu/arm720t/interrupts.c >> --- u-boot-orig/cpu/arm720t/interrupts.c 2009-03-18 00:42:12.000000000 >> +0100 >> +++ u-boot-cleanup/cpu/arm720t/interrupts.c 2009-03-24 11:48:50.000000000 >> +0100 >> @@ -29,7 +29,11 @@ >> #include <common.h> >> #include <clps7111.h> >> #include <asm/proc-armv/ptrace.h> >> +#if defined(CONFIG_LPC2468) >> +#include <asm/arch/immap.h> >> +#else >> #include <asm/hardware.h> >> +#endif > > Is there no way we can do without such a #ifdef here? The problem is that start.S needs hardware.h, but the code in immap.h should not be included in start.S, so I can not merge hardware.h and immap.h > >> #ifndef CONFIG_NETARM >> /* we always count down the max. */ >> @@ -40,6 +44,11 @@ >> #ifdef CONFIG_LPC2292 >> #undef READ_TIMER >> #define READ_TIMER (0xFFFFFFFF - GET32(T0TC)) >> +#elif defined(CONFIG_LPC2468) >> +#undef TIMER_LOAD_VAL >> +#define TIMER_LOAD_VAL 0 >> +#undef READ_TIMER >> +#define READ_TIMER (0xFFFFFFFF - 0xE0004008) > > NAK. When you have to #unifdef existing variable definitions, then > ther eis something fundamentally wrong. Please fix this problem at the > cause, i. e. where the wroing values are defined. I'll look into this. > >> #endif >> >> #else >> @@ -80,6 +89,14 @@ void do_irq (struct pt_regs *pt_regs) >> pfnct = (void (*)(void))VICVectAddr; >> >> (*pfnct)(); >> +#elif defined(CONFIG_LPC2468) >> + void (*pfnct) (void); >> + vic_2468_t *vic = &(((immap_t *)CONFIG_SYS_IMMAP)->ahb.vic); >> + >> + pfnct = (void (*)(void))(&(vic->vicaddr)); >> + >> + (*pfnct) (); > > Is there no way to combine this code with the one for the LPC2292? It > doesn't look that different to me... Interesting point. I was wondering the same. The problem lies in the fact that you want this patch to use C data structures, while the LPC2292 code uses offset lists. I can not convert the LPC2292 code to C structures, since a) I can not test the code b) I get paid to design hardware, not getting my ports published. I'm doing this to give something back to the community, since I really appreciate the work done by other OSS developers. But I can not spend time on converting complete other architectures. I leave that to other (the original LPC2292?) developers. >> --- u-boot-orig/cpu/arm720t/lpc24xx/Makefile 1970-01-01 01:00:00.000000000 >> +0100 >> +++ u-boot-cleanup/cpu/arm720t/lpc24xx/Makefile 2009-03-19 >> 10:56:53.000000000 +0100 > ... >> +$(SOBJS): >> + $(CC) $(AFLAGS) -march=armv4t -c -o $(SOBJS) iap_entry.S > > Such compile options hsould probably be set globally, not just for > this single source file? No, thumb code is less efficient in terms of performance, but this single file needs thumb code. See LPC2292. >> diff -upNr u-boot-orig/cpu/arm720t/start.S u-boot-cleanup/cpu/arm720t/start.S >> --- u-boot-orig/cpu/arm720t/start.S 2009-03-18 00:42:12.000000000 +0100 >> +++ u-boot-cleanup/cpu/arm720t/start.S 2009-03-24 11:52:35.000000000 >> +0100 >> @@ -127,7 +127,7 @@ reset: >> bl cpu_init_crit >> #endif >> >> -#ifdef CONFIG_LPC2292 >> +#if defined(CONFIG_LPC2292) || defined(CONFIG_LPC2468) > > Is there no way to combine this code with the one for the LPC2292? I'm sorry, it is combined in this case, no? >> #else >> #error No cpu_init_crit() defined for current CPU type >> #endif >> @@ -383,7 +387,7 @@ lock_loop: >> str r1, [r0] >> #endif >> >> -#ifndef CONFIG_LPC2292 >> +#if !defined(CONFIG_LPC2292) && !defined(CONFIG_LPC2468) > > Is there no way to combine this code with the one for the LPC2292? Same here. > >> mov ip, lr >> /* >> * before relocating, we have to setup RAM timing >> @@ -601,7 +605,7 @@ reset_cpu: >> * on external peripherals such as watchdog timers, etc. */ >> #elif defined(CONFIG_INTEGRATOR) && defined(CONFIG_ARCH_INTEGRATOR) >> /* No specific reset actions for IntegratorAP/CM720T as yet */ >> -#elif defined(CONFIG_LPC2292) >> +#elif defined(CONFIG_LPC2292) || defined(CONFIG_LPC2468) > > Is there no way to combine this code with the one for the LPC2292? Same here. > >> .align 5 >> .globl reset_cpu >> reset_cpu: >> diff -upNr u-boot-orig/include/asm-arm/arch-lpc24xx/hardware.h >> u-boot-cleanup/include/asm-arm/arch-lpc24xx/hardware.h >> --- u-boot-orig/include/asm-arm/arch-lpc24xx/hardware.h 1970-01-01 >> 01:00:00.000000000 +0100 >> +++ u-boot-cleanup/include/asm-arm/arch-lpc24xx/hardware.h 2009-03-24 >> 11:54:44.000000000 +0100 >> @@ -0,0 +1,32 @@ >> +#ifndef __ASM_ARCH_HARDWARE_H >> +#define __ASM_ARCH_HARDWARE_H >> + >> +/* >> + * Copyright (c) 2004 Cucy Systems (http://www.cucy.com) >> + * Curt Brune <c...@cucy.com> >> + * >> + * See file CREDITS for list of people who contributed to this >> + * project. >> + * >> + * This program is free software; you can redistribute it and/or >> + * modify it under the terms of the GNU General Public License as >> + * published by the Free Software Foundation; either version 2 of >> + * the License, or (at your option) any later version. >> + * >> + * This program is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU General Public License for more details. >> + * >> + * You should have received a copy of the GNU General Public License >> + * along with this program; if not, write to the Free Software >> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, >> + * MA 02111-1307 USA >> + */ >> + >> +#if defined(CONFIG_LPC2468) >> +#else >> +#error No hardware file defined for this configuration >> +#endif >> + >> +#endif /* __ASM_ARCH_HARDWARE_H */ > > Ummm... What exactly is this file needed for? I don't need it, but start.S wants to include it. See my comment about the #ifdef's. Other architectures left it empty too, so it seemed the best option to me. >> +/* 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)) > > Do you clain these are proper accessor functions for your processor? Yes I do. They are straight from the LPC2292 code, so once they were considered OK. I checked out the the write{s,l,b} functions in asm/io.h, but although they look similar, for some reason they simply don't work. Given the similarities between the write{s,l,b} and the PUT* functions, what is the problem with those? Furthermore, the ARM architecture doesn't use any kind of special instructions for accessing registers, everything is memory mapped. I do understand that you want the best code for U-boot, but I do not entirely agree on all points. Certainly when I look at the code already in place in U-boot. Please tell me what you really want to see changed/whether you expect me to convert the LPC2292 code. Depending on the amount of work left I'll reconsider submitting a patch. Kind regards, Remco Poelstra _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot