[PATCH v2 0/5] ARM: at91: fix irq_pm_install_action WARNING

[PATCH v2 0/5] ARM: at91: fix irq_pm_install_action WARNING

[Boris Brezillon]

Commit cab303be91dc47942bc25de33dc1140123540800 [1] introduced a WARN_ON
test which triggers a WARNING backtrace on at91 platforms.
While this WARN_ON is absolutely necessary to warn users that they should
not mix request with and without IRQF_NO_SUSPEND flags on shared IRQs,
there is no easy way to solve this issue on at91 platforms.

The main reason is that the init timer is often using a shared irq line
and thus request this irq with IRQF_NO_SUSPEND flag set, while other
peripherals request the same irq line without this flag.

As suggested by Thomas, the first 2 patches of this series add a dumb
demultiplexer irqchip implementation.
This demuxer takes register to a source interrupt and then forwards
all received interrupts to its children (it they are enabled).

Patches 3 to 5 are here for testing purpose, and should be generalized
to all impacted SoCs if the approach is accepted.

Best Regards,

Boris

Boris Brezillon (5):
  irqchip: add dumb demultiplexer implementation
  irqchip: Add DT binding doc for dumb demuxer chips
  ARM: at91/dt: select DUMB_IRQ_DEMUX for all at91 SoCs
  ARM: at91/dt: add AIC irq1 muxed peripheral id definitions
  ARM: at91/dt: define a dumb irq demultiplexer chip connected on irq1

 .../bindings/interrupt-controller/dumb-demux.txt   |  34 +++++
 arch/arm/boot/dts/at91sam9260.dtsi                 |  26 +++-
 arch/arm/mach-at91/Kconfig                         |   2 +
 drivers/irqchip/Kconfig                            |   4 +
 drivers/irqchip/Makefile                           |   1 +
 drivers/irqchip/irq-dumb-demux.c                   |  70 +++++++++++
 .../dt-bindings/interrupt-controller/atmel-aic.h   |  22 ++++
 include/linux/irq.h                                |  49 ++++++++
 include/linux/irqdomain.h                          |   1 +
 kernel/irq/Kconfig                                 |   5 +
 kernel/irq/Makefile                                |   1 +
 kernel/irq/chip.c                                  |  41 ++++++
 kernel/irq/dumb-demux-chip.c                       | 140 +++++++++++++++++++++
 kernel/irq/handle.c                                |  31 ++++-
 kernel/irq/internals.h                             |   3 +
 15 files changed, 422 insertions(+), 8 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt
 create mode 100644 drivers/irqchip/irq-dumb-demux.c
 create mode 100644 include/dt-bindings/interrupt-controller/atmel-aic.h
 create mode 100644 kernel/irq/dumb-demux-chip.c

-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 1/5] irqchip: add dumb demultiplexer implementation

[Boris Brezillon]

Some interrupt controllers are multiplexing several peripheral IRQs on
a single interrupt line.
While this is not a problem for most IRQs (as long as all peripherals
request the interrupt with IRQF_SHARED flag set), multiplexing timers and
other type of peripherals will generate a WARNING (mixing IRQF_NO_SUSPEND
and !IRQF_NO_SUSPEND is prohibited).

Create a dumb irq demultiplexer which simply forwards interrupts to all
peripherals (exactly what's happening with IRQ_SHARED) but keep a unique
irq number for each peripheral, thus preventing the IRQF_NO_SUSPEND
and !IRQF_NO_SUSPEND mix on a given interrupt.

Signed-off-by: Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 drivers/irqchip/Kconfig          |   4 ++
 drivers/irqchip/Makefile         |   1 +
 drivers/irqchip/irq-dumb-demux.c |  70 ++++++++++++++++++++
 include/linux/irq.h              |  49 ++++++++++++++
 include/linux/irqdomain.h        |   1 +
 kernel/irq/Kconfig               |   5 ++
 kernel/irq/Makefile              |   1 +
 kernel/irq/chip.c                |  41 ++++++++++++
 kernel/irq/dumb-demux-chip.c     | 140 +++++++++++++++++++++++++++++++++++++++
 kernel/irq/handle.c              |  31 ++++++++-
 kernel/irq/internals.h           |   3 +
 11 files changed, 344 insertions(+), 2 deletions(-)
 create mode 100644 drivers/irqchip/irq-dumb-demux.c
 create mode 100644 kernel/irq/dumb-demux-chip.c

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index cc79d2a..8a9df88 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -70,6 +70,10 @@ config BRCMSTB_L2_IRQ
 	select GENERIC_IRQ_CHIP
 	select IRQ_DOMAIN
 
+config DUMB_DEMUX_IRQ
+	bool
+	select DUMB_IRQ_DEMUX_CHIP
+
 config DW_APB_ICTL
 	bool
 	select GENERIC_IRQ_CHIP
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index 9516a32..77f3c51 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -8,6 +8,7 @@ obj-$(CONFIG_ARCH_MVEBU)		+= irq-armada-370-xp.o
 obj-$(CONFIG_ARCH_MXS)			+= irq-mxs.o
 obj-$(CONFIG_ARCH_S3C24XX)		+= irq-s3c24xx.o
 obj-$(CONFIG_DW_APB_ICTL)		+= irq-dw-apb-ictl.o
+obj-$(CONFIG_DUMB_DEMUX_IRQ)		+= irq-dumb-demux.o
 obj-$(CONFIG_METAG)			+= irq-metag-ext.o
 obj-$(CONFIG_METAG_PERFCOUNTER_IRQS)	+= irq-metag.o
 obj-$(CONFIG_ARCH_MOXART)		+= irq-moxart.o
diff --git a/drivers/irqchip/irq-dumb-demux.c b/drivers/irqchip/irq-dumb-demux.c
new file mode 100644
index 0000000..dfa05ce
--- /dev/null
+++ b/drivers/irqchip/irq-dumb-demux.c
@@ -0,0 +1,70 @@
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/irqdomain.h>
+#include <linux/of_irq.h>
+#include <linux/slab.h>
+
+#include "irqchip.h"
+
+static int __init dumb_irq_demux_of_init(struct device_node *node,
+					 struct device_node *parent)
+{
+	struct irq_chip_dumb_demux *demux;
+	unsigned int irq;
+	u32 valid_irqs;
+	int ret;
+
+	irq = irq_of_parse_and_map(node, 0);
+	if (!irq) {
+		pr_err("Failed to retrieve dumb irq demuxer source\n");
+		return -EINVAL;
+	}
+
+	ret = of_property_read_u32(node, "irqs", &valid_irqs);
+	if (ret) {
+		pr_err("Invalid of missing 'irqs' property\n");
+		return ret;
+	}
+
+	demux = irq_alloc_dumb_demux_chip(irq, valid_irqs,
+					  IRQ_NOREQUEST | IRQ_NOPROBE |
+					  IRQ_NOAUTOEN, 0, 0);
+	if (!demux) {
+		pr_err("Failed to allocate dumb irq demuxer struct\n");
+		return -ENOMEM;
+	}
+
+	demux->domain = irq_domain_add_linear(node, BITS_PER_LONG,
+					      &irq_dumb_demux_domain_ops,
+					      demux);
+	if (!demux->domain) {
+		ret = -ENOMEM;
+		goto err_free_demux;
+	}
+
+	ret = irq_set_handler_data(irq, demux);
+	if (ret) {
+		pr_err("Failed to assign handler data\n");
+		goto err_free_domain;
+	}
+
+	irq_set_chained_handler(irq, irq_dumb_demux_handler);
+
+	/*
+	 * Disable the src irq (automatically enabled by
+	 * irq_set_chained_handler) to prevent irqs from happening while
+	 * nobody requested any of the demuxed irqs.
+	 */
+	disable_irq(irq);
+
+	return 0;
+
+err_free_domain:
+	irq_domain_remove(demux->domain);
+
+err_free_demux:
+	kfree(demux);
+
+	return ret;
+}
+IRQCHIP_DECLARE(dumb_irq_demux, "irqchip-dumb-demux", dumb_irq_demux_of_init);
diff --git a/include/linux/irq.h b/include/linux/irq.h
index d09ec7a..ae8fa21 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -445,6 +445,10 @@ extern void handle_fasteoi_irq(unsigned int irq, struct irq_desc *desc);
 extern void handle_edge_irq(unsigned int irq, struct irq_desc *desc);
 extern void handle_edge_eoi_irq(unsigned int irq, struct irq_desc *desc);
 extern void handle_simple_irq(unsigned int irq, struct irq_desc *desc);
+#ifdef CONFIG_DUMB_IRQ_DEMUX_CHIP
+extern irqreturn_t handle_dumb_demux_irq(unsigned int irq,
+					 struct irq_desc *desc);
+#endif
 extern void handle_percpu_irq(unsigned int irq, struct irq_desc *desc);
 extern void handle_percpu_devid_irq(unsigned int irq, struct irq_desc *desc);
 extern void handle_bad_irq(unsigned int irq, struct irq_desc *desc);
@@ -862,4 +866,49 @@ static inline u32 irq_reg_readl(struct irq_chip_generic *gc,
 		return readl(gc->reg_base + reg_offset);
 }
 
+/**
+ * enum irq_dumb_demux_flags - Initialization flags for generic irq chips
+ * @IRQ_DD_INIT_NESTED_LOCK:	Set the lock class of the irqs to nested for
+ *				irq chips which need to call irq_set_wake() on
+ *				the parent irq. Usually GPIO implementations
+ */
+enum irq_dumb_demux_flags {
+	IRQ_DD_INIT_NESTED_LOCK		= 1 << 0,
+};
+
+#ifdef CONFIG_DUMB_IRQ_DEMUX_CHIP
+/**
+ * struct irq_chip_dumb_demux - Dumb demultiplexer irq chip data structure
+ * @domain:		irq domain pointer
+ * @available:		Bitfield of valid irqs
+ * @unmasked:		Bitfield containing irqs status
+ * @flags:		irq_dumb_demux_flags flags
+ * @src_irq:		irq feeding the dumb demux chip
+ *
+ * Note, that irq_chip_generic can have multiple irq_chip_type
+ * implementations which can be associated to a particular irq line of
+ * an irq_chip_generic instance. That allows to share and protect
+ * state in an irq_chip_generic instance when we need to implement
+ * different flow mechanisms (level/edge) for it.
+ */
+struct irq_chip_dumb_demux {
+	struct irq_domain *domain;
+	unsigned long available;
+	unsigned long unmasked;
+	unsigned int flags;
+	unsigned int src_irq;
+	unsigned int irq_flags_to_clear;
+	unsigned int irq_flags_to_set;
+};
+
+void irq_dumb_demux_handler(unsigned int irq, struct irq_desc *desc);
+
+struct irq_chip_dumb_demux *
+irq_alloc_dumb_demux_chip(unsigned int src_irq,
+			  unsigned long valid_irqs,
+			  unsigned int clr_flags,
+			  unsigned int set_flags,
+			  unsigned int dd_flags);
+#endif /* CONFIG_DUMB_IRQ_DEMUX_CHIP */
+
 #endif /* _LINUX_IRQ_H */
diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
index 676d730..1de3808 100644
--- a/include/linux/irqdomain.h
+++ b/include/linux/irqdomain.h
@@ -80,6 +80,7 @@ struct irq_domain_ops {
 };
 
 extern struct irq_domain_ops irq_generic_chip_ops;
+extern struct irq_domain_ops irq_dumb_demux_domain_ops;
 
 struct irq_domain_chip_generic;
 
diff --git a/kernel/irq/Kconfig b/kernel/irq/Kconfig
index 9a76e3b..d01554a 100644
--- a/kernel/irq/Kconfig
+++ b/kernel/irq/Kconfig
@@ -51,6 +51,11 @@ config GENERIC_IRQ_CHIP
        bool
        select IRQ_DOMAIN
 
+# Dumb interrupt demuxer chip implementation
+config DUMB_IRQ_DEMUX_CHIP
+	bool
+	select IRQ_DOMAIN
+
 # Generic irq_domain hw <--> linux irq number translation
 config IRQ_DOMAIN
 	bool
diff --git a/kernel/irq/Makefile b/kernel/irq/Makefile
index d121235..1cd4e42 100644
--- a/kernel/irq/Makefile
+++ b/kernel/irq/Makefile
@@ -7,3 +7,4 @@ obj-$(CONFIG_PROC_FS) += proc.o
 obj-$(CONFIG_GENERIC_PENDING_IRQ) += migration.o
 obj-$(CONFIG_PM_SLEEP) += pm.o
 obj-$(CONFIG_GENERIC_MSI_IRQ) += msi.o
+obj-$(CONFIG_DUMB_IRQ_DEMUX_CHIP) += dumb-demux-chip.o
diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index 6f1c7a5..d2a5c96 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -405,6 +405,47 @@ out_unlock:
 }
 EXPORT_SYMBOL_GPL(handle_simple_irq);
 
+#ifdef CONFIG_DUMB_IRQ_DEMUX_CHIP
+/**
+ *	handle_dumb_demux_irq - Dumb demuxer irq handle function.
+ *	@irq:	the interrupt number
+ *	@desc:	the interrupt description structure for this irq
+ *
+ *	Dumb demux interrupts are sent from a demultiplexing interrupt handler
+ *	which is not able to decide which child interrupt interrupt handler
+ *	should be called.
+ *
+ *	Note: The caller is expected to handle the ack, clear, mask and
+ *	unmask issues if necessary.
+ */
+irqreturn_t
+handle_dumb_demux_irq(unsigned int irq, struct irq_desc *desc)
+{
+	irqreturn_t retval = IRQ_NONE;
+
+	raw_spin_lock(&desc->lock);
+
+	if (!irq_may_run(desc))
+		goto out_unlock;
+
+	desc->istate &= ~(IRQS_REPLAY | IRQS_WAITING);
+	kstat_incr_irqs_this_cpu(irq, desc);
+
+	if (unlikely(!desc->action || irqd_irq_disabled(&desc->irq_data))) {
+		desc->istate |= IRQS_PENDING;
+		goto out_unlock;
+	}
+
+	retval = handle_irq_event_no_spurious_check(desc);
+
+out_unlock:
+	raw_spin_unlock(&desc->lock);
+
+	return retval;
+}
+EXPORT_SYMBOL_GPL(handle_dumb_demux_irq);
+#endif /* CONFIG_DUMB_IRQ_DEMUX_CHIP */
+
 /*
  * Called unconditionally from handle_level_irq() and only for oneshot
  * interrupts from handle_fasteoi_irq()
diff --git a/kernel/irq/dumb-demux-chip.c b/kernel/irq/dumb-demux-chip.c
new file mode 100644
index 0000000..8e2de1d
--- /dev/null
+++ b/kernel/irq/dumb-demux-chip.c
@@ -0,0 +1,140 @@
+/*
+ * Library implementing common dumb irq demux chip functions
+ *
+ * Copyright (C) 2015, Boris Brezillon
+ */
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/irq.h>
+#include <linux/slab.h>
+#include <linux/export.h>
+#include <linux/irq.h>
+#include <linux/irqdomain.h>
+#include <linux/irqchip/chained_irq.h>
+#include <linux/interrupt.h>
+#include <linux/kernel_stat.h>
+#include <linux/syscore_ops.h>
+
+#include "internals.h"
+
+static void irq_dumb_demux_mask(struct irq_data *d)
+{
+	struct irq_chip_dumb_demux *demux = irq_data_get_irq_chip_data(d);
+
+	clear_bit(d->hwirq, &demux->unmasked);
+
+	if (!demux->unmasked)
+		disable_irq_nosync(demux->src_irq);
+}
+
+static void irq_dumb_demux_unmask(struct irq_data *d)
+{
+	struct irq_chip_dumb_demux *demux = irq_data_get_irq_chip_data(d);
+	bool enable_src_irq = !demux->unmasked;
+
+	set_bit(d->hwirq, &demux->unmasked);
+
+	if (enable_src_irq)
+		enable_irq(demux->src_irq);
+}
+
+static struct irq_chip irq_dumb_demux_chip = {
+	.name =		"dumb-demux-irq",
+	.irq_mask =	irq_dumb_demux_mask,
+	.irq_unmask =	irq_dumb_demux_unmask,
+};
+
+/*
+ * Separate lockdep class for interrupt chip which can nest irq_desc
+ * lock.
+ */
+static struct lock_class_key irq_nested_lock_class;
+
+/*
+ * irq_map_dumb_demux_chip - Map a dumb demux chip for an irq domain
+ */
+static int irq_map_dumb_demux_chip(struct irq_domain *d,
+				   unsigned int virq,
+				   irq_hw_number_t hw_irq)
+{
+	struct irq_chip_dumb_demux *demux = d->host_data;
+
+	if (!test_bit(hw_irq, &demux->available))
+		return -EINVAL;
+
+	if (demux->flags & IRQ_DD_INIT_NESTED_LOCK)
+		irq_set_lockdep_class(virq, &irq_nested_lock_class);
+
+	irq_set_chip(virq, &irq_dumb_demux_chip);
+	irq_set_chip_data(virq, demux);
+	irq_modify_status(virq, demux->irq_flags_to_clear,
+			  demux->irq_flags_to_set);
+
+	return 0;
+}
+
+struct irq_domain_ops irq_dumb_demux_domain_ops = {
+	.map	= irq_map_dumb_demux_chip,
+	.xlate	= irq_domain_xlate_onecell,
+};
+EXPORT_SYMBOL_GPL(irq_dumb_demux_domain_ops);
+
+/**
+ * irq_dumb_demux_handler - Dumb demux flow handler
+ * @irq:		Virtual irq number
+ * @irq_desc:		irq descriptor
+ */
+void irq_dumb_demux_handler(unsigned int irq, struct irq_desc *desc)
+{
+	struct irq_chip_dumb_demux *demux = irq_get_handler_data(irq);
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	irqreturn_t ret = IRQ_NONE;
+	int i;
+
+	chained_irq_enter(chip, desc);
+	for_each_set_bit(i, &demux->unmasked, BITS_PER_LONG) {
+		int demuxed_irq = irq_find_mapping(demux->domain, i);
+		struct irq_desc *desc = irq_to_desc(demuxed_irq);
+
+		ret |= handle_dumb_demux_irq(demuxed_irq, desc);
+	}
+	chained_irq_exit(chip, desc);
+
+	if (!noirqdebug)
+		note_interrupt(irq, desc, ret);
+}
+EXPORT_SYMBOL_GPL(irq_dumb_demux_handler);
+
+/**
+ * irq_alloc_dumb_demux_chip - Allocate a dumb demux chip
+ * @src_irq:		irq feeding the dumb demux chip
+ * @dd_flags:		irq_dumb_demux_flags flags
+ * @valid_irqs:		Bitmask representing valid irqs
+ * @clr_flags:		irq_flags to clear when mapping an interrupt
+ * @set_flags:		irq_flags to set when mapping an interrupt
+ */
+struct irq_chip_dumb_demux *
+irq_alloc_dumb_demux_chip(unsigned int src_irq,
+			  unsigned long valid_irqs,
+			  unsigned int clr_flags,
+			  unsigned int set_flags,
+			  unsigned int dd_flags)
+{
+	struct irq_chip_dumb_demux *demux;
+
+	if (!src_irq)
+		return ERR_PTR(-EINVAL);
+
+	demux = kzalloc(sizeof(*demux), GFP_KERNEL);
+	if (!demux)
+		return ERR_PTR(-ENOMEM);
+
+	demux->available = valid_irqs;
+	demux->flags = dd_flags;
+	demux->src_irq = src_irq;
+	demux->irq_flags_to_clear = clr_flags;
+	demux->irq_flags_to_set = set_flags;
+
+	return demux;
+}
+EXPORT_SYMBOL_GPL(irq_alloc_dumb_demux_chip);
diff --git a/kernel/irq/handle.c b/kernel/irq/handle.c
index 6354802..f786850 100644
--- a/kernel/irq/handle.c
+++ b/kernel/irq/handle.c
@@ -131,7 +131,8 @@ void __irq_wake_thread(struct irq_desc *desc, struct irqaction *action)
 }
 
 irqreturn_t
-handle_irq_event_percpu(struct irq_desc *desc, struct irqaction *action)
+handle_irq_event_percpu_no_spurious_check(struct irq_desc *desc,
+					  struct irqaction *action)
 {
 	irqreturn_t retval = IRQ_NONE;
 	unsigned int flags = 0, irq = desc->irq_data.irq;
@@ -175,8 +176,18 @@ handle_irq_event_percpu(struct irq_desc *desc, struct irqaction *action)
 
 	add_interrupt_randomness(irq, flags);
 
+	return retval;
+}
+
+irqreturn_t
+handle_irq_event_percpu(struct irq_desc *desc, struct irqaction *action)
+{
+	irqreturn_t retval;
+
+	retval = handle_irq_event_percpu_no_spurious_check(desc, action);
+
 	if (!noirqdebug)
-		note_interrupt(irq, desc, retval);
+		note_interrupt(desc->irq_data.irq, desc, retval);
 	return retval;
 }
 
@@ -195,3 +206,19 @@ irqreturn_t handle_irq_event(struct irq_desc *desc)
 	irqd_clear(&desc->irq_data, IRQD_IRQ_INPROGRESS);
 	return ret;
 }
+
+irqreturn_t handle_irq_event_no_spurious_check(struct irq_desc *desc)
+{
+	struct irqaction *action = desc->action;
+	irqreturn_t ret;
+
+	desc->istate &= ~IRQS_PENDING;
+	irqd_set(&desc->irq_data, IRQD_IRQ_INPROGRESS);
+	raw_spin_unlock(&desc->lock);
+
+	ret = handle_irq_event_percpu_no_spurious_check(desc, action);
+
+	raw_spin_lock(&desc->lock);
+	irqd_clear(&desc->irq_data, IRQD_IRQ_INPROGRESS);
+	return ret;
+}
diff --git a/kernel/irq/internals.h b/kernel/irq/internals.h
index df553b0..fe056fb 100644
--- a/kernel/irq/internals.h
+++ b/kernel/irq/internals.h
@@ -90,6 +90,9 @@ extern void init_kstat_irqs(struct irq_desc *desc, int node, int nr);
 
 irqreturn_t handle_irq_event_percpu(struct irq_desc *desc, struct irqaction *action);
 irqreturn_t handle_irq_event(struct irq_desc *desc);
+irqreturn_t handle_irq_event_percpu_no_spurious_check(struct irq_desc *desc,
+						      struct irqaction *action);
+irqreturn_t handle_irq_event_no_spurious_check(struct irq_desc *desc);
 
 /* Resending of interrupts :*/
 void check_irq_resend(struct irq_desc *desc, unsigned int irq);
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/5] irqchip: add dumb demultiplexer implementation

[Thomas Gleixner]

On Tue, 13 Jan 2015, Boris Brezillon wrote:
> +	ret = irq_set_handler_data(irq, demux);
> +	if (ret) {
> +		pr_err("Failed to assign handler data\n");
> +		goto err_free_domain;
> +	}
> +
> +	irq_set_chained_handler(irq, irq_dumb_demux_handler);
> +
> +	/*
> +	 * Disable the src irq (automatically enabled by
> +	 * irq_set_chained_handler) to prevent irqs from happening while
> +	 * nobody requested any of the demuxed irqs.
> +	 */
> +	disable_irq(irq);

We rather prevent the startup of the irq line right away.

enum {
     IRQ_CHAINED_NONE,
     IRQ_CHAINED_STARTUP,
     IRQ_CHAINED_NOSTARTUP,
};

-__irq_set_handler(unsigned int irq, irq_flow_handler_t handle, int is_chained,
-                  const char *name);
+__irq_set_handler(unsigned int irq, irq_flow_handler_t handle, int chained_mode,
+                  const char *name);

{
....
        if (handle != handle_bad_irq && chained_mode) {
                irq_settings_set_noprobe(desc);
                irq_settings_set_norequest(desc);
                irq_settings_set_nothread(desc);
-               irq_startup(desc, true);
+               if (chained_mode == IRQ_CHAINED_STARTUP)
+                       irq_startup(desc, true);
	}
....
}

Hmm?

	tglx

Re: [PATCH v2 1/5] irqchip: add dumb demultiplexer implementation

[Boris Brezillon]

Hi Thomas,

On Tue, 13 Jan 2015 22:00:55 +0100 (CET)
Thomas Gleixner <tglx@linutronix.de> wrote:

> On Tue, 13 Jan 2015, Boris Brezillon wrote:
> > +	ret = irq_set_handler_data(irq, demux);
> > +	if (ret) {
> > +		pr_err("Failed to assign handler data\n");
> > +		goto err_free_domain;
> > +	}
> > +
> > +	irq_set_chained_handler(irq, irq_dumb_demux_handler);
> > +
> > +	/*
> > +	 * Disable the src irq (automatically enabled by
> > +	 * irq_set_chained_handler) to prevent irqs from happening while
> > +	 * nobody requested any of the demuxed irqs.
> > +	 */
> > +	disable_irq(irq);
> 
> We rather prevent the startup of the irq line right away.

Yep, the comment was here to draw the attention, since I wasn't sure
modifying core code was the best solution.
Anyway I agree that keeping the src_irq disabled when registering the
chained handler is better than doing it afterwards.

> 
> enum {
>      IRQ_CHAINED_NONE,
>      IRQ_CHAINED_STARTUP,
>      IRQ_CHAINED_NOSTARTUP,
> };
> 
> -__irq_set_handler(unsigned int irq, irq_flow_handler_t handle, int is_chained,
> -                  const char *name);
> +__irq_set_handler(unsigned int irq, irq_flow_handler_t handle, int chained_mode,
> +                  const char *name);
> 
> {
> ....
>         if (handle != handle_bad_irq && chained_mode) {
>                 irq_settings_set_noprobe(desc);
>                 irq_settings_set_norequest(desc);
>                 irq_settings_set_nothread(desc);
> -               irq_startup(desc, true);
> +               if (chained_mode == IRQ_CHAINED_STARTUP)
> +                       irq_startup(desc, true);
> 	}
> ....
> }
> 
> Hmm?

I'm fine with this approach, with a static inline helper:

static inline void
irq_set_chained_handler_nostartup(unsigned int irq,
				  irq_flow_handler_t handle)
{
	__irq_set_handler(irq, handle, IRQ_CHAINED_NOSTARTUP, NULL);
}

Anyway, I'll wait a clear decision regarding which approach should be
taken (see my answer to Rob) before sending a new version.

Thanks for your reviews.

Best Regards,

Boris


-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH v2 1/5] irqchip: add dumb demultiplexer implementation

[Rob Herring]

On Tue, Jan 13, 2015 at 12:46 PM, Boris Brezillon
<boris.brezillon@free-electrons.com> wrote:
> Some interrupt controllers are multiplexing several peripheral IRQs on
> a single interrupt line.
> While this is not a problem for most IRQs (as long as all peripherals
> request the interrupt with IRQF_SHARED flag set), multiplexing timers and
> other type of peripherals will generate a WARNING (mixing IRQF_NO_SUSPEND
> and !IRQF_NO_SUSPEND is prohibited).
>
> Create a dumb irq demultiplexer which simply forwards interrupts to all
> peripherals (exactly what's happening with IRQ_SHARED) but keep a unique
> irq number for each peripheral, thus preventing the IRQF_NO_SUSPEND
> and !IRQF_NO_SUSPEND mix on a given interrupt.

This really seems like a work-around for how IRQF_SHARED works. It
seems like what is really desired is just per handler disabling. It is
fragile in that devices can deadlock the system if the drivers don't
disable the interrupt source before calling disable_irq. But unlike
IRQF_SHARED, there is nothing explicit in the driver indicating it is
designed to work properly with a shared interrupt line.

I see no reason to accept this into DT either. We already can support
shared lines and modeling an OR gate as an interrupt controller is
pointless.

Rob

>
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---
>  drivers/irqchip/Kconfig          |   4 ++
>  drivers/irqchip/Makefile         |   1 +
>  drivers/irqchip/irq-dumb-demux.c |  70 ++++++++++++++++++++
>  include/linux/irq.h              |  49 ++++++++++++++
>  include/linux/irqdomain.h        |   1 +
>  kernel/irq/Kconfig               |   5 ++
>  kernel/irq/Makefile              |   1 +
>  kernel/irq/chip.c                |  41 ++++++++++++
>  kernel/irq/dumb-demux-chip.c     | 140 +++++++++++++++++++++++++++++++++++++++
>  kernel/irq/handle.c              |  31 ++++++++-
>  kernel/irq/internals.h           |   3 +
>  11 files changed, 344 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/irqchip/irq-dumb-demux.c
>  create mode 100644 kernel/irq/dumb-demux-chip.c
>
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index cc79d2a..8a9df88 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -70,6 +70,10 @@ config BRCMSTB_L2_IRQ
>         select GENERIC_IRQ_CHIP
>         select IRQ_DOMAIN
>
> +config DUMB_DEMUX_IRQ
> +       bool
> +       select DUMB_IRQ_DEMUX_CHIP
> +
>  config DW_APB_ICTL
>         bool
>         select GENERIC_IRQ_CHIP
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index 9516a32..77f3c51 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -8,6 +8,7 @@ obj-$(CONFIG_ARCH_MVEBU)                += irq-armada-370-xp.o
>  obj-$(CONFIG_ARCH_MXS)                 += irq-mxs.o
>  obj-$(CONFIG_ARCH_S3C24XX)             += irq-s3c24xx.o
>  obj-$(CONFIG_DW_APB_ICTL)              += irq-dw-apb-ictl.o
> +obj-$(CONFIG_DUMB_DEMUX_IRQ)           += irq-dumb-demux.o
>  obj-$(CONFIG_METAG)                    += irq-metag-ext.o
>  obj-$(CONFIG_METAG_PERFCOUNTER_IRQS)   += irq-metag.o
>  obj-$(CONFIG_ARCH_MOXART)              += irq-moxart.o
> diff --git a/drivers/irqchip/irq-dumb-demux.c b/drivers/irqchip/irq-dumb-demux.c
> new file mode 100644
> index 0000000..dfa05ce
> --- /dev/null
> +++ b/drivers/irqchip/irq-dumb-demux.c
> @@ -0,0 +1,70 @@
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/of_irq.h>
> +#include <linux/slab.h>
> +
> +#include "irqchip.h"
> +
> +static int __init dumb_irq_demux_of_init(struct device_node *node,
> +                                        struct device_node *parent)
> +{
> +       struct irq_chip_dumb_demux *demux;
> +       unsigned int irq;
> +       u32 valid_irqs;
> +       int ret;
> +
> +       irq = irq_of_parse_and_map(node, 0);
> +       if (!irq) {
> +               pr_err("Failed to retrieve dumb irq demuxer source\n");
> +               return -EINVAL;
> +       }
> +
> +       ret = of_property_read_u32(node, "irqs", &valid_irqs);
> +       if (ret) {
> +               pr_err("Invalid of missing 'irqs' property\n");
> +               return ret;
> +       }
> +
> +       demux = irq_alloc_dumb_demux_chip(irq, valid_irqs,
> +                                         IRQ_NOREQUEST | IRQ_NOPROBE |
> +                                         IRQ_NOAUTOEN, 0, 0);
> +       if (!demux) {
> +               pr_err("Failed to allocate dumb irq demuxer struct\n");
> +               return -ENOMEM;
> +       }
> +
> +       demux->domain = irq_domain_add_linear(node, BITS_PER_LONG,
> +                                             &irq_dumb_demux_domain_ops,
> +                                             demux);
> +       if (!demux->domain) {
> +               ret = -ENOMEM;
> +               goto err_free_demux;
> +       }
> +
> +       ret = irq_set_handler_data(irq, demux);
> +       if (ret) {
> +               pr_err("Failed to assign handler data\n");
> +               goto err_free_domain;
> +       }
> +
> +       irq_set_chained_handler(irq, irq_dumb_demux_handler);
> +
> +       /*
> +        * Disable the src irq (automatically enabled by
> +        * irq_set_chained_handler) to prevent irqs from happening while
> +        * nobody requested any of the demuxed irqs.
> +        */
> +       disable_irq(irq);
> +
> +       return 0;
> +
> +err_free_domain:
> +       irq_domain_remove(demux->domain);
> +
> +err_free_demux:
> +       kfree(demux);
> +
> +       return ret;
> +}
> +IRQCHIP_DECLARE(dumb_irq_demux, "irqchip-dumb-demux", dumb_irq_demux_of_init);
> diff --git a/include/linux/irq.h b/include/linux/irq.h
> index d09ec7a..ae8fa21 100644
> --- a/include/linux/irq.h
> +++ b/include/linux/irq.h
> @@ -445,6 +445,10 @@ extern void handle_fasteoi_irq(unsigned int irq, struct irq_desc *desc);
>  extern void handle_edge_irq(unsigned int irq, struct irq_desc *desc);
>  extern void handle_edge_eoi_irq(unsigned int irq, struct irq_desc *desc);
>  extern void handle_simple_irq(unsigned int irq, struct irq_desc *desc);
> +#ifdef CONFIG_DUMB_IRQ_DEMUX_CHIP
> +extern irqreturn_t handle_dumb_demux_irq(unsigned int irq,
> +                                        struct irq_desc *desc);
> +#endif
>  extern void handle_percpu_irq(unsigned int irq, struct irq_desc *desc);
>  extern void handle_percpu_devid_irq(unsigned int irq, struct irq_desc *desc);
>  extern void handle_bad_irq(unsigned int irq, struct irq_desc *desc);
> @@ -862,4 +866,49 @@ static inline u32 irq_reg_readl(struct irq_chip_generic *gc,
>                 return readl(gc->reg_base + reg_offset);
>  }
>
> +/**
> + * enum irq_dumb_demux_flags - Initialization flags for generic irq chips
> + * @IRQ_DD_INIT_NESTED_LOCK:   Set the lock class of the irqs to nested for
> + *                             irq chips which need to call irq_set_wake() on
> + *                             the parent irq. Usually GPIO implementations
> + */
> +enum irq_dumb_demux_flags {
> +       IRQ_DD_INIT_NESTED_LOCK         = 1 << 0,
> +};
> +
> +#ifdef CONFIG_DUMB_IRQ_DEMUX_CHIP
> +/**
> + * struct irq_chip_dumb_demux - Dumb demultiplexer irq chip data structure
> + * @domain:            irq domain pointer
> + * @available:         Bitfield of valid irqs
> + * @unmasked:          Bitfield containing irqs status
> + * @flags:             irq_dumb_demux_flags flags
> + * @src_irq:           irq feeding the dumb demux chip
> + *
> + * Note, that irq_chip_generic can have multiple irq_chip_type
> + * implementations which can be associated to a particular irq line of
> + * an irq_chip_generic instance. That allows to share and protect
> + * state in an irq_chip_generic instance when we need to implement
> + * different flow mechanisms (level/edge) for it.
> + */
> +struct irq_chip_dumb_demux {
> +       struct irq_domain *domain;
> +       unsigned long available;
> +       unsigned long unmasked;
> +       unsigned int flags;
> +       unsigned int src_irq;
> +       unsigned int irq_flags_to_clear;
> +       unsigned int irq_flags_to_set;
> +};
> +
> +void irq_dumb_demux_handler(unsigned int irq, struct irq_desc *desc);
> +
> +struct irq_chip_dumb_demux *
> +irq_alloc_dumb_demux_chip(unsigned int src_irq,
> +                         unsigned long valid_irqs,
> +                         unsigned int clr_flags,
> +                         unsigned int set_flags,
> +                         unsigned int dd_flags);
> +#endif /* CONFIG_DUMB_IRQ_DEMUX_CHIP */
> +
>  #endif /* _LINUX_IRQ_H */
> diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
> index 676d730..1de3808 100644
> --- a/include/linux/irqdomain.h
> +++ b/include/linux/irqdomain.h
> @@ -80,6 +80,7 @@ struct irq_domain_ops {
>  };
>
>  extern struct irq_domain_ops irq_generic_chip_ops;
> +extern struct irq_domain_ops irq_dumb_demux_domain_ops;
>
>  struct irq_domain_chip_generic;
>
> diff --git a/kernel/irq/Kconfig b/kernel/irq/Kconfig
> index 9a76e3b..d01554a 100644
> --- a/kernel/irq/Kconfig
> +++ b/kernel/irq/Kconfig
> @@ -51,6 +51,11 @@ config GENERIC_IRQ_CHIP
>         bool
>         select IRQ_DOMAIN
>
> +# Dumb interrupt demuxer chip implementation
> +config DUMB_IRQ_DEMUX_CHIP
> +       bool
> +       select IRQ_DOMAIN
> +
>  # Generic irq_domain hw <--> linux irq number translation
>  config IRQ_DOMAIN
>         bool
> diff --git a/kernel/irq/Makefile b/kernel/irq/Makefile
> index d121235..1cd4e42 100644
> --- a/kernel/irq/Makefile
> +++ b/kernel/irq/Makefile
> @@ -7,3 +7,4 @@ obj-$(CONFIG_PROC_FS) += proc.o
>  obj-$(CONFIG_GENERIC_PENDING_IRQ) += migration.o
>  obj-$(CONFIG_PM_SLEEP) += pm.o
>  obj-$(CONFIG_GENERIC_MSI_IRQ) += msi.o
> +obj-$(CONFIG_DUMB_IRQ_DEMUX_CHIP) += dumb-demux-chip.o
> diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
> index 6f1c7a5..d2a5c96 100644
> --- a/kernel/irq/chip.c
> +++ b/kernel/irq/chip.c
> @@ -405,6 +405,47 @@ out_unlock:
>  }
>  EXPORT_SYMBOL_GPL(handle_simple_irq);
>
> +#ifdef CONFIG_DUMB_IRQ_DEMUX_CHIP
> +/**
> + *     handle_dumb_demux_irq - Dumb demuxer irq handle function.
> + *     @irq:   the interrupt number
> + *     @desc:  the interrupt description structure for this irq
> + *
> + *     Dumb demux interrupts are sent from a demultiplexing interrupt handler
> + *     which is not able to decide which child interrupt interrupt handler
> + *     should be called.
> + *
> + *     Note: The caller is expected to handle the ack, clear, mask and
> + *     unmask issues if necessary.
> + */
> +irqreturn_t
> +handle_dumb_demux_irq(unsigned int irq, struct irq_desc *desc)
> +{
> +       irqreturn_t retval = IRQ_NONE;
> +
> +       raw_spin_lock(&desc->lock);
> +
> +       if (!irq_may_run(desc))
> +               goto out_unlock;
> +
> +       desc->istate &= ~(IRQS_REPLAY | IRQS_WAITING);
> +       kstat_incr_irqs_this_cpu(irq, desc);
> +
> +       if (unlikely(!desc->action || irqd_irq_disabled(&desc->irq_data))) {
> +               desc->istate |= IRQS_PENDING;
> +               goto out_unlock;
> +       }
> +
> +       retval = handle_irq_event_no_spurious_check(desc);
> +
> +out_unlock:
> +       raw_spin_unlock(&desc->lock);
> +
> +       return retval;
> +}
> +EXPORT_SYMBOL_GPL(handle_dumb_demux_irq);
> +#endif /* CONFIG_DUMB_IRQ_DEMUX_CHIP */
> +
>  /*
>   * Called unconditionally from handle_level_irq() and only for oneshot
>   * interrupts from handle_fasteoi_irq()
> diff --git a/kernel/irq/dumb-demux-chip.c b/kernel/irq/dumb-demux-chip.c
> new file mode 100644
> index 0000000..8e2de1d
> --- /dev/null
> +++ b/kernel/irq/dumb-demux-chip.c
> @@ -0,0 +1,140 @@
> +/*
> + * Library implementing common dumb irq demux chip functions
> + *
> + * Copyright (C) 2015, Boris Brezillon
> + */
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/irq.h>
> +#include <linux/slab.h>
> +#include <linux/export.h>
> +#include <linux/irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/irqchip/chained_irq.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel_stat.h>
> +#include <linux/syscore_ops.h>
> +
> +#include "internals.h"
> +
> +static void irq_dumb_demux_mask(struct irq_data *d)
> +{
> +       struct irq_chip_dumb_demux *demux = irq_data_get_irq_chip_data(d);
> +
> +       clear_bit(d->hwirq, &demux->unmasked);
> +
> +       if (!demux->unmasked)
> +               disable_irq_nosync(demux->src_irq);
> +}
> +
> +static void irq_dumb_demux_unmask(struct irq_data *d)
> +{
> +       struct irq_chip_dumb_demux *demux = irq_data_get_irq_chip_data(d);
> +       bool enable_src_irq = !demux->unmasked;
> +
> +       set_bit(d->hwirq, &demux->unmasked);
> +
> +       if (enable_src_irq)
> +               enable_irq(demux->src_irq);
> +}
> +
> +static struct irq_chip irq_dumb_demux_chip = {
> +       .name =         "dumb-demux-irq",
> +       .irq_mask =     irq_dumb_demux_mask,
> +       .irq_unmask =   irq_dumb_demux_unmask,
> +};
> +
> +/*
> + * Separate lockdep class for interrupt chip which can nest irq_desc
> + * lock.
> + */
> +static struct lock_class_key irq_nested_lock_class;
> +
> +/*
> + * irq_map_dumb_demux_chip - Map a dumb demux chip for an irq domain
> + */
> +static int irq_map_dumb_demux_chip(struct irq_domain *d,
> +                                  unsigned int virq,
> +                                  irq_hw_number_t hw_irq)
> +{
> +       struct irq_chip_dumb_demux *demux = d->host_data;
> +
> +       if (!test_bit(hw_irq, &demux->available))
> +               return -EINVAL;
> +
> +       if (demux->flags & IRQ_DD_INIT_NESTED_LOCK)
> +               irq_set_lockdep_class(virq, &irq_nested_lock_class);
> +
> +       irq_set_chip(virq, &irq_dumb_demux_chip);
> +       irq_set_chip_data(virq, demux);
> +       irq_modify_status(virq, demux->irq_flags_to_clear,
> +                         demux->irq_flags_to_set);
> +
> +       return 0;
> +}
> +
> +struct irq_domain_ops irq_dumb_demux_domain_ops = {
> +       .map    = irq_map_dumb_demux_chip,
> +       .xlate  = irq_domain_xlate_onecell,
> +};
> +EXPORT_SYMBOL_GPL(irq_dumb_demux_domain_ops);
> +
> +/**
> + * irq_dumb_demux_handler - Dumb demux flow handler
> + * @irq:               Virtual irq number
> + * @irq_desc:          irq descriptor
> + */
> +void irq_dumb_demux_handler(unsigned int irq, struct irq_desc *desc)
> +{
> +       struct irq_chip_dumb_demux *demux = irq_get_handler_data(irq);
> +       struct irq_chip *chip = irq_desc_get_chip(desc);
> +       irqreturn_t ret = IRQ_NONE;
> +       int i;
> +
> +       chained_irq_enter(chip, desc);
> +       for_each_set_bit(i, &demux->unmasked, BITS_PER_LONG) {
> +               int demuxed_irq = irq_find_mapping(demux->domain, i);
> +               struct irq_desc *desc = irq_to_desc(demuxed_irq);
> +
> +               ret |= handle_dumb_demux_irq(demuxed_irq, desc);
> +       }
> +       chained_irq_exit(chip, desc);
> +
> +       if (!noirqdebug)
> +               note_interrupt(irq, desc, ret);
> +}
> +EXPORT_SYMBOL_GPL(irq_dumb_demux_handler);
> +
> +/**
> + * irq_alloc_dumb_demux_chip - Allocate a dumb demux chip
> + * @src_irq:           irq feeding the dumb demux chip
> + * @dd_flags:          irq_dumb_demux_flags flags
> + * @valid_irqs:                Bitmask representing valid irqs
> + * @clr_flags:         irq_flags to clear when mapping an interrupt
> + * @set_flags:         irq_flags to set when mapping an interrupt
> + */
> +struct irq_chip_dumb_demux *
> +irq_alloc_dumb_demux_chip(unsigned int src_irq,
> +                         unsigned long valid_irqs,
> +                         unsigned int clr_flags,
> +                         unsigned int set_flags,
> +                         unsigned int dd_flags)
> +{
> +       struct irq_chip_dumb_demux *demux;
> +
> +       if (!src_irq)
> +               return ERR_PTR(-EINVAL);
> +
> +       demux = kzalloc(sizeof(*demux), GFP_KERNEL);
> +       if (!demux)
> +               return ERR_PTR(-ENOMEM);
> +
> +       demux->available = valid_irqs;
> +       demux->flags = dd_flags;
> +       demux->src_irq = src_irq;
> +       demux->irq_flags_to_clear = clr_flags;
> +       demux->irq_flags_to_set = set_flags;
> +
> +       return demux;
> +}
> +EXPORT_SYMBOL_GPL(irq_alloc_dumb_demux_chip);
> diff --git a/kernel/irq/handle.c b/kernel/irq/handle.c
> index 6354802..f786850 100644
> --- a/kernel/irq/handle.c
> +++ b/kernel/irq/handle.c
> @@ -131,7 +131,8 @@ void __irq_wake_thread(struct irq_desc *desc, struct irqaction *action)
>  }
>
>  irqreturn_t
> -handle_irq_event_percpu(struct irq_desc *desc, struct irqaction *action)
> +handle_irq_event_percpu_no_spurious_check(struct irq_desc *desc,
> +                                         struct irqaction *action)
>  {
>         irqreturn_t retval = IRQ_NONE;
>         unsigned int flags = 0, irq = desc->irq_data.irq;
> @@ -175,8 +176,18 @@ handle_irq_event_percpu(struct irq_desc *desc, struct irqaction *action)
>
>         add_interrupt_randomness(irq, flags);
>
> +       return retval;
> +}
> +
> +irqreturn_t
> +handle_irq_event_percpu(struct irq_desc *desc, struct irqaction *action)
> +{
> +       irqreturn_t retval;
> +
> +       retval = handle_irq_event_percpu_no_spurious_check(desc, action);
> +
>         if (!noirqdebug)
> -               note_interrupt(irq, desc, retval);
> +               note_interrupt(desc->irq_data.irq, desc, retval);
>         return retval;
>  }
>
> @@ -195,3 +206,19 @@ irqreturn_t handle_irq_event(struct irq_desc *desc)
>         irqd_clear(&desc->irq_data, IRQD_IRQ_INPROGRESS);
>         return ret;
>  }
> +
> +irqreturn_t handle_irq_event_no_spurious_check(struct irq_desc *desc)
> +{
> +       struct irqaction *action = desc->action;
> +       irqreturn_t ret;
> +
> +       desc->istate &= ~IRQS_PENDING;
> +       irqd_set(&desc->irq_data, IRQD_IRQ_INPROGRESS);
> +       raw_spin_unlock(&desc->lock);
> +
> +       ret = handle_irq_event_percpu_no_spurious_check(desc, action);
> +
> +       raw_spin_lock(&desc->lock);
> +       irqd_clear(&desc->irq_data, IRQD_IRQ_INPROGRESS);
> +       return ret;
> +}
> diff --git a/kernel/irq/internals.h b/kernel/irq/internals.h
> index df553b0..fe056fb 100644
> --- a/kernel/irq/internals.h
> +++ b/kernel/irq/internals.h
> @@ -90,6 +90,9 @@ extern void init_kstat_irqs(struct irq_desc *desc, int node, int nr);
>
>  irqreturn_t handle_irq_event_percpu(struct irq_desc *desc, struct irqaction *action);
>  irqreturn_t handle_irq_event(struct irq_desc *desc);
> +irqreturn_t handle_irq_event_percpu_no_spurious_check(struct irq_desc *desc,
> +                                                     struct irqaction *action);
> +irqreturn_t handle_irq_event_no_spurious_check(struct irq_desc *desc);
>
>  /* Resending of interrupts :*/
>  void check_irq_resend(struct irq_desc *desc, unsigned int irq);
> --
> 1.9.1
>

Re: [PATCH v2 1/5] irqchip: add dumb demultiplexer implementation

[Boris Brezillon]

Hi Rob,

On Tue, 13 Jan 2015 21:26:42 -0600
Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> On Tue, Jan 13, 2015 at 12:46 PM, Boris Brezillon
> <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> > Some interrupt controllers are multiplexing several peripheral IRQs on
> > a single interrupt line.
> > While this is not a problem for most IRQs (as long as all peripherals
> > request the interrupt with IRQF_SHARED flag set), multiplexing timers and
> > other type of peripherals will generate a WARNING (mixing IRQF_NO_SUSPEND
> > and !IRQF_NO_SUSPEND is prohibited).
> >
> > Create a dumb irq demultiplexer which simply forwards interrupts to all
> > peripherals (exactly what's happening with IRQ_SHARED) but keep a unique
> > irq number for each peripheral, thus preventing the IRQF_NO_SUSPEND
> > and !IRQF_NO_SUSPEND mix on a given interrupt.
> 
> This really seems like a work-around for how IRQF_SHARED works. It
> seems like what is really desired is just per handler disabling.

Like what I proposed here [1] ?

> It is
> fragile in that devices can deadlock the system if the drivers don't
> disable the interrupt source before calling disable_irq.

Not exactly deadlock since spurious interrupt detection is implemented,
but yes, things won't work as expected.

> But unlike
> IRQF_SHARED, there is nothing explicit in the driver indicating it is
> designed to work properly with a shared interrupt line.
> 
> I see no reason to accept this into DT either. We already can support
> shared lines and modeling an OR gate as an interrupt controller is
> pointless.

Okay, I guess I'll let DT and irq maintainers decide what is preferable
here (I already spent much time than I first expected to remove this
warning in a proper way).

Best Regards,

Boris

[1]https://lkml.org/lkml/2014/12/15/552

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/5] irqchip: add dumb demultiplexer implementation

[Thomas Gleixner]

On Tue, 13 Jan 2015, Rob Herring wrote:
> On Tue, Jan 13, 2015 at 12:46 PM, Boris Brezillon
> <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> > Some interrupt controllers are multiplexing several peripheral IRQs on
> > a single interrupt line.
> > While this is not a problem for most IRQs (as long as all peripherals
> > request the interrupt with IRQF_SHARED flag set), multiplexing timers and
> > other type of peripherals will generate a WARNING (mixing IRQF_NO_SUSPEND
> > and !IRQF_NO_SUSPEND is prohibited).
> >
> > Create a dumb irq demultiplexer which simply forwards interrupts to all
> > peripherals (exactly what's happening with IRQ_SHARED) but keep a unique
> > irq number for each peripheral, thus preventing the IRQF_NO_SUSPEND
> > and !IRQF_NO_SUSPEND mix on a given interrupt.
> 
> This really seems like a work-around for how IRQF_SHARED works. It

It's a workaround for a short coming of IRQF_SHARED.

IRQF_SHARED has a massive short coming versus suspend and wakeup
interrupts. If one of the demultiplexed interrupts is a valid wakeup
source then we have no sane way to express this with IRQF_SHARED
simply because the drivers need to be aware whether they run on stupid
or well designed hardware.

> seems like what is really desired is just per handler disabling. It is

So you want a magic API like disable/enable_irq_action()?

Certainly not.

You'd open just another can of worms which will bring us abuse and
hard to debug problems because driver writers think it's a good idea
to use it for random purposes.

Aside of that it would add another conditional into the interrupt
delivery hotpath which is not desired either.

> fragile in that devices can deadlock the system if the drivers don't
> disable the interrupt source before calling disable_irq. But unlike

Any misdesigned driver can do that for you.

> IRQF_SHARED, there is nothing explicit in the driver indicating it is
> designed to work properly with a shared interrupt line.

IRQF_SHARED is a pretty bad indicator. Look at all the drivers which
slap this flag onto request_irq() and have no single line of code
which makes sure that the driver would ever work on a shared line.

If it's just for annotational purposes, we can add a new IRQF flag,
which is required to request such a interrupt line.

> I see no reason to accept this into DT either. We already can support
> shared lines and modeling an OR gate as an interrupt controller is
> pointless.

It's absolutely not pointless.

All attempts to work around that have resulted in horrible bandaids so
far. That's why I guided Boris to implement this dummy demultiplexing
mechanism. It solves the problem at hand nicely without adding nasty
hackarounds into the suspend/resume code and inflicting platform
knowledge on multi-platform device drivers.

If you have a proper solution for the problem at hand which

   - avoids the demux dummy

   - works straight forward with suspend/resume/wakeup

   - does not add horrible new APIs

   - does not add conditionals to the interrupt hotpath

   - does not inflict platform knowledge about interrupt chip details
     on drivers

then I'm happy to take it.

But as long as you can't come up with anything sane, the demux dummy
is the best solution for this problem we've seen so far.

Thanks,

	tglx
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/5] irqchip: add dumb demultiplexer implementation

[Rob Herring]

On Wed, Jan 14, 2015 at 4:36 AM, Thomas Gleixner <tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org> wrote:
> On Tue, 13 Jan 2015, Rob Herring wrote:
>> On Tue, Jan 13, 2015 at 12:46 PM, Boris Brezillon
>> <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
>> > Some interrupt controllers are multiplexing several peripheral IRQs on
>> > a single interrupt line.
>> > While this is not a problem for most IRQs (as long as all peripherals
>> > request the interrupt with IRQF_SHARED flag set), multiplexing timers and
>> > other type of peripherals will generate a WARNING (mixing IRQF_NO_SUSPEND
>> > and !IRQF_NO_SUSPEND is prohibited).
>> >
>> > Create a dumb irq demultiplexer which simply forwards interrupts to all
>> > peripherals (exactly what's happening with IRQ_SHARED) but keep a unique
>> > irq number for each peripheral, thus preventing the IRQF_NO_SUSPEND
>> > and !IRQF_NO_SUSPEND mix on a given interrupt.
>>
>> This really seems like a work-around for how IRQF_SHARED works. It
>
> It's a workaround for a short coming of IRQF_SHARED.
>
> IRQF_SHARED has a massive short coming versus suspend and wakeup
> interrupts. If one of the demultiplexed interrupts is a valid wakeup
> source then we have no sane way to express this with IRQF_SHARED
> simply because the drivers need to be aware whether they run on stupid
> or well designed hardware.

Unfortunately, the drivers will still have to know this. They cannot
assume that they can call disable_irq and their device irq state does
not matter.

Perhaps we need a debug feature such that disable_irq/enable_irq are
nops with IRQF_SHARED?

>> seems like what is really desired is just per handler disabling. It is
>
> So you want a magic API like disable/enable_irq_action()?
>
> Certainly not.

Agreed.

> You'd open just another can of worms which will bring us abuse and
> hard to debug problems because driver writers think it's a good idea
> to use it for random purposes.
>
> Aside of that it would add another conditional into the interrupt
> delivery hotpath which is not desired either.
>
>> fragile in that devices can deadlock the system if the drivers don't
>> disable the interrupt source before calling disable_irq. But unlike
>
> Any misdesigned driver can do that for you.
>
>> IRQF_SHARED, there is nothing explicit in the driver indicating it is
>> designed to work properly with a shared interrupt line.
>
> IRQF_SHARED is a pretty bad indicator. Look at all the drivers which
> slap this flag onto request_irq() and have no single line of code
> which makes sure that the driver would ever work on a shared line.
>
> If it's just for annotational purposes, we can add a new IRQF flag,
> which is required to request such a interrupt line.
>
>> I see no reason to accept this into DT either. We already can support
>> shared lines and modeling an OR gate as an interrupt controller is
>> pointless.
>
> It's absolutely not pointless.
>
> All attempts to work around that have resulted in horrible bandaids so
> far. That's why I guided Boris to implement this dummy demultiplexing
> mechanism. It solves the problem at hand nicely without adding nasty
> hackarounds into the suspend/resume code and inflicting platform
> knowledge on multi-platform device drivers.

This change will break on old kernels with a new dtb. Would you be
happy if a BIOS update required a new kernel? Fixing this for any
platform requires a dtb update which may not be possible on some
platforms. I don't have a problem with this breakage for 1 platform
and the at91 guys may not care, but we'd ultimately be changing how
all shared irqs are specified for all DT. Maybe we decide that this is
how we want to describe things, but that needs much wider discussion
and agreement.

> If you have a proper solution for the problem at hand which
>
>    - avoids the demux dummy
>
>    - works straight forward with suspend/resume/wakeup
>
>    - does not add horrible new APIs
>
>    - does not add conditionals to the interrupt hotpath
>
>    - does not inflict platform knowledge about interrupt chip details
>      on drivers
>
> then I'm happy to take it.
>
> But as long as you can't come up with anything sane, the demux dummy
> is the best solution for this problem we've seen so far.

What if during suspend you move all actions w/o IRQF_NO_SUSPEND to a
suspended action list? This would leave only the actions with
IRQF_NO_SUSPEND set in the active action list. The cost would be a
pointer in irq_desc and moving the actions during suspend and resume.

There are probably ways to do this demux irqchip without a DT change.
Since we can't just move Linux irq numbers to different irq_chips
during request_irq, we would have to parse the DT up front to find all
shared interrupts and create a demux irqchip for them. That wouldn't
be very efficient, but is straight-forward. Then we'd have to handle
the translation into Linux irq numbers correctly which is probably the
more difficult part.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/5] irqchip: add dumb demultiplexer implementation

[Boris Brezillon]

Hi Rob,

On Wed, 14 Jan 2015 16:24:32 -0600
Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> On Wed, Jan 14, 2015 at 4:36 AM, Thomas Gleixner <tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org> wrote:
> > On Tue, 13 Jan 2015, Rob Herring wrote:
> >> On Tue, Jan 13, 2015 at 12:46 PM, Boris Brezillon
> >> <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> >> > Some interrupt controllers are multiplexing several peripheral IRQs on
> >> > a single interrupt line.
> >> > While this is not a problem for most IRQs (as long as all peripherals
> >> > request the interrupt with IRQF_SHARED flag set), multiplexing timers and
> >> > other type of peripherals will generate a WARNING (mixing IRQF_NO_SUSPEND
> >> > and !IRQF_NO_SUSPEND is prohibited).
> >> >
> >> > Create a dumb irq demultiplexer which simply forwards interrupts to all
> >> > peripherals (exactly what's happening with IRQ_SHARED) but keep a unique
> >> > irq number for each peripheral, thus preventing the IRQF_NO_SUSPEND
> >> > and !IRQF_NO_SUSPEND mix on a given interrupt.
> >>
> >> This really seems like a work-around for how IRQF_SHARED works. It
> >
> > It's a workaround for a short coming of IRQF_SHARED.
> >
> > IRQF_SHARED has a massive short coming versus suspend and wakeup
> > interrupts. If one of the demultiplexed interrupts is a valid wakeup
> > source then we have no sane way to express this with IRQF_SHARED
> > simply because the drivers need to be aware whether they run on stupid
> > or well designed hardware.
> 
> Unfortunately, the drivers will still have to know this. They cannot
> assume that they can call disable_irq and their device irq state does
> not matter.
> 
> Perhaps we need a debug feature such that disable_irq/enable_irq are
> nops with IRQF_SHARED?
> 
> >> seems like what is really desired is just per handler disabling. It is
> >
> > So you want a magic API like disable/enable_irq_action()?
> >
> > Certainly not.
> 
> Agreed.
> 
> > You'd open just another can of worms which will bring us abuse and
> > hard to debug problems because driver writers think it's a good idea
> > to use it for random purposes.
> >
> > Aside of that it would add another conditional into the interrupt
> > delivery hotpath which is not desired either.
> >
> >> fragile in that devices can deadlock the system if the drivers don't
> >> disable the interrupt source before calling disable_irq. But unlike
> >
> > Any misdesigned driver can do that for you.
> >
> >> IRQF_SHARED, there is nothing explicit in the driver indicating it is
> >> designed to work properly with a shared interrupt line.
> >
> > IRQF_SHARED is a pretty bad indicator. Look at all the drivers which
> > slap this flag onto request_irq() and have no single line of code
> > which makes sure that the driver would ever work on a shared line.
> >
> > If it's just for annotational purposes, we can add a new IRQF flag,
> > which is required to request such a interrupt line.
> >
> >> I see no reason to accept this into DT either. We already can support
> >> shared lines and modeling an OR gate as an interrupt controller is
> >> pointless.
> >
> > It's absolutely not pointless.
> >
> > All attempts to work around that have resulted in horrible bandaids so
> > far. That's why I guided Boris to implement this dummy demultiplexing
> > mechanism. It solves the problem at hand nicely without adding nasty
> > hackarounds into the suspend/resume code and inflicting platform
> > knowledge on multi-platform device drivers.
> 
> This change will break on old kernels with a new dtb. Would you be
> happy if a BIOS update required a new kernel? Fixing this for any
> platform requires a dtb update which may not be possible on some
> platforms. I don't have a problem with this breakage for 1 platform
> and the at91 guys may not care, but we'd ultimately be changing how
> all shared irqs are specified for all DT. Maybe we decide that this is
> how we want to describe things, but that needs much wider discussion
> and agreement.

I tried really hard on finding a DT representation that would not break
the DT ABI, but didn't find any easy solution.
How about keeping all platforms with the shared irq pattern, except for
those that really have an irq line shared by a timer and several other
devices (not sure yet, but at91 seems to be the only impacted platform
so far).

> 
> > If you have a proper solution for the problem at hand which
> >
> >    - avoids the demux dummy
> >
> >    - works straight forward with suspend/resume/wakeup
> >
> >    - does not add horrible new APIs
> >
> >    - does not add conditionals to the interrupt hotpath
> >
> >    - does not inflict platform knowledge about interrupt chip details
> >      on drivers
> >
> > then I'm happy to take it.
> >
> > But as long as you can't come up with anything sane, the demux dummy
> > is the best solution for this problem we've seen so far.
> 
> What if during suspend you move all actions w/o IRQF_NO_SUSPEND to a
> suspended action list? This would leave only the actions with
> IRQF_NO_SUSPEND set in the active action list. The cost would be a
> pointer in irq_desc and moving the actions during suspend and resume.

That really looks like what I suggested here [1].

> 
> There are probably ways to do this demux irqchip without a DT change.
> Since we can't just move Linux irq numbers to different irq_chips
> during request_irq, we would have to parse the DT up front to find all
> shared interrupts and create a demux irqchip for them. That wouldn't
> be very efficient, but is straight-forward. Then we'd have to handle
> the translation into Linux irq numbers correctly which is probably the
> more difficult part.

I'm really interested in seeing how you would do this.

Best Reagrds,

Boris

[1]https://lkml.org/lkml/2014/12/15/551

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/5] irqchip: add dumb demultiplexer implementation

[Nicolas Ferre]

Le 14/01/2015 23:55, Boris Brezillon a écrit :
> Hi Rob,
> 
> On Wed, 14 Jan 2015 16:24:32 -0600
> Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 
>> On Wed, Jan 14, 2015 at 4:36 AM, Thomas Gleixner <tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org> wrote:
>>> On Tue, 13 Jan 2015, Rob Herring wrote:
>>>> On Tue, Jan 13, 2015 at 12:46 PM, Boris Brezillon
>>>> <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
>>>>> Some interrupt controllers are multiplexing several peripheral IRQs on
>>>>> a single interrupt line.
>>>>> While this is not a problem for most IRQs (as long as all peripherals
>>>>> request the interrupt with IRQF_SHARED flag set), multiplexing timers and
>>>>> other type of peripherals will generate a WARNING (mixing IRQF_NO_SUSPEND
>>>>> and !IRQF_NO_SUSPEND is prohibited).
>>>>>
>>>>> Create a dumb irq demultiplexer which simply forwards interrupts to all
>>>>> peripherals (exactly what's happening with IRQ_SHARED) but keep a unique
>>>>> irq number for each peripheral, thus preventing the IRQF_NO_SUSPEND
>>>>> and !IRQF_NO_SUSPEND mix on a given interrupt.
>>>>
>>>> This really seems like a work-around for how IRQF_SHARED works. It
>>>
>>> It's a workaround for a short coming of IRQF_SHARED.
>>>
>>> IRQF_SHARED has a massive short coming versus suspend and wakeup
>>> interrupts. If one of the demultiplexed interrupts is a valid wakeup
>>> source then we have no sane way to express this with IRQF_SHARED
>>> simply because the drivers need to be aware whether they run on stupid
>>> or well designed hardware.
>>
>> Unfortunately, the drivers will still have to know this. They cannot
>> assume that they can call disable_irq and their device irq state does
>> not matter.
>>
>> Perhaps we need a debug feature such that disable_irq/enable_irq are
>> nops with IRQF_SHARED?
>>
>>>> seems like what is really desired is just per handler disabling. It is
>>>
>>> So you want a magic API like disable/enable_irq_action()?
>>>
>>> Certainly not.
>>
>> Agreed.
>>
>>> You'd open just another can of worms which will bring us abuse and
>>> hard to debug problems because driver writers think it's a good idea
>>> to use it for random purposes.
>>>
>>> Aside of that it would add another conditional into the interrupt
>>> delivery hotpath which is not desired either.
>>>
>>>> fragile in that devices can deadlock the system if the drivers don't
>>>> disable the interrupt source before calling disable_irq. But unlike
>>>
>>> Any misdesigned driver can do that for you.
>>>
>>>> IRQF_SHARED, there is nothing explicit in the driver indicating it is
>>>> designed to work properly with a shared interrupt line.
>>>
>>> IRQF_SHARED is a pretty bad indicator. Look at all the drivers which
>>> slap this flag onto request_irq() and have no single line of code
>>> which makes sure that the driver would ever work on a shared line.
>>>
>>> If it's just for annotational purposes, we can add a new IRQF flag,
>>> which is required to request such a interrupt line.
>>>
>>>> I see no reason to accept this into DT either. We already can support
>>>> shared lines and modeling an OR gate as an interrupt controller is
>>>> pointless.
>>>
>>> It's absolutely not pointless.
>>>
>>> All attempts to work around that have resulted in horrible bandaids so
>>> far. That's why I guided Boris to implement this dummy demultiplexing
>>> mechanism. It solves the problem at hand nicely without adding nasty
>>> hackarounds into the suspend/resume code and inflicting platform
>>> knowledge on multi-platform device drivers.
>>
>> This change will break on old kernels with a new dtb. Would you be
>> happy if a BIOS update required a new kernel? Fixing this for any
>> platform requires a dtb update which may not be possible on some
>> platforms. I don't have a problem with this breakage for 1 platform
>> and the at91 guys may not care, but we'd ultimately be changing how
>> all shared irqs are specified for all DT. Maybe we decide that this is
>> how we want to describe things, but that needs much wider discussion
>> and agreement.
> 
> I tried really hard on finding a DT representation that would not break
> the DT ABI, but didn't find any easy solution.
> How about keeping all platforms with the shared irq pattern, except for
> those that really have an irq line shared by a timer and several other
> devices (not sure yet, but at91 seems to be the only impacted platform
> so far).
> 
>>
>>> If you have a proper solution for the problem at hand which
>>>
>>>    - avoids the demux dummy
>>>
>>>    - works straight forward with suspend/resume/wakeup
>>>
>>>    - does not add horrible new APIs
>>>
>>>    - does not add conditionals to the interrupt hotpath
>>>
>>>    - does not inflict platform knowledge about interrupt chip details
>>>      on drivers
>>>
>>> then I'm happy to take it.
>>>
>>> But as long as you can't come up with anything sane, the demux dummy
>>> is the best solution for this problem we've seen so far.
>>
>> What if during suspend you move all actions w/o IRQF_NO_SUSPEND to a
>> suspended action list? This would leave only the actions with
>> IRQF_NO_SUSPEND set in the active action list. The cost would be a
>> pointer in irq_desc and moving the actions during suspend and resume.
> 
> That really looks like what I suggested here [1].
> 
>>
>> There are probably ways to do this demux irqchip without a DT change.
>> Since we can't just move Linux irq numbers to different irq_chips
>> during request_irq, we would have to parse the DT up front to find all
>> shared interrupts and create a demux irqchip for them. That wouldn't
>> be very efficient, but is straight-forward. Then we'd have to handle
>> the translation into Linux irq numbers correctly which is probably the
>> more difficult part.
> 
> I'm really interested in seeing how you would do this.

Well, it really looks like a ping-pong game and Boris tried hard to
explore both options. I suspect that he already made his best to solve
this problem.
In the meantime, we begin to see people using our (old, badly designed,
okay-okay) hardware and have the surprise of hitting this warning on
Mainline kernel. As we have several set of timers, they may even not be
affected by the core issue by selecting a timer which doesn't have a
shared interrupt line.

I mean, we discovered this when it was already written (in linux-next
actually) and tried to address the issue quickly. But now, we urgently
need a solution or a temporary workaround and Boris wrote two. I have
the feeling that we have to move on by selecting one. Clearly, if the
only issue remaining is that one solution may cause DT changes for AT91,
well, we can cope with this and accept it.

If we don't come to a conclusion quickly, maybe we would have to
consider removing this warning while we are working on the issue:
believe me, it's frightening and sometimes even inappropriate when
people select a different set of timers for their clocksource/clockevent
after boot time.

Best regards,


> [1]https://lkml.org/lkml/2014/12/15/551
> 


-- 
Nicolas Ferre
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/5] irqchip: add dumb demultiplexer implementation

[Thomas Gleixner]

On Wed, 14 Jan 2015, Rob Herring wrote:
> On Wed, Jan 14, 2015 at 4:36 AM, Thomas Gleixner <tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org> wrote:
> > All attempts to work around that have resulted in horrible bandaids so
> > far. That's why I guided Boris to implement this dummy demultiplexing
> > mechanism. It solves the problem at hand nicely without adding nasty
> > hackarounds into the suspend/resume code and inflicting platform
> > knowledge on multi-platform device drivers.
> 
> This change will break on old kernels with a new dtb. Would you be
> happy if a BIOS update required a new kernel? Fixing this for any
> platform requires a dtb update which may not be possible on some
> platforms. I don't have a problem with this breakage for 1 platform
> and the at91 guys may not care, but we'd ultimately be changing how
> all shared irqs are specified for all DT. Maybe we decide that this is
> how we want to describe things, but that needs much wider discussion
> and agreement.

We do not change shared interrupts in any way. We provide an
alternative mechanism for braindead hardware. And if the at91 folks
are fine with the DT change, then it's their decision. Nothing forces
this on everyone.
 
> > If you have a proper solution for the problem at hand which
> >
> >    - avoids the demux dummy
> >
> >    - works straight forward with suspend/resume/wakeup
> >
> >    - does not add horrible new APIs
> >
> >    - does not add conditionals to the interrupt hotpath
> >
> >    - does not inflict platform knowledge about interrupt chip details
> >      on drivers
> >
> > then I'm happy to take it.
> >
> > But as long as you can't come up with anything sane, the demux dummy
> > is the best solution for this problem we've seen so far.
> 
> What if during suspend you move all actions w/o IRQF_NO_SUSPEND to a
> suspended action list? This would leave only the actions with
> IRQF_NO_SUSPEND set in the active action list. The cost would be a
> pointer in irq_desc and moving the actions during suspend and resume.

That's exactly what we want NOT. Because it prevents us to do proper
sanity checks for IRQF_NO_SUSPEND. We've been there and I rejected it
for that very reason.
 
> There are probably ways to do this demux irqchip without a DT change.

What's the problem with a DT change for a single platform, if the
maintainers are willing to take it and deal with the fallout?

And in case of AT91 the new kernel will simply work with the old DT
and just emit the same warning vs. the IRQF_NO_SUSPEND mismatch. Older
kernels won't work with a new DT, but that's about it.

Aside of that, I'm quite amused about your DT update worries. DTs
break with every kernel version on very popular platforms in very
weird and subtle ways.

Thanks,

	tglx

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/5] irqchip: add dumb demultiplexer implementation

[Nicolas Ferre]

Le 15/01/2015 10:11, Thomas Gleixner a écrit :
> On Wed, 14 Jan 2015, Rob Herring wrote:
>> On Wed, Jan 14, 2015 at 4:36 AM, Thomas Gleixner <tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org> wrote:
>>> All attempts to work around that have resulted in horrible bandaids so
>>> far. That's why I guided Boris to implement this dummy demultiplexing
>>> mechanism. It solves the problem at hand nicely without adding nasty
>>> hackarounds into the suspend/resume code and inflicting platform
>>> knowledge on multi-platform device drivers.
>>
>> This change will break on old kernels with a new dtb. Would you be
>> happy if a BIOS update required a new kernel? Fixing this for any
>> platform requires a dtb update which may not be possible on some
>> platforms. I don't have a problem with this breakage for 1 platform
>> and the at91 guys may not care, but we'd ultimately be changing how
>> all shared irqs are specified for all DT. Maybe we decide that this is
>> how we want to describe things, but that needs much wider discussion
>> and agreement.
> 
> We do not change shared interrupts in any way. We provide an
> alternative mechanism for braindead hardware. And if the at91 folks

Let me add subtle details: "Old braindead hardware, with possibility to
use alternative set of timers which doesn't have shared interrupt lines" ;-)

> are fine with the DT change, then it's their decision. Nothing forces
> this on everyone.

Yes I do agree to change DT.

>>> If you have a proper solution for the problem at hand which
>>>
>>>    - avoids the demux dummy
>>>
>>>    - works straight forward with suspend/resume/wakeup
>>>
>>>    - does not add horrible new APIs
>>>
>>>    - does not add conditionals to the interrupt hotpath
>>>
>>>    - does not inflict platform knowledge about interrupt chip details
>>>      on drivers
>>>
>>> then I'm happy to take it.
>>>
>>> But as long as you can't come up with anything sane, the demux dummy
>>> is the best solution for this problem we've seen so far.
>>
>> What if during suspend you move all actions w/o IRQF_NO_SUSPEND to a
>> suspended action list? This would leave only the actions with
>> IRQF_NO_SUSPEND set in the active action list. The cost would be a
>> pointer in irq_desc and moving the actions during suspend and resume.
> 
> That's exactly what we want NOT. Because it prevents us to do proper
> sanity checks for IRQF_NO_SUSPEND. We've been there and I rejected it
> for that very reason.
>  
>> There are probably ways to do this demux irqchip without a DT change.
> 
> What's the problem with a DT change for a single platform, if the
> maintainers are willing to take it and deal with the fallout?
> 
> And in case of AT91 the new kernel will simply work with the old DT
> and just emit the same warning vs. the IRQF_NO_SUSPEND mismatch. Older
> kernels won't work with a new DT, but that's about it.
> 
> Aside of that, I'm quite amused about your DT update worries. DTs
> break with every kernel version on very popular platforms in very
> weird and subtle ways.

DT on AT91 is a Work In Progress (for 3.5 years) and the facts have told
us that DT updates were absolutely necessary. So, for now, I agree to
change DT as far as AT91 is concerned.

Bye,
-- 
Nicolas Ferre
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/5] irqchip: add dumb demultiplexer implementation

[Rob Herring]

On Thu, Jan 15, 2015 at 3:11 AM, Thomas Gleixner <tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org> wrote:
> On Wed, 14 Jan 2015, Rob Herring wrote:
>> On Wed, Jan 14, 2015 at 4:36 AM, Thomas Gleixner <tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org> wrote:
>> > All attempts to work around that have resulted in horrible bandaids so
>> > far. That's why I guided Boris to implement this dummy demultiplexing
>> > mechanism. It solves the problem at hand nicely without adding nasty
>> > hackarounds into the suspend/resume code and inflicting platform
>> > knowledge on multi-platform device drivers.
>>
>> This change will break on old kernels with a new dtb. Would you be
>> happy if a BIOS update required a new kernel? Fixing this for any
>> platform requires a dtb update which may not be possible on some
>> platforms. I don't have a problem with this breakage for 1 platform
>> and the at91 guys may not care, but we'd ultimately be changing how
>> all shared irqs are specified for all DT. Maybe we decide that this is
>> how we want to describe things, but that needs much wider discussion
>> and agreement.
>
> We do not change shared interrupts in any way. We provide an
> alternative mechanism for braindead hardware. And if the at91 folks
> are fine with the DT change, then it's their decision. Nothing forces
> this on everyone.

We are changing how shared interrupts are described in DT. We don't
need 2 ways to describe them. We could say this is only for AT91 and
continue to describe shared interrupts as has been done forever. Then
the next platform that hits this problem will have to go thru the same
ABI breakage. Or we change DT practices to describe all shared
interrupts with a demux node. Given the way DTs are incrementally
created, it is not something we can check with review or tools, so we
will still have the same ABI breakage problem.

>> > If you have a proper solution for the problem at hand which
>> >
>> >    - avoids the demux dummy
>> >
>> >    - works straight forward with suspend/resume/wakeup
>> >
>> >    - does not add horrible new APIs
>> >
>> >    - does not add conditionals to the interrupt hotpath
>> >
>> >    - does not inflict platform knowledge about interrupt chip details
>> >      on drivers
>> >
>> > then I'm happy to take it.
>> >
>> > But as long as you can't come up with anything sane, the demux dummy
>> > is the best solution for this problem we've seen so far.
>>
>> What if during suspend you move all actions w/o IRQF_NO_SUSPEND to a
>> suspended action list? This would leave only the actions with
>> IRQF_NO_SUSPEND set in the active action list. The cost would be a
>> pointer in irq_desc and moving the actions during suspend and resume.
>
> That's exactly what we want NOT. Because it prevents us to do proper
> sanity checks for IRQF_NO_SUSPEND. We've been there and I rejected it
> for that very reason.
>
>> There are probably ways to do this demux irqchip without a DT change.
>
> What's the problem with a DT change for a single platform, if the
> maintainers are willing to take it and deal with the fallout?

What's the solution for a platform that an ABI break is not okay and
can't deal with the fallout?

> And in case of AT91 the new kernel will simply work with the old DT
> and just emit the same warning vs. the IRQF_NO_SUSPEND mismatch. Older
> kernels won't work with a new DT, but that's about it.
>
> Aside of that, I'm quite amused about your DT update worries. DTs
> break with every kernel version on very popular platforms in very
> weird and subtle ways.

Someone has to. It is a problem that we need to get better at
preventing. It's an ABI. I don't think I need to explain what that
means.

Certainly there are folks who would just prefer DT to be a kernel data
structure so we don't have to worry about this ABI nonsense.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/5] irqchip: add dumb demultiplexer implementation

[Thomas Gleixner]

On Thu, 15 Jan 2015, Rob Herring wrote:
> On Thu, Jan 15, 2015 at 3:11 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> > On Wed, 14 Jan 2015, Rob Herring wrote:
>
> > We do not change shared interrupts in any way. We provide an
> > alternative mechanism for braindead hardware. And if the at91 folks
> > are fine with the DT change, then it's their decision. Nothing forces
> > this on everyone.
> 
> We are changing how shared interrupts are described in DT. We don't
> need 2 ways to describe them. We could say this is only for AT91 and
> continue to describe shared interrupts as has been done forever. Then
> the next platform that hits this problem will have to go thru the same
> ABI breakage. Or we change DT practices to describe all shared
> interrupts with a demux node. Given the way DTs are incrementally
> created, it is not something we can check with review or tools, so we
> will still have the same ABI breakage problem.

This is not describing the proper shared interrupts. This is a special
case for a special case of braindamaged hardware. Whats wrong with
doing that? We dont have to change that for all shared interrupts
because 99% of them have a proper hardware implementation and are not
affected by this.

What's wrong with serving the AT91 with a proper solution, which does
NOT inflict horrible hacks into the core code and does NOT weaken
sanity checks and does NOT require irq chip specific knowledge in
device drivers?

> >> There are probably ways to do this demux irqchip without a DT change.

So far you have not provided any useful hint how to do so.

> > What's the problem with a DT change for a single platform, if the
> > maintainers are willing to take it and deal with the fallout?
> 
> What's the solution for a platform that an ABI break is not okay and
> can't deal with the fallout?

There is no other platform affected. This is a break for a specific
set of devices and the 'fallout' is confined, well known and accepted.

So what's your problem, really?

Thanks,

	tglx
 

Re: [PATCH v2 1/5] irqchip: add dumb demultiplexer implementation

[Rob Herring]

On Tue, Jan 20, 2015 at 7:08 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Thu, 15 Jan 2015, Rob Herring wrote:
>> On Thu, Jan 15, 2015 at 3:11 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
>> > On Wed, 14 Jan 2015, Rob Herring wrote:
>>
>> > We do not change shared interrupts in any way. We provide an
>> > alternative mechanism for braindead hardware. And if the at91 folks
>> > are fine with the DT change, then it's their decision. Nothing forces
>> > this on everyone.
>>
>> We are changing how shared interrupts are described in DT. We don't
>> need 2 ways to describe them. We could say this is only for AT91 and
>> continue to describe shared interrupts as has been done forever. Then
>> the next platform that hits this problem will have to go thru the same
>> ABI breakage. Or we change DT practices to describe all shared
>> interrupts with a demux node. Given the way DTs are incrementally
>> created, it is not something we can check with review or tools, so we
>> will still have the same ABI breakage problem.
>
> This is not describing the proper shared interrupts. This is a special
> case for a special case of braindamaged hardware. Whats wrong with
> doing that? We dont have to change that for all shared interrupts
> because 99% of them have a proper hardware implementation and are not
> affected by this.


>
> What's wrong with serving the AT91 with a proper solution, which does
> NOT inflict horrible hacks into the core code and does NOT weaken
> sanity checks and does NOT require irq chip specific knowledge in
> device drivers?
>
>> >> There are probably ways to do this demux irqchip without a DT change.
>
> So far you have not provided any useful hint how to do so.
>
>> > What's the problem with a DT change for a single platform, if the
>> > maintainers are willing to take it and deal with the fallout?
>>
>> What's the solution for a platform that an ABI break is not okay and
>> can't deal with the fallout?
>
> There is no other platform affected. This is a break for a specific
> set of devices and the 'fallout' is confined, well known and accepted.
>
> So what's your problem, really?
>
> Thanks,
>
>         tglx
>

Re: [PATCH v2 1/5] irqchip: add dumb demultiplexer implementation

[Nicolas Ferre]

Le 13/01/2015 19:46, Boris Brezillon a écrit :
> Some interrupt controllers are multiplexing several peripheral IRQs on
> a single interrupt line.
> While this is not a problem for most IRQs (as long as all peripherals
> request the interrupt with IRQF_SHARED flag set), multiplexing timers and
> other type of peripherals will generate a WARNING (mixing IRQF_NO_SUSPEND
> and !IRQF_NO_SUSPEND is prohibited).
> 
> Create a dumb irq demultiplexer which simply forwards interrupts to all
> peripherals (exactly what's happening with IRQ_SHARED) but keep a unique
> irq number for each peripheral, thus preventing the IRQF_NO_SUSPEND
> and !IRQF_NO_SUSPEND mix on a given interrupt.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
>  drivers/irqchip/Kconfig          |   4 ++
>  drivers/irqchip/Makefile         |   1 +
>  drivers/irqchip/irq-dumb-demux.c |  70 ++++++++++++++++++++
>  include/linux/irq.h              |  49 ++++++++++++++
>  include/linux/irqdomain.h        |   1 +
>  kernel/irq/Kconfig               |   5 ++
>  kernel/irq/Makefile              |   1 +
>  kernel/irq/chip.c                |  41 ++++++++++++
>  kernel/irq/dumb-demux-chip.c     | 140 +++++++++++++++++++++++++++++++++++++++
>  kernel/irq/handle.c              |  31 ++++++++-
>  kernel/irq/internals.h           |   3 +
>  11 files changed, 344 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/irqchip/irq-dumb-demux.c
>  create mode 100644 kernel/irq/dumb-demux-chip.c
> 
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index cc79d2a..8a9df88 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -70,6 +70,10 @@ config BRCMSTB_L2_IRQ
>  	select GENERIC_IRQ_CHIP
>  	select IRQ_DOMAIN
>  
> +config DUMB_DEMUX_IRQ
> +	bool
> +	select DUMB_IRQ_DEMUX_CHIP
> +
>  config DW_APB_ICTL
>  	bool
>  	select GENERIC_IRQ_CHIP
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index 9516a32..77f3c51 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -8,6 +8,7 @@ obj-$(CONFIG_ARCH_MVEBU)		+= irq-armada-370-xp.o
>  obj-$(CONFIG_ARCH_MXS)			+= irq-mxs.o
>  obj-$(CONFIG_ARCH_S3C24XX)		+= irq-s3c24xx.o
>  obj-$(CONFIG_DW_APB_ICTL)		+= irq-dw-apb-ictl.o
> +obj-$(CONFIG_DUMB_DEMUX_IRQ)		+= irq-dumb-demux.o
>  obj-$(CONFIG_METAG)			+= irq-metag-ext.o
>  obj-$(CONFIG_METAG_PERFCOUNTER_IRQS)	+= irq-metag.o
>  obj-$(CONFIG_ARCH_MOXART)		+= irq-moxart.o
> diff --git a/drivers/irqchip/irq-dumb-demux.c b/drivers/irqchip/irq-dumb-demux.c
> new file mode 100644
> index 0000000..dfa05ce
> --- /dev/null
> +++ b/drivers/irqchip/irq-dumb-demux.c
> @@ -0,0 +1,70 @@

Maybe add a little file header here. It's always better.


> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/of_irq.h>
> +#include <linux/slab.h>
> +
> +#include "irqchip.h"
> +
> +static int __init dumb_irq_demux_of_init(struct device_node *node,
> +					 struct device_node *parent)
> +{
> +	struct irq_chip_dumb_demux *demux;
> +	unsigned int irq;
> +	u32 valid_irqs;
> +	int ret;
> +
> +	irq = irq_of_parse_and_map(node, 0);
> +	if (!irq) {
> +		pr_err("Failed to retrieve dumb irq demuxer source\n");
> +		return -EINVAL;
> +	}
> +
> +	ret = of_property_read_u32(node, "irqs", &valid_irqs);
> +	if (ret) {
> +		pr_err("Invalid of missing 'irqs' property\n");
> +		return ret;
> +	}
> +
> +	demux = irq_alloc_dumb_demux_chip(irq, valid_irqs,
> +					  IRQ_NOREQUEST | IRQ_NOPROBE |
> +					  IRQ_NOAUTOEN, 0, 0);
> +	if (!demux) {
> +		pr_err("Failed to allocate dumb irq demuxer struct\n");
> +		return -ENOMEM;
> +	}
> +
> +	demux->domain = irq_domain_add_linear(node, BITS_PER_LONG,
> +					      &irq_dumb_demux_domain_ops,
> +					      demux);
> +	if (!demux->domain) {
> +		ret = -ENOMEM;
> +		goto err_free_demux;
> +	}
> +
> +	ret = irq_set_handler_data(irq, demux);
> +	if (ret) {
> +		pr_err("Failed to assign handler data\n");
> +		goto err_free_domain;
> +	}
> +
> +	irq_set_chained_handler(irq, irq_dumb_demux_handler);
> +
> +	/*
> +	 * Disable the src irq (automatically enabled by
> +	 * irq_set_chained_handler) to prevent irqs from happening while
> +	 * nobody requested any of the demuxed irqs.
> +	 */
> +	disable_irq(irq);
> +
> +	return 0;
> +
> +err_free_domain:
> +	irq_domain_remove(demux->domain);
> +
> +err_free_demux:
> +	kfree(demux);
> +
> +	return ret;
> +}
> +IRQCHIP_DECLARE(dumb_irq_demux, "irqchip-dumb-demux", dumb_irq_demux_of_init);
> diff --git a/include/linux/irq.h b/include/linux/irq.h
> index d09ec7a..ae8fa21 100644
> --- a/include/linux/irq.h
> +++ b/include/linux/irq.h
> @@ -445,6 +445,10 @@ extern void handle_fasteoi_irq(unsigned int irq, struct irq_desc *desc);
>  extern void handle_edge_irq(unsigned int irq, struct irq_desc *desc);
>  extern void handle_edge_eoi_irq(unsigned int irq, struct irq_desc *desc);
>  extern void handle_simple_irq(unsigned int irq, struct irq_desc *desc);
> +#ifdef CONFIG_DUMB_IRQ_DEMUX_CHIP
> +extern irqreturn_t handle_dumb_demux_irq(unsigned int irq,
> +					 struct irq_desc *desc);
> +#endif
>  extern void handle_percpu_irq(unsigned int irq, struct irq_desc *desc);
>  extern void handle_percpu_devid_irq(unsigned int irq, struct irq_desc *desc);
>  extern void handle_bad_irq(unsigned int irq, struct irq_desc *desc);
> @@ -862,4 +866,49 @@ static inline u32 irq_reg_readl(struct irq_chip_generic *gc,
>  		return readl(gc->reg_base + reg_offset);
>  }
>  
> +/**
> + * enum irq_dumb_demux_flags - Initialization flags for generic irq chips
> + * @IRQ_DD_INIT_NESTED_LOCK:	Set the lock class of the irqs to nested for
> + *				irq chips which need to call irq_set_wake() on
> + *				the parent irq. Usually GPIO implementations
> + */
> +enum irq_dumb_demux_flags {
> +	IRQ_DD_INIT_NESTED_LOCK		= 1 << 0,
> +};
> +
> +#ifdef CONFIG_DUMB_IRQ_DEMUX_CHIP
> +/**
> + * struct irq_chip_dumb_demux - Dumb demultiplexer irq chip data structure
> + * @domain:		irq domain pointer
> + * @available:		Bitfield of valid irqs
> + * @unmasked:		Bitfield containing irqs status
> + * @flags:		irq_dumb_demux_flags flags
> + * @src_irq:		irq feeding the dumb demux chip
> + *
> + * Note, that irq_chip_generic can have multiple irq_chip_type
> + * implementations which can be associated to a particular irq line of
> + * an irq_chip_generic instance. That allows to share and protect
> + * state in an irq_chip_generic instance when we need to implement
> + * different flow mechanisms (level/edge) for it.
> + */
> +struct irq_chip_dumb_demux {
> +	struct irq_domain *domain;
> +	unsigned long available;
> +	unsigned long unmasked;
> +	unsigned int flags;
> +	unsigned int src_irq;
> +	unsigned int irq_flags_to_clear;
> +	unsigned int irq_flags_to_set;
> +};
> +
> +void irq_dumb_demux_handler(unsigned int irq, struct irq_desc *desc);
> +
> +struct irq_chip_dumb_demux *
> +irq_alloc_dumb_demux_chip(unsigned int src_irq,
> +			  unsigned long valid_irqs,
> +			  unsigned int clr_flags,
> +			  unsigned int set_flags,
> +			  unsigned int dd_flags);
> +#endif /* CONFIG_DUMB_IRQ_DEMUX_CHIP */
> +
>  #endif /* _LINUX_IRQ_H */
> diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
> index 676d730..1de3808 100644
> --- a/include/linux/irqdomain.h
> +++ b/include/linux/irqdomain.h
> @@ -80,6 +80,7 @@ struct irq_domain_ops {
>  };
>  
>  extern struct irq_domain_ops irq_generic_chip_ops;
> +extern struct irq_domain_ops irq_dumb_demux_domain_ops;
>  
>  struct irq_domain_chip_generic;
>  
> diff --git a/kernel/irq/Kconfig b/kernel/irq/Kconfig
> index 9a76e3b..d01554a 100644
> --- a/kernel/irq/Kconfig
> +++ b/kernel/irq/Kconfig
> @@ -51,6 +51,11 @@ config GENERIC_IRQ_CHIP
>         bool
>         select IRQ_DOMAIN
>  
> +# Dumb interrupt demuxer chip implementation
> +config DUMB_IRQ_DEMUX_CHIP
> +	bool
> +	select IRQ_DOMAIN
> +
>  # Generic irq_domain hw <--> linux irq number translation
>  config IRQ_DOMAIN
>  	bool
> diff --git a/kernel/irq/Makefile b/kernel/irq/Makefile
> index d121235..1cd4e42 100644
> --- a/kernel/irq/Makefile
> +++ b/kernel/irq/Makefile
> @@ -7,3 +7,4 @@ obj-$(CONFIG_PROC_FS) += proc.o
>  obj-$(CONFIG_GENERIC_PENDING_IRQ) += migration.o
>  obj-$(CONFIG_PM_SLEEP) += pm.o
>  obj-$(CONFIG_GENERIC_MSI_IRQ) += msi.o
> +obj-$(CONFIG_DUMB_IRQ_DEMUX_CHIP) += dumb-demux-chip.o
> diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
> index 6f1c7a5..d2a5c96 100644
> --- a/kernel/irq/chip.c
> +++ b/kernel/irq/chip.c
> @@ -405,6 +405,47 @@ out_unlock:
>  }
>  EXPORT_SYMBOL_GPL(handle_simple_irq);
>  
> +#ifdef CONFIG_DUMB_IRQ_DEMUX_CHIP
> +/**
> + *	handle_dumb_demux_irq - Dumb demuxer irq handle function.
> + *	@irq:	the interrupt number
> + *	@desc:	the interrupt description structure for this irq
> + *
> + *	Dumb demux interrupts are sent from a demultiplexing interrupt handler
> + *	which is not able to decide which child interrupt interrupt handler

typo: "interrupt interrupt"

> + *	should be called.
> + *
> + *	Note: The caller is expected to handle the ack, clear, mask and
> + *	unmask issues if necessary.
> + */
> +irqreturn_t
> +handle_dumb_demux_irq(unsigned int irq, struct irq_desc *desc)
> +{
> +	irqreturn_t retval = IRQ_NONE;
> +
> +	raw_spin_lock(&desc->lock);
> +
> +	if (!irq_may_run(desc))
> +		goto out_unlock;
> +
> +	desc->istate &= ~(IRQS_REPLAY | IRQS_WAITING);
> +	kstat_incr_irqs_this_cpu(irq, desc);
> +
> +	if (unlikely(!desc->action || irqd_irq_disabled(&desc->irq_data))) {
> +		desc->istate |= IRQS_PENDING;
> +		goto out_unlock;
> +	}
> +
> +	retval = handle_irq_event_no_spurious_check(desc);
> +
> +out_unlock:
> +	raw_spin_unlock(&desc->lock);
> +
> +	return retval;
> +}
> +EXPORT_SYMBOL_GPL(handle_dumb_demux_irq);
> +#endif /* CONFIG_DUMB_IRQ_DEMUX_CHIP */
> +
>  /*
>   * Called unconditionally from handle_level_irq() and only for oneshot
>   * interrupts from handle_fasteoi_irq()
> diff --git a/kernel/irq/dumb-demux-chip.c b/kernel/irq/dumb-demux-chip.c
> new file mode 100644
> index 0000000..8e2de1d
> --- /dev/null
> +++ b/kernel/irq/dumb-demux-chip.c
> @@ -0,0 +1,140 @@
> +/*
> + * Library implementing common dumb irq demux chip functions
> + *
> + * Copyright (C) 2015, Boris Brezillon

License here, please.

> + */
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/irq.h>
> +#include <linux/slab.h>
> +#include <linux/export.h>
> +#include <linux/irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/irqchip/chained_irq.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel_stat.h>
> +#include <linux/syscore_ops.h>
> +
> +#include "internals.h"
> +
> +static void irq_dumb_demux_mask(struct irq_data *d)
> +{
> +	struct irq_chip_dumb_demux *demux = irq_data_get_irq_chip_data(d);
> +
> +	clear_bit(d->hwirq, &demux->unmasked);
> +
> +	if (!demux->unmasked)
> +		disable_irq_nosync(demux->src_irq);
> +}
> +
> +static void irq_dumb_demux_unmask(struct irq_data *d)
> +{
> +	struct irq_chip_dumb_demux *demux = irq_data_get_irq_chip_data(d);
> +	bool enable_src_irq = !demux->unmasked;

Why this additional "bool" unlike the other function above?

> +
> +	set_bit(d->hwirq, &demux->unmasked);
> +
> +	if (enable_src_irq)
> +		enable_irq(demux->src_irq);
> +}
> +
> +static struct irq_chip irq_dumb_demux_chip = {
> +	.name =		"dumb-demux-irq",
> +	.irq_mask =	irq_dumb_demux_mask,
> +	.irq_unmask =	irq_dumb_demux_unmask,
> +};
> +
> +/*
> + * Separate lockdep class for interrupt chip which can nest irq_desc
> + * lock.
> + */
> +static struct lock_class_key irq_nested_lock_class;
> +
> +/*
> + * irq_map_dumb_demux_chip - Map a dumb demux chip for an irq domain
> + */
> +static int irq_map_dumb_demux_chip(struct irq_domain *d,
> +				   unsigned int virq,
> +				   irq_hw_number_t hw_irq)
> +{
> +	struct irq_chip_dumb_demux *demux = d->host_data;
> +
> +	if (!test_bit(hw_irq, &demux->available))
> +		return -EINVAL;
> +
> +	if (demux->flags & IRQ_DD_INIT_NESTED_LOCK)
> +		irq_set_lockdep_class(virq, &irq_nested_lock_class);
> +
> +	irq_set_chip(virq, &irq_dumb_demux_chip);
> +	irq_set_chip_data(virq, demux);
> +	irq_modify_status(virq, demux->irq_flags_to_clear,
> +			  demux->irq_flags_to_set);
> +
> +	return 0;
> +}
> +
> +struct irq_domain_ops irq_dumb_demux_domain_ops = {
> +	.map	= irq_map_dumb_demux_chip,
> +	.xlate	= irq_domain_xlate_onecell,
> +};
> +EXPORT_SYMBOL_GPL(irq_dumb_demux_domain_ops);
> +
> +/**
> + * irq_dumb_demux_handler - Dumb demux flow handler
> + * @irq:		Virtual irq number
> + * @irq_desc:		irq descriptor
> + */
> +void irq_dumb_demux_handler(unsigned int irq, struct irq_desc *desc)
> +{
> +	struct irq_chip_dumb_demux *demux = irq_get_handler_data(irq);
> +	struct irq_chip *chip = irq_desc_get_chip(desc);
> +	irqreturn_t ret = IRQ_NONE;
> +	int i;
> +
> +	chained_irq_enter(chip, desc);
> +	for_each_set_bit(i, &demux->unmasked, BITS_PER_LONG) {
> +		int demuxed_irq = irq_find_mapping(demux->domain, i);
> +		struct irq_desc *desc = irq_to_desc(demuxed_irq);
> +
> +		ret |= handle_dumb_demux_irq(demuxed_irq, desc);
> +	}
> +	chained_irq_exit(chip, desc);
> +
> +	if (!noirqdebug)
> +		note_interrupt(irq, desc, ret);
> +}
> +EXPORT_SYMBOL_GPL(irq_dumb_demux_handler);
> +
> +/**
> + * irq_alloc_dumb_demux_chip - Allocate a dumb demux chip
> + * @src_irq:		irq feeding the dumb demux chip
> + * @dd_flags:		irq_dumb_demux_flags flags
> + * @valid_irqs:		Bitmask representing valid irqs
> + * @clr_flags:		irq_flags to clear when mapping an interrupt
> + * @set_flags:		irq_flags to set when mapping an interrupt

Nit. not same order as the function parameters...

> + */
> +struct irq_chip_dumb_demux *
> +irq_alloc_dumb_demux_chip(unsigned int src_irq,
> +			  unsigned long valid_irqs,
> +			  unsigned int clr_flags,
> +			  unsigned int set_flags,
> +			  unsigned int dd_flags)
> +{
> +	struct irq_chip_dumb_demux *demux;
> +
> +	if (!src_irq)
> +		return ERR_PTR(-EINVAL);
> +
> +	demux = kzalloc(sizeof(*demux), GFP_KERNEL);
> +	if (!demux)
> +		return ERR_PTR(-ENOMEM);
> +
> +	demux->available = valid_irqs;
> +	demux->flags = dd_flags;
> +	demux->src_irq = src_irq;
> +	demux->irq_flags_to_clear = clr_flags;
> +	demux->irq_flags_to_set = set_flags;
> +
> +	return demux;
> +}
> +EXPORT_SYMBOL_GPL(irq_alloc_dumb_demux_chip);
> diff --git a/kernel/irq/handle.c b/kernel/irq/handle.c
> index 6354802..f786850 100644
> --- a/kernel/irq/handle.c
> +++ b/kernel/irq/handle.c
> @@ -131,7 +131,8 @@ void __irq_wake_thread(struct irq_desc *desc, struct irqaction *action)
>  }
>  
>  irqreturn_t
> -handle_irq_event_percpu(struct irq_desc *desc, struct irqaction *action)
> +handle_irq_event_percpu_no_spurious_check(struct irq_desc *desc,
> +					  struct irqaction *action)
>  {
>  	irqreturn_t retval = IRQ_NONE;
>  	unsigned int flags = 0, irq = desc->irq_data.irq;
> @@ -175,8 +176,18 @@ handle_irq_event_percpu(struct irq_desc *desc, struct irqaction *action)
>  
>  	add_interrupt_randomness(irq, flags);
>  
> +	return retval;
> +}
> +
> +irqreturn_t
> +handle_irq_event_percpu(struct irq_desc *desc, struct irqaction *action)
> +{
> +	irqreturn_t retval;
> +
> +	retval = handle_irq_event_percpu_no_spurious_check(desc, action);
> +
>  	if (!noirqdebug)
> -		note_interrupt(irq, desc, retval);
> +		note_interrupt(desc->irq_data.irq, desc, retval);
>  	return retval;
>  }
>  
> @@ -195,3 +206,19 @@ irqreturn_t handle_irq_event(struct irq_desc *desc)
>  	irqd_clear(&desc->irq_data, IRQD_IRQ_INPROGRESS);
>  	return ret;
>  }
> +
> +irqreturn_t handle_irq_event_no_spurious_check(struct irq_desc *desc)
> +{
> +	struct irqaction *action = desc->action;
> +	irqreturn_t ret;
> +
> +	desc->istate &= ~IRQS_PENDING;
> +	irqd_set(&desc->irq_data, IRQD_IRQ_INPROGRESS);
> +	raw_spin_unlock(&desc->lock);
> +
> +	ret = handle_irq_event_percpu_no_spurious_check(desc, action);
> +
> +	raw_spin_lock(&desc->lock);
> +	irqd_clear(&desc->irq_data, IRQD_IRQ_INPROGRESS);
> +	return ret;
> +}
> diff --git a/kernel/irq/internals.h b/kernel/irq/internals.h
> index df553b0..fe056fb 100644
> --- a/kernel/irq/internals.h
> +++ b/kernel/irq/internals.h
> @@ -90,6 +90,9 @@ extern void init_kstat_irqs(struct irq_desc *desc, int node, int nr);
>  
>  irqreturn_t handle_irq_event_percpu(struct irq_desc *desc, struct irqaction *action);
>  irqreturn_t handle_irq_event(struct irq_desc *desc);
> +irqreturn_t handle_irq_event_percpu_no_spurious_check(struct irq_desc *desc,
> +						      struct irqaction *action);
> +irqreturn_t handle_irq_event_no_spurious_check(struct irq_desc *desc);
>  
>  /* Resending of interrupts :*/
>  void check_irq_resend(struct irq_desc *desc, unsigned int irq);

