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 <[EMAIL PROTECTED]> wrote:
> Zik Saleeba wrote:
> > On Thu, Feb 21, 2008 at 2:32 PM, Zik Saleeba <[EMAIL PROTECTED]> 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 [EMAIL PROTECTED]
> 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/
_______________________________________________
spi-devel-general mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/spi-devel-general
