Message ID | 20240711-linux-next-ov5675-v1-2-69e9b6c62c16@linaro.org (mailing list archive) |
---|---|
State | New |
Delegated to: | Sakari Ailus |
Headers |
Received: from sy.mirrors.kernel.org ([147.75.48.161]) by linuxtv.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.96) (envelope-from <linux-media+bounces-14905-patchwork=linuxtv.org@vger.kernel.org>) id 1sRqur-00040n-0j for patchwork@linuxtv.org; Thu, 11 Jul 2024 10:20:52 +0000 Received: from smtp.subspace.kernel.org (wormhole.subspace.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by sy.mirrors.kernel.org (Postfix) with ESMTPS id A8EF1B21206 for <patchwork@linuxtv.org>; Thu, 11 Jul 2024 10:20:41 +0000 (UTC) Received: from localhost.localdomain (localhost.localdomain [127.0.0.1]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 99DFF159217; Thu, 11 Jul 2024 10:20:07 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b="AeDeOfgq" X-Original-To: linux-media@vger.kernel.org Received: from mail-wm1-f52.google.com (mail-wm1-f52.google.com [209.85.128.52]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id F31A115887C for <linux-media@vger.kernel.org>; Thu, 11 Jul 2024 10:20:04 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.128.52 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1720693207; cv=none; b=RE+kvVLasN5FAmbu6ilvz0fJXdjKP8qmglhamSjWmZPoUKbgFWdNVuVUtVi0GF+1Gs1Hx2WD8aaE5EhFTJwfYj6GKyF79rJmgdxr2r9+4LjuWnqcfaApqcLgyvJPG6xHMmizl/13xZ5UBrmgH9+nX3hsXl1HIjJub89E3LXPhzI= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1720693207; c=relaxed/simple; bh=bKIdUuVmWv5FJkFDtu2H6clQsAHCUTFbn8+qyaw6hHg=; h=From:Date:Subject:MIME-Version:Content-Type:Message-Id:References: In-Reply-To:To:Cc; b=N6NU3XawD3fLsPPC+BiZGsqklsXtYYSyYgTlh/EQA0nWERhK/8i/cIGTwx6D1rVRfkcNNbeR4ITp+KvdnwbV8Mgf6KBKEmaJPB0hSjBHsFftIo17DTX1dM8AKiu0qG1DhcIzwb+/Zb5QdW8UR3JQv217jU9n1SZ01WeHsX+nXe0= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linaro.org; spf=pass smtp.mailfrom=linaro.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b=AeDeOfgq; arc=none smtp.client-ip=209.85.128.52 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linaro.org Received: by mail-wm1-f52.google.com with SMTP id 5b1f17b1804b1-4279c924ca7so35395e9.2 for <linux-media@vger.kernel.org>; Thu, 11 Jul 2024 03:20:04 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1720693203; x=1721298003; darn=vger.kernel.org; h=cc:to:in-reply-to:references:message-id:content-transfer-encoding :mime-version:subject:date:from:from:to:cc:subject:date:message-id :reply-to; bh=Jw6bLyA4dVnYszcIqA96Q05ikE6tm67ysRPS44gZ2Bo=; b=AeDeOfgqmtP6YqYtgTpYNinb9oqMQl3i7j38vK4+io3o2BdQ00xqcjagYE4jzCULnB E4zK8QAZ7vgI18tg4Wl9bxBcVnzYW/fUdzoUskWQc+r5st4WQBPQ18QdyrnbNCK/CkK9 9by3L7M9izvLBeYybfLp7EGrHzIkly9reeppj256TLUcJTas4KaaYGDeL6blF2Xj67po 7JmFAm3A3hjbGCAd7Q45WmwhiU9R7cNAbVHuzPXOMnlorAPMeRcyY1KCUJmq8r2iA7vm Jmdj+/wNpcM5s0aaCWbWTgk2OVrKksnQfoAsrNXjLVa1Ng0pN2u+TS64yQ0ie+9HuytJ lUbg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1720693203; x=1721298003; h=cc:to:in-reply-to:references:message-id:content-transfer-encoding :mime-version:subject:date:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=Jw6bLyA4dVnYszcIqA96Q05ikE6tm67ysRPS44gZ2Bo=; b=Jw6ev/WrFlZjf73GPlzCavoVlW8KlbV3iEvAZVXT3RT7hO1Fp1LtfRjRT/jdGC+ia9 gS/STO3VMq0Et2caiGpB9QiTij7zaVI7QZaL7ozO2A57ldqnYfiIEHPO1d02RGnOBT/H eqXe6Zlg/tF4+1A8WWkzqA+BFmR2gu0FrK/sR1cmXDiZZV9/uDNZvNrl4fzwZsEqBR8j kGzj+du3tIVbMv4oOY4pSCpSml1WSqKYWOa8CczvhLYsg2sVysfst/Fe9hwRK8xi5dnV StBsLNES2stGdfJv3XjsS9ytzNuRZ4jjaDkM62eGlVgGoV1P6YhCC4CRd8TtSS7tEhT2 orwQ== X-Forwarded-Encrypted: i=1; AJvYcCUF2z9XLRwSMXYyurjvy9j/pAoBX/nm0JF/0A/IHg8etp8i4+kG3q7jzfr+Gy/KEqLlq68b081WEkBQ9x1CcVBtR0w3kDQm+vepoS4= X-Gm-Message-State: AOJu0YxRI0mcQc0G4WsOOj3gBcWG6qN73rwt+vDGgBx1URePWSrzEqKH GzgUltx4uGkabjd5Z4wLHT3b0nYRibSbjY6DCsrUDF9GguI/c3Jj2NT5GBOxAj4= X-Google-Smtp-Source: AGHT+IEPG6ALVrKC0vxITNNnj0AxUU2b5IhohOK+zwxcy7Y2DoKsbVYQ4rwu1v3iMbA9+oXaQb0Gcw== X-Received: by 2002:a05:600c:228f:b0:426:668f:5eca with SMTP id 5b1f17b1804b1-426706c6878mr52146665e9.7.1720693203327; Thu, 11 Jul 2024 03:20:03 -0700 (PDT) Received: from [127.0.1.1] ([176.61.106.227]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-4266e861339sm125270025e9.12.2024.07.11.03.20.02 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 11 Jul 2024 03:20:02 -0700 (PDT) From: Bryan O'Donoghue <bryan.odonoghue@linaro.org> Date: Thu, 11 Jul 2024 11:20:02 +0100 Subject: [PATCH 2/2] media: ov5675: Elongate reset to first transaction minimum gap Precedence: bulk X-Mailing-List: linux-media@vger.kernel.org List-Id: <linux-media.vger.kernel.org> List-Subscribe: <mailto:linux-media+subscribe@vger.kernel.org> List-Unsubscribe: <mailto:linux-media+unsubscribe@vger.kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Message-Id: <20240711-linux-next-ov5675-v1-2-69e9b6c62c16@linaro.org> References: <20240711-linux-next-ov5675-v1-0-69e9b6c62c16@linaro.org> In-Reply-To: <20240711-linux-next-ov5675-v1-0-69e9b6c62c16@linaro.org> To: Sakari Ailus <sakari.ailus@linux.intel.com>, Mauro Carvalho Chehab <mchehab@kernel.org>, Quentin Schulz <quentin.schulz@theobroma-systems.com>, Jacopo Mondi <jacopo@jmondi.org> Cc: Johan Hovold <johan@kernel.org>, Kieran Bingham <kieran.bingham@ideasonboard.com>, linux-media@vger.kernel.org, linux-kernel@vger.kernel.org, Bryan O'Donoghue <bryan.odonoghue@linaro.org>, stable@vger.kernel.org X-Mailer: b4 0.15-dev-13183 X-LSpam-Score: -2.6 (--) X-LSpam-Report: No, score=-2.6 required=5.0 tests=ARC_SIGNED=0.001,ARC_VALID=-0.1,BAYES_00=-1.9,DKIM_SIGNED=0.1,DKIM_VALID=-0.1,DKIM_VALID_AU=-0.1,DMARC_PASS=-0.001,HEADER_FROM_DIFFERENT_DOMAINS=0.5,MAILING_LIST_MULTI=-1,SPF_HELO_NONE=0.001,SPF_PASS=-0.001 autolearn=unavailable autolearn_force=no |
Series |
media: ov5675: Fixup ov5675 reset failures
|
|
Commit Message
Bryan O'Donoghue
July 11, 2024, 10:20 a.m. UTC
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(-)
Comments
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. Dave > > The change looks fine to me even though it feels like a band-aid patch. > > Cheers, > Quentin >
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@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. 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); // BOD spec calls for a _minimum_ of 8192 XVCLK cycles before I2C /* 8192 xvclk cycles prior to the first SCCB transation */ usleep_range(delay_us, delay_us * 2); The issue is initiating an I2C transaction too early _after_ reset completion not the duration of that reset. As I stated in the cover letter, I tried a longer reset duration, a higher drive-strength on the GPIO as well as I didn't put in my cover letter, inverting the logic of the GPIO reset, which unsurprisingly didn't work. No matter how long we hold the chip in reset, unless we give more grace time _subsequent_ to the reset before initiating an I2C transaction, we will encounter transaction failures. This is a fairly common and logical fault if you think about it. XVCLK is providing a clock to the ov5675 core to "do stuff" whatever that stuff is. Bring up an internal firmware, lock a fundamental PLL - whatever. If we start an I2C transaction before the hypothetical internal core has booted up then - meh no bueno we'll get no transaction response. That's the error - speaking too soon. A little like myself in the mornings, cranky before I've had my coffee and unresponsive. ;) > 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. Not a full second, a millisecond. 8/10ths of 1 millisecond instead of 4/10ths of one millisecond. 19.2MHz is 52.083333333333 nanoseconds per clock 52.083333333333 * 8192 => 426666 nanoseconds => 0.426666 milliseconds or 426.6 microseconds So our post reset quiesence minimum @ 19.2MHz moves from 426.6 microseconds to 853. > The change looks fine to me even though it feels like a band-aid patch. I mean it's not a second - if you feel very strongly that 426 milliseconds * 2 is wrong, I guess I could add some more complex logic however I like this simple fix for backporting. --- bod
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
On 11/07/2024 12:22, Bryan O'Donoghue wrote:
> if you feel very strongly that 426 milliseconds * 2 is wrong
MICROSECONDS !
Bah
---
bod
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@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. > > 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. > // BOD spec calls for a _minimum_ of 8192 XVCLK cycles before I2C > /* 8192 xvclk cycles prior to the first SCCB transation */ > usleep_range(delay_us, delay_us * 2); > > The issue is initiating an I2C transaction too early _after_ reset > completion not the duration of that reset. > > As I stated in the cover letter, I tried a longer reset duration, a > higher drive-strength on the GPIO as well as I didn't put in my cover > letter, inverting the logic of the GPIO reset, which unsurprisingly > didn't work. > > No matter how long we hold the chip in reset, unless we give more grace > time _subsequent_ to the reset before initiating an I2C transaction, we > will encounter transaction failures. > > This is a fairly common and logical fault if you think about it. > > XVCLK is providing a clock to the ov5675 core to "do stuff" whatever > that stuff is. Bring up an internal firmware, lock a fundamental PLL - > whatever. > > If we start an I2C transaction before the hypothetical internal core has > booted up then - meh no bueno we'll get no transaction response. > > That's the error - speaking too soon. > Yeah, that's what I meant sorry. > A little like myself in the mornings, cranky before I've had my coffee > and unresponsive. > > ;) > >> 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. > > Not a full second, a millisecond. > > 8/10ths of 1 millisecond instead of 4/10ths of one millisecond. > > 19.2MHz is 52.083333333333 nanoseconds per clock > > 52.083333333333 * 8192 => 426666 nanoseconds => 0.426666 milliseconds or > 426.6 microseconds > > So our post reset quiesence minimum @ 19.2MHz moves from 426.6 > microseconds to 853. > >> The change looks fine to me even though it feels like a band-aid patch. > > I mean it's not a second - if you feel very strongly that 426 > milliseconds * 2 is wrong, I guess I could add some more complex logic > however I like this simple fix for backporting. > Somehow I read microsecond, translated this to ms and then translated it again to millisecond. Don't ask :) 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? But ok, almost meaningless time increase in the grand scheme of things, so fine with me :) Cheers, Quentin
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@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. >> >> 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
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@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. >>> >>> 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
On 11/07/2024 13:22, Quentin Schulz wrote: > Hi Bryan, > > On 7/11/24 2:07 PM, Bryan O'Donoghue wrote: >> On 11/07/2024 12:41, Quentin Schulz wrote: >> 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. Great. Just for record, I'll update power_off() too to match the logic we are applying @ power_on since we've decided the calculation based on XVCLK is overkill. --- bod
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); return 0; }