| Message ID | 20240410134044.2138310-10-claudiu.beznea.uj@bp.renesas.com |
|---|---|
| State | New |
| Headers | show |
| Series | watchdog: rzg2l_wdt: Add support for RZ/G3S | expand |
Hi Claudiu, CC pmdomain On Thu, Apr 11, 2024 at 1:11 PM Claudiu <claudiu.beznea@tuxon.dev> wrote: > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > The rzg2l_wdt_restart() is called from atomic context. Calling > pm_runtime_{get_sync, resume_and_get}() or any other runtime PM resume > APIs is not an option as it may lead to issues as described in commit > e4cf89596c1f ("watchdog: rzg2l_wdt: Fix 'BUG: Invalid wait context'") > that removed the pm_runtime_get_sync() and used directly the > clk_prepare_enable() APIs. > > Starting with RZ/G3S the watchdog could be part of its own software > controlled power domain (see the initial implementation in Link section). > In case the watchdog is not used the power domain is off and accessing > watchdog registers leads to aborts. > > To solve this the patch powers on the power domain using > dev_pm_genpd_resume() API before enabling its clock. This is not > sleeping or taking any other locks as the power domain will not be > registered with GENPD_FLAG_IRQ_SAFE flags. > > Link: https://lore.kernel.org/all/20240208124300.2740313-1-claudiu.beznea.uj@bp.renesas.com > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> Thanks for your patch! Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- a/drivers/watchdog/rzg2l_wdt.c > +++ b/drivers/watchdog/rzg2l_wdt.c > @@ -12,6 +12,7 @@ > #include <linux/module.h> > #include <linux/of.h> > #include <linux/platform_device.h> > +#include <linux/pm_domain.h> > #include <linux/pm_runtime.h> > #include <linux/reset.h> > #include <linux/units.h> > @@ -164,6 +165,17 @@ static int rzg2l_wdt_restart(struct watchdog_device *wdev, > struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev); > int ret; > > + /* > + * The device may be part of a power domain that is currently > + * powered off. We need to power it up before accessing registers. > + * We don't undo the dev_pm_genpd_resume() as the device need to > + * be up for the reboot to happen. Also, as we are in atomic context > + * here there is no need to increment PM runtime usage counter > + * (to make sure pm_runtime_active() doesn't return wrong code). > + */ > + if (!pm_runtime_active(wdev->parent)) > + dev_pm_genpd_resume(wdev->parent); > + > clk_prepare_enable(priv->pclk); > clk_prepare_enable(priv->osc_clk); > Gr{oetje,eeting}s, Geert
On Wed, 10 Apr 2024 at 16:19, Claudiu <claudiu.beznea@tuxon.dev> wrote: > > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > The rzg2l_wdt_restart() is called from atomic context. Calling > pm_runtime_{get_sync, resume_and_get}() or any other runtime PM resume > APIs is not an option as it may lead to issues as described in commit > e4cf89596c1f ("watchdog: rzg2l_wdt: Fix 'BUG: Invalid wait context'") > that removed the pm_runtime_get_sync() and used directly the > clk_prepare_enable() APIs. > > Starting with RZ/G3S the watchdog could be part of its own software > controlled power domain (see the initial implementation in Link section). > In case the watchdog is not used the power domain is off and accessing > watchdog registers leads to aborts. > > To solve this the patch powers on the power domain using > dev_pm_genpd_resume() API before enabling its clock. This is not > sleeping or taking any other locks as the power domain will not be > registered with GENPD_FLAG_IRQ_SAFE flags. > > Link: https://lore.kernel.org/all/20240208124300.2740313-1-claudiu.beznea.uj@bp.renesas.com > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > --- > > Changes in v8: > - none, this patch is new > > drivers/watchdog/rzg2l_wdt.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c > index c8c20cfb97a3..98e5e9914a5d 100644 > --- a/drivers/watchdog/rzg2l_wdt.c > +++ b/drivers/watchdog/rzg2l_wdt.c > @@ -12,6 +12,7 @@ > #include <linux/module.h> > #include <linux/of.h> > #include <linux/platform_device.h> > +#include <linux/pm_domain.h> > #include <linux/pm_runtime.h> > #include <linux/reset.h> > #include <linux/units.h> > @@ -164,6 +165,17 @@ static int rzg2l_wdt_restart(struct watchdog_device *wdev, > struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev); > int ret; > > + /* > + * The device may be part of a power domain that is currently > + * powered off. We need to power it up before accessing registers. > + * We don't undo the dev_pm_genpd_resume() as the device need to > + * be up for the reboot to happen. Also, as we are in atomic context > + * here there is no need to increment PM runtime usage counter > + * (to make sure pm_runtime_active() doesn't return wrong code). > + */ > + if (!pm_runtime_active(wdev->parent)) > + dev_pm_genpd_resume(wdev->parent); > + I doubt this is the correct solution, but I may be wrong. Unless this is invoked at the syscore stage? > clk_prepare_enable(priv->pclk); > clk_prepare_enable(priv->osc_clk); > > -- > 2.39.2 > > Can you redirectly me to the complete series, so I can have a better overview of the problem? Kind regards Uffe
On Wed, 10 Apr 2024 at 16:19, Claudiu <claudiu.beznea@tuxon.dev> wrote: > > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > The rzg2l_wdt_restart() is called from atomic context. Calling > pm_runtime_{get_sync, resume_and_get}() or any other runtime PM resume > APIs is not an option as it may lead to issues as described in commit > e4cf89596c1f ("watchdog: rzg2l_wdt: Fix 'BUG: Invalid wait context'") > that removed the pm_runtime_get_sync() and used directly the > clk_prepare_enable() APIs. Calling clk_prepare_enable() doesn't work from an atomic context either as it may sleep on the clk prepare mutex. As I said in the other reply too, it looks like we need a different solution. I am not sure what, but I am happy to help discuss it. > > Starting with RZ/G3S the watchdog could be part of its own software > controlled power domain (see the initial implementation in Link section). > In case the watchdog is not used the power domain is off and accessing > watchdog registers leads to aborts. > > To solve this the patch powers on the power domain using > dev_pm_genpd_resume() API before enabling its clock. This is not > sleeping or taking any other locks as the power domain will not be > registered with GENPD_FLAG_IRQ_SAFE flags. > > Link: https://lore.kernel.org/all/20240208124300.2740313-1-claudiu.beznea.uj@bp.renesas.com > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> Kind regards Uffe > --- > > Changes in v8: > - none, this patch is new > > drivers/watchdog/rzg2l_wdt.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c > index c8c20cfb97a3..98e5e9914a5d 100644 > --- a/drivers/watchdog/rzg2l_wdt.c > +++ b/drivers/watchdog/rzg2l_wdt.c > @@ -12,6 +12,7 @@ > #include <linux/module.h> > #include <linux/of.h> > #include <linux/platform_device.h> > +#include <linux/pm_domain.h> > #include <linux/pm_runtime.h> > #include <linux/reset.h> > #include <linux/units.h> > @@ -164,6 +165,17 @@ static int rzg2l_wdt_restart(struct watchdog_device *wdev, > struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev); > int ret; > > + /* > + * The device may be part of a power domain that is currently > + * powered off. We need to power it up before accessing registers. > + * We don't undo the dev_pm_genpd_resume() as the device need to > + * be up for the reboot to happen. Also, as we are in atomic context > + * here there is no need to increment PM runtime usage counter > + * (to make sure pm_runtime_active() doesn't return wrong code). > + */ > + if (!pm_runtime_active(wdev->parent)) > + dev_pm_genpd_resume(wdev->parent); > + > clk_prepare_enable(priv->pclk); > clk_prepare_enable(priv->osc_clk); > > -- > 2.39.2 > >
Hi, Ulf, On 12.04.2024 14:14, Ulf Hansson wrote: > On Wed, 10 Apr 2024 at 16:19, Claudiu <claudiu.beznea@tuxon.dev> wrote: >> >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >> >> The rzg2l_wdt_restart() is called from atomic context. Calling >> pm_runtime_{get_sync, resume_and_get}() or any other runtime PM resume >> APIs is not an option as it may lead to issues as described in commit >> e4cf89596c1f ("watchdog: rzg2l_wdt: Fix 'BUG: Invalid wait context'") >> that removed the pm_runtime_get_sync() and used directly the >> clk_prepare_enable() APIs. >> >> Starting with RZ/G3S the watchdog could be part of its own software >> controlled power domain (see the initial implementation in Link section). >> In case the watchdog is not used the power domain is off and accessing >> watchdog registers leads to aborts. >> >> To solve this the patch powers on the power domain using >> dev_pm_genpd_resume() API before enabling its clock. This is not >> sleeping or taking any other locks as the power domain will not be >> registered with GENPD_FLAG_IRQ_SAFE flags. >> >> Link: https://lore.kernel.org/all/20240208124300.2740313-1-claudiu.beznea.uj@bp.renesas.com >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >> --- >> >> Changes in v8: >> - none, this patch is new >> >> drivers/watchdog/rzg2l_wdt.c | 12 ++++++++++++ >> 1 file changed, 12 insertions(+) >> >> diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c >> index c8c20cfb97a3..98e5e9914a5d 100644 >> --- a/drivers/watchdog/rzg2l_wdt.c >> +++ b/drivers/watchdog/rzg2l_wdt.c >> @@ -12,6 +12,7 @@ >> #include <linux/module.h> >> #include <linux/of.h> >> #include <linux/platform_device.h> >> +#include <linux/pm_domain.h> >> #include <linux/pm_runtime.h> >> #include <linux/reset.h> >> #include <linux/units.h> >> @@ -164,6 +165,17 @@ static int rzg2l_wdt_restart(struct watchdog_device *wdev, >> struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev); >> int ret; >> >> + /* >> + * The device may be part of a power domain that is currently >> + * powered off. We need to power it up before accessing registers. >> + * We don't undo the dev_pm_genpd_resume() as the device need to >> + * be up for the reboot to happen. Also, as we are in atomic context >> + * here there is no need to increment PM runtime usage counter >> + * (to make sure pm_runtime_active() doesn't return wrong code). >> + */ >> + if (!pm_runtime_active(wdev->parent)) >> + dev_pm_genpd_resume(wdev->parent); >> + > > I doubt this is the correct solution, but I may be wrong. Unless this > is invoked at the syscore stage? On my case I see it invoked from kernel_restart(). As of my code reading, at that point only one CPU is active with IRQs disabled (done in machine_restart()). Below is the stack trace decoded on next-20240410 with this series (https://lore.kernel.org/all/20240410134044.2138310-1-claudiu.beznea.uj@bp.renesas.com/) on top and the one from here (adding power domain support): https://lore.kernel.org/all/20240410122657.2051132-1-claudiu.beznea.uj@bp.renesas.com/ Hardware name: Renesas SMARC EVK version 2 based on r9a08g045s33 (DT) Call trace: dump_backtrace (arch/arm64/kernel/stacktrace.c:319) show_stack (arch/arm64/kernel/stacktrace.c:326) dump_stack_lvl (lib/dump_stack.c:117) dump_stack (lib/dump_stack.c:124) rzg2l_wdt_restart (drivers/watchdog/rzg2l_wdt.c:180) watchdog_restart_notifier (drivers/watchdog/watchdog_core.c:188) atomic_notifier_call_chain (kernel/notifier.c:98 kernel/notifier.c:231) do_kernel_restart (kernel/reboot.c:236) machine_restart (arch/arm64/kernel/process.c:145) kernel_restart (kernel/reboot.c:287) __do_sys_reboot (kernel/reboot.c:755) __arm64_sys_reboot (kernel/reboot.c:715) invoke_syscall (arch/arm64/include/asm/current.h:19 arch/arm64/kernel/syscall.c:53) el0_svc_common.constprop.0 (include/linux/thread_info.h:127 arch/arm64/kernel/syscall.c:141) do_el0_svc (arch/arm64/kernel/syscall.c:153) el0_svc (arch/arm64/include/asm/irqflags.h:56 arch/arm64/include/asm/irqflags.h:77 arch/arm64/kernel/entry-common.c:165 arch/arm64/kernel/entry-common.c:178 arch/arm64/kernel/entry-common.c:713) el0t_64_sync_handler (arch/arm64/kernel/entry-common.c:731) el0t_64_sync (arch/arm64/kernel/entry.S:598) The watchdog restart handler is added in restart_handler_list and this list is invoked though do_kernel_restart(). As of my code investigation the restart_handler_list is invoked only though do_kernel_restart() and only though the stack trace above. Thank you, Claudiu Beznea > >> clk_prepare_enable(priv->pclk); >> clk_prepare_enable(priv->osc_clk); >> >> -- >> 2.39.2 >> >> > > Can you redirectly me to the complete series, so I can have a better > overview of the problem? This is the series that adds power domain support for RZ/G3S SoC: https://lore.kernel.org/all/20240410122657.2051132-1-claudiu.beznea.uj@bp.renesas.com/ This is the series that adds watchdog support for RZ/G3S SoC: https://lore.kernel.org/all/20240410134044.2138310-1-claudiu.beznea.uj@bp.renesas.com/ Thank you for your review, Claudiu Beznea > > Kind regards > Uffe
Hi, Ulf, On 12.04.2024 17:02, claudiu beznea wrote: > Hi, Ulf, > > On 12.04.2024 14:14, Ulf Hansson wrote: >> On Wed, 10 Apr 2024 at 16:19, Claudiu <claudiu.beznea@tuxon.dev> wrote: >>> >>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >>> >>> The rzg2l_wdt_restart() is called from atomic context. Calling >>> pm_runtime_{get_sync, resume_and_get}() or any other runtime PM resume >>> APIs is not an option as it may lead to issues as described in commit >>> e4cf89596c1f ("watchdog: rzg2l_wdt: Fix 'BUG: Invalid wait context'") >>> that removed the pm_runtime_get_sync() and used directly the >>> clk_prepare_enable() APIs. >>> >>> Starting with RZ/G3S the watchdog could be part of its own software >>> controlled power domain (see the initial implementation in Link section). >>> In case the watchdog is not used the power domain is off and accessing >>> watchdog registers leads to aborts. >>> >>> To solve this the patch powers on the power domain using >>> dev_pm_genpd_resume() API before enabling its clock. This is not >>> sleeping or taking any other locks as the power domain will not be >>> registered with GENPD_FLAG_IRQ_SAFE flags. >>> >>> Link: https://lore.kernel.org/all/20240208124300.2740313-1-claudiu.beznea.uj@bp.renesas.com >>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >>> --- >>> >>> Changes in v8: >>> - none, this patch is new >>> >>> drivers/watchdog/rzg2l_wdt.c | 12 ++++++++++++ >>> 1 file changed, 12 insertions(+) >>> >>> diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c >>> index c8c20cfb97a3..98e5e9914a5d 100644 >>> --- a/drivers/watchdog/rzg2l_wdt.c >>> +++ b/drivers/watchdog/rzg2l_wdt.c >>> @@ -12,6 +12,7 @@ >>> #include <linux/module.h> >>> #include <linux/of.h> >>> #include <linux/platform_device.h> >>> +#include <linux/pm_domain.h> >>> #include <linux/pm_runtime.h> >>> #include <linux/reset.h> >>> #include <linux/units.h> >>> @@ -164,6 +165,17 @@ static int rzg2l_wdt_restart(struct watchdog_device *wdev, >>> struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev); >>> int ret; >>> >>> + /* >>> + * The device may be part of a power domain that is currently >>> + * powered off. We need to power it up before accessing registers. >>> + * We don't undo the dev_pm_genpd_resume() as the device need to >>> + * be up for the reboot to happen. Also, as we are in atomic context >>> + * here there is no need to increment PM runtime usage counter >>> + * (to make sure pm_runtime_active() doesn't return wrong code). >>> + */ >>> + if (!pm_runtime_active(wdev->parent)) >>> + dev_pm_genpd_resume(wdev->parent); >>> + >> >> I doubt this is the correct solution, but I may be wrong. Unless this >> is invoked at the syscore stage? > > On my case I see it invoked from kernel_restart(). As of my code reading, With the above explanations, do you consider calling dev_pm_genpd_resume() here is still wrong? Do you have any suggestions I could try? Thank you, Claudiu Beznea > at that point only one CPU is active with IRQs disabled (done in > machine_restart()). Below is the stack trace decoded on next-20240410 with > this series > (https://lore.kernel.org/all/20240410134044.2138310-1-claudiu.beznea.uj@bp.renesas.com/) > on top and the one from here (adding power domain support): > https://lore.kernel.org/all/20240410122657.2051132-1-claudiu.beznea.uj@bp.renesas.com/ > > Hardware name: Renesas SMARC EVK version 2 based on r9a08g045s33 (DT) > Call trace: > dump_backtrace (arch/arm64/kernel/stacktrace.c:319) > show_stack (arch/arm64/kernel/stacktrace.c:326) > dump_stack_lvl (lib/dump_stack.c:117) > dump_stack (lib/dump_stack.c:124) > rzg2l_wdt_restart (drivers/watchdog/rzg2l_wdt.c:180) > watchdog_restart_notifier (drivers/watchdog/watchdog_core.c:188) > atomic_notifier_call_chain (kernel/notifier.c:98 kernel/notifier.c:231) > do_kernel_restart (kernel/reboot.c:236) > machine_restart (arch/arm64/kernel/process.c:145) > kernel_restart (kernel/reboot.c:287) > __do_sys_reboot (kernel/reboot.c:755) > __arm64_sys_reboot (kernel/reboot.c:715) > invoke_syscall (arch/arm64/include/asm/current.h:19 > arch/arm64/kernel/syscall.c:53) > el0_svc_common.constprop.0 (include/linux/thread_info.h:127 > arch/arm64/kernel/syscall.c:141) > do_el0_svc (arch/arm64/kernel/syscall.c:153) > el0_svc (arch/arm64/include/asm/irqflags.h:56 > arch/arm64/include/asm/irqflags.h:77 arch/arm64/kernel/entry-common.c:165 > arch/arm64/kernel/entry-common.c:178 arch/arm64/kernel/entry-common.c:713) > el0t_64_sync_handler (arch/arm64/kernel/entry-common.c:731) > el0t_64_sync (arch/arm64/kernel/entry.S:598) > > The watchdog restart handler is added in restart_handler_list and this list > is invoked though do_kernel_restart(). As of my code investigation the > restart_handler_list is invoked only though do_kernel_restart() and only > though the stack trace above. > > Thank you, > Claudiu Beznea > >> >>> clk_prepare_enable(priv->pclk); >>> clk_prepare_enable(priv->osc_clk); >>> >>> -- >>> 2.39.2 >>> >>> >> >> Can you redirectly me to the complete series, so I can have a better >> overview of the problem? > > This is the series that adds power domain support for RZ/G3S SoC: > https://lore.kernel.org/all/20240410122657.2051132-1-claudiu.beznea.uj@bp.renesas.com/ > > This is the series that adds watchdog support for RZ/G3S SoC: > https://lore.kernel.org/all/20240410134044.2138310-1-claudiu.beznea.uj@bp.renesas.com/ > > Thank you for your review, > Claudiu Beznea > >> >> Kind regards >> Uffe
diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c index c8c20cfb97a3..98e5e9914a5d 100644 --- a/drivers/watchdog/rzg2l_wdt.c +++ b/drivers/watchdog/rzg2l_wdt.c @@ -12,6 +12,7 @@ #include <linux/module.h> #include <linux/of.h> #include <linux/platform_device.h> +#include <linux/pm_domain.h> #include <linux/pm_runtime.h> #include <linux/reset.h> #include <linux/units.h> @@ -164,6 +165,17 @@ static int rzg2l_wdt_restart(struct watchdog_device *wdev, struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev); int ret; + /* + * The device may be part of a power domain that is currently + * powered off. We need to power it up before accessing registers. + * We don't undo the dev_pm_genpd_resume() as the device need to + * be up for the reboot to happen. Also, as we are in atomic context + * here there is no need to increment PM runtime usage counter + * (to make sure pm_runtime_active() doesn't return wrong code). + */ + if (!pm_runtime_active(wdev->parent)) + dev_pm_genpd_resume(wdev->parent); + clk_prepare_enable(priv->pclk); clk_prepare_enable(priv->osc_clk);