* [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 a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox