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

[claudiu beznea <claudiu.beznea@xxxxxxxxx>]

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

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
>