| Message ID | 20240813094035.974317-1-quic_skakitap@quicinc.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| 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
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
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
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
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
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
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(-)