sound/soc/bcm/bcm2708-i2s.c bug & fix, a request, and a question

[plughplover]

While working on a simple i2s peripheral, I identified and fixed the bug alluded to here:
4ba7ea9

The fix is trivial; in this code

416 case SNDRV_PCM_FORMAT_S24_LE:
417 data_length = 24;
418 bclk_ratio = 40;
419 break;

change '40' to '60'

I can confirm that this fixes the problem for capture (my application), but as I don't have any i2s DACs I can't test it for playback. (However, I am 99% certain it also fixes that case). Of course, if/when this fix is incorporated, the afore-mentioned patch should also be backed out.

That is the bug & fix part. The request part:

My i2s peripheral uses the sound/soc/generic/simple-card.c ASoC card driver.
To use it, I must 'make menuconfig' and enable
Device drivers
Sound Card Support
Advanced Linux Sound Architecture
ALSA for SoC audio support
ASOC Simple sound card
as a loadable module, and also apply this trivial patch from 3.13
e244bb9

The request is that these two changes be incorporated into the current 3.12 branch for future 3.12.xx updates (ideally in conjunction with the bcm2708-i2s fix), subject to the answer to...

the question(s) part

My application also uses sound/soc/codecs/dmic.c in conjunction with generic/simple-card.c
Normally, Kconfig for a card driver would 'select' the codec driver(s), and on my dev sys I have added 'select SND_SOC_DMIC' to sound/soc/generic/Kconfig. But as simple-card is generic (as is dmic.c), that does not seem like the 'right thing to do'. How SHOULD this be handled? How would one get dmic included in the default raspberrypi distribution with (a patched) simple-card?

BTW, new account opened solely to post this. Have never mucked with github and forks and up/downstream and pulls and etc before, and frankly would prefer to focus my time elsewhere.

Thanks!

[popcornmix]

@hifiberry @koalo @hmbedded @iqaudio
any thoughts on the two changes suggested here (or the question asked)?

[koalo]

Hi,
the first patch looks very reasonable. Thank you very much!
Probably a typo in the driver, at least I do not remember why this should be 40. So you are probably right. Could anyone with a working setup (@hifiberry @iqaudio) please verify if it is still working?

The second patch is trivial and will not induce regression.

Regarding the question I can not say anything since I have never worked with the generic sound card. You might want to ask your question at freenode -> #alsa-soc.

Greetings,
Florian

[hifiberry]

Hi,

the patch should not break any of our drivers as we set the bclk_ratio in the sound card driver. However I'm not really sure how these defaults work at all. Most sound chips will use 64xfs for 24 and 32 bit input data .
Setting this to 60 seems a bit strange. I guess, this won't work on some chips. Why not use bclk_ratio 64?

Regards,
Daniel

[koalo]

