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

Groups > linux.kernel > #1385264 > unrolled thread

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

Started byLucas De Marchi <lucas.demarchi@intel.com>
First post2016-04-22 17:10 +0200
Last post2016-04-27 09:50 +0200
Articles 5 — 3 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH] i2c: designware: do not disable adapter after transfer Lucas De Marchi <lucas.demarchi@intel.com> - 2016-04-22 17:10 +0200
    Re: [PATCH] i2c: designware: do not disable adapter after transfer Lucas De Marchi <lucas.de.marchi@gmail.com> - 2016-04-22 17:30 +0200
    Re: [PATCH] i2c: designware: do not disable adapter after transfer Jarkko Nikula <jarkko.nikula@linux.intel.com> - 2016-04-25 14:00 +0200
      Re: [PATCH] i2c: designware: do not disable adapter after transfer Lucas De Marchi <lucas.demarchi@intel.com> - 2016-04-25 17:10 +0200
        Re: [PATCH] i2c: designware: do not disable adapter after transfer Jarkko Nikula <jarkko.nikula@linux.intel.com> - 2016-04-27 09:50 +0200

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

FromLucas De Marchi <lucas.demarchi@intel.com>
Date2016-04-22 17:10 +0200
Subject[PATCH] i2c: designware: do not disable adapter after transfer
Message-ID<rqKSv-3S9-29@gated-at.bofh.it>
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

[toc] | [next] | [standalone]

#1385281

FromLucas De Marchi <lucas.de.marchi@gmail.com>
Date2016-04-22 17:30 +0200
Message-ID<rqLbR-419-35@gated-at.bofh.it>
In reply to#1385264
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

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

#1386316

FromJarkko Nikula <jarkko.nikula@linux.intel.com>
Date2016-04-25 14:00 +0200
Message-ID<rrNlh-5e1-35@gated-at.bofh.it>
In reply to#1385264
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

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

#1386559

FromLucas De Marchi <lucas.demarchi@intel.com>
Date2016-04-25 17:10 +0200
Message-ID<rrQj8-80L-5@gated-at.bofh.it>
In reply to#1386316

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

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

#1388732

FromJarkko Nikula <jarkko.nikula@linux.intel.com>
Date2016-04-27 09:50 +0200
Message-ID<rssop-5QU-7@gated-at.bofh.it>
In reply to#1386559
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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web