[alsa-devel][PATCH 0/3] Add master mode, tmd and right-j mode support

[alsa-devel][PATCH 0/3] Add master mode, tmd and right-j mode support

[Zidan Wang]

Add sai master mode support.
Add tdm slots support.
Add Right-J mode support.

Zidan Wang (3):
  SoC: fsl_sai: add sai master mode support
  ASoC: fsl_sai: Add support for tdm slots operation
  ASoC: fsl_sai: Add support for Right-J mode

 sound/soc/fsl/fsl_sai.c | 164 ++++++++++++++++++++++++++++++++++++++++++++----
 sound/soc/fsl/fsl_sai.h |  13 +++-
 2 files changed, 164 insertions(+), 13 deletions(-)

-- 
1.9.1

[alsa-devel][PATCH 1/3] SoC: fsl_sai: add sai master mode support

[Zidan Wang]

When sai works on master mode, set its bit clock and frame clock.

SAI has 4 MCLK source, bus clock, MCLK1, MCLK2 and MCLK3. fsl_sai_set_bclk
will select proper MCLK source, then calculate and set the bit clock divider.

After fsl_sai_set_bclk, enable the selected mclk in hw_params(), and add
hw_free() to disable the mclk.

Signed-off-by: Zidan Wang <zidan.wang@freescale.com>
---
 sound/soc/fsl/fsl_sai.c | 110 +++++++++++++++++++++++++++++++++++++++++++++++-
 sound/soc/fsl/fsl_sai.h |   9 +++-
 2 files changed, 115 insertions(+), 4 deletions(-)

diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c
index ec79c3d..499cbd9 100644
--- a/sound/soc/fsl/fsl_sai.c
+++ b/sound/soc/fsl/fsl_sai.c
@@ -251,12 +251,14 @@ static int fsl_sai_set_dai_fmt_tr(struct snd_soc_dai *cpu_dai,
 		val_cr4 |= FSL_SAI_CR4_FSD_MSTR;
 		break;
 	case SND_SOC_DAIFMT_CBM_CFM:
+		sai->is_slave_mode = true;
 		break;
 	case SND_SOC_DAIFMT_CBS_CFM:
 		val_cr2 |= FSL_SAI_CR2_BCD_MSTR;
 		break;
 	case SND_SOC_DAIFMT_CBM_CFS:
 		val_cr4 |= FSL_SAI_CR4_FSD_MSTR;
+		sai->is_slave_mode = true;
 		break;
 	default:
 		return -EINVAL;
@@ -288,6 +290,75 @@ static int fsl_sai_set_dai_fmt(struct snd_soc_dai *cpu_dai, unsigned int fmt)
 	return ret;
 }
 
+static int fsl_sai_set_bclk(struct snd_soc_dai *dai, bool tx, u32 freq)
+{
+	struct fsl_sai *sai = snd_soc_dai_get_drvdata(dai);
+	unsigned long clk_rate;
+	u32 savediv = 0, ratio, savesub = freq;
+	u32 id;
+	int ret = 0;
+
+	/* Don't apply to slave mode */
+	if (sai->is_slave_mode)
+		return 0;
+
+	for (id = 0; id < FSL_SAI_MCLK_MAX; id++) {
+		clk_rate = clk_get_rate(sai->mclk_clk[id]);
+		if (!clk_rate)
+			continue;
+
+		ratio = clk_rate / freq;
+
+		ret = clk_rate - ratio * freq;
+
+		/* Drop the source that can not be divided
+		 * into the required rate
+		 * */
+		if (ret != 0 && clk_rate / ret < 1000)
+			continue;
+
+		dev_dbg(dai->dev, "ratio %d for freq %dHz based on clock %ldHz\n",
+				ratio, freq, clk_rate);
+
+		if (ratio % 2 == 0 && ratio >= 2 && ratio <= 512)
+			ratio /= 2;
+		else
+			continue;
+
+		if (ret < savesub) {
+			savediv = ratio;
+			sai->mclk_id = id;
+			savesub = ret;
+		}
+
+		if (ret == 0)
+			break;
+	}
+
+	if (savediv == 0) {
+		dev_err(dai->dev, "failed to derive required %cx rate: %d\n",
+				tx ? 'T' : 'R', freq);
+		return -EINVAL;
+	}
+
+	if ((tx && sai->synchronous[TX]) || (!tx && !sai->synchronous[RX])) {
+		regmap_update_bits(sai->regmap, FSL_SAI_RCR2,
+			FSL_SAI_CR2_MSEL_MASK, FSL_SAI_CR2_MSEL(sai->mclk_id));
+		regmap_update_bits(sai->regmap, FSL_SAI_RCR2,
+			FSL_SAI_CR2_DIV_MASK, savediv - 1);
+	} else {
+		regmap_update_bits(sai->regmap, FSL_SAI_TCR2,
+			FSL_SAI_CR2_MSEL_MASK, FSL_SAI_CR2_MSEL(sai->mclk_id));
+		regmap_update_bits(sai->regmap, FSL_SAI_TCR2,
+			FSL_SAI_CR2_DIV_MASK, savediv - 1);
+	}
+
+	dev_dbg(dai->dev, "best fit: clock id=%d, div=%d, deviation =%d\n",
+			sai->mclk_id, savediv, savesub);
+
+	return 0;
+}
+
 static int fsl_sai_hw_params(struct snd_pcm_substream *substream,
 		struct snd_pcm_hw_params *params,
 		struct snd_soc_dai *cpu_dai)
@@ -297,6 +368,24 @@ static int fsl_sai_hw_params(struct snd_pcm_substream *substream,
 	unsigned int channels = params_channels(params);
 	u32 word_width = snd_pcm_format_width(params_format(params));
 	u32 val_cr4 = 0, val_cr5 = 0;
+	int ret;
+
+	if (!sai->is_slave_mode) {
+		ret = fsl_sai_set_bclk(cpu_dai, tx,
+			2 * word_width * params_rate(params));
+		if (ret)
+			return ret;
+
+		/* Do not enable the clock if it is already enabled */
+		if (!(sai->mclk_streams & BIT(substream->stream))) {
+			ret = clk_prepare_enable(sai->mclk_clk[sai->mclk_id]);
+			if (ret)
+				return ret;
+
+			sai->mclk_streams |= BIT(substream->stream);
+		}
+
+	}
 
 	if (!sai->is_dsp_mode)
 		val_cr4 |= FSL_SAI_CR4_SYWD(word_width);
@@ -322,6 +411,21 @@ static int fsl_sai_hw_params(struct snd_pcm_substream *substream,
 	return 0;
 }
 
+static int fsl_sai_hw_free(struct snd_pcm_substream *substream,
+		struct snd_soc_dai *cpu_dai)
+{
+	struct fsl_sai *sai = snd_soc_dai_get_drvdata(cpu_dai);
+
+	if (!sai->is_slave_mode &&
+			sai->mclk_streams & BIT(substream->stream)) {
+		clk_disable_unprepare(sai->mclk_clk[sai->mclk_id]);
+		sai->mclk_streams &= ~BIT(substream->stream);
+	}
+
+	return 0;
+}
+
+
 static int fsl_sai_trigger(struct snd_pcm_substream *substream, int cmd,
 		struct snd_soc_dai *cpu_dai)
 {
@@ -428,6 +532,7 @@ static const struct snd_soc_dai_ops fsl_sai_pcm_dai_ops = {
 	.set_sysclk	= fsl_sai_set_dai_sysclk,
 	.set_fmt	= fsl_sai_set_dai_fmt,
 	.hw_params	= fsl_sai_hw_params,
+	.hw_free	= fsl_sai_hw_free,
 	.trigger	= fsl_sai_trigger,
 	.startup	= fsl_sai_startup,
 	.shutdown	= fsl_sai_shutdown,
@@ -600,8 +705,9 @@ static int fsl_sai_probe(struct platform_device *pdev)
 		sai->bus_clk = NULL;
 	}
 
-	for (i = 0; i < FSL_SAI_MCLK_MAX; i++) {
-		sprintf(tmp, "mclk%d", i + 1);
+	sai->mclk_clk[0] = sai->bus_clk;
+	for (i = 1; i < FSL_SAI_MCLK_MAX; i++) {
+		sprintf(tmp, "mclk%d", i);
 		sai->mclk_clk[i] = devm_clk_get(&pdev->dev, tmp);
 		if (IS_ERR(sai->mclk_clk[i])) {
 			dev_err(&pdev->dev, "failed to get mclk%d clock: %ld\n",
diff --git a/sound/soc/fsl/fsl_sai.h b/sound/soc/fsl/fsl_sai.h
index 3466720..2d8c830 100644
--- a/sound/soc/fsl/fsl_sai.h
+++ b/sound/soc/fsl/fsl_sai.h
@@ -72,13 +72,15 @@
 
 /* SAI Transmit and Recieve Configuration 2 Register */
 #define FSL_SAI_CR2_SYNC	BIT(30)
-#define FSL_SAI_CR2_MSEL_MASK	(0xff << 26)
+#define FSL_SAI_CR2_MSEL_MASK	(0x3 << 26)
 #define FSL_SAI_CR2_MSEL_BUS	0
 #define FSL_SAI_CR2_MSEL_MCLK1	BIT(26)
 #define FSL_SAI_CR2_MSEL_MCLK2	BIT(27)
 #define FSL_SAI_CR2_MSEL_MCLK3	(BIT(26) | BIT(27))
+#define FSL_SAI_CR2_MSEL(ID)	((ID) << 26)
 #define FSL_SAI_CR2_BCP		BIT(25)
 #define FSL_SAI_CR2_BCD_MSTR	BIT(24)
+#define FSL_SAI_CR2_DIV_MASK	0xff
 
 /* SAI Transmit and Recieve Configuration 3 Register */
 #define FSL_SAI_CR3_TRCE	BIT(16)
@@ -120,7 +122,7 @@
 #define FSL_SAI_CLK_MAST2	2
 #define FSL_SAI_CLK_MAST3	3
 
-#define FSL_SAI_MCLK_MAX	3
+#define FSL_SAI_MCLK_MAX	4
 
 /* SAI data transfer numbers per DMA request */
 #define FSL_SAI_MAXBURST_TX 6
@@ -133,10 +135,13 @@ struct fsl_sai {
 	struct clk *mclk_clk[FSL_SAI_MCLK_MAX];
 
 	bool is_lsb_first;
+	bool is_slave_mode;
 	bool is_dsp_mode;
 	bool sai_on_imx;
 	bool synchronous[2];
 
+	unsigned int mclk_id;
+	unsigned int mclk_streams;
 	struct snd_dmaengine_dai_dma_data dma_params_rx;
 	struct snd_dmaengine_dai_dma_data dma_params_tx;
 };
-- 
1.9.1

Re: [alsa-devel][PATCH 1/3] SoC: fsl_sai: add sai master mode support

[Nicolin Chen]

On Tue, Jan 20, 2015 at 08:21:18PM +0800, Zidan Wang wrote:
> +static int fsl_sai_set_bclk(struct snd_soc_dai *dai, bool tx, u32 freq)

> +	if ((tx && sai->synchronous[TX]) || (!tx && !sai->synchronous[RX])) {
> +		regmap_update_bits(sai->regmap, FSL_SAI_RCR2,
> +			FSL_SAI_CR2_MSEL_MASK, FSL_SAI_CR2_MSEL(sai->mclk_id));
> +		regmap_update_bits(sai->regmap, FSL_SAI_RCR2,
> +			FSL_SAI_CR2_DIV_MASK, savediv - 1);

Hmm...the case should be a bit more complicated here IMO.

"tx && sai->synchronous[TX]" means the playback in synchronous
mode (TX following RX). What if the recording has been already
activated with an MSEL setting at this point? Then the playback
stream, as a secondary stream, will overwrite MSEL of the first
stream -- Record. Same would happen to the DIV configuration.

> @@ -297,6 +368,24 @@ static int fsl_sai_hw_params(struct snd_pcm_substream *substream,
>  	unsigned int channels = params_channels(params);
>  	u32 word_width = snd_pcm_format_width(params_format(params));
>  	u32 val_cr4 = 0, val_cr5 = 0;
> +	int ret;
> +
> +	if (!sai->is_slave_mode) {
> +		ret = fsl_sai_set_bclk(cpu_dai, tx,
> +			2 * word_width * params_rate(params));
> +		if (ret)
> +			return ret;
> +
> +		/* Do not enable the clock if it is already enabled */

It actually doesn't matter to enable the clock again since it
purely increaes its count. But we do need a protection for MSEL
overwritten issue resulted from the fsl_sai_set_bclk() call.

> @@ -133,10 +135,13 @@ struct fsl_sai {
>  	struct clk *mclk_clk[FSL_SAI_MCLK_MAX];
>  
>  	bool is_lsb_first;
> +	bool is_slave_mode;
>  	bool is_dsp_mode;
>  	bool sai_on_imx;
>  	bool synchronous[2];
>  
> +	unsigned int mclk_id;
> +	unsigned int mclk_streams;

Besides, I doubt that only one property of mclk_id can content
Asynchronous Mode -- TX and RX can fetch their clocks from
different MCLK sources.

Nicolin

Re: [alsa-devel][PATCH 1/3] SoC: fsl_sai: add sai master mode support

[Zidan Wang]

On Tue, Jan 20, 2015 at 10:07:03PM -0800, Nicolin Chen wrote:
> On Tue, Jan 20, 2015 at 08:21:18PM +0800, Zidan Wang wrote:
> > +static int fsl_sai_set_bclk(struct snd_soc_dai *dai, bool tx, u32 freq)
> 
> > +	if ((tx && sai->synchronous[TX]) || (!tx && !sai->synchronous[RX])) {
> > +		regmap_update_bits(sai->regmap, FSL_SAI_RCR2,
> > +			FSL_SAI_CR2_MSEL_MASK, FSL_SAI_CR2_MSEL(sai->mclk_id));
> > +		regmap_update_bits(sai->regmap, FSL_SAI_RCR2,
> > +			FSL_SAI_CR2_DIV_MASK, savediv - 1);
> 
> Hmm...the case should be a bit more complicated here IMO.
> 
> "tx && sai->synchronous[TX]" means the playback in synchronous
> mode (TX following RX). What if the recording has been already
> activated with an MSEL setting at this point? Then the playback
> stream, as a secondary stream, will overwrite MSEL of the first
> stream -- Record. Same would happen to the DIV configuration.
> 
When TX following RX(or RX following TX), TX and RX works on same bit
clock and frame clock. They will use same MCLK source, and just need set
the bclk DIV for RX(or TX). The secondary stream will overwrite MSEL and
bclk DIV of the first stream, but it doesn't matter.

For RX(or TX) sync:
fsl_sai_dai.symmetric_rates = 1;
fsl_sai_dai.symmetric_channels = 1;
fsl_sai_dai.symmetric_samplebits = 1;

When TX and RX both works on async mode, TX and RX may works on
different bit clock and frame clock. We need set MCLK source and bclk
DIV for TX and RX. mclk_id just save a MCLK source id, so i need to define
mclk_id[2] for differnet stream.

For TX and RX async:
fsl_sai_dai.symmetric_rates = 0;
fsl_sai_dai.symmetric_channels = 0;
fsl_sai_dai.symmetric_samplebits = 0;

> > @@ -297,6 +368,24 @@ static int fsl_sai_hw_params(struct snd_pcm_substream *substream,
> >  	unsigned int channels = params_channels(params);
> >  	u32 word_width = snd_pcm_format_width(params_format(params));
> >  	u32 val_cr4 = 0, val_cr5 = 0;
> > +	int ret;
> > +
> > +	if (!sai->is_slave_mode) {
> > +		ret = fsl_sai_set_bclk(cpu_dai, tx,
> > +			2 * word_width * params_rate(params));
> > +		if (ret)
> > +			return ret;
> > +
> > +		/* Do not enable the clock if it is already enabled */
> 
> It actually doesn't matter to enable the clock again since it
> purely increaes its count. But we do need a protection for MSEL
> overwritten issue resulted from the fsl_sai_set_bclk() call.
> 
> > @@ -133,10 +135,13 @@ struct fsl_sai {
> >  	struct clk *mclk_clk[FSL_SAI_MCLK_MAX];
> >  
> >  	bool is_lsb_first;
> > +	bool is_slave_mode;
> >  	bool is_dsp_mode;
> >  	bool sai_on_imx;
> >  	bool synchronous[2];
> >  
> > +	unsigned int mclk_id;
> > +	unsigned int mclk_streams;
> 
> Besides, I doubt that only one property of mclk_id can content
> Asynchronous Mode -- TX and RX can fetch their clocks from
> different MCLK sources.
> 
When hw_params failed before mclk enable, hw_free will also decrease 
the clk usecount. So add mclk_streams, when clk successful enabled, 
set corresponding bit of mclk_stream. And clear such bit after clk disabled.
> Nicolin

Re: [alsa-devel][PATCH 1/3] SoC: fsl_sai: add sai master mode support

[Nicolin Chen]

On Wed, Jan 21, 2015 at 05:25:32PM +0800, Zidan Wang wrote:
> On Tue, Jan 20, 2015 at 10:07:03PM -0800, Nicolin Chen wrote:
> > On Tue, Jan 20, 2015 at 08:21:18PM +0800, Zidan Wang wrote:

> > > +	if ((tx && sai->synchronous[TX]) || (!tx && !sai->synchronous[RX])) {
> > > +		regmap_update_bits(sai->regmap, FSL_SAI_RCR2,
> > > +			FSL_SAI_CR2_MSEL_MASK, FSL_SAI_CR2_MSEL(sai->mclk_id));

> > "tx && sai->synchronous[TX]" means the playback in synchronous
> > mode (TX following RX). What if the recording has been already
> > activated with an MSEL setting at this point? Then the playback
> > stream, as a secondary stream, will overwrite MSEL of the first
> > stream -- Record. Same would happen to the DIV configuration.
> > 
> When TX following RX(or RX following TX), TX and RX works on same bit
> clock and frame clock. They will use same MCLK source, and just need set
> the bclk DIV for RX(or TX). The secondary stream will overwrite MSEL and
> bclk DIV of the first stream, but it doesn't matter.
> 
> For RX(or TX) sync:
> fsl_sai_dai.symmetric_rates = 1;
> fsl_sai_dai.symmetric_channels = 1;
> fsl_sai_dai.symmetric_samplebits = 1;

Ah, I forgot we have protection here. It's fine then.

> When TX and RX both works on async mode, TX and RX may works on
> different bit clock and frame clock. We need set MCLK source and bclk
> DIV for TX and RX. mclk_id just save a MCLK source id, so i need to define
> mclk_id[2] for differnet stream.

Yes, you need to change this part as you just realized.

And one more thing for your coding style for multi-line comment:
/*
 * Typically use this format.
 */

Thanks
Nicolin

[alsa-devel][PATCH 2/3] ASoC: fsl_sai: Add support for tdm slots operation

[Zidan Wang]

slots and slot width is set from set_dai_tdm_slot in machine driver.
We should calculate the actual slots per channel using slots/channels.

When using tdm slots, we should generate bclk depends channels, slots
and slot width. And there may be unused BCLK cycles before each LRCLK
transition.

Set TCR2 WNW bit to slot width and TCR4 FRSZ bit to slots * channels to
configure frame Length. And it is no need to set TCR4 SYWD to set frame
sync length for sai slave mode, so just do it when it is sai master mode.

Signed-off-by: Zidan Wang <zidan.wang@freescale.com>
---
 sound/soc/fsl/fsl_sai.c | 46 +++++++++++++++++++++++++++++++++++++---------
 sound/soc/fsl/fsl_sai.h |  3 +++
 2 files changed, 40 insertions(+), 9 deletions(-)

diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c
index 499cbd9..4c5040d 100644
--- a/sound/soc/fsl/fsl_sai.c
+++ b/sound/soc/fsl/fsl_sai.c
@@ -115,6 +115,17 @@ out:
 		return IRQ_HANDLED;
 }
 
+static int fsl_sai_set_dai_tdm_slot(struct snd_soc_dai *cpu_dai, u32 tx_mask,
+				u32 rx_mask, int slots, int slot_width)
+{
+	struct fsl_sai *sai = snd_soc_dai_get_drvdata(cpu_dai);
+
+	sai->slots = slots;
+	sai->slot_width = slot_width;
+
+	return 0;
+}
+
 static int fsl_sai_set_dai_sysclk_tr(struct snd_soc_dai *cpu_dai,
 		int clk_id, unsigned int freq, int fsl_dir)
 {
@@ -369,10 +380,25 @@ static int fsl_sai_hw_params(struct snd_pcm_substream *substream,
 	u32 word_width = snd_pcm_format_width(params_format(params));
 	u32 val_cr4 = 0, val_cr5 = 0;
 	int ret;
+	u32 bclk;
+
+	if (channels == 1)
+		channels = 2;
+
+	if (!sai->slots || sai->slots % channels)
+		sai->slots = channels;
+
+	sai->slots = sai->slots / channels;
+
+	if (sai->slot_width < word_width || sai->is_dsp_mode)
+		sai->slot_width = word_width;
 
 	if (!sai->is_slave_mode) {
-		ret = fsl_sai_set_bclk(cpu_dai, tx,
-			2 * word_width * params_rate(params));
+		bclk = snd_soc_calc_bclk(params_rate(params), sai->slot_width,
+				channels, sai->slots);
+
+		ret = fsl_sai_set_bclk(cpu_dai, tx, bclk);
+
 		if (ret)
 			return ret;
 
@@ -385,20 +411,19 @@ static int fsl_sai_hw_params(struct snd_pcm_substream *substream,
 			sai->mclk_streams |= BIT(substream->stream);
 		}
 
+		if (!sai->is_dsp_mode)
+			val_cr4 |= FSL_SAI_CR4_SYWD(sai->slot_width);
 	}
 
-	if (!sai->is_dsp_mode)
-		val_cr4 |= FSL_SAI_CR4_SYWD(word_width);
-
-	val_cr5 |= FSL_SAI_CR5_WNW(word_width);
-	val_cr5 |= FSL_SAI_CR5_W0W(word_width);
+	val_cr5 |= FSL_SAI_CR5_WNW(sai->slot_width);
+	val_cr5 |= FSL_SAI_CR5_W0W(sai->slot_width);
 
 	if (sai->is_lsb_first)
 		val_cr5 |= FSL_SAI_CR5_FBT(0);
 	else
 		val_cr5 |= FSL_SAI_CR5_FBT(word_width - 1);
 
-	val_cr4 |= FSL_SAI_CR4_FRSZ(channels);
+	val_cr4 |= FSL_SAI_CR4_FRSZ(sai->slots * channels);
 
 	regmap_update_bits(sai->regmap, FSL_SAI_xCR4(tx),
 			   FSL_SAI_CR4_SYWD_MASK | FSL_SAI_CR4_FRSZ_MASK,
@@ -406,7 +431,9 @@ static int fsl_sai_hw_params(struct snd_pcm_substream *substream,
 	regmap_update_bits(sai->regmap, FSL_SAI_xCR5(tx),
 			   FSL_SAI_CR5_WNW_MASK | FSL_SAI_CR5_W0W_MASK |
 			   FSL_SAI_CR5_FBT_MASK, val_cr5);
-	regmap_write(sai->regmap, FSL_SAI_xMR(tx), ~0UL - ((1 << channels) - 1));
+
+	regmap_write(sai->regmap, FSL_SAI_xMR(tx),
+		     ~0UL - ((1 << params_channels(params) * sai->slots) - 1));
 
 	return 0;
 }
@@ -531,6 +558,7 @@ static void fsl_sai_shutdown(struct snd_pcm_substream *substream,
 static const struct snd_soc_dai_ops fsl_sai_pcm_dai_ops = {
 	.set_sysclk	= fsl_sai_set_dai_sysclk,
 	.set_fmt	= fsl_sai_set_dai_fmt,
+	.set_tdm_slot	= fsl_sai_set_dai_tdm_slot,
 	.hw_params	= fsl_sai_hw_params,
 	.hw_free	= fsl_sai_hw_free,
 	.trigger	= fsl_sai_trigger,
diff --git a/sound/soc/fsl/fsl_sai.h b/sound/soc/fsl/fsl_sai.h
index 2d8c830..111dfce 100644
--- a/sound/soc/fsl/fsl_sai.h
+++ b/sound/soc/fsl/fsl_sai.h
@@ -140,6 +140,9 @@ struct fsl_sai {
 	bool sai_on_imx;
 	bool synchronous[2];
 
+	unsigned int slots;
+	unsigned int slot_width;
+
 	unsigned int mclk_id;
 	unsigned int mclk_streams;
 	struct snd_dmaengine_dai_dma_data dma_params_rx;
-- 
1.9.1

Re: [alsa-devel][PATCH 2/3] ASoC: fsl_sai: Add support for tdm slots operation

[Nicolin Chen]

On Tue, Jan 20, 2015 at 08:21:19PM +0800, Zidan Wang wrote:
> @@ -369,10 +380,25 @@ static int fsl_sai_hw_params(struct snd_pcm_substream *substream,
>  	u32 word_width = snd_pcm_format_width(params_format(params));
>  	u32 val_cr4 = 0, val_cr5 = 0;
>  	int ret;
> +	u32 bclk;
> +
> +	if (channels == 1)
> +		channels = 2;
> +
> +	if (!sai->slots || sai->slots % channels)
> +		sai->slots = channels;
> +
> +	sai->slots = sai->slots / channels;
> +
> +	if (sai->slot_width < word_width || sai->is_dsp_mode)
> +		sai->slot_width = word_width;

Could you pls explain a bit what's this overriding for?
Or it might be better to put into a comment.

Nicolin

Re: [alsa-devel][PATCH 2/3] ASoC: fsl_sai: Add support for tdm slots operation

[Zidan Wang]

On Wed, Jan 21, 2015 at 10:08:03AM -0800, Nicolin Chen wrote:
> On Tue, Jan 20, 2015 at 08:21:19PM +0800, Zidan Wang wrote:
> > @@ -369,10 +380,25 @@ static int fsl_sai_hw_params(struct snd_pcm_substream *substream,
> >  	u32 word_width = snd_pcm_format_width(params_format(params));
> >  	u32 val_cr4 = 0, val_cr5 = 0;
> >  	int ret;
> > +	u32 bclk;
> > +
> > +	if (channels == 1)
> > +		channels = 2;
> > +
> > +	if (!sai->slots || sai->slots % channels)
> > +		sai->slots = channels;
> > +
> > +	sai->slots = sai->slots / channels;
> > +
> > +	if (sai->slot_width < word_width || sai->is_dsp_mode)
> > +		sai->slot_width = word_width;
> 
> Could you pls explain a bit what's this overriding for?
> Or it might be better to put into a comment.
>
In machine driver, we should use snd_soc_dai_set_tdm_slot to set slots
and slot width. In my opinion, slots in machine driver means the slot
number of all channels. But in sai driver, the slots means the slot number
of one channel. So snd_soc_calc_bclk use fs*sample_size*channels*tdm_slots
to caculate bclk.

So i use "sai->slots = sai->slots / channels" to calculate slots per channel. 
If we missing set slots, use channel number to set slots and get one slot per 
channel. If we missing set slot width, set default slot width to word width. 

If slot width is 32 and wrod width is 16, for 2 channels and one slot
per channel, it will be 64 bit clock for one frame.

val_cr5 |= FSL_SAI_CR5_WNW(sai->slot_width).
val_cr5 |= FSL_SAI_CR5_W0W(sai->slot_width);

So sai word length is 32, it will trans 32 bit data per channel. But dma only
trans 16 bit data to fifo, the continues 16 bit will be 0. So for 16 bit data 
formate, it will just one channel has data. When it is dsp mode, let slot
width equal to word width can fix the issue.


> Nicolin

Re: [alsa-devel][PATCH 2/3] ASoC: fsl_sai: Add support for tdm slots operation

[Nicolin Chen]

On Thu, Jan 22, 2015 at 12:55:35PM +0800, Zidan Wang wrote:
> On Wed, Jan 21, 2015 at 10:08:03AM -0800, Nicolin Chen wrote:
> > On Tue, Jan 20, 2015 at 08:21:19PM +0800, Zidan Wang wrote:
> > > @@ -369,10 +380,25 @@ static int fsl_sai_hw_params(struct snd_pcm_substream *substream,
> > >  	u32 word_width = snd_pcm_format_width(params_format(params));
> > >  	u32 val_cr4 = 0, val_cr5 = 0;
> > >  	int ret;
> > > +	u32 bclk;
> > > +
> > > +	if (channels == 1)
> > > +		channels = 2;
> > > +
> > > +	if (!sai->slots || sai->slots % channels)
> > > +		sai->slots = channels;
> > > +
> > > +	sai->slots = sai->slots / channels;
> > > +
> > > +	if (sai->slot_width < word_width || sai->is_dsp_mode)
> > > +		sai->slot_width = word_width;
> > 
> > Could you pls explain a bit what's this overriding for?
> > Or it might be better to put into a comment.
> >
> In machine driver, we should use snd_soc_dai_set_tdm_slot to set slots
> and slot width. In my opinion, slots in machine driver means the slot
> number of all channels. But in sai driver, the slots means the slot number
> of one channel. So snd_soc_calc_bclk use fs*sample_size*channels*tdm_slots
> to caculate bclk.

Could you pls use the standard way as you described for slot number
in machine driver instead of defining some other meanings? That one
is really confusing, not to mention a code without any comment.

You can add some extra local variables with extinguished names and
calculate the slot number and channels you want without changing
the original sai->slots and channels in the hw_params(). As you said,
you only need that new slot number and channels for snd_soc_calc_bclk
call. So you may also put this code right before the call -- a normal
routine doesn't need to recalculate the special slots and channels,
right?

Nicolin

Re: [alsa-devel][PATCH 2/3] ASoC: fsl_sai: Add support for tdm slots operation

[Zidan Wang]

On Wed, Jan 21, 2015 at 09:44:10PM -0800, Nicolin Chen wrote:
> On Thu, Jan 22, 2015 at 12:55:35PM +0800, Zidan Wang wrote:
> > On Wed, Jan 21, 2015 at 10:08:03AM -0800, Nicolin Chen wrote:
> > > On Tue, Jan 20, 2015 at 08:21:19PM +0800, Zidan Wang wrote:
> > > > @@ -369,10 +380,25 @@ static int fsl_sai_hw_params(struct snd_pcm_substream *substream,
> > > >  	u32 word_width = snd_pcm_format_width(params_format(params));
> > > >  	u32 val_cr4 = 0, val_cr5 = 0;
> > > >  	int ret;
> > > > +	u32 bclk;
> > > > +
> > > > +	if (channels == 1)
> > > > +		channels = 2;
> > > > +
> > > > +	if (!sai->slots || sai->slots % channels)
> > > > +		sai->slots = channels;
> > > > +
> > > > +	sai->slots = sai->slots / channels;
> > > > +
> > > > +	if (sai->slot_width < word_width || sai->is_dsp_mode)
> > > > +		sai->slot_width = word_width;
> > > 
> > > Could you pls explain a bit what's this overriding for?
> > > Or it might be better to put into a comment.
> > >
> > In machine driver, we should use snd_soc_dai_set_tdm_slot to set slots
> > and slot width. In my opinion, slots in machine driver means the slot
> > number of all channels. But in sai driver, the slots means the slot number
> > of one channel. So snd_soc_calc_bclk use fs*sample_size*channels*tdm_slots
> > to caculate bclk.
> 
> Could you pls use the standard way as you described for slot number
> in machine driver instead of defining some other meanings? That one
> is really confusing, not to mention a code without any comment.
> 
> You can add some extra local variables with extinguished names and
> calculate the slot number and channels you want without changing
> the original sai->slots and channels in the hw_params(). As you said,
> you only need that new slot number and channels for snd_soc_calc_bclk
> call. So you may also put this code right before the call -- a normal
> routine doesn't need to recalculate the special slots and channels,
> right?
> 
> Nicolin

Actually i don't kown which meaning is the standard way. Because i found
in wm8993 and wm8904 codec driver, they also use slots/channels to set 
tdm_slots in set_tdm_slot function.


Best Regards,
Zidan

Re: [alsa-devel][PATCH 2/3] ASoC: fsl_sai: Add support for tdm slots operation

[Nicolin Chen]

On Thu, Jan 22, 2015 at 02:20:06PM +0800, Zidan Wang wrote:
> > > > > @@ -369,10 +380,25 @@ static int fsl_sai_hw_params(struct snd_pcm_substream *substream,
> > > > >  	u32 word_width = snd_pcm_format_width(params_format(params));
> > > > >  	u32 val_cr4 = 0, val_cr5 = 0;
> > > > >  	int ret;
> > > > > +	u32 bclk;
> > > > > +
> > > > > +	if (channels == 1)
> > > > > +		channels = 2;
> > > > > +
> > > > > +	if (!sai->slots || sai->slots % channels)
> > > > > +		sai->slots = channels;
> > > > > +
> > > > > +	sai->slots = sai->slots / channels;
> > > > > +
> > > > > +	if (sai->slot_width < word_width || sai->is_dsp_mode)
> > > > > +		sai->slot_width = word_width;
> > > > 
> > > > Could you pls explain a bit what's this overriding for?
> > > > Or it might be better to put into a comment.
> > > >
> > > In machine driver, we should use snd_soc_dai_set_tdm_slot to set slots
> > > and slot width. In my opinion, slots in machine driver means the slot
> > > number of all channels. But in sai driver, the slots means the slot number
> > > of one channel. So snd_soc_calc_bclk use fs*sample_size*channels*tdm_slots
> > > to caculate bclk.
> > 
> > Could you pls use the standard way as you described for slot number
> > in machine driver instead of defining some other meanings? That one
> > is really confusing, not to mention a code without any comment.
> > 
> > You can add some extra local variables with extinguished names and
> > calculate the slot number and channels you want without changing
> > the original sai->slots and channels in the hw_params(). As you said,
> > you only need that new slot number and channels for snd_soc_calc_bclk
> > call. So you may also put this code right before the call -- a normal
> > routine doesn't need to recalculate the special slots and channels,
> > right?

> Actually i don't kown which meaning is the standard way. Because i found
> in wm8993 and wm8904 codec driver, they also use slots/channels to set 
> tdm_slots in set_tdm_slot function.

The slots via machine driver should be total slot number per frame.
For I2S, we're passing 2 for slots parameter in our machine drivers.

So why not just following that pattern. You code for calculating slot
per channel is used to calculate bclk. But keeping slot per frame will
also allow you to do that, meanwhile you can drop some code to do the
extra calculation so as to keep it neat.

I suggest you to take a look at fsl_esai.c and to follow that way:
Override slots and slot_width if specified from machine driver.

And another problem of your patch is the configurations for FRSZ and
xMR fields. For monaural case, the channels == 1, the current code
passes it directly to set them while your code overrides it to 2.

I'm not sure if you've tested the monaural case and confirmed it works.
But, apparently, the configurations look pretty different.

Nicolin

Re: [alsa-devel][PATCH 2/3] ASoC: fsl_sai: Add support for tdm slots operation

[Zidan Wang]

On Thu, Jan 22, 2015 at 03:50:20PM -0800, Nicolin Chen wrote:
> On Thu, Jan 22, 2015 at 02:20:06PM +0800, Zidan Wang wrote:
> > > > > > @@ -369,10 +380,25 @@ static int fsl_sai_hw_params(struct snd_pcm_substream *substream,
> > > > > >  	u32 word_width = snd_pcm_format_width(params_format(params));
> > > > > >  	u32 val_cr4 = 0, val_cr5 = 0;
> > > > > >  	int ret;
> > > > > > +	u32 bclk;
> > > > > > +
> > > > > > +	if (channels == 1)
> > > > > > +		channels = 2;
> > > > > > +
> > > > > > +	if (!sai->slots || sai->slots % channels)
> > > > > > +		sai->slots = channels;
> > > > > > +
> > > > > > +	sai->slots = sai->slots / channels;
> > > > > > +
> > > > > > +	if (sai->slot_width < word_width || sai->is_dsp_mode)
> > > > > > +		sai->slot_width = word_width;
> > > > > 
> > > > > Could you pls explain a bit what's this overriding for?
> > > > > Or it might be better to put into a comment.
> > > > >
> > > > In machine driver, we should use snd_soc_dai_set_tdm_slot to set slots
> > > > and slot width. In my opinion, slots in machine driver means the slot
> > > > number of all channels. But in sai driver, the slots means the slot number
> > > > of one channel. So snd_soc_calc_bclk use fs*sample_size*channels*tdm_slots
> > > > to caculate bclk.
> > > 
> > > Could you pls use the standard way as you described for slot number
> > > in machine driver instead of defining some other meanings? That one
> > > is really confusing, not to mention a code without any comment.
> > > 
> > > You can add some extra local variables with extinguished names and
> > > calculate the slot number and channels you want without changing
> > > the original sai->slots and channels in the hw_params(). As you said,
> > > you only need that new slot number and channels for snd_soc_calc_bclk
> > > call. So you may also put this code right before the call -- a normal
> > > routine doesn't need to recalculate the special slots and channels,
> > > right?
> 
> > Actually i don't kown which meaning is the standard way. Because i found
> > in wm8993 and wm8904 codec driver, they also use slots/channels to set 
> > tdm_slots in set_tdm_slot function.
> 
> The slots via machine driver should be total slot number per frame.
> For I2S, we're passing 2 for slots parameter in our machine drivers.
> 
> So why not just following that pattern. You code for calculating slot
> per channel is used to calculate bclk. But keeping slot per frame will
> also allow you to do that, meanwhile you can drop some code to do the
> extra calculation so as to keep it neat.
> 
> I suggest you to take a look at fsl_esai.c and to follow that way:
> Override slots and slot_width if specified from machine driver.
> 
I will follow slot per frame pattern like fsl_esai.c, thanks very mush.

I have another question.

Can a channel have several slots? Or channel just means slot.

> And another problem of your patch is the configurations for FRSZ and
> xMR fields. For monaural case, the channels == 1, the current code
> passes it directly to set them while your code overrides it to 2.
> 
> I'm not sure if you've tested the monaural case and confirmed it works.
> But, apparently, the configurations look pretty different.
>
I have tested the mono case, it works well.

Best Regards,
Zidan

Re: [alsa-devel][PATCH 2/3] ASoC: fsl_sai: Add support for tdm slots operation

[Nicolin Chen]

On Fri, Jan 23, 2015 at 10:55:09AM +0800, Zidan Wang wrote:
> I have another question.
> 
> Can a channel have several slots? Or channel just means slot.

Strictly speaking, a channel surely may have several slots IMO.
The idea for TDM, even for I2S, left channel can bear multiple
slots -- each may be masked and then received by a different
peripheral.

The reason I've said that much against your code is not fully
objection against your idea of using Slot Per Channel but for
the implementation -- you divided them and then multiplied them
again while your true motivation was just to calculate the bclk.
And I could not understand it without seeing your later explain.
Code and things would be much easier if you make it straight
forward.

But actually I don't mind if you feel comfortable to make a
detour like that, but as I commented at the first place. Code
like that should be companied with comments to tell its meaning.
And I still don't fully understand why you override the channels
for monaural cases.

> > And another problem of your patch is the configurations for FRSZ and
> > xMR fields. For monaural case, the channels == 1, the current code
> > passes it directly to set them while your code overrides it to 2.
> > 
> > I'm not sure if you've tested the monaural case and confirmed it works.
> > But, apparently, the configurations look pretty different.

> I have tested the mono case, it works well.

I just ran a simple test with speaker-test -Dhw:0 -c1, it works
but the FRSZ value is changed to 0x1 for a monaural case (0x0
originally). Setting FRSZ to 0x0 makes more sense to me since
it looks like the configuration for active slot number. But
that depends on the internal logic design of SAI so it may be
better for you to double check with IC owner and run more tests.
If you confirm that, I for sure will be willing to ignore the
change here.

Nicolin

[alsa-devel][PATCH 3/3] ASoC: fsl_sai: Add support for Right-J mode

[Zidan Wang]

Add Right-J mode and set TCR5 FBT bit to let data right justify.

Signed-off-by: Zidan Wang <zidan.wang@freescale.com>
---
 sound/soc/fsl/fsl_sai.c | 14 +++++++++++---
 sound/soc/fsl/fsl_sai.h |  1 +
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c
index 4c5040d..19cd6f3 100644
--- a/sound/soc/fsl/fsl_sai.c
+++ b/sound/soc/fsl/fsl_sai.c
@@ -228,7 +228,11 @@ static int fsl_sai_set_dai_fmt_tr(struct snd_soc_dai *cpu_dai,
 		sai->is_dsp_mode = true;
 		break;
 	case SND_SOC_DAIFMT_RIGHT_J:
-		/* To be done */
+		/* Frame high, one word length for frame sync */
+		val_cr2 |= FSL_SAI_CR2_BCP;
+		sai->is_right_j_mode = true;
+		break;
+
 	default:
 		return -EINVAL;
 	}
@@ -418,9 +422,13 @@ static int fsl_sai_hw_params(struct snd_pcm_substream *substream,
 	val_cr5 |= FSL_SAI_CR5_WNW(sai->slot_width);
 	val_cr5 |= FSL_SAI_CR5_W0W(sai->slot_width);
 
-	if (sai->is_lsb_first)
+	if (sai->is_lsb_first && sai->is_right_j_mode)
 		val_cr5 |= FSL_SAI_CR5_FBT(0);
-	else
+	else if (sai->is_lsb_first && !sai->is_right_j_mode)
+		val_cr5 |= FSL_SAI_CR5_FBT(sai->slot_width - word_width);
+	else if (!sai->is_lsb_first && sai->is_right_j_mode)
+		val_cr5 |= FSL_SAI_CR5_FBT(sai->slot_width - 1);
+	else if (!sai->is_lsb_first && !sai->is_right_j_mode)
 		val_cr5 |= FSL_SAI_CR5_FBT(word_width - 1);
 
 	val_cr4 |= FSL_SAI_CR4_FRSZ(sai->slots * channels);
diff --git a/sound/soc/fsl/fsl_sai.h b/sound/soc/fsl/fsl_sai.h
index 111dfce..e220940 100644
--- a/sound/soc/fsl/fsl_sai.h
+++ b/sound/soc/fsl/fsl_sai.h
@@ -137,6 +137,7 @@ struct fsl_sai {
 	bool is_lsb_first;
 	bool is_slave_mode;
 	bool is_dsp_mode;
+	bool is_right_j_mode;
 	bool sai_on_imx;
 	bool synchronous[2];
 
-- 
1.9.1

Re: [alsa-devel][PATCH 3/3] ASoC: fsl_sai: Add support for Right-J mode

[Nicolin Chen]

On Tue, Jan 20, 2015 at 08:21:20PM +0800, Zidan Wang wrote:
> Add Right-J mode and set TCR5 FBT bit to let data right justify.
> 
> Signed-off-by: Zidan Wang <zidan.wang@freescale.com>

> -	if (sai->is_lsb_first)
> +	if (sai->is_lsb_first && sai->is_right_j_mode)
>  		val_cr5 |= FSL_SAI_CR5_FBT(0);

Are you sure that FBT(0) is correct for right justified mode?
Because the original code is using FBT(0) for the lsb_first
situation and it shouldn't be right justified mode as default.

Nicolin

Re: [alsa-devel][PATCH 3/3] ASoC: fsl_sai: Add support for Right-J mode

[Zidan Wang]

On Wed, Jan 21, 2015 at 10:53:20AM -0800, Nicolin Chen wrote:
> On Tue, Jan 20, 2015 at 08:21:20PM +0800, Zidan Wang wrote:
> > Add Right-J mode and set TCR5 FBT bit to let data right justify.
> > 
> > Signed-off-by: Zidan Wang <zidan.wang@freescale.com>
> 
> > -	if (sai->is_lsb_first)
> > +	if (sai->is_lsb_first && sai->is_right_j_mode)
> >  		val_cr5 |= FSL_SAI_CR5_FBT(0);
> 
> Are you sure that FBT(0) is correct for right justified mode?
> Because the original code is using FBT(0) for the lsb_first
> situation and it shouldn't be right justified mode as default.
>
I am not sure about that. 

I assume lsb_first as big endian data.

For 16 bit data format, the 2 bytes data will in high address of 4 bytes
fifo. So the FBT is 16 for left-j and 0 for right-j. But big endian is
bytes convert not bits convert. It makes me confuse. And send to
community for help.

> Nicolin

Re: [alsa-devel][PATCH 3/3] ASoC: fsl_sai: Add support for Right-J mode

[Nicolin Chen]

On Thu, Jan 22, 2015 at 01:13:46PM +0800, Zidan Wang wrote:
> On Wed, Jan 21, 2015 at 10:53:20AM -0800, Nicolin Chen wrote:
> > On Tue, Jan 20, 2015 at 08:21:20PM +0800, Zidan Wang wrote:
> > > Add Right-J mode and set TCR5 FBT bit to let data right justify.
> > > 
> > > Signed-off-by: Zidan Wang <zidan.wang@freescale.com>
> > 
> > > -	if (sai->is_lsb_first)
> > > +	if (sai->is_lsb_first && sai->is_right_j_mode)
> > >  		val_cr5 |= FSL_SAI_CR5_FBT(0);
> > 
> > Are you sure that FBT(0) is correct for right justified mode?
> > Because the original code is using FBT(0) for the lsb_first
> > situation and it shouldn't be right justified mode as default.
> >
> I am not sure about that. 
> 
> I assume lsb_first as big endian data.

Correct.

> For 16 bit data format, the 2 bytes data will in high address of 4 bytes
> fifo. So the FBT is 16 for left-j and 0 for right-j. But big endian is
> bytes convert not bits convert. It makes me confuse. And send to
> community for help.

Either waiting for Xiubo to test it or asking IC owner for help.

Nicolin

Re: [alsa-devel] [PATCH 0/3] Add master mode, tmd and right-j mode support

[Fabio Estevam]

Hi Zidan,

On Tue, Jan 20, 2015 at 10:21 AM, Zidan Wang <zidan.wang@freescale.com> wrote:
> Add sai master mode support.
> Add tdm slots support.
> Add Right-J mode support.
>
> Zidan Wang (3):
>   SoC: fsl_sai: add sai master mode support
>   ASoC: fsl_sai: Add support for tdm slots operation
>   ASoC: fsl_sai: Add support for Right-J mode
>
>  sound/soc/fsl/fsl_sai.c | 164 ++++++++++++++++++++++++++++++++++++++++++++----
>  sound/soc/fsl/fsl_sai.h |  13 +++-
>  2 files changed, 164 insertions(+), 13 deletions(-)

Not related to your series, but do you have a patch for imx6sx-sdb.dts
that adds sai support?

In the FSL version they use 'fsl,sdma-event-remap' property to specify
the SAI sdma channels, but this is not present in mainline? How are
you dealing with it?

Re: [alsa-devel] [PATCH 0/3] Add master mode, tmd and right-j mode support

[Nicolin Chen]

On Tue, Jan 20, 2015 at 11:15:54PM -0200, Fabio Estevam wrote:

> Not related to your series, but do you have a patch for imx6sx-sdb.dts
> that adds sai support?
> 
> In the FSL version they use 'fsl,sdma-event-remap' property to specify
> the SAI sdma channels, but this is not present in mainline? How are
> you dealing with it?

I think he must have those patches applied to his own local branch
as I do :)

I was quite hesitating to send them because the SDMA one doesn't
look flawless, plus I don't know SDMA owner's plan for upstream.

Nicolin