[PATCH] ASoC: SOF: sort out Kconfig, again

[PATCH] ASoC: SOF: sort out Kconfig, again

[Arnd Bergmann]

The imx8 config keeps causing issues:

WARNING: unmet direct dependencies detected for SND_SOC_SOF_IMX8M
  Depends on [n]: SOUND [=y] && !UML && SND [=y] && SND_SOC [=m] && SND_SOC_SOF_TOPLEVEL [=y] && SND_SOC_SOF_IMX_TOPLEVEL [=y] && IMX_DSP [=n]
  Selected by [m]:
  - SND_SOC_SOF_IMX_OF [=m] && SOUND [=y] && !UML && SND [=y] && SND_SOC [=m] && SND_SOC_SOF_TOPLEVEL [=y] && SND_SOC_SOF_IMX_TOPLEVEL [=y] && SND_SOC_SOF_IMX8M_SUPPORT [=y]

This is complicated by two drivers having dependencies on both
platform specific drivers and the SND_SOC_SOF_OF framework code,
and using an somewhat obscure method to build them the same way
as the SOC_SOF_OF symbol (built-in or modular).

My solution now ensures that the two drivers can only be enabled
when the dependencies are met:

- When the platform specific drivers are built-in, everything is
  fine, as SOC_SOF_OF is either =y or =m

- When both are loadable modules, it also works, both for Kconfig
  and at runtime

- When the hardware drivers are loadable modules or disabled, and
  SOC_SOF_OF=y, prevent the IMX_SOF_OF drivers from being turned on,
  as this would be broken.

It seems that this is just an elaborate way to describe two tristate
symbols that have straight dependencies, but maybe I'm missing some
subtle point. It seems to always build for me now.

Fixes: fe57a92c8858 ("ASoC: SOF: Add missing dependency on IMX_SCU")
Fixes: afb93d716533 ("ASoC: SOF: imx: Add i.MX8M HW support")
Fixes: cb0312f61c3e ("ASoC: SOF: imx: fix undefined reference issue")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 sound/soc/sof/imx/Kconfig | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/sound/soc/sof/imx/Kconfig b/sound/soc/sof/imx/Kconfig
index f76660e91382..66684d7590f4 100644
--- a/sound/soc/sof/imx/Kconfig
+++ b/sound/soc/sof/imx/Kconfig
@@ -21,7 +21,8 @@ config SND_SOC_SOF_IMX_OF
 
 config SND_SOC_SOF_IMX8_SUPPORT
 	bool "SOF support for i.MX8"
-	depends on IMX_SCU
+	depends on IMX_SCU=y || IMX_SCU=SND_SOC_SOF_IMX_OF
+	depends on IMX_DSP=y || IMX_DSP=SND_SOC_SOF_IMX_OF
 	help
 	  This adds support for Sound Open Firmware for NXP i.MX8 platforms
 	  Say Y if you have such a device.
@@ -29,14 +30,13 @@ config SND_SOC_SOF_IMX8_SUPPORT
 
 config SND_SOC_SOF_IMX8
 	tristate
-	depends on IMX_SCU
-	select IMX_DSP
 	help
 	  This option is not user-selectable but automagically handled by
 	  'select' statements at a higher level
 
 config SND_SOC_SOF_IMX8M_SUPPORT
 	bool "SOF support for i.MX8M"
+	depends on IMX_DSP=y || IMX_DSP=SND_SOC_SOF_OF
 	help
 	  This adds support for Sound Open Firmware for NXP i.MX8M platforms
 	  Say Y if you have such a device.
@@ -44,7 +44,6 @@ config SND_SOC_SOF_IMX8M_SUPPORT
 
 config SND_SOC_SOF_IMX8M
 	tristate
-	depends on IMX_DSP
 	help
 	  This option is not user-selectable but automagically handled by
 	  'select' statements at a higher level
-- 
2.26.0

Re: [PATCH] ASoC: SOF: sort out Kconfig, again

[Pierre-Louis Bossart]

