[alsa-devel] [PATCH 02/35] ASoC: Intel: Skylake: Add FIRMWARE_CONFIG IPC request
Pierre-Louis Bossart
pierre-louis.bossart at linux.intel.com
Mon Aug 26 18:27:23 CEST 2019
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 at 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.
More information about the Alsa-devel
mailing list