Re: [PATCH] i2c: designware: Fix regression when dynamic TAR update is disabled

[De Marchi, Lucas]

On Thu, 2017-02-09 at 22:07 +0200, Andy Shevchenko wrote:
> On Fri, 2017-02-10 at 01:20 +0530, Shah Nehal-Bakulchandra wrote:
> > The following commit causes a regression when dynamic TAR update is
> > disabled:
> > 
> >  commit 63d0f0a6952a1a02bc4f116b7da7c7887e46efa3 ("i2c:
> > designware:
> >  detect when dynamic tar update is possible")
> 
> Please, leave just 12 characters, it still enough.
> 
> > In such case, the DW_IC_CON_10BITADDR_MASTER is R/W, and is changed
> > by the logic that's trying to detect  dynamic TAR update.The original
> > value of DW_IC_CON_10BITADDR_MASTER bit should be restored.


You are right, thanks for the fix.  This may also explains why
0317e6c (i2c: designware: do not disable adapter after transfer) caused problems
and ended up being reverted.  Could you try that on your hardware?

The dynamic tar update detection was only done as preparation work to allow not
disabling the adapter, which is reverted.  We may also just revert this commit
instead of fixing the logic.


thanks
Lucas De Marchi

Re: [PATCH] i2c: designware: Fix regression when dynamic TAR update is disabled

[De Marchi, Lucas]

On Thu, 2017-02-09 at 22:07 +0200, Andy Shevchenko wrote:
> On Fri, 2017-02-10 at 01:20 +0530, Shah Nehal-Bakulchandra wrote:
> > The following commit causes a regression when dynamic TAR update is
> > disabled:
> > 
> >  commit 63d0f0a6952a1a02bc4f116b7da7c7887e46efa3 ("i2c:
> > designware:
> >  detect when dynamic tar update is possible")
> 
> Please, leave just 12 characters, it still enough.
> 
> > In such case, the DW_IC_CON_10BITADDR_MASTER is R/W, and is changed
> > by the logic that's trying to detect  dynamic TAR update.The original
> > value of DW_IC_CON_10BITADDR_MASTER bit should be restored.


You are right, thanks for the fix.  This may also explains why
0317e6c (i2c: designware: do not disable adapter after transfer) caused problems
and ended up being reverted.  Could you try that on your hardware?

The dynamic tar update detection was only done as preparation work to allow not
disabling the adapter, which is reverted.  We may also just revert this commit
instead of fixing the logic.


thanks
Lucas De Marchi

Re: [PATCH v3 2/3] i2c: designware: detect when dynamic tar update is possible

[De Marchi, Lucas]

On Wed, 2016-08-17 at 11:05 +0300, Jarkko Nikula wrote:
> On 08/16/2016 05:07 PM, De Marchi, Lucas wrote:
> > 
> > On Tue, 2016-08-16 at 17:00 +0300, Jarkko Nikula wrote:
> > > 
> > > > 
> > > > +    */
> > > > +   reg = dw_readl(dev, DW_IC_CON);
> > > > +   dw_writel(dev, reg ^ DW_IC_CON_10BITADDR_MASTER,
> > > > DW_IC_CON);
> > > > +
> > > > +   if ((dw_readl(dev, DW_IC_CON) &
> > > > DW_IC_CON_10BITADDR_MASTER) ==
> > > > +   (reg & DW_IC_CON_10BITADDR_MASTER)) {
> > > > +   dev->dynamic_tar_update_enabled = true;
> > > > +   dev_dbg(dev->dev, "Dynamic TAR update
> > > > enabled");
> > > > +   }
> > > 
> > > Is this possible to move to i2c_dw_probe()? I guess the enabled
> > > status
> > > doesn't change runtime?
> > 
> > It was actually useful at this place during development of this
> > patch
> > because we could check any unexpected change in behavior when
> > resuming.
> > We did catch a bug because of this and fixed.
> > I'm not sure if now it makes more sense to move to probe method.
> > I'd
> > leave it where it is, but I'm open to move it there.
> > 
> Can you do a quick re-test that case to see does it change runtime?
> If 
> it does then this needs a comment why there is need to do this check 
> each time when HW is reinitialized. Otherwise there is chance
> someone 
> may move this code to probe time in the future.

I already tested and it doesn't change. I'll move it to i2c_dw_probe()
then.

thanks

Lucas De Marchi

Re: [PATCH v3 2/3] i2c: designware: detect when dynamic tar update is possible

[De Marchi, Lucas]

On Wed, 2016-08-17 at 11:05 +0300, Jarkko Nikula wrote:
> On 08/16/2016 05:07 PM, De Marchi, Lucas wrote:
> > 
> > On Tue, 2016-08-16 at 17:00 +0300, Jarkko Nikula wrote:
> > > 
> > > > 
> > > > +    */
> > > > +   reg = dw_readl(dev, DW_IC_CON);
> > > > +   dw_writel(dev, reg ^ DW_IC_CON_10BITADDR_MASTER,
> > > > DW_IC_CON);
> > > > +
> > > > +   if ((dw_readl(dev, DW_IC_CON) &
> > > > DW_IC_CON_10BITADDR_MASTER) ==
> > > > +   (reg & DW_IC_CON_10BITADDR_MASTER)) {
> > > > +   dev->dynamic_tar_update_enabled = true;
> > > > +   dev_dbg(dev->dev, "Dynamic TAR update
> > > > enabled");
> > > > +   }
> > > 
> > > Is this possible to move to i2c_dw_probe()? I guess the enabled
> > > status
> > > doesn't change runtime?
> > 
> > It was actually useful at this place during development of this
> > patch
> > because we could check any unexpected change in behavior when
> > resuming.
> > We did catch a bug because of this and fixed.
> > I'm not sure if now it makes more sense to move to probe method.
> > I'd
> > leave it where it is, but I'm open to move it there.
> > 
> Can you do a quick re-test that case to see does it change runtime?
> If 
> it does then this needs a comment why there is need to do this check 
> each time when HW is reinitialized. Otherwise there is chance
> someone 
> may move this code to probe time in the future.

I already tested and it doesn't change. I'll move it to i2c_dw_probe()
then.

thanks

Lucas De Marchi

Re: [PATCH v3 2/3] i2c: designware: detect when dynamic tar update is possible

[De Marchi, Lucas]

On Tue, 2016-08-16 at 17:00 +0300, Jarkko Nikula wrote:
> Hi, + Wolfram
> 
> On 07/29/2016 01:03 AM, Lucas De Marchi wrote:
> > 
> > This adapter can be synthesized with dynamic tar update enabled or
> > disabled.
> > When enabled it is not necessary to disable the adapter to change
> > the slave
> > address in some situations, which saves some time per transaction.
> > 
> > There is no direct register to know if this feature is enabled but
> > we can do it
> > indirectly by writing to the 10BIT_ADDR field in IC_CON: this field
> > is
> > read only when dynamic tar update is enabled.
> > 
> > Signed-off-by: Lucas De Marchi 
> > Signed-off-by: José Roberto de Souza 
> > ---
> >  drivers/i2c/busses/i2c-designware-core.c | 37
> > ++--
> >  drivers/i2c/busses/i2c-designware-core.h |  1 +
> >  2 files changed, 27 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-designware-core.c
> > b/drivers/i2c/busses/i2c-designware-core.c
> > index 2c61585..a8408db 100644
> > --- a/drivers/i2c/busses/i2c-designware-core.c
> > +++ b/drivers/i2c/busses/i2c-designware-core.c
> > @@ -388,6 +388,20 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
> >     /* configure the i2c master */
> >     dw_writel(dev, dev->master_cfg , DW_IC_CON);
> > 
> > +   /*
> > +    * Test if dynamic TAR update is enabled in this
> > controller by writing to
> 
> Over 80 characters in this line.

I'll fix this and wait for more comments (or a few days) before sending
a new version.

> > +    */
> > +   reg = dw_readl(dev, DW_IC_CON);
> > +   dw_writel(dev, reg ^ DW_IC_CON_10BITADDR_MASTER,
> > DW_IC_CON);
> > +
> > +   if ((dw_readl(dev, DW_IC_CON) &
> > DW_IC_CON_10BITADDR_MASTER) ==
> > +   (reg & DW_IC_CON_10BITADDR_MASTER)) {
> > +   dev->dynamic_tar_update_enabled = true;
> > +   dev_dbg(dev->dev, "Dynamic TAR update enabled");
> > +   }
> 
> Is this possible to move to i2c_dw_probe()? I guess the enabled
> status 
> doesn't change runtime?

It was actually useful at this place during development of this patch
because we could check any unexpected change in behavior when resuming.
We did catch a bug because of this and fixed.
I'm not sure if now it makes more sense to move to probe method. I'd
leave it where it is, but I'm open to move it there.

thanks

Lucas De Marchi

Re: [PATCH v3 2/3] i2c: designware: detect when dynamic tar update is possible

[De Marchi, Lucas]

On Tue, 2016-08-16 at 17:00 +0300, Jarkko Nikula wrote:
> Hi, + Wolfram
> 
> On 07/29/2016 01:03 AM, Lucas De Marchi wrote:
> > 
> > This adapter can be synthesized with dynamic tar update enabled or
> > disabled.
> > When enabled it is not necessary to disable the adapter to change
> > the slave
> > address in some situations, which saves some time per transaction.
> > 
> > There is no direct register to know if this feature is enabled but
> > we can do it
> > indirectly by writing to the 10BIT_ADDR field in IC_CON: this field
> > is
> > read only when dynamic tar update is enabled.
> > 
> > Signed-off-by: Lucas De Marchi 
> > Signed-off-by: José Roberto de Souza 
> > ---
> >  drivers/i2c/busses/i2c-designware-core.c | 37
> > ++--
> >  drivers/i2c/busses/i2c-designware-core.h |  1 +
> >  2 files changed, 27 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-designware-core.c
> > b/drivers/i2c/busses/i2c-designware-core.c
> > index 2c61585..a8408db 100644
> > --- a/drivers/i2c/busses/i2c-designware-core.c
> > +++ b/drivers/i2c/busses/i2c-designware-core.c
> > @@ -388,6 +388,20 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
> >     /* configure the i2c master */
> >     dw_writel(dev, dev->master_cfg , DW_IC_CON);
> > 
> > +   /*
> > +    * Test if dynamic TAR update is enabled in this
> > controller by writing to
> 
> Over 80 characters in this line.

I'll fix this and wait for more comments (or a few days) before sending
a new version.

> > +    */
> > +   reg = dw_readl(dev, DW_IC_CON);
> > +   dw_writel(dev, reg ^ DW_IC_CON_10BITADDR_MASTER,
> > DW_IC_CON);
> > +
> > +   if ((dw_readl(dev, DW_IC_CON) &
> > DW_IC_CON_10BITADDR_MASTER) ==
> > +   (reg & DW_IC_CON_10BITADDR_MASTER)) {
> > +   dev->dynamic_tar_update_enabled = true;
> > +   dev_dbg(dev->dev, "Dynamic TAR update enabled");
> > +   }
> 
> Is this possible to move to i2c_dw_probe()? I guess the enabled
> status 
> doesn't change runtime?

It was actually useful at this place during development of this patch
because we could check any unexpected change in behavior when resuming.
We did catch a bug because of this and fixed.
I'm not sure if now it makes more sense to move to probe method. I'd
leave it where it is, but I'm open to move it there.

thanks

Lucas De Marchi

Re: [PATCH v3 0/3] i2c: designware: improve performance for transfers

[De Marchi, Lucas]

On Fri, 2016-08-12 at 16:59 +0200, Christian Ruppert wrote:
> On 29.07.2016 00:03, Lucas De Marchi wrote:
> > 
> > This can be considered a v3 of "i2c: designware: do not disable
> > adapter after
> > transfer". Differences are:
> > 
> > - Now there's a first patch that does not depend on IC_TAR
> > being dynamically
> >   enabled/disabled: it just doesn't wait for the state change
> > when not needed.
> > 
> > - We added a patch that allows detecting if HW supports the
> > dynamic TAR updates
> > 
> > - In the last patch the bits were changed as suggested by
> > Jarkko.
> > 
> > - This is tested on BayTrail and CherryTrail, both of them
> > returning true for
> >   "dynamically update TAR"
> 
> Tested-by: Christian Ruppert 
> on TB101 with Linux-4.7
> 
> Seems to work perfectly now, congratulations and thanks for your
> patience with our platform... This was a hard one!

Awesome, thanks for testing this series.

Looks like I forgot to CC Wolfram. CC'ing now on this cover letter, let
me know if you also need the individual patches.


Lucas De Marchi

> 
> > 
> > José Roberto de Souza (1):
> >   i2c: designware: wait for disable/enable only if necessary
> > 
> > Lucas De Marchi (2):
> >   i2c: designware: detect when dynamic tar update is possible
> >   i2c: designware: do not disable adapter after transfer
> > 
> >  drivers/i2c/busses/i2c-designware-core.c | 103
> > +--
> >  drivers/i2c/busses/i2c-designware-core.h |   1 +
> >  2 files changed, 72 insertions(+), 32 deletions(-)
> > 
> 

Re: [PATCH v3 0/3] i2c: designware: improve performance for transfers

[De Marchi, Lucas]

On Fri, 2016-08-12 at 16:59 +0200, Christian Ruppert wrote:
> On 29.07.2016 00:03, Lucas De Marchi wrote:
> > 
> > This can be considered a v3 of "i2c: designware: do not disable
> > adapter after
> > transfer". Differences are:
> > 
> > - Now there's a first patch that does not depend on IC_TAR
> > being dynamically
> >   enabled/disabled: it just doesn't wait for the state change
> > when not needed.
> > 
> > - We added a patch that allows detecting if HW supports the
> > dynamic TAR updates
> > 
> > - In the last patch the bits were changed as suggested by
> > Jarkko.
> > 
> > - This is tested on BayTrail and CherryTrail, both of them
> > returning true for
> >   "dynamically update TAR"
> 
> Tested-by: Christian Ruppert 
> on TB101 with Linux-4.7
> 
> Seems to work perfectly now, congratulations and thanks for your
> patience with our platform... This was a hard one!

Awesome, thanks for testing this series.

Looks like I forgot to CC Wolfram. CC'ing now on this cover letter, let
me know if you also need the individual patches.


Lucas De Marchi

> 
> > 
> > José Roberto de Souza (1):
> >   i2c: designware: wait for disable/enable only if necessary
> > 
> > Lucas De Marchi (2):
> >   i2c: designware: detect when dynamic tar update is possible
> >   i2c: designware: do not disable adapter after transfer
> > 
> >  drivers/i2c/busses/i2c-designware-core.c | 103
> > +--
> >  drivers/i2c/busses/i2c-designware-core.h |   1 +
> >  2 files changed, 72 insertions(+), 32 deletions(-)
> > 
> 

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

[De Marchi, Lucas]

Hi Christian,

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

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

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

:(

I really hoped this would work in your case.

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

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


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

agreed

thanks
Lucas De Marchi

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

[De Marchi, Lucas]

Hi Christian,

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

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

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

:(

I really hoped this would work in your case.

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

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


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

agreed

thanks
Lucas De Marchi

Re: [PATCH] i2c: designware: use enable on resume instead initialization

[De Marchi, Lucas]

On Wed, 2015-06-24 at 14:27 +0300, Mika Westerberg wrote:
> On Wed, Jun 24, 2015 at 09:36:43AM +0200, christian.rupp...@alitech.com wrot
> e:
> > Dear Lucas,
> > 
> > Lucas De Marchi  wrote on 23.06.2015 19:02:03:
> > > On Tue, Jun 23, 2015 at 1:45 PM,   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 
MarchiN�r��yb�X��ǧv�^�)޺{.n�+{zX����ܨ}���Ơz�:+v���zZ+��+zf���h���~i���z��w���?�&�)ߢf��^jǫy�m��@A�a���
0��h���i

Re: [PATCH] i2c: designware: use enable on resume instead initialization

[De Marchi, Lucas]

On Wed, 2015-06-24 at 14:27 +0300, Mika Westerberg wrote:
 On Wed, Jun 24, 2015 at 09:36:43AM +0200, christian.rupp...@alitech.com wrot
 e:
  Dear Lucas,
  
  Lucas De Marchi lucas.de.mar...@gmail.com wrote on 23.06.2015 19:02:03:
   On Tue, Jun 23, 2015 at 1:45 PM,  christian.rupp...@alitech.com 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 
MarchiN�r��yb�X��ǧv�^�)޺{.n�+{zX����ܨ}���Ơz�j:+v���zZ+��+zf���h���~i���z��w���?��)ߢf��^jǫy�m��@A�a���
0��h���i