[PATCH] regulator: rk808: Add support setting suspend voltage

[PATCH] regulator: rk808: Add support setting suspend voltage

[Chris Zhong]

support setting suspend voltage and disable regulator in suspend.

Signed-off-by: Chris Zhong <zyw@rock-chips.com>

---

 drivers/regulator/rk808-regulator.c |   37 +++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/drivers/regulator/rk808-regulator.c b/drivers/regulator/rk808-regulator.c
index e305416..6687e6e 100644
--- a/drivers/regulator/rk808-regulator.c
+++ b/drivers/regulator/rk808-regulator.c
@@ -36,6 +36,12 @@
 #define RK808_RAMP_RATE_6MV_PER_US	(2 << RK808_RAMP_RATE_OFFSET)
 #define RK808_RAMP_RATE_10MV_PER_US	(3 << RK808_RAMP_RATE_OFFSET)
 
+/* Offset from XXX_ON_VSEL to XXX_SLP_VSEL */
+#define RK808_SLP_REG_OFFSET 1
+
+/* Offset from XXX_EN_REG to SLEEP_SET_OFF_XXX */
+#define RK808_SLP_SET_OFF_REG_OFFSET 2
+
 static const int rk808_buck_config_regs[] = {
 	RK808_BUCK1_CONFIG_REG,
 	RK808_BUCK2_CONFIG_REG,
@@ -91,6 +97,32 @@ static int rk808_set_ramp_delay(struct regulator_dev *rdev, int ramp_delay)
 				  RK808_RAMP_RATE_MASK, ramp_value);
 }
 
