[alsa-devel] [PATCH 02/35] ASoC: Intel: Skylake: Add FIRMWARE_CONFIG IPC request
Cezary Rojewski
cezary.rojewski at intel.com
Mon Aug 26 21:34:50 CEST 2019
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 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.
>
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.
More information about the Alsa-devel
mailing list