Re: [alsa-devel] [PATCH] ASoC: intel: Fix sst-dsp dependency on dw stuff

On 2016年07月11日 16:39, Takashi Iwai wrote:

The recent commit [a92ea59b74e2: ASoC: Intel: sst: only select sst-firmware when DW DMAC is built-in] introduced more strict kconfig dependency (depends on DW_DMAC_CORE=y) for avoiding the build failures due to dependency messes in intel-sst. This makes, however, it impossible to use this driver with the modularized systems, i.e. typically on Linux distros.

The problem addressed in the commit above is that sst_dsp_new() and sst_dsp_free() includes the firmware init / finish that call dw_*() functions. Thus building it as built-in with DW_DMAC_CORE module results in the missing symbols.

However, these sst_dsp functions are basically called only from the drivers that depend on DW_DMAC_CORE already. That is, once when these functions are split out, the rest can be independent from dw stuff.

This patch attempts to solve the issue by the following:

• Split sst-dsp stuff into two modules: snd-soc-sst-dsp and snd-soc-sst-firmware.
• Move sst_dsp_new() and sst_dsp_free() to the latter module so that the former module can be independent from DW_DMAC_CORE.
• Add a new kconfig SND_SOC_INTEL_SST_FIRMWARE to select the latter module by machine drivers.

One only remaining pitfall is that each machine driver has to select SND_SOC_INTEL_SST_FIRMWARE carefully depending on DW_DMAC_CORE. This can't be done cleanly due to the restriction of the current kbuild.

is it good to let SND_SOC_INTEL_SST_FIRMWARE select DW_DMAC_CORE in your opinion? That may fix this pitfall, but I am not sure if it is comply with convention -- a module should not select another upper layer one?

Bugzilla: https://bugzilla.opensuse.org/show_bug.cgi?id=988117 Fixes: a92ea59b74e2 ('ASoC: Intel: sst: only select sst-firmware when DW DMAC is built-in') Signed-off-by: Takashi Iwai tiwai@suse.de

Acked-by: Jie Yang yang.jie@intel.com

thanks, ~Keyon

---

sound/soc/intel/Kconfig | 18 +++++++--- sound/soc/intel/common/Makefile | 4 +-- sound/soc/intel/common/sst-dsp-priv.h | 4 --- sound/soc/intel/common/sst-dsp.c | 67 ---------------------------------- sound/soc/intel/common/sst-dsp.h | 2 +- sound/soc/intel/common/sst-firmware.c | 68 +++++++++++++++++++++++++++++++++++ 6 files changed, 85 insertions(+), 78 deletions(-)

diff --git a/sound/soc/intel/Kconfig b/sound/soc/intel/Kconfig index 9c86459d0fc3..a20c3dfbcb5d 100644 --- a/sound/soc/intel/Kconfig +++ b/sound/soc/intel/Kconfig @@ -32,6 +32,12 @@ config SND_SOC_INTEL_SST select SND_SOC_INTEL_SST_MATCH if ACPI depends on (X86 || COMPILE_TEST)

+# firmware stuff depends DW_DMAC_CORE; since there is no depends-on from +# the reverse selection, each machine driver needs to select +# SND_SOC_INTEL_SST_FIRMWARE carefully depending on DW_DMAC_CORE +config SND_SOC_INTEL_SST_FIRMWARE

• tristate
• config SND_SOC_INTEL_SST_ACPI tristate

@@ -47,8 +53,9 @@ config SND_SOC_INTEL_BAYTRAIL config SND_SOC_INTEL_HASWELL_MACH tristate "ASoC Audio DSP support for Intel Haswell Lynxpoint" depends on X86_INTEL_LPSS && I2C && I2C_DESIGNWARE_PLATFORM

• depends on DW_DMAC_CORE=y

• depends on DW_DMAC_CORE select SND_SOC_INTEL_SST
• select SND_SOC_INTEL_SST_FIRMWARE select SND_SOC_INTEL_HASWELL select SND_SOC_RT5640 help

@@ -91,8 +98,9 @@ config SND_SOC_INTEL_BXT_RT298_MACH config SND_SOC_INTEL_BYT_RT5640_MACH tristate "ASoC Audio driver for Intel Baytrail with RT5640 codec" depends on X86_INTEL_LPSS && I2C

• depends on DW_DMAC_CORE=y && (SND_SST_IPC_ACPI = n)

• depends on DW_DMAC_CORE && (SND_SST_IPC_ACPI = n) select SND_SOC_INTEL_SST
• select SND_SOC_INTEL_SST_FIRMWARE select SND_SOC_INTEL_BAYTRAIL select SND_SOC_RT5640 help

