2020-09-04 20:20:06

by Prabhakar Mahadev Lad

[permalink] [raw]
Subject: [PATCH v4 1/6] media: i2c: ov5640: Remain in power down for DVP mode unless streaming

Keep the sensor in software power down mode and wake up only in
ov5640_set_stream_dvp() callback.

Signed-off-by: Lad Prabhakar <[email protected]>
Reviewed-by: Biju Das <[email protected]>
Tested-by: Jacopo Mondi <[email protected]>
---
drivers/media/i2c/ov5640.c | 19 ++++++++++++++++---
1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 2fe4a7ac0592..880fde73a030 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -34,6 +34,8 @@
#define OV5640_REG_SYS_RESET02 0x3002
#define OV5640_REG_SYS_CLOCK_ENABLE02 0x3006
#define OV5640_REG_SYS_CTRL0 0x3008
+#define OV5640_REG_SYS_CTRL0_SW_PWDN 0x42
+#define OV5640_REG_SYS_CTRL0_SW_PWUP 0x02
#define OV5640_REG_CHIP_ID 0x300a
#define OV5640_REG_IO_MIPI_CTRL00 0x300e
#define OV5640_REG_PAD_OUTPUT_ENABLE01 0x3017
@@ -1120,6 +1122,12 @@ static int ov5640_load_regs(struct ov5640_dev *sensor,
val = regs->val;
mask = regs->mask;

+ /* remain in power down mode for DVP */
+ if (regs->reg_addr == OV5640_REG_SYS_CTRL0 &&
+ val == OV5640_REG_SYS_CTRL0_SW_PWUP &&
+ sensor->ep.bus_type != V4L2_MBUS_CSI2_DPHY)
+ continue;
+
if (mask)
ret = ov5640_mod_reg(sensor, reg_addr, mask, val);
else
@@ -1297,9 +1305,14 @@ static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on)
* PAD OUTPUT ENABLE 02
* - [7:2]: D[5:0] output enable
*/
- return ov5640_write_reg(sensor,
- OV5640_REG_PAD_OUTPUT_ENABLE02,
- on ? 0xfc : 0);
+ ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE02,
+ on ? 0xfc : 0);
+ if (ret)
+ return ret;
+
+ return ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, on ?
+ OV5640_REG_SYS_CTRL0_SW_PWUP :
+ OV5640_REG_SYS_CTRL0_SW_PWDN);
}

static int ov5640_set_stream_mipi(struct ov5640_dev *sensor, bool on)
--
2.17.1


2020-09-09 16:43:16

by Hugues Fruchet

[permalink] [raw]
Subject: Re: [PATCH v4 1/6] media: i2c: ov5640: Remain in power down for DVP mode unless streaming

Hi Prabhakar,

As discussed separately I would prefer to better understand issue before
going to this patch.
Nevertheless I have some remarks in code in case we'll need it at the end.

On 9/4/20 10:18 PM, Lad Prabhakar wrote:
> Keep the sensor in software power down mode and wake up only in
> ov5640_set_stream_dvp() callback.
>
> Signed-off-by: Lad Prabhakar <[email protected]>
> Reviewed-by: Biju Das <[email protected]>
> Tested-by: Jacopo Mondi <[email protected]>
> ---
> drivers/media/i2c/ov5640.c | 19 ++++++++++++++++---
> 1 file changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index 2fe4a7ac0592..880fde73a030 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -34,6 +34,8 @@
> #define OV5640_REG_SYS_RESET02 0x3002
> #define OV5640_REG_SYS_CLOCK_ENABLE02 0x3006
> #define OV5640_REG_SYS_CTRL0 0x3008
> +#define OV5640_REG_SYS_CTRL0_SW_PWDN 0x42
> +#define OV5640_REG_SYS_CTRL0_SW_PWUP 0x02

For the time being this section was only referring to registers
addresses and bit details was explained into a comment right before
affectation, see OV5640_REG_IO_MIPI_CTRL00 for example.

> #define OV5640_REG_CHIP_ID 0x300a
> #define OV5640_REG_IO_MIPI_CTRL00 0x300e
> #define OV5640_REG_PAD_OUTPUT_ENABLE01 0x3017
> @@ -1120,6 +1122,12 @@ static int ov5640_load_regs(struct ov5640_dev *sensor,
> val = regs->val;
> mask = regs->mask;
>
> + /* remain in power down mode for DVP */
> + if (regs->reg_addr == OV5640_REG_SYS_CTRL0 &&
> + val == OV5640_REG_SYS_CTRL0_SW_PWUP &&
> + sensor->ep.bus_type != V4L2_MBUS_CSI2_DPHY)
> + continue;
> +

I understand that more or less register OV5640_REG_SYS_CTRL0 (0x3008)
has been partially removed from big init sequence: for power-up part,
but power-dwn remains at very beginning of sequence.
We should completely remove 0x3008 from init sequence, including
power-dwn, and introduce a new function ov5640_sw_powerdown(on/off) that
should be called at the right place instead.


> if (mask)
> ret = ov5640_mod_reg(sensor, reg_addr, mask, val);
> else
> @@ -1297,9 +1305,14 @@ static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on)
> * PAD OUTPUT ENABLE 02
> * - [7:2]: D[5:0] output enable
> */
> - return ov5640_write_reg(sensor,
> - OV5640_REG_PAD_OUTPUT_ENABLE02,
> - on ? 0xfc : 0);
> + ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE02,
> + on ? 0xfc : 0);
> + if (ret)
> + return ret;
> +
> + return ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, on ?
> + OV5640_REG_SYS_CTRL0_SW_PWUP :
> + OV5640_REG_SYS_CTRL0_SW_PWDN);
> }
>
> static int ov5640_set_stream_mipi(struct ov5640_dev *sensor, bool on)
>


BR,
Hugues.

2021-12-21 07:37:56

by Eugen Hristev

[permalink] [raw]
Subject: Re: [PATCH v4 1/6] media: i2c: ov5640: Remain in power down for DVP mode unless streaming

On 6/29/21 1:47 PM, Eugen Hristev - M18282 wrote:
> On 9/9/20 7:16 PM, Hugues FRUCHET wrote:
>> Hi Prabhakar,
>>
>> As discussed separately I would prefer to better understand issue before
>> going to this patch.
>> Nevertheless I have some remarks in code in case we'll need it at the end.
>>
>> On 9/4/20 10:18 PM, Lad Prabhakar wrote:
>>> Keep the sensor in software power down mode and wake up only in
>>> ov5640_set_stream_dvp() callback.
>>>
>>> Signed-off-by: Lad Prabhakar <[email protected]>
>>> Reviewed-by: Biju Das <[email protected]>
>>> Tested-by: Jacopo Mondi <[email protected]>
>>> ---
>>> drivers/media/i2c/ov5640.c | 19 ++++++++++++++++---
>>> 1 file changed, 16 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
>>> index 2fe4a7ac0592..880fde73a030 100644
>>> --- a/drivers/media/i2c/ov5640.c
>>> +++ b/drivers/media/i2c/ov5640.c
>>> @@ -34,6 +34,8 @@
>>> #define OV5640_REG_SYS_RESET02 0x3002
>>> #define OV5640_REG_SYS_CLOCK_ENABLE02 0x3006
>>> #define OV5640_REG_SYS_CTRL0 0x3008
>>> +#define OV5640_REG_SYS_CTRL0_SW_PWDN 0x42
>>> +#define OV5640_REG_SYS_CTRL0_SW_PWUP 0x02
>>
>> For the time being this section was only referring to registers
>> addresses and bit details was explained into a comment right before
>> affectation, see OV5640_REG_IO_MIPI_CTRL00 for example.
>>
>>> #define OV5640_REG_CHIP_ID 0x300a
>>> #define OV5640_REG_IO_MIPI_CTRL00 0x300e
>>> #define OV5640_REG_PAD_OUTPUT_ENABLE01 0x3017
>>> @@ -1120,6 +1122,12 @@ static int ov5640_load_regs(struct ov5640_dev *sensor,
>>> val = regs->val;
>>> mask = regs->mask;
>>>
>>> + /* remain in power down mode for DVP */
>>> + if (regs->reg_addr == OV5640_REG_SYS_CTRL0 &&
>>> + val == OV5640_REG_SYS_CTRL0_SW_PWUP &&
>>> + sensor->ep.bus_type != V4L2_MBUS_CSI2_DPHY)
>>> + continue;
>>> +
>>
>> I understand that more or less register OV5640_REG_SYS_CTRL0 (0x3008)
>> has been partially removed from big init sequence: for power-up part,
>> but power-dwn remains at very beginning of sequence.
>> We should completely remove 0x3008 from init sequence, including
>> power-dwn, and introduce a new function ov5640_sw_powerdown(on/off) that
>> should be called at the right place instead.
>>
>>
>>> if (mask)
>>> ret = ov5640_mod_reg(sensor, reg_addr, mask, val);
>>> else
>>> @@ -1297,9 +1305,14 @@ static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on)
>>> * PAD OUTPUT ENABLE 02
>>> * - [7:2]: D[5:0] output enable
>>> */
>>> - return ov5640_write_reg(sensor,
>>> - OV5640_REG_PAD_OUTPUT_ENABLE02,
>>> - on ? 0xfc : 0);
>>> + ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE02,
>>> + on ? 0xfc : 0);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + return ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, on ?
>>> + OV5640_REG_SYS_CTRL0_SW_PWUP :
>>> + OV5640_REG_SYS_CTRL0_SW_PWDN);
>>> }
>>>
>>> static int ov5640_set_stream_mipi(struct ov5640_dev *sensor, bool on)
>>>
>>
>>
>> BR,
>> Hugues.
>>
>
>
> Hello everyone,
>
> When I updated driver in my tree with this patch, I noticed that my
> setup using the ATMEL isc platform + ov5640 in parallel bus mode stopped
> working.
>
> It looks like the culprit is this patch.
> I am not sure which is the best way to fix it.
> It looks like initially things work fine when probing the driver, but
> when we want to start streaming at a later point, the register SYS_CTRL0
> cannot be written anymore.
> Here is an output of the log:
>
> --- Opening /dev/video0...
> Trying source module v4l2...
> /dev/video0 opened.
> No input was specified, using the first.
> ov5640 1-003c: ov5640_write_reg: error: reg=3008, val=2
> atmel-sama5d2-isc f0008000.isc: stream on failed in subdev -121
> Error starting stream.
> VIDIOC_STREAMON: Remote I/O error
> Unable to use mmap. Using read instead.
> Unable to use read.
>
> I am using a simple application to read from /dev/video0 (which is
> registered by the atmel-isc once the sensor probed)
>
> I have a feeling that something is wrong related to power, but I cannot
> tell for sure.
>
> Reverting this patch makes it work fine again.
>
> Help is appreciated,
> Thanks,
> Eugen
>


Gentle ping.

I would like to send a revert patch if nobody cares about this
driver/patch except me.

Thanks,
Eugen

2021-12-21 12:06:41

by Sakari Ailus

[permalink] [raw]
Subject: Re: [PATCH v4 1/6] media: i2c: ov5640: Remain in power down for DVP mode unless streaming

Hi Hugues, Eugen,

On Tue, Dec 21, 2021 at 07:37:47AM +0000, [email protected] wrote:
> On 6/29/21 1:47 PM, Eugen Hristev - M18282 wrote:
> > On 9/9/20 7:16 PM, Hugues FRUCHET wrote:
> >> Hi Prabhakar,
> >>
> >> As discussed separately I would prefer to better understand issue before
> >> going to this patch.
> >> Nevertheless I have some remarks in code in case we'll need it at the end.
> >>
> >> On 9/4/20 10:18 PM, Lad Prabhakar wrote:
> >>> Keep the sensor in software power down mode and wake up only in
> >>> ov5640_set_stream_dvp() callback.
> >>>
> >>> Signed-off-by: Lad Prabhakar <[email protected]>
> >>> Reviewed-by: Biju Das <[email protected]>
> >>> Tested-by: Jacopo Mondi <[email protected]>
> >>> ---
> >>> drivers/media/i2c/ov5640.c | 19 ++++++++++++++++---
> >>> 1 file changed, 16 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> >>> index 2fe4a7ac0592..880fde73a030 100644
> >>> --- a/drivers/media/i2c/ov5640.c
> >>> +++ b/drivers/media/i2c/ov5640.c
> >>> @@ -34,6 +34,8 @@
> >>> #define OV5640_REG_SYS_RESET02 0x3002
> >>> #define OV5640_REG_SYS_CLOCK_ENABLE02 0x3006
> >>> #define OV5640_REG_SYS_CTRL0 0x3008
> >>> +#define OV5640_REG_SYS_CTRL0_SW_PWDN 0x42
> >>> +#define OV5640_REG_SYS_CTRL0_SW_PWUP 0x02
> >>
> >> For the time being this section was only referring to registers
> >> addresses and bit details was explained into a comment right before
> >> affectation, see OV5640_REG_IO_MIPI_CTRL00 for example.
> >>
> >>> #define OV5640_REG_CHIP_ID 0x300a
> >>> #define OV5640_REG_IO_MIPI_CTRL00 0x300e
> >>> #define OV5640_REG_PAD_OUTPUT_ENABLE01 0x3017
> >>> @@ -1120,6 +1122,12 @@ static int ov5640_load_regs(struct ov5640_dev *sensor,
> >>> val = regs->val;
> >>> mask = regs->mask;
> >>>
> >>> + /* remain in power down mode for DVP */
> >>> + if (regs->reg_addr == OV5640_REG_SYS_CTRL0 &&
> >>> + val == OV5640_REG_SYS_CTRL0_SW_PWUP &&
> >>> + sensor->ep.bus_type != V4L2_MBUS_CSI2_DPHY)
> >>> + continue;
> >>> +
> >>
> >> I understand that more or less register OV5640_REG_SYS_CTRL0 (0x3008)
> >> has been partially removed from big init sequence: for power-up part,
> >> but power-dwn remains at very beginning of sequence.
> >> We should completely remove 0x3008 from init sequence, including
> >> power-dwn, and introduce a new function ov5640_sw_powerdown(on/off) that
> >> should be called at the right place instead.
> >>
> >>
> >>> if (mask)
> >>> ret = ov5640_mod_reg(sensor, reg_addr, mask, val);
> >>> else
> >>> @@ -1297,9 +1305,14 @@ static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on)
> >>> * PAD OUTPUT ENABLE 02
> >>> * - [7:2]: D[5:0] output enable
> >>> */
> >>> - return ov5640_write_reg(sensor,
> >>> - OV5640_REG_PAD_OUTPUT_ENABLE02,
> >>> - on ? 0xfc : 0);
> >>> + ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE02,
> >>> + on ? 0xfc : 0);
> >>> + if (ret)
> >>> + return ret;
> >>> +
> >>> + return ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, on ?
> >>> + OV5640_REG_SYS_CTRL0_SW_PWUP :
> >>> + OV5640_REG_SYS_CTRL0_SW_PWDN);
> >>> }
> >>>
> >>> static int ov5640_set_stream_mipi(struct ov5640_dev *sensor, bool on)
> >>>
> >>
> >>
> >> BR,
> >> Hugues.
> >>
> >
> >
> > Hello everyone,
> >
> > When I updated driver in my tree with this patch, I noticed that my
> > setup using the ATMEL isc platform + ov5640 in parallel bus mode stopped
> > working.
> >
> > It looks like the culprit is this patch.
> > I am not sure which is the best way to fix it.
> > It looks like initially things work fine when probing the driver, but
> > when we want to start streaming at a later point, the register SYS_CTRL0
> > cannot be written anymore.
> > Here is an output of the log:
> >
> > --- Opening /dev/video0...
> > Trying source module v4l2...
> > /dev/video0 opened.
> > No input was specified, using the first.
> > ov5640 1-003c: ov5640_write_reg: error: reg=3008, val=2
> > atmel-sama5d2-isc f0008000.isc: stream on failed in subdev -121
> > Error starting stream.
> > VIDIOC_STREAMON: Remote I/O error
> > Unable to use mmap. Using read instead.
> > Unable to use read.
> >
> > I am using a simple application to read from /dev/video0 (which is
> > registered by the atmel-isc once the sensor probed)
> >
> > I have a feeling that something is wrong related to power, but I cannot
> > tell for sure.
> >
> > Reverting this patch makes it work fine again.
> >
> > Help is appreciated,
> > Thanks,
> > Eugen
> >
>
>
> Gentle ping.
>
> I would like to send a revert patch if nobody cares about this
> driver/patch except me.

