| Message ID | 1461747993-11516-1-git-send-email-peda@axentia.se (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Apr 27, 2016 at 11:06:33AM +0200, Peter Rosin wrote: > While the start condition is correct for the left channel word in the I2S > case, it is not correct that the right channel word follows immediately > after the left channel word. The start of the right channel word should > be triggered by a rising edge on LRCLK in the I2S case, something which > simply does not happen. Almost every programmable serial port does this, it's a very common issue which is why we always try to go for exact clocking where we can - it greatly improves the interoperability for I2S if there are no dead clocks.
On 2016-04-27 18:23, Mark Brown wrote: > On Wed, Apr 27, 2016 at 11:06:33AM +0200, Peter Rosin wrote: > >> While the start condition is correct for the left channel word in the I2S >> case, it is not correct that the right channel word follows immediately >> after the left channel word. The start of the right channel word should >> be triggered by a rising edge on LRCLK in the I2S case, something which >> simply does not happen. > > Almost every programmable serial port does this, it's a very common > issue which is why we always try to go for exact clocking where we can - > it greatly improves the interoperability for I2S if there are no dead > clocks. Someone said: be conservative in what you send, be liberal in what you accept. This is not that at all. This is more like: be conservative in what you send, and accept only a subset of valid input. It absolutely kills interoperability if you claim I2S and then don't do I2S. If you are not looking at both edges of LRCLK it simply isn't I2S. It's something else. Like "packed I2S" or "DSP moda A with inverted symmetric LRCLK" or something, so why not invent a way to say that? Dais could be taught that an I2S LRCLK slave is compatible with this new mode as LRCLK master, but not the other way around. But since most people are not making dai link decisions, that will probably be a lot of work for questionable gain. What is needed is documenatation about quirks such as this, hence my patch. I have this codec which does I2S but there is no way to get rid of dead clocks when it masters the clocks. It can divide MCLK with 1,2,4,8 or 16 to get a BCLK, or it can generate BCLK as 48x or 64x LRCLK. But it only sports 16 bits per sample. So, if it divides MCLK, and MCLK is not matched to the needed LRCLK (there is no need, we currently use a 16MHz MCLK) there will almost certainly be some dead BCLKs, and if LRCLK is used as base for BCLK, there will be either 8 or 16 dead cycles per channel. In other words, it is difficult or next to impossible to not get dead cycles with this codec. But it does I2S correctly. When you don't know beforehand that the clock slave (Atmel SSC) is lying and isn't supporting I2S even though it claims so, it turned out to be a huge time sink for me when I tried to bring up this new codec (max9860). I naturally thought that the problem was with my new code and not the old (assumed mature and mostly free of silly bugs) code. Especially for something as well understood as I2S. I was baffled when I realized what the problem was, it wasn't even on my map. I'm probably naive... Cheers, Peter
On Wed, Apr 27, 2016 at 11:29:53PM +0200, Peter Rosin wrote: > On 2016-04-27 18:23, Mark Brown wrote: > > Almost every programmable serial port does this, it's a very common > > issue which is why we always try to go for exact clocking where we can - > > it greatly improves the interoperability for I2S if there are no dead > > clocks. > Someone said: be conservative in what you send, be liberal in what you > accept. This is not that at all. This is more like: be conservative in > what you send, and accept only a subset of valid input. > It absolutely kills interoperability if you claim I2S and then don't > do I2S. If you are not looking at both edges of LRCLK it simply isn't I2S. > It's something else. Like "packed I2S" or "DSP moda A with inverted > symmetric LRCLK" or something, so why not invent a way to say that? The reason people do this is so that they can interface I2S only CODECs (which are reasonably common, especially simpler CODECs with no register map) with SoCs that have programmable serial ports. If you want to spend the time to represent this explicitly through the system that would be useful but note that the main reason the format configuration is currently done manually is that especially with older devices it is common for there to be gotchas in this, either in terms of things like this or in terms of performance. Determining what should master the bus is an especially thorny issue, there are frequently tradeoffs between the quality of the clocks and other restrictions. > I have this codec which does I2S but there is no way to get rid of dead > clocks when it masters the clocks. It can divide MCLK with 1,2,4,8 or 16 > to get a BCLK, or it can generate BCLK as 48x or 64x LRCLK. But it only > sports 16 bits per sample. In order to make that interoperate you should declare support for 24 and 32 bit formats in the DAI formats and set sig_bits to 16. The representation of memory formats in the DAIs that don't interface with memory was a really unfortunate choice which sadly nobody has ever had the time and/or enthusiasm to address.
diff --git a/sound/soc/atmel/atmel_ssc_dai.c b/sound/soc/atmel/atmel_ssc_dai.c index 4ab1e2013238..3108c6e20ad5 100644 --- a/sound/soc/atmel/atmel_ssc_dai.c +++ b/sound/soc/atmel/atmel_ssc_dai.c @@ -558,7 +558,15 @@ static int atmel_ssc_hw_params(struct snd_pcm_substream *substream, break; case SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_CBM_CFM: - /* I2S format, CODEC supplies BCLK and LRC clocks. */ + /* + * I2S format, CODEC supplies BCLK and LRC clocks. + * NOTE! This is not really I2S, as the SSC does not allow + * for any extra empty BCLK cycles after the left channel + * word. I.e. the SSC is only looking at the falling edge + * of LRCLK and ignores the rising edge. + * In reality, this is DSP mode A with inverted LRCLK. + * Use DSP mode A instead, if your codec supports it. + */ rcmr = SSC_BF(RCMR_PERIOD, 0) | SSC_BF(RCMR_STTDLY, START_DELAY) | SSC_BF(RCMR_START, SSC_START_FALLING_RF)
The only difference in register settings between I2S and DSP mode A, for the SND_SOC_DAIFMT_CBM_CFM case, is the start condition (which is SSC_START_FALLING_RF for I2S and SSC_START_RISING_RF for DSP mode A). As this is not the only difference between the two formats, it is an indicator that something is seriously wrong. And it is the I2S support that is broken. While the start condition is correct for the left channel word in the I2S case, it is not correct that the right channel word follows immediately after the left channel word. The start of the right channel word should be triggered by a rising edge on LRCLK in the I2S case, something which simply does not happen. The correct thing to do would be to simply remove the alleged support for I2S/CBM_CFM, but that is likely to cause regressions. So, just make a note about the buggy I2S support in the source. Signed-off-by: Peter Rosin <peda@axentia.se> --- sound/soc/atmel/atmel_ssc_dai.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)