asoc tlv320aic33: skip usage of PLL in some cases

PATCH In review Daniel Mack 3 participants 14 messages Apr 17, 2008 – Apr 19, 2008

Subsystems (3)

Sound ASoC Texas Instruments audio (ASoC/HDA) drivers

lore

Share & tools

lore

Daniel Mack

author

2008-04-17 19:12 UTC #0 lore raw

PATCH 1/1 asoc tlv320aic33: skip usage of PLL in some cases

This patch makes the tlv320aic33 driver skip the initialisation of the PLL in case the sysclk is 256 * samplerate. Had to do some minor refactoring too - the check whether a sysclk set by set_sysclk() is valid is now done in set_hw_params().

Signed-off-by Daniel Mack <[email protected]>

sound/soc/codecs/tlv320aic3x.c +37 ~~-31~~

index 630684f..1c96c36 100644
@@ -720,7 +720,7 @@ static inline int aic3x_get_divs(int mclk, int rate)
 			return i;
 	}
 
-	return 0;
+	return -1;
 }
 
 static int aic3x_hw_params(struct snd_pcm_substream *substream,

@@ -730,11 +730,40 @@ static int aic3x_hw_params(struct snd_pcm_substream *substream,
 	struct snd_soc_device *socdev = rtd->socdev;
 	struct snd_soc_codec *codec = socdev->codec;
 	struct aic3x_priv *aic3x = codec->private_data;
-	int i;
+	int i, skip_pll;
 	u8 data, pll_p, pll_r, pll_j;
 	u16 pll_d;
 
-	i = aic3x_get_divs(aic3x->sysclk, params_rate(params));
+	/* select data word length */
+	data =
+	    aic3x_read_reg_cache(codec, AIC3X_ASD_INTF_CTRLB) & (~(0x3 << 4));
+	switch (params_format(params)) {
+	case SNDRV_PCM_FORMAT_S16_LE:
+		break;
+	case SNDRV_PCM_FORMAT_S20_3LE:
+		data |= (0x01 << 4);
+		break;
+	case SNDRV_PCM_FORMAT_S24_LE:
+		data |= (0x02 << 4);
+		break;
+	case SNDRV_PCM_FORMAT_S32_LE:
+		data |= (0x03 << 4);
+		break;
+	}
+	aic3x_write(codec, AIC3X_ASD_INTF_CTRLB, data);
+
+	/* Do not use the PLL in case our sysclk is 256 * sample rate.
+	 * In this case, use a fake entry in the dividers table to get
+	 * the reference freq */
+	skip_pll = (aic3x->sysclk == 256 * params_rate(params));
+
+	if (skip_pll)
+		i = aic3x_get_divs(aic3x->sysclk, params_rate(params));
+	else
+		i = aic3x_get_divs(aic3x_divs[0].mclk, params_rate(params));
+
+	if (i < 0)
+		return -EINVAL;
 
 	/* Route Left DAC to left channel input and
 	 * right DAC to right channel input */

@@ -755,6 +784,9 @@ static int aic3x_hw_params(struct snd_pcm_substream *substream,
 	}
 	aic3x_write(codec, AIC3X_CODEC_DATAPATH_REG, data);
 
+	if (skip_pll)
+		return 0;
+
 	/* codec sample rate select */
 	data = aic3x_divs[i].sr_reg;
 	data |= (data << 4);

@@ -782,24 +814,6 @@ static int aic3x_hw_params(struct snd_pcm_substream *substream,
 	aic3x_write(codec, AIC3X_PLL_PROGD_REG,
 		    (pll_d & 0x3F) << PLLD_LSB_SHIFT);
 
-	/* select data word length */
-	data =
-	    aic3x_read_reg_cache(codec, AIC3X_ASD_INTF_CTRLB) & (~(0x3 << 4));
-	switch (params_format(params)) {
-	case SNDRV_PCM_FORMAT_S16_LE:
-		break;
-	case SNDRV_PCM_FORMAT_S20_3LE:
-		data |= (0x01 << 4);
-		break;
-	case SNDRV_PCM_FORMAT_S24_LE:
-		data |= (0x02 << 4);
-		break;
-	case SNDRV_PCM_FORMAT_S32_LE:
-		data |= (0x03 << 4);
-		break;
-	}
-	aic3x_write(codec, AIC3X_ASD_INTF_CTRLB, data);
-
 	return 0;
 }

@@ -826,16 +840,8 @@ static int aic3x_set_dai_sysclk(struct snd_soc_codec_dai *codec_dai,
 	struct snd_soc_codec *codec = codec_dai->codec;
 	struct aic3x_priv *aic3x = codec->private_data;
 
-	switch (freq) {
-	case 12000000:
-	case 19200000:
-	case 22579200:
-	case 33868800:
-		aic3x->sysclk = freq;
-		return 0;
-	}
-
-	return -EINVAL;
+	aic3x->sysclk = freq;
+	return 0;
 }
 
 static int aic3x_set_dai_fmt(struct snd_soc_codec_dai *codec_dai,

_______________________________________________
Alsa-devel mailing list
[email protected]
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Daniel Mack

author

2008-04-17 19:26 UTC #1 lore raw

Quoted 4 lines · Daniel Mack

This patch makes the tlv320aic33 driver skip the initialisation of the PLL in case the sysclk is 256 * samplerate. Had to do some minor refactoring too - the check whether a sysclk set by set_sysclk() is valid is now done in set_hw_params().

Sorry, there was a small typo in the patch I just submitted. Use this one instead.

Daniel

sound/soc/codecs/tlv320aic3x.c +37 ~~-31~~

index 630684f..7d3f109 100644
@@ -720,7 +720,7 @@ static inline int aic3x_get_divs(int mclk, int rate)
 			return i;
 	}
 
-	return 0;
+	return -1;
 }
 
 static int aic3x_hw_params(struct snd_pcm_substream *substream,

@@ -730,11 +730,40 @@ static int aic3x_hw_params(struct snd_pcm_substream *substream,
 	struct snd_soc_device *socdev = rtd->socdev;
 	struct snd_soc_codec *codec = socdev->codec;
 	struct aic3x_priv *aic3x = codec->private_data;
-	int i;
+	int i, skip_pll;
 	u8 data, pll_p, pll_r, pll_j;
 	u16 pll_d;
 
-	i = aic3x_get_divs(aic3x->sysclk, params_rate(params));
+	/* select data word length */
+	data =
+	    aic3x_read_reg_cache(codec, AIC3X_ASD_INTF_CTRLB) & (~(0x3 << 4));
+	switch (params_format(params)) {
+	case SNDRV_PCM_FORMAT_S16_LE:
+		break;
+	case SNDRV_PCM_FORMAT_S20_3LE:
+		data |= (0x01 << 4);
+		break;
+	case SNDRV_PCM_FORMAT_S24_LE:
+		data |= (0x02 << 4);
+		break;
+	case SNDRV_PCM_FORMAT_S32_LE:
+		data |= (0x03 << 4);
+		break;
+	}
+	aic3x_write(codec, AIC3X_ASD_INTF_CTRLB, data);
+
+	/* Do not use the PLL in case our sysclk is 256 * sample rate.
+	 * In this case, use a fake entry in the dividers table to get
+	 * the reference freq */
+	skip_pll = (aic3x->sysclk == 256 * params_rate(params));
+
+	if (skip_pll)
+		i = aic3x_get_divs(aic3x_divs[0].mclk, params_rate(params));
+	else
+		i = aic3x_get_divs(aic3x->sysclk, params_rate(params));
+
+	if (i < 0)
+		return -EINVAL;
 
 	/* Route Left DAC to left channel input and
 	 * right DAC to right channel input */

@@ -755,6 +784,9 @@ static int aic3x_hw_params(struct snd_pcm_substream *substream,
 	}
 	aic3x_write(codec, AIC3X_CODEC_DATAPATH_REG, data);
 
+	if (skip_pll)
+		return 0;
+
 	/* codec sample rate select */
 	data = aic3x_divs[i].sr_reg;
 	data |= (data << 4);

@@ -782,24 +814,6 @@ static int aic3x_hw_params(struct snd_pcm_substream *substream,
 	aic3x_write(codec, AIC3X_PLL_PROGD_REG,
 		    (pll_d & 0x3F) << PLLD_LSB_SHIFT);
 
-	/* select data word length */
-	data =
-	    aic3x_read_reg_cache(codec, AIC3X_ASD_INTF_CTRLB) & (~(0x3 << 4));
-	switch (params_format(params)) {
-	case SNDRV_PCM_FORMAT_S16_LE:
-		break;
-	case SNDRV_PCM_FORMAT_S20_3LE:
-		data |= (0x01 << 4);
-		break;
-	case SNDRV_PCM_FORMAT_S24_LE:
-		data |= (0x02 << 4);
-		break;
-	case SNDRV_PCM_FORMAT_S32_LE:
-		data |= (0x03 << 4);
-		break;
-	}
-	aic3x_write(codec, AIC3X_ASD_INTF_CTRLB, data);
-
 	return 0;
 }

@@ -826,16 +840,8 @@ static int aic3x_set_dai_sysclk(struct snd_soc_codec_dai *codec_dai,
 	struct snd_soc_codec *codec = codec_dai->codec;
 	struct aic3x_priv *aic3x = codec->private_data;
 
-	switch (freq) {
-	case 12000000:
-	case 19200000:
-	case 22579200:
-	case 33868800:
-		aic3x->sysclk = freq;
-		return 0;
-	}
-
-	return -EINVAL;
+	aic3x->sysclk = freq;
+	return 0;
 }
 
 static int aic3x_set_dai_fmt(struct snd_soc_codec_dai *codec_dai,

_______________________________________________
Alsa-devel mailing list
[email protected]
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Jarkko Nikula

2008-04-18 07:59 UTC #2 lore raw

2008/4/17 Daniel Mack <[email protected]>:

Quoted 10 lines · Daniel Mack

Quoted 4 lines
This patch makes the tlv320aic33 driver skip the initialisation of the PLL in case the sysclk is 256 * samplerate. Had to do some minor refactoring too - the check whether a sysclk set by set_sysclk() is valid is now done in set_hw_params().
Sorry, there was a small typo in the patch I just submitted. Use this one instead.

I had a quick look to your patch and AIC33 spec.

Is this the same than 256-clock transfer mode? Should you set the bit 3 in AIC3X_ASD_INTF_CTRLB in this case? Should you also still write the AIC3X_SAMPLE_RATE_SEL_REG?

Jarkko

Daniel Mack

author

2008-04-18 08:13 UTC #3 lore raw

Hi Jarkko,

Quoted 3 lines · Jarkko Nikula

I had a quick look to your patch and AIC33 spec.
Is this the same than 256-clock transfer mode?

No, the 256-clock mode is for output only, while in my setup the TLV is in slave mode. I attached this chip to the I2S output of an PXA270 which always outputs sample rate * 256 as system clock. In this very case, the PLL can be bypassed by selecting the left path described on page 27.

Quoted 3 lines

Should you set the bit 3 in
AIC3X_ASD_INTF_CTRLB in this case? Should you also still write the AIC3X_SAMPLE_RATE_SEL_REG?

AIC3X_SAMPLE_RATE_SEL_REG defaults to 0 which is what I want in this case. Thus, I don't have to write it.

Daniel

Jarkko Nikula

2008-04-18 08:58 UTC #4 lore raw

Quoted 7 lines · Daniel Mack

Hi Jarkko,
No, the 256-clock mode is for output only, while in my setup the TLV is in slave mode. I attached this chip to the I2S output of an PXA270 which always outputs sample rate * 256 as system clock. In this very case, the PLL can be bypassed by selecting the left path described on page 27.

Ok, now I see. Probably you should refer it as, at least in comment, 128*Q instead of 256 eventhough the driver is not currently touching the Q value in AIC3X_PLL_PROGA_REG.

Quoted 4 lines

AIC3X_SAMPLE_RATE_SEL_REG defaults to 0 which is what I want in this case. Thus, I don't have to write it.

Are you sure this is a general case?

Jarkko

Daniel Mack

author

2008-04-18 09:38 UTC #5 lore raw

Quoted 9 lines · Jarkko Nikula

Quoted 5 lines
No, the 256-clock mode is for output only, while in my setup the TLV is in slave mode. I attached this chip to the I2S output of an PXA270 which always outputs sample rate * 256 as system clock. In this very case, the PLL can be bypassed by selecting the left path described on page 27.
Ok, now I see. Probably you should refer it as, at least in comment, 128*Q instead of 256 eventhough the driver is not currently touching the Q value in AIC3X_PLL_PROGA_REG.

Well, the constraint for that condition is that MCLK = 256*WCLK, and the reason why that works for the chip without PLL is that Q=2. I stated that a little better in the attached patch.

Quoted 5 lines

Quoted 4 lines
AIC3X_SAMPLE_RATE_SEL_REG defaults to 0 which is what I want in this case. Thus, I don't have to write it.
Are you sure this is a general case?

Well, the reset default is 0 (as stated on page 44) and set_hw_params() is the only location where this value is written. So if it's never written with a different value, it should always be set to its default, no?

Daniel

sound/soc/codecs/tlv320aic3x.c +39 ~~-31~~

index 630684f..0f0bda2 100644
@@ -720,7 +720,7 @@ static inline int aic3x_get_divs(int mclk, int rate)
 			return i;
 	}
 
-	return 0;
+	return -1;
 }
 
 static int aic3x_hw_params(struct snd_pcm_substream *substream,

@@ -730,11 +730,42 @@ static int aic3x_hw_params(struct snd_pcm_substream *substream,
 	struct snd_soc_device *socdev = rtd->socdev;
 	struct snd_soc_codec *codec = socdev->codec;
 	struct aic3x_priv *aic3x = codec->private_data;
-	int i;
+	int i, skip_pll;
 	u8 data, pll_p, pll_r, pll_j;
 	u16 pll_d;
 
-	i = aic3x_get_divs(aic3x->sysclk, params_rate(params));
+	/* select data word length */
+	data =
+	    aic3x_read_reg_cache(codec, AIC3X_ASD_INTF_CTRLB) & (~(0x3 << 4));
+	switch (params_format(params)) {
+	case SNDRV_PCM_FORMAT_S16_LE:
+		break;
+	case SNDRV_PCM_FORMAT_S20_3LE:
+		data |= (0x01 << 4);
+		break;
+	case SNDRV_PCM_FORMAT_S24_LE:
+		data |= (0x02 << 4);
+		break;
+	case SNDRV_PCM_FORMAT_S32_LE:
+		data |= (0x03 << 4);
+		break;
+	}
+	aic3x_write(codec, AIC3X_ASD_INTF_CTRLB, data);
+
+	/* Do not use the PLL in case our MCLK is 256 * sample rate.
+	 * The Q value defaults to 2 so the codec sees MCLK as clock
+	 * input (refer to datasheet page 27).
+	 * In this case, use a fake entry in the dividers table to get
+	 * the reference freq */
+	skip_pll = (aic3x->sysclk == params_rate(params) * 256);
+
+	if (skip_pll)
+		i = aic3x_get_divs(aic3x_divs[0].mclk, params_rate(params));
+	else
+		i = aic3x_get_divs(aic3x->sysclk, params_rate(params));
+
+	if (i < 0)
+		return -EINVAL;
 
 	/* Route Left DAC to left channel input and
 	 * right DAC to right channel input */

@@ -755,6 +786,9 @@ static int aic3x_hw_params(struct snd_pcm_substream *substream,
 	}
 	aic3x_write(codec, AIC3X_CODEC_DATAPATH_REG, data);
 
+	if (skip_pll)
+		return 0;
+
 	/* codec sample rate select */
 	data = aic3x_divs[i].sr_reg;
 	data |= (data << 4);

@@ -782,24 +816,6 @@ static int aic3x_hw_params(struct snd_pcm_substream *substream,
 	aic3x_write(codec, AIC3X_PLL_PROGD_REG,
 		    (pll_d & 0x3F) << PLLD_LSB_SHIFT);
 
-	/* select data word length */
-	data =
-	    aic3x_read_reg_cache(codec, AIC3X_ASD_INTF_CTRLB) & (~(0x3 << 4));
-	switch (params_format(params)) {
-	case SNDRV_PCM_FORMAT_S16_LE:
-		break;
-	case SNDRV_PCM_FORMAT_S20_3LE:
-		data |= (0x01 << 4);
-		break;
-	case SNDRV_PCM_FORMAT_S24_LE:
-		data |= (0x02 << 4);
-		break;
-	case SNDRV_PCM_FORMAT_S32_LE:
-		data |= (0x03 << 4);
-		break;
-	}
-	aic3x_write(codec, AIC3X_ASD_INTF_CTRLB, data);
-
 	return 0;
 }

@@ -826,16 +842,8 @@ static int aic3x_set_dai_sysclk(struct snd_soc_codec_dai *codec_dai,
 	struct snd_soc_codec *codec = codec_dai->codec;
 	struct aic3x_priv *aic3x = codec->private_data;
 
-	switch (freq) {
-	case 12000000:
-	case 19200000:
-	case 22579200:
-	case 33868800:
-		aic3x->sysclk = freq;
-		return 0;
-	}
-
-	return -EINVAL;
+	aic3x->sysclk = freq;
+	return 0;
 }
 
 static int aic3x_set_dai_fmt(struct snd_soc_codec_dai *codec_dai,

_______________________________________________
Alsa-devel mailing list
[email protected]
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Mark Brown

2008-04-18 10:09 UTC #6 lore raw

Quoted 11 lines · Daniel Mack

Quoted 4 lines · Jarkko Nikula
Quoted 2 lines
AIC3X_SAMPLE_RATE_SEL_REG defaults to 0 which is what I want in this case. Thus, I don't have to write it.
Are you sure this is a general case?
Well, the reset default is 0 (as stated on page 44) and set_hw_params() is the only location where this value is written. So if it's never written with a different value, it should always be set to its default, no?

Not all systems use a static SYSCLK rate - sometimes systems will select the rate based on their current usage. If the system has previously had to configure it then the register won't be at the default value any more.

Daniel Mack

author

2008-04-18 10:35 UTC #7 lore raw

Quoted 8 lines · Mark Brown

Quoted 4 lines
Well, the reset default is 0 (as stated on page 44) and set_hw_params() is the only location where this value is written. So if it's never written with a different value, it should always be set to its default, no?
Not all systems use a static SYSCLK rate - sometimes systems will select the rate based on their current usage. If the system has previously had to configure it then the register won't be at the default value any more.

Ok, I didn't consider that. New version of the patch attached which resets AIC3X_PLL_PROGA_REG and AIC3X_SAMPLE_RATE_SEL_REG to appropriate values in case the PLL setup is skipped.

Daniel

sound/soc/codecs/tlv320aic3x.c +41 ~~-31~~

index 630684f..83a87f6 100644
@@ -720,7 +720,7 @@ static inline int aic3x_get_divs(int mclk, int rate)
 			return i;
 	}
 
-	return 0;
+	return -1;
 }
 
 static int aic3x_hw_params(struct snd_pcm_substream *substream,

@@ -730,11 +730,44 @@ static int aic3x_hw_params(struct snd_pcm_substream *substream,
 	struct snd_soc_device *socdev = rtd->socdev;
 	struct snd_soc_codec *codec = socdev->codec;
 	struct aic3x_priv *aic3x = codec->private_data;
-	int i;
+	int i, skip_pll;
 	u8 data, pll_p, pll_r, pll_j;
 	u16 pll_d;
 
-	i = aic3x_get_divs(aic3x->sysclk, params_rate(params));
+	/* select data word length */
+	data =
+	    aic3x_read_reg_cache(codec, AIC3X_ASD_INTF_CTRLB) & (~(0x3 << 4));
+	switch (params_format(params)) {
+	case SNDRV_PCM_FORMAT_S16_LE:
+		break;
+	case SNDRV_PCM_FORMAT_S20_3LE:
+		data |= (0x01 << 4);
+		break;
+	case SNDRV_PCM_FORMAT_S24_LE:
+		data |= (0x02 << 4);
+		break;
+	case SNDRV_PCM_FORMAT_S32_LE:
+		data |= (0x03 << 4);
+		break;
+	}
+	aic3x_write(codec, AIC3X_ASD_INTF_CTRLB, data);
+
+	/* Do not use the PLL in case our MCLK is 256 * sample rate.
+	 * The Q value is set to 2 so the codec sees MCLK as clock
+	 * input (refer to datasheet page 27).
+	 * In this case, use a fake entry in the dividers table to get
+	 * the reference freq */
+	skip_pll = (aic3x->sysclk == params_rate(params) * 256);
+
+	if (skip_pll) {
+		i = aic3x_get_divs(aic3x_divs[0].mclk, params_rate(params));
+		aic3x_write(codec, AIC3X_SAMPLE_RATE_SEL_REG, 0);
+		aic3x_write(codec, AIC3X_PLL_PROGA_REG, 2 << PLLQ_SHIFT);
+	} else
+		i = aic3x_get_divs(aic3x->sysclk, params_rate(params));
+
+	if (i < 0)
+		return -EINVAL;
 
 	/* Route Left DAC to left channel input and
 	 * right DAC to right channel input */

@@ -755,6 +788,9 @@ static int aic3x_hw_params(struct snd_pcm_substream *substream,
 	}
 	aic3x_write(codec, AIC3X_CODEC_DATAPATH_REG, data);
 
+	if (skip_pll)
+		return 0;
+
 	/* codec sample rate select */
 	data = aic3x_divs[i].sr_reg;
 	data |= (data << 4);

@@ -782,24 +818,6 @@ static int aic3x_hw_params(struct snd_pcm_substream *substream,
 	aic3x_write(codec, AIC3X_PLL_PROGD_REG,
 		    (pll_d & 0x3F) << PLLD_LSB_SHIFT);
 
-	/* select data word length */
-	data =
-	    aic3x_read_reg_cache(codec, AIC3X_ASD_INTF_CTRLB) & (~(0x3 << 4));
-	switch (params_format(params)) {
-	case SNDRV_PCM_FORMAT_S16_LE:
-		break;
-	case SNDRV_PCM_FORMAT_S20_3LE:
-		data |= (0x01 << 4);
-		break;
-	case SNDRV_PCM_FORMAT_S24_LE:
-		data |= (0x02 << 4);
-		break;
-	case SNDRV_PCM_FORMAT_S32_LE:
-		data |= (0x03 << 4);
-		break;
-	}
-	aic3x_write(codec, AIC3X_ASD_INTF_CTRLB, data);
-
 	return 0;
 }

@@ -826,16 +844,8 @@ static int aic3x_set_dai_sysclk(struct snd_soc_codec_dai *codec_dai,
 	struct snd_soc_codec *codec = codec_dai->codec;
 	struct aic3x_priv *aic3x = codec->private_data;
 
-	switch (freq) {
-	case 12000000:
-	case 19200000:
-	case 22579200:
-	case 33868800:
-		aic3x->sysclk = freq;
-		return 0;
-	}
-
-	return -EINVAL;
+	aic3x->sysclk = freq;
+	return 0;
 }
 
 static int aic3x_set_dai_fmt(struct snd_soc_codec_dai *codec_dai,

sound/soc/codecs/tlv320aic3x.h +2 -0

index d0cdeeb..e310374 100644
@@ -108,6 +108,7 @@
 #define DACR1_2_RLOPM_VOL		92
 #define LLOPM_CTRL			86
 #define RLOPM_CTRL			93
+
 /* Clock generation control register */
 #define AIC3X_CLKGEN_CTRL_REG		102

@@ -128,6 +129,7 @@
 
 /* PLL registers bitfields */
 #define PLLP_SHIFT		0
+#define PLLQ_SHIFT		3
 #define PLLR_SHIFT		0
 #define PLLJ_SHIFT		2
 #define PLLD_MSB_SHIFT		0

_______________________________________________
Alsa-devel mailing list
[email protected]
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Jarkko Nikula

2008-04-18 10:47 UTC #8 lore raw

In reply to Daniel Mack

Quoted 11 lines · Daniel Mack

Quoted 1 line · Jarkko Nikula
Ok, now I see. Probably you should refer it as, at least in comment,
128*Q
Quoted 1 line
instead of 256 eventhough the driver is not currently touching the Q
value
Quoted 1 line
in AIC3X_PLL_PROGA_REG.
Well, the constraint for that condition is that MCLK = 256*WCLK, and the reason why that works for the chip without PLL is that Q=2. I stated that a little better in the attached patch.

I would rather refer there just "Fsref = CLKDIV_IN / (128*Q)" as the condition where PLL can be disabled (or is mandatory where it must be disabled?).

Referring to data sheet page 27 is not good as it's correct only for certain version of AIC33 spec and driver supports other AIC3x chips as well :-)

Quoted 9 lines

Quoted 2 lines
Quoted 1 line
Are you sure this is a general case?
Well, the reset default is 0 (as stated on page 44) and set_hw_params() is the only location where this value is written. So if it's never written with a different value, it should always be set to its default, no?

I mean even if the I2S interface Fs meets the MCLK/256 condition, the audio sample rate may be lower and it may be still required to set ADC and DAC rates.

Jarkko

Daniel Mack

author

2008-04-18 11:41 UTC #9 lore raw

Quoted 11 lines · Jarkko Nikula

Quoted 4 lines
Well, the constraint for that condition is that MCLK = 256*WCLK, and the reason why that works for the chip without PLL is that Q=2. I stated that a little better in the attached patch.
I would rather refer there just "Fsref = CLKDIV_IN / (128*Q)" as the condition where PLL can be disabled (or is mandatory where it must be disabled?).
Referring to data sheet page 27 is not good as it's correct only for certain version of AIC33 spec and driver supports other AIC3x chips as well :-)

Ok, I agree. I changed that to find an appropriate value for Q programmatically. Have a look at the attached patch, please. I hope i finally got it now ;)

Daniel

Subject: [PATCH] asoc tlv320aic33: skip usage of PLL in some cases

Try to find an appropriate value for Q during clock setup in order to bypass the usage of the PLL in some cases. This saves some entries in the dividers table.

Signed-off-by Daniel Mack <[email protected]>

sound/soc/codecs/tlv320aic3x.c +50 ~~-32~~

index 630684f..4ddfe91 100644
@@ -720,7 +720,7 @@ static inline int aic3x_get_divs(int mclk, int rate)
 			return i;
 	}
 
-	return 0;
+	return -1;
 }
 
 static int aic3x_hw_params(struct snd_pcm_substream *substream,

@@ -730,11 +730,52 @@ static int aic3x_hw_params(struct snd_pcm_substream *substream,
 	struct snd_soc_device *socdev = rtd->socdev;
 	struct snd_soc_codec *codec = socdev->codec;
 	struct aic3x_priv *aic3x = codec->private_data;
-	int i;
-	u8 data, pll_p, pll_r, pll_j;
+	int i, bypass_pll = 0;
+	u8 data, pll_p, pll_r, pll_j, pll_q;
 	u16 pll_d;
 
-	i = aic3x_get_divs(aic3x->sysclk, params_rate(params));
+	/* select data word length */
+	data =
+	    aic3x_read_reg_cache(codec, AIC3X_ASD_INTF_CTRLB) & (~(0x3 << 4));
+	switch (params_format(params)) {
+	case SNDRV_PCM_FORMAT_S16_LE:
+		break;
+	case SNDRV_PCM_FORMAT_S20_3LE:
+		data |= (0x01 << 4);
+		break;
+	case SNDRV_PCM_FORMAT_S24_LE:
+		data |= (0x02 << 4);
+		break;
+	case SNDRV_PCM_FORMAT_S32_LE:
+		data |= (0x03 << 4);
+		break;
+	}
+	aic3x_write(codec, AIC3X_ASD_INTF_CTRLB, data);
+
+	/* Try to find a value for Q which allows us to bypass the PLL and
+	 * generate CODEC_CLK directly. This saves some entries in
+	 * aic3x_divs[]. */
+	for (pll_q = 2; pll_q < 18; pll_q++)
+		if (params_rate(params) == aic3x->sysclk / (128 * pll_q)) {
+			bypass_pll = 1;
+			break;
+		}
+
+	 /* If the PLL is bypassed, use first entry in the dividers table
+	 * to lookup fsref. */
+	if (bypass_pll) {
+		pll_q &= 0xf;
+		i = aic3x_get_divs(aic3x_divs[0].mclk, params_rate(params));
+		aic3x_write(codec, AIC3X_SAMPLE_RATE_SEL_REG, 0);
+		aic3x_write(codec, AIC3X_PLL_PROGA_REG, pll_q << PLLQ_SHIFT);
+		aic3x_write(codec, AIC3X_GPIOB_REG, CODEC_CLKIN_CLKDIV);
+	} else {
+		i = aic3x_get_divs(aic3x->sysclk, params_rate(params));
+		aic3x_write(codec, AIC3X_GPIOB_REG, CODEC_CLKIN_PLLDIV);
+	}
+
+	if (i < 0)
+		return -EINVAL;
 
 	/* Route Left DAC to left channel input and
 	 * right DAC to right channel input */

@@ -755,6 +796,9 @@ static int aic3x_hw_params(struct snd_pcm_substream *substream,
 	}
 	aic3x_write(codec, AIC3X_CODEC_DATAPATH_REG, data);
 
+	if (bypass_pll)
+		return 0;
+
 	/* codec sample rate select */
 	data = aic3x_divs[i].sr_reg;
 	data |= (data << 4);

@@ -782,24 +826,6 @@ static int aic3x_hw_params(struct snd_pcm_substream *substream,
 	aic3x_write(codec, AIC3X_PLL_PROGD_REG,
 		    (pll_d & 0x3F) << PLLD_LSB_SHIFT);
 
-	/* select data word length */
-	data =
-	    aic3x_read_reg_cache(codec, AIC3X_ASD_INTF_CTRLB) & (~(0x3 << 4));
-	switch (params_format(params)) {
-	case SNDRV_PCM_FORMAT_S16_LE:
-		break;
-	case SNDRV_PCM_FORMAT_S20_3LE:
-		data |= (0x01 << 4);
-		break;
-	case SNDRV_PCM_FORMAT_S24_LE:
-		data |= (0x02 << 4);
-		break;
-	case SNDRV_PCM_FORMAT_S32_LE:
-		data |= (0x03 << 4);
-		break;
-	}
-	aic3x_write(codec, AIC3X_ASD_INTF_CTRLB, data);
-
 	return 0;
 }

@@ -826,16 +852,8 @@ static int aic3x_set_dai_sysclk(struct snd_soc_codec_dai *codec_dai,
 	struct snd_soc_codec *codec = codec_dai->codec;
 	struct aic3x_priv *aic3x = codec->private_data;
 
-	switch (freq) {
-	case 12000000:
-	case 19200000:
-	case 22579200:
-	case 33868800:
-		aic3x->sysclk = freq;
-		return 0;
-	}
-
-	return -EINVAL;
+	aic3x->sysclk = freq;
+	return 0;
 }
 
 static int aic3x_set_dai_fmt(struct snd_soc_codec_dai *codec_dai,

sound/soc/codecs/tlv320aic3x.h +4 -0

index d0cdeeb..d49d001 100644
@@ -109,6 +109,7 @@
 #define LLOPM_CTRL			86
 #define RLOPM_CTRL			93
 /* Clock generation control register */
+#define AIC3X_GPIOB_REG			101
 #define AIC3X_CLKGEN_CTRL_REG		102
 
 /* Page select register bits */

@@ -128,12 +129,15 @@
 
 /* PLL registers bitfields */
 #define PLLP_SHIFT		0
+#define PLLQ_SHIFT		3
 #define PLLR_SHIFT		0
 #define PLLJ_SHIFT		2
 #define PLLD_MSB_SHIFT		0
 #define PLLD_LSB_SHIFT		2
 
 /* Clock generation register bits */
+#define CODEC_CLKIN_PLLDIV	0
+#define CODEC_CLKIN_CLKDIV	1
 #define PLL_CLKIN_SHIFT		4
 #define MCLK_SOURCE		0x0
 #define PLL_CLKDIV_SHIFT	0

_______________________________________________
Alsa-devel mailing list
[email protected]
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Jarkko Nikula

2008-04-18 13:47 UTC #10 lore raw

Quoted 4 lines · Daniel Mack

Ok, I agree. I changed that to find an appropriate value for Q programmatically. Have a look at the attached patch, please. I hope i finally got it now ;)

That sounds a good idea. Then more cases will be covered.

What I noticed that instead of params_rate, I think we should compare here the FSref of 44.1 and 48 kHz (how about dual-rate mode?) when defining can the PLL be bypassed

if (params_rate(params) == aic3x->sysclk / (128 * pll_q))

---

Probably you forgot to move bypass case in this version after writing the AIC3X_SAMPLE_RATE_SEL_REG?

Spec is also saying that when NDAC is 1.5, 2.5, ... 5.5, then odd values of Q are not allowed.

Not the easiest chip... probably worth to ask from TI what's the case e.g. when FSref is 2*48 kHz and ADC/DAC rate is 64 kHz (NDAC is 1.5).

And of course, over-designing the driver is not the purpose and probably some special cases can be now covered just with -EINVAL and let the user who needs them to send a patch :-)

Jarkko

Daniel Mack

author

2008-04-18 14:03 UTC #11 lore raw

Quoted 5 lines · Jarkko Nikula

Quoted 3 lines
Ok, I agree. I changed that to find an appropriate value for Q programmatically. Have a look at the attached patch, please. I hope i finally got it now ;)

Quoted 7 lines

That sounds a good idea. Then more cases will be covered.
What I noticed that instead of params_rate, I think we should compare here the FSref of 44.1 and 48 kHz (how about dual-rate mode?) when defining can the PLL be bypassed
if (params_rate(params) == aic3x->sysclk / (128 * pll_q))

Hmm? Why do you think so? I'm afraid I don't get your point here.

Quoted 2 lines

Probably you forgot to move bypass case in this version after writing the AIC3X_SAMPLE_RATE_SEL_REG?

No, actually not.

Quoted 2 lines

Spec is also saying that when NDAC is 1.5, 2.5, ... 5.5, then odd values of Q are not allowed.

NDAC and NADC are both hard-coded to 0 (divider of 1) at the moment. Support for more flexible settings could be done in another step. Also, the PLL setup could be done by calculation of values rather that by a lookup table.

I'd like to see this patch applied now as base for further refinements. Do you agree?

Daniel

Jarkko Nikula

2008-04-18 19:37 UTC #12 lore raw

Quoted 5 lines · Daniel Mack

Quoted 1 line · Jarkko Nikula
if (params_rate(params) == aic3x->sysclk / (128 * pll_q))
Hmm? Why do you think so? I'm afraid I don't get your point here.

I'm might be completely wrong here since I've used this codec only in configuration where the interface rate or FSref equals to audio sampling rate , but as far as I've interpreted the spec correctly we can have e.g. the case where MCLK is 22.5792 MHz, FSref 44.1 kHz (i.e. PLL can be disabled) but audio sampling rate is 11.025 kHz (== params_rate(params)).

And I think here equation "Fsref = CLKDIV_IN / (128 × Q)" Fsref refers to 44.1 or 48 kHz not the audio sampling rate?

But I don't want to speculate or do my own interpretation here. I'll ask this from TI next week from my [email protected] and can put you as a cc as well.

Or do the driver author have any better knowledge here?

Quoted 6 lines

Quoted 1 line
Probably you forgot to move bypass case in this version after writing
the
Quoted 1 line
AIC3X_SAMPLE_RATE_SEL_REG?
No, actually not.

Your latest patch has:

+ if (bypass_pll)
+ return 0;
+

     /* codec sample rate select */
     data = aic3x_divs[i].sr_reg;
     data |= (data << 4);

Quoted 9 lines

Quoted 2 lines
Spec is also saying that when NDAC is 1.5, 2.5, ... 5.5, then odd values
of
Quoted 1 line
Q are not allowed.
NDAC and NADC are both hard-coded to 0 (divider of 1) at the moment. Support for more flexible settings could be done in another step. Also, the PLL setup could be done by calculation of values rather that by a lookup table.

Again, I'm might be wrong but I NDAC and NADC refers to register AIC3X_SAMPLE_RATE_SEL_REG which driver already takes into account properly.

Quoted 4 lines

I'd like to see this patch applied now as base for further refinements. Do you agree?

I cannot prevent it to be applied, that's the driver author or maintainers task to justify. I can only throw some discussion here if I see something to be unclear (to me).

And don't take my comments as a nitpicking. Definitely patch like this is worth to get in but we have to make sure that no possible bugs are introduced. -EINVAL is much better alternative for some new configurations.

Jarkko

Daniel Mack

author

2008-04-19 00:51 UTC #13 lore raw

Quoted 5 lines · Jarkko Nikula

I'm might be completely wrong here since I've used this codec only in configuration where the interface rate or FSref equals to audio sampling rate , but as far as I've interpreted the spec correctly we can have e.g. the case where MCLK is 22.5792 MHz, FSref 44.1 kHz (i.e. PLL can be disabled) but audio sampling rate is 11.025 kHz (== params_rate(params)).

Of course. Fsref is a setting for the filter to tell it which internal base-frequency it is operating on. But the setup of the sampling frequency has different meaning. Also, the clocking diagram (page 27 in the aic33 manual) doesn't even show that entity, or did I miss anything after staring at this for some days? ;)

Quoted 3 lines

But I don't want to speculate or do my own interpretation here. I'll ask this from TI next week from my [email protected] and can put you as a cc as well.

Ok, thanks :)

sound/soc/codecs/tlv320aic3x.c static int aic3x_hw_params(struct snd_pcm_substream *substream, · line 801

Quoted 5 lines

Your latest patch has:
sound/soc/codecs/tlv320aic3x.c +3 -0
+    if (bypass_pll)
+        return 0;
+

Yes, since all other setup can be skipped in case the PLL is bypassed. Did you miss the question here? ;)

Quoted 2 lines

Again, I'm might be wrong but I NDAC and NADC refers to register AIC3X_SAMPLE_RATE_SEL_REG which driver already takes into account properly.

Only in case the PLL is enabled. I agree that there could be some more setup around all the possible settings in this clock generation, but I'd my approach as a good first start.

Quoted 3 lines

And don't take my comments as a nitpicking. Definitely patch like this is worth to get in but we have to make sure that no possible bugs are introduced. -EINVAL is much better alternative for some new configurations.

No, I don't take it as nitpicking :) I agree that a driver for such a complex chip has to be modified with care to have any regression. But I don't see such as far, so I'd take this patch as a step towards a better solution. Unless someone comes up claiming that it's breaking anything.

Thanks and best rgeards,
Daniel