Skip to content

Simplify S0ix implementation #1481

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
ranj063 wants to merge 2 commits into from
Closed

Simplify S0ix implementation #1481

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b6d7102
ASoC: SOF: Modify logic for DSP D0i3 entry
ranj063 Nov 14, 2019
4a2494f
ASoC: SOF: Modify suspend/resume ops to specify state
ranj063 Nov 14, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 2 additions & 15 deletions sound/soc/sof/core.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -132,19 +132,6 @@ struct snd_sof_dai *snd_sof_find_dai(struct snd_sof_dev *sdev,
return NULL;
}

bool snd_sof_dsp_d0i3_on_suspend(struct snd_sof_dev *sdev)
{
struct snd_sof_pcm *spcm;

list_for_each_entry(spcm, &sdev->pcm_list, list) {
if (spcm->stream[SNDRV_PCM_STREAM_PLAYBACK].suspend_ignored ||
spcm->stream[SNDRV_PCM_STREAM_CAPTURE].suspend_ignored)
return true;
}

return false;
}

/*
* FW Panic/fault handling.
*/
Expand DownExpand Up@@ -471,8 +458,8 @@ int snd_sof_device_probe(struct device *dev, struct snd_sof_pdata *plat_data)
/* initialize sof device */
sdev->dev = dev;

/* initialize default D0 sub-state */
sdev->d0_substate = SOF_DSP_D0I0;
/* set initial DSP state */
sdev->dsp_power_state = SOF_DSP_D0I0;

sdev->pdata = plat_data;
sdev->first_boot = true;
Expand Down
39 changes: 32 additions & 7 deletions sound/soc/sof/intel/hda-dsp.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -339,13 +339,17 @@ static int hda_dsp_send_pm_gate_ipc(struct snd_sof_dev *sdev, u32 flags)
}

int hda_dsp_set_power_state(struct snd_sof_dev *sdev,
enum sof_d0_substate d0_substate)
enum sof_dsp_power_state state)
{
struct hdac_bus *bus = sof_to_bus(sdev);
u32 flags;
int ret;
u8 value;

/* Nothing to do for D3 */
if (state == SOF_DSP_D3)
return 0;

/* Write to D0I3C after Command-In-Progress bit is cleared */
ret = hda_dsp_wait_d0i3c_done(sdev);
if (ret < 0) {
Expand All@@ -354,7 +358,7 @@ int hda_dsp_set_power_state(struct snd_sof_dev *sdev,
}

/* Update D0I3C register */
value = d0_substate == SOF_DSP_D0I3 ? SOF_HDA_VS_D0I3C_I3 : 0;
value = state == SOF_DSP_D0I3 ? SOF_HDA_VS_D0I3C_I3 : 0;
snd_hdac_chip_updateb(bus, VS_D0I3C, SOF_HDA_VS_D0I3C_I3, value);
Comment thread
ranj063 marked this conversation as resolved.

/* Wait for cmd in progress to be cleared before exiting the function */
Expand All@@ -367,7 +371,7 @@ int hda_dsp_set_power_state(struct snd_sof_dev *sdev,
dev_vdbg(bus->dev, "D0I3C updated, register = 0x%x\n",
snd_hdac_chip_readb(bus, VS_D0I3C));

if (d0_substate == SOF_DSP_D0I0)
if (state == SOF_DSP_D0I0)
flags = HDA_PM_PPG;/* prevent power gating in D0 */
else
flags = HDA_PM_NO_DMA_TRACE;/* disable DMA trace in D0I3*/
Comment thread
ranj063 marked this conversation as resolved.
Expand DownExpand Up@@ -428,6 +432,8 @@ static int hda_suspend(struct snd_sof_dev *sdev, bool runtime_suspend)
return ret;
}

sdev->dsp_power_state = SOF_DSP_D3;

@keyonjiekeyonjieNov 14, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better to set D3 in the core driver, make D0 and D3 be the more generic states?

@ranj063ranj063Nov 14, 2019

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it makes sense here because the core requests the state and here's where the actual assignement happens isnt it?

@keyonjiekeyonjieNov 14, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it makes sense here because the core requests the state and here's where the actual assignement happens isnt it?

My point is that for platforms which don't have platform .suspend() ops, the dsp_power_state will not be changed then?

@ranj063ranj063Nov 14, 2019

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it will not and thats correct right? You never change the DSP state for those platforms.

return 0;
}

Expand DownExpand Up@@ -473,15 +479,26 @@ static int hda_resume(struct snd_sof_dev *sdev, bool runtime_resume)
hda_dsp_ctrl_ppcap_enable(sdev, true);
hda_dsp_ctrl_ppcap_int_enable(sdev, true);

sdev->dsp_power_state = SOF_DSP_D0I0;

return 0;
}

