Skip to contentSkip to conversation

video: ipu: avoid overflow issue

PATCH New Peng Fan 7 participants 15 messages Mar 9, 2016 – Sep 6, 2017

Subsystems (2)
lore
Share & tools
lore

Peng Fan

author
#0 lore raw

PATCH 1/3 video: ipu: avoid overflow issue

Multiplication, as "clk->parent->rate * 16" may overflow. So use do_div to avoid such issue.

Cc Anatolij Gustschin <[email protected]>
Cc Stefano Babic <[email protected]>
Cc Fabio Estevam <[email protected]>
---
 drivers/video/ipu_common.c | 73 ++++++++++++++++++++++++++++++++++------------
 1 file changed, 54 insertions(+), 19 deletions(-)
drivers/video/ipu_common.c +54 -19
index 9f85102..36d4b23 100644
@@ -19,6 +19,7 @@
 #include <asm/errno.h>
 #include <asm/arch/imx-regs.h>
 #include <asm/arch/crm_regs.h>
+#include <div64.h>
 #include "ipu.h"
 #include "ipu_regs.h"
 
@@ -275,50 +276,84 @@ static inline void ipu_ch_param_set_buffer(uint32_t ch, int bufNum,
 
 static void ipu_pixel_clk_recalc(struct clk *clk)
 {
-	u32 div = __raw_readl(DI_BS_CLKGEN0(clk->id));
-	if (div == 0)
-		clk->rate = 0;
-	else
-		clk->rate = (clk->parent->rate * 16) / div;
+	u32 div;
+	u64 final_rate = (unsigned long long)clk->parent->rate * 16;
+
+	div = __raw_readl(DI_BS_CLKGEN0(clk->id));
+	debug("read BS_CLKGEN0 div:%d, final_rate:%lld, prate:%ld\n",
+	      div, final_rate, clk->parent->rate);
+
+	clk->rate = 0;
+	if (div != 0) {
+		do_div(final_rate, div);
+		clk->rate = final_rate;
+	}
 }
 
 static unsigned long ipu_pixel_clk_round_rate(struct clk *clk,
 	unsigned long rate)
 {
-	u32 div, div1;
-	u32 tmp;
+	u64 div, final_rate;
+	u32 remainder;
+	u64 parent_rate = (unsigned long long)clk->parent->rate * 16;
+
 	/*
 	 * Calculate divider
 	 * Fractional part is 4 bits,
 	 * so simply multiply by 2^4 to get fractional part.
 	 */
-	tmp = (clk->parent->rate * 16);
-	div = tmp / rate;
-
+	div = parent_rate;
+	remainder = do_div(div, rate);
+	/* Round the divider value */
+	if (remainder > (rate / 2))
+		div++;
 	if (div < 0x10)            /* Min DI disp clock divider is 1 */
 		div = 0x10;
 	if (div & ~0xFEF)
 		div &= 0xFF8;
 	else {
-		div1 = div & 0xFE0;
-		if ((tmp/div1 - tmp/div) < rate / 4)
-			div = div1;
-		else
-			div &= 0xFF8;
+		/* Round up divider if it gets us closer to desired pix clk */
+		if ((div & 0xC) == 0xC) {
+			div += 0x10;
+			div &= ~0xF;
+		}
 	}
-	return (clk->parent->rate * 16) / div;
+	final_rate = parent_rate;
+	do_div(final_rate, div);
+
+	return final_rate;
 }
 
 static int ipu_pixel_clk_set_rate(struct clk *clk, unsigned long rate)
 {
-	u32 div = (clk->parent->rate * 16) / rate;
+	u64 div, parent_rate;
+	u32 remainder;
+
+	parent_rate = (unsigned long long)clk->parent->rate * 16;
+	div = parent_rate;
+	remainder = do_div(div, rate);
+	/* Round the divider value */
+	if (remainder > (rate / 2))
+		div++;
+
+	/* Round up divider if it gets us closer to desired pix clk */
+	if ((div & 0xC) == 0xC) {
+		div += 0x10;
+		div &= ~0xF;
+	}
+	if (div > 0x1000)
+		debug("Overflow, DI_BS_CLKGEN0 div:0x%x\n", (u32)div);
 
 	__raw_writel(div, DI_BS_CLKGEN0(clk->id));
 
-	/* Setup pixel clock timing */
+	/*
+	 * Setup pixel clock timing
+	 * Down time is half of period
+	 */
 	__raw_writel((div / 16) << 16, DI_BS_CLKGEN1(clk->id));
 
-	clk->rate = (clk->parent->rate * 16) / div;
+	clk->rate = (u64)(clk->parent->rate * 16) / div;
+
 	return 0;
 }
 
Signature · 1 line
2.6.2
author
#1 lore raw

PATCH 2/3 imx: mx6: correct IPU clock

The CONFIG_IPUV3_CLK should be 264000000, to i.MX6DL, it should be 198000000.

Cc Stefano Babic <[email protected]>
Cc Fabio Estevam <[email protected]>
Cc Peter Robinson <[email protected]>
---
 include/configs/mx6sabre_common.h | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
include/configs/mx6sabre_common.h +5 -1
index 29d1f91..a6d821b 100644
@@ -225,7 +225,11 @@
 #define CONFIG_BMP_16BPP
 #define CONFIG_VIDEO_LOGO
 #define CONFIG_VIDEO_BMP_LOGO
-#define CONFIG_IPUV3_CLK 260000000
+#ifdef CONFIG_MX6DL
+#define CONFIG_IPUV3_CLK 198000000
+#else
+#define CONFIG_IPUV3_CLK 264000000
+#endif
 #define CONFIG_IMX_HDMI
 #define CONFIG_IMX_VIDEO_SKIP
 
Signature · 1 line
2.6.2
#2 lore raw

PATCH 2/3 imx: mx6: correct IPU clock

Quoted 11 lines · On Wed, 9 Mar 2016 16:07:22 +0800 Peng Fan van.freenix at gmail.com wrote:

The CONFIG_IPUV3_CLK should be 264000000, to i.MX6DL, it should be 198000000.

Cc Stefano Babic <[email protected]>
Cc Fabio Estevam <[email protected]>
Cc Peter Robinson <[email protected]>
---
 include/configs/mx6sabre_common.h | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

applied to u-boot-video/master, thanks!

--
Anatolij

#3 lore raw

PATCH 2/3 imx: mx6: correct IPU clock

In reply to Peng Fan

Hi Peng,

Pardon the reference to an old update, but do you have a description of the symptoms that brought about this patch?

Quoted 17 lines · Peng Fan

The CONFIG_IPUV3_CLK should be 264000000, to i.MX6DL, it should be 198000000.

Cc Stefano Babic <[email protected]>
Cc Fabio Estevam <[email protected]>
Cc Peter Robinson <[email protected]>
---
  include/configs/mx6sabre_common.h | 6 +++++-
  1 file changed, 5 insertions(+), 1 deletion(-)
include/configs/mx6sabre_common.h +0 -0
index 29d1f91..a6d821b 100644

include/configs/mx6sabre_common.h line 232

Quoted 9 lines
include/configs/mx6sabre_common.h +5 -1
@@ -225,7 +225,11 @@
  #define CONFIG_BMP_16BPP
  #define CONFIG_VIDEO_LOGO
  #define CONFIG_VIDEO_BMP_LOGO
-#define CONFIG_IPUV3_CLK 260000000
+#ifdef CONFIG_MX6DL
+#define CONFIG_IPUV3_CLK 198000000
+#else
+#define CONFIG_IPUV3_CLK 264000000
+#endif

Note that this should probably be applied for other boards which are compiled for multiple CPU types.

At least the Boundary Nitrogen boards, but probably others like Wand have ordering options for DL or Solo processors and may need the reduced clock rate.

include/configs/mx6sabre_common.h line 235

Quoted 3 lines
include/configs/mx6sabre_common.h +0 -0
  #define CONFIG_IMX_HDMI
  #define CONFIG_IMX_VIDEO_SKIP
 

Please advise,

Eric

#4 lore raw

PATCH 2/3 imx: mx6: correct IPU clock

Hi all,

Adding my normal e-mail account to the chain, since the other account isn't registered on the list.

Quoted 50 lines · Eric Nelson

Hi Peng,

Pardon the reference to an old update, but do you have a description of the symptoms that brought about this patch?

Quoted 18 lines · Peng Fan

The CONFIG_IPUV3_CLK should be 264000000, to i.MX6DL, it should be 198000000.

Cc Stefano Babic <[email protected]>
Cc Fabio Estevam <[email protected]>
Cc Peter Robinson <[email protected]>
---
  include/configs/mx6sabre_common.h | 6 +++++-
  1 file changed, 5 insertions(+), 1 deletion(-)
include/configs/mx6sabre_common.h +0 -0
index 29d1f91..a6d821b 100644

include/configs/mx6sabre_common.h line 232

Quoted 9 lines
include/configs/mx6sabre_common.h +5 -1
@@ -225,7 +225,11 @@
  #define CONFIG_BMP_16BPP
  #define CONFIG_VIDEO_LOGO
  #define CONFIG_VIDEO_BMP_LOGO
-#define CONFIG_IPUV3_CLK 260000000
+#ifdef CONFIG_MX6DL
+#define CONFIG_IPUV3_CLK 198000000
+#else
+#define CONFIG_IPUV3_CLK 264000000
+#endif

Note that this should probably be applied for other boards which are compiled for multiple CPU types.

At least the Boundary Nitrogen boards, but probably others like Wand have ordering options for DL or Solo processors and may need the reduced clock rate.

include/configs/mx6sabre_common.h line 234

Quoted 2 lines
include/configs/mx6sabre_common.h +0 -0
  #define CONFIG_IMX_HDMI
  #define CONFIG_IMX_VIDEO_SKIP

Please advise,

Eric

#5 lore raw

PATCH 2/3 imx: mx6: correct IPU clock

In reply to Eric Nelson

Hi Eric,

Quoted 18 lines · Eric Nelson

include/configs/mx6sabre_common.h line 232

Quoted 12 lines
include/configs/mx6sabre_common.h +5 -1
@@ -225,7 +225,11 @@
  #define CONFIG_BMP_16BPP
  #define CONFIG_VIDEO_LOGO
  #define CONFIG_VIDEO_BMP_LOGO
-#define CONFIG_IPUV3_CLK 260000000
+#ifdef CONFIG_MX6DL
+#define CONFIG_IPUV3_CLK 198000000
+#else
+#define CONFIG_IPUV3_CLK 264000000
+#endif

Note that this should probably be applied for other boards which are compiled for multiple CPU types.

Quoted 3 lines

At least the Boundary Nitrogen boards, but probably others like Wand have ordering options for DL or Solo processors and may need the reduced clock rate.

Agreed. The clock frequency decision should be done in run-time rather than in build-time.

#6 lore raw

PATCH 2/3 imx: mx6: correct IPU clock

Quoted 22 lines · Fabio Estevam

Hi Eric,

Quoted 18 lines · Eric Nelson

include/configs/mx6sabre_common.h line 232

Quoted 12 lines
include/configs/mx6sabre_common.h +5 -1
@@ -225,7 +225,11 @@
  #define CONFIG_BMP_16BPP
  #define CONFIG_VIDEO_LOGO
  #define CONFIG_VIDEO_BMP_LOGO
-#define CONFIG_IPUV3_CLK 260000000
+#ifdef CONFIG_MX6DL
+#define CONFIG_IPUV3_CLK 198000000
+#else
+#define CONFIG_IPUV3_CLK 264000000
+#endif

Note that this should probably be applied for other boards which are compiled for multiple CPU types.

Quoted 6 lines
Quoted 3 lines

At least the Boundary Nitrogen boards, but probably others like Wand have ordering options for DL or Solo processors and may need the reduced clock rate.

Agreed. The clock frequency decision should be done in run-time rather than in build-time.

I agree, too. We have mechanism to take decisions at run time, at least based on SOC type. Anyway, Anatolji has already merged this - should be better to revert it ?

Regards,
Stefano

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
=====================================================================
#7 lore raw

PATCH 2/3 imx: mx6: correct IPU clock

Hi Stefano,

Quoted 27 lines · Stefano Babic
Quoted 26 lines · Fabio Estevam

Hi Eric,

Quoted 21 lines · Eric Nelson

include/configs/mx6sabre_common.h line 232

Quoted 12 lines
include/configs/mx6sabre_common.h +5 -1
@@ -225,7 +225,11 @@
   #define CONFIG_BMP_16BPP
   #define CONFIG_VIDEO_LOGO
   #define CONFIG_VIDEO_BMP_LOGO
-#define CONFIG_IPUV3_CLK 260000000
+#ifdef CONFIG_MX6DL
+#define CONFIG_IPUV3_CLK 198000000
+#else
+#define CONFIG_IPUV3_CLK 264000000
+#endif

Note that this should probably be applied for other boards which are compiled for multiple CPU types.

At least the Boundary Nitrogen boards, but probably others like Wand have ordering options for DL or Solo processors and may need the reduced clock rate.

Quoted 7 lines
Quoted 2 lines

Agreed. The clock frequency decision should be done in run-time rather than in build-time.

I agree, too. We have mechanism to take decisions at run time, at least based on SOC type. Anyway, Anatolji has already merged this - should be better to revert it ?

I don't think it should be reverted until we have a run-time decision in place, or we'll re-introduce whatever problem the higher rate caused, at least on SABRE boards with Solo or Dual-Lite processors.

I'm still wondering whether Peng has a description of the ramifications of the higher rate on DL/Solo processors.

Regards,

Eric

#8 lore raw

PATCH 2/3 imx: mx6: correct IPU clock

Hi Eric/Stefano,

Quoted 4 lines · Eric Nelson

I don't think it should be reverted until we have a run-time decision in place, or we'll re-introduce whatever problem the higher rate caused, at least on SABRE boards with Solo or Dual-Lite processors.

I have just sent a RFC patch that introduces the IPU clock setting in run-time on mx6.

author
#9 lore raw

PATCH 2/3 imx: mx6: correct IPU clock

In reply to Eric Nelson
Quoted 4 lines · Eric Nelson

Hi Peng,

Pardon the reference to an old update, but do you have a description of the symptoms that brought about this patch?

Sorry for late reply. Runtime calculation is better.

The clk here is IPU HSP clock, which default sources mmdc ch clock. To DL, the mmdc ch clock is 396M and the IPU HSP podf is 2, so the lock is 198M.

Regards,
Peng.

Quoted 45 lines · Peng Fan
Quoted 17 lines

The CONFIG_IPUV3_CLK should be 264000000, to i.MX6DL, it should be 198000000.

Cc Stefano Babic <[email protected]>
Cc Fabio Estevam <[email protected]>
Cc Peter Robinson <[email protected]>
---
 include/configs/mx6sabre_common.h | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
include/configs/mx6sabre_common.h +0 -0
index 29d1f91..a6d821b 100644

include/configs/mx6sabre_common.h line 232

Quoted 9 lines
include/configs/mx6sabre_common.h +5 -1
@@ -225,7 +225,11 @@
 #define CONFIG_BMP_16BPP
 #define CONFIG_VIDEO_LOGO
 #define CONFIG_VIDEO_BMP_LOGO
-#define CONFIG_IPUV3_CLK 260000000
+#ifdef CONFIG_MX6DL
+#define CONFIG_IPUV3_CLK 198000000
+#else
+#define CONFIG_IPUV3_CLK 264000000
+#endif

Note that this should probably be applied for other boards which are compiled for multiple CPU types.

At least the Boundary Nitrogen boards, but probably others like Wand have ordering options for DL or Solo processors and may need the reduced clock rate.

include/configs/mx6sabre_common.h line 234

Quoted 2 lines
include/configs/mx6sabre_common.h +0 -0
 #define CONFIG_IMX_HDMI
 #define CONFIG_IMX_VIDEO_SKIP

Please advise,

Eric

#10 lore raw

PATCH 2/3 imx: mx6: correct IPU clock

In reply to Eric Nelson
Quoted 46 lines · Eric Nelson

Hi Peng,

Pardon the reference to an old update, but do you have a description of the symptoms that brought about this patch?

Quoted 17 lines · Peng Fan

The CONFIG_IPUV3_CLK should be 264000000, to i.MX6DL, it should be 198000000.

Cc Stefano Babic <[email protected]>
Cc Fabio Estevam <[email protected]>
Cc Peter Robinson <[email protected]>
---
   include/configs/mx6sabre_common.h | 6 +++++-
   1 file changed, 5 insertions(+), 1 deletion(-)
include/configs/mx6sabre_common.h +0 -0
index 29d1f91..a6d821b 100644

include/configs/mx6sabre_common.h line 232

Quoted 9 lines
include/configs/mx6sabre_common.h +5 -1
@@ -225,7 +225,11 @@
   #define CONFIG_BMP_16BPP
   #define CONFIG_VIDEO_LOGO
   #define CONFIG_VIDEO_BMP_LOGO
-#define CONFIG_IPUV3_CLK 260000000
+#ifdef CONFIG_MX6DL
+#define CONFIG_IPUV3_CLK 198000000
+#else
+#define CONFIG_IPUV3_CLK 264000000
+#endif

Note that this should probably be applied for other boards which are compiled for multiple CPU types.

At least the Boundary Nitrogen boards, but probably others like Wand have ordering options for DL or Solo processors and may need the reduced clock rate.

include/configs/mx6sabre_common.h line 235

Quoted 3 lines
include/configs/mx6sabre_common.h +0 -0
   #define CONFIG_IMX_HDMI
   #define CONFIG_IMX_VIDEO_SKIP
 
Quoted 9 lines

Please advise,

Eric
_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
https://lists.denx.de/listinfo/u-boot

CONFIG_IPUV3_CLK is used to indicate the default rate for IPU HSP clock. The IPU driver in u-boot won't calculate the HSP clock rate according to CCM registers, it needs this setting to know current rate. 198Mhz is the correct value on DL not the 264Mhz.

If you select IPU DI clock (pixel clock) derived from HSP clock not the external clock like LDB DI clock, I believe the 264Mhz will cause problem.

Best regards,
Ye Li

#11 lore raw

PATCH 2/3 imx: mx6: correct IPU clock

Thanks Ye (and Peng).

Quoted 57 lines · Ye Li
Quoted 46 lines · Eric Nelson

Hi Peng,

Pardon the reference to an old update, but do you have a description of the symptoms that brought about this patch?

Quoted 17 lines · Peng Fan

The CONFIG_IPUV3_CLK should be 264000000, to i.MX6DL, it should be 198000000.

Cc Stefano Babic <[email protected]>
Cc Fabio Estevam <[email protected]>
Cc Peter Robinson <[email protected]>
---
    include/configs/mx6sabre_common.h | 6 +++++-
    1 file changed, 5 insertions(+), 1 deletion(-)
include/configs/mx6sabre_common.h +0 -0
index 29d1f91..a6d821b 100644

include/configs/mx6sabre_common.h line 232

Quoted 9 lines
include/configs/mx6sabre_common.h +5 -1
@@ -225,7 +225,11 @@
    #define CONFIG_BMP_16BPP
    #define CONFIG_VIDEO_LOGO
    #define CONFIG_VIDEO_BMP_LOGO
-#define CONFIG_IPUV3_CLK 260000000
+#ifdef CONFIG_MX6DL
+#define CONFIG_IPUV3_CLK 198000000
+#else
+#define CONFIG_IPUV3_CLK 264000000
+#endif

Note that this should probably be applied for other boards which are compiled for multiple CPU types.

At least the Boundary Nitrogen boards, but probably others like Wand have ordering options for DL or Solo processors and may need the reduced clock rate.

include/configs/mx6sabre_common.h line 235

Quoted 3 lines
include/configs/mx6sabre_common.h +0 -0
    #define CONFIG_IMX_HDMI
    #define CONFIG_IMX_VIDEO_SKIP
 
Quoted 9 lines

Please advise,

Eric
_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
https://lists.denx.de/listinfo/u-boot

Quoted 8 lines

CONFIG_IPUV3_CLK is used to indicate the default rate for IPU HSP clock. The IPU driver in u-boot won't calculate the HSP clock rate according to CCM registers, it needs this setting to know current rate. 198Mhz is the correct value on DL not the 264Mhz.

If you select IPU DI clock (pixel clock) derived from HSP clock not the external clock like LDB DI clock, I believe the 264Mhz will cause problem.

Do you know what sort of problem was seen (if any)?

Please advise,

Eric

author
#12 lore raw

PATCH 3/3 imx: mx6: hdmi: handle overflow condition

In reply to Peng Fan

If HDMI_IH_FC_STAT2_OVERFLOW_MASK is set, we need to do TMDS software reset and write to clear fc_invidconf register. We need minimum 3 times to write to clear the fc_invidconf register, so choose 5 loops here.

Cc Stefano Babic <[email protected]>
Cc Fabio Estevam <[email protected]>
---
 arch/arm/cpu/armv7/mx6/soc.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)
arch/arm/cpu/armv7/mx6/soc.c +12 -1
index a34675c..91a3deb 100644
@@ -548,7 +548,8 @@ void imx_setup_hdmi(void)
 {
 	struct mxc_ccm_reg *mxc_ccm = (struct mxc_ccm_reg *)CCM_BASE_ADDR;
 	struct hdmi_regs *hdmi  = (struct hdmi_regs *)HDMI_ARB_BASE_ADDR;
-	int reg;
+	int reg, count;
+	u8 val;
 
 	/* Turn on HDMI PHY clock */
 	reg = readl(&mxc_ccm->CCGR2);
@@ -565,6 +566,16 @@ void imx_setup_hdmi(void)
 		 |(CHSCCDR_IPU_PRE_CLK_540M_PFD
 		 << MXC_CCM_CHSCCDR_IPU1_DI0_PRE_CLK_SEL_OFFSET);
 	writel(reg, &mxc_ccm->chsccdr);
+
+	/* Clear the overflow condition */
+	if (readb(&hdmi->ih_fc_stat2) & HDMI_IH_FC_STAT2_OVERFLOW_MASK) {
+		/* TMDS software reset */
+		writeb((u8)~HDMI_MC_SWRSTZ_TMDSSWRST_REQ, &hdmi->mc_swrstz);
+		val = readb(&hdmi->fc_invidconf);
+		/* Need minimum 3 times to write to clear the register */
+		for (count = 0 ; count < 5 ; count++)
+			writeb(val, &hdmi->fc_invidconf);
+	}
 }
 #endif
 
