Re: [PATCH 1/2] pinctrl: renesas: rzg2l: Add suspend/resume support

[Geert Uytterhoeven]

Hi Claudiu,

On Thu, Feb 8, 2024 at 6:59 PM Claudiu <claudiu.beznea@xxxxxxxxx> wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxx>

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[] = { 1800, 2500, 3300 };
> @@ -1880,6 +1938,19 @@ static void rzg2l_gpio_irq_print_chip(struct irq_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 int on)
> +{
> +       struct gpio_chip *gc = irq_data_get_irq_chip_data(data);
> +       struct rzg2l_pinctrl *pctrl = 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 interrupt
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 = {
>         .name = "rzg2l-gpio",
>         .irq_disable = rzg2l_gpio_irq_disable,


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

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68korg

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