Message-ID: <3646cd1f-09f1-4e80-8d55-a9ac25cbf81d@cherry.de>
Date: Thu, 11 Jul 2024 14:22:02 +0200
From: Quentin Schulz <quentin.schulz@...rry.de>
To: Bryan O'Donoghue <bryan.odonoghue@...aro.org>,
 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

Hi Bryan,

On 7/11/24 2:07 PM, Bryan O'Donoghue wrote:
> 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.
> 

I think we could, checking the delay between the moment the GPIO reaches 
logical low and the moment we send the first I2C command and comparing 
this against 8192 * 1/19.2MHz. Not sure we need to spend the time on 
this though? There isn't really a strong need for optimizing this as 
much as we can I believe? (and worst case scenario, we can do it later on).

>> 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.
> 

Works for me.

Cheers,
Quentin

Powered by blists - more mailing lists