From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id CE486C9830E for ; Wed, 23 Sep 2026 16:08:29 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:In-Reply-To:Content-Type: MIME-Version:References:Message-ID:Subject:Cc:To:From:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=3e9il85E3ABPy4I7irvXMCFKFPKx9Fa96n5jIdOodN0=; b=viwbnW95dg9Kt2MaGMRxHAA7uB 5yWa4aWS3gOX4dEfDuWVhAWiMXPUqx0LIZ7DoBVkiN7tOZdVzIIC8e0BXwHbJ5tCMjLiQ6ZtrnnVB /1CKFjzO7+vVTIM//QVdVNFG6zOS9TJ18DmkGv69QL52rqzhGAqCvlOmuaixf9WW0k1O/uoscJwf0 SC56ThauMlEuKfuWq2o18aRBPD0J+UHw7fPnbRlVyLkxEMM037KBQSlV3x9U6BNG9vnM51/LHf43P KSlqZwR0PZhGUEc4FKG91HNWkohNW3thqhxnRhkb/VdFEChMITtL+vUirbTjKmxAll+Ern4Pudd75 uxG9dTmg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.99.1 #2 (Red Hat Linux)) id 1x9PWA-00000008q1B-2zqN; Wed, 23 Sep 2026 16:08:22 +0000 Received: from tor.source.kernel.org ([172.105.4.254]) by bombadil.infradead.org with esmtps (Exim 4.99.1 #2 (Red Hat Linux)) id 1x9PW8-00000008q0z-38lE; Wed, 23 Sep 2026 16:08:21 +0000 Received: from smtp.kernel.org (quasi.space.kernel.org [100.103.45.18]) by tor.source.kernel.org (Postfix) with ESMTP id E948E600AA; Wed, 23 Sep 2026 16:08:19 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 659261F000FF; Wed, 23 Sep 2026 16:08:17 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kernel.org; s=k20260515; t=1790179699; bh=3e9il85E3ABPy4I7irvXMCFKFPKx9Fa96n5jIdOodN0=; h=Date:From:To:Cc:Subject:References:In-Reply-To; b=H6DSnLqz2hqqkW7Vg5Bp2GkUPg0lC2kr8YI7cRIGOVEJ0F3gM9Nq18zRXPT9BnRQb UVL7u7NBIKYrt0EF1JgCOqhI4h15ibXjOU2rsvb4t6Yx6gKOi+E7/DoXKfJwe1JBlg e5sQzZZEhr+qyOPy5m/pGmiXqKvkKsNb0S9F+HWLEKO0E27Szuv9b0IIODuyiT/SYv jZr7r1ZBDQfkwxB4rhgrOxG56tQirvgJW34BSIaMe2XUekF0WZW18fZNRk0uFlz6SA DJMvNRxxkw0nNJPn8MdiRdj4gG/om+SAEU1Fq4haHo+YYwbwa6LMDxqLdMrnhu8Ddy xTgGsBLDKopJA== Date: Wed, 23 Sep 2026 17:08:14 +0100 From: Lee Jones To: Yaozhong Li Cc: Rob Herring , Krzysztof Kozlowski , Conor Dooley , Heiko Stuebner , Chris Zhong , Zhang Qing , mfd@lists.linux.dev, devicetree@vger.kernel.org, linux-rockchip@lists.infradead.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [RFC PATCH v2 2/3] mfd: rk8xx: Release the power hold GPIOs before powering off Message-ID: <20260923160814.GL3864833@google.com> References: <20260909092728.1859-1-yaozhonguwl@gmail.com> <20260919122930.1418-3-yaozhonguwl@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20260919122930.1418-3-yaozhonguwl@gmail.com> X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Sat, 19 Sep 2026, Yaozhong Li wrote: > 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. > > Releasing every described line matters: on a Firefly-RK3399 the board > stays off when both lines described for it are released, whereas releasing > GPIO1_D0 while GPIO1_B5 is left asserted makes it come back up. > > Signed-off-by: Yaozhong Li > --- > drivers/mfd/rk8xx-core.c | 47 +++++++++++++++++++++++++++++++++++++-- > include/linux/mfd/rk808.h | 4 ++++ > 2 files changed, 49 insertions(+), 2 deletions(-) > > diff --git a/drivers/mfd/rk8xx-core.c b/drivers/mfd/rk8xx-core.c > index 3dcf6ab..0898db0 100644 > --- a/drivers/mfd/rk8xx-core.c > +++ b/drivers/mfd/rk8xx-core.c > @@ -11,6 +11,8 @@ > */ > > #include > +#include > +#include > #include > #include > #include > @@ -703,6 +705,24 @@ 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: > + * with any of them left asserted, the rails drop and then come back > + * up. Releasing them here rather than from a separate handler keeps > + * the ordering against the I2C write explicit. > + */ > + if (rk808->power_hold_gpios) { > + unsigned int i; What is i here? Can it have a better name? > + > + for (i = 0; i < rk808->power_hold_gpios->ndescs; i++) for (int = 0; ... > + gpiod_set_value_cansleep(rk808->power_hold_gpios->desc[i], 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 +786,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 +872,29 @@ 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); Why wrap here? Some of the surrounding lines are clearly longer. > + 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"); > + > + device_property_read_u32(dev, "power-hold-delay-ms", > + &rk808->power_hold_delay_ms); What if this doesn't return a value? > + } > + > ret = devm_regmap_add_irq_chip(dev, rk808->regmap, irq, > IRQF_ONESHOT | dual_support, -1, > rk808->regmap_irq_chip, &rk808->irq_data); > @@ -872,8 +916,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..eda111e 100644 > --- a/include/linux/mfd/rk808.h > +++ b/include/linux/mfd/rk808.h > @@ -1459,6 +1459,8 @@ enum { > RK818_ID = 0x8180, > }; > > +struct gpio_descs; Do we do this for all of the other structs? Why not add the include? > + > struct rk808 { > struct device *dev; > struct regmap_irq_chip_data *irq_data; > @@ -1466,6 +1468,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 > -- Lee Jones