[U-Boot] [PATCH V2] arm: timer and interrupt init rework

[Dirk Behme]

Dear Jean-Christophe,

Jean-Christophe PLAGNIOL-VILLARD wrote:
> actually the timer init use the interrupt_init as init callback
> which make the interrupt and timer implementation difficult to follow
> 
> so now rename it as int timer_init(void) and use interrupt_init for interrupt
> 
> btw also remane the corresponding file to the functionnality implemented
> 
> as ixp arch implement two timer - one based on interrupt - so all the timer
> related code is moved to timer.c
> 
> as some timer need interrupt and we need delay in the board init
> 
> the new init sequence is now
> interrupt_init (if used)
> timer_init
> board_init
> 
> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj at jcrosoft.com>
...
> diff --git a/cpu/arm_cortexa8/omap3/Makefile b/cpu/arm_cortexa8/omap3/Makefile
> index b96b3dd..edf5cb2 100644
> --- a/cpu/arm_cortexa8/omap3/Makefile
> +++ b/cpu/arm_cortexa8/omap3/Makefile
> @@ -26,7 +26,13 @@ include $(TOPDIR)/config.mk
>  LIB	=  $(obj)lib$(SOC).a
>  
>  SOBJS	:= lowlevel_init.o
> -COBJS	:= sys_info.o board.o clock.o interrupts.o mem.o syslib.o
> +
> +COBJS	+= board.o
> +COBJS	+= clock.o
> +COBJS	+= mem.o
> +COBJS	+= syslib.o
> +COBJS	+= sys_info.o
> +COBJS	+= timer.o

What do we win with this?

Why is this related to a patch named "timer and interrupt init rework"?

Maybe I'm wrong, but it's my feeling that you would reject something 
like this with e.g. "NACK, don't mix different clean up in one patch. 
Please split into several patches"? ;)

> diff --git a/cpu/arm_cortexa8/omap3/interrupts.c b/cpu/arm_cortexa8/omap3/timer.c
> similarity index 99%
> rename from cpu/arm_cortexa8/omap3/interrupts.c
> rename to cpu/arm_cortexa8/omap3/timer.c

I have to look into the details, but if I remember correctly, this 
file contains interrupt _and_ timer code? So that each name isn't 
totally correct, why touching it?

> index 9f1189f..e99b149 100644
> --- a/cpu/arm_cortexa8/omap3/interrupts.c
> +++ b/cpu/arm_cortexa8/omap3/timer.c
> @@ -178,7 +178,7 @@ static gptimer_t *timer_base = (gptimer_t *)CONFIG_SYS_TIMERBASE;
>  #define TIMER_CLOCK	(V_SCLK / (2 << CONFIG_SYS_PTV))
>  #define TIMER_LOAD_VAL	0xffffffff
>  
> -int interrupt_init(void)
> +int timer_init(void)
>  {
>  	/* start the counter ticking up, reload value on overflow */
>  	writel(TIMER_LOAD_VAL, &timer_base->tldr);

This is ok...

> diff --git a/lib_arm/board.c b/lib_arm/board.c
> index 5d05d9b..b678a63 100644
> --- a/lib_arm/board.c
> +++ b/lib_arm/board.c
> @@ -265,8 +265,11 @@ init_fnc_t *init_sequence[] = {
>  #if defined(CONFIG_ARCH_CPU_INIT)
>  	arch_cpu_init,		/* basic arch cpu dependent setup */
>  #endif
> -	board_init,		/* basic board dependent setup */
> +#if defined(CONFIG_USE_IRQ)
>  	interrupt_init,		/* set up exceptions */
> +#endif
> +	timer_init,		/* initialize timer */
> +	board_init,		/* basic board dependent setup */
>  	env_init,		/* initialize environment */
>  	init_baudrate,		/* initialze baudrate settings */
>  	serial_init,		/* serial communications setup */

... if you tested this on an OMAP3 board: I'm not sure, but it seems 
to me that the initialization order might change by this?

Old order: board_init -> interrupt_init (including timer_init)

New order: timer_init -> board_init

Is this correct? If yes, we have to check that there are no issues 
with dependencies?

On which OMAP3 board have you tested this?

Best regards

Dirk