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 at 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 at kernel.org> wrote:
> > > >
> > > > From: "J. Neuschäfer" <j.ne at 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 at 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 at 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 at 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 at 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