[PATCH] i2c: tegra: Add delay before reset the controller

[PATCH] i2c: tegra: Add delay before reset the controller

[Alok Chauhan]

In NACK error condition, I2C controller violates
clock-to-data setup time before stop. In Software,
because of this reset of controller is happening
before I2C controller could complete STOP condition.

Added delay of 2 clock period before reset the
controller in case of NACK error.

Signed-off-by: Alok Chauhan <alokc@nvidia.com>
---
delay will be calculated based on clock frequency of the bus.
 drivers/i2c/busses/i2c-tegra.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index e978635..344282e 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -516,6 +516,14 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
 	if (likely(i2c_dev->msg_err == I2C_ERR_NONE))
 		return 0;
 
+	/*
+	 * In NACK error condition resetting of I2C controller happens
+	 * before STOP condition is properly completed by I2C controller,
+	 * so wait for 2 clock cycle to complete STOP condition.
+	 */
+	if (i2c_dev->msg_err == I2C_ERR_NO_ACK)
+		udelay(DIV_ROUND_UP(2 * 1000000, i2c_dev->bus_clk_rate));
+
 	tegra_i2c_init(i2c_dev);
 	if (i2c_dev->msg_err == I2C_ERR_NO_ACK) {
 		if (msg->flags & I2C_M_IGNORE_NAK)
-- 
1.7.4.1

Re: [PATCH] i2c: tegra: Add delay before reset the controller

[Stephen Warren]

On 03/30/2012 06:06 AM, Alok Chauhan wrote:
> In NACK error condition, I2C controller violates
> clock-to-data setup time before stop. In Software,
> because of this reset of controller is happening
> before I2C controller could complete STOP condition.

I'm not quite sure this description is correct. Surely what's happening
is simply that the controller raises the NACK interrupt as soon as it
detects a NACK condition. Then, the controller generates the STOP
condition on the bus correctly. The issue is that if software resets the
controller as soon as the NACK interrupt is raised, this will happen
before the controller has completely delivered the STOP condition onto
the bus, so we need to pause and wait for that?

I think the commit description and comment in the code should be
reworded to describe what I wrote above, assuming it's accurate.

Is there really no idle bit we can poll in the I2C controller to
determine when the STOP has completed, rather than just delaying a
"hard-coded" time? Can I2C SCL clock-stretching affect the amount of
time that must be delayed?

[PATCH] i2c: tegra: Add delay before reset the controller

[Alok Chauhan]

From: Alok Chauhan <alokc@nvidia.com>

In NACK error condition, I2C controller violates
clock-to-data setup time before stop. In Software,
because of this reset of controller is happening
before I2C controller could complete STOP condition.

Added worst case delay of 1 ms (assuming lowest
clock frequency will be 1 KHZ) before reset the
controller in case of NACK error.

Signed-off-by: Alok Chauhan <alokc@nvidia.com>
---
 drivers/i2c/busses/i2c-tegra.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 6381604..b731499 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -517,6 +517,15 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
 	if (likely(i2c_dev->msg_err == I2C_ERR_NONE))
 		return 0;
 
+	/*
+	 * In NACK error condition resetting of I2C controller happens
+	 * before STOP condition is properly completed by I2C controller,
+	 * so wait for worst case delay of 1 ms (assuming lowest clock
+	 * frequency will be 1 KHz) to complete STOP condition.
+	 */
+	if (i2c_dev->msg_err == I2C_ERR_NO_ACK)
+		mdelay(1);
+
 	tegra_i2c_init(i2c_dev);
 	if (i2c_dev->msg_err == I2C_ERR_NO_ACK) {
 		if (msg->flags & I2C_M_IGNORE_NAK)
-- 
1.7.4.1

Re: [PATCH] i2c: tegra: Add delay before reset the controller

[Olof Johansson]

Hi,

(Adding linux-tegra and Dylan Reid who was debugging this before)

On Thu, Dec 22, 2011 at 2:41 AM, Alok Chauhan <alokc@nvidia.com> wrote:
> From: Alok Chauhan <alokc@nvidia.com>
>
> In NACK error condition, I2C controller violates
> clock-to-data setup time before stop. In Software,
> because of this reset of controller is happening
> before I2C controller could complete STOP condition.
>
> Added worst case delay of 1 ms (assuming lowest
> clock frequency will be 1 KHZ) before reset the
> controller in case of NACK error.

This change causes problems systems with some models of i2c TPMs, since
the first transaction to them will always time out (TPM quirk), and the delay
means that the tpm will have time to go back to sleep and thus timeout
even on the retry. So you'll never make progress.

In other words: this patch will break some systems and thus shouldn't be
applied.


-Olof

RE: [PATCH] i2c: tegra: Add delay before reset the controller

[Alok Chauhan]

Hi,

(Adding Laxman Dewangan)

>This change causes problems systems with some models of i2c TPMs, since the first transaction to them will always time out (TPM quirk), and the delay means that the tpm will have time to go back to sleep and thus timeout >even on the retry. So you'll never make progress.
Working on this.

-Alok


-----Original Message-----
From: Olof Johansson [mailto:olof@lixom.net] 
Sent: Friday, December 23, 2011 2:48 AM
To: Alok Chauhan
Cc: khali@linux-fr.org; ben-linux@fluff.org; Stephen Warren; bones@secretlab.ca; paul.gortmaker@windriver.com; linux-i2c@vger.kernel.org; linux-kernel@vger.kernel.org; linux-tegra@vger.kernel.org; dgreid@google.com
Subject: Re: [PATCH] i2c: tegra: Add delay before reset the controller

Hi,

(Adding linux-tegra and Dylan Reid who was debugging this before)

On Thu, Dec 22, 2011 at 2:41 AM, Alok Chauhan <alokc@nvidia.com> wrote:
> From: Alok Chauhan <alokc@nvidia.com>
>
> In NACK error condition, I2C controller violates clock-to-data setup 
> time before stop. In Software, because of this reset of controller is 
> happening before I2C controller could complete STOP condition.
>
> Added worst case delay of 1 ms (assuming lowest clock frequency will 
> be 1 KHZ) before reset the controller in case of NACK error.

This change causes problems systems with some models of i2c TPMs, since the first transaction to them will always time out (TPM quirk), and the delay means that the tpm will have time to go back to sleep and thus timeout even on the retry. So you'll never make progress.

In other words: this patch will break some systems and thus shouldn't be applied.


-Olof

RE: [PATCH] i2c: tegra: Add delay before reset the controller

[Laxman Dewangan]

> -----Original Message-----
> From: Alok Chauhan
> 
> Hi,
> 
> (Adding Laxman Dewangan)
> 
> >This change causes problems systems with some models of i2c TPMs, since the
> first transaction to them will always time out (TPM quirk), and the delay
> means that the tpm will have time to go back to sleep and thus timeout >even
> on the retry. So you'll never make progress.
> Working on this.
> 
> -Alok
> 
> 
> -----Original Message-----
> From: Olof Johansson [mailto:olof@lixom.net]
> Sent: Friday, December 23, 2011 2:48 AM
> To: Alok Chauhan
> Cc: khali@linux-fr.org; ben-linux@fluff.org; Stephen Warren;
> bones@secretlab.ca; paul.gortmaker@windriver.com; linux-i2c@vger.kernel.org;
> linux-kernel@vger.kernel.org; linux-tegra@vger.kernel.org; dgreid@google.com
> Subject: Re: [PATCH] i2c: tegra: Add delay before reset the controller
> 
> Hi,
> 
> (Adding linux-tegra and Dylan Reid who was debugging this before)
> 
> On Thu, Dec 22, 2011 at 2:41 AM, Alok Chauhan <alokc@nvidia.com> wrote:
> > From: Alok Chauhan <alokc@nvidia.com>
> >
> > In NACK error condition, I2C controller violates clock-to-data setup
> > time before stop. In Software, because of this reset of controller is
> > happening before I2C controller could complete STOP condition.
> >
> > Added worst case delay of 1 ms (assuming lowest clock frequency will
> > be 1 KHZ) before reset the controller in case of NACK error.
> 
> This change causes problems systems with some models of i2c TPMs, since the
> first transaction to them will always time out (TPM quirk), and the delay
> means that the tpm will have time to go back to sleep and thus timeout even on
> the retry. So you'll never make progress.
> 
> In other words: this patch will break some systems and thus shouldn't be
> applied.
> 
I think the  delay should be calculated based on speed. So if it is 100KHz, then
delay is 10mcro second for 1 bit. We should wait for 2 bit (ack clock to be complete and
stop pulse to be transmitted).
This patch will make sure that i2c driver will not break the specs i.e. in case of NACK error, the stop
signal should be completed by i2c controller before resetting controller.
I will prefer of having the udelay(2*1000000/speed).

> 
> -Olof