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
>
