[00/10] watchdog: rzg2l_wdt: Add support for RZ/G3S

  From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Hi,

Series adds watchdog support for Renesas RZ/G3S (R9A08G045) SoC.

Patches do the following:
- patch 1/10 adds clock and reset support for watchdog
- patches 2-6/10 adds fixes and cleanup for the watchdog driver
- patch 7/10 adds suspend to RAM to the watchdog driver (to be used by
  RZ/G3S)
- patch 8/10 documents the RZ/G3S support
- patches 9-10/10 add device tree support

It is expected that the clock and device tree support will go through
Geert's tree while the rest of the patches through the watchdog tree.

Thank you,
Claudiu Beznea

Claudiu Beznea (10):
  clk: renesas: r9a08g045: Add clock and reset support for watchdog
  watchdog: rzg2l_wdt: Use pm_runtime_resume_and_get()
  watchdog: rzg2l_wdt: Check return status of pm_runtime_put()
  watchdog: rzg2l_wdt: Remove reset de-assert on probe/stop
  watchdog: rzg2l_wdt: Remove comparison with zero
  watchdog: rzg2l_wdt: Rely on the reset driver for doing proper reset
  watchdog: rzg2l_wdt: Add suspend/resume support
  dt-bindings: watchdog: renesas,wdt: Document RZ/G3S support
  arm64: dts: renesas: r9a08g045: Add watchdog node
  arm64: dts: renesas: rzg3s-smarc-som: Enable the watchdog interface

 .../bindings/watchdog/renesas,wdt.yaml        |   1 +
 arch/arm64/boot/dts/renesas/r9a08g045.dtsi    |  14 +++
 .../boot/dts/renesas/rzg3s-smarc-som.dtsi     |   5 +
 drivers/clk/renesas/r9a08g045-cpg.c           |   3 +
 drivers/watchdog/rzg2l_wdt.c                  | 100 ++++++++++--------
 5 files changed, 76 insertions(+), 47 deletions(-)
  

On Mon, Jan 22, 2024 at 01:11:13PM +0200, Claudiu wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> Document the support for the watchdog IP available on RZ/G3S SoC. The
> watchdog IP available on RZ/G3S SoC is identical to the one found on
> RZ/G2UL SoC.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Acked-by: Conor Dooley <conor.dooley@microchip.com>

Cheers,
Conor.

> ---
>  Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml b/Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml
> index 951a7d54135a..220763838df0 100644
> --- a/Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml
> +++ b/Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml
> @@ -29,6 +29,7 @@ properties:
>                - renesas,r9a07g043-wdt    # RZ/G2UL and RZ/Five
>                - renesas,r9a07g044-wdt    # RZ/G2{L,LC}
>                - renesas,r9a07g054-wdt    # RZ/V2L
> +              - renesas,r9a08g045-wdt    # RZ/G3S
>            - const: renesas,rzg2l-wdt
>  
>        - items:
> -- 
> 2.39.2
>
  

On 1/22/24 03:11, Claudiu wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> pm_runtime_put() may return an error code. Check its return status.
> 
> Fixes: 2cbc5cd0b55f ("watchdog: Add Watchdog Timer driver for RZ/G2L")
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> ---
>   drivers/watchdog/rzg2l_wdt.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c
> index 4ab9e7c5e771..0554965027cd 100644
> --- a/drivers/watchdog/rzg2l_wdt.c
> +++ b/drivers/watchdog/rzg2l_wdt.c
> @@ -144,9 +144,13 @@ static int rzg2l_wdt_start(struct watchdog_device *wdev)
>   static int rzg2l_wdt_stop(struct watchdog_device *wdev)
>   {
>   	struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
> +	int ret;
>   
>   	rzg2l_wdt_reset(priv);
> -	pm_runtime_put(wdev->parent);
> +
> +	ret = pm_runtime_put(wdev->parent);
> +	if (ret < 0)
> +		return ret;
>   
>   	return 0;
>   }

A simple
	return pm_runtime_put();
might do.

However, one question: Given that pm_runtime_put() returns -ENOSYS if
CONFIG_PM is disabled, that means the driver will depend on CONFIG_PM=y.
Assuming this is intentional, would it make sense to explicitly declare
that dependency in Kconfig ? It doesn't seem to make any sense to build
the driver if it won't work anyway.

Thanks,
Guenter
  

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

>   	},
>   	.probe = rzg2l_wdt_probe,
>   };
  

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 1/22/24 13:05, Jerry Hoemann wrote:
> 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?
> 

