| Message ID | 20190822190425.23001-3-cezary.rojewski@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: Intel: Clenaup SST initialization | expand |
On 8/22/19 2:03 PM, Cezary Rojewski wrote: > Implement interface for retrieving firmware configuration. Skylake > driver will use this data instead of hardcoded values in updates to > come. > > Most params are currently unused. In time driver dependency on fw config > will increase, and with it, more parsing will be unveiled. > > Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com> > --- > sound/soc/intel/skylake/skl-sst-ipc.c | 122 ++++++++++++++++++++++++++ > sound/soc/intel/skylake/skl-sst-ipc.h | 72 +++++++++++++++ > sound/soc/intel/skylake/skl.h | 1 + > 3 files changed, 195 insertions(+) > > diff --git a/sound/soc/intel/skylake/skl-sst-ipc.c b/sound/soc/intel/skylake/skl-sst-ipc.c > index 667cdddc289f..e9e11ec4c97b 100644 > --- a/sound/soc/intel/skylake/skl-sst-ipc.c > +++ b/sound/soc/intel/skylake/skl-sst-ipc.c > @@ -11,6 +11,7 @@ > #include "skl.h" > #include "skl-sst-dsp.h" > #include "skl-sst-ipc.h" > +#include "skl-topology.h" > #include "sound/hdaudio_ext.h" > > > @@ -1067,3 +1068,124 @@ int skl_ipc_set_d0ix(struct sst_generic_ipc *ipc, struct skl_ipc_d0ix_msg *msg) > return ret; > } > EXPORT_SYMBOL_GPL(skl_ipc_set_d0ix); > + > +int skl_ipc_fw_cfg_get(struct sst_generic_ipc *ipc, struct skl_fw_cfg *cfg) > +{ > + struct skl_ipc_large_config_msg msg = {0}; > + struct skl_tlv *tlv; > + size_t bytes = 0, offset = 0; > + u8 *payload = NULL; > + int ret; > + > + msg.module_id = 0; > + msg.instance_id = 0; > + msg.large_param_id = SKL_BASEFW_FIRMWARE_CONFIG; > + > + ret = skl_ipc_get_large_config(ipc, &msg, (u32 **)&payload, &bytes); > + if (ret) > + goto exit; > + > + while (offset < bytes) { > + tlv = (struct skl_tlv *)(payload + offset); > + > + switch (tlv->type) { > + case SKL_FW_CFG_FW_VERSION: > + memcpy(&cfg->fw_version, tlv->value, > + sizeof(cfg->fw_version)); > + break; > + > + case SKL_FW_CFG_MEMORY_RECLAIMED: > + cfg->memory_reclaimed = *tlv->value; > + break; > + > + case SKL_FW_CFG_SLOW_CLOCK_FREQ_HZ: > + cfg->slow_clock_freq_hz = *tlv->value; > + break; > + > + case SKL_FW_CFG_FAST_CLOCK_FREQ_HZ: > + cfg->fast_clock_freq_hz = *tlv->value; > + break; > + > + case SKL_FW_CFG_ALH_SUPPORT_LEVEL: > + cfg->alh_support = *tlv->value; > + break; > + > + case SKL_FW_CFG_IPC_DL_MAILBOX_BYTES: > + cfg->ipc_dl_mailbox_bytes = *tlv->value; > + break; > + > + case SKL_FW_CFG_IPC_UL_MAILBOX_BYTES: > + cfg->ipc_ul_mailbox_bytes = *tlv->value; > + break; > + > + case SKL_FW_CFG_TRACE_LOG_BYTES: > + cfg->trace_log_bytes = *tlv->value; > + break; > + > + case SKL_FW_CFG_MAX_PPL_COUNT: > + cfg->max_ppl_count = *tlv->value; > + break; > + > + case SKL_FW_CFG_MAX_ASTATE_COUNT: > + cfg->max_astate_count = *tlv->value; > + break; > + > + case SKL_FW_CFG_MAX_MODULE_PIN_COUNT: > + cfg->max_module_pin_count = *tlv->value; > + break; > + > + case SKL_FW_CFG_MODULES_COUNT: > + cfg->modules_count = *tlv->value; > + break; > + > + case SKL_FW_CFG_MAX_MOD_INST_COUNT: > + cfg->max_mod_inst_count = *tlv->value; > + break; > + > + case SKL_FW_CFG_MAX_LL_TASKS_PER_PRI_COUNT: > + cfg->max_ll_tasks_per_pri_count = *tlv->value; > + break; > + > + case SKL_FW_CFG_LL_PRI_COUNT: > + cfg->ll_pri_count = *tlv->value; > + break; > + > + case SKL_FW_CFG_MAX_DP_TASKS_COUNT: > + cfg->max_dp_tasks_count = *tlv->value; > + break; > + > + case SKL_FW_CFG_MAX_LIBS_COUNT: > + cfg->max_libs_count = *tlv->value; > + break; > + > + case SKL_FW_CFG_XTAL_FREQ_HZ: > + cfg->xtal_freq_hz = *tlv->value; > + break; > + > + case SKL_FW_CFG_UAOL_SUPPORT: > + cfg->uaol_support = *tlv->value; > + break; > + > + case SKL_FW_CFG_POWER_GATING_POLICY: > + cfg->power_gating_policy = *tlv->value; > + break; > + > + case SKL_FW_CFG_DMA_BUFFER_CONFIG: > + case SKL_FW_CFG_SCHEDULER_CONFIG: > + case SKL_FW_CFG_CLOCKS_CONFIG: > + break; > + > + default: > + dev_info(ipc->dev, "Unrecognized fw param: %d\n", > + tlv->type); > + break; Isn't this an error? If there are other possible values, why not list them and skip them, as done above? > + } > + > + offset += sizeof(*tlv) + tlv->length; > + } > + > +exit: > + kfree(payload); > + return ret; > +} > +EXPORT_SYMBOL_GPL(skl_ipc_fw_cfg_get); > +enum skl_alh_support_level { > + ALH_NO_SUPPORT = 0x00000, > + ALH_CAVS_1_8_CNL = 0x10000, > +}; Support for ALH hasn't changed even past 1.8, and references to CNL are probably not needed.
On 2019-08-23 20:24, Pierre-Louis Bossart wrote: > > > On 8/22/19 2:03 PM, Cezary Rojewski wrote: >> Implement interface for retrieving firmware configuration. Skylake >> driver will use this data instead of hardcoded values in updates to >> come. >> >> Most params are currently unused. In time driver dependency on fw config >> will increase, and with it, more parsing will be unveiled. >> >> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com> >> --- >> sound/soc/intel/skylake/skl-sst-ipc.c | 122 ++++++++++++++++++++++++++ >> sound/soc/intel/skylake/skl-sst-ipc.h | 72 +++++++++++++++ >> sound/soc/intel/skylake/skl.h | 1 + >> 3 files changed, 195 insertions(+) >> >> diff --git a/sound/soc/intel/skylake/skl-sst-ipc.c >> b/sound/soc/intel/skylake/skl-sst-ipc.c >> index 667cdddc289f..e9e11ec4c97b 100644 >> --- a/sound/soc/intel/skylake/skl-sst-ipc.c >> +++ b/sound/soc/intel/skylake/skl-sst-ipc.c >> @@ -11,6 +11,7 @@ >> #include "skl.h" >> #include "skl-sst-dsp.h" >> #include "skl-sst-ipc.h" >> +#include "skl-topology.h" >> #include "sound/hdaudio_ext.h" >> @@ -1067,3 +1068,124 @@ int skl_ipc_set_d0ix(struct sst_generic_ipc >> *ipc, struct skl_ipc_d0ix_msg *msg) >> return ret; >> } >> EXPORT_SYMBOL_GPL(skl_ipc_set_d0ix); >> + >> +int skl_ipc_fw_cfg_get(struct sst_generic_ipc *ipc, struct skl_fw_cfg >> *cfg) >> +{ >> + struct skl_ipc_large_config_msg msg = {0}; >> + struct skl_tlv *tlv; >> + size_t bytes = 0, offset = 0; >> + u8 *payload = NULL; >> + int ret; >> + >> + msg.module_id = 0; >> + msg.instance_id = 0; >> + msg.large_param_id = SKL_BASEFW_FIRMWARE_CONFIG; >> + >> + ret = skl_ipc_get_large_config(ipc, &msg, (u32 **)&payload, &bytes); >> + if (ret) >> + goto exit; >> + >> + while (offset < bytes) { >> + tlv = (struct skl_tlv *)(payload + offset); >> + >> + switch (tlv->type) { >> + case SKL_FW_CFG_FW_VERSION: >> + memcpy(&cfg->fw_version, tlv->value, >> + sizeof(cfg->fw_version)); >> + break; >> + >> + case SKL_FW_CFG_MEMORY_RECLAIMED: >> + cfg->memory_reclaimed = *tlv->value; >> + break; >> + >> + case SKL_FW_CFG_SLOW_CLOCK_FREQ_HZ: >> + cfg->slow_clock_freq_hz = *tlv->value; >> + break; >> + >> + case SKL_FW_CFG_FAST_CLOCK_FREQ_HZ: >> + cfg->fast_clock_freq_hz = *tlv->value; >> + break; >> + >> + case SKL_FW_CFG_ALH_SUPPORT_LEVEL: >> + cfg->alh_support = *tlv->value; >> + break; >> + >> + case SKL_FW_CFG_IPC_DL_MAILBOX_BYTES: >> + cfg->ipc_dl_mailbox_bytes = *tlv->value; >> + break; >> + >> + case SKL_FW_CFG_IPC_UL_MAILBOX_BYTES: >> + cfg->ipc_ul_mailbox_bytes = *tlv->value; >> + break; >> + >> + case SKL_FW_CFG_TRACE_LOG_BYTES: >> + cfg->trace_log_bytes = *tlv->value; >> + break; >> + >> + case SKL_FW_CFG_MAX_PPL_COUNT: >> + cfg->max_ppl_count = *tlv->value; >> + break; >> + >> + case SKL_FW_CFG_MAX_ASTATE_COUNT: >> + cfg->max_astate_count = *tlv->value; >> + break; >> + >> + case SKL_FW_CFG_MAX_MODULE_PIN_COUNT: >> + cfg->max_module_pin_count = *tlv->value; >> + break; >> + >> + case SKL_FW_CFG_MODULES_COUNT: >> + cfg->modules_count = *tlv->value; >> + break; >> + >> + case SKL_FW_CFG_MAX_MOD_INST_COUNT: >> + cfg->max_mod_inst_count = *tlv->value; >> + break; >> + >> + case SKL_FW_CFG_MAX_LL_TASKS_PER_PRI_COUNT: >> + cfg->max_ll_tasks_per_pri_count = *tlv->value; >> + break; >> + >> + case SKL_FW_CFG_LL_PRI_COUNT: >> + cfg->ll_pri_count = *tlv->value; >> + break; >> + >> + case SKL_FW_CFG_MAX_DP_TASKS_COUNT: >> + cfg->max_dp_tasks_count = *tlv->value; >> + break; >> + >> + case SKL_FW_CFG_MAX_LIBS_COUNT: >> + cfg->max_libs_count = *tlv->value; >> + break; >> + >> + case SKL_FW_CFG_XTAL_FREQ_HZ: >> + cfg->xtal_freq_hz = *tlv->value; >> + break; >> + >> + case SKL_FW_CFG_UAOL_SUPPORT: >> + cfg->uaol_support = *tlv->value; >> + break; >> + >> + case SKL_FW_CFG_POWER_GATING_POLICY: >> + cfg->power_gating_policy = *tlv->value; >> + break; >> + >> + case SKL_FW_CFG_DMA_BUFFER_CONFIG: >> + case SKL_FW_CFG_SCHEDULER_CONFIG: >> + case SKL_FW_CFG_CLOCKS_CONFIG: >> + break; >> + >> + default: >> + dev_info(ipc->dev, "Unrecognized fw param: %d\n", >> + tlv->type); >> + break; > > Isn't this an error? > If there are other possible values, why not list them and skip them, as > done above? > Pretty sure I cannot share names for all capabilities as these are EMBARGOed. Moreover, both FW_CFG and HW_CFG and constantly being updated and thus new constants are appended. I find it best to "break" when encountering known and un-EMBARGOED constant and simply dump info value if the opposite is true. New capabilities are always tied to newer platforms. If there will be a need for adding them, these won't be even present in SKL/KBL and such FWs. At the same time if given binary drop contains something new but not required to be handled by host in generic case, dumping error is counter-intuitive. >> + } >> + >> + offset += sizeof(*tlv) + tlv->length; >> + } >> + >> +exit: >> + kfree(payload); >> + return ret; >> +} >> +EXPORT_SYMBOL_GPL(skl_ipc_fw_cfg_get); > >> +enum skl_alh_support_level { >> + ALH_NO_SUPPORT = 0x00000, >> + ALH_CAVS_1_8_CNL = 0x10000, >> +}; > > Support for ALH hasn't changed even past 1.8, and references to CNL are > probably not needed. These are FW types and are here to be left untouched. Ensures parsed values on host side match FW side.
On 8/24/19 4:17 AM, Cezary Rojewski wrote: > On 2019-08-23 20:24, Pierre-Louis Bossart wrote: >> >> >> On 8/22/19 2:03 PM, Cezary Rojewski wrote: >>> Implement interface for retrieving firmware configuration. Skylake >>> driver will use this data instead of hardcoded values in updates to >>> come. >>> >>> Most params are currently unused. In time driver dependency on fw config >>> will increase, and with it, more parsing will be unveiled. >>> >>> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com> >>> --- >>> sound/soc/intel/skylake/skl-sst-ipc.c | 122 ++++++++++++++++++++++++++ >>> sound/soc/intel/skylake/skl-sst-ipc.h | 72 +++++++++++++++ >>> sound/soc/intel/skylake/skl.h | 1 + >>> 3 files changed, 195 insertions(+) >>> >>> diff --git a/sound/soc/intel/skylake/skl-sst-ipc.c >>> b/sound/soc/intel/skylake/skl-sst-ipc.c >>> index 667cdddc289f..e9e11ec4c97b 100644 >>> --- a/sound/soc/intel/skylake/skl-sst-ipc.c >>> +++ b/sound/soc/intel/skylake/skl-sst-ipc.c >>> @@ -11,6 +11,7 @@ >>> #include "skl.h" >>> #include "skl-sst-dsp.h" >>> #include "skl-sst-ipc.h" >>> +#include "skl-topology.h" >>> #include "sound/hdaudio_ext.h" >>> @@ -1067,3 +1068,124 @@ int skl_ipc_set_d0ix(struct sst_generic_ipc >>> *ipc, struct skl_ipc_d0ix_msg *msg) >>> return ret; >>> } >>> EXPORT_SYMBOL_GPL(skl_ipc_set_d0ix); >>> + >>> +int skl_ipc_fw_cfg_get(struct sst_generic_ipc *ipc, struct >>> skl_fw_cfg *cfg) >>> +{ >>> + struct skl_ipc_large_config_msg msg = {0}; >>> + struct skl_tlv *tlv; >>> + size_t bytes = 0, offset = 0; >>> + u8 *payload = NULL; >>> + int ret; >>> + >>> + msg.module_id = 0; >>> + msg.instance_id = 0; >>> + msg.large_param_id = SKL_BASEFW_FIRMWARE_CONFIG; >>> + >>> + ret = skl_ipc_get_large_config(ipc, &msg, (u32 **)&payload, >>> &bytes); >>> + if (ret) >>> + goto exit; >>> + >>> + while (offset < bytes) { >>> + tlv = (struct skl_tlv *)(payload + offset); >>> + >>> + switch (tlv->type) { >>> + case SKL_FW_CFG_FW_VERSION: >>> + memcpy(&cfg->fw_version, tlv->value, >>> + sizeof(cfg->fw_version)); >>> + break; >>> + >>> + case SKL_FW_CFG_MEMORY_RECLAIMED: >>> + cfg->memory_reclaimed = *tlv->value; >>> + break; >>> + >>> + case SKL_FW_CFG_SLOW_CLOCK_FREQ_HZ: >>> + cfg->slow_clock_freq_hz = *tlv->value; >>> + break; >>> + >>> + case SKL_FW_CFG_FAST_CLOCK_FREQ_HZ: >>> + cfg->fast_clock_freq_hz = *tlv->value; >>> + break; >>> + >>> + case SKL_FW_CFG_ALH_SUPPORT_LEVEL: >>> + cfg->alh_support = *tlv->value; >>> + break; >>> + >>> + case SKL_FW_CFG_IPC_DL_MAILBOX_BYTES: >>> + cfg->ipc_dl_mailbox_bytes = *tlv->value; >>> + break; >>> + >>> + case SKL_FW_CFG_IPC_UL_MAILBOX_BYTES: >>> + cfg->ipc_ul_mailbox_bytes = *tlv->value; >>> + break; >>> + >>> + case SKL_FW_CFG_TRACE_LOG_BYTES: >>> + cfg->trace_log_bytes = *tlv->value; >>> + break; >>> + >>> + case SKL_FW_CFG_MAX_PPL_COUNT: >>> + cfg->max_ppl_count = *tlv->value; >>> + break; >>> + >>> + case SKL_FW_CFG_MAX_ASTATE_COUNT: >>> + cfg->max_astate_count = *tlv->value; >>> + break; >>> + >>> + case SKL_FW_CFG_MAX_MODULE_PIN_COUNT: >>> + cfg->max_module_pin_count = *tlv->value; >>> + break; >>> + >>> + case SKL_FW_CFG_MODULES_COUNT: >>> + cfg->modules_count = *tlv->value; >>> + break; >>> + >>> + case SKL_FW_CFG_MAX_MOD_INST_COUNT: >>> + cfg->max_mod_inst_count = *tlv->value; >>> + break; >>> + >>> + case SKL_FW_CFG_MAX_LL_TASKS_PER_PRI_COUNT: >>> + cfg->max_ll_tasks_per_pri_count = *tlv->value; >>> + break; >>> + >>> + case SKL_FW_CFG_LL_PRI_COUNT: >>> + cfg->ll_pri_count = *tlv->value; >>> + break; >>> + >>> + case SKL_FW_CFG_MAX_DP_TASKS_COUNT: >>> + cfg->max_dp_tasks_count = *tlv->value; >>> + break; >>> + >>> + case SKL_FW_CFG_MAX_LIBS_COUNT: >>> + cfg->max_libs_count = *tlv->value; >>> + break; >>> + >>> + case SKL_FW_CFG_XTAL_FREQ_HZ: >>> + cfg->xtal_freq_hz = *tlv->value; >>> + break; >>> + >>> + case SKL_FW_CFG_UAOL_SUPPORT: >>> + cfg->uaol_support = *tlv->value; >>> + break; >>> + >>> + case SKL_FW_CFG_POWER_GATING_POLICY: >>> + cfg->power_gating_policy = *tlv->value; >>> + break; >>> + >>> + case SKL_FW_CFG_DMA_BUFFER_CONFIG: >>> + case SKL_FW_CFG_SCHEDULER_CONFIG: >>> + case SKL_FW_CFG_CLOCKS_CONFIG: >>> + break; >>> + >>> + default: >>> + dev_info(ipc->dev, "Unrecognized fw param: %d\n", >>> + tlv->type); >>> + break; >> >> Isn't this an error? >> If there are other possible values, why not list them and skip them, >> as done above? >> > > Pretty sure I cannot share names for all capabilities as these are > EMBARGOed. Moreover, both FW_CFG and HW_CFG and constantly being updated > and thus new constants are appended. I find it best to "break" when > encountering known and un-EMBARGOED constant and simply dump info value > if the opposite is true. > > New capabilities are always tied to newer platforms. If there will be a > need for adding them, these won't be even present in SKL/KBL and such > FWs. At the same time if given binary drop contains something new but > not required to be handled by host in generic case, dumping error is > counter-intuitive. It goes back to the compatibility issue, if you have a new firmware reporting a capability that your driver cannot handle then it's an error. if you have a new firmware that reports a new capability that can be ignored then it's fine to just skip. I am just concerned that you have no checks at all. > >>> + } >>> + >>> + offset += sizeof(*tlv) + tlv->length; >>> + } >>> + >>> +exit: >>> + kfree(payload); >>> + return ret; >>> +} >>> +EXPORT_SYMBOL_GPL(skl_ipc_fw_cfg_get); >> >>> +enum skl_alh_support_level { >>> + ALH_NO_SUPPORT = 0x00000, >>> + ALH_CAVS_1_8_CNL = 0x10000, >>> +}; >> >> Support for ALH hasn't changed even past 1.8, and references to CNL >> are probably not needed. > > These are FW types and are here to be left untouched. Ensures parsed > values on host side match FW side. Adding a comment would help then.
On 2019-08-26 18:27, Pierre-Louis Bossart wrote: > > > On 8/24/19 4:17 AM, Cezary Rojewski wrote: >> On 2019-08-23 20:24, Pierre-Louis Bossart wrote: >>> >>> >>> On 8/22/19 2:03 PM, Cezary Rojewski wrote: >>>> Implement interface for retrieving firmware configuration. Skylake >>>> driver will use this data instead of hardcoded values in updates to >>>> come. >>>> >>>> Most params are currently unused. In time driver dependency on fw >>>> config >>>> will increase, and with it, more parsing will be unveiled. >>>> >>>> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com> >>>> --- >>>> sound/soc/intel/skylake/skl-sst-ipc.c | 122 >>>> ++++++++++++++++++++++++++ >>>> sound/soc/intel/skylake/skl-sst-ipc.h | 72 +++++++++++++++ >>>> sound/soc/intel/skylake/skl.h | 1 + >>>> 3 files changed, 195 insertions(+) >>>> >>>> diff --git a/sound/soc/intel/skylake/skl-sst-ipc.c >>>> b/sound/soc/intel/skylake/skl-sst-ipc.c >>>> index 667cdddc289f..e9e11ec4c97b 100644 >>>> --- a/sound/soc/intel/skylake/skl-sst-ipc.c >>>> +++ b/sound/soc/intel/skylake/skl-sst-ipc.c >>>> @@ -11,6 +11,7 @@ >>>> #include "skl.h" >>>> #include "skl-sst-dsp.h" >>>> #include "skl-sst-ipc.h" >>>> +#include "skl-topology.h" >>>> #include "sound/hdaudio_ext.h" >>>> @@ -1067,3 +1068,124 @@ int skl_ipc_set_d0ix(struct sst_generic_ipc >>>> *ipc, struct skl_ipc_d0ix_msg *msg) >>>> return ret; >>>> } >>>> EXPORT_SYMBOL_GPL(skl_ipc_set_d0ix); >>>> + >>>> +int skl_ipc_fw_cfg_get(struct sst_generic_ipc *ipc, struct >>>> skl_fw_cfg *cfg) >>>> +{ >>>> + struct skl_ipc_large_config_msg msg = {0}; >>>> + struct skl_tlv *tlv; >>>> + size_t bytes = 0, offset = 0; >>>> + u8 *payload = NULL; >>>> + int ret; >>>> + >>>> + msg.module_id = 0; >>>> + msg.instance_id = 0; >>>> + msg.large_param_id = SKL_BASEFW_FIRMWARE_CONFIG; >>>> + >>>> + ret = skl_ipc_get_large_config(ipc, &msg, (u32 **)&payload, >>>> &bytes); >>>> + if (ret) >>>> + goto exit; >>>> + >>>> + while (offset < bytes) { >>>> + tlv = (struct skl_tlv *)(payload + offset); >>>> + >>>> + switch (tlv->type) { >>>> + case SKL_FW_CFG_FW_VERSION: >>>> + memcpy(&cfg->fw_version, tlv->value, >>>> + sizeof(cfg->fw_version)); >>>> + break; >>>> + >>>> + case SKL_FW_CFG_MEMORY_RECLAIMED: >>>> + cfg->memory_reclaimed = *tlv->value; >>>> + break; >>>> + >>>> + case SKL_FW_CFG_SLOW_CLOCK_FREQ_HZ: >>>> + cfg->slow_clock_freq_hz = *tlv->value; >>>> + break; >>>> + >>>> + case SKL_FW_CFG_FAST_CLOCK_FREQ_HZ: >>>> + cfg->fast_clock_freq_hz = *tlv->value; >>>> + break; >>>> + >>>> + case SKL_FW_CFG_ALH_SUPPORT_LEVEL: >>>> + cfg->alh_support = *tlv->value; >>>> + break; >>>> + >>>> + case SKL_FW_CFG_IPC_DL_MAILBOX_BYTES: >>>> + cfg->ipc_dl_mailbox_bytes = *tlv->value; >>>> + break; >>>> + >>>> + case SKL_FW_CFG_IPC_UL_MAILBOX_BYTES: >>>> + cfg->ipc_ul_mailbox_bytes = *tlv->value; >>>> + break; >>>> + >>>> + case SKL_FW_CFG_TRACE_LOG_BYTES: >>>> + cfg->trace_log_bytes = *tlv->value; >>>> + break; >>>> + >>>> + case SKL_FW_CFG_MAX_PPL_COUNT: >>>> + cfg->max_ppl_count = *tlv->value; >>>> + break; >>>> + >>>> + case SKL_FW_CFG_MAX_ASTATE_COUNT: >>>> + cfg->max_astate_count = *tlv->value; >>>> + break; >>>> + >>>> + case SKL_FW_CFG_MAX_MODULE_PIN_COUNT: >>>> + cfg->max_module_pin_count = *tlv->value; >>>> + break; >>>> + >>>> + case SKL_FW_CFG_MODULES_COUNT: >>>> + cfg->modules_count = *tlv->value; >>>> + break; >>>> + >>>> + case SKL_FW_CFG_MAX_MOD_INST_COUNT: >>>> + cfg->max_mod_inst_count = *tlv->value; >>>> + break; >>>> + >>>> + case SKL_FW_CFG_MAX_LL_TASKS_PER_PRI_COUNT: >>>> + cfg->max_ll_tasks_per_pri_count = *tlv->value; >>>> + break; >>>> + >>>> + case SKL_FW_CFG_LL_PRI_COUNT: >>>> + cfg->ll_pri_count = *tlv->value; >>>> + break; >>>> + >>>> + case SKL_FW_CFG_MAX_DP_TASKS_COUNT: >>>> + cfg->max_dp_tasks_count = *tlv->value; >>>> + break; >>>> + >>>> + case SKL_FW_CFG_MAX_LIBS_COUNT: >>>> + cfg->max_libs_count = *tlv->value; >>>> + break; >>>> + >>>> + case SKL_FW_CFG_XTAL_FREQ_HZ: >>>> + cfg->xtal_freq_hz = *tlv->value; >>>> + break; >>>> + >>>> + case SKL_FW_CFG_UAOL_SUPPORT: >>>> + cfg->uaol_support = *tlv->value; >>>> + break; >>>> + >>>> + case SKL_FW_CFG_POWER_GATING_POLICY: >>>> + cfg->power_gating_policy = *tlv->value; >>>> + break; >>>> + >>>> + case SKL_FW_CFG_DMA_BUFFER_CONFIG: >>>> + case SKL_FW_CFG_SCHEDULER_CONFIG: >>>> + case SKL_FW_CFG_CLOCKS_CONFIG: >>>> + break; >>>> + >>>> + default: >>>> + dev_info(ipc->dev, "Unrecognized fw param: %d\n", >>>> + tlv->type); >>>> + break; >>> >>> Isn't this an error? >>> If there are other possible values, why not list them and skip them, >>> as done above? >>> >> >> Pretty sure I cannot share names for all capabilities as these are >> EMBARGOed. Moreover, both FW_CFG and HW_CFG and constantly being >> updated and thus new constants are appended. I find it best to "break" >> when encountering known and un-EMBARGOED constant and simply dump info >> value if the opposite is true. >> >> New capabilities are always tied to newer platforms. If there will be >> a need for adding them, these won't be even present in SKL/KBL and >> such FWs. At the same time if given binary drop contains something new >> but not required to be handled by host in generic case, dumping error >> is counter-intuitive. > > It goes back to the compatibility issue, if you have a new firmware > reporting a capability that your driver cannot handle then it's an > error. if you have a new firmware that reports a new capability that can > be ignored then it's fine to just skip. > > I am just concerned that you have no checks at all. > Still wondering why should host side be concerned about capability they are not going to use at all. Let's assume FW binaries are being updated regularly. Doubtful developer will remember to append new switch-case every time new FW-cap pops up - requires checking internal FW headers. In the long run, I do believe the "desynchronization" may happen. I'll recheck with our FW guys what they think about us collapsing at this point (cfg parsing). In general, cAVS specification is very permissive for "outbound" and restrictive against "inbound" data. Parsing goes into "outbound" basket. >> >>>> + } >>>> + >>>> + offset += sizeof(*tlv) + tlv->length; >>>> + } >>>> + >>>> +exit: >>>> + kfree(payload); >>>> + return ret; >>>> +} >>>> +EXPORT_SYMBOL_GPL(skl_ipc_fw_cfg_get); >>> >>>> +enum skl_alh_support_level { >>>> + ALH_NO_SUPPORT = 0x00000, >>>> + ALH_CAVS_1_8_CNL = 0x10000, >>>> +}; >>> >>> Support for ALH hasn't changed even past 1.8, and references to CNL >>> are probably not needed. >> >> These are FW types and are here to be left untouched. Ensures parsed >> values on host side match FW side. > > Adding a comment would help then. Agreed, thanks.
diff --git a/sound/soc/intel/skylake/skl-sst-ipc.c b/sound/soc/intel/skylake/skl-sst-ipc.c index 667cdddc289f..e9e11ec4c97b 100644 --- a/sound/soc/intel/skylake/skl-sst-ipc.c +++ b/sound/soc/intel/skylake/skl-sst-ipc.c @@ -11,6 +11,7 @@ #include "skl.h" #include "skl-sst-dsp.h" #include "skl-sst-ipc.h" +#include "skl-topology.h" #include "sound/hdaudio_ext.h" @@ -1067,3 +1068,124 @@ int skl_ipc_set_d0ix(struct sst_generic_ipc *ipc, struct skl_ipc_d0ix_msg *msg) return ret; } EXPORT_SYMBOL_GPL(skl_ipc_set_d0ix); + +int skl_ipc_fw_cfg_get(struct sst_generic_ipc *ipc, struct skl_fw_cfg *cfg) +{ + struct skl_ipc_large_config_msg msg = {0}; + struct skl_tlv *tlv; + size_t bytes = 0, offset = 0; + u8 *payload = NULL; + int ret; + + msg.module_id = 0; + msg.instance_id = 0; + msg.large_param_id = SKL_BASEFW_FIRMWARE_CONFIG; + + ret = skl_ipc_get_large_config(ipc, &msg, (u32 **)&payload, &bytes); + if (ret) + goto exit; + + while (offset < bytes) { + tlv = (struct skl_tlv *)(payload + offset); + + switch (tlv->type) { + case SKL_FW_CFG_FW_VERSION: + memcpy(&cfg->fw_version, tlv->value, + sizeof(cfg->fw_version)); + break; + + case SKL_FW_CFG_MEMORY_RECLAIMED: + cfg->memory_reclaimed = *tlv->value; + break; + + case SKL_FW_CFG_SLOW_CLOCK_FREQ_HZ: + cfg->slow_clock_freq_hz = *tlv->value; + break; + + case SKL_FW_CFG_FAST_CLOCK_FREQ_HZ: + cfg->fast_clock_freq_hz = *tlv->value; + break; + + case SKL_FW_CFG_ALH_SUPPORT_LEVEL: + cfg->alh_support = *tlv->value; + break; + + case SKL_FW_CFG_IPC_DL_MAILBOX_BYTES: + cfg->ipc_dl_mailbox_bytes = *tlv->value; + break; + + case SKL_FW_CFG_IPC_UL_MAILBOX_BYTES: + cfg->ipc_ul_mailbox_bytes = *tlv->value; + break; + + case SKL_FW_CFG_TRACE_LOG_BYTES: + cfg->trace_log_bytes = *tlv->value; + break; + + case SKL_FW_CFG_MAX_PPL_COUNT: + cfg->max_ppl_count = *tlv->value; + break; + + case SKL_FW_CFG_MAX_ASTATE_COUNT: + cfg->max_astate_count = *tlv->value; + break; + + case SKL_FW_CFG_MAX_MODULE_PIN_COUNT: + cfg->max_module_pin_count = *tlv->value; + break; + + case SKL_FW_CFG_MODULES_COUNT: + cfg->modules_count = *tlv->value; + break; + + case SKL_FW_CFG_MAX_MOD_INST_COUNT: + cfg->max_mod_inst_count = *tlv->value; + break; + + case SKL_FW_CFG_MAX_LL_TASKS_PER_PRI_COUNT: + cfg->max_ll_tasks_per_pri_count = *tlv->value; + break; + + case SKL_FW_CFG_LL_PRI_COUNT: + cfg->ll_pri_count = *tlv->value; + break; + + case SKL_FW_CFG_MAX_DP_TASKS_COUNT: + cfg->max_dp_tasks_count = *tlv->value; + break; + + case SKL_FW_CFG_MAX_LIBS_COUNT: + cfg->max_libs_count = *tlv->value; + break; + + case SKL_FW_CFG_XTAL_FREQ_HZ: + cfg->xtal_freq_hz = *tlv->value; + break; + + case SKL_FW_CFG_UAOL_SUPPORT: + cfg->uaol_support = *tlv->value; + break; + + case SKL_FW_CFG_POWER_GATING_POLICY: + cfg->power_gating_policy = *tlv->value; + break; + + case SKL_FW_CFG_DMA_BUFFER_CONFIG: + case SKL_FW_CFG_SCHEDULER_CONFIG: + case SKL_FW_CFG_CLOCKS_CONFIG: + break; + + default: + dev_info(ipc->dev, "Unrecognized fw param: %d\n", + tlv->type); + break; + } + + offset += sizeof(*tlv) + tlv->length; + } + +exit: + kfree(payload); + return ret; +} +EXPORT_SYMBOL_GPL(skl_ipc_fw_cfg_get); diff --git a/sound/soc/intel/skylake/skl-sst-ipc.h b/sound/soc/intel/skylake/skl-sst-ipc.h index 0058d82bd5a4..ebc5852e15d0 100644 --- a/sound/soc/intel/skylake/skl-sst-ipc.h +++ b/sound/soc/intel/skylake/skl-sst-ipc.h @@ -15,6 +15,12 @@ struct sst_dsp; struct sst_generic_ipc; +struct skl_tlv { + u32 type; + u32 length; + u8 value[0]; +}; + enum skl_ipc_pipeline_state { PPL_INVALID_STATE = 0, PPL_UNINITIALIZED = 1, @@ -70,6 +76,69 @@ struct skl_lib_info { enum skl_basefw_runtime_param { SKL_BASEFW_ASTATE_TABLE = 4, SKL_BASEFW_DMA_CONTROL = 5, + SKL_BASEFW_FIRMWARE_CONFIG = 7, +}; + +enum skl_fw_cfg_params { + SKL_FW_CFG_FW_VERSION = 0, + SKL_FW_CFG_MEMORY_RECLAIMED, + SKL_FW_CFG_SLOW_CLOCK_FREQ_HZ, + SKL_FW_CFG_FAST_CLOCK_FREQ_HZ, + SKL_FW_CFG_DMA_BUFFER_CONFIG, + SKL_FW_CFG_ALH_SUPPORT_LEVEL, + SKL_FW_CFG_IPC_DL_MAILBOX_BYTES, + SKL_FW_CFG_IPC_UL_MAILBOX_BYTES, + SKL_FW_CFG_TRACE_LOG_BYTES, + SKL_FW_CFG_MAX_PPL_COUNT, + SKL_FW_CFG_MAX_ASTATE_COUNT, + SKL_FW_CFG_MAX_MODULE_PIN_COUNT, + SKL_FW_CFG_MODULES_COUNT, + SKL_FW_CFG_MAX_MOD_INST_COUNT, + SKL_FW_CFG_MAX_LL_TASKS_PER_PRI_COUNT, + SKL_FW_CFG_LL_PRI_COUNT, + SKL_FW_CFG_MAX_DP_TASKS_COUNT, + SKL_FW_CFG_MAX_LIBS_COUNT, + SKL_FW_CFG_SCHEDULER_CONFIG, + SKL_FW_CFG_XTAL_FREQ_HZ, + SKL_FW_CFG_CLOCKS_CONFIG, + SKL_FW_CFG_UAOL_SUPPORT, + SKL_FW_CFG_POWER_GATING_POLICY, + SKL_FW_CFG_ASSERT_MODE, +}; + +struct skl_fw_version { + u16 major; + u16 minor; + u16 hotfix; + u16 build; +}; + +enum skl_alh_support_level { + ALH_NO_SUPPORT = 0x00000, + ALH_CAVS_1_8_CNL = 0x10000, +}; + +struct skl_fw_cfg { + struct skl_fw_version fw_version; + u32 memory_reclaimed; + u32 slow_clock_freq_hz; + u32 fast_clock_freq_hz; + enum skl_alh_support_level alh_support; + u32 ipc_dl_mailbox_bytes; + u32 ipc_ul_mailbox_bytes; + u32 trace_log_bytes; + u32 max_ppl_count; + u32 max_astate_count; + u32 max_module_pin_count; + u32 modules_count; + u32 max_mod_inst_count; + u32 max_ll_tasks_per_pri_count; + u32 ll_pri_count; + u32 max_dp_tasks_count; + u32 max_libs_count; + u32 xtal_freq_hz; + u32 uaol_support; + u32 power_gating_policy; }; struct skl_ipc_init_instance_msg { @@ -171,4 +240,7 @@ int skl_ipc_process_notification(struct sst_generic_ipc *ipc, struct skl_ipc_header header); void skl_ipc_tx_data_copy(struct ipc_message *msg, char *tx_data, size_t tx_size); + +int skl_ipc_fw_cfg_get(struct sst_generic_ipc *ipc, struct skl_fw_cfg *cfg); + #endif /* __SKL_IPC_H */ diff --git a/sound/soc/intel/skylake/skl.h b/sound/soc/intel/skylake/skl.h index f8c714153610..0d1c820e11cd 100644 --- a/sound/soc/intel/skylake/skl.h +++ b/sound/soc/intel/skylake/skl.h @@ -110,6 +110,7 @@ struct skl_dev { /* Populate module information */ struct list_head uuid_list; + struct skl_fw_cfg fw_cfg; /* Is firmware loaded */ bool fw_loaded;
Implement interface for retrieving firmware configuration. Skylake driver will use this data instead of hardcoded values in updates to come. Most params are currently unused. In time driver dependency on fw config will increase, and with it, more parsing will be unveiled. Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com> --- sound/soc/intel/skylake/skl-sst-ipc.c | 122 ++++++++++++++++++++++++++ sound/soc/intel/skylake/skl-sst-ipc.h | 72 +++++++++++++++ sound/soc/intel/skylake/skl.h | 1 + 3 files changed, 195 insertions(+)