Skip to content

Possible I2S master/slave bug in RPi3 #1321

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
bonezuk opened this issue Mar 3, 2016 · 41 comments
Closed

Possible I2S master/slave bug in RPi3 #1321

bonezuk opened this issue Mar 3, 2016 · 41 comments

Comments

@bonezuk
Copy link

bonezuk commented Mar 3, 2016

Hi

I am currently investigating a number of issues on the rpi-4.1-y branch within our sound drivers concerning the playback of sound via our HiFiBerry Digi+ and DAC-Pro on the new RPi3. After a series of initial investigations we think there might be some conflict on the I2S bus line between the bcm2708_i2s and the (pcm512x and hifiberry_dacplus) modules.

Both the Digi+ and DAC-Pro modules set the

dai->dai_fmt = SND_SOC_DAIFMT_CBM_CFM |SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF;

With hifiberry_dacplus module it is initially configured to

dai->dai_fmt = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF | SND_SOC_DAIFMT_CBS_CFS;

This is done so the PCM512x on the DAC+ acts as an I2S slave. On the start in snd_rpi_hifiberry_dacplus_init it detects that wither or not a given board is a DAC-Pro and thus changes the dai_fmt to master (SND_SOC_DAIFMT_CBM_CFM).

Also the same hifiberry_dacplus module works with the DAC+ sound card where it is the I2S slave.

2016-03-02_17-15-04

The image shows the output from my logic analyser of the I2S bus where it shows errors in reading the samples due to an incorrect bit number in a given sample. This also helps tie in with explaining what is actually heard during playback: i.e. Sometimes the music can be heard with distortion, other times there is too much distortion to hear any music.

My best guess, at this moment in time, is that a bug has been introduced in the bcm2708_i2s module or associated architecture somewhere between 4.1.7 and 4.1.18+7 where-by when the dai_fmt flag is set to SND_SOC_DAIFMT_CBM_CFM then both the bcm2708 and associated the snd_soc_codec module both try to send an I2S clock signal. Where as, I would expect only the snd_soc_codec DSP to configure to setup the I2S clock signal.

Another line of investigation that I am looking at is updating and ensuring that our device drivers reflect the various changes to the device tree code. It maybe possible that because of this something is not being initialized write in the bcm2708_i2s module and thus the conflict in the I2S bus.

I'm also currently expanding my investigation of this issue to cover the 4.4 kernel branch.

Let me know if you have ideas but how I can go about resolving this issue?

Cheers,

Stuart

@pelwell
Copy link
Contributor

pelwell commented Mar 3, 2016

Are you saying this fault is only seen on a Pi3, not a Pi2?

@clivem
Copy link

clivem commented Mar 3, 2016

Yes, I've just verified this on Pi3 with 4.4.y, Geekroo Digi+, using hifiberry digi driver.....

[ 60.918339] bcm2835-i2s 3f203000.i2s: hw_params: clk_src=PLLD, clock_freq=500000000, target_freq=2822400, sampling_rate=44100, bclk_ratio=64, mash=3, divi=177, divf=631
[ 60.919527] bcm2835-i2s 3f203000.i2s: I2S SYNC error!

@pelwell
Copy link
Contributor

pelwell commented Mar 3, 2016

Can you try adding gpu_freq=300 to config.txt?

@clivem
Copy link

clivem commented Mar 3, 2016

Still ....
kernel: bcm2835-i2s 3f203000.i2s: I2S SYNC error!
..... with gpu_freq=300 to config.txt?

@clivem
Copy link

clivem commented Mar 3, 2016

Kind of annoyed with myself that I didn't notice this Mon/Tues/Wed.... I tested a bunch of DACs with Pi3, including IQ DAC+ and Berry DAC+, but of course, everything I tested was SND_SOC_DAIFMT_CBS_CFS.......

@bonezuk
Copy link
Author

bonezuk commented Mar 3, 2016

