Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1452140 > unrolled thread

[PATCH v3 0/3] i2c: designware: improve performance for transfers

Back to article view | Back to linux.kernel

Contents

  [PATCH v3 0/3] i2c: designware: improve performance for transfers Lucas De Marchi <lucas.demarchi@intel.com> - 2016-07-29 00:10 +0200
    [PATCH v3 3/3] i2c: designware: do not disable adapter after transfer Lucas De Marchi <lucas.demarchi@intel.com> - 2016-07-29 00:10 +0200
    [PATCH v3 2/3] i2c: designware: detect when dynamic tar update is possible Lucas De Marchi <lucas.demarchi@intel.com> - 2016-07-29 00:10 +0200
    [PATCH v3 1/3] i2c: designware: wait for disable/enable only if necessary Lucas De Marchi <lucas.demarchi@intel.com> - 2016-07-29 00:10 +0200

#1452140 — [PATCH v3 0/3] i2c: designware: improve performance for transfers

This can be considered a v3 of "i2c: designware: do not disable adapter after
transfer". Differences are:

    - Now there's a first patch that does not depend on IC_TAR being dynamically
      enabled/disabled: it just doesn't wait for the state change when not needed.

    - We added a patch that allows detecting if HW supports the dynamic TAR updates

    - In the last patch the bits were changed as suggested by Jarkko.

    - This is tested on BayTrail and CherryTrail, both of them returning true for
      "dynamically update TAR"

José Roberto de Souza (1):
  i2c: designware: wait for disable/enable only if necessary

Lucas De Marchi (2):
  i2c: designware: detect when dynamic tar update is possible
  i2c: designware: do not disable adapter after transfer

 drivers/i2c/busses/i2c-designware-core.c | 103 +++++++++++++++++++++----------
 drivers/i2c/busses/i2c-designware-core.h |   1 +
 2 files changed, 72 insertions(+), 32 deletions(-)

-- 
2.7.4

[toc] | [next] | [standalone]

#1452142 — [PATCH v3 3/3] i2c: designware: do not disable adapter after transfer

Disabling the adapter after each transfer adds additional delays
for each I2C transfer. Even if we don't wait for it to be disabled
anymore, on next transfer we will need to if we have several transfers
in a row.

Now during the transfer init we check if IC_TAR can be changed
dynamically, the status register for no activity and TX buffer being
empty. In this case we don't need to disable it

When a transfer fails the adapter will still be disabled - this is a
conservative approach. When transfers succeed, the adapter is left
enabled and it's configured so to disable interrupts.

Alternating register reads on 2 slaves:
perf stat -r4 chrt -f 10 ./i2c-test /dev/i2c-1 25000 0x40 0x6 0x1e 0x00

Before:
	8.638705161 seconds time elapsed                  ( +-  5.90% )
After:
	7.516821591 seconds time elapsed                  ( +-  0.11% )

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/i2c/busses/i2c-designware-core.c | 55 +++++++++++++++++++++-----------
 1 file changed, 37 insertions(+), 18 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
index a8408db..d260689 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -89,7 +89,9 @@
 					 DW_IC_INTR_TX_ABRT | \
 					 DW_IC_INTR_STOP_DET)
 
-#define DW_IC_STATUS_ACTIVITY	0x1
+#define DW_IC_STATUS_ACTIVITY		0x1
+#define DW_IC_STATUS_TFE		BIT(2)
+#define DW_IC_STATUS_MST_ACTIVITY	BIT(5)
 
 #define DW_IC_ERR_TX_ABRT	0x1
 
@@ -431,9 +433,25 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)
 {
 	struct i2c_msg *msgs = dev->msgs;
 	u32 ic_tar = 0;
+	bool enabled;
 
-	/* Disable the adapter */
-	__i2c_dw_enable_and_wait(dev, false);
+	enabled = dw_readl(dev, DW_IC_ENABLE_STATUS) & 1;
+
+	if (enabled) {
+		u32 ic_status;
+
+		/*
+		 * Only disable adapter if ic_tar and ic_con can't be
+		 * dynamically updated
+		 */
+		ic_status = dw_readl(dev, DW_IC_STATUS);
+		if (!dev->dynamic_tar_update_enabled ||
+		    (ic_status & DW_IC_STATUS_MST_ACTIVITY) ||
+		    !(ic_status & DW_IC_STATUS_TFE)) {
+			__i2c_dw_enable_and_wait(dev, false);
+			enabled = false;
+		}
+	}
 
 	/* if the slave address is ten bit address, enable 10BITADDR */
 	if (dev->dynamic_tar_update_enabled) {
@@ -462,8 +480,8 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)
 	/* enforce disabled interrupts (due to HW issues) */
 	i2c_dw_disable_int(dev);
 
-	/* Enable the adapter */
-	__i2c_dw_enable(dev, true);
+	if (!enabled)
+		__i2c_dw_enable(dev, true);
 
 	/* Clear and enable interrupts */
 	dw_readl(dev, DW_IC_CLR_INTR);
@@ -644,7 +662,8 @@ static int i2c_dw_handle_tx_abort(struct dw_i2c_dev *dev)
 }
 
 /*
- * Prepare controller for a transaction and call i2c_dw_xfer_msg
+ * Prepare controller for a transaction and start transfer by calling
+ * i2c_dw_xfer_init()
  */
 static int
 i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
@@ -691,16 +710,6 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 		goto done;
 	}
 
-	/*
-	 * We must disable the adapter before returning and signaling the end
-	 * of the current transfer. Otherwise the hardware might continue
-	 * generating interrupts which in turn causes a race condition with
-	 * the following transfer.  Needs some more investigation if the
-	 * additional interrupts are a hardware bug or this driver doesn't
-	 * handle them correctly yet.
-	 */
-	__i2c_dw_enable(dev, false);
-
 	if (dev->msg_err) {
 		ret = dev->msg_err;
 		goto done;
@@ -838,9 +847,19 @@ static irqreturn_t i2c_dw_isr(int this_irq, void *dev_id)
 	 */
 
 tx_aborted:
-	if ((stat & (DW_IC_INTR_TX_ABRT | DW_IC_INTR_STOP_DET)) || dev->msg_err)
+	if ((stat & (DW_IC_INTR_TX_ABRT | DW_IC_INTR_STOP_DET))
+			|| dev->msg_err) {
+		/*
+		 * We must disable interruts before returning and signaling
+		 * the end of the current transfer. Otherwise the hardware
+		 * might continue generating interrupts for non-existent
+		 * transfers.
+		 */
+		i2c_dw_disable_int(dev);
+		dw_readl(dev, DW_IC_CLR_INTR);
+
 		complete(&dev->cmd_complete);
-	else if (unlikely(dev->accessor_flags & ACCESS_INTR_MASK)) {
+	} else if (unlikely(dev->accessor_flags & ACCESS_INTR_MASK)) {
 		/* workaround to trigger pending interrupt */
 		stat = dw_readl(dev, DW_IC_INTR_MASK);
 		i2c_dw_disable_int(dev);
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1452144 — [PATCH v3 2/3] i2c: designware: detect when dynamic tar update is possible

This adapter can be synthesized with dynamic tar update enabled or disabled.
When enabled it is not necessary to disable the adapter to change the slave
address in some situations, which saves some time per transaction.

There is no direct register to know if this feature is enabled but we can do it
indirectly by writing to the 10BIT_ADDR field in IC_CON: this field is
read only when dynamic tar update is enabled.

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/i2c/busses/i2c-designware-core.c | 37 ++++++++++++++++++++++----------
 drivers/i2c/busses/i2c-designware-core.h |  1 +
 2 files changed, 27 insertions(+), 11 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
index 2c61585..a8408db 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -388,6 +388,20 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
 	/* configure the i2c master */
 	dw_writel(dev, dev->master_cfg , DW_IC_CON);
 
+	/*
+	 * Test if dynamic TAR update is enabled in this controller by writing to
+	 * IC_10BITADDR_MASTER field in IC_CON: when it is enabled this field
+	 * is read-only so it should not succeed
+	 */
+	reg = dw_readl(dev, DW_IC_CON);
+	dw_writel(dev, reg ^ DW_IC_CON_10BITADDR_MASTER, DW_IC_CON);
+
+	if ((dw_readl(dev, DW_IC_CON) & DW_IC_CON_10BITADDR_MASTER) ==
+	    (reg & DW_IC_CON_10BITADDR_MASTER)) {
+		dev->dynamic_tar_update_enabled = true;
+		dev_dbg(dev->dev, "Dynamic TAR update enabled");
+	}
+
 	if (dev->release_lock)
 		dev->release_lock(dev);
 	return 0;
@@ -416,28 +430,29 @@ static int i2c_dw_wait_bus_not_busy(struct dw_i2c_dev *dev)
 static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)
 {
 	struct i2c_msg *msgs = dev->msgs;
-	u32 ic_con, ic_tar = 0;
+	u32 ic_tar = 0;
 
 	/* Disable the adapter */
 	__i2c_dw_enable_and_wait(dev, false);
 
 	/* if the slave address is ten bit address, enable 10BITADDR */
-	ic_con = dw_readl(dev, DW_IC_CON);
-	if (msgs[dev->msg_write_idx].flags & I2C_M_TEN) {
-		ic_con |= DW_IC_CON_10BITADDR_MASTER;
+	if (dev->dynamic_tar_update_enabled) {
 		/*
 		 * If I2C_DYNAMIC_TAR_UPDATE is set, the 10-bit addressing
-		 * mode has to be enabled via bit 12 of IC_TAR register.
-		 * We set it always as I2C_DYNAMIC_TAR_UPDATE can't be
-		 * detected from registers.
+		 * mode has to be enabled via bit 12 of IC_TAR register,
+		 * otherwise bit 4 of IC_CON is used.
 		 */
-		ic_tar = DW_IC_TAR_10BITADDR_MASTER;
+		if (msgs[dev->msg_write_idx].flags & I2C_M_TEN)
+			ic_tar = DW_IC_TAR_10BITADDR_MASTER;
 	} else {
-		ic_con &= ~DW_IC_CON_10BITADDR_MASTER;
+		u32 ic_con = dw_readl(dev, DW_IC_CON);
+		if (msgs[dev->msg_write_idx].flags & I2C_M_TEN)
+			ic_con |= DW_IC_CON_10BITADDR_MASTER;
+		else
+			ic_con &= ~DW_IC_CON_10BITADDR_MASTER;
+		dw_writel(dev, ic_con, DW_IC_CON);
 	}
 
-	dw_writel(dev, ic_con, DW_IC_CON);
-
 	/*
 	 * Set the slave (target) address and enable 10-bit addressing mode
 	 * if applicable.
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index cd409e7..cd6e9ec 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -105,6 +105,7 @@ struct dw_i2c_dev {
 	int			(*acquire_lock)(struct dw_i2c_dev *dev);
 	void			(*release_lock)(struct dw_i2c_dev *dev);
 	bool			pm_runtime_disabled;
+	bool			dynamic_tar_update_enabled;
 };
 
 #define ACCESS_SWAP		0x00000001
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1452146 — [PATCH v3 1/3] i2c: designware: wait for disable/enable only if necessary

From: José Roberto de Souza <jose.souza@intel.com>

If we aren't going to continue using the controller we can just disable
it instead of waiting for it to complete. The biggest improvement here
is when a I2C transaction is completed and it doesn't block until
the adapter is disabled. When a new transfer is needed we will disable
and wait for its completion.

This way the adapter will continue changing its state in parallel to the
execution of the thread that requested the I2C transaction saving most
of the time 25~250 usec per I2C transaction.

A simple program doing a register read (1 byte write, 1 byte read)
alternating on 2 different slaves repeated 25k times for each and
measurements taken 4 times we get:

perf stat -r4 chrt -f 10 ./i2c-test /dev/i2c-1 25000 0x40 0x6 0x1e 0x00

Before:
	30.879317977 seconds time elapsed                 ( +- 14.83% )
After:
	8.638705161 seconds time elapsed                  ( +-  5.90% )

Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 drivers/i2c/busses/i2c-designware-core.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
index 99b54be..2c61585 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -252,10 +252,15 @@ static u32 i2c_dw_scl_lcnt(u32 ic_clk, u32 tLOW, u32 tf, int offset)
 
 static void __i2c_dw_enable(struct dw_i2c_dev *dev, bool enable)
 {
+	dw_writel(dev, enable, DW_IC_ENABLE);
+}
+
+static void __i2c_dw_enable_and_wait(struct dw_i2c_dev *dev, bool enable)
+{
 	int timeout = 100;
 
 	do {
-		dw_writel(dev, enable, DW_IC_ENABLE);
+		__i2c_dw_enable(dev, enable);
 		if ((dw_readl(dev, DW_IC_ENABLE_STATUS) & 1) == enable)
 			return;
 
@@ -321,7 +326,7 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
 	}
 
 	/* Disable the adapter */
-	__i2c_dw_enable(dev, false);
+	__i2c_dw_enable_and_wait(dev, false);
 
 	/* set standard and fast speed deviders for high/low periods */
 
@@ -414,7 +419,7 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)
 	u32 ic_con, ic_tar = 0;
 
 	/* Disable the adapter */
-	__i2c_dw_enable(dev, false);
+	__i2c_dw_enable_and_wait(dev, false);
 
 	/* if the slave address is ten bit address, enable 10BITADDR */
 	ic_con = dw_readl(dev, DW_IC_CON);
@@ -833,7 +838,7 @@ tx_aborted:
 void i2c_dw_disable(struct dw_i2c_dev *dev)
 {
 	/* Disable controller */
-	__i2c_dw_enable(dev, false);
+	__i2c_dw_enable_and_wait(dev, false);
 
 	/* Disable all interupts */
 	dw_writel(dev, 0, DW_IC_INTR_MASK);
-- 
2.7.4

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web