int hda_dsp_resume(struct snd_sof_dev *sdev)
int hda_dsp_resume(struct snd_sof_dev *sdev,
enum sof_dsp_power_state state)
{
struct sof_intel_hda_dev *hda = sdev->pdata->hw_pdata;
struct pci_dev *pci = to_pci_dev(sdev->dev);
int ret;

if (sdev->dsp_power_state == SOF_DSP_D0I3) {
/* set DSP power state */
ret = hda_dsp_set_power_state(sdev, state);
if (ret < 0)
return ret;

sdev->dsp_power_state = state;

if (sdev->s0_suspend) {
/* restore L1SEN bit */
if (hda->l1_support_changed)
snd_sof_dsp_update_bits(sdev, HDA_DSP_HDA_BAR,
Expand DownExpand Up@@ -523,14 +540,22 @@ int hda_dsp_runtime_suspend(struct snd_sof_dev *sdev)
return hda_suspend(sdev, true);
}

int hda_dsp_suspend(struct snd_sof_dev *sdev)
int hda_dsp_suspend(struct snd_sof_dev *sdev,
enum sof_dsp_power_state state)
{
struct sof_intel_hda_dev *hda = sdev->pdata->hw_pdata;
struct hdac_bus *bus = sof_to_bus(sdev);
struct pci_dev *pci = to_pci_dev(sdev->dev);
int ret;

if (sdev->s0_suspend) {
if (state == SOF_DSP_D0I3) {
/* set DSP power state */
ret = hda_dsp_set_power_state(sdev, state);
if (ret < 0)
return ret;

sdev->dsp_power_state = state;

/* enable L1SEN to make sure the system can enter S0Ix */
hda->l1_support_changed =
snd_sof_dsp_update_bits(sdev, HDA_DSP_HDA_BAR,
Expand Down
8 changes: 5 additions & 3 deletions sound/soc/sof/intel/hda.h
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -468,10 +468,12 @@ void hda_dsp_ipc_int_enable(struct snd_sof_dev *sdev);
void hda_dsp_ipc_int_disable(struct snd_sof_dev *sdev);

int hda_dsp_set_power_state(struct snd_sof_dev *sdev,
enum sof_d0_substate d0_substate);
enum sof_dsp_power_state state);

int hda_dsp_suspend(struct snd_sof_dev *sdev);
int hda_dsp_resume(struct snd_sof_dev *sdev);
int hda_dsp_suspend(struct snd_sof_dev *sdev,
enum sof_dsp_power_state state);
int hda_dsp_resume(struct snd_sof_dev *sdev,
enum sof_dsp_power_state state);
int hda_dsp_runtime_suspend(struct snd_sof_dev *sdev);
int hda_dsp_runtime_resume(struct snd_sof_dev *sdev);
int hda_dsp_runtime_idle(struct snd_sof_dev *sdev);
Expand Down
15 changes: 8 additions & 7 deletions sound/soc/sof/ops.h
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -138,18 +138,20 @@ static inline int snd_sof_dsp_get_window_offset(struct snd_sof_dev *sdev,
return -ENOTSUPP;
}
/* power management */
static inline int snd_sof_dsp_resume(struct snd_sof_dev *sdev)
static inline int snd_sof_dsp_resume(struct snd_sof_dev *sdev,
enum sof_dsp_power_state state)
{
if (sof_ops(sdev)->resume)
return sof_ops(sdev)->resume(sdev);
return sof_ops(sdev)->resume(sdev, state);

return 0;
}

static inline int snd_sof_dsp_suspend(struct snd_sof_dev *sdev)
static inline int snd_sof_dsp_suspend(struct snd_sof_dev *sdev,
enum sof_dsp_power_state state)
{
if (sof_ops(sdev)->suspend)
return sof_ops(sdev)->suspend(sdev);
return sof_ops(sdev)->suspend(sdev, state);

return 0;
}
Expand DownExpand Up@@ -194,12 +196,11 @@ static inline int snd_sof_dsp_set_clk(struct snd_sof_dev *sdev, u32 freq)
}

static inline int snd_sof_dsp_set_power_state(struct snd_sof_dev *sdev,
enum sof_d0_substate substate)
enum sof_dsp_power_state state)
{
if (sof_ops(sdev)->set_power_state)
return sof_ops(sdev)->set_power_state(sdev, substate);
return sof_ops(sdev)->set_power_state(sdev, state);

/* D0 substate is not supported */
return -ENOTSUPP;
}

Expand Down
34 changes: 17 additions & 17 deletions sound/soc/sof/pcm.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -346,13 +346,14 @@ static int sof_pcm_trigger(struct snd_soc_component *component,
stream.hdr.cmd |= SOF_IPC_STREAM_TRIG_RELEASE;
break;
case SNDRV_PCM_TRIGGER_RESUME:
if (spcm->stream[substream->stream].suspend_ignored) {
if (spcm->stream[substream->stream].d0i3_compatible &&
sdev->D0i3_pipeline_count) {
/*
* this case will be triggered when INFO_RESUME is
* supported, no need to resume streams that remained
* enabled in D0ix.
* supported, no need to start pipelines that were kept
* running during suspend.
*/
spcm->stream[substream->stream].suspend_ignored = false;
sdev->D0i3_pipeline_count--;
return 0;
}

Expand All@@ -366,27 +367,26 @@ static int sof_pcm_trigger(struct snd_soc_component *component,

/* fallthrough */
case SNDRV_PCM_TRIGGER_START:
if (spcm->stream[substream->stream].suspend_ignored) {
if (spcm->stream[substream->stream].d0i3_compatible &&
sdev->D0i3_pipeline_count) {
Comment thread
ranj063 marked this conversation as resolved.
/*
* This case will be triggered when INFO_RESUME is
* not supported, no need to re-start streams that
* remained enabled in D0ix.
* not supported, no need to re-start pipelines that
* were kept running during suspend.
*/
spcm->stream[substream->stream].suspend_ignored = false;
sdev->D0i3_pipeline_count--;
return 0;
}
stream.hdr.cmd |= SOF_IPC_STREAM_TRIG_START;
break;
case SNDRV_PCM_TRIGGER_SUSPEND:
if (sdev->s0_suspend &&
spcm->stream[substream->stream].d0i3_compatible) {
/*
* trap the event, not sending trigger stop to
* prevent the FW pipelines from being stopped,
* and mark the flag to ignore the upcoming DAPM
* PM events.
*/
spcm->stream[substream->stream].suspend_ignored = true;
/*
* D0i3-compatible pipelines should be kept running
* when suspending to S0ix.
*/
if (spcm->stream[substream->stream].d0i3_compatible &&
sdev->s0_suspend) {
sdev->D0i3_pipeline_count++;
return 0;
}
/* fallthrough */
Expand Down
71 changes: 8 additions & 63 deletions sound/soc/sof/pm.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -276,7 +276,7 @@ static int sof_resume(struct device *dev, bool runtime_resume)
if (runtime_resume)
ret = snd_sof_dsp_runtime_resume(sdev);
else
ret = snd_sof_dsp_resume(sdev);
ret = snd_sof_dsp_resume(sdev, SOF_DSP_D0I0);
if (ret < 0) {
dev_err(sdev->dev,
"error: failed to power up DSP after resume\n");
Expand DownExpand Up@@ -326,9 +326,6 @@ static int sof_resume(struct device *dev, bool runtime_resume)
"error: ctx_restore ipc error during resume %d\n",
ret);

/* initialize default D0 sub-state */
sdev->d0_substate = SOF_DSP_D0I0;

return ret;
}

Expand DownExpand Up@@ -382,7 +379,7 @@ static int sof_suspend(struct device *dev, bool runtime_suspend)
if (runtime_suspend)
ret = snd_sof_dsp_runtime_suspend(sdev);
else
ret = snd_sof_dsp_suspend(sdev);
ret = snd_sof_dsp_suspend(sdev, SOF_DSP_D3);
if (ret < 0)
dev_err(sdev->dev,
"error: failed to power down DSP during suspend %d\n",
Expand DownExpand Up@@ -411,26 +408,6 @@ int snd_sof_runtime_resume(struct device *dev)
}
EXPORT_SYMBOL(snd_sof_runtime_resume);

int snd_sof_set_d0_substate(struct snd_sof_dev *sdev,
enum sof_d0_substate d0_substate)
{
int ret;

if (sdev->d0_substate == d0_substate)
return 0;

/* do platform specific set_state */
ret = snd_sof_dsp_set_power_state(sdev, d0_substate);
if (ret < 0)
return ret;

/* update dsp D0 sub-state */
sdev->d0_substate = d0_substate;

return 0;
}
EXPORT_SYMBOL(snd_sof_set_d0_substate);

/*
* Audio DSP states may transform as below:-
*
Expand DownExpand Up@@ -466,27 +443,11 @@ EXPORT_SYMBOL(snd_sof_set_d0_substate);
int snd_sof_resume(struct device *dev)
{
struct snd_sof_dev *sdev = dev_get_drvdata(dev);
int ret;

if (snd_sof_dsp_d0i3_on_suspend(sdev)) {
/* resume from D0I3 */
dev_dbg(sdev->dev, "DSP will exit from D0i3...\n");
ret = snd_sof_set_d0_substate(sdev, SOF_DSP_D0I0);
if (ret == -ENOTSUPP) {
/* fallback to resume from D3 */
dev_dbg(sdev->dev, "D0i3 not supported, fall back to resume from D3...\n");
goto d3_resume;
} else if (ret < 0) {
dev_err(sdev->dev, "error: failed to exit from D0I3 %d\n",
ret);
return ret;
}

/* platform-specific resume from D0i3 */
return snd_sof_dsp_resume(sdev);
}
/* resume from D0i3 */
if (sdev->dsp_power_state == SOF_DSP_D0I3)
return snd_sof_dsp_resume(sdev, SOF_DSP_D0I0);

d3_resume:
/* resume from D3 */
return sof_resume(dev, false);
}
Expand All@@ -495,27 +456,11 @@ EXPORT_SYMBOL(snd_sof_resume);
int snd_sof_suspend(struct device *dev)
{
struct snd_sof_dev *sdev = dev_get_drvdata(dev);
int ret;

if (snd_sof_dsp_d0i3_on_suspend(sdev)) {
/* suspend to D0i3 */
dev_dbg(sdev->dev, "DSP is trying to enter D0i3...\n");
ret = snd_sof_set_d0_substate(sdev, SOF_DSP_D0I3);
if (ret == -ENOTSUPP) {
/* fallback to D3 suspend */
dev_dbg(sdev->dev, "D0i3 not supported, fall back to D3...\n");
goto d3_suspend;
} else if (ret < 0) {
dev_err(sdev->dev, "error: failed to enter D0I3, %d\n",
ret);
return ret;
}

/* platform-specific suspend to D0i3 */
return snd_sof_dsp_suspend(sdev);
}
/* suspend to D0i3 */
if (sdev->D0i3_pipeline_count)
return snd_sof_dsp_suspend(sdev, SOF_DSP_D0I3);

d3_suspend:
/* suspend to D3 */
return sof_suspend(dev, false);
}
Expand Down
Loading