I'm currently working on getting my testing environment and various kernel build workflows (so I looking at can pull the latest rpi-4.1.y and rpi-4.4.y via my fork of the kernel) sorted out such that I can properly go through and look at this stuff on both the RPi, RPi2 and RPi3. So hopefully, I'll be able to give more feedback about what I am seeing soon.

@pelwell
Copy link
Contributor

pelwell commented Mar 3, 2016

Thanks, Stuart - the more information you can give us the quicker we'll get to the bottom of this.

@clivem
Copy link

clivem commented Mar 5, 2016

@bonezuk Stuart, totally unscientific I know, but before you waste time chasing invisible angels, just wanted to let you know that the Geekroo Digi+ board that wouldn't work on Pi3B with hifiberry_digi driver, now works for me. I did several updates yesterday, including a VC firmware (bootcode/fixup/start) update. Can't be more specific than that.

@hifiberry
Copy link

@clivem Is the new firmware that is working for you already available via rpi-update?

@HiassofT
Copy link
Contributor

HiassofT commented Mar 6, 2016

@hifiberry I just got a report that the March 04 firmware (from rpi-update or directly from https://github.com/raspberrypi/firmware) seems to fix audio distortion on the Cirrus logic card - which also acts as an I2S master.
http://openelec.tv/forum/124-raspberry-pi/69870-wolfson-audio-card-support?start=270#159290

@bonezuk
Copy link
Author

bonezuk commented Mar 6, 2016

I've pulled both latest versions of the rpi-4.1.y and rip-4.4.y branches and got upto date kernel builds ready to test. Afraid I'm away from my desk this weekend, so haven't been able to test this yet. Back at my desk tomorrow so will post what I find. Looking forward, got my fingers crossed that it has hopefully "magically" resolved itself 😅

@clivem
Copy link

clivem commented Mar 6, 2016

@bonezuk Stuart, I'm pretty sure it was the (proprietary VC/boot) firmware update, 028b1f6......., that @popcornmix pushed on Friday, which caused that Digi+ player to "magically" start working on my Pi3B.

@HiassofT
Copy link
Contributor

HiassofT commented Mar 7, 2016

Got an updated report, the newer GPU firmware seems to have improved distortion issues but not completely fixed them
http://openelec.tv/forum/124-raspberry-pi/69870-wolfson-audio-card-support?start=270#159317

@bonezuk I had a quick look at the LA screenshot you posted but failed to spot where the decoding error should come from. I manually counted 16 clock transitions per frame and LRCLK/DATA change on the falling edge of the clock as they should. Had you maybe setup I2S decoding to sample on the wrong clock edge (falling instead of rising) or did I miss something or counted wrong?

I don't have my RPi3 yet so unfortunately I can't do any testing myself ATM...

@bonezuk
Copy link
Author

bonezuk commented Mar 7, 2016

I've just finished testing the latest 4.1.y kernel build. Unfortunately I am still experiencing exactly the same issue. To add additional information I am only seeing this problem on the RPi3. On the RPi2 and RPi playback is fine.

@HiassofT I know, I see the same, however the logic analyser is showing an error. I am going to study the readings more closely... Which was making me think that when the dsp codec is master (hence producing the I2S signal) there was a bug where by the bcm module was was also producing a conflicting I2S signal.... But just a guess.

I'll try the new GPU firmware and see if that changes anything.

I've taken some new readings. So this is the output from the Digi+ connected to the RPi3. (The original image post was from the output of the DAC-Pro)... However both show this "Error: bits don't divide evenly". I've also uploaded the recorded output distortion.
i2s_digi_1
i2s_digi_2

record_digi1.zip

@HiassofT
Copy link
Contributor

HiassofT commented Mar 7, 2016

@bonezuk could you try with a synthetic wav file with some known, simple sample values - eg alternating between 0x8001 and 0x7ffe or something like that can be easily recognized on the logic analyzer and decoded by us humans.

Recording the output from the Digi with a PC using SPDIF in would also be interesting - then we have decoded samples and can also compare them easily.

Maybe we can find a pattern, ie which bits get mangled, if there's some phase shift between samples and LRCLK or something like that.

@bonezuk
Copy link
Author

bonezuk commented Mar 7, 2016

I tried the problem with the latest firmware along with the 4.4.y and still exactly the same issue.

@HiassofT I'll get some new images of the logic analyser with more human readable signals.

@HiassofT
Copy link
Contributor

HiassofT commented Mar 7, 2016

FYI: just saw that a new firmware was published an hour ago

As a reference I checked the I2S on my RPi2+Cirrus card, channel 1 transmitting 0x8001 and channel 2 0x7ffe. First picture is the full frame then zoomed in parts when LRCLK changes. Maybe this'll help you testing

8001-7ffe
8001-7ffe-zoom1
8001-7ffe-zoom2
8001-7ffe-zoom3

@popcornmix
Copy link
Collaborator

Does core_freq=250 make a difference? That's the obvious difference between Pi2 and Pi3 (although I'd assume that overclocked Pi2's were common enough that we'd have seen this).