patch 3/10 adds an error check of the return value from pm_runtime_put().
pm_runtime_put() calls __pm_runtime_idle() which returns -ENOSYS if
CONFIG_PM=n. That means checking the return value of pm_runtime_put()
is equivalent to making CONFIG_PM mandatory. My argument is that this
should be expressed in Kconfig to avoid the impression that the driver
works with CONFIG_PM=n. If the driver depends on CONFIG_PM, pm_ptr()
is unnecessary. If it doesn't, the variable referenced by it needs
to be defined as __maybe_unused, but then the driver should actually
work with CONFIG_PM=n.

Yes, I have noticed the recent trend of adding error checks to
pm_runtime_put(), but I only now realized that it has consequences.

Guenter
  

On 22.01.2024 19:31, Guenter Roeck wrote:
> On 1/22/24 03:11, Claudiu wrote:
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> pm_runtime_put() may return an error code. Check its return status.
>>
>> Fixes: 2cbc5cd0b55f ("watchdog: Add Watchdog Timer driver for RZ/G2L")
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>> ---
>>   drivers/watchdog/rzg2l_wdt.c | 6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c
>> index 4ab9e7c5e771..0554965027cd 100644
>> --- a/drivers/watchdog/rzg2l_wdt.c
>> +++ b/drivers/watchdog/rzg2l_wdt.c
>> @@ -144,9 +144,13 @@ static int rzg2l_wdt_start(struct watchdog_device
>> *wdev)
>>   static int rzg2l_wdt_stop(struct watchdog_device *wdev)
>>   {
>>       struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
>> +    int ret;
>>         rzg2l_wdt_reset(priv);
>> -    pm_runtime_put(wdev->parent);
>> +
>> +    ret = pm_runtime_put(wdev->parent);
>> +    if (ret < 0)
>> +        return ret;
>>         return 0;
>>   }
> 
> A simple
>     return pm_runtime_put();
> might do.

pm_runtime_put() may return 1 if the device is already suspended though
this call trace:

pm_runtime_put() ->
   __pm_runtime_idle() ->
       rpm_idle() ->
           rpm_suspend() ->
               rpm_check_suspend_allowed() [1]

That return value is not considered error thus I wanted to consider it
here, too.

[1]
https://elixir.bootlin.com/linux/latest/source/drivers/base/power/runtime.c#L278

> 
> However, one question: Given that pm_runtime_put() returns -ENOSYS if
> CONFIG_PM is disabled, that means the driver will depend on CONFIG_PM=y.

Indeed, the driver depends on CONFIG_PM=y for proper working. It is for
devices selecting ARCH_RZG2L and RZ/V2M (ARM64 based uarch) which select
CONFIG_PM=y:
https://elixir.bootlin.com/linux/latest/source/drivers/soc/renesas/Kconfig#L45

The driver is written with CONFIG_PM=y dependency in mind (e.g. the clocks
are enabled though runtime PM APIs).

> Assuming this is intentional, would it make sense to explicitly declare
> that dependency in Kconfig ? It doesn't seem to make any sense to build
> the driver if it won't work anyway.

The dependency exists there for ARCH_RZG2L and RZ/V2M devices but not
directly and it is not strict (in the sense that we allow to build the
driver w/o CONFIG_PM (I think this is good to check build on different
configurations, the COMPILE_TEST is there anyway in [1]) ). E.g.:

RENESAS_RZG2LWDT depends on ARCH_RENESAS [1]
ARCH_RENESAS is the ARMv8 uarch flag [2]
SOC_RENESAS is set if ARCH_RENESAS [3]
ARCH_RZG2L is visible only if SOC_RENESAS [4]
ARCH_RZG2L selects PM [5]
RZ/V2M selects PM [6]

Please let me know what do you think about it?

Thank you,
Claudiu Beznea


[1]
https://elixir.bootlin.com/linux/latest/source/drivers/watchdog/Kconfig#L913
[2]
https://elixir.bootlin.com/linux/latest/source/arch/arm64/Kconfig.platforms#L273
[3]
https://elixir.bootlin.com/linux/latest/source/drivers/soc/renesas/Kconfig#L2
[4]
https://elixir.bootlin.com/linux/latest/source/drivers/soc/renesas/Kconfig#L9
[5]
https://elixir.bootlin.com/linux/latest/source/drivers/soc/renesas/Kconfig#L45
[6]
https://elixir.bootlin.com/linux/latest/source/drivers/soc/renesas/Kconfig#L328


> 
> Thanks,
> Guenter
>
  

