On 28.06.21 09:44, Rasmus Villemoes wrote:
On 26/06/2021 20.32, Simon Glass wrote:
Hi Rasmus,
On Tue, 22 Jun 2021 at 14:28, Rasmus Villemoes rasmus.villemoes@prevas.dk wrote:
I don't think it is good to start it in the post-probe. Can you do it separately, afterwards?
Eh, yes, of course I could do this in the loop in initr_watchdog() where I probe all watchdog devices, but the end result is exactly the same, and it seemed that this was a perfect fit for DM since it provided this post_probe hook. It's a no-op if CONFIG_WATCHDOG_AUTOSTART isn't set, and if it is, that precisely means the developer wanted to start handling the device(s) ASAP.
Probing is supposed to just probe it and it should be safe to do that without side effects.
I agree in general, but watchdog devices are a bit special compared to some random USB controller or LCD display or spi master - those devices generally don't do anything unless asked to by the CPU, while a watchdog device is the other way around, it does its thing _unless_ the CPU asks it not to (often enough). Which in turn, I suppose, is the whole reason wdt-uclass has its own hook into the initr sequence - one needs to probe and possibly start handling the watchdog(s) ASAP.
It still needs a 'start' method to make it start. Having it start on probe means the board will reset at the command line if the device is probed. Yuck.
No, because while sitting in the command line waiting for user input, WATCHDOG_RESET() is called something like a million times per second (or at least extremely often).
We have some rate-limiting in place since a few years (reset_period).
For the most common case of there only being one (or zero) DM watchdogs, I'm not changing anything at all about how things behave. I'm just expanding the handling done in the wdt-uclass provided functions initr_watchdog() and watchdog_reset() to all DM watchdogs, making things more consistent. And there's CONFIG_WATCHDOG_AUTOSTART=n which as before would make the post_probe function into a no-op.
As I said, yes, I can move the call of the post_probe function into the loop in initr_watchdog (and then it wouldn't be called post_probe, but probably named something including auto_start). In practice, that won't change anything.
Stefan, what do you think? I think this is the only contentious point at this time, so I'll do whatever you think is right, then resend the patches with Simon's other feedback incorporated.
I'm fine with this post_probe() implementation but have no strong feelings about this. So if Simon (or someone else) does not object, then please continue this way.
Reviewed-by: Stefan Roese sr@denx.de
Thanks, Stefan