If the codec supports a bclk_ratio of 60 and is accessed as slave, the audio quality will be improved since it is an integer divider of the clock frequency (Details: http://koalo.github.io/jamberry_thesis.pdf Section 5.5.2). I assume the difference is hard to recognize for most people, but that is not a valid argument if you want to deal with 24 bit on the Raspberry Pi.

[hifiberry]

I agree on this, I just did not know a DAC chip that supports this.
BTW: I did some measurements some time ago, the jitter on bitclock with non-integer dividers is in the range of 1-1.5ns.

[plughplover]

The simple-card driver does not use the set_bclk_ratio callback,
therefore bcm2708-i2s uses the defaults.
The results for '40' vs '60' for 24 bit, yields (for example)

453 target_frequency = sampling_rate * bclk_ratio;
8000_40=320000 vs 8000_60=480000
The dmic device is CBS_CFS, hence
459 divi = bcm2708_clk_freq[clk_src] / target_frequency;
19200000/320000=60 vs 19200000/480000=40
then
517 ch2pos = bclk_ratio / 2 + data_delay;
40/2+1 = 21 vs 60/2+1 = 31
and
551 mode |= BCM2708_I2S_FLEN(bclk_ratio - 1);
40-1=39 vs 60-1=59
552 mode |= BCM2708_I2S_FSLEN(bclk_ratio / 2);
40/2=20 vs 60/2=30

At '40', for 24 bit data, ch2pos, flen, and fslen are too small. There is no way to transfer 48 bits of data in 40 i2s-bitclocks. At '60' i2s-bitclocks per frame, the 48 bits of data fits.

However (opening up a can of worms, sigh) hifiberry's point is valid.

Quoting Koala's paper:

"The second solution utilizes the fact, that many audio codecs do not require a specific ratio of bit clock to frame sync as long as there are at most enough bit clock cycles within a frame sync cycle to contain the desired word length, and the bit clock frequency is not too high. The state of the data line in the remaining bits does not care as shown in Fig. 5.3.This leads to the uncommon number of 40 bit clock transitions per frame sync period resulting in a bit clock frequency of 40 · 48kHz = 1.92MHz.This can be generated form the 19.2MHz clock by an integer divider of 10. "

Koala is using a 'feature' of the Philips i2s spec in order to allow the 19.2MHz oscillator to be used for a broader set of sampling frequency & data length combinations - by throwing in extra i2s-bitclocks after the required data_length bitclocks. Quoting Philips:

"Serial data is transmitted in two’s complement with the MSB first. The MSB is transmitted first because the transmitter and receiver may have different word lengths. It isn’t necessary for the transmitter to know how many bits the receiver can handle, nor does the receiver need to know how many bits are being transmitted.
When the system word length is greater than the transmitter word length, the word is truncated (least significant data bits are set to ‘0’) for data transmission. If the receiver is sent more bits than its word length, the bits after the LSB are ignored. On the other hand, if the receiver is sent fewer bits than its word length, the missing bits are set to zero internally. And so, the MSB has a fixed position, whereas the position of the LSB depends on the word length. The transmitter always sends the MSB of the next word one clock period after the WS changes."

(note the driver declares it supports SNDRV_PCM_RATE_8000_192000)

For example, for 16 bit samples at 8000Hz, you need an i2s-bitclock of 2_16_8000=256000 clks/sec. 19200000/256000=75, so you can use the oscillator. But that is the ONLY sample rate that works. 19200000 / ({any of 16, 32, 48, 64, 96, 192k} * 32) is non-integral. However, if you say that you need 40 i2s-bitclocks per frame (to carry that 32 bits of data) then
19200000/(40_8000)= 60 works
19200000/(40_16000)= 30 works
19200000/(40_32000)=15 works
19200000/(40_48000)=10 works
19200000/(40*96000)=5 works
but 11025, 22050, 44100, 64K and 192K use the 500MHz MASH clock path through the code.

For 32 bit samples, you need 2_32_samplerate i2s-bitclocks. There is NO sample rate that works with the 19.2MHz oscillator. However, if you say the i2s bus should use 80 bitclocks to carry the 64 bits of data, then 8k, 16k, & 48k can use the oscillator. The other rates use the 500Mhz MASH clock

For 24 bit samples, things get interesting....
with bclk_ratio = 48, 8K and 16K rates work, others can't use the oscillator.
with bclk_ratio = 50, 8k, 16k, 32k, 48k, 64k ,96k & 192K ALL use the oscillator
with bclk_ratio = 60, 8k, 16k, 32k, 64k use the oscillator, 48k, 96k, 192k don't.
(and in all cases the CD related rates always use the MASH clock path)

GIven the above, why would I propose '60' rather than '50'?
Because '50' didn't occur to me until I started writing this. :)
I will go off and test '50' and report back...

HOWEVER getting back to hifiberry's point, using unusual frame length values in order to allow use of the 19.2MHz clock makes me uncomfortable also, even though it seems permissible by the Philips spec. For example, the data sheet for my 24 bit capture device seems rather insistent that there are 64 bclks in a frame (though 60 worked).

[iqaudio]

Plughplover, Daniel and I are using the TI PCM5122, Daniel also used the PCM5102 and a wolfson device for his digital out boards.

Which device are you using so we can all look at the data sheets.

Gordon

On 11 Sep 2014, at 05:23, plughplover [email protected] wrote:

The simple-card driver does not use the set_bclk_ratio callback,
therefore bcm2708-i2s uses the defaults.
The results for '40' vs '60' for 24 bit, yields (for example)

453 target_frequency = sampling_rate * bclk_ratio;
8000_40=320000 vs 8000_60=480000
The dmic device is CBS_CFS, hence
459 divi = bcm2708_clk_freq[clk_src] / target_frequency;
19200000/320000=60 vs 19200000/480000=40
then
517 ch2pos = bclk_ratio / 2 + data_delay;
40/2+1 = 21 vs 60/2+1 = 31
and
551 mode |= BCM2708_I2S_FLEN(bclk_ratio - 1);
40-1=39 vs 60-1=59
552 mode |= BCM2708_I2S_FSLEN(bclk_ratio / 2);
40/2=20 vs 60/2=30

At '40', for 24 bit data, ch2pos, flen, and fslen are too small. There is no way to transfer 48 bits of data in 40 i2s-bitclocks. At '60' i2s-bitclocks per frame, the 48 bits of data fits.

However (opening up a can of worms, sigh) hifiberry's point is valid.

Quoting Koala's paper:

"The second solution utilizes the fact, that many audio codecs do not require a specific ratio of bit clock to frame sync as long as there are at most enough bit clock cycles within a frame sync cycle to contain the desired word length, and the bit clock frequency is not too high. The state of the data line in the remaining bits does not care as shown in Fig. 5.3.This leads to the uncommon number of 40 bit clock transitions per frame sync period resulting in a bit clock frequency of 40 · 48kHz = 1.92MHz.This can be generated form the 19.2MHz clock by an integer divider of 10. "

Koala is using a 'feature' of the Philips i2s spec in order to allow the 19.2MHz oscillator to be used for a broader set of sampling frequency & data length combinations - by throwing in extra i2s-bitclocks after the required data_length bitclocks. Quoting Philips:

"Serial data is transmitted in two’s complement with the MSB first. The MSB is transmitted first because the transmitter and receiver may have different word lengths. It isn’t necessary for the transmitter to know how many bits the receiver can handle, nor does the receiver need to know how many bits are being transmitted.
When the system word length is greater than the transmitter word length, the word is truncated (least significant data bits are set to ‘0’) for data transmission. If the receiver is sent more bits than its word length, the bits after the LSB are ignored. On the other hand, if the receiver is sent fewer bits than its word length, the missing bits are set to zero internally. And so, the MSB has a fixed position, whereas the position of the LSB depends on the word length. The transmitter always sends the MSB of the next word one clock period after the WS changes."

(note the driver declares it supports SNDRV_PCM_RATE_8000_192000)

For example, for 16 bit samples at 8000Hz, you need an i2s-bitclock of 2_16_8000=256000 clks/sec. 19200000/256000=75, so you can use the oscillator. But that is the ONLY sample rate that works. 19200000 / ({any of 16, 32, 48, 64, 96, 192k} * 32) is non-integral. However, if you say that you need 40 i2s-bitclocks per frame (to carry that 32 bits of data) then
19200000/(40_8000)= 60 works
19200000/(40_16000)= 30 works
19200000/(40_32000)=15 works
19200000/(40_48000)=10 works
19200000/(40*96000)=5 works
but 11025, 22050, 44100, 64K and 192K use the 500MHz MASH clock path through the code.

For 32 bit samples, you need 2_32_samplerate i2s-bitclocks. There is NO sample rate that works with the 19.2MHz oscillator. However, if you say the i2s bus should use 80 bitclocks to carry the 64 bits of data, then 8k, 16k, & 48k can use the oscillator. The other rates use the 500Mhz MASH clock

For 24 bit samples, things get interesting....
with bclk_ratio = 48, 8K and 16K rates work, others can't use the oscillator.
with bclk_ratio = 50, 8k, 16k, 32k, 48k, 64k ,96k & 192K ALL use the oscillator
with bclk_ratio = 60, 8k, 16k, 32k, 64k use the oscillator, 48k, 96k, 192k don't.
(and in all cases the CD related rates always use the MASH clock path)

GIven the above, why would I propose '60' rather than '50'?
Because '50' didn't occur to me until I started writing this. :)
I will go off and test '50' and report back...

HOWEVER getting back to hifiberry's point, using unusual frame length values in order to allow use of the 19.2MHz clock makes me uncomfortable also, even though it seems permissible by the Philips spec. For example, the data sheet for my 24 bit capture device seems rather insistent that there are 64 bclks in a frame (though 60 worked).

—
Reply to this email directly or view it on GitHub.

[plughplover]

ADMP441 (aka INMP441)

[plughplover]

BTW, I don't know the lineage, but since
4ba7ea9
also patched the PCM5102 driver, is it safe to assume that IT was the codec that caused xbmc problems with 24 bit playback? How do we track down the problem report associated with that patch and get it retested with '60' (and/or '50')?

[hmbedded]

In principle the patch for the bclk_ratio looks fine although I agree with @plughplover that 60 may not be the best number. I did some analysis to find out which values of bclk_ratio result in an integer divide from 19.2MHz for all of the 8k sampling frequency multiples [i.e. 19.2MHz/(bclk_ratio*fs)] and in fact there are only two numbers (up to 100) that work for all frequencies: 50 and 100. All other numbers result in some of the sampling frequencies being non integer and thus reverting to MASH mode.

I therefore propose that we take this stage further and also change the bclk_ratios for 16 and 32 bit audio as follows:

16 bit: bclk_ratio = 50
24 bit: bclk_ratio = 50
32 bit: bclk_ratio = 100

This will result in all of the 8k frequency multiples running in integer mode and should therefore improve the sound quality for those that currently do not. I'd be happy to test this on the IQaudIO platform if it's generally thought to be a good idea?

[hifiberry]

I don't know what was the reason for the XBMC problem, as we could not reproduce it outside XBMC. I don't think it comes from the bclk_ratio as this was always set in the driver to 64. But if somebody could test it, this would be great.

BTW: While your MEMS microphone outputs 24bit data, with 61dbA SNR even 16bit are more than enough.

[koalo]

I vote for 50, too, after I have reproduced your computations.
By the way: I have built a calculator for this some time ago that you can find here: http://blog.koalo.de/2013/04/pcmpwm-clock-calculator.html

[plughplover]

I don't know the Linux kernel coding standards, but while preparing to do the edits for the '50' tests, it occurred to me that adding something like
static int s24_def_bclk_ratio = 60;
module_param(s24_def_bclk_ratio, int, 0644);
and then
416 case SNDRV_PCM_FORMAT_S24_LE:
417 data_length = 24;
418 bclk_ratio = s24_def_bclk_ratio;
419 break;
(for all three default bclk_ratio values) might be generally useful and side-step the need to commit to a particular set of defaults.

The question them becomes: For S16 / S24 / S32, do we build in 'strict' defaults of 32 / 64 / 64 (which make minimal use of 19.2MHz clock but likely compatible with broader range of devices) or 'loose' defaults of 40 / 50 / 80 (which make more use of that clock and remain consistent with the existing defaults), or the proposed 50 / 50 / 100 (to make maximal use of that clock)?

I don't care which defaults are chosen if I can easily tweak them if needed, and it seems like the less risky path if the chosen defaults break something 'out there'.

(btw, does editing a post cause it to be reposted to mailing list?)

[plughplover]

Tested '50'. It works for me for 24-bit capture. So I guess the consensus is:

1a) Proceed with the bug fix, and include an enhancement.
Specifically, change the three values

411 switch (params_format(params)) {
412 case SNDRV_PCM_FORMAT_S16_LE:
413 data_length = 16;
414 bclk_ratio = 40; <== 50 (enhancement)
415 break;
416 case SNDRV_PCM_FORMAT_S24_LE:
417 data_length = 24;
418 bclk_ratio = 40; <== 50 (bug-fix)
419 break;
420 case SNDRV_PCM_FORMAT_S32_LE:
421 data_length = 32;
422 bclk_ratio = 80; <== 100 (enhancement)
423 break;
424 default:
425 return -EINVAL;
426 }

This fixes the 'value to small' bug with 24 bit support, and enhances the default values to make maximal use of the 19.2MHz clock.

1b) Restore 24-bit audio playback support in bcm2708-i2s.c ONLY ref:
4ba7ea9
Specifically, uncomment the single line (and remove xbmc reference text)

783 .playback = {
784 .channels_min = 2,
785 .channels_max = 2,
786 .rates = SNDRV_PCM_RATE_8000_192000,
787 .formats = SNDRV_PCM_FMTBIT_S16_LE
788 THIS ==> // | SNDRV_PCM_FMTBIT_S24_LE : disable for now, it causes white noise with xbmc
789 | SNDRV_PCM_FMTBIT_S32_LE
790 },

