From mboxrd@z Thu Jan  1 00:00:00 1970
From: Dirk Behme <dirk.behme@googlemail.com>
Date: Wed, 06 May 2009 21:20:06 +0200
Subject: [U-Boot] [PATCH V2] arm: timer and interrupt init rework
In-Reply-To: <20090502125019.CD06F83420E8@gemini.denx.de>
References: <1240047101-6787-1-git-send-email-plagnioj@jcrosoft.com>
	<1241216732-13765-1-git-send-email-plagnioj@jcrosoft.com>
	<49FB7F1A.6090101@googlemail.com>
	<20090501232305.GI3291@game.jcrosoft.org>
	<20090502125019.CD06F83420E8@gemini.denx.de>
Message-ID: <4A01E2E6.2040308@googlemail.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Wolfgang Denk wrote:
> Dear Jean-Christophe PLAGNIOL-VILLARD,
> 
> In message <20090501232305.GI3291@game.jcrosoft.org> you wrote:
>>>> +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?
>> simple to allow vertical patch to be applied instead of have merge problem
>>
>> so yes it's needed
> 
> But it must go in a separate patch.
> 
>>>> 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?
>> maybe read the commit message will answer your question
> 
> Argh. Instead of snippy remarks you should read Dirks message yourself
> and answer his (very valid) questions:
> 
> | Is this correct? If yes, we have to check that there are no issues 
> | with dependencies?
> | 
> | On which OMAP3 board have you tested this?
> 
> Can you please explain on which boards this has actually been tested,
> and especially on which OMAP3 boards?
> 
> 
> Also, I do not see why we need to implement such a critical change.
> 
> If I understand you corrctly, your argument goes that board_init()
> needs delays (like udelay()), delays need timers, and timers need
> interrupts, so we must initialize first interrupts, then timers, and
> only then we can run board_init()?  Is this your argument?
> 
> But the I ask why udelay() would need  timers  and  interrupts?  This
> does  not fit into the design philosophy of U-Boot, which attempts to
> bring up a board at least to a state where  we  have  serial  console
> output  with  as  little as possible requirements. Your change breaks
> this, because now we have to initialize timers and interrupts  (which
> are  not  exactly  a  trivial thing to set up or debug if they aren't
> working correctly) BEFORE we have a console  output.  [I  ignore  the
> case  of CONFIG_USE_IRQ here, because only 4 boards actually use this
> feature, and they could probably be changed to do without, too.]
> 
> So while I really appreciate your attempts to clean up the timer code
> on ARM, the resulting consequences are expensive, and I  am  not  yet
> convincet  the  advantages  of  the  new  code  are  bigger than this
> disadvantage, and especially I am not convinced  thatthis  is  really
> necessary and unavoidable.
> 
> Can we not do delays without interrupts? And do  we  need  full-blown
> timer services for delays? [Keep in mind that a delay is usually used
> to  implement  a timeout in the error branch; that means, it does not
> matter if it has not 10e-6 precision or better.]

Btw, it seems that this patch is already in u-boot-arm next

http://git.denx.de/?p=u-boot/u-boot-arm.git;a=commit;h=482d69eafb6a78c82251f7a346cc67f12a9bd731

Did I miss an ACK somewhere? It's my understanding that this patch is 
still under discussion? Sorry if I missed something ;)

Best regards

Dirk