diff mbox

[2/5] ASoC : dwc : support dw i2s in AMD platform

Message ID 1443217704-28814-3-git-send-email-alexander.deucher@amd.com (mailing list archive)
State New, archived
Headers show

Commit Message

Alex Deucher Sept. 25, 2015, 9:48 p.m. UTC 
From: Maruthi Srinivas Bayyavarapu <Maruthi.Bayyavarapu@amd.com>

Vendor specific quirk was added to:
1. Support AMD platform which has two dwc controllers with different
   base address for playback and capture. Also, I2S_COMP_PARAM_*
   registers offsets differed.
2. Resume audio which was active before system suspend.
   After 'resume', dwc need to be reconfigured with params
   configured during 'hw_params' callback. With this, audio usecase
   continues from where it got stopped because of 'suspend'.

Signed-off-by: Maruthi Bayyavarapu <maruthi.bayyavarapu@amd.com>
Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
---

v2: Copy quirks from platform data to local struct since platform data
may not be available in some use cases.

 include/sound/designware_i2s.h |   3 +
 sound/soc/dwc/designware_i2s.c | 190 ++++++++++++++++++++++++++---------------
 2 files changed, 126 insertions(+), 67 deletions(-)

Comments

Mark Brown Oct. 5, 2015, 3:41 p.m. UTC | #1 
On Fri, Sep 25, 2015 at 05:48:23PM -0400, Alex Deucher wrote:
> From: Maruthi Srinivas Bayyavarapu <Maruthi.Bayyavarapu@amd.com>
> 
> Vendor specific quirk was added to:
> 1. Support AMD platform which has two dwc controllers with different
>    base address for playback and capture. Also, I2S_COMP_PARAM_*
>    registers offsets differed.

> 2. Resume audio which was active before system suspend.
>    After 'resume', dwc need to be reconfigured with params
>    configured during 'hw_params' callback. With this, audio usecase
>    continues from where it got stopped because of 'suspend'.

This is hard to review since there are several different changes in
here, one change per commit is muuch more helpful.  As is I'm not 100%
sure exactly what each bit of the change is intended to do.  One example
here is that a separate patch splitting pbase and cbase would be really
helpful.

