Wolfgang Denk schreef:
Dear Remco Poelstra,
In message 49C8BE7A.10504@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-boot@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 curt@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