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

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

[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 <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);
 
-- 
1.8.5.3

Re: [PATCH] ASoC: davinci-mcasp: set up user bits for S/PDIF mode

[Peter Ujfalusi]

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));
>  	else
>  		ret = mcasp_i2s_hw_param(mcasp, substream->stream);
>  
> 


-- 
Péter

Re: [PATCH] ASoC: davinci-mcasp: set up user bits for S/PDIF mode

[Daniel Mack]

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

Re: [PATCH] ASoC: davinci-mcasp: set up user bits for S/PDIF mode

[Peter Ujfalusi]

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.


-- 
Péter
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] ASoC: davinci-mcasp: set up user bits for S/PDIF mode

[Daniel Mack]

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

Re: [PATCH] ASoC: davinci-mcasp: set up user bits for S/PDIF mode

[Daniel Mack]

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