[RFC][PATCH 0/5] Introduce .get_voltage callback into voltdm

[RFC][PATCH 0/5] Introduce .get_voltage callback into voltdm

[Mike Turquette]

From: Mike Turquette <mturquette@linaro.org>

This series creates a new callback for struct voltagedomain,
.get_voltage.  This fetches the voltage from hardware, if possible, and
returns it to the caller.  We use this call to populate
voltdm->nominal_volt at boot time.

The need for this came out of the Adaptive Body-Bias patches.  There is
a corner case where voltdm->nominal_volt is zero at boot, but the ABB
pre-scale function needs to know the current voltage as a part of its
logic.  Without this series voltdm->nominal_volt will be zero until
after voltdm->scale completes, which is too late for the ABB pre-scale
logic.

It is possible to have ABB without this series.  In the event that
voltdm->nominal_volt is zero then the ABB pre-scale function can simply
return early.  However this opens up a condition that burns power
needlessly:

1) The bootloader enables Forward Body-Bias mode on the ABB ldo and runs
the voltage domain at a high OPP

2) Linux boots and the first transition of that voltage domain is to a
lower OPP.

3) The voltage of that vdd is unknown so the ABB pre-scale function
returns early and FBB is left enabled at an OPP that would otherwise not
need it.

This scenario is not validated and is best avoided.  This series is one
step towards that.

Mike Turquette (5):
  ARM: omap: vc: replace data_shift with data_mask
  ARM: omap: introduce .get_voltage callback
  ARM: omap: vc: .get_voltage callback
  ARM: omap: vp: .get_voltage callback
  ARM: omap: initialize voltdm->nominal_volt

 arch/arm/mach-omap2/vc.c          |   23 ++++++++++++++++++++++-
 arch/arm/mach-omap2/vc.h          |    3 ++-
 arch/arm/mach-omap2/vc3xxx_data.c |    2 +-
 arch/arm/mach-omap2/vc44xx_data.c |    2 +-
 arch/arm/mach-omap2/voltage.c     |   13 +++++++++++++
 arch/arm/mach-omap2/voltage.h     |    1 +
 arch/arm/mach-omap2/vp.c          |   22 ++++++++++++++++++++++
 arch/arm/mach-omap2/vp.h          |    1 +
 8 files changed, 63 insertions(+), 4 deletions(-)

-- 
1.7.9.5

[PATCH 1/5] ARM: omap: vc: replace data_shift with data_mask

[Mike Turquette]

From: Mike Turquette <mturquette@linaro.org>

Signed-off-by: Mike Turquette <mturquette@linaro.org>
---
 arch/arm/mach-omap2/vc.c          |    2 +-
 arch/arm/mach-omap2/vc.h          |    2 +-
 arch/arm/mach-omap2/vc3xxx_data.c |    2 +-
 arch/arm/mach-omap2/vc44xx_data.c |    2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/arm/mach-omap2/vc.c b/arch/arm/mach-omap2/vc.c
