mbox series

[v4,0/4] watchdog: rzg2l_wdt: Enable properly the watchdog clocks and power domain

Message ID 20241015164732.4085249-1-claudiu.beznea.uj@bp.renesas.com
Headers show
Series watchdog: rzg2l_wdt: Enable properly the watchdog clocks and power domain | expand

Message

Claudiu Oct. 15, 2024, 4:47 p.m. UTC
From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Hi,

Watchdog device available on RZ/G3S SoC is part of a software-controlled
power domain. The watchdog driver implements struct
watchdog_ops::restart() handler which is called in atomic context via
this call chain:

kernel_restart() ->
  machine_restart() ->
    do_kernel_restart() ->
      atomic_notifier_call_chain() ->
        watchdog_restart_notifier()
          rzg2l_wdt_restart()

When the rzg2l_wdt_restart() is called it may happen that the watchdog
clocks to be disabled and the associated power domain to be off.
Accessing watchdog registers in this state leads to aborts and system
blocks.

To solve this issue the watchdog power domain was marked as IRQ safe
as well as watchdog device (as proposed by Ulf Hansson). Along with
it the clk_prepare_enable() calls from the watchdog restart() handler
were removed and all is based now on pm_runtime_resume_and_get()
as explained in patch 03/03.

Series contains also power domain driver changes to be able to
register the watchdog PM domain as an IRQ safe one.

Initial RFC series for solving this issue was posted at [1].

It is safe to merge watchdog and PM domain driver changes though
different trees.

Thank you,
Claudiu Beznea

[1] https://lore.kernel.org/all/20240619120920.2703605-1-claudiu.beznea.uj@bp.renesas.com/

Changes in v4:
- in patch 1/1, function rzg2l_cpg_pd_setup():
-- call rzg2l_cpg_power_on() unconditionally of governor
-- drop governor's parameter and decide what governor to use based on
   always_on
- collected tags

Changes in v3:
- added patch "clk: renesas: rzg2l-cpg: Move PM domain power on in
  rzg2l_cpg_pd_setup()"
- addressed review comments
- collected tags
- per-patch changes are listed in individual patches

Changes in v2:
- adjusted patch title for patch 02/03
- adjusted description for patch 03/03 along with comment
  from code

Changes since RFC:
- dropped patches 01/03, 02/03 from RFC
- adjusted power domain driver to be able to register the
  watchdog PM domain as an IRQ safe one
- drop clock prepare approach from watchdog driver presented in RFC
  and rely only on pm_runtime_resume_and_get()
- mark the watchdog device as IRQ safe


Claudiu Beznea (4):
  clk: renesas: rzg2l-cpg: Move PM domain power on in
    rzg2l_cpg_pd_setup()
  clk: renesas: rzg2l-cpg: Use GENPD_FLAG_* flags instead of local ones
  clk: renesas: r9a08g045: Mark the watchdog and always-on PM domains as
    IRQ safe
  watchdog: rzg2l_wdt: Power on the watchdog domain in the restart
    handler

 drivers/clk/renesas/r9a08g045-cpg.c | 52 +++++++++++------------------
 drivers/clk/renesas/rzg2l-cpg.c     | 41 ++++++++++++-----------
 drivers/clk/renesas/rzg2l-cpg.h     | 10 ++----
 drivers/watchdog/rzg2l_wdt.c        | 20 +++++++++--
 4 files changed, 63 insertions(+), 60 deletions(-)

Comments

Ulf Hansson Oct. 18, 2024, 8:49 a.m. UTC | #1
On Tue, 15 Oct 2024 at 18:47, Claudiu <claudiu.beznea@tuxon.dev> wrote:
>
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>
> Hi,
>
> Watchdog device available on RZ/G3S SoC is part of a software-controlled
> power domain. The watchdog driver implements struct
> watchdog_ops::restart() handler which is called in atomic context via
> this call chain:
>
> kernel_restart() ->
>   machine_restart() ->
>     do_kernel_restart() ->
>       atomic_notifier_call_chain() ->
>         watchdog_restart_notifier()
>           rzg2l_wdt_restart()
>
> When the rzg2l_wdt_restart() is called it may happen that the watchdog
> clocks to be disabled and the associated power domain to be off.
> Accessing watchdog registers in this state leads to aborts and system
> blocks.
>
> To solve this issue the watchdog power domain was marked as IRQ safe
> as well as watchdog device (as proposed by Ulf Hansson). Along with
> it the clk_prepare_enable() calls from the watchdog restart() handler
> were removed and all is based now on pm_runtime_resume_and_get()
> as explained in patch 03/03.
>
> Series contains also power domain driver changes to be able to
> register the watchdog PM domain as an IRQ safe one.
>
> Initial RFC series for solving this issue was posted at [1].
>
> It is safe to merge watchdog and PM domain driver changes though
> different trees.
>
> Thank you,
> Claudiu Beznea
>
> [1] https://lore.kernel.org/all/20240619120920.2703605-1-claudiu.beznea.uj@bp.renesas.com/
>
> Changes in v4:
> - in patch 1/1, function rzg2l_cpg_pd_setup():
> -- call rzg2l_cpg_power_on() unconditionally of governor
> -- drop governor's parameter and decide what governor to use based on
>    always_on
> - collected tags
>
> Changes in v3:
> - added patch "clk: renesas: rzg2l-cpg: Move PM domain power on in
>   rzg2l_cpg_pd_setup()"
> - addressed review comments
> - collected tags
> - per-patch changes are listed in individual patches
>
> Changes in v2:
> - adjusted patch title for patch 02/03
> - adjusted description for patch 03/03 along with comment
>   from code
>
> Changes since RFC:
> - dropped patches 01/03, 02/03 from RFC
> - adjusted power domain driver to be able to register the
>   watchdog PM domain as an IRQ safe one
> - drop clock prepare approach from watchdog driver presented in RFC
>   and rely only on pm_runtime_resume_and_get()
> - mark the watchdog device as IRQ safe
>
>
> Claudiu Beznea (4):
>   clk: renesas: rzg2l-cpg: Move PM domain power on in
>     rzg2l_cpg_pd_setup()
>   clk: renesas: rzg2l-cpg: Use GENPD_FLAG_* flags instead of local ones
>   clk: renesas: r9a08g045: Mark the watchdog and always-on PM domains as
>     IRQ safe
>   watchdog: rzg2l_wdt: Power on the watchdog domain in the restart
>     handler
>
>  drivers/clk/renesas/r9a08g045-cpg.c | 52 +++++++++++------------------
>  drivers/clk/renesas/rzg2l-cpg.c     | 41 ++++++++++++-----------
>  drivers/clk/renesas/rzg2l-cpg.h     | 10 ++----
>  drivers/watchdog/rzg2l_wdt.c        | 20 +++++++++--
>  4 files changed, 63 insertions(+), 60 deletions(-)
>
> --
> 2.39.2
>

For the series, feel free to add:

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

Kind regards
Uffe