diff mbox series

[v2,2/3] clk: renesas: r9a08g045: Mark the watchdog and always-on PM domains as IRQ safe

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

Commit Message

Claudiu Beznea Aug. 28, 2024, 2:06 p.m. UTC 
From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

If the watchdog is part of a dedicated power domain (as it may be on
RZ/G3S) the watchdog PM domain need to be powered on in the watchdog
restart handler. Currently, only the clocks are enabled in the watchdog
restart handler. To be able to also power on the PM domain we need to
call pm_runtime_resume_and_get() on the watchdog restart handler, mark
the watchdog device as IRQ safe and register the watchdog PM domain
with GENPD_FLAG_IRQ_SAFE.

Register watchdog PM domain as IRQ safe. Along with it the always-on
PM domain (parent of the watchdog domain) was marked as IRQ safe.

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

Changes in v2:
- changed patch title; it was "clk: renesas: rzg2l-cpg: Mark
  watchdog and always-on PM domains as IRQ safe"

Changes since RFC:
- none; this patch is new

 drivers/clk/renesas/r9a08g045-cpg.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Geert Uytterhoeven Aug. 29, 2024, 12:45 p.m. UTC | #1 
Hi Claudiu,

On Wed, Aug 28, 2024 at 4:06 PM Claudiu <claudiu.beznea@tuxon.dev> wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>
> If the watchdog is part of a dedicated power domain (as it may be on
> RZ/G3S) the watchdog PM domain need to be powered on in the watchdog
> restart handler. Currently, only the clocks are enabled in the watchdog
> restart handler. To be able to also power on the PM domain we need to
> call pm_runtime_resume_and_get() on the watchdog restart handler, mark
> the watchdog device as IRQ safe and register the watchdog PM domain
> with GENPD_FLAG_IRQ_SAFE.
>
> Register watchdog PM domain as IRQ safe. Along with it the always-on
> PM domain (parent of the watchdog domain) was marked as IRQ safe.
>
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> ---
>
> Changes in v2:
> - changed patch title; it was "clk: renesas: rzg2l-cpg: Mark
>   watchdog and always-on PM domains as IRQ safe"

Thanks for the update!

