[PATCHv2 0/4] Regmap IRQ fix and related changes CPCAP

[PATCHv2 0/4] Regmap IRQ fix and related changes CPCAP

[Tony Lindgren]

Hi all,

Here's v2 set of fixes to make CPCAP PMIC interrupts work reliably when
used with multiple drivers. While working on the ADC, charger and
USB PHY drivers I noticed that the PMIC interrupt to the SoC would
eventually stop working.

All these can wait for v4.12 merge window as these issues don't show
up currently. Two of the patches are also harmless currently, so I've
kept them in this series to avoid pointless merge conflicts.

Regards,

Tony

Changes since v1:

- Updated regap-irq patch to use out_runtime_put in regmap_irq_thread
  also if pm_runtime_get() fails

- Clarify patch description for regmap-irq changes to make clear
  this is an issue with the CPCAP PMIC and not SoC GPIO edge/level
  handling based on comments from Charles Keepax
  <ckeepax@opensource.wolfsonmicro.com>

- Collected acks

Tony Lindgren (4):
  regmap: irq: Fix lost interrupts by introducing handle_reread
  mfd: cpcap: Use handle_reread flag for interrupts
  mfd: cpcap: Use ack_invert interrupts
  mfd: cpcap: Fix bad use of IRQ sense register

 drivers/base/regmap/regmap-irq.c | 77 +++++++++++++++++++++++++++-------------
 drivers/mfd/motorola-cpcap.c     |  7 +++-
 include/linux/regmap.h           |  2 ++
 3 files changed, 61 insertions(+), 25 deletions(-)

-- 
2.11.1

[PATCH 1/4] regmap: irq: Fix lost interrupts by introducing handle_reread

[Tony Lindgren]

At least Motorola CPCAP PMIC needs it's device interrupts re-read
until there are no more interrupts. Otherwise the PMIC interrupt to
the SoC will eventually stop toggling. This seems to be a bug in the
CPCAP PMIC where it can stop driving the GPIO interrupt to the SoC
with pending CPCAP interrupts.

Let's allow handling issues like this by introducing a flag for
handle_reread and by splitting regmap_irq_thread() into two separate
functions for regmap_read_irq_status() and regmap_irq_handle_pending().

Cc: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
Cc: Lee Jones <lee.jones@linaro.org>
Cc: Marcel Partap <mpartap@gmx.net>
Cc: Michael Scott <michael.scott@linaro.org>
Tested-by: Sebastian Reichel <sre@kernel.org>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/base/regmap/regmap-irq.c | 77 +++++++++++++++++++++++++++-------------
 include/linux/regmap.h           |  2 ++
 2 files changed, 55 insertions(+), 24 deletions(-)

diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c
--- a/drivers/base/regmap/regmap-irq.c
+++ b/drivers/base/regmap/regmap-irq.c
@@ -259,27 +259,11 @@ static const struct irq_chip regmap_irq_chip = {
 	.irq_set_wake		= regmap_irq_set_wake,
 };
 
-static irqreturn_t regmap_irq_thread(int irq, void *d)
+static int regmap_read_irq_status(struct regmap_irq_chip_data *data)
 {
-	struct regmap_irq_chip_data *data = d;
 	const struct regmap_irq_chip *chip = data->chip;
 	struct regmap *map = data->map;
 	int ret, i;
-	bool handled = false;
-	u32 reg;
-
-	if (chip->handle_pre_irq)
-		chip->handle_pre_irq(chip->irq_drv_data);
-
-	if (chip->runtime_pm) {
-		ret = pm_runtime_get_sync(map->dev);
-		if (ret < 0) {
-			dev_err(map->dev, "IRQ thread failed to resume: %d\n",
-				ret);
-			pm_runtime_put(map->dev);
-			goto exit;
-		}
-	}
 
 	/*
 	 * Read in the statuses, using a single bulk read if possible
@@ -299,7 +283,7 @@ static irqreturn_t regmap_irq_thread(int irq, void *d)
 		if (ret != 0) {
 			dev_err(map->dev, "Failed to read IRQ status: %d\n",
 				ret);
-			goto exit;
+			return ret;
 		}
 
 		for (i = 0; i < data->chip->num_regs; i++) {
@@ -315,7 +299,7 @@ static irqreturn_t regmap_irq_thread(int irq, void *d)
 				break;
 			default:
 				BUG();
-				goto exit;
+				return ret;
 			}
 		}
 
@@ -330,13 +314,21 @@ static irqreturn_t regmap_irq_thread(int irq, void *d)
 				dev_err(map->dev,
 					"Failed to read IRQ status: %d\n",
 					ret);
-				if (chip->runtime_pm)
-					pm_runtime_put(map->dev);
-				goto exit;
+				return ret;
 			}
 		}
 	}
 
+	return 0;
+}
+
+static int regmap_irq_handle_pending(struct regmap_irq_chip_data *data)
+{
+	const struct regmap_irq_chip *chip = data->chip;
+	struct regmap *map = data->map;
+	int ret, i, handled = 0;
+	u32 reg;
+
 	/*
 	 * Ignore masked IRQs and ack if we need to; we ack early so
 	 * there is no race between handling and acknowleding the
@@ -361,14 +353,51 @@ static irqreturn_t regmap_irq_thread(int irq, void *d)
 		if (data->status_buf[chip->irqs[i].reg_offset /
 				     map->reg_stride] & chip->irqs[i].mask) {
 			handle_nested_irq(irq_find_mapping(data->domain, i));
-			handled = true;
+			handled++;
+		}
+	}
+
+	return handled;
+}
+
+static irqreturn_t regmap_irq_thread(int irq, void *d)
+{
+	struct regmap_irq_chip_data *data = d;
+	const struct regmap_irq_chip *chip = data->chip;
+	struct regmap *map = data->map;
+	int ret, handled = 0;
+
+	if (chip->handle_pre_irq)
+		chip->handle_pre_irq(chip->irq_drv_data);
+
+	if (chip->runtime_pm) {
+		ret = pm_runtime_get_sync(map->dev);
+		if (ret < 0) {
+			dev_err(map->dev, "IRQ thread failed to resume: %d\n",
+				ret);
+			goto out_runtime_put;
 		}
 	}
 
+	do {
+		ret = regmap_read_irq_status(data);
+		if (ret)
+			goto out_runtime_put;
+
+		ret = regmap_irq_handle_pending(data);
+		if (ret < 0)
+			goto out_runtime_put;
+
+		if (!ret)
+			break;
+
+		handled += ret;
+	} while (chip->handle_reread);
+
+out_runtime_put:
 	if (chip->runtime_pm)
 		pm_runtime_put(map->dev);
 
-exit:
 	if (chip->handle_post_irq)
 		chip->handle_post_irq(chip->irq_drv_data);
 
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -897,6 +897,7 @@ struct regmap_irq {
  * @ack_invert:  Inverted ack register: cleared bits for ack.
  * @wake_invert: Inverted wake register: cleared bits are wake enabled.
  * @type_invert: Invert the type flags.
+ * @handle_reread: Read interrupt status until no more interrupts are seen.
  * @runtime_pm:  Hold a runtime PM lock on the device when accessing it.
  *
  * @num_regs:    Number of registers in each control bank.
@@ -934,6 +935,7 @@ struct regmap_irq_chip {
 	bool wake_invert:1;
 	bool runtime_pm:1;
 	bool type_invert:1;
+	bool handle_reread:1;
 
 	int num_regs;
 
-- 
2.11.1

[PATCH 2/4] mfd: cpcap: Use handle_reread flag for interrupts

[Tony Lindgren]

On CPCAP we need to keep reading interrupts until there are no
more interrupts. Otherwise the PMIC interrupt to the SoC will at
some point stop toggling. This seems to happen because new CPCAP
device interrupts show up while we're handling.

Cc: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
Cc: Lee Jones <lee.jones@linaro.org>
Cc: Marcel Partap <mpartap@gmx.net>
Cc: Michael Scott <michael.scott@linaro.org>
Tested-by: Sebastian Reichel <sre@kernel.org>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/mfd/motorola-cpcap.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/mfd/motorola-cpcap.c b/drivers/mfd/motorola-cpcap.c
--- a/drivers/mfd/motorola-cpcap.c
+++ b/drivers/mfd/motorola-cpcap.c
@@ -71,6 +71,7 @@ static struct regmap_irq_chip cpcap_irq_chip[CPCAP_NR_IRQ_CHIPS] = {
 		.ack_base = CPCAP_REG_MI1,
 		.mask_base = CPCAP_REG_MIM1,
 		.use_ack = true,
+		.handle_reread = true,
 	},
 	{
 		.name = "cpcap-m2",
@@ -79,6 +80,7 @@ static struct regmap_irq_chip cpcap_irq_chip[CPCAP_NR_IRQ_CHIPS] = {
 		.ack_base = CPCAP_REG_MI2,
 		.mask_base = CPCAP_REG_MIM2,
 		.use_ack = true,
+		.handle_reread = true,
 	},
 	{
 		.name = "cpcap1-4",
@@ -88,6 +90,7 @@ static struct regmap_irq_chip cpcap_irq_chip[CPCAP_NR_IRQ_CHIPS] = {
 		.mask_base = CPCAP_REG_INTM1,
 		.type_base = CPCAP_REG_INTS1,
 		.use_ack = true,
+		.handle_reread = true,
 	},
 };
 
-- 
2.11.1

[PATCH 3/4] mfd: cpcap: Use ack_invert interrupts

[Tony Lindgren]

We should use ack_invert as the int_read_and_clear() in the Motorola
kernel tree does "ireg_val & ~mreg_val" before writing to the mask
register.

Cc: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
Cc: Lee Jones <lee.jones@linaro.org>
Cc: Marcel Partap <mpartap@gmx.net>
Cc: Michael Scott <michael.scott@linaro.org>
Tested-by: Sebastian Reichel <sre@kernel.org>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/mfd/motorola-cpcap.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/mfd/motorola-cpcap.c b/drivers/mfd/motorola-cpcap.c
--- a/drivers/mfd/motorola-cpcap.c
+++ b/drivers/mfd/motorola-cpcap.c
@@ -72,6 +72,7 @@ static struct regmap_irq_chip cpcap_irq_chip[CPCAP_NR_IRQ_CHIPS] = {
 		.mask_base = CPCAP_REG_MIM1,
 		.use_ack = true,
 		.handle_reread = true,
+		.ack_invert = true,
 	},
 	{
 		.name = "cpcap-m2",
@@ -81,6 +82,7 @@ static struct regmap_irq_chip cpcap_irq_chip[CPCAP_NR_IRQ_CHIPS] = {
 		.mask_base = CPCAP_REG_MIM2,
 		.use_ack = true,
 		.handle_reread = true,
+		.ack_invert = true,
 	},
 	{
 		.name = "cpcap1-4",
@@ -91,6 +93,7 @@ static struct regmap_irq_chip cpcap_irq_chip[CPCAP_NR_IRQ_CHIPS] = {
 		.type_base = CPCAP_REG_INTS1,
 		.use_ack = true,
 		.handle_reread = true,
+		.ack_invert = true,
 	},
 };
 
-- 
2.11.1

[PATCH 4/4] mfd: cpcap: Fix bad use of IRQ sense register

[Tony Lindgren]

The cpcap INTS registers are for getting the value of the line,
not for configuring the type.

Cc: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
Cc: Lee Jones <lee.jones@linaro.org>
Cc: Marcel Partap <mpartap@gmx.net>
Cc: Michael Scott <michael.scott@linaro.org>
Cc: Sebastian Reichel <sre@kernel.org>
Reviewed-By: Sebastian Reichel <sre@kernel.org>
Tested-by: Sebastian Reichel <sre@kernel.org>
Acked-by: Lee Jones <lee.jones@linaro.org>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/mfd/motorola-cpcap.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/mfd/motorola-cpcap.c b/drivers/mfd/motorola-cpcap.c
--- a/drivers/mfd/motorola-cpcap.c
+++ b/drivers/mfd/motorola-cpcap.c
@@ -90,7 +90,6 @@ static struct regmap_irq_chip cpcap_irq_chip[CPCAP_NR_IRQ_CHIPS] = {
 		.status_base = CPCAP_REG_INT1,
 		.ack_base = CPCAP_REG_INT1,
 		.mask_base = CPCAP_REG_INTM1,
-		.type_base = CPCAP_REG_INTS1,
 		.use_ack = true,
 		.handle_reread = true,
 		.ack_invert = true,
-- 
2.11.1

Re: [PATCH 1/4] regmap: irq: Fix lost interrupts by introducing handle_reread

[Mark Brown]

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

On Wed, Mar 22, 2017 at 10:10:49AM -0700, Tony Lindgren wrote:
> At least Motorola CPCAP PMIC needs it's device interrupts re-read
> until there are no more interrupts. Otherwise the PMIC interrupt to
> the SoC will eventually stop toggling. This seems to be a bug in the
> CPCAP PMIC where it can stop driving the GPIO interrupt to the SoC
> with pending CPCAP interrupts.

> Let's allow handling issues like this by introducing a flag for
> handle_reread and by splitting regmap_irq_thread() into two separate
> functions for regmap_read_irq_status() and regmap_irq_handle_pending().

So, I see your use case but the fact is that as Charles observed this is
exactly the code used for emulating level triggered IRQs with edge
triggered interrupt controllers.  This means someone is doubtless going
to end up using it for precisely that.  This makes me uncomfortable, we
do have this open coded into various drivers already but this is more of
a core thing and it feels like this should be in genirq rather than
here...  that said, looking at the code:

> +	do {
> +		ret = regmap_read_irq_status(data);
> +		if (ret)
> +			goto out_runtime_put;
> +
> +		ret = regmap_irq_handle_pending(data);
> +		if (ret < 0)
> +			goto out_runtime_put;
> +
> +		if (!ret)
> +			break;
> +
> +		handled += ret;
> +	} while (chip->handle_reread);

There's no protection against screaming interrupts here, I'd really like
to see that.  Also some tracing of the number of times we spin.

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

Re: [PATCH 1/4] regmap: irq: Fix lost interrupts by introducing handle_reread

[Tony Lindgren]

* Mark Brown <broonie@kernel.org> [170327 10:52]:
> On Wed, Mar 22, 2017 at 10:10:49AM -0700, Tony Lindgren wrote:
> > At least Motorola CPCAP PMIC needs it's device interrupts re-read
> > until there are no more interrupts. Otherwise the PMIC interrupt to
> > the SoC will eventually stop toggling. This seems to be a bug in the
> > CPCAP PMIC where it can stop driving the GPIO interrupt to the SoC
> > with pending CPCAP interrupts.
> 
> > Let's allow handling issues like this by introducing a flag for
> > handle_reread and by splitting regmap_irq_thread() into two separate
> > functions for regmap_read_irq_status() and regmap_irq_handle_pending().
> 
> So, I see your use case but the fact is that as Charles observed this is
> exactly the code used for emulating level triggered IRQs with edge
> triggered interrupt controllers.  This means someone is doubtless going
> to end up using it for precisely that.  This makes me uncomfortable, we
> do have this open coded into various drivers already but this is more of
> a core thing and it feels like this should be in genirq rather than
> here...  that said, looking at the code:

Yes.. But then again we might avoid piling up yet more driver
specific hacks. I don't know what the genirq solution would look
like, handle until we get IRQ_NONE? :)

> > +	do {
> > +		ret = regmap_read_irq_status(data);
> > +		if (ret)
> > +			goto out_runtime_put;
> > +
> > +		ret = regmap_irq_handle_pending(data);
> > +		if (ret < 0)
> > +			goto out_runtime_put;
> > +
> > +		if (!ret)
> > +			break;
> > +
> > +		handled += ret;
> > +	} while (chip->handle_reread);
> 
> There's no protection against screaming interrupts here, I'd really like
> to see that.  Also some tracing of the number of times we spin.

Good idea. How about something like below where handle_reread checks
for the total time spent in the thread loop with a large enough
timeout? Or do you have some better ideas in mind?

I tested I can hit that warning with timeout set to much lower
10 ms with retries being 1 or 2 at that point.

Regards,

Tony

8< -------------------------------
>From tony Mon Sep 17 00:00:00 2001
From: Tony Lindgren <tony@atomide.com>
Date: Mon, 27 Mar 2017 08:09:36 -0700
Subject: [PATCH] regmap: irq: Fix lost interrupts by introducing
 handle_reread

At least Motorola CPCAP PMIC needs it's device interrupts re-read
until there are no more interrupts. Otherwise the PMIC interrupt to
the SoC will eventually stop toggling. This seems to be a bug in the
CPCAP PMIC where it can stop driving the GPIO interrupt to the SoC
with pending CPCAP interrupts.

Let's allow handling issues like this by introducing a flag for
handle_reread and by splitting regmap_irq_thread() into two separate
functions for regmap_read_irq_status() and regmap_irq_handle_pending().

Cc: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
Cc: Lee Jones <lee.jones@linaro.org>
Cc: Marcel Partap <mpartap@gmx.net>
Cc: Michael Scott <michael.scott@linaro.org>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/base/regmap/regmap-irq.c | 97 ++++++++++++++++++++++++++++++----------
 include/linux/regmap.h           |  2 +
 2 files changed, 75 insertions(+), 24 deletions(-)

diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c
--- a/drivers/base/regmap/regmap-irq.c
+++ b/drivers/base/regmap/regmap-irq.c
@@ -21,6 +21,8 @@
 
 #include "internal.h"
 
+#define REGMAP_IRQ_THREAD_MAX_MSECS	3000
+
 struct regmap_irq_chip_data {
 	struct mutex lock;
 	struct irq_chip irq_chip;
@@ -259,27 +261,11 @@ static const struct irq_chip regmap_irq_chip = {
 	.irq_set_wake		= regmap_irq_set_wake,
 };
 
-static irqreturn_t regmap_irq_thread(int irq, void *d)
+static int regmap_read_irq_status(struct regmap_irq_chip_data *data)
 {
-	struct regmap_irq_chip_data *data = d;
 	const struct regmap_irq_chip *chip = data->chip;
 	struct regmap *map = data->map;
 	int ret, i;
-	bool handled = false;
-	u32 reg;
-
-	if (chip->handle_pre_irq)
-		chip->handle_pre_irq(chip->irq_drv_data);
-
-	if (chip->runtime_pm) {
-		ret = pm_runtime_get_sync(map->dev);
-		if (ret < 0) {
-			dev_err(map->dev, "IRQ thread failed to resume: %d\n",
-				ret);
-			pm_runtime_put(map->dev);
-			goto exit;
-		}
-	}
 
 	/*
 	 * Read in the statuses, using a single bulk read if possible
@@ -299,7 +285,7 @@ static irqreturn_t regmap_irq_thread(int irq, void *d)
 		if (ret != 0) {
 			dev_err(map->dev, "Failed to read IRQ status: %d\n",
 				ret);
-			goto exit;
+			return ret;
 		}
 
 		for (i = 0; i < data->chip->num_regs; i++) {
@@ -315,7 +301,7 @@ static irqreturn_t regmap_irq_thread(int irq, void *d)
 				break;
 			default:
 				BUG();
-				goto exit;
+				return ret;
 			}
 		}
 
@@ -330,13 +316,21 @@ static irqreturn_t regmap_irq_thread(int irq, void *d)
 				dev_err(map->dev,
 					"Failed to read IRQ status: %d\n",
 					ret);
-				if (chip->runtime_pm)
-					pm_runtime_put(map->dev);
-				goto exit;
+				return ret;
 			}
 		}
 	}
 
+	return 0;
+}
+
+static int regmap_irq_handle_pending(struct regmap_irq_chip_data *data)
+{
+	const struct regmap_irq_chip *chip = data->chip;
+	struct regmap *map = data->map;
+	int ret, i, handled = 0;
+	u32 reg;
+
 	/*
 	 * Ignore masked IRQs and ack if we need to; we ack early so
 	 * there is no race between handling and acknowleding the
@@ -361,14 +355,69 @@ static irqreturn_t regmap_irq_thread(int irq, void *d)
 		if (data->status_buf[chip->irqs[i].reg_offset /
 				     map->reg_stride] & chip->irqs[i].mask) {
 			handle_nested_irq(irq_find_mapping(data->domain, i));
-			handled = true;
+			handled++;
 		}
 	}
 
+	return handled;
+}
+
+static irqreturn_t regmap_irq_thread(int irq, void *d)
+{
+	struct regmap_irq_chip_data *data = d;
+	const struct regmap_irq_chip *chip = data->chip;
+	struct regmap *map = data->map;
+	ktime_t calltime = 0, retrytime;
+	int ret, handled = 0, retries = 0;
+
+	if (chip->handle_reread)
+		calltime = ktime_get();
+
+	if (chip->handle_pre_irq)
+		chip->handle_pre_irq(chip->irq_drv_data);
+
+	if (chip->runtime_pm) {
+		ret = pm_runtime_get_sync(map->dev);
+		if (ret < 0) {
+			dev_err(map->dev, "IRQ thread failed to resume: %d\n",
+				ret);
+			goto out_runtime_put;
+		}
+	}
+
+	do {
+		ret = regmap_read_irq_status(data);
+		if (ret)
+			goto out_runtime_put;
+
+		ret = regmap_irq_handle_pending(data);
+		if (ret < 0)
+			goto out_runtime_put;
+
+		if (!ret)
+			break;
+
+		handled += ret;
+		retries++;
+
+		if (chip->handle_reread) {
+			s64 msecs;
+
+			retrytime = ktime_get();
+			msecs = ktime_to_ms(ktime_sub(retrytime, calltime));
+			if (msecs > REGMAP_IRQ_THREAD_MAX_MSECS) {
+				dev_err(map->dev,
+					"IRQ thread timeout after %i retries\n",
+					retries);
+				goto out_runtime_put;
+			}
+		}
+	} while (chip->handle_reread);
+
+out_runtime_put:
 	if (chip->runtime_pm)
 		pm_runtime_put(map->dev);
 
-exit:
 	if (chip->handle_post_irq)
 		chip->handle_post_irq(chip->irq_drv_data);
 
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -897,6 +897,7 @@ struct regmap_irq {
  * @ack_invert:  Inverted ack register: cleared bits for ack.
  * @wake_invert: Inverted wake register: cleared bits are wake enabled.
  * @type_invert: Invert the type flags.
+ * @handle_reread: Read interrupt status until no more interrupts are seen.
  * @runtime_pm:  Hold a runtime PM lock on the device when accessing it.
  *
  * @num_regs:    Number of registers in each control bank.
@@ -934,6 +935,7 @@ struct regmap_irq_chip {
 	bool wake_invert:1;
 	bool runtime_pm:1;
 	bool type_invert:1;
+	bool handle_reread:1;
 
 	int num_regs;
 
-- 
2.12.1

Re: [PATCH 1/4] regmap: irq: Fix lost interrupts by introducing handle_reread

[Mark Brown]

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

On Mon, Mar 27, 2017 at 05:36:48PM -0700, Tony Lindgren wrote:
> * Mark Brown <broonie@kernel.org> [170327 10:52]:

> > So, I see your use case but the fact is that as Charles observed this is
> > exactly the code used for emulating level triggered IRQs with edge
> > triggered interrupt controllers.  This means someone is doubtless going
> > to end up using it for precisely that.  This makes me uncomfortable, we
> > do have this open coded into various drivers already but this is more of
> > a core thing and it feels like this should be in genirq rather than
> > here...  that said, looking at the code:

> Yes.. But then again we might avoid piling up yet more driver
> specific hacks. I don't know what the genirq solution would look

Right, my thinking here is that by pushing into genirq we minimise the
need even further since it'll also be available to drivers not using
regmap-irq.

> like, handle until we get IRQ_NONE? :)

Well, that's what the per driver emulation does so...  yeah.  Probably
with an upper limit on the number of times we do that.

> > There's no protection against screaming interrupts here, I'd really like
> > to see that.  Also some tracing of the number of times we spin.

> Good idea. How about something like below where handle_reread checks
> for the total time spent in the thread loop with a large enough
> timeout? Or do you have some better ideas in mind?

> I tested I can hit that warning with timeout set to much lower
> 10 ms with retries being 1 or 2 at that point.

That looks sensible, yes.

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

Re: [PATCH 1/4] regmap: irq: Fix lost interrupts by introducing handle_reread

[Tony Lindgren]

* Mark Brown <broonie@kernel.org> [170328 08:21]:
> On Mon, Mar 27, 2017 at 05:36:48PM -0700, Tony Lindgren wrote:
> > * Mark Brown <broonie@kernel.org> [170327 10:52]:
> 
> > > So, I see your use case but the fact is that as Charles observed this is
> > > exactly the code used for emulating level triggered IRQs with edge
> > > triggered interrupt controllers.  This means someone is doubtless going
> > > to end up using it for precisely that.  This makes me uncomfortable, we
> > > do have this open coded into various drivers already but this is more of
> > > a core thing and it feels like this should be in genirq rather than
> > > here...  that said, looking at the code:
> 
> > Yes.. But then again we might avoid piling up yet more driver
> > specific hacks. I don't know what the genirq solution would look
> 
> Right, my thinking here is that by pushing into genirq we minimise the
> need even further since it'll also be available to drivers not using
> regmap-irq.
> 
> > like, handle until we get IRQ_NONE? :)
> 
> Well, that's what the per driver emulation does so...  yeah.  Probably
> with an upper limit on the number of times we do that.

OK let's first see how that would work. I'll send a patch
for that.

> > > There's no protection against screaming interrupts here, I'd really like
> > > to see that.  Also some tracing of the number of times we spin.
> 
> > Good idea. How about something like below where handle_reread checks
> > for the total time spent in the thread loop with a large enough
> > timeout? Or do you have some better ideas in mind?
> 
> > I tested I can hit that warning with timeout set to much lower
> > 10 ms with retries being 1 or 2 at that point.
> 
> That looks sensible, yes.

OK

Regards,

Tony

Re: [PATCH 1/4] regmap: irq: Fix lost interrupts by introducing handle_reread

[Mark Brown]

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

On Tue, Mar 28, 2017 at 08:47:41AM -0700, Tony Lindgren wrote:
> * Mark Brown <broonie@kernel.org> [170328 08:21]:

> > Right, my thinking here is that by pushing into genirq we minimise the
> > need even further since it'll also be available to drivers not using
> > regmap-irq.

> > > like, handle until we get IRQ_NONE? :)

> > Well, that's what the per driver emulation does so...  yeah.  Probably
> > with an upper limit on the number of times we do that.

> OK let's first see how that would work. I'll send a patch
> for that.

Thanks.  Can you keep me on the CC please?  It's something I keep
thinking about looking at myself.

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

Re: [PATCH 1/4] regmap: irq: Fix lost interrupts by introducing handle_reread

[Tony Lindgren]

* Mark Brown <broonie@kernel.org> [170328 09:51]:
> On Tue, Mar 28, 2017 at 08:47:41AM -0700, Tony Lindgren wrote:
> > * Mark Brown <broonie@kernel.org> [170328 08:21]:
> 
> > > Right, my thinking here is that by pushing into genirq we minimise the
> > > need even further since it'll also be available to drivers not using
> > > regmap-irq.
> 
> > > > like, handle until we get IRQ_NONE? :)
> 
> > > Well, that's what the per driver emulation does so...  yeah.  Probably
> > > with an upper limit on the number of times we do that.
> 
> > OK let's first see how that would work. I'll send a patch
> > for that.
> 
> Thanks.  Can you keep me on the CC please?  It's something I keep
> thinking about looking at myself.

Sure will do, you'll get some shared flames on it :)

Regards,

Tony

Re: [PATCH 2/4] mfd: cpcap: Use handle_reread flag for interrupts

[Lee Jones]

On Wed, 22 Mar 2017, Tony Lindgren wrote:

> On CPCAP we need to keep reading interrupts until there are no
> more interrupts. Otherwise the PMIC interrupt to the SoC will at
> some point stop toggling. This seems to happen because new CPCAP
> device interrupts show up while we're handling.
> 
> Cc: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> Cc: Lee Jones <lee.jones@linaro.org>
> Cc: Marcel Partap <mpartap@gmx.net>
> Cc: Michael Scott <michael.scott@linaro.org>
> Tested-by: Sebastian Reichel <sre@kernel.org>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>  drivers/mfd/motorola-cpcap.c | 3 +++
>  1 file changed, 3 insertions(+)

Applied, thanks.

> diff --git a/drivers/mfd/motorola-cpcap.c b/drivers/mfd/motorola-cpcap.c
> --- a/drivers/mfd/motorola-cpcap.c
> +++ b/drivers/mfd/motorola-cpcap.c
> @@ -71,6 +71,7 @@ static struct regmap_irq_chip cpcap_irq_chip[CPCAP_NR_IRQ_CHIPS] = {
>  		.ack_base = CPCAP_REG_MI1,
>  		.mask_base = CPCAP_REG_MIM1,
>  		.use_ack = true,
> +		.handle_reread = true,
>  	},
>  	{
>  		.name = "cpcap-m2",
> @@ -79,6 +80,7 @@ static struct regmap_irq_chip cpcap_irq_chip[CPCAP_NR_IRQ_CHIPS] = {
>  		.ack_base = CPCAP_REG_MI2,
>  		.mask_base = CPCAP_REG_MIM2,
>  		.use_ack = true,
> +		.handle_reread = true,
>  	},
>  	{
>  		.name = "cpcap1-4",
> @@ -88,6 +90,7 @@ static struct regmap_irq_chip cpcap_irq_chip[CPCAP_NR_IRQ_CHIPS] = {
>  		.mask_base = CPCAP_REG_INTM1,
>  		.type_base = CPCAP_REG_INTS1,
>  		.use_ack = true,
> +		.handle_reread = true,
>  	},
>  };
>  

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 3/4] mfd: cpcap: Use ack_invert interrupts

[Lee Jones]

On Wed, 22 Mar 2017, Tony Lindgren wrote:

> We should use ack_invert as the int_read_and_clear() in the Motorola
> kernel tree does "ireg_val & ~mreg_val" before writing to the mask
> register.
> 
> Cc: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> Cc: Lee Jones <lee.jones@linaro.org>
> Cc: Marcel Partap <mpartap@gmx.net>
> Cc: Michael Scott <michael.scott@linaro.org>
> Tested-by: Sebastian Reichel <sre@kernel.org>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>  drivers/mfd/motorola-cpcap.c | 3 +++
>  1 file changed, 3 insertions(+)

Applied, thanks.

> diff --git a/drivers/mfd/motorola-cpcap.c b/drivers/mfd/motorola-cpcap.c
> --- a/drivers/mfd/motorola-cpcap.c
> +++ b/drivers/mfd/motorola-cpcap.c
> @@ -72,6 +72,7 @@ static struct regmap_irq_chip cpcap_irq_chip[CPCAP_NR_IRQ_CHIPS] = {
>  		.mask_base = CPCAP_REG_MIM1,
>  		.use_ack = true,
>  		.handle_reread = true,
> +		.ack_invert = true,
>  	},
>  	{
>  		.name = "cpcap-m2",
> @@ -81,6 +82,7 @@ static struct regmap_irq_chip cpcap_irq_chip[CPCAP_NR_IRQ_CHIPS] = {
>  		.mask_base = CPCAP_REG_MIM2,
>  		.use_ack = true,
>  		.handle_reread = true,
> +		.ack_invert = true,
>  	},
>  	{
>  		.name = "cpcap1-4",
> @@ -91,6 +93,7 @@ static struct regmap_irq_chip cpcap_irq_chip[CPCAP_NR_IRQ_CHIPS] = {
>  		.type_base = CPCAP_REG_INTS1,
>  		.use_ack = true,
>  		.handle_reread = true,
> +		.ack_invert = true,
>  	},
>  };
>  

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 4/4] mfd: cpcap: Fix bad use of IRQ sense register

[Lee Jones]

On Wed, 22 Mar 2017, Tony Lindgren wrote:

> The cpcap INTS registers are for getting the value of the line,
> not for configuring the type.
> 
> Cc: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> Cc: Lee Jones <lee.jones@linaro.org>
> Cc: Marcel Partap <mpartap@gmx.net>
> Cc: Michael Scott <michael.scott@linaro.org>
> Cc: Sebastian Reichel <sre@kernel.org>
> Reviewed-By: Sebastian Reichel <sre@kernel.org>
> Tested-by: Sebastian Reichel <sre@kernel.org>
> Acked-by: Lee Jones <lee.jones@linaro.org>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>  drivers/mfd/motorola-cpcap.c | 1 -
>  1 file changed, 1 deletion(-)

Applied, thanks.

> diff --git a/drivers/mfd/motorola-cpcap.c b/drivers/mfd/motorola-cpcap.c
> --- a/drivers/mfd/motorola-cpcap.c
> +++ b/drivers/mfd/motorola-cpcap.c
> @@ -90,7 +90,6 @@ static struct regmap_irq_chip cpcap_irq_chip[CPCAP_NR_IRQ_CHIPS] = {
>  		.status_base = CPCAP_REG_INT1,
>  		.ack_base = CPCAP_REG_INT1,
>  		.mask_base = CPCAP_REG_INTM1,
> -		.type_base = CPCAP_REG_INTS1,
>  		.use_ack = true,
>  		.handle_reread = true,
>  		.ack_invert = true,

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 1/4] regmap: irq: Fix lost interrupts by introducing handle_reread

[Lee Jones]

On Wed, 22 Mar 2017, Tony Lindgren wrote:

> At least Motorola CPCAP PMIC needs it's device interrupts re-read
> until there are no more interrupts. Otherwise the PMIC interrupt to
> the SoC will eventually stop toggling. This seems to be a bug in the
> CPCAP PMIC where it can stop driving the GPIO interrupt to the SoC
> with pending CPCAP interrupts.
> 
> Let's allow handling issues like this by introducing a flag for
> handle_reread and by splitting regmap_irq_thread() into two separate
> functions for regmap_read_irq_status() and regmap_irq_handle_pending().
> 
> Cc: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> Cc: Lee Jones <lee.jones@linaro.org>
> Cc: Marcel Partap <mpartap@gmx.net>
> Cc: Michael Scott <michael.scott@linaro.org>
> Tested-by: Sebastian Reichel <sre@kernel.org>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>  drivers/base/regmap/regmap-irq.c | 77 +++++++++++++++++++++++++++-------------
>  include/linux/regmap.h           |  2 ++
>  2 files changed, 55 insertions(+), 24 deletions(-)

Looks like this is required for the MFD patches.

I'm removing them for now.

> diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c
> --- a/drivers/base/regmap/regmap-irq.c
> +++ b/drivers/base/regmap/regmap-irq.c
> @@ -259,27 +259,11 @@ static const struct irq_chip regmap_irq_chip = {
>  	.irq_set_wake		= regmap_irq_set_wake,
>  };
>  
> -static irqreturn_t regmap_irq_thread(int irq, void *d)
> +static int regmap_read_irq_status(struct regmap_irq_chip_data *data)
>  {
> -	struct regmap_irq_chip_data *data = d;
>  	const struct regmap_irq_chip *chip = data->chip;
>  	struct regmap *map = data->map;
>  	int ret, i;
> -	bool handled = false;
> -	u32 reg;
> -
> -	if (chip->handle_pre_irq)
> -		chip->handle_pre_irq(chip->irq_drv_data);
> -
> -	if (chip->runtime_pm) {
> -		ret = pm_runtime_get_sync(map->dev);
> -		if (ret < 0) {
> -			dev_err(map->dev, "IRQ thread failed to resume: %d\n",
> -				ret);
> -			pm_runtime_put(map->dev);
> -			goto exit;
> -		}
> -	}
>  
>  	/*
>  	 * Read in the statuses, using a single bulk read if possible
> @@ -299,7 +283,7 @@ static irqreturn_t regmap_irq_thread(int irq, void *d)
>  		if (ret != 0) {
>  			dev_err(map->dev, "Failed to read IRQ status: %d\n",
>  				ret);
> -			goto exit;
> +			return ret;
>  		}
>  
>  		for (i = 0; i < data->chip->num_regs; i++) {
> @@ -315,7 +299,7 @@ static irqreturn_t regmap_irq_thread(int irq, void *d)
>  				break;
>  			default:
>  				BUG();
> -				goto exit;
> +				return ret;
>  			}
>  		}
>  
> @@ -330,13 +314,21 @@ static irqreturn_t regmap_irq_thread(int irq, void *d)
>  				dev_err(map->dev,
>  					"Failed to read IRQ status: %d\n",
>  					ret);
> -				if (chip->runtime_pm)
> -					pm_runtime_put(map->dev);
> -				goto exit;
> +				return ret;
>  			}
>  		}
>  	}
>  
> +	return 0;
> +}
> +
> +static int regmap_irq_handle_pending(struct regmap_irq_chip_data *data)
> +{
> +	const struct regmap_irq_chip *chip = data->chip;
> +	struct regmap *map = data->map;
> +	int ret, i, handled = 0;
> +	u32 reg;
> +
>  	/*
>  	 * Ignore masked IRQs and ack if we need to; we ack early so
>  	 * there is no race between handling and acknowleding the
> @@ -361,14 +353,51 @@ static irqreturn_t regmap_irq_thread(int irq, void *d)
>  		if (data->status_buf[chip->irqs[i].reg_offset /
>  				     map->reg_stride] & chip->irqs[i].mask) {
>  			handle_nested_irq(irq_find_mapping(data->domain, i));
> -			handled = true;
> +			handled++;
> +		}
> +	}
> +
> +	return handled;
> +}
> +
> +static irqreturn_t regmap_irq_thread(int irq, void *d)
> +{
> +	struct regmap_irq_chip_data *data = d;
> +	const struct regmap_irq_chip *chip = data->chip;
> +	struct regmap *map = data->map;
> +	int ret, handled = 0;
> +
> +	if (chip->handle_pre_irq)
> +		chip->handle_pre_irq(chip->irq_drv_data);
> +
> +	if (chip->runtime_pm) {
> +		ret = pm_runtime_get_sync(map->dev);
> +		if (ret < 0) {
> +			dev_err(map->dev, "IRQ thread failed to resume: %d\n",
> +				ret);
> +			goto out_runtime_put;
>  		}
>  	}
>  
> +	do {
> +		ret = regmap_read_irq_status(data);
> +		if (ret)
> +			goto out_runtime_put;
> +
> +		ret = regmap_irq_handle_pending(data);
> +		if (ret < 0)
> +			goto out_runtime_put;
> +
> +		if (!ret)
> +			break;
> +
> +		handled += ret;
> +	} while (chip->handle_reread);
> +
> +out_runtime_put:
>  	if (chip->runtime_pm)
>  		pm_runtime_put(map->dev);
>  
> -exit:
>  	if (chip->handle_post_irq)
>  		chip->handle_post_irq(chip->irq_drv_data);
>  
> diff --git a/include/linux/regmap.h b/include/linux/regmap.h
> --- a/include/linux/regmap.h
> +++ b/include/linux/regmap.h
> @@ -897,6 +897,7 @@ struct regmap_irq {
>   * @ack_invert:  Inverted ack register: cleared bits for ack.
>   * @wake_invert: Inverted wake register: cleared bits are wake enabled.
>   * @type_invert: Invert the type flags.
> + * @handle_reread: Read interrupt status until no more interrupts are seen.
>   * @runtime_pm:  Hold a runtime PM lock on the device when accessing it.
>   *
>   * @num_regs:    Number of registers in each control bank.
> @@ -934,6 +935,7 @@ struct regmap_irq_chip {
>  	bool wake_invert:1;
>  	bool runtime_pm:1;
>  	bool type_invert:1;
> +	bool handle_reread:1;
>  
>  	int num_regs;
>  

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 2/4] mfd: cpcap: Use handle_reread flag for interrupts

[Tony Lindgren]

Hi,

* Lee Jones <lee.jones@linaro.org> [170403 03:23]:
> On Wed, 22 Mar 2017, Tony Lindgren wrote:
> 
> > On CPCAP we need to keep reading interrupts until there are no
> > more interrupts. Otherwise the PMIC interrupt to the SoC will at
> > some point stop toggling. This seems to happen because new CPCAP
> > device interrupts show up while we're handling.
> > 
> > Cc: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> > Cc: Lee Jones <lee.jones@linaro.org>
> > Cc: Marcel Partap <mpartap@gmx.net>
> > Cc: Michael Scott <michael.scott@linaro.org>
> > Tested-by: Sebastian Reichel <sre@kernel.org>
> > Signed-off-by: Tony Lindgren <tony@atomide.com>
> > ---
> >  drivers/mfd/motorola-cpcap.c | 3 +++
> >  1 file changed, 3 insertions(+)
> 
> Applied, thanks.

Sorry this one depends on patch 1/4 for .handle_reread. So this
one should be reverted or dropped. That will cause trivial merge
conflicts with patches 2 and 4.

Please let me know if you want me to resend just patches 2 and 4
as patches 1 and 2 need more work.

Regards,

Tony

> > diff --git a/drivers/mfd/motorola-cpcap.c b/drivers/mfd/motorola-cpcap.c
> > --- a/drivers/mfd/motorola-cpcap.c
> > +++ b/drivers/mfd/motorola-cpcap.c
> > @@ -71,6 +71,7 @@ static struct regmap_irq_chip cpcap_irq_chip[CPCAP_NR_IRQ_CHIPS] = {
> >  		.ack_base = CPCAP_REG_MI1,
> >  		.mask_base = CPCAP_REG_MIM1,
> >  		.use_ack = true,
> > +		.handle_reread = true,
> >  	},
> >  	{
> >  		.name = "cpcap-m2",
> > @@ -79,6 +80,7 @@ static struct regmap_irq_chip cpcap_irq_chip[CPCAP_NR_IRQ_CHIPS] = {
> >  		.ack_base = CPCAP_REG_MI2,
> >  		.mask_base = CPCAP_REG_MIM2,
> >  		.use_ack = true,
> > +		.handle_reread = true,
> >  	},
> >  	{
> >  		.name = "cpcap1-4",
> > @@ -88,6 +90,7 @@ static struct regmap_irq_chip cpcap_irq_chip[CPCAP_NR_IRQ_CHIPS] = {
> >  		.mask_base = CPCAP_REG_INTM1,
> >  		.type_base = CPCAP_REG_INTS1,
> >  		.use_ack = true,
> > +		.handle_reread = true,
> >  	},
> >  };
> >  
> 
> -- 
> Lee Jones
> Linaro STMicroelectronics Landing Team Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 2/4] mfd: cpcap: Use handle_reread flag for interrupts

[Lee Jones]

On Mon, 03 Apr 2017, Tony Lindgren wrote:

> Hi,
> 
> * Lee Jones <lee.jones@linaro.org> [170403 03:23]:
> > On Wed, 22 Mar 2017, Tony Lindgren wrote:
> > 
> > > On CPCAP we need to keep reading interrupts until there are no
> > > more interrupts. Otherwise the PMIC interrupt to the SoC will at
> > > some point stop toggling. This seems to happen because new CPCAP
> > > device interrupts show up while we're handling.
> > > 
> > > Cc: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> > > Cc: Lee Jones <lee.jones@linaro.org>
> > > Cc: Marcel Partap <mpartap@gmx.net>
> > > Cc: Michael Scott <michael.scott@linaro.org>
> > > Tested-by: Sebastian Reichel <sre@kernel.org>
> > > Signed-off-by: Tony Lindgren <tony@atomide.com>
> > > ---
> > >  drivers/mfd/motorola-cpcap.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > 
> > Applied, thanks.
> 
> Sorry this one depends on patch 1/4 for .handle_reread. So this
> one should be reverted or dropped. That will cause trivial merge
> conflicts with patches 2 and 4.
> 
> Please let me know if you want me to resend just patches 2 and 4
> as patches 1 and 2 need more work.

Yes, I found that out (and replied to the regmap patch).

I'll revert them all for now.

Awaiting a subsequent revision.

> > > diff --git a/drivers/mfd/motorola-cpcap.c b/drivers/mfd/motorola-cpcap.c
> > > --- a/drivers/mfd/motorola-cpcap.c
> > > +++ b/drivers/mfd/motorola-cpcap.c
> > > @@ -71,6 +71,7 @@ static struct regmap_irq_chip cpcap_irq_chip[CPCAP_NR_IRQ_CHIPS] = {
> > >  		.ack_base = CPCAP_REG_MI1,
> > >  		.mask_base = CPCAP_REG_MIM1,
> > >  		.use_ack = true,
> > > +		.handle_reread = true,
> > >  	},
> > >  	{
> > >  		.name = "cpcap-m2",
> > > @@ -79,6 +80,7 @@ static struct regmap_irq_chip cpcap_irq_chip[CPCAP_NR_IRQ_CHIPS] = {
> > >  		.ack_base = CPCAP_REG_MI2,
> > >  		.mask_base = CPCAP_REG_MIM2,
> > >  		.use_ack = true,
> > > +		.handle_reread = true,
> > >  	},
> > >  	{
> > >  		.name = "cpcap1-4",
> > > @@ -88,6 +90,7 @@ static struct regmap_irq_chip cpcap_irq_chip[CPCAP_NR_IRQ_CHIPS] = {
> > >  		.mask_base = CPCAP_REG_INTM1,
> > >  		.type_base = CPCAP_REG_INTS1,
> > >  		.use_ack = true,
> > > +		.handle_reread = true,
> > >  	},
> > >  };
> > >  
> > 

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 1/4] regmap: irq: Fix lost interrupts by introducing handle_reread

[Tony Lindgren]

Hi,

* Tony Lindgren <tony@atomide.com> [170328 10:13]:
> * Mark Brown <broonie@kernel.org> [170328 09:51]:
> > On Tue, Mar 28, 2017 at 08:47:41AM -0700, Tony Lindgren wrote:
> > > * Mark Brown <broonie@kernel.org> [170328 08:21]:
> > 
> > > > Right, my thinking here is that by pushing into genirq we minimise the
> > > > need even further since it'll also be available to drivers not using
> > > > regmap-irq.
> > 
> > > > > like, handle until we get IRQ_NONE? :)
> > 
> > > > Well, that's what the per driver emulation does so...  yeah.  Probably
> > > > with an upper limit on the number of times we do that.
> > 
> > > OK let's first see how that would work. I'll send a patch
> > > for that.
> > 
> > Thanks.  Can you keep me on the CC please?  It's something I keep
> > thinking about looking at myself.
> 
> Sure will do, you'll get some shared flames on it :)

So I found the real problem after thinking what you guys commented
on the "level device connected to an edge only GPIO". I added some
printks to verify that's not the case just to find out that the dts
GPIO interrupt edge configuration got only passed to the SPI driver :)

So the level IRQ changes I did earlier to the dts file did nothing.

The fix is simply to call devm_regmap_add_irq_chip() with
irq_get_trigger_type(cpcap->spi->irq) to get the configured
triggering passed properly from the dts.

So I'll drop the genirq/regmap_irq related hacks and resend just
the minimal MFD fixes. Similar misconfiguration may be the root
cause for other drivers too..

Regards,

Tony

Re: [PATCH 2/4] mfd: cpcap: Use handle_reread flag for interrupts

[Tony Lindgren]

* Lee Jones <lee.jones@linaro.org> [170403 07:34]:
> On Mon, 03 Apr 2017, Tony Lindgren wrote:
> 
> > Hi,
> > 
> > * Lee Jones <lee.jones@linaro.org> [170403 03:23]:
> > > On Wed, 22 Mar 2017, Tony Lindgren wrote:
> > > 
> > > > On CPCAP we need to keep reading interrupts until there are no
> > > > more interrupts. Otherwise the PMIC interrupt to the SoC will at
> > > > some point stop toggling. This seems to happen because new CPCAP
> > > > device interrupts show up while we're handling.
> > > > 
> > > > Cc: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> > > > Cc: Lee Jones <lee.jones@linaro.org>
> > > > Cc: Marcel Partap <mpartap@gmx.net>
> > > > Cc: Michael Scott <michael.scott@linaro.org>
> > > > Tested-by: Sebastian Reichel <sre@kernel.org>
> > > > Signed-off-by: Tony Lindgren <tony@atomide.com>
> > > > ---
> > > >  drivers/mfd/motorola-cpcap.c | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > 
> > > Applied, thanks.
> > 
> > Sorry this one depends on patch 1/4 for .handle_reread. So this
> > one should be reverted or dropped. That will cause trivial merge
> > conflicts with patches 2 and 4.
> > 
> > Please let me know if you want me to resend just patches 2 and 4
> > as patches 1 and 2 need more work.
> 
> Yes, I found that out (and replied to the regmap patch).
> 
> I'll revert them all for now.

OK thanks.

> Awaiting a subsequent revision.

Sent as "[PATCHv3 0/3] CPCAP PMIC IRQ fix and related changes"
with regmap changes dropped as I figured out what the real issue
with lost interrupts was :)

Regards,

Tony

Re: [PATCH 1/4] regmap: irq: Fix lost interrupts by introducing handle_reread

[Mark Brown]

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

On Mon, Apr 03, 2017 at 08:03:00PM -0700, Tony Lindgren wrote:

> So I'll drop the genirq/regmap_irq related hacks and resend just
> the minimal MFD fixes. Similar misconfiguration may be the root
> cause for other drivers too..

It is sadly far too common for people to implement interrupt controllers
that only do edge triggers, I've no idea why even on what are supposed
to be relatively high end SoCs.  It seems to be a hardware designer
thing, AFAICT they think for something to be useful it needs to be a bit
more complicated.

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

Re: [PATCH 1/4] regmap: irq: Fix lost interrupts by introducing handle_reread

[Tony Lindgren]

* Mark Brown <broonie@kernel.org> [170404 05:22]:
> On Mon, Apr 03, 2017 at 08:03:00PM -0700, Tony Lindgren wrote:
> 
> > So I'll drop the genirq/regmap_irq related hacks and resend just
> > the minimal MFD fixes. Similar misconfiguration may be the root
> > cause for other drivers too..
> 
> It is sadly far too common for people to implement interrupt controllers
> that only do edge triggers, I've no idea why even on what are supposed
> to be relatively high end SoCs.  It seems to be a hardware designer
> thing, AFAICT they think for something to be useful it needs to be a bit
> more complicated.

For edge only GPIO controllers handling level interrupts might be
somewhat fixable in software. The GPIO controller could have a loop
reading of the GPIO line status in the interrupt handler and comparing
it to the configured triggering.

Regards,

Tony

Re: [PATCH 1/4] regmap: irq: Fix lost interrupts by introducing handle_reread

[Mark Brown]

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

On Tue, Apr 04, 2017 at 06:56:30AM -0700, Tony Lindgren wrote:
> * Mark Brown <broonie@kernel.org> [170404 05:22]:

> > It is sadly far too common for people to implement interrupt controllers
> > that only do edge triggers, I've no idea why even on what are supposed
> > to be relatively high end SoCs.  It seems to be a hardware designer
> > thing, AFAICT they think for something to be useful it needs to be a bit
> > more complicated.

> For edge only GPIO controllers handling level interrupts might be
> somewhat fixable in software. The GPIO controller could have a loop
> reading of the GPIO line status in the interrupt handler and comparing
> it to the configured triggering.

Yeah, it's doable if annoying.

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

[PATCH 2/4] mfd: cpcap: Use handle_reread flag for interrupts

[Tony Lindgren]

On CPCAP we need to keep reading interrupts until there are no
more interrupts. Otherwise the PMIC interrupt to the SoC will at
some point stop toggling. This seems to happen because new CPCAP
device interrupts show up while we're handling.

Cc: Lee Jones <lee.jones@linaro.org>
Cc: Marcel Partap <mpartap@gmx.net>
Cc: Michael Scott <michael.scott@linaro.org>
Cc: Sebastian Reichel <sre@kernel.org>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/mfd/motorola-cpcap.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/mfd/motorola-cpcap.c b/drivers/mfd/motorola-cpcap.c
--- a/drivers/mfd/motorola-cpcap.c
+++ b/drivers/mfd/motorola-cpcap.c
@@ -96,6 +96,7 @@ static struct regmap_irq_chip cpcap_irq_chip[CPCAP_NR_IRQ_CHIPS] = {
 		.ack_base = CPCAP_REG_MI1,
 		.mask_base = CPCAP_REG_MIM1,
 		.use_ack = true,
+		.handle_reread = true,
 	},
 	{
 		.name = "cpcap-m2",
@@ -104,6 +105,7 @@ static struct regmap_irq_chip cpcap_irq_chip[CPCAP_NR_IRQ_CHIPS] = {
 		.ack_base = CPCAP_REG_MI2,
 		.mask_base = CPCAP_REG_MIM2,
 		.use_ack = true,
+		.handle_reread = true,
 	},
 	{
 		.name = "cpcap1-4",
@@ -113,6 +115,7 @@ static struct regmap_irq_chip cpcap_irq_chip[CPCAP_NR_IRQ_CHIPS] = {
 		.mask_base = CPCAP_REG_INTM1,
 		.type_base = CPCAP_REG_INTS1,
 		.use_ack = true,
+		.handle_reread = true,
 	},
 };
 
-- 
2.11.1