linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] mfd: rk808: add reboot support to rk808.c
@ 2021-12-17 14:55 Peter Geis
  2021-12-17 15:29 ` Dmitry Osipenko
  0 siblings, 1 reply; 6+ messages in thread
From: Peter Geis @ 2021-12-17 14:55 UTC (permalink / raw)
  To: Lee Jones
  Cc: Peter Geis, Robin Murphy, Heiko Stuebner, linux-rockchip,
	Nicolas Frattaroli, Frank Wunderlich, linux-kernel

This adds reboot support to the rk808o pmic driver and enables it for
the rk809 and rk817 devices.
This only enables if the rockchip,system-power-controller flag is set.

Signed-off-by: Peter Geis <pgwipeout@gmail.com>
Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
---
This patch was created to address issues with psci-reset on rk356x
chips. Until the rk356x series ATF open source code is released so we
can fix the issue at the source, this is the only way to ensure reliable
resetting on devices using these chips.

After testing the rk808 (thanks Robin!), it was found DEV_OFF_RST does
not reset the PMIC to a power on state. Since the rk805 and rk818 match
this register layout, I'm removing support for all three in the v2.
It may be possible to add support to them using an RTC wakeup, but that
has not been explored and is outside the scope of this patch.

Changelog:
V2:
- Squash the patch from Frank Wunderlich for rk809 support.
- Remove support for the rk805, rk808, and rk818 devices.
- Only register the reset handler for supported devices.
- Remove unnecessary dev_err and dev_warn statements.
- Register the reset handler directly

 drivers/mfd/rk808.c       | 43 +++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/rk808.h |  1 +
 2 files changed, 44 insertions(+)

diff --git a/drivers/mfd/rk808.c b/drivers/mfd/rk808.c
index b181fe401330..6fa7fbf9b695 100644
--- a/drivers/mfd/rk808.c
+++ b/drivers/mfd/rk808.c
@@ -19,6 +19,7 @@
 #include <linux/module.h>
 #include <linux/of_device.h>
 #include <linux/regmap.h>
+#include <linux/reboot.h>
 
 struct rk808_reg_data {
 	int addr;
@@ -543,6 +544,7 @@ static void rk808_pm_power_off(void)
 		reg = RK808_DEVCTRL_REG,
 		bit = DEV_OFF_RST;
 		break;
+	case RK809_ID:
 	case RK817_ID:
 		reg = RK817_SYS_CFG(3);
 		bit = DEV_OFF;
@@ -559,6 +561,34 @@ static void rk808_pm_power_off(void)
 		dev_err(&rk808_i2c_client->dev, "Failed to shutdown device!\n");
 }
 
+static int rk808_restart_notify(struct notifier_block *this, unsigned long mode, void *cmd)
+{
+	int ret;
+	unsigned int reg, bit;
+	struct rk808 *rk808 = i2c_get_clientdata(rk808_i2c_client);
+
+	switch (rk808->variant) {
+	case RK809_ID:
+	case RK817_ID:
+		reg = RK817_SYS_CFG(3);
+		bit = DEV_RST;
+		break;
+
+	default:
+		return NOTIFY_DONE;
+	}
+	ret = regmap_update_bits(rk808->regmap, reg, bit, bit);
+	if (ret)
+		dev_err(&rk808_i2c_client->dev, "Failed to restart device!\n");
+
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block rk808_restart_handler = {
+	.notifier_call = rk808_restart_notify,
+	.priority = 255,
+};
+
 static void rk8xx_shutdown(struct i2c_client *client)
 {
 	struct rk808 *rk808 = i2c_get_clientdata(client);
@@ -727,6 +757,19 @@ static int rk808_probe(struct i2c_client *client,
 	if (of_property_read_bool(np, "rockchip,system-power-controller")) {
 		rk808_i2c_client = client;
 		pm_power_off = rk808_pm_power_off;
+
+		switch (rk808->variant) {
+		case RK809_ID:
+		case RK817_ID:
+			ret = register_restart_handler(&rk808_restart_handler);
+			break;
+		default:
+			dev_info(&client->dev, "pmic controlled board reset not supported\n");
+			break;
+		}
+
+		if (ret)
+			dev_err(&client->dev, "failed to register restart handler, %d\n", ret);
 	}
 
 	return 0;
diff --git a/include/linux/mfd/rk808.h b/include/linux/mfd/rk808.h
index a96e6d43ca06..58602032e642 100644
--- a/include/linux/mfd/rk808.h
+++ b/include/linux/mfd/rk808.h
@@ -373,6 +373,7 @@ enum rk805_reg {
 #define SWITCH2_EN	BIT(6)
 #define SWITCH1_EN	BIT(5)
 #define DEV_OFF_RST	BIT(3)
+#define DEV_RST		BIT(2)
 #define DEV_OFF		BIT(0)
 #define RTC_STOP	BIT(0)
 
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] mfd: rk808: add reboot support to rk808.c
  2021-12-17 14:55 [PATCH v2] mfd: rk808: add reboot support to rk808.c Peter Geis
@ 2021-12-17 15:29 ` Dmitry Osipenko
  2021-12-17 18:16   ` Peter Geis
  0 siblings, 1 reply; 6+ messages in thread
From: Dmitry Osipenko @ 2021-12-17 15:29 UTC (permalink / raw)
  To: Peter Geis, Lee Jones
  Cc: Robin Murphy, Heiko Stuebner, linux-rockchip, Nicolas Frattaroli,
	Frank Wunderlich, linux-kernel

17.12.2021 17:55, Peter Geis пишет:
> This adds reboot support to the rk808o pmic driver and enables it for
> the rk809 and rk817 devices.
> This only enables if the rockchip,system-power-controller flag is set.
> 
> Signed-off-by: Peter Geis <pgwipeout@gmail.com>
> Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
> ---
> This patch was created to address issues with psci-reset on rk356x
> chips. Until the rk356x series ATF open source code is released so we
> can fix the issue at the source, this is the only way to ensure reliable
> resetting on devices using these chips.
> 
> After testing the rk808 (thanks Robin!), it was found DEV_OFF_RST does
> not reset the PMIC to a power on state. Since the rk805 and rk818 match
> this register layout, I'm removing support for all three in the v2.
> It may be possible to add support to them using an RTC wakeup, but that
> has not been explored and is outside the scope of this patch.
> 
> Changelog:
> V2:
> - Squash the patch from Frank Wunderlich for rk809 support.
> - Remove support for the rk805, rk808, and rk818 devices.
> - Only register the reset handler for supported devices.
> - Remove unnecessary dev_err and dev_warn statements.
> - Register the reset handler directly
> 
>  drivers/mfd/rk808.c       | 43 +++++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/rk808.h |  1 +
>  2 files changed, 44 insertions(+)
...
> +static struct notifier_block rk808_restart_handler = {
> +	.notifier_call = rk808_restart_notify,
> +	.priority = 255,
> +};

Hello!

Please use the default 128 priority if there are no other conflicting
handlers on this RK.

>  static void rk8xx_shutdown(struct i2c_client *client)
>  {
>  	struct rk808 *rk808 = i2c_get_clientdata(client);
> @@ -727,6 +757,19 @@ static int rk808_probe(struct i2c_client *client,
>  	if (of_property_read_bool(np, "rockchip,system-power-controller")) {
>  		rk808_i2c_client = client;
>  		pm_power_off = rk808_pm_power_off;
> +
> +		switch (rk808->variant) {
> +		case RK809_ID:
> +		case RK817_ID:
> +			ret = register_restart_handler(&rk808_restart_handler);

There is no corresponding unregister_restart_handler(), which you should
add to rk808_remove(). Otherwise kernel will crash on reboot if you'll
unload this driver module.

> +			break;
> +		default:
> +			dev_info(&client->dev, "pmic controlled board reset not supported\n");

I'd set ret=0 explicitly here. Later on somebody may change the code and
ret won't be zero anymore, this is not an uncommon trouble in kernel.

> +			break;
> +		}
> +
> +		if (ret)
> +			dev_err(&client->dev, "failed to register restart handler, %d\n", ret);




^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] mfd: rk808: add reboot support to rk808.c
  2021-12-17 15:29 ` Dmitry Osipenko
@ 2021-12-17 18:16   ` Peter Geis
  2021-12-17 18:30     ` Dmitry Osipenko
  0 siblings, 1 reply; 6+ messages in thread
From: Peter Geis @ 2021-12-17 18:16 UTC (permalink / raw)
  To: Dmitry Osipenko
  Cc: Lee Jones, Robin Murphy, Heiko Stuebner,
	open list:ARM/Rockchip SoC...,
	Nicolas Frattaroli, Frank Wunderlich, Linux Kernel Mailing List

On Fri, Dec 17, 2021 at 10:29 AM Dmitry Osipenko <digetx@gmail.com> wrote:
>
> 17.12.2021 17:55, Peter Geis пишет:
> > This adds reboot support to the rk808o pmic driver and enables it for
> > the rk809 and rk817 devices.
> > This only enables if the rockchip,system-power-controller flag is set.
> >
> > Signed-off-by: Peter Geis <pgwipeout@gmail.com>
> > Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
> > ---
> > This patch was created to address issues with psci-reset on rk356x
> > chips. Until the rk356x series ATF open source code is released so we
> > can fix the issue at the source, this is the only way to ensure reliable
> > resetting on devices using these chips.
> >
> > After testing the rk808 (thanks Robin!), it was found DEV_OFF_RST does
> > not reset the PMIC to a power on state. Since the rk805 and rk818 match
> > this register layout, I'm removing support for all three in the v2.
> > It may be possible to add support to them using an RTC wakeup, but that
> > has not been explored and is outside the scope of this patch.
> >
> > Changelog:
> > V2:
> > - Squash the patch from Frank Wunderlich for rk809 support.
> > - Remove support for the rk805, rk808, and rk818 devices.
> > - Only register the reset handler for supported devices.
> > - Remove unnecessary dev_err and dev_warn statements.
> > - Register the reset handler directly
> >
> >  drivers/mfd/rk808.c       | 43 +++++++++++++++++++++++++++++++++++++++
> >  include/linux/mfd/rk808.h |  1 +
> >  2 files changed, 44 insertions(+)
> ...
> > +static struct notifier_block rk808_restart_handler = {
> > +     .notifier_call = rk808_restart_notify,
> > +     .priority = 255,
> > +};
>
> Hello!
>
> Please use the default 128 priority if there are no other conflicting
> handlers on this RK.