> --- a/include/sound/designware_i2s.h
> +++ b/include/sound/designware_i2s.h
> @@ -44,6 +44,9 @@ struct i2s_platform_data {
>  	int channel;
>  	u32 snd_fmts;
>  	u32 snd_rates;
> +	#define DW_I2S_VENDOR_AMD (1 << 0)
> +	unsigned int quirks;

I would expect to see a set of quirks with the AMD devices selecting
those that apply to them rather than just a per vendor define - this
is more generic and more maintainable long term.  AMD may require
different sets of quirks with some future device and systems from other
vendors may require some but not all of the quirks that AMD requires.

> +	if (dev->quirks & DW_I2S_VENDOR_AMD) {
> +		comp1 = i2s_read_reg(dev->i2s_pbase, 0x124);
> +		comp2 = i2s_read_reg(dev->i2s_pbase, 0x120);

Please add defines for these registers.  Rather than having the quirk
code throughout the driver it might be clearer to have the comp1 and
comp2 register offsets stored in the driver data so you can just do the
actual quirk once at probe time.
maruthi srinivas Oct. 6, 2015, 7:29 p.m. UTC | #2 
On Mon, Oct 5, 2015 at 9:11 PM, Mark Brown <broonie@kernel.org> wrote:
> On Fri, Sep 25, 2015 at 05:48:23PM -0400, Alex Deucher wrote:
>> From: Maruthi Srinivas Bayyavarapu <Maruthi.Bayyavarapu@amd.com>
>>
>> Vendor specific quirk was added to:
>> 1. Support AMD platform which has two dwc controllers with different
>>    base address for playback and capture. Also, I2S_COMP_PARAM_*
>>    registers offsets differed.
>
>> 2. Resume audio which was active before system suspend.
>>    After 'resume', dwc need to be reconfigured with params
>>    configured during 'hw_params' callback. With this, audio usecase
>>    continues from where it got stopped because of 'suspend'.
>
> This is hard to review since there are several different changes in
> here, one change per commit is muuch more helpful.  As is I'm not 100%
> sure exactly what each bit of the change is intended to do.  One example
> here is that a separate patch splitting pbase and cbase would be really
> helpful.

Ok, I will split into multiple patches and send again.

>
>> --- a/include/sound/designware_i2s.h
>> +++ b/include/sound/designware_i2s.h
>> @@ -44,6 +44,9 @@ struct i2s_platform_data {
>>       int channel;
>>       u32 snd_fmts;
>>       u32 snd_rates;
>> +     #define DW_I2S_VENDOR_AMD (1 << 0)
>> +     unsigned int quirks;
>
> I would expect to see a set of quirks with the AMD devices selecting
> those that apply to them rather than just a per vendor define - this
> is more generic and more maintainable long term.  AMD may require
> different sets of quirks with some future device and systems from other
> vendors may require some but not all of the quirks that AMD requires.

Agreed. I will send a revised patch.

>
>> +     if (dev->quirks & DW_I2S_VENDOR_AMD) {
>> +             comp1 = i2s_read_reg(dev->i2s_pbase, 0x124);
>> +             comp2 = i2s_read_reg(dev->i2s_pbase, 0x120);
>
> Please add defines for these registers.  Rather than having the quirk
> code throughout the driver it might be clearer to have the comp1 and
> comp2 register offsets stored in the driver data so you can just do the
> actual quirk once at probe time.

Agreed. I will send a revised patch.

>
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>
diff mbox

Patch

diff --git a/include/sound/designware_i2s.h b/include/sound/designware_i2s.h
index 8966ba7..7e8094a 100644
--- a/include/sound/designware_i2s.h
+++ b/include/sound/designware_i2s.h
@@ -44,6 +44,9 @@  struct i2s_platform_data {
 	int channel;
 	u32 snd_fmts;
 	u32 snd_rates;
+	#define DW_I2S_VENDOR_AMD (1 << 0)
+	unsigned int quirks;
+
 
 	void *play_dma_data;
 	void *capture_dma_data;
diff --git a/sound/soc/dwc/designware_i2s.c b/sound/soc/dwc/designware_i2s.c
index 3a52f82..dce1e4a 100644
--- a/sound/soc/dwc/designware_i2s.c
+++ b/sound/soc/dwc/designware_i2s.c
@@ -89,11 +89,15 @@  union dw_i2s_snd_dma_data {
 };
 
 struct dw_i2s_dev {
-	void __iomem *i2s_base;
+	void __iomem *i2s_pbase;
+	void __iomem *i2s_cbase;
 	struct clk *clk;
 	int active;
 	unsigned int capability;
+	unsigned int quirks;
 	struct device *dev;
+	u32 ccr;
+	u32 xfer_resolution;
 
 	/* data related to DMA transfers b/w i2s and DMAC */
 	union dw_i2s_snd_dma_data play_dma_data;
@@ -118,10 +122,10 @@  static inline void i2s_disable_channels(struct dw_i2s_dev *dev, u32 stream)
 
 	if (stream == SNDRV_PCM_STREAM_PLAYBACK) {
 		for (i = 0; i < 4; i++)
-			i2s_write_reg(dev->i2s_base, TER(i), 0);
+			i2s_write_reg(dev->i2s_pbase, TER(i), 0);
 	} else {
 		for (i = 0; i < 4; i++)
-			i2s_write_reg(dev->i2s_base, RER(i), 0);
+			i2s_write_reg(dev->i2s_cbase, RER(i), 0);
 	}
 }
 
@@ -131,25 +135,25 @@  static inline void i2s_clear_irqs(struct dw_i2s_dev *dev, u32 stream)
 
 	if (stream == SNDRV_PCM_STREAM_PLAYBACK) {
 		for (i = 0; i < 4; i++)
-			i2s_write_reg(dev->i2s_base, TOR(i), 0);
+			i2s_write_reg(dev->i2s_pbase, TOR(i), 0);
 	} else {
 		for (i = 0; i < 4; i++)
-			i2s_write_reg(dev->i2s_base, ROR(i), 0);
+			i2s_write_reg(dev->i2s_cbase, ROR(i), 0);
 	}
 }
 
 static void i2s_start(struct dw_i2s_dev *dev,
 		      struct snd_pcm_substream *substream)
 {
-
-	i2s_write_reg(dev->i2s_base, IER, 1);
-
-	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
-		i2s_write_reg(dev->i2s_base, ITER, 1);
-	else
-		i2s_write_reg(dev->i2s_base, IRER, 1);
-
-	i2s_write_reg(dev->i2s_base, CER, 1);
+	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
+		i2s_write_reg(dev->i2s_pbase, IER, 1);
+		i2s_write_reg(dev->i2s_pbase, ITER, 1);
+		i2s_write_reg(dev->i2s_pbase, CER, 1);
+	} else {
+		i2s_write_reg(dev->i2s_cbase, IER, 1);
+		i2s_write_reg(dev->i2s_cbase, IRER, 1);
+		i2s_write_reg(dev->i2s_cbase, CER, 1);
+	}
 }
 
 static void i2s_stop(struct dw_i2s_dev *dev,
@@ -159,24 +163,36 @@  static void i2s_stop(struct dw_i2s_dev *dev,
 
 	i2s_clear_irqs(dev, substream->stream);
 	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
-		i2s_write_reg(dev->i2s_base, ITER, 0);
-
+		i2s_write_reg(dev->i2s_pbase, ITER, 0);
 		for (i = 0; i < 4; i++) {
-			irq = i2s_read_reg(dev->i2s_base, IMR(i));
-			i2s_write_reg(dev->i2s_base, IMR(i), irq | 0x30);
+			irq = i2s_read_reg(dev->i2s_pbase, IMR(i));
+			i2s_write_reg(dev->i2s_pbase, IMR(i),
+						irq | 0x30);
 		}
 	} else {
-		i2s_write_reg(dev->i2s_base, IRER, 0);
+		i2s_write_reg(dev->i2s_cbase, IRER, 0);
 
 		for (i = 0; i < 4; i++) {
-			irq = i2s_read_reg(dev->i2s_base, IMR(i));
-			i2s_write_reg(dev->i2s_base, IMR(i), irq | 0x03);
+			irq = i2s_read_reg(dev->i2s_cbase, IMR(i));
+			i2s_write_reg(dev->i2s_cbase, IMR(i),
+						irq | 0x03);
 		}
 	}
 
-	if (!dev->active) {
-		i2s_write_reg(dev->i2s_base, CER, 0);
-		i2s_write_reg(dev->i2s_base, IER, 0);
+	if (dev->quirks & DW_I2S_VENDOR_AMD) {
+		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
+			i2s_write_reg(dev->i2s_pbase, CER, 0);
+			i2s_write_reg(dev->i2s_pbase, IER, 0);
+		} else {
+			i2s_write_reg(dev->i2s_cbase, CER, 0);
+			i2s_write_reg(dev->i2s_cbase, IER, 0);
+		}
+
+	} else {
+		if (!dev->active) {
+			i2s_write_reg(dev->i2s_pbase, CER, 0);
+			i2s_write_reg(dev->i2s_pbase, IER, 0);
+		}
 	}
 }
 
@@ -204,31 +220,59 @@  static int dw_i2s_startup(struct snd_pcm_substream *substream,
 	return 0;
 }
 
+static void dw_i2s_config(struct dw_i2s_dev *dev, int stream)
+{
+	u32 ch_reg, irq;
+	struct i2s_clk_config_data *config = &dev->config;
+
+
+	i2s_disable_channels(dev, stream);
+
+	for (ch_reg = 0; ch_reg < (config->chan_nr / 2); ch_reg++) {
+		if (stream == SNDRV_PCM_STREAM_PLAYBACK) {
+			i2s_write_reg(dev->i2s_pbase, TCR(ch_reg),
+				      dev->xfer_resolution);
+			i2s_write_reg(dev->i2s_pbase, TFCR(ch_reg), 0x02);
+			irq = i2s_read_reg(dev->i2s_pbase, IMR(ch_reg));
+			i2s_write_reg(dev->i2s_pbase, IMR(ch_reg), irq & ~0x30);
+			i2s_write_reg(dev->i2s_pbase, TER(ch_reg), 1);
+		} else {
+			i2s_write_reg(dev->i2s_cbase, RCR(ch_reg),
+				      dev->xfer_resolution);
+			i2s_write_reg(dev->i2s_cbase, RFCR(ch_reg), 0x07);
+			irq = i2s_read_reg(dev->i2s_cbase, IMR(ch_reg));
+			i2s_write_reg(dev->i2s_cbase, IMR(ch_reg), irq & ~0x03);
+			i2s_write_reg(dev->i2s_cbase, RER(ch_reg), 1);
+		}
+
+	}
+}
+
 static int dw_i2s_hw_params(struct snd_pcm_substream *substream,
 		struct snd_pcm_hw_params *params, struct snd_soc_dai *dai)
 {
 	struct dw_i2s_dev *dev = snd_soc_dai_get_drvdata(dai);
 	struct i2s_clk_config_data *config = &dev->config;
-	u32 ccr, xfer_resolution, ch_reg, irq;
+
 	int ret;
 
 	switch (params_format(params)) {
 	case SNDRV_PCM_FORMAT_S16_LE:
 		config->data_width = 16;
-		ccr = 0x00;
-		xfer_resolution = 0x02;
+		dev->ccr = 0x00;
+		dev->xfer_resolution = 0x02;
 		break;
 
 	case SNDRV_PCM_FORMAT_S24_LE:
 		config->data_width = 24;
-		ccr = 0x08;
-		xfer_resolution = 0x04;
+		dev->ccr = 0x08;
+		dev->xfer_resolution = 0x04;
 		break;
 
 	case SNDRV_PCM_FORMAT_S32_LE:
 		config->data_width = 32;
-		ccr = 0x10;
-		xfer_resolution = 0x05;
+		dev->ccr = 0x10;
+		dev->xfer_resolution = 0x05;
 		break;
 
 	default:
@@ -249,31 +293,12 @@  static int dw_i2s_hw_params(struct snd_pcm_substream *substream,
 		return -EINVAL;
 	}
 
-	i2s_disable_channels(dev, substream->stream);
-
-	for (ch_reg = 0; ch_reg < (config->chan_nr / 2); ch_reg++) {
-		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
-			i2s_write_reg(dev->i2s_base, TCR(ch_reg),
-				      xfer_resolution);
-			i2s_write_reg(dev->i2s_base, TFCR(ch_reg), 0x02);
-			irq = i2s_read_reg(dev->i2s_base, IMR(ch_reg));
-			i2s_write_reg(dev->i2s_base, IMR(ch_reg), irq & ~0x30);
-			i2s_write_reg(dev->i2s_base, TER(ch_reg), 1);
-		} else {
-			i2s_write_reg(dev->i2s_base, RCR(ch_reg),
-				      xfer_resolution);
-			i2s_write_reg(dev->i2s_base, RFCR(ch_reg), 0x07);
-			irq = i2s_read_reg(dev->i2s_base, IMR(ch_reg));
-			i2s_write_reg(dev->i2s_base, IMR(ch_reg), irq & ~0x03);
-			i2s_write_reg(dev->i2s_base, RER(ch_reg), 1);
-		}
-	}
-
-	i2s_write_reg(dev->i2s_base, CCR, ccr);
+	dw_i2s_config(dev, substream->stream);
 
 	config->sample_rate = params_rate(params);
 
 	if (dev->capability & DW_I2S_MASTER) {
+		i2s_write_reg(dev->i2s_pbase, CCR, dev->ccr);
 		if (dev->i2s_clk_cfg) {
 			ret = dev->i2s_clk_cfg(config);
 			if (ret < 0) {
@@ -287,7 +312,7 @@  static int dw_i2s_hw_params(struct snd_pcm_substream *substream,
 			ret = clk_set_rate(dev->clk, bitclk);
 			if (ret) {
 				dev_err(dev->dev, "Can't set I2S clock rate: %d\n",
-					ret);
+				ret);
 				return ret;
 			}
 		}
@@ -307,9 +332,9 @@  static int dw_i2s_prepare(struct snd_pcm_substream *substream,
 	struct dw_i2s_dev *dev = snd_soc_dai_get_drvdata(dai);
 
 	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
-		i2s_write_reg(dev->i2s_base, TXFFR, 1);
+		i2s_write_reg(dev->i2s_pbase, TXFFR, 1);
 	else
-		i2s_write_reg(dev->i2s_base, RXFFR, 1);
+		i2s_write_reg(dev->i2s_cbase, RXFFR, 1);
 
 	return 0;
 }
@@ -370,6 +395,13 @@  static int dw_i2s_resume(struct snd_soc_dai *dai)
 
 	if (dev->capability & DW_I2S_MASTER)
 		clk_enable(dev->clk);
+
+	if (dev->quirks & DW_I2S_VENDOR_AMD) {
+		if (dai->playback_active)
+			dw_i2s_config(dev, SNDRV_PCM_STREAM_PLAYBACK);
+		if (dai->capture_active)
+			dw_i2s_config(dev, SNDRV_PCM_STREAM_CAPTURE);
+	}
 	return 0;
 }
 
@@ -419,9 +451,15 @@  static int dw_configure_dai(struct dw_i2s_dev *dev,
 	 * Read component parameter registers to extract
 	 * the I2S block's configuration.
 	 */
-	u32 comp1 = i2s_read_reg(dev->i2s_base, I2S_COMP_PARAM_1);
-	u32 comp2 = i2s_read_reg(dev->i2s_base, I2S_COMP_PARAM_2);
-	u32 idx;
+	u32 comp1, comp2, idx;
+
+	if (dev->quirks & DW_I2S_VENDOR_AMD) {
+		comp1 = i2s_read_reg(dev->i2s_pbase, 0x124);
+		comp2 = i2s_read_reg(dev->i2s_pbase, 0x120);
+	} else {
+		comp1 = i2s_read_reg(dev->i2s_pbase, I2S_COMP_PARAM_1);
+		comp2 = i2s_read_reg(dev->i2s_pbase, I2S_COMP_PARAM_2);
+	}
 
 	if (COMP1_TX_ENABLED(comp1)) {
 		dev_dbg(dev->dev, " designware: play supported\n");
@@ -463,10 +501,16 @@  static int dw_configure_dai_by_pd(struct dw_i2s_dev *dev,
 				   struct resource *res,
 				   const struct i2s_platform_data *pdata)
 {
-	u32 comp1 = i2s_read_reg(dev->i2s_base, I2S_COMP_PARAM_1);
-	u32 idx = COMP1_APB_DATA_WIDTH(comp1);
+	u32 comp1, idx;
 	int ret;
 
+	if (dev->quirks & DW_I2S_VENDOR_AMD)
+		comp1 = i2s_read_reg(dev->i2s_pbase, 0x6c);
+	else
+		comp1 = i2s_read_reg(dev->i2s_pbase, I2S_COMP_PARAM_1);
+
+	idx = COMP1_APB_DATA_WIDTH(comp1);
+
 	if (WARN_ON(idx >= ARRAY_SIZE(bus_widths)))
 		return -EINVAL;
 
@@ -474,6 +518,9 @@  static int dw_configure_dai_by_pd(struct dw_i2s_dev *dev,
 	if (ret < 0)
 		return ret;
 
+	if (dev->quirks & DW_I2S_VENDOR_AMD)
+		goto end;
+
 	/* Set DMA slaves info */
 	dev->play_dma_data.pd.data = pdata->play_dma_data;
 	dev->capture_dma_data.pd.data = pdata->capture_dma_data;
@@ -485,7 +532,7 @@  static int dw_configure_dai_by_pd(struct dw_i2s_dev *dev,
 	dev->capture_dma_data.pd.addr_width = bus_widths[idx];
 	dev->play_dma_data.pd.filter = pdata->filter;
 	dev->capture_dma_data.pd.filter = pdata->filter;
-
+end:
 	return 0;
 }
 
@@ -493,8 +540,8 @@  static int dw_configure_dai_by_dt(struct dw_i2s_dev *dev,
 				   struct snd_soc_dai_driver *dw_i2s_dai,
 				   struct resource *res)
 {
-	u32 comp1 = i2s_read_reg(dev->i2s_base, I2S_COMP_PARAM_1);
-	u32 comp2 = i2s_read_reg(dev->i2s_base, I2S_COMP_PARAM_2);
+	u32 comp1 = i2s_read_reg(dev->i2s_pbase, I2S_COMP_PARAM_1);
+	u32 comp2 = i2s_read_reg(dev->i2s_pbase, I2S_COMP_PARAM_2);
 	u32 fifo_depth = 1 << (1 + COMP1_FIFO_DEPTH_GLOBAL(comp1));
 	u32 idx = COMP1_APB_DATA_WIDTH(comp1);
 	u32 idx2;
@@ -558,14 +605,15 @@  static int dw_i2s_probe(struct platform_device *pdev)
 	dw_i2s_dai->resume = dw_i2s_resume;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	dev->i2s_base = devm_ioremap_resource(&pdev->dev, res);
-	if (IS_ERR(dev->i2s_base))
-		return PTR_ERR(dev->i2s_base);
+	dev->i2s_pbase = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(dev->i2s_pbase))
+		return PTR_ERR(dev->i2s_pbase);
 
-	dev->dev = &pdev->dev;
+	dev->i2s_cbase = dev->i2s_pbase;
 
 	if (pdata) {
 		dev->capability = pdata->cap;
+		dev->quirks = pdata->quirks;
 		clk_id = NULL;
 		ret = dw_configure_dai_by_pd(dev, dw_i2s_dai, res, pdata);
 	} else {
@@ -575,6 +623,13 @@  static int dw_i2s_probe(struct platform_device *pdev)
 	if (ret < 0)
 		return ret;
 
+	if (dev->quirks & DW_I2S_VENDOR_AMD) {
+		res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+		dev->i2s_cbase = devm_ioremap_resource(&pdev->dev, res);
+		if (IS_ERR(dev->i2s_cbase))
+			return PTR_ERR(dev->i2s_cbase);
+	}
+
 	if (dev->capability & DW_I2S_MASTER) {
 		if (pdata) {
 			dev->i2s_clk_cfg = pdata->i2s_clk_cfg;
@@ -593,6 +648,7 @@  static int dw_i2s_probe(struct platform_device *pdev)
 			return ret;
 	}
 
+	dev->dev = &pdev->dev;
 	dev_set_drvdata(&pdev->dev, dev);
 	ret = devm_snd_soc_register_component(&pdev->dev, &dw_i2s_component,
 					 dw_i2s_dai, 1);