Problems with suspend/resume of pxa2xx_spi

Problems with suspend/resume of pxa2xx_spi

[Zik Saleeba]

I've been having some more problems with my SPI application on the
PXA270. When I suspend the system it all goes into suspend mode
correctly but when it comes back on resume any call to the pxa2xx_spi
code just hangs.

Has anyone experienced this? Has anyone used suspend/resume
successfully with pxa2xx_spi?

Thanks,
Zik

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: Problems with suspend/resume of pxa2xx_spi

[Ned Forrester]

Zik Saleeba wrote:
> I've been having some more problems with my SPI application on the
> PXA270. When I suspend the system it all goes into suspend mode
> correctly but when it comes back on resume any call to the pxa2xx_spi
> code just hangs.
>
> Has anyone experienced this? Has anyone used suspend/resume
> successfully with pxa2xx_spi?

I think it is more likely that no one has tried it.  What version of
the driver/kernel are you using?  I think David recently removed some
obsolete stuff related to suspend/resume, but I don't know if there is
any reason why it would or wouldn't work in any particular version of
the kernel.

I'm not sure what you mean by "it all goes into suspend mode correctly".
If it does not resume, it seems possible that the errors could be
related to either the suspend or the resume process, if the proper
state is not remembered or restored.

Does the whole system hang (deadlock in kernel) or is it only the spi
that is stuck?

I presume you have CONFIG_PM set in the kernel config?  It is not set in
my kernel, by default.  If not set, the suspend/resume functions are NULL.

The suspend/resume support in the driver is pretty simple.  It stops the
message queue, and waits for the message queue to drain (timeout if 5sec
if it fails to drain), then it stops the SSP hardware and clock.  On
resume, it restarts the clock and queue, and then any initial message
will cause the SSP to be restarted (at the bottom of pump_transfers) in
due course.

There may be a bug in the suspend routine.  If stop_queue() fails (queue
does not drain), pxa2xx_spi_suspend() returns with an error code, but no
message.  I don't know if the calling routine is responding to the error
code, but I bet not; what's it going to do, anyway.  If this happens,
the SSP will not be shut down, and the clock not stopped.  It seems like
these actions should occur, with or without a successful queue stop.

You could add a printk in suspend to see if it is completing properly.

David:
Should there be a message if there is an error on suspend, or are
suspending devices supposed to be silent and do the best they can?

-- 
Ned Forrester                                       nforrester-/d+BM93fTQY@public.gmane.org
Oceanographic Systems Lab                                  508-289-2226
Applied Ocean Physics and Engineering Dept.
Woods Hole Oceanographic Institution          Woods Hole, MA 02543, USA
http://www.whoi.edu/sbl/liteSite.do?litesiteid=7212
http://www.whoi.edu/hpb/Site.do?id=1532
http://www.whoi.edu/page.do?pid=10079


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: Problems with suspend/resume of pxa2xx_spi

[David Brownell]

> Does the whole system hang (deadlock in kernel) or is it only the spi
> that is stuck?

Yeah, speicifally *what* is going wrong?


> David:
> Should there be a message if there is an error on suspend, or are
> suspending devices supposed to be silent and do the best they can?

It's fair to issue error message.  Doesn't the PM core do that
automatically though?

- Dave


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: Problems with suspend/resume of pxa2xx_spi

[Zik Saleeba]

On Thu, Feb 21, 2008 at 2:32 PM, Zik Saleeba <zik-fsgeVU6Z5FysTnJN9+BGXg@public.gmane.org> wrote:
> I've been having some more problems with my SPI application on the
>  PXA270. When I suspend the system it all goes into suspend mode
>  correctly but when it comes back on resume any call to the pxa2xx_spi
>  code just hangs.

I've looked into this problem a bit more. It looks like suspend/resume
can never have worked on the pxa2xx spi subsystem.

One problem I've found is that after suspend the SSP registers don't
come back in their previous state. They need to be saved and restored.
I've added in some code to do that and it means that at least the
system doesn't hang as soon as spi is accessed during a resume. It
does however still hang at some later point. I'm still tracking that
one down.

I found that saving and restoring SSCR0, SSCR1, SSITR, SST0 (for
pxa270) and SSPSP was enough to bring the system back to life again.

Would more experienced people like to comment on whether I'm on the
right track here?

Cheers,
Zik

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: Problems with suspend/resume of pxa2xx_spi

[Zik Saleeba]

I fixed my other suspend/resume problem - it was unrelated to spi. So
restoring the SSP registers is the fix. It's all working correctly for
me now.

I've never submitted a patch before so I'd appreciate a review of this
before I submit it properly:

Index: pxa2xx_spi.c
===================================================================
--- pxa2xx_spi.c        (revision 562)
+++ pxa2xx_spi.c        (working copy)
@@ -102,6 +102,12 @@
        u32 int_cr1;
        u32 clear_sr;
        u32 mask_sr;
+
+        /* saved state for suspend/resume */
+        u32 saved_sscr0;
+        u32 saved_sscr1;
+        u32 saved_sst0;
+        u32 saved_sspsp;

        /* Driver message queue */
        struct workqueue_struct *workqueue;
@@ -1570,11 +1576,16 @@
 static int pxa2xx_spi_suspend(struct platform_device *pdev, pm_message_t state)
 {
        struct driver_data *drv_data = platform_get_drvdata(pdev);
+       void *reg = drv_data->ioaddr;
        int status = 0;

-       /* Check all childern for current power state */
+        /* Save register state */
+        drv_data->saved_sscr0 = read_SSCR0(reg);
+        drv_data->saved_sscr1 = read_SSCR1(reg);
+        if (drv_data->ssp_type != PXA25x_SSP)
+                drv_data->saved_sst0 = read_SSTO(reg);
+        drv_data->saved_sspsp = read_SSPSP(reg);
+
+       /* Check all children for current power state */
        if (device_for_each_child(&pdev->dev, &state, suspend_devices) != 0) {
                dev_warn(&pdev->dev, "suspend aborted\n");
                return -1;
@@ -1592,13 +1604,21 @@
 static int pxa2xx_spi_resume(struct platform_device *pdev)
 {
        struct driver_data *drv_data = platform_get_drvdata(pdev);
+       void *reg = drv_data->ioaddr;
        int status = 0;

        /* Enable the SSP clock */
        pxa_set_cken(drv_data->master_info->clock_enable, 1);

+       /* Load saved SSP configuration */
+        write_SSCR0(0, reg);
+        write_SSCR1(drv_data->saved_sscr1, reg);
+        write_SSCR0(drv_data->saved_sscr0, reg);
+        write_SSITR(0, reg);
+       if (drv_data->ssp_type != PXA25x_SSP)
+                write_SSTO(drv_data->saved_sst0, reg);
+        write_SSPSP(drv_data->saved_sspsp, reg);
+
        /* Start the queue running */
        status = start_queue(drv_data);
        if (status != 0) {


On Fri, Feb 29, 2008 at 10:02 AM, Zik Saleeba <zik-fsgeVU6Z5FysTnJN9+BGXg@public.gmane.org> wrote:
> On Thu, Feb 21, 2008 at 2:32 PM, Zik Saleeba <zik-fsgeVU6Z5FysTnJN9+BGXg@public.gmane.org> wrote:
>  > I've been having some more problems with my SPI application on the
>  >  PXA270. When I suspend the system it all goes into suspend mode
>  >  correctly but when it comes back on resume any call to the pxa2xx_spi
>  >  code just hangs.
>
>  I've looked into this problem a bit more. It looks like suspend/resume
>  can never have worked on the pxa2xx spi subsystem.
>
>  One problem I've found is that after suspend the SSP registers don't
>  come back in their previous state. They need to be saved and restored.
>  I've added in some code to do that and it means that at least the
>  system doesn't hang as soon as spi is accessed during a resume. It
>  does however still hang at some later point. I'm still tracking that
>  one down.
>
>  I found that saving and restoring SSCR0, SSCR1, SSITR, SST0 (for
>  pxa270) and SSPSP was enough to bring the system back to life again.
>
>  Would more experienced people like to comment on whether I'm on the
>  right track here?
>
>  Cheers,
>  Zik
>

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: Problems with suspend/resume of pxa2xx_spi

[David Brownell]

On Thursday 28 February 2008, Zik Saleeba wrote:
> I fixed my other suspend/resume problem - it was unrelated to spi. So
> restoring the SSP registers is the fix. It's all working correctly for
> me now.

Good.  I can believe that driver hasn't had much suspend/resume testing;
many platforms don't get such testing, and often not all drivers get
covered.  PXA has had issues where even the reference boards (for folk
who have them) don't really leverage all the peripherals.


> I've never submitted a patch before so I'd appreciate a review of this
> before I submit it properly:

See Documentation/SubmittingPatches.  The most obvious bit:

> Index: pxa2xx_spi.c
> ===================================================================
> --- pxa2xx_spi.c        (revision 562)
> +++ pxa2xx_spi.c        (working copy)

Patch format should let it be applied at toplevei with "patch -p1" ...


> @@ -102,6 +102,12 @@

I prefer to see patches generated with "diff -p ..." so there's
more context to review *just the patch* ...


>         u32 int_cr1;
>         u32 clear_sr;
>         u32 mask_sr;
> +
> +        /* saved state for suspend/resume */
> +        u32 saved_sscr0;
> +        u32 saved_sscr1;
> +        u32 saved_sst0;
> +        u32 saved_sspsp;
> 
>         /* Driver message queue */
>         struct workqueue_struct *workqueue;
> @@ -1570,11 +1576,16 @@
>  static int pxa2xx_spi_suspend(struct platform_device *pdev, pm_message_t state)
>  {
>         struct driver_data *drv_data = platform_get_drvdata(pdev);
> +       void *reg = drv_data->ioaddr;
>         int status = 0;
> 
> -       /* Check all childern for current power state */

You should also generate patches aginst the *CURRENT* kernel,
which in this case doesn't have that needless loop.


> +        /* Save register state */
> +        drv_data->saved_sscr0 = read_SSCR0(reg);
> +        drv_data->saved_sscr1 = read_SSCR1(reg);
> +        if (drv_data->ssp_type != PXA25x_SSP)
> +                drv_data->saved_sst0 = read_SSTO(reg);
> +        drv_data->saved_sspsp = read_SSPSP(reg);
> +
> +       /* Check all children for current power state */
>         if (device_for_each_child(&pdev->dev, &state, suspend_devices) != 0) {

I think you have some indentation problems ... looks like you're
either not using TAB for indent, or added an extra space afterwards.


>                 dev_warn(&pdev->dev, "suspend aborted\n");
>                 return -1;
> @@ -1592,13 +1604,21 @@
>  static int pxa2xx_spi_resume(struct platform_device *pdev)
>  {
>         struct driver_data *drv_data = platform_get_drvdata(pdev);
> +       void *reg = drv_data->ioaddr;
>         int status = 0;
> 
>         /* Enable the SSP clock */
>         pxa_set_cken(drv_data->master_info->clock_enable, 1);
> 
> +       /* Load saved SSP configuration */
> +        write_SSCR0(0, reg);
> +        write_SSCR1(drv_data->saved_sscr1, reg);
> +        write_SSCR0(drv_data->saved_sscr0, reg);
> +        write_SSITR(0, reg);
> +       if (drv_data->ssp_type != PXA25x_SSP)
> +                write_SSTO(drv_data->saved_sst0, reg);
> +        write_SSPSP(drv_data->saved_sspsp, reg);
> +
>         /* Start the queue running */
>         status = start_queue(drv_data);
>         if (status != 0) {
> 
> 

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: Problems with suspend/resume of pxa2xx_spi

[Zik Saleeba]

Thanks for the comments David. I'll submit a proper patch later.

Cheers,
Zik

On Fri, Feb 29, 2008 at 11:56 AM, David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> wrote:
> On Thursday 28 February 2008, Zik Saleeba wrote:
>  > I fixed my other suspend/resume problem - it was unrelated to spi. So
>  > restoring the SSP registers is the fix. It's all working correctly for
>  > me now.
>
>  Good.  I can believe that driver hasn't had much suspend/resume testing;
>  many platforms don't get such testing, and often not all drivers get
>  covered.  PXA has had issues where even the reference boards (for folk
>  who have them) don't really leverage all the peripherals.
>
>
>
>  > I've never submitted a patch before so I'd appreciate a review of this
>  > before I submit it properly:
>
>  See Documentation/SubmittingPatches.  The most obvious bit:
>
>
>  > Index: pxa2xx_spi.c
>  > ===================================================================
>  > --- pxa2xx_spi.c        (revision 562)
>  > +++ pxa2xx_spi.c        (working copy)
>
>  Patch format should let it be applied at toplevei with "patch -p1" ...
>
>
>  > @@ -102,6 +102,12 @@
>
>  I prefer to see patches generated with "diff -p ..." so there's
>  more context to review *just the patch* ...
>
>
>
>  >         u32 int_cr1;
>  >         u32 clear_sr;
>  >         u32 mask_sr;
>  > +
>  > +        /* saved state for suspend/resume */
>  > +        u32 saved_sscr0;
>  > +        u32 saved_sscr1;
>  > +        u32 saved_sst0;
>  > +        u32 saved_sspsp;
>  >
>  >         /* Driver message queue */
>  >         struct workqueue_struct *workqueue;
>  > @@ -1570,11 +1576,16 @@
>  >  static int pxa2xx_spi_suspend(struct platform_device *pdev, pm_message_t state)
>  >  {
>  >         struct driver_data *drv_data = platform_get_drvdata(pdev);
>  > +       void *reg = drv_data->ioaddr;
>  >         int status = 0;
>  >
>  > -       /* Check all childern for current power state */
>
>  You should also generate patches aginst the *CURRENT* kernel,
>  which in this case doesn't have that needless loop.
>
>
>
>  > +        /* Save register state */
>  > +        drv_data->saved_sscr0 = read_SSCR0(reg);
>  > +        drv_data->saved_sscr1 = read_SSCR1(reg);
>  > +        if (drv_data->ssp_type != PXA25x_SSP)
>  > +                drv_data->saved_sst0 = read_SSTO(reg);
>  > +        drv_data->saved_sspsp = read_SSPSP(reg);
>  > +
>  > +       /* Check all children for current power state */
>  >         if (device_for_each_child(&pdev->dev, &state, suspend_devices) != 0) {
>
>  I think you have some indentation problems ... looks like you're
>  either not using TAB for indent, or added an extra space afterwards.
>
>
>
>
>  >                 dev_warn(&pdev->dev, "suspend aborted\n");
>  >                 return -1;
>  > @@ -1592,13 +1604,21 @@
>  >  static int pxa2xx_spi_resume(struct platform_device *pdev)
>  >  {
>  >         struct driver_data *drv_data = platform_get_drvdata(pdev);
>  > +       void *reg = drv_data->ioaddr;
>  >         int status = 0;
>  >
>  >         /* Enable the SSP clock */
>  >         pxa_set_cken(drv_data->master_info->clock_enable, 1);
>  >
>  > +       /* Load saved SSP configuration */
>  > +        write_SSCR0(0, reg);
>  > +        write_SSCR1(drv_data->saved_sscr1, reg);
>  > +        write_SSCR0(drv_data->saved_sscr0, reg);
>  > +        write_SSITR(0, reg);
>  > +       if (drv_data->ssp_type != PXA25x_SSP)
>  > +                write_SSTO(drv_data->saved_sst0, reg);
>  > +        write_SSPSP(drv_data->saved_sspsp, reg);
>  > +
>  >         /* Start the queue running */
>  >         status = start_queue(drv_data);
>  >         if (status != 0) {
>  >
>  >
>

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: Problems with suspend/resume of pxa2xx_spi

[Ned Forrester]

Zik Saleeba wrote:
> I found that saving and restoring SSCR0, SSCR1, SSITR, SST0 (for
> pxa270) and SSPSP was enough to bring the system back to life again.
> 
> Would more experienced people like to comment on whether I'm on the
> right track here?

Is it possible that you did not check individually to see which of these
registers is the culprit?  SSITR is a test register that is surely
initialized to 0 on powerup, and it is never touched by the driver.  So
why would this have changed?  Likewise, SSPSP is not ever set by the
driver.  SSCR0, SSCR1, are both set at the end of pump_transfers, just
before the first transfer starts, if they differ from the values in
hardware, as they most likely will after suspend (and if they don't,
then it should not matter).

Is is possible that after resume, not all of these registers recover
their power-up state?  Is SSPSP really different after resume than
before?  SSPSP should not matter if the PSP mode is not selected in SSCR0.

Can you shed some light on these questions, while I continue to look?

-- 
Ned Forrester                                       nforrester-/d+BM93fTQY@public.gmane.org
Oceanographic Systems Lab                                  508-289-2226
Applied Ocean Physics and Engineering Dept.
Woods Hole Oceanographic Institution          Woods Hole, MA 02543, USA
http://www.whoi.edu/sbl/liteSite.do?litesiteid=7212
http://www.whoi.edu/hpb/Site.do?id=1532
http://www.whoi.edu/page.do?pid=10079


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: Problems with suspend/resume of pxa2xx_spi

[Zik Saleeba]

Here's the contents of the registers before suspend:

SSCR0=00000387
SSCR1=00000ec0
SSSR=0000f024
SSITR=00000000
SSTO=00000000
SSPSP=00000000

And after suspend:

SSCR0=00000000
SSCR1=00000000
SSSR=0000f004
SSITR=00000000
SSTO=00000000
SSPSP=00000000

I chose to reinitialise the registers to a known correct state rather
than leave them in their post-suspend state. You may have some better
ideas about what to do with them.

Cheers,
Zik

On Fri, Feb 29, 2008 at 12:31 PM, Ned Forrester <nforrester-/d+BM93fTQY@public.gmane.org> wrote:
>
>  Is it possible that you did not check individually to see which of these
>  registers is the culprit?  SSITR is a test register that is surely
>  initialized to 0 on powerup, and it is never touched by the driver.  So
>  why would this have changed?  Likewise, SSPSP is not ever set by the
>  driver.  SSCR0, SSCR1, are both set at the end of pump_transfers, just
>  before the first transfer starts, if they differ from the values in
>  hardware, as they most likely will after suspend (and if they don't,
>  then it should not matter).
>
>  Is is possible that after resume, not all of these registers recover
>  their power-up state?  Is SSPSP really different after resume than
>  before?  SSPSP should not matter if the PSP mode is not selected in SSCR0.
>
>  Can you shed some light on these questions, while I continue to look?
>
>  --
>
>
> Ned Forrester                                       nforrester-/d+BM93fTQY@public.gmane.org
>  Oceanographic Systems Lab                                  508-289-2226
>  Applied Ocean Physics and Engineering Dept.
>  Woods Hole Oceanographic Institution          Woods Hole, MA 02543, USA
>  http://www.whoi.edu/sbl/liteSite.do?litesiteid=7212
>  http://www.whoi.edu/hpb/Site.do?id=1532
>  http://www.whoi.edu/page.do?pid=10079
>
>

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: Problems with suspend/resume of pxa2xx_spi

[Ned Forrester]

Zik Saleeba wrote:
> Here's the contents of the registers before suspend:
> 
> SSCR0=00000387

This means: 1/4 clock rate, SSP *enabled*, and 8 bit data.
You don't want to restore the SSP to the running state after resume
because it should not have been running after suspend.  I will comment
more in the next email.

> SSCR1=00000ec0
> SSSR=0000f024
> SSITR=00000000
> SSTO=00000000
> SSPSP=00000000
> 
> And after suspend:
> 
> SSCR0=00000000
> SSCR1=00000000
> SSSR=0000f004
> SSITR=00000000
> SSTO=00000000
> SSPSP=00000000
> 
> I chose to reinitialise the registers to a known correct state rather
> than leave them in their post-suspend state. You may have some better
> ideas about what to do with them.

This state is not the "correct" state to restore.  It has been captured
before the driver is shutdown in preparation for suspend.  If any
restoration is required, it would be to the shutdown state.

-- 
Ned Forrester                                       nforrester-/d+BM93fTQY@public.gmane.org
Oceanographic Systems Lab                                  508-289-2226
Applied Ocean Physics and Engineering Dept.
Woods Hole Oceanographic Institution          Woods Hole, MA 02543, USA
http://www.whoi.edu/sbl/liteSite.do?litesiteid=7212
http://www.whoi.edu/hpb/Site.do?id=1532
http://www.whoi.edu/page.do?pid=10079


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: Problems with suspend/resume of pxa2xx_spi

[Ned Forrester]

Zik Saleeba wrote:
> On Thu, Feb 21, 2008 at 2:32 PM, Zik Saleeba <zik-fsgeVU6Z5FysTnJN9+BGXg@public.gmane.org> wrote:
>> I've been having some more problems with my SPI application on the
>>  PXA270. When I suspend the system it all goes into suspend mode
>>  correctly but when it comes back on resume any call to the pxa2xx_spi
>>  code just hangs.
> 
> I've looked into this problem a bit more. It looks like suspend/resume
> can never have worked on the pxa2xx spi subsystem.

I'm not surprised, I've never used it with my Gumstix (I just power
cycle it, because it does not take long to boot), and I don't know if
Stephen Street tested it, either.  I'm not sure if others have tried it.
 Also, I think Stephen used a PXA255, as do I, so any differences with
PXA270 may be new.

> One problem I've found is that after suspend the SSP registers don't
> come back in their previous state. They need to be saved and restored.

Well, no.  They need to come back in a safe, stopped state.

> I found that saving and restoring SSCR0, SSCR1, SSITR, SST0 (for
> pxa270) and SSPSP was enough to bring the system back to life again.

But SSCR0, SSCR1, and SSTO are restored at the end of pump_transfers on
every transfer, including the first.  SSPSP should not matter if that
mode is not selected in SSCR0.  Is SSITR not resuming as all zeros?  Yes
it is, as the data you provided below confirms.  Something else is going on.

> Would more experienced people like to comment on whether I'm on the
> right track here?

Have you verified that the suspend process completes correctly?
Does pxa2xx_spi_suspend() return 0?

> Here's the contents of the registers before suspend:
> 
> SSCR0=00000387

As noted in earlier email, you don't want to capture the running state
(SSE = 1).

> SSCR1=00000ec0
> SSSR=0000f024
> SSITR=00000000
> SSTO=00000000

Are you really operating with with zero (no) timeout, or more likely did
this capture just catch the SSP between transfers?  (Unless there is an
error, it is normal, after a transfer, for SSE to remain high, but with
the interrupt and service enables off, and SSTO=0.) A zero timeout, in
and of itself, might cause the driver to never return messages if there
are trailing bytes after a DMA, though it should not cause problems for
the rest of the kernel.

> SSPSP=00000000

Good.  SSPSP is cleared in pxa2xx_spi_probe, and never set again.

> And after suspend:
> 
> SSCR0=00000000

This would be the correct restored value of SSCR0, because that is the
last value written during suspend.

> SSCR1=00000000

A valid state, though this would otherwise get fixed up at the end of
pump_transfers, just before the action starts.  The sate stored above
does not have any interrupt or DMA service enables set, so even if these
registers are restored, no data will be transferred until the next
transfers is started in pump_transfers.

> SSSR=0000f004
> SSITR=00000000
> SSTO=00000000
> SSPSP=00000000

These three zero values are valid, and the case of PSP and SSITR, desired.

> ===================================================================
> --- pxa2xx_spi.c        (revision 562)
> +++ pxa2xx_spi.c        (working copy)
> @@ -102,6 +102,12 @@
>         u32 int_cr1;
>         u32 clear_sr;
>         u32 mask_sr;
> +
> +        /* saved state for suspend/resume */
> +        u32 saved_sscr0;
> +        u32 saved_sscr1;
> +        u32 saved_sst0;
> +        u32 saved_sspsp;

It should be neither necessary, nor appropriate, to save these registers
in special locations.  The values of these registers are already known
to the driver (in struct chip_data), and should be restored from the
known values, except with SSCR0[SSE] low.

> @@ -1570,11 +1576,16 @@
>  static int pxa2xx_spi_suspend(struct platform_device *pdev, pm_message_t state)
>  {
>         struct driver_data *drv_data = platform_get_drvdata(pdev);
> +       void *reg = drv_data->ioaddr;
>         int status = 0;
> 
> -       /* Check all childern for current power state */
> +        /* Save register state */
> +        drv_data->saved_sscr0 = read_SSCR0(reg);
> +        drv_data->saved_sscr1 = read_SSCR1(reg);
> +        if (drv_data->ssp_type != PXA25x_SSP)
> +                drv_data->saved_sst0 = read_SSTO(reg);
> +        drv_data->saved_sspsp = read_SSPSP(reg);

The driver is about to be stopped, below, with stop_queue().  You don't
want to capture and restore a running state because that is not the
state the driver will be in during suspend!  Again, you need to verify
that suspend is completing correctly, and then, if saving these
registers really helps (I'm not convinced of that), restore the states
saved after shutdown.

> +
> +       /* Check all children for current power state */
>         if (device_for_each_child(&pdev->dev, &state, suspend_devices) != 0) {
>                 dev_warn(&pdev->dev, "suspend aborted\n");
>                 return -1;

As David pointed out, you are patching an obsolete version of the
driver.  These lines were recently removed.  I work with an old version
too, but be sure your patches are applied against the latest version.

> @@ -1592,13 +1604,21 @@
>  static int pxa2xx_spi_resume(struct platform_device *pdev)
>  {
>         struct driver_data *drv_data = platform_get_drvdata(pdev);
> +       void *reg = drv_data->ioaddr;
>         int status = 0;
> 
>         /* Enable the SSP clock */
>         pxa_set_cken(drv_data->master_info->clock_enable, 1);
> 
> +       /* Load saved SSP configuration */
> +        write_SSCR0(0, reg);
> +        write_SSCR1(drv_data->saved_sscr1, reg);
> +        write_SSCR0(drv_data->saved_sscr0, reg);

NO.  Even if it were right to restore SSCR0 to this value in
pxa2xx_spi_resume(), this register must be set last, after setting all
modes, in particular, SSPSP, below. (see section 8.5 of the PXA270
developer's manual).  This would not matter if SSCR0[SSE] was not set in
saved_sscr0, as it shouldn't be, but this is the wrong order, since you
have the bit erroneously set.

> +        write_SSITR(0, reg);
> +       if (drv_data->ssp_type != PXA25x_SSP)
> +                write_SSTO(drv_data->saved_sst0, reg);
> +        write_SSPSP(drv_data->saved_sspsp, reg);
> +
>         /* Start the queue running */
>         status = start_queue(drv_data);
>         if (status != 0) {

--

If these changes really fix your problem, then there is something more
subtle going on.  All the register values you show after resume are
valid, assuming the driver shutdown properly (queue stopped, and already
queued messages completed and given back).

Please try the following:

First, verify that you are not using a zero timeout, as that might cause
the trouble.

Next, verify that suspend is completing (empty queue).  Perhaps the
protocol driver is stopped first, and pxa2xx_spi blocks on giveback().
That would be bad.

If that is not it, then there may be something subtle about the timing
of setting up a non-zero state in SSCR1 and SSCR0.  Prior to 2.6.20, the
state was restored in pump_messages, which is somewhat earlier than
pump_transfers.  I don't know if suspend ever worked, but if it did,
that difference might be related.

Try restoring these two registers only, not with the values you saved,
but with the default values used in probe, after the comment:
/* Load default SSP configuration */

It that does not work, try (just as an experiment) loading the values
used by your first transfer after resume.

Just looking at all the other things initialized by probe and setup,
check to see if the following registers are preserved across suspend/resume:
	DRCMR(ssp->drcmr_rx) /* as accessed in probe */
	DRCMR(ssp->drcmr_tx)
Actually, on close look, these are the only other registers (than the
SSP registers already discussed) that are set in probe and setup, and
these would be fatal if not set properly on resume.  Since you have
gotten this to work, these registers are not likely to be the problem.

We need to isolate the problem, and fix the right thing.  Otherwise a
quick "fix", like the one above, is likely to break something for
someone else.  I would work on this, but I have no way to test it.

--

Since you are working on suspend/resume, it would be a good time to fix
what appears to be a bug in suspend.  If stop_queue fails, the function
returns without ever stopping the SSP or disabling its clock.  I think
that this should be changed so that a dev_err() message is output on
failure to stop queue, but that the SSP and clock should be disabled
anyway, else power savings are not achieved.  Probably in the error path
there is a need to flush the queue directly with calls to giveback()
setting the error status in each message.

If the queue is failing to flush, fixing it might solve the problem.  On
resume, start_queue() forces any message that was current
(drv_data->cur_msg != NULL) to be arbitrarily dropped, and never given
back; there aren't supposed to be any messages in progress.  Any such
message is still physically linked in the queue, however, so I'm not
quite sure what happens then.

More if I think of it.  Let me know what you think.

-- 
Ned Forrester                                       nforrester-/d+BM93fTQY@public.gmane.org
Oceanographic Systems Lab                                  508-289-2226
Applied Ocean Physics and Engineering Dept.
Woods Hole Oceanographic Institution          Woods Hole, MA 02543, USA
http://www.whoi.edu/sbl/liteSite.do?litesiteid=7212
http://www.whoi.edu/hpb/Site.do?id=1532
http://www.whoi.edu/page.do?pid=10079


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: Problems with suspend/resume of pxa2xx_spi

[Zik Saleeba]

Thanks for all your comments guys. As of yesterday my contract at NEC
expired and I no longer have access to the hardware. I was hoping to
get this sorted before then but it didn't work out that way.

You guys seem to have some ideas on what the best solution is - feel
free to either go ahead and fix it yourself or wait a couple of months
and I'll be back at NEC and will have access to the hardware again.

Cheers,
Zik

On Fri, Feb 29, 2008 at 3:31 PM, Ned Forrester <nforrester-/d+BM93fTQY@public.gmane.org> wrote:
> Zik Saleeba wrote:
>  > On Thu, Feb 21, 2008 at 2:32 PM, Zik Saleeba <zik-fsgeVU6Z5FysTnJN9+BGXg@public.gmane.org> wrote:
>  >> I've been having some more problems with my SPI application on the
>  >>  PXA270. When I suspend the system it all goes into suspend mode
>  >>  correctly but when it comes back on resume any call to the pxa2xx_spi
>  >>  code just hangs.
>  >
>  > I've looked into this problem a bit more. It looks like suspend/resume
>  > can never have worked on the pxa2xx spi subsystem.
>
>  I'm not surprised, I've never used it with my Gumstix (I just power
>  cycle it, because it does not take long to boot), and I don't know if
>  Stephen Street tested it, either.  I'm not sure if others have tried it.
>   Also, I think Stephen used a PXA255, as do I, so any differences with
>  PXA270 may be new.
>
>
>  > One problem I've found is that after suspend the SSP registers don't
>  > come back in their previous state. They need to be saved and restored.
>
>  Well, no.  They need to come back in a safe, stopped state.
>
>
>  > I found that saving and restoring SSCR0, SSCR1, SSITR, SST0 (for
>  > pxa270) and SSPSP was enough to bring the system back to life again.
>
>  But SSCR0, SSCR1, and SSTO are restored at the end of pump_transfers on
>  every transfer, including the first.  SSPSP should not matter if that
>  mode is not selected in SSCR0.  Is SSITR not resuming as all zeros?  Yes
>  it is, as the data you provided below confirms.  Something else is going on.
>
>
>  > Would more experienced people like to comment on whether I'm on the
>  > right track here?
>
>  Have you verified that the suspend process completes correctly?
>  Does pxa2xx_spi_suspend() return 0?
>
>
>  > Here's the contents of the registers before suspend:
>  >
>  > SSCR0=00000387
>
>  As noted in earlier email, you don't want to capture the running state
>  (SSE = 1).
>
>
>  > SSCR1=00000ec0
>  > SSSR=0000f024
>  > SSITR=00000000
>  > SSTO=00000000
>
>  Are you really operating with with zero (no) timeout, or more likely did
>  this capture just catch the SSP between transfers?  (Unless there is an
>  error, it is normal, after a transfer, for SSE to remain high, but with
>  the interrupt and service enables off, and SSTO=0.) A zero timeout, in
>  and of itself, might cause the driver to never return messages if there
>  are trailing bytes after a DMA, though it should not cause problems for
>  the rest of the kernel.
>
>  > SSPSP=00000000
>
>  Good.  SSPSP is cleared in pxa2xx_spi_probe, and never set again.
>
>
>  > And after suspend:
>  >
>  > SSCR0=00000000
>
>  This would be the correct restored value of SSCR0, because that is the
>  last value written during suspend.
>
>  > SSCR1=00000000
>
>  A valid state, though this would otherwise get fixed up at the end of
>  pump_transfers, just before the action starts.  The sate stored above
>  does not have any interrupt or DMA service enables set, so even if these
>  registers are restored, no data will be transferred until the next
>  transfers is started in pump_transfers.
>
>
>  > SSSR=0000f004
>  > SSITR=00000000
>  > SSTO=00000000
>  > SSPSP=00000000
>
>  These three zero values are valid, and the case of PSP and SSITR, desired.
>
>
>  > ===================================================================
>  > --- pxa2xx_spi.c        (revision 562)
>  > +++ pxa2xx_spi.c        (working copy)
>  > @@ -102,6 +102,12 @@
>
> >         u32 int_cr1;
>  >         u32 clear_sr;
>  >         u32 mask_sr;
>  > +
>  > +        /* saved state for suspend/resume */
>  > +        u32 saved_sscr0;
>  > +        u32 saved_sscr1;
>  > +        u32 saved_sst0;
>  > +        u32 saved_sspsp;
>
>  It should be neither necessary, nor appropriate, to save these registers
>  in special locations.  The values of these registers are already known
>  to the driver (in struct chip_data), and should be restored from the
>  known values, except with SSCR0[SSE] low.
>
>
>  > @@ -1570,11 +1576,16 @@
>  >  static int pxa2xx_spi_suspend(struct platform_device *pdev, pm_message_t state)
>  >  {
>  >         struct driver_data *drv_data = platform_get_drvdata(pdev);
>  > +       void *reg = drv_data->ioaddr;
>  >         int status = 0;
>  >
>  > -       /* Check all childern for current power state */
>
> > +        /* Save register state */
>  > +        drv_data->saved_sscr0 = read_SSCR0(reg);
>  > +        drv_data->saved_sscr1 = read_SSCR1(reg);
>  > +        if (drv_data->ssp_type != PXA25x_SSP)
>  > +                drv_data->saved_sst0 = read_SSTO(reg);
>  > +        drv_data->saved_sspsp = read_SSPSP(reg);
>
>  The driver is about to be stopped, below, with stop_queue().  You don't
>  want to capture and restore a running state because that is not the
>  state the driver will be in during suspend!  Again, you need to verify
>  that suspend is completing correctly, and then, if saving these
>  registers really helps (I'm not convinced of that), restore the states
>  saved after shutdown.
>
>
>  > +
>  > +       /* Check all children for current power state */
>  >         if (device_for_each_child(&pdev->dev, &state, suspend_devices) != 0) {
>
> >                 dev_warn(&pdev->dev, "suspend aborted\n");
>  >                 return -1;
>
>  As David pointed out, you are patching an obsolete version of the
>  driver.  These lines were recently removed.  I work with an old version
>  too, but be sure your patches are applied against the latest version.
>
>
>  > @@ -1592,13 +1604,21 @@
>  >  static int pxa2xx_spi_resume(struct platform_device *pdev)
>  >  {
>  >         struct driver_data *drv_data = platform_get_drvdata(pdev);
>  > +       void *reg = drv_data->ioaddr;
>  >         int status = 0;
>  >
>  >         /* Enable the SSP clock */
>  >         pxa_set_cken(drv_data->master_info->clock_enable, 1);
>  >
>  > +       /* Load saved SSP configuration */
>  > +        write_SSCR0(0, reg);
>  > +        write_SSCR1(drv_data->saved_sscr1, reg);
>  > +        write_SSCR0(drv_data->saved_sscr0, reg);
>
>  NO.  Even if it were right to restore SSCR0 to this value in
>  pxa2xx_spi_resume(), this register must be set last, after setting all
>  modes, in particular, SSPSP, below. (see section 8.5 of the PXA270
>  developer's manual).  This would not matter if SSCR0[SSE] was not set in
>  saved_sscr0, as it shouldn't be, but this is the wrong order, since you
>  have the bit erroneously set.
>
>
>  > +        write_SSITR(0, reg);
>  > +       if (drv_data->ssp_type != PXA25x_SSP)
>  > +                write_SSTO(drv_data->saved_sst0, reg);
>  > +        write_SSPSP(drv_data->saved_sspsp, reg);
>  > +
>  >         /* Start the queue running */
>  >         status = start_queue(drv_data);
>  >         if (status != 0) {
>
>  --
>
>  If these changes really fix your problem, then there is something more
>  subtle going on.  All the register values you show after resume are
>  valid, assuming the driver shutdown properly (queue stopped, and already
>  queued messages completed and given back).
>
>  Please try the following:
>
>  First, verify that you are not using a zero timeout, as that might cause
>  the trouble.
>
>  Next, verify that suspend is completing (empty queue).  Perhaps the
>  protocol driver is stopped first, and pxa2xx_spi blocks on giveback().
>  That would be bad.
>
>  If that is not it, then there may be something subtle about the timing
>  of setting up a non-zero state in SSCR1 and SSCR0.  Prior to 2.6.20, the
>  state was restored in pump_messages, which is somewhat earlier than
>  pump_transfers.  I don't know if suspend ever worked, but if it did,
>  that difference might be related.
>
>  Try restoring these two registers only, not with the values you saved,
>  but with the default values used in probe, after the comment:
>  /* Load default SSP configuration */
>
>  It that does not work, try (just as an experiment) loading the values
>  used by your first transfer after resume.
>
>  Just looking at all the other things initialized by probe and setup,
>  check to see if the following registers are preserved across suspend/resume:
>         DRCMR(ssp->drcmr_rx) /* as accessed in probe */
>         DRCMR(ssp->drcmr_tx)
>  Actually, on close look, these are the only other registers (than the
>  SSP registers already discussed) that are set in probe and setup, and
>  these would be fatal if not set properly on resume.  Since you have
>  gotten this to work, these registers are not likely to be the problem.
>
>  We need to isolate the problem, and fix the right thing.  Otherwise a
>  quick "fix", like the one above, is likely to break something for
>  someone else.  I would work on this, but I have no way to test it.
>
>  --
>
>  Since you are working on suspend/resume, it would be a good time to fix
>  what appears to be a bug in suspend.  If stop_queue fails, the function
>  returns without ever stopping the SSP or disabling its clock.  I think
>  that this should be changed so that a dev_err() message is output on
>  failure to stop queue, but that the SSP and clock should be disabled
>  anyway, else power savings are not achieved.  Probably in the error path
>  there is a need to flush the queue directly with calls to giveback()
>  setting the error status in each message.
>
>  If the queue is failing to flush, fixing it might solve the problem.  On
>  resume, start_queue() forces any message that was current
>  (drv_data->cur_msg != NULL) to be arbitrarily dropped, and never given
>  back; there aren't supposed to be any messages in progress.  Any such
>  message is still physically linked in the queue, however, so I'm not
>  quite sure what happens then.
>
>  More if I think of it.  Let me know what you think.
>
>
>
>  --
>  Ned Forrester                                       nforrester-/d+BM93fTQY@public.gmane.org
>  Oceanographic Systems Lab                                  508-289-2226
>  Applied Ocean Physics and Engineering Dept.
>  Woods Hole Oceanographic Institution          Woods Hole, MA 02543, USA
>  http://www.whoi.edu/sbl/liteSite.do?litesiteid=7212
>  http://www.whoi.edu/hpb/Site.do?id=1532
>  http://www.whoi.edu/page.do?pid=10079
>
>

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/