@@ -103,8 +111,9 @@ config SND_SOC_INTEL_BYT_RT5640_MACH config SND_SOC_INTEL_BYT_MAX98090_MACH tristate "ASoC Audio driver for Intel Baytrail with MAX98090 codec" depends on X86_INTEL_LPSS && I2C

• depends on DW_DMAC_CORE=y && (SND_SST_IPC_ACPI = n)

• depends on DW_DMAC_CORE && (SND_SST_IPC_ACPI = n) select SND_SOC_INTEL_SST
• select SND_SOC_INTEL_SST_FIRMWARE select SND_SOC_INTEL_BAYTRAIL select SND_SOC_MAX98090 help

@@ -115,8 +124,9 @@ config SND_SOC_INTEL_BROADWELL_MACH tristate "ASoC Audio DSP support for Intel Broadwell Wildcatpoint" depends on X86_INTEL_LPSS && I2C && DW_DMAC && \ I2C_DESIGNWARE_PLATFORM

• depends on DW_DMAC_CORE=y

• depends on DW_DMAC_CORE select SND_SOC_INTEL_SST
• select SND_SOC_INTEL_SST_FIRMWARE select SND_SOC_INTEL_HASWELL select SND_SOC_RT286 help

diff --git a/sound/soc/intel/common/Makefile b/sound/soc/intel/common/Makefile index fbbb25c2ceed..1a35149bcad7 100644 --- a/sound/soc/intel/common/Makefile +++ b/sound/soc/intel/common/Makefile @@ -2,9 +2,9 @@ snd-soc-sst-dsp-objs := sst-dsp.o snd-soc-sst-acpi-objs := sst-acpi.o snd-soc-sst-match-objs := sst-match-acpi.o snd-soc-sst-ipc-objs := sst-ipc.o

-snd-soc-sst-dsp-$(CONFIG_DW_DMAC_CORE) += sst-firmware.o +snd-soc-sst-firmware-objs := sst-firmware.o

obj-$(CONFIG_SND_SOC_INTEL_SST) += snd-soc-sst-dsp.o snd-soc-sst-ipc.o obj-$(CONFIG_SND_SOC_INTEL_SST_ACPI) += snd-soc-sst-acpi.o obj-$(CONFIG_SND_SOC_INTEL_SST_MATCH) += snd-soc-sst-match.o +obj-$(CONFIG_SND_SOC_INTEL_SST_FIRMWARE) += snd-soc-sst-firmware.o diff --git a/sound/soc/intel/common/sst-dsp-priv.h b/sound/soc/intel/common/sst-dsp-priv.h index 97dc1ae05e69..d13c84364c3c 100644 --- a/sound/soc/intel/common/sst-dsp-priv.h +++ b/sound/soc/intel/common/sst-dsp-priv.h @@ -383,10 +383,6 @@ struct sst_mem_block *sst_mem_block_register(struct sst_dsp *dsp, u32 offset, u32 index, void *private); void sst_mem_block_unregister_all(struct sst_dsp *dsp);

-/* Create/Free DMA resources */ -int sst_dma_new(struct sst_dsp *sst); -void sst_dma_free(struct sst_dma *dma);

• u32 sst_dsp_get_offset(struct sst_dsp *dsp, u32 offset, enum sst_mem_type type); #endif

diff --git a/sound/soc/intel/common/sst-dsp.c b/sound/soc/intel/common/sst-dsp.c index ff2196ef359f..c00ede4ea4d7 100644 --- a/sound/soc/intel/common/sst-dsp.c +++ b/sound/soc/intel/common/sst-dsp.c @@ -420,73 +420,6 @@ void sst_dsp_inbox_read(struct sst_dsp *sst, void *message, size_t bytes) } EXPORT_SYMBOL_GPL(sst_dsp_inbox_read);

-#ifdef CONFIG_DW_DMAC_CORE -struct sst_dsp *sst_dsp_new(struct device *dev,

• struct sst_dsp_device *sst_dev, struct sst_pdata *pdata)

-{

• struct sst_dsp *sst;
• int err;
• dev_dbg(dev, "initialising audio DSP id 0x%x\n", pdata->id);
• sst = devm_kzalloc(dev, sizeof(*sst), GFP_KERNEL);
• if (sst == NULL)

• return NULL;
  

• spin_lock_init(&sst->spinlock);
• mutex_init(&sst->mutex);
• sst->dev = dev;
• sst->dma_dev = pdata->dma_dev;
• sst->thread_context = sst_dev->thread_context;
• sst->sst_dev = sst_dev;
• sst->id = pdata->id;
• sst->irq = pdata->irq;
• sst->ops = sst_dev->ops;
• sst->pdata = pdata;
• INIT_LIST_HEAD(&sst->used_block_list);
• INIT_LIST_HEAD(&sst->free_block_list);
• INIT_LIST_HEAD(&sst->module_list);
• INIT_LIST_HEAD(&sst->fw_list);
• INIT_LIST_HEAD(&sst->scratch_block_list);
• /* Initialise SST Audio DSP */
• if (sst->ops->init) {

• err = sst->ops->init(sst, pdata);
  

• if (err < 0)
  

• 	return NULL;
  

• }
• /* Register the ISR */
• err = request_threaded_irq(sst->irq, sst->ops->irq_handler,

• sst_dev->thread, IRQF_SHARED, "AudioDSP", sst);
  

• if (err)

• goto irq_err;
  

• err = sst_dma_new(sst);
• if (err)

• dev_warn(dev, "sst_dma_new failed %d\n", err);
  

• return sst;

-irq_err:

• if (sst->ops->free)

• sst->ops->free(sst);
  

• return NULL;

-} -EXPORT_SYMBOL_GPL(sst_dsp_new);

