OV5640 CSI2 problems

OV5640 CSI2 problems

[Tomi Valkeinen]

Hi all,

I've been testing and debugging OV5640 with TI's DRA76 and AM65 platforms, which have the CAL IP for 
MIPI CSI2 RX.

The most clear problem is that 1280x720@30 doesn't work at all, but with all resolutions I can see 
occasional PHY errors reported when starting the streaming.

The OV5640 spec lists the video timings, but I haven't been able to figure out what exactly they 
mean, as e.g. the vsync time doesn't seem to match the other times according to my calculations.

In any case, I was poking here and there, and noticed that if I use the htot value from the spec 
(2844), instead of the current value (1896 for most resolutions), 1280x720 works, and the PHY errors 
are gone.

Testing more, I found out that the smaller the htot, the more unreliable the RX becomes, and going 
down from 2844, somewhere around 2400 I start to see errors.

I'm not that much familiar with CSI-2, and very little with OV5640. Does anyone have a clue about 
what I'm observing here? Does 1280x720@30 work on other platforms with CSI2? Where do the current 
OV5640 video timings come from?

  Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: OV5640 CSI2 problemsg

[Jacopo Mondi]

Hi Tomi,
   welcome to the ov5640 bandwagon

This driver received lot of attention and reworking, but there are
indeed several issues at the moment :(

On Fri, Mar 13, 2020 at 01:15:33PM +0200, Tomi Valkeinen wrote:
> Hi all,
>
> I've been testing and debugging OV5640 with TI's DRA76 and AM65 platforms,
> which have the CAL IP for MIPI CSI2 RX.
>
> The most clear problem is that 1280x720@30 doesn't work at all, but with all
> resolutions I can see occasional PHY errors reported when starting the
> streaming.

I've been testing a CSI-2 setup with 2 data lanes on an IMX6 platform,
just for the record

>
> The OV5640 spec lists the video timings, but I haven't been able to figure
> out what exactly they mean, as e.g. the vsync time doesn't seem to match the
> other times according to my calculations.
>

Are you referring to the ov5640_mode_info structures ?

> In any case, I was poking here and there, and noticed that if I use the htot
> value from the spec (2844), instead of the current value (1896 for most
> resolutions), 1280x720 works, and the PHY errors are gone.
>
> Testing more, I found out that the smaller the htot, the more unreliable the
> RX becomes, and going down from 2844, somewhere around 2400 I start to see
> errors.
>

That's a good finding!

I recall I had issues as well with that mode, and fixed them by
doubling the MIPI bus clock speed You might have noticed these lines
in the CSI-2 clock tree calculation function ov5640_set_mipi_pclk()

	/*
	 * 1280x720 is reported to use 'SUBSAMPLING' only,
	 * but according to the sensor manual it goes through the
	 * scaler before subsampling.
	 */
	if (mode->dn_mode == SCALING ||
	   (mode->id == OV5640_MODE_720P_1280_720))
		mipi_div = OV5640_MIPI_DIV_SCLK; // THIS is 1
	else
		mipi_div = OV5640_MIPI_DIV_PCLK; // THIS is 2: halve the MIPI clock speed

I had that mode working, but after a good year or so trying to decode
the clock tree of the sensor with only partially satisfactory results,
I can't tell if that was by accident or not :)

> I'm not that much familiar with CSI-2, and very little with OV5640. Does
> anyone have a clue about what I'm observing here? Does 1280x720@30 work on

Hugues made a great effort by sampling the actual frequencies on the
bus, and he found out the actual frequencies are off compared to my
theoretical calculations. After that I've actually dropped the ball and
moved on, but maybe throwing your htot findings in the mix could help?

Here you have the thread with more information and Hugues measurement
results:
https://patchwork.kernel.org/patch/11019673/

> other platforms with CSI2? Where do the current OV5640 video timings come
> from?
>

I suggest you have a look at
dfbfb7aa832c ("media: ov5640: Compute the clock rate at runtime")

htot is used to calculate the desired pixel clock, so it could indeed
be one of the reasons why the above clock tree calculations are off.

Hope it helps a bit.

Thanks
   j

>  Tomi
>
> --
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: OV5640 CSI2 problems

[Loic Poulain]

On Fri, 13 Mar 2020 at 12:15, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>
> Hi all,
>
> I've been testing and debugging OV5640 with TI's DRA76 and AM65 platforms, which have the CAL IP for
> MIPI CSI2 RX.
>
> The most clear problem is that 1280x720@30 doesn't work at all, but with all resolutions I can see
> occasional PHY errors reported when starting the streaming.
>
> The OV5640 spec lists the video timings, but I haven't been able to figure out what exactly they
> mean, as e.g. the vsync time doesn't seem to match the other times according to my calculations.
>
> In any case, I was poking here and there, and noticed that if I use the htot value from the spec
> (2844), instead of the current value (1896 for most resolutions), 1280x720 works, and the PHY errors
> are gone.
>
> Testing more, I found out that the smaller the htot, the more unreliable the RX becomes, and going
> down from 2844, somewhere around 2400 I start to see errors.
>
> I'm not that much familiar with CSI-2, and very little with OV5640. Does anyone have a clue about
> what I'm observing here? Does 1280x720@30 work on other platforms with CSI2? Where do the current
> OV5640 video timings come from?

On my side, It works at least with dragonboard-410c (Qualcomm APQ8016).

Regards,
Loic

Re: OV5640 CSI2 problemsg

[Tomi Valkeinen]

On 17/03/2020 12:22, Jacopo Mondi wrote:
> Hi Tomi,
>     welcome to the ov5640 bandwagon
> 
> This driver received lot of attention and reworking, but there are
> indeed several issues at the moment :(
> 
> On Fri, Mar 13, 2020 at 01:15:33PM +0200, Tomi Valkeinen wrote:
>> Hi all,
>>
>> I've been testing and debugging OV5640 with TI's DRA76 and AM65 platforms,
>> which have the CAL IP for MIPI CSI2 RX.
>>
>> The most clear problem is that 1280x720@30 doesn't work at all, but with all
>> resolutions I can see occasional PHY errors reported when starting the
>> streaming.
> 
> I've been testing a CSI-2 setup with 2 data lanes on an IMX6 platform,
> just for the record

Two lanes here too.

>> The OV5640 spec lists the video timings, but I haven't been able to figure
>> out what exactly they mean, as e.g. the vsync time doesn't seem to match the
>> other times according to my calculations.
>>
> 
> Are you referring to the ov5640_mode_info structures ?

Yes.

Looking at the git log, these values seem to have been there from the start. Initially in the raw 
register sequences, then moved from there to ov5640_mode_info. But the numbers have been the same.

I wonder where they came originally, and whether they have ever been correct.

Perhaps I'll cook up a patch where I'll update the values to what the sensor sheet suggests, and 
other people can try and see if the driver still works for them.

>> In any case, I was poking here and there, and noticed that if I use the htot
>> value from the spec (2844), instead of the current value (1896 for most
>> resolutions), 1280x720 works, and the PHY errors are gone.
>>
>> Testing more, I found out that the smaller the htot, the more unreliable the
>> RX becomes, and going down from 2844, somewhere around 2400 I start to see
>> errors.
>>
> 
> That's a good finding!
> 
> I recall I had issues as well with that mode, and fixed them by
> doubling the MIPI bus clock speed You might have noticed these lines
> in the CSI-2 clock tree calculation function ov5640_set_mipi_pclk()
> 
> 	/*
> 	 * 1280x720 is reported to use 'SUBSAMPLING' only,
> 	 * but according to the sensor manual it goes through the
> 	 * scaler before subsampling.
> 	 */
> 	if (mode->dn_mode == SCALING ||
> 	   (mode->id == OV5640_MODE_720P_1280_720))
> 		mipi_div = OV5640_MIPI_DIV_SCLK; // THIS is 1
> 	else
> 		mipi_div = OV5640_MIPI_DIV_PCLK; // THIS is 2: halve the MIPI clock speed
> 
> I had that mode working, but after a good year or so trying to decode
> the clock tree of the sensor with only partially satisfactory results,
> I can't tell if that was by accident or not :)

The comment says that the above is according to the sensor manual, but I couldn't find mention of 
that. Do you recall where you found that information?

>> I'm not that much familiar with CSI-2, and very little with OV5640. Does
>> anyone have a clue about what I'm observing here? Does 1280x720@30 work on
> 
> Hugues made a great effort by sampling the actual frequencies on the
> bus, and he found out the actual frequencies are off compared to my
> theoretical calculations. After that I've actually dropped the ball and
> moved on, but maybe throwing your htot findings in the mix could help?
> 
> Here you have the thread with more information and Hugues measurement
> results:
> https://patchwork.kernel.org/patch/11019673/
> 
>> other platforms with CSI2? Where do the current OV5640 video timings come
>> from?
>>
> 
> I suggest you have a look at
> dfbfb7aa832c ("media: ov5640: Compute the clock rate at runtime")
> 
> htot is used to calculate the desired pixel clock, so it could indeed
> be one of the reasons why the above clock tree calculations are off.
> 
> Hope it helps a bit.

Thanks! Seems that this is all a bit of a detective work =). I have no means to measure the CSI 
clock/data lanes, so debugging this is obviously rather frustrating.

  Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: OV5640 CSI2 problemsg

[Jacopo Mondi]

Hi Tomi,

On Tue, Mar 17, 2020 at 02:40:35PM +0200, Tomi Valkeinen wrote:
> On 17/03/2020 12:22, Jacopo Mondi wrote:
> > Hi Tomi,
> >     welcome to the ov5640 bandwagon
> >
> > This driver received lot of attention and reworking, but there are
> > indeed several issues at the moment :(
> >
> > On Fri, Mar 13, 2020 at 01:15:33PM +0200, Tomi Valkeinen wrote:
> > > Hi all,
> > >
> > > I've been testing and debugging OV5640 with TI's DRA76 and AM65 platforms,
> > > which have the CAL IP for MIPI CSI2 RX.
> > >
> > > The most clear problem is that 1280x720@30 doesn't work at all, but with all
> > > resolutions I can see occasional PHY errors reported when starting the
> > > streaming.
> >
> > I've been testing a CSI-2 setup with 2 data lanes on an IMX6 platform,
> > just for the record
>
> Two lanes here too.
>
> > > The OV5640 spec lists the video timings, but I haven't been able to figure
> > > out what exactly they mean, as e.g. the vsync time doesn't seem to match the
> > > other times according to my calculations.
> > >
> >
> > Are you referring to the ov5640_mode_info structures ?
>
> Yes.
>
> Looking at the git log, these values seem to have been there from the start.
> Initially in the raw register sequences, then moved from there to
> ov5640_mode_info. But the numbers have been the same.
>

Yup, that's my understanding as well

> I wonder where they came originally, and whether they have ever been correct.
>
> Perhaps I'll cook up a patch where I'll update the values to what the sensor
> sheet suggests, and other people can try and see if the driver still works
> for them.
>
> > > In any case, I was poking here and there, and noticed that if I use the htot
> > > value from the spec (2844), instead of the current value (1896 for most
> > > resolutions), 1280x720 works, and the PHY errors are gone.
> > >
> > > Testing more, I found out that the smaller the htot, the more unreliable the
> > > RX becomes, and going down from 2844, somewhere around 2400 I start to see
> > > errors.
> > >
> >
> > That's a good finding!
> >
> > I recall I had issues as well with that mode, and fixed them by
> > doubling the MIPI bus clock speed You might have noticed these lines
> > in the CSI-2 clock tree calculation function ov5640_set_mipi_pclk()
> >
> > 	/*
> > 	 * 1280x720 is reported to use 'SUBSAMPLING' only,
> > 	 * but according to the sensor manual it goes through the
> > 	 * scaler before subsampling.
> > 	 */
> > 	if (mode->dn_mode == SCALING ||
> > 	   (mode->id == OV5640_MODE_720P_1280_720))
> > 		mipi_div = OV5640_MIPI_DIV_SCLK; // THIS is 1
> > 	else
> > 		mipi_div = OV5640_MIPI_DIV_PCLK; // THIS is 2: halve the MIPI clock speed
> >
> > I had that mode working, but after a good year or so trying to decode
> > the clock tree of the sensor with only partially satisfactory results,
> > I can't tell if that was by accident or not :)
>
> The comment says that the above is according to the sensor manual, but I
> couldn't find mention of that. Do you recall where you found that
> information?
>

In my datasheet version (2.03) page 22 reports a list of modes and
the associated "scaling method". 1280x720 is reported as "cropping +
subsampling" that's where I got the mode "goes through the scaler".

> > > I'm not that much familiar with CSI-2, and very little with OV5640. Does
> > > anyone have a clue about what I'm observing here? Does 1280x720@30 work on
> >
> > Hugues made a great effort by sampling the actual frequencies on the
> > bus, and he found out the actual frequencies are off compared to my
> > theoretical calculations. After that I've actually dropped the ball and
> > moved on, but maybe throwing your htot findings in the mix could help?
> >
> > Here you have the thread with more information and Hugues measurement
> > results:
> > https://patchwork.kernel.org/patch/11019673/
> >
> > > other platforms with CSI2? Where do the current OV5640 video timings come
> > > from?
> > >
> >
> > I suggest you have a look at
> > dfbfb7aa832c ("media: ov5640: Compute the clock rate at runtime")
> >
> > htot is used to calculate the desired pixel clock, so it could indeed
> > be one of the reasons why the above clock tree calculations are off.
> >
> > Hope it helps a bit.
>
> Thanks! Seems that this is all a bit of a detective work =). I have no means

It is I'm afraid.

Thanks for your effort!

> to measure the CSI clock/data lanes, so debugging this is obviously rather
> frustrating.
>
>  Tomi
>
> --
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: OV5640 CSI2 problemsg

[Tomi Valkeinen]

On 17/03/2020 14:40, Tomi Valkeinen wrote:

> Looking at the git log, these values seem to have been there from the start. Initially in the raw 
> register sequences, then moved from there to ov5640_mode_info. But the numbers have been the same.
> 
> I wonder where they came originally, and whether they have ever been correct.
> 
> Perhaps I'll cook up a patch where I'll update the values to what the sensor sheet suggests, and 
> other people can try and see if the driver still works for them.

Here's an RFC patch which makes everything work for me.

The numbers are from the datasheet, "DVP timing specifications" table. I have to say it's not quite clear to me how all those numbers should be used, and how they match to the registers. The htot value (4) seems quite obvious, though. But then, how to come up with the vtot, I don't know. The current vtot values work for me, but where they come from, or how to calculate them, I have no idea.

So, this is obviously a bit of a hack. But if the change works for everyone else, and fixes issues on my setup, I wish it could be merged.


From a5d6caf1369396a82e9b4f9bf731194ca4907dbd Mon Sep 17 00:00:00 2001
From: Tomi Valkeinen <tomi.valkeinen@ti.com>
Date: Fri, 13 Mar 2020 10:30:02 +0200
Subject: [PATCH] media: ov5640: adjust htot

Adjust htot for most of the modes. The numbers are from the OV5640
datasheet, and with these the driver works more reliably on DRA76 EVM +
OV5640, using 2 datalanes.

Without the patch, I see often ComplexIO (i.e. PHY) errors when
starting the streaming, and 1280x720 does not work at all without this
change.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/media/i2c/ov5640.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 2fe4a7ac0592..736b286ebb4b 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -551,42 +551,42 @@ static const struct ov5640_mode_info ov5640_mode_init_data = {
 static const struct ov5640_mode_info
 ov5640_mode_data[OV5640_NUM_MODES] = {
 	{OV5640_MODE_QCIF_176_144, SUBSAMPLING,
-	 176, 1896, 144, 984,
+	 176, 2844, 144, 984,
 	 ov5640_setting_QCIF_176_144,
 	 ARRAY_SIZE(ov5640_setting_QCIF_176_144),
 	 OV5640_30_FPS},
 	{OV5640_MODE_QVGA_320_240, SUBSAMPLING,
-	 320, 1896, 240, 984,
+	 320, 2844, 240, 984,
 	 ov5640_setting_QVGA_320_240,
 	 ARRAY_SIZE(ov5640_setting_QVGA_320_240),
 	 OV5640_30_FPS},
 	{OV5640_MODE_VGA_640_480, SUBSAMPLING,
-	 640, 1896, 480, 1080,
+	 640, 2844, 480, 1080,
 	 ov5640_setting_VGA_640_480,
 	 ARRAY_SIZE(ov5640_setting_VGA_640_480),
 	 OV5640_60_FPS},
 	{OV5640_MODE_NTSC_720_480, SUBSAMPLING,
-	 720, 1896, 480, 984,
+	 720, 2844, 480, 984,
 	 ov5640_setting_NTSC_720_480,
 	 ARRAY_SIZE(ov5640_setting_NTSC_720_480),
 	OV5640_30_FPS},
 	{OV5640_MODE_PAL_720_576, SUBSAMPLING,
-	 720, 1896, 576, 984,
+	 720, 2844, 576, 984,
 	 ov5640_setting_PAL_720_576,
 	 ARRAY_SIZE(ov5640_setting_PAL_720_576),
 	 OV5640_30_FPS},
 	{OV5640_MODE_XGA_1024_768, SUBSAMPLING,
-	 1024, 1896, 768, 1080,
+	 1024, 2844, 768, 1080,
 	 ov5640_setting_XGA_1024_768,
 	 ARRAY_SIZE(ov5640_setting_XGA_1024_768),
 	 OV5640_30_FPS},
 	{OV5640_MODE_720P_1280_720, SUBSAMPLING,
-	 1280, 1892, 720, 740,
+	 1280, 2844, 720, 740,
 	 ov5640_setting_720P_1280_720,
 	 ARRAY_SIZE(ov5640_setting_720P_1280_720),
 	 OV5640_30_FPS},
 	{OV5640_MODE_1080P_1920_1080, SCALING,
-	 1920, 2500, 1080, 1120,
+	 1920, 2844, 1080, 1120,
 	 ov5640_setting_1080P_1920_1080,
 	 ARRAY_SIZE(ov5640_setting_1080P_1920_1080),
 	 OV5640_30_FPS},

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[RFC] media: i2c: ov5640: Rework CSI-2 clock tree

[Jacopo Mondi]

Re-work the ov5640_set_mipi_pclk() function to use the requested bandwidth per
lane as the desired output of the PLL generated SYSCLK.

Take into account and more clearly document the clock tree constraints reported
in the PLL diagrams. Most values are still fixed to only support 16bpp YUV
formats with a 2 lanes CSI-2 setup.

Tested by capturing and validating images in all sensor supported resolutions
at 15 and 30 FPS. All images are valid but the desired FPS is only achieved
in 30 FPS mode.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

---
Hi Tomi,
  I finally had some time to go test your patch and on top rework a bit the
messy clock calculation function for MIPI CSI-2

With this patch applied I have verified I have the following results:
I have tested YUYV_2X8 mode with the following resultions

176x144 320x240 640x480 720x480 720x576 1024x768 1280x720 1920x1080 2592x1944

I get correct images for all resolutions capturing at 15FPS and 30FPS \o/
(2592x1944 at 15FPS only).

I then tried to measure the accuracy of the clock tree settings by capturing
with yavta and discarding images, to get and FPS estimation and these are the
(improvable) results

2592x1944 @10FPS = 5 FPS
2592x1944 @15FPS = 7.5 FPS

1920x1080 @10FPS = 5 FPS
1920x1080 @15FPS = 7.5 FPS
1920x1080 @30FPS = 30 FPS

1280x720 @10FPS = 5 FPS
1280x720 @15FPS = broken ?
1280x720 @30FPS = 30 FPS

1024x768 @10FPS = 5 FPS
1024x768 @15FPS = 7.5 FPS
1024x768 @30FPS = 30 FPS

720x480 @10FPS = 5 FPS
720x480 @15FPS = 7.5 FPS
720x480 @30FPS = 30 FPS

640x480 @10FPS = 5 FPS
640x480 @15FPS = 7.5 FPS
640x480 @30FPS = 30 FPS
640x480 @60FPS = 45 FPS

320x240 @10FPS = 5 FPS
320x240 @15FPS = 7.5 FPS
320x240 @30FPS = 30 FPS
320x240 @60FPS = 30 FPS

Good news is that the 30FPS mode seems correct, that full resolution works
and that results are consistent.

Bad new is that results are still a bit off :/

Do you think you could this give these a try on your platform ?
FYI I have tested on an IMX6 board

Thank you
   j

---
 drivers/media/i2c/ov5640.c | 117 +++++++++++++++++++++----------------
 1 file changed, 68 insertions(+), 49 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 736b286ebb4b..031a047f1366 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -917,67 +917,87 @@ static unsigned long ov5640_calc_sys_clk(struct ov5640_dev *sensor,
 /*
  * ov5640_set_mipi_pclk() - Calculate the clock tree configuration values
  *			    for the MIPI CSI-2 output.
- *
- * @rate: The requested bandwidth per lane in bytes per second.
- *	  'Bandwidth Per Lane' is calculated as:
- *	  bpl = HTOT * VTOT * FPS * bpp / num_lanes;
- *
- * This function use the requested bandwidth to calculate:
- * - sample_rate = bpl / (bpp / num_lanes);
- *	         = bpl / (PLL_RDIV * BIT_DIV * PCLK_DIV * MIPI_DIV / num_lanes);
- *
- * - mipi_sclk   = bpl / MIPI_DIV / 2; ( / 2 is for CSI-2 DDR)
- *
- * with these fixed parameters:
- *	PLL_RDIV	= 2;
- *	BIT_DIVIDER	= 2; (MIPI_BIT_MODE == 8 ? 2 : 2,5);
- *	PCLK_DIV	= 1;
- *
- * The MIPI clock generation differs for modes that use the scaler and modes
- * that do not. In case the scaler is in use, the MIPI_SCLK generates the MIPI
- * BIT CLk, and thus:
- *
- * - mipi_sclk = bpl / MIPI_DIV / 2;
- *   MIPI_DIV = 1;
- *
- * For modes that do not go through the scaler, the MIPI BIT CLOCK is generated
- * from the pixel clock, and thus:
- *
- * - sample_rate = bpl / (bpp / num_lanes);
- *	         = bpl / (2 * 2 * 1 * MIPI_DIV / num_lanes);
- *		 = bpl / (4 * MIPI_DIV / num_lanes);
- * - MIPI_DIV	 = bpp / (4 * num_lanes);
+ * @rate: The requested bitrate in bits per second.
  *
  * FIXME: this have been tested with 16bpp and 2 lanes setup only.
- * MIPI_DIV is fixed to value 2, but it -might- be changed according to the
- * above formula for setups with 1 lane or image formats with different bpp.
- *
  * FIXME: this deviates from the sensor manual documentation which is quite
- * thin on the MIPI clock tree generation part.
+ *	  thin on the MIPI clock tree generation part.
  */
 static int ov5640_set_mipi_pclk(struct ov5640_dev *sensor,
 				unsigned long rate)
 {
-	const struct ov5640_mode_info *mode = sensor->current_mode;
+	u8 bit_div, mipi_div, pclk_div, sclk_div, sclk2x_div, pll_div;
+	u32 lanebandwidth, sysclk;
 	u8 prediv, mult, sysdiv;
-	u8 mipi_div;
 	int ret;

 	/*
-	 * 1280x720 is reported to use 'SUBSAMPLING' only,
-	 * but according to the sensor manual it goes through the
-	 * scaler before subsampling.
+	 * The 'rate' parameter is the bitrate = VTOT * HTOT * FPS * BPP
+	 *
+	 * Adjust it to represent the bandwidth per CSI-2 data lane and
+	 * set the PLL produced SYSCLK to that frequency.
 	 */
-	if (mode->dn_mode == SCALING ||
-	   (mode->id == OV5640_MODE_720P_1280_720))
-		mipi_div = OV5640_MIPI_DIV_SCLK;
+	lanebandwidth = rate / sensor->ep.bus.mipi_csi2.num_data_lanes;
+	ov5640_calc_sys_clk(sensor, lanebandwidth, &prediv, &mult, &sysdiv);
+
+	/*
+	 * Adjust PLL parameters to maintain the MIPI_SCLK-to-PCLK ratio;
+	 *
+	 * - bit_div : MIPI 8-bit = 2
+	 *	       MIPI 10-bit = 2,5
+	 *
+	 * - pll_div = 2 (fixed)
+	 *
+	 * - pclk_div: pclk_div = (1 lane) ? 2 : 1;
+	 *   2 lanes: MIPI_SCLK = (4 or 5) * PCLK
+	 *   1 lanes: MIPI_SCLK = (8 or 10) * PCLK
+	 *
+	 * TODO: support 10-bit formats
+	 * TODO: support 1 lane
+	 */
+	bit_div =  OV5640_PLL_CTRL0_MIPI_MODE_8BIT;
+	pclk_div = OV5640_PLL_SYS_ROOT_DIVIDER_BYPASS;
+	pll_div = OV5640_PLL_CTRL3_PLL_ROOT_DIV_2;
+
+	/*
+	 * - mipi_div - Assumptions not supported by documentation
+	 *
+	 *   The MIPI clock generation differs for modes that use the scaler and
+	 *   modes that do not.
+	 *
+	 *   In case the scaler is in use
+	 *   mipi_div = 1
+	 *
+	 *   If mode uses cropping + sub-sampling
+	 *   mipi_div = 2
+	 *
+	 * FIXME: mipi_div values verified for 16bpp with 2 data lanes
+	 */
+	if (sensor->current_mode->dn_mode == SCALING)
+		mipi_div = 1;
 	else
-		mipi_div = OV5640_MIPI_DIV_PCLK;
+		mipi_div = 2;

-	ov5640_calc_sys_clk(sensor, rate, &prediv, &mult, &sysdiv);
+	/*
+	 * Scaler clock:
+	 * - YUV: SCLK = 1/2 PCLK
+	 *   sclk_div = 2 * pclk_div * mipi_div
+	 * - RAW or JPEG: SCLK = PCLK
+	 *   sclk_div = pclk_div * mipi_div
+	 * - sclk2x_div = sclk_div / 2
+	 *
+	 * TODO: add support for RAW and JPEG modes. To maintain the
+	 * sclk2x_div to sclk_div ratio, the pclk_div should probably be
+	 * adjusted as well to 2 and PCLK recalculated.
+	 */
+	sclk_div = ilog2(2);
+	sclk2x_div = ilog2(1);

+	/* Program the clock tree registers. */
 	ret = ov5640_mod_reg(sensor, OV5640_REG_SC_PLL_CTRL0,
-			     0x0f, OV5640_PLL_CTRL0_MIPI_MODE_8BIT);
+			     0x0f, bit_div);
+	if (ret)
+		return ret;

 	ret = ov5640_mod_reg(sensor, OV5640_REG_SC_PLL_CTRL1,
 			     0xff, sysdiv << 4 | mipi_div);
@@ -989,12 +1009,12 @@ static int ov5640_set_mipi_pclk(struct ov5640_dev *sensor,
 		return ret;

 	ret = ov5640_mod_reg(sensor, OV5640_REG_SC_PLL_CTRL3,
-			     0x1f, OV5640_PLL_CTRL3_PLL_ROOT_DIV_2 | prediv);
+			     0x1f, pll_div | prediv);
 	if (ret)
 		return ret;

-	return ov5640_mod_reg(sensor, OV5640_REG_SYS_ROOT_DIVIDER,
-			      0x30, OV5640_PLL_SYS_ROOT_DIVIDER_BYPASS);
+	return ov5640_mod_reg(sensor, OV5640_REG_SYS_ROOT_DIVIDER, 0x3f,
+			      (pclk_div << 4) | (sclk2x_div << 2) | sclk_div);
 }

 static unsigned long ov5640_calc_pclk(struct ov5640_dev *sensor,
@@ -1840,7 +1860,6 @@ static int ov5640_set_mode(struct ov5640_dev *sensor)
 	 */
 	rate = ov5640_calc_pixel_rate(sensor) * 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;
--
2.26.2