Skip to contentSkip to conversation

ASoC: davinci-mcasp: set up user bits for S/PDIF mode

PATCH Superseded Daniel Mack 3 participants 6 messages Mar 26, 2014 – Mar 27, 2014

Subsystems (2)
Sound ASoC

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Daniel Mack

author
#0 lore raw

PATCH 1/1 ASoC: davinci-mcasp: set up user bits for S/PDIF mode

In DIT (S/PDIF) mode, program the transmitted user bits to reflect the configured sample rate, along with some other details.

Signed-off-by Daniel Mack <[email protected]>
---
 sound/soc/davinci/davinci-mcasp.c | 55 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 53 insertions(+), 2 deletions(-)
sound/soc/davinci/davinci-mcasp.c +53 -2
index 712a7cd..ec0463a 100644
@@ -27,6 +27,7 @@
 #include <linux/of_platform.h>
 #include <linux/of_device.h>
 
+#include <sound/asoundef.h>
 #include <sound/core.h>
 #include <sound/pcm.h>
 #include <sound/pcm_params.h>

@@ -566,8 +567,11 @@ static int mcasp_i2s_hw_param(struct davinci_mcasp *mcasp, int stream)
 }
 
 /* S/PDIF */
-static int mcasp_dit_hw_param(struct davinci_mcasp *mcasp)
+static int mcasp_dit_hw_param(struct davinci_mcasp *mcasp,
+			      unsigned int rate)
 {
+	u32 val = 0;
+
 	/* Set the TX format : 24 bit right rotation, 32 bit slot, Pad 0
 	   and LSB first */
 	mcasp_set_bits(mcasp, DAVINCI_MCASP_TXFMT_REG, TXROT(6) | TXSSZ(15));

@@ -589,6 +593,53 @@ static int mcasp_dit_hw_param(struct davinci_mcasp *mcasp)
 	/* Enable the DIT */
 	mcasp_set_bits(mcasp, DAVINCI_MCASP_TXDITCTL_REG, DITEN);
 
+	/* Set S/PDIF channel status bits */
+	mcasp_set_reg(mcasp, DAVINCI_MCASP_DITCSRA_REG + 0,
+		      IEC958_AES0_CON_NOT_COPYRIGHT);
+	mcasp_set_reg(mcasp, DAVINCI_MCASP_DITCSRB_REG + 0,
+		      IEC958_AES0_CON_NOT_COPYRIGHT);
+
+	mcasp_set_reg(mcasp, DAVINCI_MCASP_DITCSRA_REG + 1,
+		      IEC958_AES1_CON_PCM_CODER);
+	mcasp_set_reg(mcasp, DAVINCI_MCASP_DITCSRB_REG + 1,
+		      IEC958_AES1_CON_PCM_CODER);
+
+	switch (rate) {
+	case 22050:
+		val |= IEC958_AES3_CON_FS_22050;
+		break;
+	case 24000:
+		val |= IEC958_AES3_CON_FS_24000;
+		break;
+	case 32000:
+		val |= IEC958_AES3_CON_FS_32000;
+		break;
+	case 44100:
+		val |= IEC958_AES3_CON_FS_44100;
+		break;
+	case 48000:
+		val |= IEC958_AES3_CON_FS_48000;
+		break;
+	case 88200:
+		val |= IEC958_AES3_CON_FS_88200;
+		break;
+	case 96000:
+		val |= IEC958_AES3_CON_FS_96000;
+		break;
+	case 176400:
+		val |= IEC958_AES3_CON_FS_176400;
+		break;
+	case 192000:
+		val |= IEC958_AES3_CON_FS_192000;
+		break;
+	default:
+		printk(KERN_WARNING "unsupported sampling rate: %d\n", rate);
+		return -EINVAL;
+	}
+
+	mcasp_set_reg(mcasp, DAVINCI_MCASP_DITCSRA_REG + 3, val);
+	mcasp_set_reg(mcasp, DAVINCI_MCASP_DITCSRB_REG + 3, val);
+
 	return 0;
 }
 

