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

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, d->mask_buf);
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 &= ~data->mask_buf;

if (data->status_buf && chip->ack_base) {
- ret = regmap_write(map, chip->ack_base +
- (i * map->reg_stride *
- data->irq_reg_stride),
- data->status_buf);
+ reg = chip->ack_base +
+ (i * map->reg_stride * data->irq_reg_stride);
+ ret = regmap_write(map, reg, data->status_buf);
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.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 = d->mask_buf_def;
- ret = regmap_write(map, chip->mask_base + (i * map->reg_stride
- * d->irq_reg_stride),
- d->mask_buf);
+ reg = chip->mask_base +
+ (i * map->reg_stride * d->irq_reg_stride);
+ ret = regmap_write(map, reg, d->mask_buf);
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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at [vger.kernel.org]
Please read the FAQ at [www.tux.org]

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,
+ irq_create_mapping(d->irqdom, i),
+ IRQF_ONESHOT,
+ irq_bases ? irq_bases : 0,
+ chips, &d->datas);
+ if (ret != 0) {
+ dev_err(maps->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);
+ /* 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);
+
+ /* 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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at [vger.kernel.org]
Please read the FAQ at [www.tux.org]

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at [vger.kernel.org]
Please read the FAQ at [www.tux.org]

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at [vger.kernel.org]
Please read the FAQ at [www.tux.org]