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

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

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

On Fri, Jul 18, 2014 at 12:46:11PM +0100, Mark Brown wrote:
> On Wed, Jul 09, 2014 at 02:57:53PM +0530, Vinod Koul wrote:
> 
> > +static int sst_save_dsp_context_v2(struct intel_sst_drv *sst)
> 
> _v2?
v1 support older moorestown platforms. SO v2 here :)
> 
> > +static int intel_sst_runtime_suspend(struct device *dev)
> > +{
> > +	int ret = 0;
> > +	struct intel_sst_drv *ctx = dev_get_drvdata(dev);
> > +
> > +	pr_info("runtime_suspend called\n");
> 
> This is way too noisy.
> 
> > +	if (ctx->sst_state == SST_RESET) {
> > +		pr_debug("LPE is already in RESET state, No action");
> > +		return 0;
> > +	}
> > +	/*save fw context*/
> > +	if (ctx->ops->save_dsp_context(ctx))
> > +		return -EBUSY;
> > +
> > +	/* 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.
 
> > +	/* When fw_clear_cache is set, clear the cached firmware copy */
> > +	/* fw_clear_cache is set through debugfs support */
> > +	if (atomic_read(&ctx->fw_clear_cache) && ctx->fw_in_mem) {
> 
> Why?  It'd seem more direct to just have the debugfs write trigger
> trashing of the firmware on demand...
That would work too but then sequencing the DSP while running would cause
issues elsewhere. Since we dont have debugs support in the series we cna
push this bits later when we send debugfs series amoung others :)

> > +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?

> 
> > +static int intel_sst_runtime_idle(struct device *dev)
> > +{
> > +	struct intel_sst_drv *ctx = dev_get_drvdata(dev);
> > +
> > +	pr_info("runtime_idle called\n");
> > +	if (ctx->sst_state != SST_RESET) {
> > +		pm_schedule_suspend(dev, SST_SUSPEND_DELAY);
> > +		return -EBUSY;
> > +	} else {
> > +		return 0;
> > +	}
> > +	return -EBUSY;
> > +
> > +}
> 
> This is very strange, what is going on?
schedule suspend after SST_SUSPEND_DELAY. This should be updated with PM
framework calls for delay, will update


-- 
~Vinod

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

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