[alsa-devel] ADAU1761 default register value problem
Hi Lars,
A while ago I posted a question to the list regarding the default values set up for the ADAU1761 (and similar CODECs), but I didn't get a response. I really need to try and figure out a way forward here though so I'll bring this up again.
The problem I have is that the driver assumes that the CODEC has been initialized with the default register settings when the system boots. However, if the system has not been power cycled but just rebooted, this is not necessarily the case, as the CODEC has no external reset pin but derives its reset signal internally from the power level.
Furthermore, if the system upon reboot tries to set a register value to one of the default settings (for example as the result of a hw_params() call), then the regcache mechanism will disregard the write as it assumes the default value is already set, which is not necessarily the case.
As I see it the solution to this problem is simply not to assume anything about the register settings, so that the first write to a given register always takes effect.
I can work on a patch to accomplish this, however, since so much work apparently has been laid down in getting the default values into the driver in the first place, I'm assuming there's something more to the story that I'm missing, so I'd like to get a better grip on this before I dive in.
/Ricard
On 04/21/2016 04:01 PM, Ricard Wanderlof wrote:
Hi Lars,
A while ago I posted a question to the list regarding the default values set up for the ADAU1761 (and similar CODECs), but I didn't get a response. I really need to try and figure out a way forward here though so I'll bring this up again.
The problem I have is that the driver assumes that the CODEC has been initialized with the default register settings when the system boots. However, if the system has not been power cycled but just rebooted, this is not necessarily the case, as the CODEC has no external reset pin but derives its reset signal internally from the power level.
Furthermore, if the system upon reboot tries to set a register value to one of the default settings (for example as the result of a hw_params() call), then the regcache mechanism will disregard the write as it assumes the default value is already set, which is not necessarily the case.
As I see it the solution to this problem is simply not to assume anything about the register settings, so that the first write to a given register always takes effect.
I can work on a patch to accomplish this, however, since so much work apparently has been laid down in getting the default values into the driver in the first place, I'm assuming there's something more to the story that I'm missing, so I'd like to get a better grip on this before I dive in.
Hi,
I've been thinking about this for the last few days trying to come up with a good solution, but it is really difficult.
The issue you are describing is simply an oversight from my side.
One solution would be do drop the register default values and let regmap fill the register cache on demand when a read is done the first time for a register. The problem with this approach is that we leave the device in an unknown state when we probe the driver, but we really want to have it in a known state of lowest power.
The alternative is to write out the register defaults when the device is probed, sort of a poor mans software reset, this brings us into a known state. This approach has the downside that there is the overhead of having to write all registers.
I don't like either, but given the limitations of the hardware we'll have to choose one and in that case I'd slightly lean towards the later solution has it gives us consistent and predictable behavior and the overhead of writing the registers should be manageable.
- Lars
On Tue, 26 Apr 2016, Lars-Peter Clausen wrote:
On 04/21/2016 04:01 PM, Ricard Wanderlof wrote:
... The problem I have is that the driver assumes that the CODEC has been initialized with the default register settings when the system boots. However, if the system has not been power cycled but just rebooted, this is not necessarily the case, as the CODEC has no external reset pin but derives its reset signal internally from the power level. ...
Sorry I haven't been able to respond earlier.
I've been thinking about this for the last few days trying to come up with a good solution, but it is really difficult.
The issue you are describing is simply an oversight from my side.
One solution would be do drop the register default values and let regmap fill the register cache on demand when a read is done the first time for a register. The problem with this approach is that we leave the device in an unknown state when we probe the driver, but we really want to have it in a known state of lowest power.
The alternative is to write out the register defaults when the device is probed, sort of a poor mans software reset, this brings us into a known state. This approach has the downside that there is the overhead of having to write all registers.
I don't like either, but given the limitations of the hardware we'll have to choose one and in that case I'd slightly lean towards the later solution has it gives us consistent and predictable behavior and the overhead of writing the registers should be manageable.
Either way, there will be a fair amount of communication with the codec, either as a result of setting the default values, or as a result of reading the existing ones in order to determine what would be necessary to write.
I would think that having the codec in a controlled known (low power) state would tip the balance in favor of the second solution.
One question is though should one write out _all_ registers, including those not actually used by the driver? I'm thinking of an upgrade/downgrade situation, where a previously running kernel version has set up a register which the currently running version doesn't need to touch.
/Ricard
Hi Lars,
On Tue, 26 Apr 2016, Lars-Peter Clausen wrote:
A while ago I posted a question to the list regarding the default values set up for the ADAU1761 (and similar CODECs), ...
Having researched this further, it appears that commit
27d6e7d1c96c9f51379e0feb972fec26029098bc
ASoC: adau17x1: Cache writes when core clock is disabled
actually fixes this problem, at least for the ADAU1361 and ADAU1761. I'm no expert at the regcache mechanism, but it looks as if when the regcache is synced when the bias level goes from OFF to STANDBY, it actually writes out all the values including the default ones (verified by adding trace printouts, and also by verifying that my original problem of the codec misbehaving after reconfiguration after reboot is no longer there).
Thus there is no apparent need to separately write out the default values, unless I have missed something (which is not unlikely...).
Sorry I missed this commit earlier, I was working with a kernel that was not the latest, and initially didn't think the commit would help, as I incorrectly assumed it wouldn't write out the default values as a result of a sync operation, just as it wouldn't write them out if the cache was enabled.
/Ricard
On 05/11/2016 05:16 PM, Ricard Wanderlof wrote:
Hi Lars,
On Tue, 26 Apr 2016, Lars-Peter Clausen wrote:
A while ago I posted a question to the list regarding the default values set up for the ADAU1761 (and similar CODECs), ...
Having researched this further, it appears that commit
27d6e7d1c96c9f51379e0feb972fec26029098bc
ASoC: adau17x1: Cache writes when core clock is disabled
I think it is also related to commit 1c79771a7270 ("regmap: Use regcache_mark_dirty() to indicate power loss or reset"). This changes the regmap code to sync all registers under certain conditions.
actually fixes this problem, at least for the ADAU1361 and ADAU1761. I'm no expert at the regcache mechanism, but it looks as if when the regcache is synced when the bias level goes from OFF to STANDBY, it actually writes out all the values including the default ones (verified by adding trace printouts, and also by verifying that my original problem of the codec misbehaving after reconfiguration after reboot is no longer there).
We still have the issue though that the CODEC is in an undefined state until the first OFF to STANDBY transitions happens. Usually this will happen early on when the CODEC is bound to the sound card, but if it is not bound immediately it might stay there for a bit longer. So depending on the setup this may or may not be a problem.
On Fri, 13 May 2016, Lars-Peter Clausen wrote:
Having researched this further, it appears that commit
27d6e7d1c96c9f51379e0feb972fec26029098bc
ASoC: adau17x1: Cache writes when core clock is disabled
I think it is also related to commit 1c79771a7270 ("regmap: Use regcache_mark_dirty() to indicate power loss or reset"). This changes the regmap code to sync all registers under certain conditions.
Yes, that commit seems to be the key, although I must admit that I can't get my head around the logic of that commit. The commit message says: "HW was not reset (maybe it was just clock-gated), so if we cached any writes, they should be sent to the hardware regardless of whether they match the HW default.". But if the device was indeed gated off, rather than reset, why should this make a difference when writing out the cache after gating the chip on again? If it were gated off and then on, doesn't this imply that the register settings won't change? Or is the idea that the chip might loose all its state while gated off and hence essentially needs to be reinitialized?
Regardless of the commit message, the code indeed does cause all the cached data to be written out when regcache_sync() is called in this case.
We still have the issue though that the CODEC is in an undefined state until the first OFF to STANDBY transitions happens. Usually this will happen early on when the CODEC is bound to the sound card, but if it is not bound immediately it might stay there for a bit longer. So depending on the setup this may or may not be a problem.
So that means that in the probe function we should really write out the default values, by enabling SYSCLK_EN in the ADAU clock control register, and then calling regcache_sync() before disabling SYSCLK_EN again, and going to regcache_cache_only(..,true) ?
/Ricard
On 05/16/2016 01:07 PM, Ricard Wanderlof wrote:
On Fri, 13 May 2016, Lars-Peter Clausen wrote:
Having researched this further, it appears that commit
27d6e7d1c96c9f51379e0feb972fec26029098bc
ASoC: adau17x1: Cache writes when core clock is disabled
I think it is also related to commit 1c79771a7270 ("regmap: Use regcache_mark_dirty() to indicate power loss or reset"). This changes the regmap code to sync all registers under certain conditions.
Yes, that commit seems to be the key, although I must admit that I can't get my head around the logic of that commit. The commit message says: "HW was not reset (maybe it was just clock-gated), so if we cached any writes, they should be sent to the hardware regardless of whether they match the HW default.". But if the device was indeed gated off, rather than reset, why should this make a difference when writing out the cache after gating the chip on again? If it were gated off and then on, doesn't this imply that the register settings won't change? Or is the idea that the chip might loose all its state while gated off and hence essentially needs to be reinitialized?
The register settings in the device will stay constant, but the cached register settings might have changed. E.g. application changes the volume of a control while the device is powered down. Since regmap does not (yet) track which registers have changed it has to write out all of them.
Regardless of the commit message, the code indeed does cause all the cached data to be written out when regcache_sync() is called in this case.
We still have the issue though that the CODEC is in an undefined state until the first OFF to STANDBY transitions happens. Usually this will happen early on when the CODEC is bound to the sound card, but if it is not bound immediately it might stay there for a bit longer. So depending on the setup this may or may not be a problem.
So that means that in the probe function we should really write out the default values, by enabling SYSCLK_EN in the ADAU clock control register, and then calling regcache_sync() before disabling SYSCLK_EN again, and going to regcache_cache_only(..,true) ?
Sounds reasonable.
participants (2)
-
Lars-Peter Clausen -
Ricard Wanderlof