+int rk808_set_suspend_voltage(struct regulator_dev *rdev, int uv)
+{
+	unsigned int reg;
+	int sel = regulator_map_voltage_linear_range(rdev, uv, uv);
+
+	if (sel < 0)
+		return -EINVAL;
+
+	reg = rdev->desc->vsel_reg + RK808_SLP_REG_OFFSET;
+
+	return regmap_update_bits(rdev->regmap, reg,
+				  rdev->desc->vsel_mask,
+				  sel);
+}
+
+int rk808_set_suspend_disable(struct regulator_dev *rdev)
+{
+	unsigned int reg;
+
+	reg = rdev->desc->enable_reg + RK808_SLP_SET_OFF_REG_OFFSET;
+
+	return regmap_update_bits(rdev->regmap, reg,
+				  rdev->desc->enable_mask,
+				  rdev->desc->enable_mask);
+}
+
 static struct regulator_ops rk808_buck1_2_ops = {
 	.list_voltage		= regulator_list_voltage_linear_range,
 	.map_voltage		= regulator_map_voltage_linear_range,
@@ -100,6 +132,8 @@ static struct regulator_ops rk808_buck1_2_ops = {
 	.disable		= regulator_disable_regmap,
 	.is_enabled		= regulator_is_enabled_regmap,
 	.set_ramp_delay		= rk808_set_ramp_delay,
+	.set_suspend_voltage    = rk808_set_suspend_voltage,
+	.set_suspend_disable    = rk808_set_suspend_disable,
 };
 
 static struct regulator_ops rk808_reg_ops = {
@@ -110,12 +144,15 @@ static struct regulator_ops rk808_reg_ops = {
 	.enable			= regulator_enable_regmap,
 	.disable		= regulator_disable_regmap,
 	.is_enabled		= regulator_is_enabled_regmap,
+	.set_suspend_voltage    = rk808_set_suspend_voltage,
+	.set_suspend_disable    = rk808_set_suspend_disable,
 };
 
 static struct regulator_ops rk808_switch_ops = {
 	.enable = regulator_enable_regmap,
 	.disable = regulator_disable_regmap,
 	.is_enabled = regulator_is_enabled_regmap,
+	.set_suspend_disable    = rk808_set_suspend_disable,
 };
 
 static const struct regulator_desc rk808_reg[] = {
-- 
1.7.9.5

Re: [PATCH] regulator: rk808: Add support setting suspend voltage

[Doug Anderson]

Chris,

On Tue, Oct 7, 2014 at 1:43 AM, Chris Zhong <zyw@rock-chips.com> wrote:
> support setting suspend voltage and disable regulator in suspend.
>
> Signed-off-by: Chris Zhong <zyw@rock-chips.com>
>
> ---
>
>  drivers/regulator/rk808-regulator.c |   37 +++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
>
> diff --git a/drivers/regulator/rk808-regulator.c b/drivers/regulator/rk808-regulator.c
> index e305416..6687e6e 100644
> --- a/drivers/regulator/rk808-regulator.c
> +++ b/drivers/regulator/rk808-regulator.c
> @@ -36,6 +36,12 @@
>  #define RK808_RAMP_RATE_6MV_PER_US     (2 << RK808_RAMP_RATE_OFFSET)
>  #define RK808_RAMP_RATE_10MV_PER_US    (3 << RK808_RAMP_RATE_OFFSET)
>
> +/* Offset from XXX_ON_VSEL to XXX_SLP_VSEL */
> +#define RK808_SLP_REG_OFFSET 1
> +
> +/* Offset from XXX_EN_REG to SLEEP_SET_OFF_XXX */
> +#define RK808_SLP_SET_OFF_REG_OFFSET 2
> +
>  static const int rk808_buck_config_regs[] = {
>         RK808_BUCK1_CONFIG_REG,
>         RK808_BUCK2_CONFIG_REG,
> @@ -91,6 +97,32 @@ static int rk808_set_ramp_delay(struct regulator_dev *rdev, int ramp_delay)
>                                   RK808_RAMP_RATE_MASK, ramp_value);
>  }
>
> +int rk808_set_suspend_voltage(struct regulator_dev *rdev, int uv)
> +{
> +       unsigned int reg;
> +       int sel = regulator_map_voltage_linear_range(rdev, uv, uv);
> +
> +       if (sel < 0)
> +               return -EINVAL;
> +
> +       reg = rdev->desc->vsel_reg + RK808_SLP_REG_OFFSET;
> +
> +       return regmap_update_bits(rdev->regmap, reg,
> +                                 rdev->desc->vsel_mask,
> +                                 sel);
> +}
> +
> +int rk808_set_suspend_disable(struct regulator_dev *rdev)
> +{
> +       unsigned int reg;
> +
> +       reg = rdev->desc->enable_reg + RK808_SLP_SET_OFF_REG_OFFSET;
> +
> +       return regmap_update_bits(rdev->regmap, reg,
> +                                 rdev->desc->enable_mask,
> +                                 rdev->desc->enable_mask);
> +}
> +
>  static struct regulator_ops rk808_buck1_2_ops = {
>         .list_voltage           = regulator_list_voltage_linear_range,
>         .map_voltage            = regulator_map_voltage_linear_range,
> @@ -100,6 +132,8 @@ static struct regulator_ops rk808_buck1_2_ops = {
>         .disable                = regulator_disable_regmap,
>         .is_enabled             = regulator_is_enabled_regmap,
>         .set_ramp_delay         = rk808_set_ramp_delay,
> +       .set_suspend_voltage    = rk808_set_suspend_voltage,
> +       .set_suspend_disable    = rk808_set_suspend_disable,
>  };
>
>  static struct regulator_ops rk808_reg_ops = {
> @@ -110,12 +144,15 @@ static struct regulator_ops rk808_reg_ops = {
>         .enable                 = regulator_enable_regmap,
>         .disable                = regulator_disable_regmap,
>         .is_enabled             = regulator_is_enabled_regmap,
> +       .set_suspend_voltage    = rk808_set_suspend_voltage,
> +       .set_suspend_disable    = rk808_set_suspend_disable,
>  };
>
>  static struct regulator_ops rk808_switch_ops = {
>         .enable = regulator_enable_regmap,
>         .disable = regulator_disable_regmap,
>         .is_enabled = regulator_is_enabled_regmap,
> +       .set_suspend_disable    = rk808_set_suspend_disable,
>  };
>
>  static const struct regulator_desc rk808_reg[] = {

Your patch looks right to me.

Reviewed-by: Doug Anderson <dianders@chromium.org>

I don't have all the right patches to test this right now.  Hopefully
you can point me at what we're using right now.  I'd expect that this
will need the patches that Chanwoo and Javier are working on, so I've
added them to this.

One point of curiosity (maybe this is a question for Chanwoo and
Javier): I'd expect that if someone didn't explicitly setup a "suspend
voltage" that their voltage would just be left alone at suspend time.
I believe that won't be the case for your driver.  The rk808 will (I
think) automatically transition to the "suspend voltage" settings for
ALL regulators at suspend time.  If you didn't explicitly set the
suspend voltage then you'll move to whatever the default voltage is,
right?

-Doug

Re: [PATCH] regulator: rk808: Add support setting suspend voltage

[Javier Martinez Canillas]

Hello Doug,

On 10/07/2014 07:48 PM, Doug Anderson wrote:
> 
> I don't have all the right patches to test this right now.  Hopefully
> you can point me at what we're using right now.  I'd expect that this
> will need the patches that Chanwoo and Javier are working on, so I've
> added them to this.
>

Yes, you will need Chanwoo's series in order to use this. If also the
driver needs to set a default initial regulator operating mode, it will
need the series I'll post today but in that case also a .set_mode handler
is needed.
 
> One point of curiosity (maybe this is a question for Chanwoo and
> Javier): I'd expect that if someone didn't explicitly setup a "suspend
> voltage" that their voltage would just be left alone at suspend time.
> I believe that won't be the case for your driver.  The rk808 will (I
> think) automatically transition to the "suspend voltage" settings for
> ALL regulators at suspend time.  If you didn't explicitly set the
> suspend voltage then you'll move to whatever the default voltage is,
> right?
> 

On system suspend, the regulator core only calls the .set_suspend_voltage
handler if struct regulator_state *rstate->uV > 0 for the given suspend
state (mem,disk,standby) and only calls .set_suspend_{enable,disable} if
struct regulator_state *rstate->{enabled,disable} are set for that state.

Chanwoo's series have different DT properties that are used to set the
suspend state uV (regulator-volt) and disabled (regulator-off-in-suspend):

	ldoX_reg: LDOx {
		...
		regulator-state-mem {
			regulator-volt = <1200000>;
			regulator-off-in-suspend;
		};
		...
	};

So answering your question, the transition to the "suspend voltage" setting
will only happen if uV is set (regulator-volt), otherwise it will be left
alone at suspend time just like is left alone on disable.

However, Chanwoo said that he will drop the regulator-volt DT property from
v5 [0]. It seems that is a useful use case for others to set the regulator
voltage on suspend, so maybe Chanwoo can address the issues pointed out by
Mark about renaming it to regulator-uV [1] and revise the check for the
voltage range [2] but kept the DT property on v5?

> -Doug
> 

Thanks a lot and best regards,
Javier

[0]: https://lkml.org/lkml/2014/9/28/202
[1]: https://lkml.org/lkml/2014/9/4/652
[2]: https://lkml.org/lkml/2014/9/4/651

Re: [PATCH] regulator: rk808: Add support setting suspend voltage

[Doug Anderson]

Javier,

On Wed, Oct 8, 2014 at 1:46 AM, Javier Martinez Canillas
<javier.martinez@collabora.co.uk> wrote:
> Yes, you will need Chanwoo's series in order to use this. If also the
> driver needs to set a default initial regulator operating mode, it will
> need the series I'll post today but in that case also a .set_mode handler
> is needed.

In rk808 the mode isn't as complicated as the Maxim PMICs.  There's
only a "on in sleep" vs "off in sleep", not different sleep modes.
Also the "on" vs "off" isn't mixed up with the sleep mode setting.

...so I think that the "mode" stuff isn't needed for rk808.

-Doug

Re: [PATCH] regulator: rk808: Add support setting suspend voltage

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1331 bytes --]

On Tue, Oct 07, 2014 at 10:48:41AM -0700, Doug Anderson wrote:
> On Tue, Oct 7, 2014 at 1:43 AM, Chris Zhong <zyw@rock-chips.com> wrote:
> > support setting suspend voltage and disable regulator in suspend.

Doug, please delete unneeded context from mails, paging through screen
after screen of irrelevant text on the off chance of finding content
gets old fast so it's likely that some or all what you're saying will be
missed either through boredone or error.

> One point of curiosity (maybe this is a question for Chanwoo and
> Javier): I'd expect that if someone didn't explicitly setup a "suspend
> voltage" that their voltage would just be left alone at suspend time.

No, this is not the case.  The suspend mode settings are a completely
different set of settings activated when the system goes into suspend
with explicit hardware support.  If no configuration for this mode is
provided then 

> I believe that won't be the case for your driver.  The rk808 will (I
> think) automatically transition to the "suspend voltage" settings for
> ALL regulators at suspend time.  If you didn't explicitly set the
> suspend voltage then you'll move to whatever the default voltage is,
> right?

As ever the hardware configuration won't be touched by the kernel unless
it's explicitly told to do something.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] regulator: rk808: Add support setting suspend voltage

[Doug Anderson]

Mark,

On Wed, Oct 8, 2014 at 5:49 AM, Mark Brown <broonie@kernel.org> wrote:
>> One point of curiosity (maybe this is a question for Chanwoo and
>> Javier): I'd expect that if someone didn't explicitly setup a "suspend
>> voltage" that their voltage would just be left alone at suspend time.
>
> No, this is not the case.  The suspend mode settings are a completely
> different set of settings activated when the system goes into suspend
> with explicit hardware support.  If no configuration for this mode is
> provided then

I think maybe you missed finishing your sentence?


>> I believe that won't be the case for your driver.  The rk808 will (I
>> think) automatically transition to the "suspend voltage" settings for
>> ALL regulators at suspend time.  If you didn't explicitly set the
>> suspend voltage then you'll move to whatever the default voltage is,
>> right?
>
> As ever the hardware configuration won't be touched by the kernel unless
> it's explicitly told to do something.

