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@xxxxxxxxxxxxxxx> 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@xxxxxxxxx>
> Signed-off-by: Archana Patni <archana.patni@xxxxxxxxx>
> Signed-off-by: Kai Vehmanen <kai.vehmanen@xxxxxxxxxxxxxxx>
> Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>
> Reviewed-by: Péter Ujfalusi <peter.ujfalusi@xxxxxxxxxxxxxxx>
> Reviewed-by: Ranjani Sridharan <ranjani.sridharan@xxxxxxxxxxxxxxx>
Tested-by: Ricardo Ribalda <ribalda@xxxxxxxxxxxx>
> ---
> 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
