Hi!
The Tegra20 requires an enabled VDE power domain during startup. As the
VDE is currently not used, it is disabled during runtime.
Since 8f0c714ad9be, there is a workaround for the "normal restart path"
which enables the VDE before doing PMC's warm reboot. This workaround is
not executed in the "emergency restart path", leading to a hang-up
during start.
This series implements and registers a new pmic-based restart handler
for boards with tps6586x. This cold reboot ensures that the VDE power
domain is enabled during startup on tegra20-based boards.
Since bae1d3a05a8b, i2c transfers are non-atomic while preemption is
disabled (which is e.g. done during panic()). This could lead to
warnings ("Voluntary context switch within RCU") in i2c-based restart
handlers during emergency restart. The state of preemption should be
detected by i2c_in_atomic_xfer_mode() to use atomic i2c xfer when
required. Beside the new system_state check, the check is the same as
the one pre v5.2.
v4: https://lore.kernel.org/r/[email protected]
v3: https://lore.kernel.org/r/[email protected]
v2: https://lore.kernel.org/all/[email protected]/
system_state: https://lore.kernel.org/all/[email protected]/
v1: https://lore.kernel.org/all/[email protected]/
v5:
- introduce new 3 & 4, therefore 3 -> 5, 4 -> 6
- 3: provide dev to sys_off handler, if it is known
- 4: return NOTIFY_DONE from sys_off_notify, to avoid skipping
- 5: drop Reviewed-by from Dmitry, add poweroff timeout
- 5,6: return notifier value instead of direct errno from handler
- 5,6: use new dev field instead of passing dev as cb_data
- 5,6: increase timeout values based on error observations
- 6: skip unsupported reboot modes in restart handler
v4:
- 1,2: add "Fixes" and adapt commit messages
- 4: reduce delay after requesting the restart (as suggested by Dmitry)
v3:
- bring system_state back in this series
- do atomic i2c xfer if not preemptible (as suggested by Dmitry)
- fix style issues mentioned by Dmitry
- add cc stable as suggested by Dmitry
- add explanation why this is needed for Jon
v2:
- use devm-based restart handler
- convert the existing power_off handler to a devm-based handler
- handle system_state in extra series
---
Benjamin Bara (6):
kernel/reboot: emergency_restart: set correct system_state
i2c: core: run atomic i2c xfer when !preemptible
kernel/reboot: add device to sys_off_handler
kernel/reboot: sys_off_notify: always return NOTIFY_DONE
mfd: tps6586x: use devm-based power off handler
mfd: tps6586x: register restart handler
drivers/i2c/i2c-core.h | 2 +-
drivers/mfd/tps6586x.c | 55 ++++++++++++++++++++++++++++++++++++++++++--------
include/linux/reboot.h | 3 +++
kernel/reboot.c | 11 +++++++++-
4 files changed, 61 insertions(+), 10 deletions(-)
---
base-commit: 197b6b60ae7bc51dd0814953c562833143b292aa
change-id: 20230327-tegra-pmic-reboot-4175ff814a4b
Best regards,
--
Benjamin Bara <[email protected]>
From: Benjamin Bara <[email protected]>
As the emergency restart does not call kernel_restart_prepare(), the
system_state stays in SYSTEM_RUNNING.
Since bae1d3a05a8b, this hinders i2c_in_atomic_xfer_mode() from becoming
active, and therefore might lead to avoidable warnings in the restart
handlers, e.g.:
[ 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
...
[ 12.994527] atomic_notifier_call_chain from machine_restart+0x34/0x58
[ 13.001050] machine_restart from panic+0x2a8/0x32c
Avoid these by setting the correct system_state.
Fixes: bae1d3a05a8b ("i2c: core: remove use of in_atomic()")
Cc: [email protected] # v5.2+
Signed-off-by: Benjamin Bara <[email protected]>
---
kernel/reboot.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/kernel/reboot.c b/kernel/reboot.c
index 3bba88c7ffc6..6ebef11c8876 100644
--- a/kernel/reboot.c
+++ b/kernel/reboot.c
@@ -74,6 +74,7 @@ void __weak (*pm_power_off)(void);
void emergency_restart(void)
{
kmsg_dump(KMSG_DUMP_EMERG);
+ system_state = SYSTEM_RESTART;
machine_emergency_restart();
}
EXPORT_SYMBOL_GPL(emergency_restart);
--
2.34.1
From: Benjamin Bara <[email protected]>
Since bae1d3a05a8b, i2c transfers are non-atomic if preemption is
disabled. However, non-atomic i2c transfers require preemption (e.g. in
wait_for_completion() while waiting for the DMA).
panic() calls preempt_disable_notrace() before calling
emergency_restart(). Therefore, if an i2c device is used for the
restart, the xfer should be atomic. This avoids warnings like:
[ 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
...
[ 12.994527] atomic_notifier_call_chain from machine_restart+0x34/0x58
[ 13.001050] machine_restart from panic+0x2a8/0x32c
Use !preemptible() instead, which is basically the same check as
pre-v5.2.
Fixes: bae1d3a05a8b ("i2c: core: remove use of in_atomic()")
Cc: [email protected] # v5.2+
Suggested-by: Dmitry Osipenko <[email protected]>
Acked-by: Wolfram Sang <[email protected]>
Signed-off-by: Benjamin Bara <[email protected]>
---
drivers/i2c/i2c-core.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/i2c/i2c-core.h b/drivers/i2c/i2c-core.h
index 1247e6e6e975..05b8b8dfa9bd 100644
--- a/drivers/i2c/i2c-core.h
+++ b/drivers/i2c/i2c-core.h
@@ -29,7 +29,7 @@ int i2c_dev_irq_from_resources(const struct resource *resources,
*/
static inline bool i2c_in_atomic_xfer_mode(void)
{
- return system_state > SYSTEM_RUNNING && irqs_disabled();
+ return system_state > SYSTEM_RUNNING && !preemptible();
}
static inline int __i2c_lock_bus_helper(struct i2c_adapter *adap)
--
2.34.1
From: Benjamin Bara <[email protected]>
If the dev is known (e.g. a devm-based sys_off_handler is used), it can
be passed to the handler's callback to have it available there.
Otherwise, cb_data might be set to the dev in most of the cases.
Signed-off-by: Benjamin Bara <[email protected]>
---
include/linux/reboot.h | 3 +++
kernel/reboot.c | 3 +++
2 files changed, 6 insertions(+)
diff --git a/include/linux/reboot.h b/include/linux/reboot.h
index 2b6bb593be5b..c4cc3b89ced1 100644
--- a/include/linux/reboot.h
+++ b/include/linux/reboot.h
@@ -129,11 +129,14 @@ enum sys_off_mode {
* @cb_data: User's callback data.
* @cmd: Command string. Currently used only by the sys-off restart mode,
* NULL otherwise.
+ * @dev: Device of the sys-off handler. Only if known (devm_register_*),
+ * NULL otherwise.
*/
struct sys_off_data {
int mode;
void *cb_data;
const char *cmd;
+ struct device *dev;
};
struct sys_off_handler *
diff --git a/kernel/reboot.c b/kernel/reboot.c
index 6ebef11c8876..395a0ea3c7a8 100644
--- a/kernel/reboot.c
+++ b/kernel/reboot.c
@@ -55,6 +55,7 @@ struct sys_off_handler {
enum sys_off_mode mode;
bool blocking;
void *list;
+ struct device *dev;
};
/*
@@ -324,6 +325,7 @@ static int sys_off_notify(struct notifier_block *nb,
data.cb_data = handler->cb_data;
data.mode = mode;
data.cmd = cmd;
+ data.dev = handler->dev;
return handler->sys_off_cb(&data);
}
@@ -511,6 +513,7 @@ int devm_register_sys_off_handler(struct device *dev,
handler = register_sys_off_handler(mode, priority, callback, cb_data);
if (IS_ERR(handler))
return PTR_ERR(handler);
+ handler->dev = dev;
return devm_add_action_or_reset(dev, devm_unregister_sys_off_handler,
handler);
--
2.34.1
From: Benjamin Bara <[email protected]>
A failed sys_off_handler should not be able to skip other registered
handlers. Therefore, report the error but always return NOTIFY_DONE, to
indicate that atomic_notifier_call_chain() should continue.
Signed-off-by: Benjamin Bara <[email protected]>
---
kernel/reboot.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/kernel/reboot.c b/kernel/reboot.c
index 395a0ea3c7a8..689a147bc1dc 100644
--- a/kernel/reboot.c
+++ b/kernel/reboot.c
@@ -320,6 +320,7 @@ static int sys_off_notify(struct notifier_block *nb,
{
struct sys_off_handler *handler;
struct sys_off_data data = {};
+ int ret;
handler = container_of(nb, struct sys_off_handler, nb);
data.cb_data = handler->cb_data;
@@ -327,7 +328,11 @@ static int sys_off_notify(struct notifier_block *nb,
data.cmd = cmd;
data.dev = handler->dev;
- return handler->sys_off_cb(&data);
+ ret = handler->sys_off_cb(&data);
+ if (ret != NOTIFY_DONE)
+ dev_err(handler->dev, "sys_off_handler failed: %d\n", notifier_to_errno(ret));
+
+ return NOTIFY_DONE;
}
static struct sys_off_handler platform_sys_off_handler;
--
2.34.1
From: Benjamin Bara <[email protected]>
Convert the power off handler to a devm-based power off handler.
Signed-off-by: Benjamin Bara <[email protected]>
---
drivers/mfd/tps6586x.c | 29 +++++++++++++++++++++--------
1 file changed, 21 insertions(+), 8 deletions(-)
diff --git a/drivers/mfd/tps6586x.c b/drivers/mfd/tps6586x.c
index 2d947f3f606a..226e856e34e0 100644
--- a/drivers/mfd/tps6586x.c
+++ b/drivers/mfd/tps6586x.c
@@ -22,6 +22,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>
@@ -457,13 +458,21 @@ static const struct regmap_config tps6586x_regmap_config = {
.cache_type = REGCACHE_RBTREE,
};
-static struct device *tps6586x_dev;
-static void tps6586x_power_off(void)
+static int tps6586x_power_off_handler(struct sys_off_data *data)
{
- if (tps6586x_clr_bits(tps6586x_dev, TPS6586X_SUPPLYENE, EXITSLREQ_BIT))
- return;
+ int ret;
+
+ /* bring pmic into SLEEP state. this takes at least 10ms. */
+ ret = tps6586x_clr_bits(data->dev, TPS6586X_SUPPLYENE, EXITSLREQ_BIT);
+ if (ret)
+ return notifier_from_errno(ret);
+
+ ret = tps6586x_set_bits(data->dev, TPS6586X_SUPPLYENE, SLEEP_MODE_BIT);
+ if (ret)
+ return notifier_from_errno(ret);
- tps6586x_set_bits(tps6586x_dev, TPS6586X_SUPPLYENE, SLEEP_MODE_BIT);
+ mdelay(50);
+ return notifier_from_errno(-ETIME);
}
static void tps6586x_print_version(struct i2c_client *client, int version)
@@ -559,9 +568,13 @@ static int tps6586x_i2c_probe(struct i2c_client *client)
goto err_add_devs;
}
- if (pdata->pm_off && !pm_power_off) {
- tps6586x_dev = &client->dev;
- pm_power_off = tps6586x_power_off;
+ if (pdata->pm_off) {
+ ret = devm_register_power_off_handler(&client->dev, &tps6586x_power_off_handler,
+ NULL);
+ if (ret) {
+ dev_err(&client->dev, "register power off handler failed: %d\n", ret);
+ goto err_add_devs;
+ }
}
return 0;
--
2.34.1
From: Benjamin Bara <[email protected]>
There are a couple of boards which use a tps6586x as
"ti,system-power-controller", e.g. the tegra20-tamonten.dtsi.
For these, the only registered restart handler is the warm reboot via
tegra's PMC. As the bootloader of the tegra20 requires the VDE, it must
be ensured that VDE is enabled (which is the case after a cold reboot).
For the "normal reboot", this is basically the case since 8f0c714ad9be.
However, this workaround is not executed in case of an emergency restart.
In case of an emergency restart, the system now simply hangs in the
bootloader, as VDE is not enabled (because it is not used).
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,
which takes at least 10ms (as the data sheet states).
This avoids the hang-up.
Tested on a TPS658640.
Signed-off-by: Benjamin Bara <[email protected]>
---
drivers/mfd/tps6586x.c | 26 ++++++++++++++++++++++++++
1 file changed, 26 insertions(+)
diff --git a/drivers/mfd/tps6586x.c b/drivers/mfd/tps6586x.c
index 226e856e34e0..f7665b368071 100644
--- a/drivers/mfd/tps6586x.c
+++ b/drivers/mfd/tps6586x.c
@@ -30,6 +30,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)
@@ -475,6 +476,24 @@ static int tps6586x_power_off_handler(struct sys_off_data *data)
return notifier_from_errno(-ETIME);
}
+static int tps6586x_restart_handler(struct sys_off_data *data)
+{
+ int ret;
+
+ /* tps6586x only provides a hard/cold reboot, skip others. */
+ if (data->mode != REBOOT_UNDEFINED && data->mode != REBOOT_COLD &&
+ data->mode != REBOOT_HARD)
+ return NOTIFY_DONE;
+
+ /* bring pmic into HARD REBOOT state. this takes at least 10ms. */
+ ret = tps6586x_set_bits(data->dev, TPS6586X_SUPPLYENE, SOFT_RST_BIT);
+ if (ret)
+ return notifier_from_errno(ret);
+
+ mdelay(20);
+ return notifier_from_errno(-ETIME);
+}
+
static void tps6586x_print_version(struct i2c_client *client, int version)
{
const char *name;
@@ -575,6 +594,13 @@ static int tps6586x_i2c_probe(struct i2c_client *client)
dev_err(&client->dev, "register power off handler failed: %d\n", ret);
goto err_add_devs;
}
+
+ ret = devm_register_restart_handler(&client->dev, &tps6586x_restart_handler,
+ NULL);
+ if (ret) {
+ dev_err(&client->dev, "register restart handler failed: %d\n", ret);
+ goto err_add_devs;
+ }
}
return 0;
--
2.34.1
On Tue, 18 Apr 2023, Benjamin Bara wrote:
> From: Benjamin Bara <[email protected]>
>
> Convert the power off handler to a devm-based power off handler.
>
> Signed-off-by: Benjamin Bara <[email protected]>
> ---
> drivers/mfd/tps6586x.c | 29 +++++++++++++++++++++--------
> 1 file changed, 21 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/mfd/tps6586x.c b/drivers/mfd/tps6586x.c
> index 2d947f3f606a..226e856e34e0 100644
> --- a/drivers/mfd/tps6586x.c
> +++ b/drivers/mfd/tps6586x.c
> @@ -22,6 +22,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>
>
> @@ -457,13 +458,21 @@ static const struct regmap_config tps6586x_regmap_config = {
> .cache_type = REGCACHE_RBTREE,
> };
>
> -static struct device *tps6586x_dev;
> -static void tps6586x_power_off(void)
> +static int tps6586x_power_off_handler(struct sys_off_data *data)
> {
> - if (tps6586x_clr_bits(tps6586x_dev, TPS6586X_SUPPLYENE, EXITSLREQ_BIT))
> - return;
> + int ret;
> +
> + /* bring pmic into SLEEP state. this takes at least 10ms. */
"Put the PMIC into a sleep state. This takes at least 10sm."
> + ret = tps6586x_clr_bits(data->dev, TPS6586X_SUPPLYENE, EXITSLREQ_BIT);
> + if (ret)
> + return notifier_from_errno(ret);
> +
> + ret = tps6586x_set_bits(data->dev, TPS6586X_SUPPLYENE, SLEEP_MODE_BIT);
> + if (ret)
> + return notifier_from_errno(ret);
>
> - tps6586x_set_bits(tps6586x_dev, TPS6586X_SUPPLYENE, SLEEP_MODE_BIT);
> + mdelay(50);
> + return notifier_from_errno(-ETIME);
> }
>
> static void tps6586x_print_version(struct i2c_client *client, int version)
> @@ -559,9 +568,13 @@ static int tps6586x_i2c_probe(struct i2c_client *client)
> goto err_add_devs;
> }
>
> - if (pdata->pm_off && !pm_power_off) {
> - tps6586x_dev = &client->dev;
> - pm_power_off = tps6586x_power_off;
> + if (pdata->pm_off) {
> + ret = devm_register_power_off_handler(&client->dev, &tps6586x_power_off_handler,
> + NULL);
> + if (ret) {
> + dev_err(&client->dev, "register power off handler failed: %d\n", ret);
> + goto err_add_devs;
> + }
> }
>
> return 0;
>
> --
> 2.34.1
>
--
Lee Jones [李琼斯]
On Tue, 18 Apr 2023, Benjamin Bara wrote:
> From: Benjamin Bara <[email protected]>
>
> There are a couple of boards which use a tps6586x as
> "ti,system-power-controller", e.g. the tegra20-tamonten.dtsi.
> For these, the only registered restart handler is the warm reboot via
> tegra's PMC. As the bootloader of the tegra20 requires the VDE, it must
> be ensured that VDE is enabled (which is the case after a cold reboot).
> For the "normal reboot", this is basically the case since 8f0c714ad9be.
> However, this workaround is not executed in case of an emergency restart.
> In case of an emergency restart, the system now simply hangs in the
> bootloader, as VDE is not enabled (because it is not used).
>
> 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,
> which takes at least 10ms (as the data sheet states).
> This avoids the hang-up.
>
> Tested on a TPS658640.
>
> Signed-off-by: Benjamin Bara <[email protected]>
> ---
> drivers/mfd/tps6586x.c | 26 ++++++++++++++++++++++++++
> 1 file changed, 26 insertions(+)
>
> diff --git a/drivers/mfd/tps6586x.c b/drivers/mfd/tps6586x.c
> index 226e856e34e0..f7665b368071 100644
> --- a/drivers/mfd/tps6586x.c
> +++ b/drivers/mfd/tps6586x.c
> @@ -30,6 +30,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)
>
> @@ -475,6 +476,24 @@ static int tps6586x_power_off_handler(struct sys_off_data *data)
> return notifier_from_errno(-ETIME);
> }
>
> +static int tps6586x_restart_handler(struct sys_off_data *data)
> +{
> + int ret;
> +
> + /* tps6586x only provides a hard/cold reboot, skip others. */
TPS6586x
> + if (data->mode != REBOOT_UNDEFINED && data->mode != REBOOT_COLD &&
> + data->mode != REBOOT_HARD)
> + return NOTIFY_DONE;
> +
> + /* bring pmic into HARD REBOOT state. this takes at least 10ms. */
Same as the other patch.
> + ret = tps6586x_set_bits(data->dev, TPS6586X_SUPPLYENE, SOFT_RST_BIT);
> + if (ret)
> + return notifier_from_errno(ret);
> +
> + mdelay(20);
Why 20 here and 50 in the other patch?
> + return notifier_from_errno(-ETIME);
> +}
> +
> static void tps6586x_print_version(struct i2c_client *client, int version)
> {
> const char *name;
> @@ -575,6 +594,13 @@ static int tps6586x_i2c_probe(struct i2c_client *client)
> dev_err(&client->dev, "register power off handler failed: %d\n", ret);
> goto err_add_devs;
> }
> +
> + ret = devm_register_restart_handler(&client->dev, &tps6586x_restart_handler,
> + NULL);
> + if (ret) {
> + dev_err(&client->dev, "register restart handler failed: %d\n", ret);
> + goto err_add_devs;
> + }
> }
>
> return 0;
>
> --
> 2.34.1
>
--
Lee Jones [李琼斯]
Thanks for the feedback!
On Thu, 20 Apr 2023 at 16:04, Lee Jones <[email protected]> wrote:
> Why 20 here and 50 in the other patch?
The data sheet states:
The device will enter the SLEEP or HARD REBOOT state 10ms after the
SLEEP REQUEST or REBOOT REQUEST is initiated.
Also:
When the reboot request state is set an internal timer TWAIT (10ms typ)
is started (...). The reboot request ends when t > TWAIT.
But in the electrical characteristics, TWAIT is given as min 18, typ 20,
max 22.
In my observations, reboot took like typ 15ms and sleep typ 25ms, but
this might be very board-specific. I can set both to 50ms to be "on the
safe side" and have a common value?
Thanks and best regards,
Benjamin
On Thu, 20 Apr 2023, Benjamin Bara wrote:
> Thanks for the feedback!
>
> On Thu, 20 Apr 2023 at 16:04, Lee Jones <[email protected]> wrote:
> > Why 20 here and 50 in the other patch?
>
> The data sheet states:
> The device will enter the SLEEP or HARD REBOOT state 10ms after the
> SLEEP REQUEST or REBOOT REQUEST is initiated.
>
> Also:
> When the reboot request state is set an internal timer TWAIT (10ms typ)
> is started (...). The reboot request ends when t > TWAIT.
>
> But in the electrical characteristics, TWAIT is given as min 18, typ 20,
> max 22.
>
> In my observations, reboot took like typ 15ms and sleep typ 25ms, but
> this might be very board-specific. I can set both to 50ms to be "on the
> safe side" and have a common value?
The confusing part for me, the reader, was that both say "will take at
least 10ms" or words to that effect, but they sleep for a different
amount of time.
--
Lee Jones [李琼斯]
On Fri, 21 Apr 2023 at 09:25, Lee Jones <[email protected]> wrote:
> The confusing part for me, the reader, was that both say "will take at
> least 10ms" or words to that effect, but they sleep for a different
> amount of time.
Yep, got it. For the next version, I will rewrite to:
"Put the PMIC into * state. Data sheet states that this takes at least 20ms.
As this is board-specific, add some buffer to prevent the timeout error."
and change to -ETIMEDOUT instead of -ETIME.
Thanks!
On Fri, 21 Apr 2023, Benjamin Bara wrote:
> On Fri, 21 Apr 2023 at 09:25, Lee Jones <[email protected]> wrote:
> > The confusing part for me, the reader, was that both say "will take at
> > least 10ms" or words to that effect, but they sleep for a different
> > amount of time.
>
> Yep, got it. For the next version, I will rewrite to:
> "Put the PMIC into * state. Data sheet states that this takes at least 20ms.
> As this is board-specific, add some buffer to prevent the timeout error."
> and change to -ETIMEDOUT instead of -ETIME.
No need to be more verbose.
Stick to the one liner, just be more clear about the timings.
The reader can see that you've added a little buffer without explanation.
--
Lee Jones [李琼斯]