[U-Boot] [PATCH] Add support for LPC2468 from NXP

[Remco Poelstra]

Hi,

On 02-06-10 12:03, Wolfgang Denk wrote:
>
>> @@ -80,6 +82,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) ();
>>      
> Please unify with code for the LPC2292 and get rid of the #ifdef.
>    

This is not possible. I do understand that there is a lot of similarity,
but I was asked to use C structures to access registers, but the lpc22xx
code uses direct access. I cannot convert the lpc22xx code, since I don't
have access to a board and it's a very error-prone process. I think it would
be better if the current lpc22xx maintainer converts the lpc22xx code to 
use
C structures as well. Unfortunatly, the same holds for most of the code 
merge/factor out
comments below.
>>   {
>> +#if defined(CONFIG_LPC2468)
>> +	timer_2468_t *timer0=&(((immap_t *)CONFIG_SYS_IMMAP)->apb.timer0);
>> +	ulong now = readl(&(timer0->tc));
>> +	
>> +	if (lastdec<= now) {
>> +		/* normal mode */
>> +		timestamp += now - lastdec;
>> +	} else {
>> +		/* we have an overflow ... */
>> +		timestamp += now + TIMER_LOAD_VAL - lastdec;
>> +	}
>> +#else
>>   	ulong now = READ_TIMER;
>>
>>   	if (lastdec>= now) {
>> @@ -261,6 +300,8 @@ ulong get_timer_masked (void)
>>   		/* we have an overflow ... */
>>   		timestamp += lastdec + TIMER_LOAD_VAL - now;
>>   	}
>> +#endif
>>      
> Ditto here.
>    

This one is different. The lpc2468 uses an upward counting timer,
while the other ARM's seem to use a downward counting timer (as
far as I could judge from the code).
I cannot set the timer to count downward, so this code must be different.
This may actually mean that the delay functions do not work on the lpc22xx.
The code my patches are based on was not functional either.
>> diff --git a/arch/arm/cpu/arm720t/lpc24xx/iap_entry.S b/arch/arm/cpu/arm720t/lpc24xx/iap_entry.S
>> new file mode 100644
>> index 0000000..c31d519
>> --- /dev/null
>> +++ b/arch/arm/cpu/arm720t/lpc24xx/iap_entry.S
>> @@ -0,0 +1,7 @@
>> +IAP_ADDRESS:	.word	0x7FFFFFF1
>> +
>> +.globl iap_entry
>> +iap_entry:
>> +	ldr	r2, IAP_ADDRESS
>> +	bx	r2
>> +	mov	pc, lr
>>      
> Verbatim copy of arch/arm/cpu/arm720t/lpc2292/iap_entry.S - please
> unify.
>    
Can you give a hint on how I should accomplish that? Copy the
iap_netry.S to the parent directiry?

> ...
>    
>> --- /dev/null
>> +++ b/arch/arm/include/asm/arch-lpc24xx/hardware.h
>> @@ -0,0 +1,32 @@
>> +#ifndef __ASM_ARCH_HARDWARE_H
>> +#define __ASM_ARCH_HARDWARE_H
>> +
>> +/*
>>      
> ...
>    
>> + */
>> +
>> +#if defined(CONFIG_LPC2468)
>> +#else
>> +#error No hardware file defined for this configuration
>> +#endif
>> +
>> +#endif /* __ASM_ARCH_HARDWARE_H */
>>      
> Do we really need such an empty file?
>    

Yes, start.S needs this file, but since my code uses C structures, it's 
empty.

> ...
>    
>> +typedef struct ssp1_2468 {
>> +	u8 fixme[0x4000];
>> +} ssp1_2468_t;
>> +
>> +typedef struct adc_2468 {
>> +	u8 fixme[0x4000];
>> +} adc_2468_t;
>> +
>> +typedef struct can_accept_ram_2468 {
>> +	u8 fixme[0x4000];
>> +} can_accept_ram_2468_t;
>> +
>> +typedef struct can_accept_filter_2468 {
>> +	u8 fixme[0x4000];
>> +} can_accept_filter_2468_t;
>> +
>> +typedef struct can_common_2468 {
>> +	u8 fixme[0x4000];
>> +} can_common_2468_t;
>> +
>> +typedef struct can1_2468 {
>> +	u8 fixme[0x4000];
>> +} can1_2468_t;
>> +
>> +typedef struct can2_2468 {
>> +	u8 fixme[0x4000];
>> +} can2_2468_t;
>> +
>> +typedef struct i2c1_2468 {
>> +	u8 fixme[0x4000];
>> +} i2c1_2468_t;
>>      
> ...
>
>
> Do we _really_ need all this?
>    

Yes and no. Not all H/W is used. That may well change in
the future as more peripherals are supported. I do also believe
that it is more readable if the mapping of the memory is
reflected in the file, rather than undefined chunks, just because
momentarily a peripheral isn't needed. I think this provides
a better base for future development.

Kind regards,

Remco Poelstra