ADAU1761 default register value problem

ADAU1761 default register value problem

[Ricard Wanderlof]

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
-- 
Ricard Wolf Wanderlöf                           ricardw(at)axis.com
Axis Communications AB, Lund, Sweden            www.axis.com
Phone +46 46 272 2016                           Fax +46 46 13 61 30

Re: ADAU1761 default register value problem

[Lars-Peter Clausen]

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

Re: ADAU1761 default register value problem

[Ricard Wanderlof]

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
-- 
Ricard Wolf Wanderlöf                           ricardw(at)axis.com
Axis Communications AB, Lund, Sweden            www.axis.com
Phone +46 46 272 2016                           Fax +46 46 13 61 30

Re: ADAU1761 default register value problem

[Ricard Wanderlof]

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
-- 
Ricard Wolf Wanderlöf                           ricardw(at)axis.com
Axis Communications AB, Lund, Sweden            www.axis.com
Phone +46 46 272 2016                           Fax +46 46 13 61 30

Re: ADAU1761 default register value problem

[Lars-Peter Clausen]

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.

Re: ADAU1761 default register value problem

[Ricard Wanderlof]

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
-- 
Ricard Wolf Wanderlöf                           ricardw(at)axis.com
Axis Communications AB, Lund, Sweden            www.axis.com
Phone +46 46 272 2016                           Fax +46 46 13 61 30

Re: ADAU1761 default register value problem

[Lars-Peter Clausen]

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.

ADAU1761 default register value problem

[Ricard Wanderlof]

I have a problem with the ADAU1761 (although the problem I would think 
would manifest itself with a veriety of codecs).

The register values are cached to minimize the number of writes needed, 
and there is a set of default values specified. However, this assumes that 
when the software boots the chip is always in the default state. This is 
not necessarily the case, because the chip has no external reset line and 
resets its internal state using an internal power-on-reset circuit, so if 
the system is rebooted without a power cycle, the regcache will contain 
the default values of the registers, whereas the actual registers in the 
chip will retain their previous values. Hence, if attempting to set a 
value in a register which is in fact identical to the default value, it 
will not be written, and the value that is currently in the register will 
remain. This can lead to all sorts of misconfigurations, depending on the 
previous state of the chip.

One way to fix this is simply to avoid setting up a set of default 
registers (looking at regcache.c, one way seems to be to omit setting the 
reg_defaults and num_reg_defaults members of the struct regmap_config 
during initialization, and instead setting num_reg_defaults_raw to the 
number of registers the chip has, but not setting reg_defaults_raw; I 
don't know if this is how it is intended to be used however). The regcache 
framework will then read all the register values from the chip and use 
them as the new defaults.

While this would seem to be technically correct, the question is if this 
is acceptable? A drawback is that there will be a lot of I2C or SPI 
accesses just to get the default values. Or should the caching behavior at 
startup be configured in some way, for instance via devicetree?

/Ricard
-- 
Ricard Wolf Wanderlöf                           ricardw(at)axis.com
Axis Communications AB, Lund, Sweden            www.axis.com
Phone +46 46 272 2016                           Fax +46 46 13 61 30