Hugues: any thoughts on this?

Without knowing much about the sensor, the stated intent of the patch seems
reasonable. But if it breaks things, then reverting it sounds like a good
idea until a proper fix is available.

--
Kind regards,

Sakari Ailus

2021-12-21 14:47:39

by Lad, Prabhakar

[permalink] [raw]
Subject: Re: [PATCH v4 1/6] media: i2c: ov5640: Remain in power down for DVP mode unless streaming

Hi,

Sorry for the delay.

On Tue, Dec 21, 2021 at 7:37 AM <[email protected]> wrote:
>
> On 6/29/21 1:47 PM, Eugen Hristev - M18282 wrote:
> > On 9/9/20 7:16 PM, Hugues FRUCHET wrote:
> >> Hi Prabhakar,
> >>
> >> As discussed separately I would prefer to better understand issue before
> >> going to this patch.
> >> Nevertheless I have some remarks in code in case we'll need it at the end.
> >>
> >> On 9/4/20 10:18 PM, Lad Prabhakar wrote:
> >>> Keep the sensor in software power down mode and wake up only in
> >>> ov5640_set_stream_dvp() callback.
> >>>
> >>> Signed-off-by: Lad Prabhakar <[email protected]>
> >>> Reviewed-by: Biju Das <[email protected]>
> >>> Tested-by: Jacopo Mondi <[email protected]>
> >>> ---
> >>> drivers/media/i2c/ov5640.c | 19 ++++++++++++++++---
> >>> 1 file changed, 16 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> >>> index 2fe4a7ac0592..880fde73a030 100644
> >>> --- a/drivers/media/i2c/ov5640.c
> >>> +++ b/drivers/media/i2c/ov5640.c
> >>> @@ -34,6 +34,8 @@
> >>> #define OV5640_REG_SYS_RESET02 0x3002
> >>> #define OV5640_REG_SYS_CLOCK_ENABLE02 0x3006
> >>> #define OV5640_REG_SYS_CTRL0 0x3008
> >>> +#define OV5640_REG_SYS_CTRL0_SW_PWDN 0x42
> >>> +#define OV5640_REG_SYS_CTRL0_SW_PWUP 0x02
> >>
> >> For the time being this section was only referring to registers
> >> addresses and bit details was explained into a comment right before
> >> affectation, see OV5640_REG_IO_MIPI_CTRL00 for example.
> >>
> >>> #define OV5640_REG_CHIP_ID 0x300a
> >>> #define OV5640_REG_IO_MIPI_CTRL00 0x300e
> >>> #define OV5640_REG_PAD_OUTPUT_ENABLE01 0x3017
> >>> @@ -1120,6 +1122,12 @@ static int ov5640_load_regs(struct ov5640_dev *sensor,
> >>> val = regs->val;
> >>> mask = regs->mask;
> >>>
> >>> + /* remain in power down mode for DVP */
> >>> + if (regs->reg_addr == OV5640_REG_SYS_CTRL0 &&
> >>> + val == OV5640_REG_SYS_CTRL0_SW_PWUP &&
> >>> + sensor->ep.bus_type != V4L2_MBUS_CSI2_DPHY)
> >>> + continue;
> >>> +
> >>
> >> I understand that more or less register OV5640_REG_SYS_CTRL0 (0x3008)
> >> has been partially removed from big init sequence: for power-up part,
> >> but power-dwn remains at very beginning of sequence.
> >> We should completely remove 0x3008 from init sequence, including
> >> power-dwn, and introduce a new function ov5640_sw_powerdown(on/off) that
> >> should be called at the right place instead.
> >>
> >>
> >>> if (mask)
> >>> ret = ov5640_mod_reg(sensor, reg_addr, mask, val);
> >>> else
> >>> @@ -1297,9 +1305,14 @@ static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on)
> >>> * PAD OUTPUT ENABLE 02
> >>> * - [7:2]: D[5:0] output enable
> >>> */
> >>> - return ov5640_write_reg(sensor,
> >>> - OV5640_REG_PAD_OUTPUT_ENABLE02,
> >>> - on ? 0xfc : 0);
> >>> + ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE02,
> >>> + on ? 0xfc : 0);
> >>> + if (ret)
> >>> + return ret;
> >>> +
> >>> + return ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, on ?
> >>> + OV5640_REG_SYS_CTRL0_SW_PWUP :
> >>> + OV5640_REG_SYS_CTRL0_SW_PWDN);
> >>> }
> >>>
> >>> static int ov5640_set_stream_mipi(struct ov5640_dev *sensor, bool on)
> >>>
> >>
> >>
> >> BR,
> >> Hugues.
> >>
> >
> >
> > Hello everyone,
> >
> > When I updated driver in my tree with this patch, I noticed that my
> > setup using the ATMEL isc platform + ov5640 in parallel bus mode stopped
> > working.
> >
> > It looks like the culprit is this patch.
> > I am not sure which is the best way to fix it.
> > It looks like initially things work fine when probing the driver, but
> > when we want to start streaming at a later point, the register SYS_CTRL0
> > cannot be written anymore.
> > Here is an output of the log:
> >
> > --- Opening /dev/video0...
> > Trying source module v4l2...
> > /dev/video0 opened.
> > No input was specified, using the first.
> > ov5640 1-003c: ov5640_write_reg: error: reg=3008, val=2
> > atmel-sama5d2-isc f0008000.isc: stream on failed in subdev -121
> > Error starting stream.
> > VIDIOC_STREAMON: Remote I/O error
> > Unable to use mmap. Using read instead.
> > Unable to use read.
> >
> > I am using a simple application to read from /dev/video0 (which is
> > registered by the atmel-isc once the sensor probed)
> >
> > I have a feeling that something is wrong related to power, but I cannot
> > tell for sure.
> >
> > Reverting this patch makes it work fine again.
> >
> > Help is appreciated,
> > Thanks,
> > Eugen
> >
>
>
> Gentle ping.
>
> I would like to send a revert patch if nobody cares about this
> driver/patch except me.
>
I powered up the Renesas RZ/G1H connected with an ov5640 sensor and
was able to capture the video data [0] using the yavta application.
I'm fine with reverting the patch too as this works fine as well.

Just some suggestions:
- What is the application you are trying to use for capturing?
- Have you tried reducing the i2c clock speed?
- Can you try and do a dummy write for some other register in
ov5640_set_stream_dvp() to see if i2c writes are failing for all regs?
- Can you add ov5640_read_reg() in ov5640_write_reg() when
i2c_transfer() fails to check if read too is failing.

[0] https://paste.debian.net/1224317/

Cheers,
Prabhakar

2021-12-21 15:02:18

by Eugen Hristev

[permalink] [raw]
Subject: Re: [PATCH v4 1/6] media: i2c: ov5640: Remain in power down for DVP mode unless streaming

On 12/21/21 4:47 PM, Lad, Prabhakar wrote:
> Hi,
>
> Sorry for the delay.
>
> On Tue, Dec 21, 2021 at 7:37 AM <[email protected]> wrote:
>>
>> On 6/29/21 1:47 PM, Eugen Hristev - M18282 wrote:
>>> On 9/9/20 7:16 PM, Hugues FRUCHET wrote:
>>>> Hi Prabhakar,
>>>>
>>>> As discussed separately I would prefer to better understand issue before
>>>> going to this patch.
>>>> Nevertheless I have some remarks in code in case we'll need it at the end.
>>>>
>>>> On 9/4/20 10:18 PM, Lad Prabhakar wrote:
>>>>> Keep the sensor in software power down mode and wake up only in
>>>>> ov5640_set_stream_dvp() callback.
>>>>>
>>>>> Signed-off-by: Lad Prabhakar <[email protected]>
>>>>> Reviewed-by: Biju Das <[email protected]>
>>>>> Tested-by: Jacopo Mondi <[email protected]>
>>>>> ---
>>>>> drivers/media/i2c/ov5640.c | 19 ++++++++++++++++---
>>>>> 1 file changed, 16 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
>>>>> index 2fe4a7ac0592..880fde73a030 100644
>>>>> --- a/drivers/media/i2c/ov5640.c
>>>>> +++ b/drivers/media/i2c/ov5640.c
>>>>> @@ -34,6 +34,8 @@
>>>>> #define OV5640_REG_SYS_RESET02 0x3002
>>>>> #define OV5640_REG_SYS_CLOCK_ENABLE02 0x3006
>>>>> #define OV5640_REG_SYS_CTRL0 0x3008
>>>>> +#define OV5640_REG_SYS_CTRL0_SW_PWDN 0x42
>>>>> +#define OV5640_REG_SYS_CTRL0_SW_PWUP 0x02
>>>>
>>>> For the time being this section was only referring to registers
>>>> addresses and bit details was explained into a comment right before
>>>> affectation, see OV5640_REG_IO_MIPI_CTRL00 for example.
>>>>
>>>>> #define OV5640_REG_CHIP_ID 0x300a
>>>>> #define OV5640_REG_IO_MIPI_CTRL00 0x300e
>>>>> #define OV5640_REG_PAD_OUTPUT_ENABLE01 0x3017
>>>>> @@ -1120,6 +1122,12 @@ static int ov5640_load_regs(struct ov5640_dev *sensor,
>>>>> val = regs->val;
>>>>> mask = regs->mask;
>>>>>
>>>>> + /* remain in power down mode for DVP */
>>>>> + if (regs->reg_addr == OV5640_REG_SYS_CTRL0 &&
>>>>> + val == OV5640_REG_SYS_CTRL0_SW_PWUP &&
>>>>> + sensor->ep.bus_type != V4L2_MBUS_CSI2_DPHY)
>>>>> + continue;
>>>>> +
>>>>
>>>> I understand that more or less register OV5640_REG_SYS_CTRL0 (0x3008)
>>>> has been partially removed from big init sequence: for power-up part,
>>>> but power-dwn remains at very beginning of sequence.
>>>> We should completely remove 0x3008 from init sequence, including
>>>> power-dwn, and introduce a new function ov5640_sw_powerdown(on/off) that
>>>> should be called at the right place instead.
>>>>
>>>>
>>>>> if (mask)
>>>>> ret = ov5640_mod_reg(sensor, reg_addr, mask, val);
>>>>> else
>>>>> @@ -1297,9 +1305,14 @@ static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on)
>>>>> * PAD OUTPUT ENABLE 02
>>>>> * - [7:2]: D[5:0] output enable
>>>>> */
>>>>> - return ov5640_write_reg(sensor,
>>>>> - OV5640_REG_PAD_OUTPUT_ENABLE02,
>>>>> - on ? 0xfc : 0);
>>>>> + ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE02,
>>>>> + on ? 0xfc : 0);
>>>>> + if (ret)
>>>>> + return ret;
>>>>> +
>>>>> + return ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, on ?
>>>>> + OV5640_REG_SYS_CTRL0_SW_PWUP :
>>>>> + OV5640_REG_SYS_CTRL0_SW_PWDN);
>>>>> }
>>>>>
>>>>> static int ov5640_set_stream_mipi(struct ov5640_dev *sensor, bool on)
>>>>>
>>>>
>>>>
>>>> BR,
>>>> Hugues.
>>>>
>>>
>>>
>>> Hello everyone,
>>>
>>> When I updated driver in my tree with this patch, I noticed that my
>>> setup using the ATMEL isc platform + ov5640 in parallel bus mode stopped
>>> working.
>>>
>>> It looks like the culprit is this patch.
>>> I am not sure which is the best way to fix it.
>>> It looks like initially things work fine when probing the driver, but
>>> when we want to start streaming at a later point, the register SYS_CTRL0
>>> cannot be written anymore.
>>> Here is an output of the log:
>>>
>>> --- Opening /dev/video0...
>>> Trying source module v4l2...
>>> /dev/video0 opened.
>>> No input was specified, using the first.
>>> ov5640 1-003c: ov5640_write_reg: error: reg=3008, val=2
>>> atmel-sama5d2-isc f0008000.isc: stream on failed in subdev -121
>>> Error starting stream.
>>> VIDIOC_STREAMON: Remote I/O error
>>> Unable to use mmap. Using read instead.
>>> Unable to use read.
>>>
>>> I am using a simple application to read from /dev/video0 (which is
>>> registered by the atmel-isc once the sensor probed)
>>>
>>> I have a feeling that something is wrong related to power, but I cannot
>>> tell for sure.
>>>
>>> Reverting this patch makes it work fine again.
>>>
>>> Help is appreciated,
>>> Thanks,
>>> Eugen
>>>
>>
>>
>> Gentle ping.
>>
>> I would like to send a revert patch if nobody cares about this
>> driver/patch except me.
>>
> I powered up the Renesas RZ/G1H connected with an ov5640 sensor and
> was able to capture the video data [0] using the yavta application.
> I'm fine with reverting the patch too as this works fine as well.

Hi Prabhakar,

Thanks for trying this out.
ov5640 works in both parallel or CSI2 mode. Looking at the board, it
looks a parallel connection but I am not 100%, you tested using parallel
interface ?

>
> Just some suggestions:
> - What is the application you are trying to use for capturing?

I was using a simple app that reads from /dev/video0, it's called
fswebcam. I can try more apps if it's needed.

> - Have you tried reducing the i2c clock speed?

I did not, but without this patch, there is no problem whatsoever, and I
have been using this sensor since around 4.9 kernel version.

> - Can you try and do a dummy write for some other register in
> ov5640_set_stream_dvp() to see if i2c writes are failing for all regs?

I can try

> - Can you add ov5640_read_reg() in ov5640_write_reg() when
> i2c_transfer() fails to check if read too is failing.
>
> [0] https://paste.debian.net/1224317/

You seem to be able to capture successfully, I have a feeling that in my
case the sensor is somehow not powered up when the capture is being
requested.
Could you share a snippet of your device tree node for the sensor so I
can have a look ?

Thanks,
Eugen

>
> Cheers,
> Prabhakar
>

2021-12-21 15:12:25

by Lad, Prabhakar

[permalink] [raw]
Subject: Re: [PATCH v4 1/6] media: i2c: ov5640: Remain in power down for DVP mode unless streaming

