[PATCH] OMAP3: PRCM interrupt: only check for enabled PRCM IRQs

[PATCH] OMAP3: PRCM interrupt: only check for enabled PRCM IRQs

[Kevin Hilman]

While handling PRCM IRQs, mask out interrupts that are not enabled in
PRM_IRQENABLE_MPU.  If these are not masked out,  non-enabled
interrupts are caught and a WARN() is dumped.

This was noticed using SmartReflex transitions which cause the VPx_*
interrupts to be handled since they are set in PRM_IRQSTATUS_MPU even
but not enabled in PRM_IRQENABLE_MPU.

Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
---
For review here, will be queued with other PM updates for 2.6.35.

 arch/arm/mach-omap2/pm34xx.c |   15 ++++++++++-----
 1 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
index fee2efb..63d1843 100644
--- a/arch/arm/mach-omap2/pm34xx.c
+++ b/arch/arm/mach-omap2/pm34xx.c
@@ -266,13 +266,15 @@ static int _prcm_int_handle_wakeup(void)
  */
 static irqreturn_t prcm_interrupt_handler (int irq, void *dev_id)
 {
-	u32 irqstatus_mpu;
+	u32 irqenable_mpu, irqstatus_mpu;
 	int c = 0;
 
-	do {
-		irqstatus_mpu = prm_read_mod_reg(OCP_MOD,
-					OMAP3_PRM_IRQSTATUS_MPU_OFFSET);
+	irqenable_mpu = prm_read_mod_reg(OCP_MOD,
+					 OMAP3_PRM_IRQENABLE_MPU_OFFSET);
+	irqstatus_mpu = prm_read_mod_reg(OCP_MOD,
+					 OMAP3_PRM_IRQSTATUS_MPU_OFFSET);
 
+	do {
 		if (irqstatus_mpu & (OMAP3430_WKUP_ST | OMAP3430_IO_ST)) {
 			c = _prcm_int_handle_wakeup();
 
@@ -291,7 +293,10 @@ static irqreturn_t prcm_interrupt_handler (int irq, void *dev_id)
 		prm_write_mod_reg(irqstatus_mpu, OCP_MOD,
 					OMAP3_PRM_IRQSTATUS_MPU_OFFSET);
 
-	} while (prm_read_mod_reg(OCP_MOD, OMAP3_PRM_IRQSTATUS_MPU_OFFSET));
+		irqstatus_mpu = prm_read_mod_reg(OCP_MOD,
+					OMAP3_PRM_IRQSTATUS_MPU_OFFSET);
+		
+	} while (irqstatus_mpu & irqenable_mpu);
 
 	return IRQ_HANDLED;
 }
-- 
1.7.0.2

Re: [PATCH] OMAP3: PRCM interrupt: only check for enabled PRCM IRQs

[Mike Turquette]

Kevin Hilman wrote:
> While handling PRCM IRQs, mask out interrupts that are not enabled in
> PRM_IRQENABLE_MPU.  If these are not masked out,  non-enabled
> interrupts are caught and a WARN() is dumped.
> 
> This was noticed using SmartReflex transitions which cause the VPx_*
> interrupts to be handled since they are set in PRM_IRQSTATUS_MPU even
> but not enabled in PRM_IRQENABLE_MPU.
> 
> Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
> ---
> For review here, will be queued with other PM updates for 2.6.35.
> 
>  arch/arm/mach-omap2/pm34xx.c |   15 ++++++++++-----
>  1 files changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
> index fee2efb..63d1843 100644
> --- a/arch/arm/mach-omap2/pm34xx.c
> +++ b/arch/arm/mach-omap2/pm34xx.c
> @@ -266,13 +266,15 @@ static int _prcm_int_handle_wakeup(void)
>   */
>  static irqreturn_t prcm_interrupt_handler (int irq, void *dev_id)
>  {
> -	u32 irqstatus_mpu;
> +	u32 irqenable_mpu, irqstatus_mpu;
>  	int c = 0;
>  
> -	do {
> -		irqstatus_mpu = prm_read_mod_reg(OCP_MOD,
> -					OMAP3_PRM_IRQSTATUS_MPU_OFFSET);
> +	irqenable_mpu = prm_read_mod_reg(OCP_MOD,
> +					 OMAP3_PRM_IRQENABLE_MPU_OFFSET);
> +	irqstatus_mpu = prm_read_mod_reg(OCP_MOD,
> +					 OMAP3_PRM_IRQSTATUS_MPU_OFFSET);
>  
> +	do {
>  		if (irqstatus_mpu & (OMAP3430_WKUP_ST | OMAP3430_IO_ST)) {
>  			c = _prcm_int_handle_wakeup();
>  
> @@ -291,7 +293,10 @@ static irqreturn_t prcm_interrupt_handler (int irq, void *dev_id)
>  		prm_write_mod_reg(irqstatus_mpu, OCP_MOD,
>  					OMAP3_PRM_IRQSTATUS_MPU_OFFSET);

I think that this is partially correct.  Only iterating for status bits 
that correspond to enabled interrupts is correct.  However I think 
clearing all of the status bits indiscriminately is bad.

The root of the problem is that status bits get set regardless of 
whether the IRQ is enabled (which defies common assumptions).  I have 
seen the exact same issue on Android after introducing ABB and I just 
stopped clearing the register altogether:

http://review.omapzoom.org/#patch,sidebyside,2675,2,arch/arm/mach-omap2/pm34xx.c

This turned about to be a bad idea; for instance VP1_TRANXDONE_ST and 
ABB_TRANXDONE_ST are currently polled (no interrupt is enabled).  With 
the above code it is possible to have a vpforceupdate voltage change or 
an ABB OPP change, then get interrupted, and then have the timeouts for 
the polled code fail after the interrupt is served since those status 
bits got cleared resulting in wasted poll loops and false-positive 
WARNs.  This has been observed.

I think a better solution for both code bases might be,

prm_write_mod_reg((irqstatus_mpu | irqenable_mpu), OCP_MOD,
			OMAP3_PRM_MPU_OFFSET);

Regards,
Mike

>  
> -	} while (prm_read_mod_reg(OCP_MOD, OMAP3_PRM_IRQSTATUS_MPU_OFFSET));
> +		irqstatus_mpu = prm_read_mod_reg(OCP_MOD,
> +					OMAP3_PRM_IRQSTATUS_MPU_OFFSET);
> +		
> +	} while (irqstatus_mpu & irqenable_mpu);
>  
>  	return IRQ_HANDLED;
>  }

Re: [PATCH] OMAP3: PRCM interrupt: only check for enabled PRCM IRQs

[Mike Turquette]

Turquette, Mike wrote:
> Kevin Hilman wrote:
>> While handling PRCM IRQs, mask out interrupts that are not enabled in
>> PRM_IRQENABLE_MPU.  If these are not masked out,  non-enabled
>> interrupts are caught and a WARN() is dumped.
>>
>> This was noticed using SmartReflex transitions which cause the VPx_*
>> interrupts to be handled since they are set in PRM_IRQSTATUS_MPU even
>> but not enabled in PRM_IRQENABLE_MPU.
>>
>> Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
>> ---
>> For review here, will be queued with other PM updates for 2.6.35.
>>
>>  arch/arm/mach-omap2/pm34xx.c |   15 ++++++++++-----
>>  1 files changed, 10 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
>> index fee2efb..63d1843 100644
>> --- a/arch/arm/mach-omap2/pm34xx.c
>> +++ b/arch/arm/mach-omap2/pm34xx.c
>> @@ -266,13 +266,15 @@ static int _prcm_int_handle_wakeup(void)
>>   */
>>  static irqreturn_t prcm_interrupt_handler (int irq, void *dev_id)
>>  {
>> -	u32 irqstatus_mpu;
>> +	u32 irqenable_mpu, irqstatus_mpu;
>>  	int c = 0;
>>  
>> -	do {
>> -		irqstatus_mpu = prm_read_mod_reg(OCP_MOD,
>> -					OMAP3_PRM_IRQSTATUS_MPU_OFFSET);
>> +	irqenable_mpu = prm_read_mod_reg(OCP_MOD,
>> +					 OMAP3_PRM_IRQENABLE_MPU_OFFSET);
>> +	irqstatus_mpu = prm_read_mod_reg(OCP_MOD,
>> +					 OMAP3_PRM_IRQSTATUS_MPU_OFFSET);
>>  
>> +	do {
>>  		if (irqstatus_mpu & (OMAP3430_WKUP_ST | OMAP3430_IO_ST)) {
>>  			c = _prcm_int_handle_wakeup();
>>  
>> @@ -291,7 +293,10 @@ static irqreturn_t prcm_interrupt_handler (int irq, void *dev_id)
>>  		prm_write_mod_reg(irqstatus_mpu, OCP_MOD,
>>  					OMAP3_PRM_IRQSTATUS_MPU_OFFSET);
> 
> I think that this is partially correct.  Only iterating for status bits 
> that correspond to enabled interrupts is correct.  However I think 
> clearing all of the status bits indiscriminately is bad.
> 
> The root of the problem is that status bits get set regardless of 
> whether the IRQ is enabled (which defies common assumptions).  I have 
> seen the exact same issue on Android after introducing ABB and I just 
> stopped clearing the register altogether:
> 
> http://review.omapzoom.org/#patch,sidebyside,2675,2,arch/arm/mach-omap2/pm34xx.c

OOPS, misleading typo above.  I meant to say that "I stopped WARNing all 
together", but I still cleared the register indiscriminately, which 
turns out is bad.

Mike

> 
> This turned about to be a bad idea; for instance VP1_TRANXDONE_ST and 
> ABB_TRANXDONE_ST are currently polled (no interrupt is enabled).  With 
> the above code it is possible to have a vpforceupdate voltage change or 
> an ABB OPP change, then get interrupted, and then have the timeouts for 
> the polled code fail after the interrupt is served since those status 
> bits got cleared resulting in wasted poll loops and false-positive 
> WARNs.  This has been observed.
> 
> I think a better solution for both code bases might be,
> 
> prm_write_mod_reg((irqstatus_mpu | irqenable_mpu), OCP_MOD,
> 			OMAP3_PRM_MPU_OFFSET);
> 
> Regards,
> Mike
> 
>>  
>> -	} while (prm_read_mod_reg(OCP_MOD, OMAP3_PRM_IRQSTATUS_MPU_OFFSET));
>> +		irqstatus_mpu = prm_read_mod_reg(OCP_MOD,
>> +					OMAP3_PRM_IRQSTATUS_MPU_OFFSET);
>> +		
>> +	} while (irqstatus_mpu & irqenable_mpu);
>>  
>>  	return IRQ_HANDLED;
>>  }
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] OMAP3: PRCM interrupt: only check for enabled PRCM IRQs

[Kevin Hilman]

Mike Turquette <mturquette@ti.com> writes:

> Kevin Hilman wrote:
>> While handling PRCM IRQs, mask out interrupts that are not enabled in
>> PRM_IRQENABLE_MPU.  If these are not masked out,  non-enabled
>> interrupts are caught and a WARN() is dumped.
>>
>> This was noticed using SmartReflex transitions which cause the VPx_*
>> interrupts to be handled since they are set in PRM_IRQSTATUS_MPU even
>> but not enabled in PRM_IRQENABLE_MPU.
>>
>> Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
>> ---
>> For review here, will be queued with other PM updates for 2.6.35.
>>
>>  arch/arm/mach-omap2/pm34xx.c |   15 ++++++++++-----
>>  1 files changed, 10 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
>> index fee2efb..63d1843 100644
>> --- a/arch/arm/mach-omap2/pm34xx.c
>> +++ b/arch/arm/mach-omap2/pm34xx.c
>> @@ -266,13 +266,15 @@ static int _prcm_int_handle_wakeup(void)
>>   */
>>  static irqreturn_t prcm_interrupt_handler (int irq, void *dev_id)
>>  {
>> -	u32 irqstatus_mpu;
>> +	u32 irqenable_mpu, irqstatus_mpu;
>>  	int c = 0;
>>  -	do {
>> -		irqstatus_mpu = prm_read_mod_reg(OCP_MOD,
>> -					OMAP3_PRM_IRQSTATUS_MPU_OFFSET);
>> +	irqenable_mpu = prm_read_mod_reg(OCP_MOD,
>> +					 OMAP3_PRM_IRQENABLE_MPU_OFFSET);
>> +	irqstatus_mpu = prm_read_mod_reg(OCP_MOD,
>> +					 OMAP3_PRM_IRQSTATUS_MPU_OFFSET);
>>  +	do {
>>  		if (irqstatus_mpu & (OMAP3430_WKUP_ST | OMAP3430_IO_ST)) {
>>  			c = _prcm_int_handle_wakeup();
>>  @@ -291,7 +293,10 @@ static irqreturn_t prcm_interrupt_handler (int
>> irq, void *dev_id)
>>  		prm_write_mod_reg(irqstatus_mpu, OCP_MOD,
>>  					OMAP3_PRM_IRQSTATUS_MPU_OFFSET);
>
> I think that this is partially correct.  Only iterating for status
> bits that correspond to enabled interrupts is correct.  However I
> think clearing all of the status bits indiscriminately is bad.
>
> The root of the problem is that status bits get set regardless of
> whether the IRQ is enabled (which defies common assumptions).  I have
> seen the exact same issue on Android after introducing ABB and I just
> stopped clearing the register altogether:
>
> http://review.omapzoom.org/#patch,sidebyside,2675,2,arch/arm/mach-omap2/pm34xx.c
>
> This turned about to be a bad idea; for instance VP1_TRANXDONE_ST and
> ABB_TRANXDONE_ST are currently polled (no interrupt is enabled).  With
> the above code it is possible to have a vpforceupdate voltage change
> or an ABB OPP change, then get interrupted, and then have the timeouts
> for the polled code fail after the interrupt is served since those
> status bits got cleared resulting in wasted poll loops and
> false-positive WARNs.  This has been observed.
>
> I think a better solution for both code bases might be,
>
> prm_write_mod_reg((irqstatus_mpu | irqenable_mpu), OCP_MOD,
> 			OMAP3_PRM_MPU_OFFSET);

Good, thanks for the review.  I was wondering about that and decided
to keep the existing behavior.  I actually meant to add an RFC for
that in the changelog, so thanks for catching that.

Looking closer at the new SmartReflex code, it's also checking that
register and polling for transaction done, so clearing the bits in the
ISR would likely cause SmartReflex to miss some of those events.

Good catch, below is an updated version.

Thara, could you try this out with your v3 smartreflex series?  You
can just test current pm-wip-sr branch which is based on a PM branch
with this change.

Kevin


commit cfabe8a950e252d26cdeb4a9bb11e2cabb2a50c6
Author: Kevin Hilman <khilman@deeprootsystems.com>
Date:   Mon Apr 26 14:59:09 2010 -0700

    OMAP3: PRCM interrupt: only check and clear enabled PRCM IRQs
    
    While handling PRCM IRQs, mask out interrupts that are not enabled in
    PRM_IRQENABLE_MPU.  If these are not masked out, non-enabled
    interrupts are caught, a WARN() is printed due to no 'handler' and the
    events are cleared.  In addition to being noisy, this can also
    interfere with independent polling of this register by SR/VP code.
    
    This was noticed using SmartReflex transitions which cause the VPx_*
    interrupts to be handled since they are set in PRM_IRQSTATUS_MPU even
    but not enabled in PRM_IRQENABLE_MPU.
    
    Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>

diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
index fee2efb..c38016b 100644
--- a/arch/arm/mach-omap2/pm34xx.c
+++ b/arch/arm/mach-omap2/pm34xx.c
@@ -266,13 +266,16 @@ static int _prcm_int_handle_wakeup(void)
  */
 static irqreturn_t prcm_interrupt_handler (int irq, void *dev_id)
 {
-	u32 irqstatus_mpu;
+	u32 irqenable_mpu, irqstatus_mpu;
 	int c = 0;
 
-	do {
-		irqstatus_mpu = prm_read_mod_reg(OCP_MOD,
-					OMAP3_PRM_IRQSTATUS_MPU_OFFSET);
+	irqenable_mpu = prm_read_mod_reg(OCP_MOD,
+					 OMAP3_PRM_IRQENABLE_MPU_OFFSET);
+	irqstatus_mpu = prm_read_mod_reg(OCP_MOD,
+					 OMAP3_PRM_IRQSTATUS_MPU_OFFSET);
+	irqstatus_mpu &= irqenable_mpu;
 
+	do {
 		if (irqstatus_mpu & (OMAP3430_WKUP_ST | OMAP3430_IO_ST)) {
 			c = _prcm_int_handle_wakeup();
 
@@ -291,7 +294,11 @@ static irqreturn_t prcm_interrupt_handler (int irq, void *dev_id)
 		prm_write_mod_reg(irqstatus_mpu, OCP_MOD,
 					OMAP3_PRM_IRQSTATUS_MPU_OFFSET);
 
-	} while (prm_read_mod_reg(OCP_MOD, OMAP3_PRM_IRQSTATUS_MPU_OFFSET));
+		irqstatus_mpu = prm_read_mod_reg(OCP_MOD,
+					OMAP3_PRM_IRQSTATUS_MPU_OFFSET);
+		irqstatus_mpu &= irqenable_mpu;
+
+	} while (irqstatus_mpu);
 
 	return IRQ_HANDLED;
 }

Re: [PATCH] OMAP3: PRCM interrupt: only check for enabled PRCM IRQs

[Mike Turquette]

Kevin Hilman wrote:
> commit cfabe8a950e252d26cdeb4a9bb11e2cabb2a50c6
> Author: Kevin Hilman <khilman@deeprootsystems.com>
> Date:   Mon Apr 26 14:59:09 2010 -0700
> 
>     OMAP3: PRCM interrupt: only check and clear enabled PRCM IRQs
>     
>     While handling PRCM IRQs, mask out interrupts that are not enabled in
>     PRM_IRQENABLE_MPU.  If these are not masked out, non-enabled
>     interrupts are caught, a WARN() is printed due to no 'handler' and the
>     events are cleared.  In addition to being noisy, this can also
>     interfere with independent polling of this register by SR/VP code.
>     
>     This was noticed using SmartReflex transitions which cause the VPx_*
>     interrupts to be handled since they are set in PRM_IRQSTATUS_MPU even
>     but not enabled in PRM_IRQENABLE_MPU.
>     
>     Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>

ACK.

Mike

> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
> index fee2efb..c38016b 100644
> --- a/arch/arm/mach-omap2/pm34xx.c
> +++ b/arch/arm/mach-omap2/pm34xx.c
> @@ -266,13 +266,16 @@ static int _prcm_int_handle_wakeup(void)
>   */
>  static irqreturn_t prcm_interrupt_handler (int irq, void *dev_id)
>  {
> -	u32 irqstatus_mpu;
> +	u32 irqenable_mpu, irqstatus_mpu;
>  	int c = 0;
>  
> -	do {
> -		irqstatus_mpu = prm_read_mod_reg(OCP_MOD,
> -					OMAP3_PRM_IRQSTATUS_MPU_OFFSET);
> +	irqenable_mpu = prm_read_mod_reg(OCP_MOD,
> +					 OMAP3_PRM_IRQENABLE_MPU_OFFSET);
> +	irqstatus_mpu = prm_read_mod_reg(OCP_MOD,
> +					 OMAP3_PRM_IRQSTATUS_MPU_OFFSET);
> +	irqstatus_mpu &= irqenable_mpu;
>  
> +	do {
>  		if (irqstatus_mpu & (OMAP3430_WKUP_ST | OMAP3430_IO_ST)) {
>  			c = _prcm_int_handle_wakeup();
>  
> @@ -291,7 +294,11 @@ static irqreturn_t prcm_interrupt_handler (int irq, void *dev_id)
>  		prm_write_mod_reg(irqstatus_mpu, OCP_MOD,
>  					OMAP3_PRM_IRQSTATUS_MPU_OFFSET);
>  
> -	} while (prm_read_mod_reg(OCP_MOD, OMAP3_PRM_IRQSTATUS_MPU_OFFSET));
> +		irqstatus_mpu = prm_read_mod_reg(OCP_MOD,
> +					OMAP3_PRM_IRQSTATUS_MPU_OFFSET);
> +		irqstatus_mpu &= irqenable_mpu;
> +
> +	} while (irqstatus_mpu);
>  
>  	return IRQ_HANDLED;
>  }