[PATCH v2] ARM: mach-shmobile: sh7372 A4R suspend/resume order fix

[PATCH v2] ARM: mach-shmobile: sh7372 A4R suspend/resume order fix

[Guennadi Liakhovetski]

Update the sh7372 A4R code to make sure the suspend
callback gets to be called before the resume callback.

Without this fix the A4R resume callback is called before
suspend. The resume callback restores the INTCS hardware
registers with incorrect data which results in interrupts
being masked when they shouldn't be. The user will notice
this issue as IIC0 timing out during boot.

Thanks to Magnus Damm for tracking this problem down and providing the 
initial fix and the commit description text.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---

I took the liberty to reimplement the original fix by Magnus in a 
different way. I didn't quite like the idea of rewriting platform data, 
which could and, probably, should actually be const, on each 
domain-suspend. Instead, checking in resume, whether a valid state 
snapshot is available seemed a bit prettier to me:-) The maintainer is, of 
course, free to choose whichever version he prefers, they both

"Fix issue in v3.2-rc1 on the sh7372 Mackerel board."

diff --git a/arch/arm/mach-shmobile/intc-sh7372.c b/arch/arm/mach-shmobile/intc-sh7372.c
index 29cdc05..eeeb16b 100644
--- a/arch/arm/mach-shmobile/intc-sh7372.c
+++ b/arch/arm/mach-shmobile/intc-sh7372.c
@@ -627,6 +627,7 @@ void __init sh7372_init_irq(void)
 
 static unsigned short ffd2[0x200];
 static unsigned short ffd5[0x100];
+static bool intcs_state_saved = false;
 
 void sh7372_intcs_suspend(void)
 {
@@ -646,12 +647,17 @@ void sh7372_intcs_suspend(void)
 
 	for (k = 0x80; k <= 0x9c; k += 4)
 		ffd5[k] = __raw_readb(intcs_ffd5 + k);
+
+	intcs_state_saved = true;
 }
 
 void sh7372_intcs_resume(void)
 {
 	int k;
 
+	if (!intcs_state_saved)
+		return;
+
 	for (k = 0x00; k <= 0x30; k += 4)
 		__raw_writew(ffd2[k], intcs_ffd2 + k);
 

Re: [PATCH v2] ARM: mach-shmobile: sh7372 A4R suspend/resume order fix

[Rafael J. Wysocki]

On Wednesday, November 09, 2011, Guennadi Liakhovetski wrote:
> Update the sh7372 A4R code to make sure the suspend
> callback gets to be called before the resume callback.
> 
> Without this fix the A4R resume callback is called before
> suspend. The resume callback restores the INTCS hardware
> registers with incorrect data which results in interrupts
> being masked when they shouldn't be. The user will notice
> this issue as IIC0 timing out during boot.
> 
> Thanks to Magnus Damm for tracking this problem down and providing the 
> initial fix and the commit description text.
> 
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> ---
> 
> I took the liberty to reimplement the original fix by Magnus in a 
> different way. I didn't quite like the idea of rewriting platform data, 
> which could and, probably, should actually be const, on each 
> domain-suspend. Instead, checking in resume, whether a valid state 
> snapshot is available seemed a bit prettier to me:-) The maintainer is, of 
> course, free to choose whichever version he prefers, they both
> 
> "Fix issue in v3.2-rc1 on the sh7372 Mackerel board."

Magnus, Paul, which one do you want me to take?

Rafael


> diff --git a/arch/arm/mach-shmobile/intc-sh7372.c b/arch/arm/mach-shmobile/intc-sh7372.c
> index 29cdc05..eeeb16b 100644
> --- a/arch/arm/mach-shmobile/intc-sh7372.c
> +++ b/arch/arm/mach-shmobile/intc-sh7372.c
> @@ -627,6 +627,7 @@ void __init sh7372_init_irq(void)
>  
>  static unsigned short ffd2[0x200];
>  static unsigned short ffd5[0x100];
> +static bool intcs_state_saved = false;
>  
>  void sh7372_intcs_suspend(void)
>  {
> @@ -646,12 +647,17 @@ void sh7372_intcs_suspend(void)
>  
>  	for (k = 0x80; k <= 0x9c; k += 4)
>  		ffd5[k] = __raw_readb(intcs_ffd5 + k);
> +
> +	intcs_state_saved = true;
>  }
>  
>  void sh7372_intcs_resume(void)
>  {
>  	int k;
>  
> +	if (!intcs_state_saved)
> +		return;
> +
>  	for (k = 0x00; k <= 0x30; k += 4)
>  		__raw_writew(ffd2[k], intcs_ffd2 + k);
>  
> 
> 

Re: [PATCH v2] ARM: mach-shmobile: sh7372 A4R suspend/resume order fix

[Magnus Damm]

On Thu, Nov 10, 2011 at 8:30 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Wednesday, November 09, 2011, Guennadi Liakhovetski wrote:
>> Update the sh7372 A4R code to make sure the suspend
>> callback gets to be called before the resume callback.
>>
>> Without this fix the A4R resume callback is called before
>> suspend. The resume callback restores the INTCS hardware
>> registers with incorrect data which results in interrupts
>> being masked when they shouldn't be. The user will notice
>> this issue as IIC0 timing out during boot.
>>
>> Thanks to Magnus Damm for tracking this problem down and providing the
>> initial fix and the commit description text.
>>
>> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
>> ---
>>
>> I took the liberty to reimplement the original fix by Magnus in a
>> different way. I didn't quite like the idea of rewriting platform data,
>> which could and, probably, should actually be const, on each
>> domain-suspend. Instead, checking in resume, whether a valid state
>> snapshot is available seemed a bit prettier to me:-) The maintainer is, of
>> course, free to choose whichever version he prefers, they both
>>
>> "Fix issue in v3.2-rc1 on the sh7372 Mackerel board."
>
> Magnus, Paul, which one do you want me to take?

I prefer leaving the INTCS code as-is - unaware of calling order and
putting the workaround in the Runtime PM code. In the end the ordering
issue is related to Runtime PM and not INTCS. If someone knows a
better way to fix the Runtime PM code then I'd be happy to accept
that. But I don't like putting the workaround in INTCS.

Cheers,

/ magnus

Re: [PATCH v2] ARM: mach-shmobile: sh7372 A4R suspend/resume order fix

[Paul Mundt]

On Thu, Nov 10, 2011 at 11:14:42AM +0900, Magnus Damm wrote:
> On Thu, Nov 10, 2011 at 8:30 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Wednesday, November 09, 2011, Guennadi Liakhovetski wrote:
> >> I took the liberty to reimplement the original fix by Magnus in a
> >> different way. I didn't quite like the idea of rewriting platform data,
> >> which could and, probably, should actually be const, on each
> >> domain-suspend. Instead, checking in resume, whether a valid state
> >> snapshot is available seemed a bit prettier to me:-) The maintainer is, of
> >> course, free to choose whichever version he prefers, they both
> >>
> >> "Fix issue in v3.2-rc1 on the sh7372 Mackerel board."
> >
> > Magnus, Paul, which one do you want me to take?
> 
> I prefer leaving the INTCS code as-is - unaware of calling order and
> putting the workaround in the Runtime PM code. In the end the ordering
> issue is related to Runtime PM and not INTCS. If someone knows a
> better way to fix the Runtime PM code then I'd be happy to accept
> that. But I don't like putting the workaround in INTCS.
> 
Agreed. While I don't much care for the approach taken by this patch
either, keeping it insular at the framework level until the need for it
can be resolved in the runtime PM code seems like a reasonable stop-gap.
I would rather not paper over this in the platform code since it'll
invariably lead to more instances that need to be audited and cleaned up
later on, in addition to being more error prone.

Re: [PATCH v2] ARM: mach-shmobile: sh7372 A4R suspend/resume order fix

[Rafael J. Wysocki]

On Thursday, November 10, 2011, Paul Mundt wrote:
> On Thu, Nov 10, 2011 at 11:14:42AM +0900, Magnus Damm wrote:
> > On Thu, Nov 10, 2011 at 8:30 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > > On Wednesday, November 09, 2011, Guennadi Liakhovetski wrote:
> > >> I took the liberty to reimplement the original fix by Magnus in a
> > >> different way. I didn't quite like the idea of rewriting platform data,
> > >> which could and, probably, should actually be const, on each
> > >> domain-suspend. Instead, checking in resume, whether a valid state
> > >> snapshot is available seemed a bit prettier to me:-) The maintainer is, of
> > >> course, free to choose whichever version he prefers, they both
> > >>
> > >> "Fix issue in v3.2-rc1 on the sh7372 Mackerel board."
> > >
> > > Magnus, Paul, which one do you want me to take?
> > 
> > I prefer leaving the INTCS code as-is - unaware of calling order and
> > putting the workaround in the Runtime PM code. In the end the ordering
> > issue is related to Runtime PM and not INTCS. If someone knows a
> > better way to fix the Runtime PM code then I'd be happy to accept
> > that. But I don't like putting the workaround in INTCS.
> > 
> Agreed. While I don't much care for the approach taken by this patch
> either, keeping it insular at the framework level until the need for it
> can be resolved in the runtime PM code seems like a reasonable stop-gap.
> I would rather not paper over this in the platform code since it'll
> invariably lead to more instances that need to be audited and cleaned up
> later on, in addition to being more error prone.

OK, thanks!