Re: [alsa-devel] [PATCH] ASoC: intel: clean up CONFIG_SND_SST_IPC handling

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, 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 { }