[PATCH 0/2] media: ov5640: Re-implement MIPI clock tree configuration

[PATCH 0/2] media: ov5640: Re-implement MIPI clock tree configuration

[Jacopo Mondi]

Hello ov5640 people,
   this small series based on top of Maxime's v4
"media: ov5640: Misc cleanup and improvements"
which fixes configuration of the MIPI clock tree which I have found not
working in Maxime's series.

As the aforementioned series containes a lot of useful things, I based my work
on top of it with the hope this can be included in Maxime's next v5.

I have tested capture with a MIPI CSI-2 2 lane interface, with the following
results (frame rates reported by yavta)

1920x1080
Captured 100 frames in 6.696864 seconds (14.932362 fps, 61927490.138103 B/s).
Captured 100 frames in 3.355459 seconds (29.802182 fps, 123595609.386497 B/s).
Captured 100 frames in 1.656115 seconds (60.382280 fps, 37098872.964740 B/s).

1024x768
Captured 100 frames in 7.425156 seconds (13.467729 fps, 21182906.577451 B/s).
Captured 100 frames in 3.431391 seconds (29.142700 fps, 45837504.382334 B/s).
Captured 100 frames in 1.667302 seconds (59.977113 fps, 36849938.056268 B/s).

640x480
Captured 100 frames in 6.656321 seconds (15.023312 fps, 9230323.152105 B/s).
Captured 100 frames in 3.323515 seconds (30.088620 fps, 18486448.133840 B/s).
Captured 100 frames in 1.665959 seconds (60.025487 fps, 36879659.103255 B/s).

320x240
Captured 100 frames in 6.660575 seconds (15.013718 fps, 2306107.089817 B/s).
Captured 100 frames in 3.333978 seconds (29.994199 fps, 4607108.983740 B/s).
Captured 100 frames in 1.666797 seconds (59.995308 fps, 36861117.460615 B/s).

I have also verified images are good in all resolutions.

Sam has just shared his fixes for MIPI CSI-2 which are indeed different from
these ones. My dream now would be to unify all of our changes in next Maxime's
series iteration and have this merged.

Please note that once this gets in, we could then get rid of fixed framerates,
and hopefully improve mode settings and configuration \o/.

I'll try to look into Sam's series next, and see if conflicts with this, or
those can be merged together.

A lot of interesting stuff happening on this driver!

Thanks
   j

Jacopo Mondi (2):
  media: ov5640: Add check for PLL1 output max frequency
  media: ov5640: Re-implement MIPI clock configuration

 drivers/media/i2c/ov5640.c | 124 ++++++++++++++++++++++++++++++++++++---------
 1 file changed, 99 insertions(+), 25 deletions(-)

--
2.7.4

[PATCH 1/2] media: ov5640: Add check for PLL1 output max frequency

[Jacopo Mondi]

Check that the PLL1 output frequency does not exceed the maximum allowed 1GHz
frequency.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/ov5640.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index e098435..1f2e72d 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -770,7 +770,7 @@ static int ov5640_mod_reg(struct ov5640_dev *sensor, u16 reg,
  * always set to either 1 or 2 in the vendor kernels.
  */
 #define OV5640_SYSDIV_MIN	1
-#define OV5640_SYSDIV_MAX	2
+#define OV5640_SYSDIV_MAX	16
 
 /*
  * This is supposed to be ranging from 1 to 16, but the value is always
@@ -806,15 +806,20 @@ static int ov5640_mod_reg(struct ov5640_dev *sensor, u16 reg,
  * This is supposed to be ranging from 1 to 8, but the value is always
  * set to 1 in the vendor kernels.
  */
-#define OV5640_PCLK_ROOT_DIV	1
+#define OV5640_PCLK_ROOT_DIV			1
+#define OV5640_PLL_SYS_ROOT_DIVIDER_BYPASS	0x00
 
 static unsigned long ov5640_compute_sys_clk(struct ov5640_dev *sensor,
 					    u8 pll_prediv, u8 pll_mult,
 					    u8 sysdiv)
 {
-	unsigned long rate = clk_get_rate(sensor->xclk);
+	unsigned long sysclk = sensor->xclk_freq / pll_prediv * pll_mult;
 
-	return rate / pll_prediv * pll_mult / sysdiv;
+	/* PLL1 output cannot exceed 1GHz. */
+	if (sysclk / 1000000 > 1000)
+		return 0;
+
+	return sysclk / sysdiv;
 }
 
 static unsigned long ov5640_calc_sys_clk(struct ov5640_dev *sensor,
@@ -844,6 +849,16 @@ static unsigned long ov5640_calc_sys_clk(struct ov5640_dev *sensor,
 			_rate = ov5640_compute_sys_clk(sensor,
 						       OV5640_PLL_PREDIV,
 						       _pll_mult, _sysdiv);
+
+			/*
+			 * We have reached the maximum allowed PLL1 output,
+			 * increase sysdiv.
+			 */
+			if (rate == 0) {
+				_pll_mult = OV5640_PLL_MULT_MAX + 1;
+				continue;
+			}
+
 			if (abs(rate - _rate) < abs(rate - best)) {
 				best = _rate;
 				best_sysdiv = _sysdiv;
-- 
2.7.4

[PATCH 2/2] media: ov5640: Re-implement MIPI clock configuration

[Jacopo Mondi]

Modify the MIPI clock tree calculation procedure.
The implemented function receives the total bandwidth required in bytes
and calculate the sample rate and the MIPI clock rate accordingly.

Tested with capture at 1080p, 720p, VGA and QVGA.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/ov5640.c | 101 +++++++++++++++++++++++++++++++++++----------
 1 file changed, 80 insertions(+), 21 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 1f2e72d..8a0ead9 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -720,6 +720,9 @@ static int ov5640_mod_reg(struct ov5640_dev *sensor, u16 reg,
  *               +->| MIPI Divider | - reg 0x3035, bits 0-3
  *               |  +-+------------+
  *               |    +----------------> MIPI SCLK
+ *               |    +  +-----+
+ *               |    +->| / 2 |-------> MIPI BIT CLK
+ *               |       +-----+
  *               |  +--------------+
  *               +->| PLL Root Div | - reg 0x3037, bit 4
  *                  +-+------------+
@@ -782,13 +785,14 @@ static int ov5640_mod_reg(struct ov5640_dev *sensor, u16 reg,
  * This is supposed to be ranging from 1 to 2, but the value is always
  * set to 2 in the vendor kernels.
  */
-#define OV5640_PLL_ROOT_DIV	2
+#define OV5640_PLL_ROOT_DIV			2
+#define OV5640_PLL_CTRL3_PLL_ROOT_DIV_2		BIT(4)
 
 /*
- * This is supposed to be either 1, 2 or 2.5, but the value is always
- * set to 2 in the vendor kernels.
+ * We only supports 8-bit formats at the moment
  */
-#define OV5640_BIT_DIV		2
+#define OV5640_BIT_DIV				2
+#define OV5640_PLL_CTRL0_MIPI_MODE_8BIT		0x08
 
 /*
  * This is supposed to be ranging from 1 to 8, but the value is always
@@ -874,32 +878,81 @@ static unsigned long ov5640_calc_sys_clk(struct ov5640_dev *sensor,
 	*sysdiv = best_sysdiv;
 	*pll_prediv = OV5640_PLL_PREDIV;
 	*pll_mult = best_mult;
+
 	return best;
 }
 
-static unsigned long ov5640_calc_mipi_clk(struct ov5640_dev *sensor,
-					  unsigned long rate,
-					  u8 *pll_prediv, u8 *pll_mult,
-					  u8 *sysdiv, u8 *mipi_div)
+/*
+ * ov5640_set_mipi_pclk() - Calculate the clock tree configuration values
+ *			    for the MIPI CSI-2 output.
+ *
+ * @rate: The requested bandwidth in bytes per second.
+ *	  It is calculated as: HTOT * VTOT * FPS * bpp
+ *
+ * This function use the requested bandwidth to calculate:
+ * - sample_rate = bandwidth / bpp;
+ * - mipi_clk = bandwidth / num_lanes / 2; ( / 2 for CSI-2 DDR)
+ *
+ * The bandwidth corresponds to the SYSCLK frequency generated by the
+ * PLL pre-divider, the PLL multiplier and the SYS divider (see the clock
+ * tree documented here above).
+ *
+ * From the SYSCLK frequency, the MIPI CSI-2 clock tree generates the
+ * pixel clock and the MIPI BIT clock as follows:
+ *
+ * MIPI_BIT_CLK = SYSCLK / MIPI_DIV / 2;
+ * PIXEL_CLK = SYSCLK / PLL_RDVI / BIT_DIVIDER / PCLK_DIV / MIPI_DIV
+ *
+ * with this fixed parameters:
+ * PLL_RDIV	= 2;
+ * BIT_DIVIDER	= 2; (MIPI_BIT_MODE == 8 ? 2 : 2,5);
+ * PCLK_DIV	= 1;
+ *
+ * With these values we have:
+ *
+ * pixel_clock = bandwidth / bpp
+ * 	       = bandwidth / 4 / MIPI_DIV;
+ *
+ * And so we can calculate MIPI_DIV as:
+ * MIPI_DIV = bpp / 4;
+ */
+static int ov5640_set_mipi_pclk(struct ov5640_dev *sensor,
+				unsigned long rate)
 {
-	unsigned long _rate = rate * OV5640_MIPI_DIV;
+	const struct ov5640_mode_info *mode = sensor->current_mode;
+	u8 mipi_div = OV5640_MIPI_DIV;
+	u8 prediv, mult, sysdiv;
+	int ret;
 
-	_rate = ov5640_calc_sys_clk(sensor, _rate, pll_prediv, pll_mult,
-				    sysdiv);
-	*mipi_div = OV5640_MIPI_DIV;
+	/* FIXME:
+	 * Practical experience shows we get a correct frame rate by
+	 * halving the bandwidth rate by 2, to slow down SYSCLK frequency.
+	 * Divide both SYSCLK and MIPI_DIV by two (with OV5640_MIPI_DIV
+	 * currently fixed at value '2', while according to the above
+	 * formula it should have been = bpp / 4 = 4).
+	 *
+	 * So that:
+	 * pixel_clock = bandwidth / 2 / bpp
+	 * 	       = bandwidth / 2 / 4 / MIPI_DIV;
+	 * MIPI_DIV = bpp / 4 / 2;
+	 */
+	rate /= 2;
 
-	return _rate / *mipi_div;
-}
+	/* FIXME:
+	 * High resolution modes (1280x720, 1920x1080) requires an higher
+	 * clock speed. Half the MIPI_DIVIDER value to double the output
+	 * pixel clock and MIPI_CLK speeds.
+	 */
+	if (mode->hact > 1024)
+		mipi_div /= 2;
 
-static int ov5640_set_mipi_pclk(struct ov5640_dev *sensor, unsigned long rate)
-{
-	u8 prediv, mult, sysdiv, mipi_div;
-	int ret;
+	ov5640_calc_sys_clk(sensor, rate, &prediv, &mult, &sysdiv);
 
-	ov5640_calc_mipi_clk(sensor, rate, &prediv, &mult, &sysdiv, &mipi_div);
+	ret = ov5640_mod_reg(sensor, OV5640_REG_SC_PLL_CTRL0,
+			     0x0f, OV5640_PLL_CTRL0_MIPI_MODE_8BIT);
 
 	ret = ov5640_mod_reg(sensor, OV5640_REG_SC_PLL_CTRL1,
-			     0xff, sysdiv << 4 | (mipi_div - 1));
+			     0xff, sysdiv << 4 | mipi_div);
 	if (ret)
 		return ret;
 
@@ -907,7 +960,13 @@ static int ov5640_set_mipi_pclk(struct ov5640_dev *sensor, unsigned long rate)
 	if (ret)
 		return ret;
 
-	return ov5640_mod_reg(sensor, OV5640_REG_SC_PLL_CTRL3, 0xf, prediv);
+	ret = ov5640_mod_reg(sensor, OV5640_REG_SC_PLL_CTRL3,
+			     0x1f, OV5640_PLL_CTRL3_PLL_ROOT_DIV_2 | prediv);
+	if (ret)
+		return ret;
+
+	return ov5640_mod_reg(sensor, OV5640_REG_SYS_ROOT_DIVIDER,
+			      0x30, OV5640_PLL_SYS_ROOT_DIVIDER_BYPASS);
 }
 
 static unsigned long ov5640_calc_pclk(struct ov5640_dev *sensor,
-- 
2.7.4

Re: [PATCH 1/2] media: ov5640: Add check for PLL1 output max frequency

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2496 bytes --]

On Wed, Oct 17, 2018 at 09:37:17PM +0200, Jacopo Mondi wrote:
> Check that the PLL1 output frequency does not exceed the maximum allowed 1GHz
> frequency.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/i2c/ov5640.c | 23 +++++++++++++++++++----
>  1 file changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index e098435..1f2e72d 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -770,7 +770,7 @@ static int ov5640_mod_reg(struct ov5640_dev *sensor, u16 reg,
>   * always set to either 1 or 2 in the vendor kernels.
>   */
>  #define OV5640_SYSDIV_MIN	1
> -#define OV5640_SYSDIV_MAX	2
> +#define OV5640_SYSDIV_MAX	16
>  
>  /*
>   * This is supposed to be ranging from 1 to 16, but the value is always
> @@ -806,15 +806,20 @@ static int ov5640_mod_reg(struct ov5640_dev *sensor, u16 reg,
>   * This is supposed to be ranging from 1 to 8, but the value is always
>   * set to 1 in the vendor kernels.
>   */
> -#define OV5640_PCLK_ROOT_DIV	1
> +#define OV5640_PCLK_ROOT_DIV			1
> +#define OV5640_PLL_SYS_ROOT_DIVIDER_BYPASS	0x00
>  
>  static unsigned long ov5640_compute_sys_clk(struct ov5640_dev *sensor,
>  					    u8 pll_prediv, u8 pll_mult,
>  					    u8 sysdiv)
>  {
> -	unsigned long rate = clk_get_rate(sensor->xclk);
> +	unsigned long sysclk = sensor->xclk_freq / pll_prediv * pll_mult;
>  
> -	return rate / pll_prediv * pll_mult / sysdiv;
> +	/* PLL1 output cannot exceed 1GHz. */
> +	if (sysclk / 1000000 > 1000)
> +		return 0;
> +
> +	return sysclk / sysdiv;
>  }
>  
>  static unsigned long ov5640_calc_sys_clk(struct ov5640_dev *sensor,
> @@ -844,6 +849,16 @@ static unsigned long ov5640_calc_sys_clk(struct ov5640_dev *sensor,
>  			_rate = ov5640_compute_sys_clk(sensor,
>  						       OV5640_PLL_PREDIV,
>  						       _pll_mult, _sysdiv);
> +
> +			/*
> +			 * We have reached the maximum allowed PLL1 output,
> +			 * increase sysdiv.
> +			 */
> +			if (rate == 0) {
> +				_pll_mult = OV5640_PLL_MULT_MAX + 1;
> +				continue;
> +			}
> +

Both your patches look sane to me. However, I guess here you're
setting _pll_mult at this value so that you won't reach the for
condition on the next iteration?

Wouldn't it be cleaner to just use a break statement here?

Thanks!
Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] media: ov5640: Add check for PLL1 output max frequency

[jacopo mondi]

[-- Attachment #1: Type: text/plain, Size: 2809 bytes --]

Hi Maxime,

On Thu, Oct 18, 2018 at 11:15:50AM +0200, Maxime Ripard wrote:
> On Wed, Oct 17, 2018 at 09:37:17PM +0200, Jacopo Mondi wrote:
> > Check that the PLL1 output frequency does not exceed the maximum allowed 1GHz
> > frequency.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  drivers/media/i2c/ov5640.c | 23 +++++++++++++++++++----
> >  1 file changed, 19 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> > index e098435..1f2e72d 100644
> > --- a/drivers/media/i2c/ov5640.c
> > +++ b/drivers/media/i2c/ov5640.c
> > @@ -770,7 +770,7 @@ static int ov5640_mod_reg(struct ov5640_dev *sensor, u16 reg,
> >   * always set to either 1 or 2 in the vendor kernels.
> >   */
> >  #define OV5640_SYSDIV_MIN	1
> > -#define OV5640_SYSDIV_MAX	2
> > +#define OV5640_SYSDIV_MAX	16
> >
> >  /*
> >   * This is supposed to be ranging from 1 to 16, but the value is always
> > @@ -806,15 +806,20 @@ static int ov5640_mod_reg(struct ov5640_dev *sensor, u16 reg,
> >   * This is supposed to be ranging from 1 to 8, but the value is always
> >   * set to 1 in the vendor kernels.
> >   */
> > -#define OV5640_PCLK_ROOT_DIV	1
> > +#define OV5640_PCLK_ROOT_DIV			1
> > +#define OV5640_PLL_SYS_ROOT_DIVIDER_BYPASS	0x00
> >
> >  static unsigned long ov5640_compute_sys_clk(struct ov5640_dev *sensor,
> >  					    u8 pll_prediv, u8 pll_mult,
> >  					    u8 sysdiv)
> >  {
> > -	unsigned long rate = clk_get_rate(sensor->xclk);
> > +	unsigned long sysclk = sensor->xclk_freq / pll_prediv * pll_mult;
> >
> > -	return rate / pll_prediv * pll_mult / sysdiv;
> > +	/* PLL1 output cannot exceed 1GHz. */
> > +	if (sysclk / 1000000 > 1000)
> > +		return 0;
> > +
> > +	return sysclk / sysdiv;
> >  }
> >
> >  static unsigned long ov5640_calc_sys_clk(struct ov5640_dev *sensor,
> > @@ -844,6 +849,16 @@ static unsigned long ov5640_calc_sys_clk(struct ov5640_dev *sensor,
> >  			_rate = ov5640_compute_sys_clk(sensor,
> >  						       OV5640_PLL_PREDIV,
> >  						       _pll_mult, _sysdiv);
> > +
> > +			/*
> > +			 * We have reached the maximum allowed PLL1 output,
> > +			 * increase sysdiv.
> > +			 */
> > +			if (rate == 0) {
> > +				_pll_mult = OV5640_PLL_MULT_MAX + 1;
> > +				continue;
> > +			}
> > +
>
> Both your patches look sane to me. However, I guess here you're
> setting _pll_mult at this value so that you won't reach the for
> condition on the next iteration?
>
> Wouldn't it be cleaner to just use a break statement here?

Yes, it's much cleaner indeed. Not sure why I thought this was a good
idea tbh.

Would you like me to send a v2, or can you take care of this when
re-sending v5?

Thanks
   j

>
> Thanks!
> Maxime
>
> --
> Maxime Ripard, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/2] media: ov5640: Add check for PLL1 output max frequency

[Maxime Ripard]

Hi,

On Thu, Oct 18, 2018 at 03:35:25PM +0200, jacopo mondi wrote:
> Hi Maxime,
> 
> On Thu, Oct 18, 2018 at 11:15:50AM +0200, Maxime Ripard wrote:
> > On Wed, Oct 17, 2018 at 09:37:17PM +0200, Jacopo Mondi wrote:
> > > Check that the PLL1 output frequency does not exceed the maximum allowed 1GHz
> > > frequency.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > ---
> > >  drivers/media/i2c/ov5640.c | 23 +++++++++++++++++++----
> > >  1 file changed, 19 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> > > index e098435..1f2e72d 100644
> > > --- a/drivers/media/i2c/ov5640.c
> > > +++ b/drivers/media/i2c/ov5640.c
> > > @@ -770,7 +770,7 @@ static int ov5640_mod_reg(struct ov5640_dev *sensor, u16 reg,
> > >   * always set to either 1 or 2 in the vendor kernels.
> > >   */
> > >  #define OV5640_SYSDIV_MIN	1
> > > -#define OV5640_SYSDIV_MAX	2
> > > +#define OV5640_SYSDIV_MAX	16
> > >
> > >  /*
> > >   * This is supposed to be ranging from 1 to 16, but the value is always
> > > @@ -806,15 +806,20 @@ static int ov5640_mod_reg(struct ov5640_dev *sensor, u16 reg,
> > >   * This is supposed to be ranging from 1 to 8, but the value is always
> > >   * set to 1 in the vendor kernels.
> > >   */
> > > -#define OV5640_PCLK_ROOT_DIV	1
> > > +#define OV5640_PCLK_ROOT_DIV			1
> > > +#define OV5640_PLL_SYS_ROOT_DIVIDER_BYPASS	0x00
> > >
> > >  static unsigned long ov5640_compute_sys_clk(struct ov5640_dev *sensor,
> > >  					    u8 pll_prediv, u8 pll_mult,
> > >  					    u8 sysdiv)
> > >  {
> > > -	unsigned long rate = clk_get_rate(sensor->xclk);
> > > +	unsigned long sysclk = sensor->xclk_freq / pll_prediv * pll_mult;
> > >
> > > -	return rate / pll_prediv * pll_mult / sysdiv;
> > > +	/* PLL1 output cannot exceed 1GHz. */
> > > +	if (sysclk / 1000000 > 1000)
> > > +		return 0;
> > > +
> > > +	return sysclk / sysdiv;
> > >  }
> > >
> > >  static unsigned long ov5640_calc_sys_clk(struct ov5640_dev *sensor,
> > > @@ -844,6 +849,16 @@ static unsigned long ov5640_calc_sys_clk(struct ov5640_dev *sensor,
> > >  			_rate = ov5640_compute_sys_clk(sensor,
> > >  						       OV5640_PLL_PREDIV,
> > >  						       _pll_mult, _sysdiv);
> > > +
> > > +			/*
> > > +			 * We have reached the maximum allowed PLL1 output,
> > > +			 * increase sysdiv.
> > > +			 */
> > > +			if (rate == 0) {
> > > +				_pll_mult = OV5640_PLL_MULT_MAX + 1;
> > > +				continue;
> > > +			}
> > > +
> >
> > Both your patches look sane to me. However, I guess here you're
> > setting _pll_mult at this value so that you won't reach the for
> > condition on the next iteration?
> >
> > Wouldn't it be cleaner to just use a break statement here?
> 
> Yes, it's much cleaner indeed. Not sure why I thought this was a good
> idea tbh.
> 
> Would you like me to send a v2, or can you take care of this when
> re-sending v5?

I'll squash it in the v5, thanks!

Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com