Re: [PATCH v2] gpio: rockchip: restore the bank registers after hibernation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] gpio: rockchip: rockchip_irq_resume() unmasks irqs before
  noirq bank restore
- [Medium] gpio: rockchip: unclocked debounce filter in
  rockchip_gpio_resume_noirq()
--

commit cfd7c691406544bfff804ff2dac05dcea1c1f7d9
Author: Igor Velkov <iav@iav.lv>

gpio: rockchip: restore the bank registers after hibernation

This commit saves and restores the Rockchip GPIO interrupt registers in
irq_suspend/irq_resume and the data, direction, and debounce registers in
noirq suspend/resume callbacks so that bank state is preserved across
hibernation.

> diff --git a/drivers/gpio/gpio-rockchip.c b/drivers/gpio/gpio-rockchip.c
> index 0f46ff095a248..4c68a62d79851 100644
> --- a/drivers/gpio/gpio-rockchip.c
> +++ b/drivers/gpio/gpio-rockchip.c
> @@ -487,17 +487,33 @@ static void rockchip_irq_suspend(struct irq_data *d)
>  {
>  	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
>  	struct rockchip_pin_bank *bank = gc->private;
> +	const struct rockchip_gpio_regs *reg = bank->gpio_regs;
>  
> -	bank->saved_masks = irq_reg_readl(gc, bank->gpio_regs->int_mask);
> -	irq_reg_writel(gc, ~gc->wake_active, bank->gpio_regs->int_mask);
> +	bank->saved_masks = irq_reg_readl(gc, reg->int_mask);
> +	bank->saved_en = rockchip_gpio_readl(bank, reg->int_en);
> +	bank->saved_type = rockchip_gpio_readl(bank, reg->int_type);
> +	bank->saved_polarity = rockchip_gpio_readl(bank, reg->int_polarity);
> +	if (bank->gpio_type == GPIO_TYPE_V2)
> +		bank->saved_bothedge = rockchip_gpio_readl(bank, reg->int_bothedge);
> +	irq_reg_writel(gc, ~gc->wake_active, reg->int_mask);
>  }
>  
>  static void rockchip_irq_resume(struct irq_data *d)
>  {
>  	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
>  	struct rockchip_pin_bank *bank = gc->private;
> +	const struct rockchip_gpio_regs *reg = bank->gpio_regs;
>  
> -	irq_reg_writel(gc, bank->saved_masks, bank->gpio_regs->int_mask);
> +	/*
> +	 * Restore from hibernation follows a cold boot: write the interrupt
> +	 * registers back before we unmask.
> +	 */
> +	rockchip_gpio_writel(bank, bank->saved_en, reg->int_en);
> +	rockchip_gpio_writel(bank, bank->saved_type, reg->int_type);
> +	rockchip_gpio_writel(bank, bank->saved_polarity, reg->int_polarity);
> +	if (bank->gpio_type == GPIO_TYPE_V2)
> +		rockchip_gpio_writel(bank, bank->saved_bothedge, reg->int_bothedge);
> +	irq_reg_writel(gc, bank->saved_masks, reg->int_mask);
>  }

[Severity: Medium]
Does restoring the interrupt registers in rockchip_irq_resume() re-enable
edge detection and unmask wakeup interrupts before
rockchip_gpio_resume_noirq() restores the pin directions, output levels,
and debounce settings?

irq_gc_resume() invokes rockchip_irq_resume() during syscore_resume(),
which runs before local_irq_enable() and before dpm_resume_noirq() calls
rockchip_gpio_resume_noirq().  Because suspend_device_irqs() leaves wakeup
lines, IRQF_NO_SUSPEND lines, and the chained parent bank->irq enabled,
bank->saved_masks unmasks those lines while reg->port_dr, reg->port_ddr,
and reg->debounce still hold the boot kernel's reset state.  Hardware edge
detectors can then latch or fire spurious interrupts (and flip polarity on
GPIO v1 emulated both-edge lines in rockchip_irq_demux()) before the bank
configuration is restored in rockchip_gpio_resume_noirq().

In addition, irq_gc_suspend() and irq_gc_resume() skip chip.irq_suspend
and chip.irq_resume when gc->installed is 0, so a bank with no mapped
interrupts at suspend time will not restore reg->int_en if the bank loses
power.

[ ... ]

> @@ -805,6 +821,49 @@ static void rockchip_gpio_remove(struct platform_device *pdev)
>  	gpiochip_remove(&bank->gpio_chip);
>  }
>  
> +static int rockchip_gpio_suspend_noirq(struct device *dev)
> +{
> +	struct rockchip_pin_bank *bank = dev_get_drvdata(dev);
> +	const struct rockchip_gpio_regs *reg = bank->gpio_regs;
> +	unsigned long flags;
> +
> +	raw_spin_lock_irqsave(&bank->slock, flags);
> +	bank->saved_dr = rockchip_gpio_readl(bank, reg->port_dr);
> +	bank->saved_ddr = rockchip_gpio_readl(bank, reg->port_ddr);
> +	bank->saved_debounce = rockchip_gpio_readl(bank, reg->debounce);
> +	if (bank->gpio_type == GPIO_TYPE_V2) {
> +		bank->saved_dbclk_div_en = rockchip_gpio_readl(bank, reg->dbclk_div_en);
> +		bank->saved_dbclk_div_con = readl(bank->reg_base + reg->dbclk_div_con);
> +	}
> +	raw_spin_unlock_irqrestore(&bank->slock, flags);
> +
> +	return 0;
> +}
> +
> +static int rockchip_gpio_resume_noirq(struct device *dev)
> +{
> +	struct rockchip_pin_bank *bank = dev_get_drvdata(dev);
> +	const struct rockchip_gpio_regs *reg = bank->gpio_regs;
> +	unsigned long flags;
> +
> +	/* Restore from hibernation follows a cold boot: write the bank back. */
> +	raw_spin_lock_irqsave(&bank->slock, flags);
> +	rockchip_gpio_writel(bank, bank->saved_dr, reg->port_dr);
> +	rockchip_gpio_writel(bank, bank->saved_ddr, reg->port_ddr);
> +	if (bank->gpio_type == GPIO_TYPE_V2) {
> +		writel(bank->saved_dbclk_div_con, bank->reg_base + reg->dbclk_div_con);
> +		rockchip_gpio_writel(bank, bank->saved_dbclk_div_en, reg->dbclk_div_en);
> +	}
> +	rockchip_gpio_writel(bank, bank->saved_debounce, reg->debounce);
> +	raw_spin_unlock_irqrestore(&bank->slock, flags);
> +
> +	return 0;
> +}

[Severity: Medium]
On GPIO_TYPE_V2 banks, can rockchip_gpio_resume_noirq() re-enable the
hardware debounce filter while bank->db_clk is still gated off in the CRU?

Unlike bank->clk, bank->db_clk is only prepared and enabled on demand in
rockchip_gpio_set_debounce().  When hibernation restore is triggered from
initramfs after clk_disable_unused() has run in the boot kernel, the boot
kernel gates off dbclk_gpioN for any bank where it did not configure
debounce.  Because bank->db_clk is already marked enabled in the restored
kernel image, nothing ungates it in hardware on resume, so writing back
reg->dbclk_div_en and reg->debounce routes those pins through an unclocked
debounce filter and freezes their sampled input state.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261108-b4-gpio-rockchip-hib-v2-1-27d3d510cb73@iav.lv?part=1