From: Fabio Mello <[email protected]>
According to documentation and tests, initialization is not
necessary on module resume, since the controller keeps its state
between disable/enable. Change the target address is also allowed.
So, this patch replaces the initialization on module resume with a
simple enable, and removes the (non required anymore) enables and
disables.
Signed-off-by: Fabio Mello <[email protected]>
Signed-off-by: Lucas De Marchi <[email protected]>
---
These pictures explain a little more the consequence of letting the
enable+disable in the code:
http://pub.politreco.com/paste/TEK0011-before.jpg
http://pub.politreco.com/paste/TEK0007-after.jpg
The yellow line is a GPIO toggle in userspace to mark when we start and finish
the i2c transactions. The blue line is the SCL in that i2c bus. Take a look on
the huge pauses we have between any 2 transactions. These pauses are removed
with this patch and we are able to read our sensor's values in 950usec rather
than 5.24msec we had before. We are testing this using a Minnowboard Max that
has a designware i2c controller.
There's this comment in the code suggesting the designware controller might
have problems if we don't disable it after each transfer. We left a stress
code running for 3 days to check if anything bad would happen. This is the
test code using a PCA9685 (just because it was the easiest device we had
available to check read+write commands):
http://pub.politreco.com/paste/test-i2c.c
drivers/i2c/busses/i2c-designware-core.c | 19 ++++---------------
drivers/i2c/busses/i2c-designware-platdrv.c | 2 +-
2 files changed, 5 insertions(+), 16 deletions(-)
diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
index 6e25c01..b386c10 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -375,8 +375,12 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
/* configure the i2c master */
dw_writel(dev, dev->master_cfg , DW_IC_CON);
+ /* Enable the adapter */
+ __i2c_dw_enable(dev, true);
+
if (dev->release_lock)
dev->release_lock(dev);
+
return 0;
}
EXPORT_SYMBOL_GPL(i2c_dw_init);
@@ -405,9 +409,6 @@ 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 the slave address is ten bit address, enable 10BITADDR */
ic_con = dw_readl(dev, DW_IC_CON);
if (msgs[dev->msg_write_idx].flags & I2C_M_TEN) {
@@ -434,9 +435,6 @@ 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);
-
/* Clear and enable interrupts */
i2c_dw_clear_int(dev);
dw_writel(dev, DW_IC_INTR_DEFAULT_MASK, DW_IC_INTR_MASK);
@@ -665,15 +663,6 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
goto done;
}
- /*
- * We must disable the adapter before unlocking the &dev->lock mutex
- * below. 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;
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index c270f5f..0598695 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -320,7 +320,7 @@ static int dw_i2c_resume(struct device *dev)
clk_prepare_enable(i_dev->clk);
if (!i_dev->pm_runtime_disabled)
- i2c_dw_init(i_dev);
+ i2c_dw_enable(i_dev);
return 0;
}
--
2.4.2
On Mon, Jun 08, 2015 at 02:50:28PM -0300, [email protected] wrote:
> From: Fabio Mello <[email protected]>
>
> According to documentation and tests, initialization is not
> necessary on module resume, since the controller keeps its state
> between disable/enable. Change the target address is also allowed.
>
> So, this patch replaces the initialization on module resume with a
> simple enable, and removes the (non required anymore) enables and
> disables.
>
> Signed-off-by: Fabio Mello <[email protected]>
> Signed-off-by: Lucas De Marchi <[email protected]>
> ---
>
> These pictures explain a little more the consequence of letting the
> enable+disable in the code:
>
> http://pub.politreco.com/paste/TEK0011-before.jpg
> http://pub.politreco.com/paste/TEK0007-after.jpg
>
> The yellow line is a GPIO toggle in userspace to mark when we start and finish
> the i2c transactions. The blue line is the SCL in that i2c bus. Take a look on
> the huge pauses we have between any 2 transactions. These pauses are removed
> with this patch and we are able to read our sensor's values in 950usec rather
> than 5.24msec we had before. We are testing this using a Minnowboard Max that
> has a designware i2c controller.
Did you test this on any other platform than Intel Baytrail?
I'm adding Christian Ruppert (keeping the context) if he has any
comments. IIRC he added the per transfer enable/disable some time ago.
>
> There's this comment in the code suggesting the designware controller might
> have problems if we don't disable it after each transfer. We left a stress
> code running for 3 days to check if anything bad would happen. This is the
> test code using a PCA9685 (just because it was the easiest device we had
> available to check read+write commands):
>
> http://pub.politreco.com/paste/test-i2c.c
>
> drivers/i2c/busses/i2c-designware-core.c | 19 ++++---------------
> drivers/i2c/busses/i2c-designware-platdrv.c | 2 +-
> 2 files changed, 5 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
> index 6e25c01..b386c10 100644
> --- a/drivers/i2c/busses/i2c-designware-core.c
> +++ b/drivers/i2c/busses/i2c-designware-core.c
> @@ -375,8 +375,12 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
> /* configure the i2c master */
> dw_writel(dev, dev->master_cfg , DW_IC_CON);
>
> + /* Enable the adapter */
> + __i2c_dw_enable(dev, true);
> +
> if (dev->release_lock)
> dev->release_lock(dev);
> +
> return 0;
> }
> EXPORT_SYMBOL_GPL(i2c_dw_init);
> @@ -405,9 +409,6 @@ 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 the slave address is ten bit address, enable 10BITADDR */
> ic_con = dw_readl(dev, DW_IC_CON);
> if (msgs[dev->msg_write_idx].flags & I2C_M_TEN) {
> @@ -434,9 +435,6 @@ 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);
> -
> /* Clear and enable interrupts */
> i2c_dw_clear_int(dev);
> dw_writel(dev, DW_IC_INTR_DEFAULT_MASK, DW_IC_INTR_MASK);
> @@ -665,15 +663,6 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
> goto done;
> }
>
> - /*
> - * We must disable the adapter before unlocking the &dev->lock mutex
> - * below. 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;
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> index c270f5f..0598695 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -320,7 +320,7 @@ static int dw_i2c_resume(struct device *dev)
> clk_prepare_enable(i_dev->clk);
>
> if (!i_dev->pm_runtime_disabled)
> - i2c_dw_init(i_dev);
> + i2c_dw_enable(i_dev);
>
> return 0;
> }
> --
> 2.4.2
Hi Mika,
On Tue, Jun 9, 2015 at 5:51 AM, Mika Westerberg
<[email protected]> wrote:
> On Mon, Jun 08, 2015 at 02:50:28PM -0300, [email protected] wrote:
>> From: Fabio Mello <[email protected]>
>>
>> According to documentation and tests, initialization is not
>> necessary on module resume, since the controller keeps its state
>> between disable/enable. Change the target address is also allowed.
>>
>> So, this patch replaces the initialization on module resume with a
>> simple enable, and removes the (non required anymore) enables and
>> disables.
>>
>> Signed-off-by: Fabio Mello <[email protected]>
>> Signed-off-by: Lucas De Marchi <[email protected]>
>> ---
>>
>> These pictures explain a little more the consequence of letting the
>> enable+disable in the code:
>>
>> http://pub.politreco.com/paste/TEK0011-before.jpg
>> http://pub.politreco.com/paste/TEK0007-after.jpg
>>
>> The yellow line is a GPIO toggle in userspace to mark when we start and finish
>> the i2c transactions. The blue line is the SCL in that i2c bus. Take a look on
>> the huge pauses we have between any 2 transactions. These pauses are removed
>> with this patch and we are able to read our sensor's values in 950usec rather
>> than 5.24msec we had before. We are testing this using a Minnowboard Max that
>> has a designware i2c controller.
>
> Did you test this on any other platform than Intel Baytrail?
No. The only soc we have here with this controller is the Baytrail.
--
Lucas De Marchi
On Tue, Jun 09, 2015 at 03:29:01PM -0300, Lucas De Marchi wrote:
> Hi Mika,
>
> On Tue, Jun 9, 2015 at 5:51 AM, Mika Westerberg
> <[email protected]> wrote:
> > On Mon, Jun 08, 2015 at 02:50:28PM -0300, [email protected] wrote:
> >> From: Fabio Mello <[email protected]>
> >>
> >> According to documentation and tests, initialization is not
> >> necessary on module resume, since the controller keeps its state
> >> between disable/enable. Change the target address is also allowed.
> >>
> >> So, this patch replaces the initialization on module resume with a
> >> simple enable, and removes the (non required anymore) enables and
> >> disables.
> >>
> >> Signed-off-by: Fabio Mello <[email protected]>
> >> Signed-off-by: Lucas De Marchi <[email protected]>
> >> ---
> >>
> >> These pictures explain a little more the consequence of letting the
> >> enable+disable in the code:
> >>
> >> http://pub.politreco.com/paste/TEK0011-before.jpg
> >> http://pub.politreco.com/paste/TEK0007-after.jpg
> >>
> >> The yellow line is a GPIO toggle in userspace to mark when we start and finish
> >> the i2c transactions. The blue line is the SCL in that i2c bus. Take a look on
> >> the huge pauses we have between any 2 transactions. These pauses are removed
> >> with this patch and we are able to read our sensor's values in 950usec rather
> >> than 5.24msec we had before. We are testing this using a Minnowboard Max that
> >> has a designware i2c controller.
> >
> > Did you test this on any other platform than Intel Baytrail?
>
> No. The only soc we have here with this controller is the Baytrail.
My concern is that this patch might break some non-Intel platform. It
would be nice if someone (Christian?) could try this out.
On Mon, Jun 08, 2015 at 02:50:28PM -0300, [email protected] wrote:
> @@ -320,7 +320,7 @@ static int dw_i2c_resume(struct device *dev)
> clk_prepare_enable(i_dev->clk);
>
> if (!i_dev->pm_runtime_disabled)
> - i2c_dw_init(i_dev);
> + i2c_dw_enable(i_dev);
This will not work if the device power gets removed (for example being
put to D3cold) as it looses context.
On Wed, Jun 10, 2015 at 05:05:16PM +0200, [email protected] wrote:
> We should understand why the controller was disabled after successful
> transfers in the first place, however. Maybe some quirk with older
> versions of the hardware? Mika, do you have any memories about this?
I think it was in the original driver even before I did Intel specific
changes to that. I have no idea why it is done. Probably, like you said,
because some older version of the hardware needed it.
Hi,
On 06/10, [email protected] wrote:
> Mika Westerberg <[email protected]> wrote on 10.06.2015
> 09:07:22:
> > My concern is that this patch might break some non-Intel platform. It
> > would be nice if someone (Christian?) could try this out.
>
> Ouch, this one brings back painful memories. Take a look at patch
> 38d7fadef4973bb94e36897fcb6bb6a12fdd10c9 (
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=38d7fadef4973bb94e36897fcb6bb6a12fdd10c9
> ) for the context.
>
> In brief:
> - Before patch 38d7fade, the driver disabled the hardware after successful
> transfers. I do not know the reason for this and I cannot judge whether
> this is necessary or not. I thus decided not to modify this behaviour in
> patch 38d7fade.
>
> - After patch 38d7fade, the driver disabled the dw controller after all
> transfers, in particular in the case of unsuccessful transfers. This
> modification was necessary because of a race condition triggered by an i2c
> slave device which interrupted transfers in the middle. In this case, the
> dw controller (at least our version) seems to send spurious interrupts
> later if it is not disabled. The interrupt handler is not designed to be
> called on already aborted transfers, however, which leads to undesirable
> behaviour if the interrupt occurs at the wrong moment (system hangs in irq
> loop).
Yeah. So before we had:
- sucessful transfer
-> disable the adapter after complete
- timeout transfer
-> disable the adapter after complete
And your patch added the disable in case there wasn't a timeout but
dev->msg_err was set.
> I will try to dig out the test setup we used to validate the patch at the
> time but given the fact that this was two years ago this might take a
> little while. In the meantime, do you have any means to stress test the
> case of unexpected events on the bus (client aborts transfer, timeout
> etc.)? An alternative might be to only disable the controller in the case
> of errors and leave it active after successful transfers. We should
> understand why the controller was disabled after successful transfers in
It seems pretty rad to disable the controller after each transfer. It
may be due to previous revisions of the hw but may well be because of
"it was the simplest way to do it" at the time. Disabling the
controller after each transfer even if successful. For unsuccessful
transfers we may want to disable it if there's a hw bug, but it would be
good to check this indeed.
> the first place, however. Maybe some quirk with older versions of the
> hardware? Mika, do you have any memories about this?
I'm not sure if it's a hw bug or if it's just an oversight from previous
versions of this driver. So adding a quirk with correct versions will
be difficult IMO. We've been only testing this with well behaved
slaves. Do you have any idea how to test the bad guys? Maybe
connecting a slave with a long wire to introduce errors?
--
Lucas De Marchi
Hi Mika,
On Wed, Jun 10, 2015 at 4:55 AM, Mika Westerberg
<[email protected]> wrote:
> On Mon, Jun 08, 2015 at 02:50:28PM -0300, [email protected] wrote:
>> @@ -320,7 +320,7 @@ static int dw_i2c_resume(struct device *dev)
>> clk_prepare_enable(i_dev->clk);
>>
>> if (!i_dev->pm_runtime_disabled)
>> - i2c_dw_init(i_dev);
>> + i2c_dw_enable(i_dev);
>
> This will not work if the device power gets removed (for example being
> put to D3cold) as it looses context.
Do you mean we should keep the i2c_dw_init() here?
--
Lucas De Marchi
On Fri, Jun 12, 2015 at 07:45:00PM -0300, Lucas De Marchi wrote:
> Hi Mika,
>
> On Wed, Jun 10, 2015 at 4:55 AM, Mika Westerberg
> <[email protected]> wrote:
> > On Mon, Jun 08, 2015 at 02:50:28PM -0300, [email protected] wrote:
> >> @@ -320,7 +320,7 @@ static int dw_i2c_resume(struct device *dev)
> >> clk_prepare_enable(i_dev->clk);
> >>
> >> if (!i_dev->pm_runtime_disabled)
> >> - i2c_dw_init(i_dev);
> >> + i2c_dw_enable(i_dev);
> >
> > This will not work if the device power gets removed (for example being
> > put to D3cold) as it looses context.
>
> Do you mean we should keep the i2c_dw_init() here?
Yes, that's safer if the controller power gets removed.
Hello,
Christian Ruppert/ALi_GVA/ALi wrote on 10.06.2015 17:05:16:
> Mika Westerberg <[email protected]> wrote on 10.06.
> 2015 09:07:22:
> > On Tue, Jun 09, 2015 at 03:29:01PM -0300, Lucas De Marchi wrote:
> > > Hi Mika,
> > >
> > > On Tue, Jun 9, 2015 at 5:51 AM, Mika Westerberg
> > > <[email protected]> wrote:
> > > > On Mon, Jun 08, 2015 at 02:50:28PM -0300,
> [email protected] wrote:
> > > >> From: Fabio Mello <[email protected]>
> > > >>
> > > >> According to documentation and tests, initialization is not
> > > >> necessary on module resume, since the controller keeps its state
> > > >> between disable/enable. Change the target address is also
allowed.
> > > >>
> > > >> So, this patch replaces the initialization on module resume with
a
> > > >> simple enable, and removes the (non required anymore) enables and
> > > >> disables.
> > > >>
> > > >> Signed-off-by: Fabio Mello <[email protected]>
> > > >> Signed-off-by: Lucas De Marchi <[email protected]>
> > > >> ---
> > > >>
> > > >> These pictures explain a little more the consequence of letting
the
> > > >> enable+disable in the code:
> > > >>
> > > >> http://pub.politreco.com/paste/TEK0011-before.jpg
> > > >> http://pub.politreco.com/paste/TEK0007-after.jpg
> > > >>
> > > >> The yellow line is a GPIO toggle in userspace to mark when we
> > start and finish
> > > >> the i2c transactions. The blue line is the SCL in that i2c
> > bus. Take a look on
> > > >> the huge pauses we have between any 2 transactions. These
> > pauses are removed
> > > >> with this patch and we are able to read our sensor's values in
> > 950usec rather
> > > >> than 5.24msec we had before. We are testing this using a
> > Minnowboard Max that
> > > >> has a designware i2c controller.
> > > >
> > > > Did you test this on any other platform than Intel Baytrail?
> > >
> > > No. The only soc we have here with this controller is the Baytrail.
> >
> > My concern is that this patch might break some non-Intel platform. It
> > would be nice if someone (Christian?) could try this out.
>
> Ouch, this one brings back painful memories. Take a look at patch
> 38d7fadef4973bb94e36897fcb6bb6a12fdd10c9 (https://git.kernel.org/
> cgit/linux/kernel/git/torvalds/linux.git/commit/?
> id=38d7fadef4973bb94e36897fcb6bb6a12fdd10c9) for the context.
>
> In brief:
> - Before patch 38d7fade, the driver disabled the hardware after
> successful transfers. I do not know the reason for this and I cannot
> judge whether this is necessary or not. I thus decided not to modify
> this behaviour in patch 38d7fade.
>
> - After patch 38d7fade, the driver disabled the dw controller after
> all transfers, in particular in the case of unsuccessful transfers.
> This modification was necessary because of a race condition
> triggered by an i2c slave device which interrupted transfers in the
> middle. In this case, the dw controller (at least our version) seems
> to send spurious interrupts later if it is not disabled. The
> interrupt handler is not designed to be called on already aborted
> transfers, however, which leads to undesirable behaviour if the
> interrupt occurs at the wrong moment (system hangs in irq loop).
>
> I will try to dig out the test setup we used to validate the patch
> at the time but given the fact that this was two years ago this
> might take a little while. In the meantime, do you have any means to
> stress test the case of unexpected events on the bus (client aborts
> transfer, timeout etc.)? An alternative might be to only disable the
> controller in the case of errors and leave it active after
> successful transfers. We should understand why the controller was
> disabled after successful transfers in the first place, however.
> Maybe some quirk with older versions of the hardware? Mika, do you
> have any memories about this?
As promised I tried to dig out the test setup we used to validate patch
38d7fade at the time but without success. (I half expected that after such
a long time...)
So I said to myself, let's give the patch a try nevertheless and see if it
works in our system at least in the nominal case (no i2c bus errors).
The result is not very encouraging: Out of five (identical) designware i2c
controllers we have on my test SOC, the first one initialises properly but
the second one gets stuck in the famous irq loop right away when the
module is enabled in i2c_dw_init. The system never gets around to try
initialising the remaining three controllers due to the irq loop. I didn't
have the time to investigate the details yet but I suspect this is
triggered by some nastily behaved device on the bus. For example, some of
our external devices are notorious for keeping i2c lines tied to zero
before being properly powered on/reset/initialised by their respective
drivers (which in turn depend on the i2c master to be initialised first,
obviously). I'll grab an oscilloscope and dump the waves to confirm this
suspicion on the occasion.
However, similar situations might occur in multi-master setups (which we
don't have). I suspect the driver/hardware to also end up in the irq loop
after loosing arbitration with this patch. Has anybody the means to test
this in a multi-master system?
Greetings,
Christian
On Tue, Jun 23, 2015 at 1:45 PM, <[email protected]> wrote:
> Hello,
>
> Christian Ruppert/ALi_GVA/ALi wrote on 10.06.2015 17:05:16:
>> Mika Westerberg <[email protected]> wrote on 10.06.
>> 2015 09:07:22:
>> > On Tue, Jun 09, 2015 at 03:29:01PM -0300, Lucas De Marchi wrote:
>> > > Hi Mika,
>> > >
>> > > On Tue, Jun 9, 2015 at 5:51 AM, Mika Westerberg
>> > > <[email protected]> wrote:
>> > > > On Mon, Jun 08, 2015 at 02:50:28PM -0300,
>> [email protected] wrote:
>> > > >> From: Fabio Mello <[email protected]>
>> > > >>
>> > > >> According to documentation and tests, initialization is not
>> > > >> necessary on module resume, since the controller keeps its state
>> > > >> between disable/enable. Change the target address is also
> allowed.
>> > > >>
>> > > >> So, this patch replaces the initialization on module resume with
> a
>> > > >> simple enable, and removes the (non required anymore) enables and
>> > > >> disables.
>> > > >>
>> > > >> Signed-off-by: Fabio Mello <[email protected]>
>> > > >> Signed-off-by: Lucas De Marchi <[email protected]>
>> > > >> ---
>> > > >>
>> > > >> These pictures explain a little more the consequence of letting
> the
>> > > >> enable+disable in the code:
>> > > >>
>> > > >> http://pub.politreco.com/paste/TEK0011-before.jpg
>> > > >> http://pub.politreco.com/paste/TEK0007-after.jpg
>> > > >>
>> > > >> The yellow line is a GPIO toggle in userspace to mark when we
>> > start and finish
>> > > >> the i2c transactions. The blue line is the SCL in that i2c
>> > bus. Take a look on
>> > > >> the huge pauses we have between any 2 transactions. These
>> > pauses are removed
>> > > >> with this patch and we are able to read our sensor's values in
>> > 950usec rather
>> > > >> than 5.24msec we had before. We are testing this using a
>> > Minnowboard Max that
>> > > >> has a designware i2c controller.
>> > > >
>> > > > Did you test this on any other platform than Intel Baytrail?
>> > >
>> > > No. The only soc we have here with this controller is the Baytrail.
>> >
>> > My concern is that this patch might break some non-Intel platform. It
>> > would be nice if someone (Christian?) could try this out.
>>
>> Ouch, this one brings back painful memories. Take a look at patch
>> 38d7fadef4973bb94e36897fcb6bb6a12fdd10c9 (https://git.kernel.org/
>> cgit/linux/kernel/git/torvalds/linux.git/commit/?
>> id=38d7fadef4973bb94e36897fcb6bb6a12fdd10c9) for the context.
>>
>> In brief:
>> - Before patch 38d7fade, the driver disabled the hardware after
>> successful transfers. I do not know the reason for this and I cannot
>> judge whether this is necessary or not. I thus decided not to modify
>> this behaviour in patch 38d7fade.
>>
>> - After patch 38d7fade, the driver disabled the dw controller after
>> all transfers, in particular in the case of unsuccessful transfers.
>> This modification was necessary because of a race condition
>> triggered by an i2c slave device which interrupted transfers in the
>> middle. In this case, the dw controller (at least our version) seems
>> to send spurious interrupts later if it is not disabled. The
>> interrupt handler is not designed to be called on already aborted
>> transfers, however, which leads to undesirable behaviour if the
>> interrupt occurs at the wrong moment (system hangs in irq loop).
>>
>> I will try to dig out the test setup we used to validate the patch
>> at the time but given the fact that this was two years ago this
>> might take a little while. In the meantime, do you have any means to
>> stress test the case of unexpected events on the bus (client aborts
>> transfer, timeout etc.)? An alternative might be to only disable the
>> controller in the case of errors and leave it active after
>> successful transfers. We should understand why the controller was
>> disabled after successful transfers in the first place, however.
>> Maybe some quirk with older versions of the hardware? Mika, do you
>> have any memories about this?
>
> As promised I tried to dig out the test setup we used to validate patch
> 38d7fade at the time but without success. (I half expected that after such
> a long time...)
>
> So I said to myself, let's give the patch a try nevertheless and see if it
> works in our system at least in the nominal case (no i2c bus errors).
>
> The result is not very encouraging: Out of five (identical) designware i2c
> controllers we have on my test SOC, the first one initialises properly but
> the second one gets stuck in the famous irq loop right away when the
> module is enabled in i2c_dw_init. The system never gets around to try
Are you using the pci or platform driver? I noticed yesterday the pci
version is failing here with a NULL pointer dereference.
> initialising the remaining three controllers due to the irq loop. I didn't
> have the time to investigate the details yet but I suspect this is
> triggered by some nastily behaved device on the bus. For example, some of
> our external devices are notorious for keeping i2c lines tied to zero
> before being properly powered on/reset/initialised by their respective
> drivers (which in turn depend on the i2c master to be initialised first,
> obviously). I'll grab an oscilloscope and dump the waves to confirm this
> suspicion on the occasion.
Yeah, it'd be great to have it.
thanks for testing it with your setup.
--
Lucas De Marchi
Dear Lucas,
Lucas De Marchi <[email protected]> wrote on 23.06.2015 19:02:03:
> On Tue, Jun 23, 2015 at 1:45 PM, <[email protected]> wrote:
> > Hello,
> >
> > Christian Ruppert/ALi_GVA/ALi wrote on 10.06.2015 17:05:16:
> [...]
> > The result is not very encouraging: Out of five (identical) designware
i2c
> > controllers we have on my test SOC, the first one initialises properly
but
> > the second one gets stuck in the famous irq loop right away when the
> > module is enabled in i2c_dw_init. The system never gets around to try
>
> Are you using the pci or platform driver? I noticed yesterday the pci
> version is failing here with a NULL pointer dereference.
The test was performed with the platform driver (instantiated through
device tree).
I just re-checked and the ultimate problem which hangs/kills the system in
my case is the IRQ loop.
I haven't observed any NULL pointer dereferences on the road.
Greetings,
Christian
On Wed, Jun 24, 2015 at 09:36:43AM +0200, [email protected] wrote:
> Dear Lucas,
>
> Lucas De Marchi <[email protected]> wrote on 23.06.2015 19:02:03:
> > On Tue, Jun 23, 2015 at 1:45 PM, <[email protected]> wrote:
> > > Hello,
> > >
> > > Christian Ruppert/ALi_GVA/ALi wrote on 10.06.2015 17:05:16:
> > [...]
> > > The result is not very encouraging: Out of five (identical) designware
> i2c
> > > controllers we have on my test SOC, the first one initialises properly
> but
> > > the second one gets stuck in the famous irq loop right away when the
> > > module is enabled in i2c_dw_init. The system never gets around to try
> >
> > Are you using the pci or platform driver? I noticed yesterday the pci
> > version is failing here with a NULL pointer dereference.
>
> The test was performed with the platform driver (instantiated through
> device tree).
> I just re-checked and the ultimate problem which hangs/kills the system in
> my case is the IRQ loop.
> I haven't observed any NULL pointer dereferences on the road.
Thanks Christian for testing.
Since the patch causes problems on your hardware, I don't think it is
good idea to merge it.
On Wed, 2015-06-24 at 14:27 +0300, Mika Westerberg wrote:
> On Wed, Jun 24, 2015 at 09:36:43AM +0200, [email protected] wrot
> e:
> > Dear Lucas,
> >
> > Lucas De Marchi <[email protected]> wrote on 23.06.2015 19:02:03:
> > > On Tue, Jun 23, 2015 at 1:45 PM, <[email protected]> wrote:
> > > > Hello,
> > > >
> > > > Christian Ruppert/ALi_GVA/ALi wrote on 10.06.2015 17:05:16:
> > > [...]
> > > > The result is not very encouraging: Out of five (identical) designware
> > > >
> > i2c
> > > > controllers we have on my test SOC, the first one initialises properly
> > > >
> > but
> > > > the second one gets stuck in the famous irq loop right away when the
> > > > module is enabled in i2c_dw_init. The system never gets around to try
> > >
> > > Are you using the pci or platform driver? I noticed yesterday the pci
> > > version is failing here with a NULL pointer dereference.
> >
> > The test was performed with the platform driver (instantiated through
> > device tree).
> > I just re-checked and the ultimate problem which hangs/kills the system in
> >
> > my case is the IRQ loop.
> > I haven't observed any NULL pointer dereferences on the road.
>
> Thanks Christian for testing.
>
> Since the patch causes problems on your hardware, I don't think it is
> good idea to merge it.
Yeah, but it would be bad to ignore the problem as well. The way it is now
kills any possibility of using DW controller for reading sensors like
gyroscope, accelerometer, barometer that have higher sampling rate etc. I'll
try to come up with a new patch but since I can't reproduce the problem here
it'd be good to know if there's any means for me to test. What do you think
that could be done? Maybe putting the controller to sleep only in case of
errors?
thanks
--
Lucas De Marchi????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On Wed, Jun 24, 2015 at 12:56:19PM +0000, De Marchi, Lucas wrote:
> Yeah, but it would be bad to ignore the problem as well. The way it is now
> kills any possibility of using DW controller for reading sensors like
> gyroscope, accelerometer, barometer that have higher sampling rate etc. I'll
> try to come up with a new patch but since I can't reproduce the problem here
> it'd be good to know if there's any means for me to test. What do you think
> that could be done? Maybe putting the controller to sleep only in case of
> errors?
Instead of disabling the adapter after each transfer, I wonder if it is
enough if we just mask all interrupts? That should also prevent the
interrupt loop Christian is seeing on his hardware.
Dear Lucas,
"De Marchi, Lucas" <[email protected]> wrote on 24.06.2015
14:56:19:
> On Wed, 2015-06-24 at 14:27 +0300, Mika Westerberg wrote:
> > On Wed, Jun 24, 2015 at 09:36:43AM +0200,
[email protected] wrot
> > e:
> > > Dear Lucas,
> > >
> > > Lucas De Marchi <[email protected]> wrote on 23.06.2015
19:02:03:
> > > > On Tue, Jun 23, 2015 at 1:45 PM, <[email protected]>
wrote:
> > > > > Hello,
> > > > >
> > > > > Christian Ruppert/ALi_GVA/ALi wrote on 10.06.2015 17:05:16:
> > > > [...]
> > > > > The result is not very encouraging: Out of five (identical)
> designware
> > > > >
> > > i2c
> > > > > controllers we have on my test SOC, the first one
> initialises properly
> > > > >
> > > but
> > > > > the second one gets stuck in the famous irq loop right away when
the
> > > > > module is enabled in i2c_dw_init. The system never gets around
to try
> > > >
> > > > Are you using the pci or platform driver? I noticed yesterday the
pci
> > > > version is failing here with a NULL pointer dereference.
> > >
> > > The test was performed with the platform driver (instantiated
through
> > > device tree).
> > > I just re-checked and the ultimate problem which hangs/kills
thesystem in
> > >
> > > my case is the IRQ loop.
> > > I haven't observed any NULL pointer dereferences on the road.
> >
> > Thanks Christian for testing.
> >
> > Since the patch causes problems on your hardware, I don't think it is
> > good idea to merge it.
>
>
> Yeah, but it would be bad to ignore the problem as well. The way it is
now
> kills any possibility of using DW controller for reading sensors like
> gyroscope, accelerometer, barometer that have higher sampling rate etc.
I'll
> try to come up with a new patch but since I can't reproduce the problem
here
> it'd be good to know if there's any means for me to test.
I'll analyse the problem a bit further and send you a description (sda and
scl state at enable time) as soon as I can put my hands on an oscilloscope
one evening.
Greetings,
Christian
