Message-ID: <078e3274-f592-4ce0-a938-d58a0f88cf84@linaro.org>
Date: Thu, 11 Jul 2024 13:07:53 +0100
From: Bryan O'Donoghue <bryan.odonoghue@...aro.org>
To: Quentin Schulz <quentin.schulz@...rry.de>,
 Sakari Ailus <sakari.ailus@...ux.intel.com>,
 Mauro Carvalho Chehab <mchehab@...nel.org>,
 Quentin Schulz <quentin.schulz@...obroma-systems.com>,
 Jacopo Mondi <jacopo@...ndi.org>
Cc: Johan Hovold <johan@...nel.org>,
 Kieran Bingham <kieran.bingham@...asonboard.com>,
 linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
 stable@...r.kernel.org, dave.stevenson@...pberrypi.com
Subject: Re: [PATCH 2/2] media: ov5675: Elongate reset to first transaction
 minimum gap

On 11/07/2024 12:41, Quentin Schulz wrote:
> Hi Bryan and Dave,
> 
> On 7/11/24 1:22 PM, Bryan O'Donoghue wrote:
>> On 11/07/2024 11:40, Quentin Schulz wrote:
>>> Hi Bryan,
>>>
>>> On 7/11/24 12:20 PM, Bryan O'Donoghue 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, doubling the 
>>>> minimum
>>>> appears to fix the issue observed on X13s.
>>>>
>>>> Fixes: 49d9ad719e89 ("media: ov5675: add device-tree support and 
>>>> support runtime PM")
>>>> Cc: stable@...r.kernel.org
>>>> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@...aro.org>
>>>> ---
>>>>   drivers/media/i2c/ov5675.c | 9 +++++++--
>>>>   1 file changed, 7 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/media/i2c/ov5675.c b/drivers/media/i2c/ov5675.c
>>>> index 92bd35133a5d..0498f8f3064d 100644
>>>> --- a/drivers/media/i2c/ov5675.c
>>>> +++ b/drivers/media/i2c/ov5675.c
>>>> @@ -1018,8 +1018,13 @@ 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);
>>>> +    /* The spec calls for a minimum delay of 8192 XVCLK cycles 
>>>> prior to
>>>> +     * transacting on the I2C bus, which translates to about 430
>>>> +     * microseconds at 19.2 MHz.
>>>> +     * Testing shows the range 8192 - 16384 cycles to be unreliable.
>>>> +     * Grant a more liberal 2x -3x clock cycle grace time.
>>>> +     */
>>>> +    usleep_range(delay_us * 2, delay_us * 3);
>>>
>>> Would it make sense to have power_off have the same logic? We do a 
>>> usleep_range of those same values currently, so keeping them in sync 
>>> seems to make sense to me.
>>
>> I have no evidence to suggest there's a problem on the shutdown path, 
>> that's why I left the quiescence period as-is there.
>>
>>> Also, I'm wondering if it isn't an issue with the gpio not being high 
>>> right after gpoiod_set_value_cansleep() returns, i.e. the time it 
>>> actually takes for the HW to reach the IO level that means "high" for 
>>> the camera. And that this increased sleep is just a way to mitigate 
>>> that?
>>
>> No, that's not what I found.
>>
>> I tried changing
>>
>>          usleep_range(2000, 2200);
>>
>> to
>>          usleep_range(200000, 220000);
>>
>> but could still elicit the I2C transaction failure. If the time it 
>> took for the GPIO to hit logical 1 were the issue then multiplying the 
>> reset time by 100 would certainly account for that.
>>
>> // BOD set the chip into reset
>>          gpiod_set_value_cansleep(ov5675->reset_gpio, 1);
>>
>> // BOD apply power
>>          ret = regulator_bulk_enable(OV5675_NUM_SUPPLIES, 
>> ov5675->supplies);
>>          if (ret) {
>>                  clk_disable_unprepare(ov5675->xvclk);
>>                  return ret;
>>          }
>>
>>          /* Reset pulse should be at least 2ms and reset gpio released 
>> only once
>>           * regulators are stable.
>>           */
>>
>> // BOD spec specifies 2 milliseconds here not a count of XVCLKs
>>          usleep_range(2000, 2200);
>>
>>          gpiod_set_value_cansleep(ov5675->reset_gpio, 0);
>>
> 
> I meant to say this gpiod_set_value_cansleep(), which is logical LOW and 
> not HIGH, brain not braining today sorry. But the question remains the 
> same.

Ah right yes I get you, you mean how can I prove the sensor has come out 
of reset by the time we start counting for the first I2C transaction delay ?

There's no way to prove that, the only thing we can do is elongate the 
post reset delay by X, whatever X we choose.

> So, maybe this is all too complex for something that could be as simple 
> as 8192 XVCLK cycles for 6MHz as Dave suggested I believe. And have some 
> wiggle room added in case we ever support 6MHz and it has the same issue 
> as you encountered with 19.2MHz (or whatever was that rate you were 
> running the camera at). 1/6MHz * 8192 * 2 ~= 2.7ms if I'm not mistaken. 
> So maybe go with that with a comment just above to justify why we are 
> doing this with hardcoded values?

2.7 milliseconds is alot.

Worst case XVCLK period is 1.365 milliseconds.

If your theory on the GPIO is correct, its still difficult to see how @ 
6MHz - which we don't yet support and probably never will, that 1.5 
milliseconds would be insufficient.

So - I'm happy enough to throw out the first patch and give a range of 
1.5 to 1.6 milliseconds instead.

---
bod

Powered by blists - more mailing lists