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

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

[Lucas De Marchi]

From: Lucas De Marchi <lucas.demarchi@intel.com>

Disabling the adapter after each transfer is pretty bad for sensors and
other devices doing small transfers at a high rate. It slows down the
transfer rate a lot since each of them have to wait the adapter to be
enabled again.

It was done in order to avoid the adapter to generate interrupts when
it's not being used. Instead of doing that here we just disable the
interrupt generation in the controller. With a small program test to
read/write registers in a sensor the speed doubled. Example below with
write sequences of 16 bytes:

Before:
	i2c-transfer-time -w -a 0x40 -x 6 -n 20000 -- 0 0 0xd0 0x07 0 0 0xd0 0x07 0 0 0xd0 0x07 0 0 0xd0 0x07
	num_transfers=20000
	transfer_time_avg=1032.728500us

After:
	i2c-transfer-time -w -a 0x40 -x 6 -n 20000 -- 0 0 0xd0 0x07 0 0 0xd0 0x07 0 0 0xd0 0x07 0 0 0xd0 0x07
	num_transfers=20000
	transfer_time_avg=470.256050us

During the init we check the status register for no activity and TX
buffer being empty since otherwise we can't change IC_TAR dynamically.

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---

Mika / Christian,

This is a second approach to a patch sent last year:
https://patchwork.ozlabs.org/patch/481952/

I hope that now it's in a better form. This has been tested on a Baytrail soc
(MinnowBoard Turbot) and is working. Would be nice to know if it also works on
Christian's machine since it was the one giving problems.  Christian, could you
give this a try?  It has been tested on top of 4.4.6 (+ some backports) and
4.6.0-rc1.

thanks!
Lucas De Marchi

 drivers/i2c/busses/i2c-designware-core.c | 50 ++++++++++++++++++++------------
 1 file changed, 31 insertions(+), 19 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
index 99b54be..f8e6f2b 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -90,6 +90,7 @@
 					 DW_IC_INTR_STOP_DET)
 
 #define DW_IC_STATUS_ACTIVITY	0x1
+#define DW_IC_STATUS_TX_EMPTY	0x2
 
 #define DW_IC_ERR_TX_ABRT	0x1
 
@@ -383,6 +384,8 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
 	/* configure the i2c master */
 	dw_writel(dev, dev->master_cfg , DW_IC_CON);
 
+	__i2c_dw_enable(dev, true);
+
 	if (dev->release_lock)
 		dev->release_lock(dev);
 	return 0;
@@ -412,9 +415,16 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)
 {
 	struct i2c_msg *msgs = dev->msgs;
 	u32 ic_con, ic_tar = 0;
-
-	/* Disable the adapter */
-	__i2c_dw_enable(dev, false);
+	bool need_reenable = false;
+	u32 ic_status;
+
+	/* check ic_tar and ic_con can be dynamically updated */
+	ic_status = dw_readl(dev, DW_IC_STATUS);
+	if (ic_status & DW_IC_STATUS_ACTIVITY
+		|| !(ic_status & DW_IC_STATUS_TX_EMPTY)) {
+		need_reenable = true;
+		__i2c_dw_enable(dev, false);
+	}
 
 	/* if the slave address is ten bit address, enable 10BITADDR */
 	ic_con = dw_readl(dev, DW_IC_CON);
@@ -442,8 +452,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 (need_reenable)
+		__i2c_dw_enable(dev, true);
 
 	/* Clear and enable interrupts */
 	dw_readl(dev, DW_IC_CLR_INTR);
@@ -624,7 +634,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)
@@ -665,22 +676,11 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 	/* wait for tx to complete */
 	if (!wait_for_completion_timeout(&dev->cmd_complete, HZ)) {
 		dev_err(dev->dev, "controller timed out\n");
-		/* i2c_dw_init implicitly disables the adapter */
 		i2c_dw_init(dev);
 		ret = -ETIMEDOUT;
 		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;
@@ -818,9 +818,21 @@ 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 which in turn causes a
+		 * race condition with next transfer.  Needs some more
+		 * investigation if the additional interrupts are a hardware
+		 * bug or this driver doesn't handle them correctly yet.
+		 */
+		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.5.5

Re: [PATCH] i2c: designware: do not disable adapter after transfer

[Christian Ruppert]

Dear Lucas,

Sorry for the late reply but I had to put our test environment back
together to check this patch. I'll keep it around for a while in case
you have further iterations to test.

On 2016-04-01 04:47, Lucas De Marchi wrote:
> From: Lucas De Marchi <lucas.demarchi@intel.com>
> 
> Disabling the adapter after each transfer is pretty bad for sensors and
> other devices doing small transfers at a high rate. It slows down the
> transfer rate a lot since each of them have to wait the adapter to be
> enabled again.
> 
> It was done in order to avoid the adapter to generate interrupts when
> it's not being used. Instead of doing that here we just disable the
> interrupt generation in the controller. With a small program test to
> read/write registers in a sensor the speed doubled. Example below with
> write sequences of 16 bytes:
> 
> Before:
> 	i2c-transfer-time -w -a 0x40 -x 6 -n 20000 -- 0 0 0xd0 0x07 0 0 0xd0 0x07 0 0 0xd0 0x07 0 0 0xd0 0x07
> 	num_transfers=20000
> 	transfer_time_avg=1032.728500us
> 
> After:
> 	i2c-transfer-time -w -a 0x40 -x 6 -n 20000 -- 0 0 0xd0 0x07 0 0 0xd0 0x07 0 0 0xd0 0x07 0 0 0xd0 0x07
> 	num_transfers=20000
> 	transfer_time_avg=470.256050us
> 
> During the init we check the status register for no activity and TX
> buffer being empty since otherwise we can't change IC_TAR dynamically.
> 
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
> 
> Mika / Christian,
> 
> This is a second approach to a patch sent last year:
> https://patchwork.ozlabs.org/patch/481952/
> 
> I hope that now it's in a better form. This has been tested on a Baytrail soc
> (MinnowBoard Turbot) and is working. Would be nice to know if it also works on
> Christian's machine since it was the one giving problems.  Christian, could you
> give this a try?  It has been tested on top of 4.4.6 (+ some backports) and
> 4.6.0-rc1.

On Linux-4.6.0-rc2 the behaviour is still the same: The kernel locks up
in an irq loop at dwi2c driver probe time. If I don't apply the patch
everything works fine and the system boots and talks normally on the i2c
bus.

One solution might be to add a device-tree (and acpi) flag to tell the
driver that it does not need to expect any nastily behaved devices on
the bus. If this flag is given, the driver could use the faster
disable-interrupt strategy. Without the flag we need to fall back to the
conservative disable-i2c-controller strategy.

I think such a flag should be OK because I2C buses are generally quite
static and the list of devices should be known at system integration
time. In the rare cases where this is not true, users could still go
with the conservative approach. I know that the "cleaner" method would
be to rather mark nasty devices, either in device tree or in the driver,
but I guess the required changes in the I2C framework might be overkill
for this dwi2c specific problem.

Greetings,
  Christian

Re: [PATCH] i2c: designware: do not disable adapter after transfer

[De Marchi, Lucas]

Hi Christian,

On Thu, 2016-04-07 at 15:37 +0200, Christian Ruppert wrote:
> Dear Lucas,
> 
> Sorry for the late reply but I had to put our test environment back
> together to check this patch. I'll keep it around for a while in case
> you have further iterations to test.

np, I'll try to iterate faster on this patch now, too.

> On Linux-4.6.0-rc2 the behaviour is still the same: The kernel locks up
> in an irq loop at dwi2c driver probe time. If I don't apply the patch
> everything works fine and the system boots and talks normally on the i2c
> bus.

:(

I really hoped this would work in your case.

> One solution might be to add a device-tree (and acpi) flag to tell the
> driver that it does not need to expect any nastily behaved devices on
> the bus. If this flag is given, the driver could use the faster
> disable-interrupt strategy. Without the flag we need to fall back to the
> conservative disable-i2c-controller strategy.

I'd like to try some other approaches before that. What if we start with it
disabled and enable at first use? After that we keep the approach of just
disabling the interrupts.  I can prep a patch for that.


> I think such a flag should be OK because I2C buses are generally quite
> static and the list of devices should be known at system integration
> time. In the rare cases where this is not true, users could still go
> with the conservative approach. I know that the "cleaner" method would
> be to rather mark nasty devices, either in device tree or in the driver,
> but I guess the required changes in the I2C framework might be overkill
> for this dwi2c specific problem.

agreed

thanks
Lucas De Marchi

Re: [PATCH] i2c: designware: do not disable adapter after transfer

[Jarkko Nikula]

Hi

On 04/01/2016 05:47 AM, Lucas De Marchi wrote:
> From: Lucas De Marchi <lucas.demarchi@intel.com>
>
> Disabling the adapter after each transfer is pretty bad for sensors and
> other devices doing small transfers at a high rate. It slows down the
> transfer rate a lot since each of them have to wait the adapter to be
> enabled again.
>
> It was done in order to avoid the adapter to generate interrupts when
> it's not being used. Instead of doing that here we just disable the
> interrupt generation in the controller. With a small program test to
> read/write registers in a sensor the speed doubled. Example below with
> write sequences of 16 bytes:
>
> Before:
> 	i2c-transfer-time -w -a 0x40 -x 6 -n 20000 -- 0 0 0xd0 0x07 0 0 0xd0 0x07 0 0 0xd0 0x07 0 0 0xd0 0x07
> 	num_transfers=20000
> 	transfer_time_avg=1032.728500us
>
> After:
> 	i2c-transfer-time -w -a 0x40 -x 6 -n 20000 -- 0 0 0xd0 0x07 0 0 0xd0 0x07 0 0 0xd0 0x07 0 0 0xd0 0x07
> 	num_transfers=20000
> 	transfer_time_avg=470.256050us
>
I gave a test to this patch and saw similar improvements when dumping 
large set of registers from I2C connected audio codec.

echo Y > /sys/kernel/debug/regmap/i2c-10EC5640\:00/cache_bypass
time cat /sys/kernel/debug/regmap/i2c-10EC5640\:00/registers >/dev/null

I checked the runtime PM status of adapter was suspended before running 
above cat command in order to get comparable results.

Before patch time was ~0.55 - ~0.76 s and with patch applied time was 
~0.16 - ~0.25 s.

Hopefully we'll find how to prevent regression on Christian's machines.

-- 
Jarkko

Re: [PATCH] i2c: designware: do not disable adapter after transfer

[Christian Ruppert]

On 2016-04-07 19:28, De Marchi, Lucas wrote:
> Hi Christian,
> 
> On Thu, 2016-04-07 at 15:37 +0200, Christian Ruppert wrote:
>> Dear Lucas,
>>
>> Sorry for the late reply but I had to put our test environment back
>> together to check this patch. I'll keep it around for a while in case
>> you have further iterations to test.
> 
> np, I'll try to iterate faster on this patch now, too.
> 
>> On Linux-4.6.0-rc2 the behaviour is still the same: The kernel locks up
>> in an irq loop at dwi2c driver probe time. If I don't apply the patch
>> everything works fine and the system boots and talks normally on the i2c
>> bus.
> 
> :(
> 
> I really hoped this would work in your case.
> 
>> One solution might be to add a device-tree (and acpi) flag to tell the
>> driver that it does not need to expect any nastily behaved devices on
>> the bus. If this flag is given, the driver could use the faster
>> disable-interrupt strategy. Without the flag we need to fall back to the
>> conservative disable-i2c-controller strategy.
> 
> I'd like to try some other approaches before that. What if we start with it
> disabled and enable at first use?

I think this might work in our case and be worth a try. When thinking
about it, it might even be cleaner to add a way to specify a list of
reset pins (e.g. through the GPIO reset framework) to trigger before
activating the bus. This would allow for more than one badly behaved
devices on the bus and also render everything more independent of the
probe order.

I'm afraid that the general case (bad device behaviour after the first
transfer) still cannot be covered by this strategy but I'm not sure if I
have a way to test this.

> After that we keep the approach of just
> disabling the interrupts.  I can prep a patch for that.

OK, I'll give it a try when it's ready.

Greetings,
  Christian

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

[Lucas De Marchi]

Disabling the adapter after each transfer is pretty bad for sensors and
other devices doing small transfers at a high rate. It slows down the
transfer rate a lot since each of them have to wait the adapter to be
enabled again.

During the transfer init we check the status register for no activity
and TX buffer being empty since otherwise we can't change IC_TAR
dynamically.

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.

With a small program test to read/write registers in a sensor the speed
doubled. Example below with write sequences of 16 bytes:

Before:
	i2c-transfer-time -w -a 0x40 -x 6 -n 20000 -- 0 0 0xd0 0x07 0 0 0xd0 0x07 0 0 0xd0 0x07 0 0 0xd0 0x07
	num_transfers=20000
	transfer_time_avg=1032.728500us

After:
	i2c-transfer-time -w -a 0x40 -x 6 -n 20000 -- 0 0 0xd0 0x07 0 0 0xd0 0x07 0 0 0xd0 0x07 0 0 0xd0 0x07
	num_transfers=20000
	transfer_time_avg=470.256050us

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 drivers/i2c/busses/i2c-designware-core.c | 48 ++++++++++++++++++++------------
 drivers/i2c/busses/i2c-designware-core.h |  1 +
 2 files changed, 31 insertions(+), 18 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
index 99b54be..8a08e68 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -90,6 +90,7 @@
 					 DW_IC_INTR_STOP_DET)
 
 #define DW_IC_STATUS_ACTIVITY	0x1
+#define DW_IC_STATUS_TX_EMPTY	0x2
 
 #define DW_IC_ERR_TX_ABRT	0x1
 
@@ -256,8 +257,10 @@ static void __i2c_dw_enable(struct dw_i2c_dev *dev, bool enable)
 
 	do {
 		dw_writel(dev, enable, DW_IC_ENABLE);
-		if ((dw_readl(dev, DW_IC_ENABLE_STATUS) & 1) == enable)
+		if ((dw_readl(dev, DW_IC_ENABLE_STATUS) & 1) == enable) {
+			dev->enabled = enable;
 			return;
+		}
 
 		/*
 		 * Wait 10 times the signaling period of the highest I2C
@@ -413,8 +416,16 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)
 	struct i2c_msg *msgs = dev->msgs;
 	u32 ic_con, ic_tar = 0;
 
-	/* Disable the adapter */
-	__i2c_dw_enable(dev, false);
+	if (dev->enabled) {
+		u32 ic_status;
+
+		/* check ic_tar and ic_con can be dynamically updated */
+		ic_status = dw_readl(dev, DW_IC_STATUS);
+		if (ic_status & DW_IC_STATUS_ACTIVITY
+			|| !(ic_status & DW_IC_STATUS_TX_EMPTY)) {
+			__i2c_dw_enable(dev, false);
+		}
+	}
 
 	/* if the slave address is ten bit address, enable 10BITADDR */
 	ic_con = dw_readl(dev, DW_IC_CON);
@@ -442,8 +453,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 (!dev->enabled)
+		__i2c_dw_enable(dev, true);
 
 	/* Clear and enable interrupts */
 	dw_readl(dev, DW_IC_CLR_INTR);
@@ -624,7 +635,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)
@@ -671,16 +683,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;
@@ -818,9 +820,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);
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index cd409e7..115c4b0 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                    enabled;
 };
 
 #define ACCESS_SWAP		0x00000001
-- 
2.5.5

Re: [PATCH] i2c: designware: do not disable adapter after transfer

[Lucas De Marchi]

CC'ing Christian.

On Fri, Apr 22, 2016 at 12:08 PM, Lucas De Marchi
<lucas.demarchi@intel.com> wrote:
> Disabling the adapter after each transfer is pretty bad for sensors and
> other devices doing small transfers at a high rate. It slows down the
> transfer rate a lot since each of them have to wait the adapter to be
> enabled again.
>
> During the transfer init we check the status register for no activity
> and TX buffer being empty since otherwise we can't change IC_TAR
> dynamically.
>
> 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.

Christian, this is the updated patch. Now adapter starts disabled and
is disabled when there's a failed transfer. I hope this can work with
your hardware.

Leaving patch below.

Lucas De Marchi


>
> With a small program test to read/write registers in a sensor the speed
> doubled. Example below with write sequences of 16 bytes:
>
> Before:
>         i2c-transfer-time -w -a 0x40 -x 6 -n 20000 -- 0 0 0xd0 0x07 0 0 0xd0 0x07 0 0 0xd0 0x07 0 0 0xd0 0x07
>         num_transfers=20000
>         transfer_time_avg=1032.728500us
>
> After:
>         i2c-transfer-time -w -a 0x40 -x 6 -n 20000 -- 0 0 0xd0 0x07 0 0 0xd0 0x07 0 0 0xd0 0x07 0 0 0xd0 0x07
>         num_transfers=20000
>         transfer_time_avg=470.256050us
>
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
>  drivers/i2c/busses/i2c-designware-core.c | 48 ++++++++++++++++++++------------
>  drivers/i2c/busses/i2c-designware-core.h |  1 +
>  2 files changed, 31 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
> index 99b54be..8a08e68 100644
> --- a/drivers/i2c/busses/i2c-designware-core.c
> +++ b/drivers/i2c/busses/i2c-designware-core.c
> @@ -90,6 +90,7 @@
>                                          DW_IC_INTR_STOP_DET)
>
>  #define DW_IC_STATUS_ACTIVITY  0x1
> +#define DW_IC_STATUS_TX_EMPTY  0x2
>
>  #define DW_IC_ERR_TX_ABRT      0x1
>
> @@ -256,8 +257,10 @@ static void __i2c_dw_enable(struct dw_i2c_dev *dev, bool enable)
>
>         do {
>                 dw_writel(dev, enable, DW_IC_ENABLE);
> -               if ((dw_readl(dev, DW_IC_ENABLE_STATUS) & 1) == enable)
> +               if ((dw_readl(dev, DW_IC_ENABLE_STATUS) & 1) == enable) {
> +                       dev->enabled = enable;
>                         return;
> +               }
>
>                 /*
>                  * Wait 10 times the signaling period of the highest I2C
> @@ -413,8 +416,16 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)
>         struct i2c_msg *msgs = dev->msgs;
>         u32 ic_con, ic_tar = 0;
>
> -       /* Disable the adapter */
> -       __i2c_dw_enable(dev, false);
> +       if (dev->enabled) {
> +               u32 ic_status;
> +
> +               /* check ic_tar and ic_con can be dynamically updated */
> +               ic_status = dw_readl(dev, DW_IC_STATUS);
> +               if (ic_status & DW_IC_STATUS_ACTIVITY
> +                       || !(ic_status & DW_IC_STATUS_TX_EMPTY)) {
> +                       __i2c_dw_enable(dev, false);
> +               }
> +       }
>
>         /* if the slave address is ten bit address, enable 10BITADDR */
>         ic_con = dw_readl(dev, DW_IC_CON);
> @@ -442,8 +453,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 (!dev->enabled)
> +               __i2c_dw_enable(dev, true);
>
>         /* Clear and enable interrupts */
>         dw_readl(dev, DW_IC_CLR_INTR);
> @@ -624,7 +635,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)
> @@ -671,16 +683,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;
> @@ -818,9 +820,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);
> diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
> index cd409e7..115c4b0 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                    enabled;
>  };
>
>  #define ACCESS_SWAP            0x00000001
> --
> 2.5.5
>



-- 
Lucas De Marchi

Re: [PATCH] i2c: designware: do not disable adapter after transfer

[Jarkko Nikula]

On 04/22/2016 06:08 PM, Lucas De Marchi wrote:
> Disabling the adapter after each transfer is pretty bad for sensors and
> other devices doing small transfers at a high rate. It slows down the
> transfer rate a lot since each of them have to wait the adapter to be
> enabled again.
>
> During the transfer init we check the status register for no activity
> and TX buffer being empty since otherwise we can't change IC_TAR
> dynamically.
>
> 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.
>
> With a small program test to read/write registers in a sensor the speed
> doubled. Example below with write sequences of 16 bytes:
>
> Before:
> 	i2c-transfer-time -w -a 0x40 -x 6 -n 20000 -- 0 0 0xd0 0x07 0 0 0xd0 0x07 0 0 0xd0 0x07 0 0 0xd0 0x07
> 	num_transfers=20000
> 	transfer_time_avg=1032.728500us
>
> After:
> 	i2c-transfer-time -w -a 0x40 -x 6 -n 20000 -- 0 0 0xd0 0x07 0 0 0xd0 0x07 0 0 0xd0 0x07 0 0 0xd0 0x07
> 	num_transfers=20000
> 	transfer_time_avg=470.256050us
>
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
>   drivers/i2c/busses/i2c-designware-core.c | 48 ++++++++++++++++++++------------
>   drivers/i2c/busses/i2c-designware-core.h |  1 +
>   2 files changed, 31 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
> index 99b54be..8a08e68 100644
> --- a/drivers/i2c/busses/i2c-designware-core.c
> +++ b/drivers/i2c/busses/i2c-designware-core.c
> @@ -90,6 +90,7 @@
>   					 DW_IC_INTR_STOP_DET)
>
>   #define DW_IC_STATUS_ACTIVITY	0x1
> +#define DW_IC_STATUS_TX_EMPTY	0x2

...

> @@ -413,8 +416,16 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)
>   	struct i2c_msg *msgs = dev->msgs;
>   	u32 ic_con, ic_tar = 0;
>
> -	/* Disable the adapter */
> -	__i2c_dw_enable(dev, false);
> +	if (dev->enabled) {
> +		u32 ic_status;
> +
> +		/* check ic_tar and ic_con can be dynamically updated */
> +		ic_status = dw_readl(dev, DW_IC_STATUS);
> +		if (ic_status & DW_IC_STATUS_ACTIVITY
> +			|| !(ic_status & DW_IC_STATUS_TX_EMPTY)) {
> +			__i2c_dw_enable(dev, false);
> +		}
> +	}
>
Worth to double check this. I see bit 1 means TX FIFO not full and bit 2 
is TX FIFO completely empty.

Otherwise I'm fine with the patch as long as it works for Christian.

-- 
Jarkko

Re: [PATCH] i2c: designware: do not disable adapter after transfer

[Lucas De Marchi]

On 04/25/2016 08:51 AM, Jarkko Nikula wrote:

[ ... ]

>> @@ -413,8 +416,16 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)
>>       struct i2c_msg *msgs = dev->msgs;
>>       u32 ic_con, ic_tar = 0;
>>
>> -    /* Disable the adapter */
>> -    __i2c_dw_enable(dev, false);
>> +    if (dev->enabled) {
>> +        u32 ic_status;
>> +
>> +        /* check ic_tar and ic_con can be dynamically updated */
>> +        ic_status = dw_readl(dev, DW_IC_STATUS);
>> +        if (ic_status & DW_IC_STATUS_ACTIVITY
>> +            || !(ic_status & DW_IC_STATUS_TX_EMPTY)) {
>> +            __i2c_dw_enable(dev, false);
>> +        }
>> +    }
>>
> Worth to double check this. I see bit 1 means TX FIFO not full and bit 2
> is TX FIFO completely empty.

the conditions to be able to update IC_TAR dynamically are:

   - Adapter isn't doing any TX/RX operation (IC_STATUS[5] == 0) and
   - There are no entries in TX FIFO (IC_STATUS[2] == 1)

So... yeah, the condition above seems wrong. I should be reading bit 5, 
not bit 1. Thanks! However:

IC_STATUS[5] signals activity for master mode
IC_STATUS[6] signals activity for slave mode
IC_STATUS[0] is IC_STATUS[5]|IC_STATUS[6]

And this controller is never in slave mode, only master mode, so it 
should be equivalent.

I wonder if I even have to check bit 5 since AFAICS we wouldn't be able 
to even call this function if there were any operation on tx/rx.

>
> Otherwise I'm fine with the patch as long as it works for Christian.
>

Anyway, I'll re-test with bit 5 checked and send an update.


Lucas De Marchi

Re: [PATCH] i2c: designware: do not disable adapter after transfer

[Jarkko Nikula]

On 04/25/2016 06:04 PM, Lucas De Marchi wrote:
>
>
> On 04/25/2016 08:51 AM, Jarkko Nikula wrote:
>
> [ ... ]
>
>>> @@ -413,8 +416,16 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev
>>> *dev)
>>>       struct i2c_msg *msgs = dev->msgs;
>>>       u32 ic_con, ic_tar = 0;
>>>
>>> -    /* Disable the adapter */
>>> -    __i2c_dw_enable(dev, false);
>>> +    if (dev->enabled) {
>>> +        u32 ic_status;
>>> +
>>> +        /* check ic_tar and ic_con can be dynamically updated */
>>> +        ic_status = dw_readl(dev, DW_IC_STATUS);
>>> +        if (ic_status & DW_IC_STATUS_ACTIVITY
>>> +            || !(ic_status & DW_IC_STATUS_TX_EMPTY)) {
>>> +            __i2c_dw_enable(dev, false);
>>> +        }
>>> +    }
>>>
>> Worth to double check this. I see bit 1 means TX FIFO not full and bit 2
>> is TX FIFO completely empty.
>
> the conditions to be able to update IC_TAR dynamically are:
>
>    - Adapter isn't doing any TX/RX operation (IC_STATUS[5] == 0) and
>    - There are no entries in TX FIFO (IC_STATUS[2] == 1)
>
> So... yeah, the condition above seems wrong. I should be reading bit 5,
> not bit 1. Thanks! However:
>
It reads above, bit 2 instead of 1 for TX FIFO checking and then either 
bit 5 or 0 for activity checking.

I'd say it's probably better to check bit 5 instead of bit 0 even bit 0 
is or'ed from bits 5 and 6. I don't know how possible slave support and 
slave being active will play here so it's best to follow spec.

-- 
jarkko

Re: [PATCH] i2c: designware: do not disable adapter after transfer

[Christian Ruppert]

Dear Lucas,

On 22.04.2016 17:19, Lucas De Marchi wrote:
> CC'ing Christian.
> 
> On Fri, Apr 22, 2016 at 12:08 PM, Lucas De Marchi
> <lucas.demarchi@intel.com> wrote:
>> Disabling the adapter after each transfer is pretty bad for sensors and
>> other devices doing small transfers at a high rate. It slows down the
>> transfer rate a lot since each of them have to wait the adapter to be
>> enabled again.
>>
>> During the transfer init we check the status register for no activity
>> and TX buffer being empty since otherwise we can't change IC_TAR
>> dynamically.
>>
>> 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.
> 
> Christian, this is the updated patch. Now adapter starts disabled and
> is disabled when there's a failed transfer. I hope this can work with
> your hardware.

Good news: The system now boots without deadlocks.

Bad  news: Not all transfers seem to take place as they should.
           I don't have the time for a deep analysis but a few quick
           experiments seem to indicate that the adapter needs to be
           disabled while updating TAR to a value different from the
           previous one. Disabling the adapter does not seem to be
           required if TAR doesn't change from one transfer to the next.
           I don't know if there are any conditions under which we can
           leave the adapter enabled while changing TAR but at least in
           some cases it seems to work.

The adapter seems to work reliably with the following diff on top of
your patch (included a dirty version of Jarkko's comments as well, not
sure if that's required to make everything work):

diff --git a/drivers/i2c/busses/i2c-designware-core.c
b/drivers/i2c/busses/i2c-designware-core.c
index 8a08e68..e927285 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -421,8 +421,8 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)

                /* check ic_tar and ic_con can be dynamically updated */
                ic_status = dw_readl(dev, DW_IC_STATUS);
-               if (ic_status & DW_IC_STATUS_ACTIVITY
-                       || !(ic_status & DW_IC_STATUS_TX_EMPTY)) {
+               if (ic_status & (0x1<<5)
+                       || !(ic_status & (0x1<<1))) {
                        __i2c_dw_enable(dev, false);
                }
        }
@@ -442,13 +442,18 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)
                ic_con &= ~DW_IC_CON_10BITADDR_MASTER;
        }

+       ic_tar |= msgs[dev->msg_write_idx].addr;
+
        dw_writel(dev, ic_con, DW_IC_CON);

        /*
         * Set the slave (target) address and enable 10-bit addressing mode
         * if applicable.
         */
-       dw_writel(dev, msgs[dev->msg_write_idx].addr | ic_tar, DW_IC_TAR);
+       if (ic_tar != dw_readl(dev, DW_IC_TAR))
+               __i2c_dw_enable(dev, false);
+
+       dw_writel(dev, ic_tar, DW_IC_TAR);

        /* enforce disabled interrupts (due to HW issues) */
        i2c_dw_disable_int(dev);

Re: [PATCH] i2c: designware: do not disable adapter after transfer

[Lucas De Marchi]

Hi Christian,

On Mon, May 2, 2016 at 7:11 AM, Christian Ruppert
<christian.ruppert@alitech.com> wrote:
> Dear Lucas,
>
> On 22.04.2016 17:19, Lucas De Marchi wrote:
>> CC'ing Christian.
>>
>> On Fri, Apr 22, 2016 at 12:08 PM, Lucas De Marchi
>> <lucas.demarchi@intel.com> wrote:
>>> Disabling the adapter after each transfer is pretty bad for sensors and
>>> other devices doing small transfers at a high rate. It slows down the
>>> transfer rate a lot since each of them have to wait the adapter to be
>>> enabled again.
>>>
>>> During the transfer init we check the status register for no activity
>>> and TX buffer being empty since otherwise we can't change IC_TAR
>>> dynamically.
>>>
>>> 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.
>>
>> Christian, this is the updated patch. Now adapter starts disabled and
>> is disabled when there's a failed transfer. I hope this can work with
>> your hardware.
>
> Good news: The system now boots without deadlocks.

:). Thanks for testing this.

>
> Bad  news: Not all transfers seem to take place as they should.
>            I don't have the time for a deep analysis but a few quick
>            experiments seem to indicate that the adapter needs to be
>            disabled while updating TAR to a value different from the
>            previous one. Disabling the adapter does not seem to be
>            required if TAR doesn't change from one transfer to the next.
>            I don't know if there are any conditions under which we can
>            leave the adapter enabled while changing TAR but at least in
>            some cases it seems to work.

:(

This is unfortunate. If the bus is shared for 2 slave devices we will
get the slow down back. I wonder if there's a HW version or something
like that in the registers which can be used to add quirks to tweak the
behavior.  I'll dig some documentation, but if anyone knows, it'd be
nice to have it pointed out.

Lucas De Marchi

Re: [PATCH] i2c: designware: do not disable adapter after transfer

[Christian Ruppert]

Dear Lucas,

On 04.05.2016 16:38, Lucas De Marchi wrote:
> Hi Christian,
> 
> On Mon, May 2, 2016 at 7:11 AM, Christian Ruppert
> <christian.ruppert@alitech.com> wrote:
>> Dear Lucas,
>>
>> On 22.04.2016 17:19, Lucas De Marchi wrote:
>>> CC'ing Christian.
>>>
>>> On Fri, Apr 22, 2016 at 12:08 PM, Lucas De Marchi
>>> <lucas.demarchi@intel.com> wrote:
> [...]
>> Bad  news: Not all transfers seem to take place as they should.
>>            I don't have the time for a deep analysis but a few quick
>>            experiments seem to indicate that the adapter needs to be
>>            disabled while updating TAR to a value different from the
>>            previous one. Disabling the adapter does not seem to be
>>            required if TAR doesn't change from one transfer to the next.
>>            I don't know if there are any conditions under which we can
>>            leave the adapter enabled while changing TAR but at least in
>>            some cases it seems to work.
> 
> :(
> 
> This is unfortunate. If the bus is shared for 2 slave devices we will
> get the slow down back. I wonder if there's a HW version or something
> like that in the registers which can be used to add quirks to tweak the
> behavior.  I'll dig some documentation, but if anyone knows, it'd be
> nice to have it pointed out.

There is such a register (DW_IC_COMP_VERSION) and we used it before for
hold time configuration, see line 374. In my case, the value of the
register is 0x3131372a. We're now just left with the problem to find out
how many (and which) hardware issues there are and in which version they
were fixed exactly...

Greetings,
  Christian