I guess my point is that the kernel's inaction is actually causing
something unexpected to happen.  Robots can't let humans come to harm
by inaction any more than they can harm them by action.

Specifically I would expect that voltages would stay constant when the
rk808 "sleep" pin is asserted if I didn't explicitly say to disable
this regulator at sleep time and I didn't explicitly specify a voltage
at sleep time.  As Chris's patch stands right now this isn't the case.

To make it concrete, imagine that no "sleep voltage" was specified for
DCDC4 and it's setup to stay enabled during sleep.  Let's say that a
kernel driver makes a decision at runtime time to set this to 1.8V or
1.9V.

When the kernel driver sets it to 1.9V, it will go through
regulator_set_voltage_sel_regmap() which will set the BUCK4_ON_VSEL
register and we'll be at 1.9V.  Great, we're at 1.9V.  Now we're ready
to go to sleep.  I'd expect that the voltage would stay at 1.9V, but
it won't.  BUCK4_SLP_VSEL was never programmed so it's at whatever the
default is (1.8V) and we will transition there.

The above is a fictitious example since really we don't vary BUCK4 at
runtime in our system.  ...and really my concern only matters for
regulators that vary at runtime, so maybe we can ignore my comments.
To me, it does seem unexpected, though.

-Doug

Re: [PATCH] regulator: rk808: Add support setting suspend voltage

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2650 bytes --]