2a) Back-port this patch from 3.13 sound/soc/generic/simple-card.c to 3.12 tree
e244bb9

2b) Enable "ASOC Simple Sound card" as loadable module in default build
SND_SIMPLE_CARD = m

Note: This is located in 'make menuconfig' here:
Device drivers
Sound Card Support
Advanced Linux Sound Architecture
ALSA for SoC audio support
ASOC Simple sound card

3. Enable the generic Digital Microphone codec driver module in default build
   SND_SOC_DMIC = m

Note: there is no way to select this from 'make menuconfig' so I am hoping that the build gurus know of some way to simply enable it for the Raspian build.

Thanks to everyone for the input!

[popcornmix]

I believe latest firmware has the suggested changes (part from enabling SND_SOC_DMIC which only seems to exist as a part of OMAP).

Can you test?

[plughplover]

I may be doing something wrong, or perhaps there is some change I unaware of but...

I started with clean NOOBS v1.3.9 (3.12.22+) did
sudo apt-get update
sudo apt-get upgrade
which went fine except for the following error

(gtk-update-icon-cache:9044): GdkPixbuf-WARNING **: Cannot open pixbuf loader module file '/usr/lib/arm-linux-gnueabihf/gdk-pixbuf-2.0/2.10.0/loaders.cache': No such file or directory
This likely means that your installation is broken.

I ignored this and then did sudo rpi-update
which installed 973de697f8f72cb768b7609eae93bf1bdc27ce56
rebooted and now system id's as
Linux raspberrypi 3.12.28+ #713 PREEMPT Fri Sep 19 16:43:32 BST 2014 armv6l GNU/Linux

The thing is, I can't find 'modules/3.12.28+/kernel/drivers/dma/bcm2708-dmaengine.ko'
which I had to load in 3.12.22+ (which then loaded virt-dma.ko) in order to use the i2s driver.

Poking around here https://github.com/Hexxeh/rpi-firmware/commits/master/modules
it seems the module disappears after Hexxeh/rpi-firmware@995a4c0

So, before I invest more time: Am I doing something wrong? Is the 'dmaengine' module no longer needed for i2s? Or is it actually missing?

PS - fyi - simple-card driver IS present.

[hifiberry]

The module has been renamed. Can't remember the name, but you will find it if you look for dma modules.

[plughplover]

Examining this more closely
Hexxeh/rpi-firmware@995a4c0
Is it now built into the kernel image?

[plughplover]

Yup, appears it IS built into the kernel now.
Updated my scripts and code, ran tests, and everything appears to work.

Incorporating items 1 and 2 are a big help!
Will probably want to revisit 3 (dmic codec) later.
I guess this issue can be closed.
THANKS!

