[PATCH 1/3] soundwire: qcom: drop unneeded DAI .set_stream callback

[PATCH 1/3] soundwire: qcom: drop unneeded DAI .set_stream callback

[Krzysztof Kozlowski]

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,
 };
 
-- 
2.34.1

[PATCH 2/3] soundwire: qcom: set owner device of runtime stream

[Krzysztof Kozlowski]

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);
-- 
2.34.1

Re: [PATCH 2/3] soundwire: qcom: set owner device of runtime stream

[Pierre-Louis Bossart]

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);

[PATCH 3/3] ASoC: codecs: wsa884x: check if set_stream is called for proper bus

[Krzysztof Kozlowski]

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;
 
-- 
2.34.1

Re: [PATCH 3/3] ASoC: codecs: wsa884x: check if set_stream is called for proper bus

[Krzysztof Kozlowski]

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

Re: [PATCH 3/3] ASoC: codecs: wsa884x: check if set_stream is called for proper bus

[Pierre-Louis Bossart]

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;
>  

Re: [PATCH 3/3] ASoC: codecs: wsa884x: check if set_stream is called for proper bus

[Krzysztof Kozlowski]

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

Re: [PATCH 3/3] ASoC: codecs: wsa884x: check if set_stream is called for proper bus

[Pierre-Louis Bossart]

> 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.

Re: [PATCH 1/3] soundwire: qcom: drop unneeded DAI .set_stream callback

[Pierre-Louis Bossart]

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,
>  };
>  

Re: [PATCH 1/3] soundwire: qcom: drop unneeded DAI .set_stream callback

[Krzysztof Kozlowski]

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/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts#n1023

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

Re: [PATCH 1/3] soundwire: qcom: drop unneeded DAI .set_stream callback

[Pierre-Louis Bossart]

>>> 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/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts#n1023
> 
> 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.