* ADAU1761 default register value problem
@ 2016-04-21 14:01 Ricard Wanderlof
2016-04-26 8:21 ` Lars-Peter Clausen
0 siblings, 1 reply; 8+ messages in thread
From: Ricard Wanderlof @ 2016-04-21 14:01 UTC (permalink / raw)
To: Lars-Peter Clausen; +Cc: alsa-devel
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: ADAU1761 default register value problem
2016-04-21 14:01 ADAU1761 default register value problem Ricard Wanderlof
@ 2016-04-26 8:21 ` Lars-Peter Clausen
2016-05-03 8:16 ` Ricard Wanderlof
2016-05-11 15:16 ` Ricard Wanderlof
0 siblings, 2 replies; 8+ messages in thread
From: Lars-Peter Clausen @ 2016-04-26 8:21 UTC (permalink / raw)
To: Ricard Wanderlof; +Cc: alsa-devel
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: ADAU1761 default register value problem
2016-04-26 8:21 ` Lars-Peter Clausen
@ 2016-05-03 8:16 ` Ricard Wanderlof
2016-05-11 15:16 ` Ricard Wanderlof
1 sibling, 0 replies; 8+ messages in thread
From: Ricard Wanderlof @ 2016-05-03 8:16 UTC (permalink / raw)
To: Lars-Peter Clausen; +Cc: alsa-devel
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: ADAU1761 default register value problem
2016-04-26 8:21 ` Lars-Peter Clausen
2016-05-03 8:16 ` Ricard Wanderlof
@ 2016-05-11 15:16 ` Ricard Wanderlof
2016-05-13 8:20 ` Lars-Peter Clausen
1 sibling, 1 reply; 8+ messages in thread
From: Ricard Wanderlof @ 2016-05-11 15:16 UTC (permalink / raw)
To: Lars-Peter Clausen; +Cc: alsa-devel
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: ADAU1761 default register value problem
2016-05-11 15:16 ` Ricard Wanderlof
@ 2016-05-13 8:20 ` Lars-Peter Clausen
2016-05-16 11:07 ` Ricard Wanderlof
0 siblings, 1 reply; 8+ messages in thread
From: Lars-Peter Clausen @ 2016-05-13 8:20 UTC (permalink / raw)
To: Ricard Wanderlof; +Cc: alsa-devel
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: ADAU1761 default register value problem
2016-05-13 8:20 ` Lars-Peter Clausen
@ 2016-05-16 11:07 ` Ricard Wanderlof
2016-05-16 17:02 ` Lars-Peter Clausen
0 siblings, 1 reply; 8+ messages in thread
From: Ricard Wanderlof @ 2016-05-16 11:07 UTC (permalink / raw)
To: Lars-Peter Clausen; +Cc: alsa-devel
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: ADAU1761 default register value problem
2016-05-16 11:07 ` Ricard Wanderlof
@ 2016-05-16 17:02 ` Lars-Peter Clausen
0 siblings, 0 replies; 8+ messages in thread
From: Lars-Peter Clausen @ 2016-05-16 17:02 UTC (permalink / raw)
To: Ricard Wanderlof; +Cc: alsa-devel
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* ADAU1761 default register value problem
@ 2016-03-29 13:58 Ricard Wanderlof
0 siblings, 0 replies; 8+ messages in thread
From: Ricard Wanderlof @ 2016-03-29 13:58 UTC (permalink / raw)
To: alsa-devel
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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2016-05-16 17:02 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-04-21 14:01 ADAU1761 default register value problem Ricard Wanderlof
2016-04-26 8:21 ` Lars-Peter Clausen
2016-05-03 8:16 ` Ricard Wanderlof
2016-05-11 15:16 ` Ricard Wanderlof
2016-05-13 8:20 ` Lars-Peter Clausen
2016-05-16 11:07 ` Ricard Wanderlof
2016-05-16 17:02 ` Lars-Peter Clausen
-- strict thread matches above, loose matches on Subject: below --
2016-03-29 13:58 Ricard Wanderlof
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.