[PATCH] irqchip/renesas-rzv2h: Enable SKIP_SET_WAKE and MASK_ON_SUSPEND

[PATCH] irqchip/renesas-rzv2h: Enable SKIP_SET_WAKE and MASK_ON_SUSPEND

[Biju Das]

The interrupt controller found on RZ/G3E doesn't provide any facility to
configure the wakeup sources. That's the reason why the driver lacks the
irq_set_wake() callback for the interrupt chip.

But this prevent to properly enter power management states like "suspend to
idle".

Enable the flags IRQCHIP_SKIP_SET_WAKE and IRQCHIP_MASK_ON_SUSPEND so the
interrupt suspend logic can handle the chip correctly.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/irqchip/irq-renesas-rzv2h.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/irqchip/irq-renesas-rzv2h.c b/drivers/irqchip/irq-renesas-rzv2h.c
index 57c5a3c008c9..3daa5de0f05c 100644
--- a/drivers/irqchip/irq-renesas-rzv2h.c
+++ b/drivers/irqchip/irq-renesas-rzv2h.c
@@ -427,7 +427,9 @@ static const struct irq_chip rzv2h_icu_chip = {
 	.irq_retrigger		= irq_chip_retrigger_hierarchy,
 	.irq_set_type		= rzv2h_icu_set_type,
 	.irq_set_affinity	= irq_chip_set_affinity_parent,
-	.flags			= IRQCHIP_SET_TYPE_MASKED,
+	.flags			= IRQCHIP_MASK_ON_SUSPEND |
+				  IRQCHIP_SET_TYPE_MASKED |
+				  IRQCHIP_SKIP_SET_WAKE,
 };
 
 static int rzv2h_icu_alloc(struct irq_domain *domain, unsigned int virq, unsigned int nr_irqs,
-- 
2.43.0

Re: [PATCH] irqchip/renesas-rzv2h: Enable SKIP_SET_WAKE and MASK_ON_SUSPEND

[Geert Uytterhoeven]

Hi Biju,

On Tue, 1 Jul 2025 at 12:59, Biju Das <biju.das.jz@bp.renesas.com> wrote:
> The interrupt controller found on RZ/G3E doesn't provide any facility to
> configure the wakeup sources. That's the reason why the driver lacks the
> irq_set_wake() callback for the interrupt chip.
>
> But this prevent to properly enter power management states like "suspend to
> idle".
>
> Enable the flags IRQCHIP_SKIP_SET_WAKE and IRQCHIP_MASK_ON_SUSPEND so the
> interrupt suspend logic can handle the chip correctly.
>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>

Thanks for your patch!

> --- a/drivers/irqchip/irq-renesas-rzv2h.c
> +++ b/drivers/irqchip/irq-renesas-rzv2h.c
> @@ -427,7 +427,9 @@ static const struct irq_chip rzv2h_icu_chip = {
>         .irq_retrigger          = irq_chip_retrigger_hierarchy,
>         .irq_set_type           = rzv2h_icu_set_type,
>         .irq_set_affinity       = irq_chip_set_affinity_parent,
> -       .flags                  = IRQCHIP_SET_TYPE_MASKED,
> +       .flags                  = IRQCHIP_MASK_ON_SUSPEND |
> +                                 IRQCHIP_SET_TYPE_MASKED |
> +                                 IRQCHIP_SKIP_SET_WAKE,

This driver uses Runtime PM (but does not use a platform driver[1]).
So don't you need to implement .irq_set_wake() instead of setting
IRQCHIP_SKIP_SET_WAKE(), so the ICU is kept running when it is
part of the wake-up path (cfr.[2])?
Does wake-up from an ICU interrupt work?

[1] Perhaps it should be converted to a platform driver?
    That may also be needed before you can provide a
    dev_pm_ops pm_ops.suspend() callback.
[2] commit 734e036a9e1052da ("irqchip/renesas-irqc: Use wakeup_path
    i.s.o. explicit clock handling").

>  };
>
>  static int rzv2h_icu_alloc(struct irq_domain *domain, unsigned int virq, unsigned int nr_irqs,

Gr{oetje,eeting}s,

                        Geert


--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH] irqchip/renesas-rzv2h: Enable SKIP_SET_WAKE and MASK_ON_SUSPEND

[Biju Das]

Hi Geert,

> -----Original Message-----
> From: Geert Uytterhoeven <geert@linux-m68k.org>
> Sent: 01 July 2025 14:05
> Subject: Re: [PATCH] irqchip/renesas-rzv2h: Enable SKIP_SET_WAKE and MASK_ON_SUSPEND
> 
> Hi Biju,
> 
> On Tue, 1 Jul 2025 at 12:59, Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > The interrupt controller found on RZ/G3E doesn't provide any facility
> > to configure the wakeup sources. That's the reason why the driver
> > lacks the
> > irq_set_wake() callback for the interrupt chip.
> >
> > But this prevent to properly enter power management states like
> > "suspend to idle".
> >
> > Enable the flags IRQCHIP_SKIP_SET_WAKE and IRQCHIP_MASK_ON_SUSPEND so
> > the interrupt suspend logic can handle the chip correctly.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> 
> Thanks for your patch!
> 
> > --- a/drivers/irqchip/irq-renesas-rzv2h.c
> > +++ b/drivers/irqchip/irq-renesas-rzv2h.c
> > @@ -427,7 +427,9 @@ static const struct irq_chip rzv2h_icu_chip = {
> >         .irq_retrigger          = irq_chip_retrigger_hierarchy,
> >         .irq_set_type           = rzv2h_icu_set_type,
> >         .irq_set_affinity       = irq_chip_set_affinity_parent,
> > -       .flags                  = IRQCHIP_SET_TYPE_MASKED,
> > +       .flags                  = IRQCHIP_MASK_ON_SUSPEND |
> > +                                 IRQCHIP_SET_TYPE_MASKED |
> > +                                 IRQCHIP_SKIP_SET_WAKE,
> 
> This driver uses Runtime PM (but does not use a platform driver[1]).
> So don't you need to implement .irq_set_wake() instead of setting IRQCHIP_SKIP_SET_WAKE(), so the ICU
> is kept running when it is part of the wake-up path (cfr.[2])?
> Does wake-up from an ICU interrupt work?

For STR state, there is no wakeup source. Wake up is only through the Power button.

For Suspend to idle state, ICU interrupt works. I have tested with USER_SW buttons
by adding wakeup-source in board dts.

Without these flags, currently system is not entering to sleep states.

[   38.539228] gpio-keys keys: failed to configure IRQ 48 as wakeup source: -38
[   38.539402] gpio-keys keys: PM: dpm_run_callback(): platform_pm_suspend returns -38
[   38.540299] gpio-keys keys: PM: failed to suspend: error -38

Cheers,
Biju

Re: [PATCH] irqchip/renesas-rzv2h: Enable SKIP_SET_WAKE and MASK_ON_SUSPEND

[Geert Uytterhoeven]

Hi Biju,

On Tue, 1 Jul 2025 at 15:21, Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > From: Geert Uytterhoeven <geert@linux-m68k.org>
> > On Tue, 1 Jul 2025 at 12:59, Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > The interrupt controller found on RZ/G3E doesn't provide any facility
> > > to configure the wakeup sources. That's the reason why the driver
> > > lacks the
> > > irq_set_wake() callback for the interrupt chip.
> > >
> > > But this prevent to properly enter power management states like
> > > "suspend to idle".
> > >
> > > Enable the flags IRQCHIP_SKIP_SET_WAKE and IRQCHIP_MASK_ON_SUSPEND so
> > > the interrupt suspend logic can handle the chip correctly.
> > >
> > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> >
> > Thanks for your patch!
> >
> > > --- a/drivers/irqchip/irq-renesas-rzv2h.c
> > > +++ b/drivers/irqchip/irq-renesas-rzv2h.c
> > > @@ -427,7 +427,9 @@ static const struct irq_chip rzv2h_icu_chip = {
> > >         .irq_retrigger          = irq_chip_retrigger_hierarchy,
> > >         .irq_set_type           = rzv2h_icu_set_type,
> > >         .irq_set_affinity       = irq_chip_set_affinity_parent,
> > > -       .flags                  = IRQCHIP_SET_TYPE_MASKED,
> > > +       .flags                  = IRQCHIP_MASK_ON_SUSPEND |
> > > +                                 IRQCHIP_SET_TYPE_MASKED |
> > > +                                 IRQCHIP_SKIP_SET_WAKE,
> >
> > This driver uses Runtime PM (but does not use a platform driver[1]).
> > So don't you need to implement .irq_set_wake() instead of setting IRQCHIP_SKIP_SET_WAKE(), so the ICU
> > is kept running when it is part of the wake-up path (cfr.[2])?
> > Does wake-up from an ICU interrupt work?
>
> For STR state, there is no wakeup source. Wake up is only through the Power button.
>
> For Suspend to idle state, ICU interrupt works. I have tested with USER_SW buttons
> by adding wakeup-source in board dts.

OK, in that case probably the ICU does not need its clock running to
handle interrupts.

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

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] irqchip/renesas-rzv2h: Enable SKIP_SET_WAKE and MASK_ON_SUSPEND

[Thomas Gleixner]

On Tue, Jul 01 2025 at 11:59, Biju Das wrote:

Can you please cc: lkml as required by the MAINTAINER file entry on
future submissions?

Why can't people just follow documentation and thereby make the life of
people they want to deal with their submissions easier?

It's truly not rocket science.

Thanks,

        tglx

[tip: irq/drivers] irqchip/renesas-rzv2h: Enable SKIP_SET_WAKE and MASK_ON_SUSPEND

[tip-bot2 for Biju Das]

The following commit has been merged into the irq/drivers branch of tip:

Commit-ID:     de2942828e7670526289f098df7e50b112e8ff1e
Gitweb:        https://git.kernel.org/tip/de2942828e7670526289f098df7e50b112e8ff1e
Author:        Biju Das <biju.das.jz@bp.renesas.com>
AuthorDate:    Tue, 01 Jul 2025 11:59:21 +01:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Tue, 01 Jul 2025 22:13:32 +02:00

irqchip/renesas-rzv2h: Enable SKIP_SET_WAKE and MASK_ON_SUSPEND

The interrupt controller found on RZ/G3E doesn't provide any facility to
configure the wakeup sources. That's the reason why the driver lacks the
irq_set_wake() callback for the interrupt chip.

But this prevent to properly enter power management states like "suspend to
idle".

Enable the flags IRQCHIP_SKIP_SET_WAKE and IRQCHIP_MASK_ON_SUSPEND so the
interrupt suspend logic can handle the chip correctly.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Link: https://lore.kernel.org/all/20250701105923.52151-1-biju.das.jz@bp.renesas.com
---
 drivers/irqchip/irq-renesas-rzv2h.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/irqchip/irq-renesas-rzv2h.c b/drivers/irqchip/irq-renesas-rzv2h.c
index 57c5a3c..3daa5de 100644
--- a/drivers/irqchip/irq-renesas-rzv2h.c
+++ b/drivers/irqchip/irq-renesas-rzv2h.c
@@ -427,7 +427,9 @@ static const struct irq_chip rzv2h_icu_chip = {
 	.irq_retrigger		= irq_chip_retrigger_hierarchy,
 	.irq_set_type		= rzv2h_icu_set_type,
 	.irq_set_affinity	= irq_chip_set_affinity_parent,
-	.flags			= IRQCHIP_SET_TYPE_MASKED,
+	.flags			= IRQCHIP_MASK_ON_SUSPEND |
+				  IRQCHIP_SET_TYPE_MASKED |
+				  IRQCHIP_SKIP_SET_WAKE,
 };
 
 static int rzv2h_icu_alloc(struct irq_domain *domain, unsigned int virq, unsigned int nr_irqs,

RE: [PATCH] irqchip/renesas-rzv2h: Enable SKIP_SET_WAKE and MASK_ON_SUSPEND

[Biju Das]

Hi Thomas,

Thanks for the feedback.

> -----Original Message-----
> From: Thomas Gleixner <tglx@linutronix.de>
> Sent: 01 July 2025 21:10
> Subject: Re: [PATCH] irqchip/renesas-rzv2h: Enable SKIP_SET_WAKE and MASK_ON_SUSPEND
> 
> On Tue, Jul 01 2025 at 11:59, Biju Das wrote:
> 
> Can you please cc: lkml as required by the MAINTAINER file entry on future submissions?
> 
> Why can't people just follow documentation and thereby make the life of people they want to deal with
> their submissions easier?

It is my mistake. I am using a script which specifically filters out some address to
avoid noise. I will fix my script.

Cheers,
Biju