ASoC: codec: Fix IO timeout when doing jack detection #1580
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When a Slave device becomes synchronized with the bus, it may report its presence in PING frames, as well as optionally asserting an in-band PREQ signal. The bus driver will detect a new Device0, start the enumeration process and assign it a non-zero device number. The SoundWire enumeration provides an arbitration to deal with multiple Slaves reporting ATTACHED at the same time. The bus driver will also invoke the driver .probe() callback associated with this device. The probe() depends on the Linux device core, which handles the match operations and may result in modules being loaded. Once the non-zero device number is programmed, the Slave will report its new status in PING frames and the Master hardware will typically report this status change with an interrupt. At this point, the .update_status() callback of the codec driver will be invoked (usually from an interrupt thread or workqueue scheduled from the interrupt thread). The first race condition which can happen is between the .probe(), which allocates the resources, and .update_status() where initializations are typically handled. The .probe() is only called once during the initial boot, while .update_status() will be called for every bus hardware reset and if the Slave device loses synchronization (an unlikely event but with non-zero probability). The time difference between the end of the enumeration process and a change of status reported by the hardware may be as small as one SoundWire PING frame. The scheduling of the interrupt thread, which invokes .update_status() is not deterministic, but can be small enough to create a race condition. With a 48 kHz frame rate and ideal scheduling cases, the .probe() may be pre-empted within double-digit microseconds. Since there is no guarantee that the .probe() completes by the time .update_status() is invoked as a result of an interrupt, it's not unusual for the .update_status() to rely on data structures that have not been allocated yet, leading to kernel oopses. This patch adds a probe_complete utility, which is used in the sdw_update_slave_status() routine. The codec driver does not need to do anything and can safely assume all resources are allocated in its update_status() callback. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
When the Master starts the bus (be it during the initial boot or system resume), it usually performs a HardReset to make sure electrical levels are correct, then enables the control channel. While the PM framework guarantees that the Slave devices will only become 'active' once the Master completes the bus initialization, there is still a risk of a race condition: the Slave enumeration is handled in a separate interrupt thread triggered by hardware status changes, so the Slave device may not be ready to accept commands when the Slave driver tries to access the registers and restore settings in its resume or pm_runtime_resume callbacks. In those cases, any read/write commands from/to the Slave device will result in a timeout. This patch adds an enumeration_complete structure. When the bus is goes through a HardReset sequence and restarted, the Slave will be marked as UNATTACHED, which will result in a call to init_completion(). When the Slave reports its presence during PING frames as a non-zero Device, the Master hardware will issue an interrupt and the bus driver will invoke complete(). The order between init_completion()/complete() is predictable since this is a Master-initiated transition. The Slave driver may use wait_for_completion() in its resume callback. When regmap is used, the Slave driver will typically set its regmap in cache-only mode on suspend, then on resume block on wait_for_completion(&enumeration_complete) to guarantee it is safe to start read/write transactions. It may then exit the cache-only mode and use a regmap_sync to restore settings. All these steps are optional, their use completely depends on the Slave device capabilities and how the Slave driver is implemented. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Slave drivers may have different ways of handling their settings, with or without regmap. During the integration of codec drivers, done in partnership between Intel and Realtek, it became desirable to implement a predictable order between low-level initializations performed in .update_status() (invoked by an interrupt thread) and the settings restored in the resume steps (invoked by the PM core). This patch builds on the previous solution to wait for the Slave device to be fully enumerated. The complete() in this case is signaled not before the .update_status() is called, but after .update_status() returns. Without this patch, the settings were not properly restored, leading to timing-dependent 'no sound after resume' or 'no headset detected after resume' bug reports. Depending on how initialization is handled, a Slave device driver may wait for enumeration_complete, or for initialization_complete, both are valid synchronization points. They are initialized at the same time, they only differ on when complete() is invoked. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
…nces The Slave device initialization can be split in 4 different cases: 1. Master-initiated hardware reset, system suspend-resume and pm_runtime based on clock-stop mode1. To avoid timeouts and a bad audio experience, the Slave device resume operations need to wait for the Slave device to be re-enumerated and its settings restored. 2. Exit from clock-stop mode0. In this case, the Slave device is required to remain enumerated and its context preserved while the clock is stopped, so no re-initialization or wait_for_completion() is necessary. 3. Slave-initiated pm_runtime D3 transition. With the parent child relationship, it is possible that a Slave device becomes 'suspended' while its parent is still 'active' with the bus clock still toggling. In this case, during the pm_runtime resume operation, there is no need to wait for any settings to be restored. 4. Slave reset (sync loss or implementation-defined). In that case the bus remains operational and the Slave device will be re-initialized when it becomes ATTACHED again. In previous patches, we suggested the use of wait_for_completion() to deal with the case #1, but case thesofproject#2 and thesofproject#3 do not need any wait. To account for those differences, this patch adds an unattach_request field. The field is explicitly set by the Master for the case #1, and if non-zero the Slave device shall wait on resume. In all other cases, the Slave resume operations can proceed without wait. The only request tracked so far is Master HardReset, but the request is declared as a bit mask for future extensions (if needed). The definition for this value is added in bus.h and does not need to be exposed in sdw.h Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
The current interfaces between ASoC and SoundWire are limited by the platform_device infrastructure to an init() and exit() (mapped to the platform driver.probe and .remove) To help with the platform detection, machine driver selection and management of power dependencies between DSP and SoundWire IP, the ASoC side requires: a) an ACPI scan helper, to report if any devices are exposed in the DSDT tables, and if any links are disabled by the BIOS. b) a probe helper that allocates the resources without actually starting the bus. c) a startup helper which does start the bus when all power dependencies are settled. d) an exit helper to free all resources e) an interrupt_enable/disable helper, typically invoked after the startup helper but also used in suspend routines. This patch moves all required interfaces to sdw_intel.h, mainly to allow SoundWire and ASoC parts to be merged separately once the header files are shared between trees. To avoid compilation issues, the conflicts in intel_init.c are blindly removed. This would in theory prevent the code from working, but since there are no users of the Intel Soundwire driver this has no impact. Functionality will be restored when the removal of platform devices is complete. Support for SoundWire + SOF builds will only be provided once all the required pieces are upstream. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
…erations The SoundWire DAIs for Intel platform are created in drivers/soundwire/intel.c, while the communication with the Intel DSP is all controlled in soc/sof/intel When the DAI status changes, a callback is used to bridge the gap between the two subsystems. The naming of the existing 'config_stream' callback does not map well with any of ALSA/ASoC concepts. This patch renames it as 'params_stream' to be more self-explanatory. A new 'free_stream' callback is added in case any resources allocated in the 'params_stream' stage need to be released. In the SOF implementation, this is used in the hw_free case to release the DMA channels over IPC. These two callbacks now rely on structures which expose the link_id and alh_stream_id (required by the firmware IPC), instead of a list of parameters. The 'void *' definitions are changed to use explicit types, as suggested on alsa-devel during earlier reviews. Signed-off-by: Rander Wang <rander.wang@linux.intel.com> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
The existing use of 6 handlers is problematic in MSI mode. Update headers so that all shared interrupts can be handled with a single handler. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Initial support for ALC/RT700 in SoundWire mode This codec is present on Intel CNL/CML and ICL RVP boards and the code was tested by both Realtek and Intel. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> Signed-off-by: Shuming Fan <shumingf@realtek.com>
The driver for this amplifier was tested by Realtek and Intel using the Realtek 3-in-1 add-on boards connected to CometLake and IceLake platforms Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> Signed-off-by: Shuming Fan <shumingf@realtek.com>
The driver for this codec was tested by Realtek and Intel using the Realtek 3-in-1 add-on board connected to CometLake and IceLake platforms. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> Signed-off-by: Shuming Fan <shumingf@realtek.com>
The driver for this capture device was tested by Realtek and Intel using the Realtek 3-in-1 add-on boards connected to CometLake and IceLake platforms Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> Signed-off-by: Jack Yu <jack.yu@realtek.com>
Don't push upstream Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Fixes: 340dea8 ("ASoC: codecs: rt715: add SoundWire support") Signed-off-by: kbuild test robot <lkp@intel.com>
…s to 24bits It takes too many times to dump the registers when using 32 bits mapping. We change it to 24 bits to map the Realtek index defined registers. And this patch fixes some coding style. Signed-off-by: Shuming Fan <shumingf@realtek.com>
…s to 24bits It takes too many times to dump the registers when using 32 bits mapping. We change it to 24 bits to map the Realtek index defined registers. And this patch fixes some coding style. Signed-off-by: Shuming Fan <shumingf@realtek.com>
Signed-off-by: Shuming Fan <shumingf@realtek.com>
Signed-off-by: Jack Yu <jack.yu@realtek.com>
In dell project, the JD source changes to JD2. Therefore, the codec driver add the JD2 configuration in default. The application circuit also changes to JD2 as JD source. Signed-off-by: Shuming Fan <shumingf@realtek.com>
… 24bits. Signed-off-by: Jack Yu <jack.yu@realtek.com>
Signed-off-by: Shuming Fan <shumingf@realtek.com>
Signed-off-by: Shuming Fan <shumingf@realtek.com>
…olume Fix the PVDD DC calibration parameters doesn't be loaded correctly. The DAC volume gain adjusts to the 0xe7 value which HW AE suggests. Signed-off-by: Shuming Fan <shumingf@realtek.com>
…d add return handle for regmap_read. Signed-off-by: Jack Yu <jack.yu@realtek.com>
The HW engineer changes HV_Gain voltage setting to enhance the output gain. And he also adjusts the DAC volume gain. Signed-off-by: Shuming Fan <shumingf@realtek.com>
Intel add device property on machine driver. So we can't parse device before machine driver is probed. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Make sure the resume steps are delayed until all settings have been restored after the device becomes enumerated and .update_status is called. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Make sure the resume steps are delayed until all settings have been restored after the device becomes enumerated and .update_status is called. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Make sure the resume steps are delayed until all settings have been restored after the device becomes enumerated and .update_status is called. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Make sure the resume steps are delayed until all settings have been restored after the device becomes enumerated and .update_status is called Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Shuming Fan <shumingf@realtek.com>
Signed-off-by: Shuming Fan <shumingf@realtek.com>
Signed-off-by: Shuming Fan <shumingf@realtek.com>
Signed-off-by: Shuming Fan <shumingf@realtek.com>
Signed-off-by: Jack Yu <jack.yu@realtek.com>
…letion() Only start the wait_for_completion() if the Master device requested the Slave device status to become unattached, e.g with a bus reset. For normal pm_runtime resume just sync the regmap Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
…etion() Only start the wait_for_completion() if the Master device requested the Slave device status to become unattached, e.g with a bus reset. For normal pm_runtime resume just sync the regmap Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
…etion() Only start the wait_for_completion() if the Master device requested the Slave device status to become unattached, e.g with a bus reset. For normal pm_runtime resume just sync the regmap Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
…etion() Only start the wait_for_completion() if the Master device requested the Slave device status to become unattached, e.g with a bus reset. For normal pm_runtime resume just sync the regmap Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
The resume function needs to cache sync JD2 settings. Signed-off-by: Shuming Fan <shumingf@realtek.com>
…_reg_defaults[] Signed-off-by: Jack Yu <jack.yu@realtek.com>
The field "is_sdw" is used for distinguishing the driver whether is run in soundwire mode or not. That will run the separated setting in runtime to make sure the driver can be run with the same build between i2s mode and soundwire mode. Signed-off-by: Oder Chiou <oder_chiou@realtek.com>
This patch adds the soundwire support for ALC5682. Signed-off-by: Oder Chiou <oder_chiou@realtek.com>
Signed-off-by: Jack Yu <jack.yu@realtek.com>
Since btn_check_handler and jack_detect_handler work in work thread, we need to wake up codecs when doing jack detection. Signed-off-by: Rander Wang <rander.wang@intel.com>
|
@YvonneYang2 Please test this PR with soundwire-latest. Thanks! |
|
@shumingfan hi shuming, please review. thanks! |
|
@RanderWang Can we do this around slave->ops->interrupt_callback(slave, &slave_intr);? I think all slave has to be waked up when it received an interrupt. And the active time should be enough if we call pm_runtime_mark_last_busy(). |
|
I don't think the solution is correct. I am ready to bet that the timeout happens because we end-up resuming the bus while there is an alert. I think we should instead play with pm_runtime refcounts in so that we never read/write slave registers before resuming properly. |
plbossart
force-pushed
the
integration/realtek-sdw-codecs
branch
from
77b3ef4 to
dec134e
Compare
@shumingfan found in two cases now: I have question for you: codec driver should deal with pm change or caller should process pm change ? I don't find any PM get in codec driver. Thanks! |
I know what's problem you fixed now. The codec driver will access registers when the JD/Btn interrupt happened. Due to the cache_only was enabled in suspend function, the interrupt callback function will get the return value (-EBUSY) from regmap_read/write. |
yes, this a good solution. I talked to shuming about your idea and I will fix this bug in sdw_handle_slave_alerts in PR #1589 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Since btn_check_handler and jack_detect_handler work in
work thread, we need to wake up codecs when doing jack
detection.
Signed-off-by: Rander Wang rander.wang@intel.com