* Re: [PATCH] clk: qcom: clk-alpha-pll: Replace divide operator with comparison [not found] ` <4d314b61-7483-4ceb-ac72-10dbd7e4522a@linaro.org> @ 2024-08-28 13:47 ` Jon Hunter 2024-08-28 20:44 ` Dmitry Baryshkov 2024-08-30 5:33 ` Satya Priya Kakitapalli 0 siblings, 2 replies; 6+ messages in thread From: Jon Hunter @ 2024-08-28 13:47 UTC (permalink / raw) To: Vladimir Zapolskiy, Satya Priya Kakitapalli, Bjorn Andersson, Michael Turquette, Stephen Boyd Cc: linux-arm-msm, linux-clk, linux-kernel, Ajit Pandey, Imran Shaik, Taniya Das, Jagadeesh Kona, kernel test robot, Dan Carpenter, linux-tegra@vger.kernel.org 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] clk: qcom: clk-alpha-pll: Replace divide operator with comparison 2024-08-28 13:47 ` [PATCH] clk: qcom: clk-alpha-pll: Replace divide operator with comparison Jon Hunter @ 2024-08-28 20:44 ` Dmitry Baryshkov 2024-08-29 9:14 ` Dan Carpenter 2024-08-30 5:33 ` Satya Priya Kakitapalli 1 sibling, 1 reply; 6+ messages in thread From: Dmitry Baryshkov @ 2024-08-28 20:44 UTC (permalink / raw) To: Jon Hunter Cc: Vladimir Zapolskiy, Satya Priya Kakitapalli, Bjorn Andersson, Michael Turquette, Stephen Boyd, linux-arm-msm, linux-clk, linux-kernel, Ajit Pandey, Imran Shaik, Taniya Das, Jagadeesh Kona, kernel test robot, Dan Carpenter, linux-tegra@vger.kernel.org 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] clk: qcom: clk-alpha-pll: Replace divide operator with comparison 2024-08-28 20:44 ` Dmitry Baryshkov @ 2024-08-29 9:14 ` Dan Carpenter 0 siblings, 0 replies; 6+ messages in thread From: Dan Carpenter @ 2024-08-29 9:14 UTC (permalink / raw) To: Dmitry Baryshkov Cc: Jon Hunter, Vladimir Zapolskiy, Satya Priya Kakitapalli, Bjorn Andersson, Michael Turquette, Stephen Boyd, linux-arm-msm, linux-clk, linux-kernel, Ajit Pandey, Imran Shaik, Taniya Das, Jagadeesh Kona, kernel test robot, linux-tegra@vger.kernel.org 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] clk: qcom: clk-alpha-pll: Replace divide operator with comparison 2024-08-28 13:47 ` [PATCH] clk: qcom: clk-alpha-pll: Replace divide operator with comparison Jon Hunter 2024-08-28 20:44 ` Dmitry Baryshkov @ 2024-08-30 5:33 ` Satya Priya Kakitapalli 2024-09-04 21:26 ` Jon Hunter 1 sibling, 1 reply; 6+ messages in thread From: Satya Priya Kakitapalli @ 2024-08-30 5:33 UTC (permalink / raw) To: Jon Hunter, Vladimir Zapolskiy, Bjorn Andersson, Michael Turquette, Stephen Boyd Cc: linux-arm-msm, linux-clk, linux-kernel, Ajit Pandey, Imran Shaik, Taniya Das, Jagadeesh Kona, kernel test robot, Dan Carpenter, linux-tegra@vger.kernel.org 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] clk: qcom: clk-alpha-pll: Replace divide operator with comparison 2024-08-30 5:33 ` Satya Priya Kakitapalli @ 2024-09-04 21:26 ` Jon Hunter 2024-09-05 10:58 ` Satya Priya Kakitapalli 0 siblings, 1 reply; 6+ messages in thread From: Jon Hunter @ 2024-09-04 21:26 UTC (permalink / raw) To: Satya Priya Kakitapalli, Vladimir Zapolskiy, Bjorn Andersson, Michael Turquette, Stephen Boyd Cc: linux-arm-msm, linux-clk, linux-kernel, Ajit Pandey, Imran Shaik, Taniya Das, Jagadeesh Kona, kernel test robot, Dan Carpenter, linux-tegra@vger.kernel.org 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] clk: qcom: clk-alpha-pll: Replace divide operator with comparison 2024-09-04 21:26 ` Jon Hunter @ 2024-09-05 10:58 ` Satya Priya Kakitapalli 0 siblings, 0 replies; 6+ messages in thread From: Satya Priya Kakitapalli @ 2024-09-05 10:58 UTC (permalink / raw) To: Jon Hunter, Vladimir Zapolskiy, Bjorn Andersson, Michael Turquette, Stephen Boyd Cc: linux-arm-msm, linux-clk, linux-kernel, Ajit Pandey, Imran Shaik, Taniya Das, Jagadeesh Kona, kernel test robot, Dan Carpenter, linux-tegra@vger.kernel.org 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-09-05 10:58 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20240813094035.974317-1-quic_skakitap@quicinc.com>
[not found] ` <4d314b61-7483-4ceb-ac72-10dbd7e4522a@linaro.org>
2024-08-28 13:47 ` [PATCH] clk: qcom: clk-alpha-pll: Replace divide operator with comparison Jon Hunter
2024-08-28 20:44 ` Dmitry Baryshkov
2024-08-29 9:14 ` Dan Carpenter
2024-08-30 5:33 ` Satya Priya Kakitapalli
2024-09-04 21:26 ` Jon Hunter
2024-09-05 10:58 ` Satya Priya Kakitapalli
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox