| Message ID | 20240122111115.2861835-8-claudiu.beznea.uj@bp.renesas.com |
|---|---|
| State | New |
| Headers | show |
| Series | [01/10] clk: renesas: r9a08g045: Add clock and reset support for watchdog | expand |
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, >>>> }; >>> >
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), }, .probe = rzg2l_wdt_probe, };