diff mbox series

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

Message ID 20210222160300.1811121-5-bryan.odonoghue@linaro.org (mailing list archive)
State Superseded
Headers show
Series media: venus: Enable 6xx support | expand

Commit Message

Bryan O'Donoghue Feb. 22, 2021, 4:02 p.m. UTC
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(+)

Comments

Stanimir Varbanov Feb. 23, 2021, 1:25 p.m. UTC | #1
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] ?

> +		if (ret)
> +			goto err;
> +
>  		ret = clk_prepare_enable(core->clks[i]);
>  		if (ret)
>  			goto err;
>
Stanimir Varbanov March 6, 2021, 3:01 p.m. UTC | #2
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;
>>
>
Bryan O'Donoghue March 6, 2021, 4:48 p.m. UTC | #3
On 06/03/2021 15:01, Stanimir Varbanov wrote:
> Confirmed. This produces regressions on db410c (I haven't tested on
> other platforms yet).

db410c was broken for me on the reference branch prior to sm8250 
additions however AFAICT this change was fine on sdm845.

---
bod
Stanimir Varbanov March 7, 2021, 10:44 a.m. UTC | #4
On 3/6/21 6:48 PM, Bryan O'Donoghue wrote:
> On 06/03/2021 15:01, Stanimir Varbanov wrote:
>> Confirmed. This produces regressions on db410c (I haven't tested on
>> other platforms yet).
> 
> db410c was broken for me on the reference branch prior to sm8250
> additions however AFAICT this change was fine on sdm845.

I found this after I fixed db410c issues.
Bryan O'Donoghue March 10, 2021, 5:33 p.m. UTC | #5
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.

---
bod
Stanimir Varbanov March 10, 2021, 10:02 p.m. UTC | #6
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
diff mbox series

Patch

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;
 	unsigned int i;
 	int ret;
 
+	freq_tbl = core->res->freq_tbl;
+	freq_tbl_size = core->res->freq_tbl_size;
+	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);
+		if (ret)
+			goto err;
+
 		ret = clk_prepare_enable(core->clks[i]);
 		if (ret)
 			goto err;