index 84da34f..2bcac64 100644
--- a/arch/arm/mach-omap2/vc.c
+++ b/arch/arm/mach-omap2/vc.c
@@ -171,7 +171,7 @@ int omap_vc_bypass_scale(struct voltagedomain *voltdm,
 
 	vc_valid = vc->common->valid;
 	vc_bypass_val_reg = vc->common->bypass_val_reg;
-	vc_bypass_value = (target_vsel << vc->common->data_shift) |
+	vc_bypass_value = (target_vsel << __ffs(vc->common->data_mask)) |
 		(vc->volt_reg_addr << vc->common->regaddr_shift) |
 		(vc->i2c_slave_addr << vc->common->slaveaddr_shift);
 
diff --git a/arch/arm/mach-omap2/vc.h b/arch/arm/mach-omap2/vc.h
index 478bf6b..84a61b1 100644
--- a/arch/arm/mach-omap2/vc.h
+++ b/arch/arm/mach-omap2/vc.h
@@ -43,8 +43,8 @@ struct voltagedomain;
 struct omap_vc_common {
 	u32 cmd_on_mask;
 	u32 valid;
+	u32 data_mask;
 	u8 bypass_val_reg;
-	u8 data_shift;
 	u8 slaveaddr_shift;
 	u8 regaddr_shift;
 	u8 cmd_on_shift;
diff --git a/arch/arm/mach-omap2/vc3xxx_data.c b/arch/arm/mach-omap2/vc3xxx_data.c
index 5d8eaf3..2b35e82 100644
--- a/arch/arm/mach-omap2/vc3xxx_data.c
+++ b/arch/arm/mach-omap2/vc3xxx_data.c
@@ -31,7 +31,7 @@
  */
 static struct omap_vc_common omap3_vc_common = {
 	.bypass_val_reg	 = OMAP3_PRM_VC_BYPASS_VAL_OFFSET,
-	.data_shift	 = OMAP3430_DATA_SHIFT,
+	.data_mask	 = OMAP3430_DATA_MASK,
 	.slaveaddr_shift = OMAP3430_SLAVEADDR_SHIFT,
 	.regaddr_shift	 = OMAP3430_REGADDR_SHIFT,
 	.valid		 = OMAP3430_VALID_MASK,
diff --git a/arch/arm/mach-omap2/vc44xx_data.c b/arch/arm/mach-omap2/vc44xx_data.c
index d70b930..598edc9 100644
--- a/arch/arm/mach-omap2/vc44xx_data.c
+++ b/arch/arm/mach-omap2/vc44xx_data.c
@@ -32,7 +32,7 @@
  */
 static const struct omap_vc_common omap4_vc_common = {
 	.bypass_val_reg = OMAP4_PRM_VC_VAL_BYPASS_OFFSET,
-	.data_shift = OMAP4430_DATA_SHIFT,
+	.data_mask = OMAP4430_DATA_MASK,
 	.slaveaddr_shift = OMAP4430_SLAVEADDR_SHIFT,
 	.regaddr_shift = OMAP4430_REGADDR_SHIFT,
 	.valid = OMAP4430_VALID_MASK,
-- 
1.7.9.5

[PATCH 1/5] ARM: omap: vc: replace data_shift with data_mask

[Kevin Hilman]

Mike Turquette <mturquette@ti.com> writes:

> From: Mike Turquette <mturquette@linaro.org>
>
> Signed-off-by: Mike Turquette <mturquette@linaro.org>

Re: $SUBJECT...  ok, I'll bite....   why?

(I'm sure I will find out when I read the rest of the series, but I'm
impatient and picky and would like to know here.)   ;)

Kevin

[PATCH 2/5] ARM: omap: introduce .get_voltage callback

[Mike Turquette]

From: Mike Turquette <mturquette@linaro.org>

Introduces a new callback to struct voltagedomain for retrieving the
voltage of the voltage domain from the hardware.  This will be used to
populate voltdm->nominal_volt during initialization of the voltage code
instead of waiting for the first call to voltdm_scale.

In the event of an error or the inability to determine voltage the
callback must return zero.  It is the callers responsibility to
gracefully degrade when presented with a voltage of zero.

Signed-off-by: Mike Turquette <mturquette@ti.com>
Signed-off-by: Mike Turquette <mturquette@linaro.org>
---
 arch/arm/mach-omap2/voltage.h |    1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/mach-omap2/voltage.h b/arch/arm/mach-omap2/voltage.h
index 0ac2caf..0ded54f 100644
--- a/arch/arm/mach-omap2/voltage.h
+++ b/arch/arm/mach-omap2/voltage.h
@@ -85,6 +85,7 @@ struct voltagedomain {
 		u32 rate;
 	} sys_clk;
 
+	unsigned long (*get_voltage) (struct voltagedomain *voltdm);
 	int (*scale) (struct voltagedomain *voltdm,
 		      unsigned long target_volt);
 
-- 
1.7.9.5

[PATCH 3/5] ARM: omap: vc: .get_voltage callback

[Mike Turquette]

From: Mike Turquette <mturquette@linaro.org>

Implement the voltdm->get_voltage callback for the voltage controller
driver.  This reads the DATA field corresponding to each VC and returns
the voltage, after converting it from vsel format.

If DATA is zero (the reset value) then the caller must interpret this as
the PMIC running at the default power-on voltage.  In such a case DT
data for the PMIC is necessary to know the voltage.

Signed-off-by: Mike Turquette <mturquette@ti.com>
Signed-off-by: Mike Turquette <mturquette@linaro.org>
---
 arch/arm/mach-omap2/vc.c |   21 +++++++++++++++++++++
 arch/arm/mach-omap2/vc.h |    1 +
 2 files changed, 22 insertions(+)

diff --git a/arch/arm/mach-omap2/vc.c b/arch/arm/mach-omap2/vc.c
index 2bcac64..90a9ea6 100644
--- a/arch/arm/mach-omap2/vc.c
+++ b/arch/arm/mach-omap2/vc.c
@@ -101,6 +101,27 @@ static int omap_vc_config_channel(struct voltagedomain *voltdm)
 }
 
 /* Voltage scale and accessory APIs */
+unsigned long omap_vc_get_bypass_data(struct voltagedomain *voltdm)
+{
+	struct omap_vc_channel *vc = voltdm->vc;
+	u32 vc_bypass_value;
+	u8 vsel;
+	unsigned long volt;
+
+	/* sanity */
+	if (!voltdm->pmic || !voltdm->pmic->vsel_to_uv ||
+			!voltdm->read || !voltdm->write)
+		return 0;
+
+	vc_bypass_value = voltdm->read(vc->common->bypass_val_reg);
+	vc_bypass_value &= vc->common->data_mask;
+	vsel = vc_bypass_value >> __ffs(vc->common->data_mask);
+
+	volt = voltdm->pmic->vsel_to_uv(vsel);
+
+	return volt;
+}
+
 int omap_vc_pre_scale(struct voltagedomain *voltdm,
 		      unsigned long target_volt,
 		      u8 *target_vsel, u8 *current_vsel)
diff --git a/arch/arm/mach-omap2/vc.h b/arch/arm/mach-omap2/vc.h
index 84a61b1..e7d4719 100644
--- a/arch/arm/mach-omap2/vc.h
+++ b/arch/arm/mach-omap2/vc.h
@@ -112,6 +112,7 @@ extern struct omap_vc_channel omap4_vc_iva;
 extern struct omap_vc_channel omap4_vc_core;
 
 void omap_vc_init_channel(struct voltagedomain *voltdm);
+unsigned long omap_vc_get_bypass_data(struct voltagedomain *voltdm);
 int omap_vc_pre_scale(struct voltagedomain *voltdm,
 		      unsigned long target_volt,
 		      u8 *target_vsel, u8 *current_vsel);
-- 
1.7.9.5

[PATCH 3/5] ARM: omap: vc: .get_voltage callback

[Kevin Hilman]

Mike Turquette <mturquette@ti.com> writes:

> From: Mike Turquette <mturquette@linaro.org>
>
> Implement the voltdm->get_voltage callback for the voltage controller
> driver.  

nit: since it's not actually used in this series, it should say
Impelment the function that will be used as the ->get_voltage()
callback...

> This reads the DATA field corresponding to each VC and returns
> the voltage, after converting it from vsel format.
>
> If DATA is zero (the reset value) then the caller must interpret this as
> the PMIC running at the default power-on voltage.  In such a case DT
> data for the PMIC is necessary to know the voltage.
>
> Signed-off-by: Mike Turquette <mturquette@ti.com>
> Signed-off-by: Mike Turquette <mturquette@linaro.org>
> ---
>  arch/arm/mach-omap2/vc.c |   21 +++++++++++++++++++++
>  arch/arm/mach-omap2/vc.h |    1 +
>  2 files changed, 22 insertions(+)
>
> diff --git a/arch/arm/mach-omap2/vc.c b/arch/arm/mach-omap2/vc.c
> index 2bcac64..90a9ea6 100644
> --- a/arch/arm/mach-omap2/vc.c
> +++ b/arch/arm/mach-omap2/vc.c
> @@ -101,6 +101,27 @@ static int omap_vc_config_channel(struct voltagedomain *voltdm)
>  }
>  
>  /* Voltage scale and accessory APIs */
> +unsigned long omap_vc_get_bypass_data(struct voltagedomain *voltdm)
> +{
> +	struct omap_vc_channel *vc = voltdm->vc;
> +	u32 vc_bypass_value;
> +	u8 vsel;
> +	unsigned long volt;
> +
> +	/* sanity */
> +	if (!voltdm->pmic || !voltdm->pmic->vsel_to_uv ||
> +			!voltdm->read || !voltdm->write)
> +		return 0;
> +
> +	vc_bypass_value = voltdm->read(vc->common->bypass_val_reg);
> +	vc_bypass_value &= vc->common->data_mask;
> +	vsel = vc_bypass_value >> __ffs(vc->common->data_mask);

Ah, now I see where the data_mask is used.  :)

Kevin

> +	volt = voltdm->pmic->vsel_to_uv(vsel);
> +
> +	return volt;
> +}
> +
>  int omap_vc_pre_scale(struct voltagedomain *voltdm,
>  		      unsigned long target_volt,
>  		      u8 *target_vsel, u8 *current_vsel)
> diff --git a/arch/arm/mach-omap2/vc.h b/arch/arm/mach-omap2/vc.h
> index 84a61b1..e7d4719 100644
> --- a/arch/arm/mach-omap2/vc.h
> +++ b/arch/arm/mach-omap2/vc.h
> @@ -112,6 +112,7 @@ extern struct omap_vc_channel omap4_vc_iva;
>  extern struct omap_vc_channel omap4_vc_core;
>  
>  void omap_vc_init_channel(struct voltagedomain *voltdm);
> +unsigned long omap_vc_get_bypass_data(struct voltagedomain *voltdm);
>  int omap_vc_pre_scale(struct voltagedomain *voltdm,
>  		      unsigned long target_volt,
>  		      u8 *target_vsel, u8 *current_vsel);

