backport of upstream i2s and clock #1439
Conversation
Enable the clock manager by default. Signed-off-by: Martin Sperl <[email protected]>
Removed clock registers from reg and added clock to i2s devicetree node. Signed-off-by: Martin Sperl <[email protected]>
This reverts commit 3de2328.
This reverts commit 027ba80.
This reverts commit 7b44818.
This reverts commit 433841f.
This reverts commit e2a75ce.
…rect values" This reverts commit c61a7ae.
This reverts commit 569baa8.
Cleanup of includes so that they are ordered alphabetically. Signed-off-by: Martin Sperl <[email protected]> Signed-off-by: Mark Brown <[email protected]>
Since the move to the new clock framework with commit 94cb7f7 ("ARM: bcm2835: Switch to using the new clock driver support.") this driver was no longer functional as it was manipulating the clock registers locally without going true the framework. This patch moves to use the new clock framework and also moves away from the hardcoded address offsets for DMA getting the dma-address directly from the device tree. Note that the optimal bclk_ratio selection to avoid jitter due to the use of fractional dividers, which is in the current version has been removed, because not all devices support these non power of 2 sized transfers, which resulted in lots of (downstream) modules that use: snd_soc_dai_set_bclk_ratio(cpu_dai, sample_bits * 2); Signed-off-by: Martin Sperl <[email protected]> Signed-off-by: Mark Brown <[email protected]>
|
@msperl Please can you add a quick hack patch to clock manager to only allow use of OSC and PLLD and I will test again. Forget about trying to cat anything from debugfs, system is foobar. USB gone, SDCARD gone!!!! I'm left with this on the screen of serial console on a Pi2B. [ 197.902760] mmc0: timeout waiting for hardware interrupt. |
|
I fear I guess what it could be... Are you removing the driver or something? Switching clocks? I have seen something similar (minus the display dump) when loading the uart as a module... It may take me some time to get a quick working hack... Actually I got a hack, but it is not worth it - something that I had reported upstream earlier about the clock framework seems to bite us again... Essentially we have no "customer" for the parent PLLs initially: When we use any of those clocks as parent, then the clk_enable_count increases. @anholt : I had submitted a patch for that, but you turned that down because it interfered with something. So what is the solution we want to go with? As a workaround I guess I could set up the clock for the uart correctly - then that pll clock_count would be at least 1... As for the quick hack to limit to use xosc only: |
This adds 24 bit support to the I2S driver of the BCM2835 Code ported from bcm2708-i2s driver in Raspberry Pi tree. Signed-off-by: Florian Meier <[email protected]> Signed-off-by: Matthias Reichl <[email protected]> Signed-off-by: Martin Sperl <[email protected]>
We only need to enable the clock if we are a clock master. Code ported from bcm2708-i2s driver in Raspberry Pi tree. Original work by Zoltan Szenczi. Signed-off-by: Matthias Reichl <[email protected]> Signed-off-by: Martin Sperl <[email protected]>
|
@msperl Quick update..... LOL. I'll take the "dirty" hack until you and Eric come up with a solution and bring PLL's back into play. Who cares whether the music plays back at the wrong speed! (Just kidding....) Lock-ups, and having to re-image trashed SDCARD's are a thing of the past! ;) Obviously, I never had an issue with any board that was running master for bit clock and frame clock with the new code, like the Digi and the Takazine. (They aren't enabling/preparing clocks). That's one box that can be ticked. As for slave mode, with Pi as clock master, progress is progress! Good job Martin! I'd suggest that as much as we might want to get other people involved with testing, Phil doesn't pull the i2s-backport into a "next" branch until you have a solution for getting PLL clock(s) back into the the selection process, without any negative consequences. Just get the dma-backport pull into a "next" branch for now........ |
|
Couple of thoughts.... Revert "bcm2835-i2s: Enable MMAP support via a DT property" Probably want to revert the dt parts as well as code. |
|
And, if you have to work on clk code, and not too much trouble, any chance you can add a dev_dbg in clock selection to show, requested_freq, actual_freq, clk_src, divi, divf, and mash. Similar to what I have kludged into old i2s driver clock selection code, so when I compare side by side, old to new, with 2 players in sync group and tailing the journals on both, I can see exactly what I'm getting old vs new, with regard to clocks.....
|
|
@msperl Am I not allowed a divi of 1 with OSC? I'm requesting 19M2. (192k x bclk_ratio=100). Music playing at half speed, so figure I'm getting divi=2. Not possible to use OSC for PCM clk without a divider>1? Using 19M2 OSC with INT and FRAC divisors... |
|
@clivem until we fix the count issue you can always enable KMS, which claims some clocks and does not release them - pll never gets stopped. There exists a patch to expose the parent name, but it did not get accepted (as it is assumed to be too much information - I guess). The reginfo still gives you all (but encoded) - and fortunately this got accepted... As for divider of < 2 the clock hw does not support that for any clock with mash support (this was actually a point discussed in the long original thread) - similarly the max clock divider has to be <= max integer clock divider (not max fractional clock divider). |
|
So here a separate branch that avoids the clock issues: |
|
I got a patch that makes use of CLK_IS_CRITICAL (which had to get cherry-picked as well): Now the issue of the freeze as soon as the clock is released has gone in my testing. If you want to avoid the pllc then - as mentioned above - you can add a "ignore" to the "pllc_per" string. |
@msperl I need to think very carefully about how I put this, as tact isn't one of my qualities..... ;) There are/were several PITA issues with vc4, not least that it doesn't play nice, having enabled it, with a headless boot and not having HDMI connected...... Which makes enabling it a non-starter on the multiple units in my test "farm". Of course, they are all headless. I'll take a look and rebuild kernels with CRITICAL patch when I finish typing this. |
|
Well - see the critical patch - that works without KMS - at least for me. I have to admit: I have - for now - got a hdmi display connected, but that only came of the requirement to test KMS more than anything else (and it sometimes gives me serial headaches...) |
@msperl I've left the PLL's "in play", haven't added a patch to ignore them. I haven't done much testing yet, but have upgraded 10x units to this new build with CRITICAL clock patches and left them looping through a sample rate test playlist. One is connected to amp at moment, (a Pi3B with IQAudIO Zero DAC attached, so slave and using Pi clock, selection via clock manager). Gordon will be pleased to hear that all sample rates working (32k,44k1,48k,88k2,96k,176k4, & 192k) and playing back at correct speed! ;) |
|
So I have uploaded an almost complete changeset for the clock migration now. The only thing not really tested is:
Note that I had to backport(cherry-pick) the upstream aux-uart driver to make everything work fine. The only thing that remains is SMI, which does run its own clock manager, but that should be very easy to change - I can propose a change, but I can not easily test it... |
|
These latest patches.... Nothing to do with I2S, just migrating other subsystems to use clock framework, right? |
|
That is right, but that is what is needed to make everything work with the clockmanager. |
Just figuring out whether I needed to rebase straight away or not. (I'm halfway through testing every sample rate, at every possible bit depth/bclk_ratio possible with their downstream drivers, with 6 different I2S "slave" DAC's.) |
@msperl It was FYI, was not expecting you to use it as a blueprint, just showing you what I have been doing, prior to the upstream backport.
Or simply, don't even try to argue this with upstream and have a downstream patch. ;)
From a user point of view, (from the feedback I had), when choosing which noise shaping to apply, there is no need to differentiate on anything but parent clock. Doesn't matter which numbers (sample freq and frame size) we multiplied to get to a final frequency..... If PLLD is the parent, use MASH3. I hold my hands up and admit that in a blind test I couldn't tell any audible difference between MASH1/2/3. (Earlier patches of mine allowed choosing specific clock parent/div/mash for each target_frequency, but in the end, the consensus was that if using MASH2 for any clock freq with PLLD, you would want to prefer it for any freq generated with PLLD as the clock parent. So I don't really see a need for it to be very fine grained...... Avoid a config nightmare at the expense of flexibility.) |
|
@clivem : look at the measurements and the Fouriers i have calculated where I tested all variations coming to very inconsistent results - at least for the Hifiberry DAC - and there it showed that the settings needed to be different for each to get the best Signal/noise. |
|
@msperl Yes, I looked at your measurements. I try to have one foot in the objective and the other in the subjective. The thing is, that the die-hard audiphools interested in "playing with" MASH settings at the moment, aren't looking at FFT and scope traces. ;) |
|
@pelwell @popcornmix : I got a patch that currently disables pllc_per usage - the question is:
Should these also be disabled for vpu clocks during automatic parent-clock selection? Note that we have the following clocks that are (known) VPU clocks (and their default parent configured the firmware with the corresponding frequencies):
These clocks are not typically set from the kernel, so it may not be necessary to filter those. Note that for PER clocks we have eMMC clock also set to use pllc_per set by the firmware! |
|
It would be great if someone with proper test equipment (Audio Precision, Prism, ... gear) could do some SNR and THD performance tests comparing mash and oscillator settings. @iqaudio @bonezuk @hifiberry @clivem do you have access to such equipment? The tests @msperl and I performed are most certainly meaningless if not completely flawed. Clock/jitter issues should show up as spurs in the FFT and typically a 5-15dB performance drop in SNR (eg 90dB SNR instead of 100dB), but neither my M-Audio card nor the Red Pitaya with it's 14bit ADC have the specs to reveal such things. The spurs I saw in my FFTs quite certainly came from the oscillator of my M-Audio card and not the DUT... |
|
Status update: there are still discussions arround the clock interface. So there are still discussions going back and forth arround this. |
|
On a different front I tried to upstream the HifiBerry DAC driver and qot asked why (besides one glitch) we would not be using snd-soc-simple-card instead and configure that in the device-tree alone. There it became apparent that the setting of BLCK is not implemented in that driver - the one just multiplying by 2. So I am thinking now of patching that driver to get this fixed and at the same time we can also try something that would allow us to generically set the blck size and others in a generic way, so that bclk sizes of 40, 50, 80 or 100 can easily get selected - maybe also parent clocks or other parameters (but that would require some more changes to the clock core framework... Here one idea that I have: Other actions could also be possible that might set some specific extra Might something like this work for your cards or help in any way? This would be based on simple-sound-card though... |
|
This was the "kludge" I was using last week...... although setting custom bclk_ratio from bcm2835-i2s rather than simple sound card. (I have major misgivings about using simple-card for anything but simple. ie. slave, with simple codec. It's meant to be simple right? Which is fine for berry_dac/pcm5102a combo, but not really workable where you actually do need a bunch of "glue" in the card driver, IMHO.) https://gist.github.com/clivem/0670a2f43e36e9c54c22ca2b90de6dd7 |
@msperl Where can I find this conversation? ALSA mailing list archive? |
|
@clivem rpi list plus alsa |
|
@msperl Martin, what you were talking about, adding set_bclk func from simple-card.... Is this something you have already started doing? would something like this be a possibility: sound { |
|
Something along these lines: yes! I guess this could even be a generic method that could get used by other drivers as well... There must be other things to do for other drivers as well - enabling a certain clock or others... |
|
@msperl I thought I was going to have a few hours this afternoon to write some code, but I'm not. I do agree with trying to do as much as this type of thing from dt as possible, and having generic "card" drivers, whether that be simple-card from upstream or some other driver in downstream sound/soc/bcm ONLY, but I guess it depends what you can get Broonie to go along with. ;) I'm still playing with simple-card and that "kludge" to bcm2835-i2s for integer bclk divider preference.. If you do implement the set_blck_ratio from simple-card dt before I even get a chance to think about it some more, I'd like to know and whether it is accepted upstream or not, I'd test it. It could be very helpful with the stuff I'm still thinking through, to get my head around trying to rationalise the downstream driver situation, for what is effectively the same hardware...... But after a discussion with a new manufacturer, that I suspect is going to have a very popular DAC HAT board within a short space of time, this is really starting to feel like herding cats..... They have zero interest in upstreaming code, (whether that be via RPi github or mainline), they just want to do "deals" in private to make sure their board is supported with a driver in 2 of the more popular Pi audio distributions, which is where they see the market for their boards, rather than the mainstream, and wanting support in Raspbian, Arch, etc. etc.. This sort of crap thinking needs to stop. It's just making things harder! Of course, the distribution sees a benefit to their popularity by "exclusively" supporting a card. I suspect users are going to be peed off, at the point an upstream/downstream ASoC or i2s change breaks audio playback on that card, the driver not having been upstreamed..... |
|
@clivem: do you have a basic config for simple card that I could use for my HifiBerry DAC? I have thougth about this a bit more and have come up with a slight variation that may allow more "flexibility": Where "snd_hw_params_action_set_fixed_bclk_ratio" would be a method that would get used. The idea is to use dynamic linking for this, so that the method argument would be the method called Unfortunately we are unable to do static argument checking in those cases, but people may think of something... I still have some doubts about how to handle arguments in a flexible manner - parsing the DT every time we run the actions is probably not acceptable... Would u32 be sufficient or would we need to support something else as well? What are other candidate things we may need? Anyway: this should go to a separate thread... |
|
@clivem : not upstreaming and requirements could get easily get controlled by the maintainer of the relevant kernel - if they do not accept such patches, then those vendors would need to do the extra work, as otherwise they would need to maintain their own kernel... |
It is in the gist above, the simple-pcm5102a-dac config, assuming you are actually talking about the HB DAC (PCM5102) and not the HB DAC+ (PCM5122). The HB DAC+ needs to keep it's own card driver, because of the clock master option and the way they have implemented that. (That's what I'm using right now with HB first gen PCM5102 DAC on a PiB and Pimoroni PHAT DAC on Zero.) If you want the 384k support, you'll also need to grab 2 of the 384k patches..... Actually, another conversation later, and again I'm not sure that trying to "shoehorn" everything we need into simple-card is the way to go..... |
LOL. Some of the people (that we are actually talking about here) make me look like a professional! They barely know how to apply a patch, let alone maintain a driver and how the ASoC components "glue" together..... |
|
as for the patches: why not enable the 384 modes by default and configure them instead in the DT? If so then a different compatiblity string is the "better" approach... |
The problem with enabling 384k for default with pcm5102a codec is that HB is using it for both PCM5102 (original PCM5102 DAC) and DAC+ Light with (ES9023). There is a lot more going on here, than appears from the surface. Gordon (IQAudIO) on our phone call yesterday, has backed me and my judgement, even if that means he gives up an IQ specific card driver for his stuff and using a generic. I hope to speak to Daniel (on phone) this coming week. The splitting out of pcm5102 and es9023, (which has always been a piggyback on pcm5102 and not just by HB, there are 3 other board ES9023 board manu's also doing this approach), is the first stumbling block. Anyway, can we just leave that alone for the moment and concentrate on how we get the custom bclk ratio into the bcm2835-i2s driver. The ES9023 really does benefit from that. It's not important for the TI chips anyway. Once we have a way to set it, and I don't care about the how..... via a dedicated card driver, bcm2835-i2s dt prop, or simple-card dt.... Don't care whether it is upstream or downstream. I am concerned that implementing anything when it involves upstream takes too long. Your back and forth with Eric about CRITICAL/HAND_OFF for clocks, (and it still isn't concluded??????), kind of proves the point. We could have had a working backport dma/i2s impl getting tested already in a downstream next branch. It now seems that moving forward on that, is waiting on upstream "co-operation" and decision making process. And this whole thing makes me laugh. There are no card drivers upstream. No-one is using upstream for Pi and I2S DAC. Get stuff working downstream, refine and hone it before submitting it upstream. If you want to get any testing done, that's what you need to do. Only a couple of devs and their dog are running upstream kernel builds on the Pi. ;) Waiting for upstream approval, and getting things in there, before using them downstream, is just going to result in more and more people trying to do things outside of the process...... Like these new manufacturers being more bothered about distributions providing drivers, rather than first priority being to at least get them into Pi git and the distribs getting them from there, as part of a "standard" Pi kernel. Anyway, not a criticism. I appreciate your time and effort. Just being as blunt as I usually am and stating an opinion, even if it is controversial. |
|
note: current status is that:
For some reason the clock patches have not made it (yet) into the kernel On another note: it seems now to be identified that SD-Ram is using the PLLD_CORE0 clock, so For that exists a new SDRam driver that primarily owns and enables those clocks and does nothing more. But to make it complete (and avoid other pitfalls where a different driver mai claim and enable a clock and then disable it again) this will require also that we start both the clock and sdram driver early during the boot-process - using core_initcall or maybe even early_initcall... Incidentally this also does away with all those warnings about not found clocks during boot as well... |
|
@msperl Martin, I'm a little confused having looked at the most recent pull from Anholt into rpi-4.4.y.... |
|
@anholt applied some patches that went in upstream - but these do not solve all the issues with regards to plld-clocks - especially CPU uses pllc while plld-per is the one that the sdram really uses. There are instead some patch sets out that:
|
Can we not just ask Phil to pull your "upstream not accepted" branch into the "next" tree he setup? It works! We know it works! At the point your few "not accepted patches" can be replaced with mechanisms upstream has accepted, they can be reverted downstream in favour of the new mechanism. It feels as if we have hit a roadblock with clkmgr. We need to move forward. IMHO, (having put terrabyte upon terrabyte of data through it), the dma code is GOOD to go! The I2S code is GOOD to go! This clkmgr stuff, (and waiting for upstream to decide a solution is acceptable, when fully working proposals have already been thrown into the bit bucket), has already delayed moving this forward to a wider audience for several weeks. |
|
There's been a lot of back-and-forth, so I'm just waiting for a consensus and the nod before updating |
|
well - as mentioned: most of my "crashes" are related to plld_core0 (and possibly the sdram clock SDRAM), which is used by the SDRAM controller. The patchset by Eric (#1472) does not address this - hence the sdram driver that has not seen any acceptance or similar - this should solve most of the "issues" with regards to crashes - every thing else (or mostly) is patches that are related to re configuring the device-tree to use the new clock system. |
|
I guess I will review the changes in recent kernels and look into how I may rebase the patchset with everything that went in already... |
|
Note that there are still some clocks in the device tree that are not yet configured properly and use the hard-coded clocks: