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

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

[Lucas De Marchi]

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"

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(-)

-- 
2.7.4

[PATCH v3 1/3] i2c: designware: wait for disable/enable only if necessary

[Lucas De Marchi]

From: José Roberto de Souza <jose.souza@intel.com>

If we aren't going to continue using the controller we can just disable
it instead of waiting for it to complete. The biggest improvement here
is when a I2C transaction is completed and it doesn't block until
the adapter is disabled. When a new transfer is needed we will disable
and wait for its completion.

This way the adapter will continue changing its state in parallel to the
execution of the thread that requested the I2C transaction saving most
of the time 25~250 usec per I2C transaction.

A simple program doing a register read (1 byte write, 1 byte read)
alternating on 2 different slaves repeated 25k times for each and
measurements taken 4 times we get:

perf stat -r4 chrt -f 10 ./i2c-test /dev/i2c-1 25000 0x40 0x6 0x1e 0x00

Before:
	30.879317977 seconds time elapsed                 ( +- 14.83% )
After:
	8.638705161 seconds time elapsed                  ( +-  5.90% )

Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 drivers/i2c/busses/i2c-designware-core.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
index 99b54be..2c61585 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -252,10 +252,15 @@ static u32 i2c_dw_scl_lcnt(u32 ic_clk, u32 tLOW, u32 tf, int offset)
 
 static void __i2c_dw_enable(struct dw_i2c_dev *dev, bool enable)
 {
+	dw_writel(dev, enable, DW_IC_ENABLE);
+}
+
+static void __i2c_dw_enable_and_wait(struct dw_i2c_dev *dev, bool enable)
+{
 	int timeout = 100;
 
 	do {
-		dw_writel(dev, enable, DW_IC_ENABLE);
+		__i2c_dw_enable(dev, enable);
 		if ((dw_readl(dev, DW_IC_ENABLE_STATUS) & 1) == enable)
 			return;
 
@@ -321,7 +326,7 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
 	}
 
 	/* Disable the adapter */
-	__i2c_dw_enable(dev, false);
+	__i2c_dw_enable_and_wait(dev, false);
 
 	/* set standard and fast speed deviders for high/low periods */
 
@@ -414,7 +419,7 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)
 	u32 ic_con, ic_tar = 0;
 
 	/* Disable the adapter */
-	__i2c_dw_enable(dev, false);
+	__i2c_dw_enable_and_wait(dev, false);
 
 	/* if the slave address is ten bit address, enable 10BITADDR */
 	ic_con = dw_readl(dev, DW_IC_CON);
@@ -833,7 +838,7 @@ tx_aborted:
 void i2c_dw_disable(struct dw_i2c_dev *dev)
 {
 	/* Disable controller */
-	__i2c_dw_enable(dev, false);
+	__i2c_dw_enable_and_wait(dev, false);
 
 	/* Disable all interupts */
 	dw_writel(dev, 0, DW_IC_INTR_MASK);
-- 
2.7.4

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

[Lucas De Marchi]

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 <lucas.demarchi@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 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
+	 * IC_10BITADDR_MASTER field in IC_CON: when it is enabled this field
+	 * is read-only so it should not succeed
+	 */
+	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");
+	}
+
 	if (dev->release_lock)
 		dev->release_lock(dev);
 	return 0;
@@ -416,28 +430,29 @@ static int i2c_dw_wait_bus_not_busy(struct dw_i2c_dev *dev)
 static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)
 {
 	struct i2c_msg *msgs = dev->msgs;
-	u32 ic_con, ic_tar = 0;
+	u32 ic_tar = 0;
 
 	/* Disable the adapter */
 	__i2c_dw_enable_and_wait(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) {
-		ic_con |= DW_IC_CON_10BITADDR_MASTER;
+	if (dev->dynamic_tar_update_enabled) {
 		/*
 		 * If I2C_DYNAMIC_TAR_UPDATE is set, the 10-bit addressing
-		 * mode has to be enabled via bit 12 of IC_TAR register.
-		 * We set it always as I2C_DYNAMIC_TAR_UPDATE can't be
-		 * detected from registers.
+		 * mode has to be enabled via bit 12 of IC_TAR register,
+		 * otherwise bit 4 of IC_CON is used.
 		 */
-		ic_tar = DW_IC_TAR_10BITADDR_MASTER;
+		if (msgs[dev->msg_write_idx].flags & I2C_M_TEN)
+			ic_tar = DW_IC_TAR_10BITADDR_MASTER;
 	} else {
-		ic_con &= ~DW_IC_CON_10BITADDR_MASTER;
+		u32 ic_con = dw_readl(dev, DW_IC_CON);
+		if (msgs[dev->msg_write_idx].flags & I2C_M_TEN)
+			ic_con |= DW_IC_CON_10BITADDR_MASTER;
+		else
+			ic_con &= ~DW_IC_CON_10BITADDR_MASTER;
+		dw_writel(dev, ic_con, DW_IC_CON);
 	}
 
-	dw_writel(dev, ic_con, DW_IC_CON);
-
 	/*
 	 * Set the slave (target) address and enable 10-bit addressing mode
 	 * if applicable.
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index cd409e7..cd6e9ec 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -105,6 +105,7 @@ struct dw_i2c_dev {
 	int			(*acquire_lock)(struct dw_i2c_dev *dev);
 	void			(*release_lock)(struct dw_i2c_dev *dev);
 	bool			pm_runtime_disabled;
+	bool			dynamic_tar_update_enabled;
 };
 
 #define ACCESS_SWAP		0x00000001
-- 
2.7.4

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

[Lucas De Marchi]

Disabling the adapter after each transfer adds additional delays
for each I2C transfer. Even if we don't wait for it to be disabled
anymore, on next transfer we will need to if we have several transfers
in a row.

Now during the transfer init we check if IC_TAR can be changed
dynamically, the status register for no activity and TX buffer being
empty. In this case we don't need to disable it

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.

Alternating register reads on 2 slaves:
perf stat -r4 chrt -f 10 ./i2c-test /dev/i2c-1 25000 0x40 0x6 0x1e 0x00

Before:
	8.638705161 seconds time elapsed                  ( +-  5.90% )
After:
	7.516821591 seconds time elapsed                  ( +-  0.11% )

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/i2c/busses/i2c-designware-core.c | 55 +++++++++++++++++++++-----------
 1 file changed, 37 insertions(+), 18 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
index a8408db..d260689 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -89,7 +89,9 @@
 					 DW_IC_INTR_TX_ABRT | \
 					 DW_IC_INTR_STOP_DET)
 
-#define DW_IC_STATUS_ACTIVITY	0x1
+#define DW_IC_STATUS_ACTIVITY		0x1
+#define DW_IC_STATUS_TFE		BIT(2)
+#define DW_IC_STATUS_MST_ACTIVITY	BIT(5)
 
 #define DW_IC_ERR_TX_ABRT	0x1
 
@@ -431,9 +433,25 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)
 {
 	struct i2c_msg *msgs = dev->msgs;
 	u32 ic_tar = 0;
+	bool enabled;
 
-	/* Disable the adapter */
-	__i2c_dw_enable_and_wait(dev, false);
+	enabled = dw_readl(dev, DW_IC_ENABLE_STATUS) & 1;
+
+	if (enabled) {
+		u32 ic_status;
+
+		/*
+		 * Only disable adapter if ic_tar and ic_con can't be
+		 * dynamically updated
+		 */
+		ic_status = dw_readl(dev, DW_IC_STATUS);
+		if (!dev->dynamic_tar_update_enabled ||
+		    (ic_status & DW_IC_STATUS_MST_ACTIVITY) ||
+		    !(ic_status & DW_IC_STATUS_TFE)) {
+			__i2c_dw_enable_and_wait(dev, false);
+			enabled = false;
+		}
+	}
 
 	/* if the slave address is ten bit address, enable 10BITADDR */
 	if (dev->dynamic_tar_update_enabled) {
@@ -462,8 +480,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 (!enabled)
+		__i2c_dw_enable(dev, true);
 
 	/* Clear and enable interrupts */
 	dw_readl(dev, DW_IC_CLR_INTR);
@@ -644,7 +662,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)
@@ -691,16 +710,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;
@@ -838,9 +847,19 @@ static irqreturn_t i2c_dw_isr(int this_irq, void *dev_id)
 	 */
 
 tx_aborted:
-	if ((stat & (DW_IC_INTR_TX_ABRT | DW_IC_INTR_STOP_DET)) || dev->msg_err)
+	if ((stat & (DW_IC_INTR_TX_ABRT | DW_IC_INTR_STOP_DET))
+			|| dev->msg_err) {
+		/*
+		 * We must disable interruts before returning and signaling
+		 * the end of the current transfer. Otherwise the hardware
+		 * might continue generating interrupts for non-existent
+		 * transfers.
+		 */
+		i2c_dw_disable_int(dev);
+		dw_readl(dev, DW_IC_CLR_INTR);
+
 		complete(&dev->cmd_complete);
-	else if (unlikely(dev->accessor_flags & ACCESS_INTR_MASK)) {
+	} else if (unlikely(dev->accessor_flags & ACCESS_INTR_MASK)) {
 		/* workaround to trigger pending interrupt */
 		stat = dw_readl(dev, DW_IC_INTR_MASK);
 		i2c_dw_disable_int(dev);
-- 
2.7.4

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

[Christian Ruppert]

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 <christian.ruppert@alitech.com>
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!

> 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 <christian.ruppert@alitech.com>
> 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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 240 bytes --]


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

As long as the i2c-list is on cc, they will show up in patchwork and
then I'll have the patches. All fine here.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v3 1/3] i2c: designware: wait for disable/enable only if necessary

[Jarkko Nikula]

Hi, + Wolfram

On 07/29/2016 01:03 AM, Lucas De Marchi wrote:
> From: José Roberto de Souza <jose.souza@intel.com>
>
> If we aren't going to continue using the controller we can just disable
> it instead of waiting for it to complete. The biggest improvement here
> is when a I2C transaction is completed and it doesn't block until
> the adapter is disabled. When a new transfer is needed we will disable
> and wait for its completion.
>
> This way the adapter will continue changing its state in parallel to the
> execution of the thread that requested the I2C transaction saving most
> of the time 25~250 usec per I2C transaction.
>
> A simple program doing a register read (1 byte write, 1 byte read)
> alternating on 2 different slaves repeated 25k times for each and
> measurements taken 4 times we get:
>
> perf stat -r4 chrt -f 10 ./i2c-test /dev/i2c-1 25000 0x40 0x6 0x1e 0x00
>
> Before:
> 	30.879317977 seconds time elapsed                 ( +- 14.83% )
> After:
> 	8.638705161 seconds time elapsed                  ( +-  5.90% )
>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
>  drivers/i2c/busses/i2c-designware-core.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
>
Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

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

[Jarkko Nikula]

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 <lucas.demarchi@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  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.

> +	 */
> +	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?

-- 
Jarkko

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

[Jarkko Nikula]

Hi, + Wolfram

On 07/29/2016 01:03 AM, Lucas De Marchi wrote:
> Disabling the adapter after each transfer adds additional delays
> for each I2C transfer. Even if we don't wait for it to be disabled
> anymore, on next transfer we will need to if we have several transfers
> in a row.
>
> Now during the transfer init we check if IC_TAR can be changed
> dynamically, the status register for no activity and TX buffer being
> empty. In this case we don't need to disable it
>
> 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.
>
> Alternating register reads on 2 slaves:
> perf stat -r4 chrt -f 10 ./i2c-test /dev/i2c-1 25000 0x40 0x6 0x1e 0x00
>
> Before:
> 	8.638705161 seconds time elapsed                  ( +-  5.90% )
> After:
> 	7.516821591 seconds time elapsed                  ( +-  0.11% )
>
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/i2c/busses/i2c-designware-core.c | 55 +++++++++++++++++++++-----------
>  1 file changed, 37 insertions(+), 18 deletions(-)
>
Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

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 <lucas.demarchi@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  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

[Jarkko Nikula]

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.

-- 
Jarkko

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