Signature · 1 line
2.6.2
#13 lore raw

PATCH 3/3 imx: mx6: hdmi: handle overflow condition

Quoted 5 lines · On Wed, 9 Mar 2016 16:07:23 +0800 Peng Fan van.freenix at gmail.com wrote:

If HDMI_IH_FC_STAT2_OVERFLOW_MASK is set, we need to do TMDS software reset and write to clear fc_invidconf register. We need minimum 3 times to write to clear the fc_invidconf register, so choose 5 loops here.

Quoted 7 lines
Cc Stefano Babic <[email protected]>
Cc Fabio Estevam <[email protected]>
---
 arch/arm/cpu/armv7/mx6/soc.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

applied to u-boot-video/master, thanks!

--
Anatolij

#14 lore raw

PATCH 1/3 video: ipu: avoid overflow issue

In reply to Peng Fan

Hi,

Quoted 11 lines · On Wed, 9 Mar 2016 16:07:21 +0800 Peng Fan van.freenix at gmail.com wrote:

Multiplication, as "clk->parent->rate * 16" may overflow. So use do_div to avoid such issue.

Cc Anatolij Gustschin <[email protected]>
Cc Stefano Babic <[email protected]>
Cc Fabio Estevam <[email protected]>
---
 drivers/video/ipu_common.c | 73 ++++++++++++++++++++++++++++++++++------------
 1 file changed, 54 insertions(+), 19 deletions(-)

applied to u-boot-video/master, thanks!

--
Anatolij