diff mbox

ASoC: intel: clean up CONFIG_SND_SST_IPC handling

Message ID CAK8P3a0H6CbFgqQijFRYsDoYq3mEPcuBL6x01Ppp7P+_81t++A@mail.gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Arnd Bergmann Jan. 22, 2018, 10:58 a.m. UTC 
On Mon, Jan 22, 2018 at 10:51 AM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Sun, 2018-01-21 at 23:14 +0100, Arnd Bergmann wrote:
>> In a configuration with SND_SST_ATOM_HIFI2_PLATFORM_PCI=y and
>> SND_SST_ATOM_HIFI2_PLATFORM=m, we get this module link failure:
>>
>> ERROR: "sst_context_init" [sound/soc/intel/atom/sst/snd-intel-sst-
>> acpi.ko] undefined!
>> ERROR: "sst_context_cleanup" [sound/soc/intel/atom/sst/snd-intel-sst-
>> acpi.ko] undefined!
>> ERROR: "sst_alloc_drv_context" [sound/soc/intel/atom/sst/snd-intel-
>> sst-acpi.ko] undefined!
>> ERROR: "intel_sst_pm" [sound/soc/intel/atom/sst/snd-intel-sst-acpi.ko]
>> undefined!
>> ERROR: "sst_configure_runtime_pm" [sound/soc/intel/atom/sst/snd-intel-
>> sst-acpi.ko] undefined!
>>
>> The problem is that the sound/soc/intel/atom/ directory only gets
>> entered by Kbuild when SND_SST_ATOM_HIFI2_PLATFORM is set, so we
>> only build modules (obj-m) under here but not built-in files (obj-y)
>> including the snd-intel-sst-core that we clearly want need here.
>>
>> Before commit 4772c16ede52 ("ASoC: Intel: Kconfig: Simplify-clarify
>> ACPI/PCI
>> dependencies"), this could not happen as all files in
>> sound/soc/intel/atom/
>> depended on CONFIG_SND_SST_ATOM_HIFI2_PLATFORM anyway.
>>
>> Slightly later, commit 05f4434bc130 ("ASoC: Intel: remove
>> mfld_machine")
>> was added, which then removed the Merrifield machine code that
>> happened
>> to be the only user of SND_SST_ATOM_HIFI2_PLATFORM_PCI. With that
>> gone,
>
> That's what I afraid of. Intel Merrifield *should* be there. What Vinod
> did, AFAIU, is removal of Intel Medfield support, which is fine with me.
>
> So, before this can go, we need to get confirmation from Vinod and
> Pierre, that Merrifield still stays there.

Ok, I see. Checking further, I see that SND_SST_ATOM_HIFI2_PLATFORM_PCI
cannot be built without SND_SST_ATOM_HIFI2_PLATFORM:

sound/soc/intel/atom/sst/sst_drv_interface.o: In function `sst_register':
sst_drv_interface.c:(.text+0xc3e): undefined reference to `sst_register_dsp'
sound/soc/intel/atom/sst/sst_drv_interface.o: In function `sst_unregister':
sst_drv_interface.c:(.text+0xc67): undefined reference to `sst_unregister_dsp'

How about this instead:

Comments

Andy Shevchenko Jan. 22, 2018, 11:39 a.m. UTC | #1 
On Mon, 2018-01-22 at 11:58 +0100, Arnd Bergmann wrote:
> On Mon, Jan 22, 2018 at 10:51 AM, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Sun, 2018-01-21 at 23:14 +0100, Arnd Bergmann wrote:

> > > Slightly later, commit 05f4434bc130 ("ASoC: Intel: remove
> > > mfld_machine")
> > > was added, which then removed the Merrifield machine code that
> > > happened
> > > to be the only user of SND_SST_ATOM_HIFI2_PLATFORM_PCI. With that
> > > gone,
> > 
> > That's what I afraid of. Intel Merrifield *should* be there. What
> > Vinod
> > did, AFAIU, is removal of Intel Medfield support, which is fine with
> > me.
> > 
> > So, before this can go, we need to get confirmation from Vinod and
> > Pierre, that Merrifield still stays there.
> 
> Ok, I see. Checking further, I see that
> SND_SST_ATOM_HIFI2_PLATFORM_PCI
> cannot be built without SND_SST_ATOM_HIFI2_PLATFORM:
> 
> sound/soc/intel/atom/sst/sst_drv_interface.o: In function
> `sst_register':
> sst_drv_interface.c:(.text+0xc3e): undefined reference to
> `sst_register_dsp'
> sound/soc/intel/atom/sst/sst_drv_interface.o: In function
> `sst_unregister':
> sst_drv_interface.c:(.text+0xc67): undefined reference to
> `sst_unregister_dsp'

Oh.

> 
> How about this instead:
> 
> diff --git a/sound/soc/intel/Kconfig b/sound/soc/intel/Kconfig
> index f2c9e8c5970a..16344bd24eb0 100644
> --- a/sound/soc/intel/Kconfig
> +++ b/sound/soc/intel/Kconfig
> @@ -72,21 +72,8 @@ config SND_SOC_INTEL_BAYTRAIL
>           for Baytrail Chromebooks but this option is now deprecated
> and is
>           not recommended, use SND_SST_ATOM_HIFI2_PLATFORM instead.
> 
> -config SND_SST_ATOM_HIFI2_PLATFORM_PCI
> -       tristate "PCI HiFi2 (Medfield, Merrifield) Platforms"
> -       depends on X86 && PCI
> -       select SND_SST_IPC_PCI
> -       select SND_SOC_COMPRESS
> -       help
> -         If you have a Intel Medfield or Merrifield/Edison platform,
> then
> -         enable this option by saying Y or m. Distros will typically
> not
> -         enable this option: Medfield devices are not available to
> -         developers and while Merrifield/Edison can run a mainline
> kernel with
> -         limited functionality it will require a firmware file which
> -         is not in the standard firmware tree
> -
>  config SND_SST_ATOM_HIFI2_PLATFORM
> -       tristate "ACPI HiFi2 (Baytrail, Cherrytrail) Platforms"
> +       tristate "ACPI HiFi2 (Baytrail, Cherrytrail, Merrifield)
> Platforms"

Perhaps it makes sense to do something like _HIFI2 and on top
HIFI2_PLATFORM and HIFI2_PCI, but it seems like a current split.

So, it means the split itself is not accurate in the first place.
Pierre, Vinod?

> +config SND_SOC_INTEL_MRFLD_MACH
> +       tristate "Merrifield/Edison platform"

Edison should not be here (it's a board, while Merrifield is a platform)

> +       depends on X86_INTEL_LPSS && I2C && PCI

X86_INTEL_LPSS has nothing to do with Merrifield. :-)

> +       select SND_SST_IPC_PCI
> +       help
> +         This adds support for ASoC PCI driver for the Merrifield
> +         (platform) used e.g. on Intel Edison.  If you have
> +         Merrifield/Edison platform, then enable this option by
> saying
> +         Y or m. Distros will typically not enable this option: while
> +         Merrifield/Edison can run a mainline kernel with limited
> +         functionality it will require a firmware file which is not
> in
> +         the standard firmware tree.

Above looks like a solution to me, although I'm not familiar with ASoC
code, so, I would rely on Pierre, Vinod and Liam suggestions.
Pierre-Louis Bossart Jan. 22, 2018, 4:37 p.m. UTC | #2 
On 1/22/18 5:39 AM, Andy Shevchenko wrote:
> On Mon, 2018-01-22 at 11:58 +0100, Arnd Bergmann wrote:
>> On Mon, Jan 22, 2018 at 10:51 AM, Andy Shevchenko
>> <andriy.shevchenko@linux.intel.com> wrote:
>>> On Sun, 2018-01-21 at 23:14 +0100, Arnd Bergmann wrote:
> 
>>>> Slightly later, commit 05f4434bc130 ("ASoC: Intel: remove
>>>> mfld_machine")
>>>> was added, which then removed the Merrifield machine code that
>>>> happened
>>>> to be the only user of SND_SST_ATOM_HIFI2_PLATFORM_PCI. With that
>>>> gone,
>>>
>>> That's what I afraid of. Intel Merrifield *should* be there. What
>>> Vinod
>>> did, AFAIU, is removal of Intel Medfield support, which is fine with
>>> me.
>>>
>>> So, before this can go, we need to get confirmation from Vinod and
>>> Pierre, that Merrifield still stays there.
>>
>> Ok, I see. Checking further, I see that
>> SND_SST_ATOM_HIFI2_PLATFORM_PCI
>> cannot be built without SND_SST_ATOM_HIFI2_PLATFORM:
>>
>> sound/soc/intel/atom/sst/sst_drv_interface.o: In function
>> `sst_register':
>> sst_drv_interface.c:(.text+0xc3e): undefined reference to
>> `sst_register_dsp'
>> sound/soc/intel/atom/sst/sst_drv_interface.o: In function
>> `sst_unregister':
>> sst_drv_interface.c:(.text+0xc67): undefined reference to
>> `sst_unregister_dsp'
> 
> Oh.
> 
>>
>> How about this instead:
>>
>> diff --git a/sound/soc/intel/Kconfig b/sound/soc/intel/Kconfig
>> index f2c9e8c5970a..16344bd24eb0 100644
>> --- a/sound/soc/intel/Kconfig
>> +++ b/sound/soc/intel/Kconfig
>> @@ -72,21 +72,8 @@ config SND_SOC_INTEL_BAYTRAIL
>>            for Baytrail Chromebooks but this option is now deprecated
>> and is
>>            not recommended, use SND_SST_ATOM_HIFI2_PLATFORM instead.
>>
>> -config SND_SST_ATOM_HIFI2_PLATFORM_PCI
>> -       tristate "PCI HiFi2 (Medfield, Merrifield) Platforms"
>> -       depends on X86 && PCI
>> -       select SND_SST_IPC_PCI
>> -       select SND_SOC_COMPRESS
>> -       help
>> -         If you have a Intel Medfield or Merrifield/Edison platform,
>> then
>> -         enable this option by saying Y or m. Distros will typically
>> not
>> -         enable this option: Medfield devices are not available to
>> -         developers and while Merrifield/Edison can run a mainline
>> kernel with
>> -         limited functionality it will require a firmware file which
>> -         is not in the standard firmware tree
>> -
>>   config SND_SST_ATOM_HIFI2_PLATFORM
>> -       tristate "ACPI HiFi2 (Baytrail, Cherrytrail) Platforms"
>> +       tristate "ACPI HiFi2 (Baytrail, Cherrytrail, Merrifield)
>> Platforms"
> 
> Perhaps it makes sense to do something like _HIFI2 and on top
> HIFI2_PLATFORM and HIFI2_PCI, but it seems like a current split.
> 
> So, it means the split itself is not accurate in the first place.
> Pierre, Vinod?
> 
>> +config SND_SOC_INTEL_MRFLD_MACH
>> +       tristate "Merrifield/Edison platform"
> 
> Edison should not be here (it's a board, while Merrifield is a platform)
> 
>> +       depends on X86_INTEL_LPSS && I2C && PCI
> 
> X86_INTEL_LPSS has nothing to do with Merrifield. :-)
> 
>> +       select SND_SST_IPC_PCI
>> +       help
>> +         This adds support for ASoC PCI driver for the Merrifield
>> +         (platform) used e.g. on Intel Edison.  If you have
>> +         Merrifield/Edison platform, then enable this option by
>> saying
>> +         Y or m. Distros will typically not enable this option: while
>> +         Merrifield/Edison can run a mainline kernel with limited
>> +         functionality it will require a firmware file which is not
>> in
>> +         the standard firmware tree.
> 
> Above looks like a solution to me, although I'm not familiar with ASoC
> code, so, I would rely on Pierre, Vinod and Liam suggestions.

I'd suggest that we instead add SND_SST_ATOM_HIFI2_PLATFORM_ACPI (for 
symmetry with PCI) and keep the SND_SST_ATOM_HIFI2_PLATFORM as a common 
part to solve this coexistence.

e.g (untested - just idea)

config SND_SST_ATOM_HIFI2_PLATFORM_PCI
	tristate "PCI HiFi2 (Merrifield) Platforms"
	depends on X86 && PCI
	select SND_SST_IPC_PCI
	select SND_SST_ATOM_HIFI2_PLATFORM

config SND_SST_ATOM_HIFI2_PLATFORM_ACPI
	tristate "ACPI HiFi2 (Baytrail, Cherrytrail) Platforms"
	depends on X86 && ACPI
	select SND_SST_IPC_ACPI
	select SND_SST_ATOM_HIFI2_PLATFORM

config SND_SST_ATOM_HIFI2_PLATFORM
	tristate
	select SND_SOC_COMPRESS

That said changing names would break oldnoconfig so maybe something 
similar that just adds the common layer.
Arnd Bergmann Jan. 22, 2018, 8:59 p.m. UTC | #3 
On Mon, Jan 22, 2018 at 5:37 PM, Pierre-Louis Bossart
<pierre-louis.bossart@linux.intel.com> wrote:
> On 1/22/18 5:39 AM, Andy Shevchenko wrote:
>> On Mon, 2018-01-22 at 11:58 +0100, Arnd Bergmann wrote:
>>> On Mon, Jan 22, 2018 at 10:51 AM, Andy Shevchenko
>>> <andriy.shevchenko@linux.intel.com> wrote:
>>>> On Sun, 2018-01-21 at 23:14 +0100, Arnd Bergmann wrote:

>> Above looks like a solution to me, although I'm not familiar with ASoC
>> code, so, I would rely on Pierre, Vinod and Liam suggestions.
>
>
> I'd suggest that we instead add SND_SST_ATOM_HIFI2_PLATFORM_ACPI (for
> symmetry with PCI) and keep the SND_SST_ATOM_HIFI2_PLATFORM as a common part
> to solve this coexistence.
>
> e.g (untested - just idea)
>
> config SND_SST_ATOM_HIFI2_PLATFORM_PCI
>         tristate "PCI HiFi2 (Merrifield) Platforms"
>         depends on X86 && PCI
>         select SND_SST_IPC_PCI
>         select SND_SST_ATOM_HIFI2_PLATFORM
>
> config SND_SST_ATOM_HIFI2_PLATFORM_ACPI
>         tristate "ACPI HiFi2 (Baytrail, Cherrytrail) Platforms"
>         depends on X86 && ACPI
>         select SND_SST_IPC_ACPI
>         select SND_SST_ATOM_HIFI2_PLATFORM
>
> config SND_SST_ATOM_HIFI2_PLATFORM
>         tristate
>         select SND_SOC_COMPRESS
>
> That said changing names would break oldnoconfig so maybe something similar
> that just adds the common layer.

It sounds like a good idea, at least if it can be done without a
larger code rework.
For the new SND_SST_ATOM_HIFI2_PLATFORM_ACPI symbol, you could simply
make that 'default ACPI' to make at least 'oldconfig' and 'olddefconfig' work.

See https://pastebin.com/GKtkgW99 for my randconfig file for testing the
configuration that I hit. With the current state of linux-next, there are two
configurations that are broken AFAICT

- SND_SST_ATOM_HIFI2_PLATFORM_PCI=y && SND_SST_ATOM_HIFI2_PLATFORM=m
  (because of the Makefile thing I mentioned)

- SND_SST_ATOM_HIFI2_PLATFORM_PCI=y && SND_SST_ATOM_HIFI2_PLATFORM=n
  (because of missing symbols)

       Arnd
diff mbox

Patch

diff --git a/sound/soc/intel/Kconfig b/sound/soc/intel/Kconfig
index f2c9e8c5970a..16344bd24eb0 100644
--- a/sound/soc/intel/Kconfig
+++ b/sound/soc/intel/Kconfig
@@ -72,21 +72,8 @@  config SND_SOC_INTEL_BAYTRAIL
          for Baytrail Chromebooks but this option is now deprecated and is
          not recommended, use SND_SST_ATOM_HIFI2_PLATFORM instead.

-config SND_SST_ATOM_HIFI2_PLATFORM_PCI
-       tristate "PCI HiFi2 (Medfield, Merrifield) Platforms"
-       depends on X86 && PCI
-       select SND_SST_IPC_PCI
-       select SND_SOC_COMPRESS
-       help
-         If you have a Intel Medfield or Merrifield/Edison platform, then
-         enable this option by saying Y or m. Distros will typically not
-         enable this option: Medfield devices are not available to
-         developers and while Merrifield/Edison can run a mainline kernel with
-         limited functionality it will require a firmware file which
-         is not in the standard firmware tree
-
 config SND_SST_ATOM_HIFI2_PLATFORM
-       tristate "ACPI HiFi2 (Baytrail, Cherrytrail) Platforms"
+       tristate "ACPI HiFi2 (Baytrail, Cherrytrail, Merrifield) Platforms"
        depends on X86 && ACPI
        select SND_SST_IPC_ACPI
        select SND_SOC_COMPRESS
diff --git a/sound/soc/intel/boards/Kconfig b/sound/soc/intel/boards/Kconfig
index d4e103615f51..c73b19292fda 100644
--- a/sound/soc/intel/boards/Kconfig
+++ b/sound/soc/intel/boards/Kconfig
@@ -159,6 +159,19 @@  config SND_SOC_INTEL_BYT_CHT_NOCODEC_MACH

          If unsure select "N".

+config SND_SOC_INTEL_MRFLD_MACH
+       tristate "Merrifield/Edison platform"
+       depends on X86_INTEL_LPSS && I2C && PCI
+       select SND_SST_IPC_PCI
+       help
+         This adds support for ASoC PCI driver for the Merrifield
+         (platform) used e.g. on Intel Edison.  If you have
+         Merrifield/Edison platform, then enable this option by saying
+         Y or m. Distros will typically not enable this option: while
+         Merrifield/Edison can run a mainline kernel with limited
+         functionality it will require a firmware file which is not in
+         the standard firmware tree.
+
 endif ## SND_SST_ATOM_HIFI2_PLATFORM

 if SND_SOC_INTEL_SKYLAKE