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.villem...@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 <s...@denx.de>
Thanks,
Stefan