Hi Eugen,

On Tue, Dec 21, 2021 at 3:02 PM <[email protected]> wrote:
>
> On 12/21/21 4:47 PM, Lad, Prabhakar wrote:
> > Hi,
> >
> > Sorry for the delay.
> >
> > On Tue, Dec 21, 2021 at 7:37 AM <[email protected]> wrote:
> >>
> >> On 6/29/21 1:47 PM, Eugen Hristev - M18282 wrote:
> >>> On 9/9/20 7:16 PM, Hugues FRUCHET wrote:
> >>>> Hi Prabhakar,
> >>>>
> >>>> As discussed separately I would prefer to better understand issue before
> >>>> going to this patch.
> >>>> Nevertheless I have some remarks in code in case we'll need it at the end.
> >>>>
> >>>> On 9/4/20 10:18 PM, Lad Prabhakar wrote:
> >>>>> Keep the sensor in software power down mode and wake up only in
> >>>>> ov5640_set_stream_dvp() callback.
> >>>>>
> >>>>> Signed-off-by: Lad Prabhakar <[email protected]>
> >>>>> Reviewed-by: Biju Das <[email protected]>
> >>>>> Tested-by: Jacopo Mondi <[email protected]>
> >>>>> ---
> >>>>> drivers/media/i2c/ov5640.c | 19 ++++++++++++++++---
> >>>>> 1 file changed, 16 insertions(+), 3 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> >>>>> index 2fe4a7ac0592..880fde73a030 100644
> >>>>> --- a/drivers/media/i2c/ov5640.c
> >>>>> +++ b/drivers/media/i2c/ov5640.c
> >>>>> @@ -34,6 +34,8 @@
> >>>>> #define OV5640_REG_SYS_RESET02 0x3002
> >>>>> #define OV5640_REG_SYS_CLOCK_ENABLE02 0x3006
> >>>>> #define OV5640_REG_SYS_CTRL0 0x3008
> >>>>> +#define OV5640_REG_SYS_CTRL0_SW_PWDN 0x42
> >>>>> +#define OV5640_REG_SYS_CTRL0_SW_PWUP 0x02
> >>>>
> >>>> For the time being this section was only referring to registers
> >>>> addresses and bit details was explained into a comment right before
> >>>> affectation, see OV5640_REG_IO_MIPI_CTRL00 for example.
> >>>>
> >>>>> #define OV5640_REG_CHIP_ID 0x300a
> >>>>> #define OV5640_REG_IO_MIPI_CTRL00 0x300e
> >>>>> #define OV5640_REG_PAD_OUTPUT_ENABLE01 0x3017
> >>>>> @@ -1120,6 +1122,12 @@ static int ov5640_load_regs(struct ov5640_dev *sensor,
> >>>>> val = regs->val;
> >>>>> mask = regs->mask;
> >>>>>
> >>>>> + /* remain in power down mode for DVP */
> >>>>> + if (regs->reg_addr == OV5640_REG_SYS_CTRL0 &&
> >>>>> + val == OV5640_REG_SYS_CTRL0_SW_PWUP &&
> >>>>> + sensor->ep.bus_type != V4L2_MBUS_CSI2_DPHY)
> >>>>> + continue;
> >>>>> +
> >>>>
> >>>> I understand that more or less register OV5640_REG_SYS_CTRL0 (0x3008)
> >>>> has been partially removed from big init sequence: for power-up part,
> >>>> but power-dwn remains at very beginning of sequence.
> >>>> We should completely remove 0x3008 from init sequence, including
> >>>> power-dwn, and introduce a new function ov5640_sw_powerdown(on/off) that
> >>>> should be called at the right place instead.
> >>>>
> >>>>
> >>>>> if (mask)
> >>>>> ret = ov5640_mod_reg(sensor, reg_addr, mask, val);
> >>>>> else
> >>>>> @@ -1297,9 +1305,14 @@ static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on)
> >>>>> * PAD OUTPUT ENABLE 02
> >>>>> * - [7:2]: D[5:0] output enable
> >>>>> */
> >>>>> - return ov5640_write_reg(sensor,
> >>>>> - OV5640_REG_PAD_OUTPUT_ENABLE02,
> >>>>> - on ? 0xfc : 0);
> >>>>> + ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE02,
> >>>>> + on ? 0xfc : 0);
> >>>>> + if (ret)
> >>>>> + return ret;
> >>>>> +
> >>>>> + return ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, on ?
> >>>>> + OV5640_REG_SYS_CTRL0_SW_PWUP :
> >>>>> + OV5640_REG_SYS_CTRL0_SW_PWDN);
> >>>>> }
> >>>>>
> >>>>> static int ov5640_set_stream_mipi(struct ov5640_dev *sensor, bool on)
> >>>>>
> >>>>
> >>>>
> >>>> BR,
> >>>> Hugues.
> >>>>
> >>>
> >>>
> >>> Hello everyone,
> >>>
> >>> When I updated driver in my tree with this patch, I noticed that my
> >>> setup using the ATMEL isc platform + ov5640 in parallel bus mode stopped
> >>> working.
> >>>
> >>> It looks like the culprit is this patch.
> >>> I am not sure which is the best way to fix it.
> >>> It looks like initially things work fine when probing the driver, but
> >>> when we want to start streaming at a later point, the register SYS_CTRL0
> >>> cannot be written anymore.
> >>> Here is an output of the log:
> >>>
> >>> --- Opening /dev/video0...
> >>> Trying source module v4l2...
> >>> /dev/video0 opened.
> >>> No input was specified, using the first.
> >>> ov5640 1-003c: ov5640_write_reg: error: reg=3008, val=2
> >>> atmel-sama5d2-isc f0008000.isc: stream on failed in subdev -121
> >>> Error starting stream.
> >>> VIDIOC_STREAMON: Remote I/O error
> >>> Unable to use mmap. Using read instead.
> >>> Unable to use read.
> >>>
> >>> I am using a simple application to read from /dev/video0 (which is
> >>> registered by the atmel-isc once the sensor probed)
> >>>
> >>> I have a feeling that something is wrong related to power, but I cannot
> >>> tell for sure.
> >>>
> >>> Reverting this patch makes it work fine again.
> >>>
> >>> Help is appreciated,
> >>> Thanks,
> >>> Eugen
> >>>
> >>
> >>
> >> Gentle ping.
> >>
> >> I would like to send a revert patch if nobody cares about this
> >> driver/patch except me.
> >>
> > I powered up the Renesas RZ/G1H connected with an ov5640 sensor and
> > was able to capture the video data [0] using the yavta application.
> > I'm fine with reverting the patch too as this works fine as well.
>
> Hi Prabhakar,
>
> Thanks for trying this out.
> ov5640 works in both parallel or CSI2 mode. Looking at the board, it
> looks a parallel connection but I am not 100%, you tested using parallel
> interface ?
>
I have tested it in parallel interface mode as RZ/G1H supports parallel capture

> >
> > Just some suggestions:
> > - What is the application you are trying to use for capturing?
>
> I was using a simple app that reads from /dev/video0, it's called
> fswebcam. I can try more apps if it's needed.
>
could you give it a shot with yavta please.

> > - Have you tried reducing the i2c clock speed?
>
> I did not, but without this patch, there is no problem whatsoever, and I
> have been using this sensor since around 4.9 kernel version.
>
Agreed, I'm thinking aloud just to narrow things.

> > - Can you try and do a dummy write for some other register in
> > ov5640_set_stream_dvp() to see if i2c writes are failing for all regs?
>
> I can try
>
Thank you.

> > - Can you add ov5640_read_reg() in ov5640_write_reg() when
> > i2c_transfer() fails to check if read too is failing.
> >
> > [0] https://paste.debian.net/1224317/
>
> You seem to be able to capture successfully, I have a feeling that in my
> case the sensor is somehow not powered up when the capture is being
> requested.
> Could you share a snippet of your device tree node for the sensor so I
> can have a look ?
>
[0] contains the bridge node and [1] contains the sensor node.

[0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/r8a7742-iwg21d-q7-dbcm-ca.dts?h=v5.16-rc6#n309
[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/r8a7742-iwg21d-q7-dbcm-ov5640-single.dtsi?h=v5.16-rc6

Cheers,
Prabhakar

2022-01-03 11:29:33

by Eugen Hristev

[permalink] [raw]
Subject: Re: [PATCH v4 1/6] media: i2c: ov5640: Remain in power down for DVP mode unless streaming

On 12/21/21 5:11 PM, Lad, Prabhakar wrote:
> Hi Eugen,
>
> On Tue, Dec 21, 2021 at 3:02 PM <[email protected]> wrote:
>>
>> On 12/21/21 4:47 PM, Lad, Prabhakar wrote:
>>> Hi,
>>>
>>> Sorry for the delay.
>>>
>>> On Tue, Dec 21, 2021 at 7:37 AM <[email protected]> wrote:
>>>>
>>>> On 6/29/21 1:47 PM, Eugen Hristev - M18282 wrote:
>>>>> On 9/9/20 7:16 PM, Hugues FRUCHET wrote:
>>>>>> Hi Prabhakar,
>>>>>>
>>>>>> As discussed separately I would prefer to better understand issue before
>>>>>> going to this patch.
>>>>>> Nevertheless I have some remarks in code in case we'll need it at the end.
>>>>>>
>>>>>> On 9/4/20 10:18 PM, Lad Prabhakar wrote:
>>>>>>> Keep the sensor in software power down mode and wake up only in
>>>>>>> ov5640_set_stream_dvp() callback.
>>>>>>>
>>>>>>> Signed-off-by: Lad Prabhakar <[email protected]>
>>>>>>> Reviewed-by: Biju Das <[email protected]>
>>>>>>> Tested-by: Jacopo Mondi <[email protected]>
>>>>>>> ---
>>>>>>> drivers/media/i2c/ov5640.c | 19 ++++++++++++++++---
>>>>>>> 1 file changed, 16 insertions(+), 3 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
>>>>>>> index 2fe4a7ac0592..880fde73a030 100644
>>>>>>> --- a/drivers/media/i2c/ov5640.c
>>>>>>> +++ b/drivers/media/i2c/ov5640.c
>>>>>>> @@ -34,6 +34,8 @@
>>>>>>> #define OV5640_REG_SYS_RESET02 0x3002
>>>>>>> #define OV5640_REG_SYS_CLOCK_ENABLE02 0x3006
>>>>>>> #define OV5640_REG_SYS_CTRL0 0x3008
>>>>>>> +#define OV5640_REG_SYS_CTRL0_SW_PWDN 0x42
>>>>>>> +#define OV5640_REG_SYS_CTRL0_SW_PWUP 0x02
>>>>>>
>>>>>> For the time being this section was only referring to registers
>>>>>> addresses and bit details was explained into a comment right before
>>>>>> affectation, see OV5640_REG_IO_MIPI_CTRL00 for example.
>>>>>>
>>>>>>> #define OV5640_REG_CHIP_ID 0x300a
>>>>>>> #define OV5640_REG_IO_MIPI_CTRL00 0x300e
>>>>>>> #define OV5640_REG_PAD_OUTPUT_ENABLE01 0x3017
>>>>>>> @@ -1120,6 +1122,12 @@ static int ov5640_load_regs(struct ov5640_dev *sensor,
>>>>>>> val = regs->val;
>>>>>>> mask = regs->mask;
>>>>>>>
>>>>>>> + /* remain in power down mode for DVP */
>>>>>>> + if (regs->reg_addr == OV5640_REG_SYS_CTRL0 &&
>>>>>>> + val == OV5640_REG_SYS_CTRL0_SW_PWUP &&
>>>>>>> + sensor->ep.bus_type != V4L2_MBUS_CSI2_DPHY)
>>>>>>> + continue;
>>>>>>> +
>>>>>>
>>>>>> I understand that more or less register OV5640_REG_SYS_CTRL0 (0x3008)
>>>>>> has been partially removed from big init sequence: for power-up part,
>>>>>> but power-dwn remains at very beginning of sequence.
>>>>>> We should completely remove 0x3008 from init sequence, including
>>>>>> power-dwn, and introduce a new function ov5640_sw_powerdown(on/off) that
>>>>>> should be called at the right place instead.
>>>>>>
>>>>>>
>>>>>>> if (mask)
>>>>>>> ret = ov5640_mod_reg(sensor, reg_addr, mask, val);
>>>>>>> else
>>>>>>> @@ -1297,9 +1305,14 @@ static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on)
>>>>>>> * PAD OUTPUT ENABLE 02
>>>>>>> * - [7:2]: D[5:0] output enable
>>>>>>> */
>>>>>>> - return ov5640_write_reg(sensor,
>>>>>>> - OV5640_REG_PAD_OUTPUT_ENABLE02,
>>>>>>> - on ? 0xfc : 0);
>>>>>>> + ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE02,
>>>>>>> + on ? 0xfc : 0);
>>>>>>> + if (ret)
>>>>>>> + return ret;
>>>>>>> +
>>>>>>> + return ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, on ?
>>>>>>> + OV5640_REG_SYS_CTRL0_SW_PWUP :
>>>>>>> + OV5640_REG_SYS_CTRL0_SW_PWDN);
>>>>>>> }
>>>>>>>
>>>>>>> static int ov5640_set_stream_mipi(struct ov5640_dev *sensor, bool on)
>>>>>>>
>>>>>>
>>>>>>
>>>>>> BR,
>>>>>> Hugues.
>>>>>>
>>>>>
>>>>>
>>>>> Hello everyone,
>>>>>
>>>>> When I updated driver in my tree with this patch, I noticed that my
>>>>> setup using the ATMEL isc platform + ov5640 in parallel bus mode stopped
>>>>> working.
>>>>>
>>>>> It looks like the culprit is this patch.
>>>>> I am not sure which is the best way to fix it.
>>>>> It looks like initially things work fine when probing the driver, but
>>>>> when we want to start streaming at a later point, the register SYS_CTRL0
>>>>> cannot be written anymore.
>>>>> Here is an output of the log:
>>>>>
>>>>> --- Opening /dev/video0...
>>>>> Trying source module v4l2...
>>>>> /dev/video0 opened.
>>>>> No input was specified, using the first.
>>>>> ov5640 1-003c: ov5640_write_reg: error: reg=3008, val=2
>>>>> atmel-sama5d2-isc f0008000.isc: stream on failed in subdev -121
>>>>> Error starting stream.
>>>>> VIDIOC_STREAMON: Remote I/O error
>>>>> Unable to use mmap. Using read instead.
>>>>> Unable to use read.
>>>>>
>>>>> I am using a simple application to read from /dev/video0 (which is
>>>>> registered by the atmel-isc once the sensor probed)
>>>>>
>>>>> I have a feeling that something is wrong related to power, but I cannot
>>>>> tell for sure.
>>>>>
>>>>> Reverting this patch makes it work fine again.
>>>>>
>>>>> Help is appreciated,
>>>>> Thanks,
>>>>> Eugen
>>>>>
>>>>
>>>>
>>>> Gentle ping.
>>>>
>>>> I would like to send a revert patch if nobody cares about this
>>>> driver/patch except me.
>>>>
>>> I powered up the Renesas RZ/G1H connected with an ov5640 sensor and
>>> was able to capture the video data [0] using the yavta application.
>>> I'm fine with reverting the patch too as this works fine as well.
>>
>> Hi Prabhakar,
>>
>> Thanks for trying this out.
>> ov5640 works in both parallel or CSI2 mode. Looking at the board, it
>> looks a parallel connection but I am not 100%, you tested using parallel
>> interface ?
>>
> I have tested it in parallel interface mode as RZ/G1H supports parallel capture
>
>>>
>>> Just some suggestions:
>>> - What is the application you are trying to use for capturing?
>>
>> I was using a simple app that reads from /dev/video0, it's called
>> fswebcam. I can try more apps if it's needed.
>>
> could you give it a shot with yavta please.

Hello Lad,

I debugged this further, and I have some news:

It looks like the 'write 0x2 to SYS_CLTR0' does not fail itself, rather
the sensor refuses to accept a power up.

I tried to read the register before the write, and it reads 0x42.
Then, I tried to write 0x42 back, and it works fine.
So, I do not think there is a problem with i2c communication.
The only problem is that the sensor refuses to power up (accept the 0x2
into the SYS_CTRL_0 ), due to an unknown (to me) reason.

If the power up is performed at the initialization phase, it works.

I also tried to capture with v4l2-ctl, and the result is the same.

Which of the init configuration set of registers your test is using?
It may be that it does not work in a specific config .

The datasheet which I have does not claim that the 'power up' might fail
in some circumstances.

Thanks for helping,

Eugen


>
>>> - Have you tried reducing the i2c clock speed?
>>
>> I did not, but without this patch, there is no problem whatsoever, and I
>> have been using this sensor since around 4.9 kernel version.
>>
> Agreed, I'm thinking aloud just to narrow things.
>
>>> - Can you try and do a dummy write for some other register in
>>> ov5640_set_stream_dvp() to see if i2c writes are failing for all regs?
>>
>> I can try
>>
> Thank you.
>
>>> - Can you add ov5640_read_reg() in ov5640_write_reg() when
>>> i2c_transfer() fails to check if read too is failing.
>>>
>>> [0] https://paste.debian.net/1224317/
>>
>> You seem to be able to capture successfully, I have a feeling that in my
>> case the sensor is somehow not powered up when the capture is being
>> requested.
>> Could you share a snippet of your device tree node for the sensor so I
>> can have a look ?
>>
> [0] contains the bridge node and [1] contains the sensor node.
>
> [0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/r8a7742-iwg21d-q7-dbcm-ca.dts?h=v5.16-rc6#n309
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/r8a7742-iwg21d-q7-dbcm-ov5640-single.dtsi?h=v5.16-rc6
>
> Cheers,
> Prabhakar
>

2022-01-04 09:58:24

by Lad, Prabhakar

[permalink] [raw]
Subject: Re: [PATCH v4 1/6] media: i2c: ov5640: Remain in power down for DVP mode unless streaming

Hi Eugen,

On Mon, Jan 3, 2022 at 11:29 AM <[email protected]> wrote:
>
> On 12/21/21 5:11 PM, Lad, Prabhakar wrote:
> > Hi Eugen,
> >
> > On Tue, Dec 21, 2021 at 3:02 PM <[email protected]> wrote:
> >>
> >> On 12/21/21 4:47 PM, Lad, Prabhakar wrote:
> >>> Hi,
> >>>
> >>> Sorry for the delay.
> >>>
> >>> On Tue, Dec 21, 2021 at 7:37 AM <[email protected]> wrote:
> >>>>
> >>>> On 6/29/21 1:47 PM, Eugen Hristev - M18282 wrote:
> >>>>> On 9/9/20 7:16 PM, Hugues FRUCHET wrote:
> >>>>>> Hi Prabhakar,
> >>>>>>
> >>>>>> As discussed separately I would prefer to better understand issue before
> >>>>>> going to this patch.
> >>>>>> Nevertheless I have some remarks in code in case we'll need it at the end.
> >>>>>>
> >>>>>> On 9/4/20 10:18 PM, Lad Prabhakar wrote:
> >>>>>>> Keep the sensor in software power down mode and wake up only in
> >>>>>>> ov5640_set_stream_dvp() callback.
> >>>>>>>
> >>>>>>> Signed-off-by: Lad Prabhakar <[email protected]>
> >>>>>>> Reviewed-by: Biju Das <[email protected]>
> >>>>>>> Tested-by: Jacopo Mondi <[email protected]>
> >>>>>>> ---
> >>>>>>> drivers/media/i2c/ov5640.c | 19 ++++++++++++++++---
> >>>>>>> 1 file changed, 16 insertions(+), 3 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> >>>>>>> index 2fe4a7ac0592..880fde73a030 100644
> >>>>>>> --- a/drivers/media/i2c/ov5640.c
> >>>>>>> +++ b/drivers/media/i2c/ov5640.c
> >>>>>>> @@ -34,6 +34,8 @@
> >>>>>>> #define OV5640_REG_SYS_RESET02 0x3002
> >>>>>>> #define OV5640_REG_SYS_CLOCK_ENABLE02 0x3006
> >>>>>>> #define OV5640_REG_SYS_CTRL0 0x3008
> >>>>>>> +#define OV5640_REG_SYS_CTRL0_SW_PWDN 0x42
> >>>>>>> +#define OV5640_REG_SYS_CTRL0_SW_PWUP 0x02
> >>>>>>
> >>>>>> For the time being this section was only referring to registers
> >>>>>> addresses and bit details was explained into a comment right before
> >>>>>> affectation, see OV5640_REG_IO_MIPI_CTRL00 for example.
> >>>>>>
> >>>>>>> #define OV5640_REG_CHIP_ID 0x300a
> >>>>>>> #define OV5640_REG_IO_MIPI_CTRL00 0x300e
> >>>>>>> #define OV5640_REG_PAD_OUTPUT_ENABLE01 0x3017
> >>>>>>> @@ -1120,6 +1122,12 @@ static int ov5640_load_regs(struct ov5640_dev *sensor,
> >>>>>>> val = regs->val;
> >>>>>>> mask = regs->mask;
> >>>>>>>
> >>>>>>> + /* remain in power down mode for DVP */
> >>>>>>> + if (regs->reg_addr == OV5640_REG_SYS_CTRL0 &&
> >>>>>>> + val == OV5640_REG_SYS_CTRL0_SW_PWUP &&
> >>>>>>> + sensor->ep.bus_type != V4L2_MBUS_CSI2_DPHY)
> >>>>>>> + continue;
> >>>>>>> +
> >>>>>>
> >>>>>> I understand that more or less register OV5640_REG_SYS_CTRL0 (0x3008)
> >>>>>> has been partially removed from big init sequence: for power-up part,
> >>>>>> but power-dwn remains at very beginning of sequence.
> >>>>>> We should completely remove 0x3008 from init sequence, including
> >>>>>> power-dwn, and introduce a new function ov5640_sw_powerdown(on/off) that
> >>>>>> should be called at the right place instead.
> >>>>>>
> >>>>>>
> >>>>>>> if (mask)
> >>>>>>> ret = ov5640_mod_reg(sensor, reg_addr, mask, val);
> >>>>>>> else
> >>>>>>> @@ -1297,9 +1305,14 @@ static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on)
> >>>>>>> * PAD OUTPUT ENABLE 02
> >>>>>>> * - [7:2]: D[5:0] output enable
> >>>>>>> */
> >>>>>>> - return ov5640_write_reg(sensor,
> >>>>>>> - OV5640_REG_PAD_OUTPUT_ENABLE02,
> >>>>>>> - on ? 0xfc : 0);
> >>>>>>> + ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE02,
> >>>>>>> + on ? 0xfc : 0);
> >>>>>>> + if (ret)
> >>>>>>> + return ret;
> >>>>>>> +
> >>>>>>> + return ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, on ?
> >>>>>>> + OV5640_REG_SYS_CTRL0_SW_PWUP :
> >>>>>>> + OV5640_REG_SYS_CTRL0_SW_PWDN);
> >>>>>>> }
> >>>>>>>
> >>>>>>> static int ov5640_set_stream_mipi(struct ov5640_dev *sensor, bool on)
> >>>>>>>
> >>>>>>
> >>>>>>
> >>>>>> BR,
> >>>>>> Hugues.
> >>>>>>
> >>>>>
> >>>>>
> >>>>> Hello everyone,
> >>>>>
> >>>>> When I updated driver in my tree with this patch, I noticed that my
> >>>>> setup using the ATMEL isc platform + ov5640 in parallel bus mode stopped
> >>>>> working.
> >>>>>
> >>>>> It looks like the culprit is this patch.
> >>>>> I am not sure which is the best way to fix it.
> >>>>> It looks like initially things work fine when probing the driver, but
> >>>>> when we want to start streaming at a later point, the register SYS_CTRL0
> >>>>> cannot be written anymore.
> >>>>> Here is an output of the log:
> >>>>>
> >>>>> --- Opening /dev/video0...
> >>>>> Trying source module v4l2...
> >>>>> /dev/video0 opened.
> >>>>> No input was specified, using the first.
> >>>>> ov5640 1-003c: ov5640_write_reg: error: reg=3008, val=2
> >>>>> atmel-sama5d2-isc f0008000.isc: stream on failed in subdev -121
> >>>>> Error starting stream.
> >>>>> VIDIOC_STREAMON: Remote I/O error
> >>>>> Unable to use mmap. Using read instead.
> >>>>> Unable to use read.
> >>>>>
> >>>>> I am using a simple application to read from /dev/video0 (which is
> >>>>> registered by the atmel-isc once the sensor probed)
> >>>>>
> >>>>> I have a feeling that something is wrong related to power, but I cannot
> >>>>> tell for sure.
> >>>>>
> >>>>> Reverting this patch makes it work fine again.
> >>>>>
> >>>>> Help is appreciated,
> >>>>> Thanks,
> >>>>> Eugen
> >>>>>
> >>>>
> >>>>
> >>>> Gentle ping.
> >>>>
> >>>> I would like to send a revert patch if nobody cares about this
> >>>> driver/patch except me.
> >>>>
> >>> I powered up the Renesas RZ/G1H connected with an ov5640 sensor and
> >>> was able to capture the video data [0] using the yavta application.
> >>> I'm fine with reverting the patch too as this works fine as well.
> >>
> >> Hi Prabhakar,
> >>
> >> Thanks for trying this out.
> >> ov5640 works in both parallel or CSI2 mode. Looking at the board, it
> >> looks a parallel connection but I am not 100%, you tested using parallel
> >> interface ?
> >>
> > I have tested it in parallel interface mode as RZ/G1H supports parallel capture
> >
> >>>
> >>> Just some suggestions:
> >>> - What is the application you are trying to use for capturing?
> >>
> >> I was using a simple app that reads from /dev/video0, it's called
> >> fswebcam. I can try more apps if it's needed.
> >>
> > could you give it a shot with yavta please.
>
> Hello Lad,
>
> I debugged this further, and I have some news:
>
> It looks like the 'write 0x2 to SYS_CLTR0' does not fail itself, rather
> the sensor refuses to accept a power up.
>
> I tried to read the register before the write, and it reads 0x42.
> Then, I tried to write 0x42 back, and it works fine.
> So, I do not think there is a problem with i2c communication.
> The only problem is that the sensor refuses to power up (accept the 0x2
> into the SYS_CTRL_0 ), due to an unknown (to me) reason.
>
That's strange.

