[PATCH v3 0/2] media: i2c: ov5640: Re-work MIPI startup sequence

[PATCH v3 0/2] media: i2c: ov5640: Re-work MIPI startup sequence

[Jacopo Mondi]

Hello ov5640 people,
   this driver has received a lot of attention recently, and this series aims
to fix the CSI-2 interface startup on i.Mx6Q platforms.

Please refer to the v2 cover letters for more background informations:
https://www.mail-archive.com/linux-media@vger.kernel.org/msg133420.html

This two patches alone allows the MIPI interface to startup properly, but in
order to capture good images (good as in 'not completely black') exposure and
gain handling should be fixed too.
Hugues Fruchet has a series in review that fixes that issues:
[PATCH v2 0/5] Fix OV5640 exposure & gain

And I have re-based it on top of this two fixes here:
git://jmondi.org/linux ov5640/timings_exposure

Steve Longerbeam tested that branch on his I.MX6q SabreSD board and confirms he
can now capture frames (I added his Tested-by tag to this patches). I have
verified the same on Engicam iCore I.MX6q and an Intel Atom based board.

Ideally I would like to have these two fixes merged, and Hugues' ones then
applied on top. Of course, more testing on other platforms using CSI-2 is very
welcome.

Thanks
   j

v2 -> v3:
- patch [2/2] was originally sent in a different series, compared to v2 it
  removes entries from the blob array instead of adding more.

Jacopo Mondi (2):
  media: ov5640: Re-work MIPI startup sequence
  media: ov5640: Fix timings setup code

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

--
2.7.4

[PATCH v3 1/2] media: ov5640: Re-work MIPI startup sequence

[Jacopo Mondi]

From: Jacopo Mondi <jacopo@jmondi.org>

Rework the MIPI interface startup sequence with the following changes:

- Remove MIPI bus initialization from the initial settings blob
- At set_power(1) time power up MIPI Tx/Rx and set data and clock lanes in
  LP11 during 'sleep' and 'idle' with MIPI clock in non-continuous mode.
- At s_stream time enable/disable the MIPI interface output.
- Restore default settings at set_power(0) time.

Before this commit the sensor MIPI interface was initialized with settings
that require a start/stop sequence at power-up time in order to force lanes
into LP11 state, as they were initialized in LP00 when in 'sleep mode',
which is assumed to be the sensor manual definition for the D-PHY defined
stop mode.

The stream start/stop was performed by enabling disabling clock gating,
and had the side effect to change the lanes sleep mode configuration when
stream was stopped.

Clock gating/ungating:
-       ret = ov5640_mod_reg(sensor, OV5640_REG_MIPI_CTRL00, BIT(5),
-                            on ? 0 : BIT(5));
-       if (ret)

Set lanes in LP11 when in 'sleep mode':
-       ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT00,
-                              on ? 0x00 : 0x70);

This commit fixes an issue reported by Jagan Teki on i.MX6 platforms that
prevents the host interface from powering up correctly:
https://lkml.org/lkml/2018/6/1/38

It also improves MIPI capture operations stability on my testing platform
where MIPI capture often failed and returned all-purple frames.

fixes: f22996db44e2 ("media: ov5640: add support of DVP parallel interface")
Tested-by: Steve Longerbeam <slongerbeam@gmail.com>
on i.MX6q SabreSD with MIPI CSI-2 OV5640 module
Reported-by: Jagan Teki <jagan@amarulasolutions.com>
Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 drivers/media/i2c/ov5640.c | 91 ++++++++++++++++++++++++++++++++++++----------
 1 file changed, 71 insertions(+), 20 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 1ecbb7a..7bbd1d7 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -286,10 +286,10 @@ static const struct reg_value ov5640_init_setting_30fps_VGA[] = {
 	{0x3a0d, 0x04, 0, 0}, {0x3a14, 0x03, 0, 0}, {0x3a15, 0xd8, 0, 0},
 	{0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0}, {0x3000, 0x00, 0, 0},
 	{0x3002, 0x1c, 0, 0}, {0x3004, 0xff, 0, 0}, {0x3006, 0xc3, 0, 0},
-	{0x300e, 0x45, 0, 0}, {0x302e, 0x08, 0, 0}, {0x4300, 0x3f, 0, 0},
+	{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}, {0x4800, 0x04, 0, 0}, {0x3824, 0x02, 0, 0},
+	{0x4837, 0x0a, 0, 0}, {0x3824, 0x02, 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},
@@ -1102,12 +1102,18 @@ static int ov5640_set_stream_mipi(struct ov5640_dev *sensor, bool on)
 {
 	int ret;
 
-	ret = ov5640_mod_reg(sensor, OV5640_REG_MIPI_CTRL00, BIT(5),
-			     on ? 0 : BIT(5));
-	if (ret)
-		return ret;
-	ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT00,
-			       on ? 0x00 : 0x70);
+	/*
+	 * Enable/disable the MIPI interface
+	 *
+	 * 0x300e = on ? 0x45 : 0x40
+	 * [7:5] = 001	: 2 data lanes mode
+	 * [4] = 0	: Power up MIPI HS Tx
+	 * [3] = 0	: Power up MIPI LS Rx
+	 * [2] = 1/0	: MIPI interface enable/disable
+	 * [1:0] = 01/00: FIXME: 'debug'
+	 */
+	ret = ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00,
+			       on ? 0x45 : 0x40);
 	if (ret)
 		return ret;
 
@@ -1786,23 +1792,68 @@ static int ov5640_set_power(struct ov5640_dev *sensor, bool on)
 		if (ret)
 			goto power_off;
 
+		/* We're done here for DVP bus, while CSI-2 needs setup. */
+		if (sensor->ep.bus_type != V4L2_MBUS_CSI2)
+			return 0;
+
+		/*
+		 * Power up MIPI HS Tx and LS Rx; 2 data lanes mode
+		 *
+		 * 0x300e = 0x40
+		 * [7:5] = 001	: 2 data lanes mode
+		 * [4] = 0	: Power up MIPI HS Tx
+		 * [3] = 0	: Power up MIPI LS Rx
+		 * [2] = 0	: MIPI interface disabled
+		 */
+		ret = ov5640_write_reg(sensor,
+				       OV5640_REG_IO_MIPI_CTRL00, 0x40);
+		if (ret)
+			goto power_off;
+
+		/*
+		 * Gate clock and set LP11 in 'no packets mode' (idle)
+		 *
+		 * 0x4800 = 0x24
+		 * [5] = 1	: Gate clock when 'no packets'
+		 * [2] = 1	: MIPI bus in LP11 when 'no packets'
+		 */
+		ret = ov5640_write_reg(sensor,
+				       OV5640_REG_MIPI_CTRL00, 0x24);
+		if (ret)
+			goto power_off;
+
+		/*
+		 * Set data lanes and clock in LP11 when 'sleeping'
+		 *
+		 * 0x3019 = 0x70
+		 * [6] = 1	: MIPI data lane 2 in LP11 when 'sleeping'
+		 * [5] = 1	: MIPI data lane 1 in LP11 when 'sleeping'
+		 * [4] = 1	: MIPI clock lane in LP11 when 'sleeping'
+		 */
+		ret = ov5640_write_reg(sensor,
+				       OV5640_REG_PAD_OUTPUT00, 0x70);
+		if (ret)
+			goto power_off;
+
+		/* Give lanes some time to coax into LP11 state. */
+		usleep_range(500, 1000);
+
+	} else {
 		if (sensor->ep.bus_type == V4L2_MBUS_CSI2) {
-			/*
-			 * start streaming briefly followed by stream off in
-			 * order to coax the clock lane into LP-11 state.
-			 */
-			ret = ov5640_set_stream_mipi(sensor, true);
-			if (ret)
-				goto power_off;
-			usleep_range(1000, 2000);
-			ret = ov5640_set_stream_mipi(sensor, false);
-			if (ret)
-				goto power_off;
+			/* Reset MIPI bus settings to their default values. */
+			ov5640_write_reg(sensor,
+					 OV5640_REG_IO_MIPI_CTRL00, 0x58);
+			ov5640_write_reg(sensor,
+					 OV5640_REG_MIPI_CTRL00, 0x04);
+			ov5640_write_reg(sensor,
+					 OV5640_REG_PAD_OUTPUT00, 0x00);
 		}
 
-		return 0;
+		ov5640_set_power_off(sensor);
 	}
 
+	return 0;
+
 power_off:
 	ov5640_set_power_off(sensor);
 	return ret;
-- 
2.7.4

[PATCH v3 2/2] media: ov5640: Fix timings setup code

[Jacopo Mondi]

From: Jacopo Mondi <jacopo@jmondi.org>

As of:
commit 476dec012f4c ("media: ov5640: Add horizontal and vertical totals")
the timings parameters gets programmed separately from the static register
values array.

When changing capture mode, the vertical and horizontal totals gets inspected
by the set_mode_exposure_calc() functions, and only later programmed with the
new values. This means exposure, light banding filter and shutter gain are
calculated using the previous timings, and are thus not correct.

Fix this by programming timings right after the static register value table
has been sent to the sensor in the ov5640_load_regs() function.

Fixes: 476dec012f4c ("media: ov5640: Add horizontal and vertical totals")
Tested-by: Steve Longerbeam <slongerbeam@gmail.com>
on i.MX6q SabreSD with MIPI CSI-2 OV5640 module
Signed-off-by: Samuel Bobrowicz <sam@elite-embedded.com>
Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 drivers/media/i2c/ov5640.c | 50 +++++++++++++++++++---------------------------
 1 file changed, 21 insertions(+), 29 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 7bbd1d7..c81a2a7 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -908,6 +908,26 @@ static int ov5640_mod_reg(struct ov5640_dev *sensor, u16 reg,
 }
 
 /* download ov5640 settings to sensor through i2c */
+static int ov5640_set_timings(struct ov5640_dev *sensor,
+			      const struct ov5640_mode_info *mode)
+{
+	int ret;
+
+	ret = ov5640_write_reg16(sensor, OV5640_REG_TIMING_DVPHO, mode->hact);
+	if (ret < 0)
+		return ret;
+
+	ret = ov5640_write_reg16(sensor, OV5640_REG_TIMING_DVPVO, mode->vact);
+	if (ret < 0)
+		return ret;
+
+	ret = ov5640_write_reg16(sensor, OV5640_REG_TIMING_HTS, mode->htot);
+	if (ret < 0)
+		return ret;
+
+	return ov5640_write_reg16(sensor, OV5640_REG_TIMING_VTS, mode->vtot);
+}
+
 static int ov5640_load_regs(struct ov5640_dev *sensor,
 			    const struct ov5640_mode_info *mode)
 {
@@ -935,7 +955,7 @@ static int ov5640_load_regs(struct ov5640_dev *sensor,
 			usleep_range(1000 * delay_ms, 1000 * delay_ms + 100);
 	}
 
-	return ret;
+	return ov5640_set_timings(sensor, mode);
 }
 
 /* read exposure, in number of line periods */
@@ -1391,30 +1411,6 @@ static int ov5640_set_virtual_channel(struct ov5640_dev *sensor)
 	return ov5640_write_reg(sensor, OV5640_REG_DEBUG_MODE, temp);
 }
 
-static int ov5640_set_timings(struct ov5640_dev *sensor,
-			      const struct ov5640_mode_info *mode)
-{
-	int ret;
-
-	ret = ov5640_write_reg16(sensor, OV5640_REG_TIMING_DVPHO, mode->hact);
-	if (ret < 0)
-		return ret;
-
-	ret = ov5640_write_reg16(sensor, OV5640_REG_TIMING_DVPVO, mode->vact);
-	if (ret < 0)
-		return ret;
-
-	ret = ov5640_write_reg16(sensor, OV5640_REG_TIMING_HTS, mode->htot);
-	if (ret < 0)
-		return ret;
-
-	ret = ov5640_write_reg16(sensor, OV5640_REG_TIMING_VTS, mode->vtot);
-	if (ret < 0)
-		return ret;
-
-	return 0;
-}
-
 static const struct ov5640_mode_info *
 ov5640_find_mode(struct ov5640_dev *sensor, enum ov5640_frame_rate fr,
 		 int width, int height, bool nearest)
@@ -1658,10 +1654,6 @@ static int ov5640_set_mode(struct ov5640_dev *sensor,
 	if (ret < 0)
 		return ret;
 
-	ret = ov5640_set_timings(sensor, mode);
-	if (ret < 0)
-		return ret;
-
 	ret = ov5640_set_binning(sensor, dn_mode != SCALING);
 	if (ret < 0)
 		return ret;
-- 
2.7.4

Re: [PATCH v3 0/2] media: i2c: ov5640: Re-work MIPI startup sequence

[Loic Poulain]

On 15 August 2018 at 12:28, Jacopo Mondi <jacopo+renesas@jmondi.org> wrote:
> Hello ov5640 people,
>    this driver has received a lot of attention recently, and this series aims
> to fix the CSI-2 interface startup on i.Mx6Q platforms.
>
> Please refer to the v2 cover letters for more background informations:
> https://www.mail-archive.com/linux-media@vger.kernel.org/msg133420.html
>
> This two patches alone allows the MIPI interface to startup properly, but in
> order to capture good images (good as in 'not completely black') exposure and
> gain handling should be fixed too.
> Hugues Fruchet has a series in review that fixes that issues:
> [PATCH v2 0/5] Fix OV5640 exposure & gain
>
> And I have re-based it on top of this two fixes here:
> git://jmondi.org/linux ov5640/timings_exposure
>
> Steve Longerbeam tested that branch on his I.MX6q SabreSD board and confirms he
> can now capture frames (I added his Tested-by tag to this patches). I have
> verified the same on Engicam iCore I.MX6q and an Intel Atom based board.
>
> Ideally I would like to have these two fixes merged, and Hugues' ones then
> applied on top. Of course, more testing on other platforms using CSI-2 is very
> welcome.
>
> Thanks
>    j
>
> v2 -> v3:
> - patch [2/2] was originally sent in a different series, compared to v2 it
>   removes entries from the blob array instead of adding more.
>
> Jacopo Mondi (2):
>   media: ov5640: Re-work MIPI startup sequence
>   media: ov5640: Fix timings setup code
>
>  drivers/media/i2c/ov5640.c | 141 +++++++++++++++++++++++++++++----------------
>  1 file changed, 92 insertions(+), 49 deletions(-)
>
> --
> 2.7.4
>

Thanks for this work.
I've just tested this with a dragonboard-410c (MICPI/CSI) + OV5640 sensor.
It works on my side for 1280*720, 1920*1080 and 2592*1944 formats.

Tested-by: Loic Poulain <loic.poulain@linaro.org>

Re: [PATCH v3 0/2] media: i2c: ov5640: Re-work MIPI startup sequence

[jacopo mondi]

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

Hello Loic,

On Tue, Aug 28, 2018 at 05:08:07PM +0200, Loic Poulain wrote:
> On 15 August 2018 at 12:28, Jacopo Mondi <jacopo+renesas@jmondi.org> wrote:
> > Hello ov5640 people,
> >    this driver has received a lot of attention recently, and this series aims
> > to fix the CSI-2 interface startup on i.Mx6Q platforms.
> >
> > Please refer to the v2 cover letters for more background informations:
> > https://www.mail-archive.com/linux-media@vger.kernel.org/msg133420.html
> >
> > This two patches alone allows the MIPI interface to startup properly, but in
> > order to capture good images (good as in 'not completely black') exposure and
> > gain handling should be fixed too.
> > Hugues Fruchet has a series in review that fixes that issues:
> > [PATCH v2 0/5] Fix OV5640 exposure & gain
> >
> > And I have re-based it on top of this two fixes here:
> > git://jmondi.org/linux ov5640/timings_exposure
> >
> > Steve Longerbeam tested that branch on his I.MX6q SabreSD board and confirms he
> > can now capture frames (I added his Tested-by tag to this patches). I have
> > verified the same on Engicam iCore I.MX6q and an Intel Atom based board.
> >
> > Ideally I would like to have these two fixes merged, and Hugues' ones then
> > applied on top. Of course, more testing on other platforms using CSI-2 is very
> > welcome.
> >
> > Thanks
> >    j
> >
> > v2 -> v3:
> > - patch [2/2] was originally sent in a different series, compared to v2 it
> >   removes entries from the blob array instead of adding more.
> >
> > Jacopo Mondi (2):
> >   media: ov5640: Re-work MIPI startup sequence
> >   media: ov5640: Fix timings setup code
> >
> >  drivers/media/i2c/ov5640.c | 141 +++++++++++++++++++++++++++++----------------
> >  1 file changed, 92 insertions(+), 49 deletions(-)
> >
> > --
> > 2.7.4
> >
>
> Thanks for this work.
> I've just tested this with a dragonboard-410c (MICPI/CSI) + OV5640 sensor.
> It works on my side for 1280*720, 1920*1080 and 2592*1944 formats.
>
> Tested-by: Loic Poulain <loic.poulain@linaro.org>

Thanks for testing!

Just out of curiosity, did the dragonboard-410c CSI-2 interface had
issues poperly starting up before this series like the i.MX6q did?

Thanks
   j

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

Re: [PATCH v3 1/2] media: ov5640: Re-work MIPI startup sequence

[Loic Poulain]

Hi Jacopo,

> -       ret = ov5640_mod_reg(sensor, OV5640_REG_MIPI_CTRL00, BIT(5),
> -                            on ? 0 : BIT(5));
> -       if (ret)
> -               return ret;
> -       ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT00,
> -                              on ? 0x00 : 0x70);
> +       /*
> +        * Enable/disable the MIPI interface
> +        *
> +        * 0x300e = on ? 0x45 : 0x40
> +        * [7:5] = 001  : 2 data lanes mode

Does 2-Lanes work with this config?
AFAIU, if 2-Lanes is bit 5, value should be 0x25 and 0x20.

> +        * [4] = 0      : Power up MIPI HS Tx
> +        * [3] = 0      : Power up MIPI LS Rx
> +        * [2] = 1/0    : MIPI interface enable/disable
> +        * [1:0] = 01/00: FIXME: 'debug'
> +        */
> +       ret = ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00,
> +                              on ? 0x45 : 0x40);

Regards,
Loic

Re: [PATCH v3 1/2] media: ov5640: Re-work MIPI startup sequence

[jacopo mondi]

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

Hello Loic,
   thanks for looking into this

On Tue, Sep 04, 2018 at 07:22:50PM +0200, Loic Poulain wrote:
> Hi Jacopo,
>
> > -       ret = ov5640_mod_reg(sensor, OV5640_REG_MIPI_CTRL00, BIT(5),
> > -                            on ? 0 : BIT(5));
> > -       if (ret)
> > -               return ret;
> > -       ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT00,
> > -                              on ? 0x00 : 0x70);
> > +       /*
> > +        * Enable/disable the MIPI interface
> > +        *
> > +        * 0x300e = on ? 0x45 : 0x40
> > +        * [7:5] = 001  : 2 data lanes mode
>
> Does 2-Lanes work with this config?
> AFAIU, if 2-Lanes is bit 5, value should be 0x25 and 0x20.
>

Yes, confusing.

The sensor manual reports
0x300e[7:5] = 000 one lane mode
0x300e[7:5] = 001 two lanes mode

Although this configuration works with 2 lanes, and the application
note I have, with the suggested settings for MIPI CSI-2 2 lanes
reports 0x40 to be the 2 lanes mode...

I used that one, also because the removed entry from the settings blob
is:
-       {0x300e, 0x45, 0, 0}, {0x302e, 0x08, 0, 0}, {0x4300, 0x3f, 0, 0},
+       {0x302e, 0x08, 0, 0}, {0x4300, 0x3f, 0, 0},

So it was using BIT(6) already.

I do not remember if I tested BIT(5) or not, it would be interesting
if someone using a 1-lane interface could try '000' and '001' maybe.

Anyway, it works for me with 2 lanes (and I assume Steve), you have tested
too, with how many lanes are you working with?

Anyway, a comment there might be nice to have... Will add in next
version

Thanks
   j

> > +        * [4] = 0      : Power up MIPI HS Tx
> > +        * [3] = 0      : Power up MIPI LS Rx
> > +        * [2] = 1/0    : MIPI interface enable/disable
> > +        * [1:0] = 01/00: FIXME: 'debug'
> > +        */
> > +       ret = ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00,
> > +                              on ? 0x45 : 0x40);
>
> Regards,
> Loic

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

Re: [PATCH v3 1/2] media: ov5640: Re-work MIPI startup sequence

[Loic Poulain]

On 6 September 2018 at 09:48, jacopo mondi <jacopo@jmondi.org> wrote:
> Hello Loic,
>    thanks for looking into this
>
> On Tue, Sep 04, 2018 at 07:22:50PM +0200, Loic Poulain wrote:
>> Hi Jacopo,
>>
>> > -       ret = ov5640_mod_reg(sensor, OV5640_REG_MIPI_CTRL00, BIT(5),
>> > -                            on ? 0 : BIT(5));
>> > -       if (ret)
>> > -               return ret;
>> > -       ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT00,
>> > -                              on ? 0x00 : 0x70);
>> > +       /*
>> > +        * Enable/disable the MIPI interface
>> > +        *
>> > +        * 0x300e = on ? 0x45 : 0x40
>> > +        * [7:5] = 001  : 2 data lanes mode
>>
>> Does 2-Lanes work with this config?
>> AFAIU, if 2-Lanes is bit 5, value should be 0x25 and 0x20.
>>
>
> Yes, confusing.
>
> The sensor manual reports
> 0x300e[7:5] = 000 one lane mode
> 0x300e[7:5] = 001 two lanes mode
>
> Although this configuration works with 2 lanes, and the application
> note I have, with the suggested settings for MIPI CSI-2 2 lanes
> reports 0x40 to be the 2 lanes mode...
>
> I used that one, also because the removed entry from the settings blob
> is:
> -       {0x300e, 0x45, 0, 0}, {0x302e, 0x08, 0, 0}, {0x4300, 0x3f, 0, 0},
> +       {0x302e, 0x08, 0, 0}, {0x4300, 0x3f, 0, 0},
>
> So it was using BIT(6) already.

Yes, it was setting BIT(6) from static config and BIT(5) from the
ov5640_set_stream_mipi function. In your patch you don't set
BIT(5) anymore.

So it's not clear to me why it is still working, and the datasheet does
not help a lot on this (BIT(6) is for debug modes).
FYI I tried with BIT(5) only but it does not work (though I did not
investigate a lot).

> Anyway, it works for me with 2 lanes (and I assume Steve), you have tested
> too, with how many lanes are you working with?
>
> Anyway, a comment there might be nice to have... Will add in next
> version

Definitely.

Regards,
Loic

Re: [PATCH v3 1/2] media: ov5640: Re-work MIPI startup sequence

[jacopo mondi]

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

Hello Loic,

On Thu, Sep 06, 2018 at 10:13:53AM +0200, Loic Poulain wrote:
> On 6 September 2018 at 09:48, jacopo mondi <jacopo@jmondi.org> wrote:
> > Hello Loic,
> >    thanks for looking into this
> >
> > On Tue, Sep 04, 2018 at 07:22:50PM +0200, Loic Poulain wrote:
> >> Hi Jacopo,
> >>
> >> > -       ret = ov5640_mod_reg(sensor, OV5640_REG_MIPI_CTRL00, BIT(5),
> >> > -                            on ? 0 : BIT(5));
> >> > -       if (ret)
> >> > -               return ret;
> >> > -       ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT00,
> >> > -                              on ? 0x00 : 0x70);
> >> > +       /*
> >> > +        * Enable/disable the MIPI interface
> >> > +        *
> >> > +        * 0x300e = on ? 0x45 : 0x40
> >> > +        * [7:5] = 001  : 2 data lanes mode
> >>
> >> Does 2-Lanes work with this config?
> >> AFAIU, if 2-Lanes is bit 5, value should be 0x25 and 0x20.
> >>
> >
> > Yes, confusing.
> >
> > The sensor manual reports
> > 0x300e[7:5] = 000 one lane mode
> > 0x300e[7:5] = 001 two lanes mode
> >
> > Although this configuration works with 2 lanes, and the application
> > note I have, with the suggested settings for MIPI CSI-2 2 lanes
> > reports 0x40 to be the 2 lanes mode...
> >
> > I used that one, also because the removed entry from the settings blob
> > is:
> > -       {0x300e, 0x45, 0, 0}, {0x302e, 0x08, 0, 0}, {0x4300, 0x3f, 0, 0},
> > +       {0x302e, 0x08, 0, 0}, {0x4300, 0x3f, 0, 0},
> >
> > So it was using BIT(6) already.
>
> Yes, it was setting BIT(6) from static config and BIT(5) from the
> ov5640_set_stream_mipi function. In your patch you don't set
> BIT(5) anymore.
>
> So it's not clear to me why it is still working, and the datasheet does
> not help a lot on this (BIT(6) is for debug modes).
> FYI I tried with BIT(5) only but it does not work (though I did not
> investigate a lot).

Thanks. Is your setup using 1 or 2 lanes? (I assume 2...)

Another question, unrelated to this specific issue: was the ov5640
working with dragonboard before this patch? I'm asking as I've seen
different behaviors between different platforms, and knowing this
fixes a widespread one like dragonboard is, would help getting this
patches in faster :)

Thanks
   j
>
> > Anyway, it works for me with 2 lanes (and I assume Steve), you have tested
> > too, with how many lanes are you working with?
> >
> > Anyway, a comment there might be nice to have... Will add in next
> > version
>
> Definitely.
>
> Regards,
> Loic

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

Re: [PATCH v3 1/2] media: ov5640: Re-work MIPI startup sequence

[Loic Poulain]

On 6 September 2018 at 10:48, jacopo mondi <jacopo@jmondi.org> wrote:
> Hello Loic,
>
> On Thu, Sep 06, 2018 at 10:13:53AM +0200, Loic Poulain wrote:
>> On 6 September 2018 at 09:48, jacopo mondi <jacopo@jmondi.org> wrote:
>> > Hello Loic,
>> >    thanks for looking into this
>> >
>> > On Tue, Sep 04, 2018 at 07:22:50PM +0200, Loic Poulain wrote:
>> >> Hi Jacopo,
>> >>
>> >> > -       ret = ov5640_mod_reg(sensor, OV5640_REG_MIPI_CTRL00, BIT(5),
>> >> > -                            on ? 0 : BIT(5));
>> >> > -       if (ret)
>> >> > -               return ret;
>> >> > -       ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT00,
>> >> > -                              on ? 0x00 : 0x70);
>> >> > +       /*
>> >> > +        * Enable/disable the MIPI interface
>> >> > +        *
>> >> > +        * 0x300e = on ? 0x45 : 0x40
>> >> > +        * [7:5] = 001  : 2 data lanes mode
>> >>
>> >> Does 2-Lanes work with this config?
>> >> AFAIU, if 2-Lanes is bit 5, value should be 0x25 and 0x20.
>> >>
>> >
>> > Yes, confusing.
>> >
>> > The sensor manual reports
>> > 0x300e[7:5] = 000 one lane mode
>> > 0x300e[7:5] = 001 two lanes mode
>> >
>> > Although this configuration works with 2 lanes, and the application
>> > note I have, with the suggested settings for MIPI CSI-2 2 lanes
>> > reports 0x40 to be the 2 lanes mode...
>> >
>> > I used that one, also because the removed entry from the settings blob
>> > is:
>> > -       {0x300e, 0x45, 0, 0}, {0x302e, 0x08, 0, 0}, {0x4300, 0x3f, 0, 0},
>> > +       {0x302e, 0x08, 0, 0}, {0x4300, 0x3f, 0, 0},
>> >
>> > So it was using BIT(6) already.
>>
>> Yes, it was setting BIT(6) from static config and BIT(5) from the
>> ov5640_set_stream_mipi function. In your patch you don't set
>> BIT(5) anymore.
>>
>> So it's not clear to me why it is still working, and the datasheet does
>> not help a lot on this (BIT(6) is for debug modes).
>> FYI I tried with BIT(5) only but it does not work (though I did not
>> investigate a lot).
>
> Thanks. Is your setup using 1 or 2 lanes? (I assume 2...)
>
> Another question, unrelated to this specific issue: was the ov5640
> working with dragonboard before this patch? I'm asking as I've seen
> different behaviors between different platforms, and knowing this
> fixes a widespread one like dragonboard is, would help getting this
> patches in faster :)

I did not test without the patch, will do.

Re: [PATCH v3 1/2] media: ov5640: Re-work MIPI startup sequence

[jacopo mondi]

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

Hello Loic,

On Thu, Sep 06, 2018 at 10:13:53AM +0200, Loic Poulain wrote:
> On 6 September 2018 at 09:48, jacopo mondi <jacopo@jmondi.org> wrote:
> > Hello Loic,
> >    thanks for looking into this
> >
> > On Tue, Sep 04, 2018 at 07:22:50PM +0200, Loic Poulain wrote:
> >> Hi Jacopo,
> >>
> >> > -       ret = ov5640_mod_reg(sensor, OV5640_REG_MIPI_CTRL00, BIT(5),
> >> > -                            on ? 0 : BIT(5));
> >> > -       if (ret)
> >> > -               return ret;
> >> > -       ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT00,
> >> > -                              on ? 0x00 : 0x70);
> >> > +       /*
> >> > +        * Enable/disable the MIPI interface
> >> > +        *
> >> > +        * 0x300e = on ? 0x45 : 0x40
> >> > +        * [7:5] = 001  : 2 data lanes mode
> >>
> >> Does 2-Lanes work with this config?
> >> AFAIU, if 2-Lanes is bit 5, value should be 0x25 and 0x20.
> >>
> >
> > Yes, confusing.
> >
> > The sensor manual reports
> > 0x300e[7:5] = 000 one lane mode
> > 0x300e[7:5] = 001 two lanes mode
> >
> > Although this configuration works with 2 lanes, and the application
> > note I have, with the suggested settings for MIPI CSI-2 2 lanes
> > reports 0x40 to be the 2 lanes mode...
> >
> > I used that one, also because the removed entry from the settings blob
> > is:
> > -       {0x300e, 0x45, 0, 0}, {0x302e, 0x08, 0, 0}, {0x4300, 0x3f, 0, 0},
> > +       {0x302e, 0x08, 0, 0}, {0x4300, 0x3f, 0, 0},
> >
> > So it was using BIT(6) already.
>
> Yes, it was setting BIT(6) from static config and BIT(5) from the
> ov5640_set_stream_mipi function. In your patch you don't set
> BIT(5) anymore.

I've resumed looking into this series.
Just FYI, the snippet you refer to is:

-       ret = ov5640_mod_reg(sensor, OV5640_REG_MIPI_CTRL00, BIT(5),
-                            on ? 0 : BIT(5));
-       if (ret)
-               return ret;
-       ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT00,
-                              on ? 0x00 : 0x70);
+       /*
+        * Enable/disable the MIPI interface
+        *
+        * 0x300e = on ? 0x45 : 0x40
+        * [7:5] = 001  : 2 data lanes mode
+        * [4] = 0      : Power up MIPI HS Tx
+        * [3] = 0      : Power up MIPI LS Rx
+        * [2] = 1/0    : MIPI interface enable/disable
+        * [1:0] = 01/00: FIXME: 'debug'
+        */
+       ret = ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00,
+                              on ? 0x45 : 0x40)

As you can see (it took me a while) the old code was indeed setting
BIT(5) as you mentioned, but on OV5640_REG_MIPI_CTRL00 (0x4800) and
not on OV5640_REG_IO_MIPI_CTRL00 (0x300e) as mine does. So the lane
configuration mode was set to 0x45 and never changed later.
>
> So it's not clear to me why it is still working, and the datasheet does
> not help a lot on this (BIT(6) is for debug modes).
> FYI I tried with BIT(5) only but it does not work (though I did not
> investigate a lot).

I'll keep BIT(6) set, point out the discrepancy with the datasheet,
and point out it has been tested with 2 lanes, until someone can
confirm it works with 1 lane too.

Thanks
   j

>
> > Anyway, it works for me with 2 lanes (and I assume Steve), you have tested
> > too, with how many lanes are you working with?
> >
> > Anyway, a comment there might be nice to have... Will add in next
> > version
>
> Definitely.
>
> Regards,
> Loic

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