On 4/28/20 4:27 PM, Arnd Bergmann wrote:
> The imx8 config keeps causing issues:
> 
> WARNING: unmet direct dependencies detected for SND_SOC_SOF_IMX8M
>    Depends on [n]: SOUND [=y] && !UML && SND [=y] && SND_SOC [=m] && SND_SOC_SOF_TOPLEVEL [=y] && SND_SOC_SOF_IMX_TOPLEVEL [=y] && IMX_DSP [=n]
>    Selected by [m]:
>    - SND_SOC_SOF_IMX_OF [=m] && SOUND [=y] && !UML && SND [=y] && SND_SOC [=m] && SND_SOC_SOF_TOPLEVEL [=y] && SND_SOC_SOF_IMX_TOPLEVEL [=y] && SND_SOC_SOF_IMX8M_SUPPORT [=y]
> 
> This is complicated by two drivers having dependencies on both
> platform specific drivers and the SND_SOC_SOF_OF framework code,
> and using an somewhat obscure method to build them the same way
> as the SOC_SOF_OF symbol (built-in or modular).
> 
> My solution now ensures that the two drivers can only be enabled
> when the dependencies are met:
> 
> - When the platform specific drivers are built-in, everything is
>    fine, as SOC_SOF_OF is either =y or =m
> 
> - When both are loadable modules, it also works, both for Kconfig
>    and at runtime
> 
> - When the hardware drivers are loadable modules or disabled, and
>    SOC_SOF_OF=y, prevent the IMX_SOF_OF drivers from being turned on,
>    as this would be broken.
> 
> It seems that this is just an elaborate way to describe two tristate
> symbols that have straight dependencies, but maybe I'm missing some
> subtle point. It seems to always build for me now.

Thanks Arnd, do you mind sharing your config? We noticed last week that 
there's a depend/select confusion might be simpler to fix, see 
https://github.com/thesofproject/linux/pull/2047/commits

If I look at the first line I see a IMX_DSP=n which looks exactly like 
what we wanted to fix.

> 
> Fixes: fe57a92c8858 ("ASoC: SOF: Add missing dependency on IMX_SCU")
> Fixes: afb93d716533 ("ASoC: SOF: imx: Add i.MX8M HW support")
> Fixes: cb0312f61c3e ("ASoC: SOF: imx: fix undefined reference issue")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>   sound/soc/sof/imx/Kconfig | 7 +++----
>   1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/sound/soc/sof/imx/Kconfig b/sound/soc/sof/imx/Kconfig
> index f76660e91382..66684d7590f4 100644
> --- a/sound/soc/sof/imx/Kconfig
> +++ b/sound/soc/sof/imx/Kconfig
> @@ -21,7 +21,8 @@ config SND_SOC_SOF_IMX_OF
>   
>   config SND_SOC_SOF_IMX8_SUPPORT
>   	bool "SOF support for i.MX8"
> -	depends on IMX_SCU
> +	depends on IMX_SCU=y || IMX_SCU=SND_SOC_SOF_IMX_OF
> +	depends on IMX_DSP=y || IMX_DSP=SND_SOC_SOF_IMX_OF
>   	help
>   	  This adds support for Sound Open Firmware for NXP i.MX8 platforms
>   	  Say Y if you have such a device.
> @@ -29,14 +30,13 @@ config SND_SOC_SOF_IMX8_SUPPORT
>   
>   config SND_SOC_SOF_IMX8
>   	tristate
> -	depends on IMX_SCU
> -	select IMX_DSP
>   	help
>   	  This option is not user-selectable but automagically handled by
>   	  'select' statements at a higher level
>   
>   config SND_SOC_SOF_IMX8M_SUPPORT
>   	bool "SOF support for i.MX8M"
> +	depends on IMX_DSP=y || IMX_DSP=SND_SOC_SOF_OF
>   	help
>   	  This adds support for Sound Open Firmware for NXP i.MX8M platforms
>   	  Say Y if you have such a device.
> @@ -44,7 +44,6 @@ config SND_SOC_SOF_IMX8M_SUPPORT
>   
>   config SND_SOC_SOF_IMX8M
>   	tristate
> -	depends on IMX_DSP
>   	help
>   	  This option is not user-selectable but automagically handled by
>   	  'select' statements at a higher level
> 

Re: [PATCH] ASoC: SOF: sort out Kconfig, again

[Arnd Bergmann]

On Tue, Apr 28, 2020 at 11:43 PM Pierre-Louis Bossart
<pierre-louis.bossart@linux.intel.com> wrote:
> On 4/28/20 4:27 PM, Arnd Bergmann wrote:
> > The imx8 config keeps causing issues:
> >
> > WARNING: unmet direct dependencies detected for SND_SOC_SOF_IMX8M
> >    Depends on [n]: SOUND [=y] && !UML && SND [=y] && SND_SOC [=m] && SND_SOC_SOF_TOPLEVEL [=y] && SND_SOC_SOF_IMX_TOPLEVEL [=y] && IMX_DSP [=n]
> >    Selected by [m]:
> >    - SND_SOC_SOF_IMX_OF [=m] && SOUND [=y] && !UML && SND [=y] && SND_SOC [=m] && SND_SOC_SOF_TOPLEVEL [=y] && SND_SOC_SOF_IMX_TOPLEVEL [=y] && SND_SOC_SOF_IMX8M_SUPPORT [=y]
> >
> > This is complicated by two drivers having dependencies on both
> > platform specific drivers and the SND_SOC_SOF_OF framework code,
> > and using an somewhat obscure method to build them the same way
> > as the SOC_SOF_OF symbol (built-in or modular).
> >
> > My solution now ensures that the two drivers can only be enabled
> > when the dependencies are met:
> >
> > - When the platform specific drivers are built-in, everything is
> >    fine, as SOC_SOF_OF is either =y or =m
> >
> > - When both are loadable modules, it also works, both for Kconfig
> >    and at runtime
> >
> > - When the hardware drivers are loadable modules or disabled, and
> >    SOC_SOF_OF=y, prevent the IMX_SOF_OF drivers from being turned on,
> >    as this would be broken.
> >
> > It seems that this is just an elaborate way to describe two tristate
> > symbols that have straight dependencies, but maybe I'm missing some
> > subtle point. It seems to always build for me now.
>
> Thanks Arnd, do you mind sharing your config?

https://pastebin.com/HRX5xi3R

> We noticed last week that
> there's a depend/select confusion might be simpler to fix, see
> https://github.com/thesofproject/linux/pull/2047/commits
>
> If I look at the first line I see a IMX_DSP=n which looks exactly like
> what we wanted to fix.

Yes, I think that fix addresses the build warning as well, but looking
more closely I don't think it's what you want: If you do this on
a config that has the IMX_DSP disabled, it would appear to the
user that you have enabled the drivers, but the actual code is still
disabled.

      Arnd

Re: [PATCH] ASoC: SOF: sort out Kconfig, again

[Pierre-Louis Bossart]

>> Thanks Arnd, do you mind sharing your config?
> 
> https://pastebin.com/HRX5xi3R

will give it a try, thanks!

>> We noticed last week that
>> there's a depend/select confusion might be simpler to fix, see
>> https://github.com/thesofproject/linux/pull/2047/commits
>>
>> If I look at the first line I see a IMX_DSP=n which looks exactly like
>> what we wanted to fix.
> 
> Yes, I think that fix addresses the build warning as well, but looking
> more closely I don't think it's what you want: If you do this on
> a config that has the IMX_DSP disabled, it would appear to the
> user that you have enabled the drivers, but the actual code is still
> disabled.

Are you sure? we added a select IMX_DSP, so not sure how it can be disabled?

Re: [PATCH] ASoC: SOF: sort out Kconfig, again

[Pierre-Louis Bossart]

