[PATCH v5 00/11] media: ov5640: Misc cleanup and improvements

linux-media.vger.kernel.org archive mirror
 help / color / mirror / Atom feed

* [PATCH v5 00/11] media: ov5640: Misc cleanup and improvements
@ 2018-11-13 13:03 Maxime Ripard
  2018-11-13 13:03 ` [PATCH v5 01/11] media: ov5640: Adjust the clock based on the expected rate Maxime Ripard
                   ` (10 more replies)
  0 siblings, 11 replies; 17+ messages in thread
From: Maxime Ripard @ 2018-11-13 13:03 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: Laurent Pinchart, linux-media, Thomas Petazzoni,
	Mylene Josserand, Hans Verkuil, Sakari Ailus, Hugues Fruchet,
	Loic Poulain, Samuel Bobrowicz, Steve Longerbeam, Daniel Mack,
	Jacopo Mondi, Maxime Ripard

Hi,

Here is a "small" series that mostly cleans up the ov5640 driver code,
slowly getting rid of the big data array for more understandable code
(hopefully).

The biggest addition would be the clock rate computation at runtime,
instead of relying on those arrays to setup the clock tree
properly. As a side effect, it fixes the framerate that was off by
around 10% on the smaller resolutions, and we now support 60fps.

This also introduces a bunch of new features.

Let me know what you think,
Maxime

Changes from v4:
  - Squashed Jacopo patches fixing the MIPI-CSI case
  - Prefer clock rates superior to the ideal clock rate, even if it
    means having a less precise one.
  - Fix the JPEG case according to Hugues suggestions
  - Rebased on 4.20

Changes from v3:
  - Rebased on current Sakari tree
  - Fixed an error when changing only the framerate

Changes from v2:
  - Rebased on latest Sakari PR
  - Fixed the issues reported by Hugues: improper FPS returned for
    formats, improper rounding of the FPS, some with his suggestions,
    some by simplifying the logic.
  - Expanded the clock tree comments based on the feedback from Samuel
    Bobrowicz and Loic Poulain
  - Merged some of the changes made by Samuel Bobrowicz to fix the
    MIPI rate computation, fix the call sites of the
    ov5640_set_timings function, the auto-exposure calculation call,
    etc.
  - Split the patches into smaller ones in order to make it more
    readable (hopefully)

Changes from v1:
  - Integrated Hugues' suggestions to fix v4l2-compliance
  - Fixed the bus width with JPEG
  - Dropped the clock rate calculation loops for something simpler as
    suggested by Sakari
  - Cache the exposure value instead of using the control value
  - Rebased on top of 4.17

Maxime Ripard (11):
  media: ov5640: Adjust the clock based on the expected rate
  media: ov5640: Remove the clocks registers initialization
  media: ov5640: Remove redundant defines
  media: ov5640: Remove redundant register setup
  media: ov5640: Compute the clock rate at runtime
  media: ov5640: Remove pixel clock rates
  media: ov5640: Enhance FPS handling
  media: ov5640: Make the return rate type more explicit
  media: ov5640: Make the FPS clamping / rounding more extendable
  media: ov5640: Add 60 fps support
  media: ov5640: Remove duplicate auto-exposure setup

 drivers/media/i2c/ov5640.c | 748 ++++++++++++++++++++++---------------
 1 file changed, 445 insertions(+), 303 deletions(-)

-- 
2.19.1

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v5 01/11] media: ov5640: Adjust the clock based on the expected rate
  2018-11-13 13:03 [PATCH v5 00/11] media: ov5640: Misc cleanup and improvements Maxime Ripard
@ 2018-11-13 13:03 ` Maxime Ripard
  2018-11-14 13:20   ` Adam Ford
  2018-11-14 19:48   ` jacopo mondi
  2018-11-13 13:03 ` [PATCH v5 02/11] media: ov5640: Remove the clocks registers initialization Maxime Ripard
                   ` (9 subsequent siblings)
  10 siblings, 2 replies; 17+ messages in thread
From: Maxime Ripard @ 2018-11-13 13:03 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: Laurent Pinchart, linux-media, Thomas Petazzoni,
	Mylene Josserand, Hans Verkuil, Sakari Ailus, Hugues Fruchet,
	Loic Poulain, Samuel Bobrowicz, Steve Longerbeam, Daniel Mack,
	Jacopo Mondi, Maxime Ripard

The clock structure for the PCLK is quite obscure in the documentation, and
was hardcoded through the bytes array of each and every mode.

This is troublesome, since we cannot adjust it at runtime based on other
parameters (such as the number of bytes per pixel), and we can't support
either framerates that have not been used by the various vendors, since we
don't have the needed initialization sequence.

We can however understand how the clock tree works, and then implement some
functions to derive the various parameters from a given rate. And now that
those parameters are calculated at runtime, we can remove them from the
initialization sequence.

The modes also gained a new parameter which is the clock that they are
running at, from the register writes they were doing, so for now the switch
to the new algorithm should be transparent.

Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
 drivers/media/i2c/ov5640.c | 366 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 365 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index eaefdb58653b..8476f85bb8e7 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -175,6 +175,7 @@ struct ov5640_mode_info {
 	u32 htot;
 	u32 vact;
 	u32 vtot;
+	u32 pixel_clock;
 	const struct reg_value *reg_data;
 	u32 reg_data_size;
 };
@@ -700,6 +701,7 @@ static const struct reg_value ov5640_setting_15fps_QSXGA_2592_1944[] = {
 /* power-on sensor init reg table */
 static const struct ov5640_mode_info ov5640_mode_init_data = {
 	0, SUBSAMPLING, 640, 1896, 480, 984,
+	56000000,
 	ov5640_init_setting_30fps_VGA,
 	ARRAY_SIZE(ov5640_init_setting_30fps_VGA),
 };
@@ -709,74 +711,91 @@ ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES] = {
 	{
 		{OV5640_MODE_QCIF_176_144, SUBSAMPLING,
 		 176, 1896, 144, 984,
+		 28000000,
 		 ov5640_setting_15fps_QCIF_176_144,
 		 ARRAY_SIZE(ov5640_setting_15fps_QCIF_176_144)},
 		{OV5640_MODE_QVGA_320_240, SUBSAMPLING,
 		 320, 1896, 240, 984,
+		 28000000,
 		 ov5640_setting_15fps_QVGA_320_240,
 		 ARRAY_SIZE(ov5640_setting_15fps_QVGA_320_240)},
 		{OV5640_MODE_VGA_640_480, SUBSAMPLING,
 		 640, 1896, 480, 1080,
+		 28000000,
 		 ov5640_setting_15fps_VGA_640_480,
 		 ARRAY_SIZE(ov5640_setting_15fps_VGA_640_480)},
 		{OV5640_MODE_NTSC_720_480, SUBSAMPLING,
 		 720, 1896, 480, 984,
+		 28000000,
 		 ov5640_setting_15fps_NTSC_720_480,
 		 ARRAY_SIZE(ov5640_setting_15fps_NTSC_720_480)},
 		{OV5640_MODE_PAL_720_576, SUBSAMPLING,
 		 720, 1896, 576, 984,
+		 28000000,
 		 ov5640_setting_15fps_PAL_720_576,
 		 ARRAY_SIZE(ov5640_setting_15fps_PAL_720_576)},
 		{OV5640_MODE_XGA_1024_768, SUBSAMPLING,
 		 1024, 1896, 768, 1080,
+		 28000000,
 		 ov5640_setting_15fps_XGA_1024_768,
 		 ARRAY_SIZE(ov5640_setting_15fps_XGA_1024_768)},
 		{OV5640_MODE_720P_1280_720, SUBSAMPLING,
 		 1280, 1892, 720, 740,
+		 21000000,
 		 ov5640_setting_15fps_720P_1280_720,
 		 ARRAY_SIZE(ov5640_setting_15fps_720P_1280_720)},
 		{OV5640_MODE_1080P_1920_1080, SCALING,
 		 1920, 2500, 1080, 1120,
+		 42000000,
 		 ov5640_setting_15fps_1080P_1920_1080,
 		 ARRAY_SIZE(ov5640_setting_15fps_1080P_1920_1080)},
 		{OV5640_MODE_QSXGA_2592_1944, SCALING,
 		 2592, 2844, 1944, 1968,
+		 84000000,
 		 ov5640_setting_15fps_QSXGA_2592_1944,
 		 ARRAY_SIZE(ov5640_setting_15fps_QSXGA_2592_1944)},
 	}, {
 		{OV5640_MODE_QCIF_176_144, SUBSAMPLING,
 		 176, 1896, 144, 984,
+		 56000000,
 		 ov5640_setting_30fps_QCIF_176_144,
 		 ARRAY_SIZE(ov5640_setting_30fps_QCIF_176_144)},
 		{OV5640_MODE_QVGA_320_240, SUBSAMPLING,
 		 320, 1896, 240, 984,
+		 56000000,
 		 ov5640_setting_30fps_QVGA_320_240,
 		 ARRAY_SIZE(ov5640_setting_30fps_QVGA_320_240)},
 		{OV5640_MODE_VGA_640_480, SUBSAMPLING,
 		 640, 1896, 480, 1080,
+		 56000000,
 		 ov5640_setting_30fps_VGA_640_480,
 		 ARRAY_SIZE(ov5640_setting_30fps_VGA_640_480)},
 		{OV5640_MODE_NTSC_720_480, SUBSAMPLING,
 		 720, 1896, 480, 984,
+		 56000000,
 		 ov5640_setting_30fps_NTSC_720_480,
 		 ARRAY_SIZE(ov5640_setting_30fps_NTSC_720_480)},
 		{OV5640_MODE_PAL_720_576, SUBSAMPLING,
 		 720, 1896, 576, 984,
+		 56000000,
 		 ov5640_setting_30fps_PAL_720_576,
 		 ARRAY_SIZE(ov5640_setting_30fps_PAL_720_576)},
 		{OV5640_MODE_XGA_1024_768, SUBSAMPLING,
 		 1024, 1896, 768, 1080,
+		 56000000,
 		 ov5640_setting_30fps_XGA_1024_768,
 		 ARRAY_SIZE(ov5640_setting_30fps_XGA_1024_768)},
 		{OV5640_MODE_720P_1280_720, SUBSAMPLING,
 		 1280, 1892, 720, 740,
+		 42000000,
 		 ov5640_setting_30fps_720P_1280_720,
 		 ARRAY_SIZE(ov5640_setting_30fps_720P_1280_720)},
 		{OV5640_MODE_1080P_1920_1080, SCALING,
 		 1920, 2500, 1080, 1120,
+		 84000000,
 		 ov5640_setting_30fps_1080P_1920_1080,
 		 ARRAY_SIZE(ov5640_setting_30fps_1080P_1920_1080)},
-		{OV5640_MODE_QSXGA_2592_1944, -1, 0, 0, 0, 0, NULL, 0},
+		{OV5640_MODE_QSXGA_2592_1944, -1, 0, 0, 0, 0, 0, NULL, 0},
 	},
 };
 
@@ -909,6 +928,334 @@ static int ov5640_mod_reg(struct ov5640_dev *sensor, u16 reg,
 	return ov5640_write_reg(sensor, reg, val);
 }
 
+/*
+ * After trying the various combinations, reading various
+ * documentations spreaded around the net, and from the various
+ * feedback, the clock tree is probably as follows:
+ *
+ *   +--------------+
+ *   |  Ext. Clock  |
+ *   +-+------------+
+ *     |  +----------+
+ *     +->|   PLL1   | - reg 0x3036, for the multiplier
+ *        +-+--------+ - reg 0x3037, bits 0-3 for the pre-divider
+ *          |  +--------------+
+ *          +->| System Clock |  - reg 0x3035, bits 4-7
+ *             +-+------------+
+ *               |  +--------------+
+ *               +->| MIPI Divider | - reg 0x3035, bits 0-3
+ *               |  +-+------------+
+ *               |    +----------------> MIPI SCLK
+ *               |    +  +-----+
+ *               |    +->| / 2 |-------> MIPI BIT CLK
+ *               |       +-----+
+ *               |  +--------------+
+ *               +->| PLL Root Div | - reg 0x3037, bit 4
+ *                  +-+------------+
+ *                    |  +---------+
+ *                    +->| Bit Div | - reg 0x3035, bits 0-3
+ *                       +-+-------+
+ *                         |  +-------------+
+ *                         +->| SCLK Div    | - reg 0x3108, bits 0-1
+ *                         |  +-+-----------+
+ *                         |    +---------------> SCLK
+ *                         |  +-------------+
+ *                         +->| SCLK 2X Div | - reg 0x3108, bits 2-3
+ *                         |  +-+-----------+
+ *                         |    +---------------> SCLK 2X
+ *                         |  +-------------+
+ *                         +->| PCLK Div    | - reg 0x3108, bits 4-5
+ *                            +-+-----------+
+ *                              +---------------> PCLK
+ *
+ * This is deviating from the datasheet at least for the register
+ * 0x3108, since it's said here that the PCLK would be clocked from
+ * the PLL.
+ *
+ * There seems to be also (unverified) constraints:
+ *  - the PLL pre-divider output rate should be in the 4-27MHz range
+ *  - the PLL multiplier output rate should be in the 500-1000MHz range
+ *  - PCLK >= SCLK * 2 in YUV, >= SCLK in Raw or JPEG
+ *  - MIPI SCLK = (bpp / lanes) / PCLK
+ *
+ * In the two latter cases, these constraints are met since our
+ * factors are hardcoded. If we were to change that, we would need to
+ * take this into account. The only varying parts are the PLL
+ * multiplier and the system clock divider, which are shared between
+ * all these clocks so won't cause any issue.
+ */
+
+/*
+ * This is supposed to be ranging from 1 to 8, but the value is always
+ * set to 3 in the vendor kernels.
+ */
+#define OV5640_PLL_PREDIV	3
+
+#define OV5640_PLL_MULT_MIN	4
+#define OV5640_PLL_MULT_MAX	252
+
+/*
+ * This is supposed to be ranging from 1 to 16, but the value is
+ * always set to either 1 or 2 in the vendor kernels.
+ */
+#define OV5640_SYSDIV_MIN	1
+#define OV5640_SYSDIV_MAX	16
+
+/*
+ * This is supposed to be ranging from 1 to 16, but the value is always
+ * set to 2 in the vendor kernels.
+ */
+#define OV5640_MIPI_DIV		2
+
+/*
+ * 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_CTRL3_PLL_ROOT_DIV_2		BIT(4)
+
+/*
+ * We only supports 8-bit formats at the moment
+ */
+#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
+ * set to 2 in the vendor kernels.
+ */
+#define OV5640_SCLK_ROOT_DIV	2
+
+/*
+ * This is hardcoded so that the consistency is maintained between SCLK and
+ * SCLK 2x.
+ */
+#define OV5640_SCLK2X_ROOT_DIV (OV5640_SCLK_ROOT_DIV / 2)
+
+/*
+ * 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_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 sysclk = sensor->xclk_freq / pll_prediv * pll_mult;
+
+	/* 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,
+					 unsigned long rate,
+					 u8 *pll_prediv, u8 *pll_mult,
+					 u8 *sysdiv)
+{
+	unsigned long best = ~0;
+	u8 best_sysdiv = 1, best_mult = 1;
+	u8 _sysdiv, _pll_mult;
+
+	for (_sysdiv = OV5640_SYSDIV_MIN;
+	     _sysdiv <= OV5640_SYSDIV_MAX;
+	     _sysdiv++) {
+		for (_pll_mult = OV5640_PLL_MULT_MIN;
+		     _pll_mult <= OV5640_PLL_MULT_MAX;
+		     _pll_mult++) {
+			unsigned long _rate;
+
+			/*
+			 * The PLL multiplier cannot be odd if above
+			 * 127.
+			 */
+			if (_pll_mult > 127 && (_pll_mult % 2))
+				continue;
+
+			_rate = ov5640_compute_sys_clk(sensor,
+						       OV5640_PLL_PREDIV,
+						       _pll_mult, _sysdiv);
+
+			/*
+			 * We have reached the maximum allowed PLL1 output,
+			 * increase sysdiv.
+			 */
+			if (!rate)
+				break;
+
+			/*
+			 * Prefer rates above the expected clock rate than
+			 * below, even if that means being less precise.
+			 */
+			if (_rate < rate)
+				continue;
+
+			if (abs(rate - _rate) < abs(rate - best)) {
+				best = _rate;
+				best_sysdiv = _sysdiv;
+				best_mult = _pll_mult;
+			}
+
+			if (_rate == rate)
+				goto out;
+		}
+	}
+
+out:
+	*sysdiv = best_sysdiv;
+	*pll_prediv = OV5640_PLL_PREDIV;
+	*pll_mult = best_mult;
+
+	return best;
+}
+
+/*
+ * 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)
+{
+	const struct ov5640_mode_info *mode = sensor->current_mode;
+	u8 mipi_div = OV5640_MIPI_DIV;
+	u8 prediv, mult, sysdiv;
+	int ret;
+
+	/* 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;
+
+	/* 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;
+
+	ov5640_calc_sys_clk(sensor, rate, &prediv, &mult, &sysdiv);
+
+	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);
+	if (ret)
+		return ret;
+
+	ret = ov5640_mod_reg(sensor, OV5640_REG_SC_PLL_CTRL2, 0xff, mult);
+	if (ret)
+		return ret;
+
+	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,
+				      unsigned long rate,
+				      u8 *pll_prediv, u8 *pll_mult, u8 *sysdiv,
+				      u8 *pll_rdiv, u8 *bit_div, u8 *pclk_div)
+{
+	unsigned long _rate = rate * OV5640_PLL_ROOT_DIV * OV5640_BIT_DIV *
+				OV5640_PCLK_ROOT_DIV;
+
+	_rate = ov5640_calc_sys_clk(sensor, _rate, pll_prediv, pll_mult,
+				    sysdiv);
+	*pll_rdiv = OV5640_PLL_ROOT_DIV;
+	*bit_div = OV5640_BIT_DIV;
+	*pclk_div = OV5640_PCLK_ROOT_DIV;
+
+	return _rate / *pll_rdiv / *bit_div / *pclk_div;
+}
+
+static int ov5640_set_dvp_pclk(struct ov5640_dev *sensor, unsigned long rate)
+{
+	u8 prediv, mult, sysdiv, pll_rdiv, bit_div, pclk_div;
+	int ret;
+
+	ov5640_calc_pclk(sensor, rate, &prediv, &mult, &sysdiv, &pll_rdiv,
+			 &bit_div, &pclk_div);
+
+	if (bit_div == 2)
+		bit_div = 8;
+
+	ret = ov5640_mod_reg(sensor, OV5640_REG_SC_PLL_CTRL0,
+			     0x0f, bit_div);
+	if (ret)
+		return ret;
+
+	/*
+	 * We need to set sysdiv according to the clock, and to clear
+	 * the MIPI divider.
+	 */
+	ret = ov5640_mod_reg(sensor, OV5640_REG_SC_PLL_CTRL1,
+			     0xff, sysdiv << 4);
+	if (ret)
+		return ret;
+
+	ret = ov5640_mod_reg(sensor, OV5640_REG_SC_PLL_CTRL2,
+			     0xff, mult);
+	if (ret)
+		return ret;
+
+	ret = ov5640_mod_reg(sensor, OV5640_REG_SC_PLL_CTRL3,
+			     0x1f, prediv | ((pll_rdiv - 1) << 4));
+	if (ret)
+		return ret;
+
+	return ov5640_mod_reg(sensor, OV5640_REG_SYS_ROOT_DIVIDER, 0x30,
+			      (ilog2(pclk_div) << 4));
+}
+
 /* download ov5640 settings to sensor through i2c */
 static int ov5640_set_timings(struct ov5640_dev *sensor,
 			      const struct ov5640_mode_info *mode)
@@ -1637,6 +1984,7 @@ static int ov5640_set_mode(struct ov5640_dev *sensor)
 	enum ov5640_downsize_mode dn_mode, orig_dn_mode;
 	bool auto_gain = sensor->ctrls.auto_gain->val == 1;
 	bool auto_exp =  sensor->ctrls.auto_exp->val == V4L2_EXPOSURE_AUTO;
+	unsigned long rate;
 	int ret;
 
 	dn_mode = mode->dn_mode;
@@ -1655,6 +2003,22 @@ static int ov5640_set_mode(struct ov5640_dev *sensor)
 			goto restore_auto_gain;
 	}
 
+	/*
+	 * All the formats we support have 16 bits per pixel, seems to require
+	 * the same rate than YUV, so we can just use 16 bpp all the time.
+	 */
+	rate = mode->pixel_clock * 16;
+	if (sensor->ep.bus_type == V4L2_MBUS_CSI2_DPHY) {
+		rate = rate / sensor->ep.bus.mipi_csi2.num_data_lanes;
+		ret = ov5640_set_mipi_pclk(sensor, rate);
+	} else {
+		rate = rate / sensor->ep.bus.parallel.bus_width;
+		ret = ov5640_set_dvp_pclk(sensor, rate);
+	}
+
+	if (ret < 0)
+		return 0;
+
 	if ((dn_mode == SUBSAMPLING && orig_dn_mode == SCALING) ||
 	    (dn_mode == SCALING && orig_dn_mode == SUBSAMPLING)) {
 		/*
-- 
2.19.1

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v5 02/11] media: ov5640: Remove the clocks registers initialization
  2018-11-13 13:03 [PATCH v5 00/11] media: ov5640: Misc cleanup and improvements Maxime Ripard
  2018-11-13 13:03 ` [PATCH v5 01/11] media: ov5640: Adjust the clock based on the expected rate Maxime Ripard
