[PATCH v3] pinctrl: qcom: Restore the pin configuration after hibernation

[PATCH v3] pinctrl: qcom: Restore the pin configuration after hibernation

[Birk Skyum]

A hibernation image is restored after a power cycle. The TLMM registers
then hold what the boot firmware and the kernel that loaded the image
made of them. The restored kernel has the configuration it applied in
memory only, and nothing writes it to the hardware again, so every pin
that the loading kernel did not set up the same way stays wrong.

On a Lenovo Yoga Slim 7x (X1E80100) the Bluetooth and Wi-Fi enable lines
are two of them. Their drivers are loaded from the root file system, so
the kernel that loads the image never touches the pins. This is
/sys/kernel/debug/gpio before the hibernation and after the resume:

  gpio116 : out low  func0 2mA no pull
  gpio117 : out high func0 2mA no pull

  gpio116 : in  low  func0 2mA pull down
  gpio117 : out high func0 16mA no pull

The driver that owns gpio116 still believes it drives the line.

Save the control, output and interrupt configuration registers in
freeze_noirq and write them back in restore_noirq, the output level
first, so that a pin drives the right level from the moment it becomes
an output. Changing the interrupt configuration can latch an interrupt,
so the status is cleared where the register changed, as
msm_gpio_irq_set_type() does.

Only the GPIOs the kernel uses are put back: those requested as a GPIO,
muxed to a function or used as an interrupt, as pinctrl-intel does. A
valid GPIO without an owner may be in use by firmware. The SDC pads
after the GPIOs are configured without a mux function, so they never
have an owner, and their control register is always put back.

msm_pinctrl_dev_pm_ops gets the two callbacks and x1e80100 starts using
these ops. With that x1e80100 also gets their suspend and resume
callbacks, which apply the sleep and default state of the controller's
own pins where a board defines them.

Signed-off-by: Birk Skyum <birk.skyum@pm.me>
---
Changes in v3:
- Take the lock with guard() in both callbacks (Bartosz).
- Link to v2: https://lore.kernel.org/r/179206391202.36987.1416057976823693032@pm.me

Changes in v2:
- Put a GPIO back only when the kernel uses it, as a GPIO, through a
  mux function or as an interrupt, as pinctrl-intel does (Bartosz).
- Save and restore the SDC pads after the GPIOs as well (Bartosz).
- Allocate the save area under the same option as the callbacks,
  CONFIG_HIBERNATE_CALLBACKS (Sashiko).
- Clear the interrupt status only where the restore changed the
  interrupt configuration (Sashiko).
- Say in a comment that a both-edge interrupt emulated by flipping the
  polarity is not primed again. No SoC with such pins uses these ops.
- Rebased on pinctrl-qcom/for-next.
- Link to the RFC: https://lore.kernel.org/r/179163141354.18006.80461347454353634@pm.me

Tested with v2 on a Lenovo Yoga Slim 7x with v7.2 plus the patch. v3
was not run on hardware again, because both callbacks compile to the
same arm64 code as in v2. The kernel also carried the PCI change in
[1], without which this machine does not resume at all, the smp2p
change in [2] and a GENI serial change that is not posted. The
Bluetooth driver had [3] applied. On pinctrl-qcom/for-next v3 builds
without warnings with W=1 and sparse, with hibernation on, off and
without sleep support.

- Before the hibernation 45 TLMM pins have an owner, gpio116 and
  gpio117 among them (wcn7850-pmu), and four are interrupts.
- pm_test=core and test_resume pass.
- After a hibernation with a real power-off the TLMM part of
  /sys/kernel/debug/gpio differs only in the level read on two inputs
  (gpio93 and gpio153), as it did with the RFC.
- A suspend (deep) and wake still works.

The Yoga has no SD card slot and its SDC2 pads have no owner, so this
only shows that putting them back does no harm. I did not read them
back.

Not covered here: the LPASS LPI pin controller, whose pins also come
back at their reset values after a hibernation.

[1] https://lore.kernel.org/r/179181074907.22199.10556821368232442334@pm.me
[2] https://lore.kernel.org/r/179158127177.70185.5691100900886332545@pm.me
[3] https://lore.kernel.org/r/179163212446.46972.2306254853726299632@pm.me

 drivers/pinctrl/qcom/pinctrl-msm.c      | 145 +++++++++++++++++++++++-
 drivers/pinctrl/qcom/pinctrl-x1e80100.c |   1 +
 2 files changed, 144 insertions(+), 2 deletions(-)

diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index 27f06bbc113c..8ddb4261bf89 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -39,6 +39,15 @@
 #define MAX_NR_TILES 4
 #define PS_HOLD_OFFSET 0x820
 
