Re: [PATCH 5/7] ASoC: Intel: sst: add power management handling

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

[-- Attachment #1.1: Type: text/plain, Size: 2107 bytes --]

On Fri, Jul 18, 2014 at 06:14:01PM +0100, Mark Brown wrote:
> On Fri, Jul 18, 2014 at 07:53:09PM +0530, Vinod Koul wrote:
> > On Fri, Jul 18, 2014 at 12:46:11PM +0100, Mark Brown wrote:
> 
> > > > +	/* Move the SST state to Reset */
> > > > +	sst_set_fw_state_locked(ctx, SST_RESET);
> 
> > > > +	flush_workqueue(ctx->post_msg_wq);
> > > > +	synchronize_irq(ctx->irq_num);
> 
> > > This is very strange - we're flushing the work *after* resetting the DSP
> > > which suggests there might be something else trying to access the DSP
> > > while it is being put into reset.  Presumably that'd be bad.
> 
> > the macro sst_set_fw_state_locked is used here :)
> 
> > And state is marked reset, we dont do HW reset, so this is okay. After we return the
> > suspend handler, PCI will put the device to D3.
> 
> It's still setting off alarm bells from a code review point of view -
> having the DSP marked as in reset may well break something that some of
> the other activity that's going on is doing.
yes that is a fair argument. But the next two calls will push any
pending messages to DSP. The reset will stop any client calls.

But then we have runtime code too, so while we are suspending that will be
waiting, so yes will move reset later.

> 
> > > > +static int intel_sst_suspend(struct device *dev)
> > > > +{
> > > > +
> > > > +	return intel_sst_runtime_suspend(dev);
> > > > +}
> 
> > > You currently need to check that the device isn't runtime suspended
> > > before you try to reuse runtime PM ops in suspend.
> 
> > Sorry didnt quite get why?
> 
> The driver should be restoring the device to the state it was in prior
> to suspend, and not repeating work that's already been done.
we dont support suspending when active. The Android usage model is that
system will not be S3 if audio is running. So no extra save work is required
here.

Moreover it needs active DSP FW support in order to save and restore, so we
cant do yet. Yes it will make sense to check device acticty and complain
loudly if we are in such a state, will add that.

-- 
~Vinod

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]