Re: [U-Boot] [PATCH V2] arm: timer and interrupt init rework
Wolfgang Denk wrote:
> Dear Jean-Christophe PLAGNIOL-VILLARD,
>
> In message <20090501232305.gi3...@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
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH V2] arm: timer and interrupt init rework
Wolfgang Denk schrieb: > What is "slow clock"? On (some) ARM SoC there are two oscillators: A "slow" free running oscillator typically with a dedicated 32 kHz crystal and the "main" oscillator with a dedicated crystal in the MHz range. The processor starts with the slow slock enabled and init code needs to set up PLLs and muxes etc. to enable the main oscillator and switch to derived clocks like master clock, cpu clock, IO clock and so on. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH V2] arm: timer and interrupt init rework
Dear Jean-Christophe PLAGNIOL-VILLARD,
In message <20090502200026.gl25...@game.jcrosoft.org> you wrote:
>
> > 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?
>
> First it you wish to do delay before the timer is ready you will have
> to do loop delay which is cpu speed dependent so the code is not
> maintainable at the end
I disagree. See for example the PowerPC code, where we can use the
always-present Time Base Register to provide reliable delays without
using counted loops. I think similar features are available on other
processors as well. And even if we have to initialize some general
purpose hardware timer, that does not mean that we have to initialize
full time services or even interrupts.
> > 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.
> On ARM we have two kind of "serial" port the physical one which need init
> to be used before and the DCC which can be used at the start of the CPU.
> So to debug the timer and earlier init you can use this one.
I'm referring to the standard serial port only.
But your argument was that board_init() would need delays (like
udelay()) which would not work without timers and interrupts.
> My changes does not break it as precedently the timer was init by
> interrupts_init
> the only change is that the board_init will be done after the timer
Yes, and I don't want to change this without real need. And I do not
see any such need yet - the existing code is working, isn't it?
> > [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.]
> IIRC as least the S3C must use interrupt for the timer
Maybe. But as mentioned before - full-fledged timer services and
simple delays are two different things.
> > 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.
> boards actually init the timer themself in thee board_init to have a
> correct delay
Hm... you said "as least the S3C must use interrupt for the timer" -
but as fas as I can see there is no timer init code anywhere in the
board specific code for smdk2400, smdk2410, or smdk6400.
Even for AT91 it is only done for a single board:
-> g timer board/atmel/*/*
board/atmel/at91cap9adk/at91cap9adk.c: extern void
timer_init(void);
board/atmel/at91cap9adk/at91cap9adk.c: timer_init();
board/atmel/at91rm9200dk/flash.c: /* arm simple, non interrupt
dependent timer */
board/atmel/at91rm9200dk/flash.c: reset_timer_masked ();
board/atmel/at91rm9200dk/flash.c: if
(get_timer_masked () > CONFIG_SYS_FLASH_ERASE_TOUT) {
board/atmel/at91rm9200dk/flash.c: /* arm simple, non interrupt dependent
timer */
board/atmel/at91rm9200dk/flash.c: reset_timer_masked ();
board/atmel/at91rm9200dk/flash.c: if (get_timer_masked () >
CONFIG_SYS_FLASH_ERASE_TOUT) {
board/atmel/atstk1000/flash.c: start_time = get_timer(0);
-> ls board/atmel/*/Makefile | wc -l
9
One out of 9...
> > Can we not do delays without interrupts?
> no not on all soc
I don't buy that. Do you have an example?
> > And do we need full-blown timer services for delays?
> Yes as we need to init the timer which is in nearly all case soc dependent
Please re-read my question. I accept that we may need a simple,
free-running timer which we can poll for a delay loop. But we do not
need full-blown timer services just to implement 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.]
> not only
> it's use for slow clock, for chip reset timing etc...
What is "slow clock"?
And if you reset a chip, you don't really care if the delay loop is
10 or 12 milliseconds, or do you?
> but the precision is just that if you need to wait 10us you will not wait less
If you have to wait 10us or more, then you will probably set a
timeout ouf 20us anyway.
> and if you want to wait
Re: [U-Boot] [PATCH V2] arm: timer and interrupt init rework
> 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? First it you wish to do delay before the timer is ready you will have to do loop delay which is cpu speed dependent so the code is not maintainable at the end > > 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. On ARM we have two kind of "serial" port the physical one which need init to be used before and the DCC which can be used at the start of the CPU. So to debug the timer and earlier init you can use this one. My changes does not break it as precedently the timer was init by interrupts_init the only change is that the board_init will be done after the timer > [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.] IIRC as least the S3C must use interrupt for the timer > > 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. boards actually init the timer themself in thee board_init to have a correct delay > > Can we not do delays without interrupts? no not on all soc > And do we need full-blown timer services for delays? Yes as we need to init the timer which is in nearly all case soc dependent > [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.] not only it's use for slow clock, for chip reset timing etc... but the precision is just that if you need to wait 10us you will not wait less and if you want to wait 100ms you will not have to wait 1s an other example for spi transfer you will have to use some delay more you timer is precise more you transfert will be work at higher rate it's the same for nand etc... Best Regards, J. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH V2] arm: timer and interrupt init rework
Dear Jean-Christophe PLAGNIOL-VILLARD,
In message <20090501232305.gi3...@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.]
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: w...@denx.de
Thought for the day: What if there were no hypothetical situations?
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH V2] arm: timer and interrupt init rework
Dear Jean-Christophe,
Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 01:00 Sat 02 May , Dirk Behme wrote:
>> 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
>> ...
>>> 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?
> simple to allow vertical patch to be applied instead of have merge problem
>
> so yes it's needed
Please elaborate more:
What is "vertical patch"?
Why to "to be applied"? I can't find any note in this patch that
mentions a dependency to any other patch.
Which "merge problem" do you expect?
So for what "it's needed"?
>>> 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
I understand this as confirmation that the initialization order
changes by this patch.
Please answer the additional questions in
http://lists.denx.de/pipermail/u-boot/2009-May/051925.html
http://lists.denx.de/pipermail/u-boot/2009-May/051928.html
then.
Dirk
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH V2] arm: timer and interrupt init rework
On 01:00 Sat 02 May , Dirk Behme wrote:
> 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
> ...
>> 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?
simple to allow vertical patch to be applied instead of have merge problem
so yes it's needed
>> 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
Best Regards,
J.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH V2] arm: timer and interrupt init rework
Dear Dirk,
In message <49fb7f1a.6090...@googlemail.com> you wrote:
>
> > --- 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"? ;)
Indeed. The patch needs to be split.
> > 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
Well spotted.
Jean-Christophe - what is your rationale for this change?
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: w...@denx.de
How many NASA managers does it take to screw in a lightbulb? "That's
a known problem... don't worry about it."
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH V2] arm: timer and interrupt init rework
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
...
> 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 0x
>
> -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
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH V2] arm: timer and interrupt init rework
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
---
board/armltd/integratorap/integratorap.c |2 +-
board/armltd/integratorcp/integratorcp.c |2 +-
board/atmel/at91cap9adk/at91cap9adk.c|1 -
board/davinci/common/misc.h |1 -
board/m501sk/m501sk.c|2 -
board/netstar/netstar.c |2 +-
board/voiceblue/voiceblue.c |2 +-
cpu/arm1136/mx31/Makefile|3 +-
cpu/arm1136/mx31/{interrupts.c => timer.c} |3 +-
cpu/arm1136/omap24xx/Makefile|3 +-
cpu/arm1136/omap24xx/{interrupts.c => timer.c} |3 +-
cpu/arm1176/s3c64xx/Makefile |2 +-
cpu/arm1176/s3c64xx/{interrupts.c => timer.c}|2 +-
cpu/arm720t/interrupts.c | 46 ++
cpu/arm920t/at91rm9200/Makefile |2 +-
cpu/arm920t/at91rm9200/{interrupts.c => timer.c} |2 +-
cpu/arm920t/imx/Makefile |4 +-
cpu/arm920t/imx/{interrupts.c => timer.c}|2 +-
cpu/arm920t/ks8695/Makefile |3 +-
cpu/arm920t/ks8695/{interrupts.c => timer.c} | 12 ++---
cpu/arm920t/s3c24x0/Makefile |5 ++-
cpu/arm920t/s3c24x0/{interrupts.c => timer.c}|2 +-
cpu/arm925t/Makefile |5 ++-
cpu/arm925t/{interrupts.c => timer.c}|2 +-
cpu/arm926ejs/Makefile |2 +-
cpu/arm926ejs/interrupts.c | 57 --
cpu/arm_cortexa8/omap3/Makefile |8 +++-
cpu/arm_cortexa8/omap3/{interrupts.c => timer.c} |2 +-
cpu/ixp/Makefile |6 +--
cpu/ixp/interrupts.c | 55 -
cpu/ixp/timer.c | 54
cpu/lh7a40x/Makefile |2 +-
cpu/lh7a40x/{interrupts.c => timer.c}|2 +-
cpu/pxa/Makefile |8 +++-
cpu/pxa/{interrupts.c => timer.c}|7 ++-
cpu/s3c44b0/Makefile |5 ++-
cpu/s3c44b0/{interrupts.c => timer.c}|2 +-
cpu/sa1100/Makefile |4 +-
cpu/sa1100/{interrupts.c => timer.c} |5 +-
include/asm-arm/u-boot-arm.h |3 +
include/configs/ixdpg425.h |1 +
include/configs/pdnb3.h |1 +
lib_arm/board.c |5 ++-
43 files changed, 167 insertions(+), 175 deletions(-)
rename cpu/arm1136/mx31/{interrupts.c => timer.c} (97%)
rename cpu/arm1136/omap24xx/{interrupts.c => timer.c} (97%)
rename cpu/arm1176/s3c64xx/{interrupts.c => timer.c} (99%)
rename cpu/arm920t/at91rm9200/{interrupts.c => timer.c} (99%)
rename cpu/arm920t/imx/{interrupts.c => timer.c} (99%)
rename cpu/arm920t/ks8695/{interrupts.c => timer.c} (93%)
rename cpu/arm920t/s3c24x0/{interrupts.c => timer.c} (99%)
rename cpu/arm925t/{interrupts.c => timer.c} (99%)
delete mode 100644 cpu/arm926ejs/interrupts.c
rename cpu/arm_cortexa8/omap3/{interrupts.c => timer.c} (99%)
rename cpu/lh7a40x/{interrupts.c => timer.c} (99%)
rename cpu/pxa/{interrupts.c => timer.c} (96%)
rename cpu/s3c44b0/{interrupts.c => timer.c} (98%)
rename cpu/sa1100/{interrupts.c => timer.c} (95%)
diff --git a/board/armltd/integratorap/integratorap.c
b/board/armltd/integratorap/integratorap.c
index 9631967..5ececd6 100644
--- a/board/armltd/integratorap/integratorap.c
+++ b/board/armltd/integratorap/integratorap.c
@@ -540,7 +540,7 @@ static ulong div_timer = 1; /* Divisor to convert
timer reading
* - the Integrator/AP timer issues an interrupt
* each time it reaches zero
*/
-int interrupt_init (void)
+int timer_init (void)
{
/* Load timer with initial value */
*(volatile ulong *)(CONFIG_SYS_TIMERBASE + 0) = TIMER_LOAD_VAL;
diff --git a/board/armltd/integratorcp/integratorcp.c
b/board/armltd/integratorcp/integratorcp.c
index 72629ce..0d3afd8 100644
--- a/board/armltd/integratorcp/integratorcp.c
+++ b/board/armltd/integratorcp/integratorcp.c
@@ -163,7 +163,7 @@ static ulong timestamp; /* U-Boot ticks since
startup */
/* starts up a
