Re: [PATCH v4 01/12] media: ov5640: Adjust the clock based on the expected rate

[Sam Bobrowicz]

Hello Maxime and Jacopo (and other ov5640-ers),

I just submitted my version of this patch to the mailing list as RFC.
It is working on my MIPI platform. Please try it if you have time.
Hopefully we can merge these two into a single patch that doesn't
break any platforms.

Thanks,
Sam

Additional notes below.

On Tue, Oct 16, 2018 at 9:54 AM jacopo mondi  wrote:
>
> Hello Maxime,
>a few comments I have collected while testing the series.
>
> Please see below.
>
> On Thu, Oct 11, 2018 at 11:20:56AM +0200, 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 
> > ---
> >  drivers/media/i2c/ov5640.c | 289 -
> >  1 file changed, 288 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> > index 30b15e91d8be..88fb16341466 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,
> > + 5600,
> >   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,
> > +  2800,
> >ov5640_setting_15fps_QCIF_176_144,
> >ARRAY_SIZE(ov5640_setting_15fps_QCIF_176_144)},
> >   {OV5640_MODE_QVGA_320_240, SUBSAMPLING,
> >320, 1896, 240, 984,
> > +  2800,
> >ov5640_setting_15fps_QVGA_320_240,
> >ARRAY_SIZE(ov5640_setting_15fps_QVGA_320_240)},
> >   {OV5640_MODE_VGA_640_480, SUBSAMPLING,
> >640, 1896, 480, 1080,
> > +  2800,
> >ov5640_setting_15fps_VGA_640_480,
> >ARRAY_SIZE(ov5640_setting_15fps_VGA_640_480)},
> >   {OV5640_MODE_NTSC_720_480, SUBSAMPLING,
> >720, 1896, 480, 984,
> > +  2800,
> >ov5640_setting_15fps_NTSC_720_480,
> >ARRAY_SIZE(ov5640_setting_15fps_NTSC_720_480)},
> >   {OV5640_MODE_PAL_720_576, SUBSAMPLING,
> >720, 1896, 576, 984,
> > +  2800,
> >ov5640_setting_15fps_PAL_720_576,
> >ARRAY_SIZE(ov5640_setting_15fps_PAL_720_576)},
> >   {OV5640_MODE_XGA_1024_768, SUBSAMPLING,
> >1024, 1896, 768, 1080,
> > +  2800,
> >ov5640_setting_15fps_XGA_1024_768,
> >ARRAY_SIZE(ov5640_setting_15fps_XGA_1024_768)},
> >   {OV5640_MODE_720P_1280_720, SUBSAMPLING,
> >1280, 1892, 720, 740,
> > +  2100,
> >ov5640_setting_15fps_720P_1280_720,
> >ARRAY_SIZE(ov5640_setting_15fps_720P_1280_720)},
> >   {OV5640_MODE_1080P_1920_1080, SCALING,
> >1920, 2500, 1080, 1120,
> > +  4200,
> >ov5640_setting_15fps_1080P_1920_1080,
> >ARRAY_SIZE(ov5640_setting_15fps_1080P_1920_1080)},
> >   {OV5640_MODE_QSXGA_2592_1944, SCALING,
> >2592, 2844, 1944, 1968,
> > +  8400,
> >ov5640_setting_15fps_QSXGA_2592_1944,
> >ARRAY_SIZE(ov5640_setting_15fps_QSXGA_2592_1944)},
> >   }, {
> >   {OV5640_MODE_QCIF_176_144, SUBSAMPLING,
> >176, 1896, 144, 984,
> > +  5600,
> >ov5640_setting_30fps_QCIF_176_144,
> >

[RFC PATCH] media: ov5640: calculate PLL settings for modes

[Sam Bobrowicz]

Remove the PLL settings from the register blobs and
calculate them based on required clocks. This allows
more mode and input clock (xclk) configurations.

Also ensure that PCLK PERIOD register 0x4837 is set
so that MIPI receivers are not broken by this patch.

Last, a change to the init register blob that helps
ensure the following DPHY spec requirement is met:

MIN HS_ZERO + MIN HS_PREPARE > 145 + t_UI * 10

Signed-off-by: Sam Bobrowicz 
---

 This is a modified version of Maxime's patch that works on my platform.
 My platform is a dual-lane MIPI CSI2 module with xclk=24MHz connected 
 to a Xilinx Zynq Ultrascale+.

 One issue I am currently experiencing with this patch is that some 
 15Hz framerates are not working. This seems to be due to the slower 
 clocks which are generated, and may be caused by the large ADC clock
 to SCLK ratio. I will be exploring some fixes this weekend. Thoughts on
 this would be appreciated.
 
 I am submitting this so that it can be compared to Maxime's, which has 
 been reported to not be functional on MIPI platforms. I do a number of
 things differently, and I hope that those which are useful will be 
 integrated into his patch. 

 I think this patch (or the modified version of Maxime's patch) should 
 be tested under the following conditions:

 1) MIPI mode, xclk=24MHz
 2) MIPI mode, xclk!=24MHz
 3) DVP mode
 4) JPEG format

 I'm setup to test the first two, but don't have the hardware/software 
 to test 3 and 4. 

 This patch is based on the current master of media_linux 
 "media: ov5640: fix framerate update".

 drivers/media/i2c/ov5640.c | 332 ++---
 1 file changed, 281 insertions(+), 51 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index eaefdb5..c076955 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -85,6 +85,7 @@
 #define OV5640_REG_POLARITY_CTRL00 0x4740
 #define OV5640_REG_MIPI_CTRL00 0x4800
 #define OV5640_REG_DEBUG_MODE  0x4814
