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

Remco Poelstra
2009-03-25T08:29:01Z

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