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

[Stefan Roese]

On 11.08.21 14:13, Rasmus Villemoes wrote:
> On 11/08/2021 13.49, Stefan Roese wrote:
>> Hi Rasmus,
>>
>> On 11.08.21 13:32, Rasmus Villemoes wrote:
>>> On 03/08/2021 10.28, Stefan Roese wrote:
>>>> Hi Rasmus,
>>>>
>>>>>     #endif
>>>>> diff --git a/include/asm-generic/global_data.h
>>>>> b/include/asm-generic/global_data.h
>>>>> index e55070303f..28d749538c 100644
>>>>> --- a/include/asm-generic/global_data.h
>>>>> +++ b/include/asm-generic/global_data.h
>>>>> @@ -447,12 +447,6 @@ struct global_data {
>>>>>          */
>>>>>         fdt_addr_t translation_offset;
>>>>>     #endif
>>>>> -#if CONFIG_IS_ENABLED(WDT)
>>>>> -    /**
>>>>> -     * @watchdog_dev: watchdog device
>>>>> -     */
>>>>> -    struct udevice *watchdog_dev;
>>>>> -#endif
>>>>
>>>> After applying the patchset v4 to current master, I see this error when
>>>> building for "x530":
>>>
>>> I'm not sure how I missed this, I was convinced I had done a grep and
>>> seen that all references to ->watchdog_dev were gone.
>>>
>>>> board/alliedtelesis/x530/x530.c: In function 'arch_preboot_os':
>>>> board/alliedtelesis/x530/x530.c:125:13: error: 'gd_t' {aka 'volatile
>>>> struct global_data'} has no member named 'watchdog_dev'
>>>>     125 |  wdt_stop(gd->watchdog_dev);
>>>>         |             ^~
>>>> make[1]: *** [scripts/Makefile.build:254:
>>>> board/alliedtelesis/x530/x530.o] Error 1
>>>>
>>>> Perhaps we need a common function now to stop all watchdogs, which can
>>>> be called from such places?
>>>
>>> Yes, I think that's the right thing, even if there's just one single
>>> caller. Dead code elimination should remove that global function for all
>>> other boards. Here is what I have right now:
>>>
>>>       watchdog: wdt-uclass: add wdt_stop_all() helper
>>>
>>>       Since the watchdog_dev member of struct global_data is going away in
>>>       favor of the wdt-uclass handling all watchdog devices, prepare for
>>>       that by adding a helper to call wdt_stop() on all known devices.
>>>
>>>       Initially, this will only be used in one single
>>>       place (board/alliedtelesis/x530/x530.c), and that user currently
>>>       ignores the return value of wdt_stop(). It's not clear what one
>>> should
>>>       do if we encounter an error (remember the error but still stop the
>>>       remaining ones? return immediately? try to unwind and
>>>       restart the devices already stopped?). Moreover, some watchdogs
>>> are by
>>>       design always-running (and don't have a ->stop method at all), so at
>>>       the very least some exception for -ENOSYS would be in order.
>>>
>>>       So for now, and until a real use case appears from which we can
>>> decide
>>>       the desired semantics, have the function return void and just emit a
>>>       log_debug() if an error is encountered.
>>>
>>> diff --git a/drivers/watchdog/wdt-uclass.c
>>> b/drivers/watchdog/wdt-uclass.c
>>> index 358fc68e27..75ff4c2a6c 100644
>>> --- a/drivers/watchdog/wdt-uclass.c
>>> +++ b/drivers/watchdog/wdt-uclass.c
>>> @@ -116,6 +116,31 @@ int wdt_stop(struct udevice *dev)
>>>           return ret;
>>>    }
>>>
>>> +void wdt_stop_all(void)
>>> +{
>>> +       struct wdt_priv *priv;
>>> +       struct udevice *dev;
>>> +       struct uclass *uc;
>>> +       int ret;
>>> +
>>> +       ret = uclass_get(UCLASS_WDT, &uc);
>>> +       if (ret) {
>>> +               log_debug("Error getting UCLASS_WDT: %d\n", ret);
>>
>> Perhaps log_err()?
> 
> No, we've already been over this in earlier discussions (it's the exact
> same pattern and reasoning as initr_watchdog). If I made it log_err(),
> it would cost .text for something that never-ever happens in practice,
> while log_debug() is usually a no-op, but can be compiled in if
> something truly fishy seems to be going on.

Okay, then please stick with log_debug().

>>>    int wdt_reset(struct udevice *dev)
>>>    {
>>>           const struct wdt_ops *ops = device_get_ops(dev);
>>>
>>> (along with a declaration in wdt.h, and another patch for x530 to make
>>> use of it). Something like that?
>>
>> Looks good, thanks for quickly working on this. Not sure, if this new
>> function should be "void" or better "int" so that the error can be
>> returned.
> 
> That's why I included my tentative commit log, so you could see my
> explanation for why I made it void.

Ah, sorry for not reading it thoroughly. My bad.

> Until some user shows up that
> _wants_ a return value, there's no point making it return int. When that
> user shows up, we can discuss which int (return early on failure?
> remember that an error was seen but still call wdt_stop on remaining
> devices? etc. etc.).

I'm fine with it.

>> A follow-up patch on-top your patchset v4 will cause git bisect
>> problems. So you need to integrate this into your patches and send a
>> v5 I'm afraid.
> 
> Definitely, it's already reworked in between 6 and 7 in my local branch,
> I just didn't want to send a 12-patch series without some "yeah,
> something like that would work".

Perfect. Please go ahead then.

Thanks,
Stefan