@@ -621,7 +672,7 @@ static int davinci_mcasp_hw_params(struct snd_pcm_substream *substream,
 		fifo_level = mcasp->rxnumevt * active_serializers;
 
 	if (mcasp->op_mode == DAVINCI_MCASP_DIT_MODE)
-		ret = mcasp_dit_hw_param(mcasp);
+		ret = mcasp_dit_hw_param(mcasp, params_rate(params));
 	else
 		ret = mcasp_i2s_hw_param(mcasp, substream->stream);
Signature · 1 line
1.8.5.3
#1 lore raw
Quoted 34 lines · Daniel Mack

In DIT (S/PDIF) mode, program the transmitted user bits to reflect the configured sample rate, along with some other details.

Signed-off-by Daniel Mack <[email protected]>
---
 sound/soc/davinci/davinci-mcasp.c | 55 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 53 insertions(+), 2 deletions(-)
sound/soc/davinci/davinci-mcasp.c +5 -1
index 712a7cd..ec0463a 100644
@@ -27,6 +27,7 @@
 #include <linux/of_platform.h>
 #include <linux/of_device.h>
 
+#include <sound/asoundef.h>
 #include <sound/core.h>
 #include <sound/pcm.h>
 #include <sound/pcm_params.h>

@@ -566,8 +567,11 @@ static int mcasp_i2s_hw_param(struct davinci_mcasp *mcasp, int stream)
 }
 
 /* S/PDIF */
-static int mcasp_dit_hw_param(struct davinci_mcasp *mcasp)
+static int mcasp_dit_hw_param(struct davinci_mcasp *mcasp,
+			      unsigned int rate)
 {
+	u32 val = 0;
+
 	/* Set the TX format : 24 bit right rotation, 32 bit slot, Pad 0
 	   and LSB first */
 	mcasp_set_bits(mcasp, DAVINCI_MCASP_TXFMT_REG, TXROT(6) | TXSSZ(15));

sound/soc/davinci/davinci-mcasp.c static int mcasp_dit_hw_param(struct davinci_mcasp *mcasp) · line 598

Quoted 6 lines
sound/soc/davinci/davinci-mcasp.c +3 -0
@@ -589,6 +593,53 @@ static int mcasp_dit_hw_param(struct davinci_mcasp *mcasp)
 	/* Enable the DIT */
 	mcasp_set_bits(mcasp, DAVINCI_MCASP_TXDITCTL_REG, DITEN);
 
+	/* Set S/PDIF channel status bits */
+	mcasp_set_reg(mcasp, DAVINCI_MCASP_DITCSRA_REG + 0,
+		      IEC958_AES0_CON_NOT_COPYRIGHT);

I think it would be safer to set the channel status bits like:
u32 val = 0;
u8 *bytes = (u8*) &val;

byte[0] |= IEC958_AES0_CON_NOT_COPYRIGHT;
byte[1] |= IEC958_AES1_CON_PCM_CODER;

sound/soc/davinci/davinci-mcasp.c static int mcasp_dit_hw_param(struct davinci_mcasp *mcasp) · line 609

Quoted 11 lines
sound/soc/davinci/davinci-mcasp.c +11 -0
+	mcasp_set_reg(mcasp, DAVINCI_MCASP_DITCSRB_REG + 0,
+		      IEC958_AES0_CON_NOT_COPYRIGHT);
+
+	mcasp_set_reg(mcasp, DAVINCI_MCASP_DITCSRA_REG + 1,
+		      IEC958_AES1_CON_PCM_CODER);
+	mcasp_set_reg(mcasp, DAVINCI_MCASP_DITCSRB_REG + 1,
+		      IEC958_AES1_CON_PCM_CODER);
+
+	switch (rate) {
+	case 22050:
+		val |= IEC958_AES3_CON_FS_22050;

		val8[3] |= IEC958_AES3_CON_FS_22050;

sound/soc/davinci/davinci-mcasp.c static int mcasp_dit_hw_param(struct davinci_mcasp *mcasp) · line 612

Quoted 3 lines
sound/soc/davinci/davinci-mcasp.c +3 -0
+		break;
+	case 24000:
+		val |= IEC958_AES3_CON_FS_24000;

		val8[3] |= IEC958_AES3_CON_FS_24000;
Quoted 19 lines
sound/soc/davinci/davinci-mcasp.c +19 -0
+		break;
+	case 32000:
+		val |= IEC958_AES3_CON_FS_32000;
+		break;
+	case 44100:
+		val |= IEC958_AES3_CON_FS_44100;
+		break;
+	case 48000:
+		val |= IEC958_AES3_CON_FS_48000;
+		break;
+	case 88200:
+		val |= IEC958_AES3_CON_FS_88200;
+		break;
+	case 96000:
+		val |= IEC958_AES3_CON_FS_96000;
+		break;
+	case 176400:
+		val |= IEC958_AES3_CON_FS_176400;
+		break;

sound/soc/davinci/davinci-mcasp.c static int mcasp_dit_hw_param(struct davinci_mcasp *mcasp) · line 641

Quoted 10 lines
sound/soc/davinci/davinci-mcasp.c +10 -0
+	case 192000:
+		val |= IEC958_AES3_CON_FS_192000;
+		break;
+	default:
+		printk(KERN_WARNING "unsupported sampling rate: %d\n", rate);
+		return -EINVAL;
+	}
+
+	mcasp_set_reg(mcasp, DAVINCI_MCASP_DITCSRA_REG + 3, val);
+	mcasp_set_reg(mcasp, DAVINCI_MCASP_DITCSRB_REG + 3, val);

mcasp_set_reg(mcasp, DAVINCI_MCASP_DITCSRA_REG, val); mcasp_set_reg(mcasp, DAVINCI_MCASP_DITCSRB_REG, val);

Since these registers are 32 bits and the mcasp_set_reg() uses __raw_writel() at the end.
Also in this way you write only once to the registers.

Quoted 14 lines
sound/soc/davinci/davinci-mcasp.c +2 -1
+
 	return 0;
 }
 

@@ -621,7 +672,7 @@ static int davinci_mcasp_hw_params(struct snd_pcm_substream *substream,
 		fifo_level = mcasp->rxnumevt * active_serializers;
 
 	if (mcasp->op_mode == DAVINCI_MCASP_DIT_MODE)
-		ret = mcasp_dit_hw_param(mcasp);
+		ret = mcasp_dit_hw_param(mcasp, params_rate(params));
 	else
 		ret = mcasp_i2s_hw_param(mcasp, substream->stream);
Signature · 1 line
Péter
author
#2 lore raw

Hi Peter,

Thanks for your review.

Quoted 46 lines · Peter Ujfalusi
Quoted 34 lines · Daniel Mack

In DIT (S/PDIF) mode, program the transmitted user bits to reflect the configured sample rate, along with some other details.

Signed-off-by Daniel Mack <[email protected]>
---
 sound/soc/davinci/davinci-mcasp.c | 55 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 53 insertions(+), 2 deletions(-)
sound/soc/davinci/davinci-mcasp.c +5 -1
index 712a7cd..ec0463a 100644
@@ -27,6 +27,7 @@
 #include <linux/of_platform.h>
 #include <linux/of_device.h>
 
+#include <sound/asoundef.h>
 #include <sound/core.h>
 #include <sound/pcm.h>
 #include <sound/pcm_params.h>

@@ -566,8 +567,11 @@ static int mcasp_i2s_hw_param(struct davinci_mcasp *mcasp, int stream)
 }
 
 /* S/PDIF */