> If the power up is performed at the initialization phase, it works.
>
> I also tried to capture with v4l2-ctl, and the result is the same.
>
you mean yavta ?

> Which of the init configuration set of registers your test is using?
I have been testing 320x240 and 640x480. Could you give that a try please?

> It may be that it does not work in a specific config .
>
> The datasheet which I have does not claim that the 'power up' might fail
> in some circumstances.
>
Let me check if I can ping OmniVision FAE.

> Thanks for helping,
>
> Eugen
>
>
> >
> >>> - Have you tried reducing the i2c clock speed?
> >>
> >> I did not, but without this patch, there is no problem whatsoever, and I
> >> have been using this sensor since around 4.9 kernel version.
> >>
> > Agreed, I'm thinking aloud just to narrow things.
> >
> >>> - Can you try and do a dummy write for some other register in
> >>> ov5640_set_stream_dvp() to see if i2c writes are failing for all regs?
> >>
> >> I can try
> >>
> > Thank you.
> >
> >>> - Can you add ov5640_read_reg() in ov5640_write_reg() when
> >>> i2c_transfer() fails to check if read too is failing.
> >>>
> >>> [0] https://paste.debian.net/1224317/
> >>
> >> You seem to be able to capture successfully, I have a feeling that in my
> >> case the sensor is somehow not powered up when the capture is being
> >> requested.
> >> Could you share a snippet of your device tree node for the sensor so I
> >> can have a look ?
> >>
> > [0] contains the bridge node and [1] contains the sensor node.
> >
> > [0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/r8a7742-iwg21d-q7-dbcm-ca.dts?h=v5.16-rc6#n309
> > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/r8a7742-iwg21d-q7-dbcm-ov5640-single.dtsi?h=v5.16-rc6
> >
Cheers,
Prabhakar

2022-01-07 15:50:22

by Lad, Prabhakar

[permalink] [raw]
Subject: Re: [PATCH v4 1/6] media: i2c: ov5640: Remain in power down for DVP mode unless streaming

On Tue, Jan 4, 2022 at 9:57 AM Lad, Prabhakar
<[email protected]> wrote:
>
> Hi Eugen,
>
> On Mon, Jan 3, 2022 at 11:29 AM <[email protected]> wrote:
> >
> > On 12/21/21 5:11 PM, Lad, Prabhakar wrote:
> > > Hi Eugen,
> > >
<snip>
> > > could you give it a shot with yavta please.
> >
> > Hello Lad,
> >
> > I debugged this further, and I have some news:
> >
> > It looks like the 'write 0x2 to SYS_CLTR0' does not fail itself, rather
> > the sensor refuses to accept a power up.
> >
> > I tried to read the register before the write, and it reads 0x42.
> > Then, I tried to write 0x42 back, and it works fine.
> > So, I do not think there is a problem with i2c communication.
> > The only problem is that the sensor refuses to power up (accept the 0x2
> > into the SYS_CTRL_0 ), due to an unknown (to me) reason.
> >
> That's strange.
>
> > If the power up is performed at the initialization phase, it works.
> >
> > I also tried to capture with v4l2-ctl, and the result is the same.
> >
> you mean yavta ?
>
> > Which of the init configuration set of registers your test is using?
> I have been testing 320x240 and 640x480. Could you give that a try please?
>
> > It may be that it does not work in a specific config .
> >
> > The datasheet which I have does not claim that the 'power up' might fail
> > in some circumstances.
> >
> Let me check if I can ping OmniVision FAE.
>
Fyi.. I got the below feedback from OmniVision FAE.

SW standby bit is working as expected from my side.


As far as the sensor initialization is concerned we use HW power up
sequence defined in the datasheet followed by SW initialization.

SW initialization consist of the following :-

78 3103 11 ; I2C timing ( do not modify)
78 3008 82 ; SW reset
78 3008 42 ; Stop streaming
78 ……. Sensor settings for required mode

78 3008 02 ; Start streaming

Note:- 0x3008[7] SW reset bit is volatile, as soon as a reset is
applied the all the bits are cleared and 0x3008[7:0] set to
default value hence should read 0x2

Cheers,
Prabhakar