Linux-Renesas-SoC Archive on lore.kernel.org
 help / Atom feed
* [RFC PATCH 0/4] i2c: core: introduce master_xfer_irqless
@ 2018-09-20 16:14 Wolfram Sang
  2018-09-20 16:14 ` [RFC PATCH 1/4] i2c: core: remove outdated DEBUG output Wolfram Sang
                   ` (6 more replies)
  0 siblings, 7 replies; 27+ messages in thread
From: Wolfram Sang @ 2018-09-20 16:14 UTC (permalink / raw)
  To: linux-i2c
  Cc: linux-renesas-soc, Stefan Lengfeld, preid, linux-omap,
	linux-arm-kernel, Keerthy, Tero Kristo, Grygorii Strashko,
	Andy Shevchenko, Wolfram Sang

To keep the discussion about master_xfer_irqless going, I post here a
draft how I envision the changes to the I2C core. They are only build
tested. I am unsure if I can test them on hardware before next week, so
I'll send them around as RFC already, so people can get an idea and
comment. Maybe Stefan has some bandwidth to test his imx driver
implementation on top of this?

A branch can be found here:

git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/i2c/xfer_irqless

Happy hacking and thanks for all the input to this,

   Wolfram


Wolfram Sang (4):
  i2c: core: remove outdated DEBUG output
  i2c: core: remove level of indentation in i2c_transfer
  i2c: core: use I2C locking behaviour also for SMBUS
  i2c: core: introduce master_xfer_irqless callback

 drivers/i2c/i2c-core-base.c  | 44 +++++++++++++-----------------------
 drivers/i2c/i2c-core-smbus.c |  7 +++++-
 drivers/i2c/i2c-core.h       | 12 ++++++++++
 include/linux/i2c.h          | 10 +++++---
 4 files changed, 41 insertions(+), 32 deletions(-)

-- 
2.18.0

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [RFC PATCH 1/4] i2c: core: remove outdated DEBUG output
  2018-09-20 16:14 [RFC PATCH 0/4] i2c: core: introduce master_xfer_irqless Wolfram Sang
@ 2018-09-20 16:14 ` Wolfram Sang
  2018-09-20 17:23   ` Peter Rosin
  2018-10-05 16:14   ` Wolfram Sang
  2018-09-20 16:14 ` [RFC PATCH 2/4] i2c: core: remove level of indentation in i2c_transfer Wolfram Sang
                   ` (5 subsequent siblings)
  6 siblings, 2 replies; 27+ messages in thread
From: Wolfram Sang @ 2018-09-20 16:14 UTC (permalink / raw)
  To: linux-i2c
  Cc: linux-renesas-soc, Stefan Lengfeld, preid, linux-omap,
	linux-arm-kernel, Keerthy, Tero Kristo, Grygorii Strashko,
	Andy Shevchenko, Wolfram Sang

The usefulness of this debug output is questionable. It covers only
direct i2c_transfer calls and no SMBUS calls, neither direct nor
emulated ones. And the latter one is largely used in the kernel, so a
lot of stuff is missed. Also, we have a proper tracing mechanism for all
these kinds of transfers in place for years now. Remove this old one.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/i2c-core-base.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 9ee9a15e7134..c2b352c46fae 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -1940,16 +1940,6 @@ int i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 	 */
 
 	if (adap->algo->master_xfer) {
-#ifdef DEBUG
-		for (ret = 0; ret < num; ret++) {
-			dev_dbg(&adap->dev,
-				"master_xfer[%d] %c, addr=0x%02x, len=%d%s\n",
-				ret, (msgs[ret].flags & I2C_M_RD) ? 'R' : 'W',
-				msgs[ret].addr, msgs[ret].len,
-				(msgs[ret].flags & I2C_M_RECV_LEN) ? "+" : "");
-		}
-#endif
-
 		if (in_atomic() || irqs_disabled()) {
 			ret = i2c_trylock_bus(adap, I2C_LOCK_SEGMENT);
 			if (!ret)
-- 
2.18.0

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [RFC PATCH 2/4] i2c: core: remove level of indentation in i2c_transfer
  2018-09-20 16:14 [RFC PATCH 0/4] i2c: core: introduce master_xfer_irqless Wolfram Sang
  2018-09-20 16:14 ` [RFC PATCH 1/4] i2c: core: remove outdated DEBUG output Wolfram Sang
@ 2018-09-20 16:14 ` Wolfram Sang
  2018-09-20 17:26   ` Peter Rosin
  2018-10-05 16:15   ` Wolfram Sang
  2018-09-20 16:14 ` [RFC PATCH 3/4] i2c: core: use I2C locking behaviour also for SMBUS Wolfram Sang
                   ` (4 subsequent siblings)
  6 siblings, 2 replies; 27+ messages in thread
From: Wolfram Sang @ 2018-09-20 16:14 UTC (permalink / raw)
  To: linux-i2c
  Cc: linux-renesas-soc, Stefan Lengfeld, preid, linux-omap,
	linux-arm-kernel, Keerthy, Tero Kristo, Grygorii Strashko,
	Andy Shevchenko, Wolfram Sang

Using the common kernel pattern to bail out at the beginning if some
conditions are not met, we can save a level of indentation. No
functional change.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/i2c-core-base.c | 33 ++++++++++++++++-----------------
 1 file changed, 16 insertions(+), 17 deletions(-)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index c2b352c46fae..799776c6d421 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -1922,6 +1922,11 @@ int i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 {
 	int ret;
 
+	if (!adap->algo->master_xfer) {
+		dev_dbg(&adap->dev, "I2C level transfers not supported\n");
+		return -EOPNOTSUPP;
+	}
+
 	/* REVISIT the fault reporting model here is weak:
 	 *
 	 *  - When we get an error after receiving N bytes from a slave,
@@ -1938,25 +1943,19 @@ int i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 	 *    one (discarding status on the second message) or errno
 	 *    (discarding status on the first one).
 	 */
-
-	if (adap->algo->master_xfer) {
-		if (in_atomic() || irqs_disabled()) {
-			ret = i2c_trylock_bus(adap, I2C_LOCK_SEGMENT);
-			if (!ret)
-				/* I2C activity is ongoing. */
-				return -EAGAIN;
-		} else {
-			i2c_lock_bus(adap, I2C_LOCK_SEGMENT);
-		}
-
-		ret = __i2c_transfer(adap, msgs, num);
-		i2c_unlock_bus(adap, I2C_LOCK_SEGMENT);
-
-		return ret;
+	if (in_atomic() || irqs_disabled()) {
+		ret = i2c_trylock_bus(adap, I2C_LOCK_SEGMENT);
+		if (!ret)
+			/* I2C activity is ongoing. */
+			return -EAGAIN;
 	} else {
-		dev_dbg(&adap->dev, "I2C level transfers not supported\n");
-		return -EOPNOTSUPP;
+		i2c_lock_bus(adap, I2C_LOCK_SEGMENT);
 	}
+
+	ret = __i2c_transfer(adap, msgs, num);
+	i2c_unlock_bus(adap, I2C_LOCK_SEGMENT);
+
+	return ret;
 }
 EXPORT_SYMBOL(i2c_transfer);
 
-- 
2.18.0

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [RFC PATCH 3/4] i2c: core: use I2C locking behaviour also for SMBUS
  2018-09-20 16:14 [RFC PATCH 0/4] i2c: core: introduce master_xfer_irqless Wolfram Sang
  2018-09-20 16:14 ` [RFC PATCH 1/4] i2c: core: remove outdated DEBUG output Wolfram Sang
  2018-09-20 16:14 ` [RFC PATCH 2/4] i2c: core: remove level of indentation in i2c_transfer Wolfram Sang
@ 2018-09-20 16:14 ` Wolfram Sang
  2018-09-20 17:31   ` Peter Rosin
  2018-09-20 16:14 ` [RFC PATCH 4/4] i2c: core: introduce master_xfer_irqless callback Wolfram Sang
                   ` (3 subsequent siblings)
  6 siblings, 1 reply; 27+ messages in thread
From: Wolfram Sang @ 2018-09-20 16:14 UTC (permalink / raw)
  To: linux-i2c
  Cc: linux-renesas-soc, Stefan Lengfeld, preid, linux-omap,
	linux-arm-kernel, Keerthy, Tero Kristo, Grygorii Strashko,
	Andy Shevchenko, Wolfram Sang

If I2C transfers are executed in atomic contexts, trylock is used
instead of lock. This behaviour was missing for SMBUS, although a lot of
transfers are of SMBUS type, either emulated or direct. So, factor out
the locking routine into a helper and use it for I2C and SMBUS.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/i2c-core-base.c  | 11 +++--------
 drivers/i2c/i2c-core-smbus.c |  7 ++++++-
 drivers/i2c/i2c-core.h       | 12 ++++++++++++
 3 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 799776c6d421..904b4d2ebefa 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -1943,14 +1943,9 @@ int i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 	 *    one (discarding status on the second message) or errno
 	 *    (discarding status on the first one).
 	 */
-	if (in_atomic() || irqs_disabled()) {
-		ret = i2c_trylock_bus(adap, I2C_LOCK_SEGMENT);
-		if (!ret)
-			/* I2C activity is ongoing. */
-			return -EAGAIN;
-	} else {
-		i2c_lock_bus(adap, I2C_LOCK_SEGMENT);
-	}
+	ret = __i2c_lock_bus_helper(adap);
+	if (ret)
+		return ret;
 
 	ret = __i2c_transfer(adap, msgs, num);
 	i2c_unlock_bus(adap, I2C_LOCK_SEGMENT);
diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
index 9cd66cabb84f..dbb46edb8e02 100644
--- a/drivers/i2c/i2c-core-smbus.c
+++ b/drivers/i2c/i2c-core-smbus.c
@@ -20,6 +20,8 @@
 #include <linux/i2c-smbus.h>
 #include <linux/slab.h>
 
+#include "i2c-core.h"
+
 #define CREATE_TRACE_POINTS
 #include <trace/events/smbus.h>
 
@@ -530,7 +532,10 @@ s32 i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr,
 {
 	s32 res;
 
-	i2c_lock_bus(adapter, I2C_LOCK_SEGMENT);
+	res = __i2c_lock_bus_helper(adapter);
+	if (res)
+		return res;
+
 	res = __i2c_smbus_xfer(adapter, addr, flags, read_write,
 			       command, protocol, data);
 	i2c_unlock_bus(adapter, I2C_LOCK_SEGMENT);
diff --git a/drivers/i2c/i2c-core.h b/drivers/i2c/i2c-core.h
index 37576f50fe20..6e98aa811980 100644
--- a/drivers/i2c/i2c-core.h
+++ b/drivers/i2c/i2c-core.h
@@ -29,6 +29,18 @@ extern int		__i2c_first_dynamic_bus_num;
 
 int i2c_check_7bit_addr_validity_strict(unsigned short addr);
 
+static inline int __i2c_lock_bus_helper(struct i2c_adapter *adap)
+{
+	int ret = 0;
+
+	if (in_atomic() || irqs_disabled())
+		ret = i2c_trylock_bus(adap, I2C_LOCK_SEGMENT) ? 0 : -EAGAIN;
+	else
+		i2c_lock_bus(adap, I2C_LOCK_SEGMENT);
+
+	return ret;
+}
+
 #ifdef CONFIG_ACPI
 const struct acpi_device_id *
 i2c_acpi_match_device(const struct acpi_device_id *matches,
-- 
2.18.0

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [RFC PATCH 4/4] i2c: core: introduce master_xfer_irqless callback
  2018-09-20 16:14 [RFC PATCH 0/4] i2c: core: introduce master_xfer_irqless Wolfram Sang
                   ` (2 preceding siblings ...)
  2018-09-20 16:14 ` [RFC PATCH 3/4] i2c: core: use I2C locking behaviour also for SMBUS Wolfram Sang
@ 2018-09-20 16:14 ` Wolfram Sang
  2018-09-20 17:41   ` Peter Rosin
  2018-10-18 10:44   ` Russell King - ARM Linux
  2018-09-20 22:02 ` [RFC PATCH 0/4] i2c: core: introduce master_xfer_irqless Tony Lindgren
                   ` (2 subsequent siblings)
  6 siblings, 2 replies; 27+ messages in thread
From: Wolfram Sang @ 2018-09-20 16:14 UTC (permalink / raw)
  To: linux-i2c
  Cc: linux-renesas-soc, Stefan Lengfeld, preid, linux-omap,
	linux-arm-kernel, Keerthy, Tero Kristo, Grygorii Strashko,
	Andy Shevchenko, Wolfram Sang

We had the request to access devices very late when interrupts are not
available anymore multiple times now. Mostly to prepare shutdown or
reboot. Allow adapters to specify a specific callback for this case.
Note that we fall back to the generic master_xfer callback if this new
irqless one is not present. This is intentional to preserve the previous
behaviour and avoid regressions. Because there are drivers not using
interrupts or because it might have worked "accidently" before.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/i2c-core-base.c |  6 +++++-
 include/linux/i2c.h         | 10 +++++++---
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 904b4d2ebefa..f827446c3089 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -1887,7 +1887,11 @@ int __i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 	/* Retry automatically on arbitration loss */
 	orig_jiffies = jiffies;
 	for (ret = 0, try = 0; try <= adap->retries; try++) {
-		ret = adap->algo->master_xfer(adap, msgs, num);
+		if ((in_atomic() || irqs_disabled()) && adap->algo->master_xfer_irqless)
+			ret = adap->algo->master_xfer_irqless(adap, msgs, num);
+		else
+			ret = adap->algo->master_xfer(adap, msgs, num);
+
 		if (ret != -EAGAIN)
 			break;
 		if (time_after(jiffies, orig_jiffies + adap->timeout))
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 65b4eaed1d96..11e615123bd0 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -498,6 +498,8 @@ i2c_register_board_info(int busnum, struct i2c_board_info const *info,
  * @master_xfer: Issue a set of i2c transactions to the given I2C adapter
  *   defined by the msgs array, with num messages available to transfer via
  *   the adapter specified by adap.
+ * @master_xfer_irqless: same as master_xfer. Yet, not using any interrupts
+ *   so e.g. PMICs can be accessed very late before shutdown
  * @smbus_xfer: Issue smbus transactions to the given I2C adapter. If this
  *   is not present, then the bus layer will try and convert the SMBus calls
  *   into I2C transfers instead.
@@ -511,9 +513,9 @@ i2c_register_board_info(int busnum, struct i2c_board_info const *info,
  * be addressed using the same bus algorithms - i.e. bit-banging or the PCF8584
  * to name two of the most common.
  *
- * The return codes from the @master_xfer field should indicate the type of
- * error code that occurred during the transfer, as documented in the kernel
- * Documentation file Documentation/i2c/fault-codes.
+ * The return codes from the @master_xfer{_irqless} field should indicate the
+ * type of error code that occurred during the transfer, as documented in the
+ * Kernel Documentation file Documentation/i2c/fault-codes.
  */
 struct i2c_algorithm {
 	/* If an adapter algorithm can't do I2C-level access, set master_xfer
@@ -524,6 +526,8 @@ struct i2c_algorithm {
 	   processed, or a negative value on error */
 	int (*master_xfer)(struct i2c_adapter *adap, struct i2c_msg *msgs,
 			   int num);
+	int (*master_xfer_irqless)(struct i2c_adapter *adap,
+				   struct i2c_msg *msgs, int num);
 	int (*smbus_xfer) (struct i2c_adapter *adap, u16 addr,
 			   unsigned short flags, char read_write,
 			   u8 command, int size, union i2c_smbus_data *data);
-- 
2.18.0

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [RFC PATCH 1/4] i2c: core: remove outdated DEBUG output
  2018-09-20 16:14 ` [RFC PATCH 1/4] i2c: core: remove outdated DEBUG output Wolfram Sang
@ 2018-09-20 17:23   ` Peter Rosin
  2018-10-05 16:14   ` Wolfram Sang
  1 sibling, 0 replies; 27+ messages in thread
From: Peter Rosin @ 2018-09-20 17:23 UTC (permalink / raw)
  To: Wolfram Sang, linux-i2c
  Cc: linux-renesas-soc, Stefan Lengfeld, preid, linux-omap,
	linux-arm-kernel, Keerthy, Tero Kristo, Grygorii Strashko,
	Andy Shevchenko

On 2018-09-20 18:14, Wolfram Sang wrote:
> The usefulness of this debug output is questionable. It covers only
> direct i2c_transfer calls and no SMBUS calls, neither direct nor
> emulated ones. And the latter one is largely used in the kernel, so a
> lot of stuff is missed. Also, we have a proper tracing mechanism for all
> these kinds of transfers in place for years now. Remove this old one.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

This old one fires before locking and even if locking fails for the
atomic/irq case. You might want to mention that in the commit message?
But I certainly agree with the usefulness statement...

Acked-by: Peter Rosin <peda@axentia.se>

> ---
>  drivers/i2c/i2c-core-base.c | 10 ----------
>  1 file changed, 10 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 9ee9a15e7134..c2b352c46fae 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -1940,16 +1940,6 @@ int i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
>  	 */
>  
>  	if (adap->algo->master_xfer) {
> -#ifdef DEBUG
> -		for (ret = 0; ret < num; ret++) {
> -			dev_dbg(&adap->dev,
> -				"master_xfer[%d] %c, addr=0x%02x, len=%d%s\n",
> -				ret, (msgs[ret].flags & I2C_M_RD) ? 'R' : 'W',
> -				msgs[ret].addr, msgs[ret].len,
> -				(msgs[ret].flags & I2C_M_RECV_LEN) ? "+" : "");
> -		}
> -#endif
> -
>  		if (in_atomic() || irqs_disabled()) {
>  			ret = i2c_trylock_bus(adap, I2C_LOCK_SEGMENT);
>  			if (!ret)
> 

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [RFC PATCH 2/4] i2c: core: remove level of indentation in i2c_transfer
  2018-09-20 16:14 ` [RFC PATCH 2/4] i2c: core: remove level of indentation in i2c_transfer Wolfram Sang
@ 2018-09-20 17:26   ` Peter Rosin
  2018-09-20 22:46     ` Wolfram Sang
  2018-10-05 16:15   ` Wolfram Sang
  1 sibling, 1 reply; 27+ messages in thread
From: Peter Rosin @ 2018-09-20 17:26 UTC (permalink / raw)
  To: Wolfram Sang, linux-i2c
  Cc: linux-renesas-soc, Stefan Lengfeld, preid, linux-omap,
	linux-arm-kernel, Keerthy, Tero Kristo, Grygorii Strashko,
	Andy Shevchenko

On 2018-09-20 18:14, Wolfram Sang wrote:
> Using the common kernel pattern to bail out at the beginning if some
> conditions are not met, we can save a level of indentation. No
> functional change.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Yes please!

Acked-by: Peter Rosin <peda@axentia.se>

Cheers,
Peter

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [RFC PATCH 3/4] i2c: core: use I2C locking behaviour also for SMBUS
  2018-09-20 16:14 ` [RFC PATCH 3/4] i2c: core: use I2C locking behaviour also for SMBUS Wolfram Sang
@ 2018-09-20 17:31   ` Peter Rosin
  2018-09-20 22:48     ` Wolfram Sang
  0 siblings, 1 reply; 27+ messages in thread
From: Peter Rosin @ 2018-09-20 17:31 UTC (permalink / raw)
  To: Wolfram Sang, linux-i2c
  Cc: linux-renesas-soc, Stefan Lengfeld, preid, linux-omap,
	linux-arm-kernel, Keerthy, Tero Kristo, Grygorii Strashko,
	Andy Shevchenko

On 2018-09-20 18:14, Wolfram Sang wrote:
> If I2C transfers are executed in atomic contexts, trylock is used
> instead of lock. This behaviour was missing for SMBUS, although a lot of
> transfers are of SMBUS type, either emulated or direct. So, factor out
> the locking routine into a helper and use it for I2C and SMBUS.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Is it ok with static analyzers to "hide" the locking in helpers like
this? Will it not be harder for them to "see" what's going on? But I
don't think we have any annotations anyway, so...

Acked-by: Peter Rosin <peda@axentia.se>


> ---
>  drivers/i2c/i2c-core-base.c  | 11 +++--------
>  drivers/i2c/i2c-core-smbus.c |  7 ++++++-
>  drivers/i2c/i2c-core.h       | 12 ++++++++++++
>  3 files changed, 21 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 799776c6d421..904b4d2ebefa 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -1943,14 +1943,9 @@ int i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
>  	 *    one (discarding status on the second message) or errno
>  	 *    (discarding status on the first one).
>  	 */
> -	if (in_atomic() || irqs_disabled()) {
> -		ret = i2c_trylock_bus(adap, I2C_LOCK_SEGMENT);
> -		if (!ret)
> -			/* I2C activity is ongoing. */
> -			return -EAGAIN;
> -	} else {
> -		i2c_lock_bus(adap, I2C_LOCK_SEGMENT);
> -	}
> +	ret = __i2c_lock_bus_helper(adap);
> +	if (ret)
> +		return ret;
>  
>  	ret = __i2c_transfer(adap, msgs, num);
>  	i2c_unlock_bus(adap, I2C_LOCK_SEGMENT);
> diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
> index 9cd66cabb84f..dbb46edb8e02 100644
> --- a/drivers/i2c/i2c-core-smbus.c
> +++ b/drivers/i2c/i2c-core-smbus.c
> @@ -20,6 +20,8 @@
>  #include <linux/i2c-smbus.h>
>  #include <linux/slab.h>
>  
> +#include "i2c-core.h"
> +
>  #define CREATE_TRACE_POINTS
>  #include <trace/events/smbus.h>
>  
> @@ -530,7 +532,10 @@ s32 i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr,
>  {
>  	s32 res;
>  
> -	i2c_lock_bus(adapter, I2C_LOCK_SEGMENT);
> +	res = __i2c_lock_bus_helper(adapter);
> +	if (res)
> +		return res;
> +
>  	res = __i2c_smbus_xfer(adapter, addr, flags, read_write,
>  			       command, protocol, data);
>  	i2c_unlock_bus(adapter, I2C_LOCK_SEGMENT);
> diff --git a/drivers/i2c/i2c-core.h b/drivers/i2c/i2c-core.h
> index 37576f50fe20..6e98aa811980 100644
> --- a/drivers/i2c/i2c-core.h
> +++ b/drivers/i2c/i2c-core.h
> @@ -29,6 +29,18 @@ extern int		__i2c_first_dynamic_bus_num;
>  
>  int i2c_check_7bit_addr_validity_strict(unsigned short addr);
>  
> +static inline int __i2c_lock_bus_helper(struct i2c_adapter *adap)
> +{
> +	int ret = 0;
> +
> +	if (in_atomic() || irqs_disabled())
> +		ret = i2c_trylock_bus(adap, I2C_LOCK_SEGMENT) ? 0 : -EAGAIN;
> +	else
> +		i2c_lock_bus(adap, I2C_LOCK_SEGMENT);
> +
> +	return ret;
> +}
> +
>  #ifdef CONFIG_ACPI
>  const struct acpi_device_id *
>  i2c_acpi_match_device(const struct acpi_device_id *matches,
> 

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [RFC PATCH 4/4] i2c: core: introduce master_xfer_irqless callback
  2018-09-20 16:14 ` [RFC PATCH 4/4] i2c: core: introduce master_xfer_irqless callback Wolfram Sang
@ 2018-09-20 17:41   ` Peter Rosin
  2018-09-20 22:55     ` Wolfram Sang
  2018-10-18 10:44   ` Russell King - ARM Linux
  1 sibling, 1 reply; 27+ messages in thread
From: Peter Rosin @ 2018-09-20 17:41 UTC (permalink / raw)
  To: Wolfram Sang, linux-i2c
  Cc: linux-renesas-soc, Stefan Lengfeld, preid, linux-omap,
	linux-arm-kernel, Keerthy, Tero Kristo, Grygorii Strashko,
	Andy Shevchenko

On 2018-09-20 18:14, Wolfram Sang wrote:
> We had the request to access devices very late when interrupts are not
> available anymore multiple times now. Mostly to prepare shutdown or

The first sentence is a bit backwards, I'd rephrase like so:

Multiple times now we've had the request to access devices very late, when
interrupts are no longer available.

> reboot. Allow adapters to specify a specific callback for this case.
> Note that we fall back to the generic master_xfer callback if this new
> irqless one is not present. This is intentional to preserve the previous
> behaviour and avoid regressions. Because there are drivers not using
> interrupts or because it might have worked "accidently" before.

accidentally

> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  drivers/i2c/i2c-core-base.c |  6 +++++-
>  include/linux/i2c.h         | 10 +++++++---
>  2 files changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 904b4d2ebefa..f827446c3089 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -1887,7 +1887,11 @@ int __i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
>  	/* Retry automatically on arbitration loss */
>  	orig_jiffies = jiffies;
>  	for (ret = 0, try = 0; try <= adap->retries; try++) {
> -		ret = adap->algo->master_xfer(adap, msgs, num);
> +		if ((in_atomic() || irqs_disabled()) && adap->algo->master_xfer_irqless)
> +			ret = adap->algo->master_xfer_irqless(adap, msgs, num);
> +		else
> +			ret = adap->algo->master_xfer(adap, msgs, num);
> +
>  		if (ret != -EAGAIN)
>  			break;
>  		if (time_after(jiffies, orig_jiffies + adap->timeout))
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index 65b4eaed1d96..11e615123bd0 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -498,6 +498,8 @@ i2c_register_board_info(int busnum, struct i2c_board_info const *info,
>   * @master_xfer: Issue a set of i2c transactions to the given I2C adapter
>   *   defined by the msgs array, with num messages available to transfer via
>   *   the adapter specified by adap.
> + * @master_xfer_irqless: same as master_xfer. Yet, not using any interrupts

"Same" (with capital 'S') to match the other entries. Also, should it
not be @master_xfer to help the tools do the right thing?

> + *   so e.g. PMICs can be accessed very late before shutdown

Trailing period.

I'm fine with this change, but should it not wait until there is a user?
(I think there is one in the wings, so that's a very weak objection...)

Acked-by: Peter Rosin <peda@axentia.se>


>   * @smbus_xfer: Issue smbus transactions to the given I2C adapter. If this
>   *   is not present, then the bus layer will try and convert the SMBus calls
>   *   into I2C transfers instead.
> @@ -511,9 +513,9 @@ i2c_register_board_info(int busnum, struct i2c_board_info const *info,
>   * be addressed using the same bus algorithms - i.e. bit-banging or the PCF8584
>   * to name two of the most common.
>   *
> - * The return codes from the @master_xfer field should indicate the type of
> - * error code that occurred during the transfer, as documented in the kernel
> - * Documentation file Documentation/i2c/fault-codes.
> + * The return codes from the @master_xfer{_irqless} field should indicate the
> + * type of error code that occurred during the transfer, as documented in the
> + * Kernel Documentation file Documentation/i2c/fault-codes.
>   */
>  struct i2c_algorithm {
>  	/* If an adapter algorithm can't do I2C-level access, set master_xfer
> @@ -524,6 +526,8 @@ struct i2c_algorithm {
>  	   processed, or a negative value on error */
>  	int (*master_xfer)(struct i2c_adapter *adap, struct i2c_msg *msgs,
>  			   int num);
> +	int (*master_xfer_irqless)(struct i2c_adapter *adap,
> +				   struct i2c_msg *msgs, int num);
>  	int (*smbus_xfer) (struct i2c_adapter *adap, u16 addr,
>  			   unsigned short flags, char read_write,
>  			   u8 command, int size, union i2c_smbus_data *data);
> 

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [RFC PATCH 0/4] i2c: core: introduce master_xfer_irqless
  2018-09-20 16:14 [RFC PATCH 0/4] i2c: core: introduce master_xfer_irqless Wolfram Sang
                   ` (3 preceding siblings ...)
  2018-09-20 16:14 ` [RFC PATCH 4/4] i2c: core: introduce master_xfer_irqless callback Wolfram Sang
@ 2018-09-20 22:02 ` Tony Lindgren
  2018-09-20 22:56   ` Wolfram Sang
  2018-09-20 23:01 ` Wolfram Sang
  2018-09-23 20:20 ` Stefan Lengfeld
  6 siblings, 1 reply; 27+ messages in thread
From: Tony Lindgren @ 2018-09-20 22:02 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: linux-i2c, linux-renesas-soc, Stefan Lengfeld, preid, linux-omap,
	linux-arm-kernel, Keerthy, Tero Kristo, Grygorii Strashko,
	Andy Shevchenko

* Wolfram Sang <wsa+renesas@sang-engineering.com> [180920 09:19]:
> To keep the discussion about master_xfer_irqless going, I post here a
> draft how I envision the changes to the I2C core. They are only build
> tested. I am unsure if I can test them on hardware before next week, so
> I'll send them around as RFC already, so people can get an idea and
> comment. Maybe Stefan has some bandwidth to test his imx driver
> implementation on top of this?
> 
> A branch can be found here:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/i2c/xfer_irqless
> 
> Happy hacking and thanks for all the input to this,

Thanks looks nice to me:

Acked-by: Tony Lindgren <tony@atomide.com>

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [RFC PATCH 2/4] i2c: core: remove level of indentation in i2c_transfer
  2018-09-20 17:26   ` Peter Rosin
@ 2018-09-20 22:46     ` Wolfram Sang
  0 siblings, 0 replies; 27+ messages in thread
From: Wolfram Sang @ 2018-09-20 22:46 UTC (permalink / raw)
  To: Peter Rosin
  Cc: Wolfram Sang, linux-i2c, linux-renesas-soc, Stefan Lengfeld,
	preid, linux-omap, linux-arm-kernel, Keerthy, Tero Kristo,
	Grygorii Strashko, Andy Shevchenko

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

On Thu, Sep 20, 2018 at 07:26:21PM +0200, Peter Rosin wrote:
> On 2018-09-20 18:14, Wolfram Sang wrote:
> > Using the common kernel pattern to bail out at the beginning if some
> > conditions are not met, we can save a level of indentation. No
> > functional change.
> > 
> > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> Yes please!

:)


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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [RFC PATCH 3/4] i2c: core: use I2C locking behaviour also for SMBUS
  2018-09-20 17:31   ` Peter Rosin
@ 2018-09-20 22:48     ` Wolfram Sang
  0 siblings, 0 replies; 27+ messages in thread
From: Wolfram Sang @ 2018-09-20 22:48 UTC (permalink / raw)
  To: Peter Rosin
  Cc: Wolfram Sang, linux-i2c, linux-renesas-soc, Stefan Lengfeld,
	preid, linux-omap, linux-arm-kernel, Keerthy, Tero Kristo,
	Grygorii Strashko, Andy Shevchenko

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

On Thu, Sep 20, 2018 at 07:31:19PM +0200, Peter Rosin wrote:
> On 2018-09-20 18:14, Wolfram Sang wrote:
> > If I2C transfers are executed in atomic contexts, trylock is used
> > instead of lock. This behaviour was missing for SMBUS, although a lot of
> > transfers are of SMBUS type, either emulated or direct. So, factor out
> > the locking routine into a helper and use it for I2C and SMBUS.
> > 
> > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> Is it ok with static analyzers to "hide" the locking in helpers like
> this? Will it not be harder for them to "see" what's going on? But I
> don't think we have any annotations anyway, so...

Yes, you are right. Yet, I prefer this to open coding the same twice and
have the problem to keep them in sync.


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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [RFC PATCH 4/4] i2c: core: introduce master_xfer_irqless callback
  2018-09-20 17:41   ` Peter Rosin
@ 2018-09-20 22:55     ` Wolfram Sang
  0 siblings, 0 replies; 27+ messages in thread
From: Wolfram Sang @ 2018-09-20 22:55 UTC (permalink / raw)
  To: Peter Rosin
  Cc: Wolfram Sang, linux-i2c, linux-renesas-soc, Stefan Lengfeld,
	preid, linux-omap, linux-arm-kernel, Keerthy, Tero Kristo,
	Grygorii Strashko, Andy Shevchenko

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

On Thu, Sep 20, 2018 at 07:41:05PM +0200, Peter Rosin wrote:
> On 2018-09-20 18:14, Wolfram Sang wrote:
> > We had the request to access devices very late when interrupts are not
> > available anymore multiple times now. Mostly to prepare shutdown or
> 
> The first sentence is a bit backwards, I'd rephrase like so:
> 
> Multiple times now we've had the request to access devices very late, when
> interrupts are no longer available.

Ok. Don't see much difference, but I don't mind.

> > reboot. Allow adapters to specify a specific callback for this case.
> > Note that we fall back to the generic master_xfer callback if this new
> > irqless one is not present. This is intentional to preserve the previous
> > behaviour and avoid regressions. Because there are drivers not using
> > interrupts or because it might have worked "accidently" before.
> 
> accidentally

Thanks.

> > @@ -498,6 +498,8 @@ i2c_register_board_info(int busnum, struct i2c_board_info const *info,
> >   * @master_xfer: Issue a set of i2c transactions to the given I2C adapter
> >   *   defined by the msgs array, with num messages available to transfer via
> >   *   the adapter specified by adap.
> > + * @master_xfer_irqless: same as master_xfer. Yet, not using any interrupts
> 
> "Same" (with capital 'S') to match the other entries. Also, should it
> not be @master_xfer to help the tools do the right thing?

I'll check. You are probably right.

> > + *   so e.g. PMICs can be accessed very late before shutdown
> 
> Trailing period.
> 
> I'm fine with this change, but should it not wait until there is a user?
> (I think there is one in the wings, so that's a very weak objection...)

As I mentioned in the cover-letter, this series is RFC because it is
mainly meant as assistance for Stefan, so he could base his imx patches
on top of it. Or the TI folks for their omap driver.

I somewhen need to implement irqless transfers for the i2c-sh_mobile
driver. But this may take a while, so I hope the others are first. And
yes, I won't apply this series without a user and proper testing.

Thanks for the review, Peter!


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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [RFC PATCH 0/4] i2c: core: introduce master_xfer_irqless
  2018-09-20 22:02 ` [RFC PATCH 0/4] i2c: core: introduce master_xfer_irqless Tony Lindgren
@ 2018-09-20 22:56   ` Wolfram Sang
  2018-10-18 10:35     ` Keerthy
  0 siblings, 1 reply; 27+ messages in thread
From: Wolfram Sang @ 2018-09-20 22:56 UTC (permalink / raw)
  To: Tony Lindgren
  Cc: Wolfram Sang, linux-i2c, linux-renesas-soc, Stefan Lengfeld,
	preid, linux-omap, linux-arm-kernel, Keerthy, Tero Kristo,
	Grygorii Strashko, Andy Shevchenko

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


> Thanks looks nice to me:
> 
> Acked-by: Tony Lindgren <tony@atomide.com>

Thanks, Tony!


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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [RFC PATCH 0/4] i2c: core: introduce master_xfer_irqless
  2018-09-20 16:14 [RFC PATCH 0/4] i2c: core: introduce master_xfer_irqless Wolfram Sang
                   ` (4 preceding siblings ...)
  2018-09-20 22:02 ` [RFC PATCH 0/4] i2c: core: introduce master_xfer_irqless Tony Lindgren
@ 2018-09-20 23:01 ` Wolfram Sang
  2018-09-23 20:20 ` Stefan Lengfeld
  6 siblings, 0 replies; 27+ messages in thread
From: Wolfram Sang @ 2018-09-20 23:01 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: linux-i2c, linux-renesas-soc, Stefan Lengfeld, preid, linux-omap,
	linux-arm-kernel, Keerthy, Tero Kristo, Grygorii Strashko,
	Andy Shevchenko

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

On Thu, Sep 20, 2018 at 06:14:19PM +0200, Wolfram Sang wrote:
> To keep the discussion about master_xfer_irqless going, I post here a
> draft how I envision the changes to the I2C core. They are only build
> tested. I am unsure if I can test them on hardware before next week, so
> I'll send them around as RFC already, so people can get an idea and
> comment. Maybe Stefan has some bandwidth to test his imx driver
> implementation on top of this?
> 
> A branch can be found here:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/i2c/xfer_irqless

What I forgot to mention:

At some point, I'd like an adiitional check if the device is flagged to
have very late I2C transfers and trigger a fat warning if not. To keep
bugs visible.

I have ideas but not perfect yet, so I'll leave this as the next step.


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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [RFC PATCH 0/4] i2c: core: introduce master_xfer_irqless
  2018-09-20 16:14 [RFC PATCH 0/4] i2c: core: introduce master_xfer_irqless Wolfram Sang
                   ` (5 preceding siblings ...)
  2018-09-20 23:01 ` Wolfram Sang
@ 2018-09-23 20:20 ` Stefan Lengfeld
  2018-10-07 15:39   ` [RFC PATCH 0/3] " Stefan Lengfeld
  6 siblings, 1 reply; 27+ messages in thread
From: Stefan Lengfeld @ 2018-09-23 20:20 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: linux-i2c, linux-renesas-soc, preid, linux-omap,
	linux-arm-kernel, Keerthy, Tero Kristo, Grygorii Strashko,
	Andy Shevchenko

Hi Wolfram,

On Thu, Sep 20, 2018 at 06:14:19PM +0200, Wolfram Sang wrote:
> To keep the discussion about master_xfer_irqless going, I post here a
> draft how I envision the changes to the I2C core. They are only build
> tested. I am unsure if I can test them on hardware before next week, so
> I'll send them around as RFC already, so people can get an idea and
> comment. Maybe Stefan has some bandwidth to test his imx driver
> implementation on top of this?

No problem. I have some spare time in the next days and will post my
results to the mailinglist.

Kind Regards,
Stefan Lengfeld

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [RFC PATCH 1/4] i2c: core: remove outdated DEBUG output
  2018-09-20 16:14 ` [RFC PATCH 1/4] i2c: core: remove outdated DEBUG output Wolfram Sang
  2018-09-20 17:23   ` Peter Rosin
@ 2018-10-05 16:14   ` Wolfram Sang
  1 sibling, 0 replies; 27+ messages in thread
From: Wolfram Sang @ 2018-10-05 16:14 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: linux-i2c, linux-renesas-soc, Stefan Lengfeld, preid, linux-omap,
	linux-arm-kernel, Keerthy, Tero Kristo, Grygorii Strashko,
	Andy Shevchenko

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

On Thu, Sep 20, 2018 at 06:14:20PM +0200, Wolfram Sang wrote:
> The usefulness of this debug output is questionable. It covers only
> direct i2c_transfer calls and no SMBUS calls, neither direct nor
> emulated ones. And the latter one is largely used in the kernel, so a
> lot of stuff is missed. Also, we have a proper tracing mechanism for all
> these kinds of transfers in place for years now. Remove this old one.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

I think we can have this clean up already.

Applied to for-next (with updated commit message), thanks!


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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [RFC PATCH 2/4] i2c: core: remove level of indentation in i2c_transfer
  2018-09-20 16:14 ` [RFC PATCH 2/4] i2c: core: remove level of indentation in i2c_transfer Wolfram Sang
  2018-09-20 17:26   ` Peter Rosin
@ 2018-10-05 16:15   ` Wolfram Sang
  1 sibling, 0 replies; 27+ messages in thread
From: Wolfram Sang @ 2018-10-05 16:15 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: linux-i2c, linux-renesas-soc, Stefan Lengfeld, preid, linux-omap,
	linux-arm-kernel, Keerthy, Tero Kristo, Grygorii Strashko,
	Andy Shevchenko

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

On Thu, Sep 20, 2018 at 06:14:21PM +0200, Wolfram Sang wrote:
> Using the common kernel pattern to bail out at the beginning if some
> conditions are not met, we can save a level of indentation. No
> functional change.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

I think we can have this clean up already.

Applied to for-next, thanks!


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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [RFC PATCH 0/3] i2c: core: introduce master_xfer_irqless
  2018-09-23 20:20 ` Stefan Lengfeld
@ 2018-10-07 15:39   ` " Stefan Lengfeld
  2018-10-07 15:39     ` [RFC PATCH 1/3] i2c: imx: implement master_xfer_irqless callback Stefan Lengfeld
                       ` (2 more replies)
  0 siblings, 3 replies; 27+ messages in thread
From: Stefan Lengfeld @ 2018-10-07 15:39 UTC (permalink / raw)
  To: wsa+renesas
  Cc: linux-i2c, linux-renesas-soc, preid, linux-omap,
	linux-arm-kernel, j-keerthy, t-kristo, grygorii.strashko,
	andriy.shevchenko

Hi Wolfram,

I have ported my i.MX6 patches on your patch stack and tested the reboot
handler on my i.MX6 Solo Board.  Works without lockdep and non-atomic
warnings so far.

Kind Regards,
    Stefan


Stefan Christ (1):
  ARM: dts: phyboard-mira-dl: rely on PMIC for reboot and watchdog

Stefan Lengfeld (2):
  i2c: imx: implement master_xfer_irqless callback
  watchdog: da9062: avoid regmap in restart handler

 arch/arm/boot/dts/imx6dl-phytec-mira-rdk-nand.dts |   8 ++
 drivers/i2c/busses/i2c-imx.c                      | 128 ++++++++++++++++------
 drivers/mfd/da9062-core.c                         |   1 +
 drivers/watchdog/da9062_wdt.c                     |  17 ++-
 include/linux/mfd/da9062/core.h                   |   1 +
 5 files changed, 116 insertions(+), 39 deletions(-)

-- 
2.16.4

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [RFC PATCH 1/3] i2c: imx: implement master_xfer_irqless callback
  2018-10-07 15:39   ` [RFC PATCH 0/3] " Stefan Lengfeld
@ 2018-10-07 15:39     ` Stefan Lengfeld
  2018-10-08 10:06       ` Andy Shevchenko
  2018-10-07 15:39     ` [RFC PATCH 2/3] watchdog: da9062: avoid regmap in restart handler Stefan Lengfeld
  2018-10-07 15:39     ` [RFC PATCH 3/3] ARM: dts: phyboard-mira-dl: rely on PMIC for reboot and watchdog Stefan Lengfeld
  2 siblings, 1 reply; 27+ messages in thread
From: Stefan Lengfeld @ 2018-10-07 15:39 UTC (permalink / raw)
  To: wsa+renesas
  Cc: linux-i2c, linux-renesas-soc, preid, linux-omap,
	linux-arm-kernel, j-keerthy, t-kristo, grygorii.strashko,
	andriy.shevchenko

Rework the read and write code paths in the driver to support operation
in IRQ disabled contexts. The patch is currently tested only on a
phyCORE-i.MX6 Solo board.

The driver supports normal operation, DMA transfers and now the polling
mode or also called sleep-free or IRQ-less operation. It makes the code
not simpler or easier to read, but IRQ less I2C transfers are needed on
some hardware configurations, e.g. to trigger reboots on an external
PMIC chip.

Signed-off-by: Stefan Lengfeld <contact@stefanchrist.eu>
---
 drivers/i2c/busses/i2c-imx.c | 128 +++++++++++++++++++++++++++++++------------
 1 file changed, 93 insertions(+), 35 deletions(-)

diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index c406700789e1..af72a1cbedbe 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -404,7 +404,7 @@ static void i2c_imx_dma_free(struct imx_i2c_struct *i2c_imx)
 	dma->chan_using = NULL;
 }
 
-static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx, int for_busy)
+static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx, int for_busy, bool polling)
 {
 	unsigned long orig_jiffies = jiffies;
 	unsigned int temp;
@@ -434,15 +434,39 @@ static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx, int for_busy)
 				"<%s> I2C bus is busy\n", __func__);
 			return -ETIMEDOUT;
 		}
-		schedule();
+		if (!polling)
+			schedule();
+		else
+			udelay(100);
 	}
 
 	return 0;
 }
 
-static int i2c_imx_trx_complete(struct imx_i2c_struct *i2c_imx)
+static int i2c_imx_trx_complete(struct imx_i2c_struct *i2c_imx, bool polling)
 {
-	wait_event_timeout(i2c_imx->queue, i2c_imx->i2csr & I2SR_IIF, HZ / 10);
+	if (!polling) {
+		wait_event_timeout(i2c_imx->queue, i2c_imx->i2csr & I2SR_IIF, HZ / 10);
+	} else {
+		int counter = 0;
+
+		while (1) {
+			unsigned int reg;
+
+			reg = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
+			i2c_imx->i2csr = reg;
+			if (reg & I2SR_IIF)
+				break;
+
+			if (counter > 1000) {
+				dev_err(&i2c_imx->adapter.dev, "<%s> TXR timeout\n", __func__);
+				return -EIO;
+			}
+			udelay(100);
+			counter++;
+		}
+		imx_i2c_write_reg(0, i2c_imx, IMX_I2C_I2SR);
+	}
 
 	if (unlikely(!(i2c_imx->i2csr & I2SR_IIF))) {
 		dev_dbg(&i2c_imx->adapter.dev, "<%s> Timeout\n", __func__);
@@ -520,7 +544,7 @@ static int i2c_imx_clk_notifier_call(struct notifier_block *nb,
 	return NOTIFY_OK;
 }
 
-static int i2c_imx_start(struct imx_i2c_struct *i2c_imx)
+static int i2c_imx_start(struct imx_i2c_struct *i2c_imx, bool polling)
 {
 	unsigned int temp = 0;
 	int result;
@@ -533,23 +557,32 @@ static int i2c_imx_start(struct imx_i2c_struct *i2c_imx)
 	imx_i2c_write_reg(i2c_imx->hwdata->i2cr_ien_opcode, i2c_imx, IMX_I2C_I2CR);
 
 	/* Wait controller to be stable */
-	usleep_range(50, 150);
+	if (!polling)
+		usleep_range(50, 150);
+	else
+		udelay(50);
 
 	/* Start I2C transaction */
 	temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
 	temp |= I2CR_MSTA;
 	imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
-	result = i2c_imx_bus_busy(i2c_imx, 1);
+	result = i2c_imx_bus_busy(i2c_imx, 1, polling);
 	if (result)
 		return result;
 
-	temp |= I2CR_IIEN | I2CR_MTX | I2CR_TXAK;
+	if (!polling) {
+		temp |= I2CR_IIEN | I2CR_MTX | I2CR_TXAK;
+	} else {
+		temp |= I2CR_MTX | I2CR_TXAK;
+		temp &= ~I2CR_IIEN; /* Disable interrupt */
+	}
+
 	temp &= ~I2CR_DMAEN;
 	imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
 	return result;
 }
 
-static void i2c_imx_stop(struct imx_i2c_struct *i2c_imx)
+static void i2c_imx_stop(struct imx_i2c_struct *i2c_imx, bool polling)
 {
 	unsigned int temp = 0;
 
@@ -571,7 +604,7 @@ static void i2c_imx_stop(struct imx_i2c_struct *i2c_imx)
 	}
 
 	if (!i2c_imx->stopped)
-		i2c_imx_bus_busy(i2c_imx, 0);
+		i2c_imx_bus_busy(i2c_imx, 0, polling);
 
 	/* Disable I2C controller */
 	temp = i2c_imx->hwdata->i2cr_ien_opcode ^ I2CR_IEN,
@@ -652,7 +685,7 @@ static int i2c_imx_dma_write(struct imx_i2c_struct *i2c_imx,
 	/* The last data byte must be transferred by the CPU. */
 	imx_i2c_write_reg(msgs->buf[msgs->len-1],
 				i2c_imx, IMX_I2C_I2DR);
-	result = i2c_imx_trx_complete(i2c_imx);
+	result = i2c_imx_trx_complete(i2c_imx, false);
 	if (result)
 		return result;
 
@@ -711,7 +744,7 @@ static int i2c_imx_dma_read(struct imx_i2c_struct *i2c_imx,
 
 	msgs->buf[msgs->len-2] = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR);
 	/* read n byte data */
-	result = i2c_imx_trx_complete(i2c_imx);
+	result = i2c_imx_trx_complete(i2c_imx, false);
 	if (result)
 		return result;
 
@@ -724,7 +757,7 @@ static int i2c_imx_dma_read(struct imx_i2c_struct *i2c_imx,
 		temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
 		temp &= ~(I2CR_MSTA | I2CR_MTX);
 		imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
-		i2c_imx_bus_busy(i2c_imx, 0);
+		i2c_imx_bus_busy(i2c_imx, 0, false);
 	} else {
 		/*
 		 * For i2c master receiver repeat restart operation like:
@@ -742,7 +775,7 @@ static int i2c_imx_dma_read(struct imx_i2c_struct *i2c_imx,
 	return 0;
 }
 
-static int i2c_imx_write(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs)
+static int i2c_imx_write(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs, bool polling)
 {
 	int i, result;
 
@@ -751,7 +784,7 @@ static int i2c_imx_write(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs)
 
 	/* write slave address */
 	imx_i2c_write_reg(i2c_8bit_addr_from_msg(msgs), i2c_imx, IMX_I2C_I2DR);
-	result = i2c_imx_trx_complete(i2c_imx);
+	result = i2c_imx_trx_complete(i2c_imx, polling);
 	if (result)
 		return result;
 	result = i2c_imx_acked(i2c_imx);
@@ -765,7 +798,7 @@ static int i2c_imx_write(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs)
 			"<%s> write byte: B%d=0x%X\n",
 			__func__, i, msgs->buf[i]);
 		imx_i2c_write_reg(msgs->buf[i], i2c_imx, IMX_I2C_I2DR);
-		result = i2c_imx_trx_complete(i2c_imx);
+		result = i2c_imx_trx_complete(i2c_imx, polling);
 		if (result)
 			return result;
 		result = i2c_imx_acked(i2c_imx);
@@ -775,7 +808,7 @@ static int i2c_imx_write(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs)
 	return 0;
 }
 
-static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs, bool is_lastmsg)
+static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs, bool is_lastmsg, bool polling)
 {
 	int i, result;
 	unsigned int temp;
@@ -788,7 +821,7 @@ static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs, bo
 
 	/* write slave address */
 	imx_i2c_write_reg(i2c_8bit_addr_from_msg(msgs), i2c_imx, IMX_I2C_I2DR);
-	result = i2c_imx_trx_complete(i2c_imx);
+	result = i2c_imx_trx_complete(i2c_imx, polling);
 	if (result)
 		return result;
 	result = i2c_imx_acked(i2c_imx);
@@ -821,7 +854,7 @@ static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs, bo
 	for (i = 0; i < msgs->len; i++) {
 		u8 len = 0;
 
-		result = i2c_imx_trx_complete(i2c_imx);
+		result = i2c_imx_trx_complete(i2c_imx, polling);
 		if (result)
 			return result;
 		/*
@@ -849,7 +882,7 @@ static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs, bo
 				temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
 				temp &= ~(I2CR_MSTA | I2CR_MTX);
 				imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
-				i2c_imx_bus_busy(i2c_imx, 0);
+				i2c_imx_bus_busy(i2c_imx, 0, polling);
 			} else {
 				/*
 				 * For i2c master receiver repeat restart operation like:
@@ -880,8 +913,8 @@ static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs, bo
 	return 0;
 }
 
-static int i2c_imx_xfer(struct i2c_adapter *adapter,
-						struct i2c_msg *msgs, int num)
+static int i2c_imx_xfer_impl(struct i2c_adapter *adapter,
+			     struct i2c_msg *msgs, int num, bool polling)
 {
 	unsigned int i, temp;
 	int result;
@@ -895,11 +928,11 @@ static int i2c_imx_xfer(struct i2c_adapter *adapter,
 		goto out;
 
 	/* Start I2C transfer */
-	result = i2c_imx_start(i2c_imx);
+	result = i2c_imx_start(i2c_imx, polling);
 	if (result) {
 		if (i2c_imx->adapter.bus_recovery_info) {
 			i2c_recover_bus(&i2c_imx->adapter);
-			result = i2c_imx_start(i2c_imx);
+			result = i2c_imx_start(i2c_imx, polling);
 		}
 	}
 
@@ -917,7 +950,7 @@ static int i2c_imx_xfer(struct i2c_adapter *adapter,
 			temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
 			temp |= I2CR_RSTA;
 			imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
-			result = i2c_imx_bus_busy(i2c_imx, 1);
+			result = i2c_imx_bus_busy(i2c_imx, 1, polling);
 			if (result)
 				goto fail0;
 		}
@@ -941,13 +974,17 @@ static int i2c_imx_xfer(struct i2c_adapter *adapter,
 			(temp & I2SR_SRW ? 1 : 0), (temp & I2SR_IIF ? 1 : 0),
 			(temp & I2SR_RXAK ? 1 : 0));
 #endif
-		if (msgs[i].flags & I2C_M_RD)
-			result = i2c_imx_read(i2c_imx, &msgs[i], is_lastmsg);
-		else {
-			if (i2c_imx->dma && msgs[i].len >= DMA_THRESHOLD)
-				result = i2c_imx_dma_write(i2c_imx, &msgs[i]);
-			else
-				result = i2c_imx_write(i2c_imx, &msgs[i]);
+		if (msgs[i].flags & I2C_M_RD) {
+			result = i2c_imx_read(i2c_imx, &msgs[i], is_lastmsg, polling);
+		} else {
+			if (!polling) {
+				if (i2c_imx->dma && msgs[i].len >= DMA_THRESHOLD)
+					result = i2c_imx_dma_write(i2c_imx, &msgs[i]);
+				else
+					result = i2c_imx_write(i2c_imx, &msgs[i], polling);
+			} else {
+				result = i2c_imx_write(i2c_imx, &msgs[i], polling);
+			}
 		}
 		if (result)
 			goto fail0;
@@ -955,7 +992,7 @@ static int i2c_imx_xfer(struct i2c_adapter *adapter,
 
 fail0:
 	/* Stop I2C transfer */
-	i2c_imx_stop(i2c_imx);
+	i2c_imx_stop(i2c_imx, polling);
 
 	pm_runtime_mark_last_busy(i2c_imx->adapter.dev.parent);
 	pm_runtime_put_autosuspend(i2c_imx->adapter.dev.parent);
@@ -967,6 +1004,18 @@ static int i2c_imx_xfer(struct i2c_adapter *adapter,
 	return (result < 0) ? result : num;
 }
 
+static int i2c_imx_xfer(struct i2c_adapter *adapter,
+			struct i2c_msg *msgs, int num)
+{
+	return i2c_imx_xfer_impl(adapter, msgs, num, false);
+}
+
+static int i2c_imx_xfer_irqless(struct i2c_adapter *adapter,
+				struct i2c_msg *msgs, int num)
+{
+	return i2c_imx_xfer_impl(adapter, msgs, num, true);
+}
+
 static void i2c_imx_prepare_recovery(struct i2c_adapter *adap)
 {
 	struct imx_i2c_struct *i2c_imx;
@@ -1039,8 +1088,9 @@ static u32 i2c_imx_func(struct i2c_adapter *adapter)
 }
 
 static const struct i2c_algorithm i2c_imx_algo = {
-	.master_xfer	= i2c_imx_xfer,
-	.functionality	= i2c_imx_func,
+	.master_xfer = i2c_imx_xfer,
+	.master_xfer_irqless = i2c_imx_xfer_irqless,
+	.functionality = i2c_imx_func,
 };
 
 static int i2c_imx_probe(struct platform_device *pdev)
@@ -1117,6 +1167,14 @@ static int i2c_imx_probe(struct platform_device *pdev)
 	/* Set up platform driver data */
 	platform_set_drvdata(pdev, i2c_imx);
 
+	/*
+	 * Driver's PM callbacks are safe to be called in IRQ disabled
+	 * contexts. Providing this information to the PM subsystem is required
+	 * for the 'master_xfer_irqless' implementation that calls PM routines
+	 * in IRQ disabled/atomic contexts, too.
+	 */
+	pm_runtime_irq_safe(&pdev->dev);
+
 	pm_runtime_set_autosuspend_delay(&pdev->dev, I2C_PM_TIMEOUT);
 	pm_runtime_use_autosuspend(&pdev->dev);
 	pm_runtime_set_active(&pdev->dev);
-- 
2.16.4

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [RFC PATCH 2/3] watchdog: da9062: avoid regmap in restart handler
  2018-10-07 15:39   ` [RFC PATCH 0/3] " Stefan Lengfeld
  2018-10-07 15:39     ` [RFC PATCH 1/3] i2c: imx: implement master_xfer_irqless callback Stefan Lengfeld
@ 2018-10-07 15:39     ` Stefan Lengfeld
  2018-10-08 10:07       ` Andy Shevchenko
  2018-10-07 15:39     ` [RFC PATCH 3/3] ARM: dts: phyboard-mira-dl: rely on PMIC for reboot and watchdog Stefan Lengfeld
  2 siblings, 1 reply; 27+ messages in thread
From: Stefan Lengfeld @ 2018-10-07 15:39 UTC (permalink / raw)
  To: wsa+renesas
  Cc: linux-i2c, linux-renesas-soc, preid, linux-omap,
	linux-arm-kernel, j-keerthy, t-kristo, grygorii.strashko,
	andriy.shevchenko

Using i2c_transfer() directly to set the shutdown bit is more reliable
than using regmap in atomic contexts, because calls to 'schedule()' or
'sleep()' must be avoided in call code paths.

Tested on a phyCORE-i.MX6 Solo board.

Signed-off-by: Stefan Lengfeld <contact@stefanchrist.eu>
---
 drivers/mfd/da9062-core.c       |  1 +
 drivers/watchdog/da9062_wdt.c   | 17 +++++++++++++----
 include/linux/mfd/da9062/core.h |  1 +
 3 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/mfd/da9062-core.c b/drivers/mfd/da9062-core.c
index 9f6105906c09..b6a01054ba0f 100644
--- a/drivers/mfd/da9062-core.c
+++ b/drivers/mfd/da9062-core.c
@@ -609,6 +609,7 @@ static int da9062_i2c_probe(struct i2c_client *i2c,
 
 	i2c_set_clientdata(i2c, chip);
 	chip->dev = &i2c->dev;
+	chip->i2c = i2c;
 
 	if (!i2c->irq) {
 		dev_err(chip->dev, "No IRQ configured\n");
diff --git a/drivers/watchdog/da9062_wdt.c b/drivers/watchdog/da9062_wdt.c
index fe169d8e1fb2..ad6483d25f83 100644
--- a/drivers/watchdog/da9062_wdt.c
+++ b/drivers/watchdog/da9062_wdt.c
@@ -11,6 +11,7 @@
 #include <linux/platform_device.h>
 #include <linux/uaccess.h>
 #include <linux/slab.h>
+#include <linux/i2c.h>
 #include <linux/delay.h>
 #include <linux/jiffies.h>
 #include <linux/mfd/da9062/registers.h>
@@ -152,12 +153,20 @@ static int da9062_wdt_restart(struct watchdog_device *wdd, unsigned long action,
 			      void *data)
 {
 	struct da9062_watchdog *wdt = watchdog_get_drvdata(wdd);
+	struct i2c_client *client = wdt->hw->i2c;
+	__u8 buf[3] = {DA9062AA_CONTROL_F, DA9062AA_SHUTDOWN_MASK, 0x0};
+	struct i2c_msg msgs[1] = {
+		{
+			.addr = client->addr,
+			.flags = (client->flags & I2C_M_TEN),
+			.len = sizeof(buf),
+			.buf = buf,
+		}
+	};
 	int ret;
 
-	ret = regmap_write(wdt->hw->regmap,
-			   DA9062AA_CONTROL_F,
-			   DA9062AA_SHUTDOWN_MASK);
-	if (ret)
+	ret = i2c_transfer(client->adapter, msgs, sizeof(msgs));
+	if (ret < 0)
 		dev_alert(wdt->hw->dev, "Failed to shutdown (err = %d)\n",
 			  ret);
 
diff --git a/include/linux/mfd/da9062/core.h b/include/linux/mfd/da9062/core.h
index 74d33a01ddae..c994293b3aef 100644
--- a/include/linux/mfd/da9062/core.h
+++ b/include/linux/mfd/da9062/core.h
@@ -68,6 +68,7 @@ enum da9062_irqs {
 struct da9062 {
 	struct device *dev;
 	struct regmap *regmap;
+	struct i2c_client *i2c;
 	struct regmap_irq_chip_data *regmap_irq;
 	enum da9062_compatible_types chip_type;
 };
-- 
2.16.4

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [RFC PATCH 3/3] ARM: dts: phyboard-mira-dl: rely on PMIC for reboot and watchdog
  2018-10-07 15:39   ` [RFC PATCH 0/3] " Stefan Lengfeld
  2018-10-07 15:39     ` [RFC PATCH 1/3] i2c: imx: implement master_xfer_irqless callback Stefan Lengfeld
  2018-10-07 15:39     ` [RFC PATCH 2/3] watchdog: da9062: avoid regmap in restart handler Stefan Lengfeld
@ 2018-10-07 15:39     ` Stefan Lengfeld
  2 siblings, 0 replies; 27+ messages in thread
From: Stefan Lengfeld @ 2018-10-07 15:39 UTC (permalink / raw)
  To: wsa+renesas
  Cc: linux-i2c, linux-renesas-soc, preid, linux-omap,
	linux-arm-kernel, j-keerthy, t-kristo, grygorii.strashko,
	andriy.shevchenko

From: Stefan Christ <s.christ@phytec.de>

Rely on PMIC watchdog and reboot support. The i.MX6 internal watchdog
cannot be used, because it does not reset external PMIC voltages on
reset.

Signed-off-by: Stefan Christ <s.christ@phytec.de>
Signed-off-by: Christian Hemp <c.hemp@phytec.de>
---
 arch/arm/boot/dts/imx6dl-phytec-mira-rdk-nand.dts | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/boot/dts/imx6dl-phytec-mira-rdk-nand.dts b/arch/arm/boot/dts/imx6dl-phytec-mira-rdk-nand.dts
index 9f7f9f98139d..04e2d5a1b998 100644
--- a/arch/arm/boot/dts/imx6dl-phytec-mira-rdk-nand.dts
+++ b/arch/arm/boot/dts/imx6dl-phytec-mira-rdk-nand.dts
@@ -62,3 +62,11 @@
 &usdhc1 {
 	status = "okay";
 };
+
+&wdog1 {
+	/*
+	 * Rely on PMIC reboot handler. Internal i.MX6 watchdog, that is also
+	 * used for reboot, does not reset all external PMIC voltages on reset.
+	 */
+	status = "disabled";
+};
-- 
2.16.4

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [RFC PATCH 1/3] i2c: imx: implement master_xfer_irqless callback
  2018-10-07 15:39     ` [RFC PATCH 1/3] i2c: imx: implement master_xfer_irqless callback Stefan Lengfeld
@ 2018-10-08 10:06       ` Andy Shevchenko
  0 siblings, 0 replies; 27+ messages in thread
From: Andy Shevchenko @ 2018-10-08 10:06 UTC (permalink / raw)
  To: Stefan Lengfeld
  Cc: wsa+renesas, linux-i2c, linux-renesas-soc, preid, linux-omap,
	linux-arm-kernel, j-keerthy, t-kristo, grygorii.strashko

On Sun, Oct 07, 2018 at 05:39:35PM +0200, Stefan Lengfeld wrote:
> Rework the read and write code paths in the driver to support operation
> in IRQ disabled contexts. The patch is currently tested only on a
> phyCORE-i.MX6 Solo board.
> 
> The driver supports normal operation, DMA transfers and now the polling
> mode or also called sleep-free or IRQ-less operation. It makes the code
> not simpler or easier to read, but IRQ less I2C transfers are needed on
> some hardware configurations, e.g. to trigger reboots on an external
> PMIC chip.

> +		if (!polling)
> +			schedule();
> +		else
> +			udelay(100);

What the point to use negative conditional when it's pretty straightforward and
positive may be used?

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [RFC PATCH 2/3] watchdog: da9062: avoid regmap in restart handler
  2018-10-07 15:39     ` [RFC PATCH 2/3] watchdog: da9062: avoid regmap in restart handler Stefan Lengfeld
@ 2018-10-08 10:07       ` Andy Shevchenko
  0 siblings, 0 replies; 27+ messages in thread
From: Andy Shevchenko @ 2018-10-08 10:07 UTC (permalink / raw)
  To: Stefan Lengfeld
  Cc: wsa+renesas, linux-i2c, linux-renesas-soc, preid, linux-omap,
	linux-arm-kernel, j-keerthy, t-kristo, grygorii.strashko

On Sun, Oct 07, 2018 at 05:39:36PM +0200, Stefan Lengfeld wrote:
> Using i2c_transfer() directly to set the shutdown bit is more reliable
> than using regmap in atomic contexts, because calls to 'schedule()' or
> 'sleep()' must be avoided in call code paths.

>  #include <linux/platform_device.h>
>  #include <linux/uaccess.h>
>  #include <linux/slab.h>
> +#include <linux/i2c.h>
>  #include <linux/delay.h>
>  #include <linux/jiffies.h>
>  #include <linux/mfd/da9062/registers.h>

The rule of thumb when extending a header inclusion block is that try to
squeeze the new inclusion in the longest sorted part of the list.

Rationale behind is easy maintenance.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [RFC PATCH 0/4] i2c: core: introduce master_xfer_irqless
  2018-09-20 22:56   ` Wolfram Sang
@ 2018-10-18 10:35     ` Keerthy
  0 siblings, 0 replies; 27+ messages in thread
From: Keerthy @ 2018-10-18 10:35 UTC (permalink / raw)
  To: Wolfram Sang, Tony Lindgren
  Cc: Wolfram Sang, linux-i2c, linux-renesas-soc, Stefan Lengfeld,
	preid, linux-omap, linux-arm-kernel, Tero Kristo,
	Grygorii Strashko, Andy Shevchenko



On Friday 21 September 2018 04:26 AM, Wolfram Sang wrote:
> 
>> Thanks looks nice to me:
>>
>> Acked-by: Tony Lindgren <tony@atomide.com>
> 
> Thanks, Tony!

Hi Wolfram Sang,

Posted: https://lore.kernel.org/patchwork/patch/1001457/

Thanks for the series! I tested for poweroff on am572x-idk.

Tested-by: Keerthy <j-keerthy@ti.com>

Regards,
Keerthy
> 

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [RFC PATCH 4/4] i2c: core: introduce master_xfer_irqless callback
  2018-09-20 16:14 ` [RFC PATCH 4/4] i2c: core: introduce master_xfer_irqless callback Wolfram Sang
  2018-09-20 17:41   ` Peter Rosin
@ 2018-10-18 10:44   ` Russell King - ARM Linux
  2019-02-09 18:03     ` Wolfram Sang
  1 sibling, 1 reply; 27+ messages in thread
From: Russell King - ARM Linux @ 2018-10-18 10:44 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: linux-i2c, Tero Kristo, preid, Keerthy, Andy Shevchenko,
	linux-renesas-soc, Grygorii Strashko, Stefan Lengfeld,
	linux-omap, linux-arm-kernel

On Thu, Sep 20, 2018 at 06:14:23PM +0200, Wolfram Sang wrote:
> We had the request to access devices very late when interrupts are not
> available anymore multiple times now. Mostly to prepare shutdown or
> reboot. Allow adapters to specify a specific callback for this case.
> Note that we fall back to the generic master_xfer callback if this new
> irqless one is not present. This is intentional to preserve the previous
> behaviour and avoid regressions. Because there are drivers not using
> interrupts or because it might have worked "accidently" before.

It's not really about "irqless", it's about being in atomic context.
irqless makes it sound like you may sleep, but the reality is sleeping
is also out (the scheduler needs IRQs to do it's job.)

So, it may be tempting to use things like msleep() in "irqless" but
that's not permissable.  So surely "atomic" would be a better name for
this?

Also, how does this get around the issue which I pointed out with (eg)
an oops occuring, which leads to a panic followed by an attempt to
reboot if the I2C bus in question is already mid-transaction?  Won't
we deadlock?

> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  drivers/i2c/i2c-core-base.c |  6 +++++-
>  include/linux/i2c.h         | 10 +++++++---
>  2 files changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 904b4d2ebefa..f827446c3089 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -1887,7 +1887,11 @@ int __i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
>  	/* Retry automatically on arbitration loss */
>  	orig_jiffies = jiffies;
>  	for (ret = 0, try = 0; try <= adap->retries; try++) {
> -		ret = adap->algo->master_xfer(adap, msgs, num);
> +		if ((in_atomic() || irqs_disabled()) && adap->algo->master_xfer_irqless)
> +			ret = adap->algo->master_xfer_irqless(adap, msgs, num);
> +		else
> +			ret = adap->algo->master_xfer(adap, msgs, num);
> +
>  		if (ret != -EAGAIN)
>  			break;
>  		if (time_after(jiffies, orig_jiffies + adap->timeout))
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index 65b4eaed1d96..11e615123bd0 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -498,6 +498,8 @@ i2c_register_board_info(int busnum, struct i2c_board_info const *info,
>   * @master_xfer: Issue a set of i2c transactions to the given I2C adapter
>   *   defined by the msgs array, with num messages available to transfer via
>   *   the adapter specified by adap.
> + * @master_xfer_irqless: same as master_xfer. Yet, not using any interrupts
> + *   so e.g. PMICs can be accessed very late before shutdown
>   * @smbus_xfer: Issue smbus transactions to the given I2C adapter. If this
>   *   is not present, then the bus layer will try and convert the SMBus calls
>   *   into I2C transfers instead.
> @@ -511,9 +513,9 @@ i2c_register_board_info(int busnum, struct i2c_board_info const *info,
>   * be addressed using the same bus algorithms - i.e. bit-banging or the PCF8584
>   * to name two of the most common.
>   *
> - * The return codes from the @master_xfer field should indicate the type of
> - * error code that occurred during the transfer, as documented in the kernel
> - * Documentation file Documentation/i2c/fault-codes.
> + * The return codes from the @master_xfer{_irqless} field should indicate the
> + * type of error code that occurred during the transfer, as documented in the
> + * Kernel Documentation file Documentation/i2c/fault-codes.
>   */
>  struct i2c_algorithm {
>  	/* If an adapter algorithm can't do I2C-level access, set master_xfer
> @@ -524,6 +526,8 @@ struct i2c_algorithm {
>  	   processed, or a negative value on error */
>  	int (*master_xfer)(struct i2c_adapter *adap, struct i2c_msg *msgs,
>  			   int num);
> +	int (*master_xfer_irqless)(struct i2c_adapter *adap,
> +				   struct i2c_msg *msgs, int num);
>  	int (*smbus_xfer) (struct i2c_adapter *adap, u16 addr,
>  			   unsigned short flags, char read_write,
>  			   u8 command, int size, union i2c_smbus_data *data);
> -- 
> 2.18.0
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [RFC PATCH 4/4] i2c: core: introduce master_xfer_irqless callback
  2018-10-18 10:44   ` Russell King - ARM Linux
@ 2019-02-09 18:03     ` Wolfram Sang
  0 siblings, 0 replies; 27+ messages in thread
From: Wolfram Sang @ 2019-02-09 18:03 UTC (permalink / raw)
  To: Russell King - ARM Linux
  Cc: Wolfram Sang, linux-i2c, Tero Kristo, preid, Keerthy,
	Andy Shevchenko, linux-renesas-soc, Grygorii Strashko,
	Stefan Lengfeld, linux-omap, linux-arm-kernel

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

Hi Russell,

I know this has been quite a while. I didn't have further time for this
topic back then. But now I am going to revive and rework it. Still,
thank you for your input.

On Thu, Oct 18, 2018 at 11:44:46AM +0100, Russell King - ARM Linux wrote:
> On Thu, Sep 20, 2018 at 06:14:23PM +0200, Wolfram Sang wrote:
> > We had the request to access devices very late when interrupts are not
> > available anymore multiple times now. Mostly to prepare shutdown or
> > reboot. Allow adapters to specify a specific callback for this case.
> > Note that we fall back to the generic master_xfer callback if this new
> > irqless one is not present. This is intentional to preserve the previous
> > behaviour and avoid regressions. Because there are drivers not using
> > interrupts or because it might have worked "accidently" before.
> 
> It's not really about "irqless", it's about being in atomic context.
> irqless makes it sound like you may sleep, but the reality is sleeping
> is also out (the scheduler needs IRQs to do it's job.)
> 
> So, it may be tempting to use things like msleep() in "irqless" but
> that's not permissable.  So surely "atomic" would be a better name for
> this?

Full ack. It should be 'master_xfer_atomic'.

> Also, how does this get around the issue which I pointed out with (eg)
> an oops occuring, which leads to a panic followed by an attempt to
> reboot if the I2C bus in question is already mid-transaction?  Won't
> we deadlock?

Interesting question with multiple aspects:

- The above setting will cause problems but this is orthogonal to this
  patch. It works by modifying __i2c_transfer() but all the locking logic
  is one layer above in i2c_transfer(). So, there shouldn't be a
  difference.

- We won't deadlock, because I2C core will use trylock in this case.
  However, even reporting -EAGAIN won't be helpful because the
  interrupted transfer is likely to never finish without irqs. So,
  retrying won't help and we are still stuck.

- Brainstorming: we could decide that if in_atomic() and
  master_xfer_atomic() present and the I2C client is allowed to do
  ATOMIC_IO (and maybe more checks), to ignore the locking and force
  this command on the bus. Also requiring master_xfer_atomic() to reset
  the IP core properly, etc. Probably also enforcing i2c_recover_bus()
  to ensure a free bus. This is probably the best we could do, but
  still no guarantees here. Some I2C devices just lock up when
  interrupted at the wrong time.

Relying on I2C for poweroff/reboot is a somewhat fragile design to begin
with. I will re-start this series with something simple, but keep the
above scenario in mind. Then, we can see how much we can do, hopefully.

And I got the idea for a panic-fault-injector which sounds interesting.
I will try that!

Kind regards,

   Wolfram

> 
> > 
> > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> > ---
> >  drivers/i2c/i2c-core-base.c |  6 +++++-
> >  include/linux/i2c.h         | 10 +++++++---
> >  2 files changed, 12 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> > index 904b4d2ebefa..f827446c3089 100644
> > --- a/drivers/i2c/i2c-core-base.c
> > +++ b/drivers/i2c/i2c-core-base.c
> > @@ -1887,7 +1887,11 @@ int __i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
> >  	/* Retry automatically on arbitration loss */
> >  	orig_jiffies = jiffies;
> >  	for (ret = 0, try = 0; try <= adap->retries; try++) {
> > -		ret = adap->algo->master_xfer(adap, msgs, num);
> > +		if ((in_atomic() || irqs_disabled()) && adap->algo->master_xfer_irqless)
> > +			ret = adap->algo->master_xfer_irqless(adap, msgs, num);
> > +		else
> > +			ret = adap->algo->master_xfer(adap, msgs, num);
> > +
> >  		if (ret != -EAGAIN)
> >  			break;
> >  		if (time_after(jiffies, orig_jiffies + adap->timeout))
> > diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> > index 65b4eaed1d96..11e615123bd0 100644
> > --- a/include/linux/i2c.h
> > +++ b/include/linux/i2c.h
> > @@ -498,6 +498,8 @@ i2c_register_board_info(int busnum, struct i2c_board_info const *info,
> >   * @master_xfer: Issue a set of i2c transactions to the given I2C adapter
> >   *   defined by the msgs array, with num messages available to transfer via
> >   *   the adapter specified by adap.
> > + * @master_xfer_irqless: same as master_xfer. Yet, not using any interrupts
> > + *   so e.g. PMICs can be accessed very late before shutdown
> >   * @smbus_xfer: Issue smbus transactions to the given I2C adapter. If this
> >   *   is not present, then the bus layer will try and convert the SMBus calls
> >   *   into I2C transfers instead.
> > @@ -511,9 +513,9 @@ i2c_register_board_info(int busnum, struct i2c_board_info const *info,
> >   * be addressed using the same bus algorithms - i.e. bit-banging or the PCF8584
> >   * to name two of the most common.
> >   *
> > - * The return codes from the @master_xfer field should indicate the type of
> > - * error code that occurred during the transfer, as documented in the kernel
> > - * Documentation file Documentation/i2c/fault-codes.
> > + * The return codes from the @master_xfer{_irqless} field should indicate the
> > + * type of error code that occurred during the transfer, as documented in the
> > + * Kernel Documentation file Documentation/i2c/fault-codes.
> >   */
> >  struct i2c_algorithm {
> >  	/* If an adapter algorithm can't do I2C-level access, set master_xfer
> > @@ -524,6 +526,8 @@ struct i2c_algorithm {
> >  	   processed, or a negative value on error */
> >  	int (*master_xfer)(struct i2c_adapter *adap, struct i2c_msg *msgs,
> >  			   int num);
> > +	int (*master_xfer_irqless)(struct i2c_adapter *adap,
> > +				   struct i2c_msg *msgs, int num);
> >  	int (*smbus_xfer) (struct i2c_adapter *adap, u16 addr,
> >  			   unsigned short flags, char read_write,
> >  			   u8 command, int size, union i2c_smbus_data *data);
> > -- 
> > 2.18.0
> > 
> > 
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 
> -- 
> RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
> According to speedtest.net: 11.9Mbps down 500kbps up

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

end of thread, back to index

Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-20 16:14 [RFC PATCH 0/4] i2c: core: introduce master_xfer_irqless Wolfram Sang
2018-09-20 16:14 ` [RFC PATCH 1/4] i2c: core: remove outdated DEBUG output Wolfram Sang
2018-09-20 17:23   ` Peter Rosin
2018-10-05 16:14   ` Wolfram Sang
2018-09-20 16:14 ` [RFC PATCH 2/4] i2c: core: remove level of indentation in i2c_transfer Wolfram Sang
2018-09-20 17:26   ` Peter Rosin
2018-09-20 22:46     ` Wolfram Sang
2018-10-05 16:15   ` Wolfram Sang
2018-09-20 16:14 ` [RFC PATCH 3/4] i2c: core: use I2C locking behaviour also for SMBUS Wolfram Sang
2018-09-20 17:31   ` Peter Rosin
2018-09-20 22:48     ` Wolfram Sang
2018-09-20 16:14 ` [RFC PATCH 4/4] i2c: core: introduce master_xfer_irqless callback Wolfram Sang
2018-09-20 17:41   ` Peter Rosin
2018-09-20 22:55     ` Wolfram Sang
2018-10-18 10:44   ` Russell King - ARM Linux
2019-02-09 18:03     ` Wolfram Sang
2018-09-20 22:02 ` [RFC PATCH 0/4] i2c: core: introduce master_xfer_irqless Tony Lindgren
2018-09-20 22:56   ` Wolfram Sang
2018-10-18 10:35     ` Keerthy
2018-09-20 23:01 ` Wolfram Sang
2018-09-23 20:20 ` Stefan Lengfeld
2018-10-07 15:39   ` [RFC PATCH 0/3] " Stefan Lengfeld
2018-10-07 15:39     ` [RFC PATCH 1/3] i2c: imx: implement master_xfer_irqless callback Stefan Lengfeld
2018-10-08 10:06       ` Andy Shevchenko
2018-10-07 15:39     ` [RFC PATCH 2/3] watchdog: da9062: avoid regmap in restart handler Stefan Lengfeld
2018-10-08 10:07       ` Andy Shevchenko
2018-10-07 15:39     ` [RFC PATCH 3/3] ARM: dts: phyboard-mira-dl: rely on PMIC for reboot and watchdog Stefan Lengfeld

Linux-Renesas-SoC Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-renesas-soc/0 linux-renesas-soc/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-renesas-soc linux-renesas-soc/ https://lore.kernel.org/linux-renesas-soc \
		linux-renesas-soc@vger.kernel.org linux-renesas-soc@archiver.kernel.org
	public-inbox-index linux-renesas-soc


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-renesas-soc


AGPL code for this site: git clone https://public-inbox.org/ public-inbox