Add clock stop & jack detection support for SDW #1498
Closed
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
plbossart
reviewed
Nov 16, 2019
drivers/soundwire/bus.c
Outdated
|
|
||
| ret = sdw_write_no_pm(slave, SDW_SCP_SYSTEMCTRL, val); | ||
|
|
||
| if (ret != 0 && ret != -ENODATA) |
There was a problem hiding this comment.
the second part is questionable.
I believe ret = -ENODATA means the device did not respond. I am not sure we want to go ahead even in this case, or it would be to be explicit with a comment.
There was a problem hiding this comment.
According to caller, if ret == -ENODATA, clock-stop will be failed. So we can just remove this check and print error message
drivers/soundwire/bus.c
Outdated
| retry--; | ||
| } while (retry); | ||
|
|
||
| if (retry && (val == 0 || val == -ENODATA)) { |
There was a problem hiding this comment.
same here, not sure why -ENODATA is an acceptable reply on a broadcast read?
drivers/soundwire/bus.c
Outdated
| /* Identify if Slave(s) are available on Bus */ | ||
| is_slave = true; | ||
|
|
||
| if (slave->status == SDW_SLAVE_ATTACHED) { |
There was a problem hiding this comment.
if the slave status changes to ALERT, what happens?
This test is not good enough.
I think it meant ATTACHED || ALERT
drivers/soundwire/bus.c
Outdated
| if (!slave->dev_num) | ||
| continue; | ||
|
|
||
| if (slave->status != SDW_SLAVE_ATTACHED) |
drivers/soundwire/intel.c
Outdated
| @@ -1590,7 +1604,7 @@ static int intel_resume_runtime(struct device *dev) | |||
| return ret; | |||
| } | |||
|
|
|||
| ret = sdw_cdns_exit_reset(cdns); | |||
| ret = sdw_cdns_resume(cdns, false); | |||
There was a problem hiding this comment.
well it depends. We have a quirk to disable clock-stop-mode even in pm_runtime cases.
There was a problem hiding this comment.
You really want to check if clock-stop is allowed or not as I tried to explain above.
There was a problem hiding this comment.
thanks, I will move this check from next commit to here.
drivers/soundwire/intel.c
Outdated
| struct sdw_md_driver intel_sdw_driver = { | ||
| .driver = { | ||
| .name = "intel-sdw", | ||
| .owner = THIS_MODULE, | ||
| .pm = &intel_pm, | ||
| .pm = INTEL_PM, |
There was a problem hiding this comment.
no, this should be done with the routines compiled out and a fallback provided.
drivers/soundwire/intel.c
Outdated
| */ | ||
| list_for_each_entry(slave, &bus->slaves, node) { | ||
| if (slave->prop.wake_capable) { | ||
| if (slave->status != SDW_SLAVE_ATTACHED) |
There was a problem hiding this comment.
compared to normal clock stop mode, master is powered off in our clock stop implementation, so master can't response to slave. Instead, glue hw will get the wake-up event and report a PME event to PCI to wake up pci driver (sof-audio). sof-audio driver will wake up master and slave to check event
There was a problem hiding this comment.
it doesn't matter what the master does.
The point is that the slave can report in ALERT from the first PING frame, this is a different state machine. The master cannot control what the slave does.
drivers/soundwire/intel.c
Outdated
| } | ||
| } | ||
| } | ||
| EXPORT_SYMBOL(sdw_intel_jack_detect); |
There was a problem hiding this comment.
It is used in SOF. But Now I find there is no sdw change in sof driver, so I can't check it in.
RanderWang
force-pushed
the
clk_stop_jack_detection
branch
2 times, most recently
from
cb073a0 to
3c7204c
Compare
|
@plbossart Update my PR. And We also need to clear WAKEEN interrupt ASAP, or it will affect IPC interrupt. Now there is no ipc issue when WAKEEN irq is cleared in WAKEEN process function |
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>
Add clearer references to sdw_slave_driver for internal macros No change for sdw_driver and module_sdw_driver to avoid compatibility issues with existing codec devices No functionality change. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Since we want to introduce master devices, rename macro so that we have consistency between slave and master device access, following the Grey Bus example. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Align with previous renames and shorten macro No functionality change Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Before we add master driver support, make sure there is no ambiguity and no occurrences of sdw_drv_ functions. No functionality change. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
There are too many fields called 'res' so add prefix to make it easier to track what the structures are. Pure rename, no functionality change Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Currently the bus does not have any explicit support for master devices. First add explicit support for sdw_slave_type and error checks if this type is not set. In follow-up patches we can add support for the sdw_md_type (md==Master Device), following the Grey Bus example. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Currently the code deals with uevents at the bus level, but we only care for Slave events Suggested-by: Vinod Koul <vkoul@kernel.org> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Since we want an explicit support for the SoundWire Master device, add the definitions, following the Grey Bus example. Unlike for the Slave device, we do not set a variable when dealing with the master uevent. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Use sdw_master_device and driver instead of platform devices To quote GregKH: "Don't mess with a platform device unless you really have no other possible choice. And even then, don't do it and try to do something else. Platform devices are really abused, don't perpetuate it " In addition, rather than a plain-vanilla init/exit, this patch provides 3 steps in the initialization (ACPI scan, probe, startup) which make it easier to verify support and allocate required resources as early as possible, and conversely help make the startup lighter-weight with only hardware register setup. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Setting an device driver is necessary for ASoC to register DAI components. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
The existing code does not expose a prepare operation, which is very much needed to deal with underflow and resume operations. Signed-off-by: Rander Wang <rander.wang@linux.intel.com> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
The existing code does not expose a trigger callback, which is very much required for streaming. The SoundWire stream is enabled and disabled in trigger function. Signed-off-by: Rander Wang <rander.wang@linux.intel.com> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
The sdw stream is allocated and stored in dai to share the sdw runtime information. Signed-off-by: Rander Wang <rander.wang@linux.intel.com> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Make sure all calls to the SoundWire stream API are done and involve callback Signed-off-by: Rander Wang <rander.wang@linux.intel.com> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
This function is required to enable all interrupts across all links. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
The existing code uses one pair of interrupt handler/thread per link but at the hardware level the interrupt is shared. This works fine for legacy PCI interrupts, but leads to timeouts in MSI (Message-Signaled Interrupt) mode, likely due to edges being lost. This patch unifies interrupt handling for all links with a single threaded IRQ handler. The handler is simplified to the bare minimum of detecting a SoundWire interrupt, and the thread takes care of dealing with interrupt sources. This partition follows the model used for the SOF IPC on HDaudio platforms, where similar timeout issues were noticed and doing all the interrupt handling/clearing in the thread improved reliability/stability. Validation results with 4 links active in parallel show a night-and-day improvement with no timeouts noticed even during stress tests. Latency and quality of service are not affected by the change - mostly because events on a SoundWire link are throttled by the bus frame rate (typically 8..48kHz). Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
The driver probe takes care of basic initialization and is invoked when a Slave becomes attached, after a match between the Slave DevID registers and ACPI/DT entries. The update_status callback is invoked when a Slave state changes, e.g. when it is assigned a non-zero Device Number and it reports with an ATTACHED/ALERT state. The state change detection is usually hardware-based and based on the SoundWire frame rate (e.g. double-digit microseconds) while the probe is a pure software operation, which may involve a kernel module load. In corner cases, it's possible that the state changes before the probe completes. This patch suggests the use of wait_for_completion to avoid races on startup, so that the update_status callback does not rely on invalid pointers/data structures. Signed-off-by: Rander Wang <rander.wang@intel.com> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
RanderWang
force-pushed
the
clk_stop_jack_detection
branch
from
b34b84d to
1f7ecbb
Compare
ranj063
reviewed
Nov 19, 2019
drivers/soundwire/intel.c
Outdated
| struct sdw_intel *sdw = cdns_to_intel(cdns); | ||
| int ret; | ||
|
|
||
| dev_err(dev, "%s start\n", __func__); |
There was a problem hiding this comment.
thanks, dev_dbg ? I will refine it.
ranj063
reviewed
Nov 19, 2019
drivers/soundwire/intel.c
Outdated
| /* set WAKEEN interrupt for wake-up events */ | ||
| intel_shim_wake(sdw, clock_stop); | ||
|
|
||
| dev_err(dev, "%s done\n", __func__); |
There was a problem hiding this comment.
same comment for all dev_err's in this patch
ranj063
reviewed
Nov 19, 2019
ranj063
reviewed
Nov 19, 2019
drivers/soundwire/intel.c
Outdated
| wake_sts = intel_readw(sdw->link_res->shim, SDW_SHIM_WAKESTS); | ||
|
|
||
| /* disable WAKEEN interrupt ASAP to prevent interrupt flood */ | ||
| intel_shim_wake(sdw, false); |
There was a problem hiding this comment.
@RanderWang just wondering: do the above 2 statements to read the wake_sts and disable wakeen need to be inside the list iterator or can they be outside?
There was a problem hiding this comment.
@ranj063 intel_shim_wake(sdw, false) is for each link, so it can't be outside.
Yes, It is better to move wake_sts outsize. I found it was easy to get shim with sdw->link_res->shim in inside.
I will move it outside.
SoundWire supports two clock stop modes. Add support to handle the clock stop modes and add pm_runtime calls in the bus. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Add Cadence runtime power management routines, to be used for clock-stop mode. Signed-off-by: Rander Wang <rander.wang@intel.com> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
…_status When resuming from clock stop state, the Slaves in Clock Stop Mode 1 should be unattached. They will be enumerated back after exiting Clock Stop Mode 1 Signed-off-by: Rander Wang <rander.wang@intel.com>
The default behavior is to use clock_stop only for pm_runtime. And WAKEEN interrupt is set in pm_runtime function for wake-up events, such as jack event. During regular resume, the link is completely re-initialized. Signed-off-by: Rander Wang <rander.wang@intel.com> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
These two helper functions are support clock stop mode. It will be shared by runtime pm and normal pm. This commit aims to make pm changes more clear to reviewer. Signed-off-by: Rander Wang <rander.wang@intel.com>
Runtime pm works in clock stop mode while there is no change for regular pm. An kernel module option can override this policy and disable clock-stop modes for runtime pm. Signed-off-by: Rander Wang <rander.wang@intel.com> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
RanderWang
force-pushed
the
clk_stop_jack_detection
branch
from
1f7ecbb to
ce37f71
Compare
When system is suspended in clock stop mode on intel platforms, both master and slave are in clock stop mode and soundwire bus is taken over by a special hardware. The bus message for jack event is processed by this special hardware, which will trigger an interrupt to ACPI system and resume audio pci device. Then audio pci driver will resume soundwire master and slave, transfer bus ownership to master, finally slave will report jack event to master and codec driver is triggered to check jack status. Signed-off-by: Rander Wang <rander.wang@intel.com>
|
@plbossart @ranj063 I update my PR according to your comments. Now I remove the HW RST and don't clear slave status if it is in clock stop 0 |
plbossart
force-pushed
the
integration/soundwire-intel
branch
4 times, most recently
from
d787bde to
0746752
Compare
plbossart
force-pushed
the
integration/soundwire-intel
branch
8 times, most recently
from
1ed9f9f to
c6a78ee
Compare
|
closing, all the clock stop and jack detection work happens on PR #1586 |
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.
on intel platform, a glue HW helps clock stop and jack detection. The clock stop steps are:
(1) Master & slave are set to clock stop mode.
(2) Glue HW takes over soundwire bus. WAKEEN feature is set for wake-up event like jack event
(3) audio pci device will go to D3 state
(4) When slave detects jack event, it will send a signal to notify Glue HW to trigger a PME event to ACPI system to resume audio pci driver to D0 state
(5) SOF driver check WAKEEN status and resume master & slave to check the jack status.