video: ipu_common: fix build error

PATCH In review Peng Fan 9 participants 13 messages Apr 28, 2016 – Sep 7, 2017 Framebuffer layer

lore

Share & tools

lore

Peng Fan

author

2016-04-28 02:07 UTC #0 lore raw

PATCH 1/1 video: ipu_common: fix build error

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 <[email protected]>

Signed-off-by Peng Fan <[email protected]>

Cc Stefano Babic <[email protected]>

Cc Fabio Estevam <[email protected]>

Cc Tom Rini <[email protected]>

Cc Anatolij Gustschin <[email protected]>

Cc Peter Robinson <[email protected]>

---

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(-)

drivers/video/ipu_common.c +3 -1

index 36d4b23..5676a0f 100644
@@ -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;
 }

Signature · 1 line

2.6.2

Tom Rini

2016-04-28 12:52 UTC #1 lore raw

PATCH 1/1 video: ipu_common: fix build error

Quoted 10 lines · Peng Fan

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;
"

Quoted 7 lines

Reported-by Peter Robinson <[email protected]>
Signed-off-by Peng Fan <[email protected]>
Cc Stefano Babic <[email protected]>
Cc Fabio Estevam <[email protected]>
Cc Tom Rini <[email protected]>
Cc Anatolij Gustschin <[email protected]>
Cc Peter Robinson <[email protected]>

Reviewed-by Tom Rini <[email protected]>

Signature · 8 lines

Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160428/755484ed/attachment.sig>

Peter Robinson

2016-04-28 14:20 UTC #2 lore raw

PATCH 1/1 video: ipu_common: fix build error

In reply to Peng Fan

Quoted 10 lines · Peng Fan

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;
"

Quoted 7 lines

Reported-by Peter Robinson <[email protected]>
Signed-off-by Peng Fan <[email protected]>
Cc Stefano Babic <[email protected]>
Cc Fabio Estevam <[email protected]>
Cc Tom Rini <[email protected]>
Cc Anatolij Gustschin <[email protected]>
Cc Peter Robinson <[email protected]>

Tested-by Peter Robinson <[email protected]>

Quoted 6 lines

---
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

Quoted 24 lines

Thanks,
Peng.

 drivers/video/ipu_common.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

drivers/video/ipu_common.c +3 -2

index 36d4b23..5676a0f 100644
@@ -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

Stefano Babic

2016-04-28 14:29 UTC #3 lore raw

PATCH 1/1 video: ipu_common: fix build error

Quoted 52 lines · Peter Robinson

Quoted 10 lines · Peng Fan
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;
"
Quoted 7 lines
Reported-by Peter Robinson <[email protected]>
Signed-off-by Peng Fan <[email protected]>
Cc Stefano Babic <[email protected]>
Cc Fabio Estevam <[email protected]>
Cc Tom Rini <[email protected]>
Cc Anatolij Gustschin <[email protected]>
Cc Peter Robinson <[email protected]>
Tested-by Peter Robinson <[email protected]>
Quoted 6 lines
---
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
Quoted 19 lines
Thanks,
Peng.
 drivers/video/ipu_common.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/video/ipu_common.c +3 -1
index 36d4b23..5676a0f 100644
@@ -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;
 

Quoted 5 lines

Quoted 5 lines
drivers/video/ipu_common.c +0 -1
        return 0;
 }
--
2.6.2

Applied to u-boot-imx, thanks !

Best regards,
Stefano Babic

Signature · 5 lines

=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

Anatolij Gustschin

2016-04-28 14:44 UTC #4 lore raw

PATCH 1/1 video: ipu_common: fix build error

Hi Stefano,

On Thu, 28 Apr 2016 16:29:43 +0200
Stefano Babic sbabic at denx.de wrote:
...

Quoted 1 line

Applied to u-boot-imx, thanks !

Thanks!

--
Anatolij

Eric Nelson

