From: Dirk Behme <dirk.behme@googlemail.com>
To: u-boot@lists.denx.de
Subject: [U-Boot] [PATCH V2] arm: timer and interrupt init rework
Date: Wed, 06 May 2009 21:20:06 +0200 [thread overview]
Message-ID: <4A01E2E6.2040308@googlemail.com> (raw)
In-Reply-To: <20090502125019.CD06F83420E8@gemini.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
prev parent reply other threads:[~2009-05-06 19:20 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2009-04-18 9:31 [U-Boot] [PATCH] arm: rename timer init callback timer_init Jean-Christophe PLAGNIOL-VILLARD
2009-04-27 21:53 ` Wolfgang Denk
2009-05-01 22:25 ` [U-Boot] [PATCH V2] arm: timer and interrupt init rework Jean-Christophe PLAGNIOL-VILLARD
2009-05-01 23:00 ` Dirk Behme
2009-05-01 23:14 ` Wolfgang Denk
2009-05-01 23:23 ` Jean-Christophe PLAGNIOL-VILLARD
2009-05-02 6:00 ` Dirk Behme
2009-05-02 12:50 ` Wolfgang Denk
2009-05-02 20:00 ` Jean-Christophe PLAGNIOL-VILLARD
2009-05-02 21:26 ` Wolfgang Denk
2009-05-04 12:41 ` Michael Roth
2009-05-06 19:20 ` Dirk Behme [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4A01E2E6.2040308@googlemail.com \
--to=dirk.behme@googlemail.com \
--cc=u-boot@lists.denx.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.