@ 2018-11-13 13:03 ` Maxime Ripard
  2018-11-13 13:03 ` [PATCH v5 03/11] media: ov5640: Remove redundant defines Maxime Ripard
                   ` (8 subsequent siblings)
  10 siblings, 0 replies; 17+ messages in thread
From: Maxime Ripard @ 2018-11-13 13:03 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: Laurent Pinchart, linux-media, Thomas Petazzoni,
	Mylene Josserand, Hans Verkuil, Sakari Ailus, Hugues Fruchet,
	Loic Poulain, Samuel Bobrowicz, Steve Longerbeam, Daniel Mack,
	Jacopo Mondi, Maxime Ripard

Part of the hardcoded initialization sequence is to set up the proper clock
dividers. However, this is now done dynamically through proper code and as
such, the static one is now redundant.

Let's remove it.

Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
 drivers/media/i2c/ov5640.c | 46 ++++++++++++++++++--------------------
 1 file changed, 22 insertions(+), 24 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 8476f85bb8e7..584e01ea765b 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -262,8 +262,7 @@ static inline struct v4l2_subdev *ctrl_to_sd(struct v4l2_ctrl *ctrl)
 static const struct reg_value ov5640_init_setting_30fps_VGA[] = {
 	{0x3103, 0x11, 0, 0}, {0x3008, 0x82, 0, 5}, {0x3008, 0x42, 0, 0},
 	{0x3103, 0x03, 0, 0}, {0x3017, 0x00, 0, 0}, {0x3018, 0x00, 0, 0},
-	{0x3034, 0x18, 0, 0}, {0x3035, 0x14, 0, 0}, {0x3036, 0x38, 0, 0},
-	{0x3037, 0x13, 0, 0}, {0x3630, 0x36, 0, 0},
+	{0x3630, 0x36, 0, 0},
 	{0x3631, 0x0e, 0, 0}, {0x3632, 0xe2, 0, 0}, {0x3633, 0x12, 0, 0},
 	{0x3621, 0xe0, 0, 0}, {0x3704, 0xa0, 0, 0}, {0x3703, 0x5a, 0, 0},
 	{0x3715, 0x78, 0, 0}, {0x3717, 0x01, 0, 0}, {0x370b, 0x60, 0, 0},
@@ -346,7 +345,7 @@ static const struct reg_value ov5640_init_setting_30fps_VGA[] = {
 };
 
 static const struct reg_value ov5640_setting_30fps_VGA_640_480[] = {
-	{0x3035, 0x14, 0, 0}, {0x3036, 0x38, 0, 0}, {0x3c07, 0x08, 0, 0},
+	{0x3c07, 0x08, 0, 0},
 	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
 	{0x3814, 0x31, 0, 0},
 	{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
@@ -365,7 +364,7 @@ static const struct reg_value ov5640_setting_30fps_VGA_640_480[] = {
 };
 
 static const struct reg_value ov5640_setting_15fps_VGA_640_480[] = {
-	{0x3035, 0x22, 0, 0}, {0x3036, 0x38, 0, 0}, {0x3c07, 0x08, 0, 0},
+	{0x3c07, 0x08, 0, 0},
 	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
 	{0x3814, 0x31, 0, 0},
 	{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
@@ -384,7 +383,7 @@ static const struct reg_value ov5640_setting_15fps_VGA_640_480[] = {
 };
 
 static const struct reg_value ov5640_setting_30fps_XGA_1024_768[] = {
-	{0x3035, 0x14, 0, 0}, {0x3036, 0x38, 0, 0}, {0x3c07, 0x08, 0, 0},
+	{0x3c07, 0x08, 0, 0},
 	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
 	{0x3814, 0x31, 0, 0},
 	{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
@@ -400,11 +399,10 @@ static const struct reg_value ov5640_setting_30fps_XGA_1024_768[] = {
 	{0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0}, {0x4713, 0x03, 0, 0},
 	{0x4407, 0x04, 0, 0}, {0x460b, 0x35, 0, 0}, {0x460c, 0x22, 0, 0},
 	{0x3824, 0x02, 0, 0}, {0x5001, 0xa3, 0, 0}, {0x3503, 0x00, 0, 0},
-	{0x3035, 0x12, 0, 0},
 };
 
 static const struct reg_value ov5640_setting_15fps_XGA_1024_768[] = {
-	{0x3035, 0x22, 0, 0}, {0x3036, 0x38, 0, 0}, {0x3c07, 0x08, 0, 0},
+	{0x3c07, 0x08, 0, 0},
 	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
 	{0x3814, 0x31, 0, 0},
 	{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
@@ -423,7 +421,7 @@ static const struct reg_value ov5640_setting_15fps_XGA_1024_768[] = {
 };
 
 static const struct reg_value ov5640_setting_30fps_QVGA_320_240[] = {
-	{0x3035, 0x14, 0, 0}, {0x3036, 0x38, 0, 0}, {0x3c07, 0x08, 0, 0},
+	{0x3c07, 0x08, 0, 0},
 	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
 	{0x3814, 0x31, 0, 0},
 	{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
@@ -442,7 +440,7 @@ static const struct reg_value ov5640_setting_30fps_QVGA_320_240[] = {
 };
 
 static const struct reg_value ov5640_setting_15fps_QVGA_320_240[] = {
-	{0x3035, 0x22, 0, 0}, {0x3036, 0x38, 0, 0}, {0x3c07, 0x08, 0, 0},
+	{0x3c07, 0x08, 0, 0},
 	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
 	{0x3814, 0x31, 0, 0},
 	{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
@@ -461,7 +459,7 @@ static const struct reg_value ov5640_setting_15fps_QVGA_320_240[] = {
 };
 
 static const struct reg_value ov5640_setting_30fps_QCIF_176_144[] = {
-	{0x3035, 0x14, 0, 0}, {0x3036, 0x38, 0, 0}, {0x3c07, 0x08, 0, 0},
+	{0x3c07, 0x08, 0, 0},
 	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
 	{0x3814, 0x31, 0, 0},
 	{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
@@ -480,7 +478,7 @@ static const struct reg_value ov5640_setting_30fps_QCIF_176_144[] = {
 };
 
 static const struct reg_value ov5640_setting_15fps_QCIF_176_144[] = {
-	{0x3035, 0x22, 0, 0}, {0x3036, 0x38, 0, 0}, {0x3c07, 0x08, 0, 0},
+	{0x3c07, 0x08, 0, 0},
 	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
 	{0x3814, 0x31, 0, 0},
 	{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
@@ -499,7 +497,7 @@ static const struct reg_value ov5640_setting_15fps_QCIF_176_144[] = {
 };
 
 static const struct reg_value ov5640_setting_30fps_NTSC_720_480[] = {
-	{0x3035, 0x12, 0, 0}, {0x3036, 0x38, 0, 0}, {0x3c07, 0x08, 0, 0},
+	{0x3c07, 0x08, 0, 0},
 	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
 	{0x3814, 0x31, 0, 0},
 	{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
@@ -518,7 +516,7 @@ static const struct reg_value ov5640_setting_30fps_NTSC_720_480[] = {
 };
 
 static const struct reg_value ov5640_setting_15fps_NTSC_720_480[] = {
-	{0x3035, 0x22, 0, 0}, {0x3036, 0x38, 0, 0}, {0x3c07, 0x08, 0, 0},
+	{0x3c07, 0x08, 0, 0},
 	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
 	{0x3814, 0x31, 0, 0},
 	{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
@@ -537,7 +535,7 @@ static const struct reg_value ov5640_setting_15fps_NTSC_720_480[] = {
 };
 
 static const struct reg_value ov5640_setting_30fps_PAL_720_576[] = {
-	{0x3035, 0x12, 0, 0}, {0x3036, 0x38, 0, 0}, {0x3c07, 0x08, 0, 0},
+	{0x3c07, 0x08, 0, 0},
 	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
 	{0x3814, 0x31, 0, 0},
 	{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
@@ -556,7 +554,7 @@ static const struct reg_value ov5640_setting_30fps_PAL_720_576[] = {
 };
 
 static const struct reg_value ov5640_setting_15fps_PAL_720_576[] = {
-	{0x3035, 0x22, 0, 0}, {0x3036, 0x38, 0, 0}, {0x3c07, 0x08, 0, 0},
+	{0x3c07, 0x08, 0, 0},
 	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
 	{0x3814, 0x31, 0, 0},
 	{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
@@ -576,7 +574,7 @@ static const struct reg_value ov5640_setting_15fps_PAL_720_576[] = {
 
 static const struct reg_value ov5640_setting_30fps_720P_1280_720[] = {
 	{0x3008, 0x42, 0, 0},
-	{0x3035, 0x21, 0, 0}, {0x3036, 0x54, 0, 0}, {0x3c07, 0x07, 0, 0},
+	{0x3c07, 0x07, 0, 0},
 	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
 	{0x3814, 0x31, 0, 0},
 	{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
@@ -596,7 +594,7 @@ static const struct reg_value ov5640_setting_30fps_720P_1280_720[] = {
 };
 
 static const struct reg_value ov5640_setting_15fps_720P_1280_720[] = {
-	{0x3035, 0x41, 0, 0}, {0x3036, 0x54, 0, 0}, {0x3c07, 0x07, 0, 0},
+	{0x3c07, 0x07, 0, 0},
 	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
 	{0x3814, 0x31, 0, 0},
 	{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
@@ -616,7 +614,7 @@ static const struct reg_value ov5640_setting_15fps_720P_1280_720[] = {
 
 static const struct reg_value ov5640_setting_30fps_1080P_1920_1080[] = {
 	{0x3008, 0x42, 0, 0},
-	{0x3035, 0x21, 0, 0}, {0x3036, 0x54, 0, 0}, {0x3c07, 0x08, 0, 0},
+	{0x3c07, 0x08, 0, 0},
 	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
 	{0x3814, 0x11, 0, 0},
 	{0x3815, 0x11, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
@@ -631,8 +629,8 @@ static const struct reg_value ov5640_setting_30fps_1080P_1920_1080[] = {
 	{0x3a0d, 0x04, 0, 0}, {0x3a14, 0x03, 0, 0}, {0x3a15, 0xd8, 0, 0},
 	{0x4001, 0x02, 0, 0}, {0x4004, 0x06, 0, 0}, {0x4713, 0x03, 0, 0},
 	{0x4407, 0x04, 0, 0}, {0x460b, 0x35, 0, 0}, {0x460c, 0x22, 0, 0},
-	{0x3824, 0x02, 0, 0}, {0x5001, 0x83, 0, 0}, {0x3035, 0x11, 0, 0},
-	{0x3036, 0x54, 0, 0}, {0x3c07, 0x07, 0, 0}, {0x3c08, 0x00, 0, 0},
+	{0x3824, 0x02, 0, 0}, {0x5001, 0x83, 0, 0},
+	{0x3c07, 0x07, 0, 0}, {0x3c08, 0x00, 0, 0},
 	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
 	{0x3800, 0x01, 0, 0}, {0x3801, 0x50, 0, 0}, {0x3802, 0x01, 0, 0},
 	{0x3803, 0xb2, 0, 0}, {0x3804, 0x08, 0, 0}, {0x3805, 0xef, 0, 0},
@@ -649,7 +647,7 @@ static const struct reg_value ov5640_setting_30fps_1080P_1920_1080[] = {
 
 static const struct reg_value ov5640_setting_15fps_1080P_1920_1080[] = {
 	{0x3008, 0x42, 0, 0},
-	{0x3035, 0x21, 0, 0}, {0x3036, 0x54, 0, 0}, {0x3c07, 0x08, 0, 0},
+	{0x3c07, 0x08, 0, 0},
 	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
 	{0x3814, 0x11, 0, 0},
 	{0x3815, 0x11, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
@@ -664,8 +662,8 @@ static const struct reg_value ov5640_setting_15fps_1080P_1920_1080[] = {
 	{0x3a0d, 0x04, 0, 0}, {0x3a14, 0x03, 0, 0}, {0x3a15, 0xd8, 0, 0},
 	{0x4001, 0x02, 0, 0}, {0x4004, 0x06, 0, 0}, {0x4713, 0x03, 0, 0},
 	{0x4407, 0x04, 0, 0}, {0x460b, 0x35, 0, 0}, {0x460c, 0x22, 0, 0},
-	{0x3824, 0x02, 0, 0}, {0x5001, 0x83, 0, 0}, {0x3035, 0x21, 0, 0},
-	{0x3036, 0x54, 0, 1}, {0x3c07, 0x07, 0, 0}, {0x3c08, 0x00, 0, 0},
+	{0x3824, 0x02, 0, 0}, {0x5001, 0x83, 0, 0},
+	{0x3c07, 0x07, 0, 0}, {0x3c08, 0x00, 0, 0},
 	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
 	{0x3800, 0x01, 0, 0}, {0x3801, 0x50, 0, 0}, {0x3802, 0x01, 0, 0},
 	{0x3803, 0xb2, 0, 0}, {0x3804, 0x08, 0, 0}, {0x3805, 0xef, 0, 0},
@@ -680,7 +678,7 @@ static const struct reg_value ov5640_setting_15fps_1080P_1920_1080[] = {
 };
 
 static const struct reg_value ov5640_setting_15fps_QSXGA_2592_1944[] = {
-	{0x3035, 0x21, 0, 0}, {0x3036, 0x54, 0, 0}, {0x3c07, 0x08, 0, 0},
+	{0x3c07, 0x08, 0, 0},
 	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
 	{0x3814, 0x11, 0, 0},
 	{0x3815, 0x11, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
-- 
2.19.1

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v5 03/11] media: ov5640: Remove redundant defines
  2018-11-13 13:03 [PATCH v5 00/11] media: ov5640: Misc cleanup and improvements Maxime Ripard
  2018-11-13 13:03 ` [PATCH v5 01/11] media: ov5640: Adjust the clock based on the expected rate Maxime Ripard
  2018-11-13 13:03 ` [PATCH v5 02/11] media: ov5640: Remove the clocks registers initialization Maxime Ripard
@ 2018-11-13 13:03 ` Maxime Ripard
  2018-11-13 13:03 ` [PATCH v5 04/11] media: ov5640: Remove redundant register setup Maxime Ripard
                   ` (7 subsequent siblings)
  10 siblings, 0 replies; 17+ messages in thread
From: Maxime Ripard @ 2018-11-13 13:03 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: Laurent Pinchart, linux-media, Thomas Petazzoni,
	Mylene Josserand, Hans Verkuil, Sakari Ailus, Hugues Fruchet,
	Loic Poulain, Samuel Bobrowicz, Steve Longerbeam, Daniel Mack,
	Jacopo Mondi, Maxime Ripard

The OV5640_SCLK2X_ROOT_DIVIDER_DEFAULT and OV5640_SCLK_ROOT_DIVIDER_DEFAULT
defines represent exactly the same setup, and are at the same value, than
the more consistent with the rest of the driver OV5640_SCLK2X_ROOT_DIV and
OV5640_SCLK_ROOT_DIV.

Remove them.

Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
 drivers/media/i2c/ov5640.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 584e01ea765b..1b295d07aa15 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -94,9 +94,6 @@
 #define OV5640_REG_SDE_CTRL5		0x5585
 #define OV5640_REG_AVG_READOUT		0x56a1
 
-#define OV5640_SCLK2X_ROOT_DIVIDER_DEFAULT	1
-#define OV5640_SCLK_ROOT_DIVIDER_DEFAULT	2
-
 enum ov5640_mode_id {
 	OV5640_MODE_QCIF_176_144 = 0,
 	OV5640_MODE_QVGA_320_240,
@@ -2086,8 +2083,8 @@ static int ov5640_restore_mode(struct ov5640_dev *sensor)
 	sensor->last_mode = &ov5640_mode_init_data;
 
 	ret = ov5640_mod_reg(sensor, OV5640_REG_SYS_ROOT_DIVIDER, 0x3f,
-			     (ilog2(OV5640_SCLK2X_ROOT_DIVIDER_DEFAULT) << 2) |
-			     ilog2(OV5640_SCLK_ROOT_DIVIDER_DEFAULT));
+			     (ilog2(OV5640_SCLK2X_ROOT_DIV) << 2) |
+			     ilog2(OV5640_SCLK_ROOT_DIV));
 	if (ret)
 		return ret;
 
-- 
2.19.1

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v5 04/11] media: ov5640: Remove redundant register setup
  2018-11-13 13:03 [PATCH v5 00/11] media: ov5640: Misc cleanup and improvements Maxime Ripard
                   ` (2 preceding siblings ...)
  2018-11-13 13:03 ` [PATCH v5 03/11] media: ov5640: Remove redundant defines Maxime Ripard
@ 2018-11-13 13:03 ` Maxime Ripard
  2018-11-13 13:03 ` [PATCH v5 05/11] media: ov5640: Compute the clock rate at runtime Maxime Ripard
                   ` (6 subsequent siblings)
  10 siblings, 0 replies; 17+ messages in thread
From: Maxime Ripard @ 2018-11-13 13:03 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: Laurent Pinchart, linux-media, Thomas Petazzoni,
	Mylene Josserand, Hans Verkuil, Sakari Ailus, Hugues Fruchet,
	Loic Poulain, Samuel Bobrowicz, Steve Longerbeam, Daniel Mack,
	Jacopo Mondi, Maxime Ripard

The MIPI divider is also cleared as part of the clock setup sequence, so we
can remove that code.

Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
 drivers/media/i2c/ov5640.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 1b295d07aa15..25613ecf83c5 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -1403,16 +1403,6 @@ static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on)
 	 */
 
 	if (on) {
-		/*
-		 * reset MIPI PCLK/SERCLK divider
-		 *
-		 * SC PLL CONTRL1 0
-		 * - [3..0]:	MIPI PCLK/SERCLK divider
-		 */
-		ret = ov5640_mod_reg(sensor, OV5640_REG_SC_PLL_CTRL1, 0x0f, 0);
-		if (ret)
-			return ret;
-
 		/*
 		 * configure parallel port control lines polarity
 		 *
-- 
2.19.1

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v5 05/11] media: ov5640: Compute the clock rate at runtime
  2018-11-13 13:03 [PATCH v5 00/11] media: ov5640: Misc cleanup and improvements Maxime Ripard
                   ` (3 preceding siblings ...)
  2018-11-13 13:03 ` [PATCH v5 04/11] media: ov5640: Remove redundant register setup Maxime Ripard
@ 2018-11-13 13:03 ` Maxime Ripard
  2018-11-13 13:03 ` [PATCH v5 06/11] media: ov5640: Remove pixel clock rates Maxime Ripard
                   ` (5 subsequent siblings)
  10 siblings, 0 replies; 17+ messages in thread
From: Maxime Ripard @ 2018-11-13 13:03 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: Laurent Pinchart, linux-media, Thomas Petazzoni,
	Mylene Josserand, Hans Verkuil, Sakari Ailus, Hugues Fruchet,
	Loic Poulain, Samuel Bobrowicz, Steve Longerbeam, Daniel Mack,
	Jacopo Mondi, Maxime Ripard

The clock rate, while hardcoded until now, is actually a function of the
resolution, framerate and bytes per pixel. Now that we have an algorithm to
adjust our clock rate, we can select it dynamically when we change the
mode.

This changes a bit the clock rate being used, with the following effect:

+------+------+------+------+-----+-----------------+----------------+-----------+
| Hact | Vact | Htot | Vtot | FPS | Hardcoded clock | Computed clock | Deviation |
+------+------+------+------+-----+-----------------+----------------+-----------+
|  640 |  480 | 1896 | 1080 |  15 |        56000000 |       61430400 | 8.84 %    |
|  640 |  480 | 1896 | 1080 |  30 |       112000000 |      122860800 | 8.84 %    |
| 1024 |  768 | 1896 | 1080 |  15 |        56000000 |       61430400 | 8.84 %    |
| 1024 |  768 | 1896 | 1080 |  30 |       112000000 |      122860800 | 8.84 %    |
|  320 |  240 | 1896 |  984 |  15 |        56000000 |       55969920 | 0.05 %    |
|  320 |  240 | 1896 |  984 |  30 |       112000000 |      111939840 | 0.05 %    |
|  176 |  144 | 1896 |  984 |  15 |        56000000 |       55969920 | 0.05 %    |
|  176 |  144 | 1896 |  984 |  30 |       112000000 |      111939840 | 0.05 %    |
|  720 |  480 | 1896 |  984 |  15 |        56000000 |       55969920 | 0.05 %    |
|  720 |  480 | 1896 |  984 |  30 |       112000000 |      111939840 | 0.05 %    |
|  720 |  576 | 1896 |  984 |  15 |        56000000 |       55969920 | 0.05 %    |
|  720 |  576 | 1896 |  984 |  30 |       112000000 |      111939840 | 0.05 %    |
| 1280 |  720 | 1892 |  740 |  15 |        42000000 |       42002400 | 0.01 %    |
| 1280 |  720 | 1892 |  740 |  30 |        84000000 |       84004800 | 0.01 %    |
| 1920 | 1080 | 2500 | 1120 |  15 |        84000000 |       84000000 | 0.00 %    |
| 1920 | 1080 | 2500 | 1120 |  30 |       168000000 |      168000000 | 0.00 %    |
| 2592 | 1944 | 2844 | 1944 |  15 |        84000000 |      165862080 | 49.36 %   |
+------+------+------+------+-----+-----------------+----------------+-----------+

Only the 640x480, 1024x768 and 2592x1944 modes are significantly affected
by the new formula.

