[RFC PATCH v3 0/3] Fix poweroff restarting the board on Firefly-RK3399

[RFC PATCH v3 0/3] Fix poweroff restarting the board on Firefly-RK3399

[Yaozhong Li]

Still an RFC: the patched kernel has not been booted on hardware (see
Testing), and the placement and naming questions are still open.

v2: https://lore.kernel.org/all/20260919122930.1418-1-yaozhonguwl@gmail.com/
v1: https://lore.kernel.org/all/20260909092728.1859-1-yaozhonguwl@gmail.com/

Changes since v2
----------------

The main change is in 3/3, and it reverses what v2 did there.

* v2 described both GPIO1_D0 and GPIO1_B5 and dropped the backlight's
  enable-gpios, on the grounds that the backlight could otherwise keep
  GPIO1_B5 asserted. That reasoning was wrong. device_shutdown() runs
  before the power-off prepare handlers, and pwm-backlight's shutdown
  callback drives its enable GPIO low there when the backlight is on; when
  it is off, pwm-backlight has already driven the pin low. So with the
  backlight bound, GPIO1_B5 is low when the PMIC's handler runs. The v2
  failure case - GPIO1_B5 still high at that point - came from a test
  module holding the pin, which the real shutdown sequence does not do.

  v3 describes GPIO1_D0 only and leaves the backlight alone. It was tested
  with pwm-backlight bound and at full brightness, i.e. with the backlight
  driving GPIO1_B5 high while the system ran: 3 of 3 stayed off (see
  Testing).

  v2 also said pwm-backlight does not bind on this board. That was wrong
  too: my own workaround unbound it to take GPIO1_B5. Without that it binds
  normally.

Lee's review of 2/3:

* "What is i here? Can it have a better name?"
  Renamed to "line" and declared in the for statement.

* "Why wrap here? Some of the surrounding lines are clearly longer."
  The devm_gpiod_get_array_optional() call now starts on the assignment
  line; only the last argument is continued.

* "What if this doesn't return a value?" (power-hold-delay-ms)
  The property is optional and the binding documents a default of 0. rk808
  comes from devm_kzalloc(), so when the property is absent the delay stays
  0 and the shutdown request follows the release with no wait. A comment
  now says so at the call. I did not add an explicit assignment, as it
  would only repeat the zeroing. A malformed value is not reported either:
  the read fails and the delay likewise stays 0. If you would rather have
  probe fail in that case, I can do that.

* "Do we do this for all of the other structs? Why not add the include?"
  The forward declaration is gone; rk808.h includes
  <linux/gpio/consumer.h>.

Sashiko's [Low] on 1/3, not addressed in v2:

* It asked for power-hold-gpios to depend on system-power-controller. v2
  declined because the driver also accepts the deprecated
  rockchip,system-power-controller, and a plain dependency on one spelling
  would reject device trees using the other - including the binding's own
  example. v3 adds it as a dependentSchemas entry with an anyOf over both
  spellings. Checked with dtbs_check on rk3399-firefly in three variants:
  the current spelling and the deprecated one both pass, and with neither
  present the anyOf fails naming both properties.

Other:

* The binding description names both spellings, and no longer says that
  every line involved must be listed, which 3/3 no longer follows.
* The 1/3 commit message now says where the bound of two entries comes
  from (see Prior art below).
* Each patch now carries Assisted-by: LLM. v1 and v2 disclosed the
  assistance only in the cover letter.

The problem
-----------

On the Firefly-RK3399, "poweroff" drops the rails and immediately brings
them back up, so the board reboots instead of staying off. U-Boot reports
the result as a power-on reset.

Firefly's BSP drives two SoC pins low during shutdown, before writing the
RK808's shutdown bit: GPIO1_D0 and GPIO1_B5. Mainline does not describe
GPIO1_D0 at all, so nothing ever drives it.

What was established on the board is the sequence, not what happens inside
the PMIC. In the cases tested, the shutdown stuck when GPIO1_D0 went from
high to low during power-off prepare, with GPIO1_B5 low, and the shutdown
bit was written after a settle delay. It did not stick when GPIO1_D0 was
never driven, or when GPIO1_B5 was held high. With the backlight bound,
GPIO1_B5 is low at that point, as described above, so only GPIO1_D0 needs
to be described.

Prior art
---------

The vendor behaviour comes from Firefly's own change to the rk808 driver
in their BSP; it is not in Rockchip's develop-4.4 or develop-4.19 trees.
It reads two single GPIOs from the PMIC node, pmic,hold-gpio and
pmic,stby-gpio, drives both high at probe, and at power-off drives stby
low, then hold low, waits 200 ms and writes the shutdown bit. On this
board stby is GPIO1_D0 and hold is GPIO1_B5.

Its board files use one or both: both on the Firefly-RK3399 and
Firefly-RK3128 FirePrime, hold only on the ROC-RK3399-PC family, stby only
on a few others. That is where minItems 1 and maxItems 2 come from. None
of those boards other than this one uses the new property in mainline.

One of them is relevant to question 1 below. On the ROC-RK3399-PC, the
vendor describes GPIO2_A6 as pmic,hold-gpio, while mainline's
rk3399-roc-pc.dtsi describes the same pin as the enable GPIO of the vcc_sys
fixed regulator. I have not checked whether that board powers off
correctly on mainline.

The vendor device tree marks stby as GPIO_ACTIVE_LOW, but the vendor driver
uses the legacy integer GPIO API, which ignores the flag, and drives the
line high at runtime. That matches the physical level measured here, so
this series describes the line as active high.

Why not gpio-poweroff
---------------------

gpio-poweroff drives the line active, back to inactive, then active again,
waits timeout-ms and then WARN()s on the assumption that it is itself
performing the power off, and it registers at SYS_OFF_MODE_POWER_OFF. Here
the RK808 performs the power off from its own POWER_OFF_PREPARE handler,
and the line only has to be released and left released before that.

rk3188-bqedison2qc.dts does drive a pwr_hold pin from gpio-poweroff, which
works there because pulling that line low is by itself enough to cut the
power. That is not the case here - driving a line low at runtime, with no
PMIC access at all, left this board running for the 10 s it was observed.

Open questions
--------------

1. Does this belong in the PMIC driver at all, rather than a small separate
   driver registering at POWER_OFF_PREPARE with a higher priority? Or, as
   mainline already does for the ROC-RK3399-PC, should such a line be
   described as a regulator enable instead? On this board the line was
   tested going low from power-off prepare, 200 ms before the PMIC write;
   I do not know whether a regulator description would achieve that.
2. Should the property be rockchip,power-hold-gpios? And since the vendor
   has two named roles, hold and standby, would two named properties be
   preferred over an array whose order sets the release order? This board
   only needs one line, but other boards in that family use both.
3. Should the DT also carry a pinctrl group for the pin, as the vendor does?
4. The vendor calls GPIO1_B5 pmic-hold and its backlight node has no enable
   GPIO, so mainline's backlight enable-gpios on that pin may be wrong.
   This series no longer depends on the answer. On this board the pin was
   low when the shutdown bit was written in both configurations tested:
   with pwm-backlight not bound, leaving the pin an input (the v2 runs),
   and with it bound and the backlight on (v3). Without a schematic I have
   left it alone.

Testing
-------

The patched kernel has NOT been booted: CONFIG_MFD_RK8XX is built in on the
test system and a full kernel build does not fit on it.

Build and schema, on the board against a clean mainline tree (28924df2a):
the unpatched tree was built first as a control, then the series applied
and drivers/mfd/rk8xx-core.o built with W=1 with no warnings. drivers/mfd,
drivers/regulator and drivers/rtc were rebuilt for the new include in
rk808.h with no errors or warnings. dt_binding_check is clean, and
dtbs_check reports only the pre-existing usb2phy diagnostics for this
board, which the unpatched tree reports unchanged. The built DTB carries
power-hold-gpios for GPIO1_D0 only, and the backlight's enable-gpios is
still present. checkpatch is clean apart from an unknown-commit-id warning
for the Fixes: tag, caused by the shallow clone; the commit exists
upstream.

Functional, v3 configuration: the board's own device tree with the
properties of 3/3 added - power-hold-gpios for GPIO1_D0 only and a
200 ms delay, with the backlight's enable-gpios unchanged - and an
out-of-tree module using the same gpiod array consumer name, the same
GPIOD_OUT_HIGH, the same per-descriptor gpiod_set_value_cansleep() loop
and the same msleep() as this series, registered at POWER_OFF_PREPARE
with SYS_OFF_PRIO_HIGH + 1.
pwm-backlight was bound at full brightness. Before each run, a script
checked that the backlight owned GPIO1_B5 and drove it high, that the
module owned GPIO1_D0 and drove it high, and that the GPIO1 input register
read both lines high:

  GPIO1_D0 not driven (control):            restarted after 39 s
  GPIO1_D0 only, backlight owning B5:      3 of 3 stayed off

The serial console was not available for these runs. "Stayed off" was
judged by the board not coming back on the network within 5 minutes and
the fan being seen to stop; this caught the control run's restart.
As a result the level of GPIO1_B5 at the prepare handler was not recorded
directly - the module samples it, but only to the console.

The module does not exercise the probe path, so the claim ordering in 2/3
is covered only by review and the build.

Earlier runs from v2, judged on the serial console (nothing for 150 s, no
ICMP reply, no USB gadget), each from a cold boot with both pins at their
reset state (inputs, low), still apply. The fourth line is the case v2
mistook for the backlight's behaviour: there the test module held
GPIO1_B5 high.

  both lines, as in v2:                     3 of 3 stayed off
  GPIO1_D0 only, GPIO1_B5 left low:        3 of 3 stayed off
  GPIO1_B5 only, GPIO1_D0 left low:        0 of 2 stayed off
  GPIO1_D0 released, GPIO1_B5 held high:   0 of 2 stayed off
  nothing driven (mainline today):         restarts after about 2 s

The 200 ms is the value the vendor uses; the threshold was not
characterised. The msleep() was measured at 200 to 201 ms on the console
timestamps.

This series was prepared with the help of LLM-based assistants (Claude
Code, cross-reviewed with OpenAI Codex): the analysis, the code and the
text were drafted with them. The measurements were taken on my board, and
I have reviewed the result.


Yaozhong Li (3):
  dt-bindings: mfd: rk808: add board level power hold GPIOs
  mfd: rk8xx: Release the power hold GPIOs before powering off
  arm64: dts: rockchip: fix power-off on Firefly-RK3399

 .../bindings/mfd/rockchip,rk808.yaml          | 27 ++++++++++
 .../boot/dts/rockchip/rk3399-firefly.dts      |  2 +
 drivers/mfd/rk8xx-core.c                      | 49 ++++++++++++++++++-
 include/linux/mfd/rk808.h                     |  3 ++
 4 files changed, 79 insertions(+), 2 deletions(-)


base-commit: 940de590b839f71d6dc846160534bf202401b8b7
-- 
2.55.0.windows.3

[RFC PATCH v3 1/3] dt-bindings: mfd: rk808: add board level power hold GPIOs

[Yaozhong Li]

Some boards route "power hold" lines from the SoC into the board's power
circuitry. Those lines have to be held asserted while the system is
running and released before the PMIC shutdown request is written,
otherwise the rails can drop and come back up instead of staying off.

Add power-hold-gpios to describe those lines and power-hold-delay-ms for
the settle time.

The array is bounded at two entries. Firefly's vendor kernel handles
exactly two such lines, pmic,hold-gpio and pmic,stby-gpio, and its board
files use one or both of them.

The lines are released from the PMIC's power-off path, so they are only
meaningful when the PMIC is the system power controller. Require that,
accepting both the current and the deprecated spelling of the property.

Assisted-by: LLM
Signed-off-by: Yaozhong Li <yaozhonguwl@gmail.com>
---
 .../bindings/mfd/rockchip,rk808.yaml          | 27 +++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/Documentation/devicetree/bindings/mfd/rockchip,rk808.yaml b/Documentation/devicetree/bindings/mfd/rockchip,rk808.yaml
index 50dfffa..9120222 100644
--- a/Documentation/devicetree/bindings/mfd/rockchip,rk808.yaml
+++ b/Documentation/devicetree/bindings/mfd/rockchip,rk808.yaml
@@ -43,6 +43,24 @@ properties:
 
   system-power-controller: true
 
+  power-hold-gpios:
+    minItems: 1
+    maxItems: 2
+    description:
+      Board level power hold lines driven by the SoC. They have to be held
+      asserted while the system is running and released before the PMIC
+      shutdown request is written, otherwise the rails can drop and come
+      back up instead of staying off. Only meaningful when this PMIC is the
+      system power controller, through either system-power-controller or
+      the deprecated rockchip,system-power-controller, as the lines are
+      released as part of its power-off path.
+
+  power-hold-delay-ms:
+    default: 0
+    description:
+      Time to wait after releasing power-hold-gpios before the shutdown
+      request is written, to let the lines settle.
+
   wakeup-source:
     type: boolean
     description:
@@ -113,6 +131,15 @@ properties:
         unevaluatedProperties: false
     unevaluatedProperties: false
 
+dependencies:
+  power-hold-delay-ms: [ power-hold-gpios ]
+
+dependentSchemas:
+  power-hold-gpios:
+    anyOf:
+      - required: [ system-power-controller ]
+      - required: [ 'rockchip,system-power-controller' ]
+
 required:
   - compatible
   - reg
-- 
2.55.0.windows.3

[RFC PATCH v3 2/3] mfd: rk8xx: Release the power hold GPIOs before powering off

[Yaozhong Li]

rk808_power_off() writes the PMIC's shutdown bit directly. On boards where
lines driven by the SoC gate the PMIC's power-off path, a shutdown
performed this way does not stick: the rails drop and immediately come
back up, so the board restarts instead of staying off. U-Boot reports the
result as a power-on reset.

Take those lines as an optional GPIO array, hold them asserted while the
system runs and release them - waiting power-hold-delay-ms - before
writing the shutdown bit. Device trees without the property retain the
existing behaviour.

The array is claimed before any child device is registered, so that a
-EPROBE_DEFER from the GPIO provider does not tear down the regulators,
RTC and clocks that devm_mfd_add_devices() has already registered.

On a Firefly-RK3399 the shutdown stuck when such a line went from high to
low before the shutdown bit was written, and did not when the line was
never driven.

Assisted-by: LLM
Signed-off-by: Yaozhong Li <yaozhonguwl@gmail.com>
---
 drivers/mfd/rk8xx-core.c  | 49 +++++++++++++++++++++++++++++++++++++--
 include/linux/mfd/rk808.h |  3 +++
 2 files changed, 50 insertions(+), 2 deletions(-)

diff --git a/drivers/mfd/rk8xx-core.c b/drivers/mfd/rk8xx-core.c
index 3dcf6ab..137ab20 100644
--- a/drivers/mfd/rk8xx-core.c
+++ b/drivers/mfd/rk8xx-core.c
@@ -11,6 +11,8 @@
  */
 
 #include <linux/bitfield.h>
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
 #include <linux/interrupt.h>
 #include <linux/mfd/rk808.h>
 #include <linux/mfd/core.h>
@@ -703,6 +705,22 @@ static int rk808_power_off(struct sys_off_data *data)
 	default:
 		return NOTIFY_DONE;
 	}
+
+	/*
+	 * Some boards route "power hold" lines from the SoC into the board's
+	 * power circuitry. They have to be held asserted while the system is
+	 * running and released before the PMIC shutdown request is written,
+	 * otherwise the rails can drop and come back up instead of staying
+	 * off. Releasing them here rather than from a separate handler keeps
+	 * the ordering against the I2C write explicit.
+	 */
+	if (rk808->power_hold_gpios) {
+		for (unsigned int line = 0; line < rk808->power_hold_gpios->ndescs; line++)
+			gpiod_set_value_cansleep(rk808->power_hold_gpios->desc[line], 0);
+
+		msleep(rk808->power_hold_delay_ms);
+	}
+
 	ret = regmap_update_bits(rk808->regmap, reg, bit, bit);
 	if (ret)
 		dev_err(rk808->dev, "Failed to shutdown device!\n");
@@ -766,6 +784,7 @@ int rk8xx_probe(struct device *dev, int variant, unsigned int irq, struct regmap
 	struct rk808 *rk808;
 	const struct rk808_reg_data *pre_init_reg;
 	const struct mfd_cell *cells;
+	bool system_power_controller;
 	int dual_support = 0;
 	int nr_pre_init_regs;
 	u32 rst_fun = 0;
@@ -851,6 +870,33 @@ int rk8xx_probe(struct device *dev, int variant, unsigned int irq, struct regmap
 	if (!irq)
 		return dev_err_probe(dev, -EINVAL, "No interrupt support, no core IRQ\n");
 
+	system_power_controller =
+		device_property_read_bool(dev, "system-power-controller") ||
+		device_property_read_bool(dev, "rockchip,system-power-controller");
+
+	/*
+	 * Claim the optional power hold GPIOs before any child device is
+	 * registered. A -EPROBE_DEFER from the GPIO provider at this point
+	 * costs nothing, whereas deferring after devm_mfd_add_devices()
+	 * would tear the freshly registered children down again on every
+	 * retry.
+	 */
+	if (system_power_controller) {
+		rk808->power_hold_gpios = devm_gpiod_get_array_optional(dev, "power-hold",
+									GPIOD_OUT_HIGH);
+		if (IS_ERR(rk808->power_hold_gpios))
+			return dev_err_probe(dev, PTR_ERR(rk808->power_hold_gpios),
+					     "Failed to get power hold GPIOs\n");
+
+		/*
+		 * The settle time is optional. rk808 is zero initialised and
+		 * the binding documents 0 as the default, so an absent property
+		 * means the shutdown request follows the release with no wait.
+		 */
+		device_property_read_u32(dev, "power-hold-delay-ms",
+					 &rk808->power_hold_delay_ms);
+	}
+
 	ret = devm_regmap_add_irq_chip(dev, rk808->regmap, irq,
 				       IRQF_ONESHOT | dual_support, -1,
 				       rk808->regmap_irq_chip, &rk808->irq_data);
@@ -872,8 +918,7 @@ int rk8xx_probe(struct device *dev, int variant, unsigned int irq, struct regmap
 	if (ret)
 		return dev_err_probe(dev, ret, "failed to add MFD devices\n");
 
-	if (device_property_read_bool(dev, "system-power-controller") ||
-	    device_property_read_bool(dev, "rockchip,system-power-controller")) {
+	if (system_power_controller) {
 		ret = devm_register_sys_off_handler(dev,
 				    SYS_OFF_MODE_POWER_OFF_PREPARE, SYS_OFF_PRIO_HIGH,
 				    &rk808_power_off, rk808);
diff --git a/include/linux/mfd/rk808.h b/include/linux/mfd/rk808.h
index 7ffc904..e56d595 100644
--- a/include/linux/mfd/rk808.h
+++ b/include/linux/mfd/rk808.h
@@ -15,6 +15,7 @@
 #ifndef __LINUX_REGULATOR_RK808_H
 #define __LINUX_REGULATOR_RK808_H
 
+#include <linux/gpio/consumer.h>
 #include <linux/regulator/machine.h>
 #include <linux/regmap.h>
 
@@ -1466,6 +1467,8 @@ struct rk808 {
 	long				variant;
 	const struct regmap_config	*regmap_cfg;
 	const struct regmap_irq_chip	*regmap_irq_chip;
+	struct gpio_descs		*power_hold_gpios;
+	u32				power_hold_delay_ms;
 };
 
 void rk8xx_shutdown(struct device *dev);
-- 
2.55.0.windows.3

[RFC PATCH v3 3/3] arm64: dts: rockchip: fix power-off on Firefly-RK3399

[Yaozhong Li]

poweroff on this board drops the rails and immediately brings them back
up, so it reboots instead of staying off and U-Boot reports a power-on
reset.

Firefly's BSP drives GPIO1_D0 high while the system runs and low during
shutdown, before writing the RK808's shutdown bit. Mainline does not
describe GPIO1_D0 at all, so nothing drives it and the shutdown does not
stick. Describe it in the PMIC node, with the settle time the vendor
uses.

The BSP releases GPIO1_B5 in the same sequence, and in testing the board
came back up when that pin was still high as the shutdown bit was
written. Mainline describes GPIO1_B5 as the backlight enable GPIO.
pwm-backlight drives its enable GPIO low whenever it turns the backlight
off, including from its shutdown callback in device_shutdown(), which
runs before the PMIC's power-off prepare handler. That pin is therefore
left to the backlight.

Fixes: 171582e00db1 ("arm64: dts: rockchip: add support for firefly-rk3399 board")
Assisted-by: LLM
Signed-off-by: Yaozhong Li <yaozhonguwl@gmail.com>
---
 arch/arm64/boot/dts/rockchip/rk3399-firefly.dts | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3399-firefly.dts b/arch/arm64/boot/dts/rockchip/rk3399-firefly.dts
index 0568dfa..210e4cb 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399-firefly.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3399-firefly.dts
@@ -327,6 +327,8 @@ rk808: pmic@1b {
 		pinctrl-names = "default";
 		pinctrl-0 = <&pmic_int_l>;
 		system-power-controller;
+		power-hold-gpios = <&gpio1 RK_PD0 GPIO_ACTIVE_HIGH>;
+		power-hold-delay-ms = <200>;
 		wakeup-source;
 
 		vcc1-supply = <&vcc_sys>;
-- 
2.55.0.windows.3