[PATCH 1/3] regmap: fix some error messages to take account of irq_reg_stride

[PATCH 1/3] regmap: fix some error messages to take account of irq_reg_stride

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

A number of places in the code were printing error messages that included
the address of a register, but were not calculating the register address
in the same way as the access to the register. Use a temporary to solve
this.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
Note: This patch series will be a dependency for some MFD patches that I
will send shortly.

 drivers/base/regmap/regmap-irq.c |   29 +++++++++++++++--------------
 1 files changed, 15 insertions(+), 14 deletions(-)

diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c
index a897346..c7e5b18 100644
--- a/drivers/base/regmap/regmap-irq.c
+++ b/drivers/base/regmap/regmap-irq.c
@@ -59,6 +59,7 @@ static void regmap_irq_sync_unlock(struct irq_data *data)
 	struct regmap_irq_chip_data *d = irq_data_get_irq_chip_data(data);
 	struct regmap *map = d->map;
 	int i, ret;
+	u32 reg;
 
 	/*
 	 * If there's been a change in the mask write it back to the
@@ -66,13 +67,13 @@ static void regmap_irq_sync_unlock(struct irq_data *data)
 	 * suppress pointless writes.
 	 */
 	for (i = 0; i < d->chip->num_regs; i++) {
-		ret = regmap_update_bits(d->map, d->chip->mask_base +
-						(i * map->reg_stride *
-						d->irq_reg_stride),
+		reg = d->chip->mask_base +
+			(i * map->reg_stride * d->irq_reg_stride);
+		ret = regmap_update_bits(d->map, reg,
 					 d->mask_buf_def[i], d->mask_buf[i]);
 		if (ret != 0)
 			dev_err(d->map->dev, "Failed to sync masks in %x\n",
-				d->chip->mask_base + (i * map->reg_stride));
+				reg);
 	}
 
 	/* If we've changed our wakeup count propagate it to the parent */
@@ -144,6 +145,7 @@ static irqreturn_t regmap_irq_thread(int irq, void *d)
 	struct regmap *map = data->map;
 	int ret, i;
 	bool handled = false;
+	u32 reg;
 
 	/*
 	 * Ignore masked IRQs and ack if we need to; we ack early so
@@ -166,14 +168,12 @@ static irqreturn_t regmap_irq_thread(int irq, void *d)
 		data->status_buf[i] &= ~data->mask_buf[i];
 
 		if (data->status_buf[i] && chip->ack_base) {
-			ret = regmap_write(map, chip->ack_base +
-						(i * map->reg_stride *
-						data->irq_reg_stride),
-					   data->status_buf[i]);
+			reg = chip->ack_base +
+				(i * map->reg_stride * data->irq_reg_stride);
+			ret = regmap_write(map, reg, data->status_buf[i]);
 			if (ret != 0)
 				dev_err(map->dev, "Failed to ack 0x%x: %d\n",
-					chip->ack_base + (i * map->reg_stride),
-					ret);
+					reg, ret);
 		}
 	}
 
@@ -238,6 +238,7 @@ int regmap_add_irq_chip(struct regmap *map, int irq, int irq_flags,
 	struct regmap_irq_chip_data *d;
 	int i;
 	int ret = -ENOMEM;
+	u32 reg;
 
 	for (i = 0; i < chip->num_irqs; i++) {
 		if (chip->irqs[i].reg_offset % map->reg_stride)
@@ -303,12 +304,12 @@ int regmap_add_irq_chip(struct regmap *map, int irq, int irq_flags,
 	/* Mask all the interrupts by default */
 	for (i = 0; i < chip->num_regs; i++) {
 		d->mask_buf[i] = d->mask_buf_def[i];
-		ret = regmap_write(map, chip->mask_base + (i * map->reg_stride
-				   * d->irq_reg_stride),
-				   d->mask_buf[i]);
+		reg = chip->mask_base +
+			(i * map->reg_stride * d->irq_reg_stride);
+		ret = regmap_write(map, reg, d->mask_buf[i]);
 		if (ret != 0) {
 			dev_err(map->dev, "Failed to set masks in 0x%x: %d\n",
-				chip->mask_base + (i * map->reg_stride), ret);
+				reg, ret);
 			goto err_alloc;
 		}
 	}
-- 
1.7.0.4

[PATCH 2/3] regmap: implement irq chip suspend/resume operations

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

When suspending, we set up the wake mask registers as required. Some
chips don't have separate wake mask registers, so they set mask_base
equal to wake_base. In that case, when resuming, we re-program the
interrupt enable registers based on enable state rather than wake state.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 drivers/base/regmap/regmap-irq.c |   62 ++++++++++++++++++++++++++++++++++++++
 1 files changed, 62 insertions(+), 0 deletions(-)

diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c
index c7e5b18..6bb4364 100644
--- a/drivers/base/regmap/regmap-irq.c
+++ b/drivers/base/regmap/regmap-irq.c
@@ -129,6 +129,61 @@ static int regmap_irq_set_wake(struct irq_data *data, unsigned int on)
 	return 0;
 }
 
+static void regmap_irq_suspend(struct irq_data *data)
+{
+	struct regmap_irq_chip_data *d = irq_data_get_irq_chip_data(data);
+	struct regmap *map = d->map;
+	int i, ret;
+	u32 reg;
+
+	if (!d->wake_buf)
+		return;
+
+	/*
+	 * Mask all non-wakeup interrupts. This writes to wake_base, which
+	 * may be identical to mask_base.
+	 */
+	for (i = 0; i < d->chip->num_regs; i++) {
+		reg = d->chip->wake_base +
+			(i * map->reg_stride * d->irq_reg_stride);
+		ret = regmap_update_bits(d->map, reg, d->mask_buf_def[i],
+					 d->wake_buf[i]);
+		if (ret != 0)
+			dev_err(d->map->dev, "Failed to sync masks in %x\n",
+				reg);
+	}
+}
+
+static void regmap_irq_resume(struct irq_data *data)
+{
+	struct regmap_irq_chip_data *d = irq_data_get_irq_chip_data(data);
+	struct regmap *map = d->map;
+	int i, ret;
+	u32 reg;
+
+	if (!d->wake_buf)
+		return;
+
+	/*
+	 * If mask_base is a separate register to wake_base, then
+	 * regmap_irq_suspend() didn't trash the enable registers,
+	 * so there's no need to restore them.
+	 */
+	if (d->chip->mask_base != d->chip->wake_base)
+		return;
+
+	/* Mask all non-enabled interrupts */
+	for (i = 0; i < d->chip->num_regs; i++) {
+		reg = d->chip->mask_base +
+			(i * map->reg_stride * d->irq_reg_stride);
+		ret = regmap_update_bits(d->map, reg, d->mask_buf_def[i],
+					 d->mask_buf[i]);
+		if (ret != 0)
+			dev_err(d->map->dev, "Failed to sync masks in %x\n",
+				reg);
+	}
+}
+
 static struct irq_chip regmap_irq_chip = {
 	.name			= "regmap",
 	.irq_bus_lock		= regmap_irq_lock,
@@ -136,6 +191,8 @@ static struct irq_chip regmap_irq_chip = {
 	.irq_disable		= regmap_irq_disable,
 	.irq_enable		= regmap_irq_enable,
 	.irq_set_wake		= regmap_irq_set_wake,
+	.irq_suspend		= regmap_irq_suspend,
+	.irq_resume		= regmap_irq_resume,
 };
 
 static irqreturn_t regmap_irq_thread(int irq, void *d)
@@ -314,6 +371,11 @@ int regmap_add_irq_chip(struct regmap *map, int irq, int irq_flags,
 		}
 	}
 
+	/* Wake is disabled by default */
+	if (d->wake_buf)
+		for (i = 0; i < chip->num_regs; i++)
+			d->wake_buf[i] = d->mask_buf_def[i];
+
 	if (irq_base)
 		d->domain = irq_domain_add_legacy(map->dev->of_node,
 						  chip->num_irqs, irq_base, 0,
-- 
1.7.0.4

[PATCH 3/3] regmap: enhance regmap-irq to handle 1 IRQ feeding n chips

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

Some devices contain a single interrupt output, and multiple separate
interrupt controllers that all trigger that interrupt output, yet provide
no top-level interrupt controller/registers to allow determination of
which child interrupt controller caused the interrupt.

In this case, a regmap irq_chip can be created for each of the individual
"child" interrupt controllers, alongside some infra-structure to hook the
interrupt and distribute it to each "child". This patch introduces
regmap_add_irq_chips() which sets up such infra-structure, and creates
a number of child regmap irq_chips.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 drivers/base/regmap/regmap-irq.c |  187 ++++++++++++++++++++++++++++++++++++++
 include/linux/regmap.h           |    9 ++
 2 files changed, 196 insertions(+), 0 deletions(-)

diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c
index 6bb4364..9d7894c 100644
--- a/drivers/base/regmap/regmap-irq.c
+++ b/drivers/base/regmap/regmap-irq.c
@@ -2,6 +2,7 @@
  * regmap based irq_chip
  *
  * Copyright 2011 Wolfson Microelectronics plc
+ * Copyright (c) 2012, NVIDIA CORPORATION.  All rights reserved.
  *
  * Author: Mark Brown <broonie@opensource.wolfsonmicro.com>
  *
@@ -16,6 +17,7 @@
 #include <linux/irq.h>
 #include <linux/interrupt.h>
 #include <linux/irqdomain.h>
+#include <linux/pm_runtime.h>
 #include <linux/slab.h>
 
 #include "internal.h"
@@ -40,6 +42,14 @@ struct regmap_irq_chip_data {
 	unsigned int irq_reg_stride;
 };
 
+struct regmap_irq_chips_data {
+	struct device *dev;
+	int irq;
+	int nchips;
+	struct irq_domain *irqdom;
+	struct regmap_irq_chip_data **datas;
+};
+
 static inline const
 struct regmap_irq *irq_to_regmap_irq(struct regmap_irq_chip_data *data,
 				     int irq)
@@ -463,3 +473,180 @@ int regmap_irq_get_virq(struct regmap_irq_chip_data *data, int irq)
 	return irq_create_mapping(data->domain, irq);
 }
 EXPORT_SYMBOL_GPL(regmap_irq_get_virq);
+
+static void regmaps_irq_enable(struct irq_data *data)
+{
+}
+
+static void regmaps_irq_disable(struct irq_data *data)
+{
+}
+
+static struct irq_chip regmaps_irq_chip = {
+	.name		= "regmaps",
+	.irq_disable	= regmaps_irq_disable,
+	.irq_enable	= regmaps_irq_enable,
+};
+
+static int regmaps_irq_map(struct irq_domain *h, unsigned int virq,
+			   irq_hw_number_t hw)
+{
+	struct regmap_irq_chips_data *data = h->host_data;
+
+	irq_set_chip_data(virq, data);
+	irq_set_chip_and_handler(virq, &regmaps_irq_chip, handle_edge_irq);
+	irq_set_nested_thread(virq, 1);
+
+	/* ARM needs us to explicitly flag the IRQ as valid
+	 * and will set them noprobe when we do so. */
+#ifdef CONFIG_ARM
+	set_irq_flags(virq, IRQF_VALID);
+#else
+	irq_set_noprobe(virq);
+#endif
+
+	return 0;
+}
+
+static struct irq_domain_ops regmaps_domain_ops = {
+	.map	= regmaps_irq_map,
+};
+
+static irqreturn_t regmaps_irq_thread(int irq, void *data)
+{
+	struct regmap_irq_chips_data *d = data;
+	int ret, i;
+
+	ret = pm_runtime_get_sync(d->dev);
+	if (ret < 0) {
+		dev_err(d->dev, "Failed to resume device: %d\n", ret);
+		return IRQ_NONE;
+	}
+
+	for (i = 0; i < d->nchips; i++)
+		handle_nested_irq(irq_find_mapping(d->irqdom, i));
+
+	pm_runtime_mark_last_busy(d->dev);
+	pm_runtime_put_autosuspend(d->dev);
+
+	return IRQ_HANDLED;
+}
+
+/**
+ * regmap_add_irq_chips(): Call regmap_add_irq_chip for n chips on one IRQ
+ *
+ * @irq:       Primary IRQ for the device
+ * @irq_flags: The IRQF_ flags to use for the primary interrupt.
+ * @nchips:    The number of IRQ chips attached to the interrupt.
+ * @maps:      The regmap for each IRQ chip.
+ * @irq_bases  The base Linux IRQ number for each chip, or NULL.
+ * @chips:     Configuration for each interrupt controller.
+ * @data:      Runtime data structure set of controllers, allocated on success
+ *
+ * Some devices contain a single interrupt output, and multiple separate
+ * interrupt controllers that all trigger that interrupt output, yet provide
+ * no top-level interrupt controller to allow determination of which child
+ * interrupt controller caused the interrupt. regmap_add_irq_chips() creates
+ * the required N regmap irq_chips, and handles demultiplexing this virtual
+ * top-level interrupt.
+ *
+ * Returns 0 on success or an errno on failure.
+ *
+ * In order for this to be efficient the chip really should use a
+ * register cache.  The chip driver is responsible for restoring the
+ * register values used by the IRQ controller over suspend and resume.
+ */
+int regmap_add_irq_chips(int irq, int irq_flags, int nchips,
+			 struct regmap **maps, int *irq_bases,
+			 const struct regmap_irq_chip **chips,
+			 struct regmap_irq_chips_data **data)
+{
+	int ret = -ENOMEM;
+	struct regmap_irq_chips_data *d;
+	int i;
+
+	d = kzalloc(sizeof(*d), GFP_KERNEL);
+	if (!d)
+		return -ENOMEM;
+
+	d->dev = maps[0]->dev;
+	d->irq = irq;
+	d->nchips = nchips;
+
+	d->datas = kzalloc(nchips * sizeof(*(d->datas)), GFP_KERNEL);
+	if (!d->datas)
+		goto err_alloc;
+
+	d->irqdom = irq_domain_add_linear(NULL, nchips, &regmaps_domain_ops, d);
+	if (!d->irqdom) {
+		ret = -EINVAL;
+		goto err_alloc;
+	}
+
+	for (i = 0; i < nchips; i++) {
+		ret = regmap_add_irq_chip(maps[i],
+					  irq_create_mapping(d->irqdom, i),
+					  IRQF_ONESHOT,
+					  irq_bases ? irq_bases[i] : 0,
+					  chips[i], &d->datas[i]);
+		if (ret != 0) {
+			dev_err(maps[i]->dev,
+				"Failed to add chip %d IRQs: %d\n", i, ret);
+			goto err_chips;
+		}
+	}
+
+	ret = request_threaded_irq(irq, NULL, regmaps_irq_thread,
+				   irq_flags, dev_name(d->dev), d);
+	if (ret != 0) {
+		dev_err(d->dev, "Failed to request IRQ %d: %d\n", irq, ret);
+		goto err_chips;
+	}
+
+	*data = d;
+
+	return 0;
+
+err_chips:
+	for (i--; i >= 0; i--)
+		regmap_del_irq_chip(irq_create_mapping(d->irqdom, i),
+				    d->datas[i]);
+	/* We should unmap the domain but... */
+err_alloc:
+	kfree(d->datas);
+	kfree(d);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(regmap_add_irq_chips);
+
+/**
+ * regmap_del_irq_chips(): Undo regmap_add_irq_chips()
+ *
+ * @irq:    Primary IRQ for the device
+ * @d:      regmap_irq_chips_data allocated by regmap_add_irq_chips()
+ */
+void regmap_del_irq_chips(struct regmap_irq_chips_data *d)
+{
+	int i;
+
+	free_irq(d->irq, d);
+
+	for (i = d->nchips - 1; i >= 0; i--)
+		regmap_del_irq_chip(irq_create_mapping(d->irqdom, i),
+				    d->datas[i]);
+
+	/* We should unmap the domain but... */
+
+	kfree(d->datas);
+	kfree(d);
+}
+EXPORT_SYMBOL_GPL(regmap_del_irq_chips);
+
+struct regmap_irq_chip_data *regmap_irq_chips_get_chip(
+				struct regmap_irq_chips_data *d, int chip)
+{
+	if (chip >= d->nchips)
+		return NULL;
+	return d->datas[chip];
+}
+EXPORT_SYMBOL(regmap_irq_chips_get_chip);
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index 7f7e00d..b5d9699 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -307,6 +307,7 @@ struct regmap_irq_chip {
 };
 
 struct regmap_irq_chip_data;
+struct regmap_irq_chips_data;
 
 int regmap_add_irq_chip(struct regmap *map, int irq, int irq_flags,
 			int irq_base, const struct regmap_irq_chip *chip,
@@ -315,6 +316,14 @@ void regmap_del_irq_chip(int irq, struct regmap_irq_chip_data *data);
 int regmap_irq_chip_get_base(struct regmap_irq_chip_data *data);
 int regmap_irq_get_virq(struct regmap_irq_chip_data *data, int irq);
 
+int regmap_add_irq_chips(int irq, int irq_flags, int nchips,
+			 struct regmap **maps, int *irq_bases,
+			 const struct regmap_irq_chip **chips,
+			 struct regmap_irq_chips_data **data);
+void regmap_del_irq_chips(struct regmap_irq_chips_data *d);
+struct regmap_irq_chip_data *regmap_irq_chips_get_chip(
+				struct regmap_irq_chips_data *d, int chip);
+
 #else
 
 /*
-- 
1.7.0.4

Re: [PATCH 3/3] regmap: enhance regmap-irq to handle 1 IRQ feeding n chips

[Mark Brown]

On Fri, Jul 27, 2012 at 01:01:56PM -0600, Stephen Warren wrote:
> From: Stephen Warren <swarren@nvidia.com>
> 
> Some devices contain a single interrupt output, and multiple separate
> interrupt controllers that all trigger that interrupt output, yet provide
> no top-level interrupt controller/registers to allow determination of
> which child interrupt controller caused the interrupt.

This isn't really anything to do with regmap, it's about implementing
shared IRQ support for threaded interrupts.  This is generally useful
and shouldn't be tied to regmap, it's common enough for hardware
designers to want to use wired or interrupts and it's a limitation of
Linux that it can't cope currently.

If are were going to implement it in regmap we shouldn't be faffing
around setting up the virtual interrupts, we should just do the right
thing and call round all the chips without bouncing it through the IRQ
core.

>   * Copyright 2011 Wolfson Microelectronics plc
> + * Copyright (c) 2012, NVIDIA CORPORATION.  All rights reserved.

All rights reserved?  Hrm...

> +static irqreturn_t regmaps_irq_thread(int irq, void *data)
> +{
> +	struct regmap_irq_chips_data *d = data;
> +	int ret, i;
> +
> +	ret = pm_runtime_get_sync(d->dev);
> +	if (ret < 0) {

This is conditional in the core regmap runtime PM support, it may be
actively harmful if the device doesn't need it.

Re: [PATCH 2/3] regmap: implement irq chip suspend/resume operations

[Mark Brown]

On Fri, Jul 27, 2012 at 01:01:55PM -0600, Stephen Warren wrote:

> When suspending, we set up the wake mask registers as required. Some
> chips don't have separate wake mask registers, so they set mask_base
> equal to wake_base. In that case, when resuming, we re-program the

No, they shouldn't be doing that at all - that's at best confused.  The
two registers do different things and if the two ranges are set the same
then I'd not expect things to work.  Supporting that would make the code
more complex and I'm not sure what benefit we might gain from it.

Re: [PATCH 3/3] regmap: enhance regmap-irq to handle 1 IRQ feeding n chips

[Stephen Warren]

On 07/29/2012 02:36 PM, Mark Brown wrote:
> On Fri, Jul 27, 2012 at 01:01:56PM -0600, Stephen Warren wrote:
>> From: Stephen Warren <swarren@nvidia.com>
>>
>> Some devices contain a single interrupt output, and multiple separate
>> interrupt controllers that all trigger that interrupt output, yet provide
>> no top-level interrupt controller/registers to allow determination of
>> which child interrupt controller caused the interrupt.
> 
> This isn't really anything to do with regmap, it's about implementing
> shared IRQ support for threaded interrupts.  This is generally useful
> and shouldn't be tied to regmap, it's common enough for hardware
> designers to want to use wired or interrupts and it's a limitation of
> Linux that it can't cope currently.
> 
> If are were going to implement it in regmap we shouldn't be faffing
> around setting up the virtual interrupts, we should just do the right
> thing and call round all the chips without bouncing it through the IRQ
> core.

OK, so more like how the max8907.c patch I posted did it than the
pre-existing arizona.c that I converted did it.

I had implemented this in regmap since you'd specifically mentioned
doing that. If I convert the code not to use separate IRQ domains for
this, would that be acceptable?

>> +static irqreturn_t regmaps_irq_thread(int irq, void *data)
>> +{
>> +	struct regmap_irq_chips_data *d = data;
>> +	int ret, i;
>> +
>> +	ret = pm_runtime_get_sync(d->dev);
>> +	if (ret < 0) {
> 
> This is conditional in the core regmap runtime PM support, it may be
> actively harmful if the device doesn't need it.

Hmmm. I actually don't see any pm_*() usage in regmap right now. I
assume this /is/ needed to convert arizona.c, since it's making these
calls today. I don't need it for max8907.c. Should I add another flag to
regmap_add_irq_chips() indicating whether this is needed, or ...?

Re: [PATCH 2/3] regmap: implement irq chip suspend/resume operations

[Stephen Warren]

On 07/29/2012 03:04 PM, Mark Brown wrote:
> On Fri, Jul 27, 2012 at 01:01:55PM -0600, Stephen Warren wrote:
> 
>> When suspending, we set up the wake mask registers as required. Some
>> chips don't have separate wake mask registers, so they set mask_base
>> equal to wake_base. In that case, when resuming, we re-program the
> 
> No, they shouldn't be doing that at all - that's at best confused.  The
> two registers do different things and if the two ranges are set the same
> then I'd not expect things to work.  Supporting that would make the code
> more complex and I'm not sure what benefit we might gain from it.

So this change was re" your comment "This loop we should just port over
into the regmap code." at http://lkml.org/lkml/2012/7/26/466.

I believe the idea is that the chip has an interrupt output from n
sources. Only some of those n sources should trigger a wakeup from
sleep. Hence, the max8907 driver was writing out the "sleep enables" to
the enable registers whenever entering sleep, so that any other IRQ
sources within the chip didn't trigger the chip's interrupt output and
hence exit sleep. If we are to port that code into the regmap-irq core,
it seems to make sense to have enable_base==wake_base, since the same
register truly is being used for both enable/wakeup-enable, just
time-multiplexed.

Or, perhaps the IRQ core already disables all non-wake interrupts for
us, so the driver doesn't have to do this, and we can just drop that
code completely?

Re: [PATCH 3/3] regmap: enhance regmap-irq to handle 1 IRQ feeding n chips

[Mark Brown]

On Mon, Jul 30, 2012 at 11:00:04AM -0600, Stephen Warren wrote:
> On 07/29/2012 02:36 PM, Mark Brown wrote:
> > On Fri, Jul 27, 2012 at 01:01:56PM -0600, Stephen Warren wrote:

> I had implemented this in regmap since you'd specifically mentioned
> doing that. If I convert the code not to use separate IRQ domains for

I think what I'd said was that we should factor it out rather than that
it should be specifically done in regmap.

> this, would that be acceptable?

Probably.

> >> +	ret = pm_runtime_get_sync(d->dev);
> >> +	if (ret < 0) {

> > This is conditional in the core regmap runtime PM support, it may be
> > actively harmful if the device doesn't need it.

> Hmmm. I actually don't see any pm_*() usage in regmap right now. I
> assume this /is/ needed to convert arizona.c, since it's making these
> calls today. I don't need it for max8907.c. Should I add another flag to
> regmap_add_irq_chips() indicating whether this is needed, or ...?

It's not in -next yet due to the merge window.  There's already a flag
for it in the irq chip data.

Re: [PATCH 2/3] regmap: implement irq chip suspend/resume operations

[Mark Brown]

On Mon, Jul 30, 2012 at 11:10:30AM -0600, Stephen Warren wrote:

> hence exit sleep. If we are to port that code into the regmap-irq core,
> it seems to make sense to have enable_base==wake_base, since the same
> register truly is being used for both enable/wakeup-enable, just
> time-multiplexed.

This would mean that we have to go round every single driver that
doesn't have physical wake support and add a setting for the wake
registers (which seems pointless given that the core can just as well
figure this out from the fact that it's not had any wake registers
specified) and we then have to add special cases for this in the core
code.  This doesn't seem like great API design, it's not conveneint for
either side of the interface and it's error prone.

> Or, perhaps the IRQ core already disables all non-wake interrupts for
> us, so the driver doesn't have to do this, and we can just drop that
> code completely?

IIRC it does actually do this, I'd need to check though.

Re: [PATCH 2/3] regmap: implement irq chip suspend/resume operations

[Stephen Warren]

On 07/30/2012 11:38 AM, Mark Brown wrote:
> On Mon, Jul 30, 2012 at 11:10:30AM -0600, Stephen Warren wrote:
> 
>> hence exit sleep. If we are to port that code into the regmap-irq core,
>> it seems to make sense to have enable_base==wake_base, since the same
>> register truly is being used for both enable/wakeup-enable, just
>> time-multiplexed.
> 
> This would mean that we have to go round every single driver that
> doesn't have physical wake support and add a setting for the wake
> registers (which seems pointless given that the core can just as well
> figure this out from the fact that it's not had any wake registers
> specified) and we then have to add special cases for this in the core
> code.  This doesn't seem like great API design, it's not conveneint for
> either side of the interface and it's error prone.
> 
>> Or, perhaps the IRQ core already disables all non-wake interrupts for
>> us, so the driver doesn't have to do this, and we can just drop that
>> code completely?
> 
> IIRC it does actually do this, I'd need to check though.

It looks like the answer here is to set irq_chip flags
IRQCHIP_MASK_ON_SUSPEND and IRQCHIP_SKIP_SET_WAKE. Perhaps regmap-irq
should do this automatically if (!regmap_irq_chip.wake_base)?

For reference, these flags are implemented in
kernel/irq/pm.c:check_wakeup_irqs() and
kernel/irq/manage.c:set_irq_wake_real().

Re: [PATCH 3/3] regmap: enhance regmap-irq to handle 1 IRQ feeding n chips

[Stephen Warren]

On 07/30/2012 11:25 AM, Mark Brown wrote:
> On Mon, Jul 30, 2012 at 11:00:04AM -0600, Stephen Warren wrote:
>> On 07/29/2012 02:36 PM, Mark Brown wrote:
>>> On Fri, Jul 27, 2012 at 01:01:56PM -0600, Stephen Warren wrote:
> 
>> I had implemented this in regmap since you'd specifically mentioned
>> doing that. If I convert the code not to use separate IRQ domains for
> 
> I think what I'd said was that we should factor it out rather than that
> it should be specifically done in regmap.
> 
>> this, would that be acceptable?
> 
> Probably.

The more I think about this, the more I prefer the way the way it is in
the patch I posted.

I don't think it's appropriate to put this support into the IRQ core.
The main issue is that all the handlers for any shared wired-or
interrupt line have to be registered before the IRQ is enabled, to avoid
some initially active interrupt continually firing before the IRQ is
enabled. Co-ordinating this when the wired-or line is on a board outside
a device driver rather than internal to a chip and one device driver is
a bit more than the IRQ core should probably be doing, hence I imagine
why it doesn't support it.

Co-ordinating this setup where all the sources of the wired-or are in
one chip seems to belong to the chip driver, which is where my patch did
this.

I guess I could modify regmaps_irq_thread() so that instead of:

	for (i = 0; i < d->nchips; i++)
		handle_nested_irq(irq_find_mapping(d->irqdom, i));

... it short-circuited and instead did something like:

	for (i = 0; i < d->nchips; i++)
		regmap_irq_thread(irq_find_mapping(d->irqdom, i),
			d->datas[i]);

but it seems a little hokey to short-circuit the IRQ core; it would
prevent execution of any statistics gathering or stuck interrupt
handling that handle_nested_irq() might do for example.

Now, if we made each child regmap_irq not be its own IRQ domain or
irq_chip, but simply had one top-level domain/chip that aggregated them,
that argument would be moot. However, that top-level domain/chip would
become rather complex and just end up doing a bunch of demultiplexing
code that's not needed if we do it like in my patch...

Re: [PATCH 3/3] regmap: enhance regmap-irq to handle 1 IRQ feeding n chips

[Mark Brown]

On Tue, Jul 31, 2012 at 05:18:36PM -0600, Stephen Warren wrote:

> I don't think it's appropriate to put this support into the IRQ core.
> The main issue is that all the handlers for any shared wired-or
> interrupt line have to be registered before the IRQ is enabled, to avoid
> some initially active interrupt continually firing before the IRQ is
> enabled. Co-ordinating this when the wired-or line is on a board outside
> a device driver rather than internal to a chip and one device driver is
> a bit more than the IRQ core should probably be doing, hence I imagine
> why it doesn't support it.

No, that's not the issue at all - none of the above is at all different
to any other shared interrupt and obviously we support shared IRQs quite
happily (we wouldn't run on a good chunk of PCs if we didn't).  Shared
interrupts do require the hardware design not be insane but generally
hardware engineers do manage to get that right.

We don't support this for threaded IRQs due to thorny synchronisation
issues in fast paths.

> Co-ordinating this setup where all the sources of the wired-or are in
> one chip seems to belong to the chip driver, which is where my patch did
> this.

Well, no.  It did this by having a piece of framework code add a round
robin irq_chip (essentially a shared threaded IRQ) layered on top of the
existing regmap-irq code which had nothing to do with the rest of that
code.  There's nothing at all about that framework code which is at all
specific to regmap-irq, it just calls a series of sub IRQs every time
the primary IRQ goes off.

This isn't the chip driver that's doing things, it's the regmap-irq
code.  With the current round robin implementation there's no reason for
regmap to implement it, other things can quite happily do the same thing.
Having a regmap helper which used a generic facility would be reasonable
but the actual demux is a generic thing.

[Suggestion to not bounce back into the IRQ core]

> but it seems a little hokey to short-circuit the IRQ core; it would
> prevent execution of any statistics gathering or stuck interrupt
> handling that handle_nested_irq() might do for example.

This seems like a better approach if doing things entirely in regmap.  I
can't see any impact on any of the IRQ core features here, we're always
going to call each of the sub IRQs exactly once for each call to the IRQ
handler and the stuck IRQ code is still going to identify the same set
of real IRQs as stuck.

> Now, if we made each child regmap_irq not be its own IRQ domain or
> irq_chip, but simply had one top-level domain/chip that aggregated them,
> that argument would be moot. However, that top-level domain/chip would
> become rather complex and just end up doing a bunch of demultiplexing
> code that's not needed if we do it like in my patch...

That demultiplexing seems excessively complex, yes.

Re: [PATCH 1/3] regmap: fix some error messages to take account of irq_reg_stride

[Mark Brown]

On Fri, Jul 27, 2012 at 01:01:54PM -0600, Stephen Warren wrote:
> From: Stephen Warren <swarren@nvidia.com>
> 
> A number of places in the code were printing error messages that included
> the address of a register, but were not calculating the register address
> in the same way as the access to the register. Use a temporary to solve
> this.

Applied, thanks.

Re: [PATCH 2/3] regmap: implement irq chip suspend/resume operations

[Mark Brown]

On Tue, Jul 31, 2012 at 01:25:15PM -0600, Stephen Warren wrote:

> It looks like the answer here is to set irq_chip flags
> IRQCHIP_MASK_ON_SUSPEND and IRQCHIP_SKIP_SET_WAKE. Perhaps regmap-irq
> should do this automatically if (!regmap_irq_chip.wake_base)?

Sounds like a plan.