| Message ID | F12CE1A68F023D498A2691C7B5393115035F09BD@ZMY16EXM66.ds.mot.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Kevin Hilman |
| Headers | show |
"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
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
"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
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
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
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; };
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