@hifiberry
Copy link

@popcornmix core_freq=250 doesn't change the behaviour. Looking at my logic analyzer dumps it looks like LRCLK and BCLK are correctly handled.
@HiassofT It looks like DATA is mangled somehow. I just tried to create a WAV file with 2 values, but couldn't get it done with audacity. Can somebody provide a WAV file with a "pure" square wave (I guess, audacity does some filtering, which makes sense from an audio point of view)? Or can someone provide a WAV file (or command line) with a single DC value?

@juandmi
Copy link

juandmi commented Mar 8, 2016

here is a wav file with a "pure" square wave: https://www3.nd.edu/~dthain/courses/cse20211/fall2013/wavfile/square.wav . hope it helps.

@HiassofT
Copy link
Contributor

HiassofT commented Mar 8, 2016

@hifiberry here's a crude but simple way to generate the wav I used for testing:

perl -e 'print "\x01\x80\xfe\x7f" x1000000' | \
sox -t raw -c 2 -b 16 -L -e signed -r 48000 - test-8001-7ffe.wav

It would be interesting to play around with different values, that should give us a clue how the bits are mangled. As a first start the 0x8001 / 0x7ffe samples should be quite fine.

You could also try "\x01\x80\x01\x80\xfe\x7f\xfe\x7f" if you want a real square wave, not just fixed DC offsets.

@HiassofT
Copy link
Contributor

HiassofT commented Mar 9, 2016

Just received my RPi3, did some tests (latest 4.4 kernel and firmware from March 7th) and sure enough the data transmission isn't aligned to LRCLK. On repeated plays the data "shifts" by random amounts. Now the big question is why and how to avoid that.

Maybe it has something todo with the "sync error" and the RPi3 being faster (datasheet says we have to wait 2 PCMCLK cycles to synchronize, comments in the i2s code says the SYNC bit method doesn't work in slave mode and we rely on this timeout=1000 loop). But maybe it's something completely different, I'll try to investigate this.

Here are some screenshots.

First, as a reference, the screenshot from my RPi2 (same one as I posted above):
8001-7ffe

And here 3 screenshots of repeated plays of the same test file on the RPi3:
rpi3-fw0307-1
rpi3-fw0307-2
rpi3-fw0307-3

@hifiberry
Copy link

That was what I supposed (didn't had the time to check it yet).

@HiassofT
Copy link
Contributor

HiassofT commented Mar 9, 2016

I might have found something:

Loading either the pi3-disable-bt or the pi3-miniuart-bt overlay seems to fix the issue with PCM data not being aligned to LRCLK. The remaining question is why :)

My setup:

  • kernel 4.4, commit 70eab1b plus my cirrus patches
  • GPU firmware from March 9th (3a754304b032a5298ee7889b179c667bbc75dec5) - also tested with the old firmware from February 19th
  • no /boot/cmdline.txt
  • /boot/config.txt:
dtdebug=1
dtoverlay=i2s-mmap
dtoverlay=rpi-cirrus-wm5102
dtoverlay=pi3-miniuart-bt

or

dtdebug=1
dtoverlay=i2s-mmap
dtoverlay=rpi-cirrus-wm5102
dtoverlay=pi3-disable-bt

@hifiberry @bonezuk @clivem could you give that a try on your setups?

@HiassofT
Copy link
Contributor

A random thought: maybe some bug in pinmux/pinctrl? The I2S GPIOs 18-21 in /sys/kernel/debug/pinctrl/3f200000.gpio/pinmux-pins look fine both with and without BT but if the LRCLK GPIO was somehow disconnected from the I2S controller that could explain the issues.

@bonezuk
Copy link
Author

bonezuk commented Mar 10, 2016

Just a quick update, I've been working on getting my main diagnostic tool built on the RPi which has some nice tools that I had previously written for diagnosing I2S connections (tone and fixed value, for any frequency). Also been doing a careful re-read of the I2S specification so I fully understand the specifics of I2S and think through your results @HiassofT

So from my understanding, the problem seems to be that the RPi3 broadcom's chip is not sending the serial audio data in sync with the DAC's BCLK and LRCK signals. "Why?" being the $64,000 question. I'm going to try and see if I can find the rate at which the audio data bits go out of sync with the DAC clocks, for each of the various common playback frequencies and bit depths (maybe this might shed some light on adjustments in clock or frequency timing).

Also I am wondering if the bluetooth circuitry or DMA channel is somehow mixed with the I2S DMA channel (or something).... if by disabling the bluetooth (which is new to the RPi3) fixes this issue then that would be very interesting. So I'll also look into this first thing tomorrow and see if I am able to reproduce this sync fix by disabling the bluetooth.

@clivem
Copy link

clivem commented Mar 10, 2016

Hmmmmm... Didn't I read somewhere about Bluetooth A2DP profile being supported???? That would imply I2S wired to Bluetooth chip for audio via A2DP....... Without a circuit diagram or specifications...... @pelwell, is I2S wired to the Bluetooth chip? Where from? Is this "hidden"... How is configured... From VC side of things???? (Just random thoughts, thought out loud.)

@clivem
Copy link

clivem commented Mar 10, 2016

@HiassofT Confirm, either disable BT with dtoverlay=pi3-disable-bt or....
dtoverlay=pi3-miniuart-bt
core_freq=250
... and change ttyAMA0 refs in hciuart.service to ttyS0 to have both working BT and I2S (codec is master) audio, at same time.

@bonezuk
Copy link
Author

bonezuk commented Mar 10, 2016

Adding the dtoverlay=pi3-disable-bt also resolves the whole issue for playback on both Digi+ and DACPro. Fantastic work in spotting that @HiassofT. At least it gives me a solution that I can pass on to our customers.

@HiassofT
Copy link
Contributor

I think I found the culprit:

bcm2710-rpi-3-b.dts pulls in bt_pins via uart0:

&uart0 {
        pinctrl-names = "default";
        pinctrl-0 = <&uart0_pins &bt_pins>;
        status = "okay";
};

And bt_pins enables I2S/PCM on GPIOs 28-31 (it's even in the comment) - so I2S/PCM is enabled both on GPIOs 18-21 and GPIOs 28-31. Surely this must cause some internal clash for input signals :)

        bt_pins: bt_pins {
                brcm,pins =     <28 29 30 31 43>;
                brcm,function = <6 6 6 6 4>;   /* alt2:PCM alt0:GPCLK2 */
                brcm,pull =     <0 0 0 0 0>;
        };

The bt miniuart/disable overlays both change uart0 to only use uart0_pins, so bt_pins doesn't get pulled in when either of them is loaded.

If I remove the bt_pins reference from bcm2710-rpi-3-b.dts audio works just fine:

--- a/arch/arm/boot/dts/bcm2710-rpi-3-b.dts
+++ b/arch/arm/boot/dts/bcm2710-rpi-3-b.dts
@@ -95,7 +95,7 @@

 &uart0 {
        pinctrl-names = "default";
-       pinctrl-0 = <&uart0_pins &bt_pins>;
+       pinctrl-0 = <&uart0_pins>;
        status = "okay";
 };

There's probably some reason for having bt_pins in the DT (whatever the actual connections to the BT chip are), but I think that should better be configurable/optional so it doesn't clash with I2S. And if this pin routing is needed for some BT functionality like audio the miniuart-bt overlay probably shouldn't change that :)

@pelwell could you have a look at that

@clivem
Copy link

clivem commented Mar 10, 2016

bt-audio, should be disabled by default, and needs to be enabled via overlay. ie. up to now it's always been, to enable DAC via I2S, one needs to reference a dtoverlay for the DAC. I think, we need a dtoverlay=bt-audio, and it needs to be explicitly enabled from config.txt, just like any other sound card would be......

@pelwell
Copy link
Contributor

pelwell commented Mar 11, 2016

Sorry for the lack of responsiveness - I've been laid up in bed for 24 hours with labyrinthitis. When I can I'll remove the 4 PCM pins from the bt_pins section - the GPCLK2 must remain. Alternatively, if someone else can create a PR I'll merge it.

popcornmix added a commit to raspberrypi/firmware that referenced this issue Mar 15, 2016
See: #564

kernel: Remove I2S config from bt_pins
See: raspberrypi/linux#1321

kernel: bcm2835-sdhost: Workaround for slow sectors
See: raspberrypi/linux@20fe468

firmware: pwm_sdm: first pass at optimisation
See: https://www.raspberrypi.org/forums/viewtopic.php?f=29&t=136445

firmware: arm_loader: Allow frequency cap to go down to 300MHz when over temperature
See: raspberrypi/linux#1337

firmware: dtoverlay: Remove support for space/tab separators
firmware: host_applications: Add dtoverlay app
firmware: dtoverlay: Several small improvements
See: https://www.raspberrypi.org/forums/viewtopic.php?f=107&t=139732

firmware: Add gpioman changes to correctly handle dt-blob.bin for gpio_expander gpios

firmware: Updated dt-blob.dts to include Pi 3
See: https://www.raspberrypi.org/forums/viewtopic.php?f=107&t=140125
popcornmix added a commit to Hexxeh/rpi-firmware that referenced this issue Mar 15, 2016
See: raspberrypi/firmware#564

kernel: Remove I2S config from bt_pins
See: raspberrypi/linux#1321

kernel: bcm2835-sdhost: Workaround for slow sectors
See: raspberrypi/linux@20fe468

firmware: pwm_sdm: first pass at optimisation
See: https://www.raspberrypi.org/forums/viewtopic.php?f=29&t=136445

firmware: arm_loader: Allow frequency cap to go down to 300MHz when over temperature
See: raspberrypi/linux#1337

firmware: dtoverlay: Remove support for space/tab separators
firmware: host_applications: Add dtoverlay app
firmware: dtoverlay: Several small improvements
See: https://www.raspberrypi.org/forums/viewtopic.php?f=107&t=139732

firmware: Add gpioman changes to correctly handle dt-blob.bin for gpio_expander gpios

firmware: Updated dt-blob.dts to include Pi 3
See: https://www.raspberrypi.org/forums/viewtopic.php?f=107&t=140125
@pelwell
Copy link
Contributor

pelwell commented Mar 15, 2016

Thanks to the efforts of everyone on this thread I think this issue has been resolved. OK to close?

@clivem
Copy link

clivem commented Mar 15, 2016

@pelwell Phil, please don't take this the wrong way, but a lot of things are now starting to feel like a "sweeping under the carpet" exercise.... With the lack of any released documentation, circuit diagram, etc. etc. Were my guesses correct, that the I2S from the SoC is now also directly wired to the BT chip as well as to GPIO header? I'm guessing what has caused this to be resolved, (LOL and my fingerprints are on the pinctrl patch), and why it needing resolving to start with, I certainly haven't been given an explanation! ;)

@pelwell
Copy link
Contributor

pelwell commented Mar 15, 2016

No cloak and dagger, no "sweeping under the carpet", just two names for the same thing. The PCM function that could have been used for Bluetooth (but is now unlikely to be), and the I2S function used for external codecs are one and the same. The only unknown remaining is how the I2S function almost worked when multiply mapped.

I think you have the wrong idea about Raspberry Pi. Your comments make it sound as though you see us as some giant corporation withholding information from its poor users for whom we care little. The truth is that we are a very small company who has achieved a lot with scant resources, and that's in part because of the efforts of many members of the community who pitch in and help with their specialist knowledge.

@clivem
Copy link

clivem commented Mar 15, 2016

Thank you for the clarification.

I think you have the wrong idea about Raspberry Pi.

Some of the stuff I have seen over the last 6 weeks, like the fact their is an "inner circle" who are privy to information others are not, commercial partners are more important than community and users, and who got early access to hardware before launch and who was left holding their dick with their pants around their ankles, GPL'd kernel binaries being made available with source code to follow, the fact that any sort of clarification was ever needed to start with over the LICENSING for distribution of proprietary firmware....... Scant resources.... Right! Community needs to pitch-in more? Oh boy, you guys are hilarious!

@JamesH65
Copy link
Contributor

Wow, way to piss off people you need to help you.

On 15 March 2016 at 22:20, Clive Messer [email protected] wrote:

Thank you for the clarification.

I think you have the wrong idea about Raspberry Pi.
Some of the stuff I have seen over the last 6 weeks, like the fact their
is an "inner circle" who are privy to information others are not,
commercial partners are more important than community and users, and who
got early access to hardware before launch and who was left holding their
dick with their pants around their ankles, GPL'd kernel binaries being made
available with source code to follow, the fact that any sort of
clarification was ever needed to start with over the LICENSING for
distribution of proprietary firmware....... Scant resources.... Right!
Community needs to pitch-in more? Oh boy, you guys are hilarious!


You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#1321 (comment)

@Ruffio
Copy link

Ruffio commented Aug 17, 2016

@bonezuk has your issue been resolved? If so, please close this issue. Thanks.

neuschaefer pushed a commit to neuschaefer/raspi-binary-firmware that referenced this issue Feb 27, 2017
See: raspberrypi#564

kernel: Remove I2S config from bt_pins
See: raspberrypi/linux#1321

kernel: bcm2835-sdhost: Workaround for slow sectors
See: raspberrypi/linux@20fe468

firmware: pwm_sdm: first pass at optimisation
See: https://www.raspberrypi.org/forums/viewtopic.php?f=29&t=136445

firmware: arm_loader: Allow frequency cap to go down to 300MHz when over temperature
See: raspberrypi/linux#1337

firmware: dtoverlay: Remove support for space/tab separators
firmware: host_applications: Add dtoverlay app
firmware: dtoverlay: Several small improvements
See: https://www.raspberrypi.org/forums/viewtopic.php?f=107&t=139732

firmware: Add gpioman changes to correctly handle dt-blob.bin for gpio_expander gpios

firmware: Updated dt-blob.dts to include Pi 3
See: https://www.raspberrypi.org/forums/viewtopic.php?f=107&t=140125
@Dranodrinker
Copy link

Sorry if this is annoying guys, but I am very new to linux and raspberry pi's in general. What exactly do you have to do to modify the .dtb files? compile the .dts into a .dtb? Create a patch using diff? Just edit the .dtb somehow? Or even just copy the dtb off git to my /boot/ ? Again, sorry if these questions are very uneducated, I am just having a hard time finding the info I need to figure it out on my own. Thanks

@pelwell
Copy link
Contributor

pelwell commented Apr 16, 2017

If you Google "device tree", the seventh hit is (ahem) my documentation on the Raspberry Pi site: https://www.raspberrypi.org/documentation/configuration/device-tree.md

It doesn't cover everything, but it should give a reasonable introduction to the subject.

@JamesH65
Copy link
Contributor

Closing this issue as questions answered/resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

10 participants