ARM: OMAP3+: PM: VP: ensure VP is idle before disable
Subsystems (3)
A message that is not in the archive (sent off-list or not yet received): [PATCH] ARM: OMAP3+: PM: VP: ensure VP is idle before disable
PATCH v2 1/1 ARM: OMAP3+: PM: VP: ensure VP is idle before disable
From: Wenbiao Wang <[email protected]>
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 disableforceupdate 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).
---
V2: fix commit message - picked up redundant info from get_maintainer.pl Apologies on the spam
V1: http://marc.info/?l=linux-omap&m=133736493721977&w=2
arch/arm/mach-omap2/vp.c | 15 +++++++++++++-- arch/arm/mach-omap2/vp.h | 2 +- 2 files changed, 14 insertions(+), 3 deletions(-)
arch/arm/mach-omap2/vp.c +13 -2
@@ -262,6 +262,17 @@ void omap_vp_disable(struct voltagedomain *voltdm)return ; } +/* +* Wait for VP idle Typical latency is <2us. Maximum latency is ~100us +* Depending on if we catch VP in the middle of an SR operation. +*/ +omap_test_timeout ((voltdm->read (vp->vstatus)), +VP_IDLE_TIMEOUT , timeout); + +if (timeout >=VP_IDLE_TIMEOUT ) +pr_warning ("%s: vdd_%s idle timedout before disable\n" , + __func__, voltdm->name); +/* Disable VP */ vpconfig = voltdm->read (vp->vpconfig); vpconfig &= ~vp->common->vpconfig_vpenable;
@@ -274,8 +285,8 @@ void omap_vp_disable(struct voltagedomain *voltdm) VP_IDLE_TIMEOUT, timeout);if (timeout >=VP_IDLE_TIMEOUT ) -pr_warning ("%s: vdd_%s idle timedout\n" , - __func__, voltdm->name); +pr_warning ("%s: vdd_%s idle timedout after disable\n" , + __func__, voltdm->name); vp->enabled = false;
arch/arm/mach-omap2/vp.h +1 -1
@@ -31,7 +31,7 @@ struct voltagedomain;#define OMAP4_VP_VDD_MPU_ID 2/* XXX document */ -#define VP_IDLE_TIMEOUT 200 +#define VP_IDLE_TIMEOUT 500#define VP_TRANXDONE_TIMEOUT 300/**
Signature · 1 line
1.7.9.5
PATCH v2 1/1 ARM: OMAP3+: PM: VP: ensure VP is idle before disable
Hello,
Minor suggestion.
Quoted 30 lines · ext Nishanth Menon
From: Wenbiao Wang <[email protected]>
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 disableforceupdate 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).
<cut>
arch/arm/mach-omap2/vp.h struct voltagedomain; · line 34
Quoted 4 lines
/* XXX document */
arch/arm/mach-omap2/vp.h +1
-1-#define VP_IDLE_TIMEOUT 200 +#define VP_IDLE_TIMEOUT 500
I guess it is time to properly document this increasing busy loop delay.. As it is getting closer to ms scale..
Quoted 11 lines
#define
VP_TRANXDONE_TIMEOUT300/**
Signature · 7 lines
1.7.9.5 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel at lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
PATCH v2 1/1 ARM: OMAP3+: PM: VP: ensure VP is idle before disable
Sat, May 19, 2012 at 4:52 AM, Eduardo Valentin <[email protected]>
Quoted 4 lines · wrote:
I guess it is time to properly document this increasing busy loop delay.. As it is getting closer to ms scale..
Does the following sound good?
/* Maximum time for Voltage Processor to enter or exit idle */
Regards,
Nishanth Menon
PATCH v2 1/1 ARM: OMAP3+: PM: VP: ensure VP is idle before disable
Hello,
Quoted 8 lines · ext Nishanth Menon
Sat, May 19, 2012 at 4:52 AM, Eduardo Valentin <[email protected]>
Quoted 4 lines · wrote:
I guess it is time to properly document this increasing busy loop delay.. As it is getting closer to ms scale..
Does the following sound good?
/* Maximum time for Voltage Processor to enter or exit idle */
Sounds way better :-). If you have an estimation of how long it takes in the average case, it might help. But I am OK already with the above, in case you don't have the estimation.
Quoted 3 lines
Regards,
Nishanth Menon
Cheers,
--
Eduardo Valentin
PATCH v2 1/1 ARM: OMAP3+: PM: VP: ensure VP is idle before disable
Quoted 7 lines · Eduardo Valentin
Quoted 4 lines · ext Nishanth Menon
?Sat, May 19, 2012 at 4:52 AM, Eduardo Valentin <[email protected]>
Quoted 2 lines · wrote:
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
I guess it is time to properly document this increasing busy loop delay.. As it is getting closer to ms scale..
Does the following sound good?
/* Maximum time for Voltage Processor to enter or exit idle */Sounds way better :-). If you have an estimation of how long it takes in the average case, it might help. But I am OK already with the above, in case you don't have the estimation.
/*
* Maximum time for Voltage Processor to enter or exit idle
* worst case is around 100uSec depending on when we intercepted VP
* we use 5 times worst case value to be sure that the system flags
* invalid condition
*/
better?
Regards,
Nishanth Menon
PATCH v2 1/1 ARM: OMAP3+: PM: VP: ensure VP is idle before disable
Hello,
Quoted 17 lines · Nishanth
Quoted 7 lines · Eduardo Valentin
Quoted 4 lines · ext Nishanth Menon
?Sat, May 19, 2012 at 4:52 AM, Eduardo Valentin <[email protected]>
Quoted 2 lines · wrote:
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
I guess it is time to properly document this increasing busy loop delay.. As it is getting closer to ms scale..
Does the following sound good?
/* Maximum time for Voltage Processor to enter or exit idle */Sounds way better :-). If you have an estimation of how long it takes in the average case, it might help. But I am OK already with the above, in case you don't have the estimation.
Quoted 7 lines
/*
?* Maximum time for Voltage Processor to enter or exit idle ?* worst case is around 100uSec depending on when we intercepted VP ?* we use 5 times worst case value to be sure that the system flags ?* invalid condition
?*/
better?
Yes! Thanks.
Quoted 3 lines
Regards,
Nishanth Menon
Signature · 1 line
Eduardo Valentin
cover ARM: OMAP3+: VP: collate fixes
In reply to a message not in the archiveReviewing 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 (2):
ARM: OMAP3+: PM: VP: check to ensure VP is idle before forceupdate
ARM: OMAP3+: PM: VP: check only the VPINIDLE status bit
Wenbiao Wang (1):
ARM: OMAP3+: PM: VP: ensure VP is idle before disable
arch/arm/mach-omap2/vp.c | 34 ++++++++++++++++++++++++++++++---- arch/arm/mach-omap2/vp.h | 3 ++- arch/arm/mach-omap2/vp3xxx_data.c | 1 + arch/arm/mach-omap2/vp44xx_data.c | 1 + 4 files changed, 34 insertions(+), 5 deletions(-)
Cc: Tony Lindgren <[email protected]>
Cc: Kevin Hilman <[email protected]>
Cc: linux-omap at vger.kernel.org
Cc: linux-arm-kernel at lists.infradead.org
Regards,
Nishanth Menon
Signature · 1 line
1.7.9.5
PATCH 1/3 ARM: OMAP3+: PM: VP: ensure VP is idle before disable
From: Wenbiao Wang <[email protected]>
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 disableforceupdate 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).
--- arch/arm/mach-omap2/vp.c | 15 +++++++++++++-- arch/arm/mach-omap2/vp.h | 2 +- 2 files changed, 14 insertions(+), 3 deletions(-)
arch/arm/mach-omap2/vp.c +13 -2
@@ -262,6 +262,17 @@ void omap_vp_disable(struct voltagedomain *voltdm)return ; } +/* +* Wait for VP idle Typical latency is <2us. Maximum latency is ~100us +* Depending on if we catch VP in the middle of an SR operation. +*/ +omap_test_timeout ((voltdm->read (vp->vstatus)), +VP_IDLE_TIMEOUT , timeout); + +if (timeout >=VP_IDLE_TIMEOUT ) +pr_warning ("%s: vdd_%s idle timedout before disable\n" , + __func__, voltdm->name); +/* Disable VP */ vpconfig = voltdm->read (vp->vpconfig); vpconfig &= ~vp->common->vpconfig_vpenable;
@@ -274,8 +285,8 @@ void omap_vp_disable(struct voltagedomain *voltdm) VP_IDLE_TIMEOUT, timeout);if (timeout >=VP_IDLE_TIMEOUT ) -pr_warning ("%s: vdd_%s idle timedout\n" , - __func__, voltdm->name); +pr_warning ("%s: vdd_%s idle timedout after disable\n" , + __func__, voltdm->name); vp->enabled = false;
arch/arm/mach-omap2/vp.h +1 -1
@@ -31,7 +31,7 @@ struct voltagedomain;#define OMAP4_VP_VDD_MPU_ID 2/* XXX document */ -#define VP_IDLE_TIMEOUT 200 +#define VP_IDLE_TIMEOUT 500#define VP_TRANXDONE_TIMEOUT 300/**
Signature · 1 line
1.7.9.5
PATCH 1/3 ARM: OMAP3+: PM: VP: ensure VP is idle before disable
Quoted 52 lines · Nishanth Menon
From: Wenbiao Wang <[email protected]>
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 disableforceupdate 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 <[email protected]>Cc Kevin Hilman <[email protected]>Cc linux-omap at vger.kernel.orgCc linux-arm-kernel at lists.infradead.org[nm at ti.com: port from android]Signed-off-by Nishanth Menon <[email protected]>Signed-off-by Wenbiao Wang <[email protected]>--- arch/arm/mach-omap2/vp.c | 15 +++++++++++++-- arch/arm/mach-omap2/vp.h | 2 +- 2 files changed, 14 insertions(+), 3 deletions(-)arch/arm/mach-omap2/vp.c +1
-0@@ -262,6 +262,17 @@ void omap_vp_disable(struct voltagedomain *voltdm)return ; } +/*
arch/arm/mach-omap2/vp.c void omap_vp_disable(struct voltagedomain *voltdm) · line 275
Quoted 10 lines
arch/arm/mach-omap2/vp.c +10
-0+* Wait for VP idle Typical latency is <2us. Maximum latency is ~100us +* Depending on if we catch VP in the middle of an SR operation. +*/ +omap_test_timeout ((voltdm->read (vp->vstatus)), +VP_IDLE_TIMEOUT , timeout); + +if (timeout >=VP_IDLE_TIMEOUT ) +pr_warning ("%s: vdd_%s idle timedout before disable\n" , + __func__, voltdm->name); +
Shouldn't this bail out here instead of continuing to disable VP?
It would be even better to have some error recovery (or at least clean failure) here rather than just printing a warning and continuing.
Quoted 19 lines
arch/arm/mach-omap2/vp.c +2
-2/* Disable VP */ vpconfig = voltdm->read (vp->vpconfig); vpconfig &= ~vp->common->vpconfig_vpenable;@@ -274,8 +285,8 @@ void omap_vp_disable(struct voltagedomain *voltdm) VP_IDLE_TIMEOUT, timeout);if (timeout >=VP_IDLE_TIMEOUT ) -pr_warning ("%s: vdd_%s idle timedout\n" , - __func__, voltdm->name); +pr_warning ("%s: vdd_%s idle timedout after disable\n" , + __func__, voltdm->name); vp->enabled = false;arch/arm/mach-omap2/vp.h +0
-0
arch/arm/mach-omap2/vp.h struct voltagedomain; · line 34
Quoted 5 lines
arch/arm/mach-omap2/vp.h +1
-1@@ -31,7 +31,7 @@ struct voltagedomain;#define OMAP4_VP_VDD_MPU_ID 2/* XXX document */ -#define VP_IDLE_TIMEOUT 200 +#define VP_IDLE_TIMEOUT 500
Were you planning to insert the comment here as suggested by Eduardo?
Kevin
Quoted 3 lines
arch/arm/mach-omap2/vp.h +0
-0#define VP_TRANXDONE_TIMEOUT 300/**
PATCH 1/3 ARM: OMAP3+: PM: VP: ensure VP is idle before disable
Quoted 11 lines · Kevin Hilman
Quoted 10 lines
arch/arm/mach-omap2/vp.h +0
-0Quoted 9 lines
index 7c155d2..0abf895 100644
---a/arch/arm/mach-omap2/vp.h
+++b/arch/arm/mach-omap2/vp.h@@ -31,7 +31,7 @@ struct voltagedomain; #define OMAP4_VP_VDD_MPU_ID 2 /* XXX document */-#define
VP_IDLE_TIMEOUT200
+#defineVP_IDLE_TIMEOUT500Were you planning to insert the comment here as suggested by Eduardo?
Oops, ignore this comment. I see you posted a v3 with this added.
Sorry,
Kevin
PATCH 1/3 ARM: OMAP3+: PM: VP: ensure VP is idle before disable
In reply to Kevin HilmanQuoted 53 lines · Kevin Hilman
Quoted 51 lines · Nishanth Menon
From: Wenbiao Wang <[email protected]>
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 disableforceupdate 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 <[email protected]>Cc Kevin Hilman <[email protected]>Cc linux-omap at vger.kernel.orgCc linux-arm-kernel at lists.infradead.org[nm at ti.com: port from android]Signed-off-by Nishanth Menon <[email protected]>Signed-off-by Wenbiao Wang <[email protected]>--- arch/arm/mach-omap2/vp.c | 15 +++++++++++++-- arch/arm/mach-omap2/vp.h | 2 +- 2 files changed, 14 insertions(+), 3 deletions(-)arch/arm/mach-omap2/vp.c +0
-0@@ -262,6 +262,17 @@ void omap_vp_disable(struct voltagedomain *voltdm)return ; }
Quoted 13 lines
arch/arm/mach-omap2/vp.c void omap_vp_disable(struct voltagedomain *voltdm) · line 275
Quoted 11 lines
arch/arm/mach-omap2/vp.c +11
-0+/* +* Wait for VP idle Typical latency is <2us. Maximum latency is ~100us +* Depending on if we catch VP in the middle of an SR operation. +*/ +omap_test_timeout ((voltdm->read (vp->vstatus)), +VP_IDLE_TIMEOUT , timeout); + +if (timeout >=VP_IDLE_TIMEOUT ) +pr_warning ("%s: vdd_%s idle timedout before disable\n" , + __func__, voltdm->name); +Shouldn't this bail out here instead of continuing to disable VP?
yes - but then the entire voltage path is without error handling :( I think the cleanup should be done independent of this patch - just doing a return at this point might actually not effective.
Quoted 3 lines
It would be even better to have some error recovery (or at least clean failure) here rather than just printing a warning and continuing.
Once VP does not idle, it is an indication that something seriously has gone wrong in the VP state machine. VP in PRM module is reset only on cold reset - so even a traditional arch_reset would not help us here.
The intent here is to be completely sure that VP is in idle state before we trigger a disable. With the cleanup of all error handling flows, we might be able to flag all the way back up to calling event (a DVFS event) and may even be able to trigger appropriate recovery mechanism.
Quoted 22 lines
Quoted 22 lines
arch/arm/mach-omap2/vp.c +2
-2/* Disable VP */ vpconfig = voltdm->read (vp->vpconfig); vpconfig &= ~vp->common->vpconfig_vpenable;@@ -274,8 +285,8 @@ void omap_vp_disable(struct voltagedomain *voltdm) VP_IDLE_TIMEOUT, timeout);if (timeout >=VP_IDLE_TIMEOUT ) -pr_warning ("%s: vdd_%s idle timedout\n" , - __func__, voltdm->name); +pr_warning ("%s: vdd_%s idle timedout after disable\n" , + __func__, voltdm->name); vp->enabled = false;arch/arm/mach-omap2/vp.h +0
-0@@ -31,7 +31,7 @@ struct voltagedomain;#define OMAP4_VP_VDD_MPU_ID 2
Quoted 5 lines
arch/arm/mach-omap2/vp.h struct voltagedomain; · line 34
Quoted 3 lines
arch/arm/mach-omap2/vp.h +1
-1/* XXX document */ -#define VP_IDLE_TIMEOUT 200 +#define VP_IDLE_TIMEOUT 500Were you planning to insert the comment here as suggested by Eduardo?
yep. waiting in case we have more comments :)
---
Regards,
Nishanth Menon
PATCH 2/3 ARM: OMAP3+: PM: VP: check to ensure VP is idle before forceupdate
In reply to Nishanth MenonIdeally 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.
--- arch/arm/mach-omap2/vp.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
arch/arm/mach-omap2/vp.c +12 -0
@@ -123,6 +123,18 @@ int omap_vp_forceupdate_scale(struct voltagedomain *voltdm,u8 target_vsel, current_vsel;int ret, timeout =0 ; +/* +* Wait for VP idle Typical latency is <2us. Maximum latency is ~100us +* This is an additional allowance to ensure we are in proper state +* to enter into forceupdate state transition. +*/ +omap_test_timeout ((voltdm->read (vp->vstatus)), +VP_IDLE_TIMEOUT , timeout); + +if (timeout >=VP_IDLE_TIMEOUT ) +pr_warning ("%s: vdd_%s idle timedout forceupdate(v=%ld)\n" , + __func__, voltdm->name, target_volt); + ret =omap_vc_pre_scale (voltdm, target_volt, &target_vsel, ¤t_vsel);if (ret)return ret;
Signature · 1 line
1.7.9.5
PATCH 2/3 ARM: OMAP3+: PM: VP: check to ensure VP is idle before forceupdate
Quoted 21 lines · 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.
Cc Tony Lindgren <[email protected]>Cc Kevin Hilman <[email protected]>Cc linux-omap at vger.kernel.orgCc linux-arm-kernel at lists.infradead.orgSigned-off-by Vinay Amancha <[email protected]>Signed-off-by Nishanth Menon <[email protected]>--- arch/arm/mach-omap2/vp.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)arch/arm/mach-omap2/vp.c +0
-0
arch/arm/mach-omap2/vp.c int omap_vp_forceupdate_scale(struct voltagedomain *voltdm, · line 126
Quoted 4 lines
arch/arm/mach-omap2/vp.c +1
-0@@ -123,6 +123,18 @@ int omap_vp_forceupdate_scale(struct voltagedomain *voltdm,u8 target_vsel, current_vsel;int ret, timeout =0 ; +/*
missing indent
arch/arm/mach-omap2/vp.c int omap_vp_forceupdate_scale(struct voltagedomain *voltdm, · line 137
Quoted 11 lines
arch/arm/mach-omap2/vp.c +11
-0+* Wait for VP idle Typical latency is <2us. Maximum latency is ~100us +* This is an additional allowance to ensure we are in proper state +* to enter into forceupdate state transition. +*/ +omap_test_timeout ((voltdm->read (vp->vstatus)), +VP_IDLE_TIMEOUT , timeout); + +if (timeout >=VP_IDLE_TIMEOUT ) +pr_warning ("%s: vdd_%s idle timedout forceupdate(v=%ld)\n" , + __func__, voltdm->name, target_volt); +
Again, some clean failure and error recovery should be done here instead of just a print and bail.
Kevin
Quoted 3 lines
arch/arm/mach-omap2/vp.c +0
-0ret =omap_vc_pre_scale (voltdm, target_volt, &target_vsel, ¤t_vsel);if (ret)return ret;
PATCH 2/3 ARM: OMAP3+: PM: VP: check to ensure VP is idle before forceupdate
Quoted 26 lines · Kevin Hilman
Quoted 24 lines · 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.
Cc Tony Lindgren <[email protected]>Cc Kevin Hilman <[email protected]>Cc linux-omap at vger.kernel.orgCc linux-arm-kernel at lists.infradead.orgSigned-off-by Vinay Amancha <[email protected]>Signed-off-by Nishanth Menon <[email protected]>--- arch/arm/mach-omap2/vp.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)arch/arm/mach-omap2/vp.c +0
-0@@ -123,6 +123,18 @@ int omap_vp_forceupdate_scale(struct voltagedomain *voltdm,u8 target_vsel, current_vsel;int ret, timeout =0 ;
Quoted 3 lines
arch/arm/mach-omap2/vp.c int omap_vp_forceupdate_scale(struct voltagedomain *voltdm, · line 126
Quoted 1 line
arch/arm/mach-omap2/vp.c +1
-0+/* missing indent
oops. will fix
Quoted 15 lines
arch/arm/mach-omap2/vp.c int omap_vp_forceupdate_scale(struct voltagedomain *voltdm, · line 137
Quoted 12 lines
arch/arm/mach-omap2/vp.c +11
-0+* Wait for VP idle Typical latency is <2us. Maximum latency is ~100us +* This is an additional allowance to ensure we are in proper state +* to enter into forceupdate state transition. +*/ +omap_test_timeout ((voltdm->read (vp->vstatus)), +VP_IDLE_TIMEOUT , timeout); + +if (timeout >=VP_IDLE_TIMEOUT ) +pr_warning ("%s: vdd_%s idle timedout forceupdate(v=%ld)\n" , + __func__, voltdm->name, target_volt); +Again, some clean failure and error recovery should be done here instead of just a print and bail.
thx - we should at least refuse to do forceupdate at this point.
Signature · 2 lines
Regards, Nishanth Menon
PATCH 3/3 ARM: OMAP3+: PM: VP: check only the VPINIDLE status bit
In reply to Nishanth MenonCurrently 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.
--- arch/arm/mach-omap2/vp.c | 15 +++++++++------ 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, 12 insertions(+), 6 deletions(-)
arch/arm/mach-omap2/vp.c +9 -6
@@ -128,8 +128,9 @@ int omap_vp_forceupdate_scale(struct voltagedomain *voltdm,* This is an additional allowance to ensure we are in proper state * to enter into forceupdate state transition. */ -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 forceupdate(v=%ld)\n",
@@ -278,8 +279,9 @@ void omap_vp_disable(struct voltagedomain *voltdm)* Wait for VP idle Typical latency is <2us. Maximum latency is ~100us * Depending on if we catch VP in the middle of an SR operation. */ -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 before disable\n",
@@ -293,8 +295,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 after disable\n",
arch/arm/mach-omap2/vp.h +1 -0
@@ -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;
arch/arm/mach-omap2/vp3xxx_data.c +1 -0
@@ -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,
arch/arm/mach-omap2/vp44xx_data.c +1 -0
@@ -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,
Signature · 1 line
1.7.9.5
PATCH 3/3 ARM: OMAP3+: PM: VP: check only the VPINIDLE status bit
Quoted 3 lines · 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.
ouch
Quoted 9 lines
Hence, check against purely the vpinidle status bit.
Cc Tony Lindgren <[email protected]>Cc Kevin Hilman <[email protected]>Cc linux-omap at vger.kernel.orgCc linux-arm-kernel at lists.infradead.orgReported-by Vinay Amancha <[email protected]>Signed-off-by Nishanth Menon <[email protected]>
Thanks for the fix.
Looking at this combined with the previous patches, since this same thing is done several places, it looks like we need a helper function 'omap_vp_wait_for_idle()' or similar. That function will do the check, wait for timeout, and return an error if failure. The callers can than do appropriate error handling upon failure.
Thanks,
Kevin
Quoted 83 lines
--- arch/arm/mach-omap2/vp.c | 15 +++++++++------ 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, 12 insertions(+), 6 deletions(-)arch/arm/mach-omap2/vp.c +9
-6@@ -128,8 +128,9 @@ int omap_vp_forceupdate_scale(struct voltagedomain *voltdm,* This is an additional allowance to ensure we are in proper state * to enter into forceupdate state transition. */ -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 forceupdate(v=%ld)\n",@@ -278,8 +279,9 @@ void omap_vp_disable(struct voltagedomain *voltdm)* Wait for VP idle Typical latency is <2us. Maximum latency is ~100us * Depending on if we catch VP in the middle of an SR operation. */ -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 before disable\n",@@ -293,8 +295,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 after disable\n",arch/arm/mach-omap2/vp.h +1
-0@@ -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;arch/arm/mach-omap2/vp3xxx_data.c +1
-0@@ -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,arch/arm/mach-omap2/vp44xx_data.c +1
-0@@ -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,
PATCH 3/3 ARM: OMAP3+: PM: VP: check only the VPINIDLE status bit
Quoted 18 lines · Kevin Hilman
Quoted 3 lines · 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.
ouch
Quoted 9 lines
Hence, check against purely the vpinidle status bit.
Cc Tony Lindgren <[email protected]>Cc Kevin Hilman <[email protected]>Cc linux-omap at vger.kernel.orgCc linux-arm-kernel at lists.infradead.orgReported-by Vinay Amancha <[email protected]>Signed-off-by Nishanth Menon <[email protected]>
Quoted 8 lines
Thanks for the fix.
Looking at this combined with the previous patches, since this same thing is done several places, it looks like we need a helper function '
omap_vp_wait_for_idle()' or similar. That function will do the check, wait for timeout, and return an error if failure. The callers can than do appropriate error handling upon failure.
yep - i think a static inline should do just fine here considering just a test_timeout is involved.
will post out a new series in a few mins.
Signature · 2 lines
Regards, Nishanth Menon