[PATCH v2 0/6] regulator: pca9450: register restart handlers

[PATCH v2 0/6] regulator: pca9450: register restart handlers

[Benjamin Bara]

Hi!

This series implements two restart handler registrations for the pca9450
(6/6). As the pca9450 supports both, cold and warm resets, and there
exist at least one other PMIC implementation which also implements a
warm and a cold reset (tps65219), 1-5/6 should simplify/clarify the
distinction process between warm/soft and cold/hard resets/restarts.
Instead of deciding in the handler, this should be done during
registration. The series is a follow-up to Dmitry's feedback, regarding
checking the reboot_mode in the handler [1].

As the cold handler queue is executed before the warm handler queue
(when the reboot_mode is not changed/specified), cold handlers are
implicitly executed with a higher prio and therefore the default
registration can be used.

This series is based on linux-next and 6/6 depends on [2].

Thanks & best regards,
Benjamin

[1] https://lore.kernel.org/all/7eddaf8c-ab04-7670-fc45-15f0fce5eff2@collabora.com/
[2] https://lore.kernel.org/all/20230327-tegra-pmic-reboot-v7-3-18699d5dcd76@skidata.com/

---
Changes in v2:
- rebase to next-20230809
- improve commit messages
- use helper (with implicit priority) instead of explicit priority
- fallback to warm handler if hard/cold requested but failed
- Link to v1: https://lore.kernel.org/r/20230727-pca9450-reboot-v1-0-c8edb27bf404@skidata.com

---
Benjamin Bara (6):
      kernel/reboot: distinguish between cold and warm
      mfd: rk8xx: Specify restart mode
      soc/tegra: pmc: Specify restart mode
      mfd: tps65219: Specify restart mode
      kernel/reboot: remove generic restart mode
      regulator: pca9450: register restart handlers

 drivers/mfd/rk8xx-core.c              |  6 +--
 drivers/mfd/tps65219.c                | 17 +++++--
 drivers/regulator/pca9450-regulator.c | 59 ++++++++++++++++++++++++
 drivers/soc/tegra/pmc.c               |  2 +-
 include/linux/reboot.h                | 23 +++++++---
 include/linux/regulator/pca9450.h     |  7 +++
 kernel/reboot.c                       | 84 +++++++++++++++++++++++++++++------
 7 files changed, 170 insertions(+), 28 deletions(-)
---
base-commit: 21ef7b1e17d039053edaeaf41142423810572741
change-id: 20230724-pca9450-reboot-0b32218fc7a2

Best regards,
-- 
Benjamin Bara <benjamin.bara@skidata.com>

[PATCH v2 1/6] kernel/reboot: distinguish between cold and warm

[Benjamin Bara]

From: Benjamin Bara <benjamin.bara@skidata.com>

Currently, the kernel is not aware if the registered reboot handler is a
cold/hard or warm/soft one. However, there exists a 'reboot_mode' kernel
parameter which can be used to specify the type of reboot the kernel
should use. This parameter is currently not very well supported, due to
the lack of distinction possibilities. Instead, cold/hard reboot
handlers are often registered with a higher priorization to indicate the
kernel to prefer these over others. With a proper distinction
possibility, the reboot_mode kernel parameter can be respected in a much
simpler way. Additionally, a warning can be shown if no proper reboot
handler is implemented. Due to the priorization of cold/hard reboot
handlers, these are preferred implicitly and therefore the registration
helper can be used instead. The "unspecified" handlers are still
supported, as they are registered as "soft reboots" and are therefore
called as fallback.

Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
---
v2:
- fallback to warm handler if hard/cold restart requested but failed
- improve commit message
---
 include/linux/reboot.h | 22 ++++++++++++
 kernel/reboot.c        | 94 ++++++++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 110 insertions(+), 6 deletions(-)

diff --git a/include/linux/reboot.h b/include/linux/reboot.h
index 2b6bb593be5b..05199aedb696 100644
--- a/include/linux/reboot.h
+++ b/include/linux/reboot.h
@@ -119,6 +119,20 @@ enum sys_off_mode {
 	 * Handlers restart system. Handlers are disallowed to sleep.
 	 */
 	SYS_OFF_MODE_RESTART,
+
+	/**
+	 * @SYS_OFF_MODE_RESTART_COLD:
+	 *
+	 * Handlers cold restart system. Handlers are disallowed to sleep.
+	 */
+	SYS_OFF_MODE_RESTART_COLD,
+
+	/**
+	 * @SYS_OFF_MODE_RESTART_WARM:
+	 *
+	 * Handlers warm restart system. Handlers are disallowed to sleep.
+	 */
+	SYS_OFF_MODE_RESTART_WARM,
 };
 
 /**
@@ -157,6 +171,14 @@ int devm_register_restart_handler(struct device *dev,
 				  int (*callback)(struct sys_off_data *data),
 				  void *cb_data);
 
+int devm_register_cold_restart_handler(struct device *dev,
+				       int (*callback)(struct sys_off_data *data),
+				       void *cb_data);
+
+int devm_register_warm_restart_handler(struct device *dev,
+				       int (*callback)(struct sys_off_data *data),
+				       void *cb_data);
+
 int register_platform_power_off(void (*power_off)(void));
 void unregister_platform_power_off(void (*power_off)(void));
 
diff --git a/kernel/reboot.c b/kernel/reboot.c
index 3bba88c7ffc6..ab99f450801f 100644
--- a/kernel/reboot.c
+++ b/kernel/reboot.c
@@ -146,9 +146,15 @@ EXPORT_SYMBOL(devm_register_reboot_notifier);
 
 /*
  *	Notifier list for kernel code which wants to be called
- *	to restart the system.
+ *	to cold restart the system.
  */
-static ATOMIC_NOTIFIER_HEAD(restart_handler_list);
+static ATOMIC_NOTIFIER_HEAD(cold_restart_handler_list);
+
+/*
+ *	Notifier list for kernel code which wants to be called
+ *	to warm restart the system.
+ */
+static ATOMIC_NOTIFIER_HEAD(warm_restart_handler_list);
 
 /**
  *	register_restart_handler - Register function to be called to reset
@@ -190,7 +196,11 @@ static ATOMIC_NOTIFIER_HEAD(restart_handler_list);
  */
 int register_restart_handler(struct notifier_block *nb)
 {
-	return atomic_notifier_chain_register(&restart_handler_list, nb);
+	/*
+	 * Store all non-devm-based handlers in the warm list to ensure that the
+	 * "specified" handler are preferred over the "unspecified" ones.
+	 */
+	return atomic_notifier_chain_register(&warm_restart_handler_list, nb);
 }
 EXPORT_SYMBOL(register_restart_handler);
 
@@ -205,7 +215,14 @@ EXPORT_SYMBOL(register_restart_handler);
  */
 int unregister_restart_handler(struct notifier_block *nb)
 {
-	return atomic_notifier_chain_unregister(&restart_handler_list, nb);
+	int ret;
+
+	ret = atomic_notifier_chain_unregister(&warm_restart_handler_list, nb);
+	if (ret == -ENOENT)
+		ret = atomic_notifier_chain_unregister(&cold_restart_handler_list,
+						       nb);
+
+	return ret;
 }
 EXPORT_SYMBOL(unregister_restart_handler);
 
@@ -222,7 +239,20 @@ EXPORT_SYMBOL(unregister_restart_handler);
  */
 void do_kernel_restart(char *cmd)
 {
-	atomic_notifier_call_chain(&restart_handler_list, reboot_mode, cmd);
+	/*
+	 * REBOOT_GPIO can be either cold or warm -> let handler decide.
+	 * Prefer cold reboot if mode not specified.
+	 */
+	if (reboot_mode == REBOOT_UNDEFINED || reboot_mode == REBOOT_GPIO ||
+	    reboot_mode == REBOOT_HARD || reboot_mode == REBOOT_COLD) {
+		atomic_notifier_call_chain(&cold_restart_handler_list,
+					   reboot_mode, cmd);
+		if (reboot_mode == REBOOT_HARD || reboot_mode == REBOOT_COLD)
+			pr_emerg("hard/cold reboot failed, fallback!\n");
+	}
+
+	/* Always execute warm handler as fallback */
+	atomic_notifier_call_chain(&warm_restart_handler_list, reboot_mode, cmd);
 }
 
 void migrate_to_reboot_cpu(void)
@@ -414,7 +444,15 @@ register_sys_off_handler(enum sys_off_mode mode,
 		break;
 
 	case SYS_OFF_MODE_RESTART:
-		handler->list = &restart_handler_list;
+		handler->list = &warm_restart_handler_list;
+		break;
+
+	case SYS_OFF_MODE_RESTART_COLD:
+		handler->list = &cold_restart_handler_list;
+		break;
+
+	case SYS_OFF_MODE_RESTART_WARM:
+		handler->list = &warm_restart_handler_list;
 		break;
 
 	default:
@@ -560,6 +598,50 @@ int devm_register_restart_handler(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(devm_register_restart_handler);
 
+/**
+ *	devm_register_cold_restart_handler - Register cold restart handler
+ *	@dev: Device that registers callback
+ *	@callback: Callback function
+ *	@cb_data: Callback's argument
+ *
+ *	Registers resource-managed sys-off handler with a default priority
+ *	and using cold restart mode.
+ *
+ *	Returns zero on success, or error code on failure.
+ */
+int devm_register_cold_restart_handler(struct device *dev,
+				       int (*callback)(struct sys_off_data *data),
+				       void *cb_data)
+{
+	return devm_register_sys_off_handler(dev,
+					     SYS_OFF_MODE_RESTART_COLD,
+					     SYS_OFF_PRIO_DEFAULT,
+					     callback, cb_data);
+}
+EXPORT_SYMBOL_GPL(devm_register_cold_restart_handler);
+
+/**
+ *	devm_register_warm_restart_handler - Register warm restart handler
+ *	@dev: Device that registers callback
+ *	@callback: Callback function
+ *	@cb_data: Callback's argument
+ *
+ *	Registers resource-managed sys-off handler with a default priority
+ *	and using warm restart mode.
+ *
+ *	Returns zero on success, or error code on failure.
+ */
+int devm_register_warm_restart_handler(struct device *dev,
+				       int (*callback)(struct sys_off_data *data),
+				       void *cb_data)
+{
+	return devm_register_sys_off_handler(dev,
+					     SYS_OFF_MODE_RESTART_WARM,
+					     SYS_OFF_PRIO_DEFAULT,
+					     callback, cb_data);
+}
+EXPORT_SYMBOL_GPL(devm_register_warm_restart_handler);
+
 static struct sys_off_handler *platform_power_off_handler;
 
 static int platform_power_off_notify(struct sys_off_data *data)

-- 
2.34.1

[PATCH v2 2/6] mfd: rk8xx: Specify restart mode

[Benjamin Bara]

From: Benjamin Bara <benjamin.bara@skidata.com>

The current restart handler registration does not specify whether the
restart is a cold or a warm one. Instead, cold ones are typically
registered with a HIGH prio. Now, as do_kernel_restart() knows about the
type, the priorization is implicitly done (cold restarts are executed
first) and the reboot_mode kernel parameter (which is currently mostly
ignored) can be respected.

Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
---
v2:
- improve commit message
- use registration helper instead
---
 drivers/mfd/rk8xx-core.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/mfd/rk8xx-core.c b/drivers/mfd/rk8xx-core.c
index 11a831e92da8..7faf8e1189f7 100644
--- a/drivers/mfd/rk8xx-core.c
+++ b/drivers/mfd/rk8xx-core.c
@@ -696,9 +696,9 @@ int rk8xx_probe(struct device *dev, int variant, unsigned int irq, struct regmap
 		switch (rk808->variant) {
 		case RK809_ID:
 		case RK817_ID:
-			ret = devm_register_sys_off_handler(dev,
-							    SYS_OFF_MODE_RESTART, SYS_OFF_PRIO_HIGH,
-							    &rk808_restart, rk808);
+			ret = devm_register_cold_restart_handler(dev,
+								 &rk808_restart,
+								 rk808);
 			if (ret)
 				dev_warn(dev, "failed to register rst handler, %d\n", ret);
 			break;

-- 
2.34.1

[PATCH v2 3/6] soc/tegra: pmc: Specify restart mode

[Benjamin Bara]

From: Benjamin Bara <benjamin.bara@skidata.com>

The current restart handler registration does not specify whether the
restart is a cold or a warm one. Now, as do_kernel_restart() knows about
the type, the priorization is implicitly done (cold restarts are
executed first) and the reboot_mode kernel parameter (which is currently
mostly ignored) can be respected.

Acked-by: Thierry Reding <treding@nvidia.com>
Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
---
v2:
- improve commit message
---
 drivers/soc/tegra/pmc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
index 162f52456f65..4f42febb9b0f 100644
--- a/drivers/soc/tegra/pmc.c
+++ b/drivers/soc/tegra/pmc.c
@@ -2962,7 +2962,7 @@ static int tegra_pmc_probe(struct platform_device *pdev)
 	}
 
 	err = devm_register_sys_off_handler(&pdev->dev,
-					    SYS_OFF_MODE_RESTART,
+					    SYS_OFF_MODE_RESTART_WARM,
 					    SYS_OFF_PRIO_LOW,
 					    tegra_pmc_restart_handler, NULL);
 	if (err) {

-- 
2.34.1

[PATCH v2 4/6] mfd: tps65219: Specify restart mode

[Benjamin Bara]

From: Benjamin Bara <benjamin.bara@skidata.com>

The current restart handler registration does not specify whether the
restart is a cold or a warm one. Instead, cold ones are typically
registered with a HIGH prio. Now, as do_kernel_restart() knows about the
type, the priorization is implicitly done (cold restarts are executed
first) and the reboot_mode kernel parameter (which is currently mostly
ignored) can be respected.

Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
---
v2:
- improve commit message
---
 drivers/mfd/tps65219.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/mfd/tps65219.c b/drivers/mfd/tps65219.c
index 0e0c42e4fdfc..85752b93256e 100644
--- a/drivers/mfd/tps65219.c
+++ b/drivers/mfd/tps65219.c
@@ -278,12 +278,21 @@ static int tps65219_probe(struct i2c_client *client)
 		}
 	}
 
-	ret = devm_register_restart_handler(tps->dev,
-					    tps65219_restart_handler,
-					    tps);
+	ret = devm_register_cold_restart_handler(tps->dev,
+						 tps65219_restart_handler,
+						 tps);
 
 	if (ret) {
-		dev_err(tps->dev, "cannot register restart handler, %d\n", ret);
+		dev_err(tps->dev, "cannot register cold restart handler, %d\n", ret);
+		return ret;
+	}
+
+	ret = devm_register_warm_restart_handler(tps->dev,
+						 tps65219_restart_handler,
+						 tps);
+
+	if (ret) {
+		dev_err(tps->dev, "cannot register warm restart handler, %d\n", ret);
 		return ret;
 	}
 

-- 
2.34.1

[PATCH v2 5/6] kernel/reboot: remove generic restart mode

[Benjamin Bara]

From: Benjamin Bara <benjamin.bara@skidata.com>

Remove the "unspecified" restart mode, as it is not in use anymore. New
handler registrations should use the specified "cold" or "warm" instead.
These respect the "reboot_mode" kernel parameter and enable implicit
priorization (cold before warm) when the reboot_mode is unspecified.

Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
---
v2:
- improve commit message
---
 include/linux/reboot.h | 11 -----------
 kernel/reboot.c        | 26 --------------------------
 2 files changed, 37 deletions(-)

diff --git a/include/linux/reboot.h b/include/linux/reboot.h
index 05199aedb696..ad1a7b4026c0 100644
--- a/include/linux/reboot.h
+++ b/include/linux/reboot.h
@@ -113,13 +113,6 @@ enum sys_off_mode {
 	 */
 	SYS_OFF_MODE_RESTART_PREPARE,
 
-	/**
-	 * @SYS_OFF_MODE_RESTART:
-	 *
-	 * Handlers restart system. Handlers are disallowed to sleep.
-	 */
-	SYS_OFF_MODE_RESTART,
-
 	/**
 	 * @SYS_OFF_MODE_RESTART_COLD:
 	 *
@@ -167,10 +160,6 @@ int devm_register_power_off_handler(struct device *dev,
 				    int (*callback)(struct sys_off_data *data),
 				    void *cb_data);
 
-int devm_register_restart_handler(struct device *dev,
-				  int (*callback)(struct sys_off_data *data),
-				  void *cb_data);
-
 int devm_register_cold_restart_handler(struct device *dev,
 				       int (*callback)(struct sys_off_data *data),
 				       void *cb_data);
diff --git a/kernel/reboot.c b/kernel/reboot.c
index ab99f450801f..af0e090dd087 100644
--- a/kernel/reboot.c
+++ b/kernel/reboot.c
@@ -443,10 +443,6 @@ register_sys_off_handler(enum sys_off_mode mode,
 		handler->blocking = true;
 		break;
 
-	case SYS_OFF_MODE_RESTART:
-		handler->list = &warm_restart_handler_list;
-		break;
-
 	case SYS_OFF_MODE_RESTART_COLD:
 		handler->list = &cold_restart_handler_list;
 		break;
@@ -576,28 +572,6 @@ int devm_register_power_off_handler(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(devm_register_power_off_handler);
 
-/**
- *	devm_register_restart_handler - Register restart handler
- *	@dev: Device that registers callback
- *	@callback: Callback function
- *	@cb_data: Callback's argument
- *
- *	Registers resource-managed sys-off handler with a default priority
- *	and using restart mode.
- *
- *	Returns zero on success, or error code on failure.
- */
-int devm_register_restart_handler(struct device *dev,
-				  int (*callback)(struct sys_off_data *data),
-				  void *cb_data)
-{
-	return devm_register_sys_off_handler(dev,
-					     SYS_OFF_MODE_RESTART,
-					     SYS_OFF_PRIO_DEFAULT,
-					     callback, cb_data);
-}
-EXPORT_SYMBOL_GPL(devm_register_restart_handler);
-
 /**
  *	devm_register_cold_restart_handler - Register cold restart handler
  *	@dev: Device that registers callback

-- 
2.34.1

[PATCH v2 6/6] regulator: pca9450: register restart handlers

[Benjamin Bara]

From: Benjamin Bara <benjamin.bara@skidata.com>

The PCA9450 supports both, a warm and a cold reset. Implement both and
register the respective handlers.

Acked-by: Mark Brown <broonie@kernel.org>
Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
---
 drivers/regulator/pca9450-regulator.c | 59 +++++++++++++++++++++++++++++++++++
 include/linux/regulator/pca9450.h     |  7 +++++
 2 files changed, 66 insertions(+)

diff --git a/drivers/regulator/pca9450-regulator.c b/drivers/regulator/pca9450-regulator.c
index 2ab365d2749f..86903d677bf6 100644
--- a/drivers/regulator/pca9450-regulator.c
+++ b/drivers/regulator/pca9450-regulator.c
@@ -38,6 +38,11 @@ struct pca9450 {
 	int irq;
 };
 
+static inline struct pca9450 *dev_to_pca9450(struct device *dev)
+{
+	return dev_get_drvdata(dev);
+}
+
 static const struct regmap_range pca9450_status_range = {
 	.range_min = PCA9450_REG_INT1,
 	.range_max = PCA9450_REG_PWRON_STAT,
@@ -219,6 +224,42 @@ static int pca9450_set_dvs_levels(struct device_node *np,
 	return ret;
 }
 
+static int pca9450_cold_reset(struct pca9450 *pca9450)
+{
+	int ret;
+
+	ret = regmap_write(pca9450->regmap, PCA9450_REG_SWRST,
+			   SWRST_RESET_COLD_LDO12);
+	if (ret)
+		return ret;
+
+	/* t_RESTART is 250 ms. */
+	mdelay(500);
+	return -ETIME;
+}
+
+static int pca9450_warm_reset(struct pca9450 *pca9450)
+{
+	int ret;
+
+	ret = regmap_write(pca9450->regmap, PCA9450_REG_SWRST,
+			   SWRST_RESET_WARM);
+	if (ret)
+		return ret;
+
+	/* t_RESET is 20 ms. */
+	mdelay(50);
+	return -ETIME;
+}
+
+static int pca9450_restart_handler(struct sys_off_data *data)
+{
+	int (*handler)(struct pca9450 *) = data->cb_data;
+	struct pca9450 *pca9450 = dev_to_pca9450(data->dev);
+
+	return handler(pca9450);
+}
+
 static const struct pca9450_regulator_desc pca9450a_regulators[] = {
 	{
 		.desc = {
@@ -845,6 +886,24 @@ static int pca9450_i2c_probe(struct i2c_client *i2c)
 		return PTR_ERR(pca9450->sd_vsel_gpio);
 	}
 
+	ret = devm_register_cold_restart_handler(pca9450->dev,
+						 pca9450_restart_handler,
+						 pca9450_cold_reset);
+	if (ret) {
+		dev_err(&i2c->dev, "register cold restart handler failed: %d\n",
+			ret);
+		return ret;
+	}
+
+	ret = devm_register_warm_restart_handler(pca9450->dev,
+						 pca9450_restart_handler,
+						 pca9450_warm_reset);
+	if (ret) {
+		dev_err(&i2c->dev, "register warm restart handler failed: %d\n",
+			ret);
+		return ret;
+	}
+
 	dev_info(&i2c->dev, "%s probed.\n",
 		type == PCA9450_TYPE_PCA9450A ? "pca9450a" : "pca9450bc");
 
diff --git a/include/linux/regulator/pca9450.h b/include/linux/regulator/pca9450.h
index 505c908dbb81..a72fd4942d5f 100644
--- a/include/linux/regulator/pca9450.h
+++ b/include/linux/regulator/pca9450.h
@@ -93,6 +93,13 @@ enum {
 	PCA9450_MAX_REGISTER	    = 0x2F,
 };
 
+/* PCA9450 SW_RST bits */
+#define SWRST_NOACTION			0x00
+#define SWRST_RESET_REGS		0x05
+#define SWRST_RESET_COLD_LDO12		0x14
+#define SWRST_RESET_WARM		0x35
+#define SWRST_RESET_COLD		0x64
+
 /* PCA9450 BUCK ENMODE bits */
 #define BUCK_ENMODE_OFF			0x00
 #define BUCK_ENMODE_ONREQ		0x01

-- 
2.34.1

Re: [PATCH v2 3/6] soc/tegra: pmc: Specify restart mode

[Dmitry Osipenko]

On 8/9/23 22:24, Benjamin Bara wrote:
> From: Benjamin Bara <benjamin.bara@skidata.com>
> 
> The current restart handler registration does not specify whether the
> restart is a cold or a warm one. Now, as do_kernel_restart() knows about
> the type, the priorization is implicitly done (cold restarts are
> executed first) and the reboot_mode kernel parameter (which is currently
> mostly ignored) can be respected.
> 
> Acked-by: Thierry Reding <treding@nvidia.com>
> Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
> ---
> v2:
> - improve commit message
> ---
>  drivers/soc/tegra/pmc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
> index 162f52456f65..4f42febb9b0f 100644
> --- a/drivers/soc/tegra/pmc.c
> +++ b/drivers/soc/tegra/pmc.c
> @@ -2962,7 +2962,7 @@ static int tegra_pmc_probe(struct platform_device *pdev)
>  	}
>  
>  	err = devm_register_sys_off_handler(&pdev->dev,
> -					    SYS_OFF_MODE_RESTART,
> +					    SYS_OFF_MODE_RESTART_WARM,
>  					    SYS_OFF_PRIO_LOW,
>  					    tegra_pmc_restart_handler, NULL);
>  	if (err) {
> 

You have tegra-pmc restart handler that uses low priority. And then
you're adding cold/warm handlers to tps65219 and pca9450 drivers with a
default priorities. Hence this cold/warm separation of handlers doesn't
do any practical difference in yours case because tegra-pmc will never
be used as it did before your changes?

Previously you wanted to make tps6586x driver to skip the warm reboot,
but you're not touching tps6586x in this patchset. There is no real
problem that is solved by these patches?

-- 
Best regards,
Dmitry

Re: [PATCH v2 3/6] soc/tegra: pmc: Specify restart mode

[Benjamin Bara]

Hi Dmitry,

thanks for your feedback!

On Tue, 15 Aug 2023 at 00:38, Dmitry Osipenko <dmitry.osipenko@collabora.com> wrote:
> On 8/9/23 22:24, Benjamin Bara wrote:
> > From: Benjamin Bara <benjamin.bara@skidata.com>
> >
> > The current restart handler registration does not specify whether the
> > restart is a cold or a warm one. Now, as do_kernel_restart() knows about
> > the type, the priorization is implicitly done (cold restarts are
> > executed first) and the reboot_mode kernel parameter (which is currently
> > mostly ignored) can be respected.
> >
> > Acked-by: Thierry Reding <treding@nvidia.com>
> > Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
> > ---
> > v2:
> > - improve commit message
> > ---
> >  drivers/soc/tegra/pmc.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
> > index 162f52456f65..4f42febb9b0f 100644
> > --- a/drivers/soc/tegra/pmc.c
> > +++ b/drivers/soc/tegra/pmc.c
> > @@ -2962,7 +2962,7 @@ static int tegra_pmc_probe(struct platform_device *pdev)
> >       }
> > 
> >       err = devm_register_sys_off_handler(&pdev->dev,
> > -                                         SYS_OFF_MODE_RESTART,
> > +                                         SYS_OFF_MODE_RESTART_WARM,
> >                                           SYS_OFF_PRIO_LOW,
> >                                           tegra_pmc_restart_handler, NULL);
> >       if (err) {
> >
>
> You have tegra-pmc restart handler that uses low priority. And then
> you're adding cold/warm handlers to tps65219 and pca9450 drivers with a
> default priorities. 

Exactly, but I guess it makes sense to also use the handler with default
priority for the pmc reboot. The reason I kept it low prio was because
there is a comment that PMC should be last resort, but the reason
applies to any other soft restart handler too. I will adapt in the next
version.

> Hence this cold/warm separation of handlers doesn't do any practical
> difference in yours case because tegra-pmc will never be used as it
> did before your changes?

The change is e.g. relevant for platforms without PMIC-based soft reset,
e.g. when the tps6586x is in use. AFAIK, there is no possibility to
actually do a soft reboot, as the hard reboot will always be executed
first. This also happens if I set the kernel parameter "reboot_mode"
(also available via SysFS) to "soft" and a soft restart handler is
registered.

> Previously you wanted to make tps6586x driver to skip the warm reboot,
> but you're not touching tps6586x in this patchset.

True, there might also be other affected patches which are currently not
in linux-next yet. Will adapt the tps6586x too and depend on the whole
series in the next version.

> There is no real problem that is solved by these patches?

I think another problem is if the user sets the "reboot_mode" to "cold",
but there is no cold handler registered (as it was the case for me with
the pca9450), a warning should indicate that. AFAIK, there is no
possibility in user-space to find out if a cold restart handler is
registered, there is just this knob which can be switched to "cold". I
can also add a SysFS entry with "supported_modes" and check if the new
"mode" is supported on a store.

My other idea was to add a flags field to the notifier_block which
indicates (in case of a reboot notifier) the supported reboot_modes of
the registered handler, but I guess other notifier_block users won't
really benefit from an additional field, therefore I decided to add a
second list instead.

Best regards
Benjamin

Re: [PATCH v2 2/6] mfd: rk8xx: Specify restart mode

[Lee Jones]

On Wed, 09 Aug 2023, Benjamin Bara wrote:

> From: Benjamin Bara <benjamin.bara@skidata.com>
> 
> The current restart handler registration does not specify whether the
> restart is a cold or a warm one. Instead, cold ones are typically
> registered with a HIGH prio. Now, as do_kernel_restart() knows about the
> type, the priorization is implicitly done (cold restarts are executed
> first) and the reboot_mode kernel parameter (which is currently mostly
> ignored) can be respected.
> 
> Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
> ---
> v2:
> - improve commit message
> - use registration helper instead
> ---
>  drivers/mfd/rk8xx-core.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mfd/rk8xx-core.c b/drivers/mfd/rk8xx-core.c
> index 11a831e92da8..7faf8e1189f7 100644
> --- a/drivers/mfd/rk8xx-core.c
> +++ b/drivers/mfd/rk8xx-core.c
> @@ -696,9 +696,9 @@ int rk8xx_probe(struct device *dev, int variant, unsigned int irq, struct regmap
>  		switch (rk808->variant) {
>  		case RK809_ID:
>  		case RK817_ID:
> -			ret = devm_register_sys_off_handler(dev,
> -							    SYS_OFF_MODE_RESTART, SYS_OFF_PRIO_HIGH,
> -							    &rk808_restart, rk808);
> +			ret = devm_register_cold_restart_handler(dev,
> +								 &rk808_restart,
> +								 rk808);

These wraps are now superfluous.

>  			if (ret)
>  				dev_warn(dev, "failed to register rst handler, %d\n", ret);
>  			break;
> 
> -- 
> 2.34.1
> 

-- 
Lee Jones [李琼斯]

Re: [PATCH v2 4/6] mfd: tps65219: Specify restart mode

[Lee Jones]

On Wed, 09 Aug 2023, Benjamin Bara wrote:

> From: Benjamin Bara <benjamin.bara@skidata.com>
> 
> The current restart handler registration does not specify whether the
> restart is a cold or a warm one. Instead, cold ones are typically
> registered with a HIGH prio. Now, as do_kernel_restart() knows about the
> type, the priorization is implicitly done (cold restarts are executed
> first) and the reboot_mode kernel parameter (which is currently mostly
> ignored) can be respected.
> 
> Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
> ---
> v2:
> - improve commit message
> ---
>  drivers/mfd/tps65219.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mfd/tps65219.c b/drivers/mfd/tps65219.c
> index 0e0c42e4fdfc..85752b93256e 100644
> --- a/drivers/mfd/tps65219.c
> +++ b/drivers/mfd/tps65219.c
> @@ -278,12 +278,21 @@ static int tps65219_probe(struct i2c_client *client)
>  		}
>  	}
>  
> -	ret = devm_register_restart_handler(tps->dev,
> -					    tps65219_restart_handler,
> -					    tps);
> +	ret = devm_register_cold_restart_handler(tps->dev,
> +						 tps65219_restart_handler,
> +						 tps);
>  
>  	if (ret) {
> -		dev_err(tps->dev, "cannot register restart handler, %d\n", ret);
> +		dev_err(tps->dev, "cannot register cold restart handler, %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = devm_register_warm_restart_handler(tps->dev,
> +						 tps65219_restart_handler,
> +						 tps);
> +

Sorry, why do we have to now register 2 restart handlers?

Seems like a regression?

> +	if (ret) {
> +		dev_err(tps->dev, "cannot register warm restart handler, %d\n", ret);
>  		return ret;
>  	}
>  
> 
> -- 
> 2.34.1
> 

-- 
Lee Jones [李琼斯]

Re: [PATCH v2 0/6] regulator: pca9450: register restart handlers

[Lee Jones]

On Wed, 09 Aug 2023, Benjamin Bara wrote:

> Hi!
> 
> This series implements two restart handler registrations for the pca9450
> (6/6). As the pca9450 supports both, cold and warm resets, and there
> exist at least one other PMIC implementation which also implements a
> warm and a cold reset (tps65219), 1-5/6 should simplify/clarify the
> distinction process between warm/soft and cold/hard resets/restarts.
> Instead of deciding in the handler, this should be done during
> registration. The series is a follow-up to Dmitry's feedback, regarding
> checking the reboot_mode in the handler [1].
> 
> As the cold handler queue is executed before the warm handler queue
> (when the reboot_mode is not changed/specified), cold handlers are
> implicitly executed with a higher prio and therefore the default
> registration can be used.
> 
> This series is based on linux-next and 6/6 depends on [2].
> 
> Thanks & best regards,
> Benjamin
> 
> [1] https://lore.kernel.org/all/7eddaf8c-ab04-7670-fc45-15f0fce5eff2@collabora.com/
> [2] https://lore.kernel.org/all/20230327-tegra-pmic-reboot-v7-3-18699d5dcd76@skidata.com/
> 
> ---
> Changes in v2:
> - rebase to next-20230809
> - improve commit messages
> - use helper (with implicit priority) instead of explicit priority
> - fallback to warm handler if hard/cold requested but failed
> - Link to v1: https://lore.kernel.org/r/20230727-pca9450-reboot-v1-0-c8edb27bf404@skidata.com
> 
> ---
> Benjamin Bara (6):
>       kernel/reboot: distinguish between cold and warm
>       mfd: rk8xx: Specify restart mode
>       soc/tegra: pmc: Specify restart mode
>       mfd: tps65219: Specify restart mode
>       kernel/reboot: remove generic restart mode
>       regulator: pca9450: register restart handlers

What are they interdependencies between them all?

Should they all be applied at once?

>  drivers/mfd/rk8xx-core.c              |  6 +--
>  drivers/mfd/tps65219.c                | 17 +++++--
>  drivers/regulator/pca9450-regulator.c | 59 ++++++++++++++++++++++++
>  drivers/soc/tegra/pmc.c               |  2 +-
>  include/linux/reboot.h                | 23 +++++++---
>  include/linux/regulator/pca9450.h     |  7 +++
>  kernel/reboot.c                       | 84 +++++++++++++++++++++++++++++------
>  7 files changed, 170 insertions(+), 28 deletions(-)
> ---
> base-commit: 21ef7b1e17d039053edaeaf41142423810572741
> change-id: 20230724-pca9450-reboot-0b32218fc7a2
> 
> Best regards,
> -- 
> Benjamin Bara <benjamin.bara@skidata.com>
> 

-- 
Lee Jones [李琼斯]