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

[Wolfgang Denk]

Dear Remco Poelstra,

In message <49C9EB4D.5050503 at duran-audio.com> you wrote:
>
> >> --- 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

Why not? I'm not aware of such a restriction?

> >> 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?

I would like to avoid the ever growing list of

	#if defined(this) || defined(that) || defined(...) || ...

Maybe we can have a common #define that covers the common case?

> >> 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
> >> +
> >> +/*
...
> >> + */
> >> +
> >> +#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.

Hm... that doesn't really make sense to me. Also, the error checking
in this file makes little sense 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'm not an expert for this processor, but I wonder it there might be
some form of sync instruction (or memory barrier or similar) needed?


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Given a choice between two theories, take the one which is funnier.