Re: [PATCH] mfd: tps6586x: register restart handler

[Dmitry Osipenko]

On 3/16/23 19:47, Benjamin Bara wrote:
> From: Benjamin Bara <benjamin.bara@xxxxxxxxxxx>
> 
> The TPS658629-Q1 (unfortunately the only TPS6586x with public data sheet)
> provides a SOFT RST bit in the SUPPLYENE reg to request a (cold) reboot.
> 
> Use it to implement and register a restart handler/notifier.
> The DS does not clarify if only writing the SOFT RST bit has side effects.
> Therefore, regmap_update_bits() is used.
> 
> Set an appropriate system_state and disable the IRQs to activate
> i2c_in_atomic_xfer_mode(). This avoids the following WARNING:
> [   12.667612] WARNING: CPU: 1 PID: 1 at kernel/rcu/tree_plugin.h:318 rcu_note_context_switch+0x33c/0x6b0
> [   12.676926] Voluntary context switch within RCU read-side critical section!
> ...
> [   12.742376]  schedule_timeout from wait_for_completion_timeout+0x90/0x114
> [   12.749179]  wait_for_completion_timeout from tegra_i2c_wait_completion+0x40/0x70
> 
> Tested on a TPS658640.
> 
> Signed-off-by: Benjamin Bara <benjamin.bara@xxxxxxxxxxx>
> ---
>  drivers/mfd/tps6586x.c | 38 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 36 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mfd/tps6586x.c b/drivers/mfd/tps6586x.c
> index 2d947f3f606a..e40c664b5f64 100644
> --- a/drivers/mfd/tps6586x.c
> +++ b/drivers/mfd/tps6586x.c
> @@ -15,6 +15,7 @@
>  #include <linux/interrupt.h>
>  #include <linux/irq.h>
>  #include <linux/irqdomain.h>
> +#include <linux/irqflags.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/mutex.h>
> @@ -22,6 +23,7 @@
>  #include <linux/err.h>
>  #include <linux/i2c.h>
>  #include <linux/platform_device.h>
> +#include <linux/reboot.h>
>  #include <linux/regmap.h>
>  #include <linux/of.h>
>  
> @@ -29,6 +31,7 @@
>  #include <linux/mfd/tps6586x.h>
>  
>  #define TPS6586X_SUPPLYENE	0x14
> +#define SOFT_RST_BIT		BIT(0)
>  #define EXITSLREQ_BIT		BIT(1)
>  #define SLEEP_MODE_BIT		BIT(3)
>  
> @@ -466,6 +469,23 @@ static void tps6586x_power_off(void)
>  	tps6586x_set_bits(tps6586x_dev, TPS6586X_SUPPLYENE, SLEEP_MODE_BIT);
>  }
>  
> +static int tps6586x_restart_notify(struct notifier_block *this, unsigned long mode, void *data)
> +{
> +	/* bring pmic into HARD REBOOT state, disable IRQs for atomic i2c RCU */
> +	system_state = SYSTEM_RESTART;
> +	local_irq_disable();
> +	tps6586x_set_bits(tps6586x_dev, TPS6586X_SUPPLYENE, SOFT_RST_BIT);
> +	local_irq_enable();
> +
> +	mdelay(500);
> +	return NOTIFY_DONE;
> +}
> +
> +static struct notifier_block tps6586x_restart_handler = {
> +	.notifier_call = tps6586x_restart_notify,
> +	.priority = 200,
> +};
> +
>  static void tps6586x_print_version(struct i2c_client *client, int version)
>  {
>  	const char *name;
> @@ -559,9 +579,16 @@ static int tps6586x_i2c_probe(struct i2c_client *client)
>  		goto err_add_devs;
>  	}
>  
> -	if (pdata->pm_off && !pm_power_off) {
> +	if (pdata->pm_off) {
>  		tps6586x_dev = &client->dev;
> -		pm_power_off = tps6586x_power_off;
> +		if (!pm_power_off)
> +			pm_power_off = tps6586x_power_off;
> +
> +		ret = register_restart_handler(&tps6586x_restart_handler);
> +		if (ret) {
> +			dev_err(&client->dev, "register restart handler failed: %d\n", ret);
> +			goto err_add_devs;
> +		}
>  	}
>  
>  	return 0;
> @@ -582,6 +609,13 @@ static void tps6586x_i2c_remove(struct i2c_client *client)
>  	mfd_remove_devices(tps6586x->dev);
>  	if (client->irq)
>  		free_irq(client->irq, tps6586x);
> +
> +	if (tps6586x_dev == &client->dev) {
> +		tps6586x_dev = NULL;
> +		unregister_restart_handler(&tps6586x_restart_handler);
> +		if (pm_power_off == tps6586x_power_off)
> +			pm_power_off = NULL;
> +	}

There are newer devm_register_power_off/restart_handler() helpers in
kernel [1]. Will be great if you'll use them in v2.

[1]
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/include/linux/reboot.h

-- 
Best regards,
Dmitry