2017-09-05 02:48 UTC #5 lore raw

In reply to Peng Fan

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.

Quoted 37 lines · Peng Fan

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 <[email protected]>
Signed-off-by Peng Fan <[email protected]>
Cc Stefano Babic <[email protected]>
Cc Fabio Estevam <[email protected]>
Cc Tom Rini <[email protected]>
Cc Anatolij Gustschin <[email protected]>
Cc Peter Robinson <[email protected]>
Reviewed-by Tom Rini <[email protected]>
Tested-by Peter Robinson <[email protected]>
---
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(-)
drivers/video/ipu_common.c +0 -0
index 36d4b23..5676a0f 100644

drivers/video/ipu_common.c static int ipu_pixel_clk_set_rate(struct clk *clk, unsigned long rate) · line 354

Quoted 3 lines

drivers/video/ipu_common.c +0 -0

@@ -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?

drivers/video/ipu_common.c static int ipu_pixel_clk_set_rate(struct clk *clk, unsigned long rate) · line 360

Quoted 8 lines

drivers/video/ipu_common.c +3 -1

-	clk->rate = (u64)(clk->parent->rate * 16) / div;
+	do_div(parent_rate, div);
+
+	clk->rate = parent_rate;
 
  	return 0;
  }

Please advise,

Eric

Peng Fan

author

2017-09-06 01:16 UTC #6 lore raw

Hi Eric,

Quoted 41 lines · Eric Nelson

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.
Quoted 32 lines · Peng Fan
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 <[email protected]>
Signed-off-by Peng Fan <[email protected]>
Cc Stefano Babic <[email protected]>
Cc Fabio Estevam <[email protected]>
Cc Tom Rini <[email protected]>
Cc Anatolij Gustschin <[email protected]>
Cc Peter Robinson <[email protected]>
Reviewed-by Tom Rini <[email protected]>
Tested-by Peter Robinson <[email protected]>
---
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(-)

Quoted 9 lines

drivers/video/ipu_common.c static int ipu_pixel_clk_set_rate(struct clk *clk, unsigned long rate) · line 353
Quoted 7 lines
drivers/video/ipu_common.c +0 -0
index 36d4b23..5676a0f 100644
@@ -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.

Quoted 13 lines

drivers/video/ipu_common.c static int ipu_pixel_clk_set_rate(struct clk *clk, unsigned long rate)
Quoted 8 lines
drivers/video/ipu_common.c +3 -1
-	clk->rate = (u64)(clk->parent->rate * 16) / div;
+	do_div(parent_rate, div);
+
+	clk->rate = parent_rate;
 	return 0;
 }
 
Please advise,
Eric

Eric Nelson

2017-09-06 17:34 UTC #7 lore raw

Thanks Peng.

Quoted 52 lines · Peng Fan

Quoted 41 lines · Eric Nelson
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.
Quoted 32 lines · Peng Fan
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 <[email protected]>
Signed-off-by Peng Fan <[email protected]>
Cc Stefano Babic <[email protected]>
Cc Fabio Estevam <[email protected]>
Cc Tom Rini <[email protected]>
Cc Anatolij Gustschin <[email protected]>
Cc Peter Robinson <[email protected]>
Reviewed-by Tom Rini <[email protected]>
Tested-by Peter Robinson <[email protected]>
---
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(-)
Quoted 9 lines
drivers/video/ipu_common.c static int ipu_pixel_clk_set_rate(struct clk *clk, unsigned long rate) · line 353
Quoted 7 lines
drivers/video/ipu_common.c +0 -0
index 36d4b23..5676a0f 100644
@@ -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?

Quoted 3 lines

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

Lothar Waßmann

2017-09-07 06:01 UTC #8 lore raw

Hi,

Quoted 55 lines · On Wed, 6 Sep 2017 10:34:33 -0700 Eric Nelson wrote:

Thanks Peng.
Quoted 52 lines · Peng Fan
Quoted 50 lines · Eric Nelson
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.
Quoted 37 lines · Peng Fan
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 <[email protected]>
Signed-off-by Peng Fan <[email protected]>
Cc Stefano Babic <[email protected]>
Cc Fabio Estevam <[email protected]>
Cc Tom Rini <[email protected]>
Cc Anatolij Gustschin <[email protected]>
Cc Peter Robinson <[email protected]>
Reviewed-by Tom Rini <[email protected]>
Tested-by Peter Robinson <[email protected]>
---
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(-)
drivers/video/ipu_common.c +0 -0
index 36d4b23..5676a0f 100644
drivers/video/ipu_common.c static int ipu_pixel_clk_set_rate(struct clk *clk, unsigned long rate) · line 353
Quoted 2 lines
drivers/video/ipu_common.c +0 -0
@@ -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?

Quoted 7 lines

Quoted 3 lines
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

Eric Nelson

2017-09-07 07:23 UTC #9 lore raw

Quoted 61 lines · Lothar WaÃŸmann

Quoted 53 lines · On Wed, 6 Sep 2017 10:34:33 -0700 Eric Nelson wrote:
Quoted 52 lines · Peng Fan
Quoted 50 lines · Eric Nelson
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.
Quoted 37 lines · Peng Fan
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 <[email protected]>
Signed-off-by Peng Fan <[email protected]>
Cc Stefano Babic <[email protected]>
Cc Fabio Estevam <[email protected]>
Cc Tom Rini <[email protected]>
Cc Anatolij Gustschin <[email protected]>
Cc Peter Robinson <[email protected]>
Reviewed-by Tom Rini <[email protected]>
Tested-by Peter Robinson <[email protected]>
---
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(-)
drivers/video/ipu_common.c +0 -0
index 36d4b23..5676a0f 100644
drivers/video/ipu_common.c static int ipu_pixel_clk_set_rate(struct clk *clk, unsigned long rate) · line 353
Quoted 2 lines
drivers/video/ipu_common.c +0 -0
@@ -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?
Quoted 7 lines
Quoted 3 lines
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:

Quoted 7 lines

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.

Eric Nelson

2017-09-05 02:49 UTC #10 lore raw

In reply to Peng Fan

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.

Quoted 37 lines · Peng Fan

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 <[email protected]>
Signed-off-by Peng Fan <[email protected]>
Cc Stefano Babic <[email protected]>
Cc Fabio Estevam <[email protected]>
Cc Tom Rini <[email protected]>
Cc Anatolij Gustschin <[email protected]>
Cc Peter Robinson <[email protected]>
Reviewed-by Tom Rini <[email protected]>
Tested-by Peter Robinson <[email protected]>
---
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(-)
drivers/video/ipu_common.c +0 -0
index 36d4b23..5676a0f 100644

drivers/video/ipu_common.c static int ipu_pixel_clk_set_rate(struct clk *clk, unsigned long rate) · line 354

Quoted 3 lines

drivers/video/ipu_common.c +0 -0

@@ -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?

drivers/video/ipu_common.c static int ipu_pixel_clk_set_rate(struct clk *clk, unsigned long rate) · line 360

Quoted 8 lines

drivers/video/ipu_common.c +3 -1

-	clk->rate = (u64)(clk->parent->rate * 16) / div;
+	do_div(parent_rate, div);
+
+	clk->rate = parent_rate;
 
  	return 0;
  }

Please advise,

Eric

Fabio Estevam

2017-09-05 13:33 UTC #11 lore raw

Hi Eric,

Quoted 7 lines · Eric Nelson

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?

Eric Nelson

2017-09-05 13:56 UTC #12 lore raw

Hi Fabio,

Quoted 5 lines · Fabio Estevam

Hi Eric,
Quoted 2 lines · Eric Nelson
Hi Peng,

Quoted 8 lines

Quoted 5 lines
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