From: Lucas De Marchi <[email protected]>
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 <[email protected]>
---
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
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 <[email protected]>
>
> 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 <[email protected]>
> ---
>
> 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
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
Hi
On 04/01/2016 05:47 AM, Lucas De Marchi wrote:
> From: Lucas De Marchi <[email protected]>
>
> 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
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
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 <[email protected]>
---
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
CC'ing Christian.
On Fri, Apr 22, 2016 at 12:08 PM, Lucas De Marchi
<[email protected]> 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 <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
> 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
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
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