> --- a/drivers/clk/renesas/r9a08g045-cpg.c
> +++ b/drivers/clk/renesas/r9a08g045-cpg.c
> @@ -259,7 +259,7 @@ static const struct rzg2l_cpg_pm_domain_init_data r9a08g045_pm_domains[] = {
>         /* Keep always-on domain on the first position for proper domains registration. */
>         DEF_PD("always-on",     R9A08G045_PD_ALWAYS_ON,
>                                 DEF_REG_CONF(0, 0),
> -                               GENPD_FLAG_ALWAYS_ON),
> +                               GENPD_FLAG_ALWAYS_ON | GENPD_FLAG_IRQ_SAFE),
>         DEF_PD("gic",           R9A08G045_PD_GIC,
>                                 DEF_REG_CONF(CPG_BUS_ACPU_MSTOP, BIT(3)),
>                                 GENPD_FLAG_ALWAYS_ON),
> @@ -270,7 +270,8 @@ static const struct rzg2l_cpg_pm_domain_init_data r9a08g045_pm_domains[] = {
>                                 DEF_REG_CONF(CPG_BUS_REG1_MSTOP, GENMASK(3, 0)),
>                                 GENPD_FLAG_ALWAYS_ON),
>         DEF_PD("wdt0",          R9A08G045_PD_WDT0,
> -                               DEF_REG_CONF(CPG_BUS_REG0_MSTOP, BIT(0)), 0),
> +                               DEF_REG_CONF(CPG_BUS_REG0_MSTOP, BIT(0)),
> +                               GENPD_FLAG_IRQ_SAFE),
>         DEF_PD("sdhi0",         R9A08G045_PD_SDHI0,
>                                 DEF_REG_CONF(CPG_BUS_PERI_COM_MSTOP, BIT(0)), 0),
>         DEF_PD("sdhi1",         R9A08G045_PD_SDHI1,

Can't you just do this for all domains (e.g. in rzg2l_cpg_pd_setup()),
instead of limiting this to the wdt0 and always-on domains?

Gr{oetje,eeting}s,

                        Geert
Biju Das Aug. 29, 2024, 3:58 p.m. UTC | #2 
Hi Geert,

> -----Original Message-----
> From: Geert Uytterhoeven <geert@linux-m68k.org>
> Sent: Thursday, August 29, 2024 1:45 PM
> Subject: Re: [PATCH v2 2/3] clk: renesas: r9a08g045: Mark the watchdog and always-on PM domains as IRQ
> safe
> 
> Hi Claudiu,
> 
> On Wed, Aug 28, 2024 at 4:06 PM Claudiu <claudiu.beznea@tuxon.dev> wrote:
> > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >
> > If the watchdog is part of a dedicated power domain (as it may be on
> > RZ/G3S) the watchdog PM domain need to be powered on in the watchdog
> > restart handler. Currently, only the clocks are enabled in the
> > watchdog restart handler. To be able to also power on the PM domain we
> > need to call pm_runtime_resume_and_get() on the watchdog restart
> > handler, mark the watchdog device as IRQ safe and register the
> > watchdog PM domain with GENPD_FLAG_IRQ_SAFE.
> >
> > Register watchdog PM domain as IRQ safe. Along with it the always-on
> > PM domain (parent of the watchdog domain) was marked as IRQ safe.
> >
> > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> > ---
> >
> > Changes in v2:
> > - changed patch title; it was "clk: renesas: rzg2l-cpg: Mark
> >   watchdog and always-on PM domains as IRQ safe"
> 
> Thanks for the update!
> 
> > --- a/drivers/clk/renesas/r9a08g045-cpg.c
> > +++ b/drivers/clk/renesas/r9a08g045-cpg.c
> > @@ -259,7 +259,7 @@ static const struct rzg2l_cpg_pm_domain_init_data r9a08g045_pm_domains[] = {
> >         /* Keep always-on domain on the first position for proper domains registration. */
> >         DEF_PD("always-on",     R9A08G045_PD_ALWAYS_ON,
> >                                 DEF_REG_CONF(0, 0),
> > -                               GENPD_FLAG_ALWAYS_ON),
> > +                               GENPD_FLAG_ALWAYS_ON |
> > + GENPD_FLAG_IRQ_SAFE),
> >         DEF_PD("gic",           R9A08G045_PD_GIC,
> >                                 DEF_REG_CONF(CPG_BUS_ACPU_MSTOP, BIT(3)),
> >                                 GENPD_FLAG_ALWAYS_ON), @@ -270,7
> > +270,8 @@ static const struct rzg2l_cpg_pm_domain_init_data r9a08g045_pm_domains[] = {
> >                                 DEF_REG_CONF(CPG_BUS_REG1_MSTOP, GENMASK(3, 0)),
> >                                 GENPD_FLAG_ALWAYS_ON),
> >         DEF_PD("wdt0",          R9A08G045_PD_WDT0,
> > -                               DEF_REG_CONF(CPG_BUS_REG0_MSTOP, BIT(0)), 0),
> > +                               DEF_REG_CONF(CPG_BUS_REG0_MSTOP, BIT(0)),
> > +                               GENPD_FLAG_IRQ_SAFE),
> >         DEF_PD("sdhi0",         R9A08G045_PD_SDHI0,
> >                                 DEF_REG_CONF(CPG_BUS_PERI_COM_MSTOP, BIT(0)), 0),
> >         DEF_PD("sdhi1",         R9A08G045_PD_SDHI1,
> 
> Can't you just do this for all domains (e.g. in rzg2l_cpg_pd_setup()), instead of limiting this to the
> wdt0 and always-on domains?

Not sure, that will make all PM calls for all domains with interrupt disabled??

Cheers,
Biju
Claudiu Beznea Aug. 30, 2024, 7:51 a.m. UTC | #3 
Hi, Geert,

On 29.08.2024 15:45, Geert Uytterhoeven wrote:
> Hi Claudiu,
> 
> On Wed, Aug 28, 2024 at 4:06 PM Claudiu <claudiu.beznea@tuxon.dev> wrote:
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> If the watchdog is part of a dedicated power domain (as it may be on
>> RZ/G3S) the watchdog PM domain need to be powered on in the watchdog
>> restart handler. Currently, only the clocks are enabled in the watchdog
>> restart handler. To be able to also power on the PM domain we need to
>> call pm_runtime_resume_and_get() on the watchdog restart handler, mark
>> the watchdog device as IRQ safe and register the watchdog PM domain
>> with GENPD_FLAG_IRQ_SAFE.
>>
>> Register watchdog PM domain as IRQ safe. Along with it the always-on
>> PM domain (parent of the watchdog domain) was marked as IRQ safe.
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>> ---
>>
>> Changes in v2:
>> - changed patch title; it was "clk: renesas: rzg2l-cpg: Mark
>>   watchdog and always-on PM domains as IRQ safe"
> 
> Thanks for the update!
> 
>> --- a/drivers/clk/renesas/r9a08g045-cpg.c
>> +++ b/drivers/clk/renesas/r9a08g045-cpg.c
>> @@ -259,7 +259,7 @@ static const struct rzg2l_cpg_pm_domain_init_data r9a08g045_pm_domains[] = {
>>         /* Keep always-on domain on the first position for proper domains registration. */
>>         DEF_PD("always-on",     R9A08G045_PD_ALWAYS_ON,
>>                                 DEF_REG_CONF(0, 0),
>> -                               GENPD_FLAG_ALWAYS_ON),
>> +                               GENPD_FLAG_ALWAYS_ON | GENPD_FLAG_IRQ_SAFE),
>>         DEF_PD("gic",           R9A08G045_PD_GIC,
>>                                 DEF_REG_CONF(CPG_BUS_ACPU_MSTOP, BIT(3)),
>>                                 GENPD_FLAG_ALWAYS_ON),
>> @@ -270,7 +270,8 @@ static const struct rzg2l_cpg_pm_domain_init_data r9a08g045_pm_domains[] = {
>>                                 DEF_REG_CONF(CPG_BUS_REG1_MSTOP, GENMASK(3, 0)),
>>                                 GENPD_FLAG_ALWAYS_ON),
>>         DEF_PD("wdt0",          R9A08G045_PD_WDT0,
>> -                               DEF_REG_CONF(CPG_BUS_REG0_MSTOP, BIT(0)), 0),
>> +                               DEF_REG_CONF(CPG_BUS_REG0_MSTOP, BIT(0)),
>> +                               GENPD_FLAG_IRQ_SAFE),
>>         DEF_PD("sdhi0",         R9A08G045_PD_SDHI0,
>>                                 DEF_REG_CONF(CPG_BUS_PERI_COM_MSTOP, BIT(0)), 0),
>>         DEF_PD("sdhi1",         R9A08G045_PD_SDHI1,
> 
> Can't you just do this for all domains (e.g. in rzg2l_cpg_pd_setup()),
> instead of limiting this to the wdt0 and always-on domains?

I thought about it but this, too, but I wasn't sure about the behavior of
the currently unexplored drivers for RZ/G3S. AFAICT from the current code
investigation, if this approach is implemented we need to be sure there is
no sleeping in drivers runtime PM APIs.

Thank you,
Claudiu Beznea

> 
> Gr{oetje,eeting}s,
> 
>                         Geert
>
diff mbox series

Patch

diff --git a/drivers/clk/renesas/r9a08g045-cpg.c b/drivers/clk/renesas/r9a08g045-cpg.c
index ec0672651fe0..8e4f17c21dd7 100644
--- a/drivers/clk/renesas/r9a08g045-cpg.c
+++ b/drivers/clk/renesas/r9a08g045-cpg.c
@@ -259,7 +259,7 @@  static const struct rzg2l_cpg_pm_domain_init_data r9a08g045_pm_domains[] = {
 	/* Keep always-on domain on the first position for proper domains registration. */
 	DEF_PD("always-on",	R9A08G045_PD_ALWAYS_ON,
 				DEF_REG_CONF(0, 0),
-				GENPD_FLAG_ALWAYS_ON),
+				GENPD_FLAG_ALWAYS_ON | GENPD_FLAG_IRQ_SAFE),
 	DEF_PD("gic",		R9A08G045_PD_GIC,
 				DEF_REG_CONF(CPG_BUS_ACPU_MSTOP, BIT(3)),
 				GENPD_FLAG_ALWAYS_ON),
@@ -270,7 +270,8 @@  static const struct rzg2l_cpg_pm_domain_init_data r9a08g045_pm_domains[] = {
 				DEF_REG_CONF(CPG_BUS_REG1_MSTOP, GENMASK(3, 0)),
 				GENPD_FLAG_ALWAYS_ON),
 	DEF_PD("wdt0",		R9A08G045_PD_WDT0,
-				DEF_REG_CONF(CPG_BUS_REG0_MSTOP, BIT(0)), 0),
+				DEF_REG_CONF(CPG_BUS_REG0_MSTOP, BIT(0)),
+				GENPD_FLAG_IRQ_SAFE),
 	DEF_PD("sdhi0",		R9A08G045_PD_SDHI0,
 				DEF_REG_CONF(CPG_BUS_PERI_COM_MSTOP, BIT(0)), 0),
 	DEF_PD("sdhi1",		R9A08G045_PD_SDHI1,