[PATCH v4 0/4] i2c: designware: improve performance for transfers

[PATCH v4 0/4] i2c: designware: improve performance for transfers

[Lucas De Marchi]

Diff from v3:

    - Fix over 80chars in one place

    - Move check for adapter being able to dynamically update TAR to be done
      on probe time rather than init as requested by Jarkko

For the previous version, Christian had added:

Tested-by: Christian Ruppert <christian.ruppert@alitech.com>
on TB101 with Linux-4.7

And Jarkko added his Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
on patches 1 and 3 (now 4).

There's a new patch #2 as a preparatory work to move the check
mentioned above to i2c_dw_probe().

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 (3):
  i2c: designware: add common functions for locking
  i2c: designware: detect when dynamic tar update is possible
  i2c: designware: do not disable adapter after transfer

 drivers/i2c/busses/i2c-designware-core.c | 162 +++++++++++++++++++++----------
 drivers/i2c/busses/i2c-designware-core.h |   1 +
 2 files changed, 111 insertions(+), 52 deletions(-)

-- 
2.7.4

[PATCH v4 1/4] i2c: designware: wait for disable/enable only if necessary

[Lucas De Marchi]

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

[PATCH v4 2/4] i2c: designware: add common functions for locking

[Lucas De Marchi]

These are used in 2 places and will be needed in more.

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 drivers/i2c/busses/i2c-designware-core.c | 52 ++++++++++++++++++++------------
 1 file changed, 32 insertions(+), 20 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
index 2c61585..79f33b3 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -287,6 +287,28 @@ static unsigned long i2c_dw_clk_rate(struct dw_i2c_dev *dev)
 	return dev->get_clk_rate_khz(dev);
 }
 
+static int i2c_dw_acquire_lock(struct dw_i2c_dev *dev)
+{
+	int ret;
+
+	if (!dev->acquire_lock)
+		return 0;
+
+	ret = dev->acquire_lock(dev);
+	if (!ret)
+		return 0;
+
+	dev_err(dev->dev, "couldn't acquire bus ownership\n");
+
+	return ret;
+}
+
+static void i2c_dw_release_lock(struct dw_i2c_dev *dev)
+{
+	if (dev->release_lock)
+		dev->release_lock(dev);
+}
+
 /**
  * i2c_dw_init() - initialize the designware i2c master hardware
  * @dev: device private data
@@ -302,13 +324,9 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
 	u32 sda_falling_time, scl_falling_time;
 	int ret;
 
-	if (dev->acquire_lock) {
-		ret = dev->acquire_lock(dev);
-		if (ret) {
-			dev_err(dev->dev, "couldn't acquire bus ownership\n");
-			return ret;
-		}
-	}
+	ret = i2c_dw_acquire_lock(dev);
+	if (ret)
+		return ret;
 
 	reg = dw_readl(dev, DW_IC_COMP_TYPE);
 	if (reg == ___constant_swab32(DW_IC_COMP_TYPE_VALUE)) {
@@ -320,8 +338,7 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
 	} else if (reg != DW_IC_COMP_TYPE_VALUE) {
 		dev_err(dev->dev, "Unknown Synopsys component type: "
 			"0x%08x\n", reg);
-		if (dev->release_lock)
-			dev->release_lock(dev);
+		i2c_dw_release_lock(dev);
 		return -ENODEV;
 	}
 
@@ -388,8 +405,8 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
 	/* configure the i2c master */
 	dw_writel(dev, dev->master_cfg , DW_IC_CON);
 
-	if (dev->release_lock)
-		dev->release_lock(dev);
+	i2c_dw_release_lock(dev);
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(i2c_dw_init);
@@ -652,13 +669,9 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 	dev->abort_source = 0;
 	dev->rx_outstanding = 0;
 
-	if (dev->acquire_lock) {
-		ret = dev->acquire_lock(dev);
-		if (ret) {
-			dev_err(dev->dev, "couldn't acquire bus ownership\n");
-			goto done_nolock;
-		}
-	}
+	ret = i2c_dw_acquire_lock(dev);
+	if (ret)
+		goto done_nolock;
 
 	ret = i2c_dw_wait_bus_not_busy(dev);
 	if (ret < 0)
@@ -705,8 +718,7 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 	ret = -EIO;
 
 done:
-	if (dev->release_lock)
-		dev->release_lock(dev);
+	i2c_dw_release_lock(dev);
 
 done_nolock:
 	pm_runtime_mark_last_busy(dev->dev);
-- 
2.7.4

[PATCH v4 3/4] i2c: designware: detect when dynamic tar update is possible

[Lucas De Marchi]

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 | 44 ++++++++++++++++++++++++--------
 drivers/i2c/busses/i2c-designware-core.h |  1 +
 2 files changed, 34 insertions(+), 11 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
index 79f33b3..b855beb 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -433,28 +433,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.
@@ -874,6 +875,7 @@ int i2c_dw_probe(struct dw_i2c_dev *dev)
 {
 	struct i2c_adapter *adap = &dev->adapter;
 	int r;
+	u32 reg;
 
 	init_completion(&dev->cmd_complete);
 
@@ -881,6 +883,26 @@ int i2c_dw_probe(struct dw_i2c_dev *dev)
 	if (r)
 		return r;
 
+	r = i2c_dw_acquire_lock(dev);
+	if (r)
+		return r;
+
+	/*
+	 * 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");
+	}
+
+	i2c_dw_release_lock(dev);
+
 	snprintf(adap->name, sizeof(adap->name),
 		 "Synopsys DesignWare I2C adapter");
 	adap->retries = 3;
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

[PATCH v4 4/4] i2c: designware: do not disable adapter after transfer

[Lucas De Marchi]

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 b855beb..e7f0285 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
 
@@ -434,9 +436,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) {
@@ -465,8 +483,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);
@@ -647,7 +665,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)
@@ -690,16 +709,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;
@@ -836,9 +845,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

Re: [PATCH v4 0/4] i2c: designware: improve performance for transfers

[Jarkko Nikula]

On 08/24/2016 01:18 AM, Lucas De Marchi wrote:
> Diff from v3:
>
>     - Fix over 80chars in one place
>
>     - Move check for adapter being able to dynamically update TAR to be done
>       on probe time rather than init as requested by Jarkko
>
> For the previous version, Christian had added:
>
> Tested-by: Christian Ruppert <christian.ruppert@alitech.com>
> on TB101 with Linux-4.7
>
> And Jarkko added his Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> on patches 1 and 3 (now 4).
>
Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

Re: [PATCH v4 0/4] i2c: designware: improve performance for transfers

[Christian Ruppert]

On 24.08.2016 00:18, Lucas De Marchi wrote:
> Diff from v3:
> 
>     - Fix over 80chars in one place
> 
>     - Move check for adapter being able to dynamically update TAR to be done
>       on probe time rather than init as requested by Jarkko
> 
> For the previous version, Christian had added:
> 
> Tested-by: Christian Ruppert <christian.ruppert@alitech.com>
> on TB101 with Linux-4.7

Tested-by: Christian Ruppert <christian.ruppert@alitech.com>
Re-tested this iteration in the same configuration (TB101, Linux-4.7).
Still works perfectly.

> And Jarkko added his Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> on patches 1 and 3 (now 4).
> 
> There's a new patch #2 as a preparatory work to move the check
> mentioned above to i2c_dw_probe().
> 
> 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 (3):
>   i2c: designware: add common functions for locking
>   i2c: designware: detect when dynamic tar update is possible
>   i2c: designware: do not disable adapter after transfer
> 
>  drivers/i2c/busses/i2c-designware-core.c | 162 +++++++++++++++++++++----------
>  drivers/i2c/busses/i2c-designware-core.h |   1 +
>  2 files changed, 111 insertions(+), 52 deletions(-)
> 

Re: [PATCH v4 3/4] i2c: designware: detect when dynamic tar update is possible

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 256 bytes --]

> +		u32 ic_con = dw_readl(dev, DW_IC_CON);
> +		if (msgs[dev->msg_write_idx].flags & I2C_M_TEN)

Checkpatch warned:

WARNING: Missing a blank line after declarations
#49: FILE: drivers/i2c/busses/i2c-designware-core.c:452:

I fixed it for you this time.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v4 0/4] i2c: designware: improve performance for transfers

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1602 bytes --]

On Tue, Aug 23, 2016 at 07:18:52PM -0300, Lucas De Marchi wrote:
> Diff from v3:
> 
>     - Fix over 80chars in one place
> 
>     - Move check for adapter being able to dynamically update TAR to be done
>       on probe time rather than init as requested by Jarkko
> 
> For the previous version, Christian had added:
> 
> Tested-by: Christian Ruppert <christian.ruppert@alitech.com>
> on TB101 with Linux-4.7
> 
> And Jarkko added his Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> on patches 1 and 3 (now 4).
> 
> There's a new patch #2 as a preparatory work to move the check
> mentioned above to i2c_dw_probe().
> 
> 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 (3):
>   i2c: designware: add common functions for locking
>   i2c: designware: detect when dynamic tar update is possible
>   i2c: designware: do not disable adapter after transfer

Applied to for-next, thanks! And thanks to all reviewers and testers,
much appreciated!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v4 1/4] i2c: designware: wait for disable/enable only if necessary

[Andrey Utkin]

On Tue, Aug 23, 2016 at 07:18:53PM -0300, Lucas De Marchi wrote:
> 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
> 

Regression reported: https://bugzilla.kernel.org/show_bug.cgi?id=194969

Re: [PATCH v4 1/4] i2c: designware: wait for disable/enable only if necessary

[Jarkko Nikula]

On 03/26/2017 10:44 PM, Andrey Utkin wrote:
> Regression reported: https://bugzilla.kernel.org/show_bug.cgi?id=194969
>
Thanks, I commented the bug.

-- 
Jarkko