Re: [PATCH 04/35] ASoC: Intel: Skylake: Unify firmware loading mechanism - Pierre-Louis Bossart

From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
To: Cezary Rojewski <cezary.rojewski@intel.com>, alsa-devel@alsa-project.org
Cc: broonie@kernel.org, tiwai@suse.com, lgirdwood@gmail.com
Subject: Re: [PATCH 04/35] ASoC: Intel: Skylake: Unify firmware loading mechanism
Date: Mon, 26 Aug 2019 11:31:25 -0500	[thread overview]
Message-ID: <d51a0703-4a1d-e667-e29b-a4017de0a38e@linux.intel.com> (raw)
In-Reply-To: <20fea187-8a0c-c293-bbcb-e8561193bc2f@intel.com>

On 8/24/19 4:34 AM, Cezary Rojewski wrote:
> On 2019-08-23 20:40, Pierre-Louis Bossart wrote:
>>
>>> -int skl_sst_init_fw(struct device *dev, struct skl_dev *skl)
>>> +int skl_sst_init_fw(struct skl_dev *skl)
>>>   {
>>> -    int ret;
>>>       struct sst_dsp *sst = skl->dsp;
>>> +    struct device *dev = skl->dev;
>>> +    int (*lp_check)(struct sst_dsp *dsp, bool state);
>>> +    int ret;
>>> +
>>> +    lp_check = skl->ipc.ops.check_dsp_lp_on;
>>> +    skl->enable_miscbdcge(dev, false);
>>> +    skl->clock_power_gating(dev, false);
>>>       ret = sst->fw_ops.load_fw(sst);
>>>       if (ret < 0) {
>>>           dev_err(dev, "Load base fw failed : %d\n", ret);
>>> -        return ret;
>>> +        goto exit;
>>> +    }
>>> +
>>> +    if (!skl->is_first_boot)
>>> +        goto library_load;
>>> +    /* Disable power check during cfg setup */
>>> +    skl->ipc.ops.check_dsp_lp_on = NULL;
>>
>> It's very odd to play with .ops callback dynamically. Usually ops are 
>> constant, and if you want to disable them you add a flag.
>>
> 
> Yeye, keen eye! Can't do everything at once though :/
> The power check is APL+ specific and should not be part of generic ipc 
> framework at all (found in /sound/soc/intel/common/sst-ipc.c). Different 
> fate awaits said check. For now, in this single case it seems best to 
> simply disable the check and reapply it once setup is done.

What's the difference with having this callback do nothing for APL-?

> 
>>> +
>>> +    ret = skl_ipc_fw_cfg_get(&skl->ipc, &skl->fw_cfg);
>>> +    if (ret < 0) {
>>> +        dev_err(dev, "Failed to get fw cfg: %d\n", ret);
>>> +        goto exit;
>>> +    }
>>> +
>>> +    ret = skl_ipc_hw_cfg_get(&skl->ipc, &skl->hw_cfg);
>>> +    if (ret < 0) {
>>> +        dev_err(dev, "Failed to get hw cfg: %d\n", ret);
>>> +        goto exit;
>>>       }
>>>       skl_dsp_init_core_state(sst);
>>> +library_load:
>>>       if (skl->lib_count > 1) {
>>>           ret = sst->fw_ops.load_library(sst, skl->lib_info,
>>>                           skl->lib_count);
>>>           if (ret < 0) {
>>> -            dev_err(dev, "Load Library failed : %x\n", ret);
>>> -            return ret;
>>> +            dev_err(dev, "Load library failed : %x\n", ret);
>>> +            goto exit;
>>>           }
>>>       }
>>> +
>>>       skl->is_first_boot = false;
>>> +exit:
>>> +    skl->ipc.ops.check_dsp_lp_on = lp_check;
>>> +    skl->enable_miscbdcge(dev, true);
>>> +    skl->clock_power_gating(dev, true);
>>> -    return 0;
>>> +    return ret;
>>>   }
>>>   EXPORT_SYMBOL_GPL(skl_sst_init_fw);
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel