Received-SPF: pass (google.com: domain of linux-kernel+bounces-61869-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
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>
In-Reply-To: <20240208135629.2840932-2-claudiu.beznea.uj@bp.renesas.com>
From: Geert Uytterhoeven <geert@linux-m68k.org>
Date: Mon, 12 Feb 2024 16:06:28 +0100
Message-ID: <CAMuHMdUG595o8u1kgqW6DxfvBuzKuOPv7XkJhg_GQmnbRui8Tw@mail.gmail.com>
Subject: Re: [PATCH 1/2] pinctrl: renesas: rzg2l: Add suspend/resume support
To: Claudiu <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 8, 2024 at 6:59=E2=80=AFPM Claudiu <claudiu.beznea@tuxon.dev> w=
rote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>
> pinctrl-rzg2l driver is used on RZ/G3S which support deep sleep states
> where power to most of the SoC components is turned off.
>
> For this add suspend/resume support. This involves saving and restoring
> configured registers along with disabling clock in case there is no pin
> configured as wakeup sources.
>
> To save/restore registers 2 caches were allocated: one for GPIO pins and
> one for dedicated pins.
>
> On suspend path the pin controller registers are saved and if none of the
> pins are configured as wakeup sources the pinctrl clock is disabled.
> Otherwise it remains on.
>
> On resume path the configuration is done as follows:
> 1/ setup PFCs by writing to registers on pin based accesses
> 2/ setup GPIOs by writing to registers on port based accesses and
>    following configuration steps specified in hardware manual
> 3/ setup dedicated pins by writing to registers on port based accesses
> 4/ setup interrupts.
>
> Because interrupt signals are routed to IA55 interrupt controller and
> IA55 interrupt controller resumes before pin controller, patch restores
> also the configured interrupts just after pin settings are restored to
> avoid invalid interrupts while resuming.
>
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Thanks for your patch!

In my review below, I am focussing on the wake-up part, as that is
usually the hardest part to get right.

> --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> @@ -260,6 +315,9 @@ struct rzg2l_pinctrl {
>         struct mutex                    mutex; /* serialize adding groups=
 and functions */
>
>         struct rzg2l_pinctrl_pin_settings *settings;
> +       struct rzg2l_pinctrl_reg_cache  *cache;
> +       struct rzg2l_pinctrl_reg_cache  *dedicated_cache;
> +       atomic_t                        wakeup_source;

I'd call this wakeup_path, as the wake-up source is the ultimate device
that triggers the GPIO.

>  };
>
>  static const u16 available_ps[] =3D { 1800, 2500, 3300 };
> @@ -1880,6 +1938,19 @@ static void rzg2l_gpio_irq_print_chip(struct irq_d=
ata *data, struct seq_file *p)
>         seq_printf(p, dev_name(gc->parent));
>  }
>
> +static int rzg2l_gpio_irq_set_wake(struct irq_data *data, unsigned int o=
n)
> +{
> +       struct gpio_chip *gc =3D irq_data_get_irq_chip_data(data);
> +       struct rzg2l_pinctrl *pctrl =3D container_of(gc, struct rzg2l_pin=
ctrl, 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 interru=
pt
parent, instead of a parent irq_domain with multiple interrupts).

> +       if (on)
> +               atomic_inc(&pctrl->wakeup_source);
> +       else
> +               atomic_dec(&pctrl->wakeup_source);
> +
> +       return 0;
> +}
> +
>  static const struct irq_chip rzg2l_gpio_irqchip =3D {
>         .name =3D "rzg2l-gpio",
>         .irq_disable =3D rzg2l_gpio_irq_disable,


> +static int rzg2l_pinctrl_suspend_noirq(struct device *dev)
> +{
> +       struct rzg2l_pinctrl *pctrl =3D dev_get_drvdata(dev);
> +       const struct rzg2l_hwcfg *hwcfg =3D pctrl->data->hwcfg;
> +       const struct rzg2l_register_offsets *regs =3D &hwcfg->regs;
> +       struct rzg2l_pinctrl_reg_cache *cache =3D pctrl->cache;
> +
> +       rzg2l_pinctrl_pm_setup_regs(pctrl, true);
> +       rzg2l_pinctrl_pm_setup_dedicated_regs(pctrl, true);
> +
> +       for (u8 i =3D 0; i < 2; i++) {
> +               cache->sd_ch[i] =3D readl(pctrl->base + SD_CH(regs->sd_ch=
, i));
> +               cache->eth_poc[i] =3D readl(pctrl->base + ETH_POC(regs->e=
th_poc, i));
> +       }
> +
> +       cache->qspi =3D readl(pctrl->base + QSPI);
> +       cache->eth_mode =3D readl(pctrl->base + ETH_MODE);
> +
> +       if (!atomic_read(&pctrl->wakeup_source))
> +               clk_disable_unprepare(pctrl->clk);

While you handle the module clock yourself, I think there is still merit
in calling device_set_wakeup_path(dev) when the clock is kept enabled.

BTW, is there any need to save the registers when pinctrl is part of
the wake-up path, and its module clock is not disabled?

> +
> +       return 0;
> +}
> +
> +static int rzg2l_pinctrl_resume_noirq(struct device *dev)
> +{
> +       struct rzg2l_pinctrl *pctrl =3D dev_get_drvdata(dev);
> +       const struct rzg2l_hwcfg *hwcfg =3D pctrl->data->hwcfg;
> +       const struct rzg2l_register_offsets *regs =3D &hwcfg->regs;
> +       struct rzg2l_pinctrl_reg_cache *cache =3D pctrl->cache;
> +       int ret;
> +
> +       if (!atomic_read(&pctrl->wakeup_source)) {
> +               ret =3D clk_prepare_enable(pctrl->clk);
> +               if (ret)
> +                       return ret;
> +       }

Is there any need to restore the registers when pinctrl is part of
the wake-up path, and its module clock was not disabled?

> +
> +       writel(cache->qspi, pctrl->base + QSPI);
> +       writel(cache->eth_mode, pctrl->base + ETH_MODE);
> +       for (u8 i =3D 0; i < 2; i++) {
> +               writel(cache->sd_ch[i], pctrl->base + SD_CH(regs->sd_ch, =
i));
> +               writel(cache->eth_poc[i], pctrl->base + ETH_POC(regs->eth=
_poc, i));
> +       }
> +
> +       rzg2l_pinctrl_pm_setup_pfc(pctrl);
> +       rzg2l_pinctrl_pm_setup_regs(pctrl, false);
> +       rzg2l_pinctrl_pm_setup_dedicated_regs(pctrl, false);
> +       rzg2l_gpio_irq_restore(pctrl);
> +
> +       return 0;
> +}

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