+#define OV5640_REG_PCLK_PERIOD 0x4837
 #define OV5640_REG_ISP_FORMAT_MUX_CTRL 0x501f
 #define OV5640_REG_PRE_ISP_TEST_SET1   0x503d
 #define OV5640_REG_SDE_CTRL0   0x5580
@@ -94,9 +95,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,
@@ -171,6 +169,7 @@ struct reg_value {
 struct ov5640_mode_info {
enum ov5640_mode_id id;
enum ov5640_downsize_mode dn_mode;
+   bool scaler; /* Mode uses ISP scaler (reg 0x5001,BIT(5)=='1') */
u32 hact;
u32 htot;
u32 vact;
@@ -291,7 +290,7 @@ static const struct reg_value 
ov5640_init_setting_30fps_VGA[] = {
{0x302e, 0x08, 0, 0}, {0x4300, 0x3f, 0, 0},
{0x501f, 0x00, 0, 0}, {0x4713, 0x03, 0, 0}, {0x4407, 0x04, 0, 0},
{0x440e, 0x00, 0, 0}, {0x460b, 0x35, 0, 0}, {0x460c, 0x22, 0, 0},
-   {0x4837, 0x0a, 0, 0}, {0x3824, 0x02, 0, 0},
+   {0x3824, 0x02, 0, 0}, {0x482a, 0x06, 0, 0},
{0x5000, 0xa7, 0, 0}, {0x5001, 0xa3, 0, 0}, {0x5180, 0xff, 0, 0},
{0x5181, 0xf2, 0, 0}, {0x5182, 0x00, 0, 0}, {0x5183, 0x14, 0, 0},
{0x5184, 0x25, 0, 0}, {0x5185, 0x24, 0, 0}, {0x5186, 0x09, 0, 0},
@@ -345,7 +344,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},
@@ -364,7 +363,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},
@@ -383,7 +382,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},
@@ -399,11 +398,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}, {0x460

[PATCH 1/4] media: ov5640: fix resolution update

[Sam Bobrowicz]

set_fmt was not properly triggering a mode change when
a new mode was set that happened to have the same format
as the previous mode (for example, when only changing the
frame dimensions). Fix this.

Signed-off-by: Sam Bobrowicz 
---
 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 eaefdb5..5031aab 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -2045,12 +2045,12 @@ static int ov5640_set_fmt(struct v4l2_subdev *sd,
goto out;
}
 
-   if (new_mode != sensor->current_mode) {
+
+   if (new_mode != sensor->current_mode ||
+   mbus_fmt->code != sensor->fmt.code) {
+   sensor->fmt = *mbus_fmt;
sensor->current_mode = new_mode;
sensor->pending_mode_change = true;
-   }
-   if (mbus_fmt->code != sensor->fmt.code) {
-   sensor->fmt = *mbus_fmt;
sensor->pending_fmt_change = true;
}
 out:
-- 
2.7.4

[PATCH 3/4] media: ov5640: Don't access ctrl regs when off

[Sam Bobrowicz]

Add a check to g_volatile_ctrl to prevent trying to read
registers when the sensor is not powered.

Signed-off-by: Sam Bobrowicz 
---
 drivers/media/i2c/ov5640.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index f183222..a50d884 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -2336,6 +2336,13 @@ static int ov5640_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
 
/* v4l2_ctrl_lock() locks our own mutex */
 
+   /*
+* If the sensor is not powered up by the host driver, do
+* not try to access it to update the volatile controls.
+*/
+   if (sensor->power_count == 0)
+   return 0;
+
switch (ctrl->id) {
case V4L2_CID_AUTOGAIN:
val = ov5640_get_gain(sensor);
-- 
2.7.4

[PATCH 0/4] ov5640: small fixes for compatibility

[Sam Bobrowicz]

These patches include fixes for some minor annoyances and also help 
increase compatibility with some CSI2 drivers.

Based on latest media_tree commit: "media: ov5640: fix framerate update"

Sam Bobrowicz (4):
  media: ov5640: fix resolution update
  media: ov5640: fix get_light_freq on auto
  media: ov5640: Don't access ctrl regs when off
  media: ov5640: Add additional media bus formats

 drivers/media/i2c/ov5640.c | 20 
 1 file changed, 16 insertions(+), 4 deletions(-)

-- 
2.7.4

[PATCH 4/4] media: ov5640: Add additional media bus formats

[Sam Bobrowicz]

Add support for 1X16 yuv media bus formats (v4l2_mbus_framefmt).
These formats are equivalent to the 2X8 formats that are already
supported, both of which accurately describe the data present on
the CSI2 interface. This change will increase compatibility with
CSI2 RX drivers that only advertise support for the 1X16 formats.

Signed-off-by: Sam Bobrowicz 
---
 drivers/media/i2c/ov5640.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index a50d884..ca9de56 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -125,6 +125,8 @@ static const struct ov5640_pixfmt ov5640_formats[] = {
{ MEDIA_BUS_FMT_JPEG_1X8, V4L2_COLORSPACE_JPEG, },
{ MEDIA_BUS_FMT_UYVY8_2X8, V4L2_COLORSPACE_SRGB, },
{ MEDIA_BUS_FMT_YUYV8_2X8, V4L2_COLORSPACE_SRGB, },
+   { MEDIA_BUS_FMT_UYVY8_1X16, V4L2_COLORSPACE_SRGB, },
+   { MEDIA_BUS_FMT_YUYV8_1X16, V4L2_COLORSPACE_SRGB, },
{ MEDIA_BUS_FMT_RGB565_2X8_LE, V4L2_COLORSPACE_SRGB, },
{ MEDIA_BUS_FMT_RGB565_2X8_BE, V4L2_COLORSPACE_SRGB, },
 };
@@ -2069,10 +2071,12 @@ static int ov5640_set_framefmt(struct ov5640_dev 
*sensor,
 
switch (format->code) {
case MEDIA_BUS_FMT_UYVY8_2X8:
+   case MEDIA_BUS_FMT_UYVY8_1X16:
/* YUV422, UYVY */
val = 0x3f;
break;
case MEDIA_BUS_FMT_YUYV8_2X8:
+   case MEDIA_BUS_FMT_YUYV8_1X16:
/* YUV422, YUYV */
val = 0x30;
break;
-- 
2.7.4

[PATCH 2/4] media: ov5640: fix get_light_freq on auto

[Sam Bobrowicz]

Light frequency was not properly returned when in auto
mode and the detected frequency was 60Hz. Fix this.

Signed-off-by: Sam Bobrowicz 
---
 drivers/media/i2c/ov5640.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 5031aab..f183222 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -1295,6 +1295,7 @@ static int ov5640_get_light_freq(struct ov5640_dev 
*sensor)
light_freq = 50;
} else {
/* 60Hz */
+   light_freq = 60;
}
}
 
-- 
2.7.4

Re: [PATCH v3 03/12] media: ov5640: Remove the clocks registers initialization

[Sam Bobrowicz]

>> On May 21, 2018, at 12:39 AM, Maxime Ripard  
>> wrote:
>>
>>> On Fri, May 18, 2018 at 07:42:34PM -0700, Sam Bobrowicz wrote:
>>>> On Fri, May 18, 2018 at 3:35 AM, Daniel Mack  wrote:
>>>> On Thursday, May 17, 2018 10:53 AM, Maxime Ripard wrote:
>>>>
>>>> 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 
>>>> ---
>>>
>>>
>>> [...]
>>>
>>>> @@ -625,8 +623,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},
>>>
>>>
>>> This is the mode that I'm testing with. Previously, the hard-coded registers
>>> here were:
>>>
>>> OV5640_REG_SC_PLL_CTRL1 (0x3035) = 0x11
>>> OV5640_REG_SC_PLL_CTRL2 (0x3036) = 0x54
>>> OV5640_REG_SC_PLL_CTRL3 (0x3037) = 0x07
>>>
>>> Your new code that calculates the clock rates dynamically ends up with
>>> different values however:
>>>
>>> OV5640_REG_SC_PLL_CTRL1 (0x3035) = 0x11
>>> OV5640_REG_SC_PLL_CTRL2 (0x3036) = 0xa8
>>> OV5640_REG_SC_PLL_CTRL3 (0x3037) = 0x03
>>>
>>> Interestingly, leaving the hard-coded values in the array *and* letting
>>> ov5640_set_mipi_pclk() do its thing later still works. So again it seems
>>> that writes to registers after 0x3035/0x3036/0x3037 seem to depend on the
>>> values of these timing registers. You might need to leave these values as
>>> dummies in the array. Confusing.
>>>
>>> Any idea?
>>>
>>>
>>> Thanks,
>>> Daniel
>>
>> This set of patches is also not working for my MIPI platform (mine has
>> a 12 MHz external clock). I am pretty sure is isn't working because it
>> does not include the following, which my tests have found to be
>> necessary:
>>
>> 1) Setting pclk period reg in order to correct DPHY timing.
>> 2) Disabling of MIPI lanes when streaming not enabled.
>> 3) setting mipi_div to 1 when the scaler is disabled
>> 4) Doubling ADC clock on faster resolutions.
>
> Yeah, I left them out because I didn't think this was relevant to this
> patchset but should come as future improvements. However, given that
> it works with the parallel bus, maybe the two first are needed when
> adjusting the rate.
>
I agree that 1-4 are separate improvements to MIPI mode that may not
affect all modules. They do break mine, but that has been true since
the driver was released (mainly because of my 12 MHz clock and more
stringent CSI RX requirements). So it makes sense for me to address
them in a follow-up series. But I do think that we should get the
clock generation a little closer to what I know works for MIPI so we
don't break things for people that do have MIPI working.