In this case, 640x480 and 1024x768 are actually fixed by this change.
Indeed, the sensor was sending data at, for example, 27.33fps instead of
30fps. This is -9%, which is roughly what we're seeing in the array.
Testing these modes with the new clock setup actually fix that error, and
data are now sent at around 30fps.

2592x1944, on the other hand, is probably due to the fact that this mode
can only be used using MIPI-CSI2, in a two lane mode, and never really
tested with a DVP bus.

Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
 drivers/media/i2c/ov5640.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 25613ecf83c5..bcfb2b25a450 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -1992,7 +1992,8 @@ static int ov5640_set_mode(struct ov5640_dev *sensor)
 	 * All the formats we support have 16 bits per pixel, seems to require
 	 * the same rate than YUV, so we can just use 16 bpp all the time.
 	 */
-	rate = mode->pixel_clock * 16;
+	rate = mode->vtot * mode->htot * 16;
+	rate *= ov5640_framerates[sensor->current_fr];
 	if (sensor->ep.bus_type == V4L2_MBUS_CSI2_DPHY) {
 		rate = rate / sensor->ep.bus.mipi_csi2.num_data_lanes;
 		ret = ov5640_set_mipi_pclk(sensor, rate);
-- 
2.19.1

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v5 06/11] media: ov5640: Remove pixel clock rates
  2018-11-13 13:03 [PATCH v5 00/11] media: ov5640: Misc cleanup and improvements Maxime Ripard
                   ` (4 preceding siblings ...)
  2018-11-13 13:03 ` [PATCH v5 05/11] media: ov5640: Compute the clock rate at runtime Maxime Ripard
@ 2018-11-13 13:03 ` Maxime Ripard
  2018-11-13 13:03 ` [PATCH v5 07/11] media: ov5640: Enhance FPS handling Maxime Ripard
                   ` (4 subsequent siblings)
  10 siblings, 0 replies; 17+ messages in thread
From: Maxime Ripard @ 2018-11-13 13:03 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: Laurent Pinchart, linux-media, Thomas Petazzoni,
	Mylene Josserand, Hans Verkuil, Sakari Ailus, Hugues Fruchet,
	Loic Poulain, Samuel Bobrowicz, Steve Longerbeam, Daniel Mack,
	Jacopo Mondi, Maxime Ripard

The pixel clock rates were introduced to report the initially static clock
rate.

Since this is now handled dynamically, we can remove them entirely.

Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
 drivers/media/i2c/ov5640.c | 21 +--------------------
 1 file changed, 1 insertion(+), 20 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index bcfb2b25a450..e96063c9e352 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -172,7 +172,6 @@ struct ov5640_mode_info {
 	u32 htot;
 	u32 vact;
 	u32 vtot;
-	u32 pixel_clock;
 	const struct reg_value *reg_data;
 	u32 reg_data_size;
 };
@@ -696,7 +695,6 @@ static const struct reg_value ov5640_setting_15fps_QSXGA_2592_1944[] = {
 /* power-on sensor init reg table */
 static const struct ov5640_mode_info ov5640_mode_init_data = {
 	0, SUBSAMPLING, 640, 1896, 480, 984,
-	56000000,
 	ov5640_init_setting_30fps_VGA,
 	ARRAY_SIZE(ov5640_init_setting_30fps_VGA),
 };
@@ -706,91 +704,74 @@ ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES] = {
 	{
 		{OV5640_MODE_QCIF_176_144, SUBSAMPLING,
 		 176, 1896, 144, 984,
-		 28000000,
 		 ov5640_setting_15fps_QCIF_176_144,
 		 ARRAY_SIZE(ov5640_setting_15fps_QCIF_176_144)},
 		{OV5640_MODE_QVGA_320_240, SUBSAMPLING,
 		 320, 1896, 240, 984,
-		 28000000,
 		 ov5640_setting_15fps_QVGA_320_240,
 		 ARRAY_SIZE(ov5640_setting_15fps_QVGA_320_240)},
 		{OV5640_MODE_VGA_640_480, SUBSAMPLING,
 		 640, 1896, 480, 1080,
-		 28000000,
 		 ov5640_setting_15fps_VGA_640_480,
 		 ARRAY_SIZE(ov5640_setting_15fps_VGA_640_480)},
 		{OV5640_MODE_NTSC_720_480, SUBSAMPLING,
 		 720, 1896, 480, 984,
-		 28000000,
 		 ov5640_setting_15fps_NTSC_720_480,
 		 ARRAY_SIZE(ov5640_setting_15fps_NTSC_720_480)},
 		{OV5640_MODE_PAL_720_576, SUBSAMPLING,
 		 720, 1896, 576, 984,
-		 28000000,
 		 ov5640_setting_15fps_PAL_720_576,
 		 ARRAY_SIZE(ov5640_setting_15fps_PAL_720_576)},
 		{OV5640_MODE_XGA_1024_768, SUBSAMPLING,
 		 1024, 1896, 768, 1080,
-		 28000000,
 		 ov5640_setting_15fps_XGA_1024_768,
 		 ARRAY_SIZE(ov5640_setting_15fps_XGA_1024_768)},
 		{OV5640_MODE_720P_1280_720, SUBSAMPLING,
 		 1280, 1892, 720, 740,
-		 21000000,
 		 ov5640_setting_15fps_720P_1280_720,
 		 ARRAY_SIZE(ov5640_setting_15fps_720P_1280_720)},
 		{OV5640_MODE_1080P_1920_1080, SCALING,
 		 1920, 2500, 1080, 1120,
-		 42000000,
 		 ov5640_setting_15fps_1080P_1920_1080,
 		 ARRAY_SIZE(ov5640_setting_15fps_1080P_1920_1080)},
 		{OV5640_MODE_QSXGA_2592_1944, SCALING,
 		 2592, 2844, 1944, 1968,
-		 84000000,
 		 ov5640_setting_15fps_QSXGA_2592_1944,
 		 ARRAY_SIZE(ov5640_setting_15fps_QSXGA_2592_1944)},
 	}, {
 		{OV5640_MODE_QCIF_176_144, SUBSAMPLING,
 		 176, 1896, 144, 984,
-		 56000000,
 		 ov5640_setting_30fps_QCIF_176_144,
 		 ARRAY_SIZE(ov5640_setting_30fps_QCIF_176_144)},
 		{OV5640_MODE_QVGA_320_240, SUBSAMPLING,
 		 320, 1896, 240, 984,
-		 56000000,
 		 ov5640_setting_30fps_QVGA_320_240,
 		 ARRAY_SIZE(ov5640_setting_30fps_QVGA_320_240)},
 		{OV5640_MODE_VGA_640_480, SUBSAMPLING,
 		 640, 1896, 480, 1080,
-		 56000000,
 		 ov5640_setting_30fps_VGA_640_480,
 		 ARRAY_SIZE(ov5640_setting_30fps_VGA_640_480)},
 		{OV5640_MODE_NTSC_720_480, SUBSAMPLING,
 		 720, 1896, 480, 984,
-		 56000000,
 		 ov5640_setting_30fps_NTSC_720_480,
 		 ARRAY_SIZE(ov5640_setting_30fps_NTSC_720_480)},
 		{OV5640_MODE_PAL_720_576, SUBSAMPLING,
 		 720, 1896, 576, 984,
-		 56000000,
 		 ov5640_setting_30fps_PAL_720_576,
 		 ARRAY_SIZE(ov5640_setting_30fps_PAL_720_576)},
 		{OV5640_MODE_XGA_1024_768, SUBSAMPLING,
 		 1024, 1896, 768, 1080,
-		 56000000,
 		 ov5640_setting_30fps_XGA_1024_768,
 		 ARRAY_SIZE(ov5640_setting_30fps_XGA_1024_768)},
 		{OV5640_MODE_720P_1280_720, SUBSAMPLING,
 		 1280, 1892, 720, 740,
-		 42000000,
 		 ov5640_setting_30fps_720P_1280_720,
 		 ARRAY_SIZE(ov5640_setting_30fps_720P_1280_720)},
 		{OV5640_MODE_1080P_1920_1080, SCALING,
 		 1920, 2500, 1080, 1120,
-		 84000000,
 		 ov5640_setting_30fps_1080P_1920_1080,
 		 ARRAY_SIZE(ov5640_setting_30fps_1080P_1920_1080)},
-		{OV5640_MODE_QSXGA_2592_1944, -1, 0, 0, 0, 0, 0, NULL, 0},
+		{OV5640_MODE_QSXGA_2592_1944, -1, 0, 0, 0, 0, NULL, 0},
 	},
 };
 
-- 
2.19.1

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v5 07/11] media: ov5640: Enhance FPS handling
  2018-11-13 13:03 [PATCH v5 00/11] media: ov5640: Misc cleanup and improvements Maxime Ripard
                   ` (5 preceding siblings ...)
  2018-11-13 13:03 ` [PATCH v5 06/11] media: ov5640: Remove pixel clock rates Maxime Ripard
@ 2018-11-13 13:03 ` Maxime Ripard
  2018-11-13 13:03 ` [PATCH v5 08/11] media: ov5640: Make the return rate type more explicit Maxime Ripard
                   ` (3 subsequent siblings)
  10 siblings, 0 replies; 17+ messages in thread
From: Maxime Ripard @ 2018-11-13 13:03 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: Laurent Pinchart, linux-media, Thomas Petazzoni,
	Mylene Josserand, Hans Verkuil, Sakari Ailus, Hugues Fruchet,
	Loic Poulain, Samuel Bobrowicz, Steve Longerbeam, Daniel Mack,
	Jacopo Mondi, Maxime Ripard

Now that we have moved the clock generation logic out of the bytes array,
these arrays are identical between the 15fps and 30fps variants.

Remove the duplicate entries, and convert the code accordingly.

Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
 drivers/media/i2c/ov5640.c | 306 +++++++------------------------------
 1 file changed, 51 insertions(+), 255 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index e96063c9e352..be047dd7fbfc 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -340,64 +340,7 @@ static const struct reg_value ov5640_init_setting_30fps_VGA[] = {
 	{0x3a1f, 0x14, 0, 0}, {0x3008, 0x02, 0, 0}, {0x3c00, 0x04, 0, 300},
 };
 
