[1/2] mfd: rk808: add reboot support to rk808 pmic

Commit Message

Frank Wunderlich Dec. 15, 2021, 9:32 p.m. UTC

From: Peter Geis <[e-mail]>

This adds reboot support to the rk808 pmic. This only enables if the
rockchip,system-power-controller flag is set.

Signed-off-by: Peter Geis <[e-mail]>
---
 drivers/mfd/rk808.c       | 48 +++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/rk808.h |  2 ++
 2 files changed, 50 insertions(+)

Comments

Nicolas Frattaroli Dec. 15, 2021, 11:07 p.m. UTC | #1

On Mittwoch, 15. Dezember 2021 22:32:59 CET Frank Wunderlich wrote:
> From: Peter Geis <[e-mail]>
> 
> This adds reboot support to the rk808 pmic. This only enables if the
> rockchip,system-power-controller flag is set.
> 
> Signed-off-by: Peter Geis <[e-mail]>
> ---
>  drivers/mfd/rk808.c       | 48 +++++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/rk808.h |  2 ++
>  2 files changed, 50 insertions(+)
> 

Tested-by: Nicolas Frattaroli <[e-mail]>

Tested with a RK817, using a Quartz64 Model A with the patch applied
on top of v5.16-rc5. Poweroff works, reboot works.

Peter Geis Dec. 15, 2021, 11:52 p.m. UTC | #2

On Wed, Dec 15, 2021 at 6:07 PM Nicolas Frattaroli
<[e-mail]> wrote:
>
> On Mittwoch, 15. Dezember 2021 22:32:59 CET Frank Wunderlich wrote:
> > From: Peter Geis <[e-mail]>
> >
> > This adds reboot support to the rk808 pmic. This only enables if the
> > rockchip,system-power-controller flag is set.
> >
> > Signed-off-by: Peter Geis <[e-mail]>
> > ---
> >  drivers/mfd/rk808.c       | 48 +++++++++++++++++++++++++++++++++++++++
> >  include/linux/mfd/rk808.h |  2 ++
> >  2 files changed, 50 insertions(+)
> >
>
> Tested-by: Nicolas Frattaroli <[e-mail]>
>
> Tested with a RK817, using a Quartz64 Model A with the patch applied
> on top of v5.16-rc5. Poweroff works, reboot works.
>
>

As a note, this has come up due to rk356x having unreliable psci reboot.
Until we have mainline atf sources so we can fix the problem this is
the only way to reliably reset these devices.

Robin Murphy Dec. 16, 2021, 1:17 p.m. UTC | #3

On 2021-12-15 21:32, Frank Wunderlich wrote:
> From: Peter Geis <[e-mail]>
> 
> This adds reboot support to the rk808 pmic. This only enables if the
> rockchip,system-power-controller flag is set.
> 
> Signed-off-by: Peter Geis <[e-mail]>
> ---
>   drivers/mfd/rk808.c       | 48 +++++++++++++++++++++++++++++++++++++++
>   include/linux/mfd/rk808.h |  2 ++
>   2 files changed, 50 insertions(+)
> 
> diff --git a/drivers/mfd/rk808.c b/drivers/mfd/rk808.c
> index b181fe401330..afbd7e01df50 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;
> @@ -533,6 +534,7 @@ static void rk808_pm_power_off(void)
>   	int ret;
>   	unsigned int reg, bit;
>   	struct rk808 *rk808 = i2c_get_clientdata(rk808_i2c_client);
> +	dev_err(&rk808_i2c_client->dev, "poweroff device!\n");

This is not an error.

>   
>   	switch (rk808->variant) {
>   	case RK805_ID:
> @@ -552,6 +554,7 @@ static void rk808_pm_power_off(void)
>   		bit = DEV_OFF;
>   		break;
>   	default:
> +		dev_err(&rk808_i2c_client->dev, "poweroff device not supported!\n");

If we really don't support power off for the present PMIC then we should 
avoid registering the power off handler in the first place. These 
default cases should mostly just serve to keep static checkers happy.

>   		return;
>   	}
>   	ret = regmap_update_bits(rk808->regmap, reg, bit, bit);
> @@ -559,6 +562,44 @@ 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 RK805_ID:
> +		reg = RK805_DEV_CTRL_REG;
> +		bit = DEV_OFF_RST;
> +		break;
> +	case RK808_ID:
> +		reg = RK808_DEVCTRL_REG,
> +		bit = DEV_OFF;

Is that right? The RK808 datasheet does say that this bit resets itself 
once the OFF state is reached, but there doesn't seem to be any obvious 
guarantee of a power-on condition being present to automatically 
transition back to ACTIVE.

> +		break;
> +	case RK817_ID:
> +		reg = RK817_SYS_CFG(3);
> +		bit = DEV_RST;
> +		break;
> +	case RK818_ID:
> +		reg = RK818_DEVCTRL_REG;
> +		bit = DEV_OFF_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 +768,13 @@ 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;
> +		rk808->nb = &rk808_restart_handler;

The handler just relies on the global rk808_i2c_client anyway, so does 
this serve any purpose?

> +
> +		dev_warn(&client->dev, "register restart handler\n");

Don't scream a warning about normal and expected operation.

Thanks,
Robin.

> +
> +		ret = register_restart_handler(rk808->nb);
> +		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..5dfe0c4ceab1 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)
>   
> @@ -701,5 +702,6 @@ struct rk808 {
>   	long				variant;
>   	const struct regmap_config	*regmap_cfg;
>   	const struct regmap_irq_chip	*regmap_irq_chip;
> +	struct notifier_block		*nb;
>   };
>   #endif /* __LINUX_REGULATOR_RK808_H */

Frank Wunderlich Dec. 16, 2021, 4:36 p.m. UTC | #4

Hi Robin

thanks for your review

> Gesendet: Donnerstag, 16. Dezember 2021 um 14:17 Uhr
> Von: "Robin Murphy" <[e-mail]>

> > +	dev_err(&rk808_i2c_client->dev, "poweroff device!\n");
>
> This is not an error.

ack, we can change to dev_dbg/dev_info or drop completely

> > @@ -552,6 +554,7 @@ static void rk808_pm_power_off(void)
> >   		bit = DEV_OFF;
> >   		break;
> >   	default:
> > +		dev_err(&rk808_i2c_client->dev, "poweroff device not supported!\n");
>
> If we really don't support power off for the present PMIC then we should
> avoid registering the power off handler in the first place. These
> default cases should mostly just serve to keep static checkers happy.

currently we had added all device-ids supported by probe. Before my Patch (Part2) rk809 was missing.
It is not registered for (currently) unsupported IDs as probe exits with -EINVAL for unsupported IDs before.

If anyone adds init-code in future but not poweroff/restart part we ran into this. So i would leave this message.

registering handler depending on supported devices (for poweroff/restart function) imho makes code less readable.

> > @@ -559,6 +562,44 @@ 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 RK805_ID:
> > +		reg = RK805_DEV_CTRL_REG;
> > +		bit = DEV_OFF_RST;
> > +		break;
> > +	case RK808_ID:
> > +		reg = RK808_DEVCTRL_REG,
> > +		bit = DEV_OFF;
>
> Is that right? The RK808 datasheet does say that this bit resets itself
> once the OFF state is reached, but there doesn't seem to be any obvious
> guarantee of a power-on condition being present to automatically
> transition back to ACTIVE.

i think you're right...imho it should be DEV_OFF_RST in restart_notify and DEV_OFF in poweroff

> > @@ -727,6 +768,13 @@ 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;
> > +		rk808->nb = &rk808_restart_handler;
>
> The handler just relies on the global rk808_i2c_client anyway, so does
> this serve any purpose?

i guess

ret = register_restart_handler(&rk808_restart_handler);

is better here too.

i cannot figure out why storing the function-pointer in rk808->nb too as this seems not to be used anywhere else

> > +
> > +		dev_warn(&client->dev, "register restart handler\n");
>
> Don't scream a warning about normal and expected operation.

ack

> Thanks,
> Robin.

@Peter, what do you think?

regards Frank

Peter Geis Dec. 17, 2021, 2:21 a.m. UTC | #5

On Thu, Dec 16, 2021 at 11:36 AM Frank Wunderlich
<[e-mail]> wrote:
>
> Hi Robin
>
> thanks for your review

Good Evening,

>
> > Gesendet: Donnerstag, 16. Dezember 2021 um 14:17 Uhr
> > Von: "Robin Murphy" <[e-mail]>
>
> > > +   dev_err(&rk808_i2c_client->dev, "poweroff device!\n");
> >
> > This is not an error.
>
> ack, we can change to dev_dbg/dev_info or drop completely

Yes, this was set to isolate down when individuals were having issues
on a specific distro that unfortunately defaults to a silent boot.
It can be dropped at this point.

