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

Wolfgang Denk
2009-03-19T21:22:30Z

Dear Remco Poelstra,

In message <49C25F75.3080906@duran-audio.com> you wrote:
> Wolfgang Denk schreef:
> >> +/* Vectored Interrupt Controller (VIC) */
> >> +#define VIC_BASE_ADDR 0xFFFFF000
> >> +#define VICIRQStatus (*(volatile unsigned long *)(VIC_BASE_ADDR + 0x000))
> >> +#define VICFIQStatus (*(volatile unsigned long *)(VIC_BASE_ADDR + 0x004))
> >> +#define VICRawIntr (*(volatile unsigned long *)(VIC_BASE_ADDR + 0x008))
> >> +#define VICIntSelect (*(volatile unsigned long *)(VIC_BASE_ADDR + 0x00C))
> >> +#define VICIntEnable (*(volatile unsigned long *)(VIC_BASE_ADDR + 0x010))
> >> +#define VICIntEnClr (*(volatile unsigned long *)(VIC_BASE_ADDR + 0x014))
> >> +#define VICSoftInt (*(volatile unsigned long *)(VIC_BASE_ADDR + 0x018))
> >> +#define VICSoftIntClr (*(volatile unsigned long *)(VIC_BASE_ADDR + 0x01C))
> >> +#define VICProtection (*(volatile unsigned long *)(VIC_BASE_ADDR + 0x020))
> >> +#define VICSWPrioMask (*(volatile unsigned long *)(VIC_BASE_ADDR + 0x024))
> >
> > Please do not use offset lists, but define a proper C data structure
> > instead. And never ever access device regiters through simple
> > volatile pointers. Use proper accessor functions. Here and elsewhere.
>
>
> All examples I checked use the same syntax for defining registers, so I left it
> in the code. If that is a problem, can you indicate an example which does The Right Thing (tm)?

See for example the structure declarations in include/asm-ppc/*immap*

> I think the patch now matches the other criteria. The patch is a bit bigger, since
> other code did not follow the Coding Styles either. I used indent to fix my code and it fixed the other code as well.

I hope you didn't dop this in a single step. Codin style cleanup and
adding new stuff must be done separately.

> The second patch will follow when this patch is OK.
>
> Regards,
>
> Remco Poelstra
>
> ----
> --- u-boot-orig/cpu/arm720t/cpu.c 2009-03-18 00:42:12.000000000 +0100
> +++ u-boot/cpu/arm720t/cpu.c 2009-03-19 16:00:04.000000000 +0100
> @@ -41,7 +41,9 @@ int cpu_init (void)
> * setup up stacks if necessary
> */
> #ifdef CONFIG_USE_IRQ
> - IRQ_STACK_START = _armboot_start - CONFIG_SYS_MALLOC_LEN - CONFIG_SYS_GBL_DATA_SIZE - 4;
> + IRQ_STACK_START =
> + _armboot_start - CONFIG_SYS_MALLOC_LEN - CONFIG_SYS_GBL_DATA_SIZE -
> + 4;
> FIQ_STACK_START = IRQ_STACK_START - CONFIG_STACKSIZE_IRQ;
> #endif
> return 0;
> @@ -63,17 +65,17 @@ int cleanup_before_linux (void)
> disable_interrupts ();
>
> /* turn off I-cache */
> - asm ("mrc p15, 0, %0, c1, c0, 0":"=r" (i));
> + asm ("mrc p15, 0, %0, c1, c0, 0":"=r" (i));
> i &= ~0x1000;
> - asm ("mcr p15, 0, %0, c1, c0, 0": :"r" (i));
> + asm ("mcr p15, 0, %0, c1, c0, 0": :"r" (i));

What sort of patch is this? There is no commit message, no explanation
what it is, no SoB, ... ?

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
"Deliver yesterday, code today, think tomorrow."