[alsa-devel] Need help fixing pop/click artifacts in an ASOC driver
Hi,
A (commercial) DAC board, I use with a Raspberry Pi is plagued by clicks and pops and I'm trying to fix it at the driver level. I have mostly fixed it, but I could use some guidance on the problem described below. I apologize in advance for the long read, but the situation is a bit convoluted (or my understanding of it is).
The board (a HifiBerry DAC+ Pro) uses a PCM5122 chip which is driven by the pcm512x codec driver, but the Pro version also includes a set of external clocks on the board (one each for 48Khz and 44.1Khz sample rates and multiples thereof).
There are two kinds of clicks and pops: on the one hand the board clicks when the device is opened/closed, mostly because the pcm512x codec driver doesn't support the digital_mute operation. These are relatively mild clicks, which are an annoyance at most and I have a separate patch for the pcm512x, that addresses them, which I'm currently testing and will (hopefully) submit soon.
A separate case is a pop that's generated when the clock source is changed. This pop is independent of the digital volume setting and very loud. As far as I could determine, when the hw_params callback specified in the snd_soc_dai_link struct is called, it figures out what clock is needed for the giver sample rate and powers the appropriate clock through a GPIO on the PCM5122. This essentially means that the external clock source fed into the 5122 is stopped and restarted at a different rate, potentially while the DAC chip is operating. Later the hw_params callback defined in the pcm512x driver is called and it reconfigure the clock dividers for the new rate. It is at this point that the pop seems to be generated, provided that the card is not powered down or muted. If I comment out either the clock power-up/down in the dai_link hw_params, or the clock divider reconfiguration in the codec's hw_params, no pop is generated.
Now, the digital_mute patch described before, also happens to fix this loud pop. I'm not sure whether that's a happy coincidence that might cease to be the case in the future though and was considering fixing the pop problem separately, at its source: the clock change. One way to fix the problem is to switch the 5122 into standby before changing the clock source in the dai_link hw_params callback and switch it back into normal operation afterwards. This seems to work regardless of whether digital_mute is implemented or not, but has the following problem: The register that's used to put the chip into/out of standby, is also used by the pcm512x driver, in the set_bias_level operation. Since the hw_params operation is, as far as I can tell, not atomic, I'm concerned that race conditions may arise, causing the card to be left powered up, when it should be off and vice-versa.
Is there some way to synchronize the use of common chip registers in the DAI and codec drivers? Is the digital_mute somehow guaranteed to fix the pop originating at the DAI/machine driver level (whatever the hw_params operation in the dai_link struct is supposed to be)? Am I perhaps missing something entirely?
Any help or guidance is welcome.
Thanks, Dimitris
On 11/6/18 10:37 AM, Dimitris Papavasiliou wrote:
Hi,
A (commercial) DAC board, I use with a Raspberry Pi is plagued by clicks and pops and I'm trying to fix it at the driver level. I have mostly fixed it, but I could use some guidance on the problem described below. I apologize in advance for the long read, but the situation is a bit convoluted (or my understanding of it is).
The board (a HifiBerry DAC+ Pro) uses a PCM5122 chip which is driven by the pcm512x codec driver, but the Pro version also includes a set of external clocks on the board (one each for 48Khz and 44.1Khz sample rates and multiples thereof).
There are two kinds of clicks and pops: on the one hand the board clicks when the device is opened/closed, mostly because the pcm512x codec driver doesn't support the digital_mute operation. These are relatively mild clicks, which are an annoyance at most and I have a separate patch for the pcm512x, that addresses them, which I'm currently testing and will (hopefully) submit soon.
A separate case is a pop that's generated when the clock source is changed. This pop is independent of the digital volume setting and very loud. As far as I could determine, when the hw_params callback specified in the snd_soc_dai_link struct is called, it figures out what clock is needed for the giver sample rate and powers the appropriate clock through a GPIO on the PCM5122. This essentially means that the external clock source fed into the 5122 is stopped and restarted at a different rate, potentially while the DAC chip is operating. Later the hw_params callback defined in the pcm512x driver is called and it reconfigure the clock dividers for the new rate. It is at this point that the pop seems to be generated, provided that the card is not powered down or muted. If I comment out either the clock power-up/down in the dai_link hw_params, or the clock divider reconfiguration in the codec's hw_params, no pop is generated.
Now, the digital_mute patch described before, also happens to fix this loud pop. I'm not sure whether that's a happy coincidence that might cease to be the case in the future though and was considering fixing the pop problem separately, at its source: the clock change. One way to fix the problem is to switch the 5122 into standby before changing the clock source in the dai_link hw_params callback and switch it back into normal operation afterwards. This seems to work regardless of whether digital_mute is implemented or not, but has the following problem: The register that's used to put the chip into/out of standby, is also used by the pcm512x driver, in the set_bias_level operation. Since the hw_params operation is, as far as I can tell, not atomic, I'm concerned that race conditions may arise, causing the card to be left powered up, when it should be off and vice-versa.
Is there some way to synchronize the use of common chip registers in the DAI and codec drivers? Is the digital_mute somehow guaranteed to fix the pop originating at the DAI/machine driver level (whatever the hw_params operation in the dai_link struct is supposed to be)? Am I perhaps missing something entirely?
Any help or guidance is welcome.
Thanks for starting this thread. You are not the only one fighting to make those Hifiberry DAC+ PRO boards work without convoluted hacks. Our team uses them for validation of the Sound Open Firmware on the UP^2 boards and we are not quite happy with the clock definitions and don't really have a line of sight to upstream our changes.
The current implementation we have is ripped from the Raspberry kernel:
The PCM512x codec uses the clock framework to request an "sclk", and if it's not available will fall back to slave mode (SoC master). The codec driver uses this clock handle to request the current rates with clk_get_rate() and program the various dividers.
In the Raspberry kernel there is a pretend clock which does nothing but store a value on clk_set_rate() and provides the stored value with clock_get_rate().
The machine driver hw_params calls clk_set_rate(), but the silly part is that it then directly plays with the codec GPIO registers to change the clock sources without any sort of stabilization time/mute.
My view is that it's a broken model, and your point about the clock transitions not taking into account the codec state reinforces my diagnosis. Ideally we'd need a clk API implementation which is closely tied with codec driver implementation for the cases where the codec GPIOs are used to select clock sources.
For now our code uses a hack which bypasses the clock API completely. I shared it on this list [1] but was told the clock API was the way to go. I don't have a clear vision though on how to deal with a case where the codec driver is both client and provider of a clock.
I am told that Suse supports the Raspberry Pi now, maybe Takashi will chime in :-)?
-Pierre
On Thu, 08 Nov 2018 02:42:52 +0100, Pierre-Louis Bossart wrote:
I am told that Suse supports the Raspberry Pi now, maybe Takashi will chime in :-)?
Unfortunately, I already left my Pi box, and we don't work on the extension board in general...
Takashi
On 11/08/2018 03:42 AM, Pierre-Louis Bossart wrote:
Thanks for starting this thread. You are not the only one fighting to make those Hifiberry DAC+ PRO boards work without convoluted hacks. Our team uses them for validation of the Sound Open Firmware on the UP^2 boards and we are not quite happy with the clock definitions and don't really have a line of sight to upstream our changes.
Thanks for responding; I was starting to worry this thread would go by unnoticed. I've noticed the less than elegant clock setup as well, and considered patching it, so that the clocks are actually selected (i.e. powered up/down inside the clock driver). I'm not sure whether it would have any practical significance though, as after probing the card, the CODEC driver only ever stops or starts the clocks inside PM-related callbacks, which I don't think the Pi supports.
Are you also getting loud pops when switching between clocks? I use the following Python script to reproduce the problem (and also disable auto-mute via the mixer, as it can mask it):
------------------------------
import alsaaudio, time, random
d = alsaaudio.PCM() p = True
while True: time.sleep(1)
f = p and 48000 or 44100 n = p and 2 or 1 m = p and alsaaudio.PCM_FORMAT_S16_LE or alsaaudio.PCM_FORMAT_S24_LE
d.setrate(f) d.setchannels(n) d.setformat(m)
if random.random() > 0*0.25: print "pop!" p = not p
------------------------------
I've done some more testing regarding the pop, including:
* Trying to switch the DAC clock reference away from SCK (via PCM512x_DAC_REF), prior to switching the clocks (to the BCK, or to a reserved value that's supposed to cause a mute according to the datasheet).
* Halting the clocks, prior to switching the clock source (via PCM512x_SYNCHRONIZE) and letting the CODEC driver resynchronize them in its hw_params callback, or resynchronizing after the switch.
* Enabling clock error detection, which the CODEC driver mostly disables, in the hope that it will force a mute.
None of this fixes the pop, although the first two can lower its volume somewhat. All in all, the only changes that silence the pop are:
* Putting the chip into standby before switching clocks in the machine driver and resuming afterwards.
* Muting the output before switching clocks in the machine driver and unmuting afterwards.
With any of the above, I get no pop after hundreds of clock switches (using the above script), which otherwise pop every time.
Muting the output, or suspending the chip at the beginning of the CODEC driver (pcm512x) and unmuting, or resuming at the end doesn't help. This suggests to me that a spike is generated at the DAC input sometime during/after the clock switch and that suspending the chip during the switch avoids it altogether, while muting the output suppresses it.
Implementing digital_mute in the CODEC driver probably helps because the output is muted during the machine driver's hw_params callback, but it would make sense to me to fix the machine driver, so that the spike is never generated in the first place. My problem is that the PCM512x_POWER register is manipulated at the CODEC level, inside DAPM-related callbacks and, as far as I can see, hw_params, a PCM-related callback, and DAPM callbacks aren't synchronized through some mutex. Race conditions should be possible.
The best solution I've found, is to force the chip into standby via something like the following:
struct snd_soc_dapm_context *dapm = snd_soc_component_get_dapm(component);
snd_soc_dapm_mutex_lock(dapm); snd_soc_dapm_force_bias_level(dapm, SND_SOC_BIAS_OFF);
/* Switch clock source here. */
snd_soc_dapm_sync_unlocked(dapm); snd_soc_dapm_mutex_unlock(dapm);
This seems to work fine, but I'm not sure if it's the appropriate approach, or if it has unwanted side-effects. I've CCed the author of pcm512x, who's also a maintainer on ASOC and DAPM-related stuff. Perhaps he can advise on this.
-Dimitris
On Sat, Nov 10, 2018 at 05:46:33PM +0200, Dimitris Papavasiliou wrote:
Are you also getting loud pops when switching between clocks? I use the following Python script to reproduce the problem (and also disable auto-mute via the mixer, as it can mask it):
Switching between clocks while the device isn't completely idle is unlikely to work well with any device - I'd have questions about a use case that's doing that.
On 11/15/2018 12:06 AM, Mark Brown wrote:
Switching between clocks while the device isn't completely idle is unlikely to work well with any device - I'd have questions about a use case that's doing that.
The clocks are switched inside the hw_params callback of the machine driver, when the sampling rate changes from a multiple of 48kHz to a multiple of 44.1kHz and vice versa, usually after opening the device and before starting playback. That has to be so, as far as I can see.
If by idle you mean, not playing, then the device is idle during the switch. If by idle you mean biased off, then it depends on circumstance. For instance if playback at 44.1kHz is stopped and quickly restarted at 48kHz, as often happens, the device won't have had the chance to sit long enough to be biased off (and even if it has, I'm not entirely sure whether it won't be biased on before the hw_params callback). The pop may still be avoided if auto-mute is used, but that's not always the case. My proposed fix on the other hand consistently avoids generating the pop and does not rely on muting. I'm just not sure how to best implement it.
On Thu, Nov 15, 2018 at 12:50:30AM +0200, Dimitris Papavasiliou wrote:
The clocks are switched inside the hw_params callback of the machine driver, when the sampling rate changes from a multiple of 48kHz to a multiple of 44.1kHz and vice versa, usually after opening the device and before starting playback. That has to be so, as far as I can see.
That's fairly normal, yes. You can also do SRC in software and lock the rate at one.
If by idle you mean, not playing, then the device is idle during the switch. If by idle you mean biased off, then it depends on circumstance. For instance if playback at 44.1kHz is stopped and quickly restarted at 48kHz, as often happens, the device won't have had the chance to sit long enough to be biased off (and even if it has, I'm not entirely sure whether it won't be biased on before the hw_params callback). The pop may still be avoided if
Usually it's only the digital that will notice clocking changes, so long as none of the digital stuff is active you're normally fine. It does depend on the hardware though.
auto-mute is used, but that's not always the case. My proposed fix on the other hand consistently avoids generating the pop and does not rely on muting. I'm just not sure how to best implement it.
I've no idea what you're proposing, sorry.
Thank you for taking the time to help out.
On 11/15/2018 01:02 AM, Mark Brown wrote:
I've no idea what you're proposing, sorry.
I mentioned it in the message you replied to initially, so you can consult that for more details if you want, but I'll give a summary below:
As far as I could determine experimentally, switching clocks when the DAC is not suspended seems to result in a spike at the input of the DAC, the level of which doesn't depend on the digital volume setting (so that it's always very loud). I tried various potential fixes, based on guesses about probable causes of the spike (see first message where I CCed you for a brief list of attempted fixes) and it seems that a pop can be avoided either by suppressing it, by muting the DAC, or by avoiding generating it in the first place by switching the PCM5122 into standby (via bit PCM512x_RQST of the PCM512x_POWER register) before switching the external clock and switching it back out of standby afterwards.
Avoiding the spike instead of relying on muting seems preferable, but, since the power state of the PCM5122 is manipulated by the CODEC driver, in response to requests to set the bias level, I'm concerned about potential race conditions.
Right now I have the following implementation for switching the external clock:
/* Figure out if a clock switch is necessary based on currently used clock and requested sample rate . */
...
if (no_switch_needed) return;
snd_soc_dapm_mutex_lock(dapm); force = (snd_soc_dapm_get_bias_level(dapm) != SND_SOC_BIAS_OFF);
if (force) snd_soc_dapm_force_bias_level(dapm, SND_SOC_BIAS_OFF);
/* Switch the clock here. */
...
if (force) snd_soc_dapm_sync_unlocked(dapm);
snd_soc_dapm_mutex_unlock(dapm);
It seems to work fine, both in consistently avoiding the pop and in not producing any obvious side-effects. Does that look reasonable, or is there a better way to temporarily switch the PCM5122 into standby in a safe way?
On Thu, Nov 15, 2018 at 01:55:06AM +0200, Dimitris Papavasiliou wrote:
On 11/15/2018 01:02 AM, Mark Brown wrote:
I've no idea what you're proposing, sorry.
I mentioned it in the message you replied to initially, so you can consult that for more details if you want, but I'll give a summary below:
That was rather a long mail and I got a bit lost about what the proposal was.
As far as I could determine experimentally, switching clocks when the DAC is not suspended seems to result in a spike at the input of the DAC, the level of which doesn't depend on the digital volume setting (so that it's always very loud). I tried various
This is fairly normal; the DAC is partly digital.
Avoiding the spike instead of relying on muting seems preferable, but, since the power state of the PCM5122 is manipulated by the CODEC driver, in response to requests to set the bias level, I'm concerned about potential race conditions.
Given that there's no substantial delays in the power up/down paths of the driver you probably want to set idle_bias_off and possibly also configuring ignore_pmdown_time to force immediate poweroff. That should get the device powered down rapidly, though bouncing the power on and off all the time isn't great. If you do need ignore_pmdown_time then it's probably better to add a higher level interface that allows the machine driver to cancel all power down timers.
if (no_switch_needed) return;
snd_soc_dapm_mutex_lock(dapm); force = (snd_soc_dapm_get_bias_level(dapm) != SND_SOC_BIAS_OFF);
if (force) snd_soc_dapm_force_bias_level(dapm, SND_SOC_BIAS_OFF);
/* Switch the clock here. */
...
if (force) snd_soc_dapm_sync_unlocked(dapm);
snd_soc_dapm_mutex_unlock(dapm);
This is not a good idea, fiddling around with DAPM internals from drivers is going to be very fragile as things change in future.
On 11/15/18 2:33 AM, Mark Brown wrote:
On Thu, Nov 15, 2018 at 01:55:06AM +0200, Dimitris Papavasiliou wrote:
On 11/15/2018 01:02 AM, Mark Brown wrote:
I've no idea what you're proposing, sorry.
I mentioned it in the message you replied to initially, so you can consult that for more details if you want, but I'll give a summary below:
That was rather a long mail and I got a bit lost about what the proposal was.
I understand completely and I apologize for that. I try to keep the length down as much as possible, but the situation is a bit complicated (at least with my current familiarity of the ASOC layer). Please don't interpret what I said as a complaint; I merely wanted to point out that more details were available, should you require them.
Given that there's no substantial delays in the power up/down paths of the driver you probably want to set idle_bias_off and possibly also configuring ignore_pmdown_time to force immediate poweroff. That should get the device powered down rapidly, though bouncing the power on and off all the time isn't great. If you do need ignore_pmdown_time then it's probably better to add a higher level interface that allows the machine driver to cancel all power down timers.
Although I think I've mentioned it in previous messages, you may have missed it: What I'm trying to do, is fix the driver for a commercial DAC board (the HifiBerry DAC+ Pro) which was designed for the Raspberry Pi. It uses the pcm512x CODEC driver (by specifying .codec_dai_name = "pcm512x-hifi" in the dai_link struct) and simply supplies a separate machine driver and a rudimentary clock driver, to handle the necessary clock switching.
(If it would help, you can see the relevant code here:
https://github.com/raspberrypi/linux/blob/rpi-4.19.y/sound/soc/bcm/hifiberry...
Clock switching takes place in snd_rpi_hifiberry_dacplus_select_clk which is eventually called from snd_rpi_hifiberry_dacplus_hw_params.)
Unless I misunderstand, I can't change the CODEC driver's settings (such as ignore_pmdown_time) from the machine driver. Even if I could, I'm not entirely sure it would consistently fix the problem. For instance, if the auto-mute is set to 21ms, the DAC output is muted automatically by the chip after 21ms of no input and the pop still manages to get through before that on occasion.
It also seems like a roundabout solution, that depends on proper sequencing, i.e. it is assumed that the device is always biased off before the hw_params callback of the machine driver is called, something which might change in the future and cause a regression.
All that's really necessary, is calling
snd_soc_component_update_bits(component, PCM512x_POWER, PCM512x_RQSY, PCM512x_RQSY);
before the clock switch and
snd_soc_component_update_bits(component, PCM512x_POWER, PCM512x_RQSY, 0);
afterwards. Isn't there some way to lock the component/CODEC's regmap, so as to perform an operation involving more than one register change atomically? Something like snd_soc_component_lock/unlock_regmap sounds like it might be generally useful, to synchronize access to a chip's registers across CODE/machine/etc. drivers.
On Thu, Nov 15, 2018 at 01:25:28PM +0200, Dimitris Papavasiliou wrote:
Unless I misunderstand, I can't change the CODEC driver's settings (such as ignore_pmdown_time) from the machine driver. Even if I
You can get to the CODEC driver from the machine driver, and for idle_bias_off there's probably a good case for just setting it unconditionally given the lack of delays.
could, I'm not entirely sure it would consistently fix the problem. For instance, if the auto-mute is set to 21ms, the DAC output is muted automatically by the chip after 21ms of no input and the pop still manages to get through before that on occasion.
That's why I'm suggesting ignore_pmdown_time as a first thing to try - it should cause the powerdown to happen synchronously so there's no chance for anything else to go in and change the clocking to cause a pop.
It also seems like a roundabout solution, that depends on proper sequencing, i.e. it is assumed that the device is always biased off before the hw_params callback of the machine driver is called, something which might change in the future and cause a regression.
The whole goal with the combination of idle_bias_off and ignore_pmdown_time is to ensure that this happens in an orderly fashion in a way that the core knows about.
afterwards. Isn't there some way to lock the component/CODEC's regmap, so as to perform an operation involving more than one register change atomically? Something like snd_soc_component_lock/unlock_regmap sounds like it might be generally useful, to synchronize access to a chip's registers across CODE/machine/etc. drivers.
No, there's no facility for that and it's such a niche case that I'm not convinced it's a good idea to do it. Another thing you could do here though is to do the bounce as part of set_sysclk() in the CODEC driver.
On 11/15/2018 09:04 PM, Mark Brown wrote:
You can get to the CODEC driver from the machine driver, and for idle_bias_off there's probably a good case for just setting it unconditionally given the lack of delays.
...
That's why I'm suggesting ignore_pmdown_time as a first thing to try - it should cause the powerdown to happen synchronously so there's no chance for anything else to go in and change the clocking to cause a pop.
Thanks for clarifying about idle_bias_off/ignore_pmdown_time. As far as I can see, idle_bias_off seems to be set by default, as the pcm512x driver is now a component driver and idle_bias_on is not set to true. I've also tried to explicitly set it to false, as well as setting use_pmdown_time to false. As far as ignore_pmdown_time is concerned, it seems to be settable at the dai_link level now, which is convenient for my use case.
Unfortunately none of this helps. Although the chip is turned off without delay, it's turned off only while the device is closed. As soon as the device is opened, it is turned on and kept on during all subsequent hw_params calls, where clock switching takes place. The pops always get through.
No, there's no facility for that and it's such a niche case that I'm not convinced it's a good idea to do it. Another thing you could do here though is to do the bounce as part of set_sysclk() in the CODEC driver.
Regarding your suggestion to do the bounce as part of set_sysclk() in the CODEC driver, I'm unsure how to proceed. The set_sysclk callback is virtually undocumented, as far as I could see, and not supported by the pcm512x component driver. Even if I could support it, either my somehow filling in the component driver callbacks from the machine driver, or by patching the pcm512x component driver itself, if such a patch would be acceptable, I don't see how it could help with my problem. Since both set_sysclk callbacks seem to operate at the CODEC or CODEC DAI level, which knows nothing about how to switch clocks on this particular card, I don't see how I can do the switch there, where I can safely wrap it inside a bounce.
Maybe I'm just missing something, but as far as I can see, there's no way to get around the need to synchronize access to the POWER register, so as to be able to reliably power bounce the chip while the clock selection takes place outside the CODEC/component driver. If such synchronization is impossible, I'm forced to accept that the pop will be generated and rely on suppressing it via a mute. This can be done by implementing the .digital_mute callback in the pcm512x component driver, for which I have a patch, which seems to work reliably for suppressing both the loud pop, as well as various other lower volume clicks and pops generated by the chip. I still feel that not generating the pop in the first place would be preferable though.
I've submitted a patch for the pcm512x CODEC, that is relevant to this problem. The patch implements the digital_mute interface, suppressing various clicks and pops, including the loud pop under discussion here. I still feel that fixing the machine driver, so that it doesn't generate the pop at all would be desirable, as I'm not sure whether the digital_mute callback is guaranteed to have muted the device when the clock source takes place.
Does that sound reasonable, or would it be preferable to simply rely on the digital_mute mechanism (assuming the patch is acceptable)?
On Sat, Nov 24, 2018 at 10:17:49PM +0200, Dimitris Papavasiliou wrote:
discussion here. I still feel that fixing the machine driver, so that it doesn't generate the pop at all would be desirable, as I'm not sure whether the digital_mute callback is guaranteed to have muted the device when the clock source takes place.
Making the machine driver more robust is definitely better, it's more robust in case the mute is for example just a very low gain which might still let the pop through at a lower level.
On 12/13/18 7:42 PM, Mark Brown wrote:
On Sat, Nov 24, 2018 at 10:17:49PM +0200, Dimitris Papavasiliou wrote:
discussion here. I still feel that fixing the machine driver, so that it doesn't generate the pop at all would be desirable, as I'm not sure whether the digital_mute callback is guaranteed to have muted the device when the clock source takes place.
Making the machine driver more robust is definitely better, it's more robust in case the mute is for example just a very low gain which might still let the pop through at a lower level.
Even worse, it would let the pop through at full volume, as it doesn't depend on the gain. Still, there doesn't seem to be a safe way to avoid the pop altogether, as far as I can see, since the only way I have found to avoid it, is to suspend the chip during the switch, and I can't synchronize access to the relevant register by the machine and CODEC drivers.
If you have any other ideas/pointers on approaches I could investigate, please let me know.
On Mon, Dec 17, 2018 at 02:17:45PM +0200, Dimitris Papavasiliou wrote:
Even worse, it would let the pop through at full volume, as it doesn't depend on the gain. Still, there doesn't seem to be a safe way to avoid the pop altogether, as far as I can see, since the only way I have found to avoid it, is to suspend the chip during the switch, and I can't synchronize access to the relevant register by the machine and CODEC drivers.
If you have any other ideas/pointers on approaches I could investigate, please let me know.
If you're doing this during DAPM there shouldn't be anything else running on the card at the time.
On 12/17/18 2:37 PM, Mark Brown wrote:
On Mon, Dec 17, 2018 at 02:17:45PM +0200, Dimitris Papavasiliou wrote:
Even worse, it would let the pop through at full volume, as it doesn't depend on the gain. Still, there doesn't seem to be a safe way to avoid the pop altogether, as far as I can see, since the only way I have found to avoid it, is to suspend the chip during the switch, and I can't synchronize access to the relevant register by the machine and CODEC drivers.
If you have any other ideas/pointers on approaches I could investigate, please let me know.
If you're doing this during DAPM there shouldn't be anything else running on the card at the time.
In general the clocks need to be switched during the hw_params callback (when the new callback specifies a new sample rate, which needs the other clock) and, as far as I can see, this can happen at more or less any time, i.e. in all power states and regardless of whether the device is opened or closed.
I can't therefore depend on DAPM functionality to suspend the chip when needed, as far as I can see at least and apart from asking the DAPM to force-suspend the chip, as I've already suggested.
On Mon, Dec 17, 2018 at 02:58:03PM +0200, Dimitris Papavasiliou wrote:
In general the clocks need to be switched during the hw_params callback (when the new callback specifies a new sample rate, which needs the other clock) and, as far as I can see, this can happen at more or less any time, i.e. in all power states and regardless of whether the device is opened or closed.
It shouldn't be called while the stream is running, if it is it'd be entirely fine to just return an error.
On 12/17/18 4:10 PM, Mark Brown wrote:
On Mon, Dec 17, 2018 at 02:58:03PM +0200, Dimitris Papavasiliou wrote:
In general the clocks need to be switched during the hw_params callback (when the new callback specifies a new sample rate, which needs the other clock) and, as far as I can see, this can happen at more or less any time, i.e. in all power states and regardless of whether the device is opened or closed.
It shouldn't be called while the stream is running, if it is it'd be entirely fine to just return an error.
It's not called while the stream is running, or at least I haven't seen that happen. It can be called though, while the device is opened and fully powered up, for instance when switching tracks during playback, where the next track has a different sample rate. This scenario is relatively frequent during normal use of a DAC and can't be avoided, as far as I can see.
On Mon, Dec 17, 2018 at 05:03:04PM +0200, Dimitris Papavasiliou wrote:
On 12/17/18 4:10 PM, Mark Brown wrote:
It shouldn't be called while the stream is running, if it is it'd be entirely fine to just return an error.
It's not called while the stream is running, or at least I haven't seen that happen. It can be called though, while the device is opened and fully powered up, for instance when switching tracks during playback, where the next track has a different sample rate. This scenario is relatively frequent during normal use of a DAC and can't be avoided, as far as I can see.
That's why I was suggesting that you should be configuring the power down time.
On 12/17/18 5:40 PM, Mark Brown wrote:
That's why I was suggesting that you should be configuring the power down time.
I've already tried your suggestion, but unfortunately it doesn't work in the general case, as it doesn't help when the hw_params callback is called while the device is opened (but not playing).
I'm quoting my initial reply below for your convenience; you can look up the whole message if needed. Apologies in advance, if you've replied to this and I've somehow missed it.
On 11/18/18 3:37 PM, Dimitris Papavasiliou wrote:
Thanks for clarifying about idle_bias_off/ignore_pmdown_time. As far as I can see, idle_bias_off seems to be set by default, as the pcm512x driver is now a component driver and idle_bias_on is not set to true. I've also tried to explicitly set it to false, as well as setting use_pmdown_time to false. As far as ignore_pmdown_time is concerned, it seems to be settable at the dai_link level now, which is convenient for my use case.
Unfortunately none of this helps. Although the chip is turned off without delay, it's turned off only while the device is closed. As soon as the device is opened, it is turned on and kept on during all subsequent hw_params calls, where clock switching takes place. The pops always get through.
On Mon, Dec 17, 2018 at 06:23:43PM +0200, Dimitris Papavasiliou wrote:
On 12/17/18 5:40 PM, Mark Brown wrote:
That's why I was suggesting that you should be configuring the power down time.
I've already tried your suggestion, but unfortunately it doesn't work in the general case, as it doesn't help when the hw_params callback is called while the device is opened (but not playing).
The device being open shouldn't have any impact on the power state?
Unfortunately none of this helps. Although the chip is turned off without delay, it's turned off only while the device is closed. As soon as the device is opened, it is turned on and kept on during all subsequent hw_params calls, where clock switching takes place. The pops always get through.
I would expect the stream to be closed and reopened by most applications in between reconfigurations like that, it certainly used to be the common pattern.
On 12/17/18 6:52 PM, Mark Brown wrote:
On Mon, Dec 17, 2018 at 06:23:43PM +0200, Dimitris Papavasiliou wrote:
I've already tried your suggestion, but unfortunately it doesn't work in the general case, as it doesn't help when the hw_params callback is called while the device is opened (but not playing).
The device being open shouldn't have any impact on the power state?
It's been a while since I tried it, but, as far as I recall, that was the observed behavior (determined via printk statements in the CODEC's set_bias_level callback). To test potential fixes, I have a script that opens the device and switches sampling rate between 44.1kHz and 48kHz every one second. It popped almost every time.
(I've also tried variations of the script where a sample is played every one second, again switching between the above sample rates and where the device is closed an reopened at every switch. The behavior is mostly the same.)
If you're positive that the expected behavior should be different, I could try it again, providing more details.
Unfortunately none of this helps. Although the chip is turned off without delay, it's turned off only while the device is closed. As soon as the device is opened, it is turned on and kept on during all subsequent hw_params calls, where clock switching takes place. The pops always get through.
I would expect the stream to be closed and reopened by most applications in between reconfigurations like that, it certainly used to be the common pattern.
If by stream you mean the device, then perhaps they do. As explained above, I test using a script, instead of using applications, such as a media player, as it's easier to reproduce the problem repeatedly at will and under controlled conditions. At any rate, a potential fix of the problem, shouldn't depend on application behavior.
On Tue, Dec 18, 2018 at 12:39:09PM +0200, Dimitris Papavasiliou wrote:
On 12/17/18 6:52 PM, Mark Brown wrote:
The device being open shouldn't have any impact on the power state?
It's been a while since I tried it, but, as far as I recall, that was the observed behavior (determined via printk statements in the CODEC's set_bias_level callback). To test potential fixes, I have a script that opens the device and switches sampling rate between 44.1kHz and 48kHz every one second. It popped almost every time.
If you're saying you're keeping the device open while you're doing this (rather than closing the stream and reconfiguring) then I'd expect you're going to find other hardware which has similar issues, reclocking the device isn't something I'd expect to be able to do glitch free.
I would expect the stream to be closed and reopened by most applications in between reconfigurations like that, it certainly used to be the common pattern.
If by stream you mean the device, then perhaps they do. As explained above, I test using a script, instead of using applications, such as a media player, as it's easier to reproduce the problem repeatedly at will and under controlled conditions. At any rate, a potential fix of the problem, shouldn't depend on application behavior.
The application does have some responsibility for ensuring that things work cleanly, there are limiations on what hardware can reasonably do and often trying to do things behind the hardware to avoid problems in unusual situations results in a lot of effort that's perhaps not worth it.
On 12/19/18 6:19 PM, Mark Brown wrote:
If you're saying you're keeping the device open while you're doing this (rather than closing the stream and reconfiguring) then I'd expect you're going to find other hardware which has similar issues, reclocking the device isn't something I'd expect to be able to do glitch free.
I got pops even if I opened the stream, called hw_params and closed it on each iteration, but, as it turns out, that was in part because I was being lazy, using a Python script to do the above, which sneaked in a hw_params call immediately after opening the stream, to "reset" it to default values.
So I've retried your suggested approach. It looks like setting .ignore_pmdown_time = 1 in the snd_soc_dai_link of the machine driver is enough to cause immediate power ups/downs, but I get the following behavior: The device gets powered up immediately after the first hw_params call (even if nothing gets played subsequently) and it gets powered down immediately after it's closed. This does avoid the pop when the device gets configured right after opening it, which, as you say, seems to be what usually happens (I checked with the SoX play utility and the MPD), but a pop is generated if there are multiple hw_params calls and the sample rate is changed in any but the first one (where the device is still powered down).
This can conceivably happen (for instance the Python library I was using, forces you to make multiple hw_params calls, in order to set more than one parameters). Is there some way to configure the DAPM to power up the card right before playback starts, and power it down right after it stops? This would prevent pops even in such pathological cases.
From what I could glean from the documentation on DAPM it seems like
that might be possible via stream domain widgets, but the CODEC driver already defines those and the device is still powered up before playback is requested.
On Wed, Dec 19, 2018 at 11:44:46PM +0200, Dimitris Papavasiliou wrote:
On 12/19/18 6:19 PM, Mark Brown wrote:
If you're saying you're keeping the device open while you're doing this (rather than closing the stream and reconfiguring) then I'd expect you're going to find other hardware which has similar issues, reclocking the device isn't something I'd expect to be able to do glitch free.
I got pops even if I opened the stream, called hw_params and closed it on each iteration, but, as it turns out, that was in part because I was being lazy, using a Python script to do the above, which sneaked in a hw_params call immediately after opening the stream, to "reset" it to default values.
Wow, that's... interesting.
So I've retried your suggested approach. It looks like setting .ignore_pmdown_time = 1 in the snd_soc_dai_link of the machine driver is enough to cause immediate power ups/downs, but I get the following behavior: The device gets powered up immediately after the first hw_params call (even if nothing gets played subsequently) and it gets
Are you sure it's the hw_params() call itself?
powered down immediately after it's closed. This does avoid the pop when the device gets configured right after opening it, which, as you say, seems to be what usually happens (I checked with the SoX play utility and the MPD), but a pop is generated if there are multiple hw_params calls and the sample rate is changed in any but the first one (where the device is still powered down).
This can conceivably happen (for instance the Python library I was using, forces you to make multiple hw_params calls, in order to set more than one parameters). Is there some way to configure the DAPM to power up the card right before playback starts, and power it down right after it stops? This would prevent pops even in such pathological cases.
This is what's supposed to happen. DAPM is triggered in the prepare() callback, you can call hw_params() as often as you like and it should have no interaction with DAPM at all.
On 12/20/18 5:36 PM, Mark Brown wrote:
On Wed, Dec 19, 2018 at 11:44:46PM +0200, Dimitris Papavasiliou wrote:
I got pops even if I opened the stream, called hw_params and closed it on each iteration, but, as it turns out, that was in part because I was being lazy, using a Python script to do the above, which sneaked in a hw_params call immediately after opening the stream, to "reset" it to default values.
Wow, that's... interesting.
I'm not sure if you're referring to the Python module's behavior, or to the fact that I was so careless and am now boring you with it, so I'm going to assume the former. :)
So I've retried your suggested approach. It looks like setting .ignore_pmdown_time = 1 in the snd_soc_dai_link of the machine driver is enough to cause immediate power ups/downs, but I get the following behavior: The device gets powered up immediately after the first hw_params call (even if nothing gets played subsequently) and it gets
Are you sure it's the hw_params() call itself?
[...]
This is what's supposed to happen. DAPM is triggered in the prepare() callback, you can call hw_params() as often as you like and it should have no interaction with DAPM at all.
Ah, well, the explanation then probably lies in the fact that snd_pcm_hw_params, according to the documentation also prepares the stream:
Install one PCM hardware configuration chosen from a configuration space and snd_pcm_prepare it.
As far as I can see there's no other way to call hw_params from userspace, except for snd_pcm_set_params, which is just a wrapper that calls snd_pcm_hw_params internally, so I suppose calling hw_params to configure the stream, must inevitably lead to the device being powered up.
Unfortunately the documentation ("Writing an ALSA Driver") on the prepare callback even warns explicitly, to:
Be careful that this callback will be called many times at each setup, too.
I'm not sure if this implies that configuring the card in multiple steps when setting up the stream is expected behavior, but it could be interpreted that way.
Should we then accept, that some pops will be generated and hope that they'll be suppressed by the digital_mute callback?
On Thu, Dec 20, 2018 at 10:41:40PM +0200, Dimitris Papavasiliou wrote:
On 12/20/18 5:36 PM, Mark Brown wrote:
Install one PCM hardware configuration chosen from a configuration space and snd_pcm_prepare it.
As far as I can see there's no other way to call hw_params from userspace, except for snd_pcm_set_params, which is just a wrapper that calls snd_pcm_hw_params internally, so I suppose calling hw_params to configure the stream, must inevitably lead to the device being powered up.
OK, that's fun...
Unfortunately the documentation ("Writing an ALSA Driver") on the prepare callback even warns explicitly, to:
Be careful that this callback will be called many times at each setup, too.
I'm not sure if this implies that configuring the card in multiple steps when setting up the stream is expected behavior, but it could be interpreted that way.
It's certainly possible if there needs to be negotation about the parameters (but then you wouldn't prepare as the stream as you'd fail to set the parameters) but it's not normal. The repeated configurations used to be much more common as OSS had separate calls to set each parameter rather than the atomic configuration that ALSA has so you got a hw_params() call for each OSS ioctl(), with native ALSA applications it is more usual to get it right first time.
Should we then accept, that some pops will be generated and hope that they'll be suppressed by the digital_mute callback?
It's probably easiest.
On 12/21/18 12:57 PM, Mark Brown wrote:
Should we then accept, that some pops will be generated and hope that they'll be suppressed by the digital_mute callback?
It's probably easiest.
All right then; I'll just add the one line, setting .ignore_pmdown_time = 1 to the machine driver and leave it at that. I've been using the card with just the digital_mute patch for several weeks and no pop got through, so it seems to be good enough, even with just that.
First, I'd like to thank you for your support both in troubleshooting this issue and in reviewing the patch I submitted. I fully appreciate I wasn't the easiest man to work with, being new to all of this and mostly clueless, so your help is very much appreciated.
Now, I apologize in advance, but I'll ask you if you can comment on the following final issue I have with this driver, which I was determined to ignore, but can't: I've already mentioned that I'm using a card made for the Raspberry Pi, and I can't really apply my patch there as-is, because they seem to have changed the pcm512x CODEC driver, without pushing the changes upstream. You can see the patch in question here:
https://github.com/raspberrypi/linux/commit/c53a137bd151130e29d7fc43947ac3e5...
(My patch, by the way, does run without the above change if you use the card without the external crystals, so I was able to test it in the form I submitted it in.)
I can apply the patch via merge, so bothering with this is not at all necessary, but if this change is useful, perhaps it's worth submitting another patch for it. I doubt this though, as it looks a bit hackish to me.
To provide you with some context: The card uses a 24.576MHz crystal for 48kHz-derived sampling rates, and it supports the S24_LE format, which leads to non-integer LRCK dividers, and somewhat faster-than-normal playback. A solution to this, would be to play 24bit samples as 32-bit with the MSB zeroed out, as the chip supports this. As far as I understand it, the patch implements this, by sending stereo streams as TDM data with two slots, one for each channel, wide enough for the physical size of the sample, which "happens" to be 32bits. On the machine driver size the following is done:
width = snd_pcm_format_physical_width(params_format(params)); ret = snd_soc_dai_set_tdm_slot(rtd->cpu_dai, 0x03, 0x03, channels, width); ret = snd_soc_dai_set_tdm_slot(rtd->codec_dai, 0x03, 0x03, channels, width);
This works, but I'm not sure TDM is meant for things like that. Anyway, if you think it's worth disucssing this, I can start a new thread if you prefer. If there's a straightforward way to fix this at the machine driver level, that'd be even better, as it would allow me to get rid of the divergence between the Pi and mainline kernel, without bothering the latter. If none of the above is the case, I can happily just merge the patch as-is.
On Fri, Dec 21, 2018 at 03:05:51PM +0200, Dimitris Papavasiliou wrote:
without pushing the changes upstream. You can see the patch in question here:
https://github.com/raspberrypi/linux/commit/c53a137bd151130e29d7fc43947ac3e5...
That's a change half implementing TDM modes with the goal of allowing a bodge to force 32 bit frame sizes where that's required to get clocks to divide out neatly.
I can apply the patch via merge, so bothering with this is not at all necessary, but if this change is useful, perhaps it's worth submitting another patch for it. I doubt this though, as it looks a bit hackish to me.
It's useful (that's a genuine issue) but it shouldn't be bodged by TDM mode but should instead just be done as standard without the machine driver needing to do TDM - check if the clock rate required is actually generated then fall back to a less exact frame if that helps. That way everyone gets the benefit without needing custom configuration or code. Basically it's doing the right thing in terms of configuring the hardware but should be triggered differently.
On Fri, Dec 21, 2018 at 7:33 PM Mark Brown broonie@kernel.org wrote:
I can apply the patch via merge, so bothering with this is not at all necessary, but if this change is useful, perhaps it's worth submitting another patch for it. I doubt this though, as it looks a bit hackish to me.
It's useful (that's a genuine issue) but it shouldn't be bodged by TDM mode but should instead just be done as standard without the machine driver needing to do TDM - check if the clock rate required is actually generated then fall back to a less exact frame if that helps. That way everyone gets the benefit without needing custom configuration or code. Basically it's doing the right thing in terms of configuring the hardware but should be triggered differently.
I suspected as much; I'll need to read up a bit on clocking though if I'm going to give this a shot. I'll be away from my computer during the holidays so, if time permits, I'll give it a try with the new year and open a new thread if I need feedback. Thanks again for all your help so far.
On Fri, Dec 21, 2018 at 03:05:51PM +0200, Dimitris Papavasiliou wrote:
Now, I apologize in advance, but I'll ask you if you can comment on the following final issue I have with this driver, which I was determined to ignore, but can't: I've already mentioned that I'm using a card made for the Raspberry Pi, and I can't really apply my patch there as-is, because they seem to have changed the pcm512x CODEC driver, without pushing the changes upstream. You can see the patch in question here:
https://github.com/raspberrypi/linux/commit/c53a137bd151130e29d7fc43947ac3e5...
I didn't upstream the patch as I wasn't really satisfied with it and also think it's quite hackish.
Back then when the Hifiberry folks submitted their machine driver to the RPi kernel they sneaked in a change to the pcm512x which forced S24 data to use 64fs clocking https://github.com/raspberrypi/linux/pull/1152
This change had the nasty side effect that using pcm512x eg in slave mode with the simple card driver broke as the setup on both sides no longer matched. bcm2835 by default uses 32/48/64fs for 16/24/32 bit data - like most drivers do (also upstream pcm512x driver). pcm512x with the change used 32/64/64fs - something that's not possible to express in simple-card's binding.
So that change had to go and after dropping it we realized some control over fs was needed if the machine driver wanted to support S24_LE (just dropping S24_LE support and relying on alsalib to do automatic 24->32bit conversion would have been another option).
Doing that via the set_bclk_ratio interface would probably have been the cleaner approach, OTOH set_tdm_slot had the nice feature that it already had simple card DT bindings - so I (ab)used that.
I don't have a Hifiberry DAC+ card (or other cards with on-board oscillators plus pcm512x in master mode) so I haven't looked into better solutions. Main focus was to have some workaround that didn't cause collateral damage but allowed the card to keep working.
I wouldn't mind at all if you do a proper implementation and submit it upstream so the downstream patch can be dropped.
so long,
Hias
(My patch, by the way, does run without the above change if you use the card without the external crystals, so I was able to test it in the form I submitted it in.)
I can apply the patch via merge, so bothering with this is not at all necessary, but if this change is useful, perhaps it's worth submitting another patch for it. I doubt this though, as it looks a bit hackish to me.
To provide you with some context: The card uses a 24.576MHz crystal for 48kHz-derived sampling rates, and it supports the S24_LE format, which leads to non-integer LRCK dividers, and somewhat faster-than-normal playback. A solution to this, would be to play 24bit samples as 32-bit with the MSB zeroed out, as the chip supports this. As far as I understand it, the patch implements this, by sending stereo streams as TDM data with two slots, one for each channel, wide enough for the physical size of the sample, which "happens" to be 32bits. On the machine driver size the following is done:
width = snd_pcm_format_physical_width(params_format(params)); ret = snd_soc_dai_set_tdm_slot(rtd->cpu_dai, 0x03, 0x03, channels, width); ret = snd_soc_dai_set_tdm_slot(rtd->codec_dai, 0x03, 0x03, channels, width);
This works, but I'm not sure TDM is meant for things like that. Anyway, if you think it's worth disucssing this, I can start a new thread if you prefer. If there's a straightforward way to fix this at the machine driver level, that'd be even better, as it would allow me to get rid of the divergence between the Pi and mainline kernel, without bothering the latter. If none of the above is the case, I can happily just merge the patch as-is. _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
On Sat, Dec 22, 2018 at 4:44 PM Matthias Reichl hias@horus.com wrote:
I didn't upstream the patch as I wasn't really satisfied with it and also think it's quite hackish.
[...]
I wouldn't mind at all if you do a proper implementation and submit it upstream so the downstream patch can be dropped.
Thanks for chiming in Matthias!
On 12/17/18 6:17 AM, Dimitris Papavasiliou wrote:
On 12/13/18 7:42 PM, Mark Brown wrote:
On Sat, Nov 24, 2018 at 10:17:49PM +0200, Dimitris Papavasiliou wrote:
discussion here. I still feel that fixing the machine driver, so that it doesn't generate the pop at all would be desirable, as I'm not sure whether the digital_mute callback is guaranteed to have muted the device when the clock source takes place.
Making the machine driver more robust is definitely better, it's more robust in case the mute is for example just a very low gain which might still let the pop through at a lower level.
Even worse, it would let the pop through at full volume, as it doesn't depend on the gain. Still, there doesn't seem to be a safe way to avoid the pop altogether, as far as I can see, since the only way I have found to avoid it, is to suspend the chip during the switch, and I can't synchronize access to the relevant register by the machine and CODEC drivers.
If you have any other ideas/pointers on approaches I could investigate, please let me know.
I started prototyping a different approach where the codec driver passes the regmap information to the clock driver. What's missing in the patchset is the addition of a clock control in the machine driver, and logic added so that rate change can only be done in a hw_params if there was a complete stop and reset on a DAPM_OFF event. compile-tested only for now.
On Mon, Dec 17, 2018 at 09:03:33AM -0600, Pierre-Louis Bossart wrote:
I started prototyping a different approach where the codec driver passes the regmap information to the clock driver. What's missing in the patchset is the addition of a clock control in the machine driver, and logic added so that rate change can only be done in a hw_params if there was a complete stop and reset on a DAPM_OFF event. compile-tested only for now.
That looks a lot like the CODEC should be exporting a GPIO driver so the machine driver doesn't actually need the regmap? The only register touched is _GPIO_CONTROL_1.
On 12/17/18 11:39 AM, Mark Brown wrote:
On Mon, Dec 17, 2018 at 09:03:33AM -0600, Pierre-Louis Bossart wrote:
I started prototyping a different approach where the codec driver passes the regmap information to the clock driver. What's missing in the patchset is the addition of a clock control in the machine driver, and logic added so that rate change can only be done in a hw_params if there was a complete stop and reset on a DAPM_OFF event. compile-tested only for now. https://github.com/plbossart/sound/commits/hifiberry/clks
That looks a lot like the CODEC should be exporting a GPIO driver so the machine driver doesn't actually need the regmap? The only register touched is _GPIO_CONTROL_1.
I am not sure what you meant by 'exporting a GPIO driver' (mostly because I am not familiar with any GPIO framework) but indeed the local oscillator choice is controlled by a single register accessible through regmap - and changes to that register should only happen when the device is a specific state to prevent click/pops.
The machine driver should use clk_set_rate() and not directly handle regmap or codec stuff. If it does, or if the clock framework isn't relevant here then we can simplify all this as suggested in https://patchwork.kernel.org/patch/10444387/. What I was trying to do with the github update is to keep the clock framework, tie it closer with the codec parts with a state variable that prevents wild changes without going back to a 'safe' idle state (similar idea as PulseAudio clock changes, which can only happen when the PCM is not opened and used).
On Mon, Dec 17, 2018 at 12:08:36PM -0600, Pierre-Louis Bossart wrote:
On 12/17/18 11:39 AM, Mark Brown wrote:
That looks a lot like the CODEC should be exporting a GPIO driver so the machine driver doesn't actually need the regmap? The only register touched is _GPIO_CONTROL_1.
I am not sure what you meant by 'exporting a GPIO driver' (mostly because I am not familiar with any GPIO framework) but indeed the local oscillator choice is controlled by a single register accessible through regmap - and changes to that register should only happen when the device is a specific state to prevent click/pops.
The GPIO framework provides a fairly simple view of GPIOs - from a user point of view it's just getting or setting the value of a line. It looks like the register you're controlling isn't actually controlling a chip feature directly but rather is setting a GPIO on the chip which controls an external clock generator. I could be misparsing things, though. I did glance at the pcm512x datasheet and didn't see pins or anything that looked like an oscillator but I could've missed something.
The machine driver should use clk_set_rate() and not directly handle regmap or codec stuff. If it does, or if the clock framework isn't relevant here then we can simplify all this as suggested in https://patchwork.kernel.org/patch/10444387/. What I was trying to do with the github update is to keep the clock framework, tie it closer with the codec parts with a state variable that prevents wild changes without going back to a 'safe' idle state (similar idea as PulseAudio clock changes, which can only happen when the PCM is not opened and used).
Right, I think bringing in the clock framework more is good - effectively all I'm suggesting is changing the control interface used to set the clock to add an indirection through gpiolib so you don't need to pass the raw register map around.
On 12/17/18 1:02 PM, Mark Brown wrote:
On Mon, Dec 17, 2018 at 12:08:36PM -0600, Pierre-Louis Bossart wrote:
On 12/17/18 11:39 AM, Mark Brown wrote:
That looks a lot like the CODEC should be exporting a GPIO driver so the machine driver doesn't actually need the regmap? The only register touched is _GPIO_CONTROL_1.
I am not sure what you meant by 'exporting a GPIO driver' (mostly because I am not familiar with any GPIO framework) but indeed the local oscillator choice is controlled by a single register accessible through regmap - and changes to that register should only happen when the device is a specific state to prevent click/pops.
The GPIO framework provides a fairly simple view of GPIOs - from a user point of view it's just getting or setting the value of a line. It looks like the register you're controlling isn't actually controlling a chip feature directly but rather is setting a GPIO on the chip which controls an external clock generator. I could be misparsing things, though. I did glance at the pcm512x datasheet and didn't see pins or anything that looked like an oscillator but I could've missed something.
You are correct, there are two variants of the Hifiberry DAC+, the 'PRO' version with two oscillators on board (SoC configured as bitclock and fsync slave) and the regular without (all clocks provided by the SoC in that case). It's indeed a GPIO control of an external component, not an on-chip capability.
The machine driver should use clk_set_rate() and not directly handle regmap or codec stuff. If it does, or if the clock framework isn't relevant here then we can simplify all this as suggested in https://patchwork.kernel.org/patch/10444387/. What I was trying to do with the github update is to keep the clock framework, tie it closer with the codec parts with a state variable that prevents wild changes without going back to a 'safe' idle state (similar idea as PulseAudio clock changes, which can only happen when the PCM is not opened and used).
Right, I think bringing in the clock framework more is good - effectively all I'm suggesting is changing the control interface used to set the clock to add an indirection through gpiolib so you don't need to pass the raw register map around.
this looks elegant indeed, will look into it. I'll have to learn more on gpios :-)
On 12/17/18 1:14 PM, Pierre-Louis Bossart wrote:
On 12/17/18 1:02 PM, Mark Brown wrote:
On Mon, Dec 17, 2018 at 12:08:36PM -0600, Pierre-Louis Bossart wrote:
On 12/17/18 11:39 AM, Mark Brown wrote:
That looks a lot like the CODEC should be exporting a GPIO driver so the machine driver doesn't actually need the regmap? The only register touched is _GPIO_CONTROL_1.
I am not sure what you meant by 'exporting a GPIO driver' (mostly because I am not familiar with any GPIO framework) but indeed the local oscillator choice is controlled by a single register accessible through regmap
- and
changes to that register should only happen when the device is a specific state to prevent click/pops.
The GPIO framework provides a fairly simple view of GPIOs - from a user point of view it's just getting or setting the value of a line. It looks like the register you're controlling isn't actually controlling a chip feature directly but rather is setting a GPIO on the chip which controls an external clock generator. I could be misparsing things, though. I did glance at the pcm512x datasheet and didn't see pins or anything that looked like an oscillator but I could've missed something.
You are correct, there are two variants of the Hifiberry DAC+, the 'PRO' version with two oscillators on board (SoC configured as bitclock and fsync slave) and the regular without (all clocks provided by the SoC in that case). It's indeed a GPIO control of an external component, not an on-chip capability.
The machine driver should use clk_set_rate() and not directly handle regmap or codec stuff. If it does, or if the clock framework isn't relevant here then we can simplify all this as suggested in https://patchwork.kernel.org/patch/10444387/. What I was trying to do with the github update is to keep the clock framework, tie it closer with the codec parts with a state variable that prevents wild changes without going back to a 'safe' idle state (similar idea as PulseAudio clock changes, which can only happen when the PCM is not opened and used).
Right, I think bringing in the clock framework more is good - effectively all I'm suggesting is changing the control interface used to set the clock to add an indirection through gpiolib so you don't need to pass the raw register map around.
this looks elegant indeed, will look into it. I'll have to learn more on gpios :-)
I was able to expose the 6 PCM512x GPIOs, toggle them from userspace as well as from the clock driver, it's indeed simple enough and could be useful for other usages, but there are still two issues with the concept:
1. After toggling a GPIO to enable the 44.1 or 48kHz oscillators, the clock driver cannot verify that the codec is locked, for this it'd need access to the regmap to check the PCM512x_RATE_DET_4 register. This is really needed to detect if the local oscillators are populated or not and hence detect the DAC+ PRO vs. the plain vanilla DAC+. An alternative would be to let the clock driver program GPIOs, and check in the codec driver is the clock is valid or not after the call to clk_prepare_enable() and fall back to slave mode in the latter case. I believe it'd be more elegant to do all these checks in the clock driver itself, and only register a clock if those local oscillators actually exist. I also can't figure out how to pass a regmap as platform data, the size argument can't be just a pointer and sizeof(*regmap) won't work either.
2. there is a mutual dependency between codec and clock driver. The codec exposes the gpios needed by the clock driver which in turn registers a clock needed by the codec driver. is it acceptable to create the clock platform device from the codec driver to remove any sort of race conditions or need for deferred probes?
Thanks
-Pierre
On 12/17/18 8:08 PM, Pierre-Louis Bossart wrote:
The machine driver should use clk_set_rate() and not directly handle regmap or codec stuff. If it does, or if the clock framework isn't relevant here then we can simplify all this as suggested in https://patchwork.kernel.org/patch/10444387/. What I was trying to do with the github update is to keep the clock framework, tie it closer with the codec parts with a state variable that prevents wild changes without going back to a 'safe' idle state (similar idea as PulseAudio clock changes, which can only happen when the PCM is not opened and used).
I have documented, in a previous message, various approaches I tried to prevent the pop, which were mostly based on the assumption that the clock source was changed too "abruptly", or wasn't allowed to settle. In the end, the only state, where the clocks can be switched without a pop seemed to be when the chip is suspended.
It would of course be great if this could be achieved in a natural manner, by waiting for the card to be suspended and only switching clocks in such a state, but I don't see how this can be achieved robustly. The power management behavior is outside of the control of the machine driver, so it can only refuse to switch sample rate when it's not suspended. Perhaps this will work in practice, but I fear it would break applications depending on less restrained control of the hardware.
On 12/18/18 5:32 AM, Dimitris Papavasiliou wrote:
On 12/17/18 8:08 PM, Pierre-Louis Bossart wrote:
The machine driver should use clk_set_rate() and not directly handle regmap or codec stuff. If it does, or if the clock framework isn't relevant here then we can simplify all this as suggested in https://patchwork.kernel.org/patch/10444387/. What I was trying to do with the github update is to keep the clock framework, tie it closer with the codec parts with a state variable that prevents wild changes without going back to a 'safe' idle state (similar idea as PulseAudio clock changes, which can only happen when the PCM is not opened and used).
I have documented, in a previous message, various approaches I tried to prevent the pop, which were mostly based on the assumption that the clock source was changed too "abruptly", or wasn't allowed to settle. In the end, the only state, where the clocks can be switched without a pop seemed to be when the chip is suspended.
It would of course be great if this could be achieved in a natural manner, by waiting for the card to be suspended and only switching clocks in such a state, but I don't see how this can be achieved robustly. The power management behavior is outside of the control of the machine driver, so it can only refuse to switch sample rate when it's not suspended. Perhaps this will work in practice, but I fear it would break applications depending on less restrained control of the hardware.
My point was that the machine driver can track DAPM events and put the device in a 'safe' state on SND_SOC_DAPM_EVENT_OFF so that the rate can be changed when playback restarts. I don't see what prevents us from using the same config as during suspend? It's pretty common to play with clocks with these events, and calling clk_disable_unprepare() could lead to whatever configuration sequence is needed for pop-free restart on startup.
On 12/18/18 4:12 PM, Pierre-Louis Bossart wrote:
My point was that the machine driver can track DAPM events and put the device in a 'safe' state on SND_SOC_DAPM_EVENT_OFF so that the rate can be changed when playback restarts. I don't see what prevents us from using the same config as during suspend? It's pretty common to play with clocks with these events, and calling clk_disable_unprepare() could lead to whatever configuration sequence is needed for pop-free restart on startup.
I only wanted to point out that, given the current DAPM configuration of the pcm512x CODEC driver (as determined from monitoring calls to the .set_bias_level callback, which seems to be the only place where the power state of the chip is actually manipulated), it seemed like it wasn't necessarily turned off between sample rate changes, and that I'm not sure there's a 'safe' state you can put it in, so that it won't pop on the next change to the clock source, if it's powered up at the time.
The DAPM subsystem is complex and documentation on it is scarce, but it seems the former can be fixed. At least it is implied that the DAC can be turned on/off at the stream domain level, which seems to be tied to playback starting/stopping. If the DAC can be suspended right after playback stops and powered back up right before playback starts again, then I don't think anything else needs to be done to avoid pops, as all clock switching will have been done with the device suspended.
If this is not possible, then I'm not sure if you can put the device in a 'safe' state, say with both clocks turned off, or the clock reference switched away from SCK, or something like that, so that it won't pop when the sample rate (and hence clock source) is later changed, while the DAC is powered up.
Anyway, calling my familiarity with the internals of ASOC drivers superficial, would probably be stretching it, so the above is (hopefully somewhat educated) guesswork at best. Ignore it if it doesn't make sense.
participants (5)
-
Dimitris Papavasiliou -
Mark Brown -
Matthias Reichl -
Pierre-Louis Bossart -
Takashi Iwai