[alsa-devel] [PATCH 27/35] ASoC: Intel: Skylake: Define platform descriptors
Pierre-Louis Bossart
pierre-louis.bossart at linux.intel.com
Mon Aug 26 19:13:38 CEST 2019
On 8/24/19 5:51 AM, Cezary Rojewski wrote:
> On 2019-08-23 21:50, Pierre-Louis Bossart wrote:
>>
>>
>> On 8/22/19 2:04 PM, Cezary Rojewski wrote:
>>> Make use of sst_pdata and declare platform descriptors for all existing
>>> cAVS platforms. Each carries information about base_fw filename,
>>> platform specific operations and boards supported.
>>
>> if you use a constant base_fw name that cannot be made board-specific
>> for specific usages, you will restrict the ability to deal with quirks
>> and custom cases.
>>
>> real-life example: not so long ago there were two SST firmwares for
>> 'regular' solutions and ultra-low-latency ones, so by having a single
>> name for all APL-based platforms you will generate issues that don't
>> exist today, or you will force users to patch something in the core.
>>
>
> I did not bring up ULL case for a reason. Guess Pandora box is to be
> opened.. so be it.
>
> ULL stands for Ultra Low Latency and it can be described by the following:
> - exists only for APL based platforms (more like single platform/ model
> though)
> - in consequence, binary isn't present on any other FW branch and any
> other platform apart from APL
> - its existence is tied to hardware.. eh.. let's call it a "limitation"
> - number of actual vendors is too Ultra Low..
> - has limited functionality and validation
> - is not the recommended FW for end users in any case
> - binary is not going to be upstreamed
> - reference board is not going to be upstreamed
> - generic (so called main FW) and ULL share the board ACPI ID and thus
> require kernel .config to be modified -or- blacklist.conf with be updated
> - shares topology filename with generic (main) FW so user still has to
> modify his /lib/firmware. Topology names are currently NHLT-based, built
> from NHLT header data and platform id which are BIOS/ ABL and platform
> specific respectively
> (...)
I would describe your answer as 'whatabout-ism'. Yes there are plenty of
ways to screw-up, none of them is a justification for assuming that a
single filename will work for everyone.
There are also plenty of good reasons to use a different fw and topology
file name. Taking this capability away essentially corners users into
non-upstreamed custom versions.
> TLDR:
> There is total of 0 people sitting in front of their monitors who are
> consciously going to make use of ULL firmware.
> Any user that is going to, will have to play with their kconfig,
> blacklist and replace existing topology file.
that's where you are making too many assumptions, if quirks and dynamic
detection capabilities are provided then it's possible to have a single
kernel build that will deal with multiple configurations.
> This is normally done by titanic build-bot which, among billion other
> things, ensures /lib/firmware looks like it should given the configuration.
>
> -
>
> So, one could have provided a nice choice-box within menuconfig to
> ensure only one board can be chosen.
> When one does it, one realizes both generic and ULL firmwares are not
> actually tied to any specific board and with more boards (usecases) and
> more kconfigs code gets bloated.
>
> Moving further, guarding apl_desc with #if-else depending on some global
> generic-vs-ULL configuration which would adjust said descriptors with
> proper FW filename actually seems like a better solution..
>
> ..and then kBOOM comes in and actual design pattern!
> Board should have been stated tplg_filename, not the fw_filename. Said
> topology file contains manifest which tells host what libraries to load.
> And thus, we clear the mist and see that one single field (which is
> currently missing in snd_soc_acpi_mach) and some clever topology
> manifest make it all happen: platform-board conflicts cease to exist.
I am not going to argue further. I've spent a lot of time making sure
the same kernel build can be used across multiple platforms, if you want
to stick to static custom configurations I am not interested in
debating. I just hope your team has enough support folks to deal with
all these configurations.
>
> Czarek
>
>>>
>>> Signed-off-by: Cezary Rojewski <cezary.rojewski at intel.com>
>>> ---
>>> sound/soc/intel/skylake/bxt-sst.c | 4 ++--
>>> sound/soc/intel/skylake/cnl-sst.c | 4 ++--
>>> sound/soc/intel/skylake/skl-sst.c | 4 ++--
>>> sound/soc/intel/skylake/skl.c | 38 ++++++++++++++++++++++++++++++-
>>> sound/soc/intel/skylake/skl.h | 3 +++
>>> 5 files changed, 46 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/sound/soc/intel/skylake/bxt-sst.c
>>> b/sound/soc/intel/skylake/bxt-sst.c
>>> index a547fb84eee9..06da822790a5 100644
>>> --- a/sound/soc/intel/skylake/bxt-sst.c
>>> +++ b/sound/soc/intel/skylake/bxt-sst.c
>>> @@ -531,7 +531,7 @@ static const struct skl_dsp_fw_ops bxt_fw_ops = {
>>> .load_library = bxt_load_library,
>>> };
>>> -static struct sst_ops skl_ops = {
>>> +struct sst_ops apl_sst_ops = {
>>> .irq_handler = skl_dsp_sst_interrupt,
>>> .thread_fn = skl_dsp_irq_thread_handler,
>>> .write = sst_shim32_write,
>>> @@ -542,7 +542,7 @@ static struct sst_ops skl_ops = {
>>> };
>>> static struct sst_pdata skl_dev = {
>>> - .ops = &skl_ops,
>>> + .ops = &apl_sst_ops,
>>> };
>>> int bxt_sst_dsp_init(struct skl_dev *skl, const char *fw_name)
>>> diff --git a/sound/soc/intel/skylake/cnl-sst.c
>>> b/sound/soc/intel/skylake/cnl-sst.c
>>> index 5be0a8eb154d..c4dbf6655097 100644
>>> --- a/sound/soc/intel/skylake/cnl-sst.c
>>> +++ b/sound/soc/intel/skylake/cnl-sst.c
>>> @@ -408,7 +408,7 @@ static int cnl_ipc_init(struct device *dev,
>>> struct skl_dev *cnl)
>>> return 0;
>>> }
>>> -static struct sst_ops cnl_ops = {
>>> +struct sst_ops cnl_sst_ops = {
>>> .irq_handler = cnl_dsp_sst_interrupt,
>>> .thread_fn = cnl_dsp_irq_thread_handler,
>>> .write = sst_shim32_write,
>>> @@ -419,7 +419,7 @@ static struct sst_ops cnl_ops = {
>>> };
>>> static struct sst_pdata cnl_dev = {
>>> - .ops = &cnl_ops,
>>> + .ops = &cnl_sst_ops,
>>> };
>>> int cnl_sst_dsp_init(struct skl_dev *cnl, const char *fw_name)
>>> diff --git a/sound/soc/intel/skylake/skl-sst.c
>>> b/sound/soc/intel/skylake/skl-sst.c
>>> index 8ae7fe73534e..122c07290440 100644
>>> --- a/sound/soc/intel/skylake/skl-sst.c
>>> +++ b/sound/soc/intel/skylake/skl-sst.c
>>> @@ -503,7 +503,7 @@ static const struct skl_dsp_fw_ops skl_fw_ops = {
>>> .unload_mod = skl_unload_module,
>>> };
>>> -static struct sst_ops skl_ops = {
>>> +struct sst_ops skl_sst_ops = {
>>> .irq_handler = skl_dsp_sst_interrupt,
>>> .write = sst_shim32_write,
>>> .read = sst_shim32_read,
>>> @@ -513,7 +513,7 @@ static struct sst_ops skl_ops = {
>>> };
>>> static struct sst_pdata skl_dev = {
>>> - .ops = &skl_ops,
>>> + .ops = &skl_sst_ops,
>>> };
>>> int skl_sst_dsp_init(struct skl_dev *skl, const char *fw_name)
>>> diff --git a/sound/soc/intel/skylake/skl.c
>>> b/sound/soc/intel/skylake/skl.c
>>> index 54e1f957121d..d6d099aba834 100644
>>> --- a/sound/soc/intel/skylake/skl.c
>>> +++ b/sound/soc/intel/skylake/skl.c
>>> @@ -27,6 +27,7 @@
>>> #include <sound/hda_i915.h>
>>> #include <sound/hda_codec.h>
>>> #include <sound/intel-nhlt.h>
>>> +#include "../common/sst-dsp.h"
>>> #include "skl.h"
>>> #include "skl-sst-dsp.h"
>>> #include "skl-sst-ipc.h"
>>> @@ -1063,7 +1064,6 @@ static int skl_probe(struct pci_dev *pci,
>>> pci_set_drvdata(skl->pci, bus);
>>> -
>>> err = skl_find_machine(skl, (void *)pci_id->driver_data);
>>> if (err < 0) {
>>> dev_err(bus->dev, "skl_find_machine failed with err: %d\n",
>>> err);
>>> @@ -1153,6 +1153,42 @@ static void skl_remove(struct pci_dev *pci)
>>> dev_set_drvdata(&pci->dev, NULL);
>>> }
>>> +static struct sst_pdata skl_desc = {
>>> + .fw_name = "intel/dsp_fw_release.bin",
>>> + .ops = &skl_sst_ops,
>>> + .boards = snd_soc_acpi_intel_skl_machines,
>>> +};
>>> +
>>> +static struct sst_pdata kbl_desc = {
>>> + .fw_name = "intel/dsp_fw_kbl.bin",
>>> + .ops = &skl_sst_ops,
>>> + .boards = snd_soc_acpi_intel_kbl_machines,
>>> +};
>>> +
>>> +static struct sst_pdata apl_desc = {
>>> + .fw_name = "intel/dsp_fw_bxtn.bin",
>>> + .ops = &apl_sst_ops,
>>> + .boards = snd_soc_acpi_intel_bxt_machines,
>>> +};
>>> +
>>> +static struct sst_pdata glk_desc = {
>>> + .fw_name = "intel/dsp_fw_glk.bin",
>>> + .ops = &apl_sst_ops,
>>> + .boards = snd_soc_acpi_intel_glk_machines,
>>> +};
>>> +
>>> +static struct sst_pdata cnl_desc = {
>>> + .fw_name = "intel/dsp_fw_cnl.bin",
>>> + .ops = &cnl_sst_ops,
>>> + .boards = snd_soc_acpi_intel_cnl_machines,
>>> +};
>>> +
>>> +static struct sst_pdata icl_desc = {
>>> + .fw_name = "intel/dsp_fw_icl.bin",
>>> + .ops = &cnl_sst_ops,
>>> + .boards = snd_soc_acpi_intel_icl_machines,
>>> +};
>>> +
>>> /* PCI IDs */
>>> static const struct pci_device_id skl_ids[] = {
>>> #if IS_ENABLED(CONFIG_SND_SOC_INTEL_SKL)
>>> diff --git a/sound/soc/intel/skylake/skl.h
>>> b/sound/soc/intel/skylake/skl.h
>>> index 9f5aa53df9f8..2f2b5a141abf 100644
>>> --- a/sound/soc/intel/skylake/skl.h
>>> +++ b/sound/soc/intel/skylake/skl.h
>>> @@ -42,6 +42,9 @@
>>> #define AZX_REG_VS_EM2_L1SEN BIT(13)
>>> struct skl_debug;
>>> +extern struct sst_ops skl_sst_ops;
>>> +extern struct sst_ops apl_sst_ops;
>>> +extern struct sst_ops cnl_sst_ops;
>>> struct skl_astate_param {
>>> u32 kcps;
>>>
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel at alsa-project.org
> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
More information about the Alsa-devel
mailing list