| Message ID | 1395846273-26025-1-git-send-email-zonque@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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); > >
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
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);
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(-)