[alsa-devel] [PATCH v4 00/11] soundwire: update ASoC interfaces
We need new fields in existing structures to a) deal with race conditions on codec probe/enumeration b) allow for multi-step ACPI scan/probe/startup on Intel plaforms c) deal with MSI issues using a single handler/threads for all audio interrupts d) deal with access to registers shared across multiple links on Intel platforms
These structures for a) will be used by the SOF driver as well as codec drivers. The b) c) and d) cases are only for the Intel-specific implementation.
To avoid conflicts between ASoC and Soundwire trees, these 11 patches are provided out-of-order, before the functionality enabled in these header files is added in follow-up patch series which can be applied separately in the ASoC and Soundwire trees. As discussed earlier, Vinod would need to provide an immutable tag for Mark Brown, and the integration on the ASoC side of SOF changes and new codecs drivers can proceed in parallel with SoundWire core changes.
I had multiple offline discussions with Vinod/Mark/Takashi on how to proceed withe volume of SoundWire changes. Now that v5.5-rc1 is out we should go ahead with these interface changes. The next patchset has not changed, the series "[PATCH v3 00/15] soundwire: intel: implement new ASoC interfaces​" can still be reviewed.
An update for the series "[PATCH v3 00/22] soundwire: code hardening and suspend-resume support" will be provided later this week (one last minute issue to fix)
Comments and feedback welcome ~Pierre
Changes since v3: Reordered code and added kernel doc comments Added prototypes and fields needed to deal with SoundWire interrupts in a single handler/thread, following what is already done on the DSP side. Added mutex to control access to registers shared across links Added initial definitions for clock stop support on Intel platforms. Depending on power and latency requirements, different "quirks" can be supported.
Changes since v2: Added new field to deal with a race condition leading to a timeout when the codec goes through a pm_runtime suspend/resume transition while the Master remains active. Clarified commit messages with detailed explanations what those race conditions are and why the changes were introduced. Reordered fields for Intel routines Added kernel-doc definitions for structures Modified the patch subjects to make the mapping between interface definition and implementation straightforward.
Changes since v1 (no feedback received since October 23) additional initialization_complete utility to help codec drivers with their resume operation, waiting for the enumeration to complete is not always enough.
Bard Liao (2): soundwire: intel: update headers for interrupts soundwire: intel: add link_list to handle interrupts with a single thread
Pierre-Louis Bossart (7): soundwire: sdw_slave: add probe_complete structure and new fields soundwire: sdw_slave: add enumeration_complete structure soundwire: sdw_slave: add initialization_complete definition soundwire: sdw_slave: track unattach_request to handle all init sequences soundwire: intel: update interfaces between ASoC and SoundWire soundwire: intel: add mutex for shared SHIM register access soundwire: intel: add clock stop quirks
Rander Wang (2): soundwire: intel: update stream callbacks for hwparams/free stream operations soundwire: intel: add prototype for WAKEEN interrupt processing
drivers/soundwire/intel.c | 20 ++-- drivers/soundwire/intel.h | 13 ++- drivers/soundwire/intel_init.c | 31 ++---- include/linux/soundwire/sdw.h | 19 ++++ include/linux/soundwire/sdw_intel.h | 154 ++++++++++++++++++++++++++-- 5 files changed, 194 insertions(+), 43 deletions(-)
base-commit: e42617b825f8073569da76dc4510bfa019b1c35a
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 --- include/linux/soundwire/sdw.h | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h index 28745b9ba279..cb1db4a7475d 100644 --- a/include/linux/soundwire/sdw.h +++ b/include/linux/soundwire/sdw.h @@ -547,6 +547,10 @@ struct sdw_slave_ops { * @node: node for bus list * @port_ready: Port ready completion flag for each Slave port * @dev_num: Device Number assigned by Bus + * @probed: boolean tracking driver state + * @probe_complete: completion utility to control potential races + * on startup between driver probe/initialization and SoundWire + * Slave state changes/implementation-defined interrupts */ struct sdw_slave { struct sdw_slave_id id; @@ -561,6 +565,8 @@ struct sdw_slave { struct list_head node; struct completion *port_ready; u16 dev_num; + bool probed; + struct completion probe_complete; };
#define dev_to_sdw_dev(_dev) container_of(_dev, struct sdw_slave, dev)
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 --- include/linux/soundwire/sdw.h | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h index cb1db4a7475d..3fa8d875b16b 100644 --- a/include/linux/soundwire/sdw.h +++ b/include/linux/soundwire/sdw.h @@ -551,6 +551,9 @@ struct sdw_slave_ops { * @probe_complete: completion utility to control potential races * on startup between driver probe/initialization and SoundWire * Slave state changes/implementation-defined interrupts + * @enumeration_complete: completion utility to control potential races + * on startup between device enumeration and read/write access to the + * Slave device */ struct sdw_slave { struct sdw_slave_id id; @@ -567,6 +570,7 @@ struct sdw_slave { u16 dev_num; bool probed; struct completion probe_complete; + struct completion enumeration_complete; };
#define dev_to_sdw_dev(_dev) container_of(_dev, struct sdw_slave, dev)
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 --- include/linux/soundwire/sdw.h | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h index 3fa8d875b16b..ed42cd79eab7 100644 --- a/include/linux/soundwire/sdw.h +++ b/include/linux/soundwire/sdw.h @@ -554,6 +554,8 @@ struct sdw_slave_ops { * @enumeration_complete: completion utility to control potential races * on startup between device enumeration and read/write access to the * Slave device + * @initialization_complete: completion utility to control potential races + * on startup between device enumeration and settings being restored */ struct sdw_slave { struct sdw_slave_id id; @@ -571,6 +573,7 @@ struct sdw_slave { bool probed; struct completion probe_complete; struct completion enumeration_complete; + struct completion initialization_complete; };
#define dev_to_sdw_dev(_dev) container_of(_dev, struct sdw_slave, dev)
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 #2 and #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 --- include/linux/soundwire/sdw.h | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h index ed42cd79eab7..b7c9eca4332a 100644 --- a/include/linux/soundwire/sdw.h +++ b/include/linux/soundwire/sdw.h @@ -556,6 +556,11 @@ struct sdw_slave_ops { * Slave device * @initialization_complete: completion utility to control potential races * on startup between device enumeration and settings being restored + * @unattach_request: mask field to keep track why the Slave re-attached and + * was re-initialized. This is useful to deal with potential race conditions + * between the Master suspending and the codec resuming, and make sure that + * when the Master triggered a reset the Slave is properly enumerated and + * initialized */ struct sdw_slave { struct sdw_slave_id id; @@ -574,6 +579,7 @@ struct sdw_slave { struct completion probe_complete; struct completion enumeration_complete; struct completion initialization_complete; + u32 unattach_request; };
#define dev_to_sdw_dev(_dev) container_of(_dev, struct sdw_slave, dev)
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 --- drivers/soundwire/intel.h | 9 ++-- drivers/soundwire/intel_init.c | 30 +++-------- include/linux/soundwire/sdw_intel.h | 77 +++++++++++++++++++++++++++-- 3 files changed, 85 insertions(+), 31 deletions(-)
diff --git a/drivers/soundwire/intel.h b/drivers/soundwire/intel.h index d923b6262330..e4cc1d3804ff 100644 --- a/drivers/soundwire/intel.h +++ b/drivers/soundwire/intel.h @@ -5,17 +5,20 @@ #define __SDW_INTEL_LOCAL_H
/** - * struct sdw_intel_link_res - Soundwire link resources + * struct sdw_intel_link_res - Soundwire Intel link resource structure, + * typically populated by the controller driver. + * @pdev: platform_device + * @mmio_base: mmio base of SoundWire registers * @registers: Link IO registers base * @shim: Audio shim pointer * @alh: ALH (Audio Link Hub) pointer * @irq: Interrupt line * @ops: Shim callback ops * @arg: Shim callback ops argument - * - * This is set as pdata for each link instance. */ struct sdw_intel_link_res { + struct platform_device *pdev; + void __iomem *mmio_base; /* not strictly needed, useful for debug */ void __iomem *registers; void __iomem *shim; void __iomem *alh; diff --git a/drivers/soundwire/intel_init.c b/drivers/soundwire/intel_init.c index 2a2b4d8df462..f8ae199094d3 100644 --- a/drivers/soundwire/intel_init.c +++ b/drivers/soundwire/intel_init.c @@ -27,19 +27,9 @@ static int link_mask; module_param_named(sdw_link_mask, link_mask, int, 0444); MODULE_PARM_DESC(sdw_link_mask, "Intel link mask (one bit per link)");
-struct sdw_link_data { - struct sdw_intel_link_res res; - struct platform_device *pdev; -}; - -struct sdw_intel_ctx { - int count; - struct sdw_link_data *links; -}; - static int sdw_intel_cleanup_pdev(struct sdw_intel_ctx *ctx) { - struct sdw_link_data *link = ctx->links; + struct sdw_intel_link_res *link = ctx->links; int i;
if (!link) @@ -62,7 +52,7 @@ static struct sdw_intel_ctx { struct platform_device_info pdevinfo; struct platform_device *pdev; - struct sdw_link_data *link; + struct sdw_intel_link_res *link; struct sdw_intel_ctx *ctx; struct acpi_device *adev; int ret, i; @@ -123,14 +113,12 @@ static struct sdw_intel_ctx continue; }
- link->res.irq = res->irq; - link->res.registers = res->mmio_base + SDW_LINK_BASE + link->registers = res->mmio_base + SDW_LINK_BASE + (SDW_LINK_SIZE * i); - link->res.shim = res->mmio_base + SDW_SHIM_BASE; - link->res.alh = res->mmio_base + SDW_ALH_BASE; + link->shim = res->mmio_base + SDW_SHIM_BASE; + link->alh = res->mmio_base + SDW_ALH_BASE;
- link->res.ops = res->ops; - link->res.arg = res->arg; + link->ops = res->ops;
memset(&pdevinfo, 0, sizeof(pdevinfo));
@@ -138,8 +126,6 @@ static struct sdw_intel_ctx pdevinfo.name = "int-sdw"; pdevinfo.id = i; pdevinfo.fwnode = acpi_fwnode_handle(adev); - pdevinfo.data = &link->res; - pdevinfo.size_data = sizeof(link->res);
pdev = platform_device_register_full(&pdevinfo); if (IS_ERR(pdev)) { @@ -224,10 +210,8 @@ EXPORT_SYMBOL(sdw_intel_init); * * Delete the controller instances created and cleanup */ -void sdw_intel_exit(void *arg) +void sdw_intel_exit(struct sdw_intel_ctx *ctx) { - struct sdw_intel_ctx *ctx = arg; - sdw_intel_cleanup_pdev(ctx); kfree(ctx); } diff --git a/include/linux/soundwire/sdw_intel.h b/include/linux/soundwire/sdw_intel.h index c9427cb6020b..034eca8df748 100644 --- a/include/linux/soundwire/sdw_intel.h +++ b/include/linux/soundwire/sdw_intel.h @@ -16,24 +16,91 @@ struct sdw_intel_ops { };
/** - * struct sdw_intel_res - Soundwire Intel resource structure + * struct sdw_intel_acpi_info - Soundwire Intel information found in ACPI tables + * @handle: ACPI controller handle + * @count: link count found with "sdw-master-count" property + * @link_mask: bit-wise mask listing links enabled by BIOS menu + * + * this structure could be expanded to e.g. provide all the _ADR + * information in case the link_mask is not sufficient to identify + * platform capabilities. + */ +struct sdw_intel_acpi_info { + acpi_handle handle; + int count; + u32 link_mask; +}; + +struct sdw_intel_link_res; + +/** + * struct sdw_intel_ctx - context allocated by the controller + * driver probe + * @count: link count + * @mmio_base: mmio base of SoundWire registers, only used to check + * hardware capabilities after all power dependencies are settled. + * @link_mask: bit-wise mask listing SoundWire links reported by the + * Controller + * @handle: ACPI parent handle + * @links: information for each link (controller-specific and kept + * opaque here) + */ +struct sdw_intel_ctx { + int count; + void __iomem *mmio_base; + u32 link_mask; + acpi_handle handle; + struct sdw_intel_link_res *links; +}; + +/** + * struct sdw_intel_res - Soundwire Intel global resource structure, + * typically populated by the DSP driver + * + * @count: link count * @mmio_base: mmio base of SoundWire registers * @irq: interrupt number * @handle: ACPI parent handle * @parent: parent device * @ops: callback ops - * @arg: callback arg + * @dev: device implementing hwparams and free callbacks + * @link_mask: bit-wise mask listing links selected by the DSP driver + * This mask may be a subset of the one reported by the controller since + * machine-specific quirks are handled in the DSP driver. */ struct sdw_intel_res { + int count; void __iomem *mmio_base; int irq; acpi_handle handle; struct device *parent; const struct sdw_intel_ops *ops; - void *arg; + struct device *dev; + u32 link_mask; };
-void *sdw_intel_init(acpi_handle *parent_handle, struct sdw_intel_res *res); -void sdw_intel_exit(void *arg); +/* + * On Intel platforms, the SoundWire IP has dependencies on power + * rails shared with the DSP, and the initialization steps are split + * in three. First an ACPI scan to check what the firmware describes + * in DSDT tables, then an allocation step (with no hardware + * configuration but with all the relevant devices created) and last + * the actual hardware configuration. The final stage is a global + * interrupt enable which is controlled by the DSP driver. Splitting + * these phases helps simplify the boot flow and make early decisions + * on e.g. which machine driver to select (I2S mode, HDaudio or + * SoundWire). + */ +int sdw_intel_acpi_scan(acpi_handle *parent_handle, + struct sdw_intel_acpi_info *info); + +struct sdw_intel_ctx * +sdw_intel_probe(struct sdw_intel_res *res); + +int sdw_intel_startup(struct sdw_intel_ctx *ctx); + +void sdw_intel_exit(struct sdw_intel_ctx *ctx); + +void sdw_intel_enable_irq(void __iomem *mmio_base, bool enable);
#endif
On 09-12-19, 17:55, Pierre-Louis Bossart wrote:
@@ -138,8 +126,6 @@ static struct sdw_intel_ctx pdevinfo.name = "int-sdw"; pdevinfo.id = i; pdevinfo.fwnode = acpi_fwnode_handle(adev);
pdevinfo.data = &link->res;pdevinfo.size_data = sizeof(link->res);pdev = platform_device_register_full(&pdevinfo); if (IS_ERR(pdev)) {
@@ -224,10 +210,8 @@ EXPORT_SYMBOL(sdw_intel_init);
This is still exported
struct sdw_intel_res {
- int count; void __iomem *mmio_base; int irq; acpi_handle handle; struct device *parent; const struct sdw_intel_ops *ops;
- void *arg;
- struct device *dev;
- u32 link_mask;
};
-void *sdw_intel_init(acpi_handle *parent_handle, struct sdw_intel_res *res);
But prototype removed, so i think this is a miss in the series, can you fix that up
On 12/11/19 5:42 AM, Vinod Koul wrote:
On 09-12-19, 17:55, Pierre-Louis Bossart wrote:
@@ -138,8 +126,6 @@ static struct sdw_intel_ctx pdevinfo.name = "int-sdw"; pdevinfo.id = i; pdevinfo.fwnode = acpi_fwnode_handle(adev);
pdevinfo.data = &link->res;pdevinfo.size_data = sizeof(link->res);pdev = platform_device_register_full(&pdevinfo); if (IS_ERR(pdev)) {
@@ -224,10 +210,8 @@ EXPORT_SYMBOL(sdw_intel_init);
This is still exported
struct sdw_intel_res {
- int count; void __iomem *mmio_base; int irq; acpi_handle handle; struct device *parent; const struct sdw_intel_ops *ops;
- void *arg;
- struct device *dev;
- u32 link_mask; };
-void *sdw_intel_init(acpi_handle *parent_handle, struct sdw_intel_res *res);
But prototype removed, so i think this is a miss in the series, can you fix that up
This is updated in a later patch that implements the new interfaces
-EXPORT_SYMBOL(sdw_intel_init); +EXPORT_SYMBOL(sdw_intel_acpi_scan);
I will fix this but since there is no user for that function there will be no impact.
From: Rander Wang rander.wang@linux.intel.com
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 --- drivers/soundwire/intel.c | 20 ++++++++++++------ drivers/soundwire/intel.h | 4 ++-- drivers/soundwire/intel_init.c | 1 + include/linux/soundwire/sdw_intel.h | 32 +++++++++++++++++++++++++---- 4 files changed, 45 insertions(+), 12 deletions(-)
diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c index 99dc61021211..0371d3d5501a 100644 --- a/drivers/soundwire/intel.c +++ b/drivers/soundwire/intel.c @@ -529,17 +529,24 @@ intel_pdi_alh_configure(struct sdw_intel *sdw, struct sdw_cdns_pdi *pdi) intel_writel(alh, SDW_ALH_STRMZCFG(pdi->intel_alh_id), conf); }
-static int intel_config_stream(struct sdw_intel *sdw, +static int intel_params_stream(struct sdw_intel *sdw, struct snd_pcm_substream *substream, struct snd_soc_dai *dai, - struct snd_pcm_hw_params *hw_params, int link_id) + struct snd_pcm_hw_params *hw_params, + int link_id, int alh_stream_id) { struct sdw_intel_link_res *res = sdw->res; + struct sdw_intel_stream_params_data params_data;
- if (res->ops && res->ops->config_stream && res->arg) - return res->ops->config_stream(res->arg, - substream, dai, hw_params, link_id); + params_data.substream = substream; + params_data.dai = dai; + params_data.hw_params = hw_params; + params_data.link_id = link_id; + params_data.alh_stream_id = alh_stream_id;
+ if (res->ops && res->ops->params_stream && res->dev) + return res->ops->params_stream(res->dev, + ¶ms_data); return -EIO; }
@@ -654,7 +661,8 @@ static int intel_hw_params(struct snd_pcm_substream *substream,
/* Inform DSP about PDI stream number */ - ret = intel_config_stream(sdw, substream, dai, params, + ret = intel_params_stream(sdw, substream, dai, params, + sdw->instance, pdi->intel_alh_id); if (ret) goto error; diff --git a/drivers/soundwire/intel.h b/drivers/soundwire/intel.h index e4cc1d3804ff..38b7c125fb10 100644 --- a/drivers/soundwire/intel.h +++ b/drivers/soundwire/intel.h @@ -14,7 +14,7 @@ * @alh: ALH (Audio Link Hub) pointer * @irq: Interrupt line * @ops: Shim callback ops - * @arg: Shim callback ops argument + * @dev: device implementing hw_params and free callbacks */ struct sdw_intel_link_res { struct platform_device *pdev; @@ -24,7 +24,7 @@ struct sdw_intel_link_res { void __iomem *alh; int irq; const struct sdw_intel_ops *ops; - void *arg; + struct device *dev; };
#endif /* __SDW_INTEL_LOCAL_H */ diff --git a/drivers/soundwire/intel_init.c b/drivers/soundwire/intel_init.c index f8ae199094d3..6bc167c83b47 100644 --- a/drivers/soundwire/intel_init.c +++ b/drivers/soundwire/intel_init.c @@ -119,6 +119,7 @@ static struct sdw_intel_ctx link->alh = res->mmio_base + SDW_ALH_BASE;
link->ops = res->ops; + link->dev = res->dev;
memset(&pdevinfo, 0, sizeof(pdevinfo));
diff --git a/include/linux/soundwire/sdw_intel.h b/include/linux/soundwire/sdw_intel.h index 034eca8df748..3ccb38d48eef 100644 --- a/include/linux/soundwire/sdw_intel.h +++ b/include/linux/soundwire/sdw_intel.h @@ -4,15 +4,39 @@ #ifndef __SDW_INTEL_H #define __SDW_INTEL_H
+/** + * struct sdw_intel_stream_params_data: configuration passed during + * the @params_stream callback, e.g. for interaction with DSP + * firmware. + */ +struct sdw_intel_stream_params_data { + struct snd_pcm_substream *substream; + struct snd_soc_dai *dai; + struct snd_pcm_hw_params *hw_params; + int link_id; + int alh_stream_id; +}; + +/** + * struct sdw_intel_stream_free_data: configuration passed during + * the @free_stream callback, e.g. for interaction with DSP + * firmware. + */ +struct sdw_intel_stream_free_data { + struct snd_pcm_substream *substream; + struct snd_soc_dai *dai; + int link_id; +}; + /** * struct sdw_intel_ops: Intel audio driver callback ops * - * @config_stream: configure the stream with the hw_params - * the first argument containing the context is mandatory */ struct sdw_intel_ops { - int (*config_stream)(void *arg, void *substream, - void *dai, void *hw_params, int stream_num); + int (*params_stream)(struct device *dev, + struct sdw_intel_stream_params_data *params_data); + int (*free_stream)(struct device *dev, + struct sdw_intel_stream_free_data *free_data); };
/**
From: Bard Liao yung-chuan.liao@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 --- include/linux/soundwire/sdw_intel.h | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/include/linux/soundwire/sdw_intel.h b/include/linux/soundwire/sdw_intel.h index 3ccb38d48eef..47b7678ca2ab 100644 --- a/include/linux/soundwire/sdw_intel.h +++ b/include/linux/soundwire/sdw_intel.h @@ -127,4 +127,6 @@ void sdw_intel_exit(struct sdw_intel_ctx *ctx);
void sdw_intel_enable_irq(void __iomem *mmio_base, bool enable);
+irqreturn_t sdw_intel_thread(int irq, void *dev_id); + #endif
From: Bard Liao yung-chuan.liao@linux.intel.com
In MSI mode, the use of separate handlers and threads for the Intel IPC, stream and SoundWire shared interrupt leads to timeouts and lost interrupts.
The solution is to merge all interrupt handling across all links with a single thread function. The use of a linked list enables this thread function to walk through all contexts and figure out which link needs attention.
Signed-off-by: Bard Liao yung-chuan.liao@linux.intel.com Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- include/linux/soundwire/sdw_intel.h | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/include/linux/soundwire/sdw_intel.h b/include/linux/soundwire/sdw_intel.h index 47b7678ca2ab..2cf92724864c 100644 --- a/include/linux/soundwire/sdw_intel.h +++ b/include/linux/soundwire/sdw_intel.h @@ -68,6 +68,7 @@ struct sdw_intel_link_res; * @handle: ACPI parent handle * @links: information for each link (controller-specific and kept * opaque here) + * @link_list: list to handle interrupts across all links */ struct sdw_intel_ctx { int count; @@ -75,6 +76,7 @@ struct sdw_intel_ctx { u32 link_mask; acpi_handle handle; struct sdw_intel_link_res *links; + struct list_head link_list; };
/**
From: Rander Wang rander.wang@intel.com
In ClockStop mode, the PCI device will be notified of a wake, which will be handled from an interrupt thread.
Signed-off-by: Rander Wang rander.wang@intel.com Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- include/linux/soundwire/sdw_intel.h | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/include/linux/soundwire/sdw_intel.h b/include/linux/soundwire/sdw_intel.h index 2cf92724864c..f9937072dc53 100644 --- a/include/linux/soundwire/sdw_intel.h +++ b/include/linux/soundwire/sdw_intel.h @@ -120,6 +120,8 @@ struct sdw_intel_res { int sdw_intel_acpi_scan(acpi_handle *parent_handle, struct sdw_intel_acpi_info *info);
+void sdw_intel_process_wakeen_event(struct sdw_intel_ctx *ctx); + struct sdw_intel_ctx * sdw_intel_probe(struct sdw_intel_res *res);
Some of the Intel SoundWire SHIM registers contain fields for different links. Without protection, the master drivers for the different links will access these shared registers, leading to invalid configurations and timeouts (specifically when changing CPA/SPA power-related registers and polling for the changes to be applied).
A mutex is added to make sure all rmw access to those registers are serialized.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- include/linux/soundwire/sdw_intel.h | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/include/linux/soundwire/sdw_intel.h b/include/linux/soundwire/sdw_intel.h index f9937072dc53..4336bc8e3d83 100644 --- a/include/linux/soundwire/sdw_intel.h +++ b/include/linux/soundwire/sdw_intel.h @@ -69,6 +69,7 @@ struct sdw_intel_link_res; * @links: information for each link (controller-specific and kept * opaque here) * @link_list: list to handle interrupts across all links + * @shim_lock: mutex to handle concurrent rmw access to shared SHIM registers. */ struct sdw_intel_ctx { int count; @@ -77,6 +78,7 @@ struct sdw_intel_ctx { acpi_handle handle; struct sdw_intel_link_res *links; struct list_head link_list; + struct mutex shim_lock; /* lock for access to shared SHIM registers */ };
/**
Due to power rail dependencies, the SoundWire Master driver cannot make decisions on its own when entering pm runtime suspend.
Add quirk mask for each link, so that the SOF parent driver can inform the SoundWire master driver of the desired behavior: a) leave clock on b) power-off instead of clock stop c) power-off if all devices cannot generate wakes d) force bus reset on clock restart
Note that for now the interface with the SOF driver relies on a single mask for all links. If needed, the interface might be modified at a later point to provide more freedom. The code at the lower level does not assume any commonality between links.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- include/linux/soundwire/sdw_intel.h | 37 +++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+)
diff --git a/include/linux/soundwire/sdw_intel.h b/include/linux/soundwire/sdw_intel.h index 4336bc8e3d83..9976db392faf 100644 --- a/include/linux/soundwire/sdw_intel.h +++ b/include/linux/soundwire/sdw_intel.h @@ -57,6 +57,40 @@ struct sdw_intel_acpi_info {
struct sdw_intel_link_res;
+/* Intel clock-stop/pm_runtime quirk definitions */ + +/* + * Force the clock to remain on during pm_runtime suspend. This might + * be needed if Slave devices do not have an alternate clock source or + * if the latency requirements are very strict. + */ +#define SDW_INTEL_CLK_STOP_NOT_ALLOWED BIT(0) + +/* + * Stop the bus during pm_runtime suspend. If set, a complete bus + * reset and re-enumeration will be performed when the bus + * restarts. This mode shall not be used if Slave devices can generate + * in-band wakes. + */ +#define SDW_INTEL_CLK_STOP_TEARDOWN BIT(1) + +/* + * Stop the bus during pm_suspend if Slaves are not wake capable + * (e.g. speaker amplifiers). The clock-stop mode is typically + * slightly higher power than when the IP is completely powered-off. + */ +#define SDW_INTEL_CLK_STOP_WAKE_CAPABLE_ONLY BIT(2) + +/* + * Require a bus reset (and complete re-enumeration) when exiting + * clock stop modes. This may be needed if the controller power was + * turned off and all context lost. This quirk shall not be used if a + * Slave device needs to remain enumerated and keep its context, + * e.g. to provide the reasons for the wake, report acoustic events or + * pass a history buffer. + */ +#define SDW_INTEL_CLK_STOP_BUS_RESET BIT(3) + /** * struct sdw_intel_ctx - context allocated by the controller * driver probe @@ -95,6 +129,8 @@ struct sdw_intel_ctx { * @link_mask: bit-wise mask listing links selected by the DSP driver * This mask may be a subset of the one reported by the controller since * machine-specific quirks are handled in the DSP driver. + * @clock_stop_quirks: mask array of possible behaviors requested by the + * DSP driver. The quirks are common for all links for now. */ struct sdw_intel_res { int count; @@ -105,6 +141,7 @@ struct sdw_intel_res { const struct sdw_intel_ops *ops; struct device *dev; u32 link_mask; + u32 clock_stop_quirks; };
/*
participants (2)
-
Pierre-Louis Bossart -
Vinod Koul