[RFC PATCH v1] irqchip: add support for SMP irq router

[RFC PATCH v1] irqchip: add support for SMP irq router

[Sebastian Frias]

This adds support for a second-gen irq router/controller present
on some Sigma Designs chips.

Signed-off-by: Sebastian Frias <sf84@laposte.net>
---

This is a RFC because I have a few doubts:
1) I had to unroll irq_of_parse_and_map() in order to get the HW
IRQ declared in the device tree so that I can associate it with
a given domain.

2) I'm not sure about the DT specification, in particular, the use
of child nodes to declare different domains, but it works

3) I'm calling this an irq router to somehow highlight the fact
that it is not a simple interrupt controller. Indeed it does
not latch the IRQ lines by itself, and does not supports edge
detection.

4) Do I have to do something more to handle the affinity stuff?

5) checkpatch.pl reports warnings, etc. but I guess it's ok for
now since it is a RFC :-)

Please feel free to comment and suggest improvements.
---
 .../sigma,smp87xx-irqrouter.txt                    |  69 +++
 drivers/irqchip/Makefile                           |   1 +
 drivers/irqchip/irq-tango_v2.c                     | 594 +++++++++++++++++++++
 3 files changed, 664 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/sigma,smp87xx-irqrouter.txt
 create mode 100644 drivers/irqchip/irq-tango_v2.c

diff --git a/Documentation/devicetree/bindings/interrupt-controller/sigma,smp87xx-irqrouter.txt b/Documentation/devicetree/bindings/interrupt-controller/sigma,smp87xx-irqrouter.txt
new file mode 100644
index 0000000..0e404f0
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/sigma,smp87xx-irqrouter.txt
@@ -0,0 +1,69 @@
+* Sigma Designs Interrupt Router
+
+This module can route N IRQ inputs into M IRQ outputs, with N>M.
+For instance N=128, M=24.
+
+Note however that the HW does not latches the IRQ lines, so devices
+connecting to the router are expected to latch their IRQ line by themselves.
+
+A single node in the device tree is used to describe the interrupt router.
+Child nodes (up to a maximum of 'outputs') describe irqdomains for the outputs
+of the interrupt router.
+These child nodes specify, via their 'interrupts' property, how the
+interrupt router is connected to its parent interrupt controller (usually the
+GIC), and define irqdomains that can be used in other nodes' 'interrupts'
+property.
+
+Required properties:
+- compatible: Should be "sigma,smp87xx-irqrouter".
+- interrupt-controller: Identifies the node as an interrupt controller.
+- inputs: The number of IRQ lines entering the router
+- outputs: The number of IRQ lines exiting the router
+- reg: Base address and size of interrupt router registers.
+- #interrupt-cells: Should be <2>. Defines how other nodes will be able to
+interact with this node. The meaning of the cells are
+	* First Cell: HW IRQ number.
+	* Second Cell: IRQ polarity (level high or low).
+
+Required properties of child nodes:
+- interrupt-controller: Identifies the node as an interrupt controller.
+- interrupts: Defines the hwirq associated with a domain and connected to
+the parent interrupt controller. The format of the interrupt specifier
+depends on the interrupt parent controller.
+
+Optional properties:
+- interrupt-parent: pHandle of the parent interrupt controller, if not
+  inherited from the parent node.
+
+
+Example:
+
+See Documentation/devicetree/bindings/interrupt-controller/interrupts.txt and
+Documentation/devicetree/bindings/arm/gic.txt for further details.
+
+The following example declares a irqrouter with 128 inputs and 24 outputs,
+with registers @ 0x6F800 and connected to the GIC.
+The two child nodes define two irqdomains, one connected to GIC input 2
+(hwirq=2, level=high), and ther other connected to GIC input 3 (hwirq=3,
+level=low)
+
+		irq_router: irq_router@6f800 {
+			 compatible = "sigma,smp87xx-irqrouter";
+			 reg = <0x6f800 0x800>;
+			 interrupt-controller;
+			 interrupt-parent = <&gic>;
+			 inputs = <128>;
+			 outputs = <24>;
+
+			 irq0: irqdomain0@parentirq2 {
+			       interrupt-controller;
+			       #interrupt-cells = <2>;
+			       interrupts = <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>;
+			 };
+
+			 irq1: irqdomain1@parentirq3 {
+			       interrupt-controller;
+			       #interrupt-cells = <2>;
+			       interrupts = <GIC_SPI 3 IRQ_TYPE_LEVEL_LOW>;
+			 };
+		};
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index 7451245..703a2b5 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -39,6 +39,7 @@ obj-$(CONFIG_ARCH_NSPIRE)		+= irq-zevio.o
 obj-$(CONFIG_ARCH_VT8500)		+= irq-vt8500.o
 obj-$(CONFIG_ST_IRQCHIP)		+= irq-st.o
 obj-$(CONFIG_TANGO_IRQ)			+= irq-tango.o
+obj-$(CONFIG_TANGO_IRQ)			+= irq-tango_v2.o
 obj-$(CONFIG_TB10X_IRQC)		+= irq-tb10x.o
 obj-$(CONFIG_XTENSA)			+= irq-xtensa-pic.o
 obj-$(CONFIG_XTENSA_MX)			+= irq-xtensa-mx.o
diff --git a/drivers/irqchip/irq-tango_v2.c b/drivers/irqchip/irq-tango_v2.c
new file mode 100644
index 0000000..f6cf747
--- /dev/null
+++ b/drivers/irqchip/irq-tango_v2.c
@@ -0,0 +1,594 @@
+/*
+ * Copyright (C) 2014 Sebastian Frias <sf84@laposte.net>
+ *
+ * This program is free software; you can redistribute  it and/or modify it
+ * under  the terms of  the GNU General  Public License as published by the
+ * Free Software Foundation;  either version 2 of the  License, or (at your
+ * option) any later version.
+ */
+
+#include <linux/init.h>
+#include <linux/irq.h>
+#include <linux/irqchip.h>
+#include <linux/irqchip/chained_irq.h>
+#include <linux/ioport.h>
+#include <linux/io.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/slab.h>
+
+
+#define DBGERR(__format, ...)						\
+	do {								\
+		pr_err("[%s:%d] %s(): " __format, __FILE__, __LINE__, __FUNCTION__ , ##__VA_ARGS__); \
+	} while (0)
+
+#if 0
+#define DBGLOG(__format, ...)						\
+	do {								\
+		pr_info("[%s:%d] %s(): " __format, __FILE__, __LINE__, __FUNCTION__ , ##__VA_ARGS__); \
+	} while (0)
+#else
+#define DBGLOG(__format, ...) do {} while(0)
+#endif
+
+
+/*
+  HW description: IRQ router
+
+  IMPORTANT NOTE: this hw block is not a "full" interrupt controller
+  - it does not support edge detection
+  - it does not latch the inputs (devices are expected to latch their
+  IRQ output by themselves)
+
+  ---
+
+  CPU block interrupt interface is now 32bits.
+  The 24 first interrupt bits are generated from the system interrupts and the 8 msb interrupts are cpu local interrupts :
+
+  IRQs [23:0] tango system irqs.
+  IRQs [27:24] CPU core cross trigger interface interrupt (1 per core).
+  IRQs [31:28] CPU core PMU (performance unit) interrupt (1 per core).
+
+  The 24 lsb interrupts are generated through a new interrupt map module that maps the tango 128 interrupts to those 24 interrupts.
+  For each of the 128 input system interrupt, one register is dedicated to program the destination interrupt among the 24 available.
+  The mapper is configured as follows, starting at address (0x6f800) :
+
+  offset name            description
+  0x000  irq_in_0_cfg    "en"=bit[31]; "inv"=bit[16]; "dest"=bits[4:0]
+  0x004  irq_in_1_cfg    "en"=bit[31]; "inv"=bit[16]; "dest"=bits[4:0]
+  .
+  .
+  .
+  0x1FC  irq_in_127_cfg  "en"=bit[31]; "inv"=bit[16]; "dest"=bits[4:0]
+  0x400  soft_irq_cfg    "enable"=bits[15:0]
+  0x404  soft_irq_map0   "map3"=bits[28:24]; "map2"=bits[20:16]; "map1"=bits[12:8]; "map0"=bits[4:0]
+  0x408  soft_irq_map1   "map3"=bits[28:24]; "map2"=bits[20:16]; "map1"=bits[12:8]; "map0"=bits[4:0]
+  0x40C  soft_irq_map2   "map3"=bits[28:24]; "map2"=bits[20:16]; "map1"=bits[12:8]; "map0"=bits[4:0]
+  0x410  soft_irq_map3   "map3"=bits[28:24]; "map2"=bits[20:16]; "map1"=bits[12:8]; "map0"=bits[4:0]
+  0x414  soft_irq_set    "set"=bits[15:0]
+  0x418  soft_irq_clear  "clear"=bits[15:0]
+  0x41C  read_cpu_irq    "cpu_block_irq"=bits[23:0]
+  0x420  read_sys_irq0   "system_irq"=bits[31:0]; (irqs: 0->31)
+  0x424  read_sys_irq1   "system_irq"=bits[31:0]; (irqs: 32->63)
+  0x428  read_sys_irq2   "system_irq"=bits[31:0]; (irqs: 64->95)
+  0x42C  read_sys_irq3   "system_irq"=bits[31:0]; (irqs: 96->127)
+
+  irq_in_N_cfg   : input N mapping :
+  - dest bits[4:0]    => set destination interrupt among the 24 output interrupts. (if multiple inputs are mapped to the same output, result is an OR of the inputs).
+  - inv bit[16]       => if set, inverts input interrupt polarity (active at 0).
+  - en bit[31]        => enable interrupt. Acts like a mask on the input interrupt.
+  soft_irq       : this module supports up to 16 software interrupts.
+  - enable bits[15:0] => enable usage of software IRQs (SIRQ), 1 bit per SIRQ.
+  soft_irq_mapN  : For each of the 16 soft IRQ (SIRQ), map them in out IRQ[23:0] vector.
+  - mapN              => 5 bits to select where to connect the SIRQ among the 23 bits output IRQ. (if multiple SIRQ are mapped to the same output IRQ, result is an OR of those signals).
+  soft_irq_set   : 16bits, write 1 bit at one set the corresponding SIRQ. Read returns the software SIRQ vector value.
+  soft_irq_clear : 16bits, write 1 bit at one clear the corresponding software SIRQ. Read returns the software SIRQ vector value.
+  read_cpu_irq   : 24bits, returns output IRQ value (IRQs connected to the ARM cluster).
+  read_sys_irqN  : 32bits, returns input system IRQ value before mapping.
+*/
+
+#define ROUTER_INPUTS  (128)
+#define ROUTER_OUTPUTS (24)
+
+#define IRQ_ROUTER_ENABLE_MASK (BIT(31))
+#define IRQ_ROUTER_INVERT_MASK (BIT(16))
+
+#define READ_SYS_IRQ_GROUP0 (0x420)
+#define READ_SYS_IRQ_GROUP1 (0x424)
+#define READ_SYS_IRQ_GROUP2 (0x428)
+#define READ_SYS_IRQ_GROUP3 (0x42C)
+
+
+#if 0
+#define SHORT_OR_FULL_NAME full_name
+#else
+#define SHORT_OR_FULL_NAME name
+#endif
+
+#define NODE_NAME(__node__) (__node__ ? __node__->SHORT_OR_FULL_NAME : "<no-node>")
+
+#define BITMASK_VECTOR_SIZE(__count__) (__count__ / 32)
+#define IRQ_TO_OFFSET(__hwirq__) (__hwirq__ * 4)
+
+struct tango_irqrouter;
+
+/*
+  maintains the mapping between a Linux virq and a hwirq
+  on the parent controller.
+  It is used by tango_irqdomain_map() to setup the route
+  between input IRQ and output IRQ
+*/
+struct tango_irqrouter_output {
+	struct tango_irqrouter *context;
+	u32 hwirq;
+	u32 hwirq_level;
+	u32 virq;
+};
+
+/*
+  context for the driver
+*/
+struct tango_irqrouter {
+	raw_spinlock_t lock;
+	struct device_node *node;
+	void __iomem *base;
+	u32 input_count;
+	u32 output_count;
+	u32 irq_mask[BITMASK_VECTOR_SIZE(ROUTER_INPUTS)];
+	u32 irq_invert_mask[BITMASK_VECTOR_SIZE(ROUTER_INPUTS)];
+	struct tango_irqrouter_output output[ROUTER_OUTPUTS];
+};
+
+
+static inline u32 tango_readl(struct tango_irqrouter *irqrouter, int reg)
+{
+	u32 val = readl_relaxed(irqrouter->base + reg);
+	//DBGLOG("r[0x%08x + 0x%08x = 0x%08x] = 0x%08x\n", irqrouter->base, reg, irqrouter->base + reg, val);
+	return val;
+}
+
+static inline void tango_writel(struct tango_irqrouter *irqrouter, int reg, u32 val)
+{
+	//DBGLOG("w[0x%08x + 0x%08x = 0x%08x] = 0x%08x\n", irqrouter->base, reg, irqrouter->base + reg, val);
+	writel_relaxed(val, irqrouter->base + reg);
+}
+
+static inline void tango_setup_irq_route(struct tango_irqrouter *irqrouter, int irq_in, int irq_out)
+{
+	u32 offset = IRQ_TO_OFFSET(irq_in);
+	u32 value = irq_out;
+
+	DBGLOG("route hwirq(in) %d => hwirq(out) %d\n", irq_in, value);
+
+	if (value)
+		value &= ~(IRQ_ROUTER_ENABLE_MASK | IRQ_ROUTER_INVERT_MASK);
+
+	tango_writel(irqrouter, offset, value);
+}
+
+
+static inline void tango_setup_irq_inversion(struct tango_irqrouter *irqrouter, int hwirq, bool invert)
+{
+	u32 offset = IRQ_TO_OFFSET(hwirq);
+	u32 value = tango_readl(irqrouter, offset);
+	u32 hwirq_reg_index = hwirq / 32;
+	u32 hwirq_bit_index = hwirq % 32;
+
+	if (invert) {
+		irqrouter->irq_invert_mask[hwirq_reg_index] |= (1 << hwirq_bit_index);
+		value |= IRQ_ROUTER_INVERT_MASK;
+	}
+	else {
+		irqrouter->irq_invert_mask[hwirq_reg_index] &= ~(1 << hwirq_bit_index);
+		value &= ~(IRQ_ROUTER_INVERT_MASK);
+	}
+
+	DBGLOG("hwirq(in) %d %s inverted\n", hwirq, invert ? "":"not");
+
+	tango_writel(irqrouter, offset, value);
+}
+
+static void tango_irqchip_mask_irq(struct irq_data *data)
+{
+	struct irq_domain *domain = irq_data_get_irq_chip_data(data);
+	struct tango_irqrouter_output *irqrouter_output = domain->host_data;
+	struct tango_irqrouter *irqrouter = irqrouter_output->context;
+	u32 hwirq = data->hwirq;
+	u32 offset = IRQ_TO_OFFSET(hwirq);
+	u32 value = tango_readl(irqrouter, offset);
+	u32 hwirq_reg_index = hwirq / 32;
+	u32 hwirq_bit_index = hwirq % 32;
+
+	DBGLOG("%s: mask hwirq(in) %d : current regvalue 0x%x (routed to hwirq(out) %d)\n",
+	       NODE_NAME(irq_domain_get_of_node(domain)),
+	       hwirq, value, irqrouter_output->hwirq);
+
+	//mask cache
+	irqrouter->irq_mask[hwirq_reg_index] &= ~(1 << hwirq_bit_index);
+
+	value &= ~(IRQ_ROUTER_ENABLE_MASK);
+	tango_writel(irqrouter, offset, value);
+}
+
+static void tango_irqchip_unmask_irq(struct irq_data *data)
+{
+	struct irq_domain *domain = irq_data_get_irq_chip_data(data);
+	struct tango_irqrouter_output *irqrouter_output = domain->host_data;
+	struct tango_irqrouter *irqrouter = irqrouter_output->context;
+	u32 hwirq = data->hwirq;
+	u32 offset = IRQ_TO_OFFSET(hwirq);
+	u32 value = tango_readl(irqrouter, offset);
+	u32 hwirq_reg_index = hwirq / 32;
+	u32 hwirq_bit_index = hwirq % 32;
+
+	DBGLOG("%s: unmask hwirq(in) %d : current regvalue 0x%x (routed to hwirq(out) %d)\n",
+	       NODE_NAME(irq_domain_get_of_node(domain)),
+	       hwirq, value, irqrouter_output->hwirq);
+
+	//unmask cache
+	irqrouter->irq_mask[hwirq_reg_index] |= (1 << hwirq_bit_index);
+
+	value |= IRQ_ROUTER_ENABLE_MASK;
+	tango_writel(irqrouter, offset, value);
+}
+
+static int tango_irqchip_set_irq_type(struct irq_data *data, unsigned int type)
+{
+	struct irq_domain *domain = irq_data_get_irq_chip_data(data);
+	struct tango_irqrouter_output *irqrouter_output = domain->host_data;
+	struct tango_irqrouter *irqrouter = irqrouter_output->context;
+	unsigned int hwirq = data->hwirq;
+	unsigned int parent_type = (irqrouter_output->hwirq_level & IRQ_TYPE_SENSE_MASK);
+
+	DBGLOG("%s: type 0x%x for hwirq(in) %d = virq %d (routed to hwirq(out) %d)\n",
+	       NODE_NAME(irq_domain_get_of_node(domain)),
+	       type, hwirq, data->irq,
+	       irqrouter_output->hwirq);
+
+	if (parent_type & (type & IRQ_TYPE_SENSE_MASK))
+		//same polarity
+		tango_setup_irq_inversion(irqrouter, hwirq, 0);
+	else
+		//invert polarity
+		tango_setup_irq_inversion(irqrouter, hwirq, 1);
+
+	switch (type & IRQ_TYPE_SENSE_MASK) {
+	case IRQ_TYPE_EDGE_RISING:
+	case IRQ_TYPE_EDGE_FALLING:
+		panic("%s: does not support edge triggers\n",
+		      NODE_NAME(irq_domain_get_of_node(domain)));
+		break;
+	case IRQ_TYPE_LEVEL_HIGH:
+		break;
+	case IRQ_TYPE_LEVEL_LOW:
+		break;
+	default:
+		pr_err("%s: invalid trigger mode 0x%x for hwirq %d = virq %d\n",
+		       NODE_NAME(irq_domain_get_of_node(domain)),
+		       type, hwirq, data->irq);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+
+#ifdef CONFIG_SMP
+static int tango_irqchip_set_irq_affinity(struct irq_data *data,
+					  const struct cpumask *mask_val,
+					  bool force)
+{
+	struct irq_domain *domain = irq_data_get_irq_chip_data(data);
+	struct tango_irqrouter_output *irqrouter_output = domain->host_data;
+	struct irq_chip *parent_chip = irq_get_chip(irqrouter_output->virq);
+	struct irq_data *parent_data = irq_get_irq_data(irqrouter_output->virq);
+
+	DBGLOG("%s:\n", NODE_NAME(irq_domain_get_of_node(domain)));
+
+	if (parent_chip && parent_chip->irq_set_affinity)
+		return parent_chip->irq_set_affinity(parent_data, mask_val, force);
+	else
+		return -EINVAL;
+}
+#endif
+
+static struct irq_chip tango_irq_chip_ops = {
+	.name			= "ROUTER",
+	.irq_mask		= tango_irqchip_mask_irq,
+	.irq_unmask		= tango_irqchip_unmask_irq,
+	.irq_set_type		= tango_irqchip_set_irq_type,
+#ifdef CONFIG_SMP
+	.irq_set_affinity	= tango_irqchip_set_irq_affinity,
+#endif
+};
+
+
+/**
+ * @hwirq: HW IRQ of the device requesting an IRQ
+ * @virq: Linux IRQ (associated to the domain) to be given to the device
+ * @domain: IRQ domain (from the domain, we get the irqrouter_output
+ * in order to know to which output we need to route hwirq to)
+ */
+static int tango_irqdomain_map(struct irq_domain *domain,
+			       unsigned int virq,
+			       irq_hw_number_t hwirq)
+{
+	struct tango_irqrouter_output *irqrouter_output = domain->host_data;
+	struct tango_irqrouter *irqrouter = irqrouter_output->context;
+
+	DBGLOG("%s: hwirq(in) %d := virq %d, and route hwirq(in) %d => hwirq(out) %d (virq %d)\n",
+	       NODE_NAME(irq_domain_get_of_node(domain)),
+	       (u32)hwirq, virq,
+	       (u32)hwirq, irqrouter_output->hwirq, irqrouter_output->virq);
+
+	if (hwirq >= irqrouter->input_count)
+		panic("%s: invalid hwirq(in) %d >= %d\n",
+		      NODE_NAME(irq_domain_get_of_node(domain)),
+		      (u32)hwirq,
+		      irqrouter->input_count);
+
+	irq_set_chip_and_handler(virq, &tango_irq_chip_ops, handle_level_irq);
+	irq_set_chip_data(virq, domain);
+	irq_set_probe(virq);
+
+	tango_setup_irq_route(irqrouter, hwirq, irqrouter_output->hwirq);
+
+	return 0;
+}
+
+static int tango_irqdomain_xlate(struct irq_domain *domain,
+				 struct device_node *controller,
+				 const u32 *intspec,
+				 unsigned int intsize,
+				 unsigned long *out_hwirq,
+				 unsigned int *out_type)
+
+{
+	DBGLOG("%s: ctrl 0x%p, intspec 0x%x, intsize %d\n",
+	       NODE_NAME(irq_domain_get_of_node(domain)),
+	       controller, (u32)intspec, intsize);
+
+	if (irq_domain_get_of_node(domain) != controller)
+		return -EINVAL;
+
+	if (intsize < 2)
+		return -EINVAL;
+
+	DBGLOG("[0] 0x%x [1] 0x%x\n", intspec[0], intspec[1]);
+
+	*out_hwirq = intspec[0];
+	*out_type  = intspec[1] & IRQ_TYPE_SENSE_MASK;
+
+	DBGLOG("hwirq %d type 0x%x\n", (u32)*out_hwirq, (u32)*out_type);
+
+	return 0;
+}
+
+static u32 tango_dispatch_irqs(struct irq_domain *domain,
+			       struct irq_desc *desc,
+			       u32 status,
+			       int base)
+{
+	u32 hwirq;
+	u32 virq;
+
+	while (status) {
+		hwirq = __ffs(status);
+		virq = irq_find_mapping(domain, base + hwirq);
+		if (unlikely(!virq))
+			handle_bad_irq(desc);
+		else
+			generic_handle_irq(virq);
+
+		status &= ~BIT(hwirq);
+	}
+
+	return status;
+}
+
+
+static void tango_irqdomain_handle_cascade_irq(struct irq_desc *desc)
+{
+	struct irq_domain *domain = irq_desc_get_handler_data(desc);
+	struct tango_irqrouter_output *irqrouter_output = domain->host_data;
+	struct tango_irqrouter *irqrouter = irqrouter_output->context;
+	struct irq_chip *host_chip = irq_desc_get_chip(desc);
+	u32 status, status_0, status_1, status_2, status_3;
+
+	DBGLOG("%s: irqrouter_output 0x%p, hwirq(out) %d\n",
+	       NODE_NAME(irq_domain_get_of_node(domain)),
+	       irqrouter_output, irqrouter_output->hwirq);
+
+	chained_irq_enter(host_chip, desc);
+
+	raw_spin_lock(&(irqrouter->lock));
+	status_0 = tango_readl(irqrouter, READ_SYS_IRQ_GROUP0); //irqs 0 ->31
+	status_1 = tango_readl(irqrouter, READ_SYS_IRQ_GROUP1); //irqs 32->63
+	status_2 = tango_readl(irqrouter, READ_SYS_IRQ_GROUP2); //irqs 64->95
+	status_3 = tango_readl(irqrouter, READ_SYS_IRQ_GROUP3); //irqs 96->127
+	raw_spin_unlock(&(irqrouter->lock));
+
+	DBGLOG("0: 0x%08x (en 0x%08x, inv 0x%08x), 1: 0x%08x (en 0x%08x, inv 0x%08x), "
+	       "2: 0x%08x (en 0x%08x, inv 0x%08x), 3: 0x%08x (en 0x%08x, inv 0x%08x)\n",
+	       status_0, irqrouter->irq_mask[0], irqrouter->irq_invert_mask[0],
+	       status_1, irqrouter->irq_mask[1], irqrouter->irq_invert_mask[1],
+	       status_2, irqrouter->irq_mask[2], irqrouter->irq_invert_mask[2],
+	       status_3, irqrouter->irq_mask[3], irqrouter->irq_invert_mask[3]);
+
+#define HANDLE_INVERTED_LINES(__irqstatus__, __x__) ((((~__irqstatus__) & irqrouter->irq_invert_mask[__x__]) & irqrouter->irq_mask[__x__]) | __irqstatus__)
+#define HANDLE_ENABLE_AND_INVERSION_MASKS(__irqstatus__, __y__) (HANDLE_INVERTED_LINES(__irqstatus__, __y__) & irqrouter->irq_mask[__y__])
+
+	//handle inverted irq lines
+	status_0 = HANDLE_ENABLE_AND_INVERSION_MASKS(status_0, 0);
+	status_1 = HANDLE_ENABLE_AND_INVERSION_MASKS(status_1, 1);
+	status_2 = HANDLE_ENABLE_AND_INVERSION_MASKS(status_2, 2);
+	status_3 = HANDLE_ENABLE_AND_INVERSION_MASKS(status_3, 3);
+
+	status = tango_dispatch_irqs(domain, desc, status_0, 0);
+	if (status & status_0)
+		DBGERR("%s: unhandled IRQs (as a mask) 0x%x\n",
+		       NODE_NAME(irq_domain_get_of_node(domain)),
+		       status & status_0);
+	status = tango_dispatch_irqs(domain, desc, status_1, 32);
+	if (status & status_1)
+		DBGERR("%s: unhandled IRQs (as a mask) 0x%x\n",
+		       NODE_NAME(irq_domain_get_of_node(domain)),
+		       status & status_1);
+	status = tango_dispatch_irqs(domain, desc, status_2, 64);
+	if (status & status_2)
+		DBGERR("%s: unhandled IRQs (as a mask) 0x%x\n",
+		       NODE_NAME(irq_domain_get_of_node(domain)),
+		       status & status_2);
+	status = tango_dispatch_irqs(domain, desc, status_3, 96);
+	if (status & status_3)
+		DBGERR("%s: unhandled IRQs (as a mask) 0x%x\n",
+		       NODE_NAME(irq_domain_get_of_node(domain)),
+		       status & status_3);
+
+	chained_irq_exit(host_chip, desc);
+}
+
+static void of_phandle_args_to_fwspec(struct of_phandle_args *irq_data,
+				      struct irq_fwspec *fwspec)
+{
+	int i;
+
+	fwspec->fwnode = irq_data->np ? &irq_data->np->fwnode : NULL;
+	fwspec->param_count = irq_data->args_count;
+
+	for (i = 0; i < irq_data->args_count; i++)
+		fwspec->param[i] = irq_data->args[i];
+}
+
+struct irq_domain_ops tango_irqdomain_ops = {
+	.xlate	= tango_irqdomain_xlate,
+	.map	= tango_irqdomain_map,
+};
+
+static int __init tango_irq_init_domain(struct tango_irqrouter *irqrouter,
+					u32 index,
+					struct device_node *node)
+{
+	struct irq_domain *domain;
+	struct of_phandle_args of_irq;
+	struct irq_fwspec fwspec_irq;
+	u32 virq, hwirq, hwirq_level, err;
+
+	if (index >= irqrouter->output_count)
+		panic("%s: too many output IRQs\n", node->name);
+
+	//parse a node and associate its hwirq to a virq
+/*
+  unroll irq_of_parse_and_map() begin
+*/
+	err = of_irq_parse_one(node, 0, &of_irq);
+	if (err)
+		panic("%s: failed to get IRQ (%d)", node->name, err);
+
+	of_phandle_args_to_fwspec(&of_irq, &fwspec_irq);
+
+	hwirq       = fwspec_irq.param[1];
+	hwirq_level = fwspec_irq.param[2] & IRQ_TYPE_SENSE_MASK;
+
+	if (hwirq >= irqrouter->output_count)
+		panic("%s: invalid hwirq(out) %d >= %d\n", node->name,
+		      hwirq,
+		      irqrouter->output_count);
+
+	//request a virq for the hwirq
+	virq = irq_create_fwspec_mapping(&fwspec_irq);
+	if (!virq)
+		panic("%s: failed to get virq for hwirq(out) %d", node->name, hwirq);
+/*
+  unroll irq_of_parse_and_map() end
+*/
+
+	irqrouter->output[index].context     = irqrouter;
+	irqrouter->output[index].hwirq       = hwirq;
+	irqrouter->output[index].hwirq_level = hwirq_level;
+	irqrouter->output[index].virq        = virq;
+
+	//create a domain for this virq
+	domain = irq_domain_add_linear(node,
+				       irqrouter->input_count,
+				       &tango_irqdomain_ops,
+				       &(irqrouter->output[index]));
+	if (!domain)
+		panic("%s: failed to create irqdomain", node->name);
+
+	DBGLOG("%s: [%d] domain 0x%p for irqrouter_output 0x%p : "
+	       "hwirq(out) %d = virq %d\n",
+	       node->full_name, index, domain,
+	       &(irqrouter->output[index]),
+	       hwirq, virq);
+
+	//associate the domain with the virq
+	irq_set_chained_handler_and_data(virq,
+					 tango_irqdomain_handle_cascade_irq,
+					 domain);
+
+	return 0;
+}
+
+static int __init tango_of_irq_init(struct device_node *node,
+				    struct device_node *parent)
+{
+	struct tango_irqrouter *irqrouter;
+	struct device_node *child;
+	void __iomem *base;
+	u32 input_count, output_count, i;
+
+	base = of_iomap(node, 0);
+	if (!base) {
+		DBGERR("%s: failed to map combiner registers\n", node->name);
+		return -ENXIO;
+	}
+
+	of_property_read_u32(node, "inputs", &input_count);
+	if (!input_count) {
+		DBGERR("%s: missing 'inputs' property\n", node->name);
+		return -EINVAL;
+	}
+
+	of_property_read_u32(node, "outputs", &output_count);
+	if (!output_count) {
+		DBGERR("%s: missing 'outputs' property\n", node->name);
+		return -EINVAL;
+	}
+
+	if ((input_count != ROUTER_INPUTS)
+	    || (output_count != ROUTER_OUTPUTS)) {
+		DBGERR("%s: input/output count mismatch", node->name);
+		return -EINVAL;
+	}
+
+	irqrouter = kzalloc(sizeof(*irqrouter), GFP_KERNEL);
+	raw_spin_lock_init(&(irqrouter->lock));
+	irqrouter->node = node;
+	irqrouter->base = base;
+	irqrouter->input_count = input_count;
+	irqrouter->output_count = output_count;
+	pr_info("%s: base 0x%p, %d => %d router, parent %s\n",
+	       node->full_name, base,
+	       input_count, output_count,
+	       parent->full_name);
+
+	i = 0;
+	for_each_child_of_node(node, child) {
+		tango_irq_init_domain(irqrouter, i, child);
+		i++;
+	}
+
+	/*
+	  clear backward compatible map used by previous generation
+	  irq controller ("sigma,smp8642-intc")
+	*/
+	tango_setup_irq_route(irqrouter, 125, 0);
+	tango_setup_irq_route(irqrouter, 126, 0);
+	tango_setup_irq_route(irqrouter, 127, 0);
+
+	return 0;
+}
+
+IRQCHIP_DECLARE(tango_irqrouter, "sigma,smp87xx-irqrouter", tango_of_irq_init);
-- 
1.7.11.2

Re: [RFC PATCH v1] irqchip: add support for SMP irq router

[Mason]

On 30/06/2016 18:03, Sebastian Frias wrote:

> This adds support for a second-gen irq router/controller present
> on some Sigma Designs chips.

In the patch subject, do you mean SMP as in Symmetric Multi Processor?


> Signed-off-by: Sebastian Frias <sf84@laposte.net>

Is that the address you intend to submit with?


> This is a RFC because I have a few doubts:
> 1) I had to unroll irq_of_parse_and_map() in order to get the HW
> IRQ declared in the device tree so that I can associate it with
> a given domain.
> 
> 2) I'm not sure about the DT specification, in particular, the use
> of child nodes to declare different domains, but it works
> 
> 3) I'm calling this an irq router to somehow highlight the fact
> that it is not a simple interrupt controller. Indeed it does
> not latch the IRQ lines by itself, and does not supports edge
> detection.
> 
> 4) Do I have to do something more to handle the affinity stuff?
> 
> 5) checkpatch.pl reports warnings, etc. but I guess it's ok for
> now since it is a RFC :-)
> 
> Please feel free to comment and suggest improvements.

The "core" topic is over my head, so I'll just discuss the color
of the bike shed.

>  .../sigma,smp87xx-irqrouter.txt                    |  69 +++

In the *actual* submission, we can't use a wildcard like smp87xx
we'll have to use an actual part number.

>  drivers/irqchip/Makefile                           |   1 +
>  drivers/irqchip/irq-tango_v2.c                     | 594 +++++++++++++++++++++

Likewise, I don't like the "_v2" suffix, it's too generic.
Actual submission should use something more specific.

> +++ b/Documentation/devicetree/bindings/interrupt-controller/sigma,smp87xx-irqrouter.txt
> @@ -0,0 +1,69 @@
> +* Sigma Designs Interrupt Router
> +
> +This module can route N IRQ inputs into M IRQ outputs, with N>M.
> +For instance N=128, M=24.
> +
> +Note however that the HW does not latches the IRQ lines, so devices
                                     ^^^^^^^
"does not latch"

> +Required properties:
> +- compatible: Should be "sigma,smp87xx-irqrouter".

Same comment about wildcard.

> +- interrupt-controller: Identifies the node as an interrupt controller.
> +- inputs: The number of IRQ lines entering the router
> +- outputs: The number of IRQ lines exiting the router

As far as I can tell, if N > 256 then the register layout would
have to change. (Likewise, if M > 32)

Also, you hard-code the fact that N/32 = 4 with the status_i variables.

Would it make sense to use for loops?
(instead of unrolling the loops)

  e.g. for (i = 0; i < N/4; ++i) { ... }


> +Optional properties:
> +- interrupt-parent: pHandle of the parent interrupt controller, if not
> +  inherited from the parent node.

I'm not sure this is what "optional" means.


