[alsa-devel] [PATCH 5/9] ASoC: SOF: Move DSP power state transitions to platform-specific ops
Amadeusz Sławiński
amadeuszx.slawinski at linux.intel.com
Thu Jan 30 08:47:28 CET 2020
On 1/29/2020 11:07 PM, Pierre-Louis Bossart wrote:
> From: Ranjani Sridharan <ranjani.sridharan at linux.intel.com>
>
> The DSP device substates such as D0I0/D0I3
> are platform-specific. Therefore, the d0_substate
> field of struct snd_sof_dev is replaced
> with the dsp_power_state field which represents the current
> state of the DSP. This field holds both the device state
> and the platform-specific substate values.
>
> With the DSP device substates being platform-specific,
> the DSP power state transitions need to be performed in
> the platform-specific suspend/resume ops as well.
>
> In order to achieve this, the ops signature has to be
> modified to pass the target device state as an
> argument. The target substate will be determined by
> the platform-specific ops before performing the transition.
> For example, in the case of the system suspending to S0IX,
> the top-level SOF device suspend callback needs to
> only determine if the DSP will be entering
> D3 or remain in D0. The target substate in case the device
> needs to remain in D0 (D0I0 or D0I3) will be determined
> by the platform-specific suspend op.
>
> With the addition of the extended set of power states for the DSP,
> the set_power_state op for HDA platforms has to be extended
> to handle only the appropriate state transitions. So, the
> implementation for the Intel HDA platforms is also modified.
>
> Signed-off-by: Ranjani Sridharan <ranjani.sridharan at linux.intel.com>
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart at linux.intel.com>
> ---
> sound/soc/sof/core.c | 4 +-
> sound/soc/sof/intel/hda-dsp.c | 223 +++++++++++++++++++++++++++++++---
> sound/soc/sof/intel/hda.h | 10 +-
> sound/soc/sof/ops.h | 16 +--
> sound/soc/sof/pm.c | 92 ++------------
> sound/soc/sof/sof-priv.h | 18 ++-
> 6 files changed, 242 insertions(+), 121 deletions(-)
>
> diff --git a/sound/soc/sof/core.c b/sound/soc/sof/core.c
> index 34cefbaf2d2a..1d07450aff77 100644
> --- a/sound/soc/sof/core.c
> +++ b/sound/soc/sof/core.c
> @@ -286,8 +286,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;
> + /* initialize default DSP power state */
> + sdev->dsp_power_state.state = SOF_DSP_PM_D0;
>
> sdev->pdata = plat_data;
> sdev->first_boot = true;
> diff --git a/sound/soc/sof/intel/hda-dsp.c b/sound/soc/sof/intel/hda-dsp.c
> index fddf2c48904f..8c00e128a7b0 100644
> --- a/sound/soc/sof/intel/hda-dsp.c
> +++ b/sound/soc/sof/intel/hda-dsp.c
> @@ -338,13 +338,10 @@ static int hda_dsp_send_pm_gate_ipc(struct snd_sof_dev *sdev, u32 flags)
> sizeof(pm_gate), &reply, sizeof(reply));
> }
>
> -int hda_dsp_set_power_state(struct snd_sof_dev *sdev,
> - enum sof_d0_substate d0_substate)
> +static int hda_dsp_update_d0i3c_register(struct snd_sof_dev *sdev, u8 value)
> {
> struct hdac_bus *bus = sof_to_bus(sdev);
> - u32 flags;
> int ret;
> - u8 value;
>
> /* Write to D0I3C after Command-In-Progress bit is cleared */
> ret = hda_dsp_wait_d0i3c_done(sdev);
> @@ -354,7 +351,6 @@ 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;
> snd_hdac_chip_updateb(bus, VS_D0I3C, SOF_HDA_VS_D0I3C_I3, value);
>
> /* Wait for cmd in progress to be cleared before exiting the function */
> @@ -367,20 +363,160 @@ 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)
> - flags = HDA_PM_PPG;/* prevent power gating in D0 */
> - else
> - flags = HDA_PM_NO_DMA_TRACE;/* disable DMA trace in D0I3*/
> + return 0;
> +}
>
> - /* sending pm_gate IPC */
> - ret = hda_dsp_send_pm_gate_ipc(sdev, flags);
> +static int hda_dsp_set_D0_state(struct snd_sof_dev *sdev,
> + const struct sof_dsp_power_state *target_state)
> +{
> + u32 flags = 0;
> + int ret;
> + u8 value = 0;
> +
> + /*
> + * Sanity check for illegal state transitions
> + * The only allowed transitions are:
> + * 1. D3 -> D0I0
> + * 2. D0I0 -> D0I3
> + * 3. D0I3 -> D0I0
> + */
> + switch (sdev->dsp_power_state.state) {
> + case SOF_DSP_PM_D0:
> + /* Follow the sequence below for D0 substate transitions */
> + break;
> + case SOF_DSP_PM_D3:
> + /* Follow regular flow for D3 -> D0 transition */
> + return 0;
> + default:
> + dev_err(sdev->dev, "error: transition from %d to %d not allowed\n",
> + sdev->dsp_power_state.state, target_state->state);
> + return -EINVAL;
> + }
> +
> + /* Set flags and register value for D0 target substate */
> + if (target_state->substate == SOF_HDA_DSP_PM_D0I3) {
> + value = SOF_HDA_VS_D0I3C_I3;
> +
> + /* disable DMA trace in D0I3 */
> + flags = HDA_PM_NO_DMA_TRACE;
> + } else {
> + /* prevent power gating in D0I0 */
> + flags = HDA_PM_PPG;
> + }
> +
> + /* update D0I3C register */
> + ret = hda_dsp_update_d0i3c_register(sdev, value);
> if (ret < 0)
> + return ret;
> +
> + /*
> + * Notify the DSP of the state change.
> + * If this IPC fails, revert the D0I3C register update in order
> + * to prevent partial state change.
> + */
> + ret = hda_dsp_send_pm_gate_ipc(sdev, flags);
> + if (ret < 0) {
> dev_err(sdev->dev,
> "error: PM_GATE ipc error %d\n", ret);
> + goto revert;
> + }
> +
> + return ret;
> +
> +revert:
> + /* fallback to the previous register value */
> + value = value ? 0 : SOF_HDA_VS_D0I3C_I3;
> +
> + /*
> + * This can fail but return the IPC error to signal that
> + * the state change failed.
> + */
> + hda_dsp_update_d0i3c_register(sdev, value);
>
> return ret;
> }
>
> +/*
> + * All DSP power state transitions are initiated by the driver.
> + * If the requested state change fails, the error is simply returned.
> + * Further state transitions are attempted only when the set_power_save() op
> + * is called again either because of a new IPC sent to the DSP or
> + * during system suspend/resume.
> + */
> +int hda_dsp_set_power_state(struct snd_sof_dev *sdev,
> + const struct sof_dsp_power_state *target_state)
> +{
> + int ret = 0;
> +
> + /* Nothing to do if the DSP is already in the requested state */
> + if (target_state->state == sdev->dsp_power_state.state &&
> + target_state->substate == sdev->dsp_power_state.substate)
> + return 0;
> +
> + switch (target_state->state) {
> + case SOF_DSP_PM_D0:
> + ret = hda_dsp_set_D0_state(sdev, target_state);
> + break;
> + case SOF_DSP_PM_D3:
> + /* The only allowed transition is: D0I0 -> D3 */
> + if (sdev->dsp_power_state.state == SOF_DSP_PM_D0 &&
> + sdev->dsp_power_state.substate == SOF_HDA_DSP_PM_D0I0)
> + break;
> +
> + dev_err(sdev->dev,
> + "error: transition from %d to %d not allowed\n",
> + sdev->dsp_power_state.state, target_state->state);
> + return -EINVAL;
> + default:
> + dev_err(sdev->dev, "error: target state unsupported %d\n",
> + target_state->state);
> + return -EINVAL;
> + }
> + if (ret < 0) {
> + dev_err(sdev->dev,
> + "failed to set requested target DSP state %d substate %d\n",
> + target_state->state, target_state->substate);
> + return ret;
> + }
> +
> + sdev->dsp_power_state = *target_state;
> + dev_dbg(sdev->dev, "New DSP state %d substate %d\n",
> + target_state->state, target_state->substate);
> + return ret;
> +}
> +
> +/*
> + * Audio DSP states may transform as below:-
> + *
> + * D0I3 compatible stream
> + * Runtime +---------------------+ opened only, timeout
> + * suspend | +--------------------+
> + * +------------+ D0(active) | |
> + * | | <---------------+ |
> + * | +--------> | | |
> + * | |Runtime +--^--+---------^--+--+ The last | |
> + * | |resume | | | | opened D0I3 | |
> + * | | | | | | compatible | |
> + * | | resume| | | | stream closed | |
> + * | | from | | D3 | | | |
> + * | | D3 | |suspend | | d0i3 | |
> + * | | | | | |suspend | |
> + * | | | | | | | |
> + * | | | | | | | |
> + * +-v---+-----------+--v-------+ | | +------+----v----+
> + * | | | +-----------> |
> + * | D3 (suspended) | | | D0I3 +-----+
> + * | | +--------------+ | |
> + * | | resume from | | |
> + * +-------------------^--------+ d0i3 suspend +----------------+ |
> + * | |
> + * | D3 suspend |
> + * +------------------------------------------------+
> + *
> + * d0i3_suspend = s0_suspend && D0I3 stream opened,
> + * D3 suspend = !d0i3_suspend,
> + */
> +
> static int hda_suspend(struct snd_sof_dev *sdev, bool runtime_suspend)
> {
> struct sof_intel_hda_dev *hda = sdev->pdata->hw_pdata;
> @@ -480,8 +616,22 @@ int hda_dsp_resume(struct snd_sof_dev *sdev)
> {
> struct sof_intel_hda_dev *hda = sdev->pdata->hw_pdata;
> struct pci_dev *pci = to_pci_dev(sdev->dev);
> + const struct sof_dsp_power_state target_state = {
> + .state = SOF_DSP_PM_D0,
> + .substate = SOF_HDA_DSP_PM_D0I0,
> + };
> + int ret;
> +
> + /* resume from D0I3 */
> + if (sdev->dsp_power_state.state == SOF_DSP_PM_D0) {
> + /* Set DSP power state */
> + ret = hda_dsp_set_power_state(sdev, &target_state);
> + if (ret < 0) {
> + dev_err(sdev->dev, "error: setting dsp state %d substate %d\n",
> + target_state.state, target_state.substate);
> + return ret;
> + }
>
> - if (sdev->system_suspend_target == SOF_SUSPEND_S0IX) {
> /* restore L1SEN bit */
> if (hda->l1_support_changed)
> snd_sof_dsp_update_bits(sdev, HDA_DSP_HDA_BAR,
> @@ -495,13 +645,27 @@ int hda_dsp_resume(struct snd_sof_dev *sdev)
> }
>
> /* init hda controller. DSP cores will be powered up during fw boot */
> - return hda_resume(sdev, false);
> + ret = hda_resume(sdev, false);
> + if (ret < 0)
> + return ret;
> +
> + hda_dsp_set_power_state(sdev, &target_state);
Return value of hda_dsp_set_power_state() seems to be checked or
directly returned in other functions, any reason to not do it here?
> + return ret;
> }
>
> int hda_dsp_runtime_resume(struct snd_sof_dev *sdev)
More information about the Alsa-devel
mailing list