[PATCH 1/3] soundwire: qcom: drop unneeded DAI .set_stream callback
Qualcomm Soundwire controller drivers do not support multi-link setups, so DAI .set_stream() callback will not be used. What's more, if called it will overwrite the sdw_stream_runtime runtime set in DAI .startup (qcom_swrm_startup()) causing issues (unsupported multi-link error) when two Soundwire controllers are passed as codec DAIs.
Signed-off-by: Krzysztof Kozlowski krzysztof.kozlowski@linaro.org --- drivers/soundwire/qcom.c | 11 ----------- 1 file changed, 11 deletions(-)
diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c index f1b8d6ac5140..fe65c26c5281 100644 --- a/drivers/soundwire/qcom.c +++ b/drivers/soundwire/qcom.c @@ -1267,16 +1267,6 @@ static int qcom_swrm_hw_free(struct snd_pcm_substream *substream, return 0; }
-static int qcom_swrm_set_sdw_stream(struct snd_soc_dai *dai, - void *stream, int direction) -{ - struct qcom_swrm_ctrl *ctrl = dev_get_drvdata(dai->dev); - - ctrl->sruntime[dai->id] = stream; - - return 0; -} - static void *qcom_swrm_get_sdw_stream(struct snd_soc_dai *dai, int direction) { struct qcom_swrm_ctrl *ctrl = dev_get_drvdata(dai->dev); @@ -1349,7 +1339,6 @@ static const struct snd_soc_dai_ops qcom_swrm_pdm_dai_ops = { .hw_free = qcom_swrm_hw_free, .startup = qcom_swrm_startup, .shutdown = qcom_swrm_shutdown, - .set_stream = qcom_swrm_set_sdw_stream, .get_stream = qcom_swrm_get_sdw_stream, };
From: Srinivas Kandagatla srinivas.kandagatla@linaro.org
Store the pointer to struct device of Soundwire controller owning this runtime stream. This can be later used by Soundwire devices, to check if their DAI prepare callback is called for the same bus, in cases where multiple Soundwire buses are used in same soundcard codec list.
Signed-off-by: Srinivas Kandagatla srinivas.kandagatla@linaro.org Co-developed-by: Krzysztof Kozlowski krzysztof.kozlowski@linaro.org Signed-off-by: Krzysztof Kozlowski krzysztof.kozlowski@linaro.org --- drivers/soundwire/qcom.c | 1 + include/linux/soundwire/sdw.h | 2 ++ 2 files changed, 3 insertions(+)
diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c index fe65c26c5281..a95f39563b47 100644 --- a/drivers/soundwire/qcom.c +++ b/drivers/soundwire/qcom.c @@ -1298,6 +1298,7 @@ static int qcom_swrm_startup(struct snd_pcm_substream *substream, goto err_alloc; }
+ sruntime->dev = ctrl->bus.dev; ctrl->sruntime[dai->id] = sruntime;
for_each_rtd_codec_dais(rtd, i, codec_dai) { diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h index 4f3d14bb1538..650334adc261 100644 --- a/include/linux/soundwire/sdw.h +++ b/include/linux/soundwire/sdw.h @@ -1023,6 +1023,7 @@ struct sdw_stream_params { * master_list can contain only one m_rt per Master instance * for a stream * @m_rt_count: Count of Master runtime(s) in this stream + * @dev: SoundWire controller owning this runtime stream */ struct sdw_stream_runtime { const char *name; @@ -1031,6 +1032,7 @@ struct sdw_stream_runtime { enum sdw_stream_type type; struct list_head master_list; int m_rt_count; + struct device *dev; };
struct sdw_stream_runtime *sdw_alloc_stream(const char *stream_name);
On 10/25/23 09:46, Krzysztof Kozlowski wrote:
From: Srinivas Kandagatla srinivas.kandagatla@linaro.org
Store the pointer to struct device of Soundwire controller owning this runtime stream. This can be later used by Soundwire devices, to check if their DAI prepare callback is called for the same bus, in cases where multiple Soundwire buses are used in same soundcard codec list.
Signed-off-by: Srinivas Kandagatla srinivas.kandagatla@linaro.org Co-developed-by: Krzysztof Kozlowski krzysztof.kozlowski@linaro.org Signed-off-by: Krzysztof Kozlowski krzysztof.kozlowski@linaro.org
drivers/soundwire/qcom.c | 1 + include/linux/soundwire/sdw.h | 2 ++ 2 files changed, 3 insertions(+)
diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c index fe65c26c5281..a95f39563b47 100644 --- a/drivers/soundwire/qcom.c +++ b/drivers/soundwire/qcom.c @@ -1298,6 +1298,7 @@ static int qcom_swrm_startup(struct snd_pcm_substream *substream, goto err_alloc; }
sruntime->dev = ctrl->bus.dev; ctrl->sruntime[dai->id] = sruntime;
for_each_rtd_codec_dais(rtd, i, codec_dai) {
diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h index 4f3d14bb1538..650334adc261 100644 --- a/include/linux/soundwire/sdw.h +++ b/include/linux/soundwire/sdw.h @@ -1023,6 +1023,7 @@ struct sdw_stream_params {
- master_list can contain only one m_rt per Master instance
- for a stream
- @m_rt_count: Count of Master runtime(s) in this stream
- @dev: SoundWire controller owning this runtime stream
A stream connects multiple managers and multiple peripherals. The definition above does not make a lot of sense and doesn't work in general since there's no 'owner' really.
And nothing prevents the use of multiple controllers, there are not restrictions in the MIPI DisCo spec that prevent a stream from relying on different controllers.
*/ struct sdw_stream_runtime { const char *name; @@ -1031,6 +1032,7 @@ struct sdw_stream_runtime { enum sdw_stream_type type; struct list_head master_list; int m_rt_count;
- struct device *dev;
};
struct sdw_stream_runtime *sdw_alloc_stream(const char *stream_name);
From: Srinivas Kandagatla srinivas.kandagatla@linaro.org
If multiple WSA8840 speakers, from two separate Soundwire buses, are used in one codec DAI link, the set_stream() should ignore calls for setting stream from other Soundwire controller.
Signed-off-by: Srinivas Kandagatla srinivas.kandagatla@linaro.org Co-developed-by: Krzysztof Kozlowski krzysztof.kozlowski@linaro.org Signed-off-by: Krzysztof Kozlowski krzysztof.kozlowski@linaro.org --- sound/soc/codecs/wsa884x.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/sound/soc/codecs/wsa884x.c b/sound/soc/codecs/wsa884x.c index bee6e763c700..91205e8c96f1 100644 --- a/sound/soc/codecs/wsa884x.c +++ b/sound/soc/codecs/wsa884x.c @@ -1775,6 +1775,12 @@ static int wsa884x_set_stream(struct snd_soc_dai *dai, void *stream, int direction) { struct wsa884x_priv *wsa884x = dev_get_drvdata(dai->dev); + struct sdw_stream_runtime *sruntime = stream; + struct sdw_slave *sdw = dev_to_sdw_dev(dai->dev); + + /* Check if this belongs to same bus */ + if (sdw->bus->dev != sruntime->dev) + return 0;
wsa884x->sruntime = stream;
On 25/10/2023 16:46, Krzysztof Kozlowski wrote:
From: Srinivas Kandagatla srinivas.kandagatla@linaro.org
If multiple WSA8840 speakers, from two separate Soundwire buses, are used in one codec DAI link, the set_stream() should ignore calls for setting stream from other Soundwire controller.
Signed-off-by: Srinivas Kandagatla srinivas.kandagatla@linaro.org Co-developed-by: Krzysztof Kozlowski krzysztof.kozlowski@linaro.org Signed-off-by: Krzysztof Kozlowski krzysztof.kozlowski@linaro.org
I should have add a cover letter and explain that this patch depends on previous (2/3).
Best regards, Krzysztof
On 10/25/23 09:46, Krzysztof Kozlowski wrote:
From: Srinivas Kandagatla srinivas.kandagatla@linaro.org
If multiple WSA8840 speakers, from two separate Soundwire buses, are used in one codec DAI link, the set_stream() should ignore calls for setting stream from other Soundwire controller.
Signed-off-by: Srinivas Kandagatla srinivas.kandagatla@linaro.org Co-developed-by: Krzysztof Kozlowski krzysztof.kozlowski@linaro.org Signed-off-by: Krzysztof Kozlowski krzysztof.kozlowski@linaro.org
sound/soc/codecs/wsa884x.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/sound/soc/codecs/wsa884x.c b/sound/soc/codecs/wsa884x.c index bee6e763c700..91205e8c96f1 100644 --- a/sound/soc/codecs/wsa884x.c +++ b/sound/soc/codecs/wsa884x.c @@ -1775,6 +1775,12 @@ static int wsa884x_set_stream(struct snd_soc_dai *dai, void *stream, int direction) { struct wsa884x_priv *wsa884x = dev_get_drvdata(dai->dev);
- struct sdw_stream_runtime *sruntime = stream;
- struct sdw_slave *sdw = dev_to_sdw_dev(dai->dev);
- /* Check if this belongs to same bus */
- if (sdw->bus->dev != sruntime->dev)
return 0;
Sorry, maybe I am really thick or need coffee, but I can't figure out why this is necessary. Each amplifier has its own "wsa884x_priv" context and should have its own DAI, not following why the set_stream would mix-up the two dais?
We've been using two buses for two amplifiers since CometLake (2019?) and I don't see what's different?
wsa884x->sruntime = stream;
On 25/10/2023 17:03, Pierre-Louis Bossart wrote:
On 10/25/23 09:46, Krzysztof Kozlowski wrote:
From: Srinivas Kandagatla srinivas.kandagatla@linaro.org
If multiple WSA8840 speakers, from two separate Soundwire buses, are used in one codec DAI link, the set_stream() should ignore calls for setting stream from other Soundwire controller.
Signed-off-by: Srinivas Kandagatla srinivas.kandagatla@linaro.org Co-developed-by: Krzysztof Kozlowski krzysztof.kozlowski@linaro.org Signed-off-by: Krzysztof Kozlowski krzysztof.kozlowski@linaro.org
sound/soc/codecs/wsa884x.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/sound/soc/codecs/wsa884x.c b/sound/soc/codecs/wsa884x.c index bee6e763c700..91205e8c96f1 100644 --- a/sound/soc/codecs/wsa884x.c +++ b/sound/soc/codecs/wsa884x.c @@ -1775,6 +1775,12 @@ static int wsa884x_set_stream(struct snd_soc_dai *dai, void *stream, int direction) { struct wsa884x_priv *wsa884x = dev_get_drvdata(dai->dev);
- struct sdw_stream_runtime *sruntime = stream;
- struct sdw_slave *sdw = dev_to_sdw_dev(dai->dev);
- /* Check if this belongs to same bus */
- if (sdw->bus->dev != sruntime->dev)
return 0;Sorry, maybe I am really thick or need coffee, but I can't figure out why this is necessary. Each amplifier has its own "wsa884x_priv" context and should have its own DAI, not following why the set_stream would mix-up the two dais?
We've been using two buses for two amplifiers since CometLake (2019?) and I don't see what's different?
Let me start with some hardware representation in DTS.
We have two Soundwire controllers swr0 and swr3, each with two WSA884x speakers (codecs):
------------- &swr0 { status = "okay";
left_woofer: speaker@0,0 { compatible = "sdw20217020400"; reg = <0 0>; // ... };
/* WSA8845, Left Tweeter */ left_tweeter: speaker@0,1 { compatible = "sdw20217020400"; reg = <0 1>; // ... }; };
&swr3 { status = "okay";
/* WSA8845, Right Woofer */ right_woofer: speaker@0,0 { compatible = "sdw20217020400"; reg = <0 0>; // ... };
/* WSA8845, Right Tweeter */ right_tweeter: speaker@0,1 { compatible = "sdw20217020400"; reg = <0 1>; // ... }; }; -------------
Now, for four-speaker playback, we have sound card with links like:
------------- wsa-dai-link { link-name = "WSA Playback";
cpu { sound-dai = <&q6apmbedai WSA_CODEC_DMA_RX_0>; };
codec { sound-dai = <&left_woofer>, <&left_tweeter>, <&swr0 0>, <&lpass_wsamacro 0>, <&right_woofer>, <&right_tweeter>, <&swr3 0>, <&lpass_wsa2macro 0>; };
platform { sound-dai = <&q6apm>; }; }; -------------
We need to prepare the stream for all four speakers and two soundwire controllers (so two different soundwire buses), however without the patches here, the stream (sdw_stream_runtime *sruntime) right woofer/twitter is set to swr0 (the other bus!). But it should stay as swr3 (their bus).
Does it help a bit? I hope I am able to properly explain it.
Thanks for your feedback and review!
Best regards, Krzysztof
We have two Soundwire controllers swr0 and swr3, each with two WSA884x speakers (codecs):
&swr0 { status = "okay";
left_woofer: speaker@0,0 { compatible = "sdw20217020400"; reg = <0 0>; // ... };
/* WSA8845, Left Tweeter */ left_tweeter: speaker@0,1 { compatible = "sdw20217020400"; reg = <0 1>; // ... }; };
&swr3 { status = "okay";
/* WSA8845, Right Woofer */ right_woofer: speaker@0,0 { compatible = "sdw20217020400"; reg = <0 0>; // ... };
/* WSA8845, Right Tweeter */ right_tweeter: speaker@0,1 { compatible = "sdw20217020400"; reg = <0 1>; // ... }; };
Now, for four-speaker playback, we have sound card with links like:
wsa-dai-link { link-name = "WSA Playback";
cpu { sound-dai = <&q6apmbedai WSA_CODEC_DMA_RX_0>; };
codec { sound-dai = <&left_woofer>, <&left_tweeter>, <&swr0 0>, <&lpass_wsamacro 0>, <&right_woofer>, <&right_tweeter>, <&swr3 0>, <&lpass_wsa2macro 0>; };
platform { sound-dai = <&q6apm>; }; };
We need to prepare the stream for all four speakers and two soundwire controllers (so two different soundwire buses), however without the patches here, the stream (sdw_stream_runtime *sruntime) right woofer/twitter is set to swr0 (the other bus!). But it should stay as swr3 (their bus).
Does it help a bit? I hope I am able to properly explain it.
The configuration seems fine, but the problem is the "sdw_stream_runtime" definition.
You need *ONE* sdw_stream_runtime, and multiple m_rt contexts added in the linked lists of this sdw_stream_runtime. In other words, you need to call sdw_stream_add_master() twice, for swr0 and swr3 respectively.
Put differently, a sdw_stream_runtime does not point to a specific bus, it provides a top-level structure which can use multiple buses.
The best way to allocate the sdw_stream_runtime is to rely on the dailink .startup callback. From the description above that's where you have all the needed information, and then each DAI .startup (or hw_params) can call sdw_stream_add_master() to update the linked lists.
On 10/25/23 09:45, Krzysztof Kozlowski wrote:
Qualcomm Soundwire controller drivers do not support multi-link setups, so DAI .set_stream() callback will not be used. What's more, if called it will overwrite the sdw_stream_runtime runtime set in DAI .startup (qcom_swrm_startup()) causing issues (unsupported multi-link error) when two Soundwire controllers are passed as codec DAIs.
This last sentence is confusing at best.
A controller can have one or more managers, each of whom can have one or more peripherals.
only peripherals should expose codec DAIs, managers should expose CPU DAIs.
Put differently, the controller is the host part while the peripheral is the codec part. "controllers passed as codec DAIs" is not really possible, or this was a typo?
Signed-off-by: Krzysztof Kozlowski krzysztof.kozlowski@linaro.org
drivers/soundwire/qcom.c | 11 ----------- 1 file changed, 11 deletions(-)
diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c index f1b8d6ac5140..fe65c26c5281 100644 --- a/drivers/soundwire/qcom.c +++ b/drivers/soundwire/qcom.c @@ -1267,16 +1267,6 @@ static int qcom_swrm_hw_free(struct snd_pcm_substream *substream, return 0; }
-static int qcom_swrm_set_sdw_stream(struct snd_soc_dai *dai,
void *stream, int direction)-{
- struct qcom_swrm_ctrl *ctrl = dev_get_drvdata(dai->dev);
- ctrl->sruntime[dai->id] = stream;
- return 0;
-}
static void *qcom_swrm_get_sdw_stream(struct snd_soc_dai *dai, int direction) { struct qcom_swrm_ctrl *ctrl = dev_get_drvdata(dai->dev); @@ -1349,7 +1339,6 @@ static const struct snd_soc_dai_ops qcom_swrm_pdm_dai_ops = { .hw_free = qcom_swrm_hw_free, .startup = qcom_swrm_startup, .shutdown = qcom_swrm_shutdown,
- .set_stream = qcom_swrm_set_sdw_stream, .get_stream = qcom_swrm_get_sdw_stream,
};
On 25/10/2023 17:12, Pierre-Louis Bossart wrote:
On 10/25/23 09:45, Krzysztof Kozlowski wrote:
Qualcomm Soundwire controller drivers do not support multi-link setups, so DAI .set_stream() callback will not be used. What's more, if called it will overwrite the sdw_stream_runtime runtime set in DAI .startup (qcom_swrm_startup()) causing issues (unsupported multi-link error) when two Soundwire controllers are passed as codec DAIs.
This last sentence is confusing at best.
A controller can have one or more managers, each of whom can have one or more peripherals.
only peripherals should expose codec DAIs, managers should expose CPU DAIs.
Put differently, the controller is the host part while the peripheral is the codec part. "controllers passed as codec DAIs" is not really possible, or this was a typo?
No, it wasn't a typo. Take a look here: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch...
The <&swr0 0> is the controller, although probably I should call it manager, but in case of Qualcomm I think they are 1-to-1.
Best regards, Krzysztof
Qualcomm Soundwire controller drivers do not support multi-link setups, so DAI .set_stream() callback will not be used. What's more, if called it will overwrite the sdw_stream_runtime runtime set in DAI .startup (qcom_swrm_startup()) causing issues (unsupported multi-link error) when two Soundwire controllers are passed as codec DAIs.
This last sentence is confusing at best.
A controller can have one or more managers, each of whom can have one or more peripherals.
only peripherals should expose codec DAIs, managers should expose CPU DAIs.
Put differently, the controller is the host part while the peripheral is the codec part. "controllers passed as codec DAIs" is not really possible, or this was a typo?
No, it wasn't a typo. Take a look here: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch...
The <&swr0 0> is the controller, although probably I should call it manager, but in case of Qualcomm I think they are 1-to-1.
Is this a case where the SoundWire manager is part of a codec?
In that case, how are the SoundWire peripheral modeled?
The .set_stream callback was really meant to be used when you have a CPU DAI for the manager and a codec DAI for the peripheral(s). This seems to be a different configuration where CPU and codec DAIs are mixed.
participants (2)
-
Krzysztof Kozlowski -
Pierre-Louis Bossart