[PATCH] ALSA: hda: fix SND_INTEL_DSP_CONFIG dependency
From: Arnd Bergmann arnd@arndb.de
The sof-pci-dev driver fails to link when built into the kernel and CONFIG_SND_INTEL_DSP_CONFIG is set to =m:
arm-linux-gnueabi-ld: sound/soc/sof/sof-pci-dev.o: in function `sof_pci_probe': sof-pci-dev.c:(.text+0x1c): undefined reference to `snd_intel_dsp_driver_probe'
All other drivers using this interface already use a 'select SND_INTEL_DSP_CONFIG' statement to force the it to be present, so it seems reasonable to do the same here.
The stub implementation in the header makes the problem harder to find, as it avoids the link error when SND_INTEL_DSP_CONFIG is completely disabled, without any obvious upsides. Remove these stubs to make it clearer that the driver is in fact needed here.
Fixes: 82d9d54a6c0e ("ALSA: hda: add Intel DSP configuration / probe code") Signed-off-by: Arnd Bergmann arnd@arndb.de --- include/sound/intel-dsp-config.h | 17 ----------------- sound/soc/sof/Kconfig | 2 ++ 2 files changed, 2 insertions(+), 17 deletions(-)
diff --git a/include/sound/intel-dsp-config.h b/include/sound/intel-dsp-config.h index d4609077c258..94667e870029 100644 --- a/include/sound/intel-dsp-config.h +++ b/include/sound/intel-dsp-config.h @@ -18,24 +18,7 @@ enum { SND_INTEL_DSP_DRIVER_LAST = SND_INTEL_DSP_DRIVER_SOF };
-#if IS_ENABLED(CONFIG_SND_INTEL_DSP_CONFIG) - int snd_intel_dsp_driver_probe(struct pci_dev *pci); int snd_intel_acpi_dsp_driver_probe(struct device *dev, const u8 acpi_hid[ACPI_ID_LEN]);
-#else - -static inline int snd_intel_dsp_driver_probe(struct pci_dev *pci) -{ - return SND_INTEL_DSP_DRIVER_ANY; -} - -static inline -int snd_intel_acpi_dsp_driver_probe(struct device *dev, const u8 acpi_hid[ACPI_ID_LEN]) -{ - return SND_INTEL_DSP_DRIVER_ANY; -} - -#endif - #endif diff --git a/sound/soc/sof/Kconfig b/sound/soc/sof/Kconfig index 031dad5fc4c7..051fd3d27047 100644 --- a/sound/soc/sof/Kconfig +++ b/sound/soc/sof/Kconfig @@ -12,6 +12,7 @@ if SND_SOC_SOF_TOPLEVEL config SND_SOC_SOF_PCI tristate "SOF PCI enumeration support" depends on PCI + select SND_INTEL_DSP_CONFIG select SND_SOC_SOF select SND_SOC_ACPI if ACPI help @@ -23,6 +24,7 @@ config SND_SOC_SOF_PCI config SND_SOC_SOF_ACPI tristate "SOF ACPI enumeration support" depends on ACPI || COMPILE_TEST + select SND_INTEL_DSP_CONFIG select SND_SOC_SOF select SND_SOC_ACPI if ACPI select IOSF_MBI if X86 && PCI
On Sun, 03 Jan 2021 14:52:32 +0100, Arnd Bergmann wrote:
From: Arnd Bergmann arnd@arndb.de
The sof-pci-dev driver fails to link when built into the kernel and CONFIG_SND_INTEL_DSP_CONFIG is set to =m:
arm-linux-gnueabi-ld: sound/soc/sof/sof-pci-dev.o: in function `sof_pci_probe': sof-pci-dev.c:(.text+0x1c): undefined reference to `snd_intel_dsp_driver_probe'
All other drivers using this interface already use a 'select SND_INTEL_DSP_CONFIG' statement to force the it to be present, so it seems reasonable to do the same here.
The stub implementation in the header makes the problem harder to find, as it avoids the link error when SND_INTEL_DSP_CONFIG is completely disabled, without any obvious upsides. Remove these stubs to make it clearer that the driver is in fact needed here.
Fixes: 82d9d54a6c0e ("ALSA: hda: add Intel DSP configuration / probe code") Signed-off-by: Arnd Bergmann arnd@arndb.de
IMO, the problem is rather the unconditional calls of snd_intel_dsp_driver_probe() in snd-soc-sof-pci and snd-soc-sof-acpi drivers. Those calls should be done only if Intel drivers are involved. So, wrapping the call with ifdef CONFIG_SND_SOC_SOF_INTEL_PCI or CONFIG_SND_SOC_SOF_INTEL_ACPI would work better (although those are a bit uglier).
thanks,
Takashi
include/sound/intel-dsp-config.h | 17 ----------------- sound/soc/sof/Kconfig | 2 ++ 2 files changed, 2 insertions(+), 17 deletions(-)
diff --git a/include/sound/intel-dsp-config.h b/include/sound/intel-dsp-config.h index d4609077c258..94667e870029 100644 --- a/include/sound/intel-dsp-config.h +++ b/include/sound/intel-dsp-config.h @@ -18,24 +18,7 @@ enum { SND_INTEL_DSP_DRIVER_LAST = SND_INTEL_DSP_DRIVER_SOF };
-#if IS_ENABLED(CONFIG_SND_INTEL_DSP_CONFIG)
int snd_intel_dsp_driver_probe(struct pci_dev *pci); int snd_intel_acpi_dsp_driver_probe(struct device *dev, const u8 acpi_hid[ACPI_ID_LEN]);
-#else
-static inline int snd_intel_dsp_driver_probe(struct pci_dev *pci) -{
- return SND_INTEL_DSP_DRIVER_ANY;
-}
-static inline -int snd_intel_acpi_dsp_driver_probe(struct device *dev, const u8 acpi_hid[ACPI_ID_LEN]) -{
- return SND_INTEL_DSP_DRIVER_ANY;
-}
-#endif
#endif diff --git a/sound/soc/sof/Kconfig b/sound/soc/sof/Kconfig index 031dad5fc4c7..051fd3d27047 100644 --- a/sound/soc/sof/Kconfig +++ b/sound/soc/sof/Kconfig @@ -12,6 +12,7 @@ if SND_SOC_SOF_TOPLEVEL config SND_SOC_SOF_PCI tristate "SOF PCI enumeration support" depends on PCI
- select SND_INTEL_DSP_CONFIG select SND_SOC_SOF select SND_SOC_ACPI if ACPI help
@@ -23,6 +24,7 @@ config SND_SOC_SOF_PCI config SND_SOC_SOF_ACPI tristate "SOF ACPI enumeration support" depends on ACPI || COMPILE_TEST
- select SND_INTEL_DSP_CONFIG select SND_SOC_SOF select SND_SOC_ACPI if ACPI select IOSF_MBI if X86 && PCI
-- 2.29.2
On Mon, Jan 04, 2021 at 03:09:24PM +0100, Takashi Iwai wrote:
IMO, the problem is rather the unconditional calls of snd_intel_dsp_driver_probe() in snd-soc-sof-pci and snd-soc-sof-acpi drivers. Those calls should be done only if Intel drivers are involved. So, wrapping the call with ifdef CONFIG_SND_SOC_SOF_INTEL_PCI or CONFIG_SND_SOC_SOF_INTEL_ACPI would work better (although those are a bit uglier).
Or stubbing them which might be neater.
Dne 03. 01. 21 v 14:52 Arnd Bergmann napsal(a):
From: Arnd Bergmann arnd@arndb.de
The sof-pci-dev driver fails to link when built into the kernel and CONFIG_SND_INTEL_DSP_CONFIG is set to =m:
arm-linux-gnueabi-ld: sound/soc/sof/sof-pci-dev.o: in function `sof_pci_probe': sof-pci-dev.c:(.text+0x1c): undefined reference to `snd_intel_dsp_driver_probe'
All other drivers using this interface already use a 'select SND_INTEL_DSP_CONFIG' statement to force the it to be present, so it seems reasonable to do the same here.
The stub implementation in the header makes the problem harder to find, as it avoids the link error when SND_INTEL_DSP_CONFIG is completely disabled, without any obvious upsides. Remove these stubs to make it clearer that the driver is in fact needed here.
Fixes: 82d9d54a6c0e ("ALSA: hda: add Intel DSP configuration / probe code") Signed-off-by: Arnd Bergmann arnd@arndb.de
include/sound/intel-dsp-config.h | 17 ----------------- sound/soc/sof/Kconfig | 2 ++ 2 files changed, 2 insertions(+), 17 deletions(-)
diff --git a/include/sound/intel-dsp-config.h b/include/sound/intel-dsp-config.h index d4609077c258..94667e870029 100644 --- a/include/sound/intel-dsp-config.h +++ b/include/sound/intel-dsp-config.h @@ -18,24 +18,7 @@ enum { SND_INTEL_DSP_DRIVER_LAST = SND_INTEL_DSP_DRIVER_SOF };
-#if IS_ENABLED(CONFIG_SND_INTEL_DSP_CONFIG)
The SOF drivers selects the DSP config code only when required (for specific platforms - see sound/soc/sof/intel/Kconfig).
It seems that the above if should be modified as:
#if IS_BUILDIN(CONFIG_SND_INTEL_DSP_CONFIG) || (defined(MODULE) && IS_MODULE(CONFIG_SND_INTEL_DSP_CONFIG))
So the buildin drivers which do not require the DSP config probe can be compiled without this dependency.
Jaroslav
On Mon, 04 Jan 2021 16:00:05 +0100, Jaroslav Kysela wrote:
Dne 03. 01. 21 v 14:52 Arnd Bergmann napsal(a):
From: Arnd Bergmann arnd@arndb.de
The sof-pci-dev driver fails to link when built into the kernel and CONFIG_SND_INTEL_DSP_CONFIG is set to =m:
arm-linux-gnueabi-ld: sound/soc/sof/sof-pci-dev.o: in function `sof_pci_probe': sof-pci-dev.c:(.text+0x1c): undefined reference to `snd_intel_dsp_driver_probe'
All other drivers using this interface already use a 'select SND_INTEL_DSP_CONFIG' statement to force the it to be present, so it seems reasonable to do the same here.
The stub implementation in the header makes the problem harder to find, as it avoids the link error when SND_INTEL_DSP_CONFIG is completely disabled, without any obvious upsides. Remove these stubs to make it clearer that the driver is in fact needed here.
Fixes: 82d9d54a6c0e ("ALSA: hda: add Intel DSP configuration / probe code") Signed-off-by: Arnd Bergmann arnd@arndb.de
include/sound/intel-dsp-config.h | 17 ----------------- sound/soc/sof/Kconfig | 2 ++ 2 files changed, 2 insertions(+), 17 deletions(-)
diff --git a/include/sound/intel-dsp-config.h b/include/sound/intel-dsp-config.h index d4609077c258..94667e870029 100644 --- a/include/sound/intel-dsp-config.h +++ b/include/sound/intel-dsp-config.h @@ -18,24 +18,7 @@ enum { SND_INTEL_DSP_DRIVER_LAST = SND_INTEL_DSP_DRIVER_SOF };
-#if IS_ENABLED(CONFIG_SND_INTEL_DSP_CONFIG)
The SOF drivers selects the DSP config code only when required (for specific platforms - see sound/soc/sof/intel/Kconfig).
It seems that the above if should be modified as:
#if IS_BUILDIN(CONFIG_SND_INTEL_DSP_CONFIG) || (defined(MODULE) && IS_MODULE(CONFIG_SND_INTEL_DSP_CONFIG))
So the buildin drivers which do not require the DSP config probe can be compiled without this dependency.
As I wrote in another post, a part of the problem is that SOF PCI and ACPI drivers call snd_intel_dsp_driver_probe() unconditionally, even if no Intel driver is bound. So even if changing like the above (or better to use IS_REACHABLE() macro) works around the issue, the call pattern needs to be reconsidered.
thanks,
Takashi
On Mon, Jan 4, 2021 at 4:05 PM Takashi Iwai tiwai@suse.de wrote:
On Mon, 04 Jan 2021 16:00:05 +0100, Jaroslav Kysela wrote:
Dne 03. 01. 21 v 14:52 Arnd Bergmann napsal(a):
From: Arnd Bergmann arnd@arndb.de
The sof-pci-dev driver fails to link when built into the kernel and CONFIG_SND_INTEL_DSP_CONFIG is set to =m:
arm-linux-gnueabi-ld: sound/soc/sof/sof-pci-dev.o: in function `sof_pci_probe': sof-pci-dev.c:(.text+0x1c): undefined reference to `snd_intel_dsp_driver_probe'
All other drivers using this interface already use a 'select SND_INTEL_DSP_CONFIG' statement to force the it to be present, so it seems reasonable to do the same here.
The stub implementation in the header makes the problem harder to find, as it avoids the link error when SND_INTEL_DSP_CONFIG is completely disabled, without any obvious upsides. Remove these stubs to make it clearer that the driver is in fact needed here.
Fixes: 82d9d54a6c0e ("ALSA: hda: add Intel DSP configuration / probe code") Signed-off-by: Arnd Bergmann arnd@arndb.de
include/sound/intel-dsp-config.h | 17 ----------------- sound/soc/sof/Kconfig | 2 ++ 2 files changed, 2 insertions(+), 17 deletions(-)
diff --git a/include/sound/intel-dsp-config.h b/include/sound/intel-dsp-config.h index d4609077c258..94667e870029 100644 --- a/include/sound/intel-dsp-config.h +++ b/include/sound/intel-dsp-config.h @@ -18,24 +18,7 @@ enum { SND_INTEL_DSP_DRIVER_LAST = SND_INTEL_DSP_DRIVER_SOF };
-#if IS_ENABLED(CONFIG_SND_INTEL_DSP_CONFIG)
The SOF drivers selects the DSP config code only when required (for specific platforms - see sound/soc/sof/intel/Kconfig).
It seems that the above if should be modified as:
#if IS_BUILDIN(CONFIG_SND_INTEL_DSP_CONFIG) || (defined(MODULE) && IS_MODULE(CONFIG_SND_INTEL_DSP_CONFIG))
So the buildin drivers which do not require the DSP config probe can be compiled without this dependency.
As I wrote in another post, a part of the problem is that SOF PCI and ACPI drivers call snd_intel_dsp_driver_probe() unconditionally, even if no Intel driver is bound.
Makes sense. Is there an existing Kconfig that could be used to decide whether the drivers use SND_INTEL_DSP_CONFIG or not? Could it be part of the device specific driver_data?
According to sof_pci_ids[], all PCI IDs are Intel specific, but I can't tell which ones need the DSP config.
So even if changing like the above (or better to use IS_REACHABLE() macro) works around the issue, the call pattern needs to be reconsidered.
If the callers are fixed to address this, then I would expect the IS_REACHABLE() or IS_ENABLED() to no longer be needed either.
Arnd
Hey,
On Tue, 5 Jan 2021, Arnd Bergmann wrote:
On Mon, Jan 4, 2021 at 4:05 PM Takashi Iwai tiwai@suse.de wrote:
As I wrote in another post, a part of the problem is that SOF PCI and ACPI drivers call snd_intel_dsp_driver_probe() unconditionally, even if no Intel driver is bound.
Makes sense. Is there an existing Kconfig that could be used to decide whether the drivers use SND_INTEL_DSP_CONFIG or not?
no, unfortunately not. This is selected per platform in sound/soc/sof/intel/Kconfig. CONFIG_SND_SOC_SOF_INTEL_PCI is close, but there is at least one platform that does not use SND_INTEL_DSP_CONFIG.
According to sof_pci_ids[], all PCI IDs are Intel specific, but I can't tell which ones need the DSP config.
Indeed currently all the ids are Intel ones (and with exception of old Merrifield, all use DSP config). But that's just how it is now.
Could it be part of the device specific driver_data?
This would certainly be a clean way and allow to remove the Intel-specific calls from sof_pci_probe(). As a short-term solution, IS_REACHABLE() seems ok as well.
Br, Kai
On Tue, Jan 5, 2021 at 4:39 PM Kai Vehmanen kai.vehmanen@linux.intel.com wrote:
Could it be part of the device specific driver_data?
This would certainly be a clean way and allow to remove the Intel-specific calls from sof_pci_probe(). As a short-term solution, IS_REACHABLE() seems ok as well.
I looked at it some more and my conclusion is that the problem is the way the drivers mix device specific and generic data: The generic acpi or pci driver should never need to know about individual devices and their dependencies. Instead of just exporting some generic helper functions, these are the top-level drivers and the device specific drivers are the ones exporting the data.
It's a common mistake, but it always leads to complexity like this and we tend to end up having to undo it all.
I prototyped a patch to do this for the acpi driver, and it seems much more straightforward this way, please have a look.
commit a83ecfed5b31dfc862e04c9bf77d2107a1047c9b Author: Arnd Bergmann arnd@arndb.de Date: Tue Jan 5 19:47:35 2021 +0100
ASoC: SOF: Intel: avoid reverse module dependency
The SOF-ACPI driver is backwards from the normal Linux model, it has a generic driver that knows about all the specific drivers, as opposed to having hardware specific drivers that link against a common framework.
This requires ugly Kconfig magic and leads to missed dependencies as seen in this link error:
arm-linux-gnueabi-ld: sound/soc/sof/sof-pci-dev.o: in function `sof_acpi_probe': sof-pci-dev.c:(.text+0x1c): undefined reference to `snd_intel_dsp_driver_probe'
Change it to use the normal probe order of starting with a specific device in a driver, turning the sof-acpi-dev.c driver into a library.
Signed-off-by: Arnd Bergmann arnd@arndb.de
sound/soc/sof/intel/Kconfig | 34 +++--------- sound/soc/sof/intel/bdw.c | 51 ++++++++++++++++-- sound/soc/sof/intel/byt.c | 104 ++++++++++++++++++++++++++++++++---- sound/soc/sof/intel/shim.h | 10 ++-- sound/soc/sof/sof-acpi-dev.c | 122 ++----------------------------------------- 5 files changed, 156 insertions(+), 165 deletions(-)
The PCI driver is left as an exercise to the reader.
Arnd
From: Arnd Bergmann arnd@arndb.de
The SOF-ACPI driver is backwards from the normal Linux model, it has a generic driver that knows about all the specific drivers, as opposed to having hardware specific drivers that link against a common framework.
This requires ugly Kconfig magic and leads to missed dependencies as seen in this link error:
arm-linux-gnueabi-ld: sound/soc/sof/sof-pci-dev.o: in function `sof_acpi_probe': sof-pci-dev.c:(.text+0x1c): undefined reference to `snd_intel_dsp_driver_probe'
Change it to use the normal probe order of starting with a specific device in a driver, turning the sof-acpi-dev.c driver into a library.
Signed-off-by: Arnd Bergmann arnd@arndb.de --- sound/soc/sof/intel/Kconfig | 34 ++-------- sound/soc/sof/intel/bdw.c | 51 +++++++++++++-- sound/soc/sof/intel/byt.c | 104 +++++++++++++++++++++++++---- sound/soc/sof/intel/shim.h | 10 ++- sound/soc/sof/sof-acpi-dev.c | 122 ++--------------------------------- 5 files changed, 156 insertions(+), 165 deletions(-)
diff --git a/sound/soc/sof/intel/Kconfig b/sound/soc/sof/intel/Kconfig index ae9ba834814e..ff9266413a06 100644 --- a/sound/soc/sof/intel/Kconfig +++ b/sound/soc/sof/intel/Kconfig @@ -9,14 +9,6 @@ config SND_SOC_SOF_INTEL_TOPLEVEL
if SND_SOC_SOF_INTEL_TOPLEVEL
-config SND_SOC_SOF_INTEL_ACPI - def_tristate SND_SOC_SOF_ACPI - select SND_SOC_SOF_BAYTRAIL if SND_SOC_SOF_BAYTRAIL_SUPPORT - select SND_SOC_SOF_BROADWELL if SND_SOC_SOF_BROADWELL_SUPPORT - help - This option is not user-selectable but automagically handled by - 'select' statements at a higher level. - config SND_SOC_SOF_INTEL_PCI def_tristate SND_SOC_SOF_PCI select SND_SOC_SOF_MERRIFIELD if SND_SOC_SOF_MERRIFIELD_SUPPORT @@ -58,10 +50,12 @@ config SND_SOC_SOF_INTEL_COMMON This option is not user-selectable but automagically handled by 'select' statements at a higher level.
-if SND_SOC_SOF_INTEL_ACPI +if SND_SOC_SOF_ACPI
-config SND_SOC_SOF_BAYTRAIL_SUPPORT +config SND_SOC_SOF_BAYTRAIL bool "SOF support for Baytrail, Braswell and Cherrytrail" + select SND_SOC_SOF_INTEL_ATOM_HIFI_EP + select SND_INTEL_DSP_CONFIG help This adds support for Sound Open Firmware for Intel(R) platforms using the Baytrail, Braswell or Cherrytrail processors. @@ -75,17 +69,11 @@ config SND_SOC_SOF_BAYTRAIL_SUPPORT Say Y if you want to enable SOF on Baytrail/Cherrytrail. If unsure select "N".
-config SND_SOC_SOF_BAYTRAIL - tristate - select SND_SOC_SOF_INTEL_ATOM_HIFI_EP - select SND_INTEL_DSP_CONFIG - help - This option is not user-selectable but automagically handled by - 'select' statements at a higher level. - -config SND_SOC_SOF_BROADWELL_SUPPORT +config SND_SOC_SOF_BROADWELL bool "SOF support for Broadwell" select SND_INTEL_DSP_CONFIG + select SND_SOC_SOF_INTEL_COMMON + select SND_SOC_SOF_INTEL_HIFI_EP_IPC help This adds support for Sound Open Firmware for Intel(R) platforms using the Broadwell processors. @@ -100,14 +88,6 @@ config SND_SOC_SOF_BROADWELL_SUPPORT Say Y if you want to enable SOF on Broadwell. If unsure select "N".
-config SND_SOC_SOF_BROADWELL - tristate - select SND_SOC_SOF_INTEL_COMMON - select SND_SOC_SOF_INTEL_HIFI_EP_IPC - help - This option is not user-selectable but automagically handled by - 'select' statements at a higher level. - endif ## SND_SOC_SOF_INTEL_ACPI
if SND_SOC_SOF_INTEL_PCI diff --git a/sound/soc/sof/intel/bdw.c b/sound/soc/sof/intel/bdw.c index 50a4a73e6b9f..542e885733a2 100644 --- a/sound/soc/sof/intel/bdw.c +++ b/sound/soc/sof/intel/bdw.c @@ -15,6 +15,8 @@ #include <linux/module.h> #include <sound/sof.h> #include <sound/sof/xtensa.h> +#include <sound/soc-acpi.h> +#include <sound/soc-acpi-intel-match.h> #include "../ops.h" #include "shim.h" #include "../sof-audio.h" @@ -590,7 +592,7 @@ static struct snd_soc_dai_driver bdw_dai[] = { };
/* broadwell ops */ -const struct snd_sof_dsp_ops sof_bdw_ops = { +static const struct snd_sof_dsp_ops sof_bdw_ops = { /*Device init */ .probe = bdw_probe,
@@ -651,13 +653,54 @@ const struct snd_sof_dsp_ops sof_bdw_ops = {
.arch_ops = &sof_xtensa_arch_ops, }; -EXPORT_SYMBOL_NS(sof_bdw_ops, SND_SOC_SOF_BROADWELL);
-const struct sof_intel_dsp_desc bdw_chip_info = { +static const struct sof_intel_dsp_desc bdw_chip_info = { .cores_num = 1, .host_managed_cores_mask = 1, }; -EXPORT_SYMBOL_NS(bdw_chip_info, SND_SOC_SOF_BROADWELL); + +static const struct sof_dev_desc sof_acpi_broadwell_desc = { + .machines = snd_soc_acpi_intel_broadwell_machines, + .resindex_lpe_base = 0, + .resindex_pcicfg_base = 1, + .resindex_imr_base = -1, + .irqindex_host_ipc = 0, + .chip_info = &bdw_chip_info, + .default_fw_path = "intel/sof", + .default_tplg_path = "intel/sof-tplg", + .default_fw_filename = "sof-bdw.ri", + .nocodec_tplg_filename = "sof-bdw-nocodec.tplg", + .ops = &sof_bdw_ops, +}; + +static const struct acpi_device_id sof_broadwell_match[] = { + { "INT3438", (unsigned long)&sof_acpi_broadwell_desc }, + { } +}; +MODULE_DEVICE_TABLE(acpi, sof_broadwell_match); + +static int sof_broadwell_probe(struct platform_device *dev) +{ + int ret = snd_intel_acpi_dsp_driver_probe(dev, "INT3438"); + if (ret != SND_INTEL_DSP_DRIVER_SOF) { + dev_dbg(dev, "SOF ACPI driver not selected, aborting probe\n"); + return -ENODEV; + } + + return sof_acpi_probe(dev, &sof_acpi_broadwell_desc); +} + +/* acpi_driver definition */ +static struct platform_driver snd_sof_acpi_driver = { + .probe = sof_broadwell_probe, + .remove = sof_acpi_remove, + .driver = { + .name = "sof-audio-broadwell", + .pm = &sof_acpi_pm, + .acpi_match_table = sof_broadwell_match, + }, +}; +module_platform_driver(snd_sof_acpi_driver);
MODULE_LICENSE("Dual BSD/GPL"); MODULE_IMPORT_NS(SND_SOC_SOF_INTEL_HIFI_EP_IPC); diff --git a/sound/soc/sof/intel/byt.c b/sound/soc/sof/intel/byt.c index 19260dbecac5..9fa688c7711e 100644 --- a/sound/soc/sof/intel/byt.c +++ b/sound/soc/sof/intel/byt.c @@ -15,6 +15,8 @@ #include <linux/module.h> #include <sound/sof.h> #include <sound/sof/xtensa.h> +#include <sound/soc-acpi.h> +#include <sound/soc-acpi-intel-match.h> #include "../ops.h" #include "shim.h" #include "../sof-audio.h" @@ -657,8 +659,6 @@ EXPORT_SYMBOL_NS(tng_chip_info, SND_SOC_SOF_MERRIFIELD);
#endif /* CONFIG_SND_SOC_SOF_MERRIFIELD */
-#if IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL) - static void byt_reset_dsp_disable_int(struct snd_sof_dev *sdev) { /* Disable Interrupt from both sides */ @@ -822,7 +822,7 @@ static int byt_acpi_probe(struct snd_sof_dev *sdev) }
/* baytrail ops */ -const struct snd_sof_dsp_ops sof_byt_ops = { +static const struct snd_sof_dsp_ops sof_byt_ops = { /* device init */ .probe = byt_acpi_probe, .remove = byt_remove, @@ -892,16 +892,14 @@ const struct snd_sof_dsp_ops sof_byt_ops = {
.arch_ops = &sof_xtensa_arch_ops, }; -EXPORT_SYMBOL_NS(sof_byt_ops, SND_SOC_SOF_BAYTRAIL);
-const struct sof_intel_dsp_desc byt_chip_info = { +static const struct sof_intel_dsp_desc byt_chip_info = { .cores_num = 1, .host_managed_cores_mask = 1, }; -EXPORT_SYMBOL_NS(byt_chip_info, SND_SOC_SOF_BAYTRAIL);
/* cherrytrail and braswell ops */ -const struct snd_sof_dsp_ops sof_cht_ops = { +static const struct snd_sof_dsp_ops sof_cht_ops = { /* device init */ .probe = byt_acpi_probe, .remove = byt_remove, @@ -972,15 +970,99 @@ const struct snd_sof_dsp_ops sof_cht_ops = {
.arch_ops = &sof_xtensa_arch_ops, }; -EXPORT_SYMBOL_NS(sof_cht_ops, SND_SOC_SOF_BAYTRAIL);
-const struct sof_intel_dsp_desc cht_chip_info = { +static const struct sof_intel_dsp_desc cht_chip_info = { .cores_num = 1, .host_managed_cores_mask = 1, }; -EXPORT_SYMBOL_NS(cht_chip_info, SND_SOC_SOF_BAYTRAIL);
-#endif /* CONFIG_SND_SOC_SOF_BAYTRAIL */ +/* BYTCR uses different IRQ index */ +static const struct sof_dev_desc sof_acpi_baytrailcr_desc = { + .machines = snd_soc_acpi_intel_baytrail_machines, + .resindex_lpe_base = 0, + .resindex_pcicfg_base = 1, + .resindex_imr_base = 2, + .irqindex_host_ipc = 0, + .chip_info = &byt_chip_info, + .default_fw_path = "intel/sof", + .default_tplg_path = "intel/sof-tplg", + .default_fw_filename = "sof-byt.ri", + .nocodec_tplg_filename = "sof-byt-nocodec.tplg", + .ops = &sof_byt_ops, +}; + +static const struct sof_dev_desc sof_acpi_baytrail_desc = { + .machines = snd_soc_acpi_intel_baytrail_machines, + .resindex_lpe_base = 0, + .resindex_pcicfg_base = 1, + .resindex_imr_base = 2, + .irqindex_host_ipc = 5, + .chip_info = &byt_chip_info, + .default_fw_path = "intel/sof", + .default_tplg_path = "intel/sof-tplg", + .default_fw_filename = "sof-byt.ri", + .nocodec_tplg_filename = "sof-byt-nocodec.tplg", + .ops = &sof_byt_ops, +}; + +static const struct sof_dev_desc sof_acpi_cherrytrail_desc = { + .machines = snd_soc_acpi_intel_cherrytrail_machines, + .resindex_lpe_base = 0, + .resindex_pcicfg_base = 1, + .resindex_imr_base = 2, + .irqindex_host_ipc = 5, + .chip_info = &cht_chip_info, + .default_fw_path = "intel/sof", + .default_tplg_path = "intel/sof-tplg", + .default_fw_filename = "sof-cht.ri", + .nocodec_tplg_filename = "sof-cht-nocodec.tplg", + .ops = &sof_cht_ops, +}; + +static const struct acpi_device_id sof_baytrail_match[] = { + { "80860F28", (unsigned long)&sof_acpi_baytrail_desc }, + { "808622A8", (unsigned long)&sof_acpi_cherrytrail_desc }, + { } +}; +MODULE_DEVICE_TABLE(acpi, sof_baytrail_match); + +static int sof_baytrail_probe(struct platform_device *pdev) +{ + const struct sof_dev_desc *desc = device_get_match_data(&pdev->dev); + const struct acpi_device_id *id; + + id = acpi_match_device(dev->driver->acpi_match_table, dev); + if (!id) + return -ENODEV; + + ret = snd_intel_acpi_dsp_driver_probe(dev, id->id); + if (ret != SND_INTEL_DSP_DRIVER_SOF) { + dev_dbg(dev, "SOF ACPI driver not selected, aborting probe\n"); + return -ENODEV; + } + + dev_dbg(dev, "ACPI DSP detected"); + + if (!desc) + return -ENODEV; + + if (desc == &sof_acpi_baytrail_desc && soc_intel_is_byt_cr(pdev)) + desc = &sof_acpi_baytrailcr_desc; + + return sof_acpi_probe(pdev, desc); +} + +/* acpi_driver definition */ +static struct platform_driver snd_sof_baytrail_driver = { + .probe = sof_baytrail_probe, + .remove = sof_acpi_remove, + .driver = { + .name = "sof-audio-baytrail", + .pm = &sof_acpi_pm, + .acpi_match_table = sof_baytrail_match, + }, +}; +module_platform_driver(snd_sof_baytrail_driver);
MODULE_LICENSE("Dual BSD/GPL"); MODULE_IMPORT_NS(SND_SOC_SOF_INTEL_HIFI_EP_IPC); diff --git a/sound/soc/sof/intel/shim.h b/sound/soc/sof/intel/shim.h index 1e0afb5c8720..872116ee622b 100644 --- a/sound/soc/sof/intel/shim.h +++ b/sound/soc/sof/intel/shim.h @@ -166,14 +166,12 @@ struct sof_intel_dsp_desc { int ssp_base_offset; /* base address of the SSPs */ };
+extern const struct dev_pm_ops sof_acpi_pm; +extern int sof_acpi_probe(struct platform_device *pdev, const struct sof_dev_desc *desc); +extern int sof_acpi_remove(struct platform_device *pdev); + extern const struct snd_sof_dsp_ops sof_tng_ops; -extern const struct snd_sof_dsp_ops sof_byt_ops; -extern const struct snd_sof_dsp_ops sof_cht_ops; -extern const struct snd_sof_dsp_ops sof_bdw_ops;
-extern const struct sof_intel_dsp_desc byt_chip_info; -extern const struct sof_intel_dsp_desc cht_chip_info; -extern const struct sof_intel_dsp_desc bdw_chip_info; extern const struct sof_intel_dsp_desc tng_chip_info;
struct sof_intel_stream { diff --git a/sound/soc/sof/sof-acpi-dev.c b/sound/soc/sof/sof-acpi-dev.c index 2a369c2c6551..32654e042ec4 100644 --- a/sound/soc/sof/sof-acpi-dev.c +++ b/sound/soc/sof/sof-acpi-dev.c @@ -36,70 +36,7 @@ MODULE_PARM_DESC(sof_acpi_debug, "SOF ACPI debug options (0x0 all off)");
#define SOF_ACPI_DISABLE_PM_RUNTIME BIT(0)
-#if IS_ENABLED(CONFIG_ACPI) && IS_ENABLED(CONFIG_SND_SOC_SOF_BROADWELL) -static const struct sof_dev_desc sof_acpi_broadwell_desc = { - .machines = snd_soc_acpi_intel_broadwell_machines, - .resindex_lpe_base = 0, - .resindex_pcicfg_base = 1, - .resindex_imr_base = -1, - .irqindex_host_ipc = 0, - .chip_info = &bdw_chip_info, - .default_fw_path = "intel/sof", - .default_tplg_path = "intel/sof-tplg", - .default_fw_filename = "sof-bdw.ri", - .nocodec_tplg_filename = "sof-bdw-nocodec.tplg", - .ops = &sof_bdw_ops, -}; -#endif - -#if IS_ENABLED(CONFIG_ACPI) && IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL) - -/* BYTCR uses different IRQ index */ -static const struct sof_dev_desc sof_acpi_baytrailcr_desc = { - .machines = snd_soc_acpi_intel_baytrail_machines, - .resindex_lpe_base = 0, - .resindex_pcicfg_base = 1, - .resindex_imr_base = 2, - .irqindex_host_ipc = 0, - .chip_info = &byt_chip_info, - .default_fw_path = "intel/sof", - .default_tplg_path = "intel/sof-tplg", - .default_fw_filename = "sof-byt.ri", - .nocodec_tplg_filename = "sof-byt-nocodec.tplg", - .ops = &sof_byt_ops, -}; - -static const struct sof_dev_desc sof_acpi_baytrail_desc = { - .machines = snd_soc_acpi_intel_baytrail_machines, - .resindex_lpe_base = 0, - .resindex_pcicfg_base = 1, - .resindex_imr_base = 2, - .irqindex_host_ipc = 5, - .chip_info = &byt_chip_info, - .default_fw_path = "intel/sof", - .default_tplg_path = "intel/sof-tplg", - .default_fw_filename = "sof-byt.ri", - .nocodec_tplg_filename = "sof-byt-nocodec.tplg", - .ops = &sof_byt_ops, -}; - -static const struct sof_dev_desc sof_acpi_cherrytrail_desc = { - .machines = snd_soc_acpi_intel_cherrytrail_machines, - .resindex_lpe_base = 0, - .resindex_pcicfg_base = 1, - .resindex_imr_base = 2, - .irqindex_host_ipc = 5, - .chip_info = &cht_chip_info, - .default_fw_path = "intel/sof", - .default_tplg_path = "intel/sof-tplg", - .default_fw_filename = "sof-cht.ri", - .nocodec_tplg_filename = "sof-cht-nocodec.tplg", - .ops = &sof_cht_ops, -}; - -#endif - -static const struct dev_pm_ops sof_acpi_pm = { +const struct dev_pm_ops sof_acpi_pm = { SET_SYSTEM_SLEEP_PM_OPS(snd_sof_suspend, snd_sof_resume) SET_RUNTIME_PM_OPS(snd_sof_runtime_suspend, snd_sof_runtime_resume, snd_sof_runtime_idle) @@ -118,40 +55,17 @@ static void sof_acpi_probe_complete(struct device *dev) pm_runtime_enable(dev); }
-static int sof_acpi_probe(struct platform_device *pdev) +int sof_acpi_probe(struct platform_device *pdev, const struct sof_dev_desc *desc) { struct device *dev = &pdev->dev; - const struct acpi_device_id *id; - const struct sof_dev_desc *desc; struct snd_sof_pdata *sof_pdata; const struct snd_sof_dsp_ops *ops; int ret;
- id = acpi_match_device(dev->driver->acpi_match_table, dev); - if (!id) - return -ENODEV; - - ret = snd_intel_acpi_dsp_driver_probe(dev, id->id); - if (ret != SND_INTEL_DSP_DRIVER_ANY && ret != SND_INTEL_DSP_DRIVER_SOF) { - dev_dbg(dev, "SOF ACPI driver not selected, aborting probe\n"); - return -ENODEV; - } - - dev_dbg(dev, "ACPI DSP detected"); - sof_pdata = devm_kzalloc(dev, sizeof(*sof_pdata), GFP_KERNEL); if (!sof_pdata) return -ENOMEM;
- desc = device_get_match_data(dev); - if (!desc) - return -ENODEV; - -#if IS_ENABLED(CONFIG_ACPI) && IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL) - if (desc == &sof_acpi_baytrail_desc && soc_intel_is_byt_cr(pdev)) - desc = &sof_acpi_baytrailcr_desc; -#endif - /* get ops for platform */ ops = desc->ops; if (!ops) { @@ -193,8 +107,9 @@ static int sof_acpi_probe(struct platform_device *pdev)
return ret; } +EXPORT_SYMBOL_GPL(sof_acpi_probe);
-static int sof_acpi_remove(struct platform_device *pdev) +int sof_acpi_remove(struct platform_device *pdev) { if (!(sof_acpi_debug & SOF_ACPI_DISABLE_PM_RUNTIME)) pm_runtime_disable(&pdev->dev); @@ -204,33 +119,6 @@ static int sof_acpi_remove(struct platform_device *pdev)
return 0; } - -#ifdef CONFIG_ACPI -static const struct acpi_device_id sof_acpi_match[] = { -#if IS_ENABLED(CONFIG_SND_SOC_SOF_BROADWELL) - { "INT3438", (unsigned long)&sof_acpi_broadwell_desc }, -#endif -#if IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL) - { "80860F28", (unsigned long)&sof_acpi_baytrail_desc }, - { "808622A8", (unsigned long)&sof_acpi_cherrytrail_desc }, -#endif - { } -}; -MODULE_DEVICE_TABLE(acpi, sof_acpi_match); -#endif - -/* acpi_driver definition */ -static struct platform_driver snd_sof_acpi_driver = { - .probe = sof_acpi_probe, - .remove = sof_acpi_remove, - .driver = { - .name = "sof-audio-acpi", - .pm = &sof_acpi_pm, - .acpi_match_table = ACPI_PTR(sof_acpi_match), - }, -}; -module_platform_driver(snd_sof_acpi_driver); +EXPORT_SYMBOL_GPL(sof_acpi_remove);
MODULE_LICENSE("Dual BSD/GPL"); -MODULE_IMPORT_NS(SND_SOC_SOF_BAYTRAIL); -MODULE_IMPORT_NS(SND_SOC_SOF_BROADWELL);
On Tue, Jan 5, 2021 at 8:07 PM Arnd Bergmann arnd@kernel.org wrote:
From: Arnd Bergmann arnd@arndb.de
The SOF-ACPI driver is backwards from the normal Linux model, it has a generic driver that knows about all the specific drivers, as opposed to having hardware specific drivers that link against a common framework.
This requires ugly Kconfig magic and leads to missed dependencies as seen in this link error:
arm-linux-gnueabi-ld: sound/soc/sof/sof-pci-dev.o: in function `sof_acpi_probe': sof-pci-dev.c:(.text+0x1c): undefined reference to `snd_intel_dsp_driver_probe'
Change it to use the normal probe order of starting with a specific device in a driver, turning the sof-acpi-dev.c driver into a library.
Signed-off-by: Arnd Bergmann arnd@arndb.de
There were a couple of build failures introduced by this version. I have one that does build now, and can post that if others think this is the direction they want to go.
Arnd
Hi Arnd,
On Wed, 6 Jan 2021, Arnd Bergmann wrote:
On Tue, Jan 5, 2021 at 8:07 PM Arnd Bergmann arnd@kernel.org wrote:
Change it to use the normal probe order of starting with a specific device in a driver, turning the sof-acpi-dev.c driver into a library.
There were a couple of build failures introduced by this version. I have one that does build now, and can post that if others think this is the direction they want to go.
thanks for the follow-up. We have many SOF maintainers still out on holidays this week, so let's give some time for people to digest. This is certainly a big change. The version you posted is already sufficient to describe the idea for sure.
Br, Kai
On 1/5/21 1:07 PM, Arnd Bergmann wrote:
From: Arnd Bergmann arnd@arndb.de
The SOF-ACPI driver is backwards from the normal Linux model, it has a generic driver that knows about all the specific drivers, as opposed to having hardware specific drivers that link against a common framework.
This requires ugly Kconfig magic and leads to missed dependencies as seen in this link error:
arm-linux-gnueabi-ld: sound/soc/sof/sof-pci-dev.o: in function `sof_acpi_probe': sof-pci-dev.c:(.text+0x1c): undefined reference to `snd_intel_dsp_driver_probe'
Change it to use the normal probe order of starting with a specific device in a driver, turning the sof-acpi-dev.c driver into a library.
Thanks Arnd for reporting all this, much appreciated.
The initial design was that we would have one generic platform_driver (ACPI) and one generic PCI driver that would deal with all known IDs, with descriptors that would point ops and callbacks defined in device-specific drivers. It's how all Intel drivers worked so far, from HDaudio to Atom/SST and Skylake.
It's not that ugly, but to Arnd's point we do have a lot of #if IS_ENABLED at the top level with a larger and larger table of IDs, along with Kconfig magic indeed to propagate constraints from top-level to device-specific drivers. The error with DSP_CONFIG comes from the fact that this never belonged at the top-level, or should have been conditionally invoked, as noted by Takashi.
That said, the initial design which dates from 2017 can be revisited now that we start having quite a few platforms and more coming. What Arnd suggests isn't without merits, it would indeed turn the generic code into generic helpers, and have all the platform IDs maintained in device-specific drivers. It's a more distributed/scalable solution, the only minor drawback I see is that it would require multiple instances of the 'platform_driver' and 'pci_driver' structures.
I would also want to keep the top-level selection so that ACPI/PCI/DT modules can be disabled in one shot, that would mean an additional change to the Makefiles since e.g. obj-$(CONFIG_SND_SOC_SOF_ACPI) += snd-sof-acpi.o would need to be set somehow.
Since this is going to be a really invasive change, and past experience shows that mucking with Kconfigs will invariably raise a number of broken corner cases, if there is support from Mark/Takashi/Jaroslav on this idea, we should first test it in the SOF tree so that we get a good test coverage and don't break too many eggs in Mark's tree. We would also need to concurrently change our CI scripts which are dependent on module names.
Also maybe in a first pass we can remove the compilation error with IS_REACHABLE and in a second pass do more invasive surgery?
Signed-off-by: Arnd Bergmann arnd@arndb.de
sound/soc/sof/intel/Kconfig | 34 ++-------- sound/soc/sof/intel/bdw.c | 51 +++++++++++++-- sound/soc/sof/intel/byt.c | 104 +++++++++++++++++++++++++---- sound/soc/sof/intel/shim.h | 10 ++- sound/soc/sof/sof-acpi-dev.c | 122 ++--------------------------------- 5 files changed, 156 insertions(+), 165 deletions(-)
diff --git a/sound/soc/sof/intel/Kconfig b/sound/soc/sof/intel/Kconfig index ae9ba834814e..ff9266413a06 100644 --- a/sound/soc/sof/intel/Kconfig +++ b/sound/soc/sof/intel/Kconfig @@ -9,14 +9,6 @@ config SND_SOC_SOF_INTEL_TOPLEVEL
if SND_SOC_SOF_INTEL_TOPLEVEL
-config SND_SOC_SOF_INTEL_ACPI
- def_tristate SND_SOC_SOF_ACPI
- select SND_SOC_SOF_BAYTRAIL if SND_SOC_SOF_BAYTRAIL_SUPPORT
- select SND_SOC_SOF_BROADWELL if SND_SOC_SOF_BROADWELL_SUPPORT
- help
This option is not user-selectable but automagically handled by'select' statements at a higher level.- config SND_SOC_SOF_INTEL_PCI def_tristate SND_SOC_SOF_PCI select SND_SOC_SOF_MERRIFIELD if SND_SOC_SOF_MERRIFIELD_SUPPORT
@@ -58,10 +50,12 @@ config SND_SOC_SOF_INTEL_COMMON This option is not user-selectable but automagically handled by 'select' statements at a higher level.
-if SND_SOC_SOF_INTEL_ACPI +if SND_SOC_SOF_ACPI
-config SND_SOC_SOF_BAYTRAIL_SUPPORT +config SND_SOC_SOF_BAYTRAIL bool "SOF support for Baytrail, Braswell and Cherrytrail"
- select SND_SOC_SOF_INTEL_ATOM_HIFI_EP
- select SND_INTEL_DSP_CONFIG help This adds support for Sound Open Firmware for Intel(R) platforms using the Baytrail, Braswell or Cherrytrail processors.
@@ -75,17 +69,11 @@ config SND_SOC_SOF_BAYTRAIL_SUPPORT Say Y if you want to enable SOF on Baytrail/Cherrytrail. If unsure select "N".
-config SND_SOC_SOF_BAYTRAIL
- tristate
- select SND_SOC_SOF_INTEL_ATOM_HIFI_EP
- select SND_INTEL_DSP_CONFIG
- help
This option is not user-selectable but automagically handled by'select' statements at a higher level.-config SND_SOC_SOF_BROADWELL_SUPPORT +config SND_SOC_SOF_BROADWELL bool "SOF support for Broadwell" select SND_INTEL_DSP_CONFIG
- select SND_SOC_SOF_INTEL_COMMON
- select SND_SOC_SOF_INTEL_HIFI_EP_IPC help This adds support for Sound Open Firmware for Intel(R) platforms using the Broadwell processors.
@@ -100,14 +88,6 @@ config SND_SOC_SOF_BROADWELL_SUPPORT Say Y if you want to enable SOF on Broadwell. If unsure select "N".
-config SND_SOC_SOF_BROADWELL
tristate
select SND_SOC_SOF_INTEL_COMMON
select SND_SOC_SOF_INTEL_HIFI_EP_IPC
help
This option is not user-selectable but automagically handled by'select' statements at a higher level.endif ## SND_SOC_SOF_INTEL_ACPI
if SND_SOC_SOF_INTEL_PCI
diff --git a/sound/soc/sof/intel/bdw.c b/sound/soc/sof/intel/bdw.c index 50a4a73e6b9f..542e885733a2 100644 --- a/sound/soc/sof/intel/bdw.c +++ b/sound/soc/sof/intel/bdw.c @@ -15,6 +15,8 @@ #include <linux/module.h> #include <sound/sof.h> #include <sound/sof/xtensa.h> +#include <sound/soc-acpi.h> +#include <sound/soc-acpi-intel-match.h> #include "../ops.h" #include "shim.h" #include "../sof-audio.h" @@ -590,7 +592,7 @@ static struct snd_soc_dai_driver bdw_dai[] = { };
/* broadwell ops */ -const struct snd_sof_dsp_ops sof_bdw_ops = { +static const struct snd_sof_dsp_ops sof_bdw_ops = { /*Device init */ .probe = bdw_probe,
@@ -651,13 +653,54 @@ const struct snd_sof_dsp_ops sof_bdw_ops = {
.arch_ops = &sof_xtensa_arch_ops, }; -EXPORT_SYMBOL_NS(sof_bdw_ops, SND_SOC_SOF_BROADWELL);
-const struct sof_intel_dsp_desc bdw_chip_info = { +static const struct sof_intel_dsp_desc bdw_chip_info = { .cores_num = 1, .host_managed_cores_mask = 1, }; -EXPORT_SYMBOL_NS(bdw_chip_info, SND_SOC_SOF_BROADWELL);
+static const struct sof_dev_desc sof_acpi_broadwell_desc = {
- .machines = snd_soc_acpi_intel_broadwell_machines,
- .resindex_lpe_base = 0,
- .resindex_pcicfg_base = 1,
- .resindex_imr_base = -1,
- .irqindex_host_ipc = 0,
- .chip_info = &bdw_chip_info,
- .default_fw_path = "intel/sof",
- .default_tplg_path = "intel/sof-tplg",
- .default_fw_filename = "sof-bdw.ri",
- .nocodec_tplg_filename = "sof-bdw-nocodec.tplg",
- .ops = &sof_bdw_ops,
+};
+static const struct acpi_device_id sof_broadwell_match[] = {
- { "INT3438", (unsigned long)&sof_acpi_broadwell_desc },
- { }
+}; +MODULE_DEVICE_TABLE(acpi, sof_broadwell_match);
+static int sof_broadwell_probe(struct platform_device *dev) +{
- int ret = snd_intel_acpi_dsp_driver_probe(dev, "INT3438");
- if (ret != SND_INTEL_DSP_DRIVER_SOF) {
dev_dbg(dev, "SOF ACPI driver not selected, aborting probe\n");return -ENODEV;- }
- return sof_acpi_probe(dev, &sof_acpi_broadwell_desc);
+}
+/* acpi_driver definition */ +static struct platform_driver snd_sof_acpi_driver = {
- .probe = sof_broadwell_probe,
- .remove = sof_acpi_remove,
- .driver = {
.name = "sof-audio-broadwell",.pm = &sof_acpi_pm,.acpi_match_table = sof_broadwell_match,- },
+}; +module_platform_driver(snd_sof_acpi_driver);
MODULE_LICENSE("Dual BSD/GPL"); MODULE_IMPORT_NS(SND_SOC_SOF_INTEL_HIFI_EP_IPC); diff --git a/sound/soc/sof/intel/byt.c b/sound/soc/sof/intel/byt.c index 19260dbecac5..9fa688c7711e 100644 --- a/sound/soc/sof/intel/byt.c +++ b/sound/soc/sof/intel/byt.c @@ -15,6 +15,8 @@ #include <linux/module.h> #include <sound/sof.h> #include <sound/sof/xtensa.h> +#include <sound/soc-acpi.h> +#include <sound/soc-acpi-intel-match.h> #include "../ops.h" #include "shim.h" #include "../sof-audio.h" @@ -657,8 +659,6 @@ EXPORT_SYMBOL_NS(tng_chip_info, SND_SOC_SOF_MERRIFIELD);
#endif /* CONFIG_SND_SOC_SOF_MERRIFIELD */
-#if IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL)
- static void byt_reset_dsp_disable_int(struct snd_sof_dev *sdev) { /* Disable Interrupt from both sides */
@@ -822,7 +822,7 @@ static int byt_acpi_probe(struct snd_sof_dev *sdev) }
/* baytrail ops */ -const struct snd_sof_dsp_ops sof_byt_ops = { +static const struct snd_sof_dsp_ops sof_byt_ops = { /* device init */ .probe = byt_acpi_probe, .remove = byt_remove, @@ -892,16 +892,14 @@ const struct snd_sof_dsp_ops sof_byt_ops = {
.arch_ops = &sof_xtensa_arch_ops, }; -EXPORT_SYMBOL_NS(sof_byt_ops, SND_SOC_SOF_BAYTRAIL);
-const struct sof_intel_dsp_desc byt_chip_info = { +static const struct sof_intel_dsp_desc byt_chip_info = { .cores_num = 1, .host_managed_cores_mask = 1, }; -EXPORT_SYMBOL_NS(byt_chip_info, SND_SOC_SOF_BAYTRAIL);
/* cherrytrail and braswell ops */ -const struct snd_sof_dsp_ops sof_cht_ops = { +static const struct snd_sof_dsp_ops sof_cht_ops = { /* device init */ .probe = byt_acpi_probe, .remove = byt_remove, @@ -972,15 +970,99 @@ const struct snd_sof_dsp_ops sof_cht_ops = {
.arch_ops = &sof_xtensa_arch_ops, }; -EXPORT_SYMBOL_NS(sof_cht_ops, SND_SOC_SOF_BAYTRAIL);
-const struct sof_intel_dsp_desc cht_chip_info = { +static const struct sof_intel_dsp_desc cht_chip_info = { .cores_num = 1, .host_managed_cores_mask = 1, }; -EXPORT_SYMBOL_NS(cht_chip_info, SND_SOC_SOF_BAYTRAIL);
-#endif /* CONFIG_SND_SOC_SOF_BAYTRAIL */ +/* BYTCR uses different IRQ index */ +static const struct sof_dev_desc sof_acpi_baytrailcr_desc = {
- .machines = snd_soc_acpi_intel_baytrail_machines,
- .resindex_lpe_base = 0,
- .resindex_pcicfg_base = 1,
- .resindex_imr_base = 2,
- .irqindex_host_ipc = 0,
- .chip_info = &byt_chip_info,
- .default_fw_path = "intel/sof",
- .default_tplg_path = "intel/sof-tplg",
- .default_fw_filename = "sof-byt.ri",
- .nocodec_tplg_filename = "sof-byt-nocodec.tplg",
- .ops = &sof_byt_ops,
+};
+static const struct sof_dev_desc sof_acpi_baytrail_desc = {
- .machines = snd_soc_acpi_intel_baytrail_machines,
- .resindex_lpe_base = 0,
- .resindex_pcicfg_base = 1,
- .resindex_imr_base = 2,
- .irqindex_host_ipc = 5,
- .chip_info = &byt_chip_info,
- .default_fw_path = "intel/sof",
- .default_tplg_path = "intel/sof-tplg",
- .default_fw_filename = "sof-byt.ri",
- .nocodec_tplg_filename = "sof-byt-nocodec.tplg",
- .ops = &sof_byt_ops,
+};
+static const struct sof_dev_desc sof_acpi_cherrytrail_desc = {
- .machines = snd_soc_acpi_intel_cherrytrail_machines,
- .resindex_lpe_base = 0,
- .resindex_pcicfg_base = 1,
- .resindex_imr_base = 2,
- .irqindex_host_ipc = 5,
- .chip_info = &cht_chip_info,
- .default_fw_path = "intel/sof",
- .default_tplg_path = "intel/sof-tplg",
- .default_fw_filename = "sof-cht.ri",
- .nocodec_tplg_filename = "sof-cht-nocodec.tplg",
- .ops = &sof_cht_ops,
+};
+static const struct acpi_device_id sof_baytrail_match[] = {
- { "80860F28", (unsigned long)&sof_acpi_baytrail_desc },
- { "808622A8", (unsigned long)&sof_acpi_cherrytrail_desc },
- { }
+}; +MODULE_DEVICE_TABLE(acpi, sof_baytrail_match);
+static int sof_baytrail_probe(struct platform_device *pdev) +{
- const struct sof_dev_desc *desc = device_get_match_data(&pdev->dev);
- const struct acpi_device_id *id;
- id = acpi_match_device(dev->driver->acpi_match_table, dev);
- if (!id)
return -ENODEV;- ret = snd_intel_acpi_dsp_driver_probe(dev, id->id);
- if (ret != SND_INTEL_DSP_DRIVER_SOF) {
dev_dbg(dev, "SOF ACPI driver not selected, aborting probe\n");return -ENODEV;- }
- dev_dbg(dev, "ACPI DSP detected");
- if (!desc)
return -ENODEV;- if (desc == &sof_acpi_baytrail_desc && soc_intel_is_byt_cr(pdev))
desc = &sof_acpi_baytrailcr_desc;- return sof_acpi_probe(pdev, desc);
+}
+/* acpi_driver definition */ +static struct platform_driver snd_sof_baytrail_driver = {
- .probe = sof_baytrail_probe,
- .remove = sof_acpi_remove,
- .driver = {
.name = "sof-audio-baytrail",.pm = &sof_acpi_pm,.acpi_match_table = sof_baytrail_match,- },
+}; +module_platform_driver(snd_sof_baytrail_driver);
MODULE_LICENSE("Dual BSD/GPL"); MODULE_IMPORT_NS(SND_SOC_SOF_INTEL_HIFI_EP_IPC); diff --git a/sound/soc/sof/intel/shim.h b/sound/soc/sof/intel/shim.h index 1e0afb5c8720..872116ee622b 100644 --- a/sound/soc/sof/intel/shim.h +++ b/sound/soc/sof/intel/shim.h @@ -166,14 +166,12 @@ struct sof_intel_dsp_desc { int ssp_base_offset; /* base address of the SSPs */ };
+extern const struct dev_pm_ops sof_acpi_pm; +extern int sof_acpi_probe(struct platform_device *pdev, const struct sof_dev_desc *desc); +extern int sof_acpi_remove(struct platform_device *pdev);
- extern const struct snd_sof_dsp_ops sof_tng_ops;
-extern const struct snd_sof_dsp_ops sof_byt_ops; -extern const struct snd_sof_dsp_ops sof_cht_ops; -extern const struct snd_sof_dsp_ops sof_bdw_ops;
-extern const struct sof_intel_dsp_desc byt_chip_info; -extern const struct sof_intel_dsp_desc cht_chip_info; -extern const struct sof_intel_dsp_desc bdw_chip_info; extern const struct sof_intel_dsp_desc tng_chip_info;
struct sof_intel_stream { diff --git a/sound/soc/sof/sof-acpi-dev.c b/sound/soc/sof/sof-acpi-dev.c index 2a369c2c6551..32654e042ec4 100644 --- a/sound/soc/sof/sof-acpi-dev.c +++ b/sound/soc/sof/sof-acpi-dev.c @@ -36,70 +36,7 @@ MODULE_PARM_DESC(sof_acpi_debug, "SOF ACPI debug options (0x0 all off)");
#define SOF_ACPI_DISABLE_PM_RUNTIME BIT(0)
-#if IS_ENABLED(CONFIG_ACPI) && IS_ENABLED(CONFIG_SND_SOC_SOF_BROADWELL) -static const struct sof_dev_desc sof_acpi_broadwell_desc = {
- .machines = snd_soc_acpi_intel_broadwell_machines,
- .resindex_lpe_base = 0,
- .resindex_pcicfg_base = 1,
- .resindex_imr_base = -1,
- .irqindex_host_ipc = 0,
- .chip_info = &bdw_chip_info,
- .default_fw_path = "intel/sof",
- .default_tplg_path = "intel/sof-tplg",
- .default_fw_filename = "sof-bdw.ri",
- .nocodec_tplg_filename = "sof-bdw-nocodec.tplg",
- .ops = &sof_bdw_ops,
-}; -#endif
-#if IS_ENABLED(CONFIG_ACPI) && IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL)
-/* BYTCR uses different IRQ index */ -static const struct sof_dev_desc sof_acpi_baytrailcr_desc = {
- .machines = snd_soc_acpi_intel_baytrail_machines,
- .resindex_lpe_base = 0,
- .resindex_pcicfg_base = 1,
- .resindex_imr_base = 2,
- .irqindex_host_ipc = 0,
- .chip_info = &byt_chip_info,
- .default_fw_path = "intel/sof",
- .default_tplg_path = "intel/sof-tplg",
- .default_fw_filename = "sof-byt.ri",
- .nocodec_tplg_filename = "sof-byt-nocodec.tplg",
- .ops = &sof_byt_ops,
-};
-static const struct sof_dev_desc sof_acpi_baytrail_desc = {
- .machines = snd_soc_acpi_intel_baytrail_machines,
- .resindex_lpe_base = 0,
- .resindex_pcicfg_base = 1,
- .resindex_imr_base = 2,
- .irqindex_host_ipc = 5,
- .chip_info = &byt_chip_info,
- .default_fw_path = "intel/sof",
- .default_tplg_path = "intel/sof-tplg",
- .default_fw_filename = "sof-byt.ri",
- .nocodec_tplg_filename = "sof-byt-nocodec.tplg",
- .ops = &sof_byt_ops,
-};
-static const struct sof_dev_desc sof_acpi_cherrytrail_desc = {
- .machines = snd_soc_acpi_intel_cherrytrail_machines,
- .resindex_lpe_base = 0,
- .resindex_pcicfg_base = 1,
- .resindex_imr_base = 2,
- .irqindex_host_ipc = 5,
- .chip_info = &cht_chip_info,
- .default_fw_path = "intel/sof",
- .default_tplg_path = "intel/sof-tplg",
- .default_fw_filename = "sof-cht.ri",
- .nocodec_tplg_filename = "sof-cht-nocodec.tplg",
- .ops = &sof_cht_ops,
-};
-#endif
-static const struct dev_pm_ops sof_acpi_pm = { +const struct dev_pm_ops sof_acpi_pm = { SET_SYSTEM_SLEEP_PM_OPS(snd_sof_suspend, snd_sof_resume) SET_RUNTIME_PM_OPS(snd_sof_runtime_suspend, snd_sof_runtime_resume, snd_sof_runtime_idle) @@ -118,40 +55,17 @@ static void sof_acpi_probe_complete(struct device *dev) pm_runtime_enable(dev); }
-static int sof_acpi_probe(struct platform_device *pdev) +int sof_acpi_probe(struct platform_device *pdev, const struct sof_dev_desc *desc) { struct device *dev = &pdev->dev;
const struct acpi_device_id *id;
const struct sof_dev_desc *desc; struct snd_sof_pdata *sof_pdata; const struct snd_sof_dsp_ops *ops; int ret;
id = acpi_match_device(dev->driver->acpi_match_table, dev);
if (!id)
return -ENODEV;ret = snd_intel_acpi_dsp_driver_probe(dev, id->id);
if (ret != SND_INTEL_DSP_DRIVER_ANY && ret != SND_INTEL_DSP_DRIVER_SOF) {
dev_dbg(dev, "SOF ACPI driver not selected, aborting probe\n");return -ENODEV;}
dev_dbg(dev, "ACPI DSP detected");
sof_pdata = devm_kzalloc(dev, sizeof(*sof_pdata), GFP_KERNEL); if (!sof_pdata) return -ENOMEM;
desc = device_get_match_data(dev);
if (!desc)
return -ENODEV;-#if IS_ENABLED(CONFIG_ACPI) && IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL)
- if (desc == &sof_acpi_baytrail_desc && soc_intel_is_byt_cr(pdev))
desc = &sof_acpi_baytrailcr_desc;-#endif
- /* get ops for platform */ ops = desc->ops; if (!ops) {
@@ -193,8 +107,9 @@ static int sof_acpi_probe(struct platform_device *pdev)
return ret; } +EXPORT_SYMBOL_GPL(sof_acpi_probe);
-static int sof_acpi_remove(struct platform_device *pdev) +int sof_acpi_remove(struct platform_device *pdev) { if (!(sof_acpi_debug & SOF_ACPI_DISABLE_PM_RUNTIME)) pm_runtime_disable(&pdev->dev); @@ -204,33 +119,6 @@ static int sof_acpi_remove(struct platform_device *pdev)
return 0; }
-#ifdef CONFIG_ACPI -static const struct acpi_device_id sof_acpi_match[] = { -#if IS_ENABLED(CONFIG_SND_SOC_SOF_BROADWELL)
- { "INT3438", (unsigned long)&sof_acpi_broadwell_desc },
-#endif -#if IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL)
- { "80860F28", (unsigned long)&sof_acpi_baytrail_desc },
- { "808622A8", (unsigned long)&sof_acpi_cherrytrail_desc },
-#endif
- { }
-}; -MODULE_DEVICE_TABLE(acpi, sof_acpi_match); -#endif
-/* acpi_driver definition */ -static struct platform_driver snd_sof_acpi_driver = {
- .probe = sof_acpi_probe,
- .remove = sof_acpi_remove,
- .driver = {
.name = "sof-audio-acpi",.pm = &sof_acpi_pm,.acpi_match_table = ACPI_PTR(sof_acpi_match),- },
-}; -module_platform_driver(snd_sof_acpi_driver); +EXPORT_SYMBOL_GPL(sof_acpi_remove);
MODULE_LICENSE("Dual BSD/GPL"); -MODULE_IMPORT_NS(SND_SOC_SOF_BAYTRAIL); -MODULE_IMPORT_NS(SND_SOC_SOF_BROADWELL);
On Mon, 11 Jan 2021 20:54:17 +0100, Pierre-Louis Bossart wrote:
On 1/5/21 1:07 PM, Arnd Bergmann wrote:
From: Arnd Bergmann arnd@arndb.de
The SOF-ACPI driver is backwards from the normal Linux model, it has a generic driver that knows about all the specific drivers, as opposed to having hardware specific drivers that link against a common framework.
This requires ugly Kconfig magic and leads to missed dependencies as seen in this link error:
arm-linux-gnueabi-ld: sound/soc/sof/sof-pci-dev.o: in function `sof_acpi_probe': sof-pci-dev.c:(.text+0x1c): undefined reference to `snd_intel_dsp_driver_probe'
Change it to use the normal probe order of starting with a specific device in a driver, turning the sof-acpi-dev.c driver into a library.
Thanks Arnd for reporting all this, much appreciated.
The initial design was that we would have one generic platform_driver (ACPI) and one generic PCI driver that would deal with all known IDs, with descriptors that would point ops and callbacks defined in device-specific drivers. It's how all Intel drivers worked so far, from HDaudio to Atom/SST and Skylake.
It's not that ugly, but to Arnd's point we do have a lot of #if IS_ENABLED at the top level with a larger and larger table of IDs, along with Kconfig magic indeed to propagate constraints from top-level to device-specific drivers. The error with DSP_CONFIG comes from the fact that this never belonged at the top-level, or should have been conditionally invoked, as noted by Takashi.
That said, the initial design which dates from 2017 can be revisited now that we start having quite a few platforms and more coming. What Arnd suggests isn't without merits, it would indeed turn the generic code into generic helpers, and have all the platform IDs maintained in device-specific drivers. It's a more distributed/scalable solution, the only minor drawback I see is that it would require multiple instances of the 'platform_driver' and 'pci_driver' structures.
I would also want to keep the top-level selection so that ACPI/PCI/DT modules can be disabled in one shot, that would mean an additional change to the Makefiles since e.g. obj-$(CONFIG_SND_SOC_SOF_ACPI) += snd-sof-acpi.o would need to be set somehow.
Since this is going to be a really invasive change, and past experience shows that mucking with Kconfigs will invariably raise a number of broken corner cases, if there is support from Mark/Takashi/Jaroslav on this idea, we should first test it in the SOF tree so that we get a good test coverage and don't break too many eggs in Mark's tree. We would also need to concurrently change our CI scripts which are dependent on module names.
I'm in favor of the way Arnd proposed. It's more straightforward and less code.
If you find the number of modules or the too much cutting out being problematic, you can create a module snd-sof-intel-acpi and snd-sof-intel-pci containing the driver table entries for all Intel devices, too. In the case, you'll still need some conditional calls of intel-dsp-config there, but it's a good step for reducing the Kconfig complexity.
Also maybe in a first pass we can remove the compilation error with IS_REACHABLE and in a second pass do more invasive surgery?
Agreed, we'd like to keep less changes for 5.11 for now.
thanks,
Takashi
Since this is going to be a really invasive change, and past experience shows that mucking with Kconfigs will invariably raise a number of broken corner cases, if there is support from Mark/Takashi/Jaroslav on this idea, we should first test it in the SOF tree so that we get a good test coverage and don't break too many eggs in Mark's tree. We would also need to concurrently change our CI scripts which are dependent on module names.
I'm in favor of the way Arnd proposed. It's more straightforward and less code.
Thanks Takashi for the feedback.
Since yesterday I looked at another problem where we can have unmet dependencies between SoundWire (m) and SOF (y), so we probably need to rethink all this. We had similar issue with SOF and HDaudio before, it's time to revisit all this.
If you find the number of modules or the too much cutting out being problematic, you can create a module snd-sof-intel-acpi and snd-sof-intel-pci containing the driver table entries for all Intel devices, too. In the case, you'll still need some conditional calls of intel-dsp-config there, but it's a good step for reducing the Kconfig complexity.
Also maybe in a first pass we can remove the compilation error with IS_REACHABLE and in a second pass do more invasive surgery?
Agreed, we'd like to keep less changes for 5.11 for now.
Ack, we'll send smaller changes first.
On Tue, Jan 12, 2021 at 9:17 PM Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com wrote:
Since this is going to be a really invasive change, and past experience shows that mucking with Kconfigs will invariably raise a number of broken corner cases, if there is support from Mark/Takashi/Jaroslav on this idea, we should first test it in the SOF tree so that we get a good test coverage and don't break too many eggs in Mark's tree. We would also need to concurrently change our CI scripts which are dependent on module names.
I'm in favor of the way Arnd proposed. It's more straightforward and less code.
Thanks Takashi for the feedback.
Since yesterday I looked at another problem where we can have unmet dependencies between SoundWire (m) and SOF (y), so we probably need to rethink all this. We had similar issue with SOF and HDaudio before, it's time to revisit all this.
I think I ran into the same thing yesterday, and came up with a patch for that one as well. I think it should be independent of the other one but I did not try it by itself.
I'll send it along with a fixed version of the one in this thread, together the have now survived a few hundred randconfig builds.
Arnd
On Mon, Jan 4, 2021 at 4:00 PM Jaroslav Kysela perex@perex.cz wrote:
Dne 03. 01. 21 v 14:52 Arnd Bergmann napsal(a):
diff --git a/include/sound/intel-dsp-config.h b/include/sound/intel-dsp-config.h index d4609077c258..94667e870029 100644 --- a/include/sound/intel-dsp-config.h +++ b/include/sound/intel-dsp-config.h @@ -18,24 +18,7 @@ enum { SND_INTEL_DSP_DRIVER_LAST = SND_INTEL_DSP_DRIVER_SOF };
-#if IS_ENABLED(CONFIG_SND_INTEL_DSP_CONFIG)
The SOF drivers selects the DSP config code only when required (for specific platforms - see sound/soc/sof/intel/Kconfig).
It seems that the above if should be modified as:
#if IS_BUILDIN(CONFIG_SND_INTEL_DSP_CONFIG) || (defined(MODULE) && IS_MODULE(CONFIG_SND_INTEL_DSP_CONFIG))
So the buildin drivers which do not require the DSP config probe can be compiled without this dependency.
This would be the same as
#if IS_REACHABLE(CONFIG_SND_INTEL_DSP_CONFIG)
but using that macro is almost always a bad idea, as it tends to hide dependency problems and causes things to silently not work right when the Kconfig rules are incorrect.
Arnd
participants (6)
-
Arnd Bergmann -
Jaroslav Kysela -
Kai Vehmanen -
Mark Brown -
Pierre-Louis Bossart -
Takashi Iwai