Reverts commit 62d5ae4caf ("ASoC: max98090: save and restore
SHDN when changing sensitive registers").
A critical side-effect was observed: when keep playing something,
the recorded sound has chance to break (clipping).
Signed-off-by: Tzung-Bi Shih <tzungbi@google.com>
Link: https://lore.kernel.org/r/20200214105744.82258-2-tzungbi@google.com
Signed-off-by: Mark Brown <broonie@kernel.org>
Commit 2dc98af62c ("ASoC: max98090: fix lockdep warning") introduced
a helpful-less small lock: shdn_lock. Reverts the commit.
Reasons:
1. Lockdep should not be happy by either the original or current code.
From lockdep's point of view, there is a lock inversion anyway.
Let d = dapm_mutex, c = controls_rwsem, s = shdn_lock,
From the reported calling stack: lock acquisition order of
snd_soc_register_card() is: d -> c.
> snd_ctl_add_replace+0x3c/0x84
> dapm_create_or_share_kcontrol+0x24c/0x2e0
> snd_soc_dapm_new_widgets+0x308/0x594
> snd_soc_bind_card+0x80c/0xad4
> devm_snd_soc_register_card+0x34/0x6c
If calling snd_soc_dapm_put_enum_double() in kcontrol's put (e.g.
SOC_DAPM_ENUM_EXT), lock acquisition order is: c -> d. Note that,
snd_soc_dapm_put_enum_double() acquires d.
The possible lock inversion is always there if registering sound card
and putting mixer control happen at the same time. In fact, it never
happens because the control device don't show up to the userspace until
the sound card build success.
Commit 2dc98af62c ("ASoC: max98090: fix lockdep warning") changes the
order to: c -> s -> d. The lock inversion is still there.
2. Commit 62d5ae4caf ("ASoC: max98090: save and restore SHDN when
changing sensitive registers SHDN bit") designed to use dapm_mutex to
protect SHDN bit. Use a separate lock breaks the protection.
DAPM changes SHDN bit automatically when it finds the path. Thus, any
code wants to change the SHDN bit, need to acquire the dapm_mutex first.
> SND_SOC_DAPM_SUPPLY("SHDN", M98090_REG_DEVICE_SHUTDOWN,
> M98090_SHDNN_SHIFT, 0, NULL, 0),
Fixes: 2dc98af62c ("ASoC: max98090: fix lockdep warning")
Signed-off-by: Tzung-Bi Shih <tzungbi@google.com>
Link: https://lore.kernel.org/r/20200117073814.82441-2-tzungbi@google.com
Signed-off-by: Mark Brown <broonie@kernel.org>
Commit 62d5ae4caf ("ASoC: max98090: save and restore SHDN when changing
sensitive registers") extended the code for handling many controls by
adding a custom put function to them. That new custom put function
properly handles relations between codec's hardware registers. However
they used card->dapm_mutex to properly serialize those operations. This
in turn triggers a lockdep warning about possible circular dependency.
Fix this by introducing a separate mutex only for serializing the SHDN
hardware register related operations.
This fixes the following lockdep warning observed on Exynos4412-based
Odroid U3 board:
======================================================
WARNING: possible circular locking dependency detected
5.5.0-rc5-next-20200107 #166 Not tainted
------------------------------------------------------
alsactl/1104 is trying to acquire lock:
ed0d50f4 (&card->dapm_mutex){+.+.}, at: max98090_shdn_save+0x1c/0x28
but task is already holding lock:
edb4b49c (&card->controls_rwsem){++++}, at: snd_ctl_ioctl+0xcc/0xbb8
which lock already depends on the new lock.
the existing dependency chain (in reverse order) is:
-> #1 (&card->controls_rwsem){++++}:
snd_ctl_add_replace+0x3c/0x84
dapm_create_or_share_kcontrol+0x24c/0x2e0
snd_soc_dapm_new_widgets+0x308/0x594
snd_soc_bind_card+0x80c/0xad4
devm_snd_soc_register_card+0x34/0x6c
odroid_audio_probe+0x288/0x34c
platform_drv_probe+0x6c/0xa4
really_probe+0x200/0x490
driver_probe_device+0x78/0x1f8
bus_for_each_drv+0x74/0xb8
__device_attach+0xd4/0x16c
bus_probe_device+0x88/0x90
deferred_probe_work_func+0x3c/0xd0
process_one_work+0x22c/0x7c4
worker_thread+0x44/0x524
kthread+0x130/0x164
ret_from_fork+0x14/0x20
0x0
-> #0 (&card->dapm_mutex){+.+.}:
lock_acquire+0xe8/0x270
__mutex_lock+0x9c/0xb18
mutex_lock_nested+0x1c/0x24
max98090_shdn_save+0x1c/0x28
max98090_put_enum_double+0x20/0x40
snd_ctl_ioctl+0x190/0xbb8
ksys_ioctl+0x470/0xaf8
ret_fast_syscall+0x0/0x28
0xbefaa564
other info that might help us debug this:
Possible unsafe locking scenario:
CPU0 CPU1
---- ----
lock(&card->controls_rwsem);
lock(&card->dapm_mutex);
lock(&card->controls_rwsem);
lock(&card->dapm_mutex);
*** DEADLOCK ***
1 lock held by alsactl/1104:
#0: edb4b49c (&card->controls_rwsem){++++}, at: snd_ctl_ioctl+0xcc/0xbb8
stack backtrace:
CPU: 0 PID: 1104 Comm: alsactl Not tainted 5.5.0-rc5-next-20200107 #166
Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
(unwind_backtrace) from [<c010e180>] (show_stack+0x10/0x14)
(show_stack) from [<c0b2a09c>] (dump_stack+0xb4/0xe0)
(dump_stack) from [<c018a1c0>] (check_noncircular+0x1ec/0x208)
(check_noncircular) from [<c018c5dc>] (__lock_acquire+0x1210/0x25ec)
(__lock_acquire) from [<c018e2d8>] (lock_acquire+0xe8/0x270)
(lock_acquire) from [<c0b49678>] (__mutex_lock+0x9c/0xb18)
(__mutex_lock) from [<c0b4a110>] (mutex_lock_nested+0x1c/0x24)
(mutex_lock_nested) from [<c0839b3c>] (max98090_shdn_save+0x1c/0x28)
(max98090_shdn_save) from [<c083a5b8>] (max98090_put_enum_double+0x20/0x40)
(max98090_put_enum_double) from [<c080d0e8>] (snd_ctl_ioctl+0x190/0xbb8)
(snd_ctl_ioctl) from [<c02cafec>] (ksys_ioctl+0x470/0xaf8)
(ksys_ioctl) from [<c0101000>] (ret_fast_syscall+0x0/0x28)
...
Fixes: 62d5ae4caf ("ASoC: max98090: save and restore SHDN when changing sensitive registers")
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
Link: https://lore.kernel.org/r/20200108115007.31095-2-m.szyprowski@samsung.com
Signed-off-by: Mark Brown <broonie@kernel.org>
According to the datasheet, there are some registers can only be changed
when SHDN is 0. Changing these settings during SHDN = 1 can compromise
device stability and performance specifications.
Saves SHDN before writing to these sensitive registers and restores SHDN
afterward.
Here is the register list codec driver of max98090 wants to change:
M98090_REG_QUICK_SYSTEM_CLOCK 0x04
M98090_REG_QUICK_SAMPLE_RATE 0x05
M98090_REG_DAI_INTERFACE 0x06
M98090_REG_DAC_PATH 0x07
M98090_REG_MIC_DIRECT_TO_ADC 0x08
M98090_REG_LINE_TO_ADC 0x09
M98090_REG_ANALOG_MIC_LOOP 0x0A
M98090_REG_ANALOG_LINE_LOOP 0x0B
M98090_REG_SYSTEM_CLOCK 0x1B
M98090_REG_CLOCK_MODE 0x1C
M98090_REG_CLOCK_RATIO_NI_MSB 0x1D
M98090_REG_CLOCK_RATIO_NI_LSB 0x1E
M98090_REG_CLOCK_RATIO_MI_MSB 0x1F
M98090_REG_CLOCK_RATIO_MI_LSB 0x20
M98090_REG_MASTER_MODE 0x21
M98090_REG_INTERFACE_FORMAT 0x22
M98090_REG_TDM_CONTROL 0x23
M98090_REG_TDM_FORMAT 0x24
M98090_REG_IO_CONFIGURATION 0x25
M98090_REG_FILTER_CONFIG 0x26
M98090_REG_INPUT_ENABLE 0x3E
M98090_REG_OUTPUT_ENABLE 0x3F
M98090_REG_BIAS_CONTROL 0x42
M98090_REG_DAC_CONTROL 0x43
M98090_REG_ADC_CONTROL 0x44
M98090_REG_DRC_TIMING 0x33
M98090_REG_DRC_COMPRESSOR 0x34
M98090_REG_DRC_EXPANDER 0x35
M98090_REG_DSP_FILTER_ENABLE 0x41
M98090_REG_EQUALIZER_BASE 0x46
M98090_REG_RECORD_BIQUAD_BASE 0xAF
M98090_REG_DIGITAL_MIC_ENABLE 0x13
M98090_REG_DIGITAL_MIC_CONFIG 0x14
Signed-off-by: Tzung-Bi Shih <tzungbi@google.com>
Link: https://lore.kernel.org/r/20191128151908.180871-1-tzungbi@google.com
Signed-off-by: Mark Brown <broonie@kernel.org>
max98090_interrupt() and max98090_pll_work() run in 2 different threads.
There are 2 possible races:
Note: M98090_REG_DEVICE_STATUS = 0x01.
Note: ULK == 0, PLL is locked; ULK == 1, PLL is unlocked.
max98090_interrupt max98090_pll_work
----------------------------------------------
schedule max98090_pll_work
restart max98090 codec
receive ULK INT
assert ULK == 0
schedule max98090_pll_work (1).
In the case (1), the PLL is locked but max98090_interrupt unnecessarily
schedules another max98090_pll_work.
max98090_interrupt max98090_pll_work max98090 codec
----------------------------------------------------------------------
ULK = 1
receive ULK INT
read 0x01
ULK = 0 (clear on read)
schedule max98090_pll_work
restart max98090 codec
ULK = 1
receive ULK INT
read 0x01
ULK = 0 (clear on read)
read 0x01
assert ULK == 0 (2).
In the case (2), both max98090_interrupt and max98090_pll_work read
the same clear-on-read register. max98090_pll_work would falsely
thought PLL is locked.
Note: the case (2) race is introduced by the previous commit ("ASoC:
max98090: exit workaround earlier if PLL is locked") to check the status
and exit the loop earlier in max98090_pll_work.
There are 2 possible solution options:
A. turn off ULK interrupt before scheduling max98090_pll_work; and turn
on again before exiting max98090_pll_work.
B. remove the second thread of execution.
Option A cannot fix the case (2) race because it still has 2 threads
access the same clear-on-read register simultaneously. Although we
could suppose the register is volatile and read the status via I2C could
be much slower than the hardware raises the bits.
Option B introduces a maximum 10~12 msec penalty delay in the interrupt
handler. However, it could only punish the jack detection by extra
10~12 msec.
Adopts option B which is the better solution overall.
Signed-off-by: Tzung-Bi Shih <tzungbi@google.com>
Link: https://lore.kernel.org/r/20191122073114.219945-4-tzungbi@google.com
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
Based on 2 normalized pattern(s):
this program is free software you can redistribute it and or modify
it under the terms of the gnu general public license version 2 as
published by the free software foundation
this program is free software you can redistribute it and or modify
it under the terms of the gnu general public license version 2 as
published by the free software foundation #
extracted by the scancode license scanner the SPDX license identifier
GPL-2.0-only
has been chosen to replace the boilerplate/reference in 4122 file(s).
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Reviewed-by: Enrico Weigelt <info@metux.net>
Reviewed-by: Kate Stewart <kstewart@linuxfoundation.org>
Reviewed-by: Allison Randal <allison@lohutok.net>
Cc: linux-spdx@vger.kernel.org
Link: https://lkml.kernel.org/r/20190604081206.933168790@linutronix.de
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Enforce correct device sequencing when configuring a new
audio route when there is an existing active audio route(s).
This patch fixed recording noise issue while playback is active.
We have some registers which require the device to be in full shutdown
or to enter full shutdown before the register settings will take effect.
Currently the driver is not shutting down the device when a new audio
route is created. If a new audio route is made active while there is
already an active audio route, then the required register sequencing is
violated. A hardware shutdown toggle when creating a new audio route
corrects the sequencing error. The device must remain in hardware
shutdown for 40ms to allow the internal hardware core to fully shutdown.
Signed-off-by: Fang, Yang A <yang.a.fang@intel.com>
Signed-off-by: Sathyanarayana Nujella <sathyanarayana.nujella@intel.com>
Acked-by: Anish Kumar <anish.kumar@maximintegrated.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
In addtion expand the table to handle other values of sysclk. Instead
of making the table 3D, expand it to a more descriptive struct. The
divisors are specified in Table 19 of the 98090 data sheet version
0p94.
The dmic frequency was previously assumed. Instead make it explicit
and configurable through device tree. This now handles independently
set pclk and dmic frequency.
Based on downstream work by Ralph Birt.
Signed-off-by: Dylan Reid <dgreid@chromium.org>
Signed-off-by: Mark Brown <broonie@kernel.org>
There is no other use for extmic_mux than setting it to zero so remove it.
Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
struct max98090_priv member irq is now used only locally in
max98090_i2c_probe() and can be removed.
Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
All MAX98090 input clocks MCLK, LRCLK and BCLK must be running and stable
before powering on the codec in slave mode. Otherwise the PLL may not lock
to LRCLK causing silence in playback and capture. How often that happens is
somewhat hardware and clock configuration specific.
Now if wanting to follow strictly this clocks must be active before
powering the codec on requirement we should have a notification from DAI
driver to codec driver when clocks are activated and take codec out of
shutdown only after that. Plus take care of possible active bypass paths.
However, when PLL unlock occurs, MAX98090 asserts the PLL Unlock Flag which
can be configured as an IRQ source. This allows to workaround around the
issue by toggling the codec power shortly in case of PLL lock failure.
In order to prevent needlessly toggling codec power in case of short PLL
unlocks at the beginning of stream this patch implements delayed activation
for PLL unlock interrupt. Then workaround is run only when the PLL doesn't
lock at all.
Power toggling workaround for PLL unlock comes originally from
Liam Girdwood <liam.r.girdwood@linux.intel.com> and delayed activation from
me.
Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
If master clock is provided through device tree, then update
the master clock frequency during set_sysclk.
Documentation has been updated to reflect the change.
Signed-off-by: Tushar Behera <tushar.behera@linaro.org>
Signed-off-by: Mark Brown <broonie@linaro.org>
BCL is being configured in two places producing a warning message.
Make sure we only configure BCLK once and when we are master.
Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>
Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Signed-off-by: Mark Brown <broonie@linaro.org>
The driver assigns a value to the control_data field of the driver's state
struct, but never reads it again. Which means it is unused and can be removed.
Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
Signed-off-by: Mark Brown <broonie@linaro.org>
Source files shouldn't have the executable bit set.
Signed-off-by: Joe Perches <joe@perches.com>
Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
This patch completes the replacement of the existing max98090 driver,
by installing a more complete driver.
Signed-off-by: Jerry Wong <jerry.wong@maximintegrated.com>
Tested-by: Matthew Mowdy <matthew.mowdy@maximintegrated.com>
Reviewed-by: Ralph Birt <ralph.birt@maximintegrated.com>
Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
Tzung-Bi Shih