Received-SPF: pass (google.com: domain of linux-kernel+bounces-66537-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240208135629.2840932-1-claudiu.beznea.uj@bp.renesas.com>
 <20240208135629.2840932-2-claudiu.beznea.uj@bp.renesas.com>
 <CAMuHMdUG595o8u1kgqW6DxfvBuzKuOPv7XkJhg_GQmnbRui8Tw@mail.gmail.com>
 <2dab40a5-1e9b-4396-ad97-b2a810ff703d@tuxon.dev> <cdaffd20-1df4-4f73-ba96-9625e0749269@tuxon.dev>
In-Reply-To: <cdaffd20-1df4-4f73-ba96-9625e0749269@tuxon.dev>
From: Geert Uytterhoeven <geert@linux-m68k.org>
Date: Thu, 15 Feb 2024 10:27:04 +0100
Message-ID: <CAMuHMdXc1_P_9o4aMqZU2e2Rsa5B6TfPLv1oXp_sOzSNQPHn2w@mail.gmail.com>
Subject: Re: [PATCH 1/2] pinctrl: renesas: rzg2l: Add suspend/resume support
To: claudiu beznea <claudiu.beznea@tuxon.dev>
Cc: magnus.damm@gmail.com, robh@kernel.org, krzysztof.kozlowski+dt@linaro.org, 
	conor+dt@kernel.org, linus.walleij@linaro.org, 
	linux-renesas-soc@vger.kernel.org, devicetree@vger.kernel.org, 
	linux-kernel@vger.kernel.org, linux-gpio@vger.kernel.org, 
	Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Claudiu,

On Thu, Feb 15, 2024 at 10:15=E2=80=AFAM claudiu beznea
<claudiu.beznea@tuxon.dev> wrote:
> On 12.02.2024 17:35, claudiu beznea wrote:
> >>>  static const u16 available_ps[] =3D { 1800, 2500, 3300 };
> >>> @@ -1880,6 +1938,19 @@ static void rzg2l_gpio_irq_print_chip(struct i=
rq_data *data, struct seq_file *p)
> >>>         seq_printf(p, dev_name(gc->parent));
> >>>  }
> >>>
> >>> +static int rzg2l_gpio_irq_set_wake(struct irq_data *data, unsigned i=
nt on)
> >>> +{
> >>> +       struct gpio_chip *gc =3D irq_data_get_irq_chip_data(data);
> >>> +       struct rzg2l_pinctrl *pctrl =3D container_of(gc, struct rzg2l=
_pinctrl, gpio_chip);
> >>> +
> >> I think you also have to call irq_set_irq_wake(pctrl->hwirq[...]) here=
.
> >> Cfr. drivers/gpio/gpio-rcar.c (which is simpler, as it has a single in=
terrupt
> >> parent, instead of a parent irq_domain with multiple interrupts).
> > I had it in my initial implementation (done long time ago) but I don't
> > remember why I removed it. I'll re-add it anyway.
>
> I did some investigation on this. It seems adding irq_set_irq_wake() is n=
ot
> necessary as the pinctrl has no virq requested on behalf of itself.
>
> With this irqchip hierarchy (pinctrl-rzg2l -> irq-renesas-rzg2l -> gic) i=
f
> an IRQ consumer, e.g., the gpio-keys, request an interrupt then it may ca=
ll
> irq_set_irq_wake(virq) (gpio-keys does that).
>
> irq_set_irq_wake(virq) is forwarded to pinctrl as follows:
>
> irq_set_irq_wake(virq, on) ->
>     set_irq_wake_real(virq, ono) ->
>         rzg2l_gpio_irq_set_wake(irq, on)
>
> As the irq_set_irq_wake() gets a virq as argument and as we have no virq
> requested by pinctrl driver there is no need to call irq_set_irq_wake(), =
as
> of my investigation. Calling it with hwirq will return with -22 and calli=
ng
> it with virq received as argument leads to deadlock (as it's the same vir=
q
> that consumer already is configuring with irq_set_irq_wake()) due the
> following line from irq_set_irq_wake():
>
> struct irq_desc *desc =3D irq_get_desc_buslock(irq, &flags,
> IRQ_GET_DESC_CHECK_GLOBAL);
>
> What we can do is to forward irq_set_wake() to the parent IRQ chip
> (irq-renesas-rzg2l) with irq_chip_set_wake_parent() to let him set its
> wakeup_path, if any. But, at the moment the irq-renesas-rzg2l has
> IRQCHIP_SKIP_SET_WAKE thus the irq_chip_set_wake_parent() does nothing (b=
ut
> it can be updated for that). Now I remember that irq_chip_set_wake_parent=
()
> is what I've called in my initial implementation and removed it due to
> IRQCHIP_SKIP_SET_WAKE.
>
> Please let me know if you are OK to add irq_chip_set_wake_parent() and
> update the irq-renesas-rzg2l driver.

I think calling irq_chip_set_wake_parent() regardless is a good thing
to do.  Whether the irq-renesas-rzg2l needs an update for wake-up
handling, I don't know (and that is orthogonal to the above).

If you haven't already done so, you may want to browse the wake-related
git history of e.g. drivers/gpio/gpio-rcar.c.

Gr{oetje,eeting}s,

                        Geert

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

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