Re: [PATCH] i2c: designware: do not disable adapter after transfer
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 >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 >>> 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
Re: [PATCH] i2c: designware: do not disable adapter after transfer
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 > 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 >>> 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
Re: [PATCH] i2c: designware: do not disable adapter after transfer
Hi Christian, On Mon, May 2, 2016 at 7:11 AM, Christian Ruppertwrote: > 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 >> 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
Hi Christian, On Mon, May 2, 2016 at 7:11 AM, Christian Ruppert 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 >> 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
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 >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
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 > 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
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
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
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
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
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 2 -- 0 0 0xd0 0x07 0 0 0xd0 0x07 0 0 0xd0 0x07 0 0 0xd0 0x07 num_transfers=2 transfer_time_avg=1032.728500us After: i2c-transfer-time -w -a 0x40 -x 6 -n 2 -- 0 0 0xd0 0x07 0 0 0xd0 0x07 0 0 0xd0 0x07 0 0 0xd0 0x07 num_transfers=2 transfer_time_avg=470.256050us Signed-off-by: Lucas De Marchi--- 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
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 2 -- 0 0 0xd0 0x07 0 0 0xd0 0x07 0 0 0xd0 0x07 0 0 0xd0 0x07 num_transfers=2 transfer_time_avg=1032.728500us After: i2c-transfer-time -w -a 0x40 -x 6 -n 2 -- 0 0 0xd0 0x07 0 0 0xd0 0x07 0 0 0xd0 0x07 0 0 0xd0 0x07 num_transfers=2 transfer_time_avg=470.256050us Signed-off-by: Lucas De Marchi --- 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
CC'ing Christian. On Fri, Apr 22, 2016 at 12:08 PM, Lucas De Marchiwrote: > 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 2 -- 0 0 0xd0 0x07 0 0 0xd0 > 0x07 0 0 0xd0 0x07 0 0 0xd0 0x07 > num_transfers=2 > transfer_time_avg=1032.728500us > > After: > i2c-transfer-time -w -a 0x40 -x 6 -n 2 -- 0 0 0xd0 0x07 0 0 0xd0 > 0x07 0 0 0xd0 0x07 0 0 0xd0 0x07 > num_transfers=2 > transfer_time_avg=470.256050us > > Signed-off-by: Lucas De Marchi > --- > 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) > */ > >
Re: [PATCH] i2c: designware: do not disable adapter after transfer
CC'ing Christian. On Fri, Apr 22, 2016 at 12: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. 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 2 -- 0 0 0xd0 0x07 0 0 0xd0 > 0x07 0 0 0xd0 0x07 0 0 0xd0 0x07 > num_transfers=2 > transfer_time_avg=1032.728500us > > After: > i2c-transfer-time -w -a 0x40 -x 6 -n 2 -- 0 0 0xd0 0x07 0 0 0xd0 > 0x07 0 0 0xd0 0x07 0 0 0xd0 0x07 > num_transfers=2 > transfer_time_avg=470.256050us > > Signed-off-by: Lucas De Marchi > --- > 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 |
Re: [PATCH] i2c: designware: do not disable adapter after transfer
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
Re: [PATCH] i2c: designware: do not disable adapter after transfer
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
Re: [PATCH] i2c: designware: do not disable adapter after transfer
Hi On 04/01/2016 05:47 AM, Lucas De Marchi wrote: From: Lucas De MarchiDisabling 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 2 -- 0 0 0xd0 0x07 0 0 0xd0 0x07 0 0 0xd0 0x07 0 0 0xd0 0x07 num_transfers=2 transfer_time_avg=1032.728500us After: i2c-transfer-time -w -a 0x40 -x 6 -n 2 -- 0 0 0xd0 0x07 0 0 0xd0 0x07 0 0 0xd0 0x07 0 0 0xd0 0x07 num_transfers=2 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
Hi On 04/01/2016 05:47 AM, Lucas De Marchi wrote: From: 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. 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 2 -- 0 0 0xd0 0x07 0 0 0xd0 0x07 0 0 0xd0 0x07 0 0 0xd0 0x07 num_transfers=2 transfer_time_avg=1032.728500us After: i2c-transfer-time -w -a 0x40 -x 6 -n 2 -- 0 0 0xd0 0x07 0 0 0xd0 0x07 0 0 0xd0 0x07 0 0 0xd0 0x07 num_transfers=2 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
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
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
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> > 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 2 -- 0 0 0xd0 0x07 0 0 0xd0 > 0x07 0 0 0xd0 0x07 0 0 0xd0 0x07 > num_transfers=2 > transfer_time_avg=1032.728500us > > After: > i2c-transfer-time -w -a 0x40 -x 6 -n 2 -- 0 0 0xd0 0x07 0 0 0xd0 > 0x07 0 0 0xd0 0x07 0 0 0xd0 0x07 > num_transfers=2 > 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 > --- > > 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
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 > > 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 2 -- 0 0 0xd0 0x07 0 0 0xd0 > 0x07 0 0 0xd0 0x07 0 0 0xd0 0x07 > num_transfers=2 > transfer_time_avg=1032.728500us > > After: > i2c-transfer-time -w -a 0x40 -x 6 -n 2 -- 0 0 0xd0 0x07 0 0 0xd0 > 0x07 0 0 0xd0 0x07 0 0 0xd0 0x07 > num_transfers=2 > 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 > --- > > 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