On Wed, Oct 08, 2014 at 09:12:59AM -0700, Doug Anderson wrote:
> On Wed, Oct 8, 2014 at 5:49 AM, Mark Brown <broonie@kernel.org> wrote:

> > No, this is not the case.  The suspend mode settings are a completely
> > different set of settings activated when the system goes into suspend
> > with explicit hardware support.  If no configuration for this mode is
> > provided then

> I think maybe you missed finishing your sentence?

nothing will be changed.

> >> I believe that won't be the case for your driver.  The rk808 will (I
> >> think) automatically transition to the "suspend voltage" settings for
> >> ALL regulators at suspend time.  If you didn't explicitly set the
> >> suspend voltage then you'll move to whatever the default voltage is,
> >> right?

> > As ever the hardware configuration won't be touched by the kernel unless
> > it's explicitly told to do something.

> I guess my point is that the kernel's inaction is actually causing
> something unexpected to happen.  Robots can't let humans come to harm
> by inaction any more than they can harm them by action.

> Specifically I would expect that voltages would stay constant when the
> rk808 "sleep" pin is asserted if I didn't explicitly say to disable
> this regulator at sleep time and I didn't explicitly specify a voltage
> at sleep time.  As Chris's patch stands right now this isn't the case.

The assumption has to be that the configuration that the device has is
essential to bringing the system into and out of suspend, if we went and
overrode everything with the current runtime configuration I'd expect
we'd break a whole bunch of systems and severely impact the power
consumption in suspend of more.  What you're asking for is just not how
these systems are designed, complain to your electrical engineers.  With
this model for doing things suspend entry and exit is defined and
sequenced at system design time (often with limited configuration so
really at tapeout time).  

> When the kernel driver sets it to 1.9V, it will go through
> regulator_set_voltage_sel_regmap() which will set the BUCK4_ON_VSEL
> register and we'll be at 1.9V.  Great, we're at 1.9V.  Now we're ready
> to go to sleep.  I'd expect that the voltage would stay at 1.9V, but
> it won't.  BUCK4_SLP_VSEL was never programmed so it's at whatever the
> default is (1.8V) and we will transition there.

No, if the driver needs a particular state during suspend it needs to
explicitly set the suspend mode, that's what those APIs are there for.
Remember that a suspended device is supposed to not be doing anything
anyway most of the time...

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] regulator: rk808: Add support setting suspend voltage

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 477 bytes --]

On Tue, Oct 07, 2014 at 04:43:57PM +0800, Chris Zhong wrote:

> @@ -100,6 +132,8 @@ static struct regulator_ops rk808_buck1_2_ops = {
>  	.disable		= regulator_disable_regmap,
>  	.is_enabled		= regulator_is_enabled_regmap,
>  	.set_ramp_delay		= rk808_set_ramp_delay,
> +	.set_suspend_voltage    = rk808_set_suspend_voltage,
> +	.set_suspend_disable    = rk808_set_suspend_disable,
>  };

Why are there only disable operations, no enable ones?  Otherwise this
all looks good.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]