On 1/22/24 23:02, claudiu beznea wrote:
> 
> 
> On 22.01.2024 19:31, Guenter Roeck wrote:
>> On 1/22/24 03:11, Claudiu wrote:
>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>
>>> pm_runtime_put() may return an error code. Check its return status.
>>>
>>> Fixes: 2cbc5cd0b55f ("watchdog: Add Watchdog Timer driver for RZ/G2L")
>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>> ---
>>>    drivers/watchdog/rzg2l_wdt.c | 6 +++++-
>>>    1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c
>>> index 4ab9e7c5e771..0554965027cd 100644
>>> --- a/drivers/watchdog/rzg2l_wdt.c
>>> +++ b/drivers/watchdog/rzg2l_wdt.c
>>> @@ -144,9 +144,13 @@ static int rzg2l_wdt_start(struct watchdog_device
>>> *wdev)
>>>    static int rzg2l_wdt_stop(struct watchdog_device *wdev)
>>>    {
>>>        struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
>>> +    int ret;
>>>          rzg2l_wdt_reset(priv);
>>> -    pm_runtime_put(wdev->parent);
>>> +
>>> +    ret = pm_runtime_put(wdev->parent);
>>> +    if (ret < 0)
>>> +        return ret;
>>>          return 0;
>>>    }
>>
>> A simple
>>      return pm_runtime_put();
>> might do.
> 
> pm_runtime_put() may return 1 if the device is already suspended though
> this call trace:
> 
> pm_runtime_put() ->
>     __pm_runtime_idle() ->
>         rpm_idle() ->
>             rpm_suspend() ->
>                 rpm_check_suspend_allowed() [1]
> 
> That return value is not considered error thus I wanted to consider it
> here, too.
> 
Good point.

> [1]
> https://elixir.bootlin.com/linux/latest/source/drivers/base/power/runtime.c#L278
> 
>>
>> However, one question: Given that pm_runtime_put() returns -ENOSYS if
>> CONFIG_PM is disabled, that means the driver will depend on CONFIG_PM=y.
> 
> Indeed, the driver depends on CONFIG_PM=y for proper working. It is for
> devices selecting ARCH_RZG2L and RZ/V2M (ARM64 based uarch) which select
> CONFIG_PM=y:
> https://elixir.bootlin.com/linux/latest/source/drivers/soc/renesas/Kconfig#L45
> 
> The driver is written with CONFIG_PM=y dependency in mind (e.g. the clocks
> are enabled though runtime PM APIs).
> 
>> Assuming this is intentional, would it make sense to explicitly declare
>> that dependency in Kconfig ? It doesn't seem to make any sense to build
>> the driver if it won't work anyway.
> 
> The dependency exists there for ARCH_RZG2L and RZ/V2M devices but not
> directly and it is not strict (in the sense that we allow to build the
> driver w/o CONFIG_PM (I think this is good to check build on different
> configurations, the COMPILE_TEST is there anyway in [1]) ). E.g.:
> 
> RENESAS_RZG2LWDT depends on ARCH_RENESAS [1]
> ARCH_RENESAS is the ARMv8 uarch flag [2]
> SOC_RENESAS is set if ARCH_RENESAS [3]
> ARCH_RZG2L is visible only if SOC_RENESAS [4]
> ARCH_RZG2L selects PM [5]
> RZ/V2M selects PM [6]
> 
> Please let me know what do you think about it?
> 
If the driver indeed depends on CONFIG_PM, that should be made explicit.

Guenter

> Thank you,
> Claudiu Beznea
> 
> 
> [1]
> https://elixir.bootlin.com/linux/latest/source/drivers/watchdog/Kconfig#L913
> [2]
> https://elixir.bootlin.com/linux/latest/source/arch/arm64/Kconfig.platforms#L273
> [3]
> https://elixir.bootlin.com/linux/latest/source/drivers/soc/renesas/Kconfig#L2
> [4]
> https://elixir.bootlin.com/linux/latest/source/drivers/soc/renesas/Kconfig#L9
> [5]
> https://elixir.bootlin.com/linux/latest/source/drivers/soc/renesas/Kconfig#L45
> [6]
> https://elixir.bootlin.com/linux/latest/source/drivers/soc/renesas/Kconfig#L328
> 
> 
>>
>> Thanks,
>> Guenter
>>
>
  

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

Hi Claudiu,

Thanks for your patch!

On Mon, Jan 22, 2024 at 12:11 PM Claudiu <claudiu.beznea@tuxon.dev> wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>
> Document the support for the watchdog IP available on RZ/G3S SoC. The
> watchdog IP available on RZ/G3S SoC is identical to the one found on
> RZ/G2UL SoC.

Or RZ/G2L, which is considered the baseline here.

>
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert