[PATCH 1/2] ASoC: dwc: add a quirk DW_I2S_QUIRK_STOP_ON_SHUTDOWN to dwc driver

[PATCH 1/2] ASoC: dwc: add a quirk DW_I2S_QUIRK_STOP_ON_SHUTDOWN to dwc driver

[Vijendar Mukunda]

For CZ/StoneyRidge platforms, ACP DMA between ACP SRAM and
I2S FIFO should be stopped before stopping I2S Controller DMA.

When DMA is progressing and stop request received, while DMA transfer
ongoing between ACP SRAM and I2S FIFO, Stopping I2S DMA prior to ACP DMA
stop resulting DMA Channel stop failure.

This issue can't be fixed in ACP DMA driver due to design constraint.

Add a quirk DW_I2S_QUIRK_STOP_ON_SHUTDOWN in dwc driver to fix DMA stop
failure by invoking i2s_stop() sequence in shutdown() callback.

Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
---
 include/sound/designware_i2s.h | 1 +
 sound/soc/dwc/dwc-i2s.c        | 7 ++++++-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/include/sound/designware_i2s.h b/include/sound/designware_i2s.h
index 80d275b..a700d20 100644
--- a/include/sound/designware_i2s.h
+++ b/include/sound/designware_i2s.h
@@ -34,6 +34,7 @@ struct i2s_platform_data {
 	#define DW_I2S_QUIRK_COMP_REG_OFFSET	(1 << 0)
 	#define DW_I2S_QUIRK_COMP_PARAM1	(1 << 1)
 	#define DW_I2S_QUIRK_16BIT_IDX_OVERRIDE (1 << 2)
+	#define DW_I2S_QUIRK_STOP_ON_SHUTDOWN   (1 << 3)
 	unsigned int quirks;
 	unsigned int i2s_reg_comp1;
 	unsigned int i2s_reg_comp2;
diff --git a/sound/soc/dwc/dwc-i2s.c b/sound/soc/dwc/dwc-i2s.c
index fd41602..f3a681f 100644
--- a/sound/soc/dwc/dwc-i2s.c
+++ b/sound/soc/dwc/dwc-i2s.c
@@ -308,6 +308,10 @@ static int dw_i2s_hw_params(struct snd_pcm_substream *substream,
 static void dw_i2s_shutdown(struct snd_pcm_substream *substream,
 		struct snd_soc_dai *dai)
 {
+	struct dw_i2s_dev *dev = snd_soc_dai_get_drvdata(dai);
+
+	if (dev->quirks & DW_I2S_QUIRK_STOP_ON_SHUTDOWN)
+		i2s_stop(dev, substream);
 	snd_soc_dai_set_dma_data(dai, substream, NULL);
 }
 
@@ -342,7 +346,8 @@ static int dw_i2s_trigger(struct snd_pcm_substream *substream,
 	case SNDRV_PCM_TRIGGER_SUSPEND:
 	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
 		dev->active--;
-		i2s_stop(dev, substream);
+		if (!(dev->quirks & DW_I2S_QUIRK_STOP_ON_SHUTDOWN))
+			i2s_stop(dev, substream);
 		break;
 	default:
 		ret = -EINVAL;
-- 
2.7.4

[PATCH 1/2] ASoC: dwc: add a quirk DW_I2S_QUIRK_STOP_ON_SHUTDOWN to dwc driver

[Vijendar Mukunda]

For CZ/StoneyRidge platforms, ACP DMA between ACP SRAM and
I2S FIFO should be stopped before stopping I2S Controller DMA.

When DMA is progressing and stop request received, while DMA transfer
ongoing between ACP SRAM and I2S FIFO, Stopping I2S DMA prior to ACP DMA
stop resulting DMA Channel stop failure.

This issue can't be fixed in ACP DMA driver due to design constraint.

Add a quirk DW_I2S_QUIRK_STOP_ON_SHUTDOWN in dwc driver to fix DMA stop
failure by invoking i2s_stop() sequence in shutdown() callback.

Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
---
 include/sound/designware_i2s.h | 1 +
 sound/soc/dwc/dwc-i2s.c        | 7 ++++++-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/include/sound/designware_i2s.h b/include/sound/designware_i2s.h
index 80d275b..a700d20 100644
--- a/include/sound/designware_i2s.h
+++ b/include/sound/designware_i2s.h
@@ -34,6 +34,7 @@ struct i2s_platform_data {
 	#define DW_I2S_QUIRK_COMP_REG_OFFSET	(1 << 0)
 	#define DW_I2S_QUIRK_COMP_PARAM1	(1 << 1)
 	#define DW_I2S_QUIRK_16BIT_IDX_OVERRIDE (1 << 2)
+	#define DW_I2S_QUIRK_STOP_ON_SHUTDOWN   (1 << 3)
 	unsigned int quirks;
 	unsigned int i2s_reg_comp1;
 	unsigned int i2s_reg_comp2;
diff --git a/sound/soc/dwc/dwc-i2s.c b/sound/soc/dwc/dwc-i2s.c
index fd41602..f3a681f 100644
--- a/sound/soc/dwc/dwc-i2s.c
+++ b/sound/soc/dwc/dwc-i2s.c
@@ -308,6 +308,10 @@ static int dw_i2s_hw_params(struct snd_pcm_substream *substream,
 static void dw_i2s_shutdown(struct snd_pcm_substream *substream,
 		struct snd_soc_dai *dai)
 {
+	struct dw_i2s_dev *dev = snd_soc_dai_get_drvdata(dai);
+
+	if (dev->quirks & DW_I2S_QUIRK_STOP_ON_SHUTDOWN)
+		i2s_stop(dev, substream);
 	snd_soc_dai_set_dma_data(dai, substream, NULL);
 }
 
@@ -342,7 +346,8 @@ static int dw_i2s_trigger(struct snd_pcm_substream *substream,
 	case SNDRV_PCM_TRIGGER_SUSPEND:
 	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
 		dev->active--;
-		i2s_stop(dev, substream);
+		if (!(dev->quirks & DW_I2S_QUIRK_STOP_ON_SHUTDOWN))
+			i2s_stop(dev, substream);
 		break;
 	default:
 		ret = -EINVAL;
-- 
2.7.4

[PATCH 2/2] drm/amd/amdgpu: Add dwc quirk for Stoney/CZ platforms

[Vijendar Mukunda]

Add a quirk DW_I2S_QUIRK_STOP_ON_SHUTDOWN for Stoney/CZ
platforms.

For CZ/StoneyRidge platforms, ACP DMA between ACP SRAM and
I2S FIFO should be stopped before stopping I2S Controller DMA.

When DMA is progressing and stop request received, while DMA
transfer ongoing between ACP SRAM and I2S FIFO, Stopping I2S DMA
prior to ACP DMA stop resulting ACP DMA channel stop failure.

ACP DMA driver can't fix this issue due to design constraint.
By setting this quirk, I2S DMA gets stopped after ACP DMA stop
which will fix the ACP DMA stop failure.

Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c | 38 +++++++++++----------------------
 1 file changed, 13 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c
index b8655ff..6866b73 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c
@@ -245,42 +245,30 @@ static int acp_hw_init(void *handle)
 		goto failure;
 	}
 
-	switch (adev->asic_type) {
-	case CHIP_STONEY:
-		i2s_pdata[0].quirks = DW_I2S_QUIRK_COMP_REG_OFFSET |
-			DW_I2S_QUIRK_16BIT_IDX_OVERRIDE;
-		break;
-	default:
-		i2s_pdata[0].quirks = DW_I2S_QUIRK_COMP_REG_OFFSET;
-	}
+	i2s_pdata[0].quirks = DW_I2S_QUIRK_COMP_REG_OFFSET |
+			      DW_I2S_QUIRK_STOP_ON_SHUTDOWN;
+	if (adev->asic_type == CHIP_STONEY)
+		i2s_pdata[0].quirks |= DW_I2S_QUIRK_16BIT_IDX_OVERRIDE;
+
 	i2s_pdata[0].cap = DWC_I2S_PLAY;
 	i2s_pdata[0].snd_rates = SNDRV_PCM_RATE_8000_96000;
 	i2s_pdata[0].i2s_reg_comp1 = ACP_I2S_COMP1_PLAY_REG_OFFSET;
 	i2s_pdata[0].i2s_reg_comp2 = ACP_I2S_COMP2_PLAY_REG_OFFSET;
-	switch (adev->asic_type) {
-	case CHIP_STONEY:
-		i2s_pdata[1].quirks = DW_I2S_QUIRK_COMP_REG_OFFSET |
-			DW_I2S_QUIRK_COMP_PARAM1 |
-			DW_I2S_QUIRK_16BIT_IDX_OVERRIDE;
-		break;
-	default:
-		i2s_pdata[1].quirks = DW_I2S_QUIRK_COMP_REG_OFFSET |
-			DW_I2S_QUIRK_COMP_PARAM1;
-	}
+	i2s_pdata[1].quirks = DW_I2S_QUIRK_COMP_REG_OFFSET |
+			      DW_I2S_QUIRK_COMP_PARAM1 |
+			      DW_I2S_QUIRK_STOP_ON_SHUTDOWN;
+	if (adev->asic_type == CHIP_STONEY)
+		i2s_pdata[1].quirks |= DW_I2S_QUIRK_16BIT_IDX_OVERRIDE;
 
 	i2s_pdata[1].cap = DWC_I2S_RECORD;
 	i2s_pdata[1].snd_rates = SNDRV_PCM_RATE_8000_96000;
 	i2s_pdata[1].i2s_reg_comp1 = ACP_I2S_COMP1_CAP_REG_OFFSET;
 	i2s_pdata[1].i2s_reg_comp2 = ACP_I2S_COMP2_CAP_REG_OFFSET;
 
-	i2s_pdata[2].quirks = DW_I2S_QUIRK_COMP_REG_OFFSET;
-	switch (adev->asic_type) {
-	case CHIP_STONEY:
+	i2s_pdata[2].quirks = DW_I2S_QUIRK_COMP_REG_OFFSET |
+			      DW_I2S_QUIRK_STOP_ON_SHUTDOWN;
+	if (adev->asic_type == CHIP_STONEY)
 		i2s_pdata[2].quirks |= DW_I2S_QUIRK_16BIT_IDX_OVERRIDE;
-		break;
-	default:
-		break;
-	}
 
 	i2s_pdata[2].cap = DWC_I2S_PLAY | DWC_I2S_RECORD;
 	i2s_pdata[2].snd_rates = SNDRV_PCM_RATE_8000_96000;
-- 
2.7.4

[PATCH 2/2] drm/amd/amdgpu: Add dwc quirk for Stoney/CZ platforms

[Vijendar Mukunda]

Add a quirk DW_I2S_QUIRK_STOP_ON_SHUTDOWN for Stoney/CZ
platforms.

For CZ/StoneyRidge platforms, ACP DMA between ACP SRAM and
I2S FIFO should be stopped before stopping I2S Controller DMA.

When DMA is progressing and stop request received, while DMA
transfer ongoing between ACP SRAM and I2S FIFO, Stopping I2S DMA
prior to ACP DMA stop resulting ACP DMA channel stop failure.

ACP DMA driver can't fix this issue due to design constraint.
By setting this quirk, I2S DMA gets stopped after ACP DMA stop
which will fix the ACP DMA stop failure.

Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c | 38 +++++++++++----------------------
 1 file changed, 13 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c
index b8655ff..6866b73 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c
@@ -245,42 +245,30 @@ static int acp_hw_init(void *handle)
 		goto failure;
 	}
 
-	switch (adev->asic_type) {
-	case CHIP_STONEY:
-		i2s_pdata[0].quirks = DW_I2S_QUIRK_COMP_REG_OFFSET |
-			DW_I2S_QUIRK_16BIT_IDX_OVERRIDE;
-		break;
-	default:
-		i2s_pdata[0].quirks = DW_I2S_QUIRK_COMP_REG_OFFSET;
-	}
+	i2s_pdata[0].quirks = DW_I2S_QUIRK_COMP_REG_OFFSET |
+			      DW_I2S_QUIRK_STOP_ON_SHUTDOWN;
+	if (adev->asic_type == CHIP_STONEY)
+		i2s_pdata[0].quirks |= DW_I2S_QUIRK_16BIT_IDX_OVERRIDE;
+
 	i2s_pdata[0].cap = DWC_I2S_PLAY;
 	i2s_pdata[0].snd_rates = SNDRV_PCM_RATE_8000_96000;
 	i2s_pdata[0].i2s_reg_comp1 = ACP_I2S_COMP1_PLAY_REG_OFFSET;
 	i2s_pdata[0].i2s_reg_comp2 = ACP_I2S_COMP2_PLAY_REG_OFFSET;
-	switch (adev->asic_type) {
-	case CHIP_STONEY:
-		i2s_pdata[1].quirks = DW_I2S_QUIRK_COMP_REG_OFFSET |
-			DW_I2S_QUIRK_COMP_PARAM1 |
-			DW_I2S_QUIRK_16BIT_IDX_OVERRIDE;
-		break;
-	default:
-		i2s_pdata[1].quirks = DW_I2S_QUIRK_COMP_REG_OFFSET |
-			DW_I2S_QUIRK_COMP_PARAM1;
-	}
+	i2s_pdata[1].quirks = DW_I2S_QUIRK_COMP_REG_OFFSET |
+			      DW_I2S_QUIRK_COMP_PARAM1 |
+			      DW_I2S_QUIRK_STOP_ON_SHUTDOWN;
+	if (adev->asic_type == CHIP_STONEY)
+		i2s_pdata[1].quirks |= DW_I2S_QUIRK_16BIT_IDX_OVERRIDE;
 
 	i2s_pdata[1].cap = DWC_I2S_RECORD;
 	i2s_pdata[1].snd_rates = SNDRV_PCM_RATE_8000_96000;
 	i2s_pdata[1].i2s_reg_comp1 = ACP_I2S_COMP1_CAP_REG_OFFSET;
 	i2s_pdata[1].i2s_reg_comp2 = ACP_I2S_COMP2_CAP_REG_OFFSET;
 
-	i2s_pdata[2].quirks = DW_I2S_QUIRK_COMP_REG_OFFSET;
-	switch (adev->asic_type) {
-	case CHIP_STONEY:
+	i2s_pdata[2].quirks = DW_I2S_QUIRK_COMP_REG_OFFSET |
+			      DW_I2S_QUIRK_STOP_ON_SHUTDOWN;
+	if (adev->asic_type == CHIP_STONEY)
 		i2s_pdata[2].quirks |= DW_I2S_QUIRK_16BIT_IDX_OVERRIDE;
-		break;
-	default:
-		break;
-	}
 
 	i2s_pdata[2].cap = DWC_I2S_PLAY | DWC_I2S_RECORD;
 	i2s_pdata[2].snd_rates = SNDRV_PCM_RATE_8000_96000;
-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 2/2] drm/amd/amdgpu: Add dwc quirk for Stoney/CZ platforms

[Vijendar Mukunda]

Add a quirk DW_I2S_QUIRK_STOP_ON_SHUTDOWN for Stoney/CZ
platforms.

For CZ/StoneyRidge platforms, ACP DMA between ACP SRAM and
I2S FIFO should be stopped before stopping I2S Controller DMA.

When DMA is progressing and stop request received, while DMA
transfer ongoing between ACP SRAM and I2S FIFO, Stopping I2S DMA
prior to ACP DMA stop resulting ACP DMA channel stop failure.

ACP DMA driver can't fix this issue due to design constraint.
By setting this quirk, I2S DMA gets stopped after ACP DMA stop
which will fix the ACP DMA stop failure.

Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c | 38 +++++++++++----------------------
 1 file changed, 13 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c
index b8655ff..6866b73 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c
@@ -245,42 +245,30 @@ static int acp_hw_init(void *handle)
 		goto failure;
 	}
 
-	switch (adev->asic_type) {
-	case CHIP_STONEY:
-		i2s_pdata[0].quirks = DW_I2S_QUIRK_COMP_REG_OFFSET |
-			DW_I2S_QUIRK_16BIT_IDX_OVERRIDE;
-		break;
-	default:
-		i2s_pdata[0].quirks = DW_I2S_QUIRK_COMP_REG_OFFSET;
-	}
+	i2s_pdata[0].quirks = DW_I2S_QUIRK_COMP_REG_OFFSET |
+			      DW_I2S_QUIRK_STOP_ON_SHUTDOWN;
+	if (adev->asic_type == CHIP_STONEY)
+		i2s_pdata[0].quirks |= DW_I2S_QUIRK_16BIT_IDX_OVERRIDE;
+
 	i2s_pdata[0].cap = DWC_I2S_PLAY;
 	i2s_pdata[0].snd_rates = SNDRV_PCM_RATE_8000_96000;
 	i2s_pdata[0].i2s_reg_comp1 = ACP_I2S_COMP1_PLAY_REG_OFFSET;
 	i2s_pdata[0].i2s_reg_comp2 = ACP_I2S_COMP2_PLAY_REG_OFFSET;
-	switch (adev->asic_type) {
-	case CHIP_STONEY:
-		i2s_pdata[1].quirks = DW_I2S_QUIRK_COMP_REG_OFFSET |
-			DW_I2S_QUIRK_COMP_PARAM1 |
-			DW_I2S_QUIRK_16BIT_IDX_OVERRIDE;
-		break;
-	default:
-		i2s_pdata[1].quirks = DW_I2S_QUIRK_COMP_REG_OFFSET |
-			DW_I2S_QUIRK_COMP_PARAM1;
-	}
+	i2s_pdata[1].quirks = DW_I2S_QUIRK_COMP_REG_OFFSET |
+			      DW_I2S_QUIRK_COMP_PARAM1 |
+			      DW_I2S_QUIRK_STOP_ON_SHUTDOWN;
+	if (adev->asic_type == CHIP_STONEY)
+		i2s_pdata[1].quirks |= DW_I2S_QUIRK_16BIT_IDX_OVERRIDE;
 
 	i2s_pdata[1].cap = DWC_I2S_RECORD;
 	i2s_pdata[1].snd_rates = SNDRV_PCM_RATE_8000_96000;
 	i2s_pdata[1].i2s_reg_comp1 = ACP_I2S_COMP1_CAP_REG_OFFSET;
 	i2s_pdata[1].i2s_reg_comp2 = ACP_I2S_COMP2_CAP_REG_OFFSET;
 
-	i2s_pdata[2].quirks = DW_I2S_QUIRK_COMP_REG_OFFSET;
-	switch (adev->asic_type) {
-	case CHIP_STONEY:
+	i2s_pdata[2].quirks = DW_I2S_QUIRK_COMP_REG_OFFSET |
+			      DW_I2S_QUIRK_STOP_ON_SHUTDOWN;
+	if (adev->asic_type == CHIP_STONEY)
 		i2s_pdata[2].quirks |= DW_I2S_QUIRK_16BIT_IDX_OVERRIDE;
-		break;
-	default:
-		break;
-	}
 
 	i2s_pdata[2].cap = DWC_I2S_PLAY | DWC_I2S_RECORD;
 	i2s_pdata[2].snd_rates = SNDRV_PCM_RATE_8000_96000;
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/2] drm/amd/amdgpu: Add dwc quirk for Stoney/CZ platforms

[Vijendar Mukunda]

Add a quirk DW_I2S_QUIRK_STOP_ON_SHUTDOWN for Stoney/CZ
platforms.

For CZ/StoneyRidge platforms, ACP DMA between ACP SRAM and
I2S FIFO should be stopped before stopping I2S Controller DMA.

When DMA is progressing and stop request received, while DMA
transfer ongoing between ACP SRAM and I2S FIFO, Stopping I2S DMA
prior to ACP DMA stop resulting ACP DMA channel stop failure.

ACP DMA driver can't fix this issue due to design constraint.
By setting this quirk, I2S DMA gets stopped after ACP DMA stop
which will fix the ACP DMA stop failure.

Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c | 38 +++++++++++----------------------
 1 file changed, 13 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c
index b8655ff..6866b73 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c
@@ -245,42 +245,30 @@ static int acp_hw_init(void *handle)
 		goto failure;
 	}
 
-	switch (adev->asic_type) {
-	case CHIP_STONEY:
-		i2s_pdata[0].quirks = DW_I2S_QUIRK_COMP_REG_OFFSET |
-			DW_I2S_QUIRK_16BIT_IDX_OVERRIDE;
-		break;
-	default:
-		i2s_pdata[0].quirks = DW_I2S_QUIRK_COMP_REG_OFFSET;
-	}
+	i2s_pdata[0].quirks = DW_I2S_QUIRK_COMP_REG_OFFSET |
+			      DW_I2S_QUIRK_STOP_ON_SHUTDOWN;
+	if (adev->asic_type == CHIP_STONEY)
+		i2s_pdata[0].quirks |= DW_I2S_QUIRK_16BIT_IDX_OVERRIDE;
+
 	i2s_pdata[0].cap = DWC_I2S_PLAY;
 	i2s_pdata[0].snd_rates = SNDRV_PCM_RATE_8000_96000;
 	i2s_pdata[0].i2s_reg_comp1 = ACP_I2S_COMP1_PLAY_REG_OFFSET;
 	i2s_pdata[0].i2s_reg_comp2 = ACP_I2S_COMP2_PLAY_REG_OFFSET;
-	switch (adev->asic_type) {
-	case CHIP_STONEY:
-		i2s_pdata[1].quirks = DW_I2S_QUIRK_COMP_REG_OFFSET |
-			DW_I2S_QUIRK_COMP_PARAM1 |
-			DW_I2S_QUIRK_16BIT_IDX_OVERRIDE;
-		break;
-	default:
-		i2s_pdata[1].quirks = DW_I2S_QUIRK_COMP_REG_OFFSET |
-			DW_I2S_QUIRK_COMP_PARAM1;
-	}
+	i2s_pdata[1].quirks = DW_I2S_QUIRK_COMP_REG_OFFSET |
+			      DW_I2S_QUIRK_COMP_PARAM1 |
+			      DW_I2S_QUIRK_STOP_ON_SHUTDOWN;
+	if (adev->asic_type == CHIP_STONEY)
+		i2s_pdata[1].quirks |= DW_I2S_QUIRK_16BIT_IDX_OVERRIDE;
 
 	i2s_pdata[1].cap = DWC_I2S_RECORD;
 	i2s_pdata[1].snd_rates = SNDRV_PCM_RATE_8000_96000;
 	i2s_pdata[1].i2s_reg_comp1 = ACP_I2S_COMP1_CAP_REG_OFFSET;
 	i2s_pdata[1].i2s_reg_comp2 = ACP_I2S_COMP2_CAP_REG_OFFSET;
 
-	i2s_pdata[2].quirks = DW_I2S_QUIRK_COMP_REG_OFFSET;
-	switch (adev->asic_type) {
-	case CHIP_STONEY:
+	i2s_pdata[2].quirks = DW_I2S_QUIRK_COMP_REG_OFFSET |
+			      DW_I2S_QUIRK_STOP_ON_SHUTDOWN;
+	if (adev->asic_type == CHIP_STONEY)
 		i2s_pdata[2].quirks |= DW_I2S_QUIRK_16BIT_IDX_OVERRIDE;
-		break;
-	default:
-		break;
-	}
 
 	i2s_pdata[2].cap = DWC_I2S_PLAY | DWC_I2S_RECORD;
 	i2s_pdata[2].snd_rates = SNDRV_PCM_RATE_8000_96000;
-- 
2.7.4

Re: [PATCH 1/2] ASoC: dwc: add a quirk DW_I2S_QUIRK_STOP_ON_SHUTDOWN to dwc driver

[Mark Brown]

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

On Fri, Apr 23, 2021 at 09:54:38PM +0530, Vijendar Mukunda wrote:

> For CZ/StoneyRidge platforms, ACP DMA between ACP SRAM and
> I2S FIFO should be stopped before stopping I2S Controller DMA.

> When DMA is progressing and stop request received, while DMA transfer
> ongoing between ACP SRAM and I2S FIFO, Stopping I2S DMA prior to ACP DMA
> stop resulting DMA Channel stop failure.

This again...  copying in Peter for the sequencing discussion.  If we
need to do this I'm not convinced that bodging it in the driver is a
good idea, and especially not deferring it outside of the trigger
operation - for example on a suspend or pause we won't actually do a
shutdown() so the trigger will end up not happening which seems like it
may cause problems.  We'd probably be better off with the core knowing
what's going on and being able to reorder the callbacks although
designing an interface for that seems a bit annoying.

> This issue can't be fixed in ACP DMA driver due to design constraint.

What is the design constraint here - can't we fix the design?  Or is it
a hardware design constraint (presumably broken signalling between the
I2S and DMA blocks)?

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

Re: [PATCH 1/2] ASoC: dwc: add a quirk DW_I2S_QUIRK_STOP_ON_SHUTDOWN to dwc driver

[Mark Brown]

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

On Fri, Apr 23, 2021 at 09:54:38PM +0530, Vijendar Mukunda wrote:

> For CZ/StoneyRidge platforms, ACP DMA between ACP SRAM and
> I2S FIFO should be stopped before stopping I2S Controller DMA.

> When DMA is progressing and stop request received, while DMA transfer
> ongoing between ACP SRAM and I2S FIFO, Stopping I2S DMA prior to ACP DMA
> stop resulting DMA Channel stop failure.

This again...  copying in Peter for the sequencing discussion.  If we
need to do this I'm not convinced that bodging it in the driver is a
good idea, and especially not deferring it outside of the trigger
operation - for example on a suspend or pause we won't actually do a
shutdown() so the trigger will end up not happening which seems like it
may cause problems.  We'd probably be better off with the core knowing
what's going on and being able to reorder the callbacks although
designing an interface for that seems a bit annoying.

> This issue can't be fixed in ACP DMA driver due to design constraint.

What is the design constraint here - can't we fix the design?  Or is it
a hardware design constraint (presumably broken signalling between the
I2S and DMA blocks)?

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

Re: [PATCH 1/2] ASoC: dwc: add a quirk DW_I2S_QUIRK_STOP_ON_SHUTDOWN to dwc driver

[Péter Ujfalusi]

On 4/23/21 7:46 PM, Mark Brown wrote:
> On Fri, Apr 23, 2021 at 09:54:38PM +0530, Vijendar Mukunda wrote:
> 
>> For CZ/StoneyRidge platforms, ACP DMA between ACP SRAM and
>> I2S FIFO should be stopped before stopping I2S Controller DMA.
> 
>> When DMA is progressing and stop request received, while DMA transfer
>> ongoing between ACP SRAM and I2S FIFO, Stopping I2S DMA prior to ACP DMA
>> stop resulting DMA Channel stop failure.
> 
> This again...  copying in Peter for the sequencing discussion.  If we
> need to do this I'm not convinced that bodging it in the driver is a
> good idea, and especially not deferring it outside of the trigger
> operation - for example on a suspend or pause we won't actually do a
> shutdown() so the trigger will end up not happening which seems like it
> may cause problems.

It will certainly leave the i2s running and can lead to hard to explain
issues

> We'd probably be better off with the core knowing
> what's going on and being able to reorder the callbacks although
> designing an interface for that seems a bit annoying.

I agree, it would be better to have some sort of flag which tells the
core that there is an integration issue between the DMA and peripheral.
I believe this is only affecting playback?

>> This issue can't be fixed in ACP DMA driver due to design constraint.
> 
> What is the design constraint here - can't we fix the design?  Or is it
> a hardware design constraint (presumably broken signalling between the
> I2S and DMA blocks)?

From the description my guess is that stop on the DMA want to flush it's
FIFO (complete the in progress packet, segment). Since the peripheral is
stopped it will not pull in more data -> the DMA will time out internally.

The question: how the ACP DMA driver's terminate_all is implemented? It
can not really wait for the DMA to stop, we can not use
terminate_all_sync() in trigger, it must just set a stop bit and make
sure at synchronize() time that it has stopped, right?

What happens if the time between the DMA stop and the DAI stop is less
then it would take to flush the DMA FIFO? You would have the same issue,
but in a rather hard to reproducible way?

As sidenote: TI's k3-udma initially had similar issue at the design
phase on the playback side which got solved by a flush bit on the
channel to detach it from the peripheral and set it to free run to drain
w/o peripheral.
On capture however I need to push a dummy 'drain' packet to flush out
the data from the DMA (if the stop happens when we did not have active
descriptor on the channel).

With a flag to reorder the DMA/DAI stop sequence it might work most of
the time, but imho it is going to introduce a nasty time-bomb of failure.
Also your DAI will underflow instead of the DMA error.

-- 
Péter

Re: [PATCH 1/2] ASoC: dwc: add a quirk DW_I2S_QUIRK_STOP_ON_SHUTDOWN to dwc driver

[Péter Ujfalusi]

On 4/23/21 7:46 PM, Mark Brown wrote:
> On Fri, Apr 23, 2021 at 09:54:38PM +0530, Vijendar Mukunda wrote:
> 
>> For CZ/StoneyRidge platforms, ACP DMA between ACP SRAM and
>> I2S FIFO should be stopped before stopping I2S Controller DMA.
> 
>> When DMA is progressing and stop request received, while DMA transfer
>> ongoing between ACP SRAM and I2S FIFO, Stopping I2S DMA prior to ACP DMA
>> stop resulting DMA Channel stop failure.
> 
> This again...  copying in Peter for the sequencing discussion.  If we
> need to do this I'm not convinced that bodging it in the driver is a
> good idea, and especially not deferring it outside of the trigger
> operation - for example on a suspend or pause we won't actually do a
> shutdown() so the trigger will end up not happening which seems like it
> may cause problems.

It will certainly leave the i2s running and can lead to hard to explain
issues

> We'd probably be better off with the core knowing
> what's going on and being able to reorder the callbacks although
> designing an interface for that seems a bit annoying.

I agree, it would be better to have some sort of flag which tells the
core that there is an integration issue between the DMA and peripheral.
I believe this is only affecting playback?

>> This issue can't be fixed in ACP DMA driver due to design constraint.
> 
> What is the design constraint here - can't we fix the design?  Or is it
> a hardware design constraint (presumably broken signalling between the
> I2S and DMA blocks)?

From the description my guess is that stop on the DMA want to flush it's
FIFO (complete the in progress packet, segment). Since the peripheral is
stopped it will not pull in more data -> the DMA will time out internally.

The question: how the ACP DMA driver's terminate_all is implemented? It
can not really wait for the DMA to stop, we can not use
terminate_all_sync() in trigger, it must just set a stop bit and make
sure at synchronize() time that it has stopped, right?

What happens if the time between the DMA stop and the DAI stop is less
then it would take to flush the DMA FIFO? You would have the same issue,
but in a rather hard to reproducible way?

As sidenote: TI's k3-udma initially had similar issue at the design
phase on the playback side which got solved by a flush bit on the
channel to detach it from the peripheral and set it to free run to drain
w/o peripheral.
On capture however I need to push a dummy 'drain' packet to flush out
the data from the DMA (if the stop happens when we did not have active
descriptor on the channel).

With a flag to reorder the DMA/DAI stop sequence it might work most of
the time, but imho it is going to introduce a nasty time-bomb of failure.
Also your DAI will underflow instead of the DMA error.

-- 
Péter

Re: [PATCH 1/2] ASoC: dwc: add a quirk DW_I2S_QUIRK_STOP_ON_SHUTDOWN to dwc driver

[Mukunda,Vijendar]

On 4/26/21 11:49 AM, Péter Ujfalusi wrote:
> 
> 
> On 4/23/21 7:46 PM, Mark Brown wrote:
>> On Fri, Apr 23, 2021 at 09:54:38PM +0530, Vijendar Mukunda wrote:
>>
>>> For CZ/StoneyRidge platforms, ACP DMA between ACP SRAM and
>>> I2S FIFO should be stopped before stopping I2S Controller DMA.
>>
>>> When DMA is progressing and stop request received, while DMA transfer
>>> ongoing between ACP SRAM and I2S FIFO, Stopping I2S DMA prior to ACP DMA
>>> stop resulting DMA Channel stop failure.
>>
>> This again...  copying in Peter for the sequencing discussion.  If we
>> need to do this I'm not convinced that bodging it in the driver is a
>> good idea, and especially not deferring it outside of the trigger
>> operation - for example on a suspend or pause we won't actually do a
>> shutdown() so the trigger will end up not happening which seems like it
>> may cause problems.
> 
> It will certainly leave the i2s running and can lead to hard to explain
> issues

Yes I do agree. Moving code from trigger callback to some other place is
not a good idea.
> 
>> We'd probably be better off with the core knowing
>> what's going on and being able to reorder the callbacks although
>> designing an interface for that seems a bit annoying.
> 
> I agree, it would be better to have some sort of flag which tells the
> core that there is an integration issue between the DMA and peripheral.
> I believe this is only affecting playback?

No its affecting both playback and capture use cases.
> 
>>> This issue can't be fixed in ACP DMA driver due to design constraint.
>>
>> What is the design constraint here - can't we fix the design?  Or is it
>> a hardware design constraint (presumably broken signalling between the
>> I2S and DMA blocks)?

Its a hardware design constraint.


I2S driver is not directly exposing DMA interface to host.
ACP 2.x has unique design where ACP driver controls data flow between 
host and I2S as mentioned above.
ACP IP has different IP blocks within it which includes I2S controller 
and DMA controller.

ACP DMA Driver is responsible for DMA transactions between system memory 
and I2S controller.It uses two step DMA mechanism to copy data between 
system memory <-> ACP SRAM and ACP SRAM <-> I2S FIFO for 
playback/capture use cases.
ACP driver program two DMA channels for DMA transfers between System 
memory & I2S FIFO.

ACP DMA driver isn't general purpose DMA controller driver where we can 
implement terminate_all() API.

I2S controller DMA transactions are tightly coupled with ACP DMA controller.
while DMA transfer ongoing between ACP SRAM and I2S FIFO, Stopping I2S 
DMA prior to ACP DMA stop resulting DMA Channel stop failure.
Its not related to I2S FIFO flushing related handling.
Once the DMA channel failure observed during the closure of the stream,
when again new stream opened, DMA won't progress at all.

Need find a right place to implement a work around only for AMD 
stoneyridge platform.



> 
>  From the description my guess is that stop on the DMA want to flush it's
> FIFO (complete the in progress packet, segment). Since the peripheral is
> stopped it will not pull in more data -> the DMA will time out internally.
> 
> The question: how the ACP DMA driver's terminate_all is implemented? It
> can not really wait for the DMA to stop, we can not use
> terminate_all_sync() in trigger, it must just set a stop bit and make
> sure at synchronize() time that it has stopped, right?
> 
> What happens if the time between the DMA stop and the DAI stop is less
> then it would take to flush the DMA FIFO? You would have the same issue,
> but in a rather hard to reproducible way?
> 
> As sidenote: TI's k3-udma initially had similar issue at the design
> phase on the playback side which got solved by a flush bit on the
> channel to detach it from the peripheral and set it to free run to drain
> w/o peripheral.
> On capture however I need to push a dummy 'drain' packet to flush out
> the data from the DMA (if the stop happens when we did not have active
> descriptor on the channel).
> 
> With a flag to reorder the DMA/DAI stop sequence it might work most of
> the time, but imho it is going to introduce a nasty time-bomb of failure.
> Also your DAI will underflow instead of the DMA error.
> 

Re: [PATCH 1/2] ASoC: dwc: add a quirk DW_I2S_QUIRK_STOP_ON_SHUTDOWN to dwc driver

[Mukunda,Vijendar]

On 4/26/21 11:49 AM, Péter Ujfalusi wrote:
> 
> 
> On 4/23/21 7:46 PM, Mark Brown wrote:
>> On Fri, Apr 23, 2021 at 09:54:38PM +0530, Vijendar Mukunda wrote:
>>
>>> For CZ/StoneyRidge platforms, ACP DMA between ACP SRAM and
>>> I2S FIFO should be stopped before stopping I2S Controller DMA.
>>
>>> When DMA is progressing and stop request received, while DMA transfer
>>> ongoing between ACP SRAM and I2S FIFO, Stopping I2S DMA prior to ACP DMA
>>> stop resulting DMA Channel stop failure.
>>
>> This again...  copying in Peter for the sequencing discussion.  If we
>> need to do this I'm not convinced that bodging it in the driver is a
>> good idea, and especially not deferring it outside of the trigger
>> operation - for example on a suspend or pause we won't actually do a
>> shutdown() so the trigger will end up not happening which seems like it
>> may cause problems.
> 
> It will certainly leave the i2s running and can lead to hard to explain
> issues

Yes I do agree. Moving code from trigger callback to some other place is
not a good idea.
> 
>> We'd probably be better off with the core knowing
>> what's going on and being able to reorder the callbacks although
>> designing an interface for that seems a bit annoying.
> 
> I agree, it would be better to have some sort of flag which tells the
> core that there is an integration issue between the DMA and peripheral.
> I believe this is only affecting playback?

No its affecting both playback and capture use cases.
> 
>>> This issue can't be fixed in ACP DMA driver due to design constraint.
>>
>> What is the design constraint here - can't we fix the design?  Or is it
>> a hardware design constraint (presumably broken signalling between the
>> I2S and DMA blocks)?

Its a hardware design constraint.


I2S driver is not directly exposing DMA interface to host.
ACP 2.x has unique design where ACP driver controls data flow between 
host and I2S as mentioned above.
ACP IP has different IP blocks within it which includes I2S controller 
and DMA controller.

ACP DMA Driver is responsible for DMA transactions between system memory 
and I2S controller.It uses two step DMA mechanism to copy data between 
system memory <-> ACP SRAM and ACP SRAM <-> I2S FIFO for 
playback/capture use cases.
ACP driver program two DMA channels for DMA transfers between System 
memory & I2S FIFO.

ACP DMA driver isn't general purpose DMA controller driver where we can 
implement terminate_all() API.

I2S controller DMA transactions are tightly coupled with ACP DMA controller.
while DMA transfer ongoing between ACP SRAM and I2S FIFO, Stopping I2S 
DMA prior to ACP DMA stop resulting DMA Channel stop failure.
Its not related to I2S FIFO flushing related handling.
Once the DMA channel failure observed during the closure of the stream,
when again new stream opened, DMA won't progress at all.

Need find a right place to implement a work around only for AMD 
stoneyridge platform.



