Re: [PATCH 04/25] media: venus: core,pm: Vote for min clk freq during venus boot

[Stanimir Varbanov <stanimir.varbanov@linaro.org>]

On 2/23/21 3:25 PM, Stanimir Varbanov wrote:
> 
> 
> On 2/22/21 6:02 PM, Bryan O'Donoghue wrote:
>> From: Dikshita Agarwal <dikshita@codeaurora.org>
>>
>> Vote for min clk frequency for core clks during prepare and enable clocks
>> at boot sequence. Without this the controller clock runs at very low value
>> (9.6MHz) which is not sufficient to boot venus.
>>
>> Signed-off-by: Dikshita Agarwal <dikshita@codeaurora.org>
>> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>> ---
>>  drivers/media/platform/qcom/venus/pm_helpers.c | 14 ++++++++++++++
>>  1 file changed, 14 insertions(+)
>>
>> diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
>> index 4f5d42662963..767cb00d4b46 100644
>> --- a/drivers/media/platform/qcom/venus/pm_helpers.c
>> +++ b/drivers/media/platform/qcom/venus/pm_helpers.c
>> @@ -41,10 +41,24 @@ static int core_clks_get(struct venus_core *core)
>>  static int core_clks_enable(struct venus_core *core)
>>  {
>>  	const struct venus_resources *res = core->res;
>> +	const struct freq_tbl *freq_tbl = NULL;
>> +	unsigned int freq_tbl_size = 0;
>> +	unsigned long freq = 0;
> 
> no need to initialize to zero.
> 
>>  	unsigned int i;
>>  	int ret;
>>  
>> +	freq_tbl = core->res->freq_tbl;
>> +	freq_tbl_size = core->res->freq_tbl_size;
> 
> could you initialize those at the variables declaration?
> 
>> +	if (!freq_tbl)
>> +		return -EINVAL;
>> +
>> +	freq = freq_tbl[freq_tbl_size - 1].freq;
>> +
>>  	for (i = 0; i < res->clks_num; i++) {
>> +		ret = clk_set_rate(core->clks[i], freq);
> 
> I'm not sure that we have to set the rate for all core->clks[i] ?

Confirmed. This produces regressions on db410c (I haven't tested on
other platforms yet).

> 
>> +		if (ret)
>> +			goto err;
>> +
>>  		ret = clk_prepare_enable(core->clks[i]);
>>  		if (ret)
>>  			goto err;
>>
> 

-- 
regards,
Stan