On 02.08.21 11:18, Rasmus Villemoes wrote:
On 31/07/2021 12.06, Stefan Roese wrote:
Hi Rasmus,
On 15.07.21 10:15, Stefan Roese wrote:
Hi Rasmus,
On 05.07.21 17:30, Simon Glass wrote:
Hi Rasmus,
On Fri, 2 Jul 2021 at 06:45, Rasmus Villemoes
<rasmus.villem...@prevas.dk> wrote:
A board can have and make use of more than one watchdog device, say
one built into the SOC and an external gpio-petted one. Having
wdt-uclass only handle the first is both a little arbitrary and
unexpected.
So change initr_watchdog() so we visit (probe) all DM watchdog
devices. This essentially boils down to making the init_watchdog_dev
function into a .post_probe method.
Similarly let watchdog_reset() loop over the whole uclass - each
having their own ratelimiting metadata, and a separate "is this device
running" flag.
This gets rid of the watchdog_dev member of struct global_data. We
do, however, still need the GD_FLG_WDT_READY set in
initr_watchdog(). This is because watchdog_reset() can get called
before DM is ready, and I don't think we can call uclass_get() that
early.
The current code just returns 0 if "getting" the first device fails -
that can of course happen because there are no devices, but it could
also happen if its ->probe call failed. In keeping with that, continue
with the handling of the remaining devices even if one fails to
probe. This is also why we cannot use uclass_probe_all().
If desired, it's possible to later add a per-device "u-boot,autostart"
boolean property, so that one can do CONFIG_WATCHDOG_AUTOSTART
per-device.
Reviewed-by: Stefan Roese <s...@denx.de>
Signed-off-by: Rasmus Villemoes <rasmus.villem...@prevas.dk>
---
drivers/watchdog/wdt-uclass.c | 96
++++++++++++++++++-------------
include/asm-generic/global_data.h | 6 --
2 files changed, 56 insertions(+), 46 deletions(-)
Please see my belated reply on the previous version of this patch.
Rasmus, do you plan to send an updated patchset version, addressing
Simons's comments?
Any updates on this?
Just got back from vacation and trying to catch up.
I'm a little confused. On June 29, you said
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.
Then Simon does reply on July 5
Imagine you probe
the device but then go into SDRAM init that hangs the CPU for a few
seconds. We need a way to signal that we want the watchdog to start,
so the board owner has a choice as to when this happens.
[The board owner does have a choice, it's called CONFIG_WATCHDOG_AUTOSTART.]
I also understand this is not quite how things work at present and I'm
fine with copying the existing behaviour.
which I didn't read as an objection, as I am precisely not changing any
existing behaviour. As I've said previously, keeping what is done in my
current post_probe function in a separate helper, called from inside the
loop in initr_watchdog which probes all watchdog devices, won't change
anything at all.
But precisely because it won't change anything, in the interest of
getting the gpio driver in, I'll send a v4 with that change alone, then
I'll let Stefan pick whichever version he and Simon can agree to.
But let me one last time repeat why I think the post_probe approach is
the cleanest and a natural fit for DM: post_probe is (AIUI) a place
where a uclass can do some action it wants done for every device
belonging to that uclass. When CONFIG_WATCHDOG_AUTOSTART is set,
wdt_uclass does have such an action. When it's not set, the post_probe
method is a no-op.
I might have misunderstood Simon. So if Simon is "okay" with v3, then
I can push this version soon - no need to send a v4 then.
Simon?
Thanks,
Stefan
