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

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

On 3/10/21 7:33 PM, Bryan O'Donoghue wrote:
> On 06/03/2021 15:01, Stanimir Varbanov wrote:
>>
>>
>> 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;
>>>>
>>>
>>
> 
> OK, I have this now on db410c
> 
> I made a tree out of
> 
> svarbanov-linux-tv/venus-for-next-v5.13
> +
> svarbanov-linux-tv/venus-msm8916-fixes - minor fix here integrating on
> top of 5.13
> 
> and then put the sm8250 changes on top of that
> 
> https://git.linaro.org/people/bryan.odonoghue/kernel.git/log/?h=tracking-qcomlt-sm8250-venus-integrated-sm8250
> 
> 
> So confirm db410c works up to tag
> tracking-qcomlt-sm8250-venus-integrated-sm8250-02+svarbanov-linux-tv/venus-msm8916-fixes
> 
> 
> I'll work on fixing your feedback on that putative branch.

Thanks!

I fixed the regression on db410c by set the rate for the core->clks[0]
only, e.g. the clock at which the remote processor is running.

> 
> ---
> bod

-- 
regards,
Stan