> +The following example declares a irqrouter with 128 inputs and 24 outputs,
> +with registers @ 0x6F800 and connected to the GIC.
> +The two child nodes define two irqdomains, one connected to GIC input 2
> +(hwirq=2, level=high), and ther other connected to GIC input 3 (hwirq=3,
                              ^^^^

> +#define ROUTER_INPUTS  (128)
> +#define ROUTER_OUTPUTS (24)

Parentheses are unnecessary around constants.

> +#define IRQ_ROUTER_ENABLE_MASK (BIT(31))
> +#define IRQ_ROUTER_INVERT_MASK (BIT(16))

Parentheses already provided in BIT macro.

> +#define READ_SYS_IRQ_GROUP0 (0x420)
> +#define READ_SYS_IRQ_GROUP1 (0x424)
> +#define READ_SYS_IRQ_GROUP2 (0x428)
> +#define READ_SYS_IRQ_GROUP3 (0x42C)

If a for loop were used, we'd only need to
#define SYSTEM_IRQ	0x420

  for (i = 0; i < N/4; ++i) {
    status_i = readl(base + SYSTEM_IRQ + i*4);

> +#if 0
> +#define SHORT_OR_FULL_NAME full_name
> +#else
> +#define SHORT_OR_FULL_NAME name
> +#endif

Just pick one?

> +#define NODE_NAME(__node__) (__node__ ? __node__->SHORT_OR_FULL_NAME : "<no-node>")

Is it possible for a node to not have a name?

I also think prefixing/postfixing macro parameters with underscores
is positively fugly.

> +/*
> +  context for the driver
> +*/
> +struct tango_irqrouter {
> +	raw_spinlock_t lock;

Is this lock really needed?

Is tango_irqdomain_handle_cascade_irq() expected to be called
concurrently on multiple cores?

Even then, is it necessary to lock, in order to read 4 MMIO regs?

> +	struct device_node *node;
> +	void __iomem *base;
> +	u32 input_count;
> +	u32 output_count;
> +	u32 irq_mask[BITMASK_VECTOR_SIZE(ROUTER_INPUTS)];
> +	u32 irq_invert_mask[BITMASK_VECTOR_SIZE(ROUTER_INPUTS)];
> +	struct tango_irqrouter_output output[ROUTER_OUTPUTS];
> +};

Hmmm, if the driver were truly "parameterizable", I guess we should
dynamically allocate the arrays.

> +static void tango_irqchip_mask_irq(struct irq_data *data)
> +{
> +	struct irq_domain *domain = irq_data_get_irq_chip_data(data);
> +	struct tango_irqrouter_output *irqrouter_output = domain->host_data;
> +	struct tango_irqrouter *irqrouter = irqrouter_output->context;
> +	u32 hwirq = data->hwirq;
> +	u32 offset = IRQ_TO_OFFSET(hwirq);
> +	u32 value = tango_readl(irqrouter, offset);
> +	u32 hwirq_reg_index = hwirq / 32;
> +	u32 hwirq_bit_index = hwirq % 32;
> +
> +	DBGLOG("%s: mask hwirq(in) %d : current regvalue 0x%x (routed to hwirq(out) %d)\n",
> +	       NODE_NAME(irq_domain_get_of_node(domain)),
> +	       hwirq, value, irqrouter_output->hwirq);
> +
> +	//mask cache
> +	irqrouter->irq_mask[hwirq_reg_index] &= ~(1 << hwirq_bit_index);
> +
> +	value &= ~(IRQ_ROUTER_ENABLE_MASK);
> +	tango_writel(irqrouter, offset, value);
> +}
> +
> +static void tango_irqchip_unmask_irq(struct irq_data *data)
> +{
> +	struct irq_domain *domain = irq_data_get_irq_chip_data(data);
> +	struct tango_irqrouter_output *irqrouter_output = domain->host_data;
> +	struct tango_irqrouter *irqrouter = irqrouter_output->context;
> +	u32 hwirq = data->hwirq;
> +	u32 offset = IRQ_TO_OFFSET(hwirq);
> +	u32 value = tango_readl(irqrouter, offset);
> +	u32 hwirq_reg_index = hwirq / 32;
> +	u32 hwirq_bit_index = hwirq % 32;
> +
> +	DBGLOG("%s: unmask hwirq(in) %d : current regvalue 0x%x (routed to hwirq(out) %d)\n",
> +	       NODE_NAME(irq_domain_get_of_node(domain)),
> +	       hwirq, value, irqrouter_output->hwirq);
> +
> +	//unmask cache
> +	irqrouter->irq_mask[hwirq_reg_index] |= (1 << hwirq_bit_index);
> +
> +	value |= IRQ_ROUTER_ENABLE_MASK;
> +	tango_writel(irqrouter, offset, value);
> +}

There might be an opportunity to factorize the two functions,
and have mask/unmask call such "helper" with the proper args.


> +#define HANDLE_INVERTED_LINES(__irqstatus__, __x__) ((((~__irqstatus__) & irqrouter->irq_invert_mask[__x__]) & irqrouter->irq_mask[__x__]) | __irqstatus__)
> +#define HANDLE_ENABLE_AND_INVERSION_MASKS(__irqstatus__, __y__) (HANDLE_INVERTED_LINES(__irqstatus__, __y__) & irqrouter->irq_mask[__y__])

I'm pretty sure these macros make baby Jesus cry.


> +static int __init tango_of_irq_init(struct device_node *node,
> +				    struct device_node *parent)
> +{
> +	struct tango_irqrouter *irqrouter;
> +	struct device_node *child;
> +	void __iomem *base;
> +	u32 input_count, output_count, i;
> +
> +	base = of_iomap(node, 0);
> +	if (!base) {
> +		DBGERR("%s: failed to map combiner registers\n", node->name);
> +		return -ENXIO;
> +	}
> +
> +	of_property_read_u32(node, "inputs", &input_count);
> +	if (!input_count) {
> +		DBGERR("%s: missing 'inputs' property\n", node->name);
> +		return -EINVAL;
> +	}
> +
> +	of_property_read_u32(node, "outputs", &output_count);
> +	if (!output_count) {
> +		DBGERR("%s: missing 'outputs' property\n", node->name);
> +		return -EINVAL;
> +	}
> +
> +	if ((input_count != ROUTER_INPUTS)
> +	    || (output_count != ROUTER_OUTPUTS)) {
> +		DBGERR("%s: input/output count mismatch", node->name);
> +		return -EINVAL;
> +	}

So the driver is not intended to be parameterized?
(or perhaps only in a follow-up?)

Regards.

Re: [RFC PATCH v1] irqchip: add support for SMP irq router

[Sebastian Frias]

On 07/04/2016 02:11 PM, Mason wrote:
> 
> In the patch subject, do you mean SMP as in Symmetric Multi Processor?

As in Sigma Multimedia Processor :-)
The prefix for Sigma's chips is SMP.

I can change that to "Tango" if it is confusing.

> 
> Is that the address you intend to submit with?

Yes.

> 
> The "core" topic is over my head, so I'll just discuss the color
> of the bike shed.

:-)
Thanks for your comments, hopefully some knowledgeable people will discuss the core topic as well.

> 
>>  .../sigma,smp87xx-irqrouter.txt                    |  69 +++
> 
> In the *actual* submission, we can't use a wildcard like smp87xx
> we'll have to use an actual part number.

Are you sure?
That would hinder genericity.
Actually I wanted to call it "sigma,smp-irqrouter.txt" (or "sigma,smp,irqrouter.txt").

To me there's no need to link the compatible string of a given HW module with that of the chip name the module it is embedded into.
For example, the generic USB3 driver is "generic-xhci".
While this module is not generic to be embedded in chips from different manufacturers, it is supposed to be generic within Sigma, and multiple Sigma chips (with potentially different denominations) can use it.

> 
>>  drivers/irqchip/Makefile                           |   1 +
>>  drivers/irqchip/irq-tango_v2.c                     | 594 +++++++++++++++++++++
> 
> Likewise, I don't like the "_v2" suffix, it's too generic.
> Actual submission should use something more specific.

Well, the other driver is irq-tango.c that is generic as well.
I prefer versioning, as it is unrelated with the actual chip name.

Following the reasoning from my previous reply, I think the file name should be something like "smp-irqrouter.c" to be close to the "compatible" string.

However, IMHO there is a mix of various naming conventions on the kernel tree so I don't know which one is recommended.

> 
>> +++ b/Documentation/devicetree/bindings/interrupt-controller/sigma,smp87xx-irqrouter.txt
>> @@ -0,0 +1,69 @@
>> +* Sigma Designs Interrupt Router
>> +
>> +This module can route N IRQ inputs into M IRQ outputs, with N>M.
>> +For instance N=128, M=24.
>> +
>> +Note however that the HW does not latches the IRQ lines, so devices
>                                      ^^^^^^^
> "does not latch"

Ok.

> 
> Also, you hard-code the fact that N/32 = 4 with the status_i variables.
> 
> Would it make sense to use for loops?
> (instead of unrolling the loops)
> 
>   e.g. for (i = 0; i < N/4; ++i) { ... }
> 

Actually my current version uses this (also on "tango_irqdomain_handle_cascade_irq()"), but I'm not sure I'm happy with it because the person reading may think the code is generic while it is not.
I mean, there is no guarantee on how the HW guys will extend this in the future, and trying to make the code generic could end up as wasted premature optimization.

> 
>> +Optional properties:
>> +- interrupt-parent: pHandle of the parent interrupt controller, if not
>> +  inherited from the parent node.
> 
> I'm not sure this is what "optional" means.
> 

>From what I've seen in the documentation for other DT bindings, this is the meaning of "optional".
But I don't mind changing it if I get some hints about the meaning.

> 
>> +The following example declares a irqrouter with 128 inputs and 24 outputs,
>> +with registers @ 0x6F800 and connected to the GIC.
>> +The two child nodes define two irqdomains, one connected to GIC input 2
>> +(hwirq=2, level=high), and ther other connected to GIC input 3 (hwirq=3,
>                               ^^^^
> 
>> +#define ROUTER_INPUTS  (128)
>> +#define ROUTER_OUTPUTS (24)
> 
> Parentheses are unnecessary around constants.

I'm used to use parenthesis in macros to avoid operator priority side effects.

> 
>> +#define IRQ_ROUTER_ENABLE_MASK (BIT(31))
>> +#define IRQ_ROUTER_INVERT_MASK (BIT(16))
> 
> Parentheses already provided in BIT macro.

Same as above.

> 
>> +#define READ_SYS_IRQ_GROUP0 (0x420)
>> +#define READ_SYS_IRQ_GROUP1 (0x424)
>> +#define READ_SYS_IRQ_GROUP2 (0x428)
>> +#define READ_SYS_IRQ_GROUP3 (0x42C)
> 
> If a for loop were used, we'd only need to
> #define SYSTEM_IRQ	0x420
> 
>   for (i = 0; i < N/4; ++i) {
>     status_i = readl(base + SYSTEM_IRQ + i*4);
> 

I suppose you mean "status[i]".
There's a trade-off between explicit and clear code, and compact code.
I believe in this case the unrolled code not only is more clear, it may end up being faster.

As I was saying earlier, my current version uses a mixed approach with the loop unrolled for reading the registers and a for loop for the dispatch. 

If in the future we have to read much more registers, we should revisit this.

>> +#if 0
>> +#define SHORT_OR_FULL_NAME full_name
>> +#else
>> +#define SHORT_OR_FULL_NAME name
>> +#endif
> 
> Just pick one?

By making the choice explicit, the person reading/debugging is aware of the possibilities.
Picking "full_name" or "name" would likely prevent the person from knowing it has other debug options.

> 
>> +#define NODE_NAME(__node__) (__node__ ? __node__->SHORT_OR_FULL_NAME : "<no-node>")
> 
> Is it possible for a node to not have a name?

The code is about checking the 'node' is not NULL before dereferencing it;
Furthermore, printk is supposed to handle NULL pointers for %s ("be lenient with your input and strict with your output")

> 
> I also think prefixing/postfixing macro parameters with underscores
> is positively fugly.

>From my point of view, it makes a clear difference between macro parameters and rest of the code.

> 
>> +/*
>> +  context for the driver
>> +*/
>> +struct tango_irqrouter {
>> +	raw_spinlock_t lock;
> 
> Is this lock really needed?
> 
> Is tango_irqdomain_handle_cascade_irq() expected to be called
> concurrently on multiple cores?

Good question, I had asked myself the same.
include/linux/irq.h:irq_set_chained_handler_and_data() declaration has a comment saying that IRQ_NOTHREAD is used, but since I had seen a lock being used by other drivers, I thought to give it a try on this RFC.

> 
> Even then, is it necessary to lock, in order to read 4 MMIO regs?
> 
>> +	struct device_node *node;
>> +	void __iomem *base;
>> +	u32 input_count;
>> +	u32 output_count;
>> +	u32 irq_mask[BITMASK_VECTOR_SIZE(ROUTER_INPUTS)];
>> +	u32 irq_invert_mask[BITMASK_VECTOR_SIZE(ROUTER_INPUTS)];
>> +	struct tango_irqrouter_output output[ROUTER_OUTPUTS];
>> +};
> 
> Hmmm, if the driver were truly "parameterizable", I guess we should
> dynamically allocate the arrays.

The key is "if it were...", since it is likely it will never be generic, I thought that dynamically allocating the arrays was not worth it.

I agree that there's a sort of hybrid approach, part "static", part "dynamic", in the sense that some things could be dynamically allocated (like the fields above), or dynamically handled (like the "status" registers), yet constant (and thus "static") macros or unrolled loops are used to define them because the driver is not really generic.

I can bet that if I had dynamically allocated the arrays, somebody could have commented that it was not necessary since the driver is not fully generic :-)

>> +static void tango_irqchip_unmask_irq(struct irq_data *data)
>> +{
>> +	struct irq_domain *domain = irq_data_get_irq_chip_data(data);
>> +	struct tango_irqrouter_output *irqrouter_output = domain->host_data;
>> +	struct tango_irqrouter *irqrouter = irqrouter_output->context;
>> +	u32 hwirq = data->hwirq;
>> +	u32 offset = IRQ_TO_OFFSET(hwirq);
>> +	u32 value = tango_readl(irqrouter, offset);
>> +	u32 hwirq_reg_index = hwirq / 32;
>> +	u32 hwirq_bit_index = hwirq % 32;
>> +
>> +	DBGLOG("%s: unmask hwirq(in) %d : current regvalue 0x%x (routed to hwirq(out) %d)\n",
>> +	       NODE_NAME(irq_domain_get_of_node(domain)),
>> +	       hwirq, value, irqrouter_output->hwirq);
>> +
>> +	//unmask cache
>> +	irqrouter->irq_mask[hwirq_reg_index] |= (1 << hwirq_bit_index);
>> +
>> +	value |= IRQ_ROUTER_ENABLE_MASK;
>> +	tango_writel(irqrouter, offset, value);
>> +}
> 
> There might be an opportunity to factorize the two functions,
> and have mask/unmask call such "helper" with the proper args.
> 

You are right, these grew from just logging to actually doing something and did not get factored.

>> +
>> +	if ((input_count != ROUTER_INPUTS)
>> +	    || (output_count != ROUTER_OUTPUTS)) {
>> +		DBGERR("%s: input/output count mismatch", node->name);
>> +		return -EINVAL;
>> +	}
> 
> So the driver is not intended to be parameterized?
> (or perhaps only in a follow-up?)

It would be nice to have a fully generic driver, but I doubt that is possible.
Small changes would always be necessary due to the fact that the HW description itself (register distribution and their meaning) is not on the DT.
In the end it is a trade-off between code (and DT) clarity and simplicity, and fully generic code that would require lots of glue to handle a complex DT capable of describing the generic HW. In the current form, most of the code is actually to handle the IRQs, if it were generic I think a significant part of the code would be there just to deal with genericity, obscuring what the driver does.

So I went for a warning :-)

Best regards,

Sebastian

Re: [RFC PATCH v1] irqchip: add support for SMP irq router

[Jason Cooper]

Hey Sebastian, Mason,

* Please fix mailer to wrap text at a sane length.  I've re-wrapped and
trimmed.

On Tue, Jul 05, 2016 at 02:30:12PM +0200, Sebastian Frias wrote:
> On 07/04/2016 02:11 PM, Mason wrote:
...
> >>  .../sigma,smp87xx-irqrouter.txt                    |  69 +++
> > 
> > In the *actual* submission, we can't use a wildcard like smp87xx
> > we'll have to use an actual part number.
> 
> Are you sure?
> That would hinder genericity.
> Actually I wanted to call it "sigma,smp-irqrouter.txt" (or "sigma,smp,irqrouter.txt").

sigma,smp-irqrouter.txt should be fine.  The devicetree maintainers
should yelp if they want something different.

> To me there's no need to link the compatible string of a given HW
> module with that of the chip name the module it is embedded into.  For
> example, the generic USB3 driver is "generic-xhci".  While this module
> is not generic to be embedded in chips from different manufacturers,
> it is supposed to be generic within Sigma, and multiple Sigma chips
> (with potentially different denominations) can use it.
> 
> > 
> >>  drivers/irqchip/Makefile                           |   1 +
> >>  drivers/irqchip/irq-tango_v2.c                     | 594 +++++++++++++++++++++
> > 
> > Likewise, I don't like the "_v2" suffix, it's too generic.
> > Actual submission should use something more specific.
> 
> Well, the other driver is irq-tango.c that is generic as well.
> I prefer versioning, as it is unrelated with the actual chip name.

Is there a name, similar to 'tango', for this version of the IP?
Something that would spark recognition for someone looking for "the damn
driver for this XYZ irqchip I have".  If not, irq-tango_v2.c is fine.

thx,

Jason.

Re: [RFC PATCH v1] irqchip: add support for SMP irq router

[Mason]

Jason Cooper wrote:

> Sebastian Frias wrote:
>
>> Mason wrote:
>>
>>> Sebastian Frias wrote:
>>>
>>>>  .../sigma,smp87xx-irqrouter.txt                    |  69 +++
>>>
>>> In the *actual* submission, we can't use a wildcard like smp87xx
>>> we'll have to use an actual part number.
>>
>> Are you sure?
>> That would hinder genericity.
>> Actually I wanted to call it "sigma,smp-irqrouter.txt" (or "sigma,smp,irqrouter.txt").
> 
> sigma,smp-irqrouter.txt should be fine.  The devicetree maintainers
> should yelp if they want something different.

Personally, I don't like "smp" because it's too easy to confuse that
for "symmetric multi-processor".

Come to think of it, I'm not sure the *name* of the file documenting
a binding is as important to DT maintainers as the compatible string.


>> To me there's no need to link the compatible string of a given HW
>> module with that of the chip name the module it is embedded into.  For
>> example, the generic USB3 driver is "generic-xhci".  While this module
>> is not generic to be embedded in chips from different manufacturers,
>> it is supposed to be generic within Sigma, and multiple Sigma chips
>> (with potentially different denominations) can use it.
>>
>>>
>>>>  drivers/irqchip/Makefile                           |   1 +
>>>>  drivers/irqchip/irq-tango_v2.c                     | 594 +++++++++++++++++++++
>>>
>>> Likewise, I don't like the "_v2" suffix, it's too generic.
>>> Actual submission should use something more specific.
>>
>> Well, the other driver is irq-tango.c that is generic as well.
>> I prefer versioning, as it is unrelated with the actual chip name.
> 
> Is there a name, similar to 'tango', for this version of the IP?
> Something that would spark recognition for someone looking for "the damn
> driver for this XYZ irqchip I have".  If not, irq-tango_v2.c is fine.

If we go with the v2 naming scheme, I vote for irq-tango-v2.c for
consistency with the GIC drivers.

Regards.

Re: [RFC PATCH v1] irqchip: add support for SMP irq router

[Sebastian Frias]

Hi Jason,

On 07/05/2016 04:41 PM, Jason Cooper wrote:
> Hey Sebastian, Mason,
> 
> * Please fix mailer to wrap text at a sane length.  I've re-wrapped and
> trimmed.
> 
> On Tue, Jul 05, 2016 at 02:30:12PM +0200, Sebastian Frias wrote:
>> On 07/04/2016 02:11 PM, Mason wrote:
> ...
>>>>  .../sigma,smp87xx-irqrouter.txt                    |  69 +++
>>>
>>> In the *actual* submission, we can't use a wildcard like smp87xx
>>> we'll have to use an actual part number.
>>
>> Are you sure?
>> That would hinder genericity.
>> Actually I wanted to call it "sigma,smp-irqrouter.txt" (or "sigma,smp,irqrouter.txt").
> 
> sigma,smp-irqrouter.txt should be fine.  The devicetree maintainers
> should yelp if they want something different.
> 
>> To me there's no need to link the compatible string of a given HW
>> module with that of the chip name the module it is embedded into.  For
>> example, the generic USB3 driver is "generic-xhci".  While this module
>> is not generic to be embedded in chips from different manufacturers,
>> it is supposed to be generic within Sigma, and multiple Sigma chips
>> (with potentially different denominations) can use it.
>>
>>>
>>>>  drivers/irqchip/Makefile                           |   1 +
>>>>  drivers/irqchip/irq-tango_v2.c                     | 594 +++++++++++++++++++++
>>>
>>> Likewise, I don't like the "_v2" suffix, it's too generic.
>>> Actual submission should use something more specific.
>>
>> Well, the other driver is irq-tango.c that is generic as well.
>> I prefer versioning, as it is unrelated with the actual chip name.
> 
> Is there a name, similar to 'tango', for this version of the IP?
> Something that would spark recognition for someone looking for "the damn
> driver for this XYZ irqchip I have".  If not, irq-tango_v2.c is fine.
> 

Thanks for your comments.
So, aside from some naming issues, do you think the driver is ok?
Indeed, I'm not sure about the following:

1) I had to unroll irq_of_parse_and_map() in order to get the HW
IRQ declared in the device tree so that I can associate it with
a given domain. Is there another way?

2) I'm not sure about the DT specification, in particular, the use
of child nodes to declare different domains, but it works.
Advise and/or hints regarding this are welcome.

3) I'm calling this an irq router to somehow highlight the fact
that it is not a simple interrupt controller. Indeed it does
not latch the IRQ lines by itself, and does not supports edge
detection.
Does anybody else has a better idea for the name?

4) Do I have to do something more to handle the affinity stuff?

5) checkpatch.pl reports warnings, etc. but I guess it's ok for
now since it is a RFC 

6) Do we need a lock in the cascade_irq callback set with
irq_set_chained_handler_and_data() ?

Thanks in advance.
Best regards,

Sebastian

Re: [RFC PATCH v1] irqchip: add support for SMP irq router

[Jason Cooper]

Hey Sebastian,

On Tue, Jul 05, 2016 at 05:18:42PM +0200, Sebastian Frias wrote:
> On 07/05/2016 04:41 PM, Jason Cooper wrote:
> > On Tue, Jul 05, 2016 at 02:30:12PM +0200, Sebastian Frias wrote:
> >> On 07/04/2016 02:11 PM, Mason wrote:
> > ...
> >>>>  .../sigma,smp87xx-irqrouter.txt                    |  69 +++
> >>>
> >>> In the *actual* submission, we can't use a wildcard like smp87xx
> >>> we'll have to use an actual part number.
> >>
> >> Are you sure?
> >> That would hinder genericity.
> >> Actually I wanted to call it "sigma,smp-irqrouter.txt" (or "sigma,smp,irqrouter.txt").
> > 
> > sigma,smp-irqrouter.txt should be fine.  The devicetree maintainers
> > should yelp if they want something different.
> > 
> >> To me there's no need to link the compatible string of a given HW
> >> module with that of the chip name the module it is embedded into.  For
> >> example, the generic USB3 driver is "generic-xhci".  While this module
> >> is not generic to be embedded in chips from different manufacturers,
> >> it is supposed to be generic within Sigma, and multiple Sigma chips
> >> (with potentially different denominations) can use it.
> >>
> >>>
> >>>>  drivers/irqchip/Makefile                           |   1 +
> >>>>  drivers/irqchip/irq-tango_v2.c                     | 594 +++++++++++++++++++++
> >>>
> >>> Likewise, I don't like the "_v2" suffix, it's too generic.
> >>> Actual submission should use something more specific.
> >>
> >> Well, the other driver is irq-tango.c that is generic as well.
> >> I prefer versioning, as it is unrelated with the actual chip name.
> > 
> > Is there a name, similar to 'tango', for this version of the IP?
> > Something that would spark recognition for someone looking for "the damn
> > driver for this XYZ irqchip I have".  If not, irq-tango_v2.c is fine.
> > 
> 
> Thanks for your comments.
> So, aside from some naming issues, do you think the driver is ok?

Well, it's going to be few days before I can really dig in to this.
Until then, what I can say I see is that it looks like you're using
devicetree to tell Linux how to lay out the irq domains.  That's not
right :(

The devicetree should *only* describe the hardware.  Would *BSD be able
to use the description in the dtb effectively?

iiuc, I think irq-crossbar.c may be a similar enough in task to give you
an idea or two.

Beyond that, as you mention, there are a bunch of style issues,
unneeded macros, etc.

thx,

Jason.

Re: [RFC PATCH v1] irqchip: add support for SMP irq router

[Jason Cooper]

Hi Mason,

On Tue, Jul 05, 2016 at 05:07:09PM +0200, Mason wrote:
> Jason Cooper wrote:
> > Sebastian Frias wrote:
> >> Mason wrote:
> >>> Sebastian Frias wrote:
> >>>
> >>>>  .../sigma,smp87xx-irqrouter.txt                    |  69 +++
> >>>
> >>> In the *actual* submission, we can't use a wildcard like smp87xx
> >>> we'll have to use an actual part number.
> >>
> >> Are you sure?
> >> That would hinder genericity.
> >> Actually I wanted to call it "sigma,smp-irqrouter.txt" (or "sigma,smp,irqrouter.txt").
> > 
> > sigma,smp-irqrouter.txt should be fine.  The devicetree maintainers
> > should yelp if they want something different.
> 
> Personally, I don't like "smp" because it's too easy to confuse that
> for "symmetric multi-processor".

Respectfully, it's not about what any of us 'likes'.  If Sigma's
marketing people inadvertently chose a bad acronym, then it is what it
is.  Trying to paper over it just adds unnecessary layers of complexity
and confusion.  A simple expansion of the acronym in a comment block or
in the commit message is all that's needed to remove the ambiguity.

> Come to think of it, I'm not sure the *name* of the file documenting
> a binding is as important to DT maintainers as the compatible string.

Correct.  devicetee compatible strings need to be as specific as
possible.  In a series of compatible IP blocks, the string should refer
to the first version in the series, e.g. sigma,smp8710 for a series of
compatible IP blocks like 8710, 8712, 8715, 8724.  If an 8751 came along
with a different register layout or some other incompatibility, then a
new string would be sigma,smp8751.  So,

  8710 uses "sigma,smp8710"
  8712 uses "sigma,smp8710"
  8715 uses "sigma,smp8710"
  8724 uses "sigma,smp8710"
  8751 uses "sigma,smp8751"
  8754 uses "sigma,smp8751"


thx,

Jason.

Re: [RFC PATCH v1] irqchip: add support for SMP irq router

[Sebastian Frias]

Hi Jason,

On 07/05/2016 05:53 PM, Jason Cooper wrote:
>>
>> Thanks for your comments.
>> So, aside from some naming issues, do you think the driver is ok?
> 
> Well, it's going to be few days before I can really dig in to this.
> Until then, what I can say I see is that it looks like you're using
> devicetree to tell Linux how to lay out the irq domains.  That's not
> right :(

Ok, so that replies my questions 1 and 2, thanks.

> 
> The devicetree should *only* describe the hardware. Would *BSD be able
> to use the description in the dtb effectively?
> 
> iiuc, I think irq-crossbar.c may be a similar enough in task to give you
> an idea or two.

I already did something like that, you can see it here:

https://marc.info/?l=linux-kernel&m=146592235919308&w=2

the problem with that code is that it cannot handle more than 24 IRQs (the
number of outputs of the router), because they are not being shared.

Maybe I need a sort of hybrid approach by reintroducing part of
"irq-crossbar.c" code to replace the irq domain layout that is currently
being done using DT properties ?

However, I have not seen any examples of how to describe, using the DT,
an association between a device HW irq, and the GIC hwirq where it goes to,
nor how to express in the DT that multiple devices should share a given GIC
hwirq.
Basically, when a device requests the IRQ specified in its DT, I need:
- to know which GIC hwirq line should I route it to (or the GIC to tell
me which one it expects)
- two devices should be able to request to share the same GIC hwirq

If you take a look at the DT for drivers/irqchip/irq-tango.c
(arch/arm/boot/dts/tango4-common.dtsi) you will see that 3 domains are
created using DT nodes. The difference being that in the irq-tango.c case
the routing is fixed w.r.t the GIC.

Best regards,

Sebastian

Re: [RFC PATCH v1] irqchip: add support for SMP irq router

[Marc Zyngier]

On 05/07/16 17:38, Sebastian Frias wrote:
> Hi Jason,
> 
> On 07/05/2016 05:53 PM, Jason Cooper wrote:
>>>
>>> Thanks for your comments.
>>> So, aside from some naming issues, do you think the driver is ok?
>>
>> Well, it's going to be few days before I can really dig in to this.
>> Until then, what I can say I see is that it looks like you're using
>> devicetree to tell Linux how to lay out the irq domains.  That's not
>> right :(
> 
> Ok, so that replies my questions 1 and 2, thanks.
> 
>>
>> The devicetree should *only* describe the hardware. Would *BSD be able
>> to use the description in the dtb effectively?
>>
>> iiuc, I think irq-crossbar.c may be a similar enough in task to give you
>> an idea or two.
> 
> I already did something like that, you can see it here:
> 
> https://marc.info/?l=linux-kernel&m=146592235919308&w=2
> 
> the problem with that code is that it cannot handle more than 24 IRQs (the
> number of outputs of the router), because they are not being shared.
> 
> Maybe I need a sort of hybrid approach by reintroducing part of
> "irq-crossbar.c" code to replace the irq domain layout that is currently
> being done using DT properties ?
> 
> However, I have not seen any examples of how to describe, using the DT,
> an association between a device HW irq, and the GIC hwirq where it goes to,
> nor how to express in the DT that multiple devices should share a given GIC
> hwirq.
> Basically, when a device requests the IRQ specified in its DT, I need:
> - to know which GIC hwirq line should I route it to (or the GIC to tell
> me which one it expects)

You really don't need to describe this. The configuration that is
applied to your router in entirely under software control, and none of
that should appear in the DT. You could decide to mux all the interrupts
to a single one, or decide that the 23 first interrupts you discover get
their own private line to the GIC and that everything else is muxed.

So given that this is completely defined by software, it has no place in
DT. There may not be an example of such an interrupt controller in the
tree, but this doesn't look too hard to implement.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [RFC PATCH v1] irqchip: add support for SMP irq router

[Sebastian Frias]

Hi Marc,

On 07/05/2016 06:48 PM, Marc Zyngier wrote:
>> I already did something like that, you can see it here:
>>
>> https://marc.info/?l=linux-kernel&m=146592235919308&w=2
>>
>> the problem with that code is that it cannot handle more than 24 IRQs (the
>> number of outputs of the router), because they are not being shared.
>>
>> Maybe I need a sort of hybrid approach by reintroducing part of
>> "irq-crossbar.c" code to replace the irq domain layout that is currently
>> being done using DT properties ?
>>
>> However, I have not seen any examples of how to describe, using the DT,
>> an association between a device HW irq, and the GIC hwirq where it goes to,
>> nor how to express in the DT that multiple devices should share a given GIC
>> hwirq.
>> Basically, when a device requests the IRQ specified in its DT, I need:
>> - to know which GIC hwirq line should I route it to (or the GIC to tell
>> me which one it expects)
> 
> You really don't need to describe this. The configuration that is
> applied to your router in entirely under software control,

With "entirely under software control" do you mean this driver's code?

> and none of
> that should appear in the DT. You could decide to mux all the interrupts
> to a single one, or decide that the 23 first interrupts you discover get
> their own private line to the GIC and that everything else is muxed.
> 
> So given that this is completely defined by software, it has no place in
> DT. 

I think I'm missing something, what is the difference between the domains
described by nodes in the DT for irq-tango.c (arch/arm/boot/dts/tango4-common.dtsi)
and the DT from my RFC?

Alternatively, the previous DT (arch/arm/boot/dts/tango4-common.dtsi) allowed
IRQ sharing to be specified in the DT, is that wrong?

>There may not be an example of such an interrupt controller in the
> tree, but this doesn't look too hard to implement.

Well, if you the domains should not be described in the DT and that they should
be somehow hardcoded into the drivers' code, it should not be hard indeed.

Best regards,

Sebastian

Re: [RFC PATCH v1] irqchip: add support for SMP irq router

[Marc Zyngier]

On 05/07/16 17:59, Sebastian Frias wrote:
> Hi Marc,
> 
> On 07/05/2016 06:48 PM, Marc Zyngier wrote:
>>> I already did something like that, you can see it here:
>>>
>>> https://marc.info/?l=linux-kernel&m=146592235919308&w=2
>>>
>>> the problem with that code is that it cannot handle more than 24 IRQs (the
>>> number of outputs of the router), because they are not being shared.
>>>
>>> Maybe I need a sort of hybrid approach by reintroducing part of
>>> "irq-crossbar.c" code to replace the irq domain layout that is currently
>>> being done using DT properties ?
>>>
>>> However, I have not seen any examples of how to describe, using the DT,
>>> an association between a device HW irq, and the GIC hwirq where it goes to,
>>> nor how to express in the DT that multiple devices should share a given GIC
>>> hwirq.
>>> Basically, when a device requests the IRQ specified in its DT, I need:
>>> - to know which GIC hwirq line should I route it to (or the GIC to tell
>>> me which one it expects)
>>
>> You really don't need to describe this. The configuration that is
>> applied to your router in entirely under software control,
> 
> With "entirely under software control" do you mean this driver's code?

Yes.

> 
>> and none of
>> that should appear in the DT. You could decide to mux all the interrupts
>> to a single one, or decide that the 23 first interrupts you discover get
>> their own private line to the GIC and that everything else is muxed.
>>
>> So given that this is completely defined by software, it has no place in
>> DT. 
> 
> I think I'm missing something, what is the difference between the domains
> described by nodes in the DT for irq-tango.c (arch/arm/boot/dts/tango4-common.dtsi)
> and the DT from my RFC?

The fundamental difference is that with your new fancy controller, you
can decide what is going where, while the previous one is completely set
in stone (the output line is a direct function of the input line).

> 
> Alternatively, the previous DT (arch/arm/boot/dts/tango4-common.dtsi) allowed
> IRQ sharing to be specified in the DT, is that wrong?
> 
>> There may not be an example of such an interrupt controller in the
>> tree, but this doesn't look too hard to implement.
> 
> Well, if you the domains should not be described in the DT and that they should
> be somehow hardcoded into the drivers' code, it should not be hard indeed.

Hardcoded? No way. You simply implement a route allocator in your
driver, assigning them as needed. And yes, if you have more than 24
interrupts, they get muxed.

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [RFC PATCH v1] irqchip: add support for SMP irq router

[Thomas Gleixner]

On Tue, 5 Jul 2016, Marc Zyngier wrote:
> On 05/07/16 17:59, Sebastian Frias wrote:
> > Well, if you the domains should not be described in the DT and that they should
> > be somehow hardcoded into the drivers' code, it should not be hard indeed.
> 
> Hardcoded? No way. You simply implement a route allocator in your
> driver, assigning them as needed. And yes, if you have more than 24
> interrupts, they get muxed.

There is one caveat though. Under some circumstances (think RT) you want to
configure which interrupts get muxed and which not. We really should have that
option, but yes for anything which has less than 24 autorouting is the way to
go.

Thanks,

	tglx

Re: [RFC PATCH v1] irqchip: add support for SMP irq router

[Marc Zyngier]

On 05/07/16 20:24, Thomas Gleixner wrote:
> On Tue, 5 Jul 2016, Marc Zyngier wrote:
>> On 05/07/16 17:59, Sebastian Frias wrote:
>>> Well, if you the domains should not be described in the DT and that they should
>>> be somehow hardcoded into the drivers' code, it should not be hard indeed.
>>
>> Hardcoded? No way. You simply implement a route allocator in your
>> driver, assigning them as needed. And yes, if you have more than 24
>> interrupts, they get muxed.
> 
> There is one caveat though. Under some circumstances (think RT) you want to
> configure which interrupts get muxed and which not. We really should have that
> option, but yes for anything which has less than 24 autorouting is the way to
> go.

Good point. I can see two possibilities for that:

- either we describe this DT with some form of hint, indicating what are
the inputs that can be muxed to a single output. Easy, but the DT guys
are going to throw rocks at me for being Linux-specific.

- or we have a way to express QoS in the irq subsystem, and a driver can
request an interrupt with a "make it fast" flag. Of course, everybody
and his dog are going to ask for it, and we're back to square one.

Do we have a way to detect which interrupt is more likely to be
sensitive to muxing? My hunch is that if it is requested with
IRQF_SHARED, then it is effectively muxable. Thoughts?

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [RFC PATCH v1] irqchip: add support for SMP irq router

[Thomas Gleixner]

On Wed, 6 Jul 2016, Marc Zyngier wrote:
> On 05/07/16 20:24, Thomas Gleixner wrote:
> > On Tue, 5 Jul 2016, Marc Zyngier wrote:
> >> Hardcoded? No way. You simply implement a route allocator in your
> >> driver, assigning them as needed. And yes, if you have more than 24
> >> interrupts, they get muxed.
> > 
> > There is one caveat though. Under some circumstances (think RT) you want to
> > configure which interrupts get muxed and which not. We really should have that
> > option, but yes for anything which has less than 24 autorouting is the way to
> > go.
> 
> Good point. I can see two possibilities for that:
> 
> - either we describe this DT with some form of hint, indicating what are
> the inputs that can be muxed to a single output. Easy, but the DT guys
> are going to throw rocks at me for being Linux-specific.

That's not necessarily Linux specific. The problem arises with any other OS as
well.

> - or we have a way to express QoS in the irq subsystem, and a driver can
> request an interrupt with a "make it fast" flag. Of course, everybody
> and his dog are going to ask for it, and we're back to square one.

That and the driver does not know about the particular application
scenario/system configuration.
 
> Do we have a way to detect which interrupt is more likely to be
> sensitive to muxing? My hunch is that if it is requested with
> IRQF_SHARED, then it is effectively muxable. Thoughts?

That's too late. request_irq happens _after_ the interrupt is set up and the
routing established.

Thanks,

	tglx

Re: [RFC PATCH v1] irqchip: add support for SMP irq router

[Sebastian Frias]

Hi Marc,

On 07/05/2016 07:13 PM, Marc Zyngier wrote:
>>> You really don't need to describe this. The configuration that is
>>> applied to your router in entirely under software control,
>>
>> With "entirely under software control" do you mean this driver's code?
> 
> Yes.

Ok.

> 
>>
>>> and none of
>>> that should appear in the DT. You could decide to mux all the interrupts
>>> to a single one, or decide that the 23 first interrupts you discover get
>>> their own private line to the GIC and that everything else is muxed.
>>>
>>> So given that this is completely defined by software, it has no place in
>>> DT. 
>>
>> I think I'm missing something, what is the difference between the domains
>> described by nodes in the DT for irq-tango.c (arch/arm/boot/dts/tango4-common.dtsi)
>> and the DT from my RFC?
> 
> The fundamental difference is that with your new fancy controller, you
> can decide what is going where, while the previous one is completely set
> in stone (the output line is a direct function of the input line).

I think that's where part the misunderstanding comes from.
IMHO the output line is not a direct function of the input line.
Any of the 64 IRQ lines entering the "old controller" (irq-tango.c) can be
routed to any of its 3 outputs.
The only thing fixed is which GIC input is connected to those 3 outputs, ie:
GIC inputs 2, 3 and 4.

In the the "new controller" (irq-tango_v2.c, this RFC), any of 128 IRQ lines
can be routed to any of 24 outputs, connected to GIC inputs 0...23.

In a nutshell:
- "old controller": routes [0...N] => GIC inputs [2...4]
- "new controller": routes [0...M] => GIC inputs [0...23]

So, when we think about it, if the "new DT" specified 24 domains, it would
be equivalent of the "old DT" with 3 domains, right?

That's why it seemed more or less natural to keep describing the domains in
the DT, the main reason for that being that it allowed the user to specify
the IRQ sharing in the DT, and this is precisely the key point of this.

So, putting aside routing considerations and the discussion above, I think
a simpler question is: if the domains should not be described in the DT,
how can we define the IRQ sharing in the DT?

Best regards,

Sebastian

Re: [RFC PATCH v1] irqchip: add support for SMP irq router

[Sebastian Frias]

Hi,

On 07/06/2016 11:30 AM, Thomas Gleixner wrote:
> On Wed, 6 Jul 2016, Marc Zyngier wrote:
>> On 05/07/16 20:24, Thomas Gleixner wrote:
>>> On Tue, 5 Jul 2016, Marc Zyngier wrote:
>>>> Hardcoded? No way. You simply implement a route allocator in your
>>>> driver, assigning them as needed. And yes, if you have more than 24
>>>> interrupts, they get muxed.
>>>
>>> There is one caveat though. Under some circumstances (think RT) you want to
>>> configure which interrupts get muxed and which not. We really should have that
>>> option, but yes for anything which has less than 24 autorouting is the way to
>>> go.
>>
>> Good point. I can see two possibilities for that:
>>
>> - either we describe this DT with some form of hint, indicating what are
>> the inputs that can be muxed to a single output. Easy, but the DT guys
>> are going to throw rocks at me for being Linux-specific.
> 
> That's not necessarily Linux specific. The problem arises with any other OS as
> well.
> 
>> - or we have a way to express QoS in the irq subsystem, and a driver can
>> request an interrupt with a "make it fast" flag. Of course, everybody
>> and his dog are going to ask for it, and we're back to square one.
> 
> That and the driver does not know about the particular application
> scenario/system configuration.
>  
>> Do we have a way to detect which interrupt is more likely to be
>> sensitive to muxing? My hunch is that if it is requested with
>> IRQF_SHARED, then it is effectively muxable. Thoughts?
> 
> That's too late. request_irq happens _after_ the interrupt is set up and the
> routing established.
> 

What about using 3 values for the interrupt description like the GIC does?
When connecting to the GIC we say "interrupts = <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>;"
If devices using this driver (the one from the RFC) requested the interrupt like:
"interrupts = <0 38 IRQ_TYPE_LEVEL_HIGH>;"
"interrupts = <2 27 IRQ_TYPE_LEVEL_HIGH>;"
etc.
with the first field being the "group", then the driver could create a domain
for the device's IRQ (or associate it to an existing one if it has already been
created). It would thus serve as a hint on how to create domains and how to
share IRQs into the same line (domain).

I guess I can get such information from the .translate and .alloc callbacks
from a newly created domain hierarchy attached to the GIC, right?

What do you think?

Best regards,

Sebastian

Re: [RFC PATCH v1] irqchip: add support for SMP irq router

[Sebastian Frias]

Hi Jason,

On 07/05/2016 06:16 PM, Jason Cooper wrote:
>> Come to think of it, I'm not sure the *name* of the file documenting
>> a binding is as important to DT maintainers as the compatible string.
> 
> Correct.  devicetee compatible strings need to be as specific as
> possible.  

Specific with respect to what thing? To the HW module they are describing
(USB, IRQ controller, etc.) or to the chip the HW module it is embedded
into?

>In a series of compatible IP blocks, the string should refer
> to the first version in the series, e.g. sigma,smp8710 for a series of
> compatible IP blocks like 8710, 8712, 8715, 8724.  If an 8751 came along
> with a different register layout or some other incompatibility, then a
> new string would be sigma,smp8751.  So,
> 
>   8710 uses "sigma,smp8710"
>   8712 uses "sigma,smp8710"
>   8715 uses "sigma,smp8710"
>   8724 uses "sigma,smp8710"
>   8751 uses "sigma,smp8751"
>   8754 uses "sigma,smp8751"
> 

But this is not consistent with the strings for generic drivers, like
"generic-xhci", etc.

A SoC is composed of several HW modules, some are shared among different
manufacturers (i.e.: "generic-xhci"), and some are shared among different
product lines of the same manufacturer (i.e.: "sigma,smp,irqrouter").

And the DT for a given chip should describe the collection of HW modules
that make up the SoC, regardless of what chip introduced them or what
other chip uses it. Why would that be relevant anyway?

By that reasoning, I also think that drivers/net/ethernet/aurora/nb8800.c
should have had a string like "aurora,nb8800,sigma" or something, to
specify that it is a NB8800 from Aurora integrated in a Sigma platform.
That way it can behave as vanilla NB8800 when the string is "aurora,nb8800"
and it can adapt its behaviour to Sigma's platform when a different string
(like "aurora,nb8800,sigma") is used in the DT.

Later if Sigma modified the nb8800 HW, it could be "aurora,nb8800,sigma-v2".
The same way, if later Sigma used a different Aurora HW module, the DT
would say "aurora,nb2000,sigma" (to signal that another driver is required)

Instead, drivers/net/ethernet/aurora/nb8800.c has "sigma,smp8642-ethernet"
string, which ties it to a particular chip, and I don't see how does that
convey version about the module or other relevant information; Plus it is
confusing if the same module is then embedded on a SMP8756 chip.

Would you mind explaining how does that contrasts with the logic used
by DT naming conventions?

Best regards,

Sebastian

Re: [RFC PATCH v1] irqchip: add support for SMP irq router

[Marc Zyngier]

On 06/07/16 11:47, Sebastian Frias wrote:

>>> I think I'm missing something, what is the difference between the domains
>>> described by nodes in the DT for irq-tango.c (arch/arm/boot/dts/tango4-common.dtsi)
>>> and the DT from my RFC?
>>
>> The fundamental difference is that with your new fancy controller, you
>> can decide what is going where, while the previous one is completely set
>> in stone (the output line is a direct function of the input line).
> 
> I think that's where part the misunderstanding comes from.
> IMHO the output line is not a direct function of the input line.
> Any of the 64 IRQ lines entering the "old controller" (irq-tango.c) can be
> routed to any of its 3 outputs.

Then the current DT binding isn't properly describing the HW.

> The only thing fixed is which GIC input is connected to those 3 outputs, ie:
> GIC inputs 2, 3 and 4.
> 
> In the the "new controller" (irq-tango_v2.c, this RFC), any of 128 IRQ lines
> can be routed to any of 24 outputs, connected to GIC inputs 0...23.
> 
> In a nutshell:
> - "old controller": routes [0...N] => GIC inputs [2...4]
> - "new controller": routes [0...M] => GIC inputs [0...23]
> 
> So, when we think about it, if the "new DT" specified 24 domains, it would
> be equivalent of the "old DT" with 3 domains, right?

Indeed, but I consider the "old" binding to be rather misleading. It
should have been described as a router too, rather than hardcoding
things in DT. Granted, it doesn't matter much when you only have 3
possible output lines. But with 24 outputs, that becomes much more relevant.

> 
> That's why it seemed more or less natural to keep describing the domains in
> the DT, the main reason for that being that it allowed the user to specify
> the IRQ sharing in the DT, and this is precisely the key point of this.
> 
> So, putting aside routing considerations and the discussion above, I think
> a simpler question is: if the domains should not be described in the DT,
> how can we define the IRQ sharing in the DT?

You could have a set of sub-nodes saying something like this:

	mux-hint0 {
		inputs = <1 45 127>;
	}

	mux-hint1 {
		inputs = <2 33>;
	}

(or maybe you can have that as direct properties, but you get the idea).
Here, you have two output pins dedicated to muxed interrupts (assuming
they are all level interrupts), and the last 22 can be freely allocated
as direct routes.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [RFC PATCH v1] irqchip: add support for SMP irq router

[Marc Zyngier]

On 06/07/16 11:49, Sebastian Frias wrote:
> Hi,
> 
> On 07/06/2016 11:30 AM, Thomas Gleixner wrote:
>> On Wed, 6 Jul 2016, Marc Zyngier wrote:
>>> On 05/07/16 20:24, Thomas Gleixner wrote:
>>>> On Tue, 5 Jul 2016, Marc Zyngier wrote:
>>>>> Hardcoded? No way. You simply implement a route allocator in your
>>>>> driver, assigning them as needed. And yes, if you have more than 24
>>>>> interrupts, they get muxed.
>>>>
>>>> There is one caveat though. Under some circumstances (think RT) you want to
>>>> configure which interrupts get muxed and which not. We really should have that
>>>> option, but yes for anything which has less than 24 autorouting is the way to
>>>> go.
>>>
>>> Good point. I can see two possibilities for that:
>>>
>>> - either we describe this DT with some form of hint, indicating what are
>>> the inputs that can be muxed to a single output. Easy, but the DT guys
>>> are going to throw rocks at me for being Linux-specific.
>>
>> That's not necessarily Linux specific. The problem arises with any other OS as
>> well.
>>
>>> - or we have a way to express QoS in the irq subsystem, and a driver can
>>> request an interrupt with a "make it fast" flag. Of course, everybody
>>> and his dog are going to ask for it, and we're back to square one.
>>
>> That and the driver does not know about the particular application
>> scenario/system configuration.
>>  
>>> Do we have a way to detect which interrupt is more likely to be
>>> sensitive to muxing? My hunch is that if it is requested with
>>> IRQF_SHARED, then it is effectively muxable. Thoughts?
>>
>> That's too late. request_irq happens _after_ the interrupt is set up and the
>> routing established.
>>
> 
> What about using 3 values for the interrupt description like the GIC does?
> When connecting to the GIC we say "interrupts = <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>;"
> If devices using this driver (the one from the RFC) requested the interrupt like:
> "interrupts = <0 38 IRQ_TYPE_LEVEL_HIGH>;"
> "interrupts = <2 27 IRQ_TYPE_LEVEL_HIGH>;"
> etc.
> with the first field being the "group", then the driver could create a domain
> for the device's IRQ (or associate it to an existing one if it has already been
> created). It would thus serve as a hint on how to create domains and how to
> share IRQs into the same line (domain).
> 
> I guess I can get such information from the .translate and .alloc callbacks
> from a newly created domain hierarchy attached to the GIC, right?

This wouldn't work. You need to instantiate the domains long before
you've parsed a single interrupt specifier, otherwise you don't know
where to allocate it from.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [RFC PATCH v1] irqchip: add support for SMP irq router

[Jason Cooper]

Hi Sebastian,

On Wed, Jul 06, 2016 at 01:37:21PM +0200, Sebastian Frias wrote:
> On 07/05/2016 06:16 PM, Jason Cooper wrote:
> >> Come to think of it, I'm not sure the *name* of the file documenting
> >> a binding is as important to DT maintainers as the compatible string.
> > 
> > Correct.  devicetee compatible strings need to be as specific as
> > possible.  
> 
> Specific with respect to what thing? To the HW module they are describing
> (USB, IRQ controller, etc.) or to the chip the HW module it is embedded
> into?

The compatible string uniquely identifies an interface between an IP
block and the software (the devicetree binding).  We use the most
specific model number or name we can for that IP block when we create
the binding and the compatible string.

If future SoCs come out and the IP block contained within, regardless of
identifier, is compatible with the existing binding, then we can reuse
it.  This is what I was trying to show with my little chart quoted
below.

For ethernet and other major blocks it's easy because they have their
own model numbers and such.  For the smaller blocks, like irqchips, we
have to use the model number or unique name of the SoC we first found it
in.

> >In a series of compatible IP blocks, the string should refer
> > to the first version in the series, e.g. sigma,smp8710 for a series of
> > compatible IP blocks like 8710, 8712, 8715, 8724.  If an 8751 came along
> > with a different register layout or some other incompatibility, then a
> > new string would be sigma,smp8751.  So,
> > 
> >   8710 uses "sigma,smp8710"
> >   8712 uses "sigma,smp8710"
> >   8715 uses "sigma,smp8710"
> >   8724 uses "sigma,smp8710"
> >   8751 uses "sigma,smp8751"
> >   8754 uses "sigma,smp8751"
> > 
> 
> But this is not consistent with the strings for generic drivers, like
> "generic-xhci", etc.

True.  I've not dealt with xhci/dt at all.  My guess would be that there
are a lot of xhci chips whose interfaces conform to specification.
Those that do can just use "generic-xhci".

> A SoC is composed of several HW modules, some are shared among different
> manufacturers (i.e.: "generic-xhci"), and some are shared among different
> product lines of the same manufacturer (i.e.: "sigma,smp,irqrouter").

Yes, that's why it's critical to be specific.  We want to reuse drivers
where it makes sense.  We can if the interface is the same.

> And the DT for a given chip should describe the collection of HW modules
> that make up the SoC, regardless of what chip introduced them or what
> other chip uses it. Why would that be relevant anyway?

Because the first time we see a new IP block, we can't predict the
future.  We don't know where it's going to be reused.  We don't know
when it's going to be changed, or how.  So we mark it as specifically as
we can based on the information we have when we first encounter it.

When a new version comes out, we see if the interface is still
compatible.  If it is, there's nothing to do.  If it changed, we see if
we can address it by adding a property to the existing binding.  Failing
that, we create a new compatible string to indicate a new interface.

> By that reasoning, I also think that drivers/net/ethernet/aurora/nb8800.c
> should have had a string like "aurora,nb8800,sigma" or something, to
> specify that it is a NB8800 from Aurora integrated in a Sigma platform.

The fact that it is in the Sigma dts file tells this already.  Based on
the above, did the interface change when adding it to the sigma
platform?  Can the binding be reused?

> That way it can behave as vanilla NB8800 when the string is "aurora,nb8800"
> and it can adapt its behaviour to Sigma's platform when a different string
> (like "aurora,nb8800,sigma") is used in the DT.

I would have to look at the specifics of "adapt its behaviour" in order
to determine wether a new compatible is warranted, or maybe just add a
property to the binding.

> Later if Sigma modified the nb8800 HW, it could be "aurora,nb8800,sigma-v2".
> The same way, if later Sigma used a different Aurora HW module, the DT
> would say "aurora,nb2000,sigma" (to signal that another driver is required)

The compatible string is important, but we try not to overload it.
There are many flexible ways we can indicate properties or quirks in the
binding.

> Instead, drivers/net/ethernet/aurora/nb8800.c has "sigma,smp8642-ethernet"
> string, which ties it to a particular chip, and I don't see how does that
> convey version about the module or other relevant information; Plus it is
> confusing if the same module is then embedded on a SMP8756 chip.

Sorry, it's just understood that the compatible string means they have
the same binding/interface.

> Would you mind explaining how does that contrasts with the logic used
> by DT naming conventions?

I think you may be looking at DT the wrong way :-P  It's not an
inventory of components for the end user to look at.  It a list of
hardware interfaces and how they're attached; for the OS to interpret
and use.

hth,

Jason.

Re: [RFC PATCH v1] irqchip: add support for SMP irq router

[Jason Cooper]

On Wed, Jul 06, 2016 at 11:30:48AM +0200, Thomas Gleixner wrote:
> On Wed, 6 Jul 2016, Marc Zyngier wrote:
> > On 05/07/16 20:24, Thomas Gleixner wrote:
> > > On Tue, 5 Jul 2016, Marc Zyngier wrote:
> > >> Hardcoded? No way. You simply implement a route allocator in your
> > >> driver, assigning them as needed. And yes, if you have more than 24
> > >> interrupts, they get muxed.
> > > 
> > > There is one caveat though. Under some circumstances (think RT) you want to
> > > configure which interrupts get muxed and which not. We really should have that
> > > option, but yes for anything which has less than 24 autorouting is the way to
> > > go.
> > 
> > Good point. I can see two possibilities for that:
> > 
> > - either we describe this DT with some form of hint, indicating what are
> > the inputs that can be muxed to a single output. Easy, but the DT guys
> > are going to throw rocks at me for being Linux-specific.
> 
> That's not necessarily Linux specific. The problem arises with any other OS as
> well.

I could see a property

	irq,no-mux = <3 7 13 19 23 ...>;

Or "irq,fastpath".  It's describing an optimal configuration of the
system.  $driver for $OS can route those first individually.  The others
would be eligible for muxing.

thx,

Jason.

Re: [RFC PATCH v1] irqchip: add support for SMP irq router

[Sebastian Frias]

Hi Marc,

On 07/06/2016 03:50 PM, Marc Zyngier wrote:
>> I think that's where part the misunderstanding comes from.
>> IMHO the output line is not a direct function of the input line.
>> Any of the 64 IRQ lines entering the "old controller" (irq-tango.c) can be
>> routed to any of its 3 outputs.
> 
> Then the current DT binding isn't properly describing the HW.

Ok, thanks, so it is not a good example then.

>> In a nutshell:
>> - "old controller": routes [0...N] => GIC inputs [2...4]
>> - "new controller": routes [0...M] => GIC inputs [0...23]
>>
>> So, when we think about it, if the "new DT" specified 24 domains, it would
>> be equivalent of the "old DT" with 3 domains, right?
> 
> Indeed, but I consider the "old" binding to be rather misleading. It
> should have been described as a router too, rather than hardcoding
> things in DT. Granted, it doesn't matter much when you only have 3
> possible output lines. But with 24 outputs, that becomes much more relevant.

I see.

>> So, putting aside routing considerations and the discussion above, I think
>> a simpler question is: if the domains should not be described in the DT,
>> how can we define the IRQ sharing in the DT?
> 
> You could have a set of sub-nodes saying something like this:
> 
> 	mux-hint0 {
> 		inputs = <1 45 127>;
> 	}
> 
> 	mux-hint1 {
> 		inputs = <2 33>;
> 	}
> 
> (or maybe you can have that as direct properties, but you get the idea).
> Here, you have two output pins dedicated to muxed interrupts (assuming
> they are all level interrupts), and the last 22 can be freely allocated
> as direct routes.
> 

Ok, I'll try to do that.
So, aside from the DT issues (that is, that it is describing domains),
would it be ok to create a domain for each of the outputs?

Because I was looking at:
- Documentation/devicetree/bindings/interrupt-controller/samsung,exynos4210-combiner.txt
- drivers/irqchip/exynos-combiner.c
- arch/arm/boot/dts/exynos4210.dtsi

and what I see is that the DT basically list all outputs [0...15] connected
to the parent interrupt controller, although the driver does not creates
separate domains, just one. Then it attaches a chained handler for each of
the outputs. On the .map callback it attaches a irqchip to the domain.

There is also:
- Documentation/devicetree/bindings/arm/omap/crossbar.txt
- drivers/irqchip/irq-crossbar.c
- arch/arm/boot/dts/dra7.dtsi

This one creates a domain hierarchy linked to the parent domain and uses
irq_domain_alloc_irqs_parent() and irq_domain_set_hwirq_and_chip() to attach
a irqchip to the domain on the .alloc callback.

Both use a single domain, as opposed to irq-tango.c which creates 3 domains.
Right now irq-tango_v2.c is supposed to create one domain per output (if
so the DT says)
Are there guidelines regarding that?

Thanks in advance.
Best regards,

Sebastian

Re: [RFC PATCH v1] irqchip: add support for SMP irq router

[Marc Zyngier]

On 07/07/16 13:16, Sebastian Frias wrote:
> Hi Marc,
> 
> On 07/06/2016 03:50 PM, Marc Zyngier wrote:
>>> I think that's where part the misunderstanding comes from.
>>> IMHO the output line is not a direct function of the input line.
>>> Any of the 64 IRQ lines entering the "old controller" (irq-tango.c) can be
>>> routed to any of its 3 outputs.
>>
>> Then the current DT binding isn't properly describing the HW.
> 
> Ok, thanks, so it is not a good example then.
> 
>>> In a nutshell:
>>> - "old controller": routes [0...N] => GIC inputs [2...4]
>>> - "new controller": routes [0...M] => GIC inputs [0...23]
>>>
>>> So, when we think about it, if the "new DT" specified 24 domains, it would
>>> be equivalent of the "old DT" with 3 domains, right?
>>
>> Indeed, but I consider the "old" binding to be rather misleading. It
>> should have been described as a router too, rather than hardcoding
>> things in DT. Granted, it doesn't matter much when you only have 3
>> possible output lines. But with 24 outputs, that becomes much more relevant.
> 
> I see.
> 
>>> So, putting aside routing considerations and the discussion above, I think
>>> a simpler question is: if the domains should not be described in the DT,
>>> how can we define the IRQ sharing in the DT?
>>
>> You could have a set of sub-nodes saying something like this:
>>
>> 	mux-hint0 {
>> 		inputs = <1 45 127>;
>> 	}
>>
>> 	mux-hint1 {
>> 		inputs = <2 33>;
>> 	}
>>
>> (or maybe you can have that as direct properties, but you get the idea).
>> Here, you have two output pins dedicated to muxed interrupts (assuming
>> they are all level interrupts), and the last 22 can be freely allocated
>> as direct routes.
>>
> 
> Ok, I'll try to do that.
> So, aside from the DT issues (that is, that it is describing domains),
> would it be ok to create a domain for each of the outputs?
> 
> Because I was looking at:
> - Documentation/devicetree/bindings/interrupt-controller/samsung,exynos4210-combiner.txt
> - drivers/irqchip/exynos-combiner.c
> - arch/arm/boot/dts/exynos4210.dtsi
> 
> and what I see is that the DT basically list all outputs [0...15] connected
> to the parent interrupt controller, although the driver does not creates
> separate domains, just one. Then it attaches a chained handler for each of
> the outputs. On the .map callback it attaches a irqchip to the domain.
> 
> There is also:
> - Documentation/devicetree/bindings/arm/omap/crossbar.txt
> - drivers/irqchip/irq-crossbar.c
> - arch/arm/boot/dts/dra7.dtsi
> 
> This one creates a domain hierarchy linked to the parent domain and uses
> irq_domain_alloc_irqs_parent() and irq_domain_set_hwirq_and_chip() to attach
> a irqchip to the domain on the .alloc callback.
> 
> Both use a single domain, as opposed to irq-tango.c which creates 3 domains.
> Right now irq-tango_v2.c is supposed to create one domain per output (if
> so the DT says)
> Are there guidelines regarding that?

The sensible thing would be to have one domain per output that muxes
inputs, and a hierarchical domain for all the other inputs (which are
mapped 1:1 with their output).

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[RFC PATCH v2] irqchip: add support for SMP irq router

[Sebastian Frias]

This adds support for a second-gen irq router/controller present
on some Sigma Designs chips.

Signed-off-by: Sebastian Frias <sf84@laposte.net>
---

This is RFC v2 attempts to address the comments given on the previous
RFC:
- domains used to be created by the DT, now they are created internally
by the driver:
https://marc.info/?l=linux-kernel&m=146773883324507&w=2
- IRQ sharing (which does not seems to be handled by current DT spec,
https://marc.info/?l=linux-kernel&m=146779759405950&w=2) is implemented
as:
  - explicit grouping: https://marc.info/?l=linux-kernel&m=146781302410442&w=2
  - implicit grouping: https://marc.info/?l=linux-kernel&m=146780217707395&w=2

However, I still have a few doubts:
1) I have implemented two ways of declaring the IRQ sharing, implicit and
explicit IRQ grouping, see:
Documentation/devicetree/bindings/interrupt-controller/sigma,smp,irqrouter.txt
for more information.

Since it is still not clear how this is going to be used, I prefer to keep
both available for the moment.

2) In order to get a virq mapping for the domains associated with the outputs
(the outputs connected to the GIC) I request a sort of "Fake HW IRQ"
See irq-tango_v4.c:1400

	/* To request a virq we need a HW IRQ, use a "Fake HW IRQ" */
	hwirq      = index + irqrouter->input_count + irqrouter->swirq_count;

This feels a bit like a hack, so suggestions are welcome.

3) The file is called 'irq-tango_v4.c' but I think it should match the
compatible string, so I was thinking of renaming:
irq-tango_v4.c => irq-sigma_smp_irqrouter.c
irq-tango_v4.h => irq-sigma_smp_irqrouter.h

What do you think?

4) Do I have to do something more to handle the affinity stuff?

5) checkpatch.pl reports a few warnings:

- WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
I have not changed the file because I would like to be settled on the naming
first (see point 2 above)
- WARNING: quoted string split across lines
I think it is not an issue
- WARNING: line over 80 characters
This is for two big macros, I think it is not an issue

6) More of a theoretical question:
I have to #include <dt-bindings/interrupt-controller/irq-tango_v4.h> from
code that is not in the Linux tree. That's fine for now, but if in the future
there are other irq controllers and another header is required, how can the
code know which header to #include ?
Are we supposed to #include all of them, and then somehow detect which module
is actually active and act accordingly? If so, can we detect which module
matched and probed correctly to know which controller version we need to
talk to?

Please feel free to comment and suggest improvements.

---
 .../interrupt-controller/sigma,smp,irqrouter.txt   |  135 ++
 drivers/irqchip/Makefile                           |    1 +
 drivers/irqchip/irq-tango_v4.c                     | 1729 ++++++++++++++++++++
 .../interrupt-controller/irq-tango_v4.h            |   39 +
 4 files changed, 1904 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/sigma,smp,irqrouter.txt
 create mode 100644 drivers/irqchip/irq-tango_v4.c
 create mode 100644 include/dt-bindings/interrupt-controller/irq-tango_v4.h

diff --git a/Documentation/devicetree/bindings/interrupt-controller/sigma,smp,irqrouter.txt b/Documentation/devicetree/bindings/interrupt-controller/sigma,smp,irqrouter.txt
new file mode 100644
index 0000000..ff7c4d5
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/sigma,smp,irqrouter.txt
@@ -0,0 +1,135 @@
+* Sigma Designs Interrupt Router
+
+This module can route X IRQ inputs and Y SW IRQ inputs into Z IRQ outputs,
+with X+Y>Z.
+For instance X=128, Y=16, Z=24.
+
+Note however that the HW does not latch the IRQ lines, so devices
+connecting to the router are expected to latch their IRQ line by themselves.
+
+SW IRQs can be used by firmware running on different parts of the SoC to
+communicate with the CPU.
+They can be registered by specific drivers, and any entity capable of writing
+to the Interrupt Router registers can trigger and clear these interrupts.
+
+A single node in the device tree is used to describe the Interrupt Router.
+Since X+Y>Z, some IRQ inputs may need to be routed to the same IRQ output,
+thus "sharing the IRQ line".
+
+There are two ways of defining such sharing, either by specifying the number
+of IRQ groups, either by listing the IRQ groups and their contents.
+In the former case the groups are created and named "implicitly", in the
+later case the groups are created "explicitly" by grouping IRQs.
+An IRQ group will share an IRQ output, thus all input IRQs falling into that
+group will share a given IRQ line.
+
+If SW IRQs are enabled ("swirq-count" != 0), they will *ALL* be grouped
+together.
+
+Required properties:
+- compatible: Should be "sigma,smp,irqrouter".
+- interrupt-controller: Identifies the node as an interrupt controller.
+- inputs: The number of IRQ lines entering the router
+- outputs: The number of IRQ lines exiting the router
+- swirq-count: The number of SW IRQs supported. If 0, none is supported.
+- reg: Base address and size of interrupt router registers.
+- #interrupt-cells: Should be <3>. Defines how other nodes will be able to
+interact with this node. The meaning of the cells are
+	* First Cell: SIGMA_HWIRQ, SIGMA_SWIRQ, [SIGMA_IRQGROUP_1, ...,
+SIGMA_IRQGROUP_15].
+	* Second Cell: IRQ ID
+	* Third Cell: IRQ polarity (level high or low).
+
+Optional properties:
+- irq-groups: The number of IRQ groups used.
+NOTE: if present, it forces implicit group definition.
+- irq-group{i}: if 'irq-groups' is not present, then explicit group declaration
+may be required, and can be done by creating child nodes of the form:
+   irq-group0: {
+      shared-irqs = <1 38 56>;
+   };
+NOTE: the number of groups cannot exceed the number of outputs.
+NOTE2: SW IRQs have their own group so the number of total groups is
+'irq-groups'+1.
+- interrupt-parent: pHandle of the parent interrupt controller, if not
+  inherited from the parent node. It should be the GIC.
+
+
+Example:
+
+See Documentation/devicetree/bindings/interrupt-controller/interrupts.txt and
+Documentation/devicetree/bindings/arm/gic.txt for further details.
+
+The following example declares a irqrouter with 128 inputs, 24 outputs, 16
+SW IRQs, declaring 4 implicit IRQ groups, with registers @ 0x6F800 and
+connected to the GIC.
+
+		irqrouter: irqrouter@6f800 {
+			 compatible = "sigma,smp,irqrouter";
+			 reg = <0x6f800 0x800>;
+			 interrupt-controller;
+			 interrupt-parent = <&gic>;
+
+			 #interrupt-cells = <3>;
+
+			 inputs = <128>;
+			 outputs = <24>;
+			 swirq-count = <16>;
+
+			 irq-groups = <4>;
+		};
+
+Devices can then request IRQs using:
+
+a) interrupts = <SIGMA_SWIRQ      10 IRQ_TYPE_LEVEL_HIGH>;
+b) interrupts = <SIGMA_HWIRQ      10 IRQ_TYPE_LEVEL_HIGH>;
+c) interrupts = <SIGMA_IRQGROUP_2 10 IRQ_TYPE_LEVEL_HIGH>;
+
+Case a) is for SWIRQs: requests SW IRQ 10
+Case b) is for non-shared HW IRQs: requests HW IRQ 10, the irqrouter will
+assign a direct route from input (10) to a free output reserving an IRQ line on
+the parent Interrupt controller.
+Case c) is for shared HW IRQs: indeed, SIGMA_IRQGROUP_2 is used as ID, thus the
+irqrouter will then route this input (10) to the output assigned to IRQ group 2.
+
+NOTE: SW IRQ 10 and HW IRQ 10 are not the same.
+
+The following example declares the same irqrouter as the preceding example
+but using explicit IRQ group declaration.
+
+		irqrouter: irqrouter@6f800 {
+			 compatible = "sigma,smp,irqrouter";
+			 reg = <0x6f800 0x800>;
+			 interrupt-controller;
+			 interrupt-parent = <&gic>;
+
+			 #interrupt-cells = <3>;
+
+			 inputs = <128>;
+			 outputs = <24>;
+			 swirq-count = <16>;
+
+			 irq-group0 {
+			       shared-irqs = <1 38 56 11>;
+			 };
+
+			 irq-group1 {
+			       shared-irqs = <67 68>;
+			 };
+		};
+
+Devices can then request IRQs using:
+
+d) interrupts = <SIGMA_SWIRQ 10 IRQ_TYPE_LEVEL_HIGH>;
+e) interrupts = <SIGMA_HWIRQ 10 IRQ_TYPE_LEVEL_HIGH>;
+f) interrupts = <SIGMA_HWIRQ 11 IRQ_TYPE_LEVEL_HIGH>;
+
+Case d) is for SWIRQs: requests SW IRQ 10
+Case e) is for non-shared HW IRQs: requests HW IRQ 10, the irqrouter will
+assign a direct route from input (10) to a free output reserving an IRQ line on
+the parent Interrupt controller.
+Case f) is for a shared HW IRQ: indeed, 'irq-group0' lists IRQ 11 as belonging
+to a group ('irq-group0' but it is not really important), thus the irqrouter
+will route this input (11) to the output assigned to IRQ group 'irq-group0'.
+
+NOTE: SW IRQ 10 and HW IRQ 10 are not the same.
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index 4c203b6..2be8c3f 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -47,6 +47,7 @@ obj-$(CONFIG_ARCH_NSPIRE)		+= irq-zevio.o
 obj-$(CONFIG_ARCH_VT8500)		+= irq-vt8500.o
 obj-$(CONFIG_ST_IRQCHIP)		+= irq-st.o
 obj-$(CONFIG_TANGO_IRQ)			+= irq-tango.o
+obj-$(CONFIG_TANGO_IRQ)			+= irq-tango_v4.o
 obj-$(CONFIG_TB10X_IRQC)		+= irq-tb10x.o
 obj-$(CONFIG_TS4800_IRQ)		+= irq-ts4800.o
 obj-$(CONFIG_XTENSA)			+= irq-xtensa-pic.o
diff --git a/drivers/irqchip/irq-tango_v4.c b/drivers/irqchip/irq-tango_v4.c
new file mode 100644
index 0000000..51d8c0f
--- /dev/null
+++ b/drivers/irqchip/irq-tango_v4.c
@@ -0,0 +1,1729 @@
+/*
+ * Copyright (C) 2014 Sebastian Frias <sf84@laposte.net>
+ *
+ * This program is free software; you can redistribute  it and/or modify it
+ * under  the terms of  the GNU General  Public License as published by the
+ * Free Software Foundation;  either version 2 of the  License, or (at your
+ * option) any later version.
+ */
+
+#include <linux/init.h>
+#include <linux/irq.h>
+#include <linux/irqchip.h>
+#include <linux/irqchip/chained_irq.h>
+#include <linux/ioport.h>
+#include <linux/io.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/slab.h>
+#include <dt-bindings/interrupt-controller/arm-gic.h>
+#include <dt-bindings/interrupt-controller/irq-tango_v4.h>
+
+
+#define DBGERR(__format, ...)	panic("[%s:%d] %s(): " __format,	\
+				      __FILE__, __LINE__,		\
+				      __func__, ##__VA_ARGS__)
+
+#define DBGWARN(__format, ...)	pr_err("[%s:%d] %s(): " __format,	\
+				       __FILE__, __LINE__,		\
+				       __func__, ##__VA_ARGS__)
+
+
+#if 0
+#define DBGLOG(__format, ...)   pr_info("[%s:%d] %s(): " __format,	\
+					__FILE__, __LINE__,		\
+					__func__, ##__VA_ARGS__)
+#else
+#define DBGLOG(__format, ...) do {} while (0)
+#endif
+
+
+/*
+ * HW description: IRQ router
+ *
+ * IMPORTANT NOTE: this hw block is not a "full" interrupt controller
+ * - it does not support edge detection
+ * - it does not latch the inputs (devices are expected to latch their
+ * IRQ output by themselves)
+ *
+ *  ---
+ *
+ *  CPU block interrupt interface is now 32bits.
+ *  The 24 first interrupt bits are generated from the system interrupts
+ *  and the 8 msb interrupts are cpu local interrupts :
+ *
+ *  IRQs [23:0] tango system irqs.
+ *  IRQs [27:24] CPU core cross trigger interface interrupt (1 per core).
+ *  IRQs [31:28] CPU core PMU (performance unit) interrupt (1 per core).
+ *
+ *  The 24 lsb interrupts are generated through a new interrupt map module
+ *  that maps the tango 128 interrupts to those 24 interrupts.
+ *  For each of the 128 input system interrupt, one register is dedicated
+ *  to program the destination interrupt among the 24 available.
+ *  The mapper is configured as follows, starting at address (0x6f800) :
+ *
+ *  offset name            description
+ *  0x000  irq_in_0_cfg    "en"=bit[31]; "inv"=bit[16]; "dest"=bits[4:0]
+ *  0x004  irq_in_1_cfg    "en"=bit[31]; "inv"=bit[16]; "dest"=bits[4:0]
+ *  .
+ *  .
+ *  .
+ *  0x1FC  irq_in_127_cfg  "en"=bit[31]; "inv"=bit[16]; "dest"=bits[4:0]
+ *  0x400  soft_irq_cfg    "enable"=bits[15:0]
+ *  0x404  soft_irq_map0   "map3"=bits[28:24]; "map2"=bits[20:16];
+ * "map1"=bits[12:8]; "map0"=bits[4:0]
+ *  0x408  soft_irq_map1   "map7"=bits[28:24]; "map6"=bits[20:16];
+ * "map5"=bits[12:8]; "map4"=bits[4:0]
+ *  0x40C  soft_irq_map2   "map11"=bits[28:24]; "map10"=bits[20:16];
+ * "map9"=bits[12:8]; "map8"=bits[4:0]
+ *  0x410  soft_irq_map3   "map15"=bits[28:24]; "map14"=bits[20:16];
+ * "map13"=bits[12:8]; "map12"=bits[4:0]
+ *  0x414  soft_irq_set    "set"=bits[15:0]
+ *  0x418  soft_irq_clear  "clear"=bits[15:0]
+ *  0x41C  read_cpu_irq    "cpu_block_irq"=bits[23:0]
+ *  0x420  read_sys_irq0   "system_irq"=bits[31:0]; (irqs: 0->31)
+ *  0x424  read_sys_irq1   "system_irq"=bits[31:0]; (irqs: 32->63)
+ *  0x428  read_sys_irq2   "system_irq"=bits[31:0]; (irqs: 64->95)
+ *  0x42C  read_sys_irq3   "system_irq"=bits[31:0]; (irqs: 96->127)
+ *
+ *  - "irq_in_N_cfg"   : input N mapping :
+ *     - "dest" bits[4:0]    => set destination interrupt among the 24
+ *  output interrupts. (if multiple inputs are mapped to the same output,
+ *  result is an OR of the inputs).
+ *     - "inv" bit[16]       => if set, inverts input interrupt
+ *  polarity (active at 0).
+ *     - "en" bit[31]        => enable interrupt. Acts like a mask on the
+ *  input interrupt.
+ *  - "soft_irq"       : this module supports up to 16 software interrupts.
+ *     - "enable" bits[15:0] => enable usage of software IRQs (SIRQ), 1 bit
+ *  per SIRQ.
+ *  - "soft_irq_mapN"  : For each of the 16 soft IRQ (SIRQ), map them in out
+ *  IRQ[23:0] vector.
+ *     - "mapN"              => 5 bits to select where to connect the SIRQ
+ *  among the 23 bits output IRQ. (if multiple SIRQ are mapped to the same
+ *  output IRQ, result is an OR of those signals).
+ *  - "soft_irq_set"   : 16bits, write 1 bit at one set the corresponding
+ *  SIRQ. Read returns the software SIRQ vector value.
+ *  - "soft_irq_clear" : 16bits, write 1 bit at one clear the corresponding
+ *  software SIRQ. Read returns the software SIRQ vector value.
+ *  - "read_cpu_irq"   : 24bits, returns output IRQ value (IRQs connected to
+ *  the ARM cluster).
+ *  - "read_sys_irqN"  : 32bits, returns input system IRQ value before mapping.
+ */
+
+#define ROUTER_INPUTS  (128)
+#define ROUTER_OUTPUTS (24)
+#define SWIRQ_COUNT    (16)
+
+#define IRQ_ROUTER_ENABLE_MASK (BIT(31))
+#define IRQ_ROUTER_INVERT_MASK (BIT(16))
+
+/* SW irqs */
+#define SWIRQ_ENABLE      (0x400)
+#define SWIRQ_MAP_GROUP0  (0x404)
+#define SWIRQ_MAP_GROUP1  (0x408)
+#define SWIRQ_MAP_GROUP2  (0x40C)
+#define SWIRQ_MAP_GROUP3  (0x410)
+#define READ_SWIRQ_STATUS (0x414)
+
+#define READ_SYS_IRQ_GROUP0 (0x420)
+#define READ_SYS_IRQ_GROUP1 (0x424)
+#define READ_SYS_IRQ_GROUP2 (0x428)
+#define READ_SYS_IRQ_GROUP3 (0x42C)
+
+
+#if 0
+#define SHORT_OR_FULL_NAME full_name
+#else
+#define SHORT_OR_FULL_NAME name
+#endif
+
+#define NODE_NAME(__node__) (__node__ ? __node__->SHORT_OR_FULL_NAME :	\
+			     "<no-node>")
+
+#define BITMASK_VECTOR_SIZE(__count__) (__count__ / 32)
+#define IRQ_TO_OFFSET(__hwirq__) (__hwirq__ * 4)
+
+struct tango_irqrouter;
+
+/*
+ * Maintains the mapping between a Linux virq and a hwirq
+ * on the parent controller.
+ * It is used by tango_irqdomain_map() or tango_irqdomain_hierarchy_alloc()
+ * to setup the route between input IRQ and output IRQ
+ */
+struct tango_irqrouter_output {
+	struct tango_irqrouter *context;
+
+	u32 domain_id;
+
+	u32 hwirq;
+	u32 hwirq_level;
+	u32 virq;
+
+	int shared_count;
+	int *shared_irqs;
+};
+
+/*
+ * Context for the driver
+ */
+struct tango_irqrouter {
+	raw_spinlock_t lock;
+	struct device_node *node;
+	void __iomem *base;
+
+	int input_count;
+	u32 irq_mask[BITMASK_VECTOR_SIZE(ROUTER_INPUTS)];
+	u32 irq_invert_mask[BITMASK_VECTOR_SIZE(ROUTER_INPUTS)];
+
+	int swirq_count;
+	u32 swirq_mask;
+
+	int irqgroup_count;
+	int implicit_groups;
+
+	int output_count;
+	struct tango_irqrouter_output output[ROUTER_OUTPUTS];
+};
+
+
+/******************************************************************************/
+
+/* Register access */
+static inline u32 tango_readl(struct tango_irqrouter *irqrouter,
+			      int reg);
+static inline void tango_writel(struct tango_irqrouter *irqrouter,
+				int reg,
+				u32 val);
+/* IRQ enable */
+static inline void tango_set_swirq_enable(struct tango_irqrouter *irqrouter,
+					  int swirq,
+					  bool enable);
+static inline void tango_set_hwirq_enable(struct tango_irqrouter *irqrouter,
+					  int hwirq,
+					  bool enable);
+static inline int tango_set_irq_enable(struct tango_irqrouter *irqrouter,
+				       int irq_in,
+				       bool enable);
+/* IRQ polarity */
+static inline void tango_set_swirq_inversion(struct tango_irqrouter *irqrouter,
+					     int swirq,
+					     bool invert);
+static inline void tango_set_hwirq_inversion(struct tango_irqrouter *irqrouter,
+					     int hwirq,
+					     bool invert);
+static inline int tango_set_irq_inversion(struct tango_irqrouter *irqrouter,
+					  int irq_in,
+					  bool invert);
+/* IRQ routing */
+static inline void tango_set_swirq_route(struct tango_irqrouter *irqrouter,
+					 int swirq_in,
+					 int irq_out);
+static inline void tango_set_hwirq_route(struct tango_irqrouter *irqrouter,
+					 int hwirq_in,
+					 int irq_out);
+static inline int tango_set_irq_route(struct tango_irqrouter *irqrouter,
+				      int irq_in,
+				      int irq_out);
+/* Misc */
+static inline int tango_set_irq_type(struct tango_irqrouter *irqrouter,
+				     int hwirq_in,
+				     u32 type,
+				     u32 parent_type);
+static int tango_get_output_for_hwirq(struct tango_irqrouter *irqrouter,
+				      int hwirq_in,
+				      struct tango_irqrouter_output **out_val);
+static inline int tango_parse_fwspec(struct irq_domain *domain,
+				     struct irq_fwspec *fwspec,
+				     u32 *domain_id_out,
+				     irq_hw_number_t *irq_out,
+				     u32 *type_out);
+
+
+/* 'irqchip' handling callbacks
+ * Used for 'shared' IRQs, i.e.: IRQs that share a GIC input
+ * This driver performs the IRQ dispatch based on the flags
+ */
+static void tango_irqchip_mask_irq(struct irq_data *data);
+static void tango_irqchip_unmask_irq(struct irq_data *data);
+static int tango_irqchip_set_irq_type(struct irq_data *data,
+				      unsigned int type);
+#ifdef CONFIG_SMP
+static int tango_irqchip_set_irq_affinity(struct irq_data *data,
+					  const struct cpumask *mask_val,
+					  bool force);
+#endif
+static inline u32 tango_dispatch_irqs(struct irq_domain *domain,
+				      struct irq_desc *desc,
+				      u32 status,
+				      int base);
+static void tango_irqdomain_handle_cascade_irq(struct irq_desc *desc);
+
+static struct irq_chip tango_irq_chip_shared_ops = {
+	.name			= "ROUTER_SHARED_IRQ_HANDLER",
+	.irq_mask		= tango_irqchip_mask_irq,
+	.irq_unmask		= tango_irqchip_unmask_irq,
+	.irq_set_type		= tango_irqchip_set_irq_type,
+#ifdef CONFIG_SMP
+	.irq_set_affinity	= tango_irqchip_set_irq_affinity,
+#endif
+};
+
+/* Shared IRQ domain callbacks */
+static int tango_irqdomain_map(struct irq_domain *domain,
+			       unsigned int virq,
+			       irq_hw_number_t hwirq);
+static int tango_irqdomain_translate(struct irq_domain *domain,
+				     struct irq_fwspec *fwspec,
+				     unsigned long *out_hwirq,
+				     unsigned int *out_type);
+static int tango_irqdomain_select(struct irq_domain *domain,
+				  struct irq_fwspec *fwspec,
+				  enum irq_domain_bus_token bus_token);
+
+static struct irq_domain_ops tango_irqdomain_ops = {
+	.select    = tango_irqdomain_select,
+	.translate = tango_irqdomain_translate,
+	.map	   = tango_irqdomain_map,
+};
+
+
+/* 'irqrouter' handling callbacks
+ * Used for 'direct' IRQs, i.e.: IRQs that are directly routed to the GIC
+ * This driver does not dispatch the IRQs, the GIC does.
+ */
+static void tango_irqrouter_mask_irq(struct irq_data *data);
+static void tango_irqrouter_unmask_irq(struct irq_data *data);
+static int tango_irqrouter_set_irq_type(struct irq_data *data,
+					unsigned int type);
+
+static struct irq_chip tango_irq_chip_direct_ops = {
+	.name			= "ROUTER_DIRECT_IRQ_HANDLER",
+	.irq_eoi		= irq_chip_eoi_parent,
+	.irq_mask		= tango_irqrouter_mask_irq,
+	.irq_unmask		= tango_irqrouter_unmask_irq,
+	.irq_retrigger		= irq_chip_retrigger_hierarchy,
+	.irq_set_type		= tango_irqrouter_set_irq_type,
+	.flags			= IRQCHIP_MASK_ON_SUSPEND |
+				  IRQCHIP_SKIP_SET_WAKE,
+#ifdef CONFIG_SMP
+	.irq_set_affinity	= irq_chip_set_affinity_parent,
+#endif
+};
+
+/* Direct IRQ domain callbacks */
+static int tango_irqdomain_hierarchy_alloc(struct irq_domain *domain,
+					   unsigned int virq,
+					   unsigned int nr_irqs,
+					   void *data);
+static void tango_irqdomain_hierarchy_free(struct irq_domain *domain,
+					   unsigned int virq,
+					   unsigned int nr_irqs);
+static int tango_irqdomain_hierarchy_translate(struct irq_domain *domain,
+					       struct irq_fwspec *fwspec,
+					       unsigned long *out_hwirq,
+					       unsigned int *out_type);
+
+static int tango_irqdomain_hierarchy_select(struct irq_domain *domain,
+					    struct irq_fwspec *fwspec,
+					    enum irq_domain_bus_token bus_tok);
+
+static const struct irq_domain_ops tango_irqdomain_hierarchy_ops = {
+	.select    = tango_irqdomain_hierarchy_select,
+	.translate = tango_irqdomain_hierarchy_translate,
+	.alloc	   = tango_irqdomain_hierarchy_alloc,
+	.free	   = tango_irqdomain_hierarchy_free,
+};
+
+
+/******************************************************************************/
+
+
+static inline u32 tango_readl(struct tango_irqrouter *irqrouter,
+			      int reg)
+{
+	u32 val = readl_relaxed(irqrouter->base + reg);
+	/*DBGLOG("r[0x%08x + 0x%08x = 0x%08x] = 0x%08x\n",
+	  irqrouter->base, reg, irqrouter->base + reg, val);*/
+	return val;
+}
+
+
+static inline void tango_writel(struct tango_irqrouter *irqrouter,
+				int reg,
+				u32 val)
+{
+	/*DBGLOG("w[0x%08x + 0x%08x = 0x%08x] = 0x%08x\n",
+	  irqrouter->base, reg, irqrouter->base + reg, val);*/
+	writel_relaxed(val, irqrouter->base + reg);
+}
+
+
+static inline void tango_set_swirq_enable(struct tango_irqrouter *irqrouter,
+					  int swirq,
+					  bool enable)
+{
+	u32 offset = SWIRQ_ENABLE;
+	u32 value = tango_readl(irqrouter, offset);
+	u32 swirq_bit_index = swirq % SWIRQ_COUNT;
+
+#if 1
+	DBGLOG("%smask swirq(in) %d : current regvalue 0x%x\n",
+	       enable ? "un":"",
+	       swirq, value);
+#endif
+
+	if (enable) {
+		/* unmask swirq */
+		irqrouter->swirq_mask |= (1 << swirq_bit_index);
+		value |= (1 << swirq_bit_index);
+	} else {
+		/* mask swirq */
+		irqrouter->swirq_mask &= ~(1 << swirq_bit_index);
+		value &= ~(1 << swirq_bit_index);
+	}
+
+	tango_writel(irqrouter, offset, value);
+}
+
+
+static inline void tango_set_hwirq_enable(struct tango_irqrouter *irqrouter,
+					  int hwirq,
+					  bool enable)
+{
+	u32 offset = IRQ_TO_OFFSET(hwirq);
+	u32 value = tango_readl(irqrouter, offset);
+	u32 hwirq_reg_index = hwirq / 32;
+	u32 hwirq_bit_index = hwirq % 32;
+	u32 *enable_mask = &(irqrouter->irq_mask[hwirq_reg_index]);
+
+#if 1
+	DBGLOG("%smask hwirq(in) %d : current regvalue 0x%x\n",
+	       enable ? "un":"",
+	       hwirq, value);
+#endif
+
+	if (enable) {
+		/* unmask irq */
+		*enable_mask |= (1 << hwirq_bit_index);
+		value |= IRQ_ROUTER_ENABLE_MASK;
+	} else {
+		/* mask irq */
+		*enable_mask &= ~(1 << hwirq_bit_index);
+		value &= ~(IRQ_ROUTER_ENABLE_MASK);
+	}
+
+	tango_writel(irqrouter, offset, value);
+}
+
+
+static inline void tango_set_swirq_inversion(struct tango_irqrouter *irqrouter,
+					     int swirq,
+					     bool invert)
+{
+
+	DBGLOG("swirq(in) %d %s inverted\n", swirq, invert ? "":"not");
+
+	if (invert)
+		DBGERR("SW IRQs cannot be inverted!\n");
+}
+
+
+static inline void tango_set_hwirq_inversion(struct tango_irqrouter *irqrouter,
+					     int hwirq,
+					     bool invert)
+{
+	u32 offset = IRQ_TO_OFFSET(hwirq);
+	u32 value = tango_readl(irqrouter, offset);
+	u32 hwirq_reg_index = hwirq / 32;
+	u32 hwirq_bit_index = hwirq % 32;
+	u32 *invert_mask = &(irqrouter->irq_invert_mask[hwirq_reg_index]);
+
+	if (invert) {
+		*invert_mask |= (1 << hwirq_bit_index);
+		value |= IRQ_ROUTER_INVERT_MASK;
+	} else {
+		*invert_mask &= ~(1 << hwirq_bit_index);
+		value &= ~(IRQ_ROUTER_INVERT_MASK);
+	}
+
+	DBGLOG("hwirq(in) %d %s inverted\n", hwirq, invert ? "":"not");
+
+	tango_writel(irqrouter, offset, value);
+}
+
+
+static inline void tango_set_swirq_route(struct tango_irqrouter *irqrouter,
+					 int swirq_in,
+					 int irq_out)
+{
+	u32 swirq_reg_index = swirq_in / 4;
+	u32 swirq_bit_index = (swirq_in % 4) * 8;
+	u32 mask = ~(0x1f << swirq_bit_index);
+	u32 offset = SWIRQ_MAP_GROUP0 + (swirq_reg_index * 4);
+	u32 value = tango_readl(irqrouter, offset);
+
+	DBGLOG("ri %d, bi %d, mask 0x%x, offset 0x%x, val 0x%x\n",
+	       swirq_reg_index,
+	       swirq_bit_index,
+	       mask,
+	       offset,
+	       value);
+
+	DBGLOG("route swirq %d => hwirq(out) %d\n", swirq_in, irq_out);
+
+	value &= mask;
+
+	if (irq_out < 0) {
+		tango_set_irq_enable(irqrouter,
+				     swirq_in + irqrouter->input_count,
+				     0);
+	} else
+		value |= ((irq_out & 0x1f) << swirq_bit_index);
+
+	tango_writel(irqrouter, offset, value);
+}
+
+
+static inline void tango_set_hwirq_route(struct tango_irqrouter *irqrouter,
+					 int irq_in,
+					 int irq_out)
+{
+	u32 offset = IRQ_TO_OFFSET(irq_in);
+	u32 value;
+
+	DBGLOG("route hwirq(in) %d => hwirq(out) %d\n", irq_in, irq_out);
+
+	if (irq_out < 0) {
+		tango_set_irq_enable(irqrouter,
+				     irq_in,
+				     0);
+		value = 0;
+	} else
+		value = (irq_out & 0x1f);
+
+	tango_writel(irqrouter, offset, value);
+}
+
+
+static inline int tango_set_irq_enable(struct tango_irqrouter *irqrouter,
+				       int irq,
+				       bool enable)
+{
+	if (irq >= (irqrouter->input_count + irqrouter->swirq_count))
+		return -EINVAL;
+	else if (irq >= irqrouter->input_count)
+		tango_set_swirq_enable(irqrouter,
+				       irq - irqrouter->input_count,
+				       enable);
+	else
+		tango_set_hwirq_enable(irqrouter,
+				       irq,
+				       enable);
+	return 0;
+}
+
+
+static inline int tango_set_irq_inversion(struct tango_irqrouter *irqrouter,
+					  int irq_in,
+					  bool invert)
+{
+	if (irq_in >= (irqrouter->input_count + irqrouter->swirq_count))
+		return -EINVAL;
+	else if (irq_in >= irqrouter->input_count)
+		tango_set_swirq_inversion(irqrouter,
+					  irq_in - irqrouter->input_count,
+					  invert);
+	else
+		tango_set_hwirq_inversion(irqrouter,
+					  irq_in,
+					  invert);
+	return 0;
+}
+
+
+static inline int tango_set_irq_route(struct tango_irqrouter *irqrouter,
+				      int irq_in,
+				      int irq_out)
+{
+	if (irq_in >= (irqrouter->input_count + irqrouter->swirq_count))
+		return -EINVAL;
+	else if (irq_in >= irqrouter->input_count)
+		tango_set_swirq_route(irqrouter,
+				      irq_in - irqrouter->input_count,
+				      irq_out);
+	else
+		tango_set_hwirq_route(irqrouter,
+				      irq_in,
+				      irq_out);
+	return 0;
+}
+
+
+static int tango_set_irq_type(struct tango_irqrouter *irqrouter,
+			      int hwirq_in,
+			      u32 type,
+			      u32 parent_type)
+{
+	int err;
+
+	if (parent_type & (type & IRQ_TYPE_SENSE_MASK))
+		/* same polarity */
+		err = tango_set_irq_inversion(irqrouter, hwirq_in, 0);
+	else
+		/* invert polarity */
+		err = tango_set_irq_inversion(irqrouter, hwirq_in, 1);
+
+	if (err < 0) {
+		DBGWARN("Failed to setup IRQ %d polarity\n", hwirq_in);
+		return err;
+	}
+
+	switch (type & IRQ_TYPE_SENSE_MASK) {
+	case IRQ_TYPE_EDGE_RISING:
+	case IRQ_TYPE_EDGE_FALLING:
+		DBGERR("Does not support edge triggers\n");
+		break;
+	case IRQ_TYPE_LEVEL_HIGH:
+		break;
+	case IRQ_TYPE_LEVEL_LOW:
+		break;
+	default:
+		DBGWARN("Invalid trigger mode 0x%x for hwirq(in) %d\n",
+			type, hwirq_in);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+
+static int tango_get_output_for_hwirq(struct tango_irqrouter *irqrouter,
+				      int hwirq_in,
+				      struct tango_irqrouter_output **out_val)
+{
+	struct tango_irqrouter_output *irqrouter_output;
+	int i;
+
+	if (!out_val)
+		return -EINVAL;
+
+	/* Get the irqrouter_output for the hwirq */
+	for (i = 0; i < irqrouter->output_count; i++) {
+		int j;
+
+		irqrouter_output = &(irqrouter->output[i]);
+
+		for (j = 0; j < irqrouter_output->shared_count; j++) {
+			if (hwirq_in == irqrouter_output->shared_irqs[j])
+				goto found_router_output;
+		}
+	}
+	if (i == irqrouter->output_count) {
+		DBGWARN("Couldn't find hwirq mapping\n");
+		return -ENODEV;
+	}
+
+found_router_output:
+
+	*out_val = irqrouter_output;
+	return 0;
+}
+
+static inline int tango_parse_fwspec(struct irq_domain *domain,
+				     struct irq_fwspec *fwspec,
+				     u32 *domain_id_out,
+				     irq_hw_number_t *irq_out,
+				     u32 *type_out)
+{
+#if 0
+	for (i = 0; i < fwspec->param_count; i++)
+		DBGLOG("[%d] 0x%x\n", i, fwspec->param[i]);
+#endif
+
+	if (!is_of_node(fwspec->fwnode)) {
+		DBGWARN("%s:%s(0x%p): Parameter mismatch\n",
+			NODE_NAME(irq_domain_get_of_node(domain)),
+			domain->name,
+			domain);
+		return -EINVAL;
+	}
+
+	if (fwspec->fwnode != domain->fwnode) {
+		DBGLOG("Unknown domain/node\n");
+		return -EINVAL;
+	}
+
+	if (fwspec->param_count != 3) {
+		DBGWARN("We need 3 params\n");
+		return -EINVAL;
+	}
+
+	if (domain_id_out)
+		*domain_id_out = fwspec->param[0];
+	if (irq_out)
+		*irq_out       = fwspec->param[1];
+	if (type_out)
+		*type_out      = fwspec->param[2];
+
+	return 0;
+}
+
+
+/* 'irqchip' handling callbacks
+ * Used for 'shared' IRQs, i.e.: IRQs that share a GIC input
+ * This driver performs the IRQ dispatch based on the flags
+ */
+
+
+static void tango_irqchip_mask_irq(struct irq_data *data)
+{
+	struct irq_domain *domain = irq_data_get_irq_chip_data(data);
+	struct tango_irqrouter_output *irqrouter_output = domain->host_data;
+	struct tango_irqrouter *irqrouter = irqrouter_output->context;
+	int hwirq_in = (int)data->hwirq;
+
+	tango_set_irq_enable(irqrouter, hwirq_in, 0);
+}
+
+
+static void tango_irqchip_unmask_irq(struct irq_data *data)
+{
+	struct irq_domain *domain = irq_data_get_irq_chip_data(data);
+	struct tango_irqrouter_output *irqrouter_output = domain->host_data;
+	struct tango_irqrouter *irqrouter = irqrouter_output->context;
+	int hwirq_in = (int)data->hwirq;
+
+	tango_set_irq_enable(irqrouter, hwirq_in, 1);
+}
+
+
+static int tango_irqchip_set_irq_type(struct irq_data *data,
+				      unsigned int type)
+{
+	struct irq_domain *domain = irq_data_get_irq_chip_data(data);
+	struct tango_irqrouter_output *irqrouter_output = domain->host_data;
+	struct tango_irqrouter *irqrouter = irqrouter_output->context;
+	int hwirq_in = (int)data->hwirq;
+	u32 parent_type;
+
+	DBGLOG("%s:%s(0x%p) type 0x%x for hwirq(in) %d = virq %d "
+	       "(routed to hwirq(out) %d)\n",
+	       NODE_NAME(irq_domain_get_of_node(domain)),
+	       domain->name,
+	       domain,
+	       type, hwirq_in, data->irq,
+	       irqrouter_output->hwirq);
+
+	parent_type = (irqrouter_output->hwirq_level & IRQ_TYPE_SENSE_MASK);
+
+	return tango_set_irq_type(irqrouter, hwirq_in, type, parent_type);
+}
+
+
+#ifdef CONFIG_SMP
+static int tango_irqchip_set_irq_affinity(struct irq_data *data,
+					  const struct cpumask *mask_val,
+					  bool force)
+{
+	struct irq_domain *domain = irq_data_get_irq_chip_data(data);
+	struct tango_irqrouter_output *irqrouter_output = domain->host_data;
+	struct irq_chip *parent_chip = irq_get_chip(irqrouter_output->virq);
+	struct irq_data *parent_data = irq_get_irq_data(irqrouter_output->virq);
+
+	DBGLOG("%s:%s(0x%p)\n",
+	       NODE_NAME(irq_domain_get_of_node(domain)), domain->name, domain);
+
+	if (parent_chip && parent_chip->irq_set_affinity)
+		return parent_chip->irq_set_affinity(parent_data,
+						     mask_val,
+						     force);
+	else
+		return -EINVAL;
+}
+#endif
+
+
+static inline u32 tango_dispatch_irqs(struct irq_domain *domain,
+				      struct irq_desc *desc,
+				      u32 status,
+				      int base)
+{
+	u32 hwirq;
+	u32 virq;
+
+	while (status) {
+		hwirq = __ffs(status);
+		virq = irq_find_mapping(domain, base + hwirq);
+		if (unlikely(!virq))
+			handle_bad_irq(desc);
+		else
+			generic_handle_irq(virq);
+
+		status &= ~BIT(hwirq);
+	}
+
+	return status;
+}
+
+
+static void tango_irqdomain_handle_cascade_irq(struct irq_desc *desc)
+{
+	struct irq_domain *domain = irq_desc_get_handler_data(desc);
+	struct tango_irqrouter_output *irqrouter_output = domain->host_data;
+	struct tango_irqrouter *irqrouter = irqrouter_output->context;
+	struct irq_chip *host_chip = irq_desc_get_chip(desc);
+	u32 i, status;
+	u32 swirq_status, irq_status[BITMASK_VECTOR_SIZE(ROUTER_INPUTS)];
+
+#if 0
+	DBGLOG("%s:%s(0x%p): irqrouter_output 0x%p, hwirq(out) %d\n",
+	       NODE_NAME(irq_domain_get_of_node(domain)), domain->name, domain,
+	       irqrouter_output, irqrouter_output->hwirq);
+#endif
+
+	chained_irq_enter(host_chip, desc);
+
+	raw_spin_lock(&(irqrouter->lock));
+	swirq_status = tango_readl(irqrouter, READ_SWIRQ_STATUS);
+	for (i = 0; i < BITMASK_VECTOR_SIZE(ROUTER_INPUTS); i++)
+		irq_status[i] = tango_readl(irqrouter,
+					    READ_SYS_IRQ_GROUP0 + i*4);
+	raw_spin_unlock(&(irqrouter->lock));
+
+	/* HW irqs */
+	for (i = 0; i < BITMASK_VECTOR_SIZE(ROUTER_INPUTS); i++) {
+#if 0
+		DBGLOG("%d: 0x%08x (en 0x%08x, inv 0x%08x)\n",
+		       i,
+		       irq_status[i],
+		       irqrouter->irq_mask[0],
+		       irqrouter->irq_invert_mask[0]);
+#endif
+
+#define HANDLE_INVERTED_LINES(__irqstatus__, __x__) ((((~__irqstatus__) & irqrouter->irq_invert_mask[__x__]) & irqrouter->irq_mask[__x__]) | __irqstatus__)
+#define HANDLE_EN_AND_INV_MASKS(__irqstatus__, __y__) (HANDLE_INVERTED_LINES(__irqstatus__, __y__) & irqrouter->irq_mask[__y__])
+
+		irq_status[i] = HANDLE_EN_AND_INV_MASKS(irq_status[i], i);
+		status = tango_dispatch_irqs(domain, desc, irq_status[i], i*32);
+		if (status & irq_status[i])
+			DBGERR("%s: %d unhandled IRQs (as a mask) 0x%x\n",
+			       NODE_NAME(irq_domain_get_of_node(domain)),
+			       i,
+			       status & irq_status[i]);
+	}
+
+	/* SW irqs */
+	swirq_status &= irqrouter->swirq_mask;
+	status = tango_dispatch_irqs(domain, desc, swirq_status, 128);
+	if (status & swirq_status)
+		DBGERR("%s: Unhandled IRQs (as a mask) 0x%x\n",
+		       NODE_NAME(irq_domain_get_of_node(domain)),
+		       status & swirq_status);
+
+	chained_irq_exit(host_chip, desc);
+}
+
+
+/**
+ * tango_irqdomain_map - route a hwirq(in) to a hwirq(out).
+ * NOTE: The hwirq(out) must have been already allocated and enabled on
+ * the parent controller.
+ * @hwirq: HW IRQ of the device requesting an IRQ
+ * if hwirq > inputs it is a SW IRQ
+ * @virq: Linux IRQ (associated to the domain) to be given to the device
+ * @domain: IRQ domain (from the domain, we get the irqrouter_output
+ * in order to know to which output we need to route hwirq to)
+ */
+static int tango_irqdomain_map(struct irq_domain *domain,
+			       unsigned int virq,
+			       irq_hw_number_t hwirq)
+{
+	struct tango_irqrouter_output *irqrouter_output = domain->host_data;
+	struct tango_irqrouter *irqrouter = irqrouter_output->context;
+
+	DBGLOG("%s:%s(0x%p): hwirq(in) %d := virq %d, and route "
+	       "hwirq(in) %d => hwirq(out) %d (virq %d)\n",
+	       NODE_NAME(irq_domain_get_of_node(domain)),
+	       domain->name,
+	       domain,
+	       (u32)hwirq,
+	       virq,
+	       (u32)hwirq,
+	       irqrouter_output->hwirq,
+	       irqrouter_output->virq);
+
+	if (hwirq >= (irqrouter->input_count + irqrouter->swirq_count))
+		DBGERR("%s: Invalid hwirq(in) %d >= %d + %d\n",
+		       NODE_NAME(irq_domain_get_of_node(domain)),
+		       (u32)hwirq,
+		       irqrouter->input_count,
+		       irqrouter->swirq_count);
+	else if (hwirq >= irqrouter->input_count)
+		DBGLOG("Map swirq %ld\n", hwirq - irqrouter->input_count);
+
+	irq_set_chip_and_handler(virq,
+				 &tango_irq_chip_shared_ops,
+				 handle_level_irq);
+	irq_set_chip_data(virq, domain);
+	irq_set_probe(virq);
+
+	tango_set_irq_route(irqrouter, hwirq, irqrouter_output->hwirq);
+
+	return 0;
+}
+
+
+/**
+ * tango_irqdomain_translate - used to select the domain for a given
+ * irq_fwspec
+ * @domain: a registered domain
+ * @fwspec: an IRQ specification. This callback is used to translate the
+ * parameters given as irq_fwspec into a HW IRQ and Type values.
+ * @out_hwirq:
+ * @out_type:
+ */
+static int tango_irqdomain_translate(struct irq_domain *domain,
+				     struct irq_fwspec *fwspec,
+				     unsigned long *out_hwirq,
+				     unsigned int *out_type)
+{
+	struct tango_irqrouter_output *irqrouter_output = domain->host_data;
+	struct tango_irqrouter *irqrouter = irqrouter_output->context;
+	irq_hw_number_t irq;
+	u32 domain_id, type;
+	int err;
+
+	DBGLOG("%s:%s(0x%p): argc %d for hwirq(out) %d\n",
+	       NODE_NAME(irq_domain_get_of_node(domain)),
+	       domain->name,
+	       domain,
+	       fwspec->param_count,
+	       irqrouter_output->hwirq);
+
+	err = tango_parse_fwspec(domain,
+				 fwspec,
+				 &domain_id,
+				 &irq,
+				 &type);
+	if (err < 0) {
+		DBGWARN("Failed to parse fwspec\n");
+		return err;
+	}
+
+	switch (domain_id) {
+	case SIGMA_HWIRQ:
+		DBGLOG("Request is for SIGMA_HWIRQ\n");
+		break;
+	case SIGMA_SWIRQ:
+		DBGLOG("Request is for SIGMA_SWIRQ\n");
+		irq += irqrouter->input_count;
+		break;
+	default:
+		DBGLOG("Request is for domain ID 0x%x (we are 0x%x)\n",
+		       domain_id,
+		       irqrouter_output->domain_id);
+		break;
+	};
+
+	*out_hwirq = irq;
+	*out_type  = type & IRQ_TYPE_SENSE_MASK;
+
+	DBGLOG("hwirq %d type 0x%x\n", (u32)*out_hwirq, (u32)*out_type);
+
+	return 0;
+}
+
+
+/**
+ * tango_irqdomain_select - used to select the domain for a given irq_fwspec
+ * @domain: a registered domain
+ * @fwspec: an IRQ specification. This callback should return zero if the
+ * irq_fwspec does not belong to the given domain. If it does, it should
+ * return non-zero.
+ *
+ * In practice it will return non-zero if the irq_fwspec matches one of the
+ * IRQs shared within the given domain.
+ * @bus_token: a bus token
+ */
+static int tango_irqdomain_select(struct irq_domain *domain,
+				  struct irq_fwspec *fwspec,
+				  enum irq_domain_bus_token bus_token)
+{
+	struct tango_irqrouter_output *irqrouter_output = domain->host_data;
+	struct tango_irqrouter *irqrouter = irqrouter_output->context;
+	irq_hw_number_t irq;
+	u32 domain_id, type;
+	int err;
+
+	DBGLOG("%s:%s(0x%p): argc %d, 0x%p, bus 0x%x\n",
+	       NODE_NAME(irq_domain_get_of_node(domain)), domain->name, domain,
+	       fwspec->param_count, fwspec->fwnode, bus_token);
+
+	DBGLOG("router 0x%p, output 0x%p\n", irqrouter, irqrouter_output);
+
+	err = tango_parse_fwspec(domain, fwspec, &domain_id, &irq, &type);
+	if (err < 0)
+		return 0;
+
+	switch (domain_id) {
+	case SIGMA_HWIRQ:
+		DBGLOG("Request is for SIGMA_HWIRQ\n");
+		break;
+	case SIGMA_SWIRQ:
+		DBGLOG("Request is for SIGMA_SWIRQ\n");
+		break;
+	default:
+		DBGLOG("Request is for domain ID 0x%x (we are 0x%x)\n",
+		       domain_id,
+		       irqrouter_output->domain_id);
+		break;
+	};
+
+	if (!irqrouter->implicit_groups) {
+		int i;
+
+		/* Check if the requested IRQ belongs to those listed
+		 * to be sharing the output assigned to this domain
+		 */
+		if (irqrouter_output->shared_count <= 0) {
+			DBGLOG("Not shared IRQ line?\n");
+			return 0;
+		}
+
+		for (i = 0; i < irqrouter_output->shared_count; i++) {
+			if (irq == irqrouter_output->shared_irqs[i]) {
+				DBGLOG("Match: IRQ %lu\n", irq);
+				return 1;
+			}
+		}
+	} else {
+		/* Otherwise, check if the domain_id given matches
+		 * the one assigned to this output
+		 */
+		if (domain_id == irqrouter_output->domain_id) {
+			DBGLOG("Match: Domain ID %d\n", domain_id);
+			return 1;
+		}
+	}
+
+	return 0;
+}
+
+
+/* 'irqrouter' handling callbacks
+ * Used for 'direct' IRQs, i.e.: IRQs that are directly routed to the GIC
+ * This driver does not dispatch the IRQs, the GIC does.
+ */
+
+
+static void tango_irqrouter_mask_irq(struct irq_data *data)
+{
+	struct irq_domain *domain = irq_data_get_irq_chip_data(data);
+	struct tango_irqrouter *irqrouter = domain->host_data;
+	int hwirq_in = (int)data->hwirq;
+
+	DBGLOG("%s:%s(0x%p) hwirq(in) %d\n",
+	       NODE_NAME(irq_domain_get_of_node(domain)),
+	       domain->name,
+	       domain,
+	       hwirq_in);
+
+	tango_set_irq_enable(irqrouter, hwirq_in, 0);
+
+	irq_chip_mask_parent(data);
+}
+
+
+static void tango_irqrouter_unmask_irq(struct irq_data *data)
+{
+	struct irq_domain *domain = irq_data_get_irq_chip_data(data);
+	struct tango_irqrouter *irqrouter = domain->host_data;
+	int hwirq_in = (int)data->hwirq;
+
+	DBGLOG("%s:%s(0x%p) hwirq(in) %d\n",
+	       NODE_NAME(irq_domain_get_of_node(domain)),
+	       domain->name,
+	       domain,
+	       hwirq_in);
+
+	tango_set_irq_enable(irqrouter, hwirq_in, 1);
+
+	irq_chip_unmask_parent(data);
+}
+
+
+static int tango_irqrouter_set_irq_type(struct irq_data *data,
+					unsigned int type)
+{
+	struct irq_domain *domain = irq_data_get_irq_chip_data(data);
+	struct tango_irqrouter_output *irqrouter_output = NULL;
+	struct tango_irqrouter *irqrouter = domain->host_data;
+	int hwirq_in = (int)data->hwirq;
+	u32 parent_type;
+
+	DBGLOG("%s:%s(0x%p) type 0x%x for hwirq(in) %d\n",
+	       NODE_NAME(irq_domain_get_of_node(domain)),
+	       domain->name,
+	       domain,
+	       type,
+	       hwirq_in);
+
+	tango_get_output_for_hwirq(irqrouter, hwirq_in, &irqrouter_output);
+	if (!irqrouter_output)
+		goto handle_parent;
+
+	parent_type = (irqrouter_output->hwirq_level & IRQ_TYPE_SENSE_MASK);
+	tango_set_irq_type(irqrouter, hwirq_in, type, parent_type);
+
+handle_parent:
+	return irq_chip_set_type_parent(data, type);
+}
+
+
+/**
+ * tango_irqdomain_hierarchy_alloc - map/reserve a router<->GIC connection
+ * @domain: IRQ domain.
+ * @virq: Linux IRQ (associated to the domain) to be given to the device.
+ * @nr_irqs: number of IRQs to reserve. MUST BE 1.
+ * @data: (of type 'struct irq_fwspec *') the HW IRQ requested:
+ * if in [0, input_count)
+ *    => HW IRQ.
+ * if in [input_count, input_count+swirq_count)
+ *    => SW IRQ.
+ * if in [input_count+swirq_count, input_count+swirq_count+irqgroup_count)
+ *    => Fake HW IRQ.
+ */
+static int tango_irqdomain_hierarchy_alloc(struct irq_domain *domain,
+					   unsigned int virq,
+					   unsigned int nr_irqs,
+					   void *data)
+{
+	struct tango_irqrouter *irqrouter = domain->host_data;
+	struct irq_fwspec *fwspec = data;
+	struct irq_fwspec fwspec_out;
+	irq_hw_number_t hwirq_in, hwirq_out;
+	u32 hwirq_type_in, hwirq_type_out;
+	u32 domain_id_in;
+	int i, err;
+
+	DBGLOG("%s:%s(0x%p), parent %s:%s(0x%p): virq %d nr_irqs %d, argc %d\n",
+	       NODE_NAME(irq_domain_get_of_node(domain)), domain->name, domain,
+	       NODE_NAME(irq_domain_get_of_node(domain->parent)),
+	       domain->parent->name, domain->parent,
+	       virq, nr_irqs, fwspec->param_count);
+
+	if (!irq_domain_get_of_node(domain->parent)) {
+		DBGWARN("Invalid params\n");
+		return -EINVAL;
+	}
+
+	if (nr_irqs != 1) {
+		DBGWARN("IRQ ranges not handled\n");
+		return -EINVAL;
+	}
+
+	/* Requested hwirq */
+	err = tango_parse_fwspec(domain,
+				 fwspec,
+				 &domain_id_in,
+				 &hwirq_in,
+				 &hwirq_type_in);
+	if (err < 0) {
+		DBGWARN("Failed to parse fwspec\n");
+		return err;
+	}
+
+	/* Only handle HW IRQ requests.
+	 * SW IRQs are all shared and belong to another domain.
+	 */
+	switch (domain_id_in) {
+	case SIGMA_HWIRQ:
+		DBGLOG("Request is for SIGMA_HWIRQ\n");
+		break;
+	case SIGMA_SWIRQ:
+		DBGLOG("Request is for SIGMA_SWIRQ\n");
+	default:
+		DBGWARN("Unhandled domain ID 0x%x\n", domain_id_in);
+		return -EINVAL;
+	};
+
+	/* Find a route */
+	raw_spin_lock(&(irqrouter->lock));
+	for (i = irqrouter->output_count - 1; i >= 0; i--) {
+		if (irqrouter->output[i].context == NULL)
+			break;
+	}
+	raw_spin_unlock(&(irqrouter->lock));
+
+	if (i < 0) {
+		DBGWARN("No more IRQ output lines free\n");
+		return -ENODEV;
+	}
+
+	/* Request our parent controller (the GIC) an IRQ line for the
+	 * chosen route
+	 */
+	hwirq_out      = i;
+	hwirq_type_out = IRQ_TYPE_LEVEL_HIGH;
+
+	fwspec_out.fwnode = domain->parent->fwnode; /* should be the GIC */
+	fwspec_out.param_count = 3;
+	fwspec_out.param[0] = GIC_SPI;
+	fwspec_out.param[1] = hwirq_out;
+	fwspec_out.param[2] = hwirq_type_out;
+
+	err = irq_domain_alloc_irqs_parent(domain, virq, 1, &fwspec_out);
+	if (err) {
+		DBGWARN("Failed to allocate irq on parent\n");
+		return err;
+	}
+
+	/* Setup the route's output context */
+	irqrouter->output[hwirq_out].context     = irqrouter;
+	irqrouter->output[hwirq_out].hwirq       = hwirq_out;
+	irqrouter->output[hwirq_out].hwirq_level = hwirq_type_out;
+	irqrouter->output[hwirq_out].virq        = virq;
+
+	if (hwirq_in >= irqrouter->input_count + irqrouter->swirq_count) {
+		DBGLOG("Fake hwirq(in) %d for shared IRQ line hwirq(out) %d\n",
+		       (int)hwirq_in, (int)hwirq_out);
+
+		/* It is shared but we don't know yet how many IRQ
+		 * lines share this output
+		 */
+		irqrouter->output[hwirq_out].shared_count = -1;
+	} else {
+		DBGLOG("hwirq(in) %d = virq %d routed to hwirq(out) %d\n",
+		       (int)hwirq_in, virq, (int)hwirq_out);
+
+		tango_set_irq_route(irqrouter, hwirq_in, hwirq_out);
+		/* Not shared */
+		irqrouter->output[hwirq_out].shared_count = 0;
+	}
+
+	/* Setup the handler ops for this IRQ line (virq)
+	 * Since the IRQ line is allocated and handled by the GIC,
+	 * most ops are generic, although we do need to intercept
+	 * a few of them.
+	 */
+	irq_domain_set_hwirq_and_chip(domain,
+				      virq,
+				      hwirq_in,
+				      &tango_irq_chip_direct_ops,
+				      domain);
+
+	return 0;
+}
+
+
+/**
+ * tango_irqdomain_hierarchy_free - map/reserve a router<->GIC connection
+ * @domain: domain of irq to unmap
+ * @virq: virq number
+ * @nr_irqs: number of irqs to free. MUST BE 1.
+ */
+static void tango_irqdomain_hierarchy_free(struct irq_domain *domain,
+					   unsigned int virq,
+					   unsigned int nr_irqs)
+{
+	struct tango_irqrouter *irqrouter = domain->host_data;
+	struct irq_data *irqdata;
+
+	DBGLOG("%s:%s(0x%p): virq %d nr_irqs %d\n",
+	       NODE_NAME(irq_domain_get_of_node(domain)), domain->name, domain,
+	       virq, nr_irqs);
+
+	if (nr_irqs != 1) {
+		DBGERR("IRQ ranges not handled\n");
+		return;
+	}
+
+	raw_spin_lock(&(irqrouter->lock));
+
+	irqdata = irq_domain_get_irq_data(domain, virq);
+	if (irqdata) {
+		DBGLOG("Freeing virq %d: was routed to hwirq(out) %d\n",
+		       (int)virq,
+		       (int)irqdata->hwirq);
+
+		tango_set_irq_route(irqrouter, 0x0, irqdata->hwirq);
+		irqrouter->output[irqdata->hwirq].context = NULL;
+		irq_domain_reset_irq_data(irqdata);
+	} else
+		DBGERR("Failed to get irq_data for virq %d\n", virq);
+
+	raw_spin_unlock(&(irqrouter->lock));
+}
+
+
+/**
+ * tango_irqdomain_hierarchy_translate - used to select the domain for a given
+ * irq_fwspec
+ * @domain: a registered domain
+ * @fwspec: an IRQ specification. This callback is used to translate the
+ * parameters given as irq_fwspec into a HW IRQ and Type values.
+ * @out_hwirq:
+ * @out_type:
+ */
+static int tango_irqdomain_hierarchy_translate(struct irq_domain *domain,
+					       struct irq_fwspec *fwspec,
+					       unsigned long *out_hwirq,
+					       unsigned int *out_type)
+{
+	irq_hw_number_t irq;
+	u32 domain_id, type;
+	int err;
+
+	DBGLOG("%s:%s(0x%p): argc %d\n",
+	       NODE_NAME(irq_domain_get_of_node(domain)),
+	       domain->name,
+	       domain,
+	       fwspec->param_count);
+
+	err = tango_parse_fwspec(domain,
+				 fwspec,
+				 &domain_id,
+				 &irq,
+				 &type);
+	if (err < 0)
+		return err;
+
+	switch (domain_id) {
+	case SIGMA_HWIRQ:
+		DBGLOG("Request is for SIGMA_HWIRQ\n");
+		break;
+	case SIGMA_SWIRQ:
+		DBGLOG("Request is for SIGMA_SWIRQ\n");
+	default:
+		DBGWARN("Request is for domain ID 0x%x\n",
+			domain_id);
+		break;
+	};
+
+	*out_hwirq = irq;
+	*out_type  = type & IRQ_TYPE_SENSE_MASK;
+
+	DBGLOG("hwirq %d type 0x%x\n", (u32)*out_hwirq, (u32)*out_type);
+
+	return 0;
+}
+
+
+/**
+ * tango_irqdomain_hierarchy_select - used to select the domain for a given
+ * irq_fwspec
+ * @domain: a registered domain
+ * @fwspec: an IRQ specification. This callback should return zero if the
+ * irq_fwspec does not belong to the given domain. If it does, it should
+ * return non-zero.
+ * @bus_token: a bus token
+ */
+static int tango_irqdomain_hierarchy_select(struct irq_domain *domain,
+					    struct irq_fwspec *fwspec,
+					    enum irq_domain_bus_token bus_token)
+{
+	struct tango_irqrouter *irqrouter = domain->host_data;
+	irq_hw_number_t irq;
+	u32 domain_id, type;
+	int err;
+
+	DBGLOG("%s:%s(0x%p): argc %d, 0x%p, bus 0x%x\n",
+	       NODE_NAME(irq_domain_get_of_node(domain)), domain->name, domain,
+	       fwspec->param_count, fwspec->fwnode, bus_token);
+
+	DBGLOG("router 0x%p\n", irqrouter);
+
+	err = tango_parse_fwspec(domain,
+				 fwspec,
+				 &domain_id,
+				 &irq,
+				 &type);
+	if (err < 0)
+		return 0;
+
+	/* Only handle HW IRQ requests.
+	 * SW IRQs are all shared and belong to another domain.
+	 */
+	switch (domain_id) {
+	case SIGMA_HWIRQ:
+		DBGLOG("Request is for SIGMA_HWIRQ\n");
+		break;
+	case SIGMA_SWIRQ:
+		DBGLOG("Request is for SIGMA_SWIRQ\n");
+	default:
+		DBGWARN("Unhandled domain ID 0x%x\n", domain_id);
+		return 0;
+	};
+
+	if (memcmp(fwspec->fwnode,
+		   &(irqrouter->node->fwnode),
+		   sizeof(struct fwnode_handle)) == 0) {
+		DBGLOG("Match: fwnode\n");
+		return 1;
+	}
+
+	return 0;
+}
+
+
+static int __init tango_irq_init_domain(struct tango_irqrouter *irqrouter,
+					u32 index,
+					u32 domain_id,
+					struct device_node *parent,
+					struct device_node *node)
+{
+	struct irq_domain *domain;
+	struct irq_fwspec fwspec_irq;
+	u32 virq, hwirq, hwirq_type, i;
+	u32 total_irqs;
+	u32 entry_size;
+	const __be32 *irqgroup;
+
+	if (index >= irqrouter->irqgroup_count) {
+		DBGWARN("%s: Group count mismatch\n", node->name);
+		return -EINVAL;
+	}
+
+	if (!parent) {
+		DBGWARN("%s: Invalid parent\n", node->name);
+		return -EINVAL;
+	}
+
+	/* The number of IRQs could be dependent on the domain_id but
+	 * would require more code and could make it difficult to handle
+	 * implicit and explicit domains
+	 */
+	total_irqs = irqrouter->input_count + irqrouter->swirq_count;
+
+	switch (domain_id) {
+	case SIGMA_HWIRQ:
+	case SIGMA_SWIRQ:
+		break;
+	default:
+		if (!irqrouter->implicit_groups) {
+			DBGWARN("%s: Unhandled domain ID 0x%x\n",
+				node->name,
+				domain_id);
+			return -EINVAL;
+		}
+
+		DBGLOG("%s: Domain ID 0x%x\n", node->name, domain_id);
+		break;
+	};
+
+	/* To request a virq we need a HW IRQ, use a "Fake HW IRQ" */
+	hwirq      = index + irqrouter->input_count + irqrouter->swirq_count;
+	hwirq_type = IRQ_TYPE_LEVEL_HIGH;
+
+	fwspec_irq.fwnode      = &(parent->fwnode);
+	fwspec_irq.param_count = 3;
+	fwspec_irq.param[0]    = SIGMA_HWIRQ;
+	fwspec_irq.param[1]    = hwirq;
+	fwspec_irq.param[2]    = hwirq_type;
+
+	/* Request a virq for the hwirq */
+	virq = irq_create_fwspec_mapping(&fwspec_irq);
+	if (virq <= 0) {
+		DBGWARN("%s: failed to get virq for hwirq(out) %d",
+			node->name, hwirq);
+		return -ENODEV;
+	}
+
+	/* Get the irqrouter_output for the virq */
+	for (i = 0; i < irqrouter->output_count; i++) {
+		if (virq == irqrouter->output[i].virq)
+			break;
+	}
+	if (i == irqrouter->output_count) {
+		DBGWARN("%s: Couldn't find virq<=>hwirq(out) mapping\n",
+			node->name);
+		return -ENODEV;
+	}
+
+	index = i;
+
+	irqrouter->output[index].domain_id = domain_id;
+
+	/* Create a domain for this virq */
+	domain = irq_domain_add_linear(parent,
+				       total_irqs,
+				       &tango_irqdomain_ops,
+				       &(irqrouter->output[index]));
+	if (!domain) {
+		DBGERR("%s: Failed to create irqdomain", node->name);
+		return -EINVAL;
+	}
+
+	domain->name = kasprintf(GFP_KERNEL,
+				 "irqdomain%d@hwirq_out=%d",
+				 index,
+				 irqrouter->output[index].hwirq);
+
+	DBGLOG("%s:%s(0x%p) [%d], id 0x%x, %d irqs, irqrouter_output 0x%p : "
+	       "hwirq(out) %d = virq %d\n",
+	       NODE_NAME(irq_domain_get_of_node(domain)),
+	       domain->name,
+	       domain,
+	       index,
+	       domain_id,
+	       total_irqs,
+	       &(irqrouter->output[index]),
+	       irqrouter->output[index].hwirq,
+	       virq);
+
+	/* Populate list of shared IRQs */
+
+	if (domain_id == SIGMA_SWIRQ) {
+		irqrouter->output[index].shared_irqs = kcalloc(total_irqs,
+							       sizeof(int),
+							       GFP_KERNEL);
+		if (!irqrouter->output[index].shared_irqs) {
+			DBGERR("%s: Failed to allocate memory for group\n",
+			       node->name);
+			return -ENOMEM;
+		}
+		irqrouter->output[index].shared_count = total_irqs;
+
+		for (i = 0; i < total_irqs; i++)
+			irqrouter->output[index].shared_irqs[i] = i;
+	}
+
+	irqgroup = of_get_property(node, "shared-irqs", &entry_size);
+	if (irqgroup) {
+		int entry;
+
+		entry_size /= sizeof(__be32);
+
+		irqrouter->output[index].shared_irqs = kcalloc(entry_size,
+							       sizeof(int),
+							       GFP_KERNEL);
+		if (!irqrouter->output[index].shared_irqs) {
+			DBGERR("%s: Failed to allocate memory for group\n",
+			       node->name);
+			return -ENOMEM;
+		}
+
+		irqrouter->output[index].shared_count = entry_size;
+
+		for (i = 0; i < entry_size; i++) {
+			of_property_read_u32_index(node,
+						   "shared-irqs",
+						   i,
+						   &entry);
+
+			irqrouter->output[index].shared_irqs[i] = entry;
+
+			DBGLOG("%s:%s(0x%p) irq %d sharing hwirq(out) %d\n",
+			       NODE_NAME(irq_domain_get_of_node(domain)),
+			       domain->name,
+			       domain,
+			       entry,
+			       irqrouter->output[index].hwirq);
+		}
+	}
+
+
+	/* Associate the domain with the virq */
+	irq_set_chained_handler_and_data(virq,
+					 tango_irqdomain_handle_cascade_irq,
+					 domain);
+
+	return 0;
+}
+
+
+static int __init tango_of_irq_init(struct device_node *node,
+				    struct device_node *parent)
+{
+	struct irq_domain *parent_domain, *domain;
+	struct tango_irqrouter *irqrouter;
+	struct device_node *child;
+	void __iomem *base;
+	u32 i, total_irqs;
+	int input_count, swirq_count, output_count;
+	int irqgroup_count;
+	int implicit_groups = 0;
+
+	if (!parent) {
+		DBGERR("%s: Missing parent\n", node->full_name);
+		return -ENODEV;
+	}
+
+	parent_domain = irq_find_host(parent);
+	if (!parent_domain) {
+		DBGERR("%s: Cannot get parent domain\n", node->full_name);
+		return -ENXIO;
+	}
+
+	base = of_iomap(node, 0);
+	if (!base) {
+		DBGERR("%s: Failed to map registers\n", node->name);
+		return -ENXIO;
+	}
+
+	if (of_property_read_u32(node, "inputs", &input_count)) {
+		DBGWARN("%s: Missing 'inputs' property\n", node->name);
+		return -EINVAL;
+	}
+
+	if (of_property_read_u32(node, "swirq-count", &swirq_count)) {
+		DBGWARN("%s: Missing 'swirq-count' property\n", node->name);
+		return -EINVAL;
+	}
+
+	if (of_property_read_u32(node, "outputs", &output_count)) {
+		DBGWARN("%s: Missing 'outputs' property\n", node->name);
+		return -EINVAL;
+	}
+
+	if ((input_count != ROUTER_INPUTS)
+	    || (swirq_count != SWIRQ_COUNT)
+	    || (output_count != ROUTER_OUTPUTS)) {
+		DBGERR("%s: input/swirq/output count mismatch\n", node->name);
+		return -EINVAL;
+	}
+
+	/* Check IRQ group mode */
+	if (of_property_read_u32(node, "irq-groups", &irqgroup_count)) {
+		DBGLOG("%s: Using explicit IRQ group definition\n", node->name);
+
+		/* count IRQ groups */
+		irqgroup_count = 0;
+		for_each_child_of_node(node, child)
+		irqgroup_count++;
+
+		implicit_groups = 0;
+	} else {
+		DBGLOG("%s: Using implicit IRQ group definition\n", node->name);
+		implicit_groups = irqgroup_count;
+	}
+
+	/* SW IRQs are always grouped together */
+	if (swirq_count)
+		irqgroup_count++;
+
+	if (irqgroup_count > output_count) {
+		DBGERR("%s: Too many IRQ groups %d > %d outputs\n",
+		       node->name, irqgroup_count, output_count);
+		return -EINVAL;
+	}
+
+	/* Create the context */
+	irqrouter = kzalloc(sizeof(*irqrouter), GFP_KERNEL);
+	raw_spin_lock_init(&(irqrouter->lock));
+	irqrouter->node = node;
+	irqrouter->base = base;
+	irqrouter->input_count = input_count;
+	irqrouter->swirq_count = swirq_count;
+	irqrouter->irqgroup_count = irqgroup_count;
+	irqrouter->output_count = output_count;
+	irqrouter->implicit_groups = implicit_groups;
+
+	/* We probably don't need to add up swirq_count since SW irqs
+	 * have are always muxed together
+	 */
+	total_irqs = input_count + swirq_count + irqgroup_count;
+
+	domain = irq_domain_add_hierarchy(parent_domain,
+					  0,
+					  total_irqs,
+					  node,
+					  &tango_irqdomain_hierarchy_ops,
+					  irqrouter);
+	if (!domain) {
+		DBGERR("%s: Failed to allocate domain hierarchy\n", node->name);
+		iounmap(irqrouter->base);
+		kfree(irqrouter);
+		return -ENOMEM;
+	}
+
+	domain->name = node->full_name;
+
+	DBGWARN("%s:%s(0x%p) base 0x%p, %d (+ %d swirq) and %d %s IRQ groups "
+		"=> %d router 0x%p, parent %s\n",
+		NODE_NAME(irq_domain_get_of_node(domain)),
+		domain->name,
+		domain,
+		base,
+		input_count, swirq_count, irqgroup_count,
+		implicit_groups ? "implicit" : "explicit",
+		output_count,
+		irqrouter,
+		parent->full_name);
+
+	/* Allocate domains for shared IRQs */
+
+	if (irqrouter->swirq_count) {
+		int err;
+
+		/* All SW IRQs are muxed together */
+		err = tango_irq_init_domain(irqrouter,
+					    0,
+					    SIGMA_SWIRQ,
+					    node,
+					    node);
+		if (err < 0) {
+			DBGERR("%s: Failed to init SWIRQ domain\n",
+			       node->name);
+		}
+	}
+
+	if (irqrouter->implicit_groups > 0) {
+		int err;
+
+		/* NOTE that i starts at 1 because index 0 is reserved
+		 * for SW IRQs.
+		 */
+		i = 1;
+		for (; i < irqrouter->implicit_groups; i++) {
+			err = tango_irq_init_domain(irqrouter,
+						    i,
+						    SIGMA_IRQGROUP_KEY + i,
+						    node,
+						    node);
+			if (err < 0) {
+				DBGERR("%s: Failed to init domain %d\n",
+				       node->name, i);
+			}
+		}
+	} else {
+		int err;
+
+		/* NOTE that i starts at 1 because index 0 is reserved
+		 * for SW IRQs.
+		 */
+		i = 1;
+		for_each_child_of_node(node, child) {
+			err = tango_irq_init_domain(irqrouter,
+						    i,
+						    SIGMA_HWIRQ,
+						    node,
+						    child);
+			if (err < 0) {
+				DBGERR("%s: Failed to init domain %d\n",
+				       node->name, i);
+			}
+
+			i++;
+		}
+	}
+
+	/* HW IRQs: clear routing and disable them */
+	for (i = 0; i < irqrouter->input_count; i++)
+		tango_set_irq_route(irqrouter,
+				    i,
+				    -1);
+
+	/* SW IRQs: clear routing and disable them */
+	for (i = 0; i < irqrouter->swirq_count; i++)
+		tango_set_irq_route(irqrouter,
+				    irqrouter->input_count + i,
+				    -1);
+
+	return 0;
+}
+
+
+IRQCHIP_DECLARE(tango_irqrouter, "sigma,smp,irqrouter", tango_of_irq_init);
diff --git a/include/dt-bindings/interrupt-controller/irq-tango_v4.h b/include/dt-bindings/interrupt-controller/irq-tango_v4.h
new file mode 100644
index 0000000..eb85d54
--- /dev/null
+++ b/include/dt-bindings/interrupt-controller/irq-tango_v4.h
@@ -0,0 +1,39 @@
+/*
+ * Copyright (C) 2014 Sebastian Frias <sf84@laposte.net>
+ *
+ * This program is free software; you can redistribute  it and/or modify it
+ * under  the terms of  the GNU General  Public License as published by the
+ * Free Software Foundation;  either version 2 of the  License, or (at your
+ * option) any later version.
+ */
+
+#ifndef _DT_BINDINGS_INTERRUPT_CONTROLLER_SIGMA_SMP_TANGO_V4_H
+#define _DT_BINDINGS_INTERRUPT_CONTROLLER_SIGMA_SMP_TANGO_V4_H
+
+#define SIGMA_HWIRQ (0xAA)
+#define SIGMA_SWIRQ (0x55)
+
+#define SIGMA_MAX_IRQGROUPS (16)
+
+#define SIGMA_IRQGROUP_KEY (0x80)
+
+/* NOTE: SW IRQs have their own IRQ group reserved, that's why there are
+ * only (SIGMA_MAX_IRQGROUPS - 1)=15 groups listed below and available
+ */
+#define SIGMA_IRQGROUP_1  (SIGMA_IRQGROUP_KEY + 0x1)
+#define SIGMA_IRQGROUP_2  (SIGMA_IRQGROUP_KEY + 0x2)
+#define SIGMA_IRQGROUP_3  (SIGMA_IRQGROUP_KEY + 0x3)
+#define SIGMA_IRQGROUP_4  (SIGMA_IRQGROUP_KEY + 0x4)
+#define SIGMA_IRQGROUP_5  (SIGMA_IRQGROUP_KEY + 0x5)
+#define SIGMA_IRQGROUP_6  (SIGMA_IRQGROUP_KEY + 0x6)
+#define SIGMA_IRQGROUP_7  (SIGMA_IRQGROUP_KEY + 0x7)
+#define SIGMA_IRQGROUP_8  (SIGMA_IRQGROUP_KEY + 0x8)
+#define SIGMA_IRQGROUP_9  (SIGMA_IRQGROUP_KEY + 0x9)
+#define SIGMA_IRQGROUP_10 (SIGMA_IRQGROUP_KEY + 0xa)
+#define SIGMA_IRQGROUP_11 (SIGMA_IRQGROUP_KEY + 0xb)
+#define SIGMA_IRQGROUP_12 (SIGMA_IRQGROUP_KEY + 0xc)
+#define SIGMA_IRQGROUP_13 (SIGMA_IRQGROUP_KEY + 0xd)
+#define SIGMA_IRQGROUP_14 (SIGMA_IRQGROUP_KEY + 0xe)
+#define SIGMA_IRQGROUP_15 (SIGMA_IRQGROUP_KEY + 0xf)
+
+#endif
-- 
1.7.11.2

Re: [RFC PATCH v2] irqchip: add support for SMP irq router

[Thomas Gleixner]

On Tue, 19 Jul 2016, Sebastian Frias wrote:
> +
> +#define DBGERR(__format, ...)	panic("[%s:%d] %s(): " __format,	\
> +				      __FILE__, __LINE__,		\
> +				      __func__, ##__VA_ARGS__)

Please get rid of this macro mess. Use the functions (panic, ...)
directly. There is no value for this file, line, func crappola. Think about
proper texts which can be grepped for.

> +
> +#define DBGWARN(__format, ...)	pr_err("[%s:%d] %s(): " __format,	\

Very consistent. WARN == err ....

> +				       __FILE__, __LINE__,		\
> +				       __func__, ##__VA_ARGS__)
> +
> +
> +#if 0

Don't even think about posting code with "#if 0" or such in it. 

> +#define DBGLOG(__format, ...)   pr_info("[%s:%d] %s(): " __format,	\
> +					__FILE__, __LINE__,		\
> +					__func__, ##__VA_ARGS__)
> +#else
> +#define DBGLOG(__format, ...) do {} while (0)
> +#endif

pr_debug() is there for a reason.

> +#if 0
> +#define SHORT_OR_FULL_NAME full_name
> +#else
> +#define SHORT_OR_FULL_NAME name
> +#endif

Use one and be done with it. Really.

> +
> +#define NODE_NAME(__node__) (__node__ ? __node__->SHORT_OR_FULL_NAME :	\
> +			     "<no-node>")

Sigh. pr_xxx("%s", NULL) prints <null>. That's really sufficient for your
debug/error handling.

> +
> +#define BITMASK_VECTOR_SIZE(__count__) (__count__ / 32)
> +#define IRQ_TO_OFFSET(__hwirq__) (__hwirq__ * 4)
> +
> +struct tango_irqrouter;
> +
> +/*
> + * Maintains the mapping between a Linux virq and a hwirq
> + * on the parent controller.
> + * It is used by tango_irqdomain_map() or tango_irqdomain_hierarchy_alloc()
> + * to setup the route between input IRQ and output IRQ
> + */
> +struct tango_irqrouter_output {
> +	struct tango_irqrouter *context;
> +
> +	u32 domain_id;
> +
> +	u32 hwirq;
> +	u32 hwirq_level;
> +	u32 virq;
> +
> +	int shared_count;
> +	int *shared_irqs;
> +};
> +
> +/*
> + * Context for the driver
> + */
> +struct tango_irqrouter {
> +	raw_spinlock_t lock;
> +	struct device_node *node;
> +	void __iomem *base;

Plase align struct members proper

	raw_spinlock_t		lock;
	struct device_node	*node;
	void __iomem		*base;

> +
> +	int input_count;
> +	u32 irq_mask[BITMASK_VECTOR_SIZE(ROUTER_INPUTS)];
> +	u32 irq_invert_mask[BITMASK_VECTOR_SIZE(ROUTER_INPUTS)];

You only ever use BITMASK_VECTOR_SIZE(ROUTER_INPUTS). Why can't you simply
do:

#define ROUTER_INPUTs_MASK_SIZE	(ROUTER__INPUTS / 32) ????

> +static inline u32 tango_readl(struct tango_irqrouter *irqrouter,
> +			      int reg)
> +{
> +	u32 val = readl_relaxed(irqrouter->base + reg);
> +	/*DBGLOG("r[0x%08x + 0x%08x = 0x%08x] = 0x%08x\n",
> +	  irqrouter->base, reg, irqrouter->base + reg, val);*/
> +	return val;

Please get rid of this debug nonsense.

> +}
> +
> +
> +static inline void tango_writel(struct tango_irqrouter *irqrouter,
> +				int reg,
> +				u32 val)
> +{
> +	/*DBGLOG("w[0x%08x + 0x%08x = 0x%08x] = 0x%08x\n",
> +	  irqrouter->base, reg, irqrouter->base + reg, val);*/
> +	writel_relaxed(val, irqrouter->base + reg);
> +}
> +
> +
> +static inline void tango_set_swirq_enable(struct tango_irqrouter *irqrouter,
> +					  int swirq,
> +					  bool enable)
> +{
> +	u32 offset = SWIRQ_ENABLE;

If you name that varible 'reg' then it becomes obvious what it does.

> +	u32 value = tango_readl(irqrouter, offset);
> +	u32 swirq_bit_index = swirq % SWIRQ_COUNT;
> +
> +#if 1
> +	DBGLOG("%smask swirq(in) %d : current regvalue 0x%x\n",
> +	       enable ? "un":"",
> +	       swirq, value);
> +#endif
> +
> +	if (enable) {
> +		/* unmask swirq */

The whole code lacks comments, but here you document the obvious ....

> +		irqrouter->swirq_mask |= (1 << swirq_bit_index);
> +		value |= (1 << swirq_bit_index);
> +	} else {
> +		/* mask swirq */
> +		irqrouter->swirq_mask &= ~(1 << swirq_bit_index);
> +		value &= ~(1 << swirq_bit_index);
> +	}
> +
> +	tango_writel(irqrouter, offset, value);
> +}
> +
> +
> +static inline void tango_set_hwirq_enable(struct tango_irqrouter *irqrouter,
> +					  int hwirq,
> +					  bool enable)
> +{
> +	u32 offset = IRQ_TO_OFFSET(hwirq);
> +	u32 value = tango_readl(irqrouter, offset);
> +	u32 hwirq_reg_index = hwirq / 32;
> +	u32 hwirq_bit_index = hwirq % 32;
> +	u32 *enable_mask = &(irqrouter->irq_mask[hwirq_reg_index]);
> +
> +#if 1
> +	DBGLOG("%smask hwirq(in) %d : current regvalue 0x%x\n",
> +	       enable ? "un":"",
> +	       hwirq, value);
> +#endif
> +
> +	if (enable) {
> +		/* unmask irq */
> +		*enable_mask |= (1 << hwirq_bit_index);
> +		value |= IRQ_ROUTER_ENABLE_MASK;
> +	} else {
> +		/* mask irq */
> +		*enable_mask &= ~(1 << hwirq_bit_index);
> +		value &= ~(IRQ_ROUTER_ENABLE_MASK);
> +	}
> +
> +	tango_writel(irqrouter, offset, value);
> +}
> +
> +
> +static inline void tango_set_swirq_inversion(struct tango_irqrouter *irqrouter,
> +					     int swirq,
> +					     bool invert)
> +{
> +
> +	DBGLOG("swirq(in) %d %s inverted\n", swirq, invert ? "":"not");
> +
> +	if (invert)
> +		DBGERR("SW IRQs cannot be inverted!\n");

Groan.

> +}
> +
> +
> +static inline void tango_set_hwirq_inversion(struct tango_irqrouter *irqrouter,
> +					     int hwirq,
> +					     bool invert)
> +{
> +	u32 offset = IRQ_TO_OFFSET(hwirq);
> +	u32 value = tango_readl(irqrouter, offset);
> +	u32 hwirq_reg_index = hwirq / 32;
> +	u32 hwirq_bit_index = hwirq % 32;
> +	u32 *invert_mask = &(irqrouter->irq_invert_mask[hwirq_reg_index]);
> +
> +	if (invert) {
> +		*invert_mask |= (1 << hwirq_bit_index);
> +		value |= IRQ_ROUTER_INVERT_MASK;
> +	} else {
> +		*invert_mask &= ~(1 << hwirq_bit_index);
> +		value &= ~(IRQ_ROUTER_INVERT_MASK);
> +	}
> +
> +	DBGLOG("hwirq(in) %d %s inverted\n", hwirq, invert ? "":"not");
> +
> +	tango_writel(irqrouter, offset, value);
> +}
> +
> +
> +static inline void tango_set_swirq_route(struct tango_irqrouter *irqrouter,
> +					 int swirq_in,
> +					 int irq_out)
> +{
> +	u32 swirq_reg_index = swirq_in / 4;
> +	u32 swirq_bit_index = (swirq_in % 4) * 8;
> +	u32 mask = ~(0x1f << swirq_bit_index);
> +	u32 offset = SWIRQ_MAP_GROUP0 + (swirq_reg_index * 4);
> +	u32 value = tango_readl(irqrouter, offset);
> +
> +	DBGLOG("ri %d, bi %d, mask 0x%x, offset 0x%x, val 0x%x\n",
> +	       swirq_reg_index,
> +	       swirq_bit_index,
> +	       mask,
> +	       offset,
> +	       value);
> +
> +	DBGLOG("route swirq %d => hwirq(out) %d\n", swirq_in, irq_out);
> +
> +	value &= mask;
> +
> +	if (irq_out < 0) {

This logic is utterly confusing, really. What's a negative interrupt number?

> +		tango_set_irq_enable(irqrouter,
> +				     swirq_in + irqrouter->input_count,
> +				     0);

Now you write back 'value' with the bit for this irq masked. Is that correct?

> +	} else

  } else {

  please

> +		value |= ((irq_out & 0x1f) << swirq_bit_index);
> +
> +	tango_writel(irqrouter, offset, value);
> +}
> +
> +
> +static inline void tango_set_hwirq_route(struct tango_irqrouter *irqrouter,
> +					 int irq_in,
> +					 int irq_out)
> +{
> +	u32 offset = IRQ_TO_OFFSET(irq_in);
> +	u32 value;
> +
> +	DBGLOG("route hwirq(in) %d => hwirq(out) %d\n", irq_in, irq_out);
> +
> +	if (irq_out < 0) {
> +		tango_set_irq_enable(irqrouter,
> +				     irq_in,
> +				     0);

This fits in a single line.

> +		value = 0;

Please document why you are setting the value to 0 here.

> +	} else
> +		value = (irq_out & 0x1f);
> +
> +	tango_writel(irqrouter, offset, value);
> +}
> +
> +
> +static inline int tango_set_irq_enable(struct tango_irqrouter *irqrouter,
> +				       int irq,
> +				       bool enable)
> +{
> +	if (irq >= (irqrouter->input_count + irqrouter->swirq_count))
> +		return -EINVAL;

How can that happen? Paranoia programming?

> +	else if (irq >= irqrouter->input_count)

That "else" is pointless. You return above.

> +		tango_set_swirq_enable(irqrouter,
> +				       irq - irqrouter->input_count,
> +				       enable);
> +	else
> +		tango_set_hwirq_enable(irqrouter,
> +				       irq,
> +				       enable);

This can be written way simpler.

     	if (irq >= irqrouter->input_count)
	   	irq -= irqrouter->input_count;
	tango_set_hwirq_enable(irqrouter, irq, enable);
    	  
Hmm?

> +static int tango_set_irq_type(struct tango_irqrouter *irqrouter,
> +			      int hwirq_in,
> +			      u32 type,
> +			      u32 parent_type)
> +{
> +	int err;
> +
> +	if (parent_type & (type & IRQ_TYPE_SENSE_MASK))
> +		/* same polarity */
> +		err = tango_set_irq_inversion(irqrouter, hwirq_in, 0);
> +	else
> +		/* invert polarity */
> +		err = tango_set_irq_inversion(irqrouter, hwirq_in, 1);

  	bool invert = parent_type & (type & IRQ_TYPE_SENSE_MASK);

  	err = tango_set_irq_inversion(irqrouter, hwirq_in, invert);	

> +
> +	if (err < 0) {
> +		DBGWARN("Failed to setup IRQ %d polarity\n", hwirq_in);
> +		return err;
> +	}
> +
> +	switch (type & IRQ_TYPE_SENSE_MASK) {
> +	case IRQ_TYPE_EDGE_RISING:
> +	case IRQ_TYPE_EDGE_FALLING:
> +		DBGERR("Does not support edge triggers\n");

This really sucks. In fact you panic here while the code suggests that you
print a DEBUG error and continue .....

> +		break;
> +	case IRQ_TYPE_LEVEL_HIGH:
> +		break;

You can spare that break.

> +	case IRQ_TYPE_LEVEL_LOW:
> +		break;
> +	default:
> +		DBGWARN("Invalid trigger mode 0x%x for hwirq(in) %d\n",
> +			type, hwirq_in);

So here you continue with an error code, but above for EDGE you panic. I
really don't get that logic.

> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +
> +static int tango_get_output_for_hwirq(struct tango_irqrouter *irqrouter,
> +				      int hwirq_in,
> +				      struct tango_irqrouter_output **out_val)
> +{
> +	struct tango_irqrouter_output *irqrouter_output;
> +	int i;
> +
> +	if (!out_val)
> +		return -EINVAL;

This is utter crap. Look at the call site:

> +	struct tango_irqrouter_output *irqrouter_output = NULL;
> +
> +	tango_get_output_for_hwirq(irqrouter, hwirq_in, &irqrouter_output);
> +	if (!irqrouter_output)
> +		goto handle_parent;

So out_val CANNOT be NULL. What's the purpose of your return values if you
ignore them?

> +
> +	/* Get the irqrouter_output for the hwirq */

This is another really helpful comment.

> +	for (i = 0; i < irqrouter->output_count; i++) {
> +		int j;
> +
> +		irqrouter_output = &(irqrouter->output[i]);
> +
> +		for (j = 0; j < irqrouter_output->shared_count; j++) {
> +			if (hwirq_in == irqrouter_output->shared_irqs[j])
> +				goto found_router_output;
> +		}
> +	}
> +	if (i == irqrouter->output_count) {

What other reason would be to get here than this?

> +		DBGWARN("Couldn't find hwirq mapping\n");
> +		return -ENODEV;
> +	}
> +
> +found_router_output:
> +
> +	*out_val = irqrouter_output;
> +	return 0;
> +}

This whole thing can be condensed to:

static struct tango_irqrouter_output *
tango_get_output_for_hwirq(struct tango_irqrouter *irqr, int hwirq_in)
{
	struct tango_irqrouter_output *rout;
	int i, j;

	/* Scan the outputs for a matching hwirq number */
	for (i = 0, rout = irqr->output; i < irqr->output_count; i++, rout++) {
		for (j = 0; j < rout->shared_count; j++) {
			if (rout->shared_irqs[j] == hwirq_in)
			   	return rout;
		}
	}
	return NULL;
}

And the call site would become:

    	struct tango_irqrouter_output *rout;

	rout = tango_get_output_for_hwirq(irqrouter, hwirq_in);
	if (!rout)
		goto handle_parent;

Hmm?

> +/* 'irqchip' handling callbacks
> + * Used for 'shared' IRQs, i.e.: IRQs that share a GIC input
> + * This driver performs the IRQ dispatch based on the flags
> + */
> +

Multiline comments have this format:
	 
/*
 * First line
 * ....
 * Last line
 */

> +static void tango_irqchip_mask_irq(struct irq_data *data)
> +{
> +	struct irq_domain *domain = irq_data_get_irq_chip_data(data);
> +	struct tango_irqrouter_output *irqrouter_output = domain->host_data;
> +	struct tango_irqrouter *irqrouter = irqrouter_output->context;
> +	int hwirq_in = (int)data->hwirq;

Huch? What's that silly typecast for? Why can't you use u32 for hwirq_in?

> +	tango_set_irq_enable(irqrouter, hwirq_in, 0);
> +}

> +#ifdef CONFIG_SMP
> +static int tango_irqchip_set_irq_affinity(struct irq_data *data,
> +					  const struct cpumask *mask_val,
> +					  bool force)
> +{
> +	struct irq_domain *domain = irq_data_get_irq_chip_data(data);
> +	struct tango_irqrouter_output *irqrouter_output = domain->host_data;
> +	struct irq_chip *parent_chip = irq_get_chip(irqrouter_output->virq);
> +	struct irq_data *parent_data = irq_get_irq_data(irqrouter_output->virq);
> +
> +	DBGLOG("%s:%s(0x%p)\n",
> +	       NODE_NAME(irq_domain_get_of_node(domain)), domain->name, domain);
> +
> +	if (parent_chip && parent_chip->irq_set_affinity)
> +		return parent_chip->irq_set_affinity(parent_data,
> +						     mask_val,
> +						     force);
> +	else
> +		return -EINVAL;

You really can spare identation levels by writing it a bit more clever:

    	if (!parent || !parent->irq_set_affinity)
	   	 return -EINVAL;

	return parent->irq_set_affinity(parent, mask, force);

You might notice that I shortened the variable names and it still is entirely
clear what the code does.

Aside of that mask_val is a clear misnomer.

> +}
> +#endif
> +
> +
> +static inline u32 tango_dispatch_irqs(struct irq_domain *domain,
> +				      struct irq_desc *desc,
> +				      u32 status,
> +				      int base)
> +{
> +	u32 hwirq;
> +	u32 virq;

Please put same types onto a single line

> +
> +	while (status) {
> +		hwirq = __ffs(status);
> +		virq = irq_find_mapping(domain, base + hwirq);
> +		if (unlikely(!virq))
> +			handle_bad_irq(desc);
> +		else
> +			generic_handle_irq(virq);
> +
> +		status &= ~BIT(hwirq);
> +	}
> +
> +	return status;

Huch? status is guaranteed to be 0 here. So what's the point of this return
value.

> +}
> +
> +static void tango_irqdomain_handle_cascade_irq(struct irq_desc *desc)
> +{
> +	struct irq_domain *domain = irq_desc_get_handler_data(desc);
> +	struct tango_irqrouter_output *irqrouter_output = domain->host_data;
> +	struct tango_irqrouter *irqrouter = irqrouter_output->context;
> +	struct irq_chip *host_chip = irq_desc_get_chip(desc);
> +	u32 i, status;
> +	u32 swirq_status, irq_status[BITMASK_VECTOR_SIZE(ROUTER_INPUTS)];
> +
> +#if 0
> +	DBGLOG("%s:%s(0x%p): irqrouter_output 0x%p, hwirq(out) %d\n",
> +	       NODE_NAME(irq_domain_get_of_node(domain)), domain->name, domain,
> +	       irqrouter_output, irqrouter_output->hwirq);
> +#endif
> +
> +	chained_irq_enter(host_chip, desc);
> +
> +	raw_spin_lock(&(irqrouter->lock));
> +	swirq_status = tango_readl(irqrouter, READ_SWIRQ_STATUS);
> +	for (i = 0; i < BITMASK_VECTOR_SIZE(ROUTER_INPUTS); i++)
> +		irq_status[i] = tango_readl(irqrouter,
> +					    READ_SYS_IRQ_GROUP0 + i*4);

  i * 4		please

> +	raw_spin_unlock(&(irqrouter->lock));
> +
> +	/* HW irqs */

And that comment tells us what?

> +	for (i = 0; i < BITMASK_VECTOR_SIZE(ROUTER_INPUTS); i++) {
> +#if 0
> +		DBGLOG("%d: 0x%08x (en 0x%08x, inv 0x%08x)\n",
> +		       i,
> +		       irq_status[i],
> +		       irqrouter->irq_mask[0],
> +		       irqrouter->irq_invert_mask[0]);
> +#endif
> +
> +#define HANDLE_INVERTED_LINES(__irqstatus__, __x__) ((((~__irqstatus__) & irqrouter->irq_invert_mask[__x__]) & irqrouter->irq_mask[__x__]) | __irqstatus__)
> +#define HANDLE_EN_AND_INV_MASKS(__irqstatus__, __y__) (HANDLE_INVERTED_LINES(__irqstatus__, __y__) & irqrouter->irq_mask[__y__])

What the hell does this unparseable macro mess? What's so wrong with a proper
inline function?

> +
> +		irq_status[i] = HANDLE_EN_AND_INV_MASKS(irq_status[i], i);
> +		status = tango_dispatch_irqs(domain, desc, irq_status[i], i*32);
> +		if (status & irq_status[i])

Can you pretty please explain how this can happen? It can't. Because you clear
every single bit of status in tango_dispatch_irqs(). See above.

Please clean up that unholy mess of nonsensical checks and debug code.

> +			DBGERR("%s: %d unhandled IRQs (as a mask) 0x%x\n",
> +			       NODE_NAME(irq_domain_get_of_node(domain)),
> +			       i,
> +			       status & irq_status[i]);
> +	}
> +
> +	/* SW irqs */
> +	swirq_status &= irqrouter->swirq_mask;
> +	status = tango_dispatch_irqs(domain, desc, swirq_status, 128);

128 is the magic number pulled out of thin air or what?

> +	if (status & swirq_status)
> +		DBGERR("%s: Unhandled IRQs (as a mask) 0x%x\n",
> +		       NODE_NAME(irq_domain_get_of_node(domain)),
> +		       status & swirq_status);

See above.

> +
> +	chained_irq_exit(host_chip, desc);
> +}
> +
> +
> +/**
> + * tango_irqdomain_map - route a hwirq(in) to a hwirq(out).
> + * NOTE: The hwirq(out) must have been already allocated and enabled on
> + * the parent controller.

Please put notes AFTER the argument descriptions

> + * @hwirq: HW IRQ of the device requesting an IRQ
> + * if hwirq > inputs it is a SW IRQ
> + * @virq: Linux IRQ (associated to the domain) to be given to the device
> + * @domain: IRQ domain (from the domain, we get the irqrouter_output
> + * in order to know to which output we need to route hwirq to)

And if you spend a few seconds to align this proper, then it becomes suddenly
readable.

* @hwirq:	HW IRQ of the device requesting an IRQ
*		if hwirq > inputs it is a SW IRQ
* @virq:	Linux IRQ (associated to the domain) to be given to the device
* @domain:	IRQ domain (from the domain, we get the irqrouter_output
*		in order to know to which output we need to route hwirq to)

Hmm?

> + */
> +static int tango_irqdomain_map(struct irq_domain *domain,
> +			       unsigned int virq,
> +			       irq_hw_number_t hwirq)
> +{
> +	struct tango_irqrouter_output *irqrouter_output = domain->host_data;
> +	struct tango_irqrouter *irqrouter = irqrouter_output->context;
> +
> +	DBGLOG("%s:%s(0x%p): hwirq(in) %d := virq %d, and route "
> +	       "hwirq(in) %d => hwirq(out) %d (virq %d)\n",
> +	       NODE_NAME(irq_domain_get_of_node(domain)),
> +	       domain->name,
> +	       domain,
> +	       (u32)hwirq,
> +	       virq,
> +	       (u32)hwirq,
> +	       irqrouter_output->hwirq,
> +	       irqrouter_output->virq);
> +
> +	if (hwirq >= (irqrouter->input_count + irqrouter->swirq_count))
> +		DBGERR("%s: Invalid hwirq(in) %d >= %d + %d\n",
> +		       NODE_NAME(irq_domain_get_of_node(domain)),
> +		       (u32)hwirq,
> +		       irqrouter->input_count,
> +		       irqrouter->swirq_count);
> +	else if (hwirq >= irqrouter->input_count)
> +		DBGLOG("Map swirq %ld\n", hwirq - irqrouter->input_count);
> +
> +	irq_set_chip_and_handler(virq,
> +				 &tango_irq_chip_shared_ops,
> +				 handle_level_irq);
> +	irq_set_chip_data(virq, domain);
> +	irq_set_probe(virq);
> +
> +	tango_set_irq_route(irqrouter, hwirq, irqrouter_output->hwirq);
> +
> +	return 0;
> +}
> +
> +
> +/**
> + * tango_irqdomain_translate - used to select the domain for a given
> + * irq_fwspec

It hardly selects the domain. @domain is handed in as a argument. And please
get rid of that silly 'used to'. That just adds characters and no information.

    - Select the router section for a firmware spec

Hmm?

> + * @domain: a registered domain
> + * @fwspec: an IRQ specification. This callback is used to translate the
> + * parameters given as irq_fwspec into a HW IRQ and Type values.

fwspec is a callback?

> + * @out_hwirq: 
> + * @out_type:

Missing docs.....

> + */
> +static int tango_irqdomain_translate(struct irq_domain *domain,
> +				     struct irq_fwspec *fwspec,
> +				     unsigned long *out_hwirq,
> +				     unsigned int *out_type)
> +{
> +	struct tango_irqrouter_output *irqrouter_output = domain->host_data;
> +	struct tango_irqrouter *irqrouter = irqrouter_output->context;
> +	irq_hw_number_t irq;
> +	u32 domain_id, type;
> +	int err;
> +
> +	DBGLOG("%s:%s(0x%p): argc %d for hwirq(out) %d\n",
> +	       NODE_NAME(irq_domain_get_of_node(domain)),
> +	       domain->name,
> +	       domain,
> +	       fwspec->param_count,
> +	       irqrouter_output->hwirq);
> +
> +	err = tango_parse_fwspec(domain,
> +				 fwspec,
> +				 &domain_id,
> +				 &irq,
> +				 &type);

Please use the full 80 characters. Your patch does not become more valuable
when you inflate the line count. It's more valuable when it is easy to read
and understand.

> +	if (err < 0) {
> +		DBGWARN("Failed to parse fwspec\n");
> +		return err;
> +	}
> +
> +	switch (domain_id) {
> +	case SIGMA_HWIRQ:
> +		DBGLOG("Request is for SIGMA_HWIRQ\n");
> +		break;
> +	case SIGMA_SWIRQ:
> +		DBGLOG("Request is for SIGMA_SWIRQ\n");
> +		irq += irqrouter->input_count;
> +		break;
> +	default:
> +		DBGLOG("Request is for domain ID 0x%x (we are 0x%x)\n",
> +		       domain_id,
> +		       irqrouter_output->domain_id);

Huch what? You only ever handle HW and SW irqs and now you happily proceed if
the id is something else. Consistent error handling is optional and can be
replaced by random debug prints, right?

> +		break;
> +	};
> +
> +	*out_hwirq = irq;
> +	*out_type  = type & IRQ_TYPE_SENSE_MASK;
> +
> +	DBGLOG("hwirq %d type 0x%x\n", (u32)*out_hwirq, (u32)*out_type);
> +
> +	return 0;
> +}
> +
> +
> +/**
> + * tango_irqdomain_select - used to select the domain for a given irq_fwspec
> + * @domain: a registered domain
> + * @fwspec: an IRQ specification. This callback should return zero if the
> + * irq_fwspec does not belong to the given domain. If it does, it should
> + * return non-zero.

Callback????

> + * In practice it will return non-zero if the irq_fwspec matches one of the
> + * IRQs shared within the given domain.
> + * @bus_token: a bus token
> + */
> +static int tango_irqdomain_select(struct irq_domain *domain,
> +				  struct irq_fwspec *fwspec,
> +				  enum irq_domain_bus_token bus_token)
> +{
> +	struct tango_irqrouter_output *irqrouter_output = domain->host_data;
> +	struct tango_irqrouter *irqrouter = irqrouter_output->context;
> +	irq_hw_number_t irq;
> +	u32 domain_id, type;
> +	int err;
> +
> +	DBGLOG("%s:%s(0x%p): argc %d, 0x%p, bus 0x%x\n",
> +	       NODE_NAME(irq_domain_get_of_node(domain)), domain->name, domain,
> +	       fwspec->param_count, fwspec->fwnode, bus_token);
> +
> +	DBGLOG("router 0x%p, output 0x%p\n", irqrouter, irqrouter_output);
> +
> +	err = tango_parse_fwspec(domain, fwspec, &domain_id, &irq, &type);
> +	if (err < 0)
> +		return 0;
> +
> +	switch (domain_id) {
> +	case SIGMA_HWIRQ:
> +		DBGLOG("Request is for SIGMA_HWIRQ\n");
> +		break;
> +	case SIGMA_SWIRQ:
> +		DBGLOG("Request is for SIGMA_SWIRQ\n");
> +		break;
> +	default:
> +		DBGLOG("Request is for domain ID 0x%x (we are 0x%x)\n",
> +		       domain_id,
> +		       irqrouter_output->domain_id);
> +		break;
> +	};
> +
> +	if (!irqrouter->implicit_groups) {
> +		int i;
> +
> +		/* Check if the requested IRQ belongs to those listed
> +		 * to be sharing the output assigned to this domain

Your using of 'domain' is utterly confusing. Please use something like
hwdomain or whatever which makes it clear that this is something else than the
irq domain. I tripped over this several times now.

> +		 */
> +		if (irqrouter_output->shared_count <= 0) {
> +			DBGLOG("Not shared IRQ line?\n");
> +			return 0;
> +		}
> +
> +		for (i = 0; i < irqrouter_output->shared_count; i++) {
> +			if (irq == irqrouter_output->shared_irqs[i]) {
> +				DBGLOG("Match: IRQ %lu\n", irq);
> +				return 1;
> +			}
> +		}
> +	} else {
> +		/* Otherwise, check if the domain_id given matches
> +		 * the one assigned to this output
> +		 */
> +		if (domain_id == irqrouter_output->domain_id) {
> +			DBGLOG("Match: Domain ID %d\n", domain_id);
> +			return 1;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +
> +/* 'irqrouter' handling callbacks
> + * Used for 'direct' IRQs, i.e.: IRQs that are directly routed to the GIC
> + * This driver does not dispatch the IRQs, the GIC does.
> + */
> +
> +
> +static void tango_irqrouter_mask_irq(struct irq_data *data)
> +{
> +	struct irq_domain *domain = irq_data_get_irq_chip_data(data);
> +	struct tango_irqrouter *irqrouter = domain->host_data;
> +	int hwirq_in = (int)data->hwirq;
> +
> +	DBGLOG("%s:%s(0x%p) hwirq(in) %d\n",
> +	       NODE_NAME(irq_domain_get_of_node(domain)),
> +	       domain->name,
> +	       domain,
> +	       hwirq_in);
> +
> +	tango_set_irq_enable(irqrouter, hwirq_in, 0);
> +
> +	irq_chip_mask_parent(data);
> +}
> +
> +
> +static void tango_irqrouter_unmask_irq(struct irq_data *data)
> +{
> +	struct irq_domain *domain = irq_data_get_irq_chip_data(data);
> +	struct tango_irqrouter *irqrouter = domain->host_data;
> +	int hwirq_in = (int)data->hwirq;
> +
> +	DBGLOG("%s:%s(0x%p) hwirq(in) %d\n",
> +	       NODE_NAME(irq_domain_get_of_node(domain)),
> +	       domain->name,
> +	       domain,
> +	       hwirq_in);
> +
> +	tango_set_irq_enable(irqrouter, hwirq_in, 1);
> +
> +	irq_chip_unmask_parent(data);
> +}
> +
> +
> +static int tango_irqrouter_set_irq_type(struct irq_data *data,
> +					unsigned int type)
> +{
> +	struct irq_domain *domain = irq_data_get_irq_chip_data(data);
> +	struct tango_irqrouter_output *irqrouter_output = NULL;
> +	struct tango_irqrouter *irqrouter = domain->host_data;
> +	int hwirq_in = (int)data->hwirq;
> +	u32 parent_type;
> +
> +	DBGLOG("%s:%s(0x%p) type 0x%x for hwirq(in) %d\n",
> +	       NODE_NAME(irq_domain_get_of_node(domain)),
> +	       domain->name,
> +	       domain,
> +	       type,
> +	       hwirq_in);
> +
> +	tango_get_output_for_hwirq(irqrouter, hwirq_in, &irqrouter_output);
> +	if (!irqrouter_output)
> +		goto handle_parent;

So if there is no output, then you unconditionally set the type on the
parent. If that's correct, this needs a big fat comment at least.

> +	parent_type = (irqrouter_output->hwirq_level & IRQ_TYPE_SENSE_MASK);
> +	tango_set_irq_type(irqrouter, hwirq_in, type, parent_type);
> +
> +handle_parent:
> +	return irq_chip_set_type_parent(data, type);
> +}

I'm stopping here. You can apply the above to the rest of the code as well.

Thanks,

	tglx

Re: [RFC PATCH v2] irqchip: add support for SMP irq router

[Marc Gonzalez]

On 19/07/2016 16:23, Sebastian Frias wrote:
> 

I don't think there's supposed to be an empty line at the beginning of
the patch. I don't know whether 'git am' knows to zap the spurious line.

> 3) The file is called 'irq-tango_v4.c' but I think it should match the
> compatible string, so I was thinking of renaming:
> irq-tango_v4.c => irq-sigma_smp_irqrouter.c
> irq-tango_v4.h => irq-sigma_smp_irqrouter.h
> 
> What do you think?

Why is the file called irq-tango_v4? Why v4?
This is the second incarnation of a tango IRQ controller (that I know of).
Why not -v2 then?

(Could you please not mix dash and underscore? Use one or the other consistently.)

> 5) checkpatch.pl reports a few warnings:
> 
> - WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
> I have not changed the file because I would like to be settled on the naming
> first (see point 2 above)

I now have a regex pattern that matches "tango" in MAINTAINERS.
However, checkpatch only checks the current patch, not the existing
MAINTAINERS file.

>  .../interrupt-controller/sigma,smp,irqrouter.txt   |  135 ++

We've managed to use "tango" everywhere consistently (so far), please let's not
introduce the "smp" terminology at this point, it will only confuse matters.
(I use "smp87xy" to name specific boards, tango for the architecture/family.)

I found only one occurrence of the "a,b,c" syntax

$ find Documentation/devicetree/bindings -name "*,*,*"
Documentation/devicetree/bindings/net/wireless/ti,wlcore,spi.txt

Maybe we need to discuss this with the devicetree folks.

Note: Mans created
Documentation/devicetree/bindings/interrupt-controller/sigma,smp8642-intc.txt
for the v1 intc driver.

> +Required properties:
> +- compatible: Should be "sigma,smp,irqrouter".

You said in a previous message that you wanted to use a generic name.

AFAIU, the current DT trend is the opposite: to use the name of the first
board (chronologically) that used that HW incarnation.
So that would be "sigma,smp87xx-intc"
(We can fill the xx when the weenies in marketing finally decide to
announce the chip.)

The generic names you cited, like xhci, are actually naming a protocol,
which many controllers implement (because it is standard).

> +/*
> + * Copyright (C) 2014 Sebastian Frias <sf84@laposte.net>

Having discussed this topic with Thibaud, I think it is better to use
"official" email addresses for kernel submissions.

Regards.

Re: [RFC PATCH v2] irqchip: add support for SMP irq router

[Sebastian Frias]

Hi Thomas,

Thanks for your comments.
I appreciate the time you dedicated to the review.
I will take your comments into account, in the meanwhile, and since this was
a RFC, would it be possible to also get some feedback/comments from a
conceptual point of view?

I'm copy/pasting some of the questions I attached to the RFC email here:

----
2) In order to get a virq mapping for the domains associated with the outputs
(the outputs connected to the GIC) I request a sort of "Fake HW IRQ"
See irq-tango_v4.c:1400

	/* To request a virq we need a HW IRQ, use a "Fake HW IRQ" */
	hwirq      = index + irqrouter->input_count + irqrouter->swirq_count;

This feels a bit like a hack, so suggestions are welcome.

3) The file is called 'irq-tango_v4.c' but I think it should match the
compatible string, so I was thinking of renaming:
irq-tango_v4.c => irq-sigma_smp_irqrouter.c
irq-tango_v4.h => irq-sigma_smp_irqrouter.h

What do you think?

4) Do I have to do something more to handle the affinity stuff?

6) More of a theoretical question:
I have to #include <dt-bindings/interrupt-controller/irq-tango_v4.h> from
code that is not in the Linux tree. That's fine for now, but if in the future
there are other irq controllers and another header is required, how can the
code know which header to #include ?
Are we supposed to #include all of them, and then somehow detect which module
is actually active and act accordingly? If so, can we detect which module
matched and probed correctly to know which controller version we need to
talk to?
----

What the driver does is:
- creates a domain hierarchy attached to its parent domain (the GIC)
  - the callbacks for this domain hierarchy will be used to deal with IRQs
  "directly" routed to the GIC (i.e.: exclusive IRQ lines routed to the GIC
  and not shared).
  In this case the GIC dispatches the interrupts.
- create linear domains attached to this driver's node.
  - the callbacks for these linear domains will be used to deal with shared
  IRQs (they share the routing to one of the GIC inputs).
  In this case this driver dispatches the interrupts after reading the flags.

It is such concept, and then its implementation, that I would need some more
advise with, because, as stated in the questions above, it still does not
feels right, especially question 2.

I will reply to your comments below and will post a v3 later, hopefully after
dealing also with the answers to the doubts outlined above.


On 07/19/2016 06:49 PM, Thomas Gleixner wrote:
> On Tue, 19 Jul 2016, Sebastian Frias wrote:
>> +
>> +#define DBGERR(__format, ...)	panic("[%s:%d] %s(): " __format,	\
>> +				      __FILE__, __LINE__,		\
>> +				      __func__, ##__VA_ARGS__)
> 
> Please get rid of this macro mess. Use the functions (panic, ...)
> directly. There is no value for this file, line, func crappola. Think about
> proper texts which can be grepped for.

Ok.

> 
>> +
>> +#define DBGWARN(__format, ...)	pr_err("[%s:%d] %s(): " __format,	\
> 
> Very consistent. WARN == err ....
> 
>> +				       __FILE__, __LINE__,		\
>> +				       __func__, ##__VA_ARGS__)
>> +
>> +
>> +#if 0
> 
> Don't even think about posting code with "#if 0" or such in it. 
> 

The idea behind these macros was:
- to easily enable/disable them.
- to be more consistent with other code written at Sigma: this is a
Sigma-only driver.

>> +#define DBGLOG(__format, ...)   pr_info("[%s:%d] %s(): " __format,	\
>> +					__FILE__, __LINE__,		\
>> +					__func__, ##__VA_ARGS__)
>> +#else
>> +#define DBGLOG(__format, ...) do {} while (0)
>> +#endif
> 
> pr_debug() is there for a reason.

Yes, but doesn't pr_debug() requires changes to the Makefile?

> 
>> +#if 0
>> +#define SHORT_OR_FULL_NAME full_name
>> +#else
>> +#define SHORT_OR_FULL_NAME name
>> +#endif
> 
> Use one and be done with it. Really.

By making the choice explicit, the person reading/debugging is aware of the possibilities.
Picking "full_name" or "name" would likely prevent the person from knowing it has other debug options.

But I can change this if upstreaming depends on it.

> 
>> +
>> +#define NODE_NAME(__node__) (__node__ ? __node__->SHORT_OR_FULL_NAME :	\
>> +			     "<no-node>")
> 
> Sigh. pr_xxx("%s", NULL) prints <null>. That's really sufficient for your
> debug/error handling.

The code is checking for __node__ to be non-NULL before dereferencing it.

> 
>> +
>> +#define BITMASK_VECTOR_SIZE(__count__) (__count__ / 32)
>> +#define IRQ_TO_OFFSET(__hwirq__) (__hwirq__ * 4)
>> +
>> +struct tango_irqrouter;
>> +
>> +/*
>> + * Maintains the mapping between a Linux virq and a hwirq
>> + * on the parent controller.
>> + * It is used by tango_irqdomain_map() or tango_irqdomain_hierarchy_alloc()
>> + * to setup the route between input IRQ and output IRQ
>> + */
>> +struct tango_irqrouter_output {
>> +	struct tango_irqrouter *context;
>> +
>> +	u32 domain_id;
>> +
>> +	u32 hwirq;
>> +	u32 hwirq_level;
>> +	u32 virq;
>> +
>> +	int shared_count;
>> +	int *shared_irqs;
>> +};
>> +
>> +/*
>> + * Context for the driver
>> + */
>> +struct tango_irqrouter {
>> +	raw_spinlock_t lock;
>> +	struct device_node *node;
>> +	void __iomem *base;
> 
> Plase align struct members proper
> 
> 	raw_spinlock_t		lock;
> 	struct device_node	*node;
> 	void __iomem		*base;

Ok.

> 
>> +
>> +	int input_count;
>> +	u32 irq_mask[BITMASK_VECTOR_SIZE(ROUTER_INPUTS)];
>> +	u32 irq_invert_mask[BITMASK_VECTOR_SIZE(ROUTER_INPUTS)];
> 
> You only ever use BITMASK_VECTOR_SIZE(ROUTER_INPUTS). Why can't you simply
> do:
> 
> #define ROUTER_INPUTs_MASK_SIZE	(ROUTER__INPUTS / 32) ????

I guess I can do that.

> 
>> +static inline u32 tango_readl(struct tango_irqrouter *irqrouter,
>> +			      int reg)
>> +{
>> +	u32 val = readl_relaxed(irqrouter->base + reg);
>> +	/*DBGLOG("r[0x%08x + 0x%08x = 0x%08x] = 0x%08x\n",
>> +	  irqrouter->base, reg, irqrouter->base + reg, val);*/
>> +	return val;
> 
> Please get rid of this debug nonsense.

Ok, although I can guarantee it was really useful to determine and prove
that the HW documentation of the module was wrong...

> 
>> +}
>> +
>> +
>> +static inline void tango_writel(struct tango_irqrouter *irqrouter,
>> +				int reg,
>> +				u32 val)
>> +{
>> +	/*DBGLOG("w[0x%08x + 0x%08x = 0x%08x] = 0x%08x\n",
>> +	  irqrouter->base, reg, irqrouter->base + reg, val);*/
>> +	writel_relaxed(val, irqrouter->base + reg);
>> +}
>> +
>> +
>> +static inline void tango_set_swirq_enable(struct tango_irqrouter *irqrouter,
>> +					  int swirq,
>> +					  bool enable)
>> +{
>> +	u32 offset = SWIRQ_ENABLE;
> 
> If you name that varible 'reg' then it becomes obvious what it does.

Well, it was to add a sort of "consistency" between the code for SW IRQs and HW IRQs.

> 
>> +	u32 value = tango_readl(irqrouter, offset);
>> +	u32 swirq_bit_index = swirq % SWIRQ_COUNT;
>> +
>> +#if 1
>> +	DBGLOG("%smask swirq(in) %d : current regvalue 0x%x\n",
>> +	       enable ? "un":"",
>> +	       swirq, value);
>> +#endif
>> +
>> +	if (enable) {
>> +		/* unmask swirq */
> 
> The whole code lacks comments, but here you document the obvious ....

I will remove the comments.

> 
>> +		irqrouter->swirq_mask |= (1 << swirq_bit_index);
>> +		value |= (1 << swirq_bit_index);
>> +	} else {
>> +		/* mask swirq */
>> +		irqrouter->swirq_mask &= ~(1 << swirq_bit_index);
>> +		value &= ~(1 << swirq_bit_index);
>> +	}
>> +
>> +	tango_writel(irqrouter, offset, value);
>> +}
>> +
>> +
>> +static inline void tango_set_hwirq_enable(struct tango_irqrouter *irqrouter,
>> +					  int hwirq,
>> +					  bool enable)
>> +{
>> +	u32 offset = IRQ_TO_OFFSET(hwirq);
>> +	u32 value = tango_readl(irqrouter, offset);
>> +	u32 hwirq_reg_index = hwirq / 32;
>> +	u32 hwirq_bit_index = hwirq % 32;
>> +	u32 *enable_mask = &(irqrouter->irq_mask[hwirq_reg_index]);
>> +
>> +#if 1
>> +	DBGLOG("%smask hwirq(in) %d : current regvalue 0x%x\n",
>> +	       enable ? "un":"",
>> +	       hwirq, value);
>> +#endif
>> +
>> +	if (enable) {
>> +		/* unmask irq */
>> +		*enable_mask |= (1 << hwirq_bit_index);
>> +		value |= IRQ_ROUTER_ENABLE_MASK;
>> +	} else {
>> +		/* mask irq */
>> +		*enable_mask &= ~(1 << hwirq_bit_index);
>> +		value &= ~(IRQ_ROUTER_ENABLE_MASK);
>> +	}
>> +
>> +	tango_writel(irqrouter, offset, value);
>> +}
>> +
>> +
>> +static inline void tango_set_swirq_inversion(struct tango_irqrouter *irqrouter,
>> +					     int swirq,
>> +					     bool invert)
>> +{
>> +
>> +	DBGLOG("swirq(in) %d %s inverted\n", swirq, invert ? "":"not");
>> +
>> +	if (invert)
>> +		DBGERR("SW IRQs cannot be inverted!\n");
> 
> Groan.

I rather keep this to catch typos in code requesting SW IRQs.

> 
>> +}
>> +
>> +
>> +static inline void tango_set_hwirq_inversion(struct tango_irqrouter *irqrouter,
>> +					     int hwirq,
>> +					     bool invert)
>> +{
>> +	u32 offset = IRQ_TO_OFFSET(hwirq);
>> +	u32 value = tango_readl(irqrouter, offset);
>> +	u32 hwirq_reg_index = hwirq / 32;
>> +	u32 hwirq_bit_index = hwirq % 32;
>> +	u32 *invert_mask = &(irqrouter->irq_invert_mask[hwirq_reg_index]);
>> +
>> +	if (invert) {
>> +		*invert_mask |= (1 << hwirq_bit_index);
>> +		value |= IRQ_ROUTER_INVERT_MASK;
>> +	} else {
>> +		*invert_mask &= ~(1 << hwirq_bit_index);
>> +		value &= ~(IRQ_ROUTER_INVERT_MASK);
>> +	}
>> +
>> +	DBGLOG("hwirq(in) %d %s inverted\n", hwirq, invert ? "":"not");
>> +
>> +	tango_writel(irqrouter, offset, value);
>> +}
>> +
>> +
>> +static inline void tango_set_swirq_route(struct tango_irqrouter *irqrouter,
>> +					 int swirq_in,
>> +					 int irq_out)
>> +{
>> +	u32 swirq_reg_index = swirq_in / 4;
>> +	u32 swirq_bit_index = (swirq_in % 4) * 8;
>> +	u32 mask = ~(0x1f << swirq_bit_index);
>> +	u32 offset = SWIRQ_MAP_GROUP0 + (swirq_reg_index * 4);
>> +	u32 value = tango_readl(irqrouter, offset);
>> +
>> +	DBGLOG("ri %d, bi %d, mask 0x%x, offset 0x%x, val 0x%x\n",
>> +	       swirq_reg_index,
>> +	       swirq_bit_index,
>> +	       mask,
>> +	       offset,
>> +	       value);
>> +
>> +	DBGLOG("route swirq %d => hwirq(out) %d\n", swirq_in, irq_out);
>> +
>> +	value &= mask;
>> +
>> +	if (irq_out < 0) {
> 
> This logic is utterly confusing, really. What's a negative interrupt number?

It is a sort of "magic value" to disable the IRQ and the routing.

> 
>> +		tango_set_irq_enable(irqrouter,
>> +				     swirq_in + irqrouter->input_count,
>> +				     0);
> 
> Now you write back 'value' with the bit for this irq masked. Is that correct?

Yes, tango_set_irq_enable() is used to setup the mask (in this case disable the IRQ
by masking it) to one register, and later we write 'value' (which deals with the
routing) to a different register.
Since up to 4 SW IRQ routes can be programmed in one of multiple routing registers,
to "disable" the routing for one of them, we need to mask the other 3 and leave them
untouched.
'value' is ANDed with 'mask' which is keeps the 3 other routing intact and resetting
the routing for the given IRQ.

> 
>> +	} else
> 
>   } else {
> 
>   please

I think I'm confused, we need to use {} for single statements on "else" blocks,
but not on "for" blocks?

> 
>> +		value |= ((irq_out & 0x1f) << swirq_bit_index);
>> +
>> +	tango_writel(irqrouter, offset, value);
>> +}
>> +
>> +
>> +static inline void tango_set_hwirq_route(struct tango_irqrouter *irqrouter,
>> +					 int irq_in,
>> +					 int irq_out)
>> +{
>> +	u32 offset = IRQ_TO_OFFSET(irq_in);
>> +	u32 value;
>> +
>> +	DBGLOG("route hwirq(in) %d => hwirq(out) %d\n", irq_in, irq_out);
>> +
>> +	if (irq_out < 0) {
>> +		tango_set_irq_enable(irqrouter,
>> +				     irq_in,
>> +				     0);
> 
> This fits in a single line.

Ok.
Actually, I was wondering if for consistency it wouldn't be better if all
function calls had one argument per line?

> 
>> +		value = 0;
> 
> Please document why you are setting the value to 0 here.

Ok.
(it is for the same reason than for SW IRQs, to "disable" the routing, or more
precisely, to set disabled IRQs to a known value, in this case 0)

> 
>> +	} else
>> +		value = (irq_out & 0x1f);
>> +
>> +	tango_writel(irqrouter, offset, value);
>> +}
>> +
>> +
>> +static inline int tango_set_irq_enable(struct tango_irqrouter *irqrouter,
>> +				       int irq,
>> +				       bool enable)
>> +{
>> +	if (irq >= (irqrouter->input_count + irqrouter->swirq_count))
>> +		return -EINVAL;
> 
> How can that happen? Paranoia programming?

No, actually it can happen, that's the reason for question 2 of my RFC:

----
2) In order to get a virq mapping for the domains associated with the outputs
(the outputs connected to the GIC) I request a sort of "Fake HW IRQ"
See irq-tango_v4.c:1400

/* To request a virq we need a HW IRQ, use a "Fake HW IRQ" */
hwirq = index + irqrouter->input_count + irqrouter->swirq_count;

This feels a bit like a hack, so suggestions are welcome.
----


> 
>> +	else if (irq >= irqrouter->input_count)
> 
> That "else" is pointless. You return above.

Ok.

> 
>> +		tango_set_swirq_enable(irqrouter,
>> +				       irq - irqrouter->input_count,
>> +				       enable);
>> +	else
>> +		tango_set_hwirq_enable(irqrouter,
>> +				       irq,
>> +				       enable);
> 
> This can be written way simpler.
> 
>      	if (irq >= irqrouter->input_count)
> 	   	irq -= irqrouter->input_count;
> 	tango_set_hwirq_enable(irqrouter, irq, enable);
>     	  
> Hmm?

Ok, but is it ok to modify function parameters?
I thought it was not recommended.
Indeed, some people may assume they are constants.

> 
>> +static int tango_set_irq_type(struct tango_irqrouter *irqrouter,
>> +			      int hwirq_in,
>> +			      u32 type,
>> +			      u32 parent_type)
>> +{
>> +	int err;
>> +
>> +	if (parent_type & (type & IRQ_TYPE_SENSE_MASK))
>> +		/* same polarity */
>> +		err = tango_set_irq_inversion(irqrouter, hwirq_in, 0);
>> +	else
>> +		/* invert polarity */
>> +		err = tango_set_irq_inversion(irqrouter, hwirq_in, 1);
> 
>   	bool invert = parent_type & (type & IRQ_TYPE_SENSE_MASK);
> 
>   	err = tango_set_irq_inversion(irqrouter, hwirq_in, invert);	
> 

Ok.

>> +
>> +	if (err < 0) {
>> +		DBGWARN("Failed to setup IRQ %d polarity\n", hwirq_in);
>> +		return err;
>> +	}
>> +
>> +	switch (type & IRQ_TYPE_SENSE_MASK) {
>> +	case IRQ_TYPE_EDGE_RISING:
>> +	case IRQ_TYPE_EDGE_FALLING:
>> +		DBGERR("Does not support edge triggers\n");
> 
> This really sucks. In fact you panic here while the code suggests that you
> print a DEBUG error and continue .....
> 

If I use panic() here, do I still have to use 'break' as well?

>> +		break;
>> +	case IRQ_TYPE_LEVEL_HIGH:
>> +		break;
> 
> You can spare that break.

Ok.
(before there were logs so the 'break' was necessary)

> 
>> +	case IRQ_TYPE_LEVEL_LOW:
>> +		break;
>> +	default:
>> +		DBGWARN("Invalid trigger mode 0x%x for hwirq(in) %d\n",
>> +			type, hwirq_in);
> 
> So here you continue with an error code, but above for EDGE you panic. I
> really don't get that logic.

Ok.
Do you suggest I just make the code panic for any error? I'm fine with that.

> 
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +
>> +static int tango_get_output_for_hwirq(struct tango_irqrouter *irqrouter,
>> +				      int hwirq_in,
>> +				      struct tango_irqrouter_output **out_val)
>> +{
>> +	struct tango_irqrouter_output *irqrouter_output;
>> +	int i;
>> +
>> +	if (!out_val)
>> +		return -EINVAL;
> 
> This is utter crap. Look at the call site:

I think I'm confused, so it is not recommended to check for NULL pointers before
dereferencing them? Regardless of how the current code calls the function?

> 
>> +	struct tango_irqrouter_output *irqrouter_output = NULL;
>> +
>> +	tango_get_output_for_hwirq(irqrouter, hwirq_in, &irqrouter_output);
>> +	if (!irqrouter_output)
>> +		goto handle_parent;
> 
> So out_val CANNOT be NULL. 

You are right, with current code it can't.
But I thought that it was a good practice to:
"be lenient with your input and strict with your output"

>What's the purpose of your return values if you
> ignore them?

I can make the function 'void'.

> 
>> +
>> +	/* Get the irqrouter_output for the hwirq */
> 
> This is another really helpful comment.

It is true that now that this is a function, the name of the function
speaks for itself, so I can remove it.

> 
>> +	for (i = 0; i < irqrouter->output_count; i++) {
>> +		int j;
>> +
>> +		irqrouter_output = &(irqrouter->output[i]);
>> +
>> +		for (j = 0; j < irqrouter_output->shared_count; j++) {
>> +			if (hwirq_in == irqrouter_output->shared_irqs[j])
>> +				goto found_router_output;
>> +		}
>> +	}
>> +	if (i == irqrouter->output_count) {
> 
> What other reason would be to get here than this?

You are right, relic of a refactoring.

> 
>> +		DBGWARN("Couldn't find hwirq mapping\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +found_router_output:
>> +
>> +	*out_val = irqrouter_output;
>> +	return 0;
>> +}
> 
> This whole thing can be condensed to:
> 
> static struct tango_irqrouter_output *
> tango_get_output_for_hwirq(struct tango_irqrouter *irqr, int hwirq_in)
> {
> 	struct tango_irqrouter_output *rout;
> 	int i, j;
> 
> 	/* Scan the outputs for a matching hwirq number */
> 	for (i = 0, rout = irqr->output; i < irqr->output_count; i++, rout++) {
> 		for (j = 0; j < rout->shared_count; j++) {
> 			if (rout->shared_irqs[j] == hwirq_in)
> 			   	return rout;
> 		}
> 	}
> 	return NULL;
> }
> 
> And the call site would become:
> 
>     	struct tango_irqrouter_output *rout;
> 
> 	rout = tango_get_output_for_hwirq(irqrouter, hwirq_in);
> 	if (!rout)
> 		goto handle_parent;
> 
> Hmm?

Ok.

> 
>> +/* 'irqchip' handling callbacks
>> + * Used for 'shared' IRQs, i.e.: IRQs that share a GIC input
>> + * This driver performs the IRQ dispatch based on the flags
>> + */
>> +
> 
> Multiline comments have this format:
> 	 
> /*
>  * First line
>  * ....
>  * Last line
>  */

Ok.

> 
>> +static void tango_irqchip_mask_irq(struct irq_data *data)
>> +{
>> +	struct irq_domain *domain = irq_data_get_irq_chip_data(data);
>> +	struct tango_irqrouter_output *irqrouter_output = domain->host_data;
>> +	struct tango_irqrouter *irqrouter = irqrouter_output->context;
>> +	int hwirq_in = (int)data->hwirq;
> 
> Huch? What's that silly typecast for? Why can't you use u32 for hwirq_in?

data->hwirq is of type irq_hw_number_t (unsigned long), I don't know if that 
can change in the future.

So do you think it is safe to use u32 for data->hwirq (and all other "irq"
occurrences)?

> 
>> +	tango_set_irq_enable(irqrouter, hwirq_in, 0);
>> +}
> 
>> +#ifdef CONFIG_SMP
>> +static int tango_irqchip_set_irq_affinity(struct irq_data *data,
>> +					  const struct cpumask *mask_val,
>> +					  bool force)
>> +{
>> +	struct irq_domain *domain = irq_data_get_irq_chip_data(data);
>> +	struct tango_irqrouter_output *irqrouter_output = domain->host_data;
>> +	struct irq_chip *parent_chip = irq_get_chip(irqrouter_output->virq);
>> +	struct irq_data *parent_data = irq_get_irq_data(irqrouter_output->virq);
>> +
>> +	DBGLOG("%s:%s(0x%p)\n",
>> +	       NODE_NAME(irq_domain_get_of_node(domain)), domain->name, domain);
>> +
>> +	if (parent_chip && parent_chip->irq_set_affinity)
>> +		return parent_chip->irq_set_affinity(parent_data,
>> +						     mask_val,
>> +						     force);
>> +	else
>> +		return -EINVAL;
> 
> You really can spare identation levels by writing it a bit more clever:
> 
>     	if (!parent || !parent->irq_set_affinity)
> 	   	 return -EINVAL;
> 
> 	return parent->irq_set_affinity(parent, mask, force);
> 
> You might notice that I shortened the variable names and it still is entirely
> clear what the code does.

Ok.

> 
> Aside of that mask_val is a clear misnomer.

Actually, this code comes from exynos-combiner.c and is part of the reason for
question 4 of the RFC:

----
4) Do I have to do something more to handle the affinity stuff?
----

> 
>> +}
>> +#endif
>> +
>> +
>> +static inline u32 tango_dispatch_irqs(struct irq_domain *domain,
>> +				      struct irq_desc *desc,
>> +				      u32 status,
>> +				      int base)
>> +{
>> +	u32 hwirq;
>> +	u32 virq;
> 
> Please put same types onto a single line

Ok.

> 
>> +
>> +	while (status) {
>> +		hwirq = __ffs(status);
>> +		virq = irq_find_mapping(domain, base + hwirq);
>> +		if (unlikely(!virq))
>> +			handle_bad_irq(desc);
>> +		else
>> +			generic_handle_irq(virq);
>> +
>> +		status &= ~BIT(hwirq);
>> +	}
>> +
>> +	return status;
> 
> Huch? status is guaranteed to be 0 here. So what's the point of this return
> value.

You are right, relic of a refactoring.
I wanted to do:

+		if (unlikely(!virq))
+			handle_bad_irq(desc);
+		else {
+			generic_handle_irq(virq);
+                       status_out &= ~BIT(hwirq);
+               }
+		status &= ~BIT(hwirq);

with status_out=status at the start of the function.

> 
>> +}
>> +
>> +static void tango_irqdomain_handle_cascade_irq(struct irq_desc *desc)
>> +{
>> +	struct irq_domain *domain = irq_desc_get_handler_data(desc);
>> +	struct tango_irqrouter_output *irqrouter_output = domain->host_data;
>> +	struct tango_irqrouter *irqrouter = irqrouter_output->context;
>> +	struct irq_chip *host_chip = irq_desc_get_chip(desc);
>> +	u32 i, status;
>> +	u32 swirq_status, irq_status[BITMASK_VECTOR_SIZE(ROUTER_INPUTS)];
>> +
>> +#if 0
>> +	DBGLOG("%s:%s(0x%p): irqrouter_output 0x%p, hwirq(out) %d\n",
>> +	       NODE_NAME(irq_domain_get_of_node(domain)), domain->name, domain,
>> +	       irqrouter_output, irqrouter_output->hwirq);
>> +#endif
>> +
>> +	chained_irq_enter(host_chip, desc);
>> +
>> +	raw_spin_lock(&(irqrouter->lock));
>> +	swirq_status = tango_readl(irqrouter, READ_SWIRQ_STATUS);
>> +	for (i = 0; i < BITMASK_VECTOR_SIZE(ROUTER_INPUTS); i++)
>> +		irq_status[i] = tango_readl(irqrouter,
>> +					    READ_SYS_IRQ_GROUP0 + i*4);
> 
>   i * 4		please

Ok.

> 
>> +	raw_spin_unlock(&(irqrouter->lock));
>> +
>> +	/* HW irqs */
> 
> And that comment tells us what?
> 
>> +	for (i = 0; i < BITMASK_VECTOR_SIZE(ROUTER_INPUTS); i++) {
>> +#if 0
>> +		DBGLOG("%d: 0x%08x (en 0x%08x, inv 0x%08x)\n",
>> +		       i,
>> +		       irq_status[i],
>> +		       irqrouter->irq_mask[0],
>> +		       irqrouter->irq_invert_mask[0]);
>> +#endif
>> +
>> +#define HANDLE_INVERTED_LINES(__irqstatus__, __x__) ((((~__irqstatus__) & irqrouter->irq_invert_mask[__x__]) & irqrouter->irq_mask[__x__]) | __irqstatus__)
>> +#define HANDLE_EN_AND_INV_MASKS(__irqstatus__, __y__) (HANDLE_INVERTED_LINES(__irqstatus__, __y__) & irqrouter->irq_mask[__y__])
> 
> What the hell does this unparseable macro mess? What's so wrong with a proper
> inline function?

Ok, I'll inline it.

> 
>> +
>> +		irq_status[i] = HANDLE_EN_AND_INV_MASKS(irq_status[i], i);
>> +		status = tango_dispatch_irqs(domain, desc, irq_status[i], i*32);
>> +		if (status & irq_status[i])
> 
> Can you pretty please explain how this can happen? It can't. Because you clear
> every single bit of status in tango_dispatch_irqs(). See above.

You are right that with the current code of tango_dispatch_irqs() it cannot happen,
but that's more of a typo. I wanted to have a way to print something from this
driver in the event of unhandled IRQs.

Just to understand something, is leaving some debug or checks somehow not recommended?

> 
> Please clean up that unholy mess of nonsensical checks and debug code.
> 

Ok.

>> +			DBGERR("%s: %d unhandled IRQs (as a mask) 0x%x\n",
>> +			       NODE_NAME(irq_domain_get_of_node(domain)),
>> +			       i,
>> +			       status & irq_status[i]);
>> +	}
>> +
>> +	/* SW irqs */
>> +	swirq_status &= irqrouter->swirq_mask;
>> +	status = tango_dispatch_irqs(domain, desc, swirq_status, 128);
> 
> 128 is the magic number pulled out of thin air or what?

I will replace that with irqrouter->input_count.

> 
>> +	if (status & swirq_status)
>> +		DBGERR("%s: Unhandled IRQs (as a mask) 0x%x\n",
>> +		       NODE_NAME(irq_domain_get_of_node(domain)),
>> +		       status & swirq_status);
> 
> See above.
> 
>> +
>> +	chained_irq_exit(host_chip, desc);
>> +}
>> +
>> +
>> +/**
>> + * tango_irqdomain_map - route a hwirq(in) to a hwirq(out).
>> + * NOTE: The hwirq(out) must have been already allocated and enabled on
>> + * the parent controller.
> 
> Please put notes AFTER the argument descriptions

The note refers to the title:
"tango_irqdomain_map - route a hwirq(in) to a hwirq(out)."

> 
>> + * @hwirq: HW IRQ of the device requesting an IRQ
>> + * if hwirq > inputs it is a SW IRQ
>> + * @virq: Linux IRQ (associated to the domain) to be given to the device
>> + * @domain: IRQ domain (from the domain, we get the irqrouter_output
>> + * in order to know to which output we need to route hwirq to)
> 
> And if you spend a few seconds to align this proper, then it becomes suddenly
> readable.
> 
> * @hwirq:	HW IRQ of the device requesting an IRQ
> *		if hwirq > inputs it is a SW IRQ
> * @virq:	Linux IRQ (associated to the domain) to be given to the device
> * @domain:	IRQ domain (from the domain, we get the irqrouter_output
> *		in order to know to which output we need to route hwirq to)
> 
> Hmm?

Ok.

> 
>> + */
>> +static int tango_irqdomain_map(struct irq_domain *domain,
>> +			       unsigned int virq,
>> +			       irq_hw_number_t hwirq)
>> +{
>> +	struct tango_irqrouter_output *irqrouter_output = domain->host_data;
>> +	struct tango_irqrouter *irqrouter = irqrouter_output->context;
>> +
>> +	DBGLOG("%s:%s(0x%p): hwirq(in) %d := virq %d, and route "
>> +	       "hwirq(in) %d => hwirq(out) %d (virq %d)\n",
>> +	       NODE_NAME(irq_domain_get_of_node(domain)),
>> +	       domain->name,
>> +	       domain,
>> +	       (u32)hwirq,
>> +	       virq,
>> +	       (u32)hwirq,
>> +	       irqrouter_output->hwirq,
>> +	       irqrouter_output->virq);
>> +
>> +	if (hwirq >= (irqrouter->input_count + irqrouter->swirq_count))
>> +		DBGERR("%s: Invalid hwirq(in) %d >= %d + %d\n",
>> +		       NODE_NAME(irq_domain_get_of_node(domain)),
>> +		       (u32)hwirq,
>> +		       irqrouter->input_count,
>> +		       irqrouter->swirq_count);
>> +	else if (hwirq >= irqrouter->input_count)
>> +		DBGLOG("Map swirq %ld\n", hwirq - irqrouter->input_count);
>> +
>> +	irq_set_chip_and_handler(virq,
>> +				 &tango_irq_chip_shared_ops,
>> +				 handle_level_irq);
>> +	irq_set_chip_data(virq, domain);
>> +	irq_set_probe(virq);
>> +
>> +	tango_set_irq_route(irqrouter, hwirq, irqrouter_output->hwirq);
>> +
>> +	return 0;
>> +}
>> +
>> +
>> +/**
>> + * tango_irqdomain_translate - used to select the domain for a given
>> + * irq_fwspec
> 
> It hardly selects the domain. @domain is handed in as a argument. And please
> get rid of that silly 'used to'. That just adds characters and no information.
> 
>     - Select the router section for a firmware spec
> 
> Hmm?

Ok, maybe I did not express it correctly.
The callback is called from irqdomain.c:irq_find_matching_fwspec() and each
domain is tested, if the callback returns non-zero, the current domain is considered
a "match" for the fwspec and returned by irq_find_matching_fwspec().

> 
>> + * @domain: a registered domain
>> + * @fwspec: an IRQ specification. This callback is used to translate the
>> + * parameters given as irq_fwspec into a HW IRQ and Type values.
> 
> fwspec is a callback?

No, the sentence refers to the callback below (tango_irqdomain_translate) that is
being described by this comment.
I can reformulate or remove the comment.

> 
>> + * @out_hwirq: 
>> + * @out_type:
> 
> Missing docs.....

Ok.

> 
>> + */
>> +static int tango_irqdomain_translate(struct irq_domain *domain,
>> +				     struct irq_fwspec *fwspec,
>> +				     unsigned long *out_hwirq,
>> +				     unsigned int *out_type)
>> +{
>> +	struct tango_irqrouter_output *irqrouter_output = domain->host_data;
>> +	struct tango_irqrouter *irqrouter = irqrouter_output->context;
>> +	irq_hw_number_t irq;
>> +	u32 domain_id, type;
>> +	int err;
>> +
>> +	DBGLOG("%s:%s(0x%p): argc %d for hwirq(out) %d\n",
>> +	       NODE_NAME(irq_domain_get_of_node(domain)),
>> +	       domain->name,
>> +	       domain,
>> +	       fwspec->param_count,
>> +	       irqrouter_output->hwirq);
>> +
>> +	err = tango_parse_fwspec(domain,
>> +				 fwspec,
>> +				 &domain_id,
>> +				 &irq,
>> +				 &type);
> 
> Please use the full 80 characters. Your patch does not become more valuable
> when you inflate the line count. It's more valuable when it is easy to read
> and understand.

Ok.

As a side note, does the 80 characters limit still makes sense? I mean, I would
imagine that anybody now is using wide screens capable of at least double that
amount.

> 
>> +	if (err < 0) {
>> +		DBGWARN("Failed to parse fwspec\n");
>> +		return err;
>> +	}
>> +
>> +	switch (domain_id) {
>> +	case SIGMA_HWIRQ:
>> +		DBGLOG("Request is for SIGMA_HWIRQ\n");
>> +		break;
>> +	case SIGMA_SWIRQ:
>> +		DBGLOG("Request is for SIGMA_SWIRQ\n");
>> +		irq += irqrouter->input_count;
>> +		break;
>> +	default:
>> +		DBGLOG("Request is for domain ID 0x%x (we are 0x%x)\n",
>> +		       domain_id,
>> +		       irqrouter_output->domain_id);
> 
> Huch what? You only ever handle HW and SW irqs and now you happily proceed if
> the id is something else. Consistent error handling is optional and can be
> replaced by random debug prints, right?

You are right that this case should never happen as the 'select' callback must
have replied correctly.
This is more of a relic when I did not had access to the 'select' callback.
(I was working on 4.4)

> 
>> +		break;
>> +	};
>> +
>> +	*out_hwirq = irq;
>> +	*out_type  = type & IRQ_TYPE_SENSE_MASK;
>> +
>> +	DBGLOG("hwirq %d type 0x%x\n", (u32)*out_hwirq, (u32)*out_type);
>> +
>> +	return 0;
>> +}
>> +
>> +
>> +/**
>> + * tango_irqdomain_select - used to select the domain for a given irq_fwspec
>> + * @domain: a registered domain
>> + * @fwspec: an IRQ specification. This callback should return zero if the
>> + * irq_fwspec does not belong to the given domain. If it does, it should
>> + * return non-zero.
> 
> Callback????

Ok.

> 
>> + * In practice it will return non-zero if the irq_fwspec matches one of the
>> + * IRQs shared within the given domain.
>> + * @bus_token: a bus token
>> + */
>> +static int tango_irqdomain_select(struct irq_domain *domain,
>> +				  struct irq_fwspec *fwspec,
>> +				  enum irq_domain_bus_token bus_token)
>> +{
>> +	struct tango_irqrouter_output *irqrouter_output = domain->host_data;
>> +	struct tango_irqrouter *irqrouter = irqrouter_output->context;
>> +	irq_hw_number_t irq;
>> +	u32 domain_id, type;
>> +	int err;
>> +
>> +	DBGLOG("%s:%s(0x%p): argc %d, 0x%p, bus 0x%x\n",
>> +	       NODE_NAME(irq_domain_get_of_node(domain)), domain->name, domain,
>> +	       fwspec->param_count, fwspec->fwnode, bus_token);
>> +
>> +	DBGLOG("router 0x%p, output 0x%p\n", irqrouter, irqrouter_output);
>> +
>> +	err = tango_parse_fwspec(domain, fwspec, &domain_id, &irq, &type);
>> +	if (err < 0)
>> +		return 0;
>> +
>> +	switch (domain_id) {
>> +	case SIGMA_HWIRQ:
>> +		DBGLOG("Request is for SIGMA_HWIRQ\n");
>> +		break;
>> +	case SIGMA_SWIRQ:
>> +		DBGLOG("Request is for SIGMA_SWIRQ\n");
>> +		break;
>> +	default:
>> +		DBGLOG("Request is for domain ID 0x%x (we are 0x%x)\n",
>> +		       domain_id,
>> +		       irqrouter_output->domain_id);
>> +		break;
>> +	};
>> +
>> +	if (!irqrouter->implicit_groups) {
>> +		int i;
>> +
>> +		/* Check if the requested IRQ belongs to those listed
>> +		 * to be sharing the output assigned to this domain
> 
> Your using of 'domain' is utterly confusing. Please use something like
> hwdomain or whatever which makes it clear that this is something else than the
> irq domain. I tripped over this several times now.

Ok.

> 
>> +		 */
>> +		if (irqrouter_output->shared_count <= 0) {
>> +			DBGLOG("Not shared IRQ line?\n");
>> +			return 0;
>> +		}
>> +
>> +		for (i = 0; i < irqrouter_output->shared_count; i++) {
>> +			if (irq == irqrouter_output->shared_irqs[i]) {
>> +				DBGLOG("Match: IRQ %lu\n", irq);
>> +				return 1;
>> +			}
>> +		}
>> +	} else {
>> +		/* Otherwise, check if the domain_id given matches
>> +		 * the one assigned to this output
>> +		 */
>> +		if (domain_id == irqrouter_output->domain_id) {
>> +			DBGLOG("Match: Domain ID %d\n", domain_id);
>> +			return 1;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +
>> +/* 'irqrouter' handling callbacks
>> + * Used for 'direct' IRQs, i.e.: IRQs that are directly routed to the GIC
>> + * This driver does not dispatch the IRQs, the GIC does.
>> + */
>> +
>> +
>> +static void tango_irqrouter_mask_irq(struct irq_data *data)
>> +{
>> +	struct irq_domain *domain = irq_data_get_irq_chip_data(data);
>> +	struct tango_irqrouter *irqrouter = domain->host_data;
>> +	int hwirq_in = (int)data->hwirq;
>> +
>> +	DBGLOG("%s:%s(0x%p) hwirq(in) %d\n",
>> +	       NODE_NAME(irq_domain_get_of_node(domain)),
>> +	       domain->name,
>> +	       domain,
>> +	       hwirq_in);
>> +
>> +	tango_set_irq_enable(irqrouter, hwirq_in, 0);
>> +
>> +	irq_chip_mask_parent(data);
>> +}
>> +
>> +
>> +static void tango_irqrouter_unmask_irq(struct irq_data *data)
>> +{
>> +	struct irq_domain *domain = irq_data_get_irq_chip_data(data);
>> +	struct tango_irqrouter *irqrouter = domain->host_data;
>> +	int hwirq_in = (int)data->hwirq;
>> +
>> +	DBGLOG("%s:%s(0x%p) hwirq(in) %d\n",
>> +	       NODE_NAME(irq_domain_get_of_node(domain)),
>> +	       domain->name,
>> +	       domain,
>> +	       hwirq_in);
>> +
>> +	tango_set_irq_enable(irqrouter, hwirq_in, 1);
>> +
>> +	irq_chip_unmask_parent(data);
>> +}
>> +
>> +
>> +static int tango_irqrouter_set_irq_type(struct irq_data *data,
>> +					unsigned int type)
>> +{
>> +	struct irq_domain *domain = irq_data_get_irq_chip_data(data);
>> +	struct tango_irqrouter_output *irqrouter_output = NULL;
>> +	struct tango_irqrouter *irqrouter = domain->host_data;
>> +	int hwirq_in = (int)data->hwirq;
>> +	u32 parent_type;
>> +
>> +	DBGLOG("%s:%s(0x%p) type 0x%x for hwirq(in) %d\n",
>> +	       NODE_NAME(irq_domain_get_of_node(domain)),
>> +	       domain->name,
>> +	       domain,
>> +	       type,
>> +	       hwirq_in);
>> +
>> +	tango_get_output_for_hwirq(irqrouter, hwirq_in, &irqrouter_output);
>> +	if (!irqrouter_output)
>> +		goto handle_parent;
> 
> So if there is no output, then you unconditionally set the type on the
> parent. If that's correct, this needs a big fat comment at least.

Ok.
This is related to question 2 of my RFC:

----
2) In order to get a virq mapping for the domains associated with the outputs
(the outputs connected to the GIC) I request a sort of "Fake HW IRQ"
See irq-tango_v4.c:1400

/* To request a virq we need a HW IRQ, use a "Fake HW IRQ" */
hwirq = index + irqrouter->input_count + irqrouter->swirq_count;

This feels a bit like a hack, so suggestions are welcome.
----

Indeed, this callback will be called once when requesting a "fake HW IRQ" so
that alloc() callback is called and we get a free output (a GIC input) so 
that we can later route IRQs to it.
And another time when an actual HW IRQ is requested, in which case the
tango_set_irq_type() needs to be called so that we can see if inversion needs
to be implemented.

Actually, if I don't deal with inversion and consider there will never be
mismatches between the level setup on the GIC input and that one of the
device requesting the IRQ to be routed to said GIC input, then I could just
use irq_chip_set_type_parent() as callback directly.

> 
>> +	parent_type = (irqrouter_output->hwirq_level & IRQ_TYPE_SENSE_MASK);
>> +	tango_set_irq_type(irqrouter, hwirq_in, type, parent_type);
>> +
>> +handle_parent:
>> +	return irq_chip_set_type_parent(data, type);
>> +}
> 
> I'm stopping here. You can apply the above to the rest of the code as well.
> 
> 

Thanks again for your review.
Best regards,

Sebastian

Re: [RFC PATCH v1] irqchip: add support for SMP irq router

[Sebastian Frias]

Hi Jason,

On 07/06/2016 06:28 PM, Jason Cooper wrote:
> Hi Sebastian,
> 
> On Wed, Jul 06, 2016 at 01:37:21PM +0200, Sebastian Frias wrote:
>> On 07/05/2016 06:16 PM, Jason Cooper wrote:
>>>> Come to think of it, I'm not sure the *name* of the file documenting
>>>> a binding is as important to DT maintainers as the compatible string.
>>>
>>> Correct.  devicetee compatible strings need to be as specific as
>>> possible.  
>>
>> Specific with respect to what thing? To the HW module they are describing
>> (USB, IRQ controller, etc.) or to the chip the HW module it is embedded
>> into?
> 
> The compatible string uniquely identifies an interface between an IP
> block and the software (the devicetree binding).  We use the most
> specific model number or name we can for that IP block when we create
> the binding and the compatible string.

Ok, so we agree that the string identifies the HW block itself, and not the
chip in which it is embedded into.

> 
> If future SoCs come out and the IP block contained within, regardless of
> identifier, is compatible with the existing binding, then we can reuse
> it.  This is what I was trying to show with my little chart quoted
> below.

I understand, is just that the chart was using chip IDs so I was a bit
confused.

> 
> For ethernet and other major blocks it's easy because they have their
> own model numbers and such.  For the smaller blocks, like irqchips, we
> have to use the model number or unique name of the SoC we first found it
> in.

Ok, but I'm not sure I understand the logic, I thought (and it is my
understanding of your previous paragraphs) that the string is supposed to
identify the IP block (or HW module) itself.

That seems more generic and flexible than attaching it some "external"
property, like the name of the chip the HW module happens to be embedded into.

Normally such "small HW blocks" you talk about, because they are not generic
enough, probably require documentation/engineering effort from the SoC
manufacturer, so I would guess that the naming can be decided with the SoC
them.

Why is there a difference on the naming convention for "smaller blocks, 
like irqchips" and other blocks (ethernet, USB, UART, etc.) ?

>> A SoC is composed of several HW modules, some are shared among different
>> manufacturers (i.e.: "generic-xhci"), and some are shared among different
>> product lines of the same manufacturer (i.e.: "sigma,smp,irqrouter").
> 
> Yes, that's why it's critical to be specific.  We want to reuse drivers
> where it makes sense.  We can if the interface is the same.

Ok, so is "sigma,smp,irqrouter" good or not?

> 
>> And the DT for a given chip should describe the collection of HW modules
>> that make up the SoC, regardless of what chip introduced them or what
>> other chip uses it. Why would that be relevant anyway?
> 
> Because the first time we see a new IP block, we can't predict the
> future.  We don't know where it's going to be reused.  We don't know
> when it's going to be changed, or how.  So we mark it as specifically as
> we can based on the information we have when we first encounter it.
> 
> When a new version comes out, we see if the interface is still
> compatible.  If it is, there's nothing to do.  If it changed, we see if
> we can address it by adding a property to the existing binding.  Failing
> that, we create a new compatible string to indicate a new interface.

I see. I think this process is ok for when there's no information, like when
somebody reverse engineer a HW block of a chip of a given line of chips.
In that case I can understand that there's no way to know if previous,
similar or future chips use the same block, nor its name.

Yet, I still fail to see the reason to attach the chip ID to what clearly
are submodules. Especially when one can assume that such submodules can
be shared on different chip IDs.

> 
>> By that reasoning, I also think that drivers/net/ethernet/aurora/nb8800.c
>> should have had a string like "aurora,nb8800,sigma" or something, to
>> specify that it is a NB8800 from Aurora integrated in a Sigma platform.
> 
> The fact that it is in the Sigma dts file tells this already.  Based on
> the above, did the interface change when adding it to the sigma
> platform?  Can the binding be reused?

I think it did not change.
Actually, I think only Sigma uses it, as the driver only has Sigma-specific
initialisations for two Sigma-specific strings.

> 
>> That way it can behave as vanilla NB8800 when the string is "aurora,nb8800"
>> and it can adapt its behaviour to Sigma's platform when a different string
>> (like "aurora,nb8800,sigma") is used in the DT.
> 
> I would have to look at the specifics of "adapt its behaviour" in order
> to determine wether a new compatible is warranted, or maybe just add a
> property to the binding.

Ok.

> 
>> Later if Sigma modified the nb8800 HW, it could be "aurora,nb8800,sigma-v2".
>> The same way, if later Sigma used a different Aurora HW module, the DT
>> would say "aurora,nb2000,sigma" (to signal that another driver is required)
> 
> The compatible string is important, but we try not to overload it.
> There are many flexible ways we can indicate properties or quirks in the
> binding.

Ok.

> 
>> Instead, drivers/net/ethernet/aurora/nb8800.c has "sigma,smp8642-ethernet"
>> string, which ties it to a particular chip, and I don't see how does that
>> convey version about the module or other relevant information; Plus it is
>> confusing if the same module is then embedded on a SMP8756 chip.
> 
> Sorry, it's just understood that the compatible string means they have
> the same binding/interface.

Ok, so same binding/interface = same module, so we could as well name the
compatible string after the module's name, and that, regardless of which
chip it is embedded into, right? Obviously some prefix can be used, and
that's why I had proposed "sigma,smp".

> 
>> Would you mind explaining how does that contrasts with the logic used
>> by DT naming conventions?
> 
> I think you may be looking at DT the wrong way :-P  It's not an
> inventory of components for the end user to look at.  It a list of
> hardware interfaces and how they're attached; for the OS to interpret
> and use.

Exactly, and such hardware interfaces (usually HW IP blocks) are usually
shared by a number of chips, sometimes from different companies, sometimes
from the same company.

That is why I find strange to use a given chip ID to describe some HW IP
submodule.

Best regards,

Sebastian

Re: [RFC PATCH v2] irqchip: add support for SMP irq router

[Marc Zyngier]

Thomas already commented on some aspects of the code, so I'm not going
to do another review (I'll do the next round).

On 20/07/16 12:06, Sebastian Frias wrote:
> Hi Thomas,
> 
> Thanks for your comments.
> I appreciate the time you dedicated to the review.
> I will take your comments into account, in the meanwhile, and since this was
> a RFC, would it be possible to also get some feedback/comments from a
> conceptual point of view?
> 
> I'm copy/pasting some of the questions I attached to the RFC email here:
> 
> ----
> 2) In order to get a virq mapping for the domains associated with the outputs
> (the outputs connected to the GIC) I request a sort of "Fake HW IRQ"
> See irq-tango_v4.c:1400
> 
> 	/* To request a virq we need a HW IRQ, use a "Fake HW IRQ" */
> 	hwirq      = index + irqrouter->input_count + irqrouter->swirq_count;

Why are you even trying to establish a route from a made up interrupt to
a GIC line as the driver is just creating domains? Haven't you realized
that everything gets created on demand, as DT interrupt specifiers gets
parsed?

So the only time where you create a irq_fwspec out of thin air is when
you actually allocate *one* interrupt. And that's the only time. You
don't pre-populate stuff, you don't pre-allocate stuff.

> This feels a bit like a hack, so suggestions are welcome.

Just look at all the existing drivers in the tree that use stacked
domains. They are all based on the same template. At least try and
follow it.

> 
> 3) The file is called 'irq-tango_v4.c' but I think it should match the
> compatible string, so I was thinking of renaming:
> irq-tango_v4.c => irq-sigma_smp_irqrouter.c
> irq-tango_v4.h => irq-sigma_smp_irqrouter.h
> 
> What do you think?

And why should it match? Am I going to rename the GIC driver to be
"arm,gic-400_or_arm,arm11mp-gic_or_arm,arm1176jzf-devchip-gic_or_...c"?

No.

> 
> 4) Do I have to do something more to handle the affinity stuff?
> 
> 6) More of a theoretical question:
> I have to #include <dt-bindings/interrupt-controller/irq-tango_v4.h> from

No you don't. This is for DT people to play with, and I don't care about
it in the irqchip code.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [RFC PATCH v1] irqchip: add support for SMP irq router

[Jason Cooper]

Hey Sebastian,

On Wed, Jul 20, 2016 at 01:42:45PM +0200, Sebastian Frias wrote:
> On 07/06/2016 06:28 PM, Jason Cooper wrote:
> > On Wed, Jul 06, 2016 at 01:37:21PM +0200, Sebastian Frias wrote:
> >> On 07/05/2016 06:16 PM, Jason Cooper wrote:
> >>>> Come to think of it, I'm not sure the *name* of the file documenting
> >>>> a binding is as important to DT maintainers as the compatible string.
> >>>
> >>> Correct.  devicetee compatible strings need to be as specific as
> >>> possible.  
> >>
> >> Specific with respect to what thing? To the HW module they are describing
> >> (USB, IRQ controller, etc.) or to the chip the HW module it is embedded
> >> into?
> > 
> > The compatible string uniquely identifies an interface between an IP
> > block and the software (the devicetree binding).  We use the most
> > specific model number or name we can for that IP block when we create
> > the binding and the compatible string.
> 
> Ok, so we agree that the string identifies the HW block itself, and not the
> chip in which it is embedded into.

If *possible*.  When we converted kirkwood and the other legacy Marvell
ARMv5 SoCs to devicetree, the most specific way we could identify the
irq controller was 'marvell,orion-intc'.  Orion being the name of the
first platform it was seen on.

When that was chosen, mv68xx0, orion5x, kirkwood, and I think dove were
already deployed to market.  So we had a *lot* more latent information
to look at.

Now that SoC manufacturers are doing a much better job mainlining code,
the situation is reversed.  There's little information on what is
actually delivered to the market (because code is getting posted *prior*
to SoC deployment), but we have better access to specifications and
software devs.

So, if we have access to specific IP block identifiers, we'll use them.
If we don't, we're not going to let that hold up the train waiting for
the 'perfect' binding.

> >> A SoC is composed of several HW modules, some are shared among different
> >> manufacturers (i.e.: "generic-xhci"), and some are shared among different
> >> product lines of the same manufacturer (i.e.: "sigma,smp,irqrouter").
> > 
> > Yes, that's why it's critical to be specific.  We want to reuse drivers
> > where it makes sense.  We can if the interface is the same.
> 
> Ok, so is "sigma,smp,irqrouter" good or not?

If you have a unique identifier for the irqchip, use it.  If not, the
convention is to use the identifier of the SoC.

thx,

Jason.

Re: [RFC PATCH v2] irqchip: add support for SMP irq router

[Thomas Gleixner]

On Wed, 20 Jul 2016, Sebastian Frias wrote:
> ----
> 2) In order to get a virq mapping for the domains associated with the outputs
> (the outputs connected to the GIC) I request a sort of "Fake HW IRQ"
> See irq-tango_v4.c:1400
> 
> 	/* To request a virq we need a HW IRQ, use a "Fake HW IRQ" */
> 	hwirq      = index + irqrouter->input_count + irqrouter->swirq_count;
> 
> This feels a bit like a hack, so suggestions are welcome.

Well, look at it from the hardware level

                  |----------------|
		  |               0|---- Device irqs
	 	  |               1|----
 |----------| 	  |               2|----
 |       0  |-----|               3|----
 |       1  |-----|    Router     .|----
 |   GIC ...|-----|               .|----
 |       N  |-----|               .|----
 |----------|	  |               .|----
 		  |               .|----
		  |               X|----
                  |----------------|

So you have X hardware irqs in your router chip and the GIC has N. When an
interrupt is created then we 
	  
	- allocate the virq number
	- associate the hwirq number of the router (input side)
	- route it to a hwirq number of the GIC (output side)

So what kind of magic numbers do you need for that? I don't see any.

The only information which is interesting is whether the router output is
shared between several router inputs or not because that might short cut
interrupt handling. Though you can start with the simple assumption that every
output is shared. That's a few cpu cycles overhead in the irq delivery path,
but a great simplification of the code in the first place.

Your device tree describes 

     1) the association of the device irq to the router irq
     2) the routing between router input and output

#1 is standard device tree magic

#2 is a property of your router irq chip

   There is no requirement to describe all routes, you can limit it to those
   routes which you want to be exclusive. The rest can be assigned
   automatically.

   Pseuso code:

	#define ROUTE_NONE	0xFFFFFFFF

	struct router {
		u32	routes[NR_INPUTS];
		bool	exclusive[NR_OUTPUTS];
		u32	nextout;
	};

   init()
	struct router *router = kzalloc(sizeof(*router));

	/* Clear all routes */
	memset(router->routes, 0xFF, sizeof(router->routes);

	/* Scan device tree entries */
	for (info = node_get_next_route(node); info; ) {
		/* TODO: Sanity check info->in and info->out */
		router->routes[info->in] = info->out;
		router->exclusive[info->out] = true;
		info = node_get_next_route(node);
	}

   interrupt setup()
   
	/*
	 * We get the hwirq of the router input	in the DT/FW descriptor.
	 * Check whether the interrupt is routed by DT or should be
	 * assigned a route dynamically.
	 */
	if (router->routes[hwirq] != ROUTE_NONE) {
		outirq = router->routes[hwirq];		   
	} else {
		/*
		 * Try to find the next non exclusive output
		 */
		outirq = router->nextout;
		while (router->exclusive[outirq]) {
			outirq++;
			if (outirq == NR_OUTPUTS)
				outirq = 0;
			if (outirq == router->nextout)
				return -ENOSPC;
		}
		router->nextout = outirq + 1;
		if (router->nextout == NR_OUTPUTS)
			router->nextout = 0;
 	}
	
	/* Do the magic hardware initialization .... */

Hmm?	
 
> 3) The file is called 'irq-tango_v4.c' but I think it should match the
> compatible string, so I was thinking of renaming:
> irq-tango_v4.c => irq-sigma_smp_irqrouter.c
> irq-tango_v4.h => irq-sigma_smp_irqrouter.h
> 
> What do you think?

That's the least of my worries as long as the filename is unique and can be
associated to the hardware in a sensible way.
 
> 4) Do I have to do something more to handle the affinity stuff?

