[PATCH] common: board_r: Initialize interrupts before watchdog

[PATCH] common: board_r: Initialize interrupts before watchdog

[J. Neuschäfer via B4 Relay]

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>

Re: [PATCH] common: board_r: Initialize interrupts before watchdog

[Simon Glass]

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

Re: [PATCH] common: board_r: Initialize interrupts before watchdog

[J. Neuschäfer]

(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

Re: [PATCH] common: board_r: Initialize interrupts before watchdog

[Simon Glass]

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

PowerPC MPC83XX timer issues (Re: [PATCH] common: board_r: Initialize interrupts before watchdog)

[J. Neuschäfer]

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