>
> > > @@ -552,6 +554,7 @@ static void rk808_pm_power_off(void)
> > >             bit = DEV_OFF;
> > >             break;
> > >     default:
> > > +           dev_err(&rk808_i2c_client->dev, "poweroff device not supported!\n");
> >
> > If we really don't support power off for the present PMIC then we should
> > avoid registering the power off handler in the first place. These
> > default cases should mostly just serve to keep static checkers happy.
>
> currently we had added all device-ids supported by probe. Before my Patch (Part2) rk809 was missing.
> It is not registered for (currently) unsupported IDs as probe exits with -EINVAL for unsupported IDs before.
>
> If anyone adds init-code in future but not poweroff/restart part we ran into this. So i would leave this message.
>
> registering handler depending on supported devices (for poweroff/restart function) imho makes code less readable.

I added that just in case someone put in support for a new device
without checking everything.
I don't have any particular attachment to it however.

>
> > > @@ -559,6 +562,44 @@ 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 RK805_ID:
> > > +           reg = RK805_DEV_CTRL_REG;
> > > +           bit = DEV_OFF_RST;
> > > +           break;
> > > +   case RK808_ID:
> > > +           reg = RK808_DEVCTRL_REG,
> > > +           bit = DEV_OFF;
> >
> > Is that right? The RK808 datasheet does say that this bit resets itself
> > once the OFF state is reached, but there doesn't seem to be any obvious
> > guarantee of a power-on condition being present to automatically
> > transition back to ACTIVE.
>
> i think you're right...imho it should be DEV_OFF_RST in restart_notify and DEV_OFF in poweroff

I've tested this on the rockpro64, DEV_OFF_RST and DEV_OFF both power
down the rk808.
The difference is the DEV_OFF_RST "activate reset of the digital core".
This is similar to the description of pressing the RESET key, with the
exception of not powering up the PMIC.
It seems the rk808 doesn't support software resetting at all per the trm.
We should drop reset support for the rk808.
The only way I can see triggering a reset in the rk808 being possible
is by setting the RTC alarm to wake the device shortly after powering
down.

>
> > > @@ -727,6 +768,13 @@ 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;
> > > +           rk808->nb = &rk808_restart_handler;
> >
> > The handler just relies on the global rk808_i2c_client anyway, so does
> > this serve any purpose?
>
> i guess
>
> ret = register_restart_handler(&rk808_restart_handler);
>
> is better here too.
>
> i cannot figure out why storing the function-pointer in rk808->nb too as this seems not to be used anywhere else

I can't remember why I set this up like this, probably "borrowed" from
another driver somewhere.
Go ahead and change it.

>
> > > +
> > > +           dev_warn(&client->dev, "register restart handler\n");
> >
> > Don't scream a warning about normal and expected operation.
>
> ack

This was to check that it was in fact registering, drop it.

>
> > Thanks,
> > Robin.
>
> @Peter, what do you think?
>
> regards Frank

Thank you for the review Robin and the submission Frank.

Always,
Peter

Patch

diff --git a/drivers/mfd/rk808.c b/drivers/mfd/rk808.c
index b181fe401330..afbd7e01df50 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;
@@ -533,6 +534,7 @@  static void rk808_pm_power_off(void)
 	int ret;
 	unsigned int reg, bit;
 	struct rk808 *rk808 = i2c_get_clientdata(rk808_i2c_client);
+	dev_err(&rk808_i2c_client->dev, "poweroff device!\n");
 
 	switch (rk808->variant) {
 	case RK805_ID:
@@ -552,6 +554,7 @@  static void rk808_pm_power_off(void)
 		bit = DEV_OFF;
 		break;
 	default:
+		dev_err(&rk808_i2c_client->dev, "poweroff device not supported!\n");
 		return;
 	}
 	ret = regmap_update_bits(rk808->regmap, reg, bit, bit);
@@ -559,6 +562,44 @@  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 RK805_ID:
+		reg = RK805_DEV_CTRL_REG;
+		bit = DEV_OFF_RST;
+		break;
+	case RK808_ID:
+		reg = RK808_DEVCTRL_REG,
+		bit = DEV_OFF;
+		break;
+	case RK817_ID:
+		reg = RK817_SYS_CFG(3);
+		bit = DEV_RST;
+		break;
+	case RK818_ID:
+		reg = RK818_DEVCTRL_REG;
+		bit = DEV_OFF_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 +768,13 @@  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;
+		rk808->nb = &rk808_restart_handler;
+
+		dev_warn(&client->dev, "register restart handler\n");
+
+		ret = register_restart_handler(rk808->nb);
+		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..5dfe0c4ceab1 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)
 
@@ -701,5 +702,6 @@  struct rk808 {
 	long				variant;
 	const struct regmap_config	*regmap_cfg;
 	const struct regmap_irq_chip	*regmap_irq_chip;
+	struct notifier_block		*nb;
 };
 #endif /* __LINUX_REGULATOR_RK808_H */