Hi Vaibhav, Hi Yegor,

On 04.03.2013 07:22, Bedia, Vaibhav wrote:

On Fri, Mar 01, 2013 at 16:23:53, Yegor Yefremov wrote:

...

I've solved the problem with DIT/SPDIF mode (see the issue description here: http://e2e.ti.com/support/arm/sitara_arm/f/791/p/247447/870030.aspx).

In davinci_mcasp_hw_params() the DIT or I2S params will be set in the beginning. DIT mode configures the DAVINCI_MCASP_TXMASK_REG and DAVINCI_MCASP_TXFMT_REG.

And here comes the problem:

at the end of davinci_mcasp_hw_params() the davinci_config_channel_size() will touch the same registers again and thus overwrite the settings necessary for DIT. After I commented this routine I got the sound over S/PDIF and sii9022a HDMI transmitter and I could see the proper bits appearing on my oscilloscope.

What were the best way to solve this problem?

1. execute davinci_config_channel_size() only if not in DIT mode?
2. for DIT only change the DAVINCI_MCASP_TXMASK_REG according to channel width?
3. execute

if (dev->op_mode == DAVINCI_MCASP_DIT_MODE) davinci_hw_dit_param(dev); else davinci_hw_param(dev, substream->stream);

after davinci_config_channel_size()

AFAIK DIT mode was working on Davinci platforms some time back. Since AM335x has the same hardware block I was surprised to see this bug report. Not having a setup handy to test out DIT related changes, I looked at the commits on the mcasp file to figure out what happened. I suspect one of the recent patches which added 24bit support inadvertently broke the DIT support. Would it be possible for you to do a git-bisect to find out what change it was? It would be good to reference that change in the final patch.

I agree, but I also believe it's not easy to do, given the number of different trees you need for AM33xx support in general.

You could revert them one-by-one, but then again, you probably need most of them in order to make the driver work on AM33xx and in setups that differ from what the davici driver was originally written for.

Coming to the 3 options that you have listed I would suggest going with #1. DIT mode requires the configuration to be 32 bit slot with rotation set to 24 but looks like the current implementation of davinci_config_channel_size() doesn't handle this scenario very well.

Hmm, if that's the case, I fail to see how the 24-bit patches you mentioned could possibly account for that, given that they don't touch the logical around this area. Unless I miss anything very obvious :) Can you be a bit more specific about which of these patches you suspect?

I think the driver needs a bit of rework to ensure Tx and Rx paths don't end up overwriting the configuration but that needs to be taken up separately.

Yegor - could you send me a patch that fixes the problem for you? I can do regression tests on my platform.

Thanks, Daniel

On Mon, Mar 4, 2013 at 8:55 AM, Daniel Mack zonque@gmail.com wrote:

Hi Vaibhav, Hi Yegor,

On 04.03.2013 07:22, Bedia, Vaibhav wrote:

...

On Fri, Mar 01, 2013 at 16:23:53, Yegor Yefremov wrote:

...

I've solved the problem with DIT/SPDIF mode (see the issue description here: http://e2e.ti.com/support/arm/sitara_arm/f/791/p/247447/870030.aspx).

In davinci_mcasp_hw_params() the DIT or I2S params will be set in the beginning. DIT mode configures the DAVINCI_MCASP_TXMASK_REG and DAVINCI_MCASP_TXFMT_REG.

And here comes the problem:

at the end of davinci_mcasp_hw_params() the davinci_config_channel_size() will touch the same registers again and thus overwrite the settings necessary for DIT. After I commented this routine I got the sound over S/PDIF and sii9022a HDMI transmitter and I could see the proper bits appearing on my oscilloscope.

What were the best way to solve this problem?

1. execute davinci_config_channel_size() only if not in DIT mode?
2. for DIT only change the DAVINCI_MCASP_TXMASK_REG according to channel width?
3. execute

if (dev->op_mode == DAVINCI_MCASP_DIT_MODE) davinci_hw_dit_param(dev); else davinci_hw_param(dev, substream->stream);

after davinci_config_channel_size()

AFAIK DIT mode was working on Davinci platforms some time back. Since AM335x has the same hardware block I was surprised to see this bug report. Not having a setup handy to test out DIT related changes, I looked at the commits on the mcasp file to figure out what happened. I suspect one of the recent patches which added 24bit support inadvertently broke the DIT support. Would it be possible for you to do a git-bisect to find out what change it was? It would be good to reference that change in the final patch.

