On 20/02/2020 07.43, Stefan Roese wrote: > On 20.02.20 07:38, Christophe Leroy wrote: > > <snip> > >>>>> +void watchdog_reset(void) >>>>> +{ >>>>> + static ulong next_reset; >>>>> + ulong now; >>>>> + >>>>> + /* Exit if GD is not ready or watchdog is not initialized yet */ >>>>> + if (!gd || !(gd->flags & GD_FLG_WDT_READY)) >>>>> + return; >>>>> + >>>>> + /* Do not reset the watchdog too often */ >>>>> + now = get_timer(0); >>>>> + if (now > next_reset) { >>>>> + next_reset = now + 1000; /* reset every 1000ms */ >>>>> + wdt_reset(gd->watchdog_dev); >>>>> + } >>>> >>>> This is a problem for the MPC8xx. >>>> >>>> When running with a MPC8xx at 132MHz clock, the watchdog will fire >>>> about 1s after the last refresh. So the above makes the board unusable. >>> >>> So you need a shorted delay between the wdt_reset() calls? Is this >>> correct? We could introduce a new Kconfig option which defaults to >>> 1000 (ms) and you can "select" a shorter value for MPC8xx. >> >> Exactly. However, why is this limitation needed at all ? Why is it a >> problem to refresh more often ? > > Very likely its not. What is a reasonable value for your platform? 100 > or 500ms? I think we could change it to default to a shorter value, but > such a change should go in early in the merge window, so that other > platforms have a bit of time to test it. > > Please feel free to send a patch for this and please add a comment to > explain, why the delay is this "short".
IMO, this should come from DT. For a gpio watchdog (which for some reason U-Boot doesn't have a generic driver for) the linux kernel uses a hw_margin_ms property that tells the core how often the watchdog must be pinged - that could be generalized to apply for all, with 1000ms as a default if not set. And I've seen boards with a gpio watchdog with a timeout of 200 ms. Also, I'm wondering why that generic _reset only handles one watchdog device? I can easily imagine needing to reset both, say, an external gpio-triggered one and also the SOC's/CPU's built-in one. Why not loop over all DM watchdogs, and have the next_reset/hw_margin etc. metadata live with the watchdog device instead of in static variable/build-time constants? Rasmus