[PATCH V4 0/4] ARM: OMAP3+: VP: collate fixes

[PATCH V4 0/4] ARM: OMAP3+: VP: collate fixes

[Nishanth Menon]

Reviewing the android code base brought out a few fixes that upstream
does not have, so collating them and posting them out in this chain.

Nishanth Menon (3):
  ARM: OMAP3+: PM: VP: check only the VPINIDLE status bit
  ARM: OMAP3+: PM: VP: move check of idle to separate function
  ARM: OMAP3+: PM: VP: check to ensure VP is idle before forceupdate

Wenbiao Wang (1):
  ARM: OMAP3+: PM: VP: ensure VP is idle before disable

 arch/arm/mach-omap2/vp.c          |   65 +++++++++++++++++++++++++++++++------
 arch/arm/mach-omap2/vp.h          |   10 ++++--
 arch/arm/mach-omap2/vp3xxx_data.c |    1 +
 arch/arm/mach-omap2/vp44xx_data.c |    1 +
 4 files changed, 65 insertions(+), 12 deletions(-)

Changes in v4:
	vp idle moved off to a seperate function
	reordered patches to reduce loc changes
	rate controlled warnings

Previous revs:
	V1: http://marc.info/?l=linux-omap&m=133736976423629&w=2

Cc: Tony Lindgren <tony@atomide.com>
Cc: Kevin Hilman <khilman@ti.com>
--
Regards,
Nishanth Menon
-- 
1.7.9.5

[PATCH V2 1/4] ARM: OMAP3+: PM: VP: check only the VPINIDLE status bit

[Nishanth Menon]

Currently we check against the entire 32 bits of the status register
Where, bits 1-31 are marked as reserved and mentioned in TRM as read
returns undefined values. Hence, check against purely the vpinidle
status bit.

Cc: Tony Lindgren <tony@atomide.com>
Cc: Kevin Hilman <khilman@ti.com>
Reported-by: Vinay Amancha <vinaykumar@ti.com>
Signed-off-by: Nishanth Menon <nm@ti.com>
---
 arch/arm/mach-omap2/vp.c          |    5 +++--
 arch/arm/mach-omap2/vp.h          |    1 +
 arch/arm/mach-omap2/vp3xxx_data.c |    1 +
 arch/arm/mach-omap2/vp44xx_data.c |    1 +
 4 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-omap2/vp.c b/arch/arm/mach-omap2/vp.c
index f95c1ba..faefef7 100644
--- a/arch/arm/mach-omap2/vp.c
+++ b/arch/arm/mach-omap2/vp.c
@@ -270,8 +270,9 @@ void omap_vp_disable(struct voltagedomain *voltdm)
 	/*
 	 * Wait for VP idle Typical latency is <2us. Maximum latency is ~100us
 	 */
