[PATCH 1/2] ASoC: SOF: Intel: pci-tgl: unblock S5 entry if DMA stop has failed"

[Ricardo Ribalda]

Hi kai

nit: I think you have an extra " at the end of the subject

Thanks for the patch!

On Fri, 9 Dec 2022 at 12:46, Kai Vehmanen <kai.vehmanen at linux.intel.com> wrote:
>
> If system shutdown has not been completed cleanly, it is possible the
> DMA stream shutdown has not been done, or was not clean.
>
> If this is the case, Intel TGL/ADL HDA platforms may fail to shutdown
> cleanly due to pending HDA DMA transactions. To avoid this, detect this
> scenario in the shutdown callback, and perform an additional controller
> reset. This has been tested to unblock S5 entry if this condition is
> hit.
>
> Co-developed-by: Archana Patni <archana.patni at intel.com>
> Signed-off-by: Archana Patni <archana.patni at intel.com>
> Signed-off-by: Kai Vehmanen <kai.vehmanen at linux.intel.com>
> Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart at linux.intel.com>
> Reviewed-by: Péter Ujfalusi <peter.ujfalusi at linux.intel.com>
> Reviewed-by: Ranjani Sridharan <ranjani.sridharan at linux.intel.com>
Tested-by: Ricardo Ribalda <ribalda at chromium.org>
> ---
>  sound/soc/sof/intel/hda-dsp.c | 72 +++++++++++++++++++++++++++++++++++
>  sound/soc/sof/intel/hda.h     |  1 +
>  sound/soc/sof/intel/tgl.c     |  2 +-
>  3 files changed, 74 insertions(+), 1 deletion(-)
>
> diff --git a/sound/soc/sof/intel/hda-dsp.c b/sound/soc/sof/intel/hda-dsp.c
> index 5fa29df54b42..b4eacae8564c 100644
> --- a/sound/soc/sof/intel/hda-dsp.c
> +++ b/sound/soc/sof/intel/hda-dsp.c
> @@ -878,6 +878,78 @@ int hda_dsp_suspend(struct snd_sof_dev *sdev, u32 target_state)
>         return snd_sof_dsp_set_power_state(sdev, &target_dsp_state);
>  }
>
> +static unsigned int hda_dsp_check_for_dma_streams(struct snd_sof_dev *sdev)
> +{
> +       struct hdac_bus *bus = sof_to_bus(sdev);
> +       struct hdac_stream *s;
> +       unsigned int active_streams = 0;
> +       int sd_offset;
> +       u32 val;
> +
> +       list_for_each_entry(s, &bus->stream_list, list) {
> +               sd_offset = SOF_STREAM_SD_OFFSET(s);
> +               val = snd_sof_dsp_read(sdev, HDA_DSP_HDA_BAR,
> +                                      sd_offset);
> +               if (val & SOF_HDA_SD_CTL_DMA_START)
> +                       active_streams |= BIT(s->index);
> +       }
> +
> +       return active_streams;
> +}
> +
> +static int hda_dsp_s5_quirk(struct snd_sof_dev *sdev)
> +{
> +       int ret;
> +
> +       /*
> +        * Do not assume a certain timing between the prior
> +        * suspend flow, and running of this quirk function.
> +        * This is needed if the controller was just put
> +        * to reset before calling this function.
> +        */
> +       usleep_range(500, 1000);
> +
> +       /*
> +        * Take controller out of reset to flush DMA
> +        * transactions.
> +        */
> +       ret = hda_dsp_ctrl_link_reset(sdev, false);
> +       if (ret < 0)
> +               return ret;
> +
> +       usleep_range(500, 1000);
> +
> +       /* Restore state for shutdown, back to reset */
> +       ret = hda_dsp_ctrl_link_reset(sdev, true);
> +       if (ret < 0)
> +               return ret;
> +
> +       return ret;
> +}
> +
> +int hda_dsp_shutdown_dma_flush(struct snd_sof_dev *sdev)
> +{
> +       unsigned int active_streams;
> +       int ret, ret2;
> +
> +       /* check if DMA cleanup has been successful */
> +       active_streams = hda_dsp_check_for_dma_streams(sdev);
> +
> +       sdev->system_suspend_target = SOF_SUSPEND_S3;
> +       ret = snd_sof_suspend(sdev->dev);
> +
> +       if (active_streams) {
> +               dev_warn(sdev->dev,
> +                        "There were active DSP streams (%#x) at shutdown, trying to recover\n",
> +                        active_streams);
> +               ret2 = hda_dsp_s5_quirk(sdev);
> +               if (ret2 < 0)
> +                       dev_err(sdev->dev, "shutdown recovery failed (%d)\n", ret2);
> +       }
> +
> +       return ret;
> +}
> +
>  int hda_dsp_shutdown(struct snd_sof_dev *sdev)
>  {
>         sdev->system_suspend_target = SOF_SUSPEND_S3;
> diff --git a/sound/soc/sof/intel/hda.h b/sound/soc/sof/intel/hda.h
> index 022ce80968dd..caccaf8fba9c 100644
> --- a/sound/soc/sof/intel/hda.h
> +++ b/sound/soc/sof/intel/hda.h
> @@ -592,6 +592,7 @@ int hda_dsp_resume(struct snd_sof_dev *sdev);
>  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);
> +int hda_dsp_shutdown_dma_flush(struct snd_sof_dev *sdev);
>  int hda_dsp_shutdown(struct snd_sof_dev *sdev);
>  int hda_dsp_set_hw_params_upon_resume(struct snd_sof_dev *sdev);
>  void hda_dsp_dump(struct snd_sof_dev *sdev, u32 flags);
> diff --git a/sound/soc/sof/intel/tgl.c b/sound/soc/sof/intel/tgl.c
> index 30f2f49ee149..58ac3a46e6a7 100644
> --- a/sound/soc/sof/intel/tgl.c
> +++ b/sound/soc/sof/intel/tgl.c
> @@ -60,7 +60,7 @@ int sof_tgl_ops_init(struct snd_sof_dev *sdev)
>         memcpy(&sof_tgl_ops, &sof_hda_common_ops, sizeof(struct snd_sof_dsp_ops));
>
>         /* probe/remove/shutdown */
> -       sof_tgl_ops.shutdown    = hda_dsp_shutdown;
> +       sof_tgl_ops.shutdown    = hda_dsp_shutdown_dma_flush;
>
>         if (sdev->pdata->ipc_type == SOF_IPC) {
>                 /* doorbell */
> --
> 2.38.1
>


-- 
Ricardo Ribalda