[alsa-devel] [PATCH] 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 zonque@gmail.com --- sound/soc/davinci/davinci-mcasp.c | 55 +++++++++++++++++++++++++++++++++++++-- 1 file changed, 53 insertions(+), 2 deletions(-)
diff --git a/sound/soc/davinci/davinci-mcasp.c b/sound/soc/davinci/davinci-mcasp.c index 712a7cd..ec0463a 100644 --- a/sound/soc/davinci/davinci-mcasp.c +++ b/sound/soc/davinci/davinci-mcasp.c @@ -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);
On 03/26/2014 05:04 PM, Daniel Mack wrote:
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 zonque@gmail.com
sound/soc/davinci/davinci-mcasp.c | 55 +++++++++++++++++++++++++++++++++++++-- 1 file changed, 53 insertions(+), 2 deletions(-)
diff --git a/sound/soc/davinci/davinci-mcasp.c b/sound/soc/davinci/davinci-mcasp.c index 712a7cd..ec0463a 100644 --- a/sound/soc/davinci/davinci-mcasp.c +++ b/sound/soc/davinci/davinci-mcasp.c @@ -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);
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;
- 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;
break;- case 24000:
val |= IEC958_AES3_CON_FS_24000;
val8[3] |= 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);
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.
- 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));
Hi Peter,
Thanks for your review.
On 03/27/2014 09:16 AM, Peter Ujfalusi wrote:
On 03/26/2014 05:04 PM, Daniel Mack wrote:
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 zonque@gmail.com
sound/soc/davinci/davinci-mcasp.c | 55 +++++++++++++++++++++++++++++++++++++-- 1 file changed, 53 insertions(+), 2 deletions(-)
diff --git a/sound/soc/davinci/davinci-mcasp.c b/sound/soc/davinci/davinci-mcasp.c index 712a7cd..ec0463a 100644 --- a/sound/soc/davinci/davinci-mcasp.c +++ b/sound/soc/davinci/davinci-mcasp.c @@ -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);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;
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
On 03/27/2014 10:39 AM, Daniel Mack wrote:
Hi Peter,
Thanks for your review.
On 03/27/2014 09:16 AM, Peter Ujfalusi wrote:
On 03/26/2014 05:04 PM, Daniel Mack wrote:
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 zonque@gmail.com
sound/soc/davinci/davinci-mcasp.c | 55 +++++++++++++++++++++++++++++++++++++-- 1 file changed, 53 insertions(+), 2 deletions(-)
diff --git a/sound/soc/davinci/davinci-mcasp.c b/sound/soc/davinci/davinci-mcasp.c index 712a7cd..ec0463a 100644 --- a/sound/soc/davinci/davinci-mcasp.c +++ b/sound/soc/davinci/davinci-mcasp.c @@ -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);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;
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.
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.
On 03/27/2014 10:10 AM, Peter Ujfalusi wrote:
On 03/27/2014 10:39 AM, Daniel Mack wrote:
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 */
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*.
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)
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
On 03/27/2014 10:31 AM, Daniel Mack wrote:
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
participants (3)
-
Daniel Mack -
Daniel Mack -
Peter Ujfalusi