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

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>
---
 drivers/pinctrl/renesas/pinctrl-rzg2l.c | 397 +++++++++++++++++++++++-
 1 file changed, 393 insertions(+), 4 deletions(-)

On 08.02.2024 15:56, Claudiu wrote:
> 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>
> ---
[ ... ]

>  
> +/**
> + * struct rzg2l_pinctrl_reg_cache - register cache structure (to be used in suspend/resume)
> + * @p: P registers cache
> + * @pm: PM registers cache
> + * @pmc: PMC registers cache
> + * @pfc: PFC registers cache
> + * @iolh: IOLH registers cache
> + * @ien: IEN registers cache
> + * @sd_ch: SD_CH registers cache
> + * @eth_poc: ET_POC registers cache
> + * @eth_mode: ETH_MODE register cache
> + * @qspi: QSPI registers cache
> + */
> +struct rzg2l_pinctrl_reg_cache {
> +	u8	*p;
> +	u16	*pm;
> +	u8	*pmc;
> +	u32	*pfc;
> +	u32	*iolh[2];
> +	u32	*ien[2];


> +	u32	sd_ch[2];
> +	u32	eth_poc[2];
> +	u32	eth_mode;
> +	u32	qspi;

I missed it, u8 should be enough for these.

[ ... ]

Hi Claudiu,

On Thu, Feb 8, 2024 at 6:59 PM Claudiu <claudiu.beznea@tuxon.dev> wrote:
> 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[] = { 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

Hi, Geert,

On 12.02.2024 17:06, Geert Uytterhoeven wrote:
> Hi Claudiu,
> 
> On Thu, Feb 8, 2024 at 6:59 PM Claudiu <claudiu.beznea@tuxon.dev> wrote:
>> 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.

OK!

> 
>>  };
>>
>>  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).

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.

> 
>> +       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.

Ok, I'll explore it.

> 
> 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?

Yes, for scenarios where the pinctrl is part of the wake-up path but the
system is going a deep sleep state where pinctrl registers will be lost anyway.

Same for the resume path.

Thank you for your review,
Claudiu Beznea

> 
>> +
>> +       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
>