[PATCH 4/5] ARM: omap: vp: .get_voltage callback

[Mike Turquette]

From: Mike Turquette <mturquette@linaro.org>

Implement the voltdm->get_voltage callback for the voltage processor
driver.  This reads the INITVOLTAGE field corresponding to each VP and
returns the voltage, after converting it from vsel format.

If INITVOLTAGE is zero (the reset value) then the caller must interpret
this as the PMIC running at the default power-on voltage.  In such a
case DT data for the PMIC is necessary to know the voltage.

Signed-off-by: Mike Turquette <mturquette@ti.com>
Signed-off-by: Mike Turquette <mturquette@linaro.org>
---
 arch/arm/mach-omap2/vp.c |   22 ++++++++++++++++++++++
 arch/arm/mach-omap2/vp.h |    1 +
 2 files changed, 23 insertions(+)

diff --git a/arch/arm/mach-omap2/vp.c b/arch/arm/mach-omap2/vp.c
index f95c1ba..63dc03e 100644
--- a/arch/arm/mach-omap2/vp.c
+++ b/arch/arm/mach-omap2/vp.c
@@ -9,6 +9,28 @@
 #include "prm-regbits-44xx.h"
 #include "prm44xx.h"
 
+unsigned long omap_vp_get_init_voltage(struct voltagedomain *voltdm)
+{
+	struct omap_vp_instance *vp = voltdm->vp;
+	u32 vpconfig;
+	u8 vsel;
+	unsigned long volt;
+
+	/* sanity */
+	if (!voltdm->pmic || !voltdm->pmic->vsel_to_uv
+			|| !voltdm->read || !voltdm->write)
+		return 0;
+
+	vpconfig = voltdm->read(vp->vpconfig);
+	vpconfig &= vp->common->vpconfig_initvoltage_mask;
+
+	vsel = vpconfig >> __ffs(vp->common->vpconfig_initvoltage_mask);
+
+	volt = voltdm->pmic->vsel_to_uv(vsel);
+
+	return volt;
+}
+
 static u32 _vp_set_init_voltage(struct voltagedomain *voltdm, u32 volt)
 {
 	struct omap_vp_instance *vp = voltdm->vp;
diff --git a/arch/arm/mach-omap2/vp.h b/arch/arm/mach-omap2/vp.h
index 7c155d2..0747a78 100644
--- a/arch/arm/mach-omap2/vp.h
+++ b/arch/arm/mach-omap2/vp.h
@@ -120,6 +120,7 @@ extern struct omap_vp_instance omap4_vp_core;
 void omap_vp_init(struct voltagedomain *voltdm);
 void omap_vp_enable(struct voltagedomain *voltdm);
 void omap_vp_disable(struct voltagedomain *voltdm);
+unsigned long omap_vp_get_init_voltage(struct voltagedomain *voltdm);
 int omap_vp_forceupdate_scale(struct voltagedomain *voltdm,
 			      unsigned long target_volt);
 int omap_vp_update_errorgain(struct voltagedomain *voltdm,
-- 
1.7.9.5

[PATCH 5/5] ARM: omap: initialize voltdm->nominal_volt

[Mike Turquette]

From: Mike Turquette <mturquette@linaro.org>

Populate the .get_voltage callbacks for VC & VP.  Use these callbacks to
populate voltdm->nominal_volt during boot sequence.

This is useful for the Adaptive Body-Bias sequence coming in a
subsequent series.  Without this patch voltdm->nominal will not be
populated until after the first call to voltdm_scale.  This is too late
to be useful to the pre-scale sequence in the ABB series since that
sequence needs to know the voltage prior to the first transition.

Note that the .get_voltage callback will return zero for the corner case
wherein the bootloader has not scaling the voltage.  In this case it is
safe to assume that we are running at the PMIC's default power-on
voltage.  DT data providing the default voltage for each PMIC is needed
in this case.

Signed-off-by: Mike Turquette <mturquette@ti.com>
Signed-off-by: Mike Turquette <mturquette@linaro.org>
---
 arch/arm/mach-omap2/voltage.c |   13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/arch/arm/mach-omap2/voltage.c b/arch/arm/mach-omap2/voltage.c
index 4dc60e8..f45716c 100644
--- a/arch/arm/mach-omap2/voltage.c
+++ b/arch/arm/mach-omap2/voltage.c
@@ -244,9 +244,11 @@ void omap_change_voltscale_method(struct voltagedomain *voltdm,
 	switch (voltscale_method) {
 	case VOLTSCALE_VPFORCEUPDATE:
 		voltdm->scale = omap_vp_forceupdate_scale;
+		voltdm->get_voltage = omap_vp_get_init_voltage;
 		return;
 	case VOLTSCALE_VCBYPASS:
 		voltdm->scale = omap_vc_bypass_scale;
+		voltdm->get_voltage = omap_vc_get_bypass_data;
 		return;
 	default:
 		pr_warning("%s: Trying to change the method of voltage scaling"
@@ -288,13 +290,24 @@ int __init omap_voltage_late_init(void)
 
 		if (voltdm->vc) {
 			voltdm->scale = omap_vc_bypass_scale;
+			voltdm->get_voltage = omap_vc_get_bypass_data;
 			omap_vc_init_channel(voltdm);
 		}
 
 		if (voltdm->vp) {
 			voltdm->scale = omap_vp_forceupdate_scale;
+			voltdm->get_voltage = omap_vp_get_init_voltage;
 			omap_vp_init(voltdm);
 		}
+
+		/*
+		 * XXX If voltdm->nominal_volt is zero after calling
+		 * voltdm->get_voltage then we are likely running this
+		 * voltage domain at the default boot voltage of the
+		 * PMIC.  In such a case it would be best to load this
+		 * value from DT.
+		 */
+		voltdm->nominal_volt = voltdm->get_voltage(voltdm);
 	}
 
 	return 0;
-- 
1.7.9.5

[RFC][PATCH 0/5] Introduce .get_voltage callback into voltdm

[Kevin Hilman]

Hi Mike,

Mike Turquette <mturquette@ti.com> writes:

> From: Mike Turquette <mturquette@linaro.org>
>
> This series creates a new callback for struct voltagedomain,
> .get_voltage.  This fetches the voltage from hardware, if possible, and
> returns it to the caller.  We use this call to populate
> voltdm->nominal_volt at boot time.

I pointed out a couple nitpicky things on individual patches, but
otherwise this direction and motiviation for this series looks OK by me.

Just some minor comments about the structure of the series.  I tend to
prefer combining the introduction of a new function with it's usage.  It
makes review and understanding much easier, IMO.  If there are reasons
to keep them separate, that's fine too.  Just describe the reasons in
the cover letter.

Thanks,

Kevin

[RFC][PATCH 0/5] Introduce .get_voltage callback into voltdm

[Mike Turquette]

Quoting Kevin Hilman (2012-10-26 11:17:51)
> Hi Mike,
> 
> Mike Turquette <mturquette@ti.com> writes:
> 
> > From: Mike Turquette <mturquette@linaro.org>
> >
> > This series creates a new callback for struct voltagedomain,
> > .get_voltage.  This fetches the voltage from hardware, if possible, and
> > returns it to the caller.  We use this call to populate
> > voltdm->nominal_volt at boot time.
> 
> I pointed out a couple nitpicky things on individual patches, but
> otherwise this direction and motiviation for this series looks OK by me.
> 
> Just some minor comments about the structure of the series.  I tend to
> prefer combining the introduction of a new function with it's usage.  It
> makes review and understanding much easier, IMO.  If there are reasons
> to keep them separate, that's fine too.  Just describe the reasons in
> the cover letter.
> 

Will do.  I've thought on this topic a bit more and this approach isn't
quite robust enough.  For instance it assumes that the kernel's voltage
scaling approach (vp_forceupdate vs. vc_bypass) matches what the
bootloader does.  This is of course an unsafe assumption.

Also I've seen the Android guys struggle with this a bit and I'd like to
review how they approached the problem before sending my next series.
Couple with upcoming travel I'm not sure how quickly I'll get a V2 on
the list.

Thanks for reviewing,
Mike

> Thanks,
> 
> Kevin

[RFC][PATCH 0/5] Introduce .get_voltage callback into voltdm

[Paul Walmsley]

Hi Mike, Kevin,

On Wed, 3 Oct 2012, Mike Turquette wrote:

> From: Mike Turquette <mturquette@linaro.org>
> 
> This series creates a new callback for struct voltagedomain,
> .get_voltage.  This fetches the voltage from hardware, if possible, and
> returns it to the caller.  We use this call to populate
> voltdm->nominal_volt at boot time.

Just checking on the status of this series.  Is this superseded by 
Andrii's driver?  Does it need to be reposted for merging?  etc.


- Paul

[RFC][PATCH 0/5] Introduce .get_voltage callback into voltdm

[Mike Turquette]

On Sat, Mar 30, 2013 at 7:35 PM, Paul Walmsley <paul@pwsan.com> wrote:
> Hi Mike, Kevin,
>
> On Wed, 3 Oct 2012, Mike Turquette wrote:
>
>> From: Mike Turquette <mturquette@linaro.org>
>>
>> This series creates a new callback for struct voltagedomain,
>> .get_voltage.  This fetches the voltage from hardware, if possible, and
>> returns it to the caller.  We use this call to populate
>> voltdm->nominal_volt at boot time.
>
> Just checking on the status of this series.  Is this superseded by
> Andrii's driver?  Does it need to be reposted for merging?  etc.
>

Hi Paul,

Thanks for resurrecting this thread.  This series is needed if the ABB
transition is triggered by the OMAP VC/VP code.  Andrii's approach
triggers an ABB transition based on the clock rate-change notifiers
but that is still in discussion.

So the short answer is "I don't know yet".

Regards,
Mike

>
> - Paul