* [PATCH] common: board_r: Initialize interrupts before watchdog
@ 2025-02-18 15:55 ` J. Neuschäfer via B4 Relay
0 siblings, 0 replies; 6+ messages in thread
From: J. Neuschäfer @ 2025-02-18 15:55 UTC (permalink / raw)
To: Tom Rini; +Cc: u-boot, J. Neuschäfer
On some platforms, initializing the watchdog driver enables a timer
interrupt. This of course requires the interrupt handlers to be
properly initialized, otherwise U-Boot may crash or run the timer
interrupt handler of a previous bootloader stage.
To account for such systems, always initialize interrupts
(arch_initr_trap) before the watchdog (initr_watchdog).
This problem was observed on a PowerPC MPC83xx board.
Signed-off-by: J. Neuschäfer <j.ne@posteo.net>
---
NOTE: This approach seems safe and fine to me, but an argument could be
made that this should be fixed in the platform-specific drivers
instead. Please let me know what you think.
Rough stack trace (not sure if it should be part of the commit message):
initr_watchdog (drivers/watchdog/wdt-uclass.c)
device_probe(wdt@200)
device_probe(timer)
mpc8xxx_wdt_start (drivers/watchdog/mpc8xxx_wdt.c)
set_msr(get_msr() | MSR_EE);
arch_initr_trap (arch/powerpc/lib/traps.c)
trap_init (arch/powerpc/cpu/mpc83xx/start.S)
---
common/board_r.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/common/board_r.c b/common/board_r.c
index 179259b00de81f7ba9802fc5288e7c2b6e6f381a..f711cd237ae76d80ca2413017bfe131656f44180 100644
--- a/common/board_r.c
+++ b/common/board_r.c
@@ -652,11 +652,11 @@ static init_fnc_t init_sequence_r[] = {
serial_initialize,
initr_announce,
dm_announce,
+ arch_initr_trap,
#if CONFIG_IS_ENABLED(WDT)
initr_watchdog,
#endif
INIT_FUNC_WATCHDOG_RESET
- arch_initr_trap,
#if defined(CONFIG_BOARD_EARLY_INIT_R)
board_early_init_r,
#endif
---
base-commit: 064556910e61044f1295162ceaad600582b66cda
change-id: 20250218-init-dab1fc72abd2
Best regards,
--
J. Neuschäfer <j.ne@posteo.net>
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH] common: board_r: Initialize interrupts before watchdog @ 2025-02-18 15:55 ` J. Neuschäfer via B4 Relay 0 siblings, 0 replies; 6+ messages in thread From: J. Neuschäfer via B4 Relay @ 2025-02-18 15:55 UTC (permalink / raw) To: Tom Rini; +Cc: u-boot, J. Neuschäfer From: "J. Neuschäfer" <j.ne@posteo.net> On some platforms, initializing the watchdog driver enables a timer interrupt. This of course requires the interrupt handlers to be properly initialized, otherwise U-Boot may crash or run the timer interrupt handler of a previous bootloader stage. To account for such systems, always initialize interrupts (arch_initr_trap) before the watchdog (initr_watchdog). This problem was observed on a PowerPC MPC83xx board. Signed-off-by: J. Neuschäfer <j.ne@posteo.net> --- NOTE: This approach seems safe and fine to me, but an argument could be made that this should be fixed in the platform-specific drivers instead. Please let me know what you think. Rough stack trace (not sure if it should be part of the commit message): initr_watchdog (drivers/watchdog/wdt-uclass.c) device_probe(wdt@200) device_probe(timer) mpc8xxx_wdt_start (drivers/watchdog/mpc8xxx_wdt.c) set_msr(get_msr() | MSR_EE); arch_initr_trap (arch/powerpc/lib/traps.c) trap_init (arch/powerpc/cpu/mpc83xx/start.S) --- common/board_r.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/board_r.c b/common/board_r.c index 179259b00de81f7ba9802fc5288e7c2b6e6f381a..f711cd237ae76d80ca2413017bfe131656f44180 100644 --- a/common/board_r.c +++ b/common/board_r.c @@ -652,11 +652,11 @@ static init_fnc_t init_sequence_r[] = { serial_initialize, initr_announce, dm_announce, + arch_initr_trap, #if CONFIG_IS_ENABLED(WDT) initr_watchdog, #endif INIT_FUNC_WATCHDOG_RESET - arch_initr_trap, #if defined(CONFIG_BOARD_EARLY_INIT_R) board_early_init_r, #endif --- base-commit: 064556910e61044f1295162ceaad600582b66cda change-id: 20250218-init-dab1fc72abd2 Best regards, -- J. Neuschäfer <j.ne@posteo.net> ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] common: board_r: Initialize interrupts before watchdog 2025-02-18 15:55 ` J. Neuschäfer via B4 Relay (?) @ 2025-02-20 13:49 ` Simon Glass 2025-02-22 18:58 ` J. Neuschäfer -1 siblings, 1 reply; 6+ messages in thread From: Simon Glass @ 2025-02-20 13:49 UTC (permalink / raw) To: j.ne; +Cc: Tom Rini, u-boot Hi J, On Tue, 18 Feb 2025 at 08:55, J. Neuschäfer via B4 Relay <devnull+j.ne.posteo.net@kernel.org> wrote: > > From: "J. Neuschäfer" <j.ne@posteo.net> > > On some platforms, initializing the watchdog driver enables a timer > interrupt. This of course requires the interrupt handlers to be > properly initialized, otherwise U-Boot may crash or run the timer > interrupt handler of a previous bootloader stage. > > To account for such systems, always initialize interrupts > (arch_initr_trap) before the watchdog (initr_watchdog). > > This problem was observed on a PowerPC MPC83xx board. > > Signed-off-by: J. Neuschäfer <j.ne@posteo.net> > --- > NOTE: This approach seems safe and fine to me, but an argument could be > made that this should be fixed in the platform-specific drivers > instead. Please let me know what you think. > > > Rough stack trace (not sure if it should be part of the commit message): > > initr_watchdog (drivers/watchdog/wdt-uclass.c) > device_probe(wdt@200) > device_probe(timer) > mpc8xxx_wdt_start (drivers/watchdog/mpc8xxx_wdt.c) > set_msr(get_msr() | MSR_EE); > > arch_initr_trap (arch/powerpc/lib/traps.c) > trap_init (arch/powerpc/cpu/mpc83xx/start.S) > --- > common/board_r.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/common/board_r.c b/common/board_r.c > index 179259b00de81f7ba9802fc5288e7c2b6e6f381a..f711cd237ae76d80ca2413017bfe131656f44180 100644 > --- a/common/board_r.c > +++ b/common/board_r.c > @@ -652,11 +652,11 @@ static init_fnc_t init_sequence_r[] = { > serial_initialize, > initr_announce, > dm_announce, > + arch_initr_trap, > #if CONFIG_IS_ENABLED(WDT) > initr_watchdog, > #endif > INIT_FUNC_WATCHDOG_RESET > - arch_initr_trap, > #if defined(CONFIG_BOARD_EARLY_INIT_R) > board_early_init_r, > #endif > > --- > base-commit: 064556910e61044f1295162ceaad600582b66cda > change-id: 20250218-init-dab1fc72abd2 > > Best regards, > -- > J. Neuschäfer <j.ne@posteo.net> > > The driver model way of doing this would be that your UCLASS_WDT driver calls uclass_first_device(UCLASS_IRQ) to make sure interrupts are ready. The arch_initr_trap thing should probably not be used in new code. Also, we have interrupt_init() which sounds like it is similar? Regards, Simon ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] common: board_r: Initialize interrupts before watchdog 2025-02-20 13:49 ` Simon Glass @ 2025-02-22 18:58 ` J. Neuschäfer 2025-02-24 17:54 ` Simon Glass 0 siblings, 1 reply; 6+ messages in thread From: J. Neuschäfer @ 2025-02-22 18:58 UTC (permalink / raw) To: Simon Glass; +Cc: j.ne, Tom Rini, u-boot, mario.six (CC'ing Mario Six regarding MPC83xx timer driver in U-Boot) On Thu, Feb 20, 2025 at 06:49:58AM -0700, Simon Glass wrote: > Hi J, > > On Tue, 18 Feb 2025 at 08:55, J. Neuschäfer via B4 Relay > <devnull+j.ne.posteo.net@kernel.org> wrote: > > > > From: "J. Neuschäfer" <j.ne@posteo.net> > > > > On some platforms, initializing the watchdog driver enables a timer > > interrupt. This of course requires the interrupt handlers to be > > properly initialized, otherwise U-Boot may crash or run the timer > > interrupt handler of a previous bootloader stage. > > > > To account for such systems, always initialize interrupts > > (arch_initr_trap) before the watchdog (initr_watchdog). > > > > This problem was observed on a PowerPC MPC83xx board. > > > > Signed-off-by: J. Neuschäfer <j.ne@posteo.net> > > --- > > NOTE: This approach seems safe and fine to me, but an argument could be > > made that this should be fixed in the platform-specific drivers > > instead. Please let me know what you think. > > > > > > Rough stack trace (not sure if it should be part of the commit message): > > > > initr_watchdog (drivers/watchdog/wdt-uclass.c) > > device_probe(wdt@200) > > device_probe(timer) > > mpc8xxx_wdt_start (drivers/watchdog/mpc8xxx_wdt.c) > > set_msr(get_msr() | MSR_EE); > > > > arch_initr_trap (arch/powerpc/lib/traps.c) > > trap_init (arch/powerpc/cpu/mpc83xx/start.S) > > --- > > common/board_r.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/common/board_r.c b/common/board_r.c > > index 179259b00de81f7ba9802fc5288e7c2b6e6f381a..f711cd237ae76d80ca2413017bfe131656f44180 100644 > > --- a/common/board_r.c > > +++ b/common/board_r.c > > @@ -652,11 +652,11 @@ static init_fnc_t init_sequence_r[] = { > > serial_initialize, > > initr_announce, > > dm_announce, > > + arch_initr_trap, > > #if CONFIG_IS_ENABLED(WDT) > > initr_watchdog, > > #endif > > INIT_FUNC_WATCHDOG_RESET > > - arch_initr_trap, > > #if defined(CONFIG_BOARD_EARLY_INIT_R) > > board_early_init_r, > > #endif > > > > --- > > base-commit: 064556910e61044f1295162ceaad600582b66cda > > change-id: 20250218-init-dab1fc72abd2 > > > > Best regards, > > -- > > J. Neuschäfer <j.ne@posteo.net> > > > > > > The driver model way of doing this would be that your UCLASS_WDT > driver calls uclass_first_device(UCLASS_IRQ) to make sure interrupts > are ready. This sounds good in principle, but would require some reworking of the MPC83xx interrupt infrastructure, because it currently doesn't declare a UCLASS_IRQ driver. > The arch_initr_trap thing should probably not be used in new code. > Also, we have interrupt_init() which sounds like it is similar? This isn't really *new* code, the MPC83xx platform support has been in U-Boot for a long time (I am touching it now due to a newly to be added board, though). I see various functions called interrupt_init(), one of them in mpc83xx_timer.c (i.e. relevant to my problems): /* * TODO(mario.six@gdsys.cc): This should really be done by timer_init, and the * interrupt init should go into a interrupt driver. */ int interrupt_init(void) { ... } it seems someone has stumbled on this oddity before. Another version of interrupt_init is in arch/powerpc/lib/interrupts.c. Fortunately, as I just found out, my PowerPC MPC8314E board works fine with the generic timer/interrupt support in arch/powerpc, with CONFIG_TIMER=n. In other words, the problem is localized to CONFIG_MPC83XX_TIMER. J. Neuschäfer ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] common: board_r: Initialize interrupts before watchdog 2025-02-22 18:58 ` J. Neuschäfer @ 2025-02-24 17:54 ` Simon Glass 2025-02-24 18:48 ` PowerPC MPC83XX timer issues (Re: [PATCH] common: board_r: Initialize interrupts before watchdog) J. Neuschäfer 0 siblings, 1 reply; 6+ messages in thread From: Simon Glass @ 2025-02-24 17:54 UTC (permalink / raw) To: J. Neuschäfer; +Cc: Tom Rini, u-boot, mario.six Hi J, On Sat, 22 Feb 2025 at 11:58, J. Neuschäfer <j.ne@posteo.net> wrote: > > (CC'ing Mario Six regarding MPC83xx timer driver in U-Boot) > > On Thu, Feb 20, 2025 at 06:49:58AM -0700, Simon Glass wrote: > > Hi J, > > > > On Tue, 18 Feb 2025 at 08:55, J. Neuschäfer via B4 Relay > > <devnull+j.ne.posteo.net@kernel.org> wrote: > > > > > > From: "J. Neuschäfer" <j.ne@posteo.net> > > > > > > On some platforms, initializing the watchdog driver enables a timer > > > interrupt. This of course requires the interrupt handlers to be > > > properly initialized, otherwise U-Boot may crash or run the timer > > > interrupt handler of a previous bootloader stage. > > > > > > To account for such systems, always initialize interrupts > > > (arch_initr_trap) before the watchdog (initr_watchdog). > > > > > > This problem was observed on a PowerPC MPC83xx board. > > > > > > Signed-off-by: J. Neuschäfer <j.ne@posteo.net> > > > --- > > > NOTE: This approach seems safe and fine to me, but an argument could be > > > made that this should be fixed in the platform-specific drivers > > > instead. Please let me know what you think. > > > > > > > > > Rough stack trace (not sure if it should be part of the commit message): > > > > > > initr_watchdog (drivers/watchdog/wdt-uclass.c) > > > device_probe(wdt@200) > > > device_probe(timer) > > > mpc8xxx_wdt_start (drivers/watchdog/mpc8xxx_wdt.c) > > > set_msr(get_msr() | MSR_EE); > > > > > > arch_initr_trap (arch/powerpc/lib/traps.c) > > > trap_init (arch/powerpc/cpu/mpc83xx/start.S) > > > --- > > > common/board_r.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/common/board_r.c b/common/board_r.c > > > index 179259b00de81f7ba9802fc5288e7c2b6e6f381a..f711cd237ae76d80ca2413017bfe131656f44180 100644 > > > --- a/common/board_r.c > > > +++ b/common/board_r.c > > > @@ -652,11 +652,11 @@ static init_fnc_t init_sequence_r[] = { > > > serial_initialize, > > > initr_announce, > > > dm_announce, > > > + arch_initr_trap, > > > #if CONFIG_IS_ENABLED(WDT) > > > initr_watchdog, > > > #endif > > > INIT_FUNC_WATCHDOG_RESET > > > - arch_initr_trap, > > > #if defined(CONFIG_BOARD_EARLY_INIT_R) > > > board_early_init_r, > > > #endif > > > > > > --- > > > base-commit: 064556910e61044f1295162ceaad600582b66cda > > > change-id: 20250218-init-dab1fc72abd2 > > > > > > Best regards, > > > -- > > > J. Neuschäfer <j.ne@posteo.net> > > > > > > > > > > The driver model way of doing this would be that your UCLASS_WDT > > driver calls uclass_first_device(UCLASS_IRQ) to make sure interrupts > > are ready. > > This sounds good in principle, but would require some reworking of the > MPC83xx interrupt infrastructure, because it currently doesn't declare a > UCLASS_IRQ driver. > > > The arch_initr_trap thing should probably not be used in new code. > > Also, we have interrupt_init() which sounds like it is similar? > > This isn't really *new* code, the MPC83xx platform support has been in > U-Boot for a long time (I am touching it now due to a newly to be added > board, though). > > I see various functions called interrupt_init(), one of them in > mpc83xx_timer.c (i.e. relevant to my problems): > > /* > * TODO(mario.six@gdsys.cc): This should really be done by timer_init, and the > * interrupt init should go into a interrupt driver. > */ > int interrupt_init(void) > { ... } > > it seems someone has stumbled on this oddity before. > > Another version of interrupt_init is in arch/powerpc/lib/interrupts.c. > Fortunately, as I just found out, my PowerPC MPC8314E board works fine > with the generic timer/interrupt support in arch/powerpc, with > CONFIG_TIMER=n. In other words, the problem is localized to > CONFIG_MPC83XX_TIMER. Thanks for all the info. So do you think you could clean this up in PowerPC? Regards, Simon ^ permalink raw reply [flat|nested] 6+ messages in thread
* PowerPC MPC83XX timer issues (Re: [PATCH] common: board_r: Initialize interrupts before watchdog) 2025-02-24 17:54 ` Simon Glass @ 2025-02-24 18:48 ` J. Neuschäfer 0 siblings, 0 replies; 6+ messages in thread From: J. Neuschäfer @ 2025-02-24 18:48 UTC (permalink / raw) To: Simon Glass; +Cc: J. Neuschäfer, Tom Rini, u-boot, mario.six On Mon, Feb 24, 2025 at 10:54:05AM -0700, Simon Glass wrote: > Hi J, > > On Sat, 22 Feb 2025 at 11:58, J. Neuschäfer <j.ne@posteo.net> wrote: > > > > (CC'ing Mario Six regarding MPC83xx timer driver in U-Boot) > > > > On Thu, Feb 20, 2025 at 06:49:58AM -0700, Simon Glass wrote: > > > Hi J, > > > > > > On Tue, 18 Feb 2025 at 08:55, J. Neuschäfer via B4 Relay > > > <devnull+j.ne.posteo.net@kernel.org> wrote: > > > > > > > > From: "J. Neuschäfer" <j.ne@posteo.net> > > > > > > > > On some platforms, initializing the watchdog driver enables a timer > > > > interrupt. This of course requires the interrupt handlers to be > > > > properly initialized, otherwise U-Boot may crash or run the timer > > > > interrupt handler of a previous bootloader stage. > > > > > > > > To account for such systems, always initialize interrupts > > > > (arch_initr_trap) before the watchdog (initr_watchdog). > > > > > > > > This problem was observed on a PowerPC MPC83xx board. > > > > > > > > Signed-off-by: J. Neuschäfer <j.ne@posteo.net> > > > > --- > > > > NOTE: This approach seems safe and fine to me, but an argument could be > > > > made that this should be fixed in the platform-specific drivers > > > > instead. Please let me know what you think. > > > > > > > > > > > > Rough stack trace (not sure if it should be part of the commit message): > > > > > > > > initr_watchdog (drivers/watchdog/wdt-uclass.c) > > > > device_probe(wdt@200) > > > > device_probe(timer) > > > > mpc8xxx_wdt_start (drivers/watchdog/mpc8xxx_wdt.c) > > > > set_msr(get_msr() | MSR_EE); > > > > > > > > arch_initr_trap (arch/powerpc/lib/traps.c) > > > > trap_init (arch/powerpc/cpu/mpc83xx/start.S) > > > > --- > > > > common/board_r.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/common/board_r.c b/common/board_r.c > > > > index 179259b00de81f7ba9802fc5288e7c2b6e6f381a..f711cd237ae76d80ca2413017bfe131656f44180 100644 > > > > --- a/common/board_r.c > > > > +++ b/common/board_r.c > > > > @@ -652,11 +652,11 @@ static init_fnc_t init_sequence_r[] = { > > > > serial_initialize, > > > > initr_announce, > > > > dm_announce, > > > > + arch_initr_trap, > > > > #if CONFIG_IS_ENABLED(WDT) > > > > initr_watchdog, > > > > #endif > > > > INIT_FUNC_WATCHDOG_RESET > > > > - arch_initr_trap, > > > > #if defined(CONFIG_BOARD_EARLY_INIT_R) > > > > board_early_init_r, > > > > #endif > > > > > > > > --- > > > > base-commit: 064556910e61044f1295162ceaad600582b66cda > > > > change-id: 20250218-init-dab1fc72abd2 > > > > > > > > Best regards, > > > > -- > > > > J. Neuschäfer <j.ne@posteo.net> > > > > > > > > > > > > > > The driver model way of doing this would be that your UCLASS_WDT > > > driver calls uclass_first_device(UCLASS_IRQ) to make sure interrupts > > > are ready. > > > > This sounds good in principle, but would require some reworking of the > > MPC83xx interrupt infrastructure, because it currently doesn't declare a > > UCLASS_IRQ driver. > > > > > The arch_initr_trap thing should probably not be used in new code. > > > Also, we have interrupt_init() which sounds like it is similar? > > > > This isn't really *new* code, the MPC83xx platform support has been in > > U-Boot for a long time (I am touching it now due to a newly to be added > > board, though). > > > > I see various functions called interrupt_init(), one of them in > > mpc83xx_timer.c (i.e. relevant to my problems): > > > > /* > > * TODO(mario.six@gdsys.cc): This should really be done by timer_init, and the > > * interrupt init should go into a interrupt driver. > > */ > > int interrupt_init(void) > > { ... } > > > > it seems someone has stumbled on this oddity before. > > > > Another version of interrupt_init is in arch/powerpc/lib/interrupts.c. > > Fortunately, as I just found out, my PowerPC MPC8314E board works fine > > with the generic timer/interrupt support in arch/powerpc, with > > CONFIG_TIMER=n. In other words, the problem is localized to > > CONFIG_MPC83XX_TIMER. > > Thanks for all the info. So do you think you could clean this up in PowerPC? At the current moment, I'll rather use the the generic (non-MPC83xx) timer/irq support, for two reasons: 1. it works, and 2. I'm not familiar enough with U-Boot architecture and internals to come up with with a fix, without further research. Best regards, J. Neuschäfer ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-02-24 18:48 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-02-18 15:55 [PATCH] common: board_r: Initialize interrupts before watchdog J. Neuschäfer 2025-02-18 15:55 ` J. Neuschäfer via B4 Relay 2025-02-20 13:49 ` Simon Glass 2025-02-22 18:58 ` J. Neuschäfer 2025-02-24 17:54 ` Simon Glass 2025-02-24 18:48 ` PowerPC MPC83XX timer issues (Re: [PATCH] common: board_r: Initialize interrupts before watchdog) J. Neuschäfer
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.