HiFiBerry Amp+ 48kHz not working in 4.9 #2016
Comments
|
Is this just on the Amp or also Dac/Digi? |
|
I'm currently not aware of any issues. There were a few changes between 4.4 and 4.9 that could be The latter change now leads to a "jumping" I2S clock when RPi eg on 48kHz with 64fs the clock is derived from 19.2MHz with This could be problematic, eg when the codec runs a PLL off that On kernel 4.4 the 19.2MHz OSC clock was only used in integer @hifiberry could you do a test on kernel 4.9 with the hacky patch below - it's a quick hack to run all clocks off PLLD and disable OSC, I2S clock for 48kHz should be the same as with kernel 4.4 BTW: of course, the issue could be caused by something else, these were just the first things that came to my mind. |
|
Thanks for this. I will test this. This clock change could be the cause. What was the specific reason to move to the other clock source in 4.9? Having a bit clock with a lot more jitter seems not to be a good thing. |
|
Ok, I can confirm that the clock change fixes the problem. A clock that has a jitter of almost 15% really is a problem for the PLL in the TAS5713. While PLLs of other chips might be a bit more robust, I don't think it's a good idea to have that much jitter on the signal if there are better clocks available. @HiassofT How can we go back to the old behaviour? Can the quick hack above be used or does it need major changes in the clock code? |
|
I don't think the above change is any more hacky than the patch that added those lines - it's just a refinement. If there are no adverse effects of removing OSC as a potential source then I'm happy to make that change official, but it would be nicer to allow integer divisors of OSC. @anholt ? |
|
As all downstream drivers here would only use OSC in integer mode at 8kHz/16bit/stereo (both with 4.4 and 4.9 kernels) I'd say restricting PCM clock to only the PLLD parent is indeed a proper fix for the issue. Using OSC in fractional mode is causing a real bug and the benefit of keeping OSC as a possible parent only for 8kHz/16bit is next to zero. So, let's apply this here. If someone else wants to go down the rabbit hole of implementing a generic PCM clock selection scheme that can be upstreamed and follows the "only use fractional divisors on the highest frequency clock source" rule he/she is welcome, but that someone won't be me :) |
|
The generic parent selection scheme sounds nice, but I would be happy to see a patch sent upstream that limited PCM to PLLD_PER only until someone wanted to do better. |
Sometimes I just want to bang my head against the wall out of complete frustration! By the minute, it becomes more and more obvious that every single minute I spent last summer was a complete waste of time!!!!!!!!!!!!!!! |
|
@msperl Martin, did anything ever get beyond the "talking about it stage", regarding the ability to be able to have audio driver request specific things via the clock framework, eg. MASH1/2/3/4 for fractional dividers, only integer dividers otherwise fallback to PLLD parent? |
|
it did not go any further - besides that "generic" case in simple-card that could have easily get extended to include methods exported from i2s to handle those kind of things. Obviously the "policy" could also get added to the bcm2835-i2s driver itself to get configured via configfs as this is not supposed to describe HW, but policy. |
That's kind of dead in the water, in as much as it was already vetoed upstream.
I had a "simple" patchset that could be used via overlay, (for MASH selection and prefer int div), much as the enable MMAP overlay could be used with bcm2835-i2s driver up to 4.4.y..... Abandoned post 4.4.y with the switch to clockmgr..... But now that the clocks are not directly accessible from the platform driver, trying to resurrect that and having a bunch of clock logic code in the I2S driver, wouldn't fly anyway...... |
|
I think the real problem is that the clock driver doesn't model jitter. If bcm2835_clock_choose_div_and_prate returned a range of frequencies then the best rate selection code could optionally minimise the difference from the ideal frequency to the worst-case (rather than the average), the choice of scheme being fixed for each clock. |
|
Oh yeah.... That's flippin' genius! Disable OSC as a clock source all PCM, even when it could be chosen with an integer divider...... (That's a regression over rpi-4.4.y, for any hardware for which OSC with an integer divider would have been chosen.) The best part about this, is that I had a Skype conversation with Daniel a year ago, (last week of May/first week June, thereabouts in 2016), where he was made aware of the coming changes, that they had been fully tested with his original 'B' DAC, Digi, DACLight, DAC+, DAC+ Pro, (as part of the wider testing I did with 20+ boards), but I couldn't test with anything else, like his AMP board, as I didn't have the hardware, and that he should test any hardware of his I didn't have. A year later, "I've been informed that 48kHz on the HiFiBerry Amp+ isn't working with the latest kernel anymore." So much for having done any testing a year ago! |
|
I have the code open now with a view to implementing a minimal-deviation search, initially just for "pcm". |
|
@pelwell it'd probably be easiest to change the clock selection for the PCM clock to allow fractional mode only on the highest frequency parent and use integer mode on all others - only one additional loop over the parents and a slight change to the "find best parent" loop is needed:
As this follows the recommendation from the BCM2835 datasheet (p. 105) it should be easy to argue with upstream why it's implemented that way
|
|
I had just decided to add a low_jitter flag to the clock data, set only for PCM initially which scores each parent by:
In this way the existing logic of choosing the clock closest to the ideal but without exceeding it can still be used, and the fastest parent doesn't have to be treated specially. |
|
Can you try this patch? It chooses PLLD_PER for me when playing a 48KHz clip, but it should still select XOSC it if there is a suitable integer divisor or if the jitter is very small. |
|
@pelwell thanks a lot, your patch is looking good! I hacked up a quick test to check the common sample- and bclk-rates and the output looks as expected. If someone wants to play with it, the tree is here: When bcm2835-i2s is loaded it checks the rates and prints the result in kernel log. |
|
For the curious, the output from @HiassofT's test (thanks!) is: and Although the patch doesn't explicitly preclude the use of fractional divisors with OSC, in practice that is what it does. |
|
Any objections before I merge the patch? @clivem? |
|
@pelwell Sorry Phil, I'm not in a position to do any testing at the moment, (let alone what I was doing last year)...... If it results in the OSC with an integer divider being preferred, when possible, and PLLD for everything else..... That's cool. And results in the same behavior as was had back in the rpi-4.4.y branch, that was the default behavior for many years, so no complaints from me, just a thank you for the doing the work! |
|
Thanks, Clive. I've merged to rpi-4.9.y, and I'll start the upstreaming process. |
|
@pelwell thanks a lot! BTW: could you push the fix to the 4.11 and 4.12 trees as well? |
|
I was going to give it a day or two after the next firmware release, but the non-stable trees are easy enough to patch - done. 4.11 and 4.12 also needed the patch to fix #1949. |
rpi-4.9.y commit 76527b4e6a5dbe55e0b2d8ab533c2388b36c86be This fixes 48kHz samplerate not working on HifiBerry Amp+. See raspberrypi/linux#2016 This patch can be dropped on the next RPi kernel bump. Signed-off-by: Matthias Reichl <[email protected]>
rpi-4.9.y commit 76527b4e6a5dbe55e0b2d8ab533c2388b36c86be This fixes 48kHz samplerate not working on HifiBerry Amp+. See raspberrypi/linux#2016 This patch can be dropped on the next RPi kernel bump. Signed-off-by: Matthias Reichl <[email protected]>
kernel: dwc_otg: make periodic scheduling behave properly for FS buses See: raspberrypi/linux#2020 kernel: HID: usbhid: extend polling interval configuration to joysticks See: raspberrypi/linux#2036 kernel: clk: bcm2835: Minimise clock jitter for PCM clock See: raspberrypi/linux#2016 kernel: Add mpu6050 device tree overlay See: raspberrypi/linux#2031 kernel: config: Add CONFIG_IPV6_SIT_6RD See: raspberrypi/linux#1598 kernel: config: Add CONFIG_IPV6_ROUTE_INFO See: raspberrypi/linux#1957
kernel: dwc_otg: make periodic scheduling behave properly for FS buses See: raspberrypi/linux#2020 kernel: HID: usbhid: extend polling interval configuration to joysticks See: raspberrypi/linux#2036 kernel: clk: bcm2835: Minimise clock jitter for PCM clock See: raspberrypi/linux#2016 kernel: Add mpu6050 device tree overlay See: raspberrypi/linux#2031 kernel: config: Add CONFIG_IPV6_SIT_6RD See: raspberrypi/linux#1598 kernel: config: Add CONFIG_IPV6_ROUTE_INFO See: raspberrypi/linux#1957
|
Should be in latest rpi-update kernel. Please test and close if happy. |
|
Thanks for the great work! This is working and the issue can be closed. |
I've been informed that 48kHz on the HiFiBerry Amp+ isn't working with the latest kernel anymore. So I started debugging this.
The issue seems to be related to the 4.9 kernel. The latest 4.4 kernel is still working.
The easiest way to reproduce the issue is using sox:
Working with 4.4:
Not working with 4.9 (this is the first 4.9 release from rpi-update)
I had a short look on the I2S signal lines and there is no visible difference between 4.4 and 4.9. As 44.1kHz is working fine, I also expect that the I2C communication is working fine (did not yet check this with a logic analyser)
AFAIK, the driver itself hasn't changed between 4.4 and 4.9
Any idea what has changed in 4.9 ?
The text was updated successfully, but these errors were encountered: