mbox series

[v3,0/3] media: ov5640: Fix power up sequence delays

Message ID 20221227173634.5752-1-j-luthra@ti.com
Headers show
Series media: ov5640: Fix power up sequence delays | expand

Message

Jai Luthra Dec. 27, 2022, 5:36 p.m. UTC 
This series fixes the power-up sequence delays to support some 15-pin 
FFC compatible OV5640 modules.

Without appropriate delays after both gpio and register-based powerdown 
and reset the sensor SCCB was not very stable, and probe would sometimes 
fail at check_chip_id.

Changes in v3:
- Move register-based reset to the common powerup_sequence function
- Only add 5ms delay for PWUP not for PWDN in ov5640_set_stream_dvp

v2: https://lore.kernel.org/all/20221219143056.4070-1-j-luthra@ti.com/

Jai Luthra (2):
  media: ov5640: Handle delays when no reset_gpio set
  media: ov5640: Fix soft reset sequence and timings

Nishanth Menon (1):
  media: ov5640: Honor power on time in init_setting

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

Comments

Jacopo Mondi Dec. 29, 2022, 5:37 p.m. UTC | #1 
Hi Jai

On Tue, Dec 27, 2022 at 11:06:34PM +0530, Jai Luthra wrote:
> From: Nishanth Menon <nm@ti.com>
>
> OV5640 Datasheet[1] Figures 2-3 and 2-4 indicate the timing sequences
> that is expected during various initialization steps. Note the power
> on time includes t0 + t1 + t2 >= 5ms, delay for poweron.
>
> As indicated in section 2.8, the PWDN assertion can either be via
> external pin control OR via the register 0x3008 bit 6 (see table 7-1 in
> [1])
>
> [1] https://cdn.sparkfun.com/datasheets/Sensors/LightImaging/OV5640_datasheet.pdf
>
> Fixes: 19a81c1426c1 ("[media] add Omnivision OV5640 sensor driver")
> Signed-off-by: Nishanth Menon <nm@ti.com>
> Signed-off-by: Jai Luthra <j-luthra@ti.com>
> ---
>  drivers/media/i2c/ov5640.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index 5df16fb6f0a0..bd7cc294cfe6 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -609,7 +609,7 @@ static const struct reg_value ov5640_init_setting[] = {
>  	{0x583b, 0x28, 0, 0}, {0x583c, 0x42, 0, 0}, {0x583d, 0xce, 0, 0},
>  	{0x5025, 0x00, 0, 0}, {0x3a0f, 0x30, 0, 0}, {0x3a10, 0x28, 0, 0},
>  	{0x3a1b, 0x30, 0, 0}, {0x3a1e, 0x26, 0, 0}, {0x3a11, 0x60, 0, 0},
> -	{0x3a1f, 0x14, 0, 0}, {0x3008, 0x02, 0, 0}, {0x3c00, 0x04, 0, 300},
> +	{0x3a1f, 0x14, 0, 0}, {0x3008, 0x02, 0, 5}, {0x3c00, 0x04, 0, 300},
>  };
>
>  static const struct reg_value ov5640_setting_low_res[] = {
> @@ -1810,9 +1810,17 @@ static int ov5640_set_autogain(struct ov5640_dev *sensor, bool on)
>
>  static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on)
>  {
> -	return ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, on ?
> -				OV5640_REG_SYS_CTRL0_SW_PWUP :
> -				OV5640_REG_SYS_CTRL0_SW_PWDN);
> +	int ret;
> +
> +	if (on) {
> +		ret = ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0,
> +				       OV5640_REG_SYS_CTRL0_SW_PWUP);
> +		usleep_range(5000, 6000);
> +	} else {
> +		ret = ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0,
> +				       OV5640_REG_SYS_CTRL0_SW_PWDN);
> +	}
> +	return ret;

Nipicking, I prefer maintaining the original version with just an
additional usleep

        int ret = ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, on ?
                                   OV5640_REG_SYS_CTRL0_SW_PWUP :
                                   OV5640_REG_SYS_CTRL0_SW_PWDN);

        if (on)
                usleep_range(5000, 6000);

        return ret;

As you prefer

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>



>  }
>
>  static int ov5640_set_stream_mipi(struct ov5640_dev *sensor, bool on)
> --
> 2.17.1
>
Jacopo Mondi Dec. 29, 2022, 6:10 p.m. UTC | #2 
Hi again

On Tue, Dec 27, 2022 at 11:06:34PM +0530, Jai Luthra wrote:
> From: Nishanth Menon <nm@ti.com>
>
> OV5640 Datasheet[1] Figures 2-3 and 2-4 indicate the timing sequences
> that is expected during various initialization steps. Note the power
> on time includes t0 + t1 + t2 >= 5ms, delay for poweron.
>
> As indicated in section 2.8, the PWDN assertion can either be via
> external pin control OR via the register 0x3008 bit 6 (see table 7-1 in
> [1])
>
> [1] https://cdn.sparkfun.com/datasheets/Sensors/LightImaging/OV5640_datasheet.pdf
>
> Fixes: 19a81c1426c1 ("[media] add Omnivision OV5640 sensor driver")
> Signed-off-by: Nishanth Menon <nm@ti.com>
> Signed-off-by: Jai Luthra <j-luthra@ti.com>
> ---
>  drivers/media/i2c/ov5640.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index 5df16fb6f0a0..bd7cc294cfe6 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -609,7 +609,7 @@ static const struct reg_value ov5640_init_setting[] = {
>  	{0x583b, 0x28, 0, 0}, {0x583c, 0x42, 0, 0}, {0x583d, 0xce, 0, 0},
>  	{0x5025, 0x00, 0, 0}, {0x3a0f, 0x30, 0, 0}, {0x3a10, 0x28, 0, 0},
>  	{0x3a1b, 0x30, 0, 0}, {0x3a1e, 0x26, 0, 0}, {0x3a11, 0x60, 0, 0},
> -	{0x3a1f, 0x14, 0, 0}, {0x3008, 0x02, 0, 0}, {0x3c00, 0x04, 0, 300},
> +	{0x3a1f, 0x14, 0, 0}, {0x3008, 0x02, 0, 5}, {0x3c00, 0x04, 0, 300},

This might also allow to remove the

		/* remain in power down mode for DVP */
		if (regs->reg_addr == OV5640_REG_SYS_CTRL0 &&
		    val == OV5640_REG_SYS_CTRL0_SW_PWUP &&
		    !ov5640_is_csi2(sensor))
			continue;

in ov5640_load_regs()

Prabhakar since you have introduced it, coudl you test if you still
need it with Nishanth's patch applied ?

Thanks
  j


>  };
>
>  static const struct reg_value ov5640_setting_low_res[] = {
> @@ -1810,9 +1810,17 @@ static int ov5640_set_autogain(struct ov5640_dev *sensor, bool on)
>
>  static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on)
>  {
> -	return ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, on ?
> -				OV5640_REG_SYS_CTRL0_SW_PWUP :
> -				OV5640_REG_SYS_CTRL0_SW_PWDN);
> +	int ret;
> +
> +	if (on) {
> +		ret = ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0,
> +				       OV5640_REG_SYS_CTRL0_SW_PWUP);
> +		usleep_range(5000, 6000);
> +	} else {
> +		ret = ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0,
> +				       OV5640_REG_SYS_CTRL0_SW_PWDN);
> +	}
> +	return ret;
>  }
>
>  static int ov5640_set_stream_mipi(struct ov5640_dev *sensor, bool on)
> --
> 2.17.1
>
Jacopo Mondi Jan. 2, 2023, 12:59 p.m. UTC | #3 
Hello again

On Thu, Dec 29, 2022 at 07:10:36PM +0100, Jacopo Mondi wrote:
> Hi again
>
> On Tue, Dec 27, 2022 at 11:06:34PM +0530, Jai Luthra wrote:
> > From: Nishanth Menon <nm@ti.com>
> >
> > OV5640 Datasheet[1] Figures 2-3 and 2-4 indicate the timing sequences
> > that is expected during various initialization steps. Note the power
> > on time includes t0 + t1 + t2 >= 5ms, delay for poweron.
> >
> > As indicated in section 2.8, the PWDN assertion can either be via
> > external pin control OR via the register 0x3008 bit 6 (see table 7-1 in
> > [1])
> >
> > [1] https://cdn.sparkfun.com/datasheets/Sensors/LightImaging/OV5640_datasheet.pdf
> >
> > Fixes: 19a81c1426c1 ("[media] add Omnivision OV5640 sensor driver")
> > Signed-off-by: Nishanth Menon <nm@ti.com>
> > Signed-off-by: Jai Luthra <j-luthra@ti.com>
> > ---
> >  drivers/media/i2c/ov5640.c | 16 ++++++++++++----
> >  1 file changed, 12 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> > index 5df16fb6f0a0..bd7cc294cfe6 100644
> > --- a/drivers/media/i2c/ov5640.c
> > +++ b/drivers/media/i2c/ov5640.c
> > @@ -609,7 +609,7 @@ static const struct reg_value ov5640_init_setting[] = {
> >  	{0x583b, 0x28, 0, 0}, {0x583c, 0x42, 0, 0}, {0x583d, 0xce, 0, 0},
> >  	{0x5025, 0x00, 0, 0}, {0x3a0f, 0x30, 0, 0}, {0x3a10, 0x28, 0, 0},
> >  	{0x3a1b, 0x30, 0, 0}, {0x3a1e, 0x26, 0, 0}, {0x3a11, 0x60, 0, 0},
> > -	{0x3a1f, 0x14, 0, 0}, {0x3008, 0x02, 0, 0}, {0x3c00, 0x04, 0, 300},
> > +	{0x3a1f, 0x14, 0, 0}, {0x3008, 0x02, 0, 5}, {0x3c00, 0x04, 0, 300},
>
> This might also allow to remove the
>
> 		/* remain in power down mode for DVP */
> 		if (regs->reg_addr == OV5640_REG_SYS_CTRL0 &&
> 		    val == OV5640_REG_SYS_CTRL0_SW_PWUP &&
> 		    !ov5640_is_csi2(sensor))
> 			continue;
>
> in ov5640_load_regs()
>
> Prabhakar since you have introduced it, coudl you test if you still
> need it with Nishanth's patch applied ?

No, it doesn't, sorry for the confusion.

The following patch would allow to remove the above quirk by removing
        {0x3008, 0x02}
from the init_sequence[].

Unfortunately the first 3 images are then black when running in CSI-2
mode.

-------------------------------------------------------------------------------
diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 96b986051414..bfb60648c72a 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -609,7 +609,7 @@ static const struct reg_value ov5640_init_setting[] = {
        {0x583b, 0x28, 0, 0}, {0x583c, 0x42, 0, 0}, {0x583d, 0xce, 0, 0},
        {0x5025, 0x00, 0, 0}, {0x3a0f, 0x30, 0, 0}, {0x3a10, 0x28, 0, 0},
        {0x3a1b, 0x30, 0, 0}, {0x3a1e, 0x26, 0, 0}, {0x3a11, 0x60, 0, 0},
-       {0x3a1f, 0x14, 0, 0}, {0x3008, 0x02, 0, 5}, {0x3c00, 0x04, 0, 300},
+       {0x3a1f, 0x14, 0, 0}, {0x3c00, 0x04, 0, 300},
 };

 static const struct reg_value ov5640_setting_low_res[] = {
@@ -1808,7 +1808,7 @@ static int ov5640_set_autogain(struct ov5640_dev *sensor, bool on)
                              BIT(1), on ? 0 : BIT(1));
 }

-static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on)
+static int ov5640_set_stream(struct ov5640_dev *sensor, bool on)
 {
        int ret;

@@ -3725,11 +3725,13 @@ static int ov5640_s_stream(struct v4l2_subdev *sd, int enable)
                        sensor->pending_fmt_change = false;
                }

-               if (ov5640_is_csi2(sensor))
+               if (ov5640_is_csi2(sensor)) {
                        ret = ov5640_set_stream_mipi(sensor, enable);
-               else
-                       ret = ov5640_set_stream_dvp(sensor, enable);
+                       if (ret)
+                               goto out;
+               }

+               ret = ov5640_set_stream(sensor, enable);
                if (!ret)
                        sensor->streaming = enable;
        }
-------------------------------------------------------------------------------

I do however now question the patch utility itself. SW PWDN is the
software standby state, while Figure 2.3 and 2.4 you mention in the
commit message report:

t2 = 5 ms: delay from DVDD stable to sensor power up stable

I doubt this apply to SW standby as it the delay is probably required
to have the internal circuitry stabilize after the power rail has been
enabled.

Does this patch make any practical difference in your setup ? I'm
asking as in my case it doesn't, but I'm on a CSI-2 setup.

>
> Thanks
>   j
>
>
> >  };
> >
> >  static const struct reg_value ov5640_setting_low_res[] = {
> > @@ -1810,9 +1810,17 @@ static int ov5640_set_autogain(struct ov5640_dev *sensor, bool on)
> >
> >  static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on)
> >  {
> > -	return ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, on ?
> > -				OV5640_REG_SYS_CTRL0_SW_PWUP :
> > -				OV5640_REG_SYS_CTRL0_SW_PWDN);
> > +	int ret;
> > +
> > +	if (on) {
> > +		ret = ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0,
> > +				       OV5640_REG_SYS_CTRL0_SW_PWUP);
> > +		usleep_range(5000, 6000);
> > +	} else {
> > +		ret = ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0,
> > +				       OV5640_REG_SYS_CTRL0_SW_PWDN);
> > +	}
> > +	return ret;
> >  }
> >
> >  static int ov5640_set_stream_mipi(struct ov5640_dev *sensor, bool on)
> > --
> > 2.17.1
> >
Jai Luthra Jan. 3, 2023, 8:39 a.m. UTC | #4 
Hi Jacopo,

On 02/01/23 18:29, Jacopo Mondi wrote:
> Hello again
> 
> On Thu, Dec 29, 2022 at 07:10:36PM +0100, Jacopo Mondi wrote:
>> Hi again
>>
>> On Tue, Dec 27, 2022 at 11:06:34PM +0530, Jai Luthra wrote:
>>> From: Nishanth Menon <nm@ti.com>
>>>
>>> OV5640 Datasheet[1] Figures 2-3 and 2-4 indicate the timing sequences
>>> that is expected during various initialization steps. Note the power
>>> on time includes t0 + t1 + t2 >= 5ms, delay for poweron.
>>>
>>> As indicated in section 2.8, the PWDN assertion can either be via
>>> external pin control OR via the register 0x3008 bit 6 (see table 7-1 in
>>> [1])
>>>
>>> [1] https://cdn.sparkfun.com/datasheets/Sensors/LightImaging/OV5640_datasheet.pdf
>>>
>>> Fixes: 19a81c1426c1 ("[media] add Omnivision OV5640 sensor driver")
>>> Signed-off-by: Nishanth Menon <nm@ti.com>
>>> Signed-off-by: Jai Luthra <j-luthra@ti.com>
>>> ---
>>>   drivers/media/i2c/ov5640.c | 16 ++++++++++++----
>>>   1 file changed, 12 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
>>> index 5df16fb6f0a0..bd7cc294cfe6 100644
>>> --- a/drivers/media/i2c/ov5640.c
>>> +++ b/drivers/media/i2c/ov5640.c
>>> @@ -609,7 +609,7 @@ static const struct reg_value ov5640_init_setting[] = {
>>>   	{0x583b, 0x28, 0, 0}, {0x583c, 0x42, 0, 0}, {0x583d, 0xce, 0, 0},
>>>   	{0x5025, 0x00, 0, 0}, {0x3a0f, 0x30, 0, 0}, {0x3a10, 0x28, 0, 0},
>>>   	{0x3a1b, 0x30, 0, 0}, {0x3a1e, 0x26, 0, 0}, {0x3a11, 0x60, 0, 0},
>>> -	{0x3a1f, 0x14, 0, 0}, {0x3008, 0x02, 0, 0}, {0x3c00, 0x04, 0, 300},
>>> +	{0x3a1f, 0x14, 0, 0}, {0x3008, 0x02, 0, 5}, {0x3c00, 0x04, 0, 300},
>>
>> This might also allow to remove the
>>
>> 		/* remain in power down mode for DVP */
>> 		if (regs->reg_addr == OV5640_REG_SYS_CTRL0 &&
>> 		    val == OV5640_REG_SYS_CTRL0_SW_PWUP &&
>> 		    !ov5640_is_csi2(sensor))
>> 			continue;
>>
>> in ov5640_load_regs()
>>
>> Prabhakar since you have introduced it, coudl you test if you still
>> need it with Nishanth's patch applied ?
> 
> No, it doesn't, sorry for the confusion.
> 
> The following patch would allow to remove the above quirk by removing
>          {0x3008, 0x02}
> from the init_sequence[].
> 
> Unfortunately the first 3 images are then black when running in CSI-2
> mode.
> 
> -------------------------------------------------------------------------------
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index 96b986051414..bfb60648c72a 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -609,7 +609,7 @@ static const struct reg_value ov5640_init_setting[] = {
>          {0x583b, 0x28, 0, 0}, {0x583c, 0x42, 0, 0}, {0x583d, 0xce, 0, 0},
>          {0x5025, 0x00, 0, 0}, {0x3a0f, 0x30, 0, 0}, {0x3a10, 0x28, 0, 0},
>          {0x3a1b, 0x30, 0, 0}, {0x3a1e, 0x26, 0, 0}, {0x3a11, 0x60, 0, 0},
> -       {0x3a1f, 0x14, 0, 0}, {0x3008, 0x02, 0, 5}, {0x3c00, 0x04, 0, 300},
> +       {0x3a1f, 0x14, 0, 0}, {0x3c00, 0x04, 0, 300},
>   };
> 
>   static const struct reg_value ov5640_setting_low_res[] = {
> @@ -1808,7 +1808,7 @@ static int ov5640_set_autogain(struct ov5640_dev *sensor, bool on)
>                                BIT(1), on ? 0 : BIT(1));
>   }
> 
> -static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on)
> +static int ov5640_set_stream(struct ov5640_dev *sensor, bool on)
>   {
>          int ret;
> 
> @@ -3725,11 +3725,13 @@ static int ov5640_s_stream(struct v4l2_subdev *sd, int enable)
>                          sensor->pending_fmt_change = false;
>                  }
> 
> -               if (ov5640_is_csi2(sensor))
> +               if (ov5640_is_csi2(sensor)) {
>                          ret = ov5640_set_stream_mipi(sensor, enable);
> -               else
> -                       ret = ov5640_set_stream_dvp(sensor, enable);
> +                       if (ret)
> +                               goto out;
> +               }
> 
> +               ret = ov5640_set_stream(sensor, enable);
>                  if (!ret)
>                          sensor->streaming = enable;
>          }
> -------------------------------------------------------------------------------
> 
> I do however now question the patch utility itself. SW PWDN is the
> software standby state, while Figure 2.3 and 2.4 you mention in the
> commit message report:
> 
> t2 = 5 ms: delay from DVDD stable to sensor power up stable
> 
> I doubt this apply to SW standby as it the delay is probably required
> to have the internal circuitry stabilize after the power rail has been
> enabled.