Otherwise, it look okay to me:

Acked-by: Nicolas Ferre <nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>

I'm still a little bit frighten about the additional latency added to
the timer interrupt. But if it's a valid way to solve the problem for
these older SoCs, I'm for it...

Bye,
-- 
Nicolas Ferre
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/5] irqchip: add dumb demultiplexer implementation

[Boris Brezillon]

On Wed, 14 Jan 2015 14:36:42 +0100
Nicolas Ferre <nicolas.ferre@atmel.com> wrote:

> Le 13/01/2015 19:46, Boris Brezillon a écrit :
> > Some interrupt controllers are multiplexing several peripheral IRQs on
> > a single interrupt line.
> > While this is not a problem for most IRQs (as long as all peripherals
> > request the interrupt with IRQF_SHARED flag set), multiplexing timers and
> > other type of peripherals will generate a WARNING (mixing IRQF_NO_SUSPEND
> > and !IRQF_NO_SUSPEND is prohibited).
> > 
> > Create a dumb irq demultiplexer which simply forwards interrupts to all
> > peripherals (exactly what's happening with IRQ_SHARED) but keep a unique
> > irq number for each peripheral, thus preventing the IRQF_NO_SUSPEND
> > and !IRQF_NO_SUSPEND mix on a given interrupt.
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > ---
> >  drivers/irqchip/Kconfig          |   4 ++
> >  drivers/irqchip/Makefile         |   1 +
> >  drivers/irqchip/irq-dumb-demux.c |  70 ++++++++++++++++++++
> >  include/linux/irq.h              |  49 ++++++++++++++
> >  include/linux/irqdomain.h        |   1 +
> >  kernel/irq/Kconfig               |   5 ++
> >  kernel/irq/Makefile              |   1 +
> >  kernel/irq/chip.c                |  41 ++++++++++++
> >  kernel/irq/dumb-demux-chip.c     | 140 +++++++++++++++++++++++++++++++++++++++
> >  kernel/irq/handle.c              |  31 ++++++++-
> >  kernel/irq/internals.h           |   3 +
> >  11 files changed, 344 insertions(+), 2 deletions(-)
> >  create mode 100644 drivers/irqchip/irq-dumb-demux.c
> >  create mode 100644 kernel/irq/dumb-demux-chip.c
> > 
> > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> > index cc79d2a..8a9df88 100644
> > --- a/drivers/irqchip/Kconfig
> > +++ b/drivers/irqchip/Kconfig
> > @@ -70,6 +70,10 @@ config BRCMSTB_L2_IRQ
> >  	select GENERIC_IRQ_CHIP
> >  	select IRQ_DOMAIN
> >  
> > +config DUMB_DEMUX_IRQ
> > +	bool
> > +	select DUMB_IRQ_DEMUX_CHIP
> > +
> >  config DW_APB_ICTL
> >  	bool
> >  	select GENERIC_IRQ_CHIP
> > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> > index 9516a32..77f3c51 100644
> > --- a/drivers/irqchip/Makefile
> > +++ b/drivers/irqchip/Makefile
> > @@ -8,6 +8,7 @@ obj-$(CONFIG_ARCH_MVEBU)		+= irq-armada-370-xp.o
> >  obj-$(CONFIG_ARCH_MXS)			+= irq-mxs.o
> >  obj-$(CONFIG_ARCH_S3C24XX)		+= irq-s3c24xx.o
> >  obj-$(CONFIG_DW_APB_ICTL)		+= irq-dw-apb-ictl.o
> > +obj-$(CONFIG_DUMB_DEMUX_IRQ)		+= irq-dumb-demux.o
> >  obj-$(CONFIG_METAG)			+= irq-metag-ext.o
> >  obj-$(CONFIG_METAG_PERFCOUNTER_IRQS)	+= irq-metag.o
> >  obj-$(CONFIG_ARCH_MOXART)		+= irq-moxart.o
> > diff --git a/drivers/irqchip/irq-dumb-demux.c b/drivers/irqchip/irq-dumb-demux.c
> > new file mode 100644
> > index 0000000..dfa05ce
> > --- /dev/null
> > +++ b/drivers/irqchip/irq-dumb-demux.c
> > @@ -0,0 +1,70 @@
> 
> Maybe add a little file header here. It's always better.

Sure, I just forgot it.

> > +#ifdef CONFIG_DUMB_IRQ_DEMUX_CHIP
> > +/**
> > + *	handle_dumb_demux_irq - Dumb demuxer irq handle function.
> > + *	@irq:	the interrupt number
> > + *	@desc:	the interrupt description structure for this irq
> > + *
> > + *	Dumb demux interrupts are sent from a demultiplexing interrupt handler
> > + *	which is not able to decide which child interrupt interrupt handler
> 
> typo: "interrupt interrupt"

I'll fix that.

> 
> > + *	should be called.
> > + *
> > + *	Note: The caller is expected to handle the ack, clear, mask and
> > + *	unmask issues if necessary.
> > + */

[...]

> > +
> >  /*
> >   * Called unconditionally from handle_level_irq() and only for oneshot
> >   * interrupts from handle_fasteoi_irq()
> > diff --git a/kernel/irq/dumb-demux-chip.c b/kernel/irq/dumb-demux-chip.c
> > new file mode 100644
> > index 0000000..8e2de1d
> > --- /dev/null
> > +++ b/kernel/irq/dumb-demux-chip.c
> > @@ -0,0 +1,140 @@
> > +/*
> > + * Library implementing common dumb irq demux chip functions
> > + *
> > + * Copyright (C) 2015, Boris Brezillon
> 
> License here, please.

Yep, I'll add it.

> 
> > + */
> > +#include <linux/err.h>
> > +#include <linux/io.h>
> > +#include <linux/irq.h>
> > +#include <linux/slab.h>
> > +#include <linux/export.h>
> > +#include <linux/irq.h>
> > +#include <linux/irqdomain.h>
> > +#include <linux/irqchip/chained_irq.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/kernel_stat.h>
> > +#include <linux/syscore_ops.h>
> > +
> > +#include "internals.h"
> > +
> > +static void irq_dumb_demux_mask(struct irq_data *d)
> > +{
> > +	struct irq_chip_dumb_demux *demux = irq_data_get_irq_chip_data(d);
> > +
> > +	clear_bit(d->hwirq, &demux->unmasked);
> > +
> > +	if (!demux->unmasked)
> > +		disable_irq_nosync(demux->src_irq);
> > +}
> > +
> > +static void irq_dumb_demux_unmask(struct irq_data *d)
> > +{
> > +	struct irq_chip_dumb_demux *demux = irq_data_get_irq_chip_data(d);
> > +	bool enable_src_irq = !demux->unmasked;
> 
> Why this additional "bool" unlike the other function above?

Because set_bit will modify the unmasked status and we must check if it
is equal to 0 (in other terms, all irqs are masked) before modifying it
in order to know whether we should enable the src irq or not.

> 
> > +
> > +	set_bit(d->hwirq, &demux->unmasked);
> > +
> > +	if (enable_src_irq)
> > +		enable_irq(demux->src_irq);
> > +}
> > +

[...]

> > +
> > +/**
> > + * irq_alloc_dumb_demux_chip - Allocate a dumb demux chip
> > + * @src_irq:		irq feeding the dumb demux chip
> > + * @dd_flags:		irq_dumb_demux_flags flags
> > + * @valid_irqs:		Bitmask representing valid irqs
> > + * @clr_flags:		irq_flags to clear when mapping an interrupt
> > + * @set_flags:		irq_flags to set when mapping an interrupt
> 
> Nit. not same order as the function parameters...

I'll fix that.

Thanks,

Boris
-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/5] irqchip: add dumb demultiplexer implementation

[Nicolas Ferre]

Le 14/01/2015 15:03, Boris Brezillon a écrit :
> On Wed, 14 Jan 2015 14:36:42 +0100
> Nicolas Ferre <nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org> wrote:
> 
>> Le 13/01/2015 19:46, Boris Brezillon a écrit :
>>> Some interrupt controllers are multiplexing several peripheral IRQs on
>>> a single interrupt line.
>>> While this is not a problem for most IRQs (as long as all peripherals
>>> request the interrupt with IRQF_SHARED flag set), multiplexing timers and
>>> other type of peripherals will generate a WARNING (mixing IRQF_NO_SUSPEND
>>> and !IRQF_NO_SUSPEND is prohibited).
>>>
>>> Create a dumb irq demultiplexer which simply forwards interrupts to all
>>> peripherals (exactly what's happening with IRQ_SHARED) but keep a unique
>>> irq number for each peripheral, thus preventing the IRQF_NO_SUSPEND
>>> and !IRQF_NO_SUSPEND mix on a given interrupt.
>>>
>>> Signed-off-by: Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
>>> ---
>>>  drivers/irqchip/Kconfig          |   4 ++
>>>  drivers/irqchip/Makefile         |   1 +
>>>  drivers/irqchip/irq-dumb-demux.c |  70 ++++++++++++++++++++
>>>  include/linux/irq.h              |  49 ++++++++++++++
>>>  include/linux/irqdomain.h        |   1 +
>>>  kernel/irq/Kconfig               |   5 ++
>>>  kernel/irq/Makefile              |   1 +
>>>  kernel/irq/chip.c                |  41 ++++++++++++
>>>  kernel/irq/dumb-demux-chip.c     | 140 +++++++++++++++++++++++++++++++++++++++
>>>  kernel/irq/handle.c              |  31 ++++++++-
>>>  kernel/irq/internals.h           |   3 +
>>>  11 files changed, 344 insertions(+), 2 deletions(-)
>>>  create mode 100644 drivers/irqchip/irq-dumb-demux.c
>>>  create mode 100644 kernel/irq/dumb-demux-chip.c

[..]

>>> +static void irq_dumb_demux_mask(struct irq_data *d)
>>> +{
>>> +	struct irq_chip_dumb_demux *demux = irq_data_get_irq_chip_data(d);
>>> +
>>> +	clear_bit(d->hwirq, &demux->unmasked);
>>> +
>>> +	if (!demux->unmasked)
>>> +		disable_irq_nosync(demux->src_irq);
>>> +}
>>> +
>>> +static void irq_dumb_demux_unmask(struct irq_data *d)
>>> +{
>>> +	struct irq_chip_dumb_demux *demux = irq_data_get_irq_chip_data(d);
>>> +	bool enable_src_irq = !demux->unmasked;
>>
>> Why this additional "bool" unlike the other function above?
> 
> Because set_bit will modify the unmasked status and we must check if it
> is equal to 0 (in other terms, all irqs are masked) before modifying it
> in order to know whether we should enable the src irq or not.

pfffff! ok, sorry for the noise then ;-)


>>> +
>>> +	set_bit(d->hwirq, &demux->unmasked);
>>> +
>>> +	if (enable_src_irq)
>>> +		enable_irq(demux->src_irq);
>>> +}
>>> +

[...]

Bye,
-- 
Nicolas Ferre
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/5] irqchip: Add DT binding doc for dumb demuxer chips

[Boris Brezillon]

Add documentation for the dumb demuxer.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 .../bindings/interrupt-controller/dumb-demux.txt   | 34 ++++++++++++++++++++++
 1 file changed, 34 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt

diff --git a/Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt b/Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt
new file mode 100644
index 0000000..1c777ef
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt
@@ -0,0 +1,34 @@
+* Generic Dumb Interrupt Demultiplexer
+
+This Dumb demultiplixer simply forward all incoming interrupts to its
+enabled/unmasked children.
+
+Required properties:
+- compatible: Should be "irqchip-dumb-demux".
+- interrupt-controller: Identifies the node as an interrupt controller.
+- interrupts-extended or interrupt-parent and interrupts: Reference the source
+  interrupt connected to this dumb demuxer.
+- #interrupt-cells: The number of cells to define the interrupts (should be 1).
+  The only cell is the IRQ number.
+- irqs: u32 bitfield specifying the interrupts provided by the demuxer.
+
+Examples:
+	/*
+	 * Dumb demuxer controller
+	 */
+	dumb_irq1_demux: dumb-irq-demux@1 {
+		compatible = "irqchip-dumb-demux";
+		interrupt-controller;
+		#interrupt-cells = <1>;
+		interrupts-extended = <&aic 1 IRQ_TYPE_LEVEL_HIGH 7>;
+		irqs = <0x3f>;
+	};
+
+	/*
+	 * Device connected on this dumb demuxer
+	 */
+	dma: dma-controller@ffffec00 {
+		compatible = "atmel,at91sam9g45-dma";
+		reg = <0xffffec00 0x200>;
+		interrupts-extended = <&dumb_irq1_demux 0>;
+	};
-- 
1.9.1

Re: [PATCH v2 2/5] irqchip: Add DT binding doc for dumb demuxer chips

[Jason Cooper]

Boris,

On Tue, Jan 13, 2015 at 07:46:18PM +0100, Boris Brezillon wrote:
> Add documentation for the dumb demuxer.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
>  .../bindings/interrupt-controller/dumb-demux.txt   | 34 ++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt
> 
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt b/Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt
> new file mode 100644
> index 0000000..1c777ef
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt
> @@ -0,0 +1,34 @@
> +* Generic Dumb Interrupt Demultiplexer
> +
> +This Dumb demultiplixer simply forward all incoming interrupts to its
> +enabled/unmasked children.

Please forgive the potentially naïve question, but what hardware is this
describing?

thx,

Jason.

> +
> +Required properties:
> +- compatible: Should be "irqchip-dumb-demux".
> +- interrupt-controller: Identifies the node as an interrupt controller.
> +- interrupts-extended or interrupt-parent and interrupts: Reference the source
> +  interrupt connected to this dumb demuxer.
> +- #interrupt-cells: The number of cells to define the interrupts (should be 1).
> +  The only cell is the IRQ number.
> +- irqs: u32 bitfield specifying the interrupts provided by the demuxer.
> +
> +Examples:
> +	/*
> +	 * Dumb demuxer controller
> +	 */
> +	dumb_irq1_demux: dumb-irq-demux@1 {
> +		compatible = "irqchip-dumb-demux";
> +		interrupt-controller;
> +		#interrupt-cells = <1>;
> +		interrupts-extended = <&aic 1 IRQ_TYPE_LEVEL_HIGH 7>;
> +		irqs = <0x3f>;
> +	};
> +
> +	/*
> +	 * Device connected on this dumb demuxer
> +	 */
> +	dma: dma-controller@ffffec00 {
> +		compatible = "atmel,at91sam9g45-dma";
> +		reg = <0xffffec00 0x200>;
> +		interrupts-extended = <&dumb_irq1_demux 0>;
> +	};
> -- 
> 1.9.1
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/5] irqchip: Add DT binding doc for dumb demuxer chips

[Boris Brezillon]

Hi Jason,

On Tue, 13 Jan 2015 14:00:50 -0500
Jason Cooper <jason-NLaQJdtUoK4Be96aLqz0jA@public.gmane.org> wrote:

> Boris,
> 
> On Tue, Jan 13, 2015 at 07:46:18PM +0100, Boris Brezillon wrote:
> > Add documentation for the dumb demuxer.
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> > ---
> >  .../bindings/interrupt-controller/dumb-demux.txt   | 34 ++++++++++++++++++++++
> >  1 file changed, 34 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt b/Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt
> > new file mode 100644
> > index 0000000..1c777ef
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt
> > @@ -0,0 +1,34 @@
> > +* Generic Dumb Interrupt Demultiplexer
> > +
> > +This Dumb demultiplixer simply forward all incoming interrupts to its
> > +enabled/unmasked children.
> 
> Please forgive the potentially naïve question, but what hardware is this
> describing?

That's not a real hardware per se, but on some hardware (like at91 SoCs)
some IRQ line are shared by several peripherals, and this dumb
demultiplex is here to represent such shared irq lines which cannot be
easily demultiplexed (because they do not provide a 'cause'
register).

You can see it as a virtual irqchip provided to address broken hardware
designs.

Regards,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/5] irqchip: Add DT binding doc for dumb demuxer chips

[Jason Cooper]

Hey Boris,

On Tue, Jan 13, 2015 at 09:52:07PM +0100, Boris Brezillon wrote:
> On Tue, 13 Jan 2015 14:00:50 -0500 Jason Cooper <jason-NLaQJdtUoK4Be96aLqz0jA@public.gmane.org> wrote:
> > On Tue, Jan 13, 2015 at 07:46:18PM +0100, Boris Brezillon wrote:
> > > Add documentation for the dumb demuxer.
> > > 
> > > Signed-off-by: Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTGGXanvQGlWp@public.gmane.orgm>
> > > ---
> > >  .../bindings/interrupt-controller/dumb-demux.txt   | 34 ++++++++++++++++++++++
> > >  1 file changed, 34 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt
> > > 
> > > diff --git a/Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt b/Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt
> > > new file mode 100644
> > > index 0000000..1c777ef
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt
> > > @@ -0,0 +1,34 @@
> > > +* Generic Dumb Interrupt Demultiplexer
> > > +
> > > +This Dumb demultiplixer simply forward all incoming interrupts to its
> > > +enabled/unmasked children.
> > 
> > Please forgive the potentially naïve question, but what hardware is this
> > describing?
> 
> That's not a real hardware per se, but on some hardware (like at91 SoCs)
> some IRQ line are shared by several peripherals, and this dumb
> demultiplex is here to represent such shared irq lines which cannot be
> easily demultiplexed (because they do not provide a 'cause'
> register).
> 
> You can see it as a virtual irqchip provided to address broken hardware
> designs.

Hmm.  Well, given tglx's recent reply, I suppose I'll *not* go down the
rabbit hole of "the DT is for describing hardware."  :-P

However, it would probably be a lot more palatable to the DT maintainers
if we at least change the compatible to prepend "linux,".  This way, if
someone does come up with a better solution down the road, it will be
much easier to deprecate the binding.

I would also be amenable to "virt,", or "hack,", or even
"work-around-piss-poor-hw,".  Basically, anything that would indicate to
consumers of the DT that this is not a true reflection of the hardware,
and that it may be superseded by a better solution later.

It would also be helpful to explain the situation more fully in the
binding document.

thx,

Jason.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/5] irqchip: Add DT binding doc for dumb demuxer chips

[Boris Brezillon]

Hi Jason,

On Wed, 14 Jan 2015 13:56:19 -0500
Jason Cooper <jason-NLaQJdtUoK4Be96aLqz0jA@public.gmane.org> wrote:

> Hey Boris,
> 
> On Tue, Jan 13, 2015 at 09:52:07PM +0100, Boris Brezillon wrote:
> > On Tue, 13 Jan 2015 14:00:50 -0500 Jason Cooper <jason@lakedaemon.net> wrote:
> > > On Tue, Jan 13, 2015 at 07:46:18PM +0100, Boris Brezillon wrote:
> > > > Add documentation for the dumb demuxer.
> > > > 
> > > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > > > ---
> > > >  .../bindings/interrupt-controller/dumb-demux.txt   | 34 ++++++++++++++++++++++
> > > >  1 file changed, 34 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt b/Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt
> > > > new file mode 100644
> > > > index 0000000..1c777ef
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt
> > > > @@ -0,0 +1,34 @@
> > > > +* Generic Dumb Interrupt Demultiplexer
> > > > +
> > > > +This Dumb demultiplixer simply forward all incoming interrupts to its
> > > > +enabled/unmasked children.
> > > 
> > > Please forgive the potentially naïve question, but what hardware is this
> > > describing?
> > 
> > That's not a real hardware per se, but on some hardware (like at91 SoCs)
> > some IRQ line are shared by several peripherals, and this dumb
> > demultiplex is here to represent such shared irq lines which cannot be
> > easily demultiplexed (because they do not provide a 'cause'
> > register).
> > 
> > You can see it as a virtual irqchip provided to address broken hardware
> > designs.
> 
> Hmm.  Well, given tglx's recent reply, I suppose I'll *not* go down the
> rabbit hole of "the DT is for describing hardware."  :-P

Actually I'm a bit surprised no one else already mentioned that :-) (I
thought this would be the first complain regarding this dumb irq
demux chip).

Anyway, IMHO, this can be considered as hardware description since
these irq-lines are really multiplexed into a single one...

> 
> However, it would probably be a lot more palatable to the DT maintainers
> if we at least change the compatible to prepend "linux,".  This way, if
> someone does come up with a better solution down the road, it will be
> much easier to deprecate the binding.
> 
> I would also be amenable to "virt,", or "hack,", or even
> "work-around-piss-poor-hw,".  Basically, anything that would indicate to
> consumers of the DT that this is not a true reflection of the hardware,
> and that it may be superseded by a better solution later.

Actually I thought about changing it to "virtual,dumb-irq-demux" :-).

> 
> It would also be helpful to explain the situation more fully in the
> binding document.

Sure, I'll add a few lines to describe what this irqchip really is.

Regards,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/5] irqchip: Add DT binding doc for dumb demuxer chips

[Jason Cooper]

On Wed, Jan 14, 2015 at 08:08:09PM +0100, Boris Brezillon wrote:
> Hi Jason,
> 
> On Wed, 14 Jan 2015 13:56:19 -0500
> Jason Cooper <jason-NLaQJdtUoK4Be96aLqz0jA@public.gmane.org> wrote:
> 
> > Hey Boris,
> > 
> > On Tue, Jan 13, 2015 at 09:52:07PM +0100, Boris Brezillon wrote:
> > > On Tue, 13 Jan 2015 14:00:50 -0500 Jason Cooper <jason@lakedaemon.net> wrote:
> > > > On Tue, Jan 13, 2015 at 07:46:18PM +0100, Boris Brezillon wrote:
> > > > > Add documentation for the dumb demuxer.
> > > > > 
> > > > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > > > > ---
> > > > >  .../bindings/interrupt-controller/dumb-demux.txt   | 34 ++++++++++++++++++++++
> > > > >  1 file changed, 34 insertions(+)
> > > > >  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt
> > > > > 
> > > > > diff --git a/Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt b/Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt
> > > > > new file mode 100644
> > > > > index 0000000..1c777ef
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt
> > > > > @@ -0,0 +1,34 @@
> > > > > +* Generic Dumb Interrupt Demultiplexer
> > > > > +
> > > > > +This Dumb demultiplixer simply forward all incoming interrupts to its
> > > > > +enabled/unmasked children.
> > > > 
> > > > Please forgive the potentially naïve question, but what hardware is this
> > > > describing?
> > > 
> > > That's not a real hardware per se, but on some hardware (like at91 SoCs)
> > > some IRQ line are shared by several peripherals, and this dumb
> > > demultiplex is here to represent such shared irq lines which cannot be
> > > easily demultiplexed (because they do not provide a 'cause'
> > > register).
> > > 
> > > You can see it as a virtual irqchip provided to address broken hardware
> > > designs.
> > 
> > Hmm.  Well, given tglx's recent reply, I suppose I'll *not* go down the
> > rabbit hole of "the DT is for describing hardware."  :-P
> 
> Actually I'm a bit surprised no one else already mentioned that :-) (I
> thought this would be the first complain regarding this dumb irq
> demux chip).
> 
> Anyway, IMHO, this can be considered as hardware description since
> these irq-lines are really multiplexed into a single one...

True, I think the issue is more that the DT node doesn't strictly
represent a block of IP.  We just need a way to make that clear in the
binding doc.

> > However, it would probably be a lot more palatable to the DT maintainers
> > if we at least change the compatible to prepend "linux,".  This way, if
> > someone does come up with a better solution down the road, it will be
> > much easier to deprecate the binding.
> > 
> > I would also be amenable to "virt,", or "hack,", or even
> > "work-around-piss-poor-hw,".  Basically, anything that would indicate to
> > consumers of the DT that this is not a true reflection of the hardware,
> > and that it may be superseded by a better solution later.
> 
> Actually I thought about changing it to "virtual,dumb-irq-demux" :-).

Works for me. :)

thx,

Jason.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/5] irqchip: Add DT binding doc for dumb demuxer chips

[Nicolas Ferre]

Le 13/01/2015 19:46, Boris Brezillon a écrit :
> Add documentation for the dumb demuxer.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

Acked-by: Nicolas Ferre <nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>

> ---
>  .../bindings/interrupt-controller/dumb-demux.txt   | 34 ++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt
> 
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt b/Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt
> new file mode 100644
> index 0000000..1c777ef
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt
> @@ -0,0 +1,34 @@
> +* Generic Dumb Interrupt Demultiplexer
> +
> +This Dumb demultiplixer simply forward all incoming interrupts to its
> +enabled/unmasked children.
> +
> +Required properties:
> +- compatible: Should be "irqchip-dumb-demux".
> +- interrupt-controller: Identifies the node as an interrupt controller.
> +- interrupts-extended or interrupt-parent and interrupts: Reference the source
> +  interrupt connected to this dumb demuxer.
> +- #interrupt-cells: The number of cells to define the interrupts (should be 1).
> +  The only cell is the IRQ number.
> +- irqs: u32 bitfield specifying the interrupts provided by the demuxer.
> +
> +Examples:
> +	/*
> +	 * Dumb demuxer controller
> +	 */
> +	dumb_irq1_demux: dumb-irq-demux@1 {
> +		compatible = "irqchip-dumb-demux";
> +		interrupt-controller;
> +		#interrupt-cells = <1>;
> +		interrupts-extended = <&aic 1 IRQ_TYPE_LEVEL_HIGH 7>;
> +		irqs = <0x3f>;
> +	};
> +
> +	/*
> +	 * Device connected on this dumb demuxer
> +	 */
> +	dma: dma-controller@ffffec00 {
> +		compatible = "atmel,at91sam9g45-dma";
> +		reg = <0xffffec00 0x200>;
> +		interrupts-extended = <&dumb_irq1_demux 0>;
> +	};
> 


-- 
Nicolas Ferre
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 3/5] ARM: at91/dt: select DUMB_IRQ_DEMUX for all at91 SoCs

[Boris Brezillon]

at91 SoCs need an dumb irq demuxer to gracefully support the fact that
irq1 is shared by several devices and a timer.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 arch/arm/mach-at91/Kconfig | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/mach-at91/Kconfig b/arch/arm/mach-at91/Kconfig
index 2395c68..8ff2c2a 100644
--- a/arch/arm/mach-at91/Kconfig
+++ b/arch/arm/mach-at91/Kconfig
@@ -28,6 +28,7 @@ config HAVE_AT91_H32MX
 config SOC_AT91SAM9
 	bool
 	select ATMEL_AIC_IRQ
+	select DUMB_DEMUX_IRQ
 	select COMMON_CLK_AT91
 	select CPU_ARM926T
 	select GENERIC_CLOCKEVENTS
@@ -98,6 +99,7 @@ if SOC_SAM_V4_V5
 config SOC_AT91RM9200
 	bool "AT91RM9200"
 	select ATMEL_AIC_IRQ
+	select DUMB_DEMUX_IRQ
 	select COMMON_CLK_AT91
 	select CPU_ARM920T
 	select GENERIC_CLOCKEVENTS
-- 
1.9.1

Re: [PATCH v2 3/5] ARM: at91/dt: select DUMB_IRQ_DEMUX for all at91 SoCs

[Nicolas Ferre]

Le 13/01/2015 19:46, Boris Brezillon a écrit :
> at91 SoCs need an dumb irq demuxer to gracefully support the fact that

"Older AT91 SoCs" please ;-)

Nit.: "an dumb demuxer" => "a virtual dumb demuxer"

> irq1 is shared by several devices and a timer.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

As the newer SoCs don't need it, we can't select this option for the
whole CONFIG_ARCH_AT91, so indeed:
Acked-by: Nicolas Ferre <nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>

> ---
>  arch/arm/mach-at91/Kconfig | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/arm/mach-at91/Kconfig b/arch/arm/mach-at91/Kconfig
> index 2395c68..8ff2c2a 100644
> --- a/arch/arm/mach-at91/Kconfig
> +++ b/arch/arm/mach-at91/Kconfig
> @@ -28,6 +28,7 @@ config HAVE_AT91_H32MX
>  config SOC_AT91SAM9
>  	bool
>  	select ATMEL_AIC_IRQ
> +	select DUMB_DEMUX_IRQ
>  	select COMMON_CLK_AT91
>  	select CPU_ARM926T
>  	select GENERIC_CLOCKEVENTS
> @@ -98,6 +99,7 @@ if SOC_SAM_V4_V5
>  config SOC_AT91RM9200
>  	bool "AT91RM9200"
>  	select ATMEL_AIC_IRQ
> +	select DUMB_DEMUX_IRQ
>  	select COMMON_CLK_AT91
>  	select CPU_ARM920T
>  	select GENERIC_CLOCKEVENTS
> 


-- 
Nicolas Ferre
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 4/5] ARM: at91/dt: add AIC irq1 muxed peripheral id definitions

[Boris Brezillon]

These ids will be used to define irqs multiplexed on the first irq line.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 .../dt-bindings/interrupt-controller/atmel-aic.h   | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)
 create mode 100644 include/dt-bindings/interrupt-controller/atmel-aic.h

diff --git a/include/dt-bindings/interrupt-controller/atmel-aic.h b/include/dt-bindings/interrupt-controller/atmel-aic.h
new file mode 100644
index 0000000..e14a94f
--- /dev/null
+++ b/include/dt-bindings/interrupt-controller/atmel-aic.h
@@ -0,0 +1,22 @@
+#ifndef _DT_BINDINGS_INTERRUPT_CONTROLLER_ARM_GIC_H
+#define _DT_BINDINGS_INTERRUPT_CONTROLLER_ARM_GIC_H
+
+#include <dt-bindings/interrupt-controller/irq.h>
+
+#define AIC_IRQ1_PMC		0
+#define AIC_IRQ1_ST		1
+#define AIC_IRQ1_PIT		1
+#define AIC_IRQ1_DBGU		2
+#define AIC_IRQ1_RTC		3
+#define AIC_IRQ1_RTT		4
+#define AIC_IRQ1_WATCHDOG	5
+#define AIC_IRQ1_MC		6
+#define AIC_IRQ1_SDRAMC		6
+#define AIC_IRQ1_DDRSDRC	6
+#define AIC_IRQ1_RSTC		7
+#define AIC_IRQ1_PMERRLOC	8
+#define AIC_IRQ1_PMECC		9
+
+#define AIC_IRQ_MASK(x)		(1 << x)
+
+#endif
-- 
1.9.1

Re: [PATCH v2 4/5] ARM: at91/dt: add AIC irq1 muxed peripheral id definitions

[Nicolas Ferre]

Le 13/01/2015 19:46, Boris Brezillon a écrit :
> These ids will be used to define irqs multiplexed on the first irq line.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
>  .../dt-bindings/interrupt-controller/atmel-aic.h   | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
>  create mode 100644 include/dt-bindings/interrupt-controller/atmel-aic.h
> 
> diff --git a/include/dt-bindings/interrupt-controller/atmel-aic.h b/include/dt-bindings/interrupt-controller/atmel-aic.h
> new file mode 100644
> index 0000000..e14a94f
> --- /dev/null
> +++ b/include/dt-bindings/interrupt-controller/atmel-aic.h
> @@ -0,0 +1,22 @@
> +#ifndef _DT_BINDINGS_INTERRUPT_CONTROLLER_ARM_GIC_H
> +#define _DT_BINDINGS_INTERRUPT_CONTROLLER_ARM_GIC_H

Nit.: why ARM_GIC?

> +#include <dt-bindings/interrupt-controller/irq.h>
> +
> +#define AIC_IRQ1_PMC		0
> +#define AIC_IRQ1_ST		1
> +#define AIC_IRQ1_PIT		1
> +#define AIC_IRQ1_DBGU		2
> +#define AIC_IRQ1_RTC		3
> +#define AIC_IRQ1_RTT		4
> +#define AIC_IRQ1_WATCHDOG	5
> +#define AIC_IRQ1_MC		6
> +#define AIC_IRQ1_SDRAMC		6
> +#define AIC_IRQ1_DDRSDRC	6
> +#define AIC_IRQ1_RSTC		7
> +#define AIC_IRQ1_PMERRLOC	8
> +#define AIC_IRQ1_PMECC		9




> +
> +#define AIC_IRQ_MASK(x)		(1 << x)
> +
> +#endif
> 


-- 
Nicolas Ferre
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 4/5] ARM: at91/dt: add AIC irq1 muxed peripheral id definitions

[Boris Brezillon]

On Wed, 14 Jan 2015 14:21:47 +0100
Nicolas Ferre <nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org> wrote:

> Le 13/01/2015 19:46, Boris Brezillon a écrit :
> > These ids will be used to define irqs multiplexed on the first irq line.
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> > ---
> >  .../dt-bindings/interrupt-controller/atmel-aic.h   | 22 ++++++++++++++++++++++
> >  1 file changed, 22 insertions(+)
> >  create mode 100644 include/dt-bindings/interrupt-controller/atmel-aic.h
> > 
> > diff --git a/include/dt-bindings/interrupt-controller/atmel-aic.h b/include/dt-bindings/interrupt-controller/atmel-aic.h
> > new file mode 100644
> > index 0000000..e14a94f
> > --- /dev/null
> > +++ b/include/dt-bindings/interrupt-controller/atmel-aic.h
> > @@ -0,0 +1,22 @@
> > +#ifndef _DT_BINDINGS_INTERRUPT_CONTROLLER_ARM_GIC_H
> > +#define _DT_BINDINGS_INTERRUPT_CONTROLLER_ARM_GIC_H
> 
> Nit.: why ARM_GIC?

Because I copied it from the arm-gic.h header and forgot to rename
those macros :-).
I'll fix that.

> 
> > +#include <dt-bindings/interrupt-controller/irq.h>
> > +
> > +#define AIC_IRQ1_PMC		0
> > +#define AIC_IRQ1_ST		1
> > +#define AIC_IRQ1_PIT		1
> > +#define AIC_IRQ1_DBGU		2
> > +#define AIC_IRQ1_RTC		3
> > +#define AIC_IRQ1_RTT		4
> > +#define AIC_IRQ1_WATCHDOG	5
> > +#define AIC_IRQ1_MC		6
> > +#define AIC_IRQ1_SDRAMC		6
> > +#define AIC_IRQ1_DDRSDRC	6
> > +#define AIC_IRQ1_RSTC		7
> > +#define AIC_IRQ1_PMERRLOC	8
> > +#define AIC_IRQ1_PMECC		9

Following up the discussion we had privately regarding these
macros (whether we really need them or not), maybe adding a comment
above the irqs property would be enough:

	dumb_irq1_demux: dumb-irq-demux@1 {
		compatible = "irqchip-dumb-demux";
		interrupt-controller;
		#interrupt-cells = <1>;
		interrupts = <1 IRQ_TYPE_LEVEL_HIGH 7>;
		/*
		 * Interrupt lines:
		 * 0: PMC
		 * 1: PIT
		 * 2: DBGU
		 * 3: RTT
		 * 4: WATCHDOG
		 * 5: RSTC
		 */
		irqs = <0x3f>;
	};

Thanks,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 4/5] ARM: at91/dt: add AIC irq1 muxed peripheral id definitions

[Nicolas Ferre]

Le 14/01/2015 14:34, Boris Brezillon a écrit :
> On Wed, 14 Jan 2015 14:21:47 +0100
> Nicolas Ferre <nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org> wrote:
> 
>> Le 13/01/2015 19:46, Boris Brezillon a écrit :
>>> These ids will be used to define irqs multiplexed on the first irq line.
>>>
>>> Signed-off-by: Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
>>> ---
>>>  .../dt-bindings/interrupt-controller/atmel-aic.h   | 22 ++++++++++++++++++++++
>>>  1 file changed, 22 insertions(+)
>>>  create mode 100644 include/dt-bindings/interrupt-controller/atmel-aic.h
>>>
>>> diff --git a/include/dt-bindings/interrupt-controller/atmel-aic.h b/include/dt-bindings/interrupt-controller/atmel-aic.h
>>> new file mode 100644
>>> index 0000000..e14a94f
>>> --- /dev/null
>>> +++ b/include/dt-bindings/interrupt-controller/atmel-aic.h
>>> @@ -0,0 +1,22 @@
>>> +#ifndef _DT_BINDINGS_INTERRUPT_CONTROLLER_ARM_GIC_H
>>> +#define _DT_BINDINGS_INTERRUPT_CONTROLLER_ARM_GIC_H
>>
>> Nit.: why ARM_GIC?
> 
> Because I copied it from the arm-gic.h header and forgot to rename
> those macros :-).
> I'll fix that.
> 
>>
>>> +#include <dt-bindings/interrupt-controller/irq.h>
>>> +
>>> +#define AIC_IRQ1_PMC		0
>>> +#define AIC_IRQ1_ST		1
>>> +#define AIC_IRQ1_PIT		1
>>> +#define AIC_IRQ1_DBGU		2
>>> +#define AIC_IRQ1_RTC		3
>>> +#define AIC_IRQ1_RTT		4
>>> +#define AIC_IRQ1_WATCHDOG	5
>>> +#define AIC_IRQ1_MC		6
>>> +#define AIC_IRQ1_SDRAMC		6
>>> +#define AIC_IRQ1_DDRSDRC	6
>>> +#define AIC_IRQ1_RSTC		7
>>> +#define AIC_IRQ1_PMERRLOC	8
>>> +#define AIC_IRQ1_PMECC		9
> 
> Following up the discussion we had privately regarding these
> macros (whether we really need them or not), maybe adding a comment
> above the irqs property would be enough:
> 
> 	dumb_irq1_demux: dumb-irq-demux@1 {
> 		compatible = "irqchip-dumb-demux";
> 		interrupt-controller;
> 		#interrupt-cells = <1>;
> 		interrupts = <1 IRQ_TYPE_LEVEL_HIGH 7>;
> 		/*
> 		 * Interrupt lines:
> 		 * 0: PMC
> 		 * 1: PIT
> 		 * 2: DBGU
> 		 * 3: RTT
> 		 * 4: WATCHDOG
> 		 * 5: RSTC
> 		 */
> 		irqs = <0x3f>;
> 	};

Well, I tend to prefer when no additional macros are defined in DT for
no real added value. So, this comment seems to be enough... I would vote
for this solution ;-)

Thanks, bye,
-- 
Nicolas Ferre
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 5/5] ARM: at91/dt: define a dumb irq demultiplexer chip connected on irq1

[Boris Brezillon]

IRQ is multiplexing several peripheral IRQs, but there's no way to
properly demultiplex those IRQs.
Use a dumb irq demux chip to achieve this demultiplexing operation.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 arch/arm/boot/dts/at91sam9260.dtsi | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

diff --git a/arch/arm/boot/dts/at91sam9260.dtsi b/arch/arm/boot/dts/at91sam9260.dtsi
index dd1313c..debe8d2 100644
--- a/arch/arm/boot/dts/at91sam9260.dtsi
+++ b/arch/arm/boot/dts/at91sam9260.dtsi
@@ -10,7 +10,7 @@
 
 #include "skeleton.dtsi"
 #include <dt-bindings/pinctrl/at91.h>
-#include <dt-bindings/interrupt-controller/irq.h>
+#include <dt-bindings/interrupt-controller/atmel-aic.h>
 #include <dt-bindings/gpio/gpio.h>
 #include <dt-bindings/clock/at91.h>
 
@@ -89,6 +89,7 @@
 				atmel,external-irqs = <29 30 31>;
 			};
 
+
 			ramc0: ramc@ffffea00 {
 				compatible = "atmel,at91sam9260-sdramc";
 				reg = <0xffffea00 0x200>;
@@ -97,7 +98,7 @@
 			pmc: pmc@fffffc00 {
 				compatible = "atmel,at91sam9260-pmc";
 				reg = <0xfffffc00 0x100>;
-				interrupts = <1 IRQ_TYPE_LEVEL_HIGH 7>;
+				interrupts-extended = <&dumb_irq1_demux AIC_IRQ1_PMC>;
 				interrupt-controller;
 				#address-cells = <1>;
 				#size-cells = <0>;
@@ -364,7 +365,7 @@
 			pit: timer@fffffd30 {
 				compatible = "atmel,at91sam9260-pit";
 				reg = <0xfffffd30 0xf>;
-				interrupts = <1 IRQ_TYPE_LEVEL_HIGH 7>;
+				interrupts-extended = <&dumb_irq1_demux AIC_IRQ1_PIT>;
 				clocks = <&mck>;
 			};
 
@@ -750,7 +751,7 @@
 			dbgu: serial@fffff200 {
 				compatible = "atmel,at91sam9260-usart";
 				reg = <0xfffff200 0x200>;
-				interrupts = <1 IRQ_TYPE_LEVEL_HIGH 7>;
+				interrupts-extended = <&dumb_irq1_demux AIC_IRQ1_DBGU>;
 				pinctrl-names = "default";
 				pinctrl-0 = <&pinctrl_dbgu>;
 				clocks = <&mck>;
@@ -959,7 +960,7 @@
 			rtc@fffffd20 {
 				compatible = "atmel,at91sam9260-rtt";
 				reg = <0xfffffd20 0x10>;
-				interrupts = <1 IRQ_TYPE_LEVEL_HIGH 7>;
+				interrupts-extended = <&dumb_irq1_demux AIC_IRQ1_RTT>;
 				clocks = <&clk32k>;
 				status = "disabled";
 			};
@@ -967,7 +968,7 @@
 			watchdog@fffffd40 {
 				compatible = "atmel,at91sam9260-wdt";
 				reg = <0xfffffd40 0x10>;
-				interrupts = <1 IRQ_TYPE_LEVEL_HIGH 7>;
+				interrupts-extended = <&dumb_irq1_demux AIC_IRQ1_WATCHDOG>;
 				atmel,watchdog-type = "hardware";
 				atmel,reset-type = "all";
 				atmel,dbg-halt;
@@ -1010,6 +1011,19 @@
 		};
 	};
 
+	dumb_irq1_demux: dumb-irq-demux@1 {
+		compatible = "irqchip-dumb-demux";
+		interrupt-controller;
+		#interrupt-cells = <1>;
+		interrupts = <1 IRQ_TYPE_LEVEL_HIGH 7>;
+		irqs = <(AIC_IRQ_MASK(AIC_IRQ1_PMC) |
+			 AIC_IRQ_MASK(AIC_IRQ1_PIT) |
+			 AIC_IRQ_MASK(AIC_IRQ1_DBGU) |
+			 AIC_IRQ_MASK(AIC_IRQ1_RTT) |
+			 AIC_IRQ_MASK(AIC_IRQ1_WATCHDOG) |
+			 AIC_IRQ_MASK(AIC_IRQ1_RSTC))>;
+	};
+
 	i2c@0 {
 		compatible = "i2c-gpio";
 		gpios = <&pioA 23 GPIO_ACTIVE_HIGH /* sda */
-- 
1.9.1

Re: [PATCH v2 5/5] ARM: at91/dt: define a dumb irq demultiplexer chip connected on irq1

[Nicolas Ferre]

Le 13/01/2015 19:46, Boris Brezillon a écrit :
> IRQ is multiplexing several peripheral IRQs, but there's no way to
> properly demultiplex those IRQs.
> Use a dumb irq demux chip to achieve this demultiplexing operation.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
>  arch/arm/boot/dts/at91sam9260.dtsi | 26 ++++++++++++++++++++------
>  1 file changed, 20 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/at91sam9260.dtsi b/arch/arm/boot/dts/at91sam9260.dtsi
> index dd1313c..debe8d2 100644
> --- a/arch/arm/boot/dts/at91sam9260.dtsi
> +++ b/arch/arm/boot/dts/at91sam9260.dtsi
> @@ -10,7 +10,7 @@
>  
>  #include "skeleton.dtsi"
>  #include <dt-bindings/pinctrl/at91.h>
> -#include <dt-bindings/interrupt-controller/irq.h>
> +#include <dt-bindings/interrupt-controller/atmel-aic.h>
>  #include <dt-bindings/gpio/gpio.h>
>  #include <dt-bindings/clock/at91.h>
>  
> @@ -89,6 +89,7 @@
>  				atmel,external-irqs = <29 30 31>;
>  			};
>  
> +
>  			ramc0: ramc@ffffea00 {
>  				compatible = "atmel,at91sam9260-sdramc";
>  				reg = <0xffffea00 0x200>;
> @@ -97,7 +98,7 @@
>  			pmc: pmc@fffffc00 {
>  				compatible = "atmel,at91sam9260-pmc";
>  				reg = <0xfffffc00 0x100>;
> -				interrupts = <1 IRQ_TYPE_LEVEL_HIGH 7>;
> +				interrupts-extended = <&dumb_irq1_demux AIC_IRQ1_PMC>;
>  				interrupt-controller;
>  				#address-cells = <1>;
>  				#size-cells = <0>;
> @@ -364,7 +365,7 @@
>  			pit: timer@fffffd30 {
>  				compatible = "atmel,at91sam9260-pit";
>  				reg = <0xfffffd30 0xf>;
> -				interrupts = <1 IRQ_TYPE_LEVEL_HIGH 7>;
> +				interrupts-extended = <&dumb_irq1_demux AIC_IRQ1_PIT>;
>  				clocks = <&mck>;
>  			};
>  
> @@ -750,7 +751,7 @@
>  			dbgu: serial@fffff200 {
>  				compatible = "atmel,at91sam9260-usart";
>  				reg = <0xfffff200 0x200>;
> -				interrupts = <1 IRQ_TYPE_LEVEL_HIGH 7>;
> +				interrupts-extended = <&dumb_irq1_demux AIC_IRQ1_DBGU>;
>  				pinctrl-names = "default";
>  				pinctrl-0 = <&pinctrl_dbgu>;
>  				clocks = <&mck>;
> @@ -959,7 +960,7 @@
>  			rtc@fffffd20 {
>  				compatible = "atmel,at91sam9260-rtt";
>  				reg = <0xfffffd20 0x10>;
> -				interrupts = <1 IRQ_TYPE_LEVEL_HIGH 7>;
> +				interrupts-extended = <&dumb_irq1_demux AIC_IRQ1_RTT>;
>  				clocks = <&clk32k>;
>  				status = "disabled";
>  			};
> @@ -967,7 +968,7 @@
>  			watchdog@fffffd40 {
>  				compatible = "atmel,at91sam9260-wdt";
>  				reg = <0xfffffd40 0x10>;
> -				interrupts = <1 IRQ_TYPE_LEVEL_HIGH 7>;
> +				interrupts-extended = <&dumb_irq1_demux AIC_IRQ1_WATCHDOG>;
>  				atmel,watchdog-type = "hardware";
>  				atmel,reset-type = "all";
>  				atmel,dbg-halt;
> @@ -1010,6 +1011,19 @@
>  		};
>  	};
>  
> +	dumb_irq1_demux: dumb-irq-demux@1 {
> +		compatible = "irqchip-dumb-demux";
> +		interrupt-controller;
> +		#interrupt-cells = <1>;
> +		interrupts = <1 IRQ_TYPE_LEVEL_HIGH 7>;
> +		irqs = <(AIC_IRQ_MASK(AIC_IRQ1_PMC) |
> +			 AIC_IRQ_MASK(AIC_IRQ1_PIT) |
> +			 AIC_IRQ_MASK(AIC_IRQ1_DBGU) |
> +			 AIC_IRQ_MASK(AIC_IRQ1_RTT) |
> +			 AIC_IRQ_MASK(AIC_IRQ1_WATCHDOG) |
> +			 AIC_IRQ_MASK(AIC_IRQ1_RSTC))>;
> +	};
> +

Yep, with the modification that you suggested in "Re: [PATCH v2 4/5]
ARM: at91/dt: add AIC irq1 muxed peripheral id definitions" it would be
even simpler.

Acked-by: Nicolas Ferre <nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>

>  	i2c@0 {
>  		compatible = "i2c-gpio";
>  		gpios = <&pioA 23 GPIO_ACTIVE_HIGH /* sda */
> 


-- 
Nicolas Ferre
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html