[PATCH v3 07/10] watchdog: wdt-uclass.c: handle all DM watchdogs in watchdog_reset()

[Simon Glass]

Hi Rasmus,

On Mon, 2 Aug 2021 at 03:18, Rasmus Villemoes
<rasmus.villemoes at prevas.dk> 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.villemoes at 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 <sr at denx.de>
> >>>> Signed-off-by: Rasmus Villemoes <rasmus.villemoes at 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 can certainly see you point. It definitely looks very natural for DM.

But I still think the problem of a device possibly resetting the board
when probed (despite the various watchdog-reset things sprinkled
throughout the code) many means it is worth having an explicit 'start'
operation in this case. In fact this fits with DM even better, since
probe() is supposed to just fire up the hardware, not start any
operations.

Regards,
Simon