-	omap_test_timeout((voltdm->read(vp->vstatus)),
-			  VP_IDLE_TIMEOUT, timeout);
+	omap_test_timeout((voltdm->read(vp->vstatus) &
+			   vp->common->vstatus_vpidle), VP_IDLE_TIMEOUT,
+			  timeout);
 
 	if (timeout >= VP_IDLE_TIMEOUT)
 		pr_warning("%s: vdd_%s idle timedout\n",
diff --git a/arch/arm/mach-omap2/vp.h b/arch/arm/mach-omap2/vp.h
index 7c155d2..ac1d6cf 100644
--- a/arch/arm/mach-omap2/vp.h
+++ b/arch/arm/mach-omap2/vp.h
@@ -73,6 +73,7 @@ struct omap_vp_common {
 	u8 vpconfig_initvdd;
 	u8 vpconfig_forceupdate;
 	u8 vpconfig_vpenable;
+	u8 vstatus_vpidle;
 	u8 vstepmin_stepmin_shift;
 	u8 vstepmin_smpswaittimemin_shift;
 	u8 vstepmax_stepmax_shift;
diff --git a/arch/arm/mach-omap2/vp3xxx_data.c b/arch/arm/mach-omap2/vp3xxx_data.c
index bd89f80..a65909b 100644
--- a/arch/arm/mach-omap2/vp3xxx_data.c
+++ b/arch/arm/mach-omap2/vp3xxx_data.c
@@ -44,6 +44,7 @@ static const struct omap_vp_common omap3_vp_common = {
 	.vpconfig_initvdd = OMAP3430_INITVDD_MASK,
 	.vpconfig_forceupdate = OMAP3430_FORCEUPDATE_MASK,
 	.vpconfig_vpenable = OMAP3430_VPENABLE_MASK,
+	.vstatus_vpidle = OMAP3430_VPINIDLE_MASK,
 	.vstepmin_smpswaittimemin_shift = OMAP3430_SMPSWAITTIMEMIN_SHIFT,
 	.vstepmax_smpswaittimemax_shift = OMAP3430_SMPSWAITTIMEMAX_SHIFT,
 	.vstepmin_stepmin_shift = OMAP3430_VSTEPMIN_SHIFT,
diff --git a/arch/arm/mach-omap2/vp44xx_data.c b/arch/arm/mach-omap2/vp44xx_data.c
index 8c031d1..9d14881 100644
--- a/arch/arm/mach-omap2/vp44xx_data.c
+++ b/arch/arm/mach-omap2/vp44xx_data.c
@@ -44,6 +44,7 @@ static const struct omap_vp_common omap4_vp_common = {
 	.vpconfig_initvdd = OMAP4430_INITVDD_MASK,
 	.vpconfig_forceupdate = OMAP4430_FORCEUPDATE_MASK,
 	.vpconfig_vpenable = OMAP4430_VPENABLE_MASK,
+	.vstatus_vpidle = OMAP4430_VPINIDLE_MASK,
 	.vstepmin_smpswaittimemin_shift = OMAP4430_SMPSWAITTIMEMIN_SHIFT,
 	.vstepmax_smpswaittimemax_shift = OMAP4430_SMPSWAITTIMEMAX_SHIFT,
 	.vstepmin_stepmin_shift = OMAP4430_VSTEPMIN_SHIFT,
-- 
1.7.9.5

[PATCH V2 2/4] ARM: OMAP3+: PM: VP: move check of idle to separate function

[Nishanth Menon]

In general, the idle check tends to be time sensitive so that the
followon operation after the idle status verification is done in
a timely manner. However, since we'd be using this in multiple
places in follow-on patches, isolate this out to a separate inline
function.

As part of this change, convert the pr_warning to a rate limited
warning as the situation is most probably going to stick around
(since Voltage Processor state machine might be stuck) + a WARN_ONCE
to ensure adequate attention is received. Document the information
about idle timeout. Based on the idea from Kevin Hilman.

Cc: Tony Lindgren <tony@atomide.com>
Cc: Kevin Hilman <khilman@ti.com>
Cc: Eduardo Valentin <eduardo.valentin@ti.com>
Signed-off-by: Nishanth Menon <nm@ti.com>
---
 arch/arm/mach-omap2/vp.c |   55 ++++++++++++++++++++++++++++++++++++----------
 arch/arm/mach-omap2/vp.h |    6 ++++-
 2 files changed, 49 insertions(+), 12 deletions(-)

diff --git a/arch/arm/mach-omap2/vp.c b/arch/arm/mach-omap2/vp.c
index faefef7..4723879 100644
--- a/arch/arm/mach-omap2/vp.c
+++ b/arch/arm/mach-omap2/vp.c
@@ -1,5 +1,6 @@
 #include <linux/kernel.h>
 #include <linux/init.h>
+#include <linux/ratelimit.h>
 
 #include "common.h"
 
@@ -9,6 +10,45 @@
 #include "prm-regbits-44xx.h"
 #include "prm44xx.h"
 
+/**
+ * _vp_wait_for_idle() - wait for voltage processor to idle
+ * @voltdm:	voltage domain
+ * @vp:		voltage processor instance
+ *
+ * In some conditions, it is important to ensure that Voltage Processor
+ * is idle before performing operations on the Voltage Processor(VP).
+ * This is primarily to ensure that VP state machine does not enter into
+ * invalid state.
+ *
+ * Returns -ETIMEDOUT if timeout occurs - This could be critical failure
+ * as it indicates that Voltage processor might have it's state machine
+ * stuck up without recovering out(theoretically should never happen
+ * ofcourse). Returns 0 if idle state is detected.
+ *
+ * Note: callers are expected to ensure requisite checks are performed
+ * on the pointers passed.
+ */
+static inline int _vp_wait_for_idle(struct voltagedomain *voltdm,
+				    struct omap_vp_instance *vp)
+{
+	int timeout;
+
+	omap_test_timeout((voltdm->read(vp->vstatus) &
+			   vp->common->vstatus_vpidle), VP_IDLE_TIMEOUT,
+			  timeout);
+
+	if (timeout >= VP_IDLE_TIMEOUT) {
+		/* Dont spam the console but ensure we catch attention */
+		pr_warn_ratelimited("%s: vdd_%s idle timedout\n",
+				    __func__, voltdm->name);
+		WARN_ONCE("vdd_%s idle timedout\n", voltdm->name);
+
+		return -ETIMEDOUT;
+	}
+
+	return 0;
+}
+
 static u32 _vp_set_init_voltage(struct voltagedomain *voltdm, u32 volt)
 {
 	struct omap_vp_instance *vp = voltdm->vp;
@@ -241,7 +281,6 @@ void omap_vp_disable(struct voltagedomain *voltdm)
 {
 	struct omap_vp_instance *vp;
 	u32 vpconfig;
-	int timeout;
 
 	if (!voltdm || IS_ERR(voltdm)) {
 		pr_warning("%s: VDD specified does not exist!\n", __func__);
@@ -267,16 +306,10 @@ void omap_vp_disable(struct voltagedomain *voltdm)
 	vpconfig &= ~vp->common->vpconfig_vpenable;
 	voltdm->write(vpconfig, vp->vpconfig);
 
-	/*
-	 * Wait for VP idle Typical latency is <2us. Maximum latency is ~100us
-	 */
-	omap_test_timeout((voltdm->read(vp->vstatus) &
-			   vp->common->vstatus_vpidle), VP_IDLE_TIMEOUT,
-			  timeout);
-
-	if (timeout >= VP_IDLE_TIMEOUT)
-		pr_warning("%s: vdd_%s idle timedout\n",
-			__func__, voltdm->name);
+	if (_vp_wait_for_idle(voltdm, vp)) {
+		pr_warn_ratelimited("%s: vdd_%s timedout after disable!!\n",
+				    __func__, voltdm->name);
+	}
 
 	vp->enabled = false;
 
diff --git a/arch/arm/mach-omap2/vp.h b/arch/arm/mach-omap2/vp.h
index ac1d6cf..4655b39 100644
--- a/arch/arm/mach-omap2/vp.h
+++ b/arch/arm/mach-omap2/vp.h
@@ -30,7 +30,11 @@ struct voltagedomain;
 #define OMAP4_VP_VDD_IVA_ID 1
 #define OMAP4_VP_VDD_MPU_ID 2
 
-/* XXX document */
+/*
+ * Time out for Voltage processor in micro seconds. Typical latency is < 2uS,
+ * but worst case latencies could be around 3-200uS depending on where we
+ * interrupted VP's operation.
+ */
 #define VP_IDLE_TIMEOUT		200
 #define VP_TRANXDONE_TIMEOUT	300
 
-- 
1.7.9.5

[PATCH V2 2/4] ARM: OMAP3+: PM: VP: move check of idle to separate function

[Kevin Hilman]

Nishanth Menon <nm@ti.com> writes:

> In general, the idle check tends to be time sensitive so that the
> followon operation after the idle status verification is done in
> a timely manner. However, since we'd be using this in multiple
> places in follow-on patches, isolate this out to a separate inline
> function.
>
> As part of this change, convert the pr_warning to a rate limited
> warning as the situation is most probably going to stick around
> (since Voltage Processor state machine might be stuck) + a WARN_ONCE
> to ensure adequate attention is received. Document the information
> about idle timeout. Based on the idea from Kevin Hilman.
>
> Cc: Tony Lindgren <tony@atomide.com>
> Cc: Kevin Hilman <khilman@ti.com>
> Cc: Eduardo Valentin <eduardo.valentin@ti.com>
> Signed-off-by: Nishanth Menon <nm@ti.com>
> ---
>  arch/arm/mach-omap2/vp.c |   55 ++++++++++++++++++++++++++++++++++++----------
>  arch/arm/mach-omap2/vp.h |    6 ++++-
>  2 files changed, 49 insertions(+), 12 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/vp.c b/arch/arm/mach-omap2/vp.c
> index faefef7..4723879 100644
> --- a/arch/arm/mach-omap2/vp.c
> +++ b/arch/arm/mach-omap2/vp.c
> @@ -1,5 +1,6 @@
>  #include <linux/kernel.h>
>  #include <linux/init.h>
> +#include <linux/ratelimit.h>
>  
>  #include "common.h"
>  
> @@ -9,6 +10,45 @@
>  #include "prm-regbits-44xx.h"
>  #include "prm44xx.h"
>  
> +/**
> + * _vp_wait_for_idle() - wait for voltage processor to idle
> + * @voltdm:	voltage domain
> + * @vp:		voltage processor instance
> + *
> + * In some conditions, it is important to ensure that Voltage Processor
> + * is idle before performing operations on the Voltage Processor(VP).
> + * This is primarily to ensure that VP state machine does not enter into
> + * invalid state.
> + *
> + * Returns -ETIMEDOUT if timeout occurs - This could be critical failure
> + * as it indicates that Voltage processor might have it's state machine
> + * stuck up without recovering out(theoretically should never happen
> + * ofcourse). Returns 0 if idle state is detected.
> + *
> + * Note: callers are expected to ensure requisite checks are performed
> + * on the pointers passed.
> + */
> +static inline int _vp_wait_for_idle(struct voltagedomain *voltdm,
> +				    struct omap_vp_instance *vp)
> +{
> +	int timeout;
> +
> +	omap_test_timeout((voltdm->read(vp->vstatus) &
> +			   vp->common->vstatus_vpidle), VP_IDLE_TIMEOUT,
> +			  timeout);
> +
> +	if (timeout >= VP_IDLE_TIMEOUT) {
> +		/* Dont spam the console but ensure we catch attention */
> +		pr_warn_ratelimited("%s: vdd_%s idle timedout\n",

s/idle timedout/timeout waiting for VP idle/

> +				    __func__, voltdm->name);
> +		WARN_ONCE("vdd_%s idle timedout\n", voltdm->name);
> +

Maybe just leave the WARN_ONCE() here since all the callers are using
the rate-limited pr_warn() in the case of error.  Otherwise, there will
b a bunch of duplicated messages on the console.

> +		return -ETIMEDOUT;
> +	}
> +
> +	return 0;
> +}
> +
>  static u32 _vp_set_init_voltage(struct voltagedomain *voltdm, u32 volt)
>  {
>  	struct omap_vp_instance *vp = voltdm->vp;
> @@ -241,7 +281,6 @@ void omap_vp_disable(struct voltagedomain *voltdm)
>  {
>  	struct omap_vp_instance *vp;
>  	u32 vpconfig;
> -	int timeout;
>  
>  	if (!voltdm || IS_ERR(voltdm)) {
>  		pr_warning("%s: VDD specified does not exist!\n", __func__);
> @@ -267,16 +306,10 @@ void omap_vp_disable(struct voltagedomain *voltdm)
>  	vpconfig &= ~vp->common->vpconfig_vpenable;
>  	voltdm->write(vpconfig, vp->vpconfig);
>  
> -	/*
> -	 * Wait for VP idle Typical latency is <2us. Maximum latency is ~100us
> -	 */
> -	omap_test_timeout((voltdm->read(vp->vstatus) &
> -			   vp->common->vstatus_vpidle), VP_IDLE_TIMEOUT,
> -			  timeout);
> -
> -	if (timeout >= VP_IDLE_TIMEOUT)
> -		pr_warning("%s: vdd_%s idle timedout\n",
> -			__func__, voltdm->name);
> +	if (_vp_wait_for_idle(voltdm, vp)) {
> +		pr_warn_ratelimited("%s: vdd_%s timedout after disable!!\n",

s/timedout/VP idle timeout/

> +				    __func__, voltdm->name);
> +	}
>  
>  	vp->enabled = false;
>  
> diff --git a/arch/arm/mach-omap2/vp.h b/arch/arm/mach-omap2/vp.h
> index ac1d6cf..4655b39 100644
> --- a/arch/arm/mach-omap2/vp.h
> +++ b/arch/arm/mach-omap2/vp.h
> @@ -30,7 +30,11 @@ struct voltagedomain;
>  #define OMAP4_VP_VDD_IVA_ID 1
>  #define OMAP4_VP_VDD_MPU_ID 2
>  
> -/* XXX document */
> +/*
> + * Time out for Voltage processor in micro seconds. Typical latency is < 2uS,
> + * but worst case latencies could be around 3-200uS depending on where we
> + * interrupted VP's operation.
> + */
>  #define VP_IDLE_TIMEOUT		200
>  #define VP_TRANXDONE_TIMEOUT	300

Kevin

[PATCH V2 3/4] ARM: OMAP3+: PM: VP: check to ensure VP is idle before forceupdate

[Nishanth Menon]

Ideally in the flow of DVFS programming, VP should be in idle state
(since we disabled it) before entering forceupdate. Ensure that
this is the case. Not doing this could cause VP statemachine
to enter invalid states. Use ratelimited warnings to prevent spam
if VP state machine is stuck.

Cc: Tony Lindgren <tony@atomide.com>
Cc: Kevin Hilman <khilman@ti.com>

Signed-off-by: Vinay Amancha <vinaykumar@ti.com>
Signed-off-by: Nishanth Menon <nm@ti.com>
---
 arch/arm/mach-omap2/vp.c |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm/mach-omap2/vp.c b/arch/arm/mach-omap2/vp.c
index 4723879..2a8a085 100644
--- a/arch/arm/mach-omap2/vp.c
+++ b/arch/arm/mach-omap2/vp.c
@@ -163,6 +163,13 @@ int omap_vp_forceupdate_scale(struct voltagedomain *voltdm,
 	u8 target_vsel, current_vsel;
 	int ret, timeout = 0;
 
+	ret = _vp_wait_for_idle(voltdm, vp);
+	if (ret) {
+		pr_warn_ratelimited("%s: vdd_%s idle timedout (v=%ld)\n",
+				    __func__, voltdm->name, target_volt);
+		return ret;
+	}
+
 	ret = omap_vc_pre_scale(voltdm, target_volt, &target_vsel, &current_vsel);
 	if (ret)
 		return ret;
-- 
1.7.9.5

[PATCH V2 3/4] ARM: OMAP3+: PM: VP: check to ensure VP is idle before forceupdate

[Kevin Hilman]

Nishanth Menon <nm@ti.com> writes:

> Ideally in the flow of DVFS programming, VP should be in idle state
> (since we disabled it) before entering forceupdate. Ensure that
> this is the case. Not doing this could cause VP statemachine
> to enter invalid states. Use ratelimited warnings to prevent spam
> if VP state machine is stuck.
>
> Cc: Tony Lindgren <tony@atomide.com>
> Cc: Kevin Hilman <khilman@ti.com>
>
> Signed-off-by: Vinay Amancha <vinaykumar@ti.com>
> Signed-off-by: Nishanth Menon <nm@ti.com>
> ---
>  arch/arm/mach-omap2/vp.c |    7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/arch/arm/mach-omap2/vp.c b/arch/arm/mach-omap2/vp.c
> index 4723879..2a8a085 100644
> --- a/arch/arm/mach-omap2/vp.c
> +++ b/arch/arm/mach-omap2/vp.c
> @@ -163,6 +163,13 @@ int omap_vp_forceupdate_scale(struct voltagedomain *voltdm,
>  	u8 target_vsel, current_vsel;
>  	int ret, timeout = 0;
>  
> +	ret = _vp_wait_for_idle(voltdm, vp);
> +	if (ret) {
> +		pr_warn_ratelimited("%s: vdd_%s idle timedout (v=%ld)\n",

nit: s/idle timedout/VP idle timeout/

Kevin

> +				    __func__, voltdm->name, target_volt);
> +		return ret;
> +	}
> +
>  	ret = omap_vc_pre_scale(voltdm, target_volt, &target_vsel, &current_vsel);
>  	if (ret)
>  		return ret;

[PATCH V2 4/4] ARM: OMAP3+: PM: VP: ensure VP is idle before disable

[Nishanth Menon]

From: Wenbiao Wang <wwang@ti.com>

Voltage Processor state machine transition to disable need to
occur from IDLE state. When we transition OPP in a functioning
system, the call sequence for an OPP transition is as follows:
omap_sr_disable
      -> sr class 3 disable
           -> vp disable
           -> sr disable
forceupdate to voltage/frequency scale depending on which OPP
we are transitioning to.

If we hit a critical timing window where SR had commanded VP
for a voltage transition and VP is in the middle of operating
on that command, it needs to go through a few states before
going to update state(where it actually sends the command to
VC). Initial view of h/w owners is that the state disable of VP
is expected to be sampled for the next transition.

Instead, to be on a safer side, we ensure that the valid states
of the VP state machine is diligently followed by software. This
can be done by waiting for VP to be in idle  prior to disabling
VP. Existing prints have been updated to ensure context is
available on error messages.

As part of this change, increase timeout for VP idle check to
improbable 500uSec to be certain that system is indeed unable
to continue before crashing out with error(worst case expectancy
remains the same 3-100uSec depending on when we caught VP).

Cc: Tony Lindgren <tony@atomide.com>
Cc: Kevin Hilman <khilman@ti.com>

[nm at ti.com: port from android]
Signed-off-by: Nishanth Menon <nm@ti.com>
Signed-off-by: Wenbiao Wang <wwang@ti.com>
---
 arch/arm/mach-omap2/vp.c |    4 ++++
 arch/arm/mach-omap2/vp.h |    5 +++--
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-omap2/vp.c b/arch/arm/mach-omap2/vp.c
index 2a8a085..9a72deb 100644
--- a/arch/arm/mach-omap2/vp.c
+++ b/arch/arm/mach-omap2/vp.c
@@ -308,6 +308,10 @@ void omap_vp_disable(struct voltagedomain *voltdm)
 		return;
 	}
 
+	if (_vp_wait_for_idle(voltdm, vp)) {
+		pr_warn_ratelimited("%s: vdd_%s timedout!Ignore and try\n",
+				    __func__, voltdm->name);
+	}
 	/* Disable VP */
 	vpconfig = voltdm->read(vp->vpconfig);
 	vpconfig &= ~vp->common->vpconfig_vpenable;
diff --git a/arch/arm/mach-omap2/vp.h b/arch/arm/mach-omap2/vp.h
index 4655b39..4b4eeb6 100644
--- a/arch/arm/mach-omap2/vp.h
+++ b/arch/arm/mach-omap2/vp.h
@@ -33,9 +33,10 @@ struct voltagedomain;
 /*
  * Time out for Voltage processor in micro seconds. Typical latency is < 2uS,
  * but worst case latencies could be around 3-200uS depending on where we
- * interrupted VP's operation.
+ * interrupted VP's operation. Use an improbable timeout value to be
+ * sure that timeout events are beyond doubt.
  */
-#define VP_IDLE_TIMEOUT		200
+#define VP_IDLE_TIMEOUT		500
 #define VP_TRANXDONE_TIMEOUT	300
 
 /**
-- 
1.7.9.5

[PATCH V2 4/4] ARM: OMAP3+: PM: VP: ensure VP is idle before disable

[Kevin Hilman]

Nishanth Menon <nm@ti.com> writes:

> From: Wenbiao Wang <wwang@ti.com>
>
> Voltage Processor state machine transition to disable need to
> occur from IDLE state. When we transition OPP in a functioning
> system, the call sequence for an OPP transition is as follows:
> omap_sr_disable
>       -> sr class 3 disable
>            -> vp disable
>            -> sr disable
> forceupdate to voltage/frequency scale depending on which OPP
> we are transitioning to.
>
> If we hit a critical timing window where SR had commanded VP
> for a voltage transition and VP is in the middle of operating
> on that command, it needs to go through a few states before
> going to update state(where it actually sends the command to
> VC). Initial view of h/w owners is that the state disable of VP
> is expected to be sampled for the next transition.
>
> Instead, to be on a safer side, we ensure that the valid states
> of the VP state machine is diligently followed by software. This
> can be done by waiting for VP to be in idle  prior to disabling
> VP. Existing prints have been updated to ensure context is
> available on error messages.
>
> As part of this change, increase timeout for VP idle check to
> improbable 500uSec to be certain that system is indeed unable
> to continue before crashing out with error(worst case expectancy
> remains the same 3-100uSec depending on when we caught VP).
>
> Cc: Tony Lindgren <tony@atomide.com>
> Cc: Kevin Hilman <khilman@ti.com>
>
> [nm at ti.com: port from android]

and you also convert to use new _vp_wait_for_idle()

> Signed-off-by: Nishanth Menon <nm@ti.com>
> Signed-off-by: Wenbiao Wang <wwang@ti.com>
> ---
>  arch/arm/mach-omap2/vp.c |    4 ++++
>  arch/arm/mach-omap2/vp.h |    5 +++--
>  2 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/vp.c b/arch/arm/mach-omap2/vp.c
> index 2a8a085..9a72deb 100644
> --- a/arch/arm/mach-omap2/vp.c
> +++ b/arch/arm/mach-omap2/vp.c
> @@ -308,6 +308,10 @@ void omap_vp_disable(struct voltagedomain *voltdm)
>  		return;
>  	}
>  
> +	if (_vp_wait_for_idle(voltdm, vp)) {
> +		pr_warn_ratelimited("%s: vdd_%s timedout!Ignore and try\n",

s/timedout/timed out/
no space after '!', also I don't get the "Ignore and try" part

Kevin

> +				    __func__, voltdm->name);
> +	}
>  	/* Disable VP */
>  	vpconfig = voltdm->read(vp->vpconfig);
>  	vpconfig &= ~vp->common->vpconfig_vpenable;
> diff --git a/arch/arm/mach-omap2/vp.h b/arch/arm/mach-omap2/vp.h
> index 4655b39..4b4eeb6 100644
> --- a/arch/arm/mach-omap2/vp.h
> +++ b/arch/arm/mach-omap2/vp.h
> @@ -33,9 +33,10 @@ struct voltagedomain;
>  /*
>   * Time out for Voltage processor in micro seconds. Typical latency is < 2uS,
>   * but worst case latencies could be around 3-200uS depending on where we
> - * interrupted VP's operation.
> + * interrupted VP's operation. Use an improbable timeout value to be
> + * sure that timeout events are beyond doubt.
>   */
> -#define VP_IDLE_TIMEOUT		200
> +#define VP_IDLE_TIMEOUT		500
>  #define VP_TRANXDONE_TIMEOUT	300
>  
>  /**

[PATCH V2 4/4] ARM: OMAP3+: PM: VP: ensure VP is idle before disable

[Menon, Nishanth]

Regards,
Nishanth Menon


On Fri, Jun 1, 2012 at 4:03 PM, Kevin Hilman <khilman@ti.com> wrote:
> Nishanth Menon <nm@ti.com> writes:
>
>> From: Wenbiao Wang <wwang@ti.com>
>>
>> Voltage Processor state machine transition to disable need to
>> occur from IDLE state. When we transition OPP in a functioning
>> system, the call sequence for an OPP transition is as follows:
>> omap_sr_disable
>> ? ? ? -> sr class 3 disable
>> ? ? ? ? ? ?-> vp disable
>> ? ? ? ? ? ?-> sr disable
>> forceupdate to voltage/frequency scale depending on which OPP
>> we are transitioning to.
>>
>> If we hit a critical timing window where SR had commanded VP
>> for a voltage transition and VP is in the middle of operating
>> on that command, it needs to go through a few states before
>> going to update state(where it actually sends the command to
>> VC). Initial view of h/w owners is that the state disable of VP
>> is expected to be sampled for the next transition.
>>
>> Instead, to be on a safer side, we ensure that the valid states
>> of the VP state machine is diligently followed by software. This
>> can be done by waiting for VP to be in idle ?prior to disabling
>> VP. Existing prints have been updated to ensure context is
>> available on error messages.
>>
>> As part of this change, increase timeout for VP idle check to
>> improbable 500uSec to be certain that system is indeed unable
>> to continue before crashing out with error(worst case expectancy
>> remains the same 3-100uSec depending on when we caught VP).
>>
>> Cc: Tony Lindgren <tony@atomide.com>
>> Cc: Kevin Hilman <khilman@ti.com>
>>
>> [nm at ti.com: port from android]
>
> and you also convert to use new _vp_wait_for_idle()
>
>> Signed-off-by: Nishanth Menon <nm@ti.com>
>> Signed-off-by: Wenbiao Wang <wwang@ti.com>
>> ---
>> ?arch/arm/mach-omap2/vp.c | ? ?4 ++++
>> ?arch/arm/mach-omap2/vp.h | ? ?5 +++--
>> ?2 files changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap2/vp.c b/arch/arm/mach-omap2/vp.c
>> index 2a8a085..9a72deb 100644
>> --- a/arch/arm/mach-omap2/vp.c
>> +++ b/arch/arm/mach-omap2/vp.c
>> @@ -308,6 +308,10 @@ void omap_vp_disable(struct voltagedomain *voltdm)
>> ? ? ? ? ? ? ? return;
>> ? ? ? }
>>
>> + ? ? if (_vp_wait_for_idle(voltdm, vp)) {
>> + ? ? ? ? ? ? pr_warn_ratelimited("%s: vdd_%s timedout!Ignore and try\n",
>
> s/timedout/timed out/
> no space after '!',
Kinda wanted to stay under 80 character and not split string out to
two lines and make sparse angry, yet did not want to loose information
which was being presented out.

> also I don't get the "Ignore and try" part

if we fail, just try the disable anyways.. (at least till we have
voltage processor recovery mechanism(cold reset) introduced upstream -
the intent of the patch was not to introduce a recovery mechanism, but
to ensure proper checkpoint is in place)..

>
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? __func__, voltdm->name);
>> + ? ? }
>> ? ? ? /* Disable VP */
>> ? ? ? vpconfig = voltdm->read(vp->vpconfig);
>> ? ? ? vpconfig &= ~vp->common->vpconfig_vpenable;
>> diff --git a/arch/arm/mach-omap2/vp.h b/arch/arm/mach-omap2/vp.h
>> index 4655b39..4b4eeb6 100644
>> --- a/arch/arm/mach-omap2/vp.h
>> +++ b/arch/arm/mach-omap2/vp.h
>> @@ -33,9 +33,10 @@ struct voltagedomain;
>> ?/*
>> ? * Time out for Voltage processor in micro seconds. Typical latency is < 2uS,
>> ? * but worst case latencies could be around 3-200uS depending on where we
>> - * interrupted VP's operation.
>> + * interrupted VP's operation. Use an improbable timeout value to be
>> + * sure that timeout events are beyond doubt.
>> ? */
>> -#define VP_IDLE_TIMEOUT ? ? ? ? ? ? ?200
>> +#define VP_IDLE_TIMEOUT ? ? ? ? ? ? ?500
>> ?#define VP_TRANXDONE_TIMEOUT 300
>>
>> ?/**

[PATCH V2 4/4] ARM: OMAP3+: PM: VP: ensure VP is idle before disable

[Kevin Hilman]

"Menon, Nishanth" <nm@ti.com> writes:

> Regards,
> Nishanth Menon
>
>
> On Fri, Jun 1, 2012 at 4:03 PM, Kevin Hilman <khilman@ti.com> wrote:
>> Nishanth Menon <nm@ti.com> writes:
>>
>>> From: Wenbiao Wang <wwang@ti.com>
>>>
>>> Voltage Processor state machine transition to disable need to
>>> occur from IDLE state. When we transition OPP in a functioning
>>> system, the call sequence for an OPP transition is as follows:
>>> omap_sr_disable
>>> ? ? ? -> sr class 3 disable
>>> ? ? ? ? ? ?-> vp disable
>>> ? ? ? ? ? ?-> sr disable
>>> forceupdate to voltage/frequency scale depending on which OPP
>>> we are transitioning to.
>>>
>>> If we hit a critical timing window where SR had commanded VP
>>> for a voltage transition and VP is in the middle of operating
>>> on that command, it needs to go through a few states before
>>> going to update state(where it actually sends the command to
>>> VC). Initial view of h/w owners is that the state disable of VP
>>> is expected to be sampled for the next transition.
>>>
>>> Instead, to be on a safer side, we ensure that the valid states
>>> of the VP state machine is diligently followed by software. This
>>> can be done by waiting for VP to be in idle ?prior to disabling
>>> VP. Existing prints have been updated to ensure context is
>>> available on error messages.
>>>
>>> As part of this change, increase timeout for VP idle check to
>>> improbable 500uSec to be certain that system is indeed unable
>>> to continue before crashing out with error(worst case expectancy
>>> remains the same 3-100uSec depending on when we caught VP).
>>>
>>> Cc: Tony Lindgren <tony@atomide.com>
>>> Cc: Kevin Hilman <khilman@ti.com>
>>>
>>> [nm at ti.com: port from android]
>>
>> and you also convert to use new _vp_wait_for_idle()
>>
>>> Signed-off-by: Nishanth Menon <nm@ti.com>
>>> Signed-off-by: Wenbiao Wang <wwang@ti.com>
>>> ---
>>> ?arch/arm/mach-omap2/vp.c | ? ?4 ++++
>>> ?arch/arm/mach-omap2/vp.h | ? ?5 +++--
>>> ?2 files changed, 7 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arch/arm/mach-omap2/vp.c b/arch/arm/mach-omap2/vp.c
>>> index 2a8a085..9a72deb 100644
>>> --- a/arch/arm/mach-omap2/vp.c
>>> +++ b/arch/arm/mach-omap2/vp.c
>>> @@ -308,6 +308,10 @@ void omap_vp_disable(struct voltagedomain *voltdm)
>>> ? ? ? ? ? ? ? return;
>>> ? ? ? }
>>>
>>> + ? ? if (_vp_wait_for_idle(voltdm, vp)) {
>>> + ? ? ? ? ? ? pr_warn_ratelimited("%s: vdd_%s timedout!Ignore and try\n",
>>
>> s/timedout/timed out/
>> no space after '!',
> Kinda wanted to stay under 80 character and not split string out to
> two lines and make sparse angry, yet did not want to loose information
> which was being presented out.

Readable error messages are more important.

>> also I don't get the "Ignore and try" part
>
> if we fail, just try the disable anyways.. 
> (at least till we have
> voltage processor recovery mechanism(cold reset) introduced upstream -
> the intent of the patch was not to introduce a recovery mechanism, but
> to ensure proper checkpoint is in place)..

I understand.  My complaint is only about the readability of the error
messages.  Seeing this go by:

        omap_vp_disable: vdd_mpu timedout!Ignore and try

in the kernel logs will still make me ask "try what?"  IMO, it should say

        omap_vp_disable: WARNING: vdd_mpu timed out, ignoring.

Kevin