I agree, but I also believe it's not easy to do, given the number of different trees you need for AM33xx support in general.

You could revert them one-by-one, but then again, you probably need most of them in order to make the driver work on AM33xx and in setups that differ from what the davici driver was originally written for.

...

Coming to the 3 options that you have listed I would suggest going with #1. DIT mode requires the configuration to be 32 bit slot with rotation set to 24 but looks like the current implementation of davinci_config_channel_size() doesn't handle this scenario very well.

Hmm, if that's the case, I fail to see how the 24-bit patches you mentioned could possibly account for that, given that they don't touch the logical around this area. Unless I miss anything very obvious :) Can you be a bit more specific about which of these patches you suspect?

I think the DIT support was dead on arrival ;-) Look at the initial commit: http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/sound/...

Size adjustment is made after DIT mode settings and though davinci_config_channel_size(dev, word_length) doesn't overwrite all needed settings, it overwrites XSSZ value and thus disables 32-bit slot. Do I see it right?

...

I think the driver needs a bit of rework to ensure Tx and Rx paths don't end up overwriting the configuration but that needs to be taken up separately.

Yegor - could you send me a patch that fixes the problem for you? I can do regression tests on my platform.

I'll do.

Yegor

On Mon, Mar 4, 2013 at 11:10 AM, Bedia, Vaibhav vaibhav.bedia@ti.com wrote:

On Mon, Mar 04, 2013 at 13:35:10, Yegor Yefremov wrote:

...

On Mon, Mar 4, 2013 at 8:55 AM, Daniel Mack zonque@gmail.com wrote:

...

Hi Vaibhav, Hi Yegor,

On 04.03.2013 07:22, Bedia, Vaibhav wrote:

...

On Fri, Mar 01, 2013 at 16:23:53, Yegor Yefremov wrote:

...

I've solved the problem with DIT/SPDIF mode (see the issue description here: http://e2e.ti.com/support/arm/sitara_arm/f/791/p/247447/870030.aspx).

In davinci_mcasp_hw_params() the DIT or I2S params will be set in the beginning. DIT mode configures the DAVINCI_MCASP_TXMASK_REG and DAVINCI_MCASP_TXFMT_REG.

And here comes the problem:

at the end of davinci_mcasp_hw_params() the davinci_config_channel_size() will touch the same registers again and thus overwrite the settings necessary for DIT. After I commented this routine I got the sound over S/PDIF and sii9022a HDMI transmitter and I could see the proper bits appearing on my oscilloscope.

What were the best way to solve this problem?

1. execute davinci_config_channel_size() only if not in DIT mode?
2. for DIT only change the DAVINCI_MCASP_TXMASK_REG according to channel width?
3. execute

if (dev->op_mode == DAVINCI_MCASP_DIT_MODE) davinci_hw_dit_param(dev); else davinci_hw_param(dev, substream->stream);

after davinci_config_channel_size()

AFAIK DIT mode was working on Davinci platforms some time back. Since AM335x has the same hardware block I was surprised to see this bug report. Not having a setup handy to test out DIT related changes, I looked at the commits on the mcasp file to figure out what happened. I suspect one of the recent patches which added 24bit support inadvertently broke the DIT support. Would it be possible for you to do a git-bisect to find out what change it was? It would be good to reference that change in the final patch.

I agree, but I also believe it's not easy to do, given the number of different trees you need for AM33xx support in general.

You could revert them one-by-one, but then again, you probably need most of them in order to make the driver work on AM33xx and in setups that differ from what the davici driver was originally written for.

...

Coming to the 3 options that you have listed I would suggest going with #1. DIT mode requires the configuration to be 32 bit slot with rotation set to 24 but looks like the current implementation of davinci_config_channel_size() doesn't handle this scenario very well.

Hmm, if that's the case, I fail to see how the 24-bit patches you mentioned could possibly account for that, given that they don't touch the logical around this area. Unless I miss anything very obvious :) Can you be a bit more specific about which of these patches you suspect?

I looked at Daniel's changes a bit closer and it has not changed the behavior. Sorry about that. But this would also imply that DIT mode has been broken for quite some time now :(

...

I think the DIT support was dead on arrival ;-) Look at the initial commit: http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/sound/...

Size adjustment is made after DIT mode settings and though davinci_config_channel_size(dev, word_length) doesn't overwrite all needed settings, it overwrites XSSZ value and thus disables 32-bit slot. Do I see it right?

I can only assume that the original author tested the code before posting it :(

I can't find anything in the spec which mentions that the slot size matters in DIT mode. The only restriction seems to be that the valid data bits are in bits 24:0. When the actual transmit happens other things like the preamble, VUCP bits will get filled in and I am guessing this does not depend on the slot size that is programmed. That's the only scenario in which the original code could have worked.

I found this passage in 22.3.8.3.2 of the "AM335x ARM® Cortex™-A8 Microprocessors (MPUs) Technical Reference Manual" Literature Number: SPRUH73G

The DIT transmitter only works in the following configuration: • In transmit frame control register (AFSXCTL): – Internally-generated transmit frame sync, FSXM = 1. – Rising-edge frame sync, FSXP = 0. – Bit-width frame sync, FXWID = 0. – 384-slot TDM, XMOD = 1 1000 0000b. • In transmit clock control register (ACLKXCTL), ASYNC = 1. • In transmit bitstream format register (XFMT), XSSZ = 1111 (32-bit slot size).

So slot size does matter for DIT mode. On the same page you have frame format description. It says that two following rotation settings are valid for DIT XROT = 010 and XROT = 000, but the only one working with right-aligned data is 0x6 i.e. 24-bit rotation. This value was used in the davinci-mcasp.c driver source.

Yegor

On Mon, Mar 4, 2013 at 12:25 PM, Bedia, Vaibhav vaibhav.bedia@ti.com wrote:

On Mon, Mar 04, 2013 at 15:56:47, Yegor Yefremov wrote: [...]

...

I found this passage in 22.3.8.3.2 of the "AM335x ARM® Cortex™-A8 Microprocessors (MPUs) Technical Reference Manual" Literature Number: SPRUH73G

The DIT transmitter only works in the following configuration: • In transmit frame control register (AFSXCTL): – Internally-generated transmit frame sync, FSXM = 1. – Rising-edge frame sync, FSXP = 0. – Bit-width frame sync, FXWID = 0. – 384-slot TDM, XMOD = 1 1000 0000b. • In transmit clock control register (ACLKXCTL), ASYNC = 1. • In transmit bitstream format register (XFMT), XSSZ = 1111 (32-bit slot size).

So slot size does matter for DIT mode. On the same page you have frame format description. It says that two following rotation settings are valid for DIT XROT = 010 and XROT = 000, but the only one working with right-aligned data is 0x6 i.e. 24-bit rotation. This value was used in the davinci-mcasp.c driver source.

Ok. Since you seem to have a h/w handy, if you have time to spare, could you please try out a config which sets the slot size to 16 instead of 32 and let us know whether it works? Depending on the test result we'll know for sure whether the original code ever worked.

I've made test with XSSZ == 0x7 (16-bit) and I heard sound. So the initial version could be functional, because only XSSZ was touched. Later came settings for mask and rotation. It seems like rotation is the most important setting for DIT. So the Technical Reference Manual should be thoroughly revised:

1. XSSZ 32-bit is not mandatory 2. rotation for right-aligned data must properly specified (0x6 - 24-bit and not 0x0)

Yegor

On Tue, Mar 5, 2013 at 7:33 AM, Bedia, Vaibhav vaibhav.bedia@ti.com wrote:

On Mon, Mar 04, 2013 at 21:26:25, Yegor Yefremov wrote:

...

I've made test with XSSZ == 0x7 (16-bit) and I heard sound. So the initial version could be functional, because only XSSZ was touched. Later came settings for mask and rotation. It seems like rotation is the most important setting for DIT. So the Technical Reference Manual should be thoroughly revised:

1. XSSZ 32-bit is not mandatory
2. rotation for right-aligned data must properly specified (0x6 -

24-bit and not 0x0)

Thanks for the confirmation. I'll try chasing down the h/w guys to get this sorted out and also raise a bug on the Technical Reference Manual.

Thanks for your help and feedback. Could you please pull both mcasp patches into http://arago-project.org/git/projects/?p=linux-am33x.git;a=shortlog;h=refs/h... too?

Yegor