[PATCH] clk: qcom: clk-alpha-pll: Replace divide operator with comparison

[PATCH] clk: qcom: clk-alpha-pll: Replace divide operator with comparison

[Satya Priya Kakitapalli]

In zonda_pll_adjust_l_val() replace the divide operator with comparison
operator since comparisons are faster than divisions.

Fixes: f4973130d255 ("clk: qcom: clk-alpha-pll: Update set_rate for Zonda PLL")
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Closes: https://lore.kernel.org/r/202408110724.8pqbpDiD-lkp@intel.com/
Signed-off-by: Satya Priya Kakitapalli <quic_skakitap@quicinc.com>
---
 drivers/clk/qcom/clk-alpha-pll.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/qcom/clk-alpha-pll.c b/drivers/clk/qcom/clk-alpha-pll.c
index 2f620ccb41cb..fd8a82bb3690 100644
--- a/drivers/clk/qcom/clk-alpha-pll.c
+++ b/drivers/clk/qcom/clk-alpha-pll.c
@@ -2126,7 +2126,7 @@ static void zonda_pll_adjust_l_val(unsigned long rate, unsigned long prate, u32
 	remainder = do_div(quotient, prate);
 	*l = quotient;
 
-	if ((remainder * 2) / prate)
+	if ((remainder * 2) >= prate)
 		*l = *l + 1;
 }
 
-- 
2.25.1

Re: [PATCH] clk: qcom: clk-alpha-pll: Replace divide operator with comparison

[Vladimir Zapolskiy]

On 8/13/24 12:40, Satya Priya Kakitapalli wrote:
> In zonda_pll_adjust_l_val() replace the divide operator with comparison
> operator since comparisons are faster than divisions.
> 
> Fixes: f4973130d255 ("clk: qcom: clk-alpha-pll: Update set_rate for Zonda PLL")

Apparently the change is not a fix, therefore I believe the Fixes tag
shall be removed.

> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Closes: https://lore.kernel.org/r/202408110724.8pqbpDiD-lkp@intel.com/
> Signed-off-by: Satya Priya Kakitapalli <quic_skakitap@quicinc.com>
> ---
>   drivers/clk/qcom/clk-alpha-pll.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/qcom/clk-alpha-pll.c b/drivers/clk/qcom/clk-alpha-pll.c
> index 2f620ccb41cb..fd8a82bb3690 100644
> --- a/drivers/clk/qcom/clk-alpha-pll.c
> +++ b/drivers/clk/qcom/clk-alpha-pll.c
> @@ -2126,7 +2126,7 @@ static void zonda_pll_adjust_l_val(unsigned long rate, unsigned long prate, u32
>   	remainder = do_div(quotient, prate);
>   	*l = quotient;

Since it's not a fix, but a simplification, you may wish to remove
an unnecessary 'quotient' local variable:

remainder = do_div(rate, prate);

>   
> -	if ((remainder * 2) / prate)
> +	if ((remainder * 2) >= prate)
>   		*l = *l + 1;

*l = rate + (u32)(remainder * 2 >= prate);

I hope the assignment above is quite clear...

>   }
>   

With the review comments above implemented, feel free to add to v2

Reviewed-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>

--
Best wishes,
Vladimir

Re: [PATCH] clk: qcom: clk-alpha-pll: Replace divide operator with comparison

[Jon Hunter]

Hi Satya, Vladimir,

On 13/08/2024 21:01, Vladimir Zapolskiy wrote:
> On 8/13/24 12:40, Satya Priya Kakitapalli wrote:
>> In zonda_pll_adjust_l_val() replace the divide operator with comparison
>> operator since comparisons are faster than divisions.
>>
>> Fixes: f4973130d255 ("clk: qcom: clk-alpha-pll: Update set_rate for 
>> Zonda PLL")
> 
> Apparently the change is not a fix, therefore I believe the Fixes tag
> shall be removed.


 From the commit message it is not clear that this is a fix, but I
believe that it is. With the current -next I am seeing the following
build error (with GCC 7.3.1) on ARM ...

drivers/clk/qcom/clk-alpha-pll.o: In function `clk_zonda_pll_set_rate':
clk-alpha-pll.c:(.text+0x45dc): undefined reference to `__aeabi_uldivmod'
  
>> Reported-by: kernel test robot <lkp@intel.com>
>> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
>> Closes: https://lore.kernel.org/r/202408110724.8pqbpDiD-lkp@intel.com/

There is also the above smatch warning that was reported.

>> Signed-off-by: Satya Priya Kakitapalli <quic_skakitap@quicinc.com>
>> ---
>>   drivers/clk/qcom/clk-alpha-pll.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/clk/qcom/clk-alpha-pll.c 
>> b/drivers/clk/qcom/clk-alpha-pll.c
>> index 2f620ccb41cb..fd8a82bb3690 100644
>> --- a/drivers/clk/qcom/clk-alpha-pll.c
>> +++ b/drivers/clk/qcom/clk-alpha-pll.c
>> @@ -2126,7 +2126,7 @@ static void zonda_pll_adjust_l_val(unsigned long 
>> rate, unsigned long prate, u32
>>       remainder = do_div(quotient, prate);
>>       *l = quotient;
> 
> Since it's not a fix, but a simplification, you may wish to remove
> an unnecessary 'quotient' local variable:
> 
> remainder = do_div(rate, prate);
> 
>> -    if ((remainder * 2) / prate)
>> +    if ((remainder * 2) >= prate)
>>           *l = *l + 1;
> 
> *l = rate + (u32)(remainder * 2 >= prate);


The above change does fix this build error for me.

Satya, did you intend this to be a fix? Can we get this into -next?

Thanks
Jon

-- 
nvpublic

Re: [PATCH] clk: qcom: clk-alpha-pll: Replace divide operator with comparison

[Dmitry Baryshkov]

On Wed, Aug 28, 2024 at 02:47:05PM GMT, Jon Hunter wrote:
> Hi Satya, Vladimir,
> 
> On 13/08/2024 21:01, Vladimir Zapolskiy wrote:
> > On 8/13/24 12:40, Satya Priya Kakitapalli wrote:
> > > In zonda_pll_adjust_l_val() replace the divide operator with comparison
> > > operator since comparisons are faster than divisions.
> > > 
> > > Fixes: f4973130d255 ("clk: qcom: clk-alpha-pll: Update set_rate for
> > > Zonda PLL")
> > 
> > Apparently the change is not a fix, therefore I believe the Fixes tag
> > shall be removed.
> 
> 
> From the commit message it is not clear that this is a fix, but I
> believe that it is. With the current -next I am seeing the following
> build error (with GCC 7.3.1) on ARM ...
> 
> drivers/clk/qcom/clk-alpha-pll.o: In function `clk_zonda_pll_set_rate':
> clk-alpha-pll.c:(.text+0x45dc): undefined reference to `__aeabi_uldivmod'

This should be a part of the commit message

> > > Reported-by: kernel test robot <lkp@intel.com>
> > > Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> > > Closes: https://lore.kernel.org/r/202408110724.8pqbpDiD-lkp@intel.com/

this Closes tag must come after lkp's Reported-by. Please also add
Closes with the link to Dan's report.

> 
> There is also the above smatch warning that was reported.

And the Smatch warning too should be a part of the commit message.

Last, but not least, as it is a fix, there should be a Fixes: tag and
optionally a cc:stable.

> 
> > > Signed-off-by: Satya Priya Kakitapalli <quic_skakitap@quicinc.com>
> > > ---
> > >   drivers/clk/qcom/clk-alpha-pll.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/clk/qcom/clk-alpha-pll.c
> > > b/drivers/clk/qcom/clk-alpha-pll.c
> > > index 2f620ccb41cb..fd8a82bb3690 100644
> > > --- a/drivers/clk/qcom/clk-alpha-pll.c
> > > +++ b/drivers/clk/qcom/clk-alpha-pll.c
> > > @@ -2126,7 +2126,7 @@ static void zonda_pll_adjust_l_val(unsigned
> > > long rate, unsigned long prate, u32
> > >       remainder = do_div(quotient, prate);
> > >       *l = quotient;
> > 
> > Since it's not a fix, but a simplification, you may wish to remove
> > an unnecessary 'quotient' local variable:
> > 
> > remainder = do_div(rate, prate);
> > 
> > > -    if ((remainder * 2) / prate)
> > > +    if ((remainder * 2) >= prate)
> > >           *l = *l + 1;
> > 
> > *l = rate + (u32)(remainder * 2 >= prate);
> 
> 
> The above change does fix this build error for me.
> 
> Satya, did you intend this to be a fix? Can we get this into -next?
> 
> Thanks
> Jon
> 
> -- 
> nvpublic

-- 
With best wishes
Dmitry

Re: [PATCH] clk: qcom: clk-alpha-pll: Replace divide operator with comparison

[Dan Carpenter]

On Wed, Aug 28, 2024 at 11:44:20PM +0300, Dmitry Baryshkov wrote:
> > > > Reported-by: kernel test robot <lkp@intel.com>
> > > > Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> > > > Closes: https://lore.kernel.org/r/202408110724.8pqbpDiD-lkp@intel.com/
> 
> this Closes tag must come after lkp's Reported-by. Please also add
> Closes with the link to Dan's report.
> 

No, this one is okay.  What happens is with some Smatch warnings, the bot
sends the email to me, I look it over and either discard or forward it on so
we get two Reported-bys for one email.

> > 
> > There is also the above smatch warning that was reported.
> 
> And the Smatch warning too should be a part of the commit message.
> 
> Last, but not least, as it is a fix, there should be a Fixes: tag and
> optionally a cc:stable.
> 

To be fair, at the time no one thought this was a Fix, just a cleanup.

regards,
dan carpenter

Re: [PATCH] clk: qcom: clk-alpha-pll: Replace divide operator with comparison

[Satya Priya Kakitapalli]

Hi Jon,


On 8/28/2024 7:17 PM, Jon Hunter wrote:
> Hi Satya, Vladimir,
>
> On 13/08/2024 21:01, Vladimir Zapolskiy wrote:
>> On 8/13/24 12:40, Satya Priya Kakitapalli wrote:
>>> In zonda_pll_adjust_l_val() replace the divide operator with comparison
>>> operator since comparisons are faster than divisions.
>>>
>>> Fixes: f4973130d255 ("clk: qcom: clk-alpha-pll: Update set_rate for 
>>> Zonda PLL")
>>
>> Apparently the change is not a fix, therefore I believe the Fixes tag
>> shall be removed.
>
>
> From the commit message it is not clear that this is a fix, but I
> believe that it is. With the current -next I am seeing the following
> build error (with GCC 7.3.1) on ARM ...
>
> drivers/clk/qcom/clk-alpha-pll.o: In function `clk_zonda_pll_set_rate':
> clk-alpha-pll.c:(.text+0x45dc): undefined reference to `__aeabi_uldivmod'
>
>>> Reported-by: kernel test robot <lkp@intel.com>
>>> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
>>> Closes: https://lore.kernel.org/r/202408110724.8pqbpDiD-lkp@intel.com/
>
> There is also the above smatch warning that was reported.
>
>>> Signed-off-by: Satya Priya Kakitapalli <quic_skakitap@quicinc.com>
>>> ---
>>>   drivers/clk/qcom/clk-alpha-pll.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/clk/qcom/clk-alpha-pll.c 
>>> b/drivers/clk/qcom/clk-alpha-pll.c
>>> index 2f620ccb41cb..fd8a82bb3690 100644
>>> --- a/drivers/clk/qcom/clk-alpha-pll.c
>>> +++ b/drivers/clk/qcom/clk-alpha-pll.c
>>> @@ -2126,7 +2126,7 @@ static void zonda_pll_adjust_l_val(unsigned 
>>> long rate, unsigned long prate, u32
>>>       remainder = do_div(quotient, prate);
>>>       *l = quotient;
>>
>> Since it's not a fix, but a simplification, you may wish to remove
>> an unnecessary 'quotient' local variable:
>>
>> remainder = do_div(rate, prate);
>>
>>> -    if ((remainder * 2) / prate)
>>> +    if ((remainder * 2) >= prate)
>>>           *l = *l + 1;
>>
>> *l = rate + (u32)(remainder * 2 >= prate);
>
>
> The above change does fix this build error for me.
>
> Satya, did you intend this to be a fix? Can we get this into -next?
>

Yes, I have posted a v2 for this last week, but there are few open 
comments on that, I'll address them and post V3 including the build 
error you reported in commit-text.


[v2] 
https://lore.kernel.org/linux-clk/20240814102005.33493-1-quic_skakitap@quicinc.com/


Thanks,

Satya Priya

Re: [PATCH] clk: qcom: clk-alpha-pll: Replace divide operator with comparison

[Jon Hunter]

Hi Satya,

On 30/08/2024 06:33, Satya Priya Kakitapalli wrote:
> Hi Jon,
> 
> 
> On 8/28/2024 7:17 PM, Jon Hunter wrote:
>> Hi Satya, Vladimir,
>>
>> On 13/08/2024 21:01, Vladimir Zapolskiy wrote:
>>> On 8/13/24 12:40, Satya Priya Kakitapalli wrote:
>>>> In zonda_pll_adjust_l_val() replace the divide operator with comparison
>>>> operator since comparisons are faster than divisions.
>>>>
>>>> Fixes: f4973130d255 ("clk: qcom: clk-alpha-pll: Update set_rate for 
>>>> Zonda PLL")
>>>
>>> Apparently the change is not a fix, therefore I believe the Fixes tag
>>> shall be removed.
>>
>>
>> From the commit message it is not clear that this is a fix, but I
>> believe that it is. With the current -next I am seeing the following
>> build error (with GCC 7.3.1) on ARM ...
>>
>> drivers/clk/qcom/clk-alpha-pll.o: In function `clk_zonda_pll_set_rate':
>> clk-alpha-pll.c:(.text+0x45dc): undefined reference to `__aeabi_uldivmod'
>>
>>>> Reported-by: kernel test robot <lkp@intel.com>
>>>> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
>>>> Closes: https://lore.kernel.org/r/202408110724.8pqbpDiD-lkp@intel.com/
>>
>> There is also the above smatch warning that was reported.
>>
>>>> Signed-off-by: Satya Priya Kakitapalli <quic_skakitap@quicinc.com>
>>>> ---
>>>>   drivers/clk/qcom/clk-alpha-pll.c | 2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/clk/qcom/clk-alpha-pll.c 
>>>> b/drivers/clk/qcom/clk-alpha-pll.c
>>>> index 2f620ccb41cb..fd8a82bb3690 100644
>>>> --- a/drivers/clk/qcom/clk-alpha-pll.c
>>>> +++ b/drivers/clk/qcom/clk-alpha-pll.c
>>>> @@ -2126,7 +2126,7 @@ static void zonda_pll_adjust_l_val(unsigned 
>>>> long rate, unsigned long prate, u32
>>>>       remainder = do_div(quotient, prate);
>>>>       *l = quotient;
>>>
>>> Since it's not a fix, but a simplification, you may wish to remove
>>> an unnecessary 'quotient' local variable:
>>>
>>> remainder = do_div(rate, prate);
>>>
>>>> -    if ((remainder * 2) / prate)
>>>> +    if ((remainder * 2) >= prate)
>>>>           *l = *l + 1;
>>>
>>> *l = rate + (u32)(remainder * 2 >= prate);
>>
>>
>> The above change does fix this build error for me.
>>
>> Satya, did you intend this to be a fix? Can we get this into -next?
>>
> 
> Yes, I have posted a v2 for this last week, but there are few open 
> comments on that, I'll address them and post V3 including the build 
> error you reported in commit-text.
> 
> 
> [v2] 
> https://lore.kernel.org/linux-clk/20240814102005.33493-1-quic_skakitap@quicinc.com/


Have you push a V3 yet?

Thanks
Jon

-- 
nvpublic

Re: [PATCH] clk: qcom: clk-alpha-pll: Replace divide operator with comparison

[Satya Priya Kakitapalli]

Hi Jon,


>>>>
>>>> remainder = do_div(rate, prate);
>>>>
>>>>> -    if ((remainder * 2) / prate)
>>>>> +    if ((remainder * 2) >= prate)
>>>>>           *l = *l + 1;
>>>>
>>>> *l = rate + (u32)(remainder * 2 >= prate);
>>>
>>>
>>> The above change does fix this build error for me.
>>>
>>> Satya, did you intend this to be a fix? Can we get this into -next?
>>>
>>
>> Yes, I have posted a v2 for this last week, but there are few open 
>> comments on that, I'll address them and post V3 including the build 
>> error you reported in commit-text.
>>
>>
>> [v2] 
>> https://lore.kernel.org/linux-clk/20240814102005.33493-1-quic_skakitap@quicinc.com/
>
>
> Have you push a V3 yet?


Not yet Jon, I had a query regarding Bjorn's comment where he mentioned 
the patch wouldn't compile on arm32 target, I am trying to check that 
with him. Once I get the details I'll make the changes accordingly and 
post V3.


Thanks,

Satya priya

Re: [PATCH] clk: qcom: clk-alpha-pll: Replace divide operator with comparison

[Satya Priya Kakitapalli]

Hi Vladimir,


On 8/14/2024 1:31 AM, Vladimir Zapolskiy wrote:
> On 8/13/24 12:40, Satya Priya Kakitapalli wrote:
>> In zonda_pll_adjust_l_val() replace the divide operator with comparison
>> operator since comparisons are faster than divisions.
>>
>> Fixes: f4973130d255 ("clk: qcom: clk-alpha-pll: Update set_rate for 
>> Zonda PLL")
>
> Apparently the change is not a fix, therefore I believe the Fixes tag
> shall be removed.
>
>> Reported-by: kernel test robot <lkp@intel.com>
>> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
>> Closes: https://lore.kernel.org/r/202408110724.8pqbpDiD-lkp@intel.com/
>> Signed-off-by: Satya Priya Kakitapalli <quic_skakitap@quicinc.com>
>> ---
>>   drivers/clk/qcom/clk-alpha-pll.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/clk/qcom/clk-alpha-pll.c 
>> b/drivers/clk/qcom/clk-alpha-pll.c
>> index 2f620ccb41cb..fd8a82bb3690 100644
>> --- a/drivers/clk/qcom/clk-alpha-pll.c
>> +++ b/drivers/clk/qcom/clk-alpha-pll.c
>> @@ -2126,7 +2126,7 @@ static void zonda_pll_adjust_l_val(unsigned 
>> long rate, unsigned long prate, u32
>>       remainder = do_div(quotient, prate);
>>       *l = quotient;
>
> Since it's not a fix, but a simplification, you may wish to remove
> an unnecessary 'quotient' local variable:
>
> remainder = do_div(rate, prate);
>

I tried removing the quotient variable, but it is leading to below build 
errors on arm32 architectures, so I will add the quotient variable back 
on V3,  to make the pointer type compatible for both arm32 and arm64.


error: passing argument 1 of '__div64_32' from incompatible pointer type 
[-Werror=incompatible-pointer-types]
   238 |   __rem = __div64_32(&(n), __base);


expected 'uint64_t *' {aka 'long long unsigned int *'} but argument is 
of type 'long unsigned int *'
    24 | static inline uint32_t __div64_32(uint64_t *n, uint32_t base)


>>   -    if ((remainder * 2) / prate)
>> +    if ((remainder * 2) >= prate)
>>           *l = *l + 1;
>
> *l = rate + (u32)(remainder * 2 >= prate);
>
> I hope the assignment above is quite clear...
>
>>   }
>
> With the review comments above implemented, feel free to add to v2
>
> Reviewed-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
>
> -- 
> Best wishes,
> Vladimir