[alsa-devel] [PATCH 2/3] ASoC: pxa-ssp.c, Automatically set TDM when needed

[Daniel Mack]

On Fri, Aug 14, 2009 at 01:17:41PM -0300, Daniel Ribeiro wrote:
> Em Qua, 2009-08-12 às 20:17 +0200, Daniel Mack escreveu:
> > 
> > 	snd_soc_dai_set_tdm_slot(cpu_dai, 3, 3, 2, 16);
> > 
> > Which translates into "give me 2 slots with 16 bits each, and distribute
> > them over the timeline as a 0-1-0-1 pattern". Which is exactly what I
> > need, right?
> 
> No. in an ideal world, set_tdm_slot(3, 3, 2, 16) shoud give you 2x16
> bits slots, on a 1-1 timeline, and shouldn't be a valid network mode
> configuration, in this case, one should use a frame width of 32bits and
> _not_ use network mode.
> 
> 0-1-0-1 should be 4 slots, with slots 2 and 4 active, but I think that
> what you want is 1-0-1-0 = set_tdm_slot(5, 5, 4, 16). But... See below.

Maybe we should first decide which TDM mode _should_ be correct for the
mode I'm working on, and then amend the actual implementation? And
provide some documentation once the API is decided.

> > But unfortunately, the code didn't make the SSP port do that. I ended up
> > having a asynchronous clock and the data somewhere where I wouldn't have
> > expected them: http://caiaq.de/download/tmp/1.png
> 
> I think that the source of the problem is that we can't assert SFRM past
> the end of the first slot, and DMYSTOP changes the slot_width on the
> wire.

Yes, I've seen this, too.

(Just a sidenote: one way to debug that engine is to interrupt the
execution using Marvell's XDB tool and then manually modify the register
set while the PXA keeps on playing the same DMA buffer over and over
again. You'll get ~1/2 sec of audio in a loop - enough to judge whether
it's playing at the correct speed.)

> Consider that PXA is _slave_ of SFRM: (SFRMDIR=1)
> 
> * SFRMWDTH is ignored.
> * Only one edge (set by SFRMP) is considered.
> 
> 
> And when PXA is _master_ of SFRM: (SFRMDIR=0)

As Mark pointed out already, this implementation details should be
hidden behind the API. Users really shouldn't need to care.

> * SFRMWDTH is needed for I2S.
> * SFRMWDTH can't be asserted past the end of DMYSTOP.
> * DMYSTOP changes the slot width, as it generates extra cycles after the
> last bit of data.
> * DMYSTOP only changes the _active_ slots width. (this is what causes
> async SFRM).
> * DMYSTOP is restricted to 0-3 on PXA2XX
> * DMYSTOP must be cleared in network mode, or when FSRT is set.

Well, for this last point: that's not actually true. The configuration
I'm currently working with _does_ have the MOD bit set which indicates
a 'network' mode (whatever the PXA's implementation does with that
particular information) and DMYSTOP _does_ matter. Which is a
contradiction to the specs, yes, but that's what it's like.

> We want to write 2x16bit samples with 1-0-1-0 pattern for I2S, so we
> can:
> 
> * set_tdm_slot(5, 5, 4, 16) if pxa is slave of SFRM.

Wouldn't 1-0-1-0 rather be 0xa?

> * use SFRMWDTH(16) | DMYSTOP(16) if pxa is master of SFRM. (pxa3 only)
> * use DMYSTOP(16) if pxa is slave of SFRM. (pxa3 only)

> 
> > I did some minor modifications to the patches 2/3 and 3/3 which made it
> > work for me eventually - find them inline.
> > 
> > Not sure whether I got the theory about the API right, and of course I
> > didn't test on any other hardware than the one I have.
> > 
> > 
> 
> > > -	cpu_dai->dma_data = ssp_get_dma_params(ssp,
> > > -			((chn == 2) && (ttsa != 1)) || (width == 32),
> > > +	cpu_dai->dma_data = ssp_get_dma_params(ssp, slot_width > 16,
> > >  			substream->stream == SNDRV_PCM_STREAM_PLAYBACK);
> > 
> > The condition for the width4 parameter needs to be
> > (slot_width * slots) > 16 for me. With that parameter set to 0, audio
> > plays at half speed on my system.
> 
> Consider that you have real network mode, and you are using 2 slots of
> 16 bit mono audio. You want to ignore 16bit for each 16bit of data. ie,
> only one channel active. You really don't want 32bit DMA in this case.

I can just say that in case the DMA material is _not_ 32bit, audio will
play at half speed. So the mode I'm using seems to need the double amout
of PCM data. For whatever reason.

> I agree that this should be (slot_width * number_of_active_slots) > 16
> tough.

Ok. And that won't break your use case?

> > > +			sspsp |= SSPSP_DMYSTOP(
> > > +					(slot_width * slots) / 2 - width - 1);
> > > +			sspsp |= SSPSP_SFRMWDTH((slot_width * slots) / 2);
> > 
> > These two calculations are wrong my case. What works here is
> > 
> > 	sspsp |= SSPSP_DMYSTOP(
> > 			(slot_width * slots * chn) / 2 - width - 1);
> > 	sspsp |= SSPSP_SFRMWDTH((slot_width * slots * chn) / 2);
> > 
> > ... which is another multiplication by factor 2. But I'm not sure if I
> > got the wrong variable with value 2 :)
> 
> DMYSTOP itself should be accounted for slot_width, but this will only
> work if all slots are active otherwise you will get asynchronous SFRM.
> Something like:

[...]

Care to send modified patches? I'd be lucky to test them.

> This should be able to deal with I2S when pxa is slave, on both pxa2xx
> and pxa3xx, with set_tdm_slot(5, 5, 4, 16).
> 
> Or when pxa is master, on pxa3xx only, with set_tdm_slot(3, 3, 2, 16).

Again - shouldn't we care for a consistent calling convention here and
handle the special cases in the implementation rather than feeding
nebulous integer arguments to it, which vary depending on the clocking
directions?

> This still violates the "DMYSTOP must be clear on network mode" rule,
> but as all slots are active its not really network mode. And it seems to
> work for you, so... ;)

Yeah, that makes no sense at all. I think we should ignore that comment.
It seems to be simply wrong, or we're not in network mode afterall, even
though we selected it. I don't know.

Thanks,
Daniel