[alsa-devel] [PATCH] ASoC: intel: clean up CONFIG_SND_SST_IPC handling
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, it appears that we can completely remove that symbol as well, along with the SND_SST_IPC_PCI code and everything associated with it that is now unused.
For clarify, this also removes the SND_SST_IPC_ACPI symbol that is now synonymous with SND_SST_IPC, and links both into a single loadable module.
Fixes: 4772c16ede52 ("ASoC: Intel: Kconfig: Simplify-clarify ACPI/PCI dependencies") Fixes: 05f4434bc130 ("ASoC: Intel: remove mfld_machine") Signed-off-by: Arnd Bergmann arnd@arndb.de --- I checked that this patch leads to a clean compilation, and that the code I'm removing is only used for the now-obsolete Merrifield PCI ID.
If I got something else wrong, or you want a different fix, please just send a replacement patch and treat this as a bug report. --- sound/soc/intel/Kconfig | 27 +---- sound/soc/intel/atom/sst/Makefile | 6 +- sound/soc/intel/atom/sst/sst_pci.c | 209 ------------------------------------- sound/soc/intel/common/sst-acpi.c | 4 +- 4 files changed, 5 insertions(+), 241 deletions(-) delete mode 100644 sound/soc/intel/atom/sst/sst_pci.c
diff --git a/sound/soc/intel/Kconfig b/sound/soc/intel/Kconfig index f2c9e8c5970a..2dc8cda7a7cd 100644 --- a/sound/soc/intel/Kconfig +++ b/sound/soc/intel/Kconfig @@ -17,21 +17,11 @@ if SND_SOC_INTEL_SST_TOPLEVEL config SND_SST_IPC tristate # This option controls the IPC core for HiFi2 platforms - -config SND_SST_IPC_PCI - tristate - select SND_SST_IPC - # This option controls the PCI-based IPC for HiFi2 platforms - # (Medfield, Merrifield). - -config SND_SST_IPC_ACPI - tristate - select SND_SST_IPC - # This option controls the ACPI-based IPC for HiFi2 platforms # (Baytrail, Cherrytrail)
config SND_SOC_INTEL_SST_ACPI tristate + select SND_SST_IPC # This option controls ACPI-based probing on # Haswell/Broadwell/Baytrail legacy and will be set # when these platforms are enabled @@ -72,23 +62,10 @@ 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" depends on X86 && ACPI - select SND_SST_IPC_ACPI + select SND_SST_IPC select SND_SOC_COMPRESS select SND_SOC_ACPI_INTEL_MATCH select IOSF_MBI diff --git a/sound/soc/intel/atom/sst/Makefile b/sound/soc/intel/atom/sst/Makefile index 795d1cf8f386..f5b7008b4cea 100644 --- a/sound/soc/intel/atom/sst/Makefile +++ b/sound/soc/intel/atom/sst/Makefile @@ -1,8 +1,4 @@ # SPDX-License-Identifier: GPL-2.0 -snd-intel-sst-core-objs := sst.o sst_ipc.o sst_stream.o sst_drv_interface.o sst_loader.o sst_pvt.o -snd-intel-sst-pci-objs += sst_pci.o -snd-intel-sst-acpi-objs += sst_acpi.o +snd-intel-sst-core-objs := sst.o sst_ipc.o sst_stream.o sst_drv_interface.o sst_loader.o sst_pvt.o sst_acpi.o
obj-$(CONFIG_SND_SST_IPC) += snd-intel-sst-core.o -obj-$(CONFIG_SND_SST_IPC_PCI) += snd-intel-sst-pci.o -obj-$(CONFIG_SND_SST_IPC_ACPI) += snd-intel-sst-acpi.o diff --git a/sound/soc/intel/atom/sst/sst_pci.c b/sound/soc/intel/atom/sst/sst_pci.c deleted file mode 100644 index 6906ee624cf6..000000000000 --- a/sound/soc/intel/atom/sst/sst_pci.c +++ /dev/null @@ -1,209 +0,0 @@ -/* - * sst_pci.c - SST (LPE) driver init file for pci enumeration. - * - * Copyright (C) 2008-14 Intel Corp - * Authors: Vinod Koul vinod.koul@intel.com - * Harsha Priya priya.harsha@intel.com - * Dharageswari R dharageswari.r@intel.com - * KP Jeeja jeeja.kp@intel.com - * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ - * - * This program is free software; you can redistribute it and/or modify - * it under the terms of the GNU General Public License as published by - * the Free Software Foundation; version 2 of the License. - * - * This program is distributed in the hope that it will be useful, but - * WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - * General Public License for more details. - * - * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ - */ -#include <linux/module.h> -#include <linux/pci.h> -#include <linux/fs.h> -#include <linux/firmware.h> -#include <linux/pm_runtime.h> -#include <sound/core.h> -#include <sound/soc.h> -#include <asm/platform_sst_audio.h> -#include "../sst-mfld-platform.h" -#include "sst.h" - -static int sst_platform_get_resources(struct intel_sst_drv *ctx) -{ - int ddr_base, ret = 0; - struct pci_dev *pci = ctx->pci; - - ret = pci_request_regions(pci, SST_DRV_NAME); - if (ret) - return ret; - - /* map registers */ - /* DDR base */ - if (ctx->dev_id == SST_MRFLD_PCI_ID) { - ctx->ddr_base = pci_resource_start(pci, 0); - /* check that the relocated IMR base matches with FW Binary */ - ddr_base = relocate_imr_addr_mrfld(ctx->ddr_base); - if (!ctx->pdata->lib_info) { - dev_err(ctx->dev, "lib_info pointer NULL\n"); - ret = -EINVAL; - goto do_release_regions; - } - if (ddr_base != ctx->pdata->lib_info->mod_base) { - dev_err(ctx->dev, - "FW LSP DDR BASE does not match with IFWI\n"); - ret = -EINVAL; - goto do_release_regions; - } - ctx->ddr_end = pci_resource_end(pci, 0); - - ctx->ddr = pcim_iomap(pci, 0, - pci_resource_len(pci, 0)); - if (!ctx->ddr) { - ret = -EINVAL; - goto do_release_regions; - } - dev_dbg(ctx->dev, "sst: DDR Ptr %p\n", ctx->ddr); - } else { - ctx->ddr = NULL; - } - /* SHIM */ - ctx->shim_phy_add = pci_resource_start(pci, 1); - ctx->shim = pcim_iomap(pci, 1, pci_resource_len(pci, 1)); - if (!ctx->shim) { - ret = -EINVAL; - goto do_release_regions; - } - dev_dbg(ctx->dev, "SST Shim Ptr %p\n", ctx->shim); - - /* Shared SRAM */ - ctx->mailbox_add = pci_resource_start(pci, 2); - ctx->mailbox = pcim_iomap(pci, 2, pci_resource_len(pci, 2)); - if (!ctx->mailbox) { - ret = -EINVAL; - goto do_release_regions; - } - dev_dbg(ctx->dev, "SRAM Ptr %p\n", ctx->mailbox); - - /* IRAM */ - ctx->iram_end = pci_resource_end(pci, 3); - ctx->iram_base = pci_resource_start(pci, 3); - ctx->iram = pcim_iomap(pci, 3, pci_resource_len(pci, 3)); - if (!ctx->iram) { - ret = -EINVAL; - goto do_release_regions; - } - dev_dbg(ctx->dev, "IRAM Ptr %p\n", ctx->iram); - - /* DRAM */ - ctx->dram_end = pci_resource_end(pci, 4); - ctx->dram_base = pci_resource_start(pci, 4); - ctx->dram = pcim_iomap(pci, 4, pci_resource_len(pci, 4)); - if (!ctx->dram) { - ret = -EINVAL; - goto do_release_regions; - } - dev_dbg(ctx->dev, "DRAM Ptr %p\n", ctx->dram); -do_release_regions: - pci_release_regions(pci); - return 0; -} - -/* - * intel_sst_probe - PCI probe function - * - * @pci: PCI device structure - * @pci_id: PCI device ID structure - * - */ -static int intel_sst_probe(struct pci_dev *pci, - const struct pci_device_id *pci_id) -{ - int ret = 0; - struct intel_sst_drv *sst_drv_ctx; - struct sst_platform_info *sst_pdata = pci->dev.platform_data; - - dev_dbg(&pci->dev, "Probe for DID %x\n", pci->device); - ret = sst_alloc_drv_context(&sst_drv_ctx, &pci->dev, pci->device); - if (ret < 0) - return ret; - - sst_drv_ctx->pdata = sst_pdata; - sst_drv_ctx->irq_num = pci->irq; - snprintf(sst_drv_ctx->firmware_name, sizeof(sst_drv_ctx->firmware_name), - "%s%04x%s", "fw_sst_", - sst_drv_ctx->dev_id, ".bin"); - - ret = sst_context_init(sst_drv_ctx); - if (ret < 0) - return ret; - - /* Init the device */ - ret = pcim_enable_device(pci); - if (ret) { - dev_err(sst_drv_ctx->dev, - "device can't be enabled. Returned err: %d\n", ret); - goto do_free_drv_ctx; - } - sst_drv_ctx->pci = pci_dev_get(pci); - ret = sst_platform_get_resources(sst_drv_ctx); - if (ret < 0) - goto do_free_drv_ctx; - - pci_set_drvdata(pci, sst_drv_ctx); - sst_configure_runtime_pm(sst_drv_ctx); - - return ret; - -do_free_drv_ctx: - sst_context_cleanup(sst_drv_ctx); - dev_err(sst_drv_ctx->dev, "Probe failed with %d\n", ret); - return ret; -} - -/** - * intel_sst_remove - PCI remove function - * - * @pci: PCI device structure - * - * This function is called by OS when a device is unloaded - * This frees the interrupt etc - */ -static void intel_sst_remove(struct pci_dev *pci) -{ - struct intel_sst_drv *sst_drv_ctx = pci_get_drvdata(pci); - - sst_context_cleanup(sst_drv_ctx); - pci_dev_put(sst_drv_ctx->pci); - pci_release_regions(pci); - pci_set_drvdata(pci, NULL); -} - -/* PCI Routines */ -static const struct pci_device_id intel_sst_ids[] = { - { PCI_VDEVICE(INTEL, SST_MRFLD_PCI_ID), 0}, - { 0, } -}; - -static struct pci_driver sst_driver = { - .name = SST_DRV_NAME, - .id_table = intel_sst_ids, - .probe = intel_sst_probe, - .remove = intel_sst_remove, -#ifdef CONFIG_PM - .driver = { - .pm = &intel_sst_pm, - }, -#endif -}; - -module_pci_driver(sst_driver); - -MODULE_DESCRIPTION("Intel (R) SST(R) Audio Engine PCI Driver"); -MODULE_AUTHOR("Vinod Koul vinod.koul@intel.com"); -MODULE_AUTHOR("Harsha Priya priya.harsha@intel.com"); -MODULE_AUTHOR("Dharageswari R dharageswari.r@intel.com"); -MODULE_AUTHOR("KP Jeeja jeeja.kp@intel.com"); -MODULE_LICENSE("GPL v2"); -MODULE_ALIAS("sst"); diff --git a/sound/soc/intel/common/sst-acpi.c b/sound/soc/intel/common/sst-acpi.c index cf6fbbd4e378..3b669d5adbb6 100644 --- a/sound/soc/intel/common/sst-acpi.c +++ b/sound/soc/intel/common/sst-acpi.c @@ -206,7 +206,7 @@ static struct sst_acpi_desc sst_acpi_broadwell_desc = { .dma_size = SST_LPT_DSP_DMA_SIZE, };
-#if !IS_ENABLED(CONFIG_SND_SST_IPC_ACPI) +#if !IS_ENABLED(CONFIG_SND_SST_IPC) static struct sst_acpi_desc sst_acpi_baytrail_desc = { .drv_name = "baytrail-pcm-audio", .machines = snd_soc_acpi_intel_baytrail_legacy_machines, @@ -222,7 +222,7 @@ static struct sst_acpi_desc sst_acpi_baytrail_desc = { static const struct acpi_device_id sst_acpi_match[] = { { "INT33C8", (unsigned long)&sst_acpi_haswell_desc }, { "INT3438", (unsigned long)&sst_acpi_broadwell_desc }, -#if !IS_ENABLED(CONFIG_SND_SST_IPC_ACPI) +#if !IS_ENABLED(CONFIG_SND_SST_IPC) { "80860F28", (unsigned long)&sst_acpi_baytrail_desc }, #endif { }
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.
it appears that we can completely remove that symbol as well, along with the SND_SST_IPC_PCI code and everything associated with it that is now unused.
For clarify, this also removes the SND_SST_IPC_ACPI symbol that is now synonymous with SND_SST_IPC, and links both into a single loadable module.
I checked that this patch leads to a clean compilation, and that the code I'm removing is only used for the now-obsolete Merrifield PCI ID.
NACK for this part.
If I got something else wrong, or you want a different fix, please just send a replacement patch and treat this as a bug report.
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:
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
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 && PCIselect SND_SST_IPC_PCIselect SND_SOC_COMPRESShelpIf you have a Intel Medfield or Merrifield/Edison platform,then
enable this option by saying Y or m. Distros will typicallynot
enable this option: Medfield devices are not available todevelopers and while Merrifield/Edison can run a mainlinekernel with
limited functionality it will require a firmware file whichis not in the standard firmware treeconfig 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_PCIhelpThis adds support for ASoC PCI driver for the Merrifield(platform) used e.g. on Intel Edison. If you haveMerrifield/Edison platform, then enable this option bysaying
Y or m. Distros will typically not enable this option: whileMerrifield/Edison can run a mainline kernel with limitedfunctionality it will require a firmware file which is notin
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.
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 && PCIselect SND_SST_IPC_PCIselect SND_SOC_COMPRESShelpIf you have a Intel Medfield or Merrifield/Edison platform,then
enable this option by saying Y or m. Distros will typicallynot
enable this option: Medfield devices are not available todevelopers and while Merrifield/Edison can run a mainlinekernel with
limited functionality it will require a firmware file whichis 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 && PCIX86_INTEL_LPSS has nothing to do with Merrifield. :-)
select SND_SST_IPC_PCIhelpThis adds support for ASoC PCI driver for the Merrifield(platform) used e.g. on Intel Edison. If you haveMerrifield/Edison platform, then enable this option bysaying
Y or m. Distros will typically not enable this option: whileMerrifield/Edison can run a mainline kernel with limitedfunctionality it will require a firmware file which is notin
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.
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
On 1/21/18 4:14 PM, 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, it appears that we can completely remove that symbol as well, along with the SND_SST_IPC_PCI code and everything associated with it that is now unused.
there's a misunderstanding here. The Medfield code was removed. We want to keep the PCI stuff to support Merrifield aka Tangier/Edison - at least for now.
For clarify, this also removes the SND_SST_IPC_ACPI symbol that is now synonymous with SND_SST_IPC, and links both into a single loadable module.
Fixes: 4772c16ede52 ("ASoC: Intel: Kconfig: Simplify-clarify ACPI/PCI dependencies") Fixes: 05f4434bc130 ("ASoC: Intel: remove mfld_machine") Signed-off-by: Arnd Bergmann arnd@arndb.de
I checked that this patch leads to a clean compilation, and that the code I'm removing is only used for the now-obsolete Merrifield PCI ID.
If I got something else wrong, or you want a different fix, please just send a replacement patch and treat this as a bug report.
sound/soc/intel/Kconfig | 27 +---- sound/soc/intel/atom/sst/Makefile | 6 +- sound/soc/intel/atom/sst/sst_pci.c | 209 ------------------------------------- sound/soc/intel/common/sst-acpi.c | 4 +- 4 files changed, 5 insertions(+), 241 deletions(-) delete mode 100644 sound/soc/intel/atom/sst/sst_pci.c
diff --git a/sound/soc/intel/Kconfig b/sound/soc/intel/Kconfig index f2c9e8c5970a..2dc8cda7a7cd 100644 --- a/sound/soc/intel/Kconfig +++ b/sound/soc/intel/Kconfig @@ -17,21 +17,11 @@ if SND_SOC_INTEL_SST_TOPLEVEL config SND_SST_IPC tristate # This option controls the IPC core for HiFi2 platforms
-config SND_SST_IPC_PCI
- tristate
- select SND_SST_IPC
- # This option controls the PCI-based IPC for HiFi2 platforms
- # (Medfield, Merrifield).
-config SND_SST_IPC_ACPI
tristate
select SND_SST_IPC
# This option controls the ACPI-based IPC for HiFi2 platforms # (Baytrail, Cherrytrail)
config SND_SOC_INTEL_SST_ACPI tristate
- select SND_SST_IPC # This option controls ACPI-based probing on # Haswell/Broadwell/Baytrail legacy and will be set # when these platforms are enabled
@@ -72,23 +62,10 @@ 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, thenenable this option by saying Y or m. Distros will typically notenable this option: Medfield devices are not available todevelopers and while Merrifield/Edison can run a mainline kernel withlimited functionality it will require a firmware file whichis not in the standard firmware tree- config SND_SST_ATOM_HIFI2_PLATFORM tristate "ACPI HiFi2 (Baytrail, Cherrytrail) Platforms" depends on X86 && ACPI
- select SND_SST_IPC_ACPI
- select SND_SST_IPC select SND_SOC_COMPRESS select SND_SOC_ACPI_INTEL_MATCH select IOSF_MBI
diff --git a/sound/soc/intel/atom/sst/Makefile b/sound/soc/intel/atom/sst/Makefile index 795d1cf8f386..f5b7008b4cea 100644 --- a/sound/soc/intel/atom/sst/Makefile +++ b/sound/soc/intel/atom/sst/Makefile @@ -1,8 +1,4 @@ # SPDX-License-Identifier: GPL-2.0 -snd-intel-sst-core-objs := sst.o sst_ipc.o sst_stream.o sst_drv_interface.o sst_loader.o sst_pvt.o -snd-intel-sst-pci-objs += sst_pci.o -snd-intel-sst-acpi-objs += sst_acpi.o +snd-intel-sst-core-objs := sst.o sst_ipc.o sst_stream.o sst_drv_interface.o sst_loader.o sst_pvt.o sst_acpi.o
obj-$(CONFIG_SND_SST_IPC) += snd-intel-sst-core.o -obj-$(CONFIG_SND_SST_IPC_PCI) += snd-intel-sst-pci.o -obj-$(CONFIG_SND_SST_IPC_ACPI) += snd-intel-sst-acpi.o diff --git a/sound/soc/intel/atom/sst/sst_pci.c b/sound/soc/intel/atom/sst/sst_pci.c deleted file mode 100644 index 6906ee624cf6..000000000000 --- a/sound/soc/intel/atom/sst/sst_pci.c +++ /dev/null @@ -1,209 +0,0 @@ -/*
- sst_pci.c - SST (LPE) driver init file for pci enumeration.
- Copyright (C) 2008-14 Intel Corp
- Authors: Vinod Koul vinod.koul@intel.com
Harsha Priya <priya.harsha@intel.com>
Dharageswari R <dharageswari.r@intel.com>
KP Jeeja <jeeja.kp@intel.com>
- This program is free software; you can redistribute it and/or modify
- it under the terms of the GNU General Public License as published by
- the Free Software Foundation; version 2 of the License.
- This program is distributed in the hope that it will be useful, but
- WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
- General Public License for more details.
- */
-#include <linux/module.h> -#include <linux/pci.h> -#include <linux/fs.h> -#include <linux/firmware.h> -#include <linux/pm_runtime.h> -#include <sound/core.h> -#include <sound/soc.h> -#include <asm/platform_sst_audio.h> -#include "../sst-mfld-platform.h" -#include "sst.h"
-static int sst_platform_get_resources(struct intel_sst_drv *ctx) -{
- int ddr_base, ret = 0;
- struct pci_dev *pci = ctx->pci;
- ret = pci_request_regions(pci, SST_DRV_NAME);
- if (ret)
return ret;- /* map registers */
- /* DDR base */
- if (ctx->dev_id == SST_MRFLD_PCI_ID) {
ctx->ddr_base = pci_resource_start(pci, 0);/* check that the relocated IMR base matches with FW Binary */ddr_base = relocate_imr_addr_mrfld(ctx->ddr_base);if (!ctx->pdata->lib_info) {dev_err(ctx->dev, "lib_info pointer NULL\n");ret = -EINVAL;goto do_release_regions;}if (ddr_base != ctx->pdata->lib_info->mod_base) {dev_err(ctx->dev,"FW LSP DDR BASE does not match with IFWI\n");ret = -EINVAL;goto do_release_regions;}ctx->ddr_end = pci_resource_end(pci, 0);ctx->ddr = pcim_iomap(pci, 0,pci_resource_len(pci, 0));if (!ctx->ddr) {ret = -EINVAL;goto do_release_regions;}dev_dbg(ctx->dev, "sst: DDR Ptr %p\n", ctx->ddr);- } else {
ctx->ddr = NULL;- }
- /* SHIM */
- ctx->shim_phy_add = pci_resource_start(pci, 1);
- ctx->shim = pcim_iomap(pci, 1, pci_resource_len(pci, 1));
- if (!ctx->shim) {
ret = -EINVAL;goto do_release_regions;- }
- dev_dbg(ctx->dev, "SST Shim Ptr %p\n", ctx->shim);
- /* Shared SRAM */
- ctx->mailbox_add = pci_resource_start(pci, 2);
- ctx->mailbox = pcim_iomap(pci, 2, pci_resource_len(pci, 2));
- if (!ctx->mailbox) {
ret = -EINVAL;goto do_release_regions;- }
- dev_dbg(ctx->dev, "SRAM Ptr %p\n", ctx->mailbox);
- /* IRAM */
- ctx->iram_end = pci_resource_end(pci, 3);
- ctx->iram_base = pci_resource_start(pci, 3);
- ctx->iram = pcim_iomap(pci, 3, pci_resource_len(pci, 3));
- if (!ctx->iram) {
ret = -EINVAL;goto do_release_regions;- }
- dev_dbg(ctx->dev, "IRAM Ptr %p\n", ctx->iram);
- /* DRAM */
- ctx->dram_end = pci_resource_end(pci, 4);
- ctx->dram_base = pci_resource_start(pci, 4);
- ctx->dram = pcim_iomap(pci, 4, pci_resource_len(pci, 4));
- if (!ctx->dram) {
ret = -EINVAL;goto do_release_regions;- }
- dev_dbg(ctx->dev, "DRAM Ptr %p\n", ctx->dram);
-do_release_regions:
- pci_release_regions(pci);
- return 0;
-}
-/*
- intel_sst_probe - PCI probe function
- @pci: PCI device structure
- @pci_id: PCI device ID structure
- */
-static int intel_sst_probe(struct pci_dev *pci,
const struct pci_device_id *pci_id)-{
- int ret = 0;
- struct intel_sst_drv *sst_drv_ctx;
- struct sst_platform_info *sst_pdata = pci->dev.platform_data;
- dev_dbg(&pci->dev, "Probe for DID %x\n", pci->device);
- ret = sst_alloc_drv_context(&sst_drv_ctx, &pci->dev, pci->device);
- if (ret < 0)
return ret;- sst_drv_ctx->pdata = sst_pdata;
- sst_drv_ctx->irq_num = pci->irq;
- snprintf(sst_drv_ctx->firmware_name, sizeof(sst_drv_ctx->firmware_name),
"%s%04x%s", "fw_sst_",sst_drv_ctx->dev_id, ".bin");- ret = sst_context_init(sst_drv_ctx);
- if (ret < 0)
return ret;- /* Init the device */
- ret = pcim_enable_device(pci);
- if (ret) {
dev_err(sst_drv_ctx->dev,"device can't be enabled. Returned err: %d\n", ret);goto do_free_drv_ctx;- }
- sst_drv_ctx->pci = pci_dev_get(pci);
- ret = sst_platform_get_resources(sst_drv_ctx);
- if (ret < 0)
goto do_free_drv_ctx;- pci_set_drvdata(pci, sst_drv_ctx);
- sst_configure_runtime_pm(sst_drv_ctx);
- return ret;
-do_free_drv_ctx:
- sst_context_cleanup(sst_drv_ctx);
- dev_err(sst_drv_ctx->dev, "Probe failed with %d\n", ret);
- return ret;
-}
-/**
- intel_sst_remove - PCI remove function
- @pci: PCI device structure
- This function is called by OS when a device is unloaded
- This frees the interrupt etc
- */
-static void intel_sst_remove(struct pci_dev *pci) -{
- struct intel_sst_drv *sst_drv_ctx = pci_get_drvdata(pci);
- sst_context_cleanup(sst_drv_ctx);
- pci_dev_put(sst_drv_ctx->pci);
- pci_release_regions(pci);
- pci_set_drvdata(pci, NULL);
-}
-/* PCI Routines */ -static const struct pci_device_id intel_sst_ids[] = {
- { PCI_VDEVICE(INTEL, SST_MRFLD_PCI_ID), 0},
- { 0, }
-};
-static struct pci_driver sst_driver = {
- .name = SST_DRV_NAME,
- .id_table = intel_sst_ids,
- .probe = intel_sst_probe,
- .remove = intel_sst_remove,
-#ifdef CONFIG_PM
- .driver = {
.pm = &intel_sst_pm,- },
-#endif -};
-module_pci_driver(sst_driver);
-MODULE_DESCRIPTION("Intel (R) SST(R) Audio Engine PCI Driver"); -MODULE_AUTHOR("Vinod Koul vinod.koul@intel.com"); -MODULE_AUTHOR("Harsha Priya priya.harsha@intel.com"); -MODULE_AUTHOR("Dharageswari R dharageswari.r@intel.com"); -MODULE_AUTHOR("KP Jeeja jeeja.kp@intel.com"); -MODULE_LICENSE("GPL v2"); -MODULE_ALIAS("sst"); diff --git a/sound/soc/intel/common/sst-acpi.c b/sound/soc/intel/common/sst-acpi.c index cf6fbbd4e378..3b669d5adbb6 100644 --- a/sound/soc/intel/common/sst-acpi.c +++ b/sound/soc/intel/common/sst-acpi.c @@ -206,7 +206,7 @@ static struct sst_acpi_desc sst_acpi_broadwell_desc = { .dma_size = SST_LPT_DSP_DMA_SIZE, };
-#if !IS_ENABLED(CONFIG_SND_SST_IPC_ACPI) +#if !IS_ENABLED(CONFIG_SND_SST_IPC) static struct sst_acpi_desc sst_acpi_baytrail_desc = { .drv_name = "baytrail-pcm-audio", .machines = snd_soc_acpi_intel_baytrail_legacy_machines, @@ -222,7 +222,7 @@ static struct sst_acpi_desc sst_acpi_baytrail_desc = { static const struct acpi_device_id sst_acpi_match[] = { { "INT33C8", (unsigned long)&sst_acpi_haswell_desc }, { "INT3438", (unsigned long)&sst_acpi_broadwell_desc }, -#if !IS_ENABLED(CONFIG_SND_SST_IPC_ACPI) +#if !IS_ENABLED(CONFIG_SND_SST_IPC) { "80860F28", (unsigned long)&sst_acpi_baytrail_desc }, #endif { }
participants (3)
-
Andy Shevchenko -
Arnd Bergmann -
Pierre-Louis Bossart