Patchwork i2c-designware: Manually set RESTART bit between messages

login
register
mail settings
Submitter Chew Chiau Ee
Date June 21, 2013, 7:05 a.m.
Message ID <1371798328-31931-1-git-send-email-chiaue.ee.chew@intel.com>
Download mbox | patch
Permalink /patch/253146/
State Accepted
Headers show

Comments

Chew Chiau Ee - June 21, 2013, 7:05 a.m.
From: Chew, Chiau Ee <chiau.ee.chew@intel.com>

If both IC_EMPTYFIFO_HOLD_MASTER_EN and IC_RESTART_EN are set to 1, the
Designware I2C controller doesn't generate RESTART unless user specifically
requests it by setting RESTART bit in IC_DATA_CMD register.

Since IC_EMPTYFIFO_HOLD_MASTER_EN setting can't be detected from hardware
register, we must always manually set the restart bit between messages.

Signed-off-by: Chew, Chiau Ee <chiau.ee.chew@intel.com>
---
 drivers/i2c/busses/i2c-designware-core.c |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)
Wolfram Sang - July 3, 2013, 8:15 p.m.
CCing Mika and Christian.

On Fri, Jun 21, 2013 at 03:05:28PM +0800, Chew Chiau Ee wrote:
> From: Chew, Chiau Ee <chiau.ee.chew@intel.com>
> 
> If both IC_EMPTYFIFO_HOLD_MASTER_EN and IC_RESTART_EN are set to 1, the
> Designware I2C controller doesn't generate RESTART unless user specifically
> requests it by setting RESTART bit in IC_DATA_CMD register.
> 
> Since IC_EMPTYFIFO_HOLD_MASTER_EN setting can't be detected from hardware
> register, we must always manually set the restart bit between messages.
> 
> Signed-off-by: Chew, Chiau Ee <chiau.ee.chew@intel.com>

How come restart has worked before? Or did it not?

> ---
>  drivers/i2c/busses/i2c-designware-core.c |   14 ++++++++++++++
>  1 files changed, 14 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
> index 3de5494..9348439 100644
> --- a/drivers/i2c/busses/i2c-designware-core.c
> +++ b/drivers/i2c/busses/i2c-designware-core.c
> @@ -403,6 +403,7 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
>  	u32 addr = msgs[dev->msg_write_idx].addr;
>  	u32 buf_len = dev->tx_buf_len;
>  	u8 *buf = dev->tx_buf;
> +	bool need_restart = false;
>  
>  	intr_mask = DW_IC_INTR_DEFAULT_MASK;
>  
> @@ -430,6 +431,14 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
>  			/* new i2c_msg */
>  			buf = msgs[dev->msg_write_idx].buf;
>  			buf_len = msgs[dev->msg_write_idx].len;
> +
> +			/* If both IC_EMPTYFIFO_HOLD_MASTER_EN and
> +			 * IC_RESTART_EN are set, we must manually
> +			 * set restart bit between messages.
> +			 */
> +			if ((dev->master_cfg & DW_IC_CON_RESTART_EN) &&
> +					(dev->msg_write_idx > 0))
> +				need_restart = true;
>  		}
>  
>  		tx_limit = dev->tx_fifo_depth - dw_readl(dev, DW_IC_TXFLR);
> @@ -448,6 +457,11 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
>  			    buf_len == 1)
>  				cmd |= BIT(9);
>  
> +			if (need_restart) {
> +				cmd |= BIT(10);
> +				need_restart = false;
> +			}
> +
>  			if (msgs[dev->msg_write_idx].flags & I2C_M_RD) {
>  
>  				/* avoid rx buffer overrun */
> -- 
> 1.7.4.4
>
Mika Westerberg - July 4, 2013, 7:41 a.m.
On Wed, Jul 03, 2013 at 10:15:11PM +0200, Wolfram Sang wrote:
> 
> CCing Mika and Christian.
> 
> On Fri, Jun 21, 2013 at 03:05:28PM +0800, Chew Chiau Ee wrote:
> > From: Chew, Chiau Ee <chiau.ee.chew@intel.com>
> > 
> > If both IC_EMPTYFIFO_HOLD_MASTER_EN and IC_RESTART_EN are set to 1, the
> > Designware I2C controller doesn't generate RESTART unless user specifically
> > requests it by setting RESTART bit in IC_DATA_CMD register.
> > 
> > Since IC_EMPTYFIFO_HOLD_MASTER_EN setting can't be detected from hardware
> > register, we must always manually set the restart bit between messages.
> > 
> > Signed-off-by: Chew, Chiau Ee <chiau.ee.chew@intel.com>
> 
> How come restart has worked before? Or did it not?

It works fine. However IC_EMPTYFIFO_HOLD_MASTER_EN=1 makes a difference. We
had similar thing with the STOP that was fixed previously.

If I understand the dw i2c databook right, RESTART is only issued if the
transfer direction changes. So if you have two or more consecutive
transfers that have the same direction (not sure how common that is) there
is no RESTART between them unless DW_IC_CON_RESTART_EN is set.

Chiau Ee, does this fix a real problem?
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Chew, Chiau Ee - July 16, 2013, 3:27 a.m.
On Wed, Jul 03, 2013 at 10:15:11PM +0200, Wolfram Sang wrote:
> 
> CCing Mika and Christian.
> 
> On Fri, Jun 21, 2013 at 03:05:28PM +0800, Chew Chiau Ee wrote:
> > From: Chew, Chiau Ee <chiau.ee.chew@intel.com>
> > 
> > If both IC_EMPTYFIFO_HOLD_MASTER_EN and IC_RESTART_EN are set to 1, 
> > the Designware I2C controller doesn't generate RESTART unless user 
> > specifically requests it by setting RESTART bit in IC_DATA_CMD register.
> > 
> > Since IC_EMPTYFIFO_HOLD_MASTER_EN setting can't be detected from 
> > hardware register, we must always manually set the restart bit between messages.
> > 
> > Signed-off-by: Chew, Chiau Ee <chiau.ee.chew@intel.com>
> 
> How come restart has worked before? Or did it not?

> It works fine. However IC_EMPTYFIFO_HOLD_MASTER_EN=1 makes a difference. We had similar thing with the STOP that was fixed previously.

>If I understand the dw i2c databook right, RESTART is only issued if the transfer direction changes. So if you have two or more consecutive transfers that have the same direction (not sure how common that is) there is no RESTART between them unless >DW_IC_CON_RESTART_EN is set.

>Chiau Ee, does this fix a real problem?

Yes, it fixed a problem in one of our use case. we have a use case whereby the control path of the audio codec is using I2C bus. During our validation based on that use case,  we observed there is issue with back-to-back message transfer if this fix is not being added.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Wolfram Sang - Aug. 7, 2013, 2:28 p.m.
On Fri, Jun 21, 2013 at 03:05:28PM +0800, Chew Chiau Ee wrote:
> From: Chew, Chiau Ee <chiau.ee.chew@intel.com>
> 
> If both IC_EMPTYFIFO_HOLD_MASTER_EN and IC_RESTART_EN are set to 1, the
> Designware I2C controller doesn't generate RESTART unless user specifically
> requests it by setting RESTART bit in IC_DATA_CMD register.
> 
> Since IC_EMPTYFIFO_HOLD_MASTER_EN setting can't be detected from hardware
> register, we must always manually set the restart bit between messages.
> 
> Signed-off-by: Chew, Chiau Ee <chiau.ee.chew@intel.com>

Applied to for-next, thanks!

Patch

diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
index 3de5494..9348439 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -403,6 +403,7 @@  i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
 	u32 addr = msgs[dev->msg_write_idx].addr;
 	u32 buf_len = dev->tx_buf_len;
 	u8 *buf = dev->tx_buf;
+	bool need_restart = false;
 
 	intr_mask = DW_IC_INTR_DEFAULT_MASK;
 
@@ -430,6 +431,14 @@  i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
 			/* new i2c_msg */
 			buf = msgs[dev->msg_write_idx].buf;
 			buf_len = msgs[dev->msg_write_idx].len;
+
+			/* If both IC_EMPTYFIFO_HOLD_MASTER_EN and
+			 * IC_RESTART_EN are set, we must manually
+			 * set restart bit between messages.
+			 */
+			if ((dev->master_cfg & DW_IC_CON_RESTART_EN) &&
+					(dev->msg_write_idx > 0))
+				need_restart = true;
 		}
 
 		tx_limit = dev->tx_fifo_depth - dw_readl(dev, DW_IC_TXFLR);
@@ -448,6 +457,11 @@  i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
 			    buf_len == 1)
 				cmd |= BIT(9);
 
+			if (need_restart) {
+				cmd |= BIT(10);
+				need_restart = false;
+			}
+
 			if (msgs[dev->msg_write_idx].flags & I2C_M_RD) {
 
 				/* avoid rx buffer overrun */