Re: [U-Boot] video: ipu_common: fix build error
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 Signed-off-by: Peng Fan Cc: Stefano Babic Cc: Fabio Estevam Cc: Tom Rini Cc: Anatolij Gustschin Cc: Peter Robinson Reviewed-by: Tom Rini Tested-by: Peter Robinson --- 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. ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] video: ipu_common: fix build error
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 > >>> Signed-off-by: Peng Fan > >>> Cc: Stefano Babic > >>> Cc: Fabio Estevam > >>> Cc: Tom Rini > >>> Cc: Anatolij Gustschin > >>> Cc: Peter Robinson > >>> Reviewed-by: Tom Rini > >>> Tested-by: Peter Robinson > >>> --- > >>> > >>> 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 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] video: ipu_common: fix build error
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 Signed-off-by: Peng Fan Cc: Stefano Babic Cc: Fabio Estevam Cc: Tom Rini Cc: Anatolij Gustschin Cc: Peter Robinson Reviewed-by: Tom Rini Tested-by: Peter Robinson --- 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 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] video: ipu_common: fix build error
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 >>Signed-off-by: Peng Fan >>Cc: Stefano Babic >>Cc: Fabio Estevam >>Cc: Tom Rini >>Cc: Anatolij Gustschin >>Cc: Peter Robinson >>Reviewed-by: Tom Rini >>Tested-by: Peter Robinson >>--- >> >>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 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] video: ipu_common: fix build error
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 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 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] video: ipu_common: fix build error
Hi Eric, On Mon, Sep 4, 2017 at 11:49 PM, 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. Just curious: how does the regression manifest itself? ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] video: ipu_common: fix build error
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 Signed-off-by: Peng Fan Cc: Stefano Babic Cc: Fabio Estevam Cc: Tom Rini Cc: Anatolij Gustschin Cc: Peter Robinson Reviewed-by: Tom Rini Tested-by: Peter Robinson --- 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 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] video: ipu_common: fix build error
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 Signed-off-by: Peng Fan Cc: Stefano Babic Cc: Fabio Estevam Cc: Tom Rini Cc: Anatolij Gustschin Cc: Peter Robinson Reviewed-by: Tom Rini Tested-by: Peter Robinson --- 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 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
