[4/4] I2C: OMAP3: PM: (re)init for every transfer to support off-mode
diff mbox

Message ID F12CE1A68F023D498A2691C7B5393115035F09BD@ZMY16EXM66.ds.mot.com
State Superseded
Delegated to: Kevin Hilman
Headers show

Commit Message

HU TAO-TGHK48 June 29, 2009, 8:51 a.m. UTC
I think we also need extra patch for fixing stability issue.

Sometimes after back from retention/off mode, OMAP3430 I2C controller
stops working even all register settings are restored.

OMAP3430 TRM says: "During active mode (I2Ci.I2C_CON[15] I2C_EN bit is
set to 1), make no changes to the I2Ci.I2C_SCLL and I2Ci.I2C_SCLH
registers. 
Changes may result in unpredictable behavior."


 static inline void omap_i2c_write_reg(struct omap_i2c_dev *i2c_dev, @@
-230,9 +236,16 @@ static void omap_i2c_unidle(struct omap_i2c_dev *dev)
 
 	clk_enable(dev->iclk);
 	clk_enable(dev->fclk);
+	if (cpu_is_omap34xx()) {
+		omap_i2c_write_reg(dev, OMAP_I2C_PSC_REG,
dev->pscstate);
+		omap_i2c_write_reg(dev, OMAP_I2C_SCLL_REG,
dev->scllstate);
+		omap_i2c_write_reg(dev, OMAP_I2C_SCLH_REG,
dev->sclhstate);
+		omap_i2c_write_reg(dev, OMAP_I2C_BUF_REG,
dev->bufstate);
+		omap_i2c_write_reg(dev, OMAP_I2C_SYSC_REG,
dev->syscstate);
+		omap_i2c_write_reg(dev, OMAP_I2C_WE_REG, dev->westate);
+	}
 	dev->idle = 0;
-	if (dev->iestate)
-		omap_i2c_write_reg(dev, OMAP_I2C_IE_REG, dev->iestate);
+	omap_i2c_write_reg(dev, OMAP_I2C_IE_REG, dev->iestate);
 }
 
 static void omap_i2c_idle(struct omap_i2c_dev *dev) @@ -258,7 +271,7 @@
static void omap_i2c_idle(struct omap_i2c_dev *dev)
 
 static int omap_i2c_init(struct omap_i2c_dev *dev)  {
-	u16 psc = 0, scll = 0, sclh = 0;
+	u16 psc = 0, scll = 0, sclh = 0, buf = 0;
 	u16 fsscll = 0, fssclh = 0, hsscll = 0, hssclh = 0;
 	unsigned long fclk_rate = 12000000;
 	unsigned long timeout;
@@ -287,24 +300,22 @@ static int omap_i2c_init(struct omap_i2c_dev *dev)
 					   SYSC_AUTOIDLE_MASK);
 
 		} else if (dev->rev >= OMAP_I2C_REV_ON_3430) {
-			u32 v;
-
-			v = SYSC_AUTOIDLE_MASK;
-			v |= SYSC_ENAWAKEUP_MASK;
-			v |= (SYSC_IDLEMODE_SMART <<
+			dev->syscstate = SYSC_AUTOIDLE_MASK;
+			dev->syscstate |= SYSC_ENAWAKEUP_MASK;
+			dev->syscstate |= (SYSC_IDLEMODE_SMART <<
 			      __ffs(SYSC_SIDLEMODE_MASK));
-			v |= (SYSC_CLOCKACTIVITY_FCLK <<
+			dev->syscstate |= (SYSC_CLOCKACTIVITY_FCLK <<
 			      __ffs(SYSC_CLOCKACTIVITY_MASK));
 
-			omap_i2c_write_reg(dev, OMAP_I2C_SYSC_REG, v);
+			omap_i2c_write_reg(dev, OMAP_I2C_SYSC_REG,
+							dev->syscstate);
 			/*
 			 * Enabling all wakup sources to stop I2C
freezing on
 			 * WFI instruction.
 			 * REVISIT: Some wkup sources might not be
needed.
 			 */
-			omap_i2c_write_reg(dev, OMAP_I2C_WE_REG,
-
OMAP_I2C_WE_ALL);
-
+			dev->westate = OMAP_I2C_WE_ALL;
+			omap_i2c_write_reg(dev, OMAP_I2C_WE_REG,
dev->westate);
 		}
 	}
 	omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0); @@ -373,23 +384,28
@@ static int omap_i2c_init(struct omap_i2c_dev *dev)
 	omap_i2c_write_reg(dev, OMAP_I2C_SCLL_REG, scll);
 	omap_i2c_write_reg(dev, OMAP_I2C_SCLH_REG, sclh);
 
-	if (dev->fifo_size)
-		/* Note: setup required fifo size - 1 */
-		omap_i2c_write_reg(dev, OMAP_I2C_BUF_REG,
-					(dev->fifo_size - 1) << 8 | /*
RTRSH */
-					OMAP_I2C_BUF_RXFIF_CLR |
-					(dev->fifo_size - 1) | /* XTRSH
*/
-					OMAP_I2C_BUF_TXFIF_CLR);
+	if (dev->fifo_size) {
+		/* Note: setup required fifo size - 1. RTRSH and XTRSH
*/
+		buf = (dev->fifo_size - 1) << 8 | OMAP_I2C_BUF_RXFIF_CLR
|
+			(dev->fifo_size - 1) | OMAP_I2C_BUF_TXFIF_CLR;
+		omap_i2c_write_reg(dev, OMAP_I2C_BUF_REG, buf);
+	}
 
 	/* Take the I2C module out of reset: */
 	omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN);
 
 	/* Enable interrupts */
-	omap_i2c_write_reg(dev, OMAP_I2C_IE_REG,
-			(OMAP_I2C_IE_XRDY | OMAP_I2C_IE_RRDY |
+	dev->iestate = (OMAP_I2C_IE_XRDY | OMAP_I2C_IE_RRDY |
 			OMAP_I2C_IE_ARDY | OMAP_I2C_IE_NACK |
 			OMAP_I2C_IE_AL)  | ((dev->fifo_size) ?
-				(OMAP_I2C_IE_RDR | OMAP_I2C_IE_XDR) :
0));
+				(OMAP_I2C_IE_RDR | OMAP_I2C_IE_XDR) :
0);
+	omap_i2c_write_reg(dev, OMAP_I2C_IE_REG, dev->iestate);
+	if (cpu_is_omap34xx()) {
+		dev->pscstate = psc;
+		dev->scllstate = scll;
+		dev->sclhstate = sclh;
+		dev->bufstate = buf;
+	}
 	return 0;
 }
 
--
1.6.3.2

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org More majordomo info
at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Kevin Hilman June 29, 2009, 5:04 p.m. UTC | #1
"HU TAO-TGHK48" <taohu@motorola.com> writes:

> I think we also need extra patch for fixing stability issue.
>
> Sometimes after back from retention/off mode, OMAP3430 I2C controller
> stops working even all register settings are restored.
>
> OMAP3430 TRM says: "During active mode (I2Ci.I2C_CON[15] I2C_EN bit is
> set to 1), make no changes to the I2Ci.I2C_SCLL and I2Ci.I2C_SCLH
> registers. 
> Changes may result in unpredictable behavior."
>
> diff --git a/drivers/i2c/busses/i2c-omap.c
> b/drivers/i2c/busses/i2c-omap.c index 5ce055c..b084209 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -238,12 +238,14 @@ static void omap_i2c_unidle(struct omap_i2c_dev
> *dev)
>  		clk_enable(dev->iclk);
>  	clk_enable(dev->fclk);
>  	if (cpu_is_omap34xx()) {
> +		omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0);
>  		omap_i2c_write_reg(dev, OMAP_I2C_PSC_REG,
> dev->pscstate);
>  		omap_i2c_write_reg(dev, OMAP_I2C_SCLL_REG,
> dev->scllstate);
>  		omap_i2c_write_reg(dev, OMAP_I2C_SCLH_REG,
> dev->sclhstate);
>  		omap_i2c_write_reg(dev, OMAP_I2C_BUF_REG,
> dev->bufstate);
>  		omap_i2c_write_reg(dev, OMAP_I2C_SYSC_REG,
> dev->syscstate);
>  		omap_i2c_write_reg(dev, OMAP_I2C_WE_REG, dev->westate);
> +		omap_i2c_write_reg(dev, OMAP_I2C_CON_REG,
> OMAP_I2C_CON_EN);
>  	}
>  	dev->idle = 0;
>  	omap_i2c_write_reg(dev, OMAP_I2C_IE_REG, dev->iestate);

Shouldn't you do a read-modify-write of I2C_CON_REG here?  Otherwise,
you're loosing any of the other settings in I2C_CON_REG.

Not being an expert in the I2C hardware, I'm not sure if it matters,
but this doesn't seem quite right due to possible side effects.

Kevin

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hunter, Jon June 29, 2009, 7:28 p.m. UTC | #2
Hi Kevin,

