* [PATCH] ASoC: SOF: amd: fix soundwire dependencies
@ 2024-02-19 9:38 Arnd Bergmann
2024-02-20 5:57 ` Mukunda,Vijendar
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Arnd Bergmann @ 2024-02-19 9:38 UTC (permalink / raw
To: Pierre-Louis Bossart, Liam Girdwood, Peter Ujfalusi, Bard Liao,
Ranjani Sridharan, Daniel Baluta, Mark Brown
Cc: Arnd Bergmann, Kai Vehmanen, Jaroslav Kysela, Takashi Iwai,
Vijendar Mukunda, V sujith kumar Reddy, Venkata Prasad Potturu,
sound-open-firmware, linux-sound, linux-kernel
From: Arnd Bergmann <arnd@arndb.de>
The soundwire-amd driver has a bit of a layering violation requiring
the SOF driver to directly call into its exported symbols rather than
through an abstraction.
The SND_SOC_SOF_AMD_SOUNDWIRE Kconfig symbol tries to deal with the
dependency by selecting SOUNDWIRE_AMD in a complicated set of conditions,
but gets it wrong for a configuration involving SND_SOC_SOF_AMD_COMMON=y,
SND_SOC_SOF_AMD_ACP63=m, and SND_SOC_SOF_AMD_SOUNDWIRE_LINK_BASELINE=m
SOUNDWIRE_AMD=m, which results in a link failure:
ld.lld: error: undefined symbol: sdw_amd_get_slave_info
>>> referenced by acp-common.c
ld.lld: error: undefined symbol: amd_sdw_scan_controller
ld.lld: error: undefined symbol: sdw_amd_probe
ld.lld: error: undefined symbol: sdw_amd_exit
>>> referenced by acp.c
>>> sound/soc/sof/amd/acp.o:(amd_sof_acp_remove) in archive vmlinux.a
In essence, the SND_SOC_SOF_AMD_COMMON option cannot be built-in when
trying to link against a modular SOUNDWIRE_AMD driver.
Since CONFIG_SOUNDWIRE_AMD is a user-visible option, it really should
never be selected by another driver in the first place, so replace the
extra complexity with a normal Kconfig dependency in SND_SOC_SOF_AMD_SOUNDWIRE,
plus a top-level check that forbids any of the AMD SOF drivers from being
built-in with CONFIG_SOUNDWIRE_AMD=m.
In normal configs, they should all either be built-in or all loadable
modules anyway, so this simplification does not limit any real usecases.
Fixes: d948218424bf ("ASoC: SOF: amd: add code for invoking soundwire manager helper functions")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
sound/soc/sof/amd/Kconfig | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/sound/soc/sof/amd/Kconfig b/sound/soc/sof/amd/Kconfig
index c3bbe6c70fb2..2729c6eb3feb 100644
--- a/sound/soc/sof/amd/Kconfig
+++ b/sound/soc/sof/amd/Kconfig
@@ -6,6 +6,7 @@
config SND_SOC_SOF_AMD_TOPLEVEL
tristate "SOF support for AMD audio DSPs"
+ depends on SOUNDWIRE_AMD || !SOUNDWIRE_AMD
depends on X86 || COMPILE_TEST
help
This adds support for Sound Open Firmware for AMD platforms.
@@ -62,15 +63,14 @@ config SND_SOC_SOF_ACP_PROBES
config SND_SOC_SOF_AMD_SOUNDWIRE_LINK_BASELINE
tristate
- select SOUNDWIRE_AMD if SND_SOC_SOF_AMD_SOUNDWIRE != n
select SND_AMD_SOUNDWIRE_ACPI if ACPI
config SND_SOC_SOF_AMD_SOUNDWIRE
tristate "SOF support for SoundWire based AMD platforms"
default SND_SOC_SOF_AMD_SOUNDWIRE_LINK_BASELINE
depends on SND_SOC_SOF_AMD_SOUNDWIRE_LINK_BASELINE
- depends on ACPI && SOUNDWIRE
- depends on !(SOUNDWIRE=m && SND_SOC_SOF_AMD_SOUNDWIRE_LINK_BASELINE=y)
+ depends on ACPI
+ depends on SOUNDWIRE_AMD
help
This adds support for SoundWire with Sound Open Firmware
for AMD platforms.
--
2.39.2
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] ASoC: SOF: amd: fix soundwire dependencies
2024-02-19 9:38 [PATCH] ASoC: SOF: amd: fix soundwire dependencies Arnd Bergmann
@ 2024-02-20 5:57 ` Mukunda,Vijendar
2024-02-20 6:13 ` Arnd Bergmann
2024-02-20 10:19 ` Mukunda,Vijendar
2024-02-21 0:48 ` Mark Brown
2 siblings, 1 reply; 10+ messages in thread
From: Mukunda,Vijendar @ 2024-02-20 5:57 UTC (permalink / raw
To: Arnd Bergmann, Pierre-Louis Bossart, Liam Girdwood,
Peter Ujfalusi, Bard Liao, Ranjani Sridharan, Daniel Baluta,
Mark Brown
Cc: Arnd Bergmann, Kai Vehmanen, Jaroslav Kysela, Takashi Iwai,
V sujith kumar Reddy, Venkata Prasad Potturu, sound-open-firmware,
linux-sound, linux-kernel
On 19/02/24 15:08, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
>
> The soundwire-amd driver has a bit of a layering violation requiring
> the SOF driver to directly call into its exported symbols rather than
> through an abstraction.
>
> The SND_SOC_SOF_AMD_SOUNDWIRE Kconfig symbol tries to deal with the
> dependency by selecting SOUNDWIRE_AMD in a complicated set of conditions,
> but gets it wrong for a configuration involving SND_SOC_SOF_AMD_COMMON=y,
> SND_SOC_SOF_AMD_ACP63=m, and SND_SOC_SOF_AMD_SOUNDWIRE_LINK_BASELINE=m
> SOUNDWIRE_AMD=m, which results in a link failure:
>
> ld.lld: error: undefined symbol: sdw_amd_get_slave_info
>>>> referenced by acp-common.c
> ld.lld: error: undefined symbol: amd_sdw_scan_controller
> ld.lld: error: undefined symbol: sdw_amd_probe
> ld.lld: error: undefined symbol: sdw_amd_exit
>>>> referenced by acp.c
>>>> sound/soc/sof/amd/acp.o:(amd_sof_acp_remove) in archive vmlinux.a
> In essence, the SND_SOC_SOF_AMD_COMMON option cannot be built-in when
> trying to link against a modular SOUNDWIRE_AMD driver.
>
> Since CONFIG_SOUNDWIRE_AMD is a user-visible option, it really should
> never be selected by another driver in the first place, so replace the
> extra complexity with a normal Kconfig dependency in SND_SOC_SOF_AMD_SOUNDWIRE,
> plus a top-level check that forbids any of the AMD SOF drivers from being
> built-in with CONFIG_SOUNDWIRE_AMD=m.
>
> In normal configs, they should all either be built-in or all loadable
> modules anyway, so this simplification does not limit any real usecases.
Tested this patch. SOUNWIRE_AMD flag is not selected by default causing
AMD SOF driver for ACP 6.3 platform is build without enabling SoundWire.
>
> Fixes: d948218424bf ("ASoC: SOF: amd: add code for invoking soundwire manager helper functions")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> sound/soc/sof/amd/Kconfig | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/sound/soc/sof/amd/Kconfig b/sound/soc/sof/amd/Kconfig
> index c3bbe6c70fb2..2729c6eb3feb 100644
> --- a/sound/soc/sof/amd/Kconfig
> +++ b/sound/soc/sof/amd/Kconfig
> @@ -6,6 +6,7 @@
>
> config SND_SOC_SOF_AMD_TOPLEVEL
> tristate "SOF support for AMD audio DSPs"
> + depends on SOUNDWIRE_AMD || !SOUNDWIRE_AMD
> depends on X86 || COMPILE_TEST
> help
> This adds support for Sound Open Firmware for AMD platforms.
> @@ -62,15 +63,14 @@ config SND_SOC_SOF_ACP_PROBES
>
> config SND_SOC_SOF_AMD_SOUNDWIRE_LINK_BASELINE
> tristate
> - select SOUNDWIRE_AMD if SND_SOC_SOF_AMD_SOUNDWIRE != n
> select SND_AMD_SOUNDWIRE_ACPI if ACPI
>
> config SND_SOC_SOF_AMD_SOUNDWIRE
> tristate "SOF support for SoundWire based AMD platforms"
> default SND_SOC_SOF_AMD_SOUNDWIRE_LINK_BASELINE
> depends on SND_SOC_SOF_AMD_SOUNDWIRE_LINK_BASELINE
> - depends on ACPI && SOUNDWIRE
> - depends on !(SOUNDWIRE=m && SND_SOC_SOF_AMD_SOUNDWIRE_LINK_BASELINE=y)
> + depends on ACPI
> + depends on SOUNDWIRE_AMD
> help
> This adds support for SoundWire with Sound Open Firmware
> for AMD platforms.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] ASoC: SOF: amd: fix soundwire dependencies
2024-02-20 5:57 ` Mukunda,Vijendar
@ 2024-02-20 6:13 ` Arnd Bergmann
2024-02-20 6:23 ` Mukunda,Vijendar
0 siblings, 1 reply; 10+ messages in thread
From: Arnd Bergmann @ 2024-02-20 6:13 UTC (permalink / raw
To: Vijendar Mukunda, Arnd Bergmann, Pierre-Louis Bossart,
Liam Girdwood, Peter Ujfalusi, Bard Liao, Ranjani Sridharan,
Daniel Baluta, Mark Brown
Cc: Kai Vehmanen, Jaroslav Kysela, Takashi Iwai, V sujith kumar Reddy,
Venkata Prasad Potturu, sound-open-firmware, linux-sound,
linux-kernel
On Tue, Feb 20, 2024, at 06:57, Mukunda,Vijendar wrote:
> On 19/02/24 15:08, Arnd Bergmann wrote:
>> From: Arnd Bergmann <arnd@arndb.de>
>> In normal configs, they should all either be built-in or all loadable
>> modules anyway, so this simplification does not limit any real usecases.
>
> Tested this patch. SOUNWIRE_AMD flag is not selected by default causing
> AMD SOF driver for ACP 6.3 platform is build without enabling SoundWire.
Yes, that is what I described. But as SOUNWIRE_AMD is a user visible
symbol, there is no problem in expecting users to enable it when they
have this hardware, and distros just enable all the drivers anyway.
Arnd
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] ASoC: SOF: amd: fix soundwire dependencies
2024-02-20 6:13 ` Arnd Bergmann
@ 2024-02-20 6:23 ` Mukunda,Vijendar
2024-02-20 7:10 ` Arnd Bergmann
0 siblings, 1 reply; 10+ messages in thread
From: Mukunda,Vijendar @ 2024-02-20 6:23 UTC (permalink / raw
To: Arnd Bergmann, Arnd Bergmann, Pierre-Louis Bossart, Liam Girdwood,
Peter Ujfalusi, Bard Liao, Ranjani Sridharan, Daniel Baluta,
Mark Brown
Cc: Kai Vehmanen, Jaroslav Kysela, Takashi Iwai, V sujith kumar Reddy,
Venkata Prasad Potturu, sound-open-firmware, linux-sound,
linux-kernel
On 20/02/24 11:43, Arnd Bergmann wrote:
> On Tue, Feb 20, 2024, at 06:57, Mukunda,Vijendar wrote:
>> On 19/02/24 15:08, Arnd Bergmann wrote:
>>> From: Arnd Bergmann <arnd@arndb.de>
>>> In normal configs, they should all either be built-in or all loadable
>>> modules anyway, so this simplification does not limit any real usecases.
>> Tested this patch. SOUNWIRE_AMD flag is not selected by default causing
>> AMD SOF driver for ACP 6.3 platform is build without enabling SoundWire.
> Yes, that is what I described. But as SOUNWIRE_AMD is a user visible
> symbol, there is no problem in expecting users to enable it when they
> have this hardware, and distros just enable all the drivers anyway.
Want to set SOUNDWIRE_AMD flag by default, similar to Intel & Qcom
platforms instead of explicitly enabling the Kconfig option.
>
> Arnd
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] ASoC: SOF: amd: fix soundwire dependencies
2024-02-20 6:23 ` Mukunda,Vijendar
@ 2024-02-20 7:10 ` Arnd Bergmann
2024-02-20 7:54 ` Mukunda,Vijendar
0 siblings, 1 reply; 10+ messages in thread
From: Arnd Bergmann @ 2024-02-20 7:10 UTC (permalink / raw
To: Vijendar Mukunda, Arnd Bergmann, Pierre-Louis Bossart,
Liam Girdwood, Peter Ujfalusi, Bard Liao, Ranjani Sridharan,
Daniel Baluta, Mark Brown
Cc: Kai Vehmanen, Jaroslav Kysela, Takashi Iwai, V sujith kumar Reddy,
Venkata Prasad Potturu, sound-open-firmware, linux-sound,
linux-kernel
On Tue, Feb 20, 2024, at 07:23, Mukunda,Vijendar wrote:
> On 20/02/24 11:43, Arnd Bergmann wrote:
>> On Tue, Feb 20, 2024, at 06:57, Mukunda,Vijendar wrote:
>>> On 19/02/24 15:08, Arnd Bergmann wrote:
>>>> From: Arnd Bergmann <arnd@arndb.de>
>>>> In normal configs, they should all either be built-in or all loadable
>>>> modules anyway, so this simplification does not limit any real usecases.
>>> Tested this patch. SOUNWIRE_AMD flag is not selected by default causing
>>> AMD SOF driver for ACP 6.3 platform is build without enabling SoundWire.
>> Yes, that is what I described. But as SOUNWIRE_AMD is a user visible
>> symbol, there is no problem in expecting users to enable it when they
>> have this hardware, and distros just enable all the drivers anyway.
>
> Want to set SOUNDWIRE_AMD flag by default, similar to Intel & Qcom
> platforms instead of explicitly enabling the Kconfig option.
Maybe use 'default SND_SOC_SOF_AMD_TOPLEVEL' then?
I don't think copying the mistake from the intel driver
is helpful, though I agree it would be nice to be consistent
between them.
As a general rule, you should not have a Kconfig symbol that
is both user visible and also selected by the drivers that
depend on it.
To avoid the dependency problems from coming back and keep
the complexity to a minimum, I think there are two logical
ways to handle soundwire:
a) keep the current drivers/soundwire/Kconfig contents and
change all the 'select SOUNDWIRE_foo' to 'depends on'.
b) keep all the 'select SOUNDWIRE_foo' but drop the prompts,
requiring that all drivers that use soundwire have the
correct select statements, with the main CONFIG_SOUNDWIRE
getting selected by the individual drivers.
Arnd
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] ASoC: SOF: amd: fix soundwire dependencies
2024-02-20 7:10 ` Arnd Bergmann
@ 2024-02-20 7:54 ` Mukunda,Vijendar
2024-02-20 8:21 ` Arnd Bergmann
0 siblings, 1 reply; 10+ messages in thread
From: Mukunda,Vijendar @ 2024-02-20 7:54 UTC (permalink / raw
To: Arnd Bergmann, Arnd Bergmann, Pierre-Louis Bossart, Liam Girdwood,
Peter Ujfalusi, Bard Liao, Ranjani Sridharan, Daniel Baluta,
Mark Brown
Cc: Kai Vehmanen, Jaroslav Kysela, Takashi Iwai,
Venkata Prasad Potturu, sound-open-firmware, linux-sound,
linux-kernel, Dommati, Sunil-kumar
On 20/02/24 12:40, Arnd Bergmann wrote:
> On Tue, Feb 20, 2024, at 07:23, Mukunda,Vijendar wrote:
>> On 20/02/24 11:43, Arnd Bergmann wrote:
>>> On Tue, Feb 20, 2024, at 06:57, Mukunda,Vijendar wrote:
>>>> On 19/02/24 15:08, Arnd Bergmann wrote:
>>>>> From: Arnd Bergmann <arnd@arndb.de>
>>>>> In normal configs, they should all either be built-in or all loadable
>>>>> modules anyway, so this simplification does not limit any real usecases.
>>>> Tested this patch. SOUNWIRE_AMD flag is not selected by default causing
>>>> AMD SOF driver for ACP 6.3 platform is build without enabling SoundWire.
>>> Yes, that is what I described. But as SOUNWIRE_AMD is a user visible
>>> symbol, there is no problem in expecting users to enable it when they
>>> have this hardware, and distros just enable all the drivers anyway.
>> Want to set SOUNDWIRE_AMD flag by default, similar to Intel & Qcom
>> platforms instead of explicitly enabling the Kconfig option.
> Maybe use 'default SND_SOC_SOF_AMD_TOPLEVEL' then?
Didn't get your point.
Even with the current patch, if we select 'SOUNDWIRE_AMD' flag explicitly
AMD ACP63 SOF driver Kconfig option is not visible for user configuration.
This results in ACP63 SOF driver won't be built at all.
>
> I don't think copying the mistake from the intel driver
> is helpful, though I agree it would be nice to be consistent
> between them.
>
> As a general rule, you should not have a Kconfig symbol that
> is both user visible and also selected by the drivers that
> depend on it.
>
> To avoid the dependency problems from coming back and keep
> the complexity to a minimum, I think there are two logical
> ways to handle soundwire:
>
> a) keep the current drivers/soundwire/Kconfig contents and
> change all the 'select SOUNDWIRE_foo' to 'depends on'.
Current patch already using 'depends on SOUNDWIRE_AMD" for
SND_SOC_SOF_AMD_SOUNDWIRE Kconfig option.
Still we couldn't see SND_SOC_SOF_AMD_ACP63 Kconfig option
is enabled.
>
> b) keep all the 'select SOUNDWIRE_foo' but drop the prompts,
> requiring that all drivers that use soundwire have the
> correct select statements, with the main CONFIG_SOUNDWIRE
> getting selected by the individual drivers.
Didn't get your point. Could you please elaborate?
> Arnd
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] ASoC: SOF: amd: fix soundwire dependencies
2024-02-20 7:54 ` Mukunda,Vijendar
@ 2024-02-20 8:21 ` Arnd Bergmann
2024-02-20 10:15 ` Mukunda,Vijendar
0 siblings, 1 reply; 10+ messages in thread
From: Arnd Bergmann @ 2024-02-20 8:21 UTC (permalink / raw
To: Vijendar Mukunda, Arnd Bergmann, Pierre-Louis Bossart,
Liam Girdwood, Peter Ujfalusi, Bard Liao, Ranjani Sridharan,
Daniel Baluta, Mark Brown
Cc: Kai Vehmanen, Jaroslav Kysela, Takashi Iwai,
Venkata Prasad Potturu, sound-open-firmware, linux-sound,
linux-kernel, Dommati, Sunil-kumar
On Tue, Feb 20, 2024, at 08:54, Mukunda,Vijendar wrote:
> On 20/02/24 12:40, Arnd Bergmann wrote:
>> On Tue, Feb 20, 2024, at 07:23, Mukunda,Vijendar wrote:
>>> On 20/02/24 11:43, Arnd Bergmann wrote:
>>>> On Tue, Feb 20, 2024, at 06:57, Mukunda,Vijendar wrote:
>>>>> On 19/02/24 15:08, Arnd Bergmann wrote:
>>>>>> From: Arnd Bergmann <arnd@arndb.de>
>>>>>> In normal configs, they should all either be built-in or all loadable
>>>>>> modules anyway, so this simplification does not limit any real usecases.
>>>>> Tested this patch. SOUNWIRE_AMD flag is not selected by default causing
>>>>> AMD SOF driver for ACP 6.3 platform is build without enabling SoundWire.
>>>> Yes, that is what I described. But as SOUNWIRE_AMD is a user visible
>>>> symbol, there is no problem in expecting users to enable it when they
>>>> have this hardware, and distros just enable all the drivers anyway.
>>> Want to set SOUNDWIRE_AMD flag by default, similar to Intel & Qcom
>>> platforms instead of explicitly enabling the Kconfig option.
>> Maybe use 'default SND_SOC_SOF_AMD_TOPLEVEL' then?
> Didn't get your point.
>
> Even with the current patch, if we select 'SOUNDWIRE_AMD' flag explicitly
> AMD ACP63 SOF driver Kconfig option is not visible for user configuration.
> This results in ACP63 SOF driver won't be built at all.
I don't understand what you mean here. What I see
in linux-next both with and without my patch is
config SND_SOC_SOF_AMD_ACP63
tristate "SOF support for ACP6.3 platform"
depends on SND_SOC_SOF_PCI
so it clearly should be visible as long as SND_SOC_SOF_PCI
is enabled.
There is still a problem that SND_SOC_SOF_AMD_TOPLEVEL
can't use my "depends on SOUNDWIRE_AMD || !SOUNDWIRE_AMD"
trick if SOUNDWIRE_AMD in turn uses
"default SND_SOC_SOF_AMD_TOPLEVEL", but I don't think you
meant that, right?
>> I don't think copying the mistake from the intel driver
>> is helpful, though I agree it would be nice to be consistent
>> between them.
>>
>> As a general rule, you should not have a Kconfig symbol that
>> is both user visible and also selected by the drivers that
>> depend on it.
>>
>> To avoid the dependency problems from coming back and keep
>> the complexity to a minimum, I think there are two logical
>> ways to handle soundwire:
>>
>> a) keep the current drivers/soundwire/Kconfig contents and
>> change all the 'select SOUNDWIRE_foo' to 'depends on'.
>
> Current patch already using 'depends on SOUNDWIRE_AMD" for
> SND_SOC_SOF_AMD_SOUNDWIRE Kconfig option.
Correct, because this is the Kconfig option that actually
controls whether sound/soc/sof/amd/acp-common.c calls into
the soundwire-amd module.
> Still we couldn't see SND_SOC_SOF_AMD_ACP63 Kconfig option
> is enabled.
I need more information here. Do you have additional
patches on top of what is in today's linux-next?
I have it enabled on my build here.
Arnd
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] ASoC: SOF: amd: fix soundwire dependencies
2024-02-20 8:21 ` Arnd Bergmann
@ 2024-02-20 10:15 ` Mukunda,Vijendar
0 siblings, 0 replies; 10+ messages in thread
From: Mukunda,Vijendar @ 2024-02-20 10:15 UTC (permalink / raw
To: Arnd Bergmann, Arnd Bergmann, Pierre-Louis Bossart, Liam Girdwood,
Peter Ujfalusi, Bard Liao, Ranjani Sridharan, Daniel Baluta,
Mark Brown
Cc: Kai Vehmanen, Jaroslav Kysela, Takashi Iwai,
Venkata Prasad Potturu, sound-open-firmware, linux-sound,
linux-kernel, Dommati, Sunil-kumar
On 20/02/24 13:51, Arnd Bergmann wrote:
> On Tue, Feb 20, 2024, at 08:54, Mukunda,Vijendar wrote:
>> On 20/02/24 12:40, Arnd Bergmann wrote:
>>> On Tue, Feb 20, 2024, at 07:23, Mukunda,Vijendar wrote:
>>>> On 20/02/24 11:43, Arnd Bergmann wrote:
>>>>> On Tue, Feb 20, 2024, at 06:57, Mukunda,Vijendar wrote:
>>>>>> On 19/02/24 15:08, Arnd Bergmann wrote:
>>>>>>> From: Arnd Bergmann <arnd@arndb.de>
>>>>>>> In normal configs, they should all either be built-in or all loadable
>>>>>>> modules anyway, so this simplification does not limit any real usecases.
>>>>>> Tested this patch. SOUNWIRE_AMD flag is not selected by default causing
>>>>>> AMD SOF driver for ACP 6.3 platform is build without enabling SoundWire.
>>>>> Yes, that is what I described. But as SOUNWIRE_AMD is a user visible
>>>>> symbol, there is no problem in expecting users to enable it when they
>>>>> have this hardware, and distros just enable all the drivers anyway.
>>>> Want to set SOUNDWIRE_AMD flag by default, similar to Intel & Qcom
>>>> platforms instead of explicitly enabling the Kconfig option.
>>> Maybe use 'default SND_SOC_SOF_AMD_TOPLEVEL' then?
>> Didn't get your point.
>>
>> Even with the current patch, if we select 'SOUNDWIRE_AMD' flag explicitly
>> AMD ACP63 SOF driver Kconfig option is not visible for user configuration.
>> This results in ACP63 SOF driver won't be built at all.
> I don't understand what you mean here. What I see
> in linux-next both with and without my patch is
>
> config SND_SOC_SOF_AMD_ACP63
> tristate "SOF support for ACP6.3 platform"
> depends on SND_SOC_SOF_PCI
>
> so it clearly should be visible as long as SND_SOC_SOF_PCI
> is enabled.
>
> There is still a problem that SND_SOC_SOF_AMD_TOPLEVEL
> can't use my "depends on SOUNDWIRE_AMD || !SOUNDWIRE_AMD"
> trick if SOUNDWIRE_AMD in turn uses
> "default SND_SOC_SOF_AMD_TOPLEVEL", but I don't think you
> meant that, right?
Yes, you are correct.
>
>>> I don't think copying the mistake from the intel driver
>>> is helpful, though I agree it would be nice to be consistent
>>> between them.
>>>
>>> As a general rule, you should not have a Kconfig symbol that
>>> is both user visible and also selected by the drivers that
>>> depend on it.
>>>
>>> To avoid the dependency problems from coming back and keep
>>> the complexity to a minimum, I think there are two logical
>>> ways to handle soundwire:
>>>
>>> a) keep the current drivers/soundwire/Kconfig contents and
>>> change all the 'select SOUNDWIRE_foo' to 'depends on'.
>> Current patch already using 'depends on SOUNDWIRE_AMD" for
>> SND_SOC_SOF_AMD_SOUNDWIRE Kconfig option.
> Correct, because this is the Kconfig option that actually
> controls whether sound/soc/sof/amd/acp-common.c calls into
> the soundwire-amd module.
>
>> Still we couldn't see SND_SOC_SOF_AMD_ACP63 Kconfig option
>> is enabled.
> I need more information here. Do you have additional
> patches on top of what is in today's linux-next?
> I have it enabled on my build here.
Sorry, it's my bad. My local patches created the problem.
Validated patch on our side. It's working fine.
>
> Arnd
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] ASoC: SOF: amd: fix soundwire dependencies
2024-02-19 9:38 [PATCH] ASoC: SOF: amd: fix soundwire dependencies Arnd Bergmann
2024-02-20 5:57 ` Mukunda,Vijendar
@ 2024-02-20 10:19 ` Mukunda,Vijendar
2024-02-21 0:48 ` Mark Brown
2 siblings, 0 replies; 10+ messages in thread
From: Mukunda,Vijendar @ 2024-02-20 10:19 UTC (permalink / raw
To: Arnd Bergmann, Pierre-Louis Bossart, Liam Girdwood,
Peter Ujfalusi, Bard Liao, Ranjani Sridharan, Daniel Baluta,
Mark Brown
Cc: Arnd Bergmann, Kai Vehmanen, Jaroslav Kysela, Takashi Iwai,
V sujith kumar Reddy, Venkata Prasad Potturu, sound-open-firmware,
linux-sound, linux-kernel
On 19/02/24 15:08, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
>
> The soundwire-amd driver has a bit of a layering violation requiring
> the SOF driver to directly call into its exported symbols rather than
> through an abstraction.
>
> The SND_SOC_SOF_AMD_SOUNDWIRE Kconfig symbol tries to deal with the
> dependency by selecting SOUNDWIRE_AMD in a complicated set of conditions,
> but gets it wrong for a configuration involving SND_SOC_SOF_AMD_COMMON=y,
> SND_SOC_SOF_AMD_ACP63=m, and SND_SOC_SOF_AMD_SOUNDWIRE_LINK_BASELINE=m
> SOUNDWIRE_AMD=m, which results in a link failure:
>
> ld.lld: error: undefined symbol: sdw_amd_get_slave_info
>>>> referenced by acp-common.c
> ld.lld: error: undefined symbol: amd_sdw_scan_controller
> ld.lld: error: undefined symbol: sdw_amd_probe
> ld.lld: error: undefined symbol: sdw_amd_exit
>>>> referenced by acp.c
>>>> sound/soc/sof/amd/acp.o:(amd_sof_acp_remove) in archive vmlinux.a
> In essence, the SND_SOC_SOF_AMD_COMMON option cannot be built-in when
> trying to link against a modular SOUNDWIRE_AMD driver.
>
> Since CONFIG_SOUNDWIRE_AMD is a user-visible option, it really should
> never be selected by another driver in the first place, so replace the
> extra complexity with a normal Kconfig dependency in SND_SOC_SOF_AMD_SOUNDWIRE,
> plus a top-level check that forbids any of the AMD SOF drivers from being
> built-in with CONFIG_SOUNDWIRE_AMD=m.
>
> In normal configs, they should all either be built-in or all loadable
> modules anyway, so this simplification does not limit any real usecases.
Tested-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
>
> Fixes: d948218424bf ("ASoC: SOF: amd: add code for invoking soundwire manager helper functions")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> sound/soc/sof/amd/Kconfig | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/sound/soc/sof/amd/Kconfig b/sound/soc/sof/amd/Kconfig
> index c3bbe6c70fb2..2729c6eb3feb 100644
> --- a/sound/soc/sof/amd/Kconfig
> +++ b/sound/soc/sof/amd/Kconfig
> @@ -6,6 +6,7 @@
>
> config SND_SOC_SOF_AMD_TOPLEVEL
> tristate "SOF support for AMD audio DSPs"
> + depends on SOUNDWIRE_AMD || !SOUNDWIRE_AMD
> depends on X86 || COMPILE_TEST
> help
> This adds support for Sound Open Firmware for AMD platforms.
> @@ -62,15 +63,14 @@ config SND_SOC_SOF_ACP_PROBES
>
> config SND_SOC_SOF_AMD_SOUNDWIRE_LINK_BASELINE
> tristate
> - select SOUNDWIRE_AMD if SND_SOC_SOF_AMD_SOUNDWIRE != n
> select SND_AMD_SOUNDWIRE_ACPI if ACPI
>
> config SND_SOC_SOF_AMD_SOUNDWIRE
> tristate "SOF support for SoundWire based AMD platforms"
> default SND_SOC_SOF_AMD_SOUNDWIRE_LINK_BASELINE
> depends on SND_SOC_SOF_AMD_SOUNDWIRE_LINK_BASELINE
> - depends on ACPI && SOUNDWIRE
> - depends on !(SOUNDWIRE=m && SND_SOC_SOF_AMD_SOUNDWIRE_LINK_BASELINE=y)
> + depends on ACPI
> + depends on SOUNDWIRE_AMD
> help
> This adds support for SoundWire with Sound Open Firmware
> for AMD platforms.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] ASoC: SOF: amd: fix soundwire dependencies
2024-02-19 9:38 [PATCH] ASoC: SOF: amd: fix soundwire dependencies Arnd Bergmann
2024-02-20 5:57 ` Mukunda,Vijendar
2024-02-20 10:19 ` Mukunda,Vijendar
@ 2024-02-21 0:48 ` Mark Brown
2 siblings, 0 replies; 10+ messages in thread
From: Mark Brown @ 2024-02-21 0:48 UTC (permalink / raw
To: Pierre-Louis Bossart, Liam Girdwood, Peter Ujfalusi, Bard Liao,
Ranjani Sridharan, Daniel Baluta, Arnd Bergmann
Cc: Arnd Bergmann, Kai Vehmanen, Jaroslav Kysela, Takashi Iwai,
Vijendar Mukunda, V sujith kumar Reddy, Venkata Prasad Potturu,
sound-open-firmware, linux-sound, linux-kernel
On Mon, 19 Feb 2024 10:38:45 +0100, Arnd Bergmann wrote:
> The soundwire-amd driver has a bit of a layering violation requiring
> the SOF driver to directly call into its exported symbols rather than
> through an abstraction.
>
> The SND_SOC_SOF_AMD_SOUNDWIRE Kconfig symbol tries to deal with the
> dependency by selecting SOUNDWIRE_AMD in a complicated set of conditions,
> but gets it wrong for a configuration involving SND_SOC_SOF_AMD_COMMON=y,
> SND_SOC_SOF_AMD_ACP63=m, and SND_SOC_SOF_AMD_SOUNDWIRE_LINK_BASELINE=m
> SOUNDWIRE_AMD=m, which results in a link failure:
>
> [...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
Thanks!
[1/1] ASoC: SOF: amd: fix soundwire dependencies
commit: a3d543b9e6599fbbb9efc1876919627960c5e97a
All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying
to this mail.
Thanks,
Mark
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2024-02-21 0:48 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-02-19 9:38 [PATCH] ASoC: SOF: amd: fix soundwire dependencies Arnd Bergmann
2024-02-20 5:57 ` Mukunda,Vijendar
2024-02-20 6:13 ` Arnd Bergmann
2024-02-20 6:23 ` Mukunda,Vijendar
2024-02-20 7:10 ` Arnd Bergmann
2024-02-20 7:54 ` Mukunda,Vijendar
2024-02-20 8:21 ` Arnd Bergmann
2024-02-20 10:15 ` Mukunda,Vijendar
2024-02-20 10:19 ` Mukunda,Vijendar
2024-02-21 0:48 ` Mark Brown
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.