Re: PM issue with Intel SST Atom driver

[Vinod Koul <vinod.koul@intel.com>]

On Mon, Apr 24, 2017 at 09:15:04AM +0200, Takashi Iwai wrote:
> On Mon, 24 Apr 2017 07:01:44 +0200,
> Vinod Koul wrote:
> > 
> > On Fri, Apr 21, 2017 at 03:42:43PM +0200, Takashi Iwai wrote:
> > > Hi,
> > > 
> > > I noticed that the unstable PM behavior on my Cherrytrail laptop
> > > actually comes from the sound driver.  First off, the atom/sst/sst.c
> > > has the following suspend code:
> > > 
> > > static int intel_sst_suspend(struct device *dev)
> > > {
> > > ....
> > > 	/*
> > > 	 * check if any stream is active and running
> > > 	 * they should already by suspend by soc_suspend
> > > 	 */
> > > 	for (i = 1; i <= ctx->info.max_streams; i++) {
> > > 		struct stream_info *stream = &ctx->streams[i];
> > > 
> > > 		if (stream->status == STREAM_RUNNING) {
> > > 			dev_err(dev, "stream %d is running, can't suspend, abort\n", i);
> > > 			return -EBUSY;
> > > 		}
> > > 	}
> > > 
> > > This doesn't look good, and I actually hit this when I tried to
> > > suspend while playing something.  In general, the driver shouldn't
> > > reject at this point, because this is the PM suspend callback,
> > > i.e. the user wants to suspend the device inevitably.  The driver
> > > should return an error only when it faces to a fatal error.
> > 
> > Mea culpa
> > 
> > And you are now second person to complain about this so I wonder if we
> > should rework this
> 
> Well, something is definitely wrong :)
> 
> > So from hardware POV, we need to first suspend all running streams and then
> > save the context from DSP (in order to restore them later) and then we can
> > shutdown the DSP.
> > 
> > The problem is driver being split into platform (which knows streams) and
> > sst dsp driver. So we devised a careful sequence where platform suspend is
> > invoked first (calls using asoc pm ops) and then DSP
> > 
> > This results is ASoC doing stream suspend for us and when we hit
> > intel_sst_suspend() we are supposed to have stream suspended, so save and
> > shut off DSP.
> > 
> > Now for you issue you see can you check why platform suspend is not getting
> > called?
> > 
> > > 
> > > But I wondered why this happened at all, and noticed that the machine
> > > driver (in my case bytcr_rt5640) has no its own PM ops.  But hooking
> > > the snd_soc_pm_ops there seems causing a hang up at suspend by some
> > > reason.
> > 
> > O yes, thats due to double suspend
> > 
> > See 3639ac1cd5177685a5c8abb7230096b680e1d497
> 
> I haven't followed the code deeply enough.  Who is calling to trigger
> double-suspend?

the machine and the platform see sst_soc_prepare()

> 
> 
> > > Maybe this wasn't a big problem until now since the BYT/CHT didn't
> > > support the suspend/resume properly in the past.  But now PM suspend
> > > is supported on these devices, so the problem surfaced more often.
> > 
> > The Chromebooks shipped on BSW use this method so..
> 
> Interestingly, when I checked another CHT machine with cx2072x codec,
> the PM works (although the playback doesn't restart at resume
> properly).

okay which machine driver was for cx2072x and which one are you using. I can
take a look at the code

> Wait...  Now closely looking at the code, I noticed the
> "ignore_suspend" marks in many places in bytcr_rt5640.c.  Why is this
> needed?
> 
> Other two machine drivers I've tested (cht_bsw_rt5672 and Pierre's
> cht_cx2072x) have no such a flag set, thus they work.  With the
> ignore_suspend, the PCM suspend calls are prevented, and it shall hit
> the problem above.

So ignore_suspend is used to keep PM on those devices even when platform is
suspended. It is quite used in keeping BIAS on when suspend, or doing
modem-codec interactions when SoC is in suspend.

I don't think we need this for a generic PC/laptop case, so removing them
should do the trick.

Use the working machine as ref :)

-- 
~Vinod