linux-kernel - Re: [PATCH v4] mfd: rk808: add reboot support to rk808.c

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <YfgIFM9FWkHWEnuC@google.com>
Date:   Mon, 31 Jan 2022 16:02:28 +0000
From:   Lee Jones <lee.jones@...aro.org>
To:     Peter Geis <pgwipeout@...il.com>
Cc:     Robin Murphy <robin.murphy@....com>,
        Heiko Stuebner <heiko@...ech.de>,
        linux-rockchip@...ts.infradead.org,
        Nicolas Frattaroli <frattaroli.nicolas@...il.com>,
        Dmitry Osipenko <digetx@...il.com>,
        Frank Wunderlich <frank-w@...lic-files.de>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4] mfd: rk808: add reboot support to rk808.c

On Tue, 21 Dec 2021, Peter Geis wrote:

> This adds reboot support to the rk808 pmic driver and enables it for
> the rk809 and rk817 devices.
> This only enables if the rockchip,system-power-controller flag is set.
> 
> Signed-off-by: Peter Geis <pgwipeout@...il.com>
> Signed-off-by: Frank Wunderlich <frank-w@...lic-files.de>
> Reviewed-by: Dmitry Osipenko <digetx@...il.com>
> ---
> (Resending due to incorrect Subject line)
> 
> This patch was created to address issues with psci-reset on rk356x
> chips. Until the rk356x series ATF open source code is released so we
> can fix the issue at the source, this is the only way to ensure reliable
> resetting on devices using these chips.
> 
> After testing the rk808 (thanks Robin!), it was found DEV_OFF_RST does
> not reset the PMIC to a power on state. Since the rk805 and rk818 match
> this register layout, I'm removing support for all three in the v2.
> It may be possible to add support to them using an RTC wakeup, but that
> has not been explored and is outside the scope of this patch.
> 
> Changelog:
> V4:
> - reorder rk808_restart_notify (Thanks Dmitry)
> - drop of_property_read_bool before unregister (Good catch Frank)
> 
> V3: Thanks Dmitry!
> - Adjust priority to be in line with other pmic drivers
> - Move ret handling to case switch
> - Make default registration debug
> - Add unregister function on removal
> 
> V2:
> - Squash the patch from Frank Wunderlich for rk809 support.
> - Remove support for the rk805, rk808, and rk818 devices.
> - Only register the reset handler for supported devices.
> - Remove unnecessary dev_err and dev_warn statements.
> - Register the reset handler directly
> 
>  drivers/mfd/rk808.c       | 44 +++++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/rk808.h |  1 +
>  2 files changed, 45 insertions(+)
> 
> diff --git a/drivers/mfd/rk808.c b/drivers/mfd/rk808.c
> index b181fe401330..874d461dda44 100644
> --- a/drivers/mfd/rk808.c
> +++ b/drivers/mfd/rk808.c
> @@ -19,6 +19,7 @@
>  #include <linux/module.h>
>  #include <linux/of_device.h>
>  #include <linux/regmap.h>
> +#include <linux/reboot.h>
>  
>  struct rk808_reg_data {
>  	int addr;
> @@ -543,6 +544,7 @@ static void rk808_pm_power_off(void)
>  		reg = RK808_DEVCTRL_REG,
>  		bit = DEV_OFF_RST;
>  		break;
> +	case RK809_ID:
>  	case RK817_ID:
>  		reg = RK817_SYS_CFG(3);
>  		bit = DEV_OFF;
> @@ -559,6 +561,34 @@ static void rk808_pm_power_off(void)
>  		dev_err(&rk808_i2c_client->dev, "Failed to shutdown device!\n");
>  }
>  
> +static int rk808_restart_notify(struct notifier_block *this, unsigned long mode, void *cmd)
> +{
> +	struct rk808 *rk808 = i2c_get_clientdata(rk808_i2c_client);
> +	unsigned int reg, bit;
> +	int ret;
> +
> +	switch (rk808->variant) {
> +	case RK809_ID:
> +	case RK817_ID:
> +		reg = RK817_SYS_CFG(3);
> +		bit = DEV_RST;
> +		break;
> +
> +	default:
> +		return NOTIFY_DONE;
> +	}
> +	ret = regmap_update_bits(rk808->regmap, reg, bit, bit);
> +	if (ret)
> +		dev_err(&rk808_i2c_client->dev, "Failed to restart device!\n");
> +
> +	return NOTIFY_DONE;
> +}
> +
> +static struct notifier_block rk808_restart_handler = {
> +	.notifier_call = rk808_restart_notify,
> +	.priority = 192,
> +};
> +
>  static void rk8xx_shutdown(struct i2c_client *client)
>  {
>  	struct rk808 *rk808 = i2c_get_clientdata(client);
> @@ -727,6 +757,18 @@ static int rk808_probe(struct i2c_client *client,
>  	if (of_property_read_bool(np, "rockchip,system-power-controller")) {
>  		rk808_i2c_client = client;
>  		pm_power_off = rk808_pm_power_off;
> +
> +		switch (rk808->variant) {
> +		case RK809_ID:
> +		case RK817_ID:
> +			ret = register_restart_handler(&rk808_restart_handler);
> +			if (ret)
> +				dev_warn(&client->dev, "failed to register restart handler, %d\n", ret);

This line looks very long.

I'm surprised it survived checkpatch.pl!

Once this has been fixed, please resubmit with my:

For my own reference (apply this as-is to your sign-off block):

  Acked-for-MFD-by: Lee Jones <lee.jones@...aro.org>

> +			break;
> +		default:
> +			dev_dbg(&client->dev, "pmic controlled board reset not supported\n");
> +			break;
> +		}
>  	}
>  
>  	return 0;
> @@ -749,6 +791,8 @@ static int rk808_remove(struct i2c_client *client)
>  	if (pm_power_off == rk808_pm_power_off)
>  		pm_power_off = NULL;
>  
> +	unregister_restart_handler(&rk808_restart_handler);
> +
>  	return 0;
>  }
>  
> diff --git a/include/linux/mfd/rk808.h b/include/linux/mfd/rk808.h
> index a96e6d43ca06..58602032e642 100644
> --- a/include/linux/mfd/rk808.h
> +++ b/include/linux/mfd/rk808.h
> @@ -373,6 +373,7 @@ enum rk805_reg {
>  #define SWITCH2_EN	BIT(6)
>  #define SWITCH1_EN	BIT(5)
>  #define DEV_OFF_RST	BIT(3)
> +#define DEV_RST		BIT(2)
>  #define DEV_OFF		BIT(0)
>  #define RTC_STOP	BIT(0)
>  

-- 
Lee Jones [李琼斯]
Principal Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog