| Message ID | 20240813094035.974317-1-quic_skakitap@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | clk: qcom: clk-alpha-pll: Replace divide operator with comparison | expand |
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
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
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
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
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; }
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(-)