Unfortunately the psci-reboot handler is set to 129, I'll adjust this
to 192 which is in line with other PMIC reboot drivers.

>
> >  static void rk8xx_shutdown(struct i2c_client *client)
> >  {
> >       struct rk808 *rk808 = i2c_get_clientdata(client);
> > @@ -727,6 +757,19 @@ static int rk808_probe(struct i2c_client *client,
> >       if (of_property_read_bool(np, "rockchip,system-power-controller")) {
> >               rk808_i2c_client = client;
> >               pm_power_off = rk808_pm_power_off;
> > +
> > +             switch (rk808->variant) {
> > +             case RK809_ID:
> > +             case RK817_ID:
> > +                     ret = register_restart_handler(&rk808_restart_handler);
>
> There is no corresponding unregister_restart_handler(), which you should
> add to rk808_remove(). Otherwise kernel will crash on reboot if you'll
> unload this driver module.

Thanks, added!

>
> > +                     break;
> > +             default:
> > +                     dev_info(&client->dev, "pmic controlled board reset not supported\n");
>
> I'd set ret=0 explicitly here. Later on somebody may change the code and
> ret won't be zero anymore, this is not an uncommon trouble in kernel.

It took me a moment to see the logic here, but I understand it now.

>
> > +                     break;
> > +             }
> > +
> > +             if (ret)
> > +                     dev_err(&client->dev, "failed to register restart handler, %d\n", ret);
>
>
>

Thank you for the review.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] mfd: rk808: add reboot support to rk808.c
  2021-12-17 18:16   ` Peter Geis
@ 2021-12-17 18:30     ` Dmitry Osipenko
  2021-12-17 18:42       ` Peter Geis
  0 siblings, 1 reply; 6+ messages in thread
From: Dmitry Osipenko @ 2021-12-17 18:30 UTC (permalink / raw)
  To: Peter Geis
  Cc: Lee Jones, Robin Murphy, Heiko Stuebner,
	open list:ARM/Rockchip SoC...,
	Nicolas Frattaroli, Frank Wunderlich, Linux Kernel Mailing List

17.12.2021 21:16, Peter Geis пишет:
>>> +                     break;
>>> +             default:
>>> +                     dev_info(&client->dev, "pmic controlled board reset not supported\n");

I'd change this dev_info to dev_dbg to not clutter KMSG.

>> I'd set ret=0 explicitly here. Later on somebody may change the code and
>> ret won't be zero anymore, this is not an uncommon trouble in kernel.
> It took me a moment to see the logic here, but I understand it now.
> 

Could be even better to place the error message simply right after the
register_restart_handler().

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] mfd: rk808: add reboot support to rk808.c
  2021-12-17 18:30     ` Dmitry Osipenko
@ 2021-12-17 18:42       ` Peter Geis
  2021-12-20 11:54         ` Robin Murphy
  0 siblings, 1 reply; 6+ messages in thread
From: Peter Geis @ 2021-12-17 18:42 UTC (permalink / raw)
  To: Dmitry Osipenko
  Cc: Lee Jones, Robin Murphy, Heiko Stuebner,
	open list:ARM/Rockchip SoC...,
	Nicolas Frattaroli, Frank Wunderlich, Linux Kernel Mailing List

On Fri, Dec 17, 2021 at 1:30 PM Dmitry Osipenko <digetx@gmail.com> wrote:
>
> 17.12.2021 21:16, Peter Geis пишет:
> >>> +                     break;
> >>> +             default:
> >>> +                     dev_info(&client->dev, "pmic controlled board reset not supported\n");
>
> I'd change this dev_info to dev_dbg to not clutter KMSG.

I'd prefer to leave this as info, since the device is designated as
the system power controller but it is only capable of powering down
the system, not rebooting it.
But on second thought, anyone who's making these changes would be
investigating the driver anyway.
So I'll change it to dev_dbg.

>
> >> I'd set ret=0 explicitly here. Later on somebody may change the code and
> >> ret won't be zero anymore, this is not an uncommon trouble in kernel.
> > It took me a moment to see the logic here, but I understand it now.
> >
>
> Could be even better to place the error message simply right after the
> register_restart_handler().

Good point, thanks

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] mfd: rk808: add reboot support to rk808.c
  2021-12-17 18:42       ` Peter Geis
@ 2021-12-20 11:54         ` Robin Murphy
  0 siblings, 0 replies; 6+ messages in thread
From: Robin Murphy @ 2021-12-20 11:54 UTC (permalink / raw)
  To: Peter Geis, Dmitry Osipenko
  Cc: Lee Jones, Heiko Stuebner, open list:ARM/Rockchip SoC...,
	Nicolas Frattaroli, Frank Wunderlich, Linux Kernel Mailing List

On 2021-12-17 18:42, Peter Geis wrote:
> On Fri, Dec 17, 2021 at 1:30 PM Dmitry Osipenko <digetx@gmail.com> wrote:
>>
>> 17.12.2021 21:16, Peter Geis пишет:
>>>>> +                     break;
>>>>> +             default:
>>>>> +                     dev_info(&client->dev, "pmic controlled board reset not supported\n");
>>
>> I'd change this dev_info to dev_dbg to not clutter KMSG.
> 
> I'd prefer to leave this as info, since the device is designated as
> the system power controller but it is only capable of powering down
> the system, not rebooting it.
> But on second thought, anyone who's making these changes would be
> investigating the driver anyway.
> So I'll change it to dev_dbg.

Indeed, this is the expected case for RK808, which has to be 
system-power-controller if you want shutdown to actually power off.

Cheers,
Robin.

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-12-20 11:54 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-17 14:55 [PATCH v2] mfd: rk808: add reboot support to rk808.c Peter Geis
2021-12-17 15:29 ` Dmitry Osipenko
2021-12-17 18:16   ` Peter Geis
2021-12-17 18:30     ` Dmitry Osipenko
2021-12-17 18:42       ` Peter Geis
2021-12-20 11:54         ` Robin Murphy

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).