Re: [PATCH 07/10] watchdog: rzg2l_wdt: Add suspend/resume support

[claudiu beznea <claudiu.beznea@tuxon.dev>]

On 23.01.2024 12:09, Guenter Roeck wrote:
> On 1/22/24 23:13, claudiu beznea wrote:
>>
>>
>> On 22.01.2024 19:39, Guenter Roeck wrote:
>>> On 1/22/24 03:11, Claudiu wrote:
>>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>>
>>>> The RZ/G3S supports deep sleep states where power to most of the IP blocks
>>>> is cut off. To ensure proper working of the watchdog when resuming from
>>>> such states, the suspend function is stopping the watchdog and the resume
>>>> function is starting it. There is no need to configure the watchdog
>>>> in case the watchdog was stopped prior to starting suspend.
>>>>
>>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>> ---
>>>>    drivers/watchdog/rzg2l_wdt.c | 26 ++++++++++++++++++++++++++
>>>>    1 file changed, 26 insertions(+)
>>>>
>>>> diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c
>>>> index 9333dc1a75ab..186796b739f7 100644
>>>> --- a/drivers/watchdog/rzg2l_wdt.c
>>>> +++ b/drivers/watchdog/rzg2l_wdt.c
>>>> @@ -279,6 +279,7 @@ static int rzg2l_wdt_probe(struct platform_device
>>>> *pdev)
>>>>        priv->wdev.timeout = WDT_DEFAULT_TIMEOUT;
>>>>          watchdog_set_drvdata(&priv->wdev, priv);
>>>> +    dev_set_drvdata(dev, priv);
>>>>        ret = devm_add_action_or_reset(&pdev->dev, rzg2l_wdt_pm_disable,
>>>> &priv->wdev);
>>>>        if (ret)
>>>>            return ret;
>>>> @@ -300,10 +301,35 @@ static const struct of_device_id rzg2l_wdt_ids[] = {
>>>>    };
>>>>    MODULE_DEVICE_TABLE(of, rzg2l_wdt_ids);
>>>>    +static int rzg2l_wdt_suspend_late(struct device *dev)
>>>> +{
>>>> +    struct rzg2l_wdt_priv *priv = dev_get_drvdata(dev);
>>>> +
>>>> +    if (!watchdog_active(&priv->wdev))
>>>> +        return 0;
>>>> +
>>>> +    return rzg2l_wdt_stop(&priv->wdev);
>>>> +}
>>>> +
>>>> +static int rzg2l_wdt_resume_early(struct device *dev)
>>>> +{
>>>> +    struct rzg2l_wdt_priv *priv = dev_get_drvdata(dev);
>>>> +
>>>> +    if (!watchdog_active(&priv->wdev))
>>>> +        return 0;
>>>> +
>>>> +    return rzg2l_wdt_start(&priv->wdev);
>>>> +}
>>>> +
>>>> +static const struct dev_pm_ops rzg2l_wdt_pm_ops = {
>>>> +    LATE_SYSTEM_SLEEP_PM_OPS(rzg2l_wdt_suspend_late,
>>>> rzg2l_wdt_resume_early)
>>>> +};
>>>> +
>>>>    static struct platform_driver rzg2l_wdt_driver = {
>>>>        .driver = {
>>>>            .name = "rzg2l_wdt",
>>>>            .of_match_table = rzg2l_wdt_ids,
>>>> +        .pm = pm_ptr(&rzg2l_wdt_pm_ops),
>>>
>>> I think this will create a build error if CONFIG_PM=n because
>>> rzg2l_wdt_pm_ops
>>> will be unused but is not marked with __maybe_unused.
>>
>> The necessity of __maybe_unused has been removed along with the
>> introduction of LATE_SYSTEM_SLEEP_PM_OPS() and friends (and
>> *SET_*LATE_SYSTEM_SLEEP_PM_OPS along with the other helpers were marked
>> deprecated for that) and we can use pm_ptr() along with
>> LATE_SYSTEM_SLEEP_PM_OPS() to avoid build errors you mentioned.
>>
>> FYI, I just build the driver with CONFIG_PM=n and all good.
>>
> 
> Ok, but are you sure you did ? You just mentioned earlier that CONFIG_PM
> is set automatically through ARCH_RZG2L.

Yes, I disabled everything that selected the CONFIG_PM, checked that
CONFIG_PM is disabled in my .config, enabled COMPILE_TEST and
RENESAS_RZG2LWDT (sorry, I missed to mention all these).

> 
>>> But then the driver
>>> won't be
>>> operational with CONFIG_PM=n, so I really wonder if it makes sense to
>>> include any
>>> such conditional code instead of making the driver depend on CONFIG_PM.
>>
>> That's true. The driver wouldn't work if the CONFIG_PM=n but then it
>> depends on COMPILE_TEST which is exactly for this (just to compile test it
>> for platforms that don't support it). I see many watchdog drivers depends
>> on COMPILE_TEST.
>>
>> Give this, please let me know would you like me to proceed with it.
>>
> 
> FWIW, COMPILE_TEST dependencies on watchdog drivers fails for most of them.
> Regarding pm_ptr(), it is there for practical reasons and not associated with
> COMPILE_TEST. Again, if the driver depends on CONFIG_PM to work, using
> constructs
> such as pm_ptr() just hides that and creates the impression that it would work
> without it. 
> I do not think that is a good idea. You can use something like
> 
>     depends on (ARCH_RENESAS && PM) || COMPILE_TEST
>

Ok, I don't have anything against. I'm not sure if this will trigger any
circular dependency for Kconfig. I'll check it.

> to make that explicit. Even if not, I _really_ don't see the point in using
> pm_ptr() even without above dependency. What do you see as its benefit ?

I remember it comes on the same package with the LATE_SYSTEM_SLEEP_PM_OPS()
kind of macros. Looking at it's definition I see it useful because it sets
properly the struct platform_driver::driver::pm. AFAIK, at the moment there
are no checks on this member in the driver core code so we should be safe
w/o using it. I checked the compilation w/ COMPILE_TEST=y and CONFIG_PM=n
and compilation is good, too.

Thank you,
Claudiu Beznea

> 
> Thanks,
> Guenter
> 
>> Thank you,
>> Claudiu Beznea
>>
>>>
>>> I really don't think it is desirable to suggest that the driver would work
>>> with
>>> CONFIG_PM=n if that isn't really true.
>>>
>>> Guenter
>>>
>>>>        },
>>>>        .probe = rzg2l_wdt_probe,
>>>>    };
>>>
>