diff mbox series

[v3,01/19] media: venus: pm_helpers: Only set rate of the core clock in core_clks_enable

Message ID 20230911-topic-mars-v3-1-79f23b81c261@linaro.org (mailing list archive)
State New
Headers show
Series Venus cleanups | expand

Commit Message

Konrad Dybcio March 27, 2024, 6:08 p.m. UTC
Commit c22b1a29497c ("media: venus: core,pm: Vote for min clk freq
during venus boot") intended to up the rate of the Venus core clock
from the XO minimum to something more reasonable, based on the per-
SoC frequency table.

Unfortunately, it ended up calling set_rate with that same argument
on all clocks in res->clks. Fix that using the OPP API.

Fixes: c22b1a29497c ("media: venus: core,pm: Vote for min clk freq during venus boot")
Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/media/platform/qcom/venus/pm_helpers.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

Comments

Dikshita Agarwal April 5, 2024, 7:31 a.m. UTC | #1
On 3/27/2024 11:38 PM, Konrad Dybcio wrote:
> Commit c22b1a29497c ("media: venus: core,pm: Vote for min clk freq
> during venus boot") intended to up the rate of the Venus core clock
> from the XO minimum to something more reasonable, based on the per-
> SoC frequency table.
> 
> Unfortunately, it ended up calling set_rate with that same argument
> on all clocks in res->clks. Fix that using the OPP API.
> 
> Fixes: c22b1a29497c ("media: venus: core,pm: Vote for min clk freq during venus boot")
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>  drivers/media/platform/qcom/venus/pm_helpers.c | 23 +++++++++++------------
>  1 file changed, 11 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
> index 502822059498..8bd0ce4ce69d 100644
> --- a/drivers/media/platform/qcom/venus/pm_helpers.c
> +++ b/drivers/media/platform/qcom/venus/pm_helpers.c
> @@ -41,24 +41,23 @@ 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 = core->res->freq_tbl;
> -	unsigned int freq_tbl_size = core->res->freq_tbl_size;
> -	unsigned long freq;
> +	struct dev_pm_opp *opp;
> +	unsigned long freq = 0;
>  	unsigned int i;
>  	int ret;
>  
> -	if (!freq_tbl)
> -		return -EINVAL;
> +	if (core->has_opp_table) {
> +		opp = dev_pm_opp_find_freq_ceil(core->dev, &freq);
> +		if (IS_ERR(opp))
> +			return PTR_ERR(opp);
> +		dev_pm_opp_put(opp);
>  
> -	freq = freq_tbl[freq_tbl_size - 1].freq;
> +		ret = dev_pm_opp_set_rate(core->dev, freq);
> +		if (ret)
> +			return ret;
> +	}
Earlier clk_set_rate is called for only V6 target, this change is calling
it unconditionally. Opp table is available for v4 target as well.
>  
>  	for (i = 0; i < res->clks_num; i++) {
> -		if (IS_V6(core)) {
> -			ret = clk_set_rate(core->clks[i], freq);
> -			if (ret)
> -				goto err;
> -		}
> -
>  		ret = clk_prepare_enable(core->clks[i]);
>  		if (ret)
>  			goto err;
> 

Thanks,
Dikshita
Vikash Garodia April 5, 2024, 8:49 a.m. UTC | #2
Hi Konrad,

On 4/5/2024 1:01 PM, Dikshita Agarwal wrote:
> 
> 
> On 3/27/2024 11:38 PM, Konrad Dybcio wrote:
>> Commit c22b1a29497c ("media: venus: core,pm: Vote for min clk freq
>> during venus boot") intended to up the rate of the Venus core clock
>> from the XO minimum to something more reasonable, based on the per-
>> SoC frequency table.
>>
>> Unfortunately, it ended up calling set_rate with that same argument
>> on all clocks in res->clks. Fix that using the OPP API.
It called with same argument, but not with same frequency. The argument is
platform specific and would have different values. Do not see it fixing anything
in existing code, so "Fixes" does not look applicable here. OR, am i  missing
something ?

Thanks,
Vikash
>>
>> Fixes: c22b1a29497c ("media: venus: core,pm: Vote for min clk freq during venus boot")
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> ---
>>  drivers/media/platform/qcom/venus/pm_helpers.c | 23 +++++++++++------------
>>  1 file changed, 11 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
>> index 502822059498..8bd0ce4ce69d 100644
>> --- a/drivers/media/platform/qcom/venus/pm_helpers.c
>> +++ b/drivers/media/platform/qcom/venus/pm_helpers.c
>> @@ -41,24 +41,23 @@ 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 = core->res->freq_tbl;
>> -	unsigned int freq_tbl_size = core->res->freq_tbl_size;
>> -	unsigned long freq;
>> +	struct dev_pm_opp *opp;
>> +	unsigned long freq = 0;
>>  	unsigned int i;
>>  	int ret;
>>  
>> -	if (!freq_tbl)
>> -		return -EINVAL;
>> +	if (core->has_opp_table) {
>> +		opp = dev_pm_opp_find_freq_ceil(core->dev, &freq);
>> +		if (IS_ERR(opp))
>> +			return PTR_ERR(opp);
>> +		dev_pm_opp_put(opp);
>>  
>> -	freq = freq_tbl[freq_tbl_size - 1].freq;
>> +		ret = dev_pm_opp_set_rate(core->dev, freq);
>> +		if (ret)
>> +			return ret;
>> +	}
> Earlier clk_set_rate is called for only V6 target, this change is calling
> it unconditionally. Opp table is available for v4 target as well.
>>  
>>  	for (i = 0; i < res->clks_num; i++) {
>> -		if (IS_V6(core)) {
>> -			ret = clk_set_rate(core->clks[i], freq);
>> -			if (ret)
>> -				goto err;
>> -		}
>> -
>>  		ret = clk_prepare_enable(core->clks[i]);
>>  		if (ret)
>>  			goto err;
>>
> 
> Thanks,
> Dikshita
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 502822059498..8bd0ce4ce69d 100644
--- a/drivers/media/platform/qcom/venus/pm_helpers.c
+++ b/drivers/media/platform/qcom/venus/pm_helpers.c
@@ -41,24 +41,23 @@  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 = core->res->freq_tbl;
-	unsigned int freq_tbl_size = core->res->freq_tbl_size;
-	unsigned long freq;
+	struct dev_pm_opp *opp;
+	unsigned long freq = 0;
 	unsigned int i;
 	int ret;
 
-	if (!freq_tbl)
-		return -EINVAL;
+	if (core->has_opp_table) {
+		opp = dev_pm_opp_find_freq_ceil(core->dev, &freq);
+		if (IS_ERR(opp))
+			return PTR_ERR(opp);
+		dev_pm_opp_put(opp);
 
-	freq = freq_tbl[freq_tbl_size - 1].freq;
+		ret = dev_pm_opp_set_rate(core->dev, freq);
+		if (ret)
+			return ret;
+	}
 
 	for (i = 0; i < res->clks_num; i++) {
-		if (IS_V6(core)) {
-			ret = clk_set_rate(core->clks[i], freq);
-			if (ret)
-				goto err;
-		}
-
 		ret = clk_prepare_enable(core->clks[i]);
 		if (ret)
 			goto err;