Re: [PATCH v2] media: ov5675: Fix power on/off delay timings

[Dave Stevenson]

Hi Bryan

On Thu, 11 Jul 2024 at 15:38, Bryan O'Donoghue
<bryan.odonoghue@xxxxxxxxxx> wrote:
>
> The ov5675 specification says that the gap between XSHUTDN deassert and the
> first I2C transaction should be a minimum of 8192 XVCLK cycles.
>
> Right now we use a usleep_rage() that gives a sleep time of between about
> 430 and 860 microseconds.
>
> On the Lenovo X13s we have observed that in about 1/20 cases the current
> timing is too tight and we start transacting before the ov5675's reset
> cycle completes, leading to I2C bus transaction failures.
>
> The reset racing is sometimes triggered at initial chip probe but, more
> usually on a subsequent power-off/power-on cycle e.g.
>
> [   71.451662] ov5675 24-0010: failed to write reg 0x0103. error = -5
> [   71.451686] ov5675 24-0010: failed to set plls
>
> The current quiescence period we have is too tight. Instead of expressing
> the post reset delay in terms of the current XVCLK this patch converts the
> power-on and power-off delays to the maximum theoretical delay @ 6 MHz with
> an additional buffer.
>
> 1.365 milliseconds on the power-on path is 1.5 milliseconds with grace.
> 853 microseconds on the power-off path is 900 microseconds with grace.

I think you've got the decimal point in the wrong place for power off.

The comment you've removed in the power off path says
/* 512 xvclk cycles after the last SCCB transation or MIPI frame end */

512 clocks at 6MHz I make 85.3usecs.

I'm happy to be corrected if I've blundered on my maths though.

  Dave

>
> Fixes: 49d9ad719e89 ("media: ov5675: add device-tree support and support runtime PM")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@xxxxxxxxxx>
> ---
> v2:
> - Drop patch to read and act on reported XVCLK
> - Use worst-case timings + a reasonable grace period in-lieu of previous
>   xvclk calculations on power-on and power-off.
> - Link to v1: https://lore.kernel.org/r/20240711-linux-next-ov5675-v1-0-69e9b6c62c16@xxxxxxxxxx
>
> v1:
> One long running saga for me on the Lenovo X13s is the occasional failure
> to either probe or subsequently bring-up the ov5675 main RGB sensor on the
> laptop.
>
> Initially I suspected the PMIC for this part as the PMIC is using a new
> interface on an I2C bus instead of an SPMI bus. In particular I thought
> perhaps the I2C write to PMIC had completed but the regulator output hadn't
> become stable from the perspective of the SoC. This however doesn't appear
> to be the case - I can introduce a delay of milliseconds on the PMIC path
> without resolving the sensor reset problem.
>
> Secondly I thought about reset pin polarity or drive-strength but, again
> playing about with both didn't yield decent results.
>
> I also played with the duration of reset to no avail.
>
> The error manifested as an I2C write timeout to the sensor which indicated
> that the chip likely hadn't come out reset. An intermittent fault appearing
> in perhaps 1/10 or 1/20 reset cycles.
>
> Looking at the expression of the reset we see that there is a minimum time
> expressed in XVCLK cycles between reset completion and first I2C
> transaction to the sensor. The specification calls out the minimum delay @
> 8192 XVCLK cycles and the ov5675 driver meets that timing almost exactly.
>
> A little too exactly - testing finally showed that we were too racy with
> respect to the minimum quiescence between reset completion and first
> command to the chip.
>
> Fixing this error I choose to base the fix again on the number of clocks
> but to also support any clock rate the chip could support by moving away
> from a define to reading and using the XVCLK.
>
> True enough only 19.2 MHz is currently supported but for the hypothetical
> case where some other frequency is supported in the future, I wanted the
> fix introduced in this series to still hold.
>
> Hence this series:
>
> 1. Allows for any clock rate to be used in the valid range for the reset.
> 2. Elongates the post-reset period based on clock cycles which can now
> vary.
>
> Patch #2 can still be backported to stable irrespective of patch #1.
> ---
>  drivers/media/i2c/ov5675.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/media/i2c/ov5675.c b/drivers/media/i2c/ov5675.c
> index 3641911bc73f..547d6fab816a 100644
> --- a/drivers/media/i2c/ov5675.c
> +++ b/drivers/media/i2c/ov5675.c
> @@ -972,12 +972,10 @@ static int ov5675_set_stream(struct v4l2_subdev *sd, int enable)
>
>  static int ov5675_power_off(struct device *dev)
>  {
> -       /* 512 xvclk cycles after the last SCCB transation or MIPI frame end */
> -       u32 delay_us = DIV_ROUND_UP(512, OV5675_XVCLK_19_2 / 1000 / 1000);
>         struct v4l2_subdev *sd = dev_get_drvdata(dev);
>         struct ov5675 *ov5675 = to_ov5675(sd);
>
> -       usleep_range(delay_us, delay_us * 2);
> +       usleep_range(900, 1000);
>
>         clk_disable_unprepare(ov5675->xvclk);
>         gpiod_set_value_cansleep(ov5675->reset_gpio, 1);
> @@ -988,7 +986,6 @@ static int ov5675_power_off(struct device *dev)
>
>  static int ov5675_power_on(struct device *dev)
>  {
> -       u32 delay_us = DIV_ROUND_UP(8192, OV5675_XVCLK_19_2 / 1000 / 1000);
>         struct v4l2_subdev *sd = dev_get_drvdata(dev);
>         struct ov5675 *ov5675 = to_ov5675(sd);
>         int ret;
> @@ -1014,8 +1011,11 @@ static int ov5675_power_on(struct device *dev)
>
>         gpiod_set_value_cansleep(ov5675->reset_gpio, 0);
>
> -       /* 8192 xvclk cycles prior to the first SCCB transation */
> -       usleep_range(delay_us, delay_us * 2);
> +       /* Worst case quiesence gap is 1.365 milliseconds @ 6MHz XVCLK
> +        * Add an additional threshold grace period to ensure reset
> +        * completion before initiating our first I2C transaction.
> +        */
> +       usleep_range(1500, 1600);
>
>         return 0;
>  }
>
> ---
> base-commit: 523b23f0bee3014a7a752c9bb9f5c54f0eddae88
> change-id: 20240710-linux-next-ov5675-60b0e83c73f1
>
> Best regards,
> --
> Bryan O'Donoghue <bryan.odonoghue@xxxxxxxxxx>
>
>