Well, I have no idea how your hardware works, but I assume that the affinity
is solely handled by the GIC. So delegating the affinity setting to the GIC is
all you can do.

> 6) More of a theoretical question:
> I have to #include <dt-bindings/interrupt-controller/irq-tango_v4.h> from
> code that is not in the Linux tree. That's fine for now, but if in the future
> there are other irq controllers and another header is required, how can the
> code know which header to #include ?
> Are we supposed to #include all of them, and then somehow detect which module
> is actually active and act accordingly? If so, can we detect which module
> matched and probed correctly to know which controller version we need to
> talk to?

-ENOPARSE

> What the driver does is:
> - creates a domain hierarchy attached to its parent domain (the GIC)
>   - the callbacks for this domain hierarchy will be used to deal with IRQs
>   "directly" routed to the GIC (i.e.: exclusive IRQ lines routed to the GIC
>   and not shared).
>   In this case the GIC dispatches the interrupts.
> - create linear domains attached to this driver's node.
>   - the callbacks for these linear domains will be used to deal with shared
>   IRQs (they share the routing to one of the GIC inputs).
>   In this case this driver dispatches the interrupts after reading the flags.
> 
> It is such concept, and then its implementation, that I would need some more
> advise with, because, as stated in the questions above, it still does not
> feels right, especially question 2.