-static int mcasp_dit_hw_param(struct davinci_mcasp *mcasp)
+static int mcasp_dit_hw_param(struct davinci_mcasp *mcasp,
+			      unsigned int rate)
 {
+	u32 val = 0;
+
 	/* Set the TX format : 24 bit right rotation, 32 bit slot, Pad 0
 	   and LSB first */
 	mcasp_set_bits(mcasp, DAVINCI_MCASP_TXFMT_REG, TXROT(6) | TXSSZ(15));

sound/soc/davinci/davinci-mcasp.c static int mcasp_dit_hw_param(struct davinci_mcasp *mcasp) · line 598

Quoted 6 lines
sound/soc/davinci/davinci-mcasp.c +3 -0
@@ -589,6 +593,53 @@ static int mcasp_dit_hw_param(struct davinci_mcasp *mcasp)
 	/* Enable the DIT */
 	mcasp_set_bits(mcasp, DAVINCI_MCASP_TXDITCTL_REG, DITEN);
 
+	/* Set S/PDIF channel status bits */
+	mcasp_set_reg(mcasp, DAVINCI_MCASP_DITCSRA_REG + 0,
+		      IEC958_AES0_CON_NOT_COPYRIGHT);

I think it would be safer to set the channel status bits like:
u32 val = 0;
u8 *bytes = (u8*) &val;

Quoted 2 lines

byte[0] |= IEC958_AES0_CON_NOT_COPYRIGHT;
byte[1] |= IEC958_AES1_CON_PCM_CODER;

No, these defines are mapped on to 32-bit values, as seen in include/sound/asounddef.h. Over all 6 registers, 192 bits can be stored, which is the full length of channel status bits. Hence, they really need an individual mcasp_set_reg() each.

Also, I've measured the output of the S/PDIF port with external S/PDIF introspectors and the result is correct :)

Thanks,
Daniel

#3 lore raw
Quoted 51 lines · Daniel Mack

Hi Peter,

Thanks for your review.

Quoted 46 lines · Peter Ujfalusi
Quoted 34 lines · Daniel Mack

In DIT (S/PDIF) mode, program the transmitted user bits to reflect the configured sample rate, along with some other details.

Signed-off-by Daniel Mack <[email protected]>
---
 sound/soc/davinci/davinci-mcasp.c | 55 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 53 insertions(+), 2 deletions(-)
sound/soc/davinci/davinci-mcasp.c +5 -1
index 712a7cd..ec0463a 100644
@@ -27,6 +27,7 @@
 #include <linux/of_platform.h>
 #include <linux/of_device.h>
 
+#include <sound/asoundef.h>
 #include <sound/core.h>
 #include <sound/pcm.h>
 #include <sound/pcm_params.h>

@@ -566,8 +567,11 @@ static int mcasp_i2s_hw_param(struct davinci_mcasp *mcasp, int stream)
 }
 
 /* S/PDIF */