[-- Attachment #1: Type: text/plain, Size: 1242 bytes --]



>>> Thanks Arnd, do you mind sharing your config?
>>
>> https://pastebin.com/HRX5xi3R
> 
> will give it a try, thanks!
> 
>>> We noticed last week that
>>> there's a depend/select confusion might be simpler to fix, see
>>> https://github.com/thesofproject/linux/pull/2047/commits
>>>
>>> If I look at the first line I see a IMX_DSP=n which looks exactly like
>>> what we wanted to fix.
>>
>> Yes, I think that fix addresses the build warning as well, but looking
>> more closely I don't think it's what you want: If you do this on
>> a config that has the IMX_DSP disabled, it would appear to the
>> user that you have enabled the drivers, but the actual code is still
>> disabled.
> 
> Are you sure? we added a select IMX_DSP, so not sure how it can be 
> disabled?

I just tested Arnd's config with the patch we came up with for SOF 
(attached) and it makes the unmet dependency go away and builds fine. 
the problem is really using select IMX_DSP if it can be disabled by 
something else. My proposal looks simpler but I will agree it's not 
necessarily super elegant to move the dependency on IMX_BOX into SOF, so 
no sustained objection from me on Arnd's proposal.

Daniel, this is your part of SOF, please chime in.

Thanks
-Pierre



[-- Attachment #2: 0001-ASoC-SOF-imx-fix-depends-select-IMX_DSP-confusion.patch --]
[-- Type: text/x-patch, Size: 1841 bytes --]

From 208e61ae18d3a4aa93ffa73db01c4e3c24a4979f Mon Sep 17 00:00:00 2001
From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Date: Wed, 22 Apr 2020 06:21:56 -0500
Subject: [PATCH] ASoC: SOF: imx: fix depends/select IMX_DSP confusion

The two IMX targets don't use depends/select in a consistent way and
there's a potential for an unmet dependency. Move the dependency check
to a higher level and select IMX_DSP to avoid builtin/module issues.

Fixes: afb93d716533dd ("ASoC: SOF: imx: Add i.MX8M HW support")
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/soc/sof/imx/Kconfig | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/sound/soc/sof/imx/Kconfig b/sound/soc/sof/imx/Kconfig
index f76660e91382..a11ecc1d56db 100644
--- a/sound/soc/sof/imx/Kconfig
+++ b/sound/soc/sof/imx/Kconfig
@@ -21,7 +21,7 @@ config SND_SOC_SOF_IMX_OF
 
 config SND_SOC_SOF_IMX8_SUPPORT
 	bool "SOF support for i.MX8"
-	depends on IMX_SCU
+	depends on IMX_SCU && IMX_MBOX
 	help
 	  This adds support for Sound Open Firmware for NXP i.MX8 platforms
 	  Say Y if you have such a device.
@@ -29,7 +29,6 @@ config SND_SOC_SOF_IMX8_SUPPORT
 
 config SND_SOC_SOF_IMX8
 	tristate
-	depends on IMX_SCU
 	select IMX_DSP
 	help
 	  This option is not user-selectable but automagically handled by
@@ -37,6 +36,7 @@ config SND_SOC_SOF_IMX8
 
 config SND_SOC_SOF_IMX8M_SUPPORT
 	bool "SOF support for i.MX8M"
+	depends on IMX_MBOX
 	help
 	  This adds support for Sound Open Firmware for NXP i.MX8M platforms
 	  Say Y if you have such a device.
@@ -44,7 +44,7 @@ config SND_SOC_SOF_IMX8M_SUPPORT
 
 config SND_SOC_SOF_IMX8M
 	tristate
-	depends on IMX_DSP
+	select IMX_DSP
 	help
 	  This option is not user-selectable but automagically handled by
 	  'select' statements at a higher level
-- 
2.20.1

Re: [PATCH] ASoC: SOF: sort out Kconfig, again

[Daniel Baluta]

Hi Arnd, Pierre,

Thanks for looking at this.

> >>> Thanks Arnd, do you mind sharing your config?
> >>
> >> https://pastebin.com/HRX5xi3R
> >
> > will give it a try, thanks!
> >
> >>> We noticed last week that
> >>> there's a depend/select confusion might be simpler to fix, see
> >>> https://github.com/thesofproject/linux/pull/2047/commits
> >>>
> >>> If I look at the first line I see a IMX_DSP=n which looks exactly like
> >>> what we wanted to fix.
> >>
> >> Yes, I think that fix addresses the build warning as well, but looking
> >> more closely I don't think it's what you want: If you do this on
> >> a config that has the IMX_DSP disabled, it would appear to the
> >> user that you have enabled the drivers, but the actual code is still
> >> disabled.
> >
> > Are you sure? we added a select IMX_DSP, so not sure how it can be
> > disabled?
>
> I just tested Arnd's config with the patch we came up with for SOF
> (attached) and it makes the unmet dependency go away and builds fine.
> the problem is really using select IMX_DSP if it can be disabled by
> something else. My proposal looks simpler but I will agree it's not
> necessarily super elegant to move the dependency on IMX_BOX into SOF, so
> no sustained objection from me on Arnd's proposal.
>
> Daniel, this is your part of SOF, please chime in.

I would go in favor of Arnd's patch as it gets rid of exposing
IMX_MBOX into SOF.
The code will be fine even IMX_DSP=n, because the exported functions
used by SOF have dummy implementations in case IMX_DSP is not selected.

One concern is that we could end up in a case where IMX_DSP={y|m} but
IMX_MBOX=n.

Technically this is not possible because IMX_DSP depends on IMX_MBOX. So,
one cannot generate such a .config file from menuconfig interface.

You can add my:

Acked-by: Daniel Baluta <daniel.baluta@nxp.com>

Re: [PATCH] ASoC: SOF: sort out Kconfig, again

[Arnd Bergmann]

On Wed, Apr 29, 2020 at 1:00 AM Pierre-Louis Bossart
<pierre-louis.bossart@linux.intel.com> wrote:
> >>> Thanks Arnd, do you mind sharing your config?
> >>
> >> https://pastebin.com/HRX5xi3R
> >
> > will give it a try, thanks!
> >
> >>> We noticed last week that
> >>> there's a depend/select confusion might be simpler to fix, see
> >>> https://github.com/thesofproject/linux/pull/2047/commits
> >>>
> >>> If I look at the first line I see a IMX_DSP=n which looks exactly like
> >>> what we wanted to fix.
> >>
> >> Yes, I think that fix addresses the build warning as well, but looking
> >> more closely I don't think it's what you want: If you do this on
> >> a config that has the IMX_DSP disabled, it would appear to the
> >> user that you have enabled the drivers, but the actual code is still
> >> disabled.
> >
> > Are you sure? we added a select IMX_DSP, so not sure how it can be
> > disabled?
>
> I just tested Arnd's config with the patch we came up with for SOF
> (attached) and it makes the unmet dependency go away and builds fine.
> the problem is really using select IMX_DSP if it can be disabled by
> something else. My proposal looks simpler but I will agree it's not
> necessarily super elegant to move the dependency on IMX_BOX into SOF, so
> no sustained objection from me on Arnd's proposal.

Ok, thanks for testing!

I looked at the bigger picture again and found that the more fundamental
problem is the dependency reversal in sound/soc/sof/sof-of-dev.c, where
you have common code that knows about and links against a hardware
specific driver. This is something we try hard do avoid in general in the
kernel, as it causes all kinds of problems:

- Expressing the Kconfig dependencies is rather unnatural and error-prone,
  as you found

- Adding multiple new drivers at the same time leads to merge conflicts

- A kernel that supports multiple SoC families, like all general-purpose
  distros do, and Android is going to do in the future means that you have
  to load every hardware specific module in order to just use one of them.

- In Android's case, it also breaks the model of having one vendor provide
  support for a new SoC by enabling additional modules they ship in
  their vendor specific partition

I think this is all solved by moving the "module_platform_driver()"
and of_device_id array for each driver into the module that defines
the corresponding sof_dev_desc structure, and have those drivers
call the exported sof_of_probe() and sof_of_remove() functions.

      Arnd

Re: [PATCH] ASoC: SOF: sort out Kconfig, again

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 679 bytes --]

On Wed, Apr 29, 2020 at 10:24:45AM +0200, Arnd Bergmann wrote:

> I looked at the bigger picture again and found that the more fundamental
> problem is the dependency reversal in sound/soc/sof/sof-of-dev.c, where
> you have common code that knows about and links against a hardware
> specific driver. This is something we try hard do avoid in general in the
> kernel, as it causes all kinds of problems:

This is a legacy of this being factored out of the x86 code, since ACPI
is not really fit for purpose when used to describe at least the audio
hardware on modern laptops essentially all the enumeration is quirk
based.  It really needs cleaning up for the non-x86 SOF users.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] ASoC: SOF: sort out Kconfig, again

[Mark Brown]

On Tue, 28 Apr 2020 23:27:36 +0200, Arnd Bergmann wrote:
> The imx8 config keeps causing issues:
> 
> WARNING: unmet direct dependencies detected for SND_SOC_SOF_IMX8M
>   Depends on [n]: SOUND [=y] && !UML && SND [=y] && SND_SOC [=m] && SND_SOC_SOF_TOPLEVEL [=y] && SND_SOC_SOF_IMX_TOPLEVEL [=y] && IMX_DSP [=n]
>   Selected by [m]:
>   - SND_SOC_SOF_IMX_OF [=m] && SOUND [=y] && !UML && SND [=y] && SND_SOC [=m] && SND_SOC_SOF_TOPLEVEL [=y] && SND_SOC_SOF_IMX_TOPLEVEL [=y] && SND_SOC_SOF_IMX8M_SUPPORT [=y]
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-5.8

Thanks!

[1/1] ASoC: SOF: sort out Kconfig, again
      commit: f9dfa8f25462a2b9bb47dcb563688d616e21ee83

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