> Shouldn't you do a read-modify-write of I2C_CON_REG here?  Otherwise,
> you're loosing any of the other settings in I2C_CON_REG.
>
> Not being an expert in the I2C hardware, I'm not sure if it matters,
> but this doesn't seem quite right due to possible side effects.

This is a good question and this exact same issue came up on the omapzoom tree. For the omapzoom tree we ended up not implementing a read-modify-write here. The reason being that omap_i2c_unidle is called at the beginning of every transfer and we are re-configuring the I2C_CON register for every transfer. So when consulting with the TI linux team they said that it is ok to simply write 0 and clear the register here so we start over fresh for each transfer.  

I was trying to think if there would be any harm in doing a read-modify-write here. Probably not. You would not want the STT bit (generate a start command) to get set, however, this bit should not be set in the first place when entering this function. 

This change has been implemented in the omapzoom tree and so for you reference please see:
http://git.omapzoom.org/?p=repo/omapkernel.git;a=commit;h=ec70a0af52df54638a4fa33fc0dc3d24b1f893f1

Cheers
Jon
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kevin Hilman June 29, 2009, 9:15 p.m. UTC | #3
"Hunter, Jon" <jon-hunter@ti.com> writes:

> Hi Kevin,
>
>> Shouldn't you do a read-modify-write of I2C_CON_REG here?  Otherwise,
>> you're loosing any of the other settings in I2C_CON_REG.
>>
>> Not being an expert in the I2C hardware, I'm not sure if it matters,
>> but this doesn't seem quite right due to possible side effects.
>
> This is a good question and this exact same issue came up on the
> omapzoom tree. For the omapzoom tree we ended up not implementing a
> read-modify-write here. The reason being that omap_i2c_unidle is
> called at the beginning of every transfer and we are re-configuring
> the I2C_CON register for every transfer. So when consulting with the
> TI linux team they said that it is ok to simply write 0 and clear
> the register here so we start over fresh for each transfer.
>
> I was trying to think if there would be any harm in doing a
> read-modify-write here. Probably not. You would not want the STT bit
> (generate a start command) to get set, however, this bit should not
> be set in the first place when entering this function.
>
> This change has been implemented in the omapzoom tree and so for you
>reference please see:
>http://git.omapzoom.org/?p=repo/omapkernel.git;a=commit;h=ec70a0af52df54638a4fa33fc0dc3d24b1f893f1

Jon, thanks for the clarification.  I will fold this change into
the upstream-bound I2C changes.

Also thanks for the pointer to the original patch with author/signoff
credits.

Tao, in the future please be sure to cite original authors and/or
sources when submitting patches to the list.

Thanks,

Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hunter, Jon June 30, 2009, 4:49 p.m. UTC | #4
Hi Kevin,

> Jon, thanks for the clarification.  I will fold this change into
> the upstream-bound I2C changes.

Thanks.

> Tao, in the future please be sure to cite original authors and/or
> sources when submitting patches to the list.

Actually, I should point out that Hu Tao and I worked on this together originally for the omapzoom.org tree. So I should have added Hu Tao's name to this when submitting originally to omapzoom.org. So we should give credit to Hu Tao on this. Sorry about that.

Cheers,
Jon--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
HU TAO-TGHK48 July 1, 2009, 9:43 a.m. UTC | #5
Hi, Jon

It is fine.

In fact, the credit should be given to Xiaolong Chen
(A21785@motorola.com).



-----Original Message-----
From: Hunter, Jon [mailto:jon-hunter@ti.com] 
Sent: Wednesday, July 01, 2009 12:49 AM
To: Kevin Hilman
Cc: HU TAO-TGHK48; Aaro Koskinen; linux-omap@vger.kernel.org; Nayak,
Rajendra; Hogander Jouni (Nokia-D/Tampere); Pakaravoor, Jagadeesh
Subject: RE: [PATCH 4/4] I2C: OMAP3: PM: (re)init for every transfer to
support off-mode

Hi Kevin,

> Jon, thanks for the clarification.  I will fold this change into the 
> upstream-bound I2C changes.

Thanks.

> Tao, in the future please be sure to cite original authors and/or 
> sources when submitting patches to the list.

Actually, I should point out that Hu Tao and I worked on this together
originally for the omapzoom.org tree. So I should have added Hu Tao's
name to this when submitting originally to omapzoom.org. So we should
give credit to Hu Tao on this. Sorry about that.

Cheers,
Jon
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/drivers/i2c/busses/i2c-omap.c
b/drivers/i2c/busses/i2c-omap.c index 5ce055c..b084209 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -238,12 +238,14 @@  static void omap_i2c_unidle(struct omap_i2c_dev
*dev)
 		clk_enable(dev->iclk);
 	clk_enable(dev->fclk);
 	if (cpu_is_omap34xx()) {
+		omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0);
 		omap_i2c_write_reg(dev, OMAP_I2C_PSC_REG,
dev->pscstate);
 		omap_i2c_write_reg(dev, OMAP_I2C_SCLL_REG,
dev->scllstate);
 		omap_i2c_write_reg(dev, OMAP_I2C_SCLH_REG,
dev->sclhstate);
 		omap_i2c_write_reg(dev, OMAP_I2C_BUF_REG,
dev->bufstate);
 		omap_i2c_write_reg(dev, OMAP_I2C_SYSC_REG,
dev->syscstate);
 		omap_i2c_write_reg(dev, OMAP_I2C_WE_REG, dev->westate);
+		omap_i2c_write_reg(dev, OMAP_I2C_CON_REG,
OMAP_I2C_CON_EN);
 	}
 	dev->idle = 0;
 	omap_i2c_write_reg(dev, OMAP_I2C_IE_REG, dev->iestate);

-----Original Message-----
From: linux-omap-owner@vger.kernel.org
[mailto:linux-omap-owner@vger.kernel.org] On Behalf Of Kevin Hilman
Sent: Friday, June 26, 2009 10:19 PM
To: Aaro Koskinen
Cc: linux-omap@vger.kernel.org; Rajendra Nayak; Hogander Jouni
(Nokia-D/Tampere); Jagadeesh Bhaskar Pakaravoor
Subject: Re: [PATCH 4/4] I2C: OMAP3: PM: (re)init for every transfer to
support off-mode

Aaro Koskinen <aaro.koskinen@nokia.com> writes:

> Kevin Hilman wrote:
>> From: Rajendra Nayak <rnayak@ti.com>
>>
>> Because of OMAP off-mode, powerdomain can go off when I2C is idle.
>> Save enough state, and do a re-init for each transfer.
>>
>> Additional save/restore state added by Jagadeesh Bhaskar Pakaravoor
>> (SYSC_REG) and Aaro Koskinen (wakeup sources.)
>>
>> Signed-off-by: Jouni Hogander <jouni.hogander@nokia.com>
>> Signed-off-by: Rajendra Nayak <rnayak@ti.com>
>> Cc: Jagadeesh Bhaskar Pakaravoor <j-pakaravoor@ti.com>
>> Cc: Aaro Koskinen <aaro.koskinen@nokia.com>
>> Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
>
> This patch introduces a compiler warning:
>
>   CC      drivers/i2c/busses/i2c-omap.o
> drivers/i2c/busses/i2c-omap.c: In function 'omap_i2c_init':
> drivers/i2c/busses/i2c-omap.c:303: warning: unused variable 'v'
>
> So the declaration of "v" should be also removed here:
>

Thanks, updated patch in my queue an below for reference.

Kevin

From 6880534c0a42beea0e500a566f910875342c313b Mon Sep 17 00:00:00 2001
From: Rajendra Nayak <rnayak@ti.com>
Date: Fri, 26 Sep 2008 17:48:07 +0530
Subject: [PATCH] I2C: OMAP3: PM: (re)init for every transfer to support
off-mode

Because of OMAP off-mode, powerdomain can go off when I2C is idle.
Save enough state, and do a re-init for each transfer.

Additional save/restore state added by Jagadeesh Bhaskar Pakaravoor
(SYSC_REG) and Aaro Koskinen (wakeup sources.)

Signed-off-by: Jouni Hogander <jouni.hogander@nokia.com>
Signed-off-by: Rajendra Nayak <rnayak@ti.com>
Cc: Jagadeesh Bhaskar Pakaravoor <j-pakaravoor@ti.com>
Cc: Aaro Koskinen <aaro.koskinen@nokia.com>
Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
---
 drivers/i2c/busses/i2c-omap.c |   62
+++++++++++++++++++++++++---------------
 1 files changed, 39 insertions(+), 23 deletions(-)

diff --git a/drivers/i2c/busses/i2c-omap.c
b/drivers/i2c/busses/i2c-omap.c index ece0125..6b28e8a 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -178,6 +178,12 @@  struct omap_i2c_dev {
 	unsigned		b_hw:1;		/* bad h/w fixes */
 	unsigned		idle:1;
 	u16			iestate;	/* Saved interrupt
register */
+	u16			pscstate;
+	u16			scllstate;
+	u16			sclhstate;
+	u16			bufstate;
+	u16			syscstate;
+	u16			westate;
 };