-static int mcasp_dit_hw_param(struct davinci_mcasp *mcasp)
+static int mcasp_dit_hw_param(struct davinci_mcasp *mcasp,
+			      unsigned int rate)
 {
+	u32 val = 0;
+
 	/* Set the TX format : 24 bit right rotation, 32 bit slot, Pad 0
 	   and LSB first */
 	mcasp_set_bits(mcasp, DAVINCI_MCASP_TXFMT_REG, TXROT(6) | TXSSZ(15));

sound/soc/davinci/davinci-mcasp.c static int mcasp_dit_hw_param(struct davinci_mcasp *mcasp) · line 598

Quoted 6 lines
sound/soc/davinci/davinci-mcasp.c +3 -0
@@ -589,6 +593,53 @@ static int mcasp_dit_hw_param(struct davinci_mcasp *mcasp)
 	/* Enable the DIT */
 	mcasp_set_bits(mcasp, DAVINCI_MCASP_TXDITCTL_REG, DITEN);
 
+	/* Set S/PDIF channel status bits */
+	mcasp_set_reg(mcasp, DAVINCI_MCASP_DITCSRA_REG + 0,
+		      IEC958_AES0_CON_NOT_COPYRIGHT);

I think it would be safer to set the channel status bits like:
u32 val = 0;
u8 *bytes = (u8*) &val;

Quoted 7 lines
Quoted 2 lines

byte[0] |= IEC958_AES0_CON_NOT_COPYRIGHT;
byte[1] |= IEC958_AES1_CON_PCM_CODER;

No, these defines are mapped on to 32-bit values, as seen in include/sound/asounddef.h. Over all 6 registers, 192 bits can be stored, which is the full length of channel status bits. Hence, they really need an individual mcasp_set_reg() each.

I don't think they are mapped for the 32bit:
#define IEC958_AES0_CON_NOT_COPYRIGHT (1<<2)

#define IEC958_AES1_CON_DIGDIGCONV_ID 0x02
#define IEC958_AES1_CON_PCM_CODER (IEC958_AES1_CON_DIGDIGCONV_ID|0x00)

#define IEC958_AES3_CON_FS_48000        (2<<0)  /* 48kHz */
#define IEC958_AES3_CON_FS_32000        (3<<0)  /* 32kHz */
#define IEC958_AES3_CON_FS_22050        (4<<0)  /* 22.05kHz */
#define IEC958_AES3_CON_FS_24000        (6<<0)  /* 24kHz */

From the AES3 spec (byte numbers are 1 based while bit numbers are 0 based for some reason in the specs):
Copyright bit is on Byte1's bit 2
Byte2 has the category code
and Byte4's 0-3 bits for the sampling frequency.

All of the setting you are changing ar in the first 32bit of the whole channels status array, which is 24 byte long.

Quoted 2 lines

Also, I've measured the output of the S/PDIF port with external S/PDIF introspectors and the result is correct :)

Well you essentially writing to the correct byte offset but I don't think in a correct way:

write to DAVINCI_MCASP_DITCSRA_REG + 0 is a write starting at Byte1 write to DAVINCI_MCASP_DITCSRA_REG + 1 is a write starting at Byte2 write to DAVINCI_MCASP_DITCSRA_REG + 3 is a write starting at Byte4

The interconnect I think is clever enough to figure out that you try to write to non 32bit boundary and does corrects the access (shifting the data to it's correct place) but I don't think we should rely on that. It is much simpler to configure the bits first and write it as one 32bit.

Signature · 5 lines
Péter
_______________________________________________
Alsa-devel mailing list
[email protected]
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
author
#4 lore raw
Quoted 10 lines · Peter Ujfalusi
Quoted 4 lines · Daniel Mack

No, these defines are mapped on to 32-bit values, as seen in include/sound/asounddef.h. Over all 6 registers, 192 bits can be stored, which is the full length of channel status bits. Hence, they really need an individual mcasp_set_reg() each.

I don't think they are mapped for the 32bit:
#define IEC958_AES0_CON_NOT_COPYRIGHT (1<<2)

Quoted 7 lines

#define IEC958_AES1_CON_DIGDIGCONV_ID 0x02
#define IEC958_AES1_CON_PCM_CODER (IEC958_AES1_CON_DIGDIGCONV_ID|0x00)

#define IEC958_AES3_CON_FS_48000        (2<<0)  /* 48kHz */
#define IEC958_AES3_CON_FS_32000        (3<<0)  /* 32kHz */
#define IEC958_AES3_CON_FS_22050        (4<<0)  /* 22.05kHz */
#define IEC958_AES3_CON_FS_24000        (6<<0)  /* 24kHz */

Hmm, that's odd. What about this one then, which definitely needs a larger type than uint16?

#define IEC958_AES4_CON_ORIGFS_44100 (15<<4) /* 44.1kHz */

I considered DITCSRA0 to DITCSRA5 to carry 6 32 bit values, which map to the values defined as IEC958_AESX*.

Quoted 3 lines

From the AES3 spec (byte numbers are 1 based while bit numbers are 0 based for some reason in the specs):
Copyright bit is on Byte1's bit 2

"Byte 1" is the at index 0, right?

#define IEC958_AES0_CON_NOT_COPYRIGHT (1<<2)

Quoted 2 lines

Byte2 has the category code
and Byte4's 0-3 bits for the sampling frequency.

#define IEC958_AES3_CON_FS_44100 (0<<0)
...

You're right that my register address calculation has to go in 32bit (DAVINCI_MCASP_DITCSRA_REG + 4, DAVINCI_MCASP_DITCSRA_REG + 8, ...), but I still don't think the u8-mapping is correct.

I'm confused. :)

Daniel

#5 lore raw
Quoted 5 lines · Daniel Mack

You're right that my register address calculation has to go in 32bit (DAVINCI_MCASP_DITCSRA_REG + 4, DAVINCI_MCASP_DITCSRA_REG + 8, ...), but I still don't think the u8-mapping is correct.

I'm confused. :)

Ok ok, I am indeed. You're right, got it now. What puzzled me is that there are registers DITCSRA0 to DITCSRA5, of which only 2 seem to be of use.

I'll resend a new version later today.

Thanks!
Daniel