Hi J, On Sat, 22 Feb 2025 at 11:58, J. Neuschäfer <[email protected]> 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 > > <[email protected]> wrote: > > > > > > From: "J. Neuschäfer" <[email protected]> > > > > > > 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 <[email protected]> > > > --- > > > 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 <[email protected]> > > > > > > > > > > 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([email protected]): 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