+/* Register values of one pin group, kept across a hibernation. */
+struct msm_pin_state {
+	u32 ctl;
+	u32 io;
+	u32 intr_cfg;
+	u32 intr_target;
+	bool saved;
+};
+
 /**
  * struct msm_pinctrl - state for a pinctrl-msm device
  * @dev:            device handle.
@@ -58,6 +67,7 @@
  * @soc:            Reference to soc_data of platform specific data.
  * @regs:           Base addresses for the TLMM tiles.
  * @phys_base:      Physical base address
+ * @saved:          Pin registers kept while a hibernation image is taken.
  */
 struct msm_pinctrl {
 	struct device *dev;
@@ -80,6 +90,8 @@ struct msm_pinctrl {
 	const struct msm_pinctrl_soc_data *soc;
 	void __iomem *regs[MAX_NR_TILES];
 	u32 phys_base[MAX_NR_TILES];
+
+	struct msm_pin_state *saved;
 };
 
 #define MSM_ACCESSOR(name) \
@@ -1583,9 +1595,131 @@ static __maybe_unused int msm_pinctrl_resume(struct device *dev)
 	return pinctrl_force_default(pctrl->pctrl);
 }
 
-SIMPLE_DEV_PM_OPS(msm_pinctrl_dev_pm_ops, msm_pinctrl_suspend,
-		  msm_pinctrl_resume);
+static bool msm_pin_has_own_intr_target(struct msm_pinctrl *pctrl,
+					const struct msm_pingroup *g)
+{
+	return !pctrl->intr_target_use_scm && g->intr_target_reg &&
+	       g->intr_target_reg != g->intr_cfg_reg;
+}
+
+/*
+ * A GPIO is only put back when the kernel uses it, as a GPIO, through a mux
+ * function or as an interrupt. Other valid pins may be in use by firmware.
+ * The pads after the GPIOs (SDC) have no GPIO and are configured without a
+ * mux function, so they never have an owner and are always put back.
+ */
+static bool msm_pin_in_use(struct msm_pinctrl *pctrl, unsigned int i)
+{
+	const struct pin_desc *desc;
+
+	if (i >= pctrl->chip.ngpio)
+		return true;
+
+	if (!gpiochip_line_is_valid(&pctrl->chip, i))
+		return false;
+
+	desc = pin_desc_get(pctrl->pctrl, i);
+
+	return desc && (desc->mux_owner || desc->gpio_owner ||
+			gpiochip_line_is_irq(&pctrl->chip, i));
+}
+
+/* Remember how the pins are set up when a hibernation image is taken. */
+static int msm_pinctrl_freeze_noirq(struct device *dev)
+{
+	struct msm_pinctrl *pctrl = dev_get_drvdata(dev);
+	const struct msm_pingroup *g;
+	struct msm_pin_state *s;
+	unsigned int i;
+
+	guard(raw_spinlock_irqsave)(&pctrl->lock);
+
+	for (i = 0; i < pctrl->soc->ngroups; i++) {
+		g = &pctrl->soc->groups[i];
+		s = &pctrl->saved[i];
+		s->saved = msm_pin_in_use(pctrl, i);
+		if (!s->saved)
+			continue;
+
+		s->ctl = msm_readl_ctl(pctrl, g);
+		if (i >= pctrl->chip.ngpio)
+			continue;
+
+		s->io = msm_readl_io(pctrl, g);
+		if (!g->intr_cfg_reg)
+			continue;
+
+		s->intr_cfg = msm_readl_intr_cfg(pctrl, g);
+		if (msm_pin_has_own_intr_target(pctrl, g))
+			s->intr_target = msm_readl_intr_target(pctrl, g);
+	}
 
+	return 0;
+}
+
+/*
+ * The image is restored after a power cycle, when the pins are back at what
+ * the firmware and the kernel that loaded the image made of them. Nothing else
+ * writes the configuration again, so put back what was there.
+ *
+ * A both-edge interrupt that is emulated by flipping the polarity gets the
+ * polarity it had, which is not primed for a line that changed meanwhile. No
+ * SoC with such pins (one-bit detection) uses these callbacks.
+ */
+static int msm_pinctrl_restore_noirq(struct device *dev)
+{
+	struct msm_pinctrl *pctrl = dev_get_drvdata(dev);
+	const struct msm_pingroup *g;
+	struct msm_pin_state *s;
+	unsigned int i;
+	u32 val;
+
+	guard(raw_spinlock_irqsave)(&pctrl->lock);
+
+	for (i = 0; i < pctrl->soc->ngroups; i++) {
+		g = &pctrl->soc->groups[i];
+		s = &pctrl->saved[i];
+		if (!s->saved)
+			continue;
+
+		if (i >= pctrl->chip.ngpio) {
+			msm_writel_ctl(s->ctl, pctrl, g);
+			continue;
+		}
+
+		/* Set the level before the pin starts driving it. */
+		val = msm_readl_io(pctrl, g);
+		val &= ~BIT(g->out_bit);
+		val |= s->io & BIT(g->out_bit);
+		msm_writel_io(val, pctrl, g);
+
+		msm_writel_ctl(s->ctl, pctrl, g);
+		if (!g->intr_cfg_reg)
+			continue;
+
+		if (msm_pin_has_own_intr_target(pctrl, g))
+			msm_writel_intr_target(s->intr_target, pctrl, g);
+		val = msm_readl_intr_cfg(pctrl, g);
+		msm_writel_intr_cfg(s->intr_cfg, pctrl, g);
+
+		/*
+		 * Changing the configuration can latch an interrupt. One that
+		 * is latched with the configuration unchanged is a real one.
+		 */
+		if (val != s->intr_cfg)
+			msm_ack_intr_status(pctrl, g);
+	}
+
+	return 0;
+}
+
+const struct dev_pm_ops msm_pinctrl_dev_pm_ops = {
+	SYSTEM_SLEEP_PM_OPS(msm_pinctrl_suspend, msm_pinctrl_resume)
+	.freeze_noirq = PTR_IF(IS_ENABLED(CONFIG_HIBERNATE_CALLBACKS),
+			       msm_pinctrl_freeze_noirq),
+	.restore_noirq = PTR_IF(IS_ENABLED(CONFIG_HIBERNATE_CALLBACKS),
+				msm_pinctrl_restore_noirq),
+};
 EXPORT_SYMBOL(msm_pinctrl_dev_pm_ops);
 
 int msm_pinctrl_probe(struct platform_device *pdev,
@@ -1663,6 +1797,13 @@ int msm_pinctrl_probe(struct platform_device *pdev,
 	if (ret)
 		return ret;
 
+	if (IS_ENABLED(CONFIG_HIBERNATE_CALLBACKS)) {
+		pctrl->saved = devm_kcalloc(&pdev->dev, pctrl->soc->ngroups,
+					    sizeof(*pctrl->saved), GFP_KERNEL);
+		if (!pctrl->saved)
+			return -ENOMEM;
+	}
+
 	platform_set_drvdata(pdev, pctrl);
 
 	dev_dbg(&pdev->dev, "Probed Qualcomm pinctrl driver\n");
diff --git a/drivers/pinctrl/qcom/pinctrl-x1e80100.c b/drivers/pinctrl/qcom/pinctrl-x1e80100.c
index e4c0abcd95b9..bfe122fc7e79 100644
--- a/drivers/pinctrl/qcom/pinctrl-x1e80100.c
+++ b/drivers/pinctrl/qcom/pinctrl-x1e80100.c
@@ -1855,6 +1855,7 @@ static struct platform_driver x1e80100_pinctrl_driver = {
 	.driver = {
 		.name = "x1e80100-tlmm",
 		.of_match_table = x1e80100_pinctrl_of_match,
+		.pm = &msm_pinctrl_dev_pm_ops,
 	},
 	.probe = x1e80100_pinctrl_probe,
 };
-- 
2.55.0

Re: [PATCH v3] pinctrl: qcom: Restore the pin configuration after hibernation

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] pinctrl: qcom: msm_pinctrl_restore_noirq() enables IRQ before clearing status
--

commit d8439285386d0a36cbc72eb8f4753391eb0fd921
Author: Birk Skyum <birk.skyum@pm.me>

pinctrl: qcom: Restore the pin configuration after hibernation

This commit adds freeze_noirq and restore_noirq hibernation callbacks to
pinctrl-msm to save and restore the control, output, and interrupt
configuration registers for pins in use by the kernel as well as the SDC
pads. It also wires up msm_pinctrl_dev_pm_ops in the x1e80100 pinctrl
driver.

> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> index 27f06bbc113c9..8ddb4261bf89a 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c

[ ... ]

> +static int msm_pinctrl_restore_noirq(struct device *dev)
> +{
> +	struct msm_pinctrl *pctrl = dev_get_drvdata(dev);
> +	const struct msm_pingroup *g;
> +	struct msm_pin_state *s;
> +	unsigned int i;
> +	u32 val;
> +
> +	guard(raw_spinlock_irqsave)(&pctrl->lock);
> +
> +	for (i = 0; i < pctrl->soc->ngroups; i++) {
> +		g = &pctrl->soc->groups[i];
> +		s = &pctrl->saved[i];
> +		if (!s->saved)
> +			continue;
> +
> +		if (i >= pctrl->chip.ngpio) {
> +			msm_writel_ctl(s->ctl, pctrl, g);
> +			continue;
> +		}
> +
> +		/* Set the level before the pin starts driving it. */
> +		val = msm_readl_io(pctrl, g);
> +		val &= ~BIT(g->out_bit);
> +		val |= s->io & BIT(g->out_bit);
> +		msm_writel_io(val, pctrl, g);
> +
> +		msm_writel_ctl(s->ctl, pctrl, g);
> +		if (!g->intr_cfg_reg)
> +			continue;
> +
> +		if (msm_pin_has_own_intr_target(pctrl, g))
> +			msm_writel_intr_target(s->intr_target, pctrl, g);
> +		val = msm_readl_intr_cfg(pctrl, g);
> +		msm_writel_intr_cfg(s->intr_cfg, pctrl, g);
> +
> +		/*
> +		 * Changing the configuration can latch an interrupt. One that
> +		 * is latched with the configuration unchanged is a real one.
> +		 */
> +		if (val != s->intr_cfg)
> +			msm_ack_intr_status(pctrl, g);

[Severity: Medium]
Can writing s->intr_cfg before calling msm_ack_intr_status() in
msm_pinctrl_restore_noirq() in drivers/pinctrl/qcom/pinctrl-msm.c assert a
spurious interrupt on the TLMM summary IRQ line?

In msm_gpio_irq_set_type(), writing intr_cfg before clearing the latched
status is noted as safe only because the interrupt is masked:

drivers/pinctrl/qcom/pinctrl-msm.c:msm_gpio_irq_set_type() {
	...
	/*
	 * The first time we set RAW_STATUS_EN it could trigger an interrupt.
	 * Clear the interrupt.  This is safe because we have
	 * IRQCHIP_SET_TYPE_MASKED. When changing the interrupt type, we could
	 * also still have a non-matching interrupt latched, so clear whenever
	 * making changes to the interrupt configuration.
	 */
	if (val != oldval)
		msm_ack_intr_status(pctrl, g);
	...
}

During hibernation freeze, device_wakeup_arm_wake_irqs() and
suspend_device_irqs() -> suspend_device_irq() in kernel/irq/pm.c run before
msm_pinctrl_freeze_noirq(). For any GPIO IRQ that is not in
pctrl->skip_wake_irqs and has wakeup enabled (or uses IRQF_NO_SUSPEND),
suspend_device_irq() leaves the line unmasked:

kernel/irq/pm.c:suspend_device_irq() {
	if (!desc->action || irq_desc_is_chained(desc) ||
	    desc->no_suspend_depth)
		return false;

	if (irqd_is_wakeup_set(irqd)) {
		irqd_set(irqd, IRQD_WAKEUP_ARMED);

		if ((chipflags & IRQCHIP_ENABLE_WAKEUP_ON_SUSPEND) &&
		     irqd_irq_disabled(irqd)) {
			__enable_irq(desc);
			irqd_set(irqd, IRQD_IRQ_ENABLED_ON_SUSPEND);
		}
		return true;
	}
	...
}

As a result, msm_pinctrl_freeze_noirq() saves s->intr_cfg with both
BIT(g->intr_raw_status_bit) and BIT(g->intr_enable_bit) set, and bit i
remains set in pctrl->enabled_irqs.

On hibernation restore in create_image() in kernel/power/hibernate.c,
local_irq_enable() and pm_sleep_enable_secondary_cpus() run before
dpm_resume_start(PMSG_RESTORE) -> dpm_resume_noirq():

kernel/power/hibernate.c:create_image() {
	...
 Enable_irqs:
	system_state = SYSTEM_RUNNING;
	local_irq_enable();

 Enable_cpus:
	pm_sleep_enable_secondary_cpus();
	...
	dpm_resume_start(in_suspend ?
		(error ? PMSG_RECOVER : PMSG_THAW) : PMSG_RESTORE);
	...
}

Because the parent TLMM summary IRQ (pctrl->irq) is a chained handler
(msm_gpio_irq_handler), suspend_device_irq() skips it via
irq_desc_is_chained(), leaving it active and unmasked at the GIC while
secondary CPUs are online with IRQs enabled.

When msm_pinctrl_restore_noirq() writes s->intr_cfg with
BIT(g->intr_enable_bit) set before calling msm_ack_intr_status(), any
status bit latched by toggling RAW_STATUS_EN or changing detection/polarity
bits can immediately assert the summary IRQ. Since msm_gpio_irq_handler()
does not take pctrl->lock, another CPU servicing the summary IRQ could
either observe the bogus status bit and dispatch a spurious interrupt, or
observe handled == 0 after msm_ack_intr_status() clears it and call
handle_bad_irq().

Would it be safer to write s->intr_cfg with BIT(g->intr_enable_bit) masked
first, call msm_ack_intr_status() if the configuration changed, and only
then set BIT(g->intr_enable_bit) if it was saved as enabled?

> +	}
> +
> +	return 0;
> +}

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021195250.83540-1-birk.skyum@pm.me?part=1