Re: [PATCH 02/35] ASoC: Intel: Skylake: Add FIRMWARE_CONFIG IPC request

From: Cezary Rojewski <cezary.rojewski@intel.com>
To: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>,
	alsa-devel@alsa-project.org
Cc: broonie@kernel.org, tiwai@suse.com, lgirdwood@gmail.com
Subject: Re: [PATCH 02/35] ASoC: Intel: Skylake: Add FIRMWARE_CONFIG IPC request
Date: Sat, 24 Aug 2019 11:17:47 +0200	[thread overview]
Message-ID: <f05b3a3e-db2b-8d07-3531-14e9afd81d1a@intel.com> (raw)
In-Reply-To: <3173be35-0721-a36d-d328-b8e2ad3b5008@linux.intel.com>

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.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel