[alsa-devel] [PATCH 03/35] ASoC: Intel: Skylake: Add HARDWARE_CONFIG IPC request

[Cezary Rojewski]

On 2019-08-23 20:32, Pierre-Louis Bossart wrote:
> 
>> +    while (offset < bytes) {
>> +        tlv = (struct skl_tlv *)(payload + offset);
>> +
>> +        switch (tlv->type) {
>> +        case SKL_HW_CFG_CAVS_VER:
>> +            cfg->cavs_version = *tlv->value;
>> +            break;
>> +
>> +        case SKL_HW_CFG_DSP_CORES:
>> +            cfg->dsp_cores = *tlv->value;
>> +            break;
>> +
>> +        case SKL_HW_CFG_MEM_PAGE_BYTES:
>> +            cfg->mem_page_bytes = *tlv->value;
>> +            break;
>> +
>> +        case SKL_HW_CFG_TOTAL_PHYS_MEM_PAGES:
>> +            cfg->total_phys_mem_pages = *tlv->value;
>> +            break;
>> +
>> +        case SKL_HW_CFG_I2S_CAPS:
>> +            cfg->i2s_caps.version = tlv->value[0];
>> +            size = tlv->value[1];
>> +            cfg->i2s_caps.ctrl_count = size;
>> +            if (!size)
>> +                break;
>> +
>> +            size *= sizeof(*cfg->i2s_caps.ctrl_base_addr);
>> +            cfg->i2s_caps.ctrl_base_addr =
>> +                kmemdup(&tlv->value[2], size, GFP_KERNEL);
> 
> shouldn't the size be that of the source buffer instead of the destination?
> 

I2S_CAPS are represented by:

struct skl_i2s_caps {
	enum skl_i2s_version version;
	u32 ctrl_count;
	u32 *ctrl_base_addr;
};

As you can see, second DWORD coming from V (TL_V_) specifies number of 
elements in array ctrl_base_addr. So, what we do is set i2s_caps.version 
to DWORD[0], i2s_caps.ctrl_count to DWORD[1] and then multiply count by 
the size of element type and thus we know how much memory to copy.

>> +            if (!cfg->i2s_caps.ctrl_base_addr) {
>> +                ret = -ENOMEM;
>> +                goto exit;
>> +            }
>> +            break;
>> +
>> +        case SKL_HW_CFG_GATEWAY_COUNT:
>> +            cfg->gateway_count = *tlv->value;
>> +            break;
>> +
>> +        case SKL_HW_CFG_HP_EBB_COUNT:
>> +            cfg->hp_ebb_count = *tlv->value;
>> +            break;
>> +
>> +        case SKL_HW_CFG_LP_EBB_COUNT:
>> +            cfg->lp_ebb_count = *tlv->value;
>> +            break;
>> +
>> +        case SKL_HW_CFG_EBB_SIZE_BYTES:
>> +            cfg->ebb_size_bytes = *tlv->value;
>> +            break;
>> +
>> +        case SKL_HW_CFG_GPDMA_CAPS:
>> +        case SKL_HW_CFG_UAOL_CAPS:
>> +            break;
>> +
>> +        default:
>> +            dev_info(ipc->dev, "Unrecognized hw param: %d\n",
>> +                tlv->type);
>> +            break;
> 
> same feedback, it's usually better to list all values and skip them, and 
> fail big if you see something unexpected.

Same answer as for FIRMWARE_CONFIG.

> 
>> +        }
>> +
>> +        offset += sizeof(*tlv) + tlv->length;
>> +    }
>> +
>> +exit:
>> +    kfree(payload);
>> +    return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(skl_ipc_hw_cfg_get);
>> diff --git a/sound/soc/intel/skylake/skl-sst-ipc.h 
>> b/sound/soc/intel/skylake/skl-sst-ipc.h
>> index ebc5852e15d0..eefa52f7f97a 100644
>> --- a/sound/soc/intel/skylake/skl-sst-ipc.h
>> +++ b/sound/soc/intel/skylake/skl-sst-ipc.h
>> @@ -77,6 +77,7 @@ enum skl_basefw_runtime_param {
>>       SKL_BASEFW_ASTATE_TABLE = 4,
>>       SKL_BASEFW_DMA_CONTROL = 5,
>>       SKL_BASEFW_FIRMWARE_CONFIG = 7,
>> +    SKL_BASEFW_HARDWARE_CONFIG = 8,
>>   };
>>   enum skl_fw_cfg_params {
>> @@ -141,6 +142,50 @@ struct skl_fw_cfg {
>>       u32 power_gating_policy;
>>   };
>> +enum skl_hw_cfg_params {
>> +    SKL_HW_CFG_CAVS_VER,
>> +    SKL_HW_CFG_DSP_CORES,
>> +    SKL_HW_CFG_MEM_PAGE_BYTES,
>> +    SKL_HW_CFG_TOTAL_PHYS_MEM_PAGES,
>> +    SKL_HW_CFG_I2S_CAPS,
>> +    SKL_HW_CFG_GPDMA_CAPS,
>> +    SKL_HW_CFG_GATEWAY_COUNT,
>> +    SKL_HW_CFG_HP_EBB_COUNT,
>> +    SKL_HW_CFG_LP_EBB_COUNT,
>> +    SKL_HW_CFG_EBB_SIZE_BYTES,
>> +    SKL_HW_CFG_UAOL_CAPS
>> +};
>> +
>> +enum skl_cavs_version {
>> +    SKL_CAVS_VER_1_5 = 0x10005,
>> +    SKL_CAVS_VER_1_8 = 0x10008,
>> +};
>> +
>> +enum skl_i2s_version {
>> +    SKL_I2S_VER_15_SKYLAKE   = 0x00000,
>> +    SKL_I2S_VER_15_BROXTON   = 0x10000,
>> +    SKL_I2S_VER_15_BROXTON_P = 0x20000,
>> +    SKL_I2S_VER_18_KBL_CNL   = 0x30000,
>> +};
> 
> The encoding is odd.
> Do these values mean anything (e.g. tied to firmware definitions?)

Exactly. Right now I'm mirroring FW side. Don't blame me for encoding, I 
know it's weird : D