| Message ID | 1461809273-4603-1-git-send-email-van.freenix@gmail.com |
|---|---|
| State | Accepted |
| Commit | c510f2e436008e55a50b063f2180cb1e63984224 |
| Headers | show |
On Thu, Apr 28, 2016 at 10:07:53AM +0800, Peng Fan wrote: > Some toolchains fail to build > "clk->rate = (u64)(clk->parent->rate * 16) / div;" > And the cast usage is wrong. > > Use the following code to fix the issue, > " > do_div(parent_rate, div); > clk->rate = parent_rate; > " > > Reported-by: Peter Robinson <pbrobinson@gmail.com> > Signed-off-by: Peng Fan <van.freenix@gmail.com> > Cc: Stefano Babic <sbabic@denx.de> > Cc: Fabio Estevam <fabio.estevam@nxp.com> > Cc: Tom Rini <trini@konsulko.com> > Cc: Anatolij Gustschin <agust@denx.de> > Cc: Peter Robinson <pbrobinson@gmail.com> Reviewed-by: Tom Rini <trini@konsulko.com>
On Thu, Apr 28, 2016 at 3:07 AM, Peng Fan <van.freenix@gmail.com> wrote: > Some toolchains fail to build > "clk->rate = (u64)(clk->parent->rate * 16) / div;" > And the cast usage is wrong. > > Use the following code to fix the issue, > " > do_div(parent_rate, div); > clk->rate = parent_rate; > " > > Reported-by: Peter Robinson <pbrobinson@gmail.com> > Signed-off-by: Peng Fan <van.freenix@gmail.com> > Cc: Stefano Babic <sbabic@denx.de> > Cc: Fabio Estevam <fabio.estevam@nxp.com> > Cc: Tom Rini <trini@konsulko.com> > Cc: Anatolij Gustschin <agust@denx.de> > Cc: Peter Robinson <pbrobinson@gmail.com> Tested-by: Peter Robinson <pbrobinson@gmail.com> > --- > > Hi Peter, > > Please help test this patch to see whether this fix your issue or not. > Thanks for pointing out this issue. It fixes the build issue, I'll be installing it onto a number of devices over the next couple of days. Thanks, Peter > Thanks, > Peng. > > drivers/video/ipu_common.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/video/ipu_common.c b/drivers/video/ipu_common.c > index 36d4b23..5676a0f 100644 > --- a/drivers/video/ipu_common.c > +++ b/drivers/video/ipu_common.c > @@ -352,7 +352,9 @@ static int ipu_pixel_clk_set_rate(struct clk *clk, unsigned long rate) > */ > __raw_writel((div / 16) << 16, DI_BS_CLKGEN1(clk->id)); > > - clk->rate = (u64)(clk->parent->rate * 16) / div; > + do_div(parent_rate, div); > + > + clk->rate = parent_rate; > > return 0; > } > -- > 2.6.2 >
On 28/04/2016 16:20, Peter Robinson wrote: > On Thu, Apr 28, 2016 at 3:07 AM, Peng Fan <van.freenix@gmail.com> wrote: >> Some toolchains fail to build >> "clk->rate = (u64)(clk->parent->rate * 16) / div;" >> And the cast usage is wrong. >> >> Use the following code to fix the issue, >> " >> do_div(parent_rate, div); >> clk->rate = parent_rate; >> " >> >> Reported-by: Peter Robinson <pbrobinson@gmail.com> >> Signed-off-by: Peng Fan <van.freenix@gmail.com> >> Cc: Stefano Babic <sbabic@denx.de> >> Cc: Fabio Estevam <fabio.estevam@nxp.com> >> Cc: Tom Rini <trini@konsulko.com> >> Cc: Anatolij Gustschin <agust@denx.de> >> Cc: Peter Robinson <pbrobinson@gmail.com> > Tested-by: Peter Robinson <pbrobinson@gmail.com> > >> --- >> >> Hi Peter, >> >> Please help test this patch to see whether this fix your issue or not. >> Thanks for pointing out this issue. > > It fixes the build issue, I'll be installing it onto a number of > devices over the next couple of days. > > Thanks, > Peter > >> Thanks, >> Peng. >> >> drivers/video/ipu_common.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/video/ipu_common.c b/drivers/video/ipu_common.c >> index 36d4b23..5676a0f 100644 >> --- a/drivers/video/ipu_common.c >> +++ b/drivers/video/ipu_common.c >> @@ -352,7 +352,9 @@ static int ipu_pixel_clk_set_rate(struct clk *clk, unsigned long rate) >> */ >> __raw_writel((div / 16) << 16, DI_BS_CLKGEN1(clk->id)); >> >> - clk->rate = (u64)(clk->parent->rate * 16) / div; >> + do_div(parent_rate, div); >> + >> + clk->rate = parent_rate; >> >> return 0; >> } >> -- >> 2.6.2 >> Applied to u-boot-imx, thanks ! Best regards, Stefano Babic
Hi Stefano,
On Thu, 28 Apr 2016 16:29:43 +0200
Stefano Babic sbabic@denx.de wrote:
...
> Applied to u-boot-imx, thanks !
Thanks!
--
Anatolij
Hi Peng, Can you tell that I'm hunting a bug in an old version? I'm seeing a **very** intermittent regression between U-Boot versions 2015.07 and 2016.05 and happened to spot something in this patch. On 04/27/2016 07:07 PM, Peng Fan wrote: > Some toolchains fail to build > "clk->rate = (u64)(clk->parent->rate * 16) / div;" > And the cast usage is wrong. > > Use the following code to fix the issue, > " > do_div(parent_rate, div); > clk->rate = parent_rate; > " > > Reported-by: Peter Robinson <pbrobinson@gmail.com> > Signed-off-by: Peng Fan <van.freenix@gmail.com> > Cc: Stefano Babic <sbabic@denx.de> > Cc: Fabio Estevam <fabio.estevam@nxp.com> > Cc: Tom Rini <trini@konsulko.com> > Cc: Anatolij Gustschin <agust@denx.de> > Cc: Peter Robinson <pbrobinson@gmail.com> > Reviewed-by: Tom Rini <trini@konsulko.com> > Tested-by: Peter Robinson <pbrobinson@gmail.com> > --- > > Hi Peter, > > Please help test this patch to see whether this fix your issue or not. > Thanks for pointing out this issue. > > Thanks, > Peng. > > drivers/video/ipu_common.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/video/ipu_common.c b/drivers/video/ipu_common.c > index 36d4b23..5676a0f 100644 > --- a/drivers/video/ipu_common.c > +++ b/drivers/video/ipu_common.c > @@ -352,7 +352,9 @@ static int ipu_pixel_clk_set_rate(struct clk *clk, unsigned long rate) > */ > __raw_writel((div / 16) << 16, DI_BS_CLKGEN1(clk->id)); > Did we lose a multiply by 16 in this change? > - clk->rate = (u64)(clk->parent->rate * 16) / div; > + do_div(parent_rate, div); > + > + clk->rate = parent_rate; > > return 0; > } > Please advise, Eric
Hi Peng, Can you tell that I'm hunting a bug in an old version? I'm seeing a **very** intermittent regression between U-Boot versions 2015.07 and 2016.05 and happened to spot something in this patch. On 04/27/2016 07:07 PM, Peng Fan wrote: > Some toolchains fail to build > "clk->rate = (u64)(clk->parent->rate * 16) / div;" > And the cast usage is wrong. > > Use the following code to fix the issue, > " > do_div(parent_rate, div); > clk->rate = parent_rate; > " > > Reported-by: Peter Robinson <pbrobinson@gmail.com> > Signed-off-by: Peng Fan <van.freenix@gmail.com> > Cc: Stefano Babic <sbabic@denx.de> > Cc: Fabio Estevam <fabio.estevam@nxp.com> > Cc: Tom Rini <trini@konsulko.com> > Cc: Anatolij Gustschin <agust@denx.de> > Cc: Peter Robinson <pbrobinson@gmail.com> > Reviewed-by: Tom Rini <trini@konsulko.com> > Tested-by: Peter Robinson <pbrobinson@gmail.com> > --- > > Hi Peter, > > Please help test this patch to see whether this fix your issue or not. > Thanks for pointing out this issue. > > Thanks, > Peng. > > drivers/video/ipu_common.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/video/ipu_common.c b/drivers/video/ipu_common.c > index 36d4b23..5676a0f 100644 > --- a/drivers/video/ipu_common.c > +++ b/drivers/video/ipu_common.c > @@ -352,7 +352,9 @@ static int ipu_pixel_clk_set_rate(struct clk *clk, unsigned long rate) > */ > __raw_writel((div / 16) << 16, DI_BS_CLKGEN1(clk->id)); > Did you intend to lose a multiply by 16 here? > - clk->rate = (u64)(clk->parent->rate * 16) / div; > + do_div(parent_rate, div); > + > + clk->rate = parent_rate; > > return 0; > } > Please advise, Eric
Hi Eric, On Mon, Sep 4, 2017 at 11:49 PM, Eric Nelson <eric@nelint.com> wrote: > Hi Peng, > > Can you tell that I'm hunting a bug in an old version? > > I'm seeing a **very** intermittent regression between U-Boot > versions 2015.07 and 2016.05 and happened to spot something > in this patch. Just curious: how does the regression manifest itself?
Hi Fabio, On 09/05/2017 06:33 AM, Fabio Estevam wrote: > Hi Eric, > > On Mon, Sep 4, 2017 at 11:49 PM, Eric Nelson <eric@nelint.com> wrote: >> Hi Peng, >> >> Can you tell that I'm hunting a bug in an old version? >> >> I'm seeing a **very** intermittent regression between U-Boot >> versions 2015.07 and 2016.05 and happened to spot something >> in this patch. > > Just curious: how does the regression manifest itself? > With **some** televisions at a client site, on **some** power-on cycles, the HDMI output under Linux doesn't seem to be generated properly. We haven't been able to reproduce it in-house, so testing is taking a while, and we haven't (yet) determined if the divisor patch has anything to do with the problem. We are running on an i.MX6DL, but the IPU clock frequency change doesn't fix the issue (running at 19.8MHz instead of 26MHz). All we know at the moment is that version 2015.07 works and 2016.05 fails with essentially no changes to the board files. We're doing this remotely across time zones with limited access to failing machine(s), so it may take the rest of the week before we know for sure. I'll update the thread when we nail it down. Regards, Eric
Hi Eric, On Mon, Sep 04, 2017 at 07:48:56PM -0700, Eric Nelson wrote: >Hi Peng, > >Can you tell that I'm hunting a bug in an old version? > >I'm seeing a **very** intermittent regression between U-Boot >versions 2015.07 and 2016.05 and happened to spot something >in this patch. > >On 04/27/2016 07:07 PM, Peng Fan wrote: >>Some toolchains fail to build >>"clk->rate = (u64)(clk->parent->rate * 16) / div;" >>And the cast usage is wrong. >> >>Use the following code to fix the issue, >>" >> do_div(parent_rate, div); >> clk->rate = parent_rate; >>" >> >>Reported-by: Peter Robinson <pbrobinson@gmail.com> >>Signed-off-by: Peng Fan <van.freenix@gmail.com> >>Cc: Stefano Babic <sbabic@denx.de> >>Cc: Fabio Estevam <fabio.estevam@nxp.com> >>Cc: Tom Rini <trini@konsulko.com> >>Cc: Anatolij Gustschin <agust@denx.de> >>Cc: Peter Robinson <pbrobinson@gmail.com> >>Reviewed-by: Tom Rini <trini@konsulko.com> >>Tested-by: Peter Robinson <pbrobinson@gmail.com> >>--- >> >>Hi Peter, >> >> Please help test this patch to see whether this fix your issue or not. >> Thanks for pointing out this issue. >> >>Thanks, >>Peng. >> >> drivers/video/ipu_common.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >>diff --git a/drivers/video/ipu_common.c b/drivers/video/ipu_common.c >>index 36d4b23..5676a0f 100644 >>--- a/drivers/video/ipu_common.c >>+++ b/drivers/video/ipu_common.c >>@@ -352,7 +352,9 @@ static int ipu_pixel_clk_set_rate(struct clk *clk, unsigned long rate) >> */ >> __raw_writel((div / 16) << 16, DI_BS_CLKGEN1(clk->id)); > >Did we lose a multiply by 16 in this change? We already have "parent_rate = (unsigned long long)clk->parent->rate * 16;" in this function. Thanks, Peng. > >>- clk->rate = (u64)(clk->parent->rate * 16) / div; >>+ do_div(parent_rate, div); >>+ >>+ clk->rate = parent_rate; >> return 0; >> } >> > >Please advise, > > >Eric
Thanks Peng. On 09/05/2017 06:16 PM, Peng Fan wrote: > On Mon, Sep 04, 2017 at 07:48:56PM -0700, Eric Nelson wrote: >> Hi Peng, >> >> Can you tell that I'm hunting a bug in an old version? >> >> I'm seeing a **very** intermittent regression between U-Boot >> versions 2015.07 and 2016.05 and happened to spot something >> in this patch. >> >> On 04/27/2016 07:07 PM, Peng Fan wrote: >>> Some toolchains fail to build >>> "clk->rate = (u64)(clk->parent->rate * 16) / div;" >>> And the cast usage is wrong. >>> >>> Use the following code to fix the issue, >>> " >>> do_div(parent_rate, div); >>> clk->rate = parent_rate; >>> " >>> >>> Reported-by: Peter Robinson <pbrobinson@gmail.com> >>> Signed-off-by: Peng Fan <van.freenix@gmail.com> >>> Cc: Stefano Babic <sbabic@denx.de> >>> Cc: Fabio Estevam <fabio.estevam@nxp.com> >>> Cc: Tom Rini <trini@konsulko.com> >>> Cc: Anatolij Gustschin <agust@denx.de> >>> Cc: Peter Robinson <pbrobinson@gmail.com> >>> Reviewed-by: Tom Rini <trini@konsulko.com> >>> Tested-by: Peter Robinson <pbrobinson@gmail.com> >>> --- >>> >>> Hi Peter, >>> >>> Please help test this patch to see whether this fix your issue or not. >>> Thanks for pointing out this issue. >>> >>> Thanks, >>> Peng. >>> >>> drivers/video/ipu_common.c | 4 +++- >>> 1 file changed, 3 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/video/ipu_common.c b/drivers/video/ipu_common.c >>> index 36d4b23..5676a0f 100644 >>> --- a/drivers/video/ipu_common.c >>> +++ b/drivers/video/ipu_common.c >>> @@ -352,7 +352,9 @@ static int ipu_pixel_clk_set_rate(struct clk *clk, unsigned long rate) >>> */ >>> __raw_writel((div / 16) << 16, DI_BS_CLKGEN1(clk->id)); >> >> Did we lose a multiply by 16 in this change? > > We already have "parent_rate = (unsigned long long)clk->parent->rate * 16;" > in this function. > Hmmm. So this patch also fixed a bug, since we previously had **two** multiply-by-16's: http://git.denx.de/?p=u-boot.git;a=blob;f=drivers/video/ipu_common.c;hb=3cb4f25cc702db17455583599d0940c81337a17a Regards, Eric
Hi, On Wed, 6 Sep 2017 10:34:33 -0700 Eric Nelson wrote: > Thanks Peng. > > On 09/05/2017 06:16 PM, Peng Fan wrote: > > On Mon, Sep 04, 2017 at 07:48:56PM -0700, Eric Nelson wrote: > >> Hi Peng, > >> > >> Can you tell that I'm hunting a bug in an old version? > >> > >> I'm seeing a **very** intermittent regression between U-Boot > >> versions 2015.07 and 2016.05 and happened to spot something > >> in this patch. > >> > >> On 04/27/2016 07:07 PM, Peng Fan wrote: > >>> Some toolchains fail to build > >>> "clk->rate = (u64)(clk->parent->rate * 16) / div;" > >>> And the cast usage is wrong. > >>> > >>> Use the following code to fix the issue, > >>> " > >>> do_div(parent_rate, div); > >>> clk->rate = parent_rate; > >>> " > >>> > >>> Reported-by: Peter Robinson <pbrobinson@gmail.com> > >>> Signed-off-by: Peng Fan <van.freenix@gmail.com> > >>> Cc: Stefano Babic <sbabic@denx.de> > >>> Cc: Fabio Estevam <fabio.estevam@nxp.com> > >>> Cc: Tom Rini <trini@konsulko.com> > >>> Cc: Anatolij Gustschin <agust@denx.de> > >>> Cc: Peter Robinson <pbrobinson@gmail.com> > >>> Reviewed-by: Tom Rini <trini@konsulko.com> > >>> Tested-by: Peter Robinson <pbrobinson@gmail.com> > >>> --- > >>> > >>> Hi Peter, > >>> > >>> Please help test this patch to see whether this fix your issue or not. > >>> Thanks for pointing out this issue. > >>> > >>> Thanks, > >>> Peng. > >>> > >>> drivers/video/ipu_common.c | 4 +++- > >>> 1 file changed, 3 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/drivers/video/ipu_common.c b/drivers/video/ipu_common.c > >>> index 36d4b23..5676a0f 100644 > >>> --- a/drivers/video/ipu_common.c > >>> +++ b/drivers/video/ipu_common.c > >>> @@ -352,7 +352,9 @@ static int ipu_pixel_clk_set_rate(struct clk *clk, unsigned long rate) > >>> */ > >>> __raw_writel((div / 16) << 16, DI_BS_CLKGEN1(clk->id)); > >> > >> Did we lose a multiply by 16 in this change? > > > > We already have "parent_rate = (unsigned long long)clk->parent->rate * 16;" > > in this function. > > > > Hmmm. So this patch also fixed a bug, since we previously had > **two** multiply-by-16's: > No! The 'second' multiply by 16 used the clk->parent->rate, not the 'parent_rate' which was multiplied by 16... | parent_rate = (unsigned long long)clk->parent->rate * 16; [...] | clk->rate = (u64)(clk->parent->rate * 16) / div; Lothar Waßmann
On 09/06/2017 11:01 PM, Lothar Waßmann wrote: > On Wed, 6 Sep 2017 10:34:33 -0700 Eric Nelson wrote: >> On 09/05/2017 06:16 PM, Peng Fan wrote: >>> On Mon, Sep 04, 2017 at 07:48:56PM -0700, Eric Nelson wrote: >>>> Hi Peng, >>>> >>>> Can you tell that I'm hunting a bug in an old version? >>>> >>>> I'm seeing a **very** intermittent regression between U-Boot >>>> versions 2015.07 and 2016.05 and happened to spot something >>>> in this patch. >>>> >>>> On 04/27/2016 07:07 PM, Peng Fan wrote: >>>>> Some toolchains fail to build >>>>> "clk->rate = (u64)(clk->parent->rate * 16) / div;" >>>>> And the cast usage is wrong. >>>>> >>>>> Use the following code to fix the issue, >>>>> " >>>>> do_div(parent_rate, div); >>>>> clk->rate = parent_rate; >>>>> " >>>>> >>>>> Reported-by: Peter Robinson <pbrobinson@gmail.com> >>>>> Signed-off-by: Peng Fan <van.freenix@gmail.com> >>>>> Cc: Stefano Babic <sbabic@denx.de> >>>>> Cc: Fabio Estevam <fabio.estevam@nxp.com> >>>>> Cc: Tom Rini <trini@konsulko.com> >>>>> Cc: Anatolij Gustschin <agust@denx.de> >>>>> Cc: Peter Robinson <pbrobinson@gmail.com> >>>>> Reviewed-by: Tom Rini <trini@konsulko.com> >>>>> Tested-by: Peter Robinson <pbrobinson@gmail.com> >>>>> --- >>>>> >>>>> Hi Peter, >>>>> >>>>> Please help test this patch to see whether this fix your issue or not. >>>>> Thanks for pointing out this issue. >>>>> >>>>> Thanks, >>>>> Peng. >>>>> >>>>> drivers/video/ipu_common.c | 4 +++- >>>>> 1 file changed, 3 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/video/ipu_common.c b/drivers/video/ipu_common.c >>>>> index 36d4b23..5676a0f 100644 >>>>> --- a/drivers/video/ipu_common.c >>>>> +++ b/drivers/video/ipu_common.c >>>>> @@ -352,7 +352,9 @@ static int ipu_pixel_clk_set_rate(struct clk *clk, unsigned long rate) >>>>> */ >>>>> __raw_writel((div / 16) << 16, DI_BS_CLKGEN1(clk->id)); >>>> >>>> Did we lose a multiply by 16 in this change? >>> >>> We already have "parent_rate = (unsigned long long)clk->parent->rate * 16;" >>> in this function. >>> >> >> Hmmm. So this patch also fixed a bug, since we previously had >> **two** multiply-by-16's: >> > No! The 'second' multiply by 16 used the clk->parent->rate, not the > 'parent_rate' which was multiplied by 16... > > | parent_rate = (unsigned long long)clk->parent->rate * 16; > [...] > | clk->rate = (u64)(clk->parent->rate * 16) / div; > Doh! I clearly missed that. Thanks Lothar.
diff --git a/drivers/video/ipu_common.c b/drivers/video/ipu_common.c index 36d4b23..5676a0f 100644 --- a/drivers/video/ipu_common.c +++ b/drivers/video/ipu_common.c @@ -352,7 +352,9 @@ static int ipu_pixel_clk_set_rate(struct clk *clk, unsigned long rate) */ __raw_writel((div / 16) << 16, DI_BS_CLKGEN1(clk->id)); - clk->rate = (u64)(clk->parent->rate * 16) / div; + do_div(parent_rate, div); + + clk->rate = parent_rate; return 0; }
Some toolchains fail to build "clk->rate = (u64)(clk->parent->rate * 16) / div;" And the cast usage is wrong. Use the following code to fix the issue, " do_div(parent_rate, div); clk->rate = parent_rate; " Reported-by: Peter Robinson <pbrobinson@gmail.com> Signed-off-by: Peng Fan <van.freenix@gmail.com> Cc: Stefano Babic <sbabic@denx.de> Cc: Fabio Estevam <fabio.estevam@nxp.com> Cc: Tom Rini <trini@konsulko.com> Cc: Anatolij Gustschin <agust@denx.de> Cc: Peter Robinson <pbrobinson@gmail.com> --- Hi Peter, Please help test this patch to see whether this fix your issue or not. Thanks for pointing out this issue. Thanks, Peng. drivers/video/ipu_common.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)