diff mbox

[4/8] ASoC: AMD: added condition checks for CZ specific code

Message ID 1498235706-31111-5-git-send-email-alexander.deucher@amd.com (mailing list archive)
State New, archived
Headers show

Commit Message

Alex Deucher June 23, 2017, 4:35 p.m. UTC 
From: Vijendar Mukunda <Vijendar.Mukunda@amd.com>

Added condition checks for CZ specific code based on asic_type.
Stoney specific code will be added in a future commit.

Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
---
 sound/soc/amd/acp-pcm-dma.c | 62 ++++++++++++++++++++++++++++-----------------
 1 file changed, 39 insertions(+), 23 deletions(-)

Comments

Mark Brown June 28, 2017, 6:05 p.m. UTC | #1 
On Fri, Jun 23, 2017 at 12:35:02PM -0400, Alex Deucher wrote:

> Reviewed-by: Alex Deucher <alexander.deucher@amd.com>


> index dcbf997..e48ae5d 100644
> --- a/sound/soc/amd/acp-pcm-dma.c
> +++ b/sound/soc/amd/acp-pcm-dma.c
> @@ -34,6 +34,8 @@
>  
>  #define MAX_BUFFER (PLAYBACK_MAX_PERIOD_SIZE * PLAYBACK_MAX_NUM_PERIODS)
>  #define MIN_BUFFER MAX_BUFFER
> +#define CHIP_STONEY 14
> +#define CHIP_CARRIZO 13

These defines are being added in the middle of a file but CHIP_STONEY is
also used in another file in the previous patch (and apparently
extensively throughout the DRM driver already).  This is obviously not
good, we shouldn't have multiple copies of the definition.

> -	} else {
> +		if (adata->asic_type == CHIP_CARRIZO) {
> +			for (bank = 1; bank <= 4; bank++)
> +				acp_set_sram_bank_state(adata->acp_mmio, bank,
> +				false);

I'm not seeing any poweroff cases for other chips being added, and again
switch statements please.
Vijendar Mukunda June 29, 2017, 12:58 p.m. UTC | #2 
-----Original Message-----
From: Mark Brown [mailto:broonie@kernel.org] 
Sent: Wednesday, June 28, 2017 11:36 PM
To: Alex Deucher
Cc: dri-devel@lists.freedesktop.org; amd-gfx@lists.freedesktop.org; alsa-devel@alsa-project.org; airlied@gmail.com; Mukunda, Vijendar; rajeevkumar.linux@gmail.com; lgirdwood@gmail.com; tiwai@suse.de; perex@perex.cz; Deucher, Alexander
Subject: Re: [PATCH 4/8] ASoC: AMD: added condition checks for CZ specific code

On Fri, Jun 23, 2017 at 12:35:02PM -0400, Alex Deucher wrote:

> Reviewed-by: Alex Deucher <alexander.deucher@amd.com>


> index dcbf997..e48ae5d 100644
> --- a/sound/soc/amd/acp-pcm-dma.c
> +++ b/sound/soc/amd/acp-pcm-dma.c
> @@ -34,6 +34,8 @@
>  
>  #define MAX_BUFFER (PLAYBACK_MAX_PERIOD_SIZE * 
> PLAYBACK_MAX_NUM_PERIODS)  #define MIN_BUFFER MAX_BUFFER
> +#define CHIP_STONEY 14
> +#define CHIP_CARRIZO 13

>These defines are being added in the middle of a file but CHIP_STONEY is also used in another file in the previous patch (and apparently extensively throughout the DRM driver already).  This is obviously not good, >we shouldn't have multiple copies of the definition.

> -	} else {
> +		if (adata->asic_type == CHIP_CARRIZO) {
> +			for (bank = 1; bank <= 4; bank++)
> +				acp_set_sram_bank_state(adata->acp_mmio, bank,
> +				false);

> I'm not seeing any poweroff cases for other chips being added, and again switch statements please.

We will modify code to use single definition for CHIP_STONEY and CHIP_CARRIZO.
There are only two chip sets based on ACP 2.x design(Carrizo and Stoney).
Our future Chip sets going to use different design based on next ACP IP version.

In the current patch, Condition checks added for Carrizo for setting SRAM BANK state.
Memory Gating is disabled in Stoney,i.e SRAM Bank's won't be turned off. The default state for SRAM banks is ON.
As Memory Gating is disabled, there is no need to add condition checks for Stoney to set SRAM Bank state.

Apart from Memory Gating, there are  few more differences between Stoney and Carrizo chip sets.
Stoney specific DMA driver changes submitted in a separate patch.

Vijendar
Mark Brown June 30, 2017, 10:16 a.m. UTC | #3 
On Thu, Jun 29, 2017 at 12:58:02PM +0000, Mukunda, Vijendar wrote:

> -----Original Message-----
> From: Mark Brown [mailto:broonie@kernel.org] 
> Sent: Wednesday, June 28, 2017 11:36 PM
> To: Alex Deucher

Please fix your mail client to quote mails in a more normal fashion,
this looks pretty broken...

> >These defines are being added in the middle of a file but CHIP_STONEY is also used in another file in the previous patch (and apparently extensively throughout the DRM driver already).  This is obviously not good, >we shouldn't have multiple copies of the definition.

...especially in that it's reflowing the message it's replying to to
cause 80 column problems and has serious problems in that regard itself.

> We will modify code to use single definition for CHIP_STONEY and CHIP_CARRIZO.
> There are only two chip sets based on ACP 2.x design(Carrizo and Stoney).
> Our future Chip sets going to use different design based on next ACP IP version.

Write the code well, that way we don't have bad patterns in the codebase
and if plans change with regard to new variants you're covered.

> In the current patch, Condition checks added for Carrizo for setting SRAM BANK state.
> Memory Gating is disabled in Stoney,i.e SRAM Bank's won't be turned off. The default state for SRAM banks is ON.
> As Memory Gating is disabled, there is no need to add condition checks for Stoney to set SRAM Bank state.

Some documentation of this in the code would be good.
diff mbox

Patch

diff --git a/sound/soc/amd/acp-pcm-dma.c b/sound/soc/amd/acp-pcm-dma.c
index dcbf997..e48ae5d 100644
--- a/sound/soc/amd/acp-pcm-dma.c
+++ b/sound/soc/amd/acp-pcm-dma.c
@@ -34,6 +34,8 @@ 
 
 #define MAX_BUFFER (PLAYBACK_MAX_PERIOD_SIZE * PLAYBACK_MAX_NUM_PERIODS)
 #define MIN_BUFFER MAX_BUFFER
+#define CHIP_STONEY 14
+#define CHIP_CARRIZO 13
 
 static const struct snd_pcm_hardware acp_pcm_hardware_playback = {
 	.info = SNDRV_PCM_INFO_INTERLEAVED |
@@ -419,7 +421,7 @@  static void acp_set_sram_bank_state(void __iomem *acp_mmio, u16 bank,
 }
 
 /* Initialize and bring ACP hardware to default state. */
-static int acp_init(void __iomem *acp_mmio)
+static int acp_init(void __iomem *acp_mmio, u32 asic_type)
 {
 	u16 bank;
 	u32 val, count, sram_pte_offset;
@@ -494,8 +496,10 @@  static int acp_init(void __iomem *acp_mmio)
 	* Now, turn off all of them. This can't be done in 'poweron' of
 	* ACP pm domain, as this requires ACP to be initialized.
 	*/
-	for (bank = 1; bank < 48; bank++)
-		acp_set_sram_bank_state(acp_mmio, bank, false);
+	if (asic_type == CHIP_CARRIZO) {
+		for (bank = 1; bank < 48; bank++)
+			acp_set_sram_bank_state(acp_mmio, bank, false);
+	}
 
 	return 0;
 }
@@ -646,14 +650,18 @@  static int acp_dma_open(struct snd_pcm_substream *substream)
 
 	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
 		intr_data->play_stream = substream;
-		for (bank = 1; bank <= 4; bank++)
-			acp_set_sram_bank_state(intr_data->acp_mmio, bank,
-						true);
+		if (intr_data->asic_type == CHIP_CARRIZO) {
+			for (bank = 1; bank <= 4; bank++)
+				acp_set_sram_bank_state(intr_data->acp_mmio,
+							bank, true);
+		}
 	} else {
 		intr_data->capture_stream = substream;
-		for (bank = 5; bank <= 8; bank++)
-			acp_set_sram_bank_state(intr_data->acp_mmio, bank,
-						true);
+		if (intr_data->asic_type == CHIP_CARRIZO) {
+			for (bank = 5; bank <= 8; bank++)
+				acp_set_sram_bank_state(intr_data->acp_mmio,
+							bank, true);
+		}
 	}
 
 	return 0;
@@ -869,14 +877,18 @@  static int acp_dma_close(struct snd_pcm_substream *substream)
 
 	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
 		adata->play_stream = NULL;
-		for (bank = 1; bank <= 4; bank++)
-			acp_set_sram_bank_state(adata->acp_mmio, bank,
-						false);
-	} else {
+		if (adata->asic_type == CHIP_CARRIZO) {
+			for (bank = 1; bank <= 4; bank++)
+				acp_set_sram_bank_state(adata->acp_mmio, bank,
+				false);
+		}
+	} else  {
 		adata->capture_stream = NULL;
-		for (bank = 5; bank <= 8; bank++)
-			acp_set_sram_bank_state(adata->acp_mmio, bank,
-						false);
+		if (adata->asic_type == CHIP_CARRIZO) {
+			for (bank = 5; bank <= 8; bank++)
+				acp_set_sram_bank_state(adata->acp_mmio, bank,
+						     false);
+		}
 	}
 
 	/* Disable ACP irq, when the current stream is being closed and
@@ -945,7 +957,7 @@  static int acp_audio_probe(struct platform_device *pdev)
 	dev_set_drvdata(&pdev->dev, audio_drv_data);
 
 	/* Initialize the ACP */
-	acp_init(audio_drv_data->acp_mmio);
+	acp_init(audio_drv_data->acp_mmio, audio_drv_data->asic_type);
 
 	status = snd_soc_register_platform(&pdev->dev, &acp_asoc_platform);
 	if (status != 0) {
@@ -976,19 +988,23 @@  static int acp_pcm_resume(struct device *dev)
 	u16 bank;
 	struct audio_drv_data *adata = dev_get_drvdata(dev);
 
-	acp_init(adata->acp_mmio);
+	acp_init(adata->acp_mmio, adata->asic_type);
 
 	if (adata->play_stream && adata->play_stream->runtime) {
-		for (bank = 1; bank <= 4; bank++)
-			acp_set_sram_bank_state(adata->acp_mmio, bank,
+		if (adata->asic_type == CHIP_CARRIZO) {
+			for (bank = 1; bank <= 4; bank++)
+				acp_set_sram_bank_state(adata->acp_mmio, bank,
 						true);
+		}
 		config_acp_dma(adata->acp_mmio,
 				adata->play_stream->runtime->private_data);
 	}
 	if (adata->capture_stream && adata->capture_stream->runtime) {
-		for (bank = 5; bank <= 8; bank++)
-			acp_set_sram_bank_state(adata->acp_mmio, bank,
+		if (adata->asic_type == CHIP_CARRIZO) {
+			for (bank = 5; bank <= 8; bank++)
+				acp_set_sram_bank_state(adata->acp_mmio, bank,
 						true);
+		}
 		config_acp_dma(adata->acp_mmio,
 				adata->capture_stream->runtime->private_data);
 	}
@@ -1009,7 +1025,7 @@  static int acp_pcm_runtime_resume(struct device *dev)
 {
 	struct audio_drv_data *adata = dev_get_drvdata(dev);
 
-	acp_init(adata->acp_mmio);
+	acp_init(adata->acp_mmio, adata->asic_type);
 	acp_reg_write(1, adata->acp_mmio, mmACP_EXTERNAL_INTR_ENB);
 	return 0;
 }