Re: [PATCH 2/2] media: ov5675: Elongate reset to first transaction minimum gap

[Bryan O'Donoghue <bryan.odonoghue@linaro.org>]

On 11/07/2024 12:17, Dave Stevenson wrote:
> Hi Quentin and Bryan
> 
> On Thu, 11 Jul 2024 at 11:40, Quentin Schulz <quentin.schulz@cherry.de> 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@vger.kernel.org
>>> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.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.
>>
>> 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?
>>
>> With this patch we essentially postpone the power_on by another 430ms
>> making it almost a full second before we can start using the camera.
>> That's quite a lot I think? We don't have a usecase right now that
>> requires this to be blazing fast (and we anyway would need at the very
>> least 430ms), so take this remark as what it is, a remark.
> 
> I think you've misread 430 usec as 430 msec.
> 
> I was looking at the series and trying to decide whether it's worth
> going to the effort of computing the time at all when even on the
> slowest 6MHz XVCLK we're sub 1.5ms for the required delay.
> At the max XVLCK of 24MHz you could save 1ms. I know of very few use
> cases that would suffer for a 1ms delay.
> 
> I know we all like to be precise, but it sounds like the precision
> actually causes grief in this situation.

Yeah the first draft of the patch just had a post-reset delay of I 
forget - I think I just used usleep_range(2000, 2200); again but I kind 
respected the attempt to hit the specification and wanted to fix the 
original logic, which is close but no cigar ATM.

---
bod