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

[Stefan Roese]

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.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 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