> The mipi divider however seems to be a bit more complicated than you
> report here. It is indeed set to 1 when the scaler is enabled (all
> resolutions > 1280 * 960), but it's also set to 4 in some cases
> (640x480@30, 320x240@30, 176x144@30). I couldn't really find any
> relationship between the resolution/framerate and whether to use a
> divider of 2 or 4.

I didn't notice the divide by 4, interesting. I have a theory
though... it could be that the constraint of PCLK relative to SCLK is:

SCLK*(cpp/scaler ratio)<=PCLK<= ?
cpp=Components/pixel (1 for JPEG, 2 for YUV, e.g.)

Since the scaler is in auto mode, the scaler ratio might automatically
change depending on the resolution selected, something like (using int
math):

(hTotal/hActive) * (vTotal/vActive) = scaler ratio

If SCLK is responsible for reading the data into the scaler, and PCLK
is responsible for reading data out to the physical interface, this
would make sense. It will require more experiments to verify any of
this, though, and unfortunately I don't have a lot of time to put into
this right now.

On my platform I have also run i

Re: [PATCH v3 03/12] media: ov5640: Remove the clocks registers initialization

[Sam Bobrowicz]

On Fri, May 18, 2018 at 3:35 AM, Daniel Mack  wrote:
> On Thursday, May 17, 2018 10:53 AM, Maxime Ripard wrote:
>>
>> 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 
>> ---
>
>
> [...]
>
>> @@ -625,8 +623,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},
>
>
> This is the mode that I'm testing with. Previously, the hard-coded registers
> here were:
>
>  OV5640_REG_SC_PLL_CTRL1 (0x3035) = 0x11
>  OV5640_REG_SC_PLL_CTRL2 (0x3036) = 0x54
>  OV5640_REG_SC_PLL_CTRL3 (0x3037) = 0x07
>
> Your new code that calculates the clock rates dynamically ends up with
> different values however:
>
>  OV5640_REG_SC_PLL_CTRL1 (0x3035) = 0x11
>  OV5640_REG_SC_PLL_CTRL2 (0x3036) = 0xa8
>  OV5640_REG_SC_PLL_CTRL3 (0x3037) = 0x03
>
> Interestingly, leaving the hard-coded values in the array *and* letting
> ov5640_set_mipi_pclk() do its thing later still works. So again it seems
> that writes to registers after 0x3035/0x3036/0x3037 seem to depend on the
> values of these timing registers. You might need to leave these values as
> dummies in the array. Confusing.
>
> Any idea?
>
>
> Thanks,
> Daniel

This set of patches is also not working for my MIPI platform (mine has
a 12 MHz external clock). I am pretty sure is isn't working because it
does not include the following, which my tests have found to be
necessary:

1) Setting pclk period reg in order to correct DPHY timing.
2) Disabling of MIPI lanes when streaming not enabled.
3) setting mipi_div to 1 when the scaler is disabled
4) Doubling ADC clock on faster resolutions.

I will run some more tests to see if anything else is broken and come
back with some suggestions.

I should mention that the upstream driver has never worked with my
platform. I suspect that the driver only ever worked previously with
MIPI platforms that have loose DPHY timing requirements and a specific
xclk (24MHz maybe?). Out of the interest of collecting more data, can
you provide the following info on your platform?

a) External clock frequency
b) List of resolutions (including framerates) that are working with
these patches (and your fix) applied
c) List of resolutions that were working prior to the regression you
experienced with the set_timings function

Sam

Re: [PATCH v2 00/12] media: ov5640: Misc cleanup and improvements

[Sam Bobrowicz]

> 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 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 (10):
>   media: ov5640: Don't force the auto exposure state at start time
>   media: ov5640: Init properly the SCLK dividers
>   media: ov5640: Change horizontal and vertical resolutions name
>   media: ov5640: Add horizontal and vertical totals
>   media: ov5640: Program the visible resolution
>   media: ov5640: Adjust the clock based on the expected rate
>   media: ov5640: Compute the clock rate at runtime
>   media: ov5640: Enhance FPS handling
>   media: ov5640: Add 60 fps support
>   media: ov5640: Remove duplicate auto-exposure setup
>
> Mylène Josserand (2):
>   media: ov5640: Add auto-focus feature
>   media: ov5640: Add light frequency control
>
>  drivers/media/i2c/ov5640.c | 752 +
>  1 file changed, 422 insertions(+), 330 deletions(-)
>
> --
> 2.17.0
>

As discussed, MIPI required some additional work. Please see the
patches here which add support for MIPI:
https://www.dropbox.com/s/73epty7808yzq1t/ov5640_mipi_fixes.zip?dl=0

The first 3 patches are fixes I believe should be made to earlier
patches prior to submitting v2 of this series. The remaining 4 patches
should probably just be added onto the end of this series as-is (or
with feedback incorporated if needed).

I will note that this is still not working correctly on my system for
any resolution that requires a 672 Mbps mipi rate. This includes
1080p@30hz, full@15hz, and 720p@60hz. My CSI2 receiver is reporting
CRC errors though, so this could be an integrity issue on my module.
I'm curious to hear if others have success at these resolutions.

Please try this out on other MIPI and DVP platforms with as many
different resolutions as possible and let me know if it works.

I'd also recommend that someone add a patch to the series that enables
720p@60hz. I didn't have time to do it yet, but it should be really
easy.

Sam

Re: [PATCH v2 00/12] media: ov5640: Misc cleanup and improvements

[Sam Bobrowicz]

> Hi,
>
>> Good news, MIPI is now working on my platform. I had to make several
>> changes to how the mipi clocking is calculated in order to get things
>> stable, but I think I got it figured out. Maxime's changes were really
>> helpful.
>
> Great, I also try to make it work with MIPI-CSI2, If you have found
> the magic formula to configure the registers, I would be pleased to
> test it on my side.
>
>>
>> I will try to get some patches out today or tomorrow that should get
>> you up and running.
>>
>> Maxime, I'd prefer to create the patches myself for the experience.
>> I've read all of the submission guidelines and I understand the
>> general process, but how should I submit them to the mailing list
>> since they are based to your patches, which are still under review?
>> Should I send the patch series to the mailing list myself and just
>> mention this patch series, maybe with the In-Reply-To header? Or
>> should I just post a link to them here so you can review them and add
>> them to a new version of your patch series?
>
> Yes, I think your patch(es) should be integrated in the Maxime's series.
>
> Regards,
> Loic

Based on Maxime's reply, I have decided to make the patches based off
of that series, post it on dropbox, and provide a link here. Then we
can discuss how to integrate them into v2 of the series.

