[PATCH] ASoC: add dai_reoder flag to reverse the stop sequence

[PATCH] ASoC: add dai_reoder flag to reverse the stop sequence

[Vijendar Mukunda]

From: Vijendar Mukunda <Vijendar.Mukunda@amd.com>

On stream stop, currently CPU DAI stop sequence is invoked first
followed by DMA. For Few platforms, it is required to stop the
DMA first before stopping CPU DAI.

For Stoneyridge platform, it is required to invoke DMA driver stop
first rather than invoking DWC I2S controller stop.
Introduced new flag in sound card structure for reordering stop sequence.
Based on flag check, ASoC core will re-order the stop requence.

Signed-off-by: Vijendar Mukunda <vijendar.mukunda@amd.com>
---
 include/sound/soc.h                  |  1 +
 sound/soc/amd/acp-da7219-max98357a.c |  1 +
 sound/soc/soc-pcm.c                  | 23 +++++++++++++++++------
 3 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/include/sound/soc.h b/include/sound/soc.h
index 675849d07284..12f79cb70600 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -982,6 +982,7 @@ struct snd_soc_card {
 	unsigned int disable_route_checks:1;
 	unsigned int probed:1;
 	unsigned int component_chaining:1;
+	unsigned int dai_reorder:1;
 
 	void *drvdata;
 };
diff --git a/sound/soc/amd/acp-da7219-max98357a.c b/sound/soc/amd/acp-da7219-max98357a.c
index 84e3906abd4f..b0533b4b7b18 100644
--- a/sound/soc/amd/acp-da7219-max98357a.c
+++ b/sound/soc/amd/acp-da7219-max98357a.c
@@ -742,6 +742,7 @@ static int cz_probe(struct platform_device *pdev)
 	if (!machine)
 		return -ENOMEM;
 	card->dev = &pdev->dev;
+	cz_card.dai_reorder = true;
 	platform_set_drvdata(pdev, card);
 	snd_soc_card_set_drvdata(card, machine);
 	ret = devm_snd_soc_register_card(&pdev->dev, card);
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 46513bb97904..4a9a51e6c988 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -1015,6 +1015,8 @@ static int soc_pcm_hw_params(struct snd_pcm_substream *substream,
 
 static int soc_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
 {
+	struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
++	struct snd_soc_card *card = rtd->card;
 	int ret = -EINVAL, _ret = 0;
 	int rollback = 0;
 
@@ -1055,14 +1057,23 @@ static int soc_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
 	case SNDRV_PCM_TRIGGER_STOP:
 	case SNDRV_PCM_TRIGGER_SUSPEND:
 	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
-		ret = snd_soc_pcm_dai_trigger(substream, cmd, rollback);
-		if (ret < 0)
-			break;
+		if (card->dai_reorder) {
+			ret = snd_soc_pcm_component_trigger(substream, cmd, rollback);
+			if (ret < 0)
+				break;
 
-		ret = snd_soc_pcm_component_trigger(substream, cmd, rollback);
-		if (ret < 0)
-			break;
+			ret = snd_soc_pcm_dai_trigger(substream, cmd, rollback);
+			if (ret < 0)
+				break;
+		} else {
+			ret = snd_soc_pcm_dai_trigger(substream, cmd, rollback);
+			if (ret < 0)
+				break;
 
+			ret = snd_soc_pcm_component_trigger(substream, cmd, rollback);
+			if (ret < 0)
+				break;
+		}
 		ret = snd_soc_link_trigger(substream, cmd, rollback);
 		break;
 	}
-- 
2.17.1

Re: [PATCH] ASoC: add dai_reoder flag to reverse the stop sequence

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 602 bytes --]

On Mon, Jul 05, 2021 at 09:28:28PM +0530, Vijendar Mukunda wrote:

> @@ -982,6 +982,7 @@ struct snd_soc_card {
>  	unsigned int disable_route_checks:1;
>  	unsigned int probed:1;
>  	unsigned int component_chaining:1;
> +	unsigned int dai_reorder:1;

This feels like it should be a per dai_link option rather than a card
wide option - the system could have a mix of links that do and don't
want this depending on why it's an issue.  The name probably also wants
to be more specific to what's being reordered, something like
stop_dma_first for example since it's only for stops and moves the DMA
first.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] ASoC: add dai_reoder flag to reverse the stop sequence

[Mukunda,Vijendar]

On 7/5/21 11:12 PM, Mark Brown wrote:
> On Mon, Jul 05, 2021 at 09:28:28PM +0530, Vijendar Mukunda wrote:
> 
>> @@ -982,6 +982,7 @@ struct snd_soc_card {
>>  	unsigned int disable_route_checks:1;
>>  	unsigned int probed:1;
>>  	unsigned int component_chaining:1;
>> +	unsigned int dai_reorder:1;
> 
> This feels like it should be a per dai_link option rather than a card
> wide option - the system could have a mix of links that do and don't
> want this depending on why it's an issue.  The name probably also wants
> to be more specific to what's being reordered, something like
> stop_dma_first for example since it's only for stops and moves the DMA
> first.
> 
As per our understanding by going with card wide option is easier rather
than checking dai link name for re-ordering the stop sequence for
specific platforms.
We will rename the flag as "stop_dma_fist" and will post the new version.

Re: [PATCH] ASoC: add dai_reoder flag to reverse the stop sequence

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 478 bytes --]

On Tue, Jul 06, 2021 at 12:30:10AM +0530, Mukunda,Vijendar wrote:

> As per our understanding by going with card wide option is easier rather
> than checking dai link name for re-ordering the stop sequence for
> specific platforms.
> We will rename the flag as "stop_dma_fist" and will post the new version.

Why would we need to check the name for the link?  Presumably all
affected AMD cards would just unconditionally set the flag, and other
systems could do what they like?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] ASoC: add dai_reoder flag to reverse the stop sequence

[Mukunda,Vijendar]

On 7/6/21 12:59 AM, Mark Brown wrote:
> On Tue, Jul 06, 2021 at 12:30:10AM +0530, Mukunda,Vijendar wrote:
> 
>> As per our understanding by going with card wide option is easier rather
>> than checking dai link name for re-ordering the stop sequence for
>> specific platforms.
>> We will rename the flag as "stop_dma_fist" and will post the new version.
> 
> Why would we need to check the name for the link?  Presumably all
> affected AMD cards would just unconditionally set the flag, and other
> systems could do what they like?
> 

This issue is only observed with older ACP2.x AMD platforms.
To make AMD specific platform change(which uses ACP 2.x IP), As per our
understanding we should only update the flag in ACP DMA driver by adding
flag in snd_pcm_substream structure rather than adding flag in card
structure.
Please suggest us, if there is any better place holder to add
"stop_dma_first" flag.

Due to some problem with mail client, i have received mail response
late. I have posted v2 version already.

Based on your suggestion, We will make the changes and post the new version.

Re: [PATCH] ASoC: add dai_reoder flag to reverse the stop sequence

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 550 bytes --]

On Tue, Jul 06, 2021 at 01:40:59AM +0530, Mukunda,Vijendar wrote:

> To make AMD specific platform change(which uses ACP 2.x IP), As per our
> understanding we should only update the flag in ACP DMA driver by adding
> flag in snd_pcm_substream structure rather than adding flag in card
> structure.
> Please suggest us, if there is any better place holder to add
> "stop_dma_first" flag.

I'd expect this to be configured by the machine driver in the dai_link.
It might need copying over to the substream for runtime use but the core
should do that.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]