| Message ID | 20230327-tegra-pmic-reboot-v6-5-af44a4cd82e9@skidata.com |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | mfd: tps6586x: register restart handler | expand |
On Tue, 09 May 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 provides a SOFT RST bit in the SUPPLYENE reg to request > a (cold) reboot, which takes at least 20ms (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(+) I plan to apply the whole set once you have all required Acks. For my own reference (apply this as-is to your sign-off block): Acked-for-MFD-by: Lee Jones <lee@kernel.org>
On 5/9/23 22:03, 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 provides a SOFT RST bit in the SUPPLYENE reg to request > a (cold) reboot, which takes at least 20ms (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 b12c9e18970a..3b8faa058e59 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; Not sure whether it's worthwhile to care about the reboot mode. If we would really care, then the supported modes should be a part of sys-off handler definition. Maybe Rafael could comment on it. Otherwise looks good. Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
On 5/18/23 14:48, Dmitry Osipenko wrote: > On 5/9/23 22:03, 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 provides a SOFT RST bit in the SUPPLYENE reg to request >> a (cold) reboot, which takes at least 20ms (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 b12c9e18970a..3b8faa058e59 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; > > Not sure whether it's worthwhile to care about the reboot mode. If we > would really care, then the supported modes should be a part of sys-off > handler definition. Maybe Rafael could comment on it. > > Otherwise looks good. > > Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> Benjamin, could you please add acks and review tags to the commits and send v7? I'd also suggest to drop the "data->mode" checks unless there is a good practical reason to keep them. There are no other drivers in kernel that do such checks.
diff --git a/drivers/mfd/tps6586x.c b/drivers/mfd/tps6586x.c index b12c9e18970a..3b8faa058e59 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; + + /* Put the PMIC into hard reboot state. This takes at least 20ms. */ + ret = tps6586x_set_bits(data->dev, TPS6586X_SUPPLYENE, SOFT_RST_BIT); + if (ret) + return notifier_from_errno(ret); + + mdelay(50); + 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;