> 
>  From the description my guess is that stop on the DMA want to flush it's
> FIFO (complete the in progress packet, segment). Since the peripheral is
> stopped it will not pull in more data -> the DMA will time out internally.
> 
> The question: how the ACP DMA driver's terminate_all is implemented? It
> can not really wait for the DMA to stop, we can not use
> terminate_all_sync() in trigger, it must just set a stop bit and make
> sure at synchronize() time that it has stopped, right?
> 
> What happens if the time between the DMA stop and the DAI stop is less
> then it would take to flush the DMA FIFO? You would have the same issue,
> but in a rather hard to reproducible way?
> 
> As sidenote: TI's k3-udma initially had similar issue at the design
> phase on the playback side which got solved by a flush bit on the
> channel to detach it from the peripheral and set it to free run to drain
> w/o peripheral.
> On capture however I need to push a dummy 'drain' packet to flush out
> the data from the DMA (if the stop happens when we did not have active
> descriptor on the channel).
> 
> With a flag to reorder the DMA/DAI stop sequence it might work most of
> the time, but imho it is going to introduce a nasty time-bomb of failure.
> Also your DAI will underflow instead of the DMA error.
> 

Re: [PATCH 1/2] ASoC: dwc: add a quirk DW_I2S_QUIRK_STOP_ON_SHUTDOWN to dwc driver

[Péter Ujfalusi]

On 4/26/21 3:21 PM, Mukunda,Vijendar wrote:

>>> What is the design constraint here - can't we fix the design?  Or is it
>>> a hardware design constraint (presumably broken signalling between the
>>> I2S and DMA blocks)?
> 
> Its a hardware design constraint.
> 
> 
> I2S driver is not directly exposing DMA interface to host.
> ACP 2.x has unique design where ACP driver controls data flow between
> host and I2S as mentioned above.
> ACP IP has different IP blocks within it which includes I2S controller
> and DMA controller.
> 
> ACP DMA Driver is responsible for DMA transactions between system memory
> and I2S controller.It uses two step DMA mechanism to copy data between
> system memory <-> ACP SRAM and ACP SRAM <-> I2S FIFO for
> playback/capture use cases.
> ACP driver program two DMA channels for DMA transfers between System
> memory & I2S FIFO.
> 
> ACP DMA driver isn't general purpose DMA controller driver where we can
> implement terminate_all() API.
> 
> I2S controller DMA transactions are tightly coupled with ACP DMA
> controller.
> while DMA transfer ongoing between ACP SRAM and I2S FIFO, Stopping I2S
> DMA prior to ACP DMA stop resulting DMA Channel stop failure.
> Its not related to I2S FIFO flushing related handling.
> Once the DMA channel failure observed during the closure of the stream,
> when again new stream opened, DMA won't progress at all.

Thanks for the explanation.
This is not upstream, right?

What is still not clear to me is which channel fails?
A) the DMA between ACP FIFO and the I2S
B) the DMA between ACP FIFO and system memory

in acp-pcm-dma.c on stop you have a busy loop (10000 iterations) to
check if the channel is in fact stopped in response to the cleared run,
IOCEn bits and the set Rst bit.

Channel closer to the destination is stopped first which sounds
reasonable, but on playback you ignore timeout from A, on capture you
ignore the timeout from B.

Still the issue sounds like exactly what I have described. One of the
DMA is failing to drain because the IP is stopped?

> Need find a right place to implement a work around only for AMD
> stoneyridge platform.

Is this really only affecting stoneyridge platform? Are there other
platforms using drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c ?

-- 
Péter

Re: [PATCH 1/2] ASoC: dwc: add a quirk DW_I2S_QUIRK_STOP_ON_SHUTDOWN to dwc driver

[Péter Ujfalusi]

On 4/26/21 3:21 PM, Mukunda,Vijendar wrote:

>>> What is the design constraint here - can't we fix the design?  Or is it
>>> a hardware design constraint (presumably broken signalling between the
>>> I2S and DMA blocks)?
> 
> Its a hardware design constraint.
> 
> 
> I2S driver is not directly exposing DMA interface to host.
> ACP 2.x has unique design where ACP driver controls data flow between
> host and I2S as mentioned above.
> ACP IP has different IP blocks within it which includes I2S controller
> and DMA controller.
> 
> ACP DMA Driver is responsible for DMA transactions between system memory
> and I2S controller.It uses two step DMA mechanism to copy data between
> system memory <-> ACP SRAM and ACP SRAM <-> I2S FIFO for
> playback/capture use cases.
> ACP driver program two DMA channels for DMA transfers between System
> memory & I2S FIFO.
> 
> ACP DMA driver isn't general purpose DMA controller driver where we can
> implement terminate_all() API.
> 
> I2S controller DMA transactions are tightly coupled with ACP DMA
> controller.
> while DMA transfer ongoing between ACP SRAM and I2S FIFO, Stopping I2S
> DMA prior to ACP DMA stop resulting DMA Channel stop failure.
> Its not related to I2S FIFO flushing related handling.
> Once the DMA channel failure observed during the closure of the stream,
> when again new stream opened, DMA won't progress at all.

Thanks for the explanation.
This is not upstream, right?

What is still not clear to me is which channel fails?
A) the DMA between ACP FIFO and the I2S
B) the DMA between ACP FIFO and system memory

in acp-pcm-dma.c on stop you have a busy loop (10000 iterations) to
check if the channel is in fact stopped in response to the cleared run,
IOCEn bits and the set Rst bit.

Channel closer to the destination is stopped first which sounds
reasonable, but on playback you ignore timeout from A, on capture you
ignore the timeout from B.

Still the issue sounds like exactly what I have described. One of the
DMA is failing to drain because the IP is stopped?

> Need find a right place to implement a work around only for AMD
> stoneyridge platform.

Is this really only affecting stoneyridge platform? Are there other
platforms using drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c ?

-- 
Péter

Re: [PATCH 1/2] ASoC: dwc: add a quirk DW_I2S_QUIRK_STOP_ON_SHUTDOWN to dwc driver

[Mukunda,Vijendar]

>> ACP DMA Driver is responsible for DMA transactions between system memory
>> and I2S controller.It uses two step DMA mechanism to copy data between
>> system memory <-> ACP SRAM and ACP SRAM <-> I2S FIFO for
>> playback/capture use cases.
>> ACP driver program two DMA channels for DMA transfers between System
>> memory & I2S FIFO.
>>
>> ACP DMA driver isn't general purpose DMA controller driver where we can
>> implement terminate_all() API.
>>
>> I2S controller DMA transactions are tightly coupled with ACP DMA
>> controller.
>> while DMA transfer ongoing between ACP SRAM and I2S FIFO, Stopping I2S
>> DMA prior to ACP DMA stop resulting DMA Channel stop failure.
>> Its not related to I2S FIFO flushing related handling.
>> Once the DMA channel failure observed during the closure of the stream,
>> when again new stream opened, DMA won't progress at all.
> 
> Thanks for the explanation.
> This is not upstream, right?

Driver is already upstreamed.
Stoneyridge platform based products already into market and working fine 
with 4.14 kernel version.
Currently Kernel migration from v4.14 to v5.10 is in progress for 
Stoneyridge platform and release got blocked due to Audio use cases 
failures.
In v5.10 kernel base, re-ordering of stop trigger sequence is causing 
DMA channel stop failure for both playback & capture use cases.

> 
> What is still not clear to me is which channel fails?
> A) the DMA between ACP FIFO and the I2S
> B) the DMA between ACP FIFO and system memory

There is difference for playback and Capture use cases.

Playback:

channel 1 : DMA transfer from System memory -> ACP memory
channel 2 : DMA transfer from ACP memory -> I2S memory

Capture:

channel 1: DMA transfer from I2S memory to ACP memory
channel 2: DMA transfer from ACP memory to System memory

In case of playback, Channel 2 is failing where as in case of
capture channel 1 is failing.

> 
> in acp-pcm-dma.c on stop you have a busy loop (10000 iterations) to
> check if the channel is in fact stopped in response to the cleared run,
> IOCEn bits and the set Rst bit.

DMA channel run bit is cleared and Ioc bit also cleared for channel 2 in 
case of playback .
After that as part of DMA stop sequence, DMA channel reset is applied.
When DMA channel status is polled for stop, its failed to stop.
> 
> Channel closer to the destination is stopped first which sounds
> reasonable, but on playback you ignore timeout from A, on capture you
> ignore the timeout from B.

Please refer above explanation.

> Still the issue sounds like exactly what I have described. One of the
> DMA is failing to drain because the IP is stopped?

As per our understanding, failing to stop the DMA by hardware is causing 
the issue.

> 
>> Need find a right place to implement a work around only for AMD
>> stoneyridge platform.
> 
> Is this really only affecting stoneyridge platform? Are there other
> platforms using drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c ?
> 
This design is being used only in stoneyridge and Carrizo platforms.
But only stoneyridge platform is productized.
New design is implemented for later generations of APU series.

Re: [PATCH 1/2] ASoC: dwc: add a quirk DW_I2S_QUIRK_STOP_ON_SHUTDOWN to dwc driver

[Mukunda,Vijendar]

>> ACP DMA Driver is responsible for DMA transactions between system memory
>> and I2S controller.It uses two step DMA mechanism to copy data between
>> system memory <-> ACP SRAM and ACP SRAM <-> I2S FIFO for
>> playback/capture use cases.
>> ACP driver program two DMA channels for DMA transfers between System
>> memory & I2S FIFO.
>>
>> ACP DMA driver isn't general purpose DMA controller driver where we can
>> implement terminate_all() API.
>>
>> I2S controller DMA transactions are tightly coupled with ACP DMA
>> controller.
>> while DMA transfer ongoing between ACP SRAM and I2S FIFO, Stopping I2S
>> DMA prior to ACP DMA stop resulting DMA Channel stop failure.
>> Its not related to I2S FIFO flushing related handling.
>> Once the DMA channel failure observed during the closure of the stream,
>> when again new stream opened, DMA won't progress at all.
> 
> Thanks for the explanation.
> This is not upstream, right?

Driver is already upstreamed.
Stoneyridge platform based products already into market and working fine 
with 4.14 kernel version.
Currently Kernel migration from v4.14 to v5.10 is in progress for 
Stoneyridge platform and release got blocked due to Audio use cases 
failures.
In v5.10 kernel base, re-ordering of stop trigger sequence is causing 
DMA channel stop failure for both playback & capture use cases.

> 
> What is still not clear to me is which channel fails?
> A) the DMA between ACP FIFO and the I2S
> B) the DMA between ACP FIFO and system memory

There is difference for playback and Capture use cases.

Playback:

channel 1 : DMA transfer from System memory -> ACP memory
channel 2 : DMA transfer from ACP memory -> I2S memory

Capture:

channel 1: DMA transfer from I2S memory to ACP memory
channel 2: DMA transfer from ACP memory to System memory

In case of playback, Channel 2 is failing where as in case of
capture channel 1 is failing.

> 
> in acp-pcm-dma.c on stop you have a busy loop (10000 iterations) to
> check if the channel is in fact stopped in response to the cleared run,
> IOCEn bits and the set Rst bit.

DMA channel run bit is cleared and Ioc bit also cleared for channel 2 in 
case of playback .
After that as part of DMA stop sequence, DMA channel reset is applied.
When DMA channel status is polled for stop, its failed to stop.
> 
> Channel closer to the destination is stopped first which sounds
> reasonable, but on playback you ignore timeout from A, on capture you
> ignore the timeout from B.

Please refer above explanation.

> Still the issue sounds like exactly what I have described. One of the
> DMA is failing to drain because the IP is stopped?

As per our understanding, failing to stop the DMA by hardware is causing 
the issue.

> 
>> Need find a right place to implement a work around only for AMD
>> stoneyridge platform.
> 
> Is this really only affecting stoneyridge platform? Are there other
> platforms using drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c ?
> 
This design is being used only in stoneyridge and Carrizo platforms.
But only stoneyridge platform is productized.
New design is implemented for later generations of APU series.

Re: [PATCH 1/2] ASoC: dwc: add a quirk DW_I2S_QUIRK_STOP_ON_SHUTDOWN to dwc driver

[Péter Ujfalusi]

On 28.4.2021 18.35, Mukunda,Vijendar wrote:
>> Thanks for the explanation.
>> This is not upstream, right?
> 
> Driver is already upstreamed.
> Stoneyridge platform based products already into market and working fine
> with 4.14 kernel version.
> Currently Kernel migration from v4.14 to v5.10 is in progress for
> Stoneyridge platform and release got blocked due to Audio use cases
> failures.
> In v5.10 kernel base, re-ordering of stop trigger sequence is causing
> DMA channel stop failure for both playback & capture use cases.

The dai - pcm start/stop ordering has changed in v5.5, more than a year
ago. If the support is upstream it should have been noticed by users.

>> What is still not clear to me is which channel fails?
>> A) the DMA between ACP FIFO and the I2S
>> B) the DMA between ACP FIFO and system memory
> 
> There is difference for playback and Capture use cases.
> 
> Playback:
> 
> channel 1 : DMA transfer from System memory -> ACP memory
> channel 2 : DMA transfer from ACP memory -> I2S memory
> 
> Capture:
> 
> channel 1: DMA transfer from I2S memory to ACP memory
> channel 2: DMA transfer from ACP memory to System memory

Yes, this is why I used A and B to refer to the DMA channels.

> In case of playback, Channel 2 is failing where as in case of
> capture channel 1 is failing.

So channel A is failing.

>> in acp-pcm-dma.c on stop you have a busy loop (10000 iterations) to
>> check if the channel is in fact stopped in response to the cleared run,
>> IOCEn bits and the set Rst bit.
> 
> DMA channel run bit is cleared and Ioc bit also cleared for channel 2 in
> case of playback .
> After that as part of DMA stop sequence, DMA channel reset is applied.
> When DMA channel status is polled for stop, its failed to stop.
>>
>> Channel closer to the destination is stopped first which sounds
>> reasonable, but on playback you ignore timeout from A, on capture you
>> ignore the timeout from B.
> 
> Please refer above explanation.
> 
>> Still the issue sounds like exactly what I have described. One of the
>> DMA is failing to drain because the IP is stopped?
> 
> As per our understanding, failing to stop the DMA by hardware is causing
> the issue.
>
>>
>>> Need find a right place to implement a work around only for AMD
>>> stoneyridge platform.
>>
>> Is this really only affecting stoneyridge platform? Are there other
>> platforms using drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c ?
>>
> This design is being used only in stoneyridge and Carrizo platforms.
> But only stoneyridge platform is productized.
> New design is implemented for later generations of APU series.

Right.

This version of ACP is only used with the designware I2S IP?

I would try to find a way to force stop the DMA in case the DAI has been
already stopped.

If this is not possible than the only solution is to do this in core, imho.

For that you would need a flag to say that the platform (DMA) needs the
DAI to be active when stopping it.

If the same ACP have problems with different DAIs, then it is a property
of the platform driver.
If the ACP only have problem against the designware I2S then it is a
link property.
If this ACP only used with the designware I2S then it is again, most
likely the property of the platform driver.

It is surely not a designware IP issue, trying to solve it there is wrong.

-- 
Péter

Re: [PATCH 1/2] ASoC: dwc: add a quirk DW_I2S_QUIRK_STOP_ON_SHUTDOWN to dwc driver

[Péter Ujfalusi]

On 28.4.2021 18.35, Mukunda,Vijendar wrote:
>> Thanks for the explanation.
>> This is not upstream, right?
> 
> Driver is already upstreamed.
> Stoneyridge platform based products already into market and working fine
> with 4.14 kernel version.
> Currently Kernel migration from v4.14 to v5.10 is in progress for
> Stoneyridge platform and release got blocked due to Audio use cases
> failures.
> In v5.10 kernel base, re-ordering of stop trigger sequence is causing
> DMA channel stop failure for both playback & capture use cases.

The dai - pcm start/stop ordering has changed in v5.5, more than a year
ago. If the support is upstream it should have been noticed by users.

>> What is still not clear to me is which channel fails?
>> A) the DMA between ACP FIFO and the I2S
>> B) the DMA between ACP FIFO and system memory
> 
> There is difference for playback and Capture use cases.
> 
> Playback:
> 
> channel 1 : DMA transfer from System memory -> ACP memory
> channel 2 : DMA transfer from ACP memory -> I2S memory
> 
> Capture:
> 
> channel 1: DMA transfer from I2S memory to ACP memory
> channel 2: DMA transfer from ACP memory to System memory

Yes, this is why I used A and B to refer to the DMA channels.

> In case of playback, Channel 2 is failing where as in case of
> capture channel 1 is failing.

So channel A is failing.

>> in acp-pcm-dma.c on stop you have a busy loop (10000 iterations) to
>> check if the channel is in fact stopped in response to the cleared run,
>> IOCEn bits and the set Rst bit.
> 
> DMA channel run bit is cleared and Ioc bit also cleared for channel 2 in
> case of playback .
> After that as part of DMA stop sequence, DMA channel reset is applied.
> When DMA channel status is polled for stop, its failed to stop.
>>
>> Channel closer to the destination is stopped first which sounds
>> reasonable, but on playback you ignore timeout from A, on capture you
>> ignore the timeout from B.
> 
> Please refer above explanation.
> 
>> Still the issue sounds like exactly what I have described. One of the
>> DMA is failing to drain because the IP is stopped?
> 
> As per our understanding, failing to stop the DMA by hardware is causing
> the issue.
>
>>
>>> Need find a right place to implement a work around only for AMD
>>> stoneyridge platform.
>>
>> Is this really only affecting stoneyridge platform? Are there other
>> platforms using drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c ?
>>
> This design is being used only in stoneyridge and Carrizo platforms.
> But only stoneyridge platform is productized.
> New design is implemented for later generations of APU series.

Right.

This version of ACP is only used with the designware I2S IP?

I would try to find a way to force stop the DMA in case the DAI has been
already stopped.

If this is not possible than the only solution is to do this in core, imho.

For that you would need a flag to say that the platform (DMA) needs the
DAI to be active when stopping it.

If the same ACP have problems with different DAIs, then it is a property
of the platform driver.
If the ACP only have problem against the designware I2S then it is a
link property.
If this ACP only used with the designware I2S then it is again, most
likely the property of the platform driver.

It is surely not a designware IP issue, trying to solve it there is wrong.

-- 
Péter

Re: [PATCH 1/2] ASoC: dwc: add a quirk DW_I2S_QUIRK_STOP_ON_SHUTDOWN to dwc driver

[Mukunda,Vijendar]

On 4/30/21 11:12 AM, Péter Ujfalusi wrote:
> 
> 
> On 28.4.2021 18.35, Mukunda,Vijendar wrote:
>>> Thanks for the explanation.
>>> This is not upstream, right?
>>
>> Driver is already upstreamed.
>> Stoneyridge platform based products already into market and working fine
>> with 4.14 kernel version.
>> Currently Kernel migration from v4.14 to v5.10 is in progress for
>> Stoneyridge platform and release got blocked due to Audio use cases
>> failures.
>> In v5.10 kernel base, re-ordering of stop trigger sequence is causing
>> DMA channel stop failure for both playback & capture use cases.
> 
> The dai - pcm start/stop ordering has changed in v5.5, more than a year
> ago. If the support is upstream it should have been noticed by users.

AMD partner using their own repositories where kernel is not migrated to 
5.10. Still products are running with v4.14 kernel only.
That's why users hasn't faced this issue.
> 
>>> What is still not clear to me is which channel fails?
>>> A) the DMA between ACP FIFO and the I2S
>>> B) the DMA between ACP FIFO and system memory
>>
>> There is difference for playback and Capture use cases.
>>
>> Playback:
>>
>> channel 1 : DMA transfer from System memory -> ACP memory
>> channel 2 : DMA transfer from ACP memory -> I2S memory
>>
>> Capture:
>>
>> channel 1: DMA transfer from I2S memory to ACP memory
>> channel 2: DMA transfer from ACP memory to System memory
> 
> Yes, this is why I used A and B to refer to the DMA channels.
> 
>> In case of playback, Channel 2 is failing where as in case of
>> capture channel 1 is failing.
> 
> So channel A is failing.
> 
>>> in acp-pcm-dma.c on stop you have a busy loop (10000 iterations) to
>>> check if the channel is in fact stopped in response to the cleared run,
>>> IOCEn bits and the set Rst bit.
>>
>> DMA channel run bit is cleared and Ioc bit also cleared for channel 2 in
>> case of playback .
>> After that as part of DMA stop sequence, DMA channel reset is applied.
>> When DMA channel status is polled for stop, its failed to stop.
>>>
>>> Channel closer to the destination is stopped first which sounds
>>> reasonable, but on playback you ignore timeout from A, on capture you
>>> ignore the timeout from B.
>>
>> Please refer above explanation.
>>
>>> Still the issue sounds like exactly what I have described. One of the
>>> DMA is failing to drain because the IP is stopped?
>>
>> As per our understanding, failing to stop the DMA by hardware is causing
>> the issue.
>>
>>>
>>>> Need find a right place to implement a work around only for AMD
>>>> stoneyridge platform.
>>>
>>> Is this really only affecting stoneyridge platform? Are there other
>>> platforms using drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c ?
>>>
>> This design is being used only in stoneyridge and Carrizo platforms.
>> But only stoneyridge platform is productized.
>> New design is implemented for later generations of APU series.
> 
> Right.
> 
> This version of ACP is only used with the designware I2S IP?
Yes this version of ACP only uses designware I2S controller.
> 
> I would try to find a way to force stop the DMA in case the DAI has been
> already stopped.
> 
> If this is not possible than the only solution is to do this in core, imho.
> 
> For that you would need a flag to say that the platform (DMA) needs the
> DAI to be active when stopping it.
> 
> If the same ACP have problems with different DAIs, then it is a property
> of the platform driver.
> If the ACP only have problem against the designware I2S then it is a
> link property.
> If this ACP only used with the designware I2S then it is again, most
> likely the property of the platform driver.
>
Hardware signal broken between ACP and Designware I2S controller with 
re-ordering the sequence.
  > It is surely not a designware IP issue, trying to solve it there is 
wrong.
> 
As it's not a designware IP issue, initially we started idea with 
introducing quirk that applies for this ACP version based AMD platforms.

Re: [PATCH 1/2] ASoC: dwc: add a quirk DW_I2S_QUIRK_STOP_ON_SHUTDOWN to dwc driver

[Mukunda,Vijendar]

On 4/30/21 11:12 AM, Péter Ujfalusi wrote:
> 
> 
> On 28.4.2021 18.35, Mukunda,Vijendar wrote:
>>> Thanks for the explanation.
>>> This is not upstream, right?
>>
>> Driver is already upstreamed.
>> Stoneyridge platform based products already into market and working fine
>> with 4.14 kernel version.
>> Currently Kernel migration from v4.14 to v5.10 is in progress for
>> Stoneyridge platform and release got blocked due to Audio use cases
>> failures.
>> In v5.10 kernel base, re-ordering of stop trigger sequence is causing
>> DMA channel stop failure for both playback & capture use cases.
> 
> The dai - pcm start/stop ordering has changed in v5.5, more than a year
> ago. If the support is upstream it should have been noticed by users.

AMD partner using their own repositories where kernel is not migrated to 
5.10. Still products are running with v4.14 kernel only.
That's why users hasn't faced this issue.
> 
>>> What is still not clear to me is which channel fails?
>>> A) the DMA between ACP FIFO and the I2S
>>> B) the DMA between ACP FIFO and system memory
>>
>> There is difference for playback and Capture use cases.
>>
>> Playback:
>>
>> channel 1 : DMA transfer from System memory -> ACP memory
>> channel 2 : DMA transfer from ACP memory -> I2S memory
>>
>> Capture:
>>
>> channel 1: DMA transfer from I2S memory to ACP memory
>> channel 2: DMA transfer from ACP memory to System memory
> 
> Yes, this is why I used A and B to refer to the DMA channels.
> 
>> In case of playback, Channel 2 is failing where as in case of
>> capture channel 1 is failing.
> 
> So channel A is failing.
> 
>>> in acp-pcm-dma.c on stop you have a busy loop (10000 iterations) to
>>> check if the channel is in fact stopped in response to the cleared run,
>>> IOCEn bits and the set Rst bit.
>>
>> DMA channel run bit is cleared and Ioc bit also cleared for channel 2 in
>> case of playback .
>> After that as part of DMA stop sequence, DMA channel reset is applied.
>> When DMA channel status is polled for stop, its failed to stop.
>>>
>>> Channel closer to the destination is stopped first which sounds
>>> reasonable, but on playback you ignore timeout from A, on capture you
>>> ignore the timeout from B.
>>
>> Please refer above explanation.
>>
>>> Still the issue sounds like exactly what I have described. One of the
>>> DMA is failing to drain because the IP is stopped?
>>
>> As per our understanding, failing to stop the DMA by hardware is causing
>> the issue.
>>
>>>
>>>> Need find a right place to implement a work around only for AMD
>>>> stoneyridge platform.
>>>
>>> Is this really only affecting stoneyridge platform? Are there other
>>> platforms using drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c ?
>>>
>> This design is being used only in stoneyridge and Carrizo platforms.
>> But only stoneyridge platform is productized.
>> New design is implemented for later generations of APU series.
> 
> Right.
> 
> This version of ACP is only used with the designware I2S IP?
Yes this version of ACP only uses designware I2S controller.
> 
> I would try to find a way to force stop the DMA in case the DAI has been
> already stopped.
> 
> If this is not possible than the only solution is to do this in core, imho.
> 
> For that you would need a flag to say that the platform (DMA) needs the
> DAI to be active when stopping it.
> 
> If the same ACP have problems with different DAIs, then it is a property
> of the platform driver.
> If the ACP only have problem against the designware I2S then it is a
> link property.
> If this ACP only used with the designware I2S then it is again, most
> likely the property of the platform driver.
>
Hardware signal broken between ACP and Designware I2S controller with 
re-ordering the sequence.
  > It is surely not a designware IP issue, trying to solve it there is 
wrong.
> 
As it's not a designware IP issue, initially we started idea with 
introducing quirk that applies for this ACP version based AMD platforms.

Re: [PATCH 1/2] ASoC: dwc: add a quirk DW_I2S_QUIRK_STOP_ON_SHUTDOWN to dwc driver

[Mukunda,Vijendar]

On 5/1/21 12:05 AM, Mukunda,Vijendar wrote:
> 
> 
> On 4/30/21 11:12 AM, Péter Ujfalusi wrote:
>>
>>
>> On 28.4.2021 18.35, Mukunda,Vijendar wrote:
>>>> Thanks for the explanation.
>>>> This is not upstream, right?
>>>
>>> Driver is already upstreamed.
>>> Stoneyridge platform based products already into market and working fine
>>> with 4.14 kernel version.
>>> Currently Kernel migration from v4.14 to v5.10 is in progress for
>>> Stoneyridge platform and release got blocked due to Audio use cases
>>> failures.
>>> In v5.10 kernel base, re-ordering of stop trigger sequence is causing
>>> DMA channel stop failure for both playback & capture use cases.
>>
>> The dai - pcm start/stop ordering has changed in v5.5, more than a year
>> ago. If the support is upstream it should have been noticed by users.
> 
> AMD partner using their own repositories where kernel is not migrated to 
> 5.10. Still products are running with v4.14 kernel only.
> That's why users hasn't faced this issue.
>>
>>>> What is still not clear to me is which channel fails?
>>>> A) the DMA between ACP FIFO and the I2S
>>>> B) the DMA between ACP FIFO and system memory
>>>
>>> There is difference for playback and Capture use cases.
>>>
>>> Playback:
>>>
>>> channel 1 : DMA transfer from System memory -> ACP memory
>>> channel 2 : DMA transfer from ACP memory -> I2S memory
>>>
>>> Capture:
>>>
>>> channel 1: DMA transfer from I2S memory to ACP memory
>>> channel 2: DMA transfer from ACP memory to System memory
>>
>> Yes, this is why I used A and B to refer to the DMA channels.
>>
>>> In case of playback, Channel 2 is failing where as in case of
>>> capture channel 1 is failing.
>>
>> So channel A is failing.
>>
>>>> in acp-pcm-dma.c on stop you have a busy loop (10000 iterations) to
>>>> check if the channel is in fact stopped in response to the cleared run,
>>>> IOCEn bits and the set Rst bit.
>>>
>>> DMA channel run bit is cleared and Ioc bit also cleared for channel 2 in
>>> case of playback .
>>> After that as part of DMA stop sequence, DMA channel reset is applied.
>>> When DMA channel status is polled for stop, its failed to stop.
>>>>
>>>> Channel closer to the destination is stopped first which sounds
>>>> reasonable, but on playback you ignore timeout from A, on capture you
>>>> ignore the timeout from B.
>>>
>>> Please refer above explanation.
>>>
>>>> Still the issue sounds like exactly what I have described. One of the
>>>> DMA is failing to drain because the IP is stopped?
>>>
>>> As per our understanding, failing to stop the DMA by hardware is causing
>>> the issue.
>>>
>>>>
>>>>> Need find a right place to implement a work around only for AMD
>>>>> stoneyridge platform.
>>>>
>>>> Is this really only affecting stoneyridge platform? Are there other
>>>> platforms using drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c ?
>>>>
>>> This design is being used only in stoneyridge and Carrizo platforms.
>>> But only stoneyridge platform is productized.
>>> New design is implemented for later generations of APU series.
>>
>> Right.
>>
>> This version of ACP is only used with the designware I2S IP?
> Yes this version of ACP only uses designware I2S controller.
>>
>> I would try to find a way to force stop the DMA in case the DAI has been
>> already stopped.
>>
>> If this is not possible than the only solution is to do this in core, 
>> imho.
>>
>> For that you would need a flag to say that the platform (DMA) needs the
>> DAI to be active when stopping it.
>>
>> If the same ACP have problems with different DAIs, then it is a property
>> of the platform driver.
>> If the ACP only have problem against the designware I2S then it is a
>> link property.
>> If this ACP only used with the designware I2S then it is again, most
>> likely the property of the platform driver.
>>
> Hardware signal broken between ACP and Designware I2S controller with 
> re-ordering the sequence.
>   > It is surely not a designware IP issue, trying to solve it there is 
> wrong.
>>
> As it's not a designware IP issue, initially we started idea with 
> introducing quirk that applies for this ACP version based AMD platforms.
> 

Hi Peter,

Any suggestion on the work around for this issue?
How about declaring a flag in sound card structure and this flag will be 
set in stoneyridge machine driver.

Based on flag check trigger stop sequence will be re-ordered.

Thanks,
Vijendar

Re: [PATCH 1/2] ASoC: dwc: add a quirk DW_I2S_QUIRK_STOP_ON_SHUTDOWN to dwc driver

[Mukunda,Vijendar]

On 5/1/21 12:05 AM, Mukunda,Vijendar wrote:
> 
> 
> On 4/30/21 11:12 AM, Péter Ujfalusi wrote:
>>
>>
>> On 28.4.2021 18.35, Mukunda,Vijendar wrote:
>>>> Thanks for the explanation.
>>>> This is not upstream, right?
>>>
>>> Driver is already upstreamed.
>>> Stoneyridge platform based products already into market and working fine
>>> with 4.14 kernel version.
>>> Currently Kernel migration from v4.14 to v5.10 is in progress for
>>> Stoneyridge platform and release got blocked due to Audio use cases
>>> failures.
>>> In v5.10 kernel base, re-ordering of stop trigger sequence is causing
>>> DMA channel stop failure for both playback & capture use cases.
>>
>> The dai - pcm start/stop ordering has changed in v5.5, more than a year
>> ago. If the support is upstream it should have been noticed by users.
> 
> AMD partner using their own repositories where kernel is not migrated to 
> 5.10. Still products are running with v4.14 kernel only.
> That's why users hasn't faced this issue.
>>
>>>> What is still not clear to me is which channel fails?
>>>> A) the DMA between ACP FIFO and the I2S
>>>> B) the DMA between ACP FIFO and system memory
>>>
>>> There is difference for playback and Capture use cases.
>>>
>>> Playback:
>>>
>>> channel 1 : DMA transfer from System memory -> ACP memory
>>> channel 2 : DMA transfer from ACP memory -> I2S memory
>>>
>>> Capture:
>>>
>>> channel 1: DMA transfer from I2S memory to ACP memory
>>> channel 2: DMA transfer from ACP memory to System memory
>>
>> Yes, this is why I used A and B to refer to the DMA channels.
>>
>>> In case of playback, Channel 2 is failing where as in case of
>>> capture channel 1 is failing.
>>
>> So channel A is failing.
>>
>>>> in acp-pcm-dma.c on stop you have a busy loop (10000 iterations) to
>>>> check if the channel is in fact stopped in response to the cleared run,
>>>> IOCEn bits and the set Rst bit.
>>>
>>> DMA channel run bit is cleared and Ioc bit also cleared for channel 2 in
>>> case of playback .
>>> After that as part of DMA stop sequence, DMA channel reset is applied.
>>> When DMA channel status is polled for stop, its failed to stop.
>>>>
>>>> Channel closer to the destination is stopped first which sounds
>>>> reasonable, but on playback you ignore timeout from A, on capture you
>>>> ignore the timeout from B.
>>>
>>> Please refer above explanation.
>>>
>>>> Still the issue sounds like exactly what I have described. One of the
>>>> DMA is failing to drain because the IP is stopped?
>>>
>>> As per our understanding, failing to stop the DMA by hardware is causing
>>> the issue.
>>>
>>>>
>>>>> Need find a right place to implement a work around only for AMD
>>>>> stoneyridge platform.
>>>>
>>>> Is this really only affecting stoneyridge platform? Are there other
>>>> platforms using drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c ?
>>>>
>>> This design is being used only in stoneyridge and Carrizo platforms.
>>> But only stoneyridge platform is productized.
>>> New design is implemented for later generations of APU series.
>>
>> Right.
>>
>> This version of ACP is only used with the designware I2S IP?
> Yes this version of ACP only uses designware I2S controller.
>>
>> I would try to find a way to force stop the DMA in case the DAI has been
>> already stopped.
>>
>> If this is not possible than the only solution is to do this in core, 
>> imho.
>>
>> For that you would need a flag to say that the platform (DMA) needs the
>> DAI to be active when stopping it.
>>
>> If the same ACP have problems with different DAIs, then it is a property
>> of the platform driver.
>> If the ACP only have problem against the designware I2S then it is a
>> link property.
>> If this ACP only used with the designware I2S then it is again, most
>> likely the property of the platform driver.
>>
> Hardware signal broken between ACP and Designware I2S controller with 
> re-ordering the sequence.
>   > It is surely not a designware IP issue, trying to solve it there is 
> wrong.
>>
> As it's not a designware IP issue, initially we started idea with 
> introducing quirk that applies for this ACP version based AMD platforms.
> 

Hi Peter,

Any suggestion on the work around for this issue?
How about declaring a flag in sound card structure and this flag will be 
set in stoneyridge machine driver.

Based on flag check trigger stop sequence will be re-ordered.

Thanks,
Vijendar

Re: [PATCH 1/2] ASoC: dwc: add a quirk DW_I2S_QUIRK_STOP_ON_SHUTDOWN to dwc driver

[Mark Brown]

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

On Mon, May 10, 2021 at 10:57:25PM +0530, Mukunda,Vijendar wrote:

> How about declaring a flag in sound card structure and this flag will be set
> in stoneyridge machine driver.

> Based on flag check trigger stop sequence will be re-ordered.

A couple of people suggested that already, making sure the core knows
what's going on is probably the best way forwards here.

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

Re: [PATCH 1/2] ASoC: dwc: add a quirk DW_I2S_QUIRK_STOP_ON_SHUTDOWN to dwc driver

[Mark Brown]

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

On Mon, May 10, 2021 at 10:57:25PM +0530, Mukunda,Vijendar wrote:

> How about declaring a flag in sound card structure and this flag will be set
> in stoneyridge machine driver.

> Based on flag check trigger stop sequence will be re-ordered.

A couple of people suggested that already, making sure the core knows
what's going on is probably the best way forwards here.

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

Re: [PATCH 1/2] ASoC: dwc: add a quirk DW_I2S_QUIRK_STOP_ON_SHUTDOWN to dwc driver

[Péter Ujfalusi]

On 10/05/2021 20:27, Mukunda,Vijendar wrote:
>> Hardware signal broken between ACP and Designware I2S controller with
>> re-ordering the sequence.
>>   > It is surely not a designware IP issue, trying to solve it there
>> is wrong.
>>>
>> As it's not a designware IP issue, initially we started idea with
>> introducing quirk that applies for this ACP version based AMD platforms.
>>
> 
> Hi Peter,
> 
> Any suggestion on the work around for this issue?
> How about declaring a flag in sound card structure and this flag will be
> set in stoneyridge machine driver.

This can only be solved in the core, that's clear.
If this issue only affects this version of ACP with dw I2S (the same ACP
version works fine with other audio IP), then it is more like machine
driver level of quirk.
If this ACP have the same issue with other audio IPs as well then it is
platform quirk.
If the this ACP is only used in this setup then I would consider machine
level quirk as it might be simpler to implement.

Other thing to consider is how other setups with similar issues can use
the new quirk/flag... Some might need to make sure that a component is
first, not last for example.

> Based on flag check trigger stop sequence will be re-ordered.
> 
> Thanks,
> Vijendar

-- 
Péter

Re: [PATCH 1/2] ASoC: dwc: add a quirk DW_I2S_QUIRK_STOP_ON_SHUTDOWN to dwc driver

[Péter Ujfalusi]

On 10/05/2021 20:27, Mukunda,Vijendar wrote:
>> Hardware signal broken between ACP and Designware I2S controller with
>> re-ordering the sequence.
>>   > It is surely not a designware IP issue, trying to solve it there
>> is wrong.
>>>
>> As it's not a designware IP issue, initially we started idea with
>> introducing quirk that applies for this ACP version based AMD platforms.
>>
> 
> Hi Peter,
> 
> Any suggestion on the work around for this issue?
> How about declaring a flag in sound card structure and this flag will be
> set in stoneyridge machine driver.

This can only be solved in the core, that's clear.
If this issue only affects this version of ACP with dw I2S (the same ACP
version works fine with other audio IP), then it is more like machine
driver level of quirk.
If this ACP have the same issue with other audio IPs as well then it is
platform quirk.
If the this ACP is only used in this setup then I would consider machine
level quirk as it might be simpler to implement.

Other thing to consider is how other setups with similar issues can use
the new quirk/flag... Some might need to make sure that a component is
first, not last for example.

> Based on flag check trigger stop sequence will be re-ordered.
> 
> Thanks,
> Vijendar

-- 
Péter