| Message ID | 20230327-tegra-pmic-reboot-v5-6-ab090e03284d@skidata.com |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | mfd: tps6586x: register restart handler | expand |
On Tue, 18 Apr 2023, Benjamin Bara wrote: > From: Benjamin Bara <benjamin.bara@skidata.com> > > 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 <benjamin.bara@skidata.com> > --- > 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 >
Thanks for the feedback!
On Thu, 20 Apr 2023 at 16:04, Lee Jones <lee@kernel.org> 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 <lee@kernel.org> 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.
On Fri, 21 Apr 2023 at 09:25, Lee Jones <lee@kernel.org> 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 <lee@kernel.org> 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.
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;