The patches are taking longer than expected, my tree is pretty chaotic
since I've been running rapid-fire experiments for weeks now. I'm
still working on getting the changes organized into an appropriate set
of patches. I think they will go out over the weekend.

Sam

Re: [PATCH v2 00/12] media: ov5640: Misc cleanup and improvements

[Sam Bobrowicz]

> Hi,
>
> On 3 May 2018 at 17:16, Maxime Ripard <maxime.rip...@bootlin.com> wrote:
> > Hi,
> >
> > On Wed, May 02, 2018 at 11:11:55AM -0700, Sam Bobrowicz wrote:
> >> > On Wednesday, 25 April 2018 01:11:19 EEST Sam Bobrowicz wrote:
> >> >> FYI, still hard at work on this. Did some more experiments last week
> >> >> that seemed to corroborate the clock tree in the spreadsheet. It also
> >> >> seems that the output of the P divider cell, SCLK cell and MIPI Rate
> >> >> cell in the spreadsheet must have a ratio of 2x:1x:8x (respectively)
> >> >> in order for the sensor to work properly on my platform, and that the
> >> >> SCLK value must be close to the "rate" variable that you calculate and
> >> >> pass to set_mipi_pclk. Unfortunately, I've only got the sensor working
> >> >> well for 1080p@15Hz and 720p@30Hz, both with a SCLK of 42MHz (aka
> >> >> 84:42:336). I'm running experiments now trying to adjust the htot and
> >> >> vtot values to create different required rates, and also to try to get
> >> >> faster Mipi rates working. Any information you have on the
> >> >> requirements of the htot and vtot values with respect to vact and hact
> >> >> values would likely be helpful.
> >> >>
> >> >> I'm also keeping an eye on the scaler clock, which I think may be
> >> >> affecting certain resolutions, but haven't been able to see it make a
> >> >> difference yet (see register 0x3824 and 0x460c)
> >> >>
> >> >> I plan on pushing a set of patches once I get this figured out, we can
> >> >> discuss what I should base them on when I get closer to that point.
> >> >> I'm new to this process :)
> >> >
> >> > I'm also interested in getting the ov5640 driver working with MIPI CSI-2.
> >> > Studying the datasheet and the driver code, I found the stream on 
> >> > sequence to
> >> > be a bit weird. In particular the configuration of 
> >> > OV5640_REG_IO_MIPI_CTRL00,
> >> > OV5640_REG_PAD_OUTPUT00 and OV5640_REG_MIPI_CTRL00 caught my attention.
> >> >
> >> > OV5640_REG_IO_MIPI_CTRL00 (0x300e) is set to 0x45 in the large array of 
> >> > init
> >> > mode data and never touched for MIPI CSI-2 (the register is only touched 
> >> > in
> >> > ov5640_set_stream_dvp). The value means
> >> >
> >> > - mipi_lane_mode: 010 is documented as "debug mode", I would have 
> >> > expected 000
> >> > for one lane or 001 for two lanes.
> >>
> >> I noticed this too, but it seems that 010 is the correct value for two
> >> lane mode. I think this is a typo in the datasheet.
> >>
> >> >
> >> > - MIPI TX PHY power down: 0 is documented as "debug mode" and 1 as 
> >> > "Power down
> >> > PHY HS TX", so I suppose 0 is correct.
> >> >
> >> > - MIPI RX PHY power down: 0 is documented as "debug mode" and 1 as 
> >> > "Power down
> >> > PHY LP RX module", so I suppose 0 is correct. I however wonder why 
> >> > there's a
> >> > RX PHY, it could be a typo.
> >> >
> >> > - mipi_en: 1 means MIPI enable, which should be correct.
> >> >
> >> > - BIT(0) is undocumented.
> >> >
> >> > OV5640_REG_PAD_OUTPUT00 (0x3019) isn't initialized explicitly and thus 
> >> > retains
> >> > its default value of 0x00, and is controlled when starting and stopping 
> >> > the
> >> > stream where it's set to 0x00 and 0x70 respectively. Bits 6:4 control the
> >> > "sleep mode" state of lane 2, lane 1 and clock lane respectively, and 
> >> > should
> >> > be LP11 in my opinion (that's the PHY stop state). However, setting them 
> >> > to
> >> > 0x00 when starting the stream mean that LP00 is selected as the sleep 
> >> > state at
> >> > stream start, and LP11 when stopping the stream. Maybe "sleep mode" means
> >> > LPDT, but I would expect that to be controlled by the idle status bit in
> >> > register 0x4800.
> >> >
> >>
> >> I did not need to mess with the accesses to 0x3019 in order to get
> >> things working on my system. I'm not sure of this registers actual
> >> behavior, but it might affect idling while not streamin

Re: [PATCH v2 00/12] media: ov5640: Misc cleanup and improvements

[Sam Bobrowicz]

On Fri, Apr 27, 2018 at 2:27 AM, Laurent Pinchart
<laurent.pinch...@ideasonboard.com> wrote:
> Hi Sam,
>
> On Wednesday, 25 April 2018 01:11:19 EEST Sam Bobrowicz wrote:
>> FYI, still hard at work on this. Did some more experiments last week
>> that seemed to corroborate the clock tree in the spreadsheet. It also
>> seems that the output of the P divider cell, SCLK cell and MIPI Rate
>> cell in the spreadsheet must have a ratio of 2x:1x:8x (respectively)
>> in order for the sensor to work properly on my platform, and that the
>> SCLK value must be close to the "rate" variable that you calculate and
>> pass to set_mipi_pclk. Unfortunately, I've only got the sensor working
>> well for 1080p@15Hz and 720p@30Hz, both with a SCLK of 42MHz (aka
>> 84:42:336). I'm running experiments now trying to adjust the htot and
>> vtot values to create different required rates, and also to try to get
>> faster Mipi rates working. Any information you have on the
>> requirements of the htot and vtot values with respect to vact and hact
>> values would likely be helpful.
>>
>> I'm also keeping an eye on the scaler clock, which I think may be
>> affecting certain resolutions, but haven't been able to see it make a
>> difference yet (see register 0x3824 and 0x460c)
>>
>> I plan on pushing a set of patches once I get this figured out, we can
>> discuss what I should base them on when I get closer to that point.
>> I'm new to this process :)
>
> I'm also interested in getting the ov5640 driver working with MIPI CSI-2.
> Studying the datasheet and the driver code, I found the stream on sequence to
> be a bit weird. In particular the configuration of OV5640_REG_IO_MIPI_CTRL00,
> OV5640_REG_PAD_OUTPUT00 and OV5640_REG_MIPI_CTRL00 caught my attention.
>
> OV5640_REG_IO_MIPI_CTRL00 (0x300e) is set to 0x45 in the large array of init
> mode data and never touched for MIPI CSI-2 (the register is only touched in
> ov5640_set_stream_dvp). The value means
>
> - mipi_lane_mode: 010 is documented as "debug mode", I would have expected 000
> for one lane or 001 for two lanes.

I noticed this too, but it seems that 010 is the correct value for two
lane mode. I think this is a typo in the datasheet.

>
> - MIPI TX PHY power down: 0 is documented as "debug mode" and 1 as "Power down
> PHY HS TX", so I suppose 0 is correct.
>
> - MIPI RX PHY power down: 0 is documented as "debug mode" and 1 as "Power down
> PHY LP RX module", so I suppose 0 is correct. I however wonder why there's a
> RX PHY, it could be a typo.
>
> - mipi_en: 1 means MIPI enable, which should be correct.
>
> - BIT(0) is undocumented.
>
> OV5640_REG_PAD_OUTPUT00 (0x3019) isn't initialized explicitly and thus retains
> its default value of 0x00, and is controlled when starting and stopping the
> stream where it's set to 0x00 and 0x70 respectively. Bits 6:4 control the
> "sleep mode" state of lane 2, lane 1 and clock lane respectively, and should
> be LP11 in my opinion (that's the PHY stop state). However, setting them to
> 0x00 when starting the stream mean that LP00 is selected as the sleep state at
> stream start, and LP11 when stopping the stream. Maybe "sleep mode" means
> LPDT, but I would expect that to be controlled by the idle status bit in
> register 0x4800.
>

I did not need to mess with the accesses to 0x3019 in order to get
things working on my system. I'm not sure of this registers actual
behavior, but it might affect idling while not streaming (after power
on). My pipeline currently only powers the sensor while streaming, so
I might be missing some ramifications of this register.

> OV5640_REG_MIPI_CTRL00 (0x4800) is set to 0x04 in the large array of init mode
> data, and BIT(5) is then cleared at stream on time and set at stream off time.
> This means:
>
> - Clock lane gate enable: Clock lane is free running
> - Line sync enable: Do not send line short packets for each line (I assume
> that's LS/LE)
> - Lane select: Use lane1 as default data lane.
> - Idle status: MIPI bus will be LP11 when no packet to transmit. I would have
> expected the idle status to correspond to LPDT, and thus be LP00 (as opposed
> to the stop state that should be LP11, which I believe is named "sleep mode"
> in the datasheet and controlled in register 0x3019).
>
> BIT(5) is the clock lane gate enable, so at stream on time the clock is set to
> free running, and at stream off time to "Gate clock lane when no packet to
> transmit". Couldn't we always enable clock gating ?

Good question, it might be worth testing. Same as above, I didn't need
to mess with this reg either.

> Do you have any insight on th

Re: [PATCH v2 00/12] media: ov5640: Misc cleanup and improvements

[Sam Bobrowicz]

FYI, still hard at work on this. Did some more experiments last week
that seemed to corroborate the clock tree in the spreadsheet. It also
seems that the output of the P divider cell, SCLK cell and MIPI Rate
cell in the spreadsheet must have a ratio of 2x:1x:8x (respectively)
in order for the sensor to work properly on my platform, and that the
SCLK value must be close to the "rate" variable that you calculate and
pass to set_mipi_pclk. Unfortunately, I've only got the sensor working
well for 1080p@15Hz and 720p@30Hz, both with a SCLK of 42MHz (aka
84:42:336). I'm running experiments now trying to adjust the htot and
vtot values to create different required rates, and also to try to get
faster Mipi rates working. Any information you have on the
requirements of the htot and vtot values with respect to vact and hact
values would likely be helpful.

I'm also keeping an eye on the scaler clock, which I think may be
affecting certain resolutions, but haven't been able to see it make a
difference yet (see register 0x3824 and 0x460c)

I plan on pushing a set of patches once I get this figured out, we can
discuss what I should base them on when I get closer to that point.
I'm new to this process :)

--Sam
-------
Sam Bobrowicz
Elite Embedded Consulting LLC
elite-embedded.com


On Thu, Apr 19, 2018 at 5:32 AM, Maxime Ripard
<maxime.rip...@bootlin.com> wrote:
> Hi Samuel,
>
> On Wed, Apr 18, 2018 at 04:39:06PM -0700, Samuel Bobrowicz wrote:
>> I applied your patches, and they are a big improvement for what I am
>> trying to do, but things still aren't working right on my platform.
>>
>> How confident are you that the MIPI mode will work with this version
>> of the driver?
>
> Not too confident. Like I said, I did all my tests on a parallel
> camera with a scope, so I'm pretty confident for the parallel bus. But
> I haven't been able to test the MIPI-CSI side of things and tried to
> deduce it from the datasheet.
>
> tl; dr: I might very well be wrong.
>
>> I am having issues that I believe are due to incorrect clock
>> generation. Our engineers did some reverse engineering of the clock
>> tree themselves, and came up with a slightly different model.  I've
>> captured their model in a spreadsheet here:
>> https://tinyurl.com/pll-calc . Just modify the register and xclk
>> values to see the clocks change. Do your tests disagree with this
>> potential model?
>
> At least on the parallel side, it looks fairly similar, so I guess we
> can come to an agreement :)
>
> There's just the SCLK2x divider that is no longer in the path to PCLK
> but has been replaced with BIT Divider that has the same value, so it
> should work as well.
>
>> I'm not sure which model is more correct, but my tests suggest the
>> high speed MIPI clock is generated directly off the PLL. This means
>> the PLL multiplier you are generating in your algorithm is not high
>> enough to satisfy the bandwidth. If this is the case, MIPI mode will
>> require a different set of parameters that enable some of the
>> downstream dividers, so that the PLL multiplier can be higher while
>> the PCLK value still matches the needed rate calculated from the
>> resolution.
>>
>> Any thoughts on this before I dive in and start tweaking the algorithm
>> in mipi mode?
>
> Like I said, I did that analysis by plugging the camera to a scope and
> look at the PCLK generated for various combinations. Your analysis
> seems not too far off for the setup I've tested, so I guess this makes
> sense. And let me know how it works for MIPI-CSI2 so that I can update
> the patches :)
>
> Maxime
>
> --
> Maxime Ripard, Bootlin (formerly Free Electrons)
> Embedded Linux and Kernel engineering
> https://bootlin.com