Good catch.

> 
> Does this patch make any practical difference in your setup ? I'm
> asking as in my case it doesn't, but I'm on a CSI-2 setup.

I tested today with this patch removed, and it does not make any 
difference to the probe success rate. We missed this during development 
as we first tried the SW powerdown and reset timing changes, and later 
the hardware reset timing changes [1/3].

I will drop this patch in the next series. Thanks for pointing this out.

> 
>>
>> Thanks
>>    j
>>
>>
>>>   };
>>>
>>>   static const struct reg_value ov5640_setting_low_res[] = {
>>> @@ -1810,9 +1810,17 @@ static int ov5640_set_autogain(struct ov5640_dev *sensor, bool on)
>>>
>>>   static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on)
>>>   {
>>> -	return ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, on ?
>>> -				OV5640_REG_SYS_CTRL0_SW_PWUP :
>>> -				OV5640_REG_SYS_CTRL0_SW_PWDN);
>>> +	int ret;
>>> +
>>> +	if (on) {
>>> +		ret = ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0,
>>> +				       OV5640_REG_SYS_CTRL0_SW_PWUP);
>>> +		usleep_range(5000, 6000);
>>> +	} else {
>>> +		ret = ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0,
>>> +				       OV5640_REG_SYS_CTRL0_SW_PWDN);
>>> +	}
>>> +	return ret;
>>>   }
>>>
>>>   static int ov5640_set_stream_mipi(struct ov5640_dev *sensor, bool on)
>>> --
>>> 2.17.1
>>>

Thanks,
Jai