BTW, is there a schedule / target for when 3.12.28 will be considered 'done'? I have an enhancement to the i2s module in the works (and want to think about 3 some more). If closing it is not imminent, rolling the addition(s) into the final 3.12.28 might be appropriate...

[popcornmix]

There is no deadline for 3.12.28 being done.
There will be firmware updates once or twice a week and they will include any new commits.
I will also bump to 3.12.29 when it appears (if it doesn't appear to break anything).

Typically minor version bumps from upstream kernel occur quite regularly (interval is usually somewhere between a fortnight and a month).

Submit your PR when it's ready. It may end up in a 3.12.28 or a 3.12.29 kernel, but that doesn't matter.

[popcornmix]

Looks like the white noise in xbmc has returned after these changes:
http://forum.xbmc.org/showthread.php?tid=192380&pid=1799395#pid1799395

I think 4ba7ea9 will need to be reinstated unless someone can suggest a better fix?

[iqaudio]

Florian, if xbmc only why block (if I read this right) 24bit support for everyone?

Is that the suggested "fix"?

So far we've had no reported issues with 24/96 or 24/192 playback.

Gordon

On 22 Sep 2014, at 21:08, popcornmix [email protected] wrote:

Looks like the white noise in xbmc has returned after these changes:
http://forum.xbmc.org/showthread.php?tid=192380&pid=1799395#pid1799395

I think 4ba7ea9 will need to be reinstated unless someone can suggest a better fix?

—
Reply to this email directly or view it on GitHub.

[popcornmix]

Well it depends if the bug is in xbmc, the I2S audio driver or the HifiBerry driver.
I think it's only been reported with Hifiberry DAC, but we haven't got evidence of exactly where the bug is.

[plughplover]

fwiw, http://www.raspberrypi.org/forums/viewtopic.php?p=614050#p614050
says the fix re-enabled 24 bit playback with their Wolfson wm8731 DAC.

I agree that blocking this for everyone in the driver is not the correct approach. If something along those lines MUST be done, I suggest the more targeted approach I proposed a few posts earlier (see "1b"), which was to re-enable it in the i2s driver but leave it disabled in pcm5102a.c. (Have there have been xbmc reports against any other DACs?)

However, while admittedly not knowing the specifics of how xbmc interfaces with alsa, can't they just put a constraint in .asoundrc until they track it down? Something along the lines of
pcm.!default {
type hw
card 1
format s16_le}

Finally, I took a quick look at the 5102a data sheet, the pcm5102a driver, and the hifiberry_dac driver, and while it looks ok, I couldn't help but wonder what would happen if this
37 unsigned int sample_bits =
38 snd_pcm_format_physical_width(params_format(params));
39
40 return snd_soc_dai_set_bclk_ratio(cpu_dai, sample_bits * 2);
used snd_pcm_format_width instead, which page 14 of data sheet implies should also work.

[popcornmix]

For now I've added 617139f to 3.16 branch (used in openelec build that had the bug report). I'll see if the original bug report is resolved and if any other I2S sound card users report an issue.

[plughplover]

In case my 'wondering' wasn't clear, with existing code 16/24/32 bit yields bclk 32/64/64 but with other call yields 32/48/64. According to page 14, both should work, but (hazarding a guess) if the board is using 'System Clock PLL Mode' (page 12) (i.e. synthesized sclk), well hmmm...

fwiw, I also read ALL the threads that user linked to and frankly it isn't clear to me if their problem(s) are related to this specific issue. For example they are also having a problem 'aplay'ing 16 bit audio.

[JamesH65]

@popcornmix Difficult to track whether this is fixed from the comment above. Can it be closed?

[popcornmix]

Yes I believe original issue was reported as solved here: #681 (comment)