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

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

[Igor Velkov via B4 Relay]

From: Igor Velkov <iav@iav.lv>

Restore from hibernation follows a cold boot. The boot kernel probes
the GPIO banks, sets every interrupt enable bit, leaves the trigger of
each line it did not request at the reset value, and then hands over
to the image. The image restores only the interrupt masks, so a line
it had set up for both edges comes back with the reset trigger.

On ODROID-M1 the headphone detect line (GPIO0_B0) is requested for
both edges and is low with nothing plugged in. After a power loss in
hybrid sleep its reset trigger fires again as soon as the edge handler
acknowledges it, and the system does not come back.

The rest of the bank also comes back as the boot kernel left it. On
the same board, debounce requested on GPIO3_C1 was off after a power
loss in hybrid sleep.

Save the interrupt registers with the masks and write them back before
unmasking. Save the output values, directions and debounce of every
bank, including banks with no interrupt in use, in the noirq phase,
before the drivers that use the lines resume, and write them back on
resume. gpio-dwapb saves the same registers. When the bank kept its
registers, this writes back what is already there.

Fixes: 936ee2675eee ("gpio/rockchip: add driver for rockchip gpio")
Cc: stable@vger.kernel.org
Signed-off-by: Igor Velkov <iav@iav.lv>
---
Tested on v7.3.3 with the Armbian rockchip64 config:
- ODROID-M1 (RK3568, GPIO v2): hybrid sleep with the power removed during
  sleep 4/4, debounce register bits kept; deep sleep 10/10. Without the
  patch 0/5.
- ROCK Pi 4A (RK3399, GPIO v1): hibernation with the power removed 3/3,
  both-edge emulation and debounce register bits kept. Without the patch
  the debounce bit was lost and the line got a spurious interrupt (1 run).
---
Changes in v2:
- Also save and restore the output values, directions and debounce of
  every bank, in the noirq phase (Heiko asked about them).
- Restore the saved polarity of emulated both-edge lines instead of taking
  it from the pin level, which dropped an edge that came during sleep
  (Sashiko).
- Link to v1: https://lore.kernel.org/r/20261107-b4-gpio-rockchip-hib-v1-1-19f4d163a55e@iav.lv
---
 drivers/gpio/gpio-rockchip.c       | 66 ++++++++++++++++++++++++++++++++++++--
 drivers/pinctrl/pinctrl-rockchip.h | 18 +++++++++++
 2 files changed, 81 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/gpio-rockchip.c b/drivers/gpio/gpio-rockchip.c
index 0f46ff095a24..4c68a62d7985 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);
 }
 
 static void rockchip_irq_enable(struct irq_data *d)
@@ -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;
+}
+
+static DEFINE_NOIRQ_DEV_PM_OPS(rockchip_gpio_pm_ops,
+			       rockchip_gpio_suspend_noirq,
+			       rockchip_gpio_resume_noirq);
+
 static const struct of_device_id rockchip_gpio_match[] = {
 	{ .compatible = "rockchip,gpio-bank" },
 	{ .compatible = "rockchip,rk3188-gpio-bank0" },
@@ -817,6 +876,7 @@ static struct platform_driver rockchip_gpio_driver = {
 	.driver		= {
 		.name	= "rockchip-gpio",
 		.of_match_table = rockchip_gpio_match,
+		.pm	= pm_sleep_ptr(&rockchip_gpio_pm_ops),
 	},
 };
 
diff --git a/drivers/pinctrl/pinctrl-rockchip.h b/drivers/pinctrl/pinctrl-rockchip.h
index 950097699643..855a21bd3703 100644
--- a/drivers/pinctrl/pinctrl-rockchip.h
+++ b/drivers/pinctrl/pinctrl-rockchip.h
@@ -303,6 +303,15 @@ struct rockchip_drv {
  * @db_clk: clock of the gpio debounce
  * @irq: interrupt of the gpio bank
  * @saved_masks: Saved content of GPIO_INTEN at suspend time.
+ * @saved_dr: output values, saved at suspend time.
+ * @saved_ddr: directions, saved at suspend time.
+ * @saved_en: interrupt enables, saved at suspend time.
+ * @saved_type: interrupt level/edge type, saved at suspend time.
+ * @saved_polarity: interrupt polarity, saved at suspend time.
+ * @saved_debounce: debounce enables, saved at suspend time.
+ * @saved_bothedge: interrupt both-edge enables (GPIO v2), saved at suspend time.
+ * @saved_dbclk_div_en: debounce clock divider enables (GPIO v2), saved at suspend time.
+ * @saved_dbclk_div_con: debounce clock divider (GPIO v2), saved at suspend time.
  * @pin_base: first pin number
  * @nr_pins: number of pins in this bank
  * @name: name of the bank
@@ -332,6 +341,15 @@ struct rockchip_pin_bank {
 	struct clk			*db_clk;
 	int				irq;
 	u32				saved_masks;
+	u32				saved_dr;
+	u32				saved_ddr;
+	u32				saved_en;
+	u32				saved_type;
+	u32				saved_polarity;
+	u32				saved_debounce;
+	u32				saved_bothedge;
+	u32				saved_dbclk_div_en;
+	u32				saved_dbclk_div_con;
 	u32				pin_base;
 	u8				nr_pins;
 	char				*name;

---
base-commit: 0fa38ba784896f2fc32c9dacd33822f844b21906
change-id: 20261107-b4-gpio-rockchip-hib-705c3fd52b49

Best regards,
-- 
Igor Velkov <iav@iav.lv>

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

[sashiko-bot]

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

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

[Igor Velkov]

Hi Heiko,

Please hold v2: Sashiko is right about the debounce clock. On ODROID-M1,
after hybrid sleep with a power cut, a line with debounce saw 0/8 edges
and a control line without debounce 8/8 (one run). dbclk_gpio3 and its
parent dbclk_gpio read enable count 1 with the hardware gate closed;
opening both by hand gave 8/8.

With WoL on, aclk_gmac0 and pclk_gmac0 come back the same way and the
network stays down after restore. So this looks wider than GPIO, and
fixing it per driver looks like the wrong level.

Should the Rockchip clock driver write its gates back after restore, as
at91, tegra210 and ti do through clk_restore_context()? I find no
restore_context in drivers/clk/rockchip.

Igor