| Message ID | 20190822190425.23001-5-cezary.rojewski@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: Intel: Clenaup SST initialization | expand |
> -int skl_sst_init_fw(struct device *dev, struct skl_dev *skl) > +int skl_sst_init_fw(struct skl_dev *skl) > { > - int ret; > struct sst_dsp *sst = skl->dsp; > + struct device *dev = skl->dev; > + int (*lp_check)(struct sst_dsp *dsp, bool state); > + int ret; > + > + lp_check = skl->ipc.ops.check_dsp_lp_on; > + skl->enable_miscbdcge(dev, false); > + skl->clock_power_gating(dev, false); > > ret = sst->fw_ops.load_fw(sst); > if (ret < 0) { > dev_err(dev, "Load base fw failed : %d\n", ret); > - return ret; > + goto exit; > + } > + > + if (!skl->is_first_boot) > + goto library_load; > + /* Disable power check during cfg setup */ > + skl->ipc.ops.check_dsp_lp_on = NULL; It's very odd to play with .ops callback dynamically. Usually ops are constant, and if you want to disable them you add a flag. > + > + ret = skl_ipc_fw_cfg_get(&skl->ipc, &skl->fw_cfg); > + if (ret < 0) { > + dev_err(dev, "Failed to get fw cfg: %d\n", ret); > + goto exit; > + } > + > + ret = skl_ipc_hw_cfg_get(&skl->ipc, &skl->hw_cfg); > + if (ret < 0) { > + dev_err(dev, "Failed to get hw cfg: %d\n", ret); > + goto exit; > } > > skl_dsp_init_core_state(sst); > > +library_load: > if (skl->lib_count > 1) { > ret = sst->fw_ops.load_library(sst, skl->lib_info, > skl->lib_count); > if (ret < 0) { > - dev_err(dev, "Load Library failed : %x\n", ret); > - return ret; > + dev_err(dev, "Load library failed : %x\n", ret); > + goto exit; > } > } > + > skl->is_first_boot = false; > +exit: > + skl->ipc.ops.check_dsp_lp_on = lp_check; > + skl->enable_miscbdcge(dev, true); > + skl->clock_power_gating(dev, true); > > - return 0; > + return ret; > } > EXPORT_SYMBOL_GPL(skl_sst_init_fw);
On 2019-08-23 20:40, Pierre-Louis Bossart wrote: > >> -int skl_sst_init_fw(struct device *dev, struct skl_dev *skl) >> +int skl_sst_init_fw(struct skl_dev *skl) >> { >> - int ret; >> struct sst_dsp *sst = skl->dsp; >> + struct device *dev = skl->dev; >> + int (*lp_check)(struct sst_dsp *dsp, bool state); >> + int ret; >> + >> + lp_check = skl->ipc.ops.check_dsp_lp_on; >> + skl->enable_miscbdcge(dev, false); >> + skl->clock_power_gating(dev, false); >> ret = sst->fw_ops.load_fw(sst); >> if (ret < 0) { >> dev_err(dev, "Load base fw failed : %d\n", ret); >> - return ret; >> + goto exit; >> + } >> + >> + if (!skl->is_first_boot) >> + goto library_load; >> + /* Disable power check during cfg setup */ >> + skl->ipc.ops.check_dsp_lp_on = NULL; > > It's very odd to play with .ops callback dynamically. Usually ops are > constant, and if you want to disable them you add a flag. > Yeye, keen eye! Can't do everything at once though :/ The power check is APL+ specific and should not be part of generic ipc framework at all (found in /sound/soc/intel/common/sst-ipc.c). Different fate awaits said check. For now, in this single case it seems best to simply disable the check and reapply it once setup is done. >> + >> + ret = skl_ipc_fw_cfg_get(&skl->ipc, &skl->fw_cfg); >> + if (ret < 0) { >> + dev_err(dev, "Failed to get fw cfg: %d\n", ret); >> + goto exit; >> + } >> + >> + ret = skl_ipc_hw_cfg_get(&skl->ipc, &skl->hw_cfg); >> + if (ret < 0) { >> + dev_err(dev, "Failed to get hw cfg: %d\n", ret); >> + goto exit; >> } >> skl_dsp_init_core_state(sst); >> +library_load: >> if (skl->lib_count > 1) { >> ret = sst->fw_ops.load_library(sst, skl->lib_info, >> skl->lib_count); >> if (ret < 0) { >> - dev_err(dev, "Load Library failed : %x\n", ret); >> - return ret; >> + dev_err(dev, "Load library failed : %x\n", ret); >> + goto exit; >> } >> } >> + >> skl->is_first_boot = false; >> +exit: >> + skl->ipc.ops.check_dsp_lp_on = lp_check; >> + skl->enable_miscbdcge(dev, true); >> + skl->clock_power_gating(dev, true); >> - return 0; >> + return ret; >> } >> EXPORT_SYMBOL_GPL(skl_sst_init_fw);
On 8/24/19 4:34 AM, Cezary Rojewski wrote: > On 2019-08-23 20:40, Pierre-Louis Bossart wrote: >> >>> -int skl_sst_init_fw(struct device *dev, struct skl_dev *skl) >>> +int skl_sst_init_fw(struct skl_dev *skl) >>> { >>> - int ret; >>> struct sst_dsp *sst = skl->dsp; >>> + struct device *dev = skl->dev; >>> + int (*lp_check)(struct sst_dsp *dsp, bool state); >>> + int ret; >>> + >>> + lp_check = skl->ipc.ops.check_dsp_lp_on; >>> + skl->enable_miscbdcge(dev, false); >>> + skl->clock_power_gating(dev, false); >>> ret = sst->fw_ops.load_fw(sst); >>> if (ret < 0) { >>> dev_err(dev, "Load base fw failed : %d\n", ret); >>> - return ret; >>> + goto exit; >>> + } >>> + >>> + if (!skl->is_first_boot) >>> + goto library_load; >>> + /* Disable power check during cfg setup */ >>> + skl->ipc.ops.check_dsp_lp_on = NULL; >> >> It's very odd to play with .ops callback dynamically. Usually ops are >> constant, and if you want to disable them you add a flag. >> > > Yeye, keen eye! Can't do everything at once though :/ > The power check is APL+ specific and should not be part of generic ipc > framework at all (found in /sound/soc/intel/common/sst-ipc.c). Different > fate awaits said check. For now, in this single case it seems best to > simply disable the check and reapply it once setup is done. What's the difference with having this callback do nothing for APL-? > >>> + >>> + ret = skl_ipc_fw_cfg_get(&skl->ipc, &skl->fw_cfg); >>> + if (ret < 0) { >>> + dev_err(dev, "Failed to get fw cfg: %d\n", ret); >>> + goto exit; >>> + } >>> + >>> + ret = skl_ipc_hw_cfg_get(&skl->ipc, &skl->hw_cfg); >>> + if (ret < 0) { >>> + dev_err(dev, "Failed to get hw cfg: %d\n", ret); >>> + goto exit; >>> } >>> skl_dsp_init_core_state(sst); >>> +library_load: >>> if (skl->lib_count > 1) { >>> ret = sst->fw_ops.load_library(sst, skl->lib_info, >>> skl->lib_count); >>> if (ret < 0) { >>> - dev_err(dev, "Load Library failed : %x\n", ret); >>> - return ret; >>> + dev_err(dev, "Load library failed : %x\n", ret); >>> + goto exit; >>> } >>> } >>> + >>> skl->is_first_boot = false; >>> +exit: >>> + skl->ipc.ops.check_dsp_lp_on = lp_check; >>> + skl->enable_miscbdcge(dev, true); >>> + skl->clock_power_gating(dev, true); >>> - return 0; >>> + return ret; >>> } >>> EXPORT_SYMBOL_GPL(skl_sst_init_fw);
On 2019-08-26 18:31, Pierre-Louis Bossart wrote: > > > On 8/24/19 4:34 AM, Cezary Rojewski wrote: >> On 2019-08-23 20:40, Pierre-Louis Bossart wrote: >>> >>>> -int skl_sst_init_fw(struct device *dev, struct skl_dev *skl) >>>> +int skl_sst_init_fw(struct skl_dev *skl) >>>> { >>>> - int ret; >>>> struct sst_dsp *sst = skl->dsp; >>>> + struct device *dev = skl->dev; >>>> + int (*lp_check)(struct sst_dsp *dsp, bool state); >>>> + int ret; >>>> + >>>> + lp_check = skl->ipc.ops.check_dsp_lp_on; >>>> + skl->enable_miscbdcge(dev, false); >>>> + skl->clock_power_gating(dev, false); >>>> ret = sst->fw_ops.load_fw(sst); >>>> if (ret < 0) { >>>> dev_err(dev, "Load base fw failed : %d\n", ret); >>>> - return ret; >>>> + goto exit; >>>> + } >>>> + >>>> + if (!skl->is_first_boot) >>>> + goto library_load; >>>> + /* Disable power check during cfg setup */ >>>> + skl->ipc.ops.check_dsp_lp_on = NULL; >>> >>> It's very odd to play with .ops callback dynamically. Usually ops are >>> constant, and if you want to disable them you add a flag. >>> >> >> Yeye, keen eye! Can't do everything at once though :/ >> The power check is APL+ specific and should not be part of generic ipc >> framework at all (found in /sound/soc/intel/common/sst-ipc.c). >> Different fate awaits said check. For now, in this single case it >> seems best to simply disable the check and reapply it once setup is done. > > What's the difference with having this callback do nothing for APL-? > The entire check_dsp_lp_on is actually D0ix thingy. -- The power-management is being addressed in following segment. -- D0ix is a power-optimization feature and implemented in APL and onward. So for SKL/ KBL there is no check_dsp_lp_on. You can see the difference in /skylake/skl-sst.c: static const struct skl_dsp_fw_ops skl_fw_ops (lack of set_state_D0ix handlers) Once dsp-host interaction can be described as idle (e.g. ongoing playback with no IPC traffic), host may enable FW to power gate some components and thus reduce power consumption. D0ix is abbrev for D0i0 <-> D0i3 transitions. Once dsp enters D0i3 (power gated), no IPCs can be consumed apart from one and only SetD0ix - to wake dsp back to D0i0 state. In general you can think of D0i0 as D0. Again, since SKL/ KBL have no D0ix, there is no need to wake anything, no checks are required.
diff --git a/sound/soc/intel/skylake/bxt-sst.c b/sound/soc/intel/skylake/bxt-sst.c index 92a82e6b5fe6..5bece3a6d741 100644 --- a/sound/soc/intel/skylake/bxt-sst.c +++ b/sound/soc/intel/skylake/bxt-sst.c @@ -591,33 +591,6 @@ int bxt_sst_dsp_init(struct device *dev, void __iomem *mmio_base, int irq, } EXPORT_SYMBOL_GPL(bxt_sst_dsp_init); -int bxt_sst_init_fw(struct device *dev, struct skl_dev *skl) -{ - int ret; - struct sst_dsp *sst = skl->dsp; - - ret = sst->fw_ops.load_fw(sst); - if (ret < 0) { - dev_err(dev, "Load base fw failed: %x\n", ret); - return ret; - } - - skl_dsp_init_core_state(sst); - - if (skl->lib_count > 1) { - ret = sst->fw_ops.load_library(sst, skl->lib_info, - skl->lib_count); - if (ret < 0) { - dev_err(dev, "Load Library failed : %x\n", ret); - return ret; - } - } - skl->is_first_boot = false; - - return 0; -} -EXPORT_SYMBOL_GPL(bxt_sst_init_fw); - void bxt_sst_dsp_cleanup(struct device *dev, struct skl_dev *skl) { diff --git a/sound/soc/intel/skylake/cnl-sst-dsp.h b/sound/soc/intel/skylake/cnl-sst-dsp.h index 7bd4d2a8fdfa..50f4a53a607c 100644 --- a/sound/soc/intel/skylake/cnl-sst-dsp.h +++ b/sound/soc/intel/skylake/cnl-sst-dsp.h @@ -97,7 +97,6 @@ void cnl_ipc_free(struct sst_generic_ipc *ipc); int cnl_sst_dsp_init(struct device *dev, void __iomem *mmio_base, int irq, const char *fw_name, struct skl_dsp_loader_ops dsp_ops, struct skl_dev **dsp); -int cnl_sst_init_fw(struct device *dev, struct skl_dev *skl); void cnl_sst_dsp_cleanup(struct device *dev, struct skl_dev *skl); #endif /*__CNL_SST_DSP_H__*/ diff --git a/sound/soc/intel/skylake/cnl-sst.c b/sound/soc/intel/skylake/cnl-sst.c index 4f64f097e9ae..8984653d925d 100644 --- a/sound/soc/intel/skylake/cnl-sst.c +++ b/sound/soc/intel/skylake/cnl-sst.c @@ -453,25 +453,6 @@ int cnl_sst_dsp_init(struct device *dev, void __iomem *mmio_base, int irq, } EXPORT_SYMBOL_GPL(cnl_sst_dsp_init); -int cnl_sst_init_fw(struct device *dev, struct skl_dev *skl) -{ - int ret; - struct sst_dsp *sst = skl->dsp; - - ret = skl->dsp->fw_ops.load_fw(sst); - if (ret < 0) { - dev_err(dev, "load base fw failed: %d", ret); - return ret; - } - - skl_dsp_init_core_state(sst); - - skl->is_first_boot = false; - - return 0; -} -EXPORT_SYMBOL_GPL(cnl_sst_init_fw); - void cnl_sst_dsp_cleanup(struct device *dev, struct skl_dev *skl) { if (skl->dsp->fw) diff --git a/sound/soc/intel/skylake/skl-messages.c b/sound/soc/intel/skylake/skl-messages.c index d28b4887de27..cc949904717e 100644 --- a/sound/soc/intel/skylake/skl-messages.c +++ b/sound/soc/intel/skylake/skl-messages.c @@ -173,7 +173,6 @@ static const struct skl_dsp_ops dsp_ops[] = { .num_cores = 2, .loader_ops = skl_get_loader_ops, .init = skl_sst_dsp_init, - .init_fw = skl_sst_init_fw, .cleanup = skl_sst_dsp_cleanup }, { @@ -181,7 +180,6 @@ static const struct skl_dsp_ops dsp_ops[] = { .num_cores = 2, .loader_ops = skl_get_loader_ops, .init = skl_sst_dsp_init, - .init_fw = skl_sst_init_fw, .cleanup = skl_sst_dsp_cleanup }, { @@ -189,7 +187,6 @@ static const struct skl_dsp_ops dsp_ops[] = { .num_cores = 2, .loader_ops = bxt_get_loader_ops, .init = bxt_sst_dsp_init, - .init_fw = bxt_sst_init_fw, .cleanup = bxt_sst_dsp_cleanup }, { @@ -197,7 +194,6 @@ static const struct skl_dsp_ops dsp_ops[] = { .num_cores = 2, .loader_ops = bxt_get_loader_ops, .init = bxt_sst_dsp_init, - .init_fw = bxt_sst_init_fw, .cleanup = bxt_sst_dsp_cleanup }, { @@ -205,7 +201,6 @@ static const struct skl_dsp_ops dsp_ops[] = { .num_cores = 4, .loader_ops = bxt_get_loader_ops, .init = cnl_sst_dsp_init, - .init_fw = cnl_sst_init_fw, .cleanup = cnl_sst_dsp_cleanup }, { @@ -213,7 +208,6 @@ static const struct skl_dsp_ops dsp_ops[] = { .num_cores = 4, .loader_ops = bxt_get_loader_ops, .init = cnl_sst_dsp_init, - .init_fw = cnl_sst_init_fw, .cleanup = cnl_sst_dsp_cleanup }, { @@ -221,7 +215,6 @@ static const struct skl_dsp_ops dsp_ops[] = { .num_cores = 4, .loader_ops = bxt_get_loader_ops, .init = cnl_sst_dsp_init, - .init_fw = cnl_sst_init_fw, .cleanup = cnl_sst_dsp_cleanup }, { @@ -229,7 +222,6 @@ static const struct skl_dsp_ops dsp_ops[] = { .num_cores = 4, .loader_ops = bxt_get_loader_ops, .init = cnl_sst_dsp_init, - .init_fw = cnl_sst_init_fw, .cleanup = cnl_sst_dsp_cleanup }, }; diff --git a/sound/soc/intel/skylake/skl-pcm.c b/sound/soc/intel/skylake/skl-pcm.c index 7f287424af9b..1dbab3eac0e5 100644 --- a/sound/soc/intel/skylake/skl-pcm.c +++ b/sound/soc/intel/skylake/skl-pcm.c @@ -1408,7 +1408,6 @@ static int skl_platform_soc_probe(struct snd_soc_component *component) { struct hdac_bus *bus = dev_get_drvdata(component->dev); struct skl_dev *skl = bus_to_skl(bus); - const struct skl_dsp_ops *ops; int ret; pm_runtime_get_sync(component->dev); @@ -1424,25 +1423,10 @@ static int skl_platform_soc_probe(struct snd_soc_component *component) return ret; } - /* load the firmwares, since all is set */ - ops = skl_get_dsp_ops(skl->pci->device); - if (!ops) - return -EIO; - - /* - * Disable dynamic clock and power gating during firmware - * and library download - */ - skl->enable_miscbdcge(component->dev, false); - skl->clock_power_gating(component->dev, false); - - ret = ops->init_fw(component->dev, skl); - skl->enable_miscbdcge(component->dev, true); - skl->clock_power_gating(component->dev, true); - if (ret < 0) { - dev_err(component->dev, "Failed to boot first fw: %d\n", ret); + ret = skl_sst_init_fw(skl); + if (ret < 0) return ret; - } + skl_populate_modules(skl); skl->update_d0i3c = skl_update_d0i3c; diff --git a/sound/soc/intel/skylake/skl-sst-dsp.c b/sound/soc/intel/skylake/skl-sst-dsp.c index 225706d148d8..0eecf26986f9 100644 --- a/sound/soc/intel/skylake/skl-sst-dsp.c +++ b/sound/soc/intel/skylake/skl-sst-dsp.c @@ -44,6 +44,7 @@ void skl_dsp_init_core_state(struct sst_dsp *ctx) skl->cores.usage_count[i] = 0; } } +EXPORT_SYMBOL_GPL(skl_dsp_init_core_state); /* Get the mask for all enabled cores */ unsigned int skl_dsp_get_enabled_cores(struct sst_dsp *ctx) diff --git a/sound/soc/intel/skylake/skl-sst-dsp.h b/sound/soc/intel/skylake/skl-sst-dsp.h index cdfec0fca577..4da240582454 100644 --- a/sound/soc/intel/skylake/skl-sst-dsp.h +++ b/sound/soc/intel/skylake/skl-sst-dsp.h @@ -226,8 +226,6 @@ int skl_sst_dsp_init(struct device *dev, void __iomem *mmio_base, int irq, int bxt_sst_dsp_init(struct device *dev, void __iomem *mmio_base, int irq, const char *fw_name, struct skl_dsp_loader_ops dsp_ops, struct skl_dev **dsp); -int skl_sst_init_fw(struct device *dev, struct skl_dev *skl); -int bxt_sst_init_fw(struct device *dev, struct skl_dev *skl); void skl_sst_dsp_cleanup(struct device *dev, struct skl_dev *skl); void bxt_sst_dsp_cleanup(struct device *dev, struct skl_dev *skl); diff --git a/sound/soc/intel/skylake/skl-sst.c b/sound/soc/intel/skylake/skl-sst.c index 8af7546def1f..8a8ecb9a4fc6 100644 --- a/sound/soc/intel/skylake/skl-sst.c +++ b/sound/soc/intel/skylake/skl-sst.c @@ -555,30 +555,59 @@ int skl_sst_dsp_init(struct device *dev, void __iomem *mmio_base, int irq, } EXPORT_SYMBOL_GPL(skl_sst_dsp_init); -int skl_sst_init_fw(struct device *dev, struct skl_dev *skl) +int skl_sst_init_fw(struct skl_dev *skl) { - int ret; struct sst_dsp *sst = skl->dsp; + struct device *dev = skl->dev; + int (*lp_check)(struct sst_dsp *dsp, bool state); + int ret; + + lp_check = skl->ipc.ops.check_dsp_lp_on; + skl->enable_miscbdcge(dev, false); + skl->clock_power_gating(dev, false); ret = sst->fw_ops.load_fw(sst); if (ret < 0) { dev_err(dev, "Load base fw failed : %d\n", ret); - return ret; + goto exit; + } + + if (!skl->is_first_boot) + goto library_load; + /* Disable power check during cfg setup */ + skl->ipc.ops.check_dsp_lp_on = NULL; + + ret = skl_ipc_fw_cfg_get(&skl->ipc, &skl->fw_cfg); + if (ret < 0) { + dev_err(dev, "Failed to get fw cfg: %d\n", ret); + goto exit; + } + + ret = skl_ipc_hw_cfg_get(&skl->ipc, &skl->hw_cfg); + if (ret < 0) { + dev_err(dev, "Failed to get hw cfg: %d\n", ret); + goto exit; } skl_dsp_init_core_state(sst); +library_load: if (skl->lib_count > 1) { ret = sst->fw_ops.load_library(sst, skl->lib_info, skl->lib_count); if (ret < 0) { - dev_err(dev, "Load Library failed : %x\n", ret); - return ret; + dev_err(dev, "Load library failed : %x\n", ret); + goto exit; } } + skl->is_first_boot = false; +exit: + skl->ipc.ops.check_dsp_lp_on = lp_check; + skl->enable_miscbdcge(dev, true); + skl->clock_power_gating(dev, true); - return 0; + return ret; } EXPORT_SYMBOL_GPL(skl_sst_init_fw); diff --git a/sound/soc/intel/skylake/skl.h b/sound/soc/intel/skylake/skl.h index 972de5ddf2b7..1f86543fe954 100644 --- a/sound/soc/intel/skylake/skl.h +++ b/sound/soc/intel/skylake/skl.h @@ -161,7 +161,6 @@ struct skl_dsp_ops { int irq, const char *fw_name, struct skl_dsp_loader_ops loader_ops, struct skl_dev **skl_sst); - int (*init_fw)(struct device *dev, struct skl_dev *skl); void (*cleanup)(struct device *dev, struct skl_dev *skl); }; @@ -175,6 +174,7 @@ struct nhlt_specific_cfg *skl_get_ep_blob(struct skl_dev *skl, u32 instance, int skl_nhlt_update_topology_bin(struct skl_dev *skl); int skl_init_dsp(struct skl_dev *skl); int skl_free_dsp(struct skl_dev *skl); +int skl_sst_init_fw(struct skl_dev *skl); int skl_suspend_late_dsp(struct skl_dev *skl); int skl_suspend_dsp(struct skl_dev *skl); int skl_resume_dsp(struct skl_dev *skl);
There are certain operations we want to do before and after firmware loading e.g.: disabling/ enabling power and clock gating. To make code coherent, provide skl_init_fw as a unified way for loading dsp firmware. In consequence, this change provides CNL load library support during fw initialization which was previously missing. skl_dsp_fw_ops already takes care of fw and library load customization. New post-load additions in form of fw and hw config assignments make this change even more welcome. Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com> --- sound/soc/intel/skylake/bxt-sst.c | 27 ----------------- sound/soc/intel/skylake/cnl-sst-dsp.h | 1 - sound/soc/intel/skylake/cnl-sst.c | 19 ------------ sound/soc/intel/skylake/skl-messages.c | 8 ----- sound/soc/intel/skylake/skl-pcm.c | 22 ++------------ sound/soc/intel/skylake/skl-sst-dsp.c | 1 + sound/soc/intel/skylake/skl-sst-dsp.h | 2 -- sound/soc/intel/skylake/skl-sst.c | 41 ++++++++++++++++++++++---- sound/soc/intel/skylake/skl.h | 2 +- 9 files changed, 40 insertions(+), 83 deletions(-)