-static const struct reg_value ov5640_setting_30fps_VGA_640_480[] = {
-	{0x3c07, 0x08, 0, 0},
-	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
-	{0x3814, 0x31, 0, 0},
-	{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
-	{0x3802, 0x00, 0, 0}, {0x3803, 0x04, 0, 0}, {0x3804, 0x0a, 0, 0},
-	{0x3805, 0x3f, 0, 0}, {0x3806, 0x07, 0, 0}, {0x3807, 0x9b, 0, 0},
-	{0x3810, 0x00, 0, 0},
-	{0x3811, 0x10, 0, 0}, {0x3812, 0x00, 0, 0}, {0x3813, 0x06, 0, 0},
-	{0x3618, 0x00, 0, 0}, {0x3612, 0x29, 0, 0}, {0x3708, 0x64, 0, 0},
-	{0x3709, 0x52, 0, 0}, {0x370c, 0x03, 0, 0}, {0x3a02, 0x03, 0, 0},
-	{0x3a03, 0xd8, 0, 0}, {0x3a08, 0x01, 0, 0}, {0x3a09, 0x0e, 0, 0},
-	{0x3a0a, 0x00, 0, 0}, {0x3a0b, 0xf6, 0, 0}, {0x3a0e, 0x03, 0, 0},
-	{0x3a0d, 0x04, 0, 0}, {0x3a14, 0x03, 0, 0}, {0x3a15, 0xd8, 0, 0},
-	{0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0}, {0x4713, 0x03, 0, 0},
-	{0x4407, 0x04, 0, 0}, {0x460b, 0x35, 0, 0}, {0x460c, 0x22, 0, 0},
-	{0x3824, 0x02, 0, 0}, {0x5001, 0xa3, 0, 0}, {0x3503, 0x00, 0, 0},
-};
-
-static const struct reg_value ov5640_setting_15fps_VGA_640_480[] = {
-	{0x3c07, 0x08, 0, 0},
-	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
-	{0x3814, 0x31, 0, 0},
-	{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
-	{0x3802, 0x00, 0, 0}, {0x3803, 0x04, 0, 0}, {0x3804, 0x0a, 0, 0},
-	{0x3805, 0x3f, 0, 0}, {0x3806, 0x07, 0, 0}, {0x3807, 0x9b, 0, 0},
-	{0x3810, 0x00, 0, 0},
-	{0x3811, 0x10, 0, 0}, {0x3812, 0x00, 0, 0}, {0x3813, 0x06, 0, 0},
-	{0x3618, 0x00, 0, 0}, {0x3612, 0x29, 0, 0}, {0x3708, 0x64, 0, 0},
-	{0x3709, 0x52, 0, 0}, {0x370c, 0x03, 0, 0}, {0x3a02, 0x03, 0, 0},
-	{0x3a03, 0xd8, 0, 0}, {0x3a08, 0x01, 0, 0}, {0x3a09, 0x27, 0, 0},
-	{0x3a0a, 0x00, 0, 0}, {0x3a0b, 0xf6, 0, 0}, {0x3a0e, 0x03, 0, 0},
-	{0x3a0d, 0x04, 0, 0}, {0x3a14, 0x03, 0, 0}, {0x3a15, 0xd8, 0, 0},
-	{0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0}, {0x4713, 0x03, 0, 0},
-	{0x4407, 0x04, 0, 0}, {0x460b, 0x35, 0, 0}, {0x460c, 0x22, 0, 0},
-	{0x3824, 0x02, 0, 0}, {0x5001, 0xa3, 0, 0},
-};
-
-static const struct reg_value ov5640_setting_30fps_XGA_1024_768[] = {
-	{0x3c07, 0x08, 0, 0},
-	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
-	{0x3814, 0x31, 0, 0},
-	{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
-	{0x3802, 0x00, 0, 0}, {0x3803, 0x04, 0, 0}, {0x3804, 0x0a, 0, 0},
-	{0x3805, 0x3f, 0, 0}, {0x3806, 0x07, 0, 0}, {0x3807, 0x9b, 0, 0},
-	{0x3810, 0x00, 0, 0},
-	{0x3811, 0x10, 0, 0}, {0x3812, 0x00, 0, 0}, {0x3813, 0x06, 0, 0},
-	{0x3618, 0x00, 0, 0}, {0x3612, 0x29, 0, 0}, {0x3708, 0x64, 0, 0},
-	{0x3709, 0x52, 0, 0}, {0x370c, 0x03, 0, 0}, {0x3a02, 0x03, 0, 0},
-	{0x3a03, 0xd8, 0, 0}, {0x3a08, 0x01, 0, 0}, {0x3a09, 0x0e, 0, 0},
-	{0x3a0a, 0x00, 0, 0}, {0x3a0b, 0xf6, 0, 0}, {0x3a0e, 0x03, 0, 0},
-	{0x3a0d, 0x04, 0, 0}, {0x3a14, 0x03, 0, 0}, {0x3a15, 0xd8, 0, 0},
-	{0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0}, {0x4713, 0x03, 0, 0},
-	{0x4407, 0x04, 0, 0}, {0x460b, 0x35, 0, 0}, {0x460c, 0x22, 0, 0},
-	{0x3824, 0x02, 0, 0}, {0x5001, 0xa3, 0, 0}, {0x3503, 0x00, 0, 0},
-};
-
-static const struct reg_value ov5640_setting_15fps_XGA_1024_768[] = {
+static const struct reg_value ov5640_setting_VGA_640_480[] = {
 	{0x3c07, 0x08, 0, 0},
 	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
 	{0x3814, 0x31, 0, 0},
@@ -416,7 +359,7 @@ static const struct reg_value ov5640_setting_15fps_XGA_1024_768[] = {
 	{0x3824, 0x02, 0, 0}, {0x5001, 0xa3, 0, 0},
 };
 
-static const struct reg_value ov5640_setting_30fps_QVGA_320_240[] = {
+static const struct reg_value ov5640_setting_XGA_1024_768[] = {
 	{0x3c07, 0x08, 0, 0},
 	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
 	{0x3814, 0x31, 0, 0},
@@ -435,7 +378,7 @@ static const struct reg_value ov5640_setting_30fps_QVGA_320_240[] = {
 	{0x3824, 0x02, 0, 0}, {0x5001, 0xa3, 0, 0},
 };
 
-static const struct reg_value ov5640_setting_15fps_QVGA_320_240[] = {
+static const struct reg_value ov5640_setting_QVGA_320_240[] = {
 	{0x3c07, 0x08, 0, 0},
 	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
 	{0x3814, 0x31, 0, 0},
@@ -454,7 +397,7 @@ static const struct reg_value ov5640_setting_15fps_QVGA_320_240[] = {
 	{0x3824, 0x02, 0, 0}, {0x5001, 0xa3, 0, 0},
 };
 
-static const struct reg_value ov5640_setting_30fps_QCIF_176_144[] = {
+static const struct reg_value ov5640_setting_QCIF_176_144[] = {
 	{0x3c07, 0x08, 0, 0},
 	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
 	{0x3814, 0x31, 0, 0},
@@ -473,26 +416,7 @@ static const struct reg_value ov5640_setting_30fps_QCIF_176_144[] = {
 	{0x3824, 0x02, 0, 0}, {0x5001, 0xa3, 0, 0},
 };
 
-static const struct reg_value ov5640_setting_15fps_QCIF_176_144[] = {
-	{0x3c07, 0x08, 0, 0},
-	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
-	{0x3814, 0x31, 0, 0},
-	{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
-	{0x3802, 0x00, 0, 0}, {0x3803, 0x04, 0, 0}, {0x3804, 0x0a, 0, 0},
-	{0x3805, 0x3f, 0, 0}, {0x3806, 0x07, 0, 0}, {0x3807, 0x9b, 0, 0},
-	{0x3810, 0x00, 0, 0},
-	{0x3811, 0x10, 0, 0}, {0x3812, 0x00, 0, 0}, {0x3813, 0x06, 0, 0},
-	{0x3618, 0x00, 0, 0}, {0x3612, 0x29, 0, 0}, {0x3708, 0x64, 0, 0},
-	{0x3709, 0x52, 0, 0}, {0x370c, 0x03, 0, 0}, {0x3a02, 0x03, 0, 0},
-	{0x3a03, 0xd8, 0, 0}, {0x3a08, 0x01, 0, 0}, {0x3a09, 0x27, 0, 0},
-	{0x3a0a, 0x00, 0, 0}, {0x3a0b, 0xf6, 0, 0}, {0x3a0e, 0x03, 0, 0},
-	{0x3a0d, 0x04, 0, 0}, {0x3a14, 0x03, 0, 0}, {0x3a15, 0xd8, 0, 0},
-	{0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0}, {0x4713, 0x03, 0, 0},
-	{0x4407, 0x04, 0, 0}, {0x460b, 0x35, 0, 0}, {0x460c, 0x22, 0, 0},
-	{0x3824, 0x02, 0, 0}, {0x5001, 0xa3, 0, 0},
-};
-
-static const struct reg_value ov5640_setting_30fps_NTSC_720_480[] = {
+static const struct reg_value ov5640_setting_NTSC_720_480[] = {
 	{0x3c07, 0x08, 0, 0},
 	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
 	{0x3814, 0x31, 0, 0},
@@ -511,26 +435,7 @@ static const struct reg_value ov5640_setting_30fps_NTSC_720_480[] = {
 	{0x3824, 0x02, 0, 0}, {0x5001, 0xa3, 0, 0},
 };
 
-static const struct reg_value ov5640_setting_15fps_NTSC_720_480[] = {
-	{0x3c07, 0x08, 0, 0},
-	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
-	{0x3814, 0x31, 0, 0},
-	{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
-	{0x3802, 0x00, 0, 0}, {0x3803, 0x04, 0, 0}, {0x3804, 0x0a, 0, 0},
-	{0x3805, 0x3f, 0, 0}, {0x3806, 0x07, 0, 0}, {0x3807, 0x9b, 0, 0},
-	{0x3810, 0x00, 0, 0},
-	{0x3811, 0x10, 0, 0}, {0x3812, 0x00, 0, 0}, {0x3813, 0x3c, 0, 0},
-	{0x3618, 0x00, 0, 0}, {0x3612, 0x29, 0, 0}, {0x3708, 0x64, 0, 0},
-	{0x3709, 0x52, 0, 0}, {0x370c, 0x03, 0, 0}, {0x3a02, 0x03, 0, 0},
-	{0x3a03, 0xd8, 0, 0}, {0x3a08, 0x01, 0, 0}, {0x3a09, 0x27, 0, 0},
-	{0x3a0a, 0x00, 0, 0}, {0x3a0b, 0xf6, 0, 0}, {0x3a0e, 0x03, 0, 0},
-	{0x3a0d, 0x04, 0, 0}, {0x3a14, 0x03, 0, 0}, {0x3a15, 0xd8, 0, 0},
-	{0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0}, {0x4713, 0x03, 0, 0},
-	{0x4407, 0x04, 0, 0}, {0x460b, 0x35, 0, 0}, {0x460c, 0x22, 0, 0},
-	{0x3824, 0x02, 0, 0}, {0x5001, 0xa3, 0, 0},
-};
-
-static const struct reg_value ov5640_setting_30fps_PAL_720_576[] = {
+static const struct reg_value ov5640_setting_PAL_720_576[] = {
 	{0x3c07, 0x08, 0, 0},
 	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
 	{0x3814, 0x31, 0, 0},
@@ -549,47 +454,7 @@ static const struct reg_value ov5640_setting_30fps_PAL_720_576[] = {
 	{0x3824, 0x02, 0, 0}, {0x5001, 0xa3, 0, 0},
 };
 
-static const struct reg_value ov5640_setting_15fps_PAL_720_576[] = {
-	{0x3c07, 0x08, 0, 0},
-	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
-	{0x3814, 0x31, 0, 0},
-	{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
-	{0x3802, 0x00, 0, 0}, {0x3803, 0x04, 0, 0}, {0x3804, 0x0a, 0, 0},
-	{0x3805, 0x3f, 0, 0}, {0x3806, 0x07, 0, 0}, {0x3807, 0x9b, 0, 0},
-	{0x3810, 0x00, 0, 0},
-	{0x3811, 0x38, 0, 0}, {0x3812, 0x00, 0, 0}, {0x3813, 0x06, 0, 0},
-	{0x3618, 0x00, 0, 0}, {0x3612, 0x29, 0, 0}, {0x3708, 0x64, 0, 0},
-	{0x3709, 0x52, 0, 0}, {0x370c, 0x03, 0, 0}, {0x3a02, 0x03, 0, 0},
-	{0x3a03, 0xd8, 0, 0}, {0x3a08, 0x01, 0, 0}, {0x3a09, 0x27, 0, 0},
-	{0x3a0a, 0x00, 0, 0}, {0x3a0b, 0xf6, 0, 0}, {0x3a0e, 0x03, 0, 0},
-	{0x3a0d, 0x04, 0, 0}, {0x3a14, 0x03, 0, 0}, {0x3a15, 0xd8, 0, 0},
-	{0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0}, {0x4713, 0x03, 0, 0},
-	{0x4407, 0x04, 0, 0}, {0x460b, 0x35, 0, 0}, {0x460c, 0x22, 0, 0},
-	{0x3824, 0x02, 0, 0}, {0x5001, 0xa3, 0, 0},
-};
-
-static const struct reg_value ov5640_setting_30fps_720P_1280_720[] = {
-	{0x3008, 0x42, 0, 0},
-	{0x3c07, 0x07, 0, 0},
-	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
-	{0x3814, 0x31, 0, 0},
-	{0x3815, 0x31, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
-	{0x3802, 0x00, 0, 0}, {0x3803, 0xfa, 0, 0}, {0x3804, 0x0a, 0, 0},
-	{0x3805, 0x3f, 0, 0}, {0x3806, 0x06, 0, 0}, {0x3807, 0xa9, 0, 0},
-	{0x3810, 0x00, 0, 0},
-	{0x3811, 0x10, 0, 0}, {0x3812, 0x00, 0, 0}, {0x3813, 0x04, 0, 0},
-	{0x3618, 0x00, 0, 0}, {0x3612, 0x29, 0, 0}, {0x3708, 0x64, 0, 0},
-	{0x3709, 0x52, 0, 0}, {0x370c, 0x03, 0, 0}, {0x3a02, 0x02, 0, 0},
-	{0x3a03, 0xe4, 0, 0}, {0x3a08, 0x01, 0, 0}, {0x3a09, 0xbc, 0, 0},
-	{0x3a0a, 0x01, 0, 0}, {0x3a0b, 0x72, 0, 0}, {0x3a0e, 0x01, 0, 0},
-	{0x3a0d, 0x02, 0, 0}, {0x3a14, 0x02, 0, 0}, {0x3a15, 0xe4, 0, 0},
-	{0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0}, {0x4713, 0x02, 0, 0},
-	{0x4407, 0x04, 0, 0}, {0x460b, 0x37, 0, 0}, {0x460c, 0x20, 0, 0},
-	{0x3824, 0x04, 0, 0}, {0x5001, 0x83, 0, 0}, {0x4005, 0x1a, 0, 0},
-	{0x3008, 0x02, 0, 0}, {0x3503, 0,    0, 0},
-};
-
-static const struct reg_value ov5640_setting_15fps_720P_1280_720[] = {
+static const struct reg_value ov5640_setting_720P_1280_720[] = {
 	{0x3c07, 0x07, 0, 0},
 	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
 	{0x3814, 0x31, 0, 0},
@@ -608,40 +473,7 @@ static const struct reg_value ov5640_setting_15fps_720P_1280_720[] = {
 	{0x3824, 0x04, 0, 0}, {0x5001, 0x83, 0, 0},
 };
 
-static const struct reg_value ov5640_setting_30fps_1080P_1920_1080[] = {
-	{0x3008, 0x42, 0, 0},
-	{0x3c07, 0x08, 0, 0},
-	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
-	{0x3814, 0x11, 0, 0},
-	{0x3815, 0x11, 0, 0}, {0x3800, 0x00, 0, 0}, {0x3801, 0x00, 0, 0},
-	{0x3802, 0x00, 0, 0}, {0x3803, 0x00, 0, 0}, {0x3804, 0x0a, 0, 0},
-	{0x3805, 0x3f, 0, 0}, {0x3806, 0x07, 0, 0}, {0x3807, 0x9f, 0, 0},
-	{0x3810, 0x00, 0, 0},
-	{0x3811, 0x10, 0, 0}, {0x3812, 0x00, 0, 0}, {0x3813, 0x04, 0, 0},
-	{0x3618, 0x04, 0, 0}, {0x3612, 0x29, 0, 0}, {0x3708, 0x21, 0, 0},
-	{0x3709, 0x12, 0, 0}, {0x370c, 0x00, 0, 0}, {0x3a02, 0x03, 0, 0},
-	{0x3a03, 0xd8, 0, 0}, {0x3a08, 0x01, 0, 0}, {0x3a09, 0x27, 0, 0},
-	{0x3a0a, 0x00, 0, 0}, {0x3a0b, 0xf6, 0, 0}, {0x3a0e, 0x03, 0, 0},
-	{0x3a0d, 0x04, 0, 0}, {0x3a14, 0x03, 0, 0}, {0x3a15, 0xd8, 0, 0},
-	{0x4001, 0x02, 0, 0}, {0x4004, 0x06, 0, 0}, {0x4713, 0x03, 0, 0},
-	{0x4407, 0x04, 0, 0}, {0x460b, 0x35, 0, 0}, {0x460c, 0x22, 0, 0},
-	{0x3824, 0x02, 0, 0}, {0x5001, 0x83, 0, 0},
-	{0x3c07, 0x07, 0, 0}, {0x3c08, 0x00, 0, 0},
-	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
-	{0x3800, 0x01, 0, 0}, {0x3801, 0x50, 0, 0}, {0x3802, 0x01, 0, 0},
-	{0x3803, 0xb2, 0, 0}, {0x3804, 0x08, 0, 0}, {0x3805, 0xef, 0, 0},
-	{0x3806, 0x05, 0, 0}, {0x3807, 0xf1, 0, 0},
-	{0x3612, 0x2b, 0, 0}, {0x3708, 0x64, 0, 0},
-	{0x3a02, 0x04, 0, 0}, {0x3a03, 0x60, 0, 0}, {0x3a08, 0x01, 0, 0},
-	{0x3a09, 0x50, 0, 0}, {0x3a0a, 0x01, 0, 0}, {0x3a0b, 0x18, 0, 0},
-	{0x3a0e, 0x03, 0, 0}, {0x3a0d, 0x04, 0, 0}, {0x3a14, 0x04, 0, 0},
-	{0x3a15, 0x60, 0, 0}, {0x4713, 0x02, 0, 0}, {0x4407, 0x04, 0, 0},
-	{0x460b, 0x37, 0, 0}, {0x460c, 0x20, 0, 0}, {0x3824, 0x04, 0, 0},
-	{0x4005, 0x1a, 0, 0}, {0x3008, 0x02, 0, 0},
-	{0x3503, 0, 0, 0},
-};
-
-static const struct reg_value ov5640_setting_15fps_1080P_1920_1080[] = {
+static const struct reg_value ov5640_setting_1080P_1920_1080[] = {
 	{0x3008, 0x42, 0, 0},
 	{0x3c07, 0x08, 0, 0},
 	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
@@ -673,7 +505,7 @@ static const struct reg_value ov5640_setting_15fps_1080P_1920_1080[] = {
 	{0x4005, 0x1a, 0, 0}, {0x3008, 0x02, 0, 0}, {0x3503, 0, 0, 0},
 };
 
-static const struct reg_value ov5640_setting_15fps_QSXGA_2592_1944[] = {
+static const struct reg_value ov5640_setting_QSXGA_2592_1944[] = {
 	{0x3c07, 0x08, 0, 0},
 	{0x3c09, 0x1c, 0, 0}, {0x3c0a, 0x9c, 0, 0}, {0x3c0b, 0x40, 0, 0},
 	{0x3814, 0x11, 0, 0},
@@ -700,79 +532,43 @@ static const struct ov5640_mode_info ov5640_mode_init_data = {
 };
 
 static const struct ov5640_mode_info
-ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES] = {
-	{
-		{OV5640_MODE_QCIF_176_144, SUBSAMPLING,
-		 176, 1896, 144, 984,
-		 ov5640_setting_15fps_QCIF_176_144,
-		 ARRAY_SIZE(ov5640_setting_15fps_QCIF_176_144)},
-		{OV5640_MODE_QVGA_320_240, SUBSAMPLING,
-		 320, 1896, 240, 984,
-		 ov5640_setting_15fps_QVGA_320_240,
-		 ARRAY_SIZE(ov5640_setting_15fps_QVGA_320_240)},
-		{OV5640_MODE_VGA_640_480, SUBSAMPLING,
-		 640, 1896, 480, 1080,
-		 ov5640_setting_15fps_VGA_640_480,
-		 ARRAY_SIZE(ov5640_setting_15fps_VGA_640_480)},
-		{OV5640_MODE_NTSC_720_480, SUBSAMPLING,
-		 720, 1896, 480, 984,
-		 ov5640_setting_15fps_NTSC_720_480,
-		 ARRAY_SIZE(ov5640_setting_15fps_NTSC_720_480)},
-		{OV5640_MODE_PAL_720_576, SUBSAMPLING,
-		 720, 1896, 576, 984,
-		 ov5640_setting_15fps_PAL_720_576,
-		 ARRAY_SIZE(ov5640_setting_15fps_PAL_720_576)},
-		{OV5640_MODE_XGA_1024_768, SUBSAMPLING,
-		 1024, 1896, 768, 1080,
-		 ov5640_setting_15fps_XGA_1024_768,
-		 ARRAY_SIZE(ov5640_setting_15fps_XGA_1024_768)},
-		{OV5640_MODE_720P_1280_720, SUBSAMPLING,
-		 1280, 1892, 720, 740,
-		 ov5640_setting_15fps_720P_1280_720,
-		 ARRAY_SIZE(ov5640_setting_15fps_720P_1280_720)},
-		{OV5640_MODE_1080P_1920_1080, SCALING,
-		 1920, 2500, 1080, 1120,
-		 ov5640_setting_15fps_1080P_1920_1080,
-		 ARRAY_SIZE(ov5640_setting_15fps_1080P_1920_1080)},
-		{OV5640_MODE_QSXGA_2592_1944, SCALING,
-		 2592, 2844, 1944, 1968,
-		 ov5640_setting_15fps_QSXGA_2592_1944,
-		 ARRAY_SIZE(ov5640_setting_15fps_QSXGA_2592_1944)},
-	}, {
-		{OV5640_MODE_QCIF_176_144, SUBSAMPLING,
-		 176, 1896, 144, 984,
-		 ov5640_setting_30fps_QCIF_176_144,
-		 ARRAY_SIZE(ov5640_setting_30fps_QCIF_176_144)},
-		{OV5640_MODE_QVGA_320_240, SUBSAMPLING,
-		 320, 1896, 240, 984,
-		 ov5640_setting_30fps_QVGA_320_240,
-		 ARRAY_SIZE(ov5640_setting_30fps_QVGA_320_240)},
-		{OV5640_MODE_VGA_640_480, SUBSAMPLING,
-		 640, 1896, 480, 1080,
-		 ov5640_setting_30fps_VGA_640_480,
-		 ARRAY_SIZE(ov5640_setting_30fps_VGA_640_480)},
-		{OV5640_MODE_NTSC_720_480, SUBSAMPLING,
-		 720, 1896, 480, 984,
-		 ov5640_setting_30fps_NTSC_720_480,
-		 ARRAY_SIZE(ov5640_setting_30fps_NTSC_720_480)},
-		{OV5640_MODE_PAL_720_576, SUBSAMPLING,
-		 720, 1896, 576, 984,
-		 ov5640_setting_30fps_PAL_720_576,
-		 ARRAY_SIZE(ov5640_setting_30fps_PAL_720_576)},
-		{OV5640_MODE_XGA_1024_768, SUBSAMPLING,
-		 1024, 1896, 768, 1080,
-		 ov5640_setting_30fps_XGA_1024_768,
-		 ARRAY_SIZE(ov5640_setting_30fps_XGA_1024_768)},
-		{OV5640_MODE_720P_1280_720, SUBSAMPLING,
-		 1280, 1892, 720, 740,
-		 ov5640_setting_30fps_720P_1280_720,
-		 ARRAY_SIZE(ov5640_setting_30fps_720P_1280_720)},
-		{OV5640_MODE_1080P_1920_1080, SCALING,
-		 1920, 2500, 1080, 1120,
-		 ov5640_setting_30fps_1080P_1920_1080,
-		 ARRAY_SIZE(ov5640_setting_30fps_1080P_1920_1080)},
-		{OV5640_MODE_QSXGA_2592_1944, -1, 0, 0, 0, 0, NULL, 0},
-	},
+ov5640_mode_data[OV5640_NUM_MODES] = {
+	{OV5640_MODE_QCIF_176_144, SUBSAMPLING,
+	 176, 1896, 144, 984,
+	 ov5640_setting_QCIF_176_144,
+	 ARRAY_SIZE(ov5640_setting_QCIF_176_144)},
+	{OV5640_MODE_QVGA_320_240, SUBSAMPLING,
+	 320, 1896, 240, 984,
+	 ov5640_setting_QVGA_320_240,
+	 ARRAY_SIZE(ov5640_setting_QVGA_320_240)},
+	{OV5640_MODE_VGA_640_480, SUBSAMPLING,
+	 640, 1896, 480, 1080,
+	 ov5640_setting_VGA_640_480,
+	 ARRAY_SIZE(ov5640_setting_VGA_640_480)},
+	{OV5640_MODE_NTSC_720_480, SUBSAMPLING,
+	 720, 1896, 480, 984,
+	 ov5640_setting_NTSC_720_480,
+	 ARRAY_SIZE(ov5640_setting_NTSC_720_480)},
+	{OV5640_MODE_PAL_720_576, SUBSAMPLING,
+	 720, 1896, 576, 984,
+	 ov5640_setting_PAL_720_576,
+	 ARRAY_SIZE(ov5640_setting_PAL_720_576)},
+	{OV5640_MODE_XGA_1024_768, SUBSAMPLING,
+	 1024, 1896, 768, 1080,
+	 ov5640_setting_XGA_1024_768,
+	 ARRAY_SIZE(ov5640_setting_XGA_1024_768)},
+	{OV5640_MODE_720P_1280_720, SUBSAMPLING,
+	 1280, 1892, 720, 740,
+	 ov5640_setting_720P_1280_720,
+	 ARRAY_SIZE(ov5640_setting_720P_1280_720)},
+	{OV5640_MODE_1080P_1920_1080, SCALING,
+	 1920, 2500, 1080, 1120,
+	 ov5640_setting_1080P_1920_1080,
+	 ARRAY_SIZE(ov5640_setting_1080P_1920_1080)},
+	{OV5640_MODE_QSXGA_2592_1944, SCALING,
+	 2592, 2844, 1944, 1968,
+	 ov5640_setting_QSXGA_2592_1944,
+	 ARRAY_SIZE(ov5640_setting_QSXGA_2592_1944)},
 };
 
 static int ov5640_init_slave_id(struct ov5640_dev *sensor)
@@ -1757,8 +1553,8 @@ ov5640_find_mode(struct ov5640_dev *sensor, enum ov5640_frame_rate fr,
 {
 	const struct ov5640_mode_info *mode;
 
-	mode = v4l2_find_nearest_size(ov5640_mode_data[fr],
-				      ARRAY_SIZE(ov5640_mode_data[fr]),
+	mode = v4l2_find_nearest_size(ov5640_mode_data,
+				      ARRAY_SIZE(ov5640_mode_data),
 				      hact, vact,
 				      width, height);
 
@@ -2833,10 +2629,10 @@ static int ov5640_enum_frame_size(struct v4l2_subdev *sd,
 		return -EINVAL;
 
 	fse->min_width =
-		ov5640_mode_data[0][fse->index].hact;
+		ov5640_mode_data[fse->index].hact;
 	fse->max_width = fse->min_width;
 	fse->min_height =
-		ov5640_mode_data[0][fse->index].vact;
+		ov5640_mode_data[fse->index].vact;
 	fse->max_height = fse->min_height;
 
 	return 0;
@@ -3067,7 +2863,7 @@ static int ov5640_probe(struct i2c_client *client,
 	sensor->frame_interval.denominator = ov5640_framerates[OV5640_30_FPS];
 	sensor->current_fr = OV5640_30_FPS;
 	sensor->current_mode =
-		&ov5640_mode_data[OV5640_30_FPS][OV5640_MODE_VGA_640_480];
+		&ov5640_mode_data[OV5640_MODE_VGA_640_480];
 	sensor->last_mode = sensor->current_mode;
 
 	sensor->ae_target = 52;
-- 
2.19.1

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v5 08/11] media: ov5640: Make the return rate type more explicit
  2018-11-13 13:03 [PATCH v5 00/11] media: ov5640: Misc cleanup and improvements Maxime Ripard
                   ` (6 preceding siblings ...)
  2018-11-13 13:03 ` [PATCH v5 07/11] media: ov5640: Enhance FPS handling Maxime Ripard
@ 2018-11-13 13:03 ` Maxime Ripard
  2018-11-13 13:03 ` [PATCH v5 09/11] media: ov5640: Make the FPS clamping / rounding more extendable Maxime Ripard
                   ` (2 subsequent siblings)
  10 siblings, 0 replies; 17+ messages in thread
From: Maxime Ripard @ 2018-11-13 13:03 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: Laurent Pinchart, linux-media, Thomas Petazzoni,
	Mylene Josserand, Hans Verkuil, Sakari Ailus, Hugues Fruchet,
	Loic Poulain, Samuel Bobrowicz, Steve Longerbeam, Daniel Mack,
	Jacopo Mondi, Maxime Ripard

In the ov5640_try_frame_interval function, the ret variable actually holds
the frame rate index to use, which is represented by the enum
ov5640_frame_rate in the driver.

Make it more obvious.

Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
 drivers/media/i2c/ov5640.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index be047dd7fbfc..fc2e03193da6 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -2052,8 +2052,8 @@ static int ov5640_try_frame_interval(struct ov5640_dev *sensor,
 				     u32 width, u32 height)
 {
 	const struct ov5640_mode_info *mode;
+	enum ov5640_frame_rate rate = OV5640_30_FPS;
 	u32 minfps, maxfps, fps;
-	int ret;
 
 	minfps = ov5640_framerates[OV5640_15_FPS];
 	maxfps = ov5640_framerates[OV5640_30_FPS];
@@ -2076,10 +2076,10 @@ static int ov5640_try_frame_interval(struct ov5640_dev *sensor,
 	else
 		fi->denominator = minfps;
 
-	ret = (fi->denominator == minfps) ? OV5640_15_FPS : OV5640_30_FPS;
+	rate = (fi->denominator == minfps) ? OV5640_15_FPS : OV5640_30_FPS;
 
-	mode = ov5640_find_mode(sensor, ret, width, height, false);
-	return mode ? ret : -EINVAL;
+	mode = ov5640_find_mode(sensor, rate, width, height, false);
+	return mode ? rate : -EINVAL;
 }
 
 static int ov5640_get_fmt(struct v4l2_subdev *sd,
-- 
2.19.1

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v5 09/11] media: ov5640: Make the FPS clamping / rounding more extendable
  2018-11-13 13:03 [PATCH v5 00/11] media: ov5640: Misc cleanup and improvements Maxime Ripard
                   ` (7 preceding siblings ...)
  2018-11-13 13:03 ` [PATCH v5 08/11] media: ov5640: Make the return rate type more explicit Maxime Ripard
@ 2018-11-13 13:03 ` Maxime Ripard
  2018-11-13 13:03 ` [PATCH v5 10/11] media: ov5640: Add 60 fps support Maxime Ripard
  2018-11-13 13:03 ` [PATCH v5 11/11] media: ov5640: Remove duplicate auto-exposure setup Maxime Ripard
  10 siblings, 0 replies; 17+ messages in thread
From: Maxime Ripard @ 2018-11-13 13:03 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: Laurent Pinchart, linux-media, Thomas Petazzoni,
	Mylene Josserand, Hans Verkuil, Sakari Ailus, Hugues Fruchet,
	Loic Poulain, Samuel Bobrowicz, Steve Longerbeam, Daniel Mack,
	Jacopo Mondi, Maxime Ripard

The current code uses an algorithm to clamp the FPS values and round them
to the closest supported one that isn't really allows to be extended to
more than two values.

Rework it a bit to make it much easier to extend the amount of FPS options
we support.

Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
 drivers/media/i2c/ov5640.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index fc2e03193da6..dd6a07a8a4e5 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -2053,7 +2053,8 @@ static int ov5640_try_frame_interval(struct ov5640_dev *sensor,
 {
 	const struct ov5640_mode_info *mode;
 	enum ov5640_frame_rate rate = OV5640_30_FPS;
-	u32 minfps, maxfps, fps;
+	int minfps, maxfps, best_fps, fps;
+	int i;
 
 	minfps = ov5640_framerates[OV5640_15_FPS];
 	maxfps = ov5640_framerates[OV5640_30_FPS];
@@ -2064,19 +2065,21 @@ static int ov5640_try_frame_interval(struct ov5640_dev *sensor,
 		return OV5640_30_FPS;
 	}
 
-	fps = DIV_ROUND_CLOSEST(fi->denominator, fi->numerator);
+	fps = clamp_val(DIV_ROUND_CLOSEST(fi->denominator, fi->numerator),
+			minfps, maxfps);
+
+	best_fps = minfps;
+	for (i = 0; i < ARRAY_SIZE(ov5640_framerates); i++) {
+		int curr_fps = ov5640_framerates[i];
+
+		if (abs(curr_fps - fps) < abs(best_fps - fps)) {
+			best_fps = curr_fps;
+			rate = i;
+		}
+	}
 
 	fi->numerator = 1;
-	if (fps > maxfps)
-		fi->denominator = maxfps;
-	else if (fps < minfps)
-		fi->denominator = minfps;
-	else if (2 * fps >= 2 * minfps + (maxfps - minfps))
-		fi->denominator = maxfps;
-	else
-		fi->denominator = minfps;
-
-	rate = (fi->denominator == minfps) ? OV5640_15_FPS : OV5640_30_FPS;
+	fi->denominator = best_fps;
 
 	mode = ov5640_find_mode(sensor, rate, width, height, false);
 	return mode ? rate : -EINVAL;
-- 
2.19.1

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v5 10/11] media: ov5640: Add 60 fps support
  2018-11-13 13:03 [PATCH v5 00/11] media: ov5640: Misc cleanup and improvements Maxime Ripard
                   ` (8 preceding siblings ...)
  2018-11-13 13:03 ` [PATCH v5 09/11] media: ov5640: Make the FPS clamping / rounding more extendable Maxime Ripard
@ 2018-11-13 13:03 ` Maxime Ripard
  2018-11-13 13:03 ` [PATCH v5 11/11] media: ov5640: Remove duplicate auto-exposure setup Maxime Ripard
  10 siblings, 0 replies; 17+ messages in thread
From: Maxime Ripard @ 2018-11-13 13:03 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: Laurent Pinchart, linux-media, Thomas Petazzoni,
	Mylene Josserand, Hans Verkuil, Sakari Ailus, Hugues Fruchet,
	Loic Poulain, Samuel Bobrowicz, Steve Longerbeam, Daniel Mack,
	Jacopo Mondi, Maxime Ripard

Now that we have everything in place to compute the clock rate at runtime,
we can enable the 60fps framerate for the mode we tested it with.

Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
 drivers/media/i2c/ov5640.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index dd6a07a8a4e5..7fa508f61dc6 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -110,6 +110,7 @@ enum ov5640_mode_id {
 enum ov5640_frame_rate {
 	OV5640_15_FPS = 0,
 	OV5640_30_FPS,
+	OV5640_60_FPS,
 	OV5640_NUM_FRAMERATES,
 };
 
@@ -138,6 +139,7 @@ MODULE_PARM_DESC(virtual_channel,
 static const int ov5640_framerates[] = {
 	[OV5640_15_FPS] = 15,
 	[OV5640_30_FPS] = 30,
+	[OV5640_60_FPS] = 60,
 };
 
 /* regulator supplies */
@@ -1562,6 +1564,11 @@ ov5640_find_mode(struct ov5640_dev *sensor, enum ov5640_frame_rate fr,
 	    (!nearest && (mode->hact != width || mode->vact != height)))
 		return NULL;
 
+	/* Only 640x480 can operate at 60fps (for now) */
+	if (fr == OV5640_60_FPS &&
+	    !(mode->hact == 640 && mode->vact == 480))
+		return NULL;
+
 	return mode;
 }
 
@@ -2057,12 +2064,13 @@ static int ov5640_try_frame_interval(struct ov5640_dev *sensor,
 	int i;
 
 	minfps = ov5640_framerates[OV5640_15_FPS];
-	maxfps = ov5640_framerates[OV5640_30_FPS];
+	maxfps = ov5640_framerates[OV5640_60_FPS];
 
 	if (fi->numerator == 0) {
 		fi->denominator = maxfps;
 		fi->numerator = 1;
-		return OV5640_30_FPS;
+		rate = OV5640_60_FPS;
+		goto find_mode;
 	}
 
 	fps = clamp_val(DIV_ROUND_CLOSEST(fi->denominator, fi->numerator),
@@ -2081,6 +2089,7 @@ static int ov5640_try_frame_interval(struct ov5640_dev *sensor,
 	fi->numerator = 1;
 	fi->denominator = best_fps;
 
+find_mode:
 	mode = ov5640_find_mode(sensor, rate, width, height, false);
 	return mode ? rate : -EINVAL;
 }
@@ -2700,8 +2709,11 @@ static int ov5640_s_frame_interval(struct v4l2_subdev *sd,
 
 	frame_rate = ov5640_try_frame_interval(sensor, &fi->interval,
 					       mode->hact, mode->vact);
-	if (frame_rate < 0)
-		frame_rate = OV5640_15_FPS;
+	if (frame_rate < 0) {
+		/* Always return a valid frame interval value */
+		fi->interval = sensor->frame_interval;
+		goto out;
+	}
 
 	mode = ov5640_find_mode(sensor, frame_rate, mode->hact,
 				mode->vact, true);
-- 
2.19.1

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v5 11/11] media: ov5640: Remove duplicate auto-exposure setup
  2018-11-13 13:03 [PATCH v5 00/11] media: ov5640: Misc cleanup and improvements Maxime Ripard
                   ` (9 preceding siblings ...)
  2018-11-13 13:03 ` [PATCH v5 10/11] media: ov5640: Add 60 fps support Maxime Ripard
@ 2018-11-13 13:03 ` Maxime Ripard
  10 siblings, 0 replies; 17+ messages in thread
From: Maxime Ripard @ 2018-11-13 13:03 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: Laurent Pinchart, linux-media, Thomas Petazzoni,
	Mylene Josserand, Hans Verkuil, Sakari Ailus, Hugues Fruchet,
	Loic Poulain, Samuel Bobrowicz, Steve Longerbeam, Daniel Mack,
	Jacopo Mondi, Maxime Ripard

The autoexposure setup in the 1080p init array is redundant with the
default value of the sensor.

Remove it.

Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
 drivers/media/i2c/ov5640.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 7fa508f61dc6..0bb5f78571fe 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -504,7 +504,7 @@ static const struct reg_value ov5640_setting_1080P_1920_1080[] = {
 	{0x3a0e, 0x03, 0, 0}, {0x3a0d, 0x04, 0, 0}, {0x3a14, 0x04, 0, 0},
 	{0x3a15, 0x60, 0, 0}, {0x4713, 0x02, 0, 0}, {0x4407, 0x04, 0, 0},
 	{0x460b, 0x37, 0, 0}, {0x460c, 0x20, 0, 0}, {0x3824, 0x04, 0, 0},
-	{0x4005, 0x1a, 0, 0}, {0x3008, 0x02, 0, 0}, {0x3503, 0, 0, 0},
+	{0x4005, 0x1a, 0, 0}, {0x3008, 0x02, 0, 0},
 };
 
 static const struct reg_value ov5640_setting_QSXGA_2592_1944[] = {
-- 
2.19.1

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH v5 01/11] media: ov5640: Adjust the clock based on the expected rate
  2018-11-13 13:03 ` [PATCH v5 01/11] media: ov5640: Adjust the clock based on the expected rate Maxime Ripard
@ 2018-11-14 13:20   ` Adam Ford
  2018-11-19 15:03     ` Adam Ford
  2018-11-14 19:48   ` jacopo mondi
  1 sibling, 1 reply; 17+ messages in thread
From: Adam Ford @ 2018-11-14 13:20 UTC (permalink / raw)
  To: maxime.ripard
  Cc: mchehab, Laurent Pinchart, linux-media, thomas.petazzoni,
	mylene.josserand, hans.verkuil, sakari.ailus, hugues.fruchet,
	loic.poulain, sam, slongerbeam, daniel, jacopo

On Tue, Nov 13, 2018 at 7:04 AM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
>
> The clock structure for the PCLK is quite obscure in the documentation, and
> was hardcoded through the bytes array of each and every mode.
>
> This is troublesome, since we cannot adjust it at runtime based on other
> parameters (such as the number of bytes per pixel), and we can't support
> either framerates that have not been used by the various vendors, since we
> don't have the needed initialization sequence.
>
> We can however understand how the clock tree works, and then implement some
> functions to derive the various parameters from a given rate. And now that
> those parameters are calculated at runtime, we can remove them from the
> initialization sequence.
>
> The modes also gained a new parameter which is the clock that they are
> running at, from the register writes they were doing, so for now the switch
> to the new algorithm should be transparent.
>
> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>

Thanks for the patches! I tested the whole series.  I am stil learning
the v4l2 stuff, but I'm trying to test what and where I can.
media-ctl shows the camera is talking at 60fps, but my imx6 is only
capturing at 30, but I don't think it's the fault of the ov5640
driver.

Tested-by: Adam Ford <aford173@gmail.com> #imx6dq

adam
> ---
>  drivers/media/i2c/ov5640.c | 366 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 365 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index eaefdb58653b..8476f85bb8e7 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -175,6 +175,7 @@ struct ov5640_mode_info {
>         u32 htot;
>         u32 vact;
>         u32 vtot;
> +       u32 pixel_clock;
>         const struct reg_value *reg_data;
>         u32 reg_data_size;
>  };
> @@ -700,6 +701,7 @@ static const struct reg_value ov5640_setting_15fps_QSXGA_2592_1944[] = {
>  /* power-on sensor init reg table */
>  static const struct ov5640_mode_info ov5640_mode_init_data = {
>         0, SUBSAMPLING, 640, 1896, 480, 984,
> +       56000000,
>         ov5640_init_setting_30fps_VGA,
>         ARRAY_SIZE(ov5640_init_setting_30fps_VGA),
>  };
> @@ -709,74 +711,91 @@ ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES] = {
>         {
>                 {OV5640_MODE_QCIF_176_144, SUBSAMPLING,
>                  176, 1896, 144, 984,
> +                28000000,
>                  ov5640_setting_15fps_QCIF_176_144,
>                  ARRAY_SIZE(ov5640_setting_15fps_QCIF_176_144)},
>                 {OV5640_MODE_QVGA_320_240, SUBSAMPLING,
>                  320, 1896, 240, 984,
> +                28000000,
>                  ov5640_setting_15fps_QVGA_320_240,
>                  ARRAY_SIZE(ov5640_setting_15fps_QVGA_320_240)},
>                 {OV5640_MODE_VGA_640_480, SUBSAMPLING,
>                  640, 1896, 480, 1080,
> +                28000000,
>                  ov5640_setting_15fps_VGA_640_480,
>                  ARRAY_SIZE(ov5640_setting_15fps_VGA_640_480)},
>                 {OV5640_MODE_NTSC_720_480, SUBSAMPLING,
>                  720, 1896, 480, 984,
> +                28000000,
>                  ov5640_setting_15fps_NTSC_720_480,
>                  ARRAY_SIZE(ov5640_setting_15fps_NTSC_720_480)},
>                 {OV5640_MODE_PAL_720_576, SUBSAMPLING,
>                  720, 1896, 576, 984,
> +                28000000,
>                  ov5640_setting_15fps_PAL_720_576,
>                  ARRAY_SIZE(ov5640_setting_15fps_PAL_720_576)},
>                 {OV5640_MODE_XGA_1024_768, SUBSAMPLING,
>                  1024, 1896, 768, 1080,
> +                28000000,
>                  ov5640_setting_15fps_XGA_1024_768,
>                  ARRAY_SIZE(ov5640_setting_15fps_XGA_1024_768)},
>                 {OV5640_MODE_720P_1280_720, SUBSAMPLING,
>                  1280, 1892, 720, 740,
> +                21000000,
>                  ov5640_setting_15fps_720P_1280_720,
>                  ARRAY_SIZE(ov5640_setting_15fps_720P_1280_720)},
>                 {OV5640_MODE_1080P_1920_1080, SCALING,
>                  1920, 2500, 1080, 1120,
> +                42000000,
>                  ov5640_setting_15fps_1080P_1920_1080,
>                  ARRAY_SIZE(ov5640_setting_15fps_1080P_1920_1080)},
>                 {OV5640_MODE_QSXGA_2592_1944, SCALING,
>                  2592, 2844, 1944, 1968,
> +                84000000,
>                  ov5640_setting_15fps_QSXGA_2592_1944,
>                  ARRAY_SIZE(ov5640_setting_15fps_QSXGA_2592_1944)},
>         }, {
>                 {OV5640_MODE_QCIF_176_144, SUBSAMPLING,
>                  176, 1896, 144, 984,
> +                56000000,
>                  ov5640_setting_30fps_QCIF_176_144,
>                  ARRAY_SIZE(ov5640_setting_30fps_QCIF_176_144)},
>                 {OV5640_MODE_QVGA_320_240, SUBSAMPLING,
>                  320, 1896, 240, 984,
> +                56000000,
>                  ov5640_setting_30fps_QVGA_320_240,
>                  ARRAY_SIZE(ov5640_setting_30fps_QVGA_320_240)},
>                 {OV5640_MODE_VGA_640_480, SUBSAMPLING,
>                  640, 1896, 480, 1080,
> +                56000000,
>                  ov5640_setting_30fps_VGA_640_480,
>                  ARRAY_SIZE(ov5640_setting_30fps_VGA_640_480)},
>                 {OV5640_MODE_NTSC_720_480, SUBSAMPLING,
>                  720, 1896, 480, 984,
> +                56000000,
>                  ov5640_setting_30fps_NTSC_720_480,
>                  ARRAY_SIZE(ov5640_setting_30fps_NTSC_720_480)},
>                 {OV5640_MODE_PAL_720_576, SUBSAMPLING,
>                  720, 1896, 576, 984,
> +                56000000,
>                  ov5640_setting_30fps_PAL_720_576,
>                  ARRAY_SIZE(ov5640_setting_30fps_PAL_720_576)},
>                 {OV5640_MODE_XGA_1024_768, SUBSAMPLING,
>                  1024, 1896, 768, 1080,
> +                56000000,
>                  ov5640_setting_30fps_XGA_1024_768,
>                  ARRAY_SIZE(ov5640_setting_30fps_XGA_1024_768)},
>                 {OV5640_MODE_720P_1280_720, SUBSAMPLING,
>                  1280, 1892, 720, 740,
> +                42000000,
>                  ov5640_setting_30fps_720P_1280_720,
>                  ARRAY_SIZE(ov5640_setting_30fps_720P_1280_720)},
>                 {OV5640_MODE_1080P_1920_1080, SCALING,
>                  1920, 2500, 1080, 1120,
> +                84000000,
>                  ov5640_setting_30fps_1080P_1920_1080,
>                  ARRAY_SIZE(ov5640_setting_30fps_1080P_1920_1080)},
> -               {OV5640_MODE_QSXGA_2592_1944, -1, 0, 0, 0, 0, NULL, 0},
> +               {OV5640_MODE_QSXGA_2592_1944, -1, 0, 0, 0, 0, 0, NULL, 0},
>         },
>  };
>
> @@ -909,6 +928,334 @@ static int ov5640_mod_reg(struct ov5640_dev *sensor, u16 reg,
>         return ov5640_write_reg(sensor, reg, val);
>  }
>
> +/*
> + * After trying the various combinations, reading various
> + * documentations spreaded around the net, and from the various
> + * feedback, the clock tree is probably as follows:
> + *
> + *   +--------------+
> + *   |  Ext. Clock  |
> + *   +-+------------+
> + *     |  +----------+
> + *     +->|   PLL1   | - reg 0x3036, for the multiplier
> + *        +-+--------+ - reg 0x3037, bits 0-3 for the pre-divider
> + *          |  +--------------+
> + *          +->| System Clock |  - reg 0x3035, bits 4-7
> + *             +-+------------+
> + *               |  +--------------+
> + *               +->| MIPI Divider | - reg 0x3035, bits 0-3
> + *               |  +-+------------+
> + *               |    +----------------> MIPI SCLK
> + *               |    +  +-----+
> + *               |    +->| / 2 |-------> MIPI BIT CLK
> + *               |       +-----+
> + *               |  +--------------+
> + *               +->| PLL Root Div | - reg 0x3037, bit 4
> + *                  +-+------------+
> + *                    |  +---------+
> + *                    +->| Bit Div | - reg 0x3035, bits 0-3
> + *                       +-+-------+
> + *                         |  +-------------+
> + *                         +->| SCLK Div    | - reg 0x3108, bits 0-1
> + *                         |  +-+-----------+
> + *                         |    +---------------> SCLK
> + *                         |  +-------------+
> + *                         +->| SCLK 2X Div | - reg 0x3108, bits 2-3
> + *                         |  +-+-----------+
> + *                         |    +---------------> SCLK 2X
> + *                         |  +-------------+
> + *                         +->| PCLK Div    | - reg 0x3108, bits 4-5
> + *                            +-+-----------+
> + *                              +---------------> PCLK
> + *
> + * This is deviating from the datasheet at least for the register
> + * 0x3108, since it's said here that the PCLK would be clocked from
> + * the PLL.
> + *
> + * There seems to be also (unverified) constraints:
> + *  - the PLL pre-divider output rate should be in the 4-27MHz range
> + *  - the PLL multiplier output rate should be in the 500-1000MHz range
> + *  - PCLK >= SCLK * 2 in YUV, >= SCLK in Raw or JPEG
> + *  - MIPI SCLK = (bpp / lanes) / PCLK
> + *
> + * In the two latter cases, these constraints are met since our
> + * factors are hardcoded. If we were to change that, we would need to
> + * take this into account. The only varying parts are the PLL
> + * multiplier and the system clock divider, which are shared between
> + * all these clocks so won't cause any issue.
> + */
> +
> +/*
> + * This is supposed to be ranging from 1 to 8, but the value is always
> + * set to 3 in the vendor kernels.
> + */
> +#define OV5640_PLL_PREDIV      3
> +
> +#define OV5640_PLL_MULT_MIN    4
> +#define OV5640_PLL_MULT_MAX    252
> +
> +/*
> + * This is supposed to be ranging from 1 to 16, but the value is
> + * always set to either 1 or 2 in the vendor kernels.
> + */
> +#define OV5640_SYSDIV_MIN      1
> +#define OV5640_SYSDIV_MAX      16
> +
> +/*
> + * This is supposed to be ranging from 1 to 16, but the value is always
> + * set to 2 in the vendor kernels.
> + */
> +#define OV5640_MIPI_DIV                2
> +
> +/*
> + * 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_CTRL3_PLL_ROOT_DIV_2                BIT(4)
> +
> +/*
> + * We only supports 8-bit formats at the moment
> + */
> +#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
> + * set to 2 in the vendor kernels.
> + */
> +#define OV5640_SCLK_ROOT_DIV   2
> +
> +/*
> + * This is hardcoded so that the consistency is maintained between SCLK and
> + * SCLK 2x.
> + */
> +#define OV5640_SCLK2X_ROOT_DIV (OV5640_SCLK_ROOT_DIV / 2)
> +
> +/*
> + * 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_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 sysclk = sensor->xclk_freq / pll_prediv * pll_mult;
> +
> +       /* 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,
> +                                        unsigned long rate,
> +                                        u8 *pll_prediv, u8 *pll_mult,
> +                                        u8 *sysdiv)
> +{
> +       unsigned long best = ~0;
> +       u8 best_sysdiv = 1, best_mult = 1;
> +       u8 _sysdiv, _pll_mult;
> +
> +       for (_sysdiv = OV5640_SYSDIV_MIN;
> +            _sysdiv <= OV5640_SYSDIV_MAX;
> +            _sysdiv++) {
> +               for (_pll_mult = OV5640_PLL_MULT_MIN;
> +                    _pll_mult <= OV5640_PLL_MULT_MAX;
> +                    _pll_mult++) {
> +                       unsigned long _rate;
> +
> +                       /*
> +                        * The PLL multiplier cannot be odd if above
> +                        * 127.
> +                        */
> +                       if (_pll_mult > 127 && (_pll_mult % 2))
> +                               continue;
> +
> +                       _rate = ov5640_compute_sys_clk(sensor,
> +                                                      OV5640_PLL_PREDIV,
> +                                                      _pll_mult, _sysdiv);
> +
> +                       /*
> +                        * We have reached the maximum allowed PLL1 output,
> +                        * increase sysdiv.
> +                        */
> +                       if (!rate)
> +                               break;
> +
> +                       /*
> +                        * Prefer rates above the expected clock rate than
> +                        * below, even if that means being less precise.
> +                        */
> +                       if (_rate < rate)
> +                               continue;
> +
> +                       if (abs(rate - _rate) < abs(rate - best)) {
> +                               best = _rate;
> +                               best_sysdiv = _sysdiv;
> +                               best_mult = _pll_mult;
> +                       }
> +
> +                       if (_rate == rate)
> +                               goto out;
> +               }
> +       }
> +
> +out:
> +       *sysdiv = best_sysdiv;
> +       *pll_prediv = OV5640_PLL_PREDIV;
> +       *pll_mult = best_mult;
> +
> +       return best;
> +}
> +
> +/*
> + * 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)
> +{
> +       const struct ov5640_mode_info *mode = sensor->current_mode;
> +       u8 mipi_div = OV5640_MIPI_DIV;
> +       u8 prediv, mult, sysdiv;
> +       int ret;
> +
> +       /* 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;
> +
> +       /* 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;
> +
> +       ov5640_calc_sys_clk(sensor, rate, &prediv, &mult, &sysdiv);
> +
> +       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);
> +       if (ret)
> +               return ret;
> +
> +       ret = ov5640_mod_reg(sensor, OV5640_REG_SC_PLL_CTRL2, 0xff, mult);
> +       if (ret)
> +               return ret;
> +
> +       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,
> +                                     unsigned long rate,
> +                                     u8 *pll_prediv, u8 *pll_mult, u8 *sysdiv,
> +                                     u8 *pll_rdiv, u8 *bit_div, u8 *pclk_div)
> +{
> +       unsigned long _rate = rate * OV5640_PLL_ROOT_DIV * OV5640_BIT_DIV *
> +                               OV5640_PCLK_ROOT_DIV;
> +
> +       _rate = ov5640_calc_sys_clk(sensor, _rate, pll_prediv, pll_mult,
> +                                   sysdiv);
> +       *pll_rdiv = OV5640_PLL_ROOT_DIV;
> +       *bit_div = OV5640_BIT_DIV;
> +       *pclk_div = OV5640_PCLK_ROOT_DIV;
> +
> +       return _rate / *pll_rdiv / *bit_div / *pclk_div;
> +}
> +
> +static int ov5640_set_dvp_pclk(struct ov5640_dev *sensor, unsigned long rate)
> +{
> +       u8 prediv, mult, sysdiv, pll_rdiv, bit_div, pclk_div;
> +       int ret;
> +
> +       ov5640_calc_pclk(sensor, rate, &prediv, &mult, &sysdiv, &pll_rdiv,
> +                        &bit_div, &pclk_div);
> +
> +       if (bit_div == 2)
> +               bit_div = 8;
> +
> +       ret = ov5640_mod_reg(sensor, OV5640_REG_SC_PLL_CTRL0,
> +                            0x0f, bit_div);
> +       if (ret)
> +               return ret;
> +
> +       /*
> +        * We need to set sysdiv according to the clock, and to clear
> +        * the MIPI divider.
> +        */
> +       ret = ov5640_mod_reg(sensor, OV5640_REG_SC_PLL_CTRL1,
> +                            0xff, sysdiv << 4);
> +       if (ret)
> +               return ret;
> +
> +       ret = ov5640_mod_reg(sensor, OV5640_REG_SC_PLL_CTRL2,
> +                            0xff, mult);
> +       if (ret)
> +               return ret;
> +
> +       ret = ov5640_mod_reg(sensor, OV5640_REG_SC_PLL_CTRL3,
> +                            0x1f, prediv | ((pll_rdiv - 1) << 4));
> +       if (ret)
> +               return ret;
> +
> +       return ov5640_mod_reg(sensor, OV5640_REG_SYS_ROOT_DIVIDER, 0x30,
> +                             (ilog2(pclk_div) << 4));
> +}
> +
>  /* download ov5640 settings to sensor through i2c */
>  static int ov5640_set_timings(struct ov5640_dev *sensor,
>                               const struct ov5640_mode_info *mode)
> @@ -1637,6 +1984,7 @@ static int ov5640_set_mode(struct ov5640_dev *sensor)
>         enum ov5640_downsize_mode dn_mode, orig_dn_mode;
>         bool auto_gain = sensor->ctrls.auto_gain->val == 1;
>         bool auto_exp =  sensor->ctrls.auto_exp->val == V4L2_EXPOSURE_AUTO;
> +       unsigned long rate;
>         int ret;
>
>         dn_mode = mode->dn_mode;
> @@ -1655,6 +2003,22 @@ static int ov5640_set_mode(struct ov5640_dev *sensor)
>                         goto restore_auto_gain;
>         }
>
> +       /*
> +        * All the formats we support have 16 bits per pixel, seems to require
> +        * the same rate than YUV, so we can just use 16 bpp all the time.
> +        */
> +       rate = mode->pixel_clock * 16;
> +       if (sensor->ep.bus_type == V4L2_MBUS_CSI2_DPHY) {
> +               rate = rate / sensor->ep.bus.mipi_csi2.num_data_lanes;
> +               ret = ov5640_set_mipi_pclk(sensor, rate);
> +       } else {
> +               rate = rate / sensor->ep.bus.parallel.bus_width;
> +               ret = ov5640_set_dvp_pclk(sensor, rate);
> +       }
> +
> +       if (ret < 0)
> +               return 0;
> +
>         if ((dn_mode == SUBSAMPLING && orig_dn_mode == SCALING) ||
>             (dn_mode == SCALING && orig_dn_mode == SUBSAMPLING)) {
>                 /*
> --
> 2.19.1
>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v5 01/11] media: ov5640: Adjust the clock based on the expected rate
  2018-11-13 13:03 ` [PATCH v5 01/11] media: ov5640: Adjust the clock based on the expected rate Maxime Ripard
  2018-11-14 13:20   ` Adam Ford
@ 2018-11-14 19:48   ` jacopo mondi
  2018-11-20  9:48     ` Maxime Ripard
  1 sibling, 1 reply; 17+ messages in thread
From: jacopo mondi @ 2018-11-14 19:48 UTC (permalink / raw)
  To: Maxime Ripard
  Cc: Mauro Carvalho Chehab, Laurent Pinchart, linux-media,
	Thomas Petazzoni, Mylene Josserand, Hans Verkuil, Sakari Ailus,
	Hugues Fruchet, Loic Poulain, Samuel Bobrowicz, Steve Longerbeam,
	Daniel Mack

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

Hi Maxime,
   many thanks for re-sending this updated version

On Tue, Nov 13, 2018 at 02:03:15PM +0100, Maxime Ripard wrote:
> The clock structure for the PCLK is quite obscure in the documentation, and
> was hardcoded through the bytes array of each and every mode.
>
> This is troublesome, since we cannot adjust it at runtime based on other
> parameters (such as the number of bytes per pixel), and we can't support
> either framerates that have not been used by the various vendors, since we
> don't have the needed initialization sequence.
>
> We can however understand how the clock tree works, and then implement some
> functions to derive the various parameters from a given rate. And now that
> those parameters are calculated at runtime, we can remove them from the
> initialization sequence.
>
> The modes also gained a new parameter which is the clock that they are
> running at, from the register writes they were doing, so for now the switch
> to the new algorithm should be transparent.
>
> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>

As you've squashed in my MIPI CSI-2 fixes, do you think it is
appropriate adding my So-b tag here?

> ---
>  drivers/media/i2c/ov5640.c | 366 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 365 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index eaefdb58653b..8476f85bb8e7 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -175,6 +175,7 @@ struct ov5640_mode_info {
>  	u32 htot;
>  	u32 vact;
>  	u32 vtot;
> +	u32 pixel_clock;
>  	const struct reg_value *reg_data;
>  	u32 reg_data_size;
>  };
> @@ -700,6 +701,7 @@ static const struct reg_value ov5640_setting_15fps_QSXGA_2592_1944[] = {
>  /* power-on sensor init reg table */
>  static const struct ov5640_mode_info ov5640_mode_init_data = {
>  	0, SUBSAMPLING, 640, 1896, 480, 984,
> +	56000000,
>  	ov5640_init_setting_30fps_VGA,
>  	ARRAY_SIZE(ov5640_init_setting_30fps_VGA),
>  };
> @@ -709,74 +711,91 @@ ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES] = {
>  	{
>  		{OV5640_MODE_QCIF_176_144, SUBSAMPLING,
>  		 176, 1896, 144, 984,
> +		 28000000,
>  		 ov5640_setting_15fps_QCIF_176_144,
>  		 ARRAY_SIZE(ov5640_setting_15fps_QCIF_176_144)},
>  		{OV5640_MODE_QVGA_320_240, SUBSAMPLING,
>  		 320, 1896, 240, 984,
> +		 28000000,
>  		 ov5640_setting_15fps_QVGA_320_240,
>  		 ARRAY_SIZE(ov5640_setting_15fps_QVGA_320_240)},
>  		{OV5640_MODE_VGA_640_480, SUBSAMPLING,
>  		 640, 1896, 480, 1080,
> +		 28000000,
>  		 ov5640_setting_15fps_VGA_640_480,
>  		 ARRAY_SIZE(ov5640_setting_15fps_VGA_640_480)},
>  		{OV5640_MODE_NTSC_720_480, SUBSAMPLING,
>  		 720, 1896, 480, 984,
> +		 28000000,
>  		 ov5640_setting_15fps_NTSC_720_480,
>  		 ARRAY_SIZE(ov5640_setting_15fps_NTSC_720_480)},
>  		{OV5640_MODE_PAL_720_576, SUBSAMPLING,
>  		 720, 1896, 576, 984,
> +		 28000000,
>  		 ov5640_setting_15fps_PAL_720_576,
>  		 ARRAY_SIZE(ov5640_setting_15fps_PAL_720_576)},
>  		{OV5640_MODE_XGA_1024_768, SUBSAMPLING,
>  		 1024, 1896, 768, 1080,
> +		 28000000,
>  		 ov5640_setting_15fps_XGA_1024_768,
>  		 ARRAY_SIZE(ov5640_setting_15fps_XGA_1024_768)},
>  		{OV5640_MODE_720P_1280_720, SUBSAMPLING,
>  		 1280, 1892, 720, 740,
> +		 21000000,
>  		 ov5640_setting_15fps_720P_1280_720,
>  		 ARRAY_SIZE(ov5640_setting_15fps_720P_1280_720)},
>  		{OV5640_MODE_1080P_1920_1080, SCALING,
>  		 1920, 2500, 1080, 1120,
> +		 42000000,
>  		 ov5640_setting_15fps_1080P_1920_1080,
>  		 ARRAY_SIZE(ov5640_setting_15fps_1080P_1920_1080)},
>  		{OV5640_MODE_QSXGA_2592_1944, SCALING,
>  		 2592, 2844, 1944, 1968,
> +		 84000000,
>  		 ov5640_setting_15fps_QSXGA_2592_1944,
>  		 ARRAY_SIZE(ov5640_setting_15fps_QSXGA_2592_1944)},
>  	}, {
>  		{OV5640_MODE_QCIF_176_144, SUBSAMPLING,
>  		 176, 1896, 144, 984,
> +		 56000000,
>  		 ov5640_setting_30fps_QCIF_176_144,
>  		 ARRAY_SIZE(ov5640_setting_30fps_QCIF_176_144)},
>  		{OV5640_MODE_QVGA_320_240, SUBSAMPLING,
>  		 320, 1896, 240, 984,
> +		 56000000,
>  		 ov5640_setting_30fps_QVGA_320_240,
>  		 ARRAY_SIZE(ov5640_setting_30fps_QVGA_320_240)},
>  		{OV5640_MODE_VGA_640_480, SUBSAMPLING,
>  		 640, 1896, 480, 1080,
> +		 56000000,
>  		 ov5640_setting_30fps_VGA_640_480,
>  		 ARRAY_SIZE(ov5640_setting_30fps_VGA_640_480)},
>  		{OV5640_MODE_NTSC_720_480, SUBSAMPLING,
>  		 720, 1896, 480, 984,
> +		 56000000,
>  		 ov5640_setting_30fps_NTSC_720_480,
>  		 ARRAY_SIZE(ov5640_setting_30fps_NTSC_720_480)},
>  		{OV5640_MODE_PAL_720_576, SUBSAMPLING,
>  		 720, 1896, 576, 984,
> +		 56000000,
>  		 ov5640_setting_30fps_PAL_720_576,
>  		 ARRAY_SIZE(ov5640_setting_30fps_PAL_720_576)},
>  		{OV5640_MODE_XGA_1024_768, SUBSAMPLING,
>  		 1024, 1896, 768, 1080,
> +		 56000000,
>  		 ov5640_setting_30fps_XGA_1024_768,
>  		 ARRAY_SIZE(ov5640_setting_30fps_XGA_1024_768)},
>  		{OV5640_MODE_720P_1280_720, SUBSAMPLING,
>  		 1280, 1892, 720, 740,
> +		 42000000,
>  		 ov5640_setting_30fps_720P_1280_720,
>  		 ARRAY_SIZE(ov5640_setting_30fps_720P_1280_720)},
>  		{OV5640_MODE_1080P_1920_1080, SCALING,
>  		 1920, 2500, 1080, 1120,
> +		 84000000,
>  		 ov5640_setting_30fps_1080P_1920_1080,
>  		 ARRAY_SIZE(ov5640_setting_30fps_1080P_1920_1080)},
> -		{OV5640_MODE_QSXGA_2592_1944, -1, 0, 0, 0, 0, NULL, 0},
> +		{OV5640_MODE_QSXGA_2592_1944, -1, 0, 0, 0, 0, 0, NULL, 0},
>  	},
>  };
>
> @@ -909,6 +928,334 @@ static int ov5640_mod_reg(struct ov5640_dev *sensor, u16 reg,
>  	return ov5640_write_reg(sensor, reg, val);
>  }
>
> +/*
> + * After trying the various combinations, reading various
> + * documentations spreaded around the net, and from the various
> + * feedback, the clock tree is probably as follows:
> + *
> + *   +--------------+
> + *   |  Ext. Clock  |
> + *   +-+------------+
> + *     |  +----------+
> + *     +->|   PLL1   | - reg 0x3036, for the multiplier
> + *        +-+--------+ - reg 0x3037, bits 0-3 for the pre-divider
> + *          |  +--------------+
> + *          +->| System Clock |  - reg 0x3035, bits 4-7
> + *             +-+------------+
> + *               |  +--------------+
> + *               +->| MIPI Divider | - reg 0x3035, bits 0-3
> + *               |  +-+------------+
> + *               |    +----------------> MIPI SCLK
> + *               |    +  +-----+
> + *               |    +->| / 2 |-------> MIPI BIT CLK
> + *               |       +-----+
> + *               |  +--------------+
> + *               +->| PLL Root Div | - reg 0x3037, bit 4
> + *                  +-+------------+
> + *                    |  +---------+
> + *                    +->| Bit Div | - reg 0x3035, bits 0-3
> + *                       +-+-------+
> + *                         |  +-------------+
> + *                         +->| SCLK Div    | - reg 0x3108, bits 0-1
> + *                         |  +-+-----------+
> + *                         |    +---------------> SCLK
> + *                         |  +-------------+
> + *                         +->| SCLK 2X Div | - reg 0x3108, bits 2-3
> + *                         |  +-+-----------+
> + *                         |    +---------------> SCLK 2X
> + *                         |  +-------------+
> + *                         +->| PCLK Div    | - reg 0x3108, bits 4-5
> + *                            +-+-----------+
> + *                              +---------------> PCLK
> + *
> + * This is deviating from the datasheet at least for the register
> + * 0x3108, since it's said here that the PCLK would be clocked from
> + * the PLL.
> + *
> + * There seems to be also (unverified) constraints:
> + *  - the PLL pre-divider output rate should be in the 4-27MHz range
> + *  - the PLL multiplier output rate should be in the 500-1000MHz range
> + *  - PCLK >= SCLK * 2 in YUV, >= SCLK in Raw or JPEG
> + *  - MIPI SCLK = (bpp / lanes) / PCLK
> + *
> + * In the two latter cases, these constraints are met since our
> + * factors are hardcoded. If we were to change that, we would need to
> + * take this into account. The only varying parts are the PLL
> + * multiplier and the system clock divider, which are shared between
> + * all these clocks so won't cause any issue.
> + */
> +
> +/*
> + * This is supposed to be ranging from 1 to 8, but the value is always
> + * set to 3 in the vendor kernels.
> + */
> +#define OV5640_PLL_PREDIV	3
> +
> +#define OV5640_PLL_MULT_MIN	4
> +#define OV5640_PLL_MULT_MAX	252
> +
> +/*
> + * This is supposed to be ranging from 1 to 16, but the value is
> + * always set to either 1 or 2 in the vendor kernels.
> + */

I forgot to fix this comment in my patches you squahed in here (the
value now ranges from 1 to 16)

> +#define OV5640_SYSDIV_MIN	1
> +#define OV5640_SYSDIV_MAX	16
> +
> +/*
> + * This is supposed to be ranging from 1 to 16, but the value is always
> + * set to 2 in the vendor kernels.
> + */
> +#define OV5640_MIPI_DIV		2
> +
> +/*
> + * 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_CTRL3_PLL_ROOT_DIV_2		BIT(4)
> +
> +/*
> + * We only supports 8-bit formats at the moment
> + */
> +#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
> + * set to 2 in the vendor kernels.
> + */
> +#define OV5640_SCLK_ROOT_DIV	2
> +
> +/*
> + * This is hardcoded so that the consistency is maintained between SCLK and
> + * SCLK 2x.
> + */
> +#define OV5640_SCLK2X_ROOT_DIV (OV5640_SCLK_ROOT_DIV / 2)
> +
> +/*
> + * 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_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 sysclk = sensor->xclk_freq / pll_prediv * pll_mult;
> +
> +	/* PLL1 output cannot exceed 1GHz. */
> +	if (sysclk / 1000000 > 1000)
> +		return 0;
> +
> +	return sysclk / sysdiv;
> +}

That's better, but Sam's version is even better. I plan to pick some
part of his patch and apply them on top of this (for this and a few
other things I'm pointing out a here below). How would you like to
have those updates? Incremental patches on top of this series once it
gets merged (and it can now be merged, since it works for both CSI-2
and DVP), or would you like to receive those patches, squash them in
and re-send?

> +
> +static unsigned long ov5640_calc_sys_clk(struct ov5640_dev *sensor,
> +					 unsigned long rate,
> +					 u8 *pll_prediv, u8 *pll_mult,
> +					 u8 *sysdiv)
> +{
> +	unsigned long best = ~0;
> +	u8 best_sysdiv = 1, best_mult = 1;
> +	u8 _sysdiv, _pll_mult;
> +
> +	for (_sysdiv = OV5640_SYSDIV_MIN;
> +	     _sysdiv <= OV5640_SYSDIV_MAX;
> +	     _sysdiv++) {
> +		for (_pll_mult = OV5640_PLL_MULT_MIN;
> +		     _pll_mult <= OV5640_PLL_MULT_MAX;
> +		     _pll_mult++) {
> +			unsigned long _rate;
> +
> +			/*
> +			 * The PLL multiplier cannot be odd if above
> +			 * 127.
> +			 */
> +			if (_pll_mult > 127 && (_pll_mult % 2))
> +				continue;
> +
> +			_rate = ov5640_compute_sys_clk(sensor,
> +						       OV5640_PLL_PREDIV,
> +						       _pll_mult, _sysdiv);
> +
> +			/*
> +			 * We have reached the maximum allowed PLL1 output,
> +			 * increase sysdiv.
> +			 */
> +			if (!rate)
> +				break;
> +
> +			/*
> +			 * Prefer rates above the expected clock rate than
> +			 * below, even if that means being less precise.
> +			 */
> +			if (_rate < rate)
> +				continue;
> +
> +			if (abs(rate - _rate) < abs(rate - best)) {
> +				best = _rate;
> +				best_sysdiv = _sysdiv;
> +				best_mult = _pll_mult;
> +			}
> +
> +			if (_rate == rate)
> +				goto out;
> +		}
> +	}
> +
> +out:
> +	*sysdiv = best_sysdiv;
> +	*pll_prediv = OV5640_PLL_PREDIV;
> +	*pll_mult = best_mult;
> +
> +	return best;
> +}
> +
> +/*
> + * 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
> + *

Almost. This is actually the bandwidth per lane. My bad, I need to update
this whole comment block.

> + * 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)
> +{
> +	const struct ov5640_mode_info *mode = sensor->current_mode;
> +	u8 mipi_div = OV5640_MIPI_DIV;
> +	u8 prediv, mult, sysdiv;
> +	int ret;
> +
> +	/* 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;
> +
> +	/* 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;

Sam patches are better here. He found out the reason for this, as
'high resolutions modes' use the scaler, and a ration between pixel
clock and scaler clock has to be respected. My patches you squahsed in
here use this rather sub-optimal "hact > 1024" check, I would rather use his
"does the mode use the scaler?" way instead. That's something else
that could be squashed in a forthcoming version, or fixed with
incremental patches.

> +
> +	ov5640_calc_sys_clk(sensor, rate, &prediv, &mult, &sysdiv);
> +
> +	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);
> +	if (ret)
> +		return ret;
> +
> +	ret = ov5640_mod_reg(sensor, OV5640_REG_SC_PLL_CTRL2, 0xff, mult);
> +	if (ret)
> +		return ret;
> +
> +	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);

Again, there might be something to bring in from Sam patches here
(programming register 0x4837 with the pixel clock in particular).
Again, let me know how would you like to receive updates.

> +}
> +
> +static unsigned long ov5640_calc_pclk(struct ov5640_dev *sensor,
> +				      unsigned long rate,
> +				      u8 *pll_prediv, u8 *pll_mult, u8 *sysdiv,
> +				      u8 *pll_rdiv, u8 *bit_div, u8 *pclk_div)
> +{
> +	unsigned long _rate = rate * OV5640_PLL_ROOT_DIV * OV5640_BIT_DIV *
> +				OV5640_PCLK_ROOT_DIV;
> +
> +	_rate = ov5640_calc_sys_clk(sensor, _rate, pll_prediv, pll_mult,
> +				    sysdiv);
> +	*pll_rdiv = OV5640_PLL_ROOT_DIV;
> +	*bit_div = OV5640_BIT_DIV;
> +	*pclk_div = OV5640_PCLK_ROOT_DIV;
> +
> +	return _rate / *pll_rdiv / *bit_div / *pclk_div;
> +}

This function is now called from a single place, maybe you want to
remove it and inline its code.

> +
> +static int ov5640_set_dvp_pclk(struct ov5640_dev *sensor, unsigned long rate)
> +{
> +	u8 prediv, mult, sysdiv, pll_rdiv, bit_div, pclk_div;
> +	int ret;
> +
> +	ov5640_calc_pclk(sensor, rate, &prediv, &mult, &sysdiv, &pll_rdiv,
> +			 &bit_div, &pclk_div);
> +
> +	if (bit_div == 2)
> +		bit_div = 8;
> +
> +	ret = ov5640_mod_reg(sensor, OV5640_REG_SC_PLL_CTRL0,
> +			     0x0f, bit_div);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * We need to set sysdiv according to the clock, and to clear
> +	 * the MIPI divider.
> +	 */
> +	ret = ov5640_mod_reg(sensor, OV5640_REG_SC_PLL_CTRL1,
> +			     0xff, sysdiv << 4);
> +	if (ret)
> +		return ret;
> +
> +	ret = ov5640_mod_reg(sensor, OV5640_REG_SC_PLL_CTRL2,
> +			     0xff, mult);
> +	if (ret)
> +		return ret;
> +
> +	ret = ov5640_mod_reg(sensor, OV5640_REG_SC_PLL_CTRL3,
> +			     0x1f, prediv | ((pll_rdiv - 1) << 4));
> +	if (ret)
> +		return ret;
> +
> +	return ov5640_mod_reg(sensor, OV5640_REG_SYS_ROOT_DIVIDER, 0x30,
> +			      (ilog2(pclk_div) << 4));
> +}
> +
>  /* download ov5640 settings to sensor through i2c */
>  static int ov5640_set_timings(struct ov5640_dev *sensor,
>  			      const struct ov5640_mode_info *mode)
> @@ -1637,6 +1984,7 @@ static int ov5640_set_mode(struct ov5640_dev *sensor)
>  	enum ov5640_downsize_mode dn_mode, orig_dn_mode;
>  	bool auto_gain = sensor->ctrls.auto_gain->val == 1;
>  	bool auto_exp =  sensor->ctrls.auto_exp->val == V4L2_EXPOSURE_AUTO;
> +	unsigned long rate;
>  	int ret;
>
>  	dn_mode = mode->dn_mode;
> @@ -1655,6 +2003,22 @@ static int ov5640_set_mode(struct ov5640_dev *sensor)
>  			goto restore_auto_gain;
>  	}
>
> +	/*
> +	 * All the formats we support have 16 bits per pixel, seems to require
> +	 * the same rate than YUV, so we can just use 16 bpp all the time.
> +	 */
> +	rate = mode->pixel_clock * 16;
> +	if (sensor->ep.bus_type == V4L2_MBUS_CSI2_DPHY) {
> +		rate = rate / sensor->ep.bus.mipi_csi2.num_data_lanes;
> +		ret = ov5640_set_mipi_pclk(sensor, rate);
> +	} else {
> +		rate = rate / sensor->ep.bus.parallel.bus_width;
> +		ret = ov5640_set_dvp_pclk(sensor, rate);
> +	}
> +
> +	if (ret < 0)
> +		return 0;
> +
>  	if ((dn_mode == SUBSAMPLING && orig_dn_mode == SCALING) ||
>  	    (dn_mode == SCALING && orig_dn_mode == SUBSAMPLING)) {
>  		/*
> --
> 2.19.1
>

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v5 01/11] media: ov5640: Adjust the clock based on the expected rate
  2018-11-14 13:20   ` Adam Ford
@ 2018-11-19 15:03     ` Adam Ford
  0 siblings, 0 replies; 17+ messages in thread
From: Adam Ford @ 2018-11-19 15:03 UTC (permalink / raw)
  To: maxime.ripard
  Cc: mchehab, Laurent Pinchart, linux-media, thomas.petazzoni,
	mylene.josserand, hans.verkuil, sakari.ailus, hugues.fruchet,
	loic.poulain, sam, Steve Longerbeam, daniel, jacopo

On Wed, Nov 14, 2018 at 7:20 AM Adam Ford <aford173@gmail.com> wrote:
>
> On Tue, Nov 13, 2018 at 7:04 AM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> >
> > The clock structure for the PCLK is quite obscure in the documentation, and
> > was hardcoded through the bytes array of each and every mode.
> >
> > This is troublesome, since we cannot adjust it at runtime based on other
> > parameters (such as the number of bytes per pixel), and we can't support
> > either framerates that have not been used by the various vendors, since we
> > don't have the needed initialization sequence.
> >
> > We can however understand how the clock tree works, and then implement some
> > functions to derive the various parameters from a given rate. And now that
> > those parameters are calculated at runtime, we can remove them from the
> > initialization sequence.
> >
> > The modes also gained a new parameter which is the clock that they are
> > running at, from the register writes they were doing, so for now the switch
> > to the new algorithm should be transparent.
> >
> > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
>
> Thanks for the patches! I tested the whole series.  I am stil learning
> the v4l2 stuff, but I'm trying to test what and where I can.
> media-ctl shows the camera is talking at 60fps, but my imx6 is only
> capturing at 30, but I don't think it's the fault of the ov5640
> driver.
>
> Tested-by: Adam Ford <aford173@gmail.com> #imx6dq
>

For what it's worth, I would I like to request that this series be
applied to 4.20 fixes (possibly 4.19 if appropriate) as it fixes some
issues where I am trying to capture JPEG images that previously came
up garbled, and now they are clear.  The only change to my kernel is
this series.

adam

> adam
> > ---
> >  drivers/media/i2c/ov5640.c | 366 ++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 365 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> > index eaefdb58653b..8476f85bb8e7 100644
> > --- a/drivers/media/i2c/ov5640.c
> > +++ b/drivers/media/i2c/ov5640.c
> > @@ -175,6 +175,7 @@ struct ov5640_mode_info {
> >         u32 htot;
> >         u32 vact;
> >         u32 vtot;
> > +       u32 pixel_clock;
> >         const struct reg_value *reg_data;
> >         u32 reg_data_size;
> >  };
> > @@ -700,6 +701,7 @@ static const struct reg_value ov5640_setting_15fps_QSXGA_2592_1944[] = {
> >  /* power-on sensor init reg table */
> >  static const struct ov5640_mode_info ov5640_mode_init_data = {
> >         0, SUBSAMPLING, 640, 1896, 480, 984,
> > +       56000000,
> >         ov5640_init_setting_30fps_VGA,
> >         ARRAY_SIZE(ov5640_init_setting_30fps_VGA),
> >  };
> > @@ -709,74 +711,91 @@ ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES] = {
> >         {
> >                 {OV5640_MODE_QCIF_176_144, SUBSAMPLING,
> >                  176, 1896, 144, 984,
> > +                28000000,
> >                  ov5640_setting_15fps_QCIF_176_144,
> >                  ARRAY_SIZE(ov5640_setting_15fps_QCIF_176_144)},
> >                 {OV5640_MODE_QVGA_320_240, SUBSAMPLING,
> >                  320, 1896, 240, 984,
> > +                28000000,
> >                  ov5640_setting_15fps_QVGA_320_240,
> >                  ARRAY_SIZE(ov5640_setting_15fps_QVGA_320_240)},
> >                 {OV5640_MODE_VGA_640_480, SUBSAMPLING,
> >                  640, 1896, 480, 1080,
> > +                28000000,
> >                  ov5640_setting_15fps_VGA_640_480,
> >                  ARRAY_SIZE(ov5640_setting_15fps_VGA_640_480)},
> >                 {OV5640_MODE_NTSC_720_480, SUBSAMPLING,
> >                  720, 1896, 480, 984,
> > +                28000000,
> >                  ov5640_setting_15fps_NTSC_720_480,
> >                  ARRAY_SIZE(ov5640_setting_15fps_NTSC_720_480)},
> >                 {OV5640_MODE_PAL_720_576, SUBSAMPLING,
> >                  720, 1896, 576, 984,
> > +                28000000,
> >                  ov5640_setting_15fps_PAL_720_576,
> >                  ARRAY_SIZE(ov5640_setting_15fps_PAL_720_576)},
> >                 {OV5640_MODE_XGA_1024_768, SUBSAMPLING,
> >                  1024, 1896, 768, 1080,
> > +                28000000,
> >                  ov5640_setting_15fps_XGA_1024_768,
> >                  ARRAY_SIZE(ov5640_setting_15fps_XGA_1024_768)},
> >                 {OV5640_MODE_720P_1280_720, SUBSAMPLING,
> >                  1280, 1892, 720, 740,
> > +                21000000,
> >                  ov5640_setting_15fps_720P_1280_720,
> >                  ARRAY_SIZE(ov5640_setting_15fps_720P_1280_720)},
> >                 {OV5640_MODE_1080P_1920_1080, SCALING,
> >                  1920, 2500, 1080, 1120,
> > +                42000000,
> >                  ov5640_setting_15fps_1080P_1920_1080,
> >                  ARRAY_SIZE(ov5640_setting_15fps_1080P_1920_1080)},
> >                 {OV5640_MODE_QSXGA_2592_1944, SCALING,
> >                  2592, 2844, 1944, 1968,
> > +                84000000,
> >                  ov5640_setting_15fps_QSXGA_2592_1944,
> >                  ARRAY_SIZE(ov5640_setting_15fps_QSXGA_2592_1944)},
> >         }, {
> >                 {OV5640_MODE_QCIF_176_144, SUBSAMPLING,
> >                  176, 1896, 144, 984,
> > +                56000000,
> >                  ov5640_setting_30fps_QCIF_176_144,
> >                  ARRAY_SIZE(ov5640_setting_30fps_QCIF_176_144)},
> >                 {OV5640_MODE_QVGA_320_240, SUBSAMPLING,
> >                  320, 1896, 240, 984,
> > +                56000000,
> >                  ov5640_setting_30fps_QVGA_320_240,
> >                  ARRAY_SIZE(ov5640_setting_30fps_QVGA_320_240)},
> >                 {OV5640_MODE_VGA_640_480, SUBSAMPLING,
> >                  640, 1896, 480, 1080,
> > +                56000000,
> >                  ov5640_setting_30fps_VGA_640_480,
> >                  ARRAY_SIZE(ov5640_setting_30fps_VGA_640_480)},
> >                 {OV5640_MODE_NTSC_720_480, SUBSAMPLING,
> >                  720, 1896, 480, 984,
> > +                56000000,
> >                  ov5640_setting_30fps_NTSC_720_480,
> >                  ARRAY_SIZE(ov5640_setting_30fps_NTSC_720_480)},
> >                 {OV5640_MODE_PAL_720_576, SUBSAMPLING,
> >                  720, 1896, 576, 984,
> > +                56000000,
> >                  ov5640_setting_30fps_PAL_720_576,
> >                  ARRAY_SIZE(ov5640_setting_30fps_PAL_720_576)},
> >                 {OV5640_MODE_XGA_1024_768, SUBSAMPLING,
> >                  1024, 1896, 768, 1080,
> > +                56000000,
> >                  ov5640_setting_30fps_XGA_1024_768,
> >                  ARRAY_SIZE(ov5640_setting_30fps_XGA_1024_768)},
> >                 {OV5640_MODE_720P_1280_720, SUBSAMPLING,
> >                  1280, 1892, 720, 740,
> > +                42000000,
> >                  ov5640_setting_30fps_720P_1280_720,
> >                  ARRAY_SIZE(ov5640_setting_30fps_720P_1280_720)},
> >                 {OV5640_MODE_1080P_1920_1080, SCALING,
> >                  1920, 2500, 1080, 1120,
> > +                84000000,
> >                  ov5640_setting_30fps_1080P_1920_1080,
> >                  ARRAY_SIZE(ov5640_setting_30fps_1080P_1920_1080)},
> > -               {OV5640_MODE_QSXGA_2592_1944, -1, 0, 0, 0, 0, NULL, 0},
> > +               {OV5640_MODE_QSXGA_2592_1944, -1, 0, 0, 0, 0, 0, NULL, 0},
> >         },
> >  };
> >
> > @@ -909,6 +928,334 @@ static int ov5640_mod_reg(struct ov5640_dev *sensor, u16 reg,
> >         return ov5640_write_reg(sensor, reg, val);
> >  }
> >
> > +/*
> > + * After trying the various combinations, reading various
> > + * documentations spreaded around the net, and from the various
> > + * feedback, the clock tree is probably as follows:
> > + *
> > + *   +--------------+
> > + *   |  Ext. Clock  |
> > + *   +-+------------+
> > + *     |  +----------+
> > + *     +->|   PLL1   | - reg 0x3036, for the multiplier
> > + *        +-+--------+ - reg 0x3037, bits 0-3 for the pre-divider
> > + *          |  +--------------+
> > + *          +->| System Clock |  - reg 0x3035, bits 4-7
> > + *             +-+------------+
> > + *               |  +--------------+
> > + *               +->| MIPI Divider | - reg 0x3035, bits 0-3
> > + *               |  +-+------------+
> > + *               |    +----------------> MIPI SCLK
> > + *               |    +  +-----+
> > + *               |    +->| / 2 |-------> MIPI BIT CLK
> > + *               |       +-----+
> > + *               |  +--------------+
> > + *               +->| PLL Root Div | - reg 0x3037, bit 4
> > + *                  +-+------------+
> > + *                    |  +---------+
> > + *                    +->| Bit Div | - reg 0x3035, bits 0-3
> > + *                       +-+-------+
> > + *                         |  +-------------+
> > + *                         +->| SCLK Div    | - reg 0x3108, bits 0-1
> > + *                         |  +-+-----------+
> > + *                         |    +---------------> SCLK
> > + *                         |  +-------------+
> > + *                         +->| SCLK 2X Div | - reg 0x3108, bits 2-3
> > + *                         |  +-+-----------+
> > + *                         |    +---------------> SCLK 2X
> > + *                         |  +-------------+
> > + *                         +->| PCLK Div    | - reg 0x3108, bits 4-5
> > + *                            +-+-----------+
> > + *                              +---------------> PCLK
> > + *
> > + * This is deviating from the datasheet at least for the register
> > + * 0x3108, since it's said here that the PCLK would be clocked from
> > + * the PLL.
> > + *
> > + * There seems to be also (unverified) constraints:
> > + *  - the PLL pre-divider output rate should be in the 4-27MHz range
> > + *  - the PLL multiplier output rate should be in the 500-1000MHz range
> > + *  - PCLK >= SCLK * 2 in YUV, >= SCLK in Raw or JPEG
> > + *  - MIPI SCLK = (bpp / lanes) / PCLK
> > + *
> > + * In the two latter cases, these constraints are met since our
> > + * factors are hardcoded. If we were to change that, we would need to
> > + * take this into account. The only varying parts are the PLL
> > + * multiplier and the system clock divider, which are shared between
> > + * all these clocks so won't cause any issue.
> > + */
> > +
> > +/*
> > + * This is supposed to be ranging from 1 to 8, but the value is always
> > + * set to 3 in the vendor kernels.
> > + */
> > +#define OV5640_PLL_PREDIV      3
> > +
> > +#define OV5640_PLL_MULT_MIN    4
> > +#define OV5640_PLL_MULT_MAX    252
> > +
> > +/*
> > + * This is supposed to be ranging from 1 to 16, but the value is
> > + * always set to either 1 or 2 in the vendor kernels.
> > + */
> > +#define OV5640_SYSDIV_MIN      1
> > +#define OV5640_SYSDIV_MAX      16
> > +
> > +/*
> > + * This is supposed to be ranging from 1 to 16, but the value is always
> > + * set to 2 in the vendor kernels.
> > + */
> > +#define OV5640_MIPI_DIV                2
> > +
> > +/*
> > + * 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_CTRL3_PLL_ROOT_DIV_2                BIT(4)
> > +
> > +/*
> > + * We only supports 8-bit formats at the moment
> > + */
> > +#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
> > + * set to 2 in the vendor kernels.
> > + */
> > +#define OV5640_SCLK_ROOT_DIV   2
> > +
> > +/*
> > + * This is hardcoded so that the consistency is maintained between SCLK and
> > + * SCLK 2x.
> > + */
> > +#define OV5640_SCLK2X_ROOT_DIV (OV5640_SCLK_ROOT_DIV / 2)
> > +
> > +/*
> > + * 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_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 sysclk = sensor->xclk_freq / pll_prediv * pll_mult;
> > +
> > +       /* 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,
> > +                                        unsigned long rate,
> > +                                        u8 *pll_prediv, u8 *pll_mult,
> > +                                        u8 *sysdiv)
> > +{
> > +       unsigned long best = ~0;
> > +       u8 best_sysdiv = 1, best_mult = 1;
> > +       u8 _sysdiv, _pll_mult;
> > +
> > +       for (_sysdiv = OV5640_SYSDIV_MIN;
> > +            _sysdiv <= OV5640_SYSDIV_MAX;
> > +            _sysdiv++) {
> > +               for (_pll_mult = OV5640_PLL_MULT_MIN;
> > +                    _pll_mult <= OV5640_PLL_MULT_MAX;
> > +                    _pll_mult++) {
> > +                       unsigned long _rate;
> > +
> > +                       /*
> > +                        * The PLL multiplier cannot be odd if above
> > +                        * 127.
> > +                        */
> > +                       if (_pll_mult > 127 && (_pll_mult % 2))
> > +                               continue;
> > +
> > +                       _rate = ov5640_compute_sys_clk(sensor,
> > +                                                      OV5640_PLL_PREDIV,
> > +                                                      _pll_mult, _sysdiv);
> > +
> > +                       /*
> > +                        * We have reached the maximum allowed PLL1 output,
> > +                        * increase sysdiv.
> > +                        */
> > +                       if (!rate)
> > +                               break;
> > +
> > +                       /*
> > +                        * Prefer rates above the expected clock rate than
> > +                        * below, even if that means being less precise.
> > +                        */
> > +                       if (_rate < rate)
> > +                               continue;
> > +
> > +                       if (abs(rate - _rate) < abs(rate - best)) {
> > +                               best = _rate;
> > +                               best_sysdiv = _sysdiv;
> > +                               best_mult = _pll_mult;
> > +                       }
> > +
> > +                       if (_rate == rate)
> > +                               goto out;
> > +               }
> > +       }
> > +
> > +out:
> > +       *sysdiv = best_sysdiv;
> > +       *pll_prediv = OV5640_PLL_PREDIV;
> > +       *pll_mult = best_mult;
> > +
> > +       return best;
> > +}
> > +
> > +/*
> > + * 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)
> > +{
> > +       const struct ov5640_mode_info *mode = sensor->current_mode;
> > +       u8 mipi_div = OV5640_MIPI_DIV;
> > +       u8 prediv, mult, sysdiv;
> > +       int ret;
> > +
> > +       /* 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;
> > +
> > +       /* 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;
> > +
> > +       ov5640_calc_sys_clk(sensor, rate, &prediv, &mult, &sysdiv);
> > +
> > +       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);
> > +       if (ret)
> > +               return ret;
> > +
> > +       ret = ov5640_mod_reg(sensor, OV5640_REG_SC_PLL_CTRL2, 0xff, mult);
> > +       if (ret)
> > +               return ret;
> > +
> > +       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,
> > +                                     unsigned long rate,
> > +                                     u8 *pll_prediv, u8 *pll_mult, u8 *sysdiv,
> > +                                     u8 *pll_rdiv, u8 *bit_div, u8 *pclk_div)
> > +{
> > +       unsigned long _rate = rate * OV5640_PLL_ROOT_DIV * OV5640_BIT_DIV *
> > +                               OV5640_PCLK_ROOT_DIV;
> > +
> > +       _rate = ov5640_calc_sys_clk(sensor, _rate, pll_prediv, pll_mult,
> > +                                   sysdiv);
> > +       *pll_rdiv = OV5640_PLL_ROOT_DIV;
> > +       *bit_div = OV5640_BIT_DIV;
> > +       *pclk_div = OV5640_PCLK_ROOT_DIV;
> > +
> > +       return _rate / *pll_rdiv / *bit_div / *pclk_div;
> > +}
> > +
> > +static int ov5640_set_dvp_pclk(struct ov5640_dev *sensor, unsigned long rate)
> > +{
> > +       u8 prediv, mult, sysdiv, pll_rdiv, bit_div, pclk_div;
> > +       int ret;
> > +
> > +       ov5640_calc_pclk(sensor, rate, &prediv, &mult, &sysdiv, &pll_rdiv,
> > +                        &bit_div, &pclk_div);
> > +
> > +       if (bit_div == 2)
> > +               bit_div = 8;
> > +
> > +       ret = ov5640_mod_reg(sensor, OV5640_REG_SC_PLL_CTRL0,
> > +                            0x0f, bit_div);
> > +       if (ret)
> > +               return ret;
> > +
> > +       /*
> > +        * We need to set sysdiv according to the clock, and to clear
> > +        * the MIPI divider.
> > +        */
> > +       ret = ov5640_mod_reg(sensor, OV5640_REG_SC_PLL_CTRL1,
> > +                            0xff, sysdiv << 4);
> > +       if (ret)
> > +               return ret;
> > +
> > +       ret = ov5640_mod_reg(sensor, OV5640_REG_SC_PLL_CTRL2,
> > +                            0xff, mult);
> > +       if (ret)
> > +               return ret;
> > +
> > +       ret = ov5640_mod_reg(sensor, OV5640_REG_SC_PLL_CTRL3,
> > +                            0x1f, prediv | ((pll_rdiv - 1) << 4));
> > +       if (ret)
> > +               return ret;
> > +
> > +       return ov5640_mod_reg(sensor, OV5640_REG_SYS_ROOT_DIVIDER, 0x30,
> > +                             (ilog2(pclk_div) << 4));
> > +}
> > +
> >  /* download ov5640 settings to sensor through i2c */
> >  static int ov5640_set_timings(struct ov5640_dev *sensor,
> >                               const struct ov5640_mode_info *mode)
> > @@ -1637,6 +1984,7 @@ static int ov5640_set_mode(struct ov5640_dev *sensor)
> >         enum ov5640_downsize_mode dn_mode, orig_dn_mode;
> >         bool auto_gain = sensor->ctrls.auto_gain->val == 1;
> >         bool auto_exp =  sensor->ctrls.auto_exp->val == V4L2_EXPOSURE_AUTO;
> > +       unsigned long rate;
> >         int ret;
> >
> >         dn_mode = mode->dn_mode;
> > @@ -1655,6 +2003,22 @@ static int ov5640_set_mode(struct ov5640_dev *sensor)
> >                         goto restore_auto_gain;
> >         }
> >
> > +       /*
> > +        * All the formats we support have 16 bits per pixel, seems to require
> > +        * the same rate than YUV, so we can just use 16 bpp all the time.
> > +        */
> > +       rate = mode->pixel_clock * 16;
> > +       if (sensor->ep.bus_type == V4L2_MBUS_CSI2_DPHY) {
> > +               rate = rate / sensor->ep.bus.mipi_csi2.num_data_lanes;
> > +               ret = ov5640_set_mipi_pclk(sensor, rate);
> > +       } else {
> > +               rate = rate / sensor->ep.bus.parallel.bus_width;
> > +               ret = ov5640_set_dvp_pclk(sensor, rate);
> > +       }
> > +
> > +       if (ret < 0)
> > +               return 0;
> > +
> >         if ((dn_mode == SUBSAMPLING && orig_dn_mode == SCALING) ||
> >             (dn_mode == SCALING && orig_dn_mode == SUBSAMPLING)) {
> >                 /*
> > --
> > 2.19.1
> >

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v5 01/11] media: ov5640: Adjust the clock based on the expected rate
  2018-11-14 19:48   ` jacopo mondi
@ 2018-11-20  9:48     ` Maxime Ripard
  2018-11-20 16:51       ` jacopo mondi
  0 siblings, 1 reply; 17+ messages in thread
From: Maxime Ripard @ 2018-11-20  9:48 UTC (permalink / raw)
  To: jacopo mondi
  Cc: Mauro Carvalho Chehab, Laurent Pinchart, linux-media,
	Thomas Petazzoni, Mylene Josserand, Hans Verkuil, Sakari Ailus,
	Hugues Fruchet, Loic Poulain, Samuel Bobrowicz, Steve Longerbeam,
	Daniel Mack

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

Hi Jacopo,

On Wed, Nov 14, 2018 at 08:48:47PM +0100, jacopo mondi wrote:
> On Tue, Nov 13, 2018 at 02:03:15PM +0100, Maxime Ripard wrote:
> > The clock structure for the PCLK is quite obscure in the documentation, and
> > was hardcoded through the bytes array of each and every mode.
> >
> > This is troublesome, since we cannot adjust it at runtime based on other
> > parameters (such as the number of bytes per pixel), and we can't support
> > either framerates that have not been used by the various vendors, since we
> > don't have the needed initialization sequence.
> >
> > We can however understand how the clock tree works, and then implement some
> > functions to derive the various parameters from a given rate. And now that
> > those parameters are calculated at runtime, we can remove them from the
> > initialization sequence.
> >
> > The modes also gained a new parameter which is the clock that they are
> > running at, from the register writes they were doing, so for now the switch
> > to the new algorithm should be transparent.
> >
> > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> 
> As you've squashed in my MIPI CSI-2 fixes, do you think it is
> appropriate adding my So-b tag here?

Yeah, I'll add your co-developped-by tag as well, if that's ok.

> > +/*
> > + * This is supposed to be ranging from 1 to 16, but the value is
> > + * always set to either 1 or 2 in the vendor kernels.
> > + */
> 
> I forgot to fix this comment in my patches you squahed in here (the
> value now ranges from 1 to 16

Ok.

> > +#define OV5640_SYSDIV_MIN	1
> > +#define OV5640_SYSDIV_MAX	16
> > +
> > +/*
> > + * This is supposed to be ranging from 1 to 16, but the value is always
> > + * set to 2 in the vendor kernels.
> > + */
> > +#define OV5640_MIPI_DIV		2
> > +
> > +/*
> > + * 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_CTRL3_PLL_ROOT_DIV_2		BIT(4)
> > +
> > +/*
> > + * We only supports 8-bit formats at the moment
> > + */
> > +#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
> > + * set to 2 in the vendor kernels.
> > + */
> > +#define OV5640_SCLK_ROOT_DIV	2
> > +
> > +/*
> > + * This is hardcoded so that the consistency is maintained between SCLK and
> > + * SCLK 2x.
> > + */
> > +#define OV5640_SCLK2X_ROOT_DIV (OV5640_SCLK_ROOT_DIV / 2)
> > +
> > +/*
> > + * 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_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 sysclk = sensor->xclk_freq / pll_prediv * pll_mult;
> > +
> > +	/* PLL1 output cannot exceed 1GHz. */
> > +	if (sysclk / 1000000 > 1000)
> > +		return 0;
> > +
> > +	return sysclk / sysdiv;
> > +}
> 
> That's better, but Sam's version is even better. I plan to pick some
> part of his patch and apply them on top of this (for this and a few
> other things I'm pointing out a here below). How would you like to
> have those updates? Incremental patches on top of this series once it
> gets merged (and it can now be merged, since it works for both CSI-2
> and DVP), or would you like to receive those patches, squash them in
> and re-send?

The first solution would be better, having to constantly piling up
patches in a series is a very efficient way to not get anything
merged.

> > +
> > +static unsigned long ov5640_calc_sys_clk(struct ov5640_dev *sensor,
> > +					 unsigned long rate,
> > +					 u8 *pll_prediv, u8 *pll_mult,
> > +					 u8 *sysdiv)
> > +{
> > +	unsigned long best = ~0;
> > +	u8 best_sysdiv = 1, best_mult = 1;
> > +	u8 _sysdiv, _pll_mult;
> > +
> > +	for (_sysdiv = OV5640_SYSDIV_MIN;
> > +	     _sysdiv <= OV5640_SYSDIV_MAX;
> > +	     _sysdiv++) {
> > +		for (_pll_mult = OV5640_PLL_MULT_MIN;
> > +		     _pll_mult <= OV5640_PLL_MULT_MAX;
> > +		     _pll_mult++) {
> > +			unsigned long _rate;
> > +
> > +			/*
> > +			 * The PLL multiplier cannot be odd if above
> > +			 * 127.
> > +			 */
> > +			if (_pll_mult > 127 && (_pll_mult % 2))
> > +				continue;
> > +
> > +			_rate = ov5640_compute_sys_clk(sensor,
> > +						       OV5640_PLL_PREDIV,
> > +						       _pll_mult, _sysdiv);
> > +
> > +			/*
> > +			 * We have reached the maximum allowed PLL1 output,
> > +			 * increase sysdiv.
> > +			 */
> > +			if (!rate)
> > +				break;
> > +
> > +			/*
> > +			 * Prefer rates above the expected clock rate than
> > +			 * below, even if that means being less precise.
> > +			 */
> > +			if (_rate < rate)
> > +				continue;
> > +
> > +			if (abs(rate - _rate) < abs(rate - best)) {
> > +				best = _rate;
> > +				best_sysdiv = _sysdiv;
> > +				best_mult = _pll_mult;
> > +			}
> > +
> > +			if (_rate == rate)
> > +				goto out;
> > +		}
> > +	}
> > +
> > +out:
> > +	*sysdiv = best_sysdiv;
> > +	*pll_prediv = OV5640_PLL_PREDIV;
> > +	*pll_mult = best_mult;
> > +
> > +	return best;
> > +}
> > +
> > +/*
> > + * 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
> > + *
> 
> Almost. This is actually the bandwidth per lane. My bad, I need to update
> this whole comment block.

Can you send a patch? I'll squash it in.

> > + * 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)
> > +{
> > +	const struct ov5640_mode_info *mode = sensor->current_mode;
> > +	u8 mipi_div = OV5640_MIPI_DIV;
> > +	u8 prediv, mult, sysdiv;
> > +	int ret;
> > +
> > +	/* 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;
> > +
> > +	/* 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;
> 
> Sam patches are better here. He found out the reason for this, as
> 'high resolutions modes' use the scaler, and a ration between pixel
> clock and scaler clock has to be respected. My patches you squahsed in
> here use this rather sub-optimal "hact > 1024" check, I would rather use his
> "does the mode use the scaler?" way instead. That's something else
> that could be squashed in a forthcoming version, or fixed with
> incremental patches.
> 
> > +
> > +	ov5640_calc_sys_clk(sensor, rate, &prediv, &mult, &sysdiv);
> > +
> > +	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);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = ov5640_mod_reg(sensor, OV5640_REG_SC_PLL_CTRL2, 0xff, mult);
> > +	if (ret)
> > +		return ret;
> > +
> > +	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);
> 
> Again, there might be something to bring in from Sam patches here
> (programming register 0x4837 with the pixel clock in particular).
> Again, let me know how would you like to receive updates.
> 
> > +}
> > +
> > +static unsigned long ov5640_calc_pclk(struct ov5640_dev *sensor,
> > +				      unsigned long rate,
> > +				      u8 *pll_prediv, u8 *pll_mult, u8 *sysdiv,
> > +				      u8 *pll_rdiv, u8 *bit_div, u8 *pclk_div)
> > +{
> > +	unsigned long _rate = rate * OV5640_PLL_ROOT_DIV * OV5640_BIT_DIV *
> > +				OV5640_PCLK_ROOT_DIV;
> > +
> > +	_rate = ov5640_calc_sys_clk(sensor, _rate, pll_prediv, pll_mult,
> > +				    sysdiv);
> > +	*pll_rdiv = OV5640_PLL_ROOT_DIV;
> > +	*bit_div = OV5640_BIT_DIV;
> > +	*pclk_div = OV5640_PCLK_ROOT_DIV;
> > +
> > +	return _rate / *pll_rdiv / *bit_div / *pclk_div;
> > +}
> 
> This function is now called from a single place, maybe you want to
> remove it and inline its code.

I still find it easier to understand, and the compiler will probbaly
end up inlining it anyway.

Thanks!
Maxime


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

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v5 01/11] media: ov5640: Adjust the clock based on the expected rate
  2018-11-20  9:48     ` Maxime Ripard
@ 2018-11-20 16:51       ` jacopo mondi
  0 siblings, 0 replies; 17+ messages in thread
From: jacopo mondi @ 2018-11-20 16:51 UTC (permalink / raw)
  To: Maxime Ripard
  Cc: Mauro Carvalho Chehab, Laurent Pinchart, linux-media,
	Thomas Petazzoni, Mylene Josserand, Hans Verkuil, Sakari Ailus,
	Hugues Fruchet, Loic Poulain, Samuel Bobrowicz, Steve Longerbeam,
	Daniel Mack

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

Hi Maxime,

On Tue, Nov 20, 2018 at 10:48:39AM +0100, Maxime Ripard wrote:
> Hi Jacopo,
>
> On Wed, Nov 14, 2018 at 08:48:47PM +0100, jacopo mondi wrote:
> > On Tue, Nov 13, 2018 at 02:03:15PM +0100, Maxime Ripard wrote:
> > > The clock structure for the PCLK is quite obscure in the documentation, and
> > > was hardcoded through the bytes array of each and every mode.
> > >
> > > This is troublesome, since we cannot adjust it at runtime based on other
> > > parameters (such as the number of bytes per pixel), and we can't support
> > > either framerates that have not been used by the various vendors, since we
> > > don't have the needed initialization sequence.
> > >
> > > We can however understand how the clock tree works, and then implement some
> > > functions to derive the various parameters from a given rate. And now that
> > > those parameters are calculated at runtime, we can remove them from the
> > > initialization sequence.
> > >
> > > The modes also gained a new parameter which is the clock that they are
> > > running at, from the register writes they were doing, so for now the switch
> > > to the new algorithm should be transparent.
> > >
> > > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> >
> > As you've squashed in my MIPI CSI-2 fixes, do you think it is
> > appropriate adding my So-b tag here?
>
> Yeah, I'll add your co-developped-by tag as well, if that's ok.
>

Yeah, whatever works for you here... Thanks ;)

> > > +/*
> > > + * This is supposed to be ranging from 1 to 16, but the value is
> > > + * always set to either 1 or 2 in the vendor kernels.
> > > + */
> >
> > I forgot to fix this comment in my patches you squahed in here (the
> > value now ranges from 1 to 16
>
> Ok.
>
> > > +#define OV5640_SYSDIV_MIN	1
> > > +#define OV5640_SYSDIV_MAX	16
> > > +
> > > +/*
> > > + * This is supposed to be ranging from 1 to 16, but the value is always
> > > + * set to 2 in the vendor kernels.
> > > + */
> > > +#define OV5640_MIPI_DIV		2
> > > +
> > > +/*
> > > + * 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_CTRL3_PLL_ROOT_DIV_2		BIT(4)
> > > +
> > > +/*
> > > + * We only supports 8-bit formats at the moment
> > > + */
> > > +#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
> > > + * set to 2 in the vendor kernels.
> > > + */
> > > +#define OV5640_SCLK_ROOT_DIV	2
> > > +
> > > +/*
> > > + * This is hardcoded so that the consistency is maintained between SCLK and
> > > + * SCLK 2x.
> > > + */
> > > +#define OV5640_SCLK2X_ROOT_DIV (OV5640_SCLK_ROOT_DIV / 2)
> > > +
> > > +/*
> > > + * 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_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 sysclk = sensor->xclk_freq / pll_prediv * pll_mult;
> > > +
> > > +	/* PLL1 output cannot exceed 1GHz. */
> > > +	if (sysclk / 1000000 > 1000)
> > > +		return 0;
> > > +
> > > +	return sysclk / sysdiv;
> > > +}
> >
> > That's better, but Sam's version is even better. I plan to pick some
> > part of his patch and apply them on top of this (for this and a few
> > other things I'm pointing out a here below). How would you like to
> > have those updates? Incremental patches on top of this series once it
> > gets merged (and it can now be merged, since it works for both CSI-2
> > and DVP), or would you like to receive those patches, squash them in
> > and re-send?
>
> The first solution would be better, having to constantly piling up
> patches in a series is a very efficient way to not get anything
> merged.
>

I know -.-

Fine, I'll have some more patches for ov5640 for next cycle then :)
(For this and all other changes to the CSI-2 portion of this patch)

> > > +
> > > +static unsigned long ov5640_calc_sys_clk(struct ov5640_dev *sensor,
> > > +					 unsigned long rate,
> > > +					 u8 *pll_prediv, u8 *pll_mult,
> > > +					 u8 *sysdiv)
> > > +{
> > > +	unsigned long best = ~0;
> > > +	u8 best_sysdiv = 1, best_mult = 1;
> > > +	u8 _sysdiv, _pll_mult;
> > > +
> > > +	for (_sysdiv = OV5640_SYSDIV_MIN;
> > > +	     _sysdiv <= OV5640_SYSDIV_MAX;
> > > +	     _sysdiv++) {
> > > +		for (_pll_mult = OV5640_PLL_MULT_MIN;
> > > +		     _pll_mult <= OV5640_PLL_MULT_MAX;
> > > +		     _pll_mult++) {
> > > +			unsigned long _rate;
> > > +
> > > +			/*
> > > +			 * The PLL multiplier cannot be odd if above
> > > +			 * 127.
> > > +			 */
> > > +			if (_pll_mult > 127 && (_pll_mult % 2))
> > > +				continue;
> > > +
> > > +			_rate = ov5640_compute_sys_clk(sensor,
> > > +						       OV5640_PLL_PREDIV,
> > > +						       _pll_mult, _sysdiv);
> > > +
> > > +			/*
> > > +			 * We have reached the maximum allowed PLL1 output,
> > > +			 * increase sysdiv.
> > > +			 */
> > > +			if (!rate)
> > > +				break;
> > > +
> > > +			/*
> > > +			 * Prefer rates above the expected clock rate than
> > > +			 * below, even if that means being less precise.
> > > +			 */
> > > +			if (_rate < rate)
> > > +				continue;
> > > +
> > > +			if (abs(rate - _rate) < abs(rate - best)) {
> > > +				best = _rate;
> > > +				best_sysdiv = _sysdiv;
> > > +				best_mult = _pll_mult;
> > > +			}
> > > +
> > > +			if (_rate == rate)
> > > +				goto out;
> > > +		}
> > > +	}
> > > +
> > > +out:
> > > +	*sysdiv = best_sysdiv;
> > > +	*pll_prediv = OV5640_PLL_PREDIV;
> > > +	*pll_mult = best_mult;
> > > +
> > > +	return best;
> > > +}
> > > +
> > > +/*
> > > + * 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
> > > + *
> >
> > Almost. This is actually the bandwidth per lane. My bad, I need to update
> > this whole comment block.
>
> Can you send a patch? I'll squash it in.
>

Ok, this one is 'easy', just have to re-write the comment block, can
be squashed in.

> > > + * 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)
> > > +{
> > > +	const struct ov5640_mode_info *mode = sensor->current_mode;
> > > +	u8 mipi_div = OV5640_MIPI_DIV;
> > > +	u8 prediv, mult, sysdiv;
> > > +	int ret;
> > > +
> > > +	/* 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;
> > > +
> > > +	/* 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;
> >
> > Sam patches are better here. He found out the reason for this, as
> > 'high resolutions modes' use the scaler, and a ration between pixel
> > clock and scaler clock has to be respected. My patches you squahsed in
> > here use this rather sub-optimal "hact > 1024" check, I would rather use his
> > "does the mode use the scaler?" way instead. That's something else
> > that could be squashed in a forthcoming version, or fixed with
> > incremental patches.
> >
> > > +
> > > +	ov5640_calc_sys_clk(sensor, rate, &prediv, &mult, &sysdiv);
> > > +
> > > +	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);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	ret = ov5640_mod_reg(sensor, OV5640_REG_SC_PLL_CTRL2, 0xff, mult);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	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);
> >
> > Again, there might be something to bring in from Sam patches here
> > (programming register 0x4837 with the pixel clock in particular).
> > Again, let me know how would you like to receive updates.
> >
> > > +}
> > > +
> > > +static unsigned long ov5640_calc_pclk(struct ov5640_dev *sensor,
> > > +				      unsigned long rate,
> > > +				      u8 *pll_prediv, u8 *pll_mult, u8 *sysdiv,
> > > +				      u8 *pll_rdiv, u8 *bit_div, u8 *pclk_div)
> > > +{
> > > +	unsigned long _rate = rate * OV5640_PLL_ROOT_DIV * OV5640_BIT_DIV *
> > > +				OV5640_PCLK_ROOT_DIV;
> > > +
> > > +	_rate = ov5640_calc_sys_clk(sensor, _rate, pll_prediv, pll_mult,
> > > +				    sysdiv);
> > > +	*pll_rdiv = OV5640_PLL_ROOT_DIV;
> > > +	*bit_div = OV5640_BIT_DIV;
> > > +	*pclk_div = OV5640_PCLK_ROOT_DIV;
> > > +
> > > +	return _rate / *pll_rdiv / *bit_div / *pclk_div;
> > > +}
> >
> > This function is now called from a single place, maybe you want to
> > remove it and inline its code.
>
> I still find it easier to understand, and the compiler will probbaly
> end up inlining it anyway.
>

Up to you, this is very minor stuff.

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2018-11-21  3:22 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-13 13:03 [PATCH v5 00/11] media: ov5640: Misc cleanup and improvements Maxime Ripard
2018-11-13 13:03 ` [PATCH v5 01/11] media: ov5640: Adjust the clock based on the expected rate Maxime Ripard
2018-11-14 13:20   ` Adam Ford
2018-11-19 15:03     ` Adam Ford
2018-11-14 19:48   ` jacopo mondi
2018-11-20  9:48     ` Maxime Ripard
2018-11-20 16:51       ` jacopo mondi
2018-11-13 13:03 ` [PATCH v5 02/11] media: ov5640: Remove the clocks registers initialization Maxime Ripard
2018-11-13 13:03 ` [PATCH v5 03/11] media: ov5640: Remove redundant defines Maxime Ripard
2018-11-13 13:03 ` [PATCH v5 04/11] media: ov5640: Remove redundant register setup Maxime Ripard
2018-11-13 13:03 ` [PATCH v5 05/11] media: ov5640: Compute the clock rate at runtime Maxime Ripard
2018-11-13 13:03 ` [PATCH v5 06/11] media: ov5640: Remove pixel clock rates Maxime Ripard
2018-11-13 13:03 ` [PATCH v5 07/11] media: ov5640: Enhance FPS handling Maxime Ripard
2018-11-13 13:03 ` [PATCH v5 08/11] media: ov5640: Make the return rate type more explicit Maxime Ripard
2018-11-13 13:03 ` [PATCH v5 09/11] media: ov5640: Make the FPS clamping / rounding more extendable Maxime Ripard
2018-11-13 13:03 ` [PATCH v5 10/11] media: ov5640: Add 60 fps support Maxime Ripard
2018-11-13 13:03 ` [PATCH v5 11/11] media: ov5640: Remove duplicate auto-exposure setup Maxime Ripard

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).