-void sst_dsp_free(struct sst_dsp *sst) -{

• free_irq(sst->irq, sst);
• if (sst->ops->free)

• sst->ops->free(sst);
  

• sst_dma_free(sst->dma);

-} -EXPORT_SYMBOL_GPL(sst_dsp_free); -#endif

• /* Module information */ MODULE_AUTHOR("Liam Girdwood"); MODULE_DESCRIPTION("Intel SST Core");

diff --git a/sound/soc/intel/common/sst-dsp.h b/sound/soc/intel/common/sst-dsp.h index 0b84c719ec48..859f0de00339 100644 --- a/sound/soc/intel/common/sst-dsp.h +++ b/sound/soc/intel/common/sst-dsp.h @@ -216,7 +216,7 @@ struct sst_pdata { void *dsp; };

-#ifdef CONFIG_DW_DMAC_CORE +#if IS_ENABLED(CONFIG_DW_DMAC_CORE) /* Initialization */ struct sst_dsp *sst_dsp_new(struct device *dev, struct sst_dsp_device *sst_dev, struct sst_pdata *pdata); diff --git a/sound/soc/intel/common/sst-firmware.c b/sound/soc/intel/common/sst-firmware.c index 25993527370b..a086c35f91bb 100644 --- a/sound/soc/intel/common/sst-firmware.c +++ b/sound/soc/intel/common/sst-firmware.c @@ -1211,3 +1211,71 @@ u32 sst_dsp_get_offset(struct sst_dsp *dsp, u32 offset, } } EXPORT_SYMBOL_GPL(sst_dsp_get_offset);

+struct sst_dsp *sst_dsp_new(struct device *dev,

• struct sst_dsp_device *sst_dev, struct sst_pdata *pdata)

+{

• struct sst_dsp *sst;
• int err;
• dev_dbg(dev, "initialising audio DSP id 0x%x\n", pdata->id);
• sst = devm_kzalloc(dev, sizeof(*sst), GFP_KERNEL);
• if (sst == NULL)

• return NULL;
  

• spin_lock_init(&sst->spinlock);
• mutex_init(&sst->mutex);
• sst->dev = dev;
• sst->dma_dev = pdata->dma_dev;
• sst->thread_context = sst_dev->thread_context;
• sst->sst_dev = sst_dev;
• sst->id = pdata->id;
• sst->irq = pdata->irq;
• sst->ops = sst_dev->ops;
• sst->pdata = pdata;
• INIT_LIST_HEAD(&sst->used_block_list);
• INIT_LIST_HEAD(&sst->free_block_list);
• INIT_LIST_HEAD(&sst->module_list);
• INIT_LIST_HEAD(&sst->fw_list);
• INIT_LIST_HEAD(&sst->scratch_block_list);
• /* Initialise SST Audio DSP */
• if (sst->ops->init) {

• err = sst->ops->init(sst, pdata);
  

• if (err < 0)
  

• 	return NULL;
  

• }
• /* Register the ISR */
• err = request_threaded_irq(sst->irq, sst->ops->irq_handler,

• sst_dev->thread, IRQF_SHARED, "AudioDSP", sst);
  

• if (err)

• goto irq_err;
  

• err = sst_dma_new(sst);
• if (err)

• dev_warn(dev, "sst_dma_new failed %d\n", err);
  

• return sst;

+irq_err:

• if (sst->ops->free)

• sst->ops->free(sst);
  

• return NULL;

+} +EXPORT_SYMBOL_GPL(sst_dsp_new);

+void sst_dsp_free(struct sst_dsp *sst) +{

• free_irq(sst->irq, sst);
• if (sst->ops->free)

• sst->ops->free(sst);
  

• sst_dma_free(sst->dma);

+} +EXPORT_SYMBOL_GPL(sst_dsp_free);

+MODULE_DESCRIPTION("Intel SST Firmware Loader"); +MODULE_LICENSE("GPL v2");