The non shared case is merily an optimization of the shared case, but I
recommend to omit it for now and just deal with a very straight forward
implementation of the recommendation I outlined above. Making the non-shared
case special is simple once you got the above right.

> >> +#if 0
> > 
> > Don't even think about posting code with "#if 0" or such in it. 
> > 
> 
> The idea behind these macros was:
> - to easily enable/disable them.
> - to be more consistent with other code written at Sigma: this is a
> Sigma-only driver.

I don't care about Sigmas coding habits. This is kernel code and not
Sigma-only playground.
 
> > pr_debug() is there for a reason.
> 
> Yes, but doesn't pr_debug() requires changes to the Makefile?

# git grep pr_debug Documentation/
 
> > 
> >> +#if 0
> >> +#define SHORT_OR_FULL_NAME full_name
> >> +#else
> >> +#define SHORT_OR_FULL_NAME name
> >> +#endif
> > 
> > Use one and be done with it. Really.
> 
> By making the choice explicit, the person reading/debugging is aware of the possibilities.
> Picking "full_name" or "name" would likely prevent the person from knowing it has other debug options.
> 

This does prevent absolutely nothing. Random #if 0 all over the place is a
nightmare and nothing else. And what do you win by printing one or the other
name? Absolutely nothing, really. Again, it might be Sigmas way to obfuscate
code and at the same time claiming that it makes it better to read/debug, but
that choice of coding style/habits have absolutely no place in proper kernel
code.

> >> +
> >> +#define NODE_NAME(__node__) (__node__ ? __node__->SHORT_OR_FULL_NAME :	\
> >> +			     "<no-node>")
> > 
> > Sigh. pr_xxx("%s", NULL) prints <null>. That's really sufficient for your
> > debug/error handling.
> 
> The code is checking for __node__ to be non-NULL before dereferencing it.

  pr_xxx("%s", node->name ? node->name: NULL)

What's wrong with explicitely writing stuff into the code where people can see
it instead of obfuscating everything with gazillion of macros.
 
> >> +static inline u32 tango_readl(struct tango_irqrouter *irqrouter,
> >> +			      int reg)
> >> +{
> >> +	u32 val = readl_relaxed(irqrouter->base + reg);
> >> +	/*DBGLOG("r[0x%08x + 0x%08x = 0x%08x] = 0x%08x\n",
> >> +	  irqrouter->base, reg, irqrouter->base + reg, val);*/
> >> +	return val;
> > 
> > Please get rid of this debug nonsense.
> 
> Ok, although I can guarantee it was really useful to determine and prove
> that the HW documentation of the module was wrong...

That's fine. But that commented out stuff has no place in proper code. Use
pr_debug() if you really think that this information is valuable for debugging
beyond the initial experimentation. If it's of no use for the daily trouble
shooting with users, get rid of it.

> >> +static inline void tango_set_swirq_enable(struct tango_irqrouter *irqrouter,
> >> +					  int swirq,
> >> +					  bool enable)
> >> +{
> >> +	u32 offset = SWIRQ_ENABLE;
> > 
> > If you name that varible 'reg' then it becomes obvious what it does.
> 
> Well, it was to add a sort of "consistency" between the code for SW IRQs and
> HW IRQs.

You try to force different things into the same implementation, which is
always a bad sign.

> > 
 >> +	u32 value = tango_readl(irqrouter, offset);
> >> +	u32 swirq_bit_index = swirq % SWIRQ_COUNT;
> >> +
> >> +#if 1
> >> +	DBGLOG("%smask swirq(in) %d : current regvalue 0x%x\n",
> >> +	       enable ? "un":"",
> >> +	       swirq, value);
> >> +#endif
> >> +
> >> +	if (enable) {
> >> +		/* unmask swirq */
> > 
> > The whole code lacks comments, but here you document the obvious ....
> 
> I will remove the comments.

Instead of just removing the comments, please add comments where the stuff is
non obvious.
 
> >> +static inline void tango_set_swirq_inversion(struct tango_irqrouter *irqrouter,
> >> +					     int swirq,
> >> +					     bool invert)
> >> +{
> >> +
> >> +	DBGLOG("swirq(in) %d %s inverted\n", swirq, invert ? "":"not");
> >> +
> >> +	if (invert)
> >> +		DBGERR("SW IRQs cannot be inverted!\n");
> > 
> > Groan.
> 
> I rather keep this to catch typos in code requesting SW IRQs.

Come on. Is that coding kindergarden or what?
 
> >> +	if (irq_out < 0) {
> > 
> > This logic is utterly confusing, really. What's a negative interrupt number?
> 
> It is a sort of "magic value" to disable the IRQ and the routing.

And of course completely undocumented. Again you try to implement stuff which
should be seperate into a single implementation and end up with an unreadable
and therefor unmaintainable mess.
 
> >> +	} else
> > 
> >   } else {
> > 
> >   please
> 
> I think I'm confused, we need to use {} for single statements on "else" blocks,
> but not on "for" blocks?

      for (i = 0; i < MAX; i++)
      	  do_stuff(i);

      for (i = 0; i < MAX; i++) {
      	  do_stuff(i);
	  do_more_stuff(i);
      }

      if (bla < MAX)
      	   do_stuff(bla);
      else
	   do_something_else(bla);

      if (bla < MAX) {
      	   do_stuff(bla);
	   do_more_stuff(bla);
      } else {
	   do_something_else(bla);
	   do_more_stuff(bla);
      }

      if (bla < MAX) {
      	   do_stuff(bla);
	   do_more_stuff(bla);
      } else {
	   do_something_else(bla);
      }

      if (bla < MAX) {
      	   do_stuff(bla);
      } else {
	   do_something_else(bla);
	   do_more_stuff(bla);
      }

Now compare that to:
      
      if (bla < MAX)
      	   do_stuff(bla);
      else {
	   do_something_else(bla);
	   do_more_stuff(bla);
      }
 
It's harder to parse simply because the 'else {' is assymetric. Ditto for

      if (bla < MAX) {
      	   do_stuff(bla);
	   do_more_stuff(bla);
      } else
	   do_something_else(bla);

Ok?

> >> +	if (irq_out < 0) {
> >> +		tango_set_irq_enable(irqrouter,
> >> +				     irq_in,
> >> +				     0);
> > 
> > This fits in a single line.
> 
> Ok.
> Actually, I was wondering if for consistency it wouldn't be better if all
> function calls had one argument per line?

Err no. You waste precious vertical space and therefor context.
 
> >> +static inline int tango_set_irq_enable(struct tango_irqrouter *irqrouter,
> >> +				       int irq,
> >> +				       bool enable)
> >> +{
> >> +	if (irq >= (irqrouter->input_count + irqrouter->swirq_count))
> >> +		return -EINVAL;
> > 
> > How can that happen? Paranoia programming?
> 
> No, actually it can happen, that's the reason for question 2 of my RFC:
> 
> ----
> 2) In order to get a virq mapping for the domains associated with the outputs
> (the outputs connected to the GIC) I request a sort of "Fake HW IRQ"
> See irq-tango_v4.c:1400
> 
> /* To request a virq we need a HW IRQ, use a "Fake HW IRQ" */
> hwirq = index + irqrouter->input_count + irqrouter->swirq_count;
> 
> This feels a bit like a hack, so suggestions are welcome.

It is a hack. See above.

> > This can be written way simpler.
> > 
> >      	if (irq >= irqrouter->input_count)
> > 	   	irq -= irqrouter->input_count;
> > 	tango_set_hwirq_enable(irqrouter, irq, enable);
> >     	  
> > Hmm?
> 
> Ok, but is it ok to modify function parameters?
> I thought it was not recommended.
> Indeed, some people may assume they are constants.

They are only constant when they have a const qualifier in the function
prototype. Random assumptions of random people are irrelevant. You can use an
intermediate local variable and do the above if that makes you feel
better. That still will be way better readable and understandable code.
 
> >> +
> >> +	if (err < 0) {
> >> +		DBGWARN("Failed to setup IRQ %d polarity\n", hwirq_in);
> >> +		return err;
> >> +	}
> >> +
> >> +	switch (type & IRQ_TYPE_SENSE_MASK) {
> >> +	case IRQ_TYPE_EDGE_RISING:
> >> +	case IRQ_TYPE_EDGE_FALLING:
> >> +		DBGERR("Does not support edge triggers\n");
> > 
> > This really sucks. In fact you panic here while the code suggests that you
> > print a DEBUG error and continue .....
> > 
> 
> If I use panic() here, do I still have to use 'break' as well?

It does not matter much.
 
> >> +	default:
> >> +		DBGWARN("Invalid trigger mode 0x%x for hwirq(in) %d\n",
> >> +			type, hwirq_in);
> > 
> > So here you continue with an error code, but above for EDGE you panic. I
> > really don't get that logic.
> 
> Ok.
> Do you suggest I just make the code panic for any error? I'm fine with that.

I ask for consistent handling. Either you always panic or you try to handle
all errors gracefully. I prefer the latter because if it's not a essential
interrupt then you still have the chance to retrieve debug info.

panic or BUG() are the last resort when there is no way to keep the machine
alive and any action would perhaps cause even more fatal problems. That's
hardly the case when an interrupt cannot be set up.
 
> >> +	if (!out_val)
> >> +		return -EINVAL;
> > 
> > This is utter crap. Look at the call site:
> 
> I think I'm confused, so it is not recommended to check for NULL pointers
> before dereferencing them? Regardless of how the current code calls the
> function?

There is nothing wrong with defensive programming, but just adding checks to
every function bloats the code for no value. If you have a helper with a
single callsite, then such a check is just pointless and only there to make
you feel good or whatever. Global functions which can be called by random code
certainly want to have such checks, but then the callers are also required to
check the return values.

> > 
> >> +	struct tango_irqrouter_output *irqrouter_output = NULL;
> >> +
> >> +	tango_get_output_for_hwirq(irqrouter, hwirq_in, &irqrouter_output);
> >> +	if (!irqrouter_output)
> >> +		goto handle_parent;
> > 
> > So out_val CANNOT be NULL. 
> 
> You are right, with current code it can't.
> But I thought that it was a good practice to:
> "be lenient with your input and strict with your output"

See above.
 
> >What's the purpose of your return values if you
> > ignore them?
> 
> I can make the function 'void'.

I made you a proposal for a useful replacement of that thing. See below.
 
> > This whole thing can be condensed to:
> > 
> > static struct tango_irqrouter_output *
> > tango_get_output_for_hwirq(struct tango_irqrouter *irqr, int hwirq_in)
> > {
> > 	struct tango_irqrouter_output *rout;
> > 	int i, j;
> > 
> > 	/* Scan the outputs for a matching hwirq number */
> > 	for (i = 0, rout = irqr->output; i < irqr->output_count; i++, rout++) {
> > 		for (j = 0; j < rout->shared_count; j++) {
> > 			if (rout->shared_irqs[j] == hwirq_in)
> > 			   	return rout;
> > 		}
> > 	}
> > 	return NULL;
> > }
> > 
> > And the call site would become:
> > 
> >     	struct tango_irqrouter_output *rout;
> > 
> > 	rout = tango_get_output_for_hwirq(irqrouter, hwirq_in);
> > 	if (!rout)
> > 		goto handle_parent;
> > 
> > Hmm?
> 
> Ok.
 
> >> +static void tango_irqchip_mask_irq(struct irq_data *data)
> >> +{
> >> +	struct irq_domain *domain = irq_data_get_irq_chip_data(data);
> >> +	struct tango_irqrouter_output *irqrouter_output = domain->host_data;
> >> +	struct tango_irqrouter *irqrouter = irqrouter_output->context;
> >> +	int hwirq_in = (int)data->hwirq;
> > 
> > Huch? What's that silly typecast for? Why can't you use u32 for hwirq_in?
> 
> data->hwirq is of type irq_hw_number_t (unsigned long), I don't know if that 
> can change in the future.

And because something might change in the future you slap a type cast on
it. Which will nicely prevent a compiler warning when the conversion after the
change is crap.

No. We only do type casts when there is a real reason and not something like
'might change in the future'.

Guess what happens if we change data->hwirq to be a pointer? Your typecast
still works ....
 
> So do you think it is safe to use u32 for data->hwirq (and all other "irq"
> occurrences)?

If your hwirq numbers are not going to exceed 2^32 ....
 
> > 
> > Aside of that mask_val is a clear misnomer.
> 
> Actually, this code comes from exynos-combiner.c and is part of the reason for

Copy and paste is not an excuse for bad code.

> >> +
> >> +		irq_status[i] = HANDLE_EN_AND_INV_MASKS(irq_status[i], i);
> >> +		status = tango_dispatch_irqs(domain, desc, irq_status[i], i*32);
> >> +		if (status & irq_status[i])
> > 

> > Can you pretty please explain how this can happen? It can't. Because you
> > clear every single bit of status in tango_dispatch_irqs(). See above.
>
> You are right that with the current code of tango_dispatch_irqs() it cannot
> happen, but that's more of a typo. I wanted to have a way to print something
> from this driver in the event of unhandled IRQs.

That's the fricking wrong place. If the dispatcher cannot handle an irq then
the dispatcher should issue the printout, not some random call site.

> >> +/**
> >> + * tango_irqdomain_map - route a hwirq(in) to a hwirq(out).
> >> + * NOTE: The hwirq(out) must have been already allocated and enabled on
> >> + * the parent controller.
> > 
> > Please put notes AFTER the argument descriptions
> 
> The note refers to the title:
> "tango_irqdomain_map - route a hwirq(in) to a hwirq(out)."

That does not make the comment a valid kernel doc comment. Format is:

/**
 * function_name - Short function description
 * @arg1:		Description of arg1
 * @argument2:		Description of argument2
 *
 * Long description of the function including return values.
 */

Hint. You can build the kerneldoc stuff. It will tell you what's wrong.

> >> + * @domain: a registered domain
> >> + * @fwspec: an IRQ specification. This callback is used to translate the
> >> + * parameters given as irq_fwspec into a HW IRQ and Type values.
> > 
> > fwspec is a callback?
> 
> No, the sentence refers to the callback below (tango_irqdomain_translate) that is
> being described by this comment.
> I can reformulate or remove the comment.

Please do not remove useful comments. Make them proper.

> >> +	err = tango_parse_fwspec(domain,
> >> +				 fwspec,
> >> +				 &domain_id,
> >> +				 &irq,
> >> +				 &type);
> > 
> > Please use the full 80 characters. Your patch does not become more valuable
> > when you inflate the line count. It's more valuable when it is easy to read
> > and understand.
> 
> Ok.
> 
> As a side note, does the 80 characters limit still makes sense? I mean, I would
> imagine that anybody now is using wide screens capable of at least double that
> amount.

That's horrible. Did you ever think about why stuff is printed in columns or
the width is restricted?
 
There is a very simple reason: Your eyes cannot parse wide printed lines fast
without moving the focus while you can scan a limited width in one go.
  
> This is more of a relic when I did not had access to the 'select' callback.
> (I was working on 4.4)

Please remove relics. They are just a burden for review and they will confuse
the hell out of you 3 month from now.

Thanks,

	tglx