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@xxxxxxxxxxxxxxx
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@xxxxxxxxxx>
---
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
