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

[Remco Poelstra <remco.poelstra@duran-audio.com>]

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