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

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(+)

On Mon, Jan 22, 2024 at 09:39:27AM -0800, 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. 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.
> 
> 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

Guenter,

I'm working on a similar patch.

Is your concern limited to the use of the "pm_ptr" macro?  Or is it
wider?

Thanks

Jerry

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.

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

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,
>>   };
>

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.

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

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 ?

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,
>>>    };
>>

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,
>>>>    };
>>>
>