| Message ID | 20190822190425.23001-28-cezary.rojewski@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: Intel: Clenaup SST initialization | expand |
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. > > Signed-off-by: Cezary Rojewski <cezary.rojewski@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; >
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 (...) 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. 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. Czarek >> >> Signed-off-by: Cezary Rojewski <cezary.rojewski@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; >>
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@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@alsa-project.org > https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
On 2019-08-26 19:13, Pierre-Louis Bossart wrote: > > > 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. There is no "different filename" for /skylake topology on upstream, only fw. > >> 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. > Not arguing at all, just stating the facts. Idea behind is rather straightforward and my guess it that either you missed the key part -or- my explanations were lackluster. We do want kernel to support multiple configurations dynamically. Same goes for allowing for customizations, depending on board chosen. Although, we think topology alone is more than enough. Existing mach::fw_filename and single tplg file based on data provided from NHLT (function: skl_nhlt_update_topology_bin skl-nhlt.c) already fails us. What is present on upstream is not sufficient and thus build-bots are doing more than they are supposed to. Let uss checkout sof machine fields: - sof_fw_filename - sof_tplg_filename and then again, the skl one(s): - fw_filename Is the difference clear now? The key player here is topology file name. Once you tie board with topology you have more than enough customization to do whatever you want. As said, clever usage of topology may even allow you to skip fw_filename entirely. And this is the exact opposite of static configuration. In my opinion, SOF needs sof_fw_filename neither.
On 8/26/19 2:18 PM, Cezary Rojewski wrote: > On 2019-08-26 19:13, Pierre-Louis Bossart wrote: >> >> >> 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. > > There is no "different filename" for /skylake topology on upstream, only > fw. I don't know why we would restrict the discussion to what has been upstreamed. This is a fundamental capability that will impact non-upstreamed configurations as well. > >> >>> 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. >> > > Not arguing at all, just stating the facts. > Idea behind is rather straightforward and my guess it that either you > missed the key part -or- my explanations were lackluster. > We do want kernel to support multiple configurations dynamically. Same > goes for allowing for customizations, depending on board chosen. > Although, we think topology alone is more than enough. I am having a hard time on this one. Most quirks are ACPI or DMI based, and assuming that the topology magically tells you about the right information is a leap of faith that I am not ready to take. The complexity of the tools and skillset needed to generate a functional topology are several orders of magnitude larger than changing a file name in a C structure. Even for SOF, the topology is the source of many errors, and it feels a lot safer to keep a quirk mechanism that anyone can understand rather than assuming that everyone can generate a new topology file. There are fewer than 10 people on this planet who understand the alsa topology layers in depth, and I don't count myself in that lot... Also for new platforms it's quite common that the tools are delivered sometime after power-on steps, so in the initial steps people in the trenches make the system work with a topology that was used on a previous silicon generation. Tying hardware-related debug and topology-related debug is not recommended. > Existing mach::fw_filename and single tplg file based on data provided > from NHLT (function: skl_nhlt_update_topology_bin skl-nhlt.c) already > fails us. What is present on upstream is not sufficient and thus > build-bots are doing more than they are supposed to. > > Let uss checkout sof machine fields: > - sof_fw_filename > - sof_tplg_filename > > and then again, the skl one(s): > - fw_filename > > Is the difference clear now? > The key player here is topology file name. Once you tie board with > topology you have more than enough customization to do whatever you > want. As said, clever usage of topology may even allow you to skip > fw_filename entirely. And this is the exact opposite of static > configuration. Except that the boot flow is to first try to boot the DSP with a firmware file, then work on topology-related configurations. You would be adding a new step that is not desirable in all cases, e.g when you care about boot time. It's also not necessary simpler to base your solution on topology. We have tons of topology configurations that are exactly the same for different devices, so it's useful to avoid bundling the firmware name inside of the topology file. > In my opinion, SOF needs sof_fw_filename neither. we will definitely keep the ability to add board-specific quirks. It's very useful for e.g. derivatives and experimental cases, e.g. A/B testing with one firmware selected with one ACPI ID and another firmware selected with another ACPI ID, without mucking with the topology.
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;
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. Signed-off-by: Cezary Rojewski <cezary.rojewski@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(-)