Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1434709 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [RFC PATCH v1] irqchip: add support for SMP irq router Sebastian Frias <sf84@laposte.net> - 2016-06-30 18:20 +0200
    Re: [RFC PATCH v1] irqchip: add support for SMP irq router Mason <slash.tmp@free.fr> - 2016-07-04 14:20 +0200
      Re: [RFC PATCH v1] irqchip: add support for SMP irq router Sebastian Frias <sf84@laposte.net> - 2016-07-05 14:40 +0200
        Re: [RFC PATCH v1] irqchip: add support for SMP irq router Jason Cooper <jason@lakedaemon.net> - 2016-07-05 16:50 +0200
          Re: [RFC PATCH v1] irqchip: add support for SMP irq router Mason <slash.tmp@free.fr> - 2016-07-05 17:10 +0200
            Re: [RFC PATCH v1] irqchip: add support for SMP irq router Jason Cooper <jason@lakedaemon.net> - 2016-07-05 18:20 +0200
              Re: [RFC PATCH v1] irqchip: add support for SMP irq router Sebastian Frias <sf84@laposte.net> - 2016-07-06 13:40 +0200
                Re: [RFC PATCH v1] irqchip: add support for SMP irq router Jason Cooper <jason@lakedaemon.net> - 2016-07-06 18:30 +0200
          Re: [RFC PATCH v1] irqchip: add support for SMP irq router Sebastian Frias <sf84@laposte.net> - 2016-07-05 17:20 +0200
            Re: [RFC PATCH v1] irqchip: add support for SMP irq router Jason Cooper <jason@lakedaemon.net> - 2016-07-05 18:00 +0200
              Re: [RFC PATCH v1] irqchip: add support for SMP irq router Sebastian Frias <sf84@laposte.net> - 2016-07-05 18:40 +0200
                Re: [RFC PATCH v1] irqchip: add support for SMP irq router Marc Zyngier <marc.zyngier@arm.com> - 2016-07-05 18:50 +0200
                  Re: [RFC PATCH v1] irqchip: add support for SMP irq router Sebastian Frias <sf84@laposte.net> - 2016-07-05 19:00 +0200
                    Re: [RFC PATCH v1] irqchip: add support for SMP irq router Marc Zyngier <marc.zyngier@arm.com> - 2016-07-05 19:20 +0200
                      Re: [RFC PATCH v1] irqchip: add support for SMP irq router Thomas Gleixner <tglx@linutronix.de> - 2016-07-05 21:30 +0200
                        Re: [RFC PATCH v1] irqchip: add support for SMP irq router Marc Zyngier <marc.zyngier@arm.com> - 2016-07-06 11:00 +0200
                          Re: [RFC PATCH v1] irqchip: add support for SMP irq router Thomas Gleixner <tglx@linutronix.de> - 2016-07-06 11:40 +0200
                            Re: [RFC PATCH v1] irqchip: add support for SMP irq router Sebastian Frias <sf84@laposte.net> - 2016-07-06 12:50 +0200
                              Re: [RFC PATCH v1] irqchip: add support for SMP irq router Marc Zyngier <marc.zyngier@arm.com> - 2016-07-06 16:00 +0200
                            Re: [RFC PATCH v1] irqchip: add support for SMP irq router Jason Cooper <jason@lakedaemon.net> - 2016-07-06 18:50 +0200
                      Re: [RFC PATCH v1] irqchip: add support for SMP irq router Sebastian Frias <sf84@laposte.net> - 2016-07-06 12:50 +0200
                        Re: [RFC PATCH v1] irqchip: add support for SMP irq router Marc Zyngier <marc.zyngier@arm.com> - 2016-07-06 16:00 +0200
                          Re: [RFC PATCH v1] irqchip: add support for SMP irq router Sebastian Frias <sf84@laposte.net> - 2016-07-07 14:20 +0200
                            Re: [RFC PATCH v1] irqchip: add support for SMP irq router Marc Zyngier <marc.zyngier@arm.com> - 2016-07-07 14:50 +0200

Page 1 of 2  [1] 2  Next page →

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

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

[toc] | [next] | [standalone]

#1436439

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.

[toc] | [prev] | [next] | [standalone]

#1436964

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

[toc] | [prev] | [next] | [standalone]

#1437068

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.

[toc] | [prev] | [next] | [standalone]

#1437104

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.

[toc] | [prev] | [next] | [standalone]

#1437156

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.

[toc] | [prev] | [next] | [standalone]

#1437649

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

[toc] | [prev] | [next] | [standalone]

#1437833

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.

[toc] | [prev] | [next] | [standalone]

#1437117

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

[toc] | [prev] | [next] | [standalone]

#1437147

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.

[toc] | [prev] | [next] | [standalone]

#1437182

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

[toc] | [prev] | [next] | [standalone]

#1437188

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...

[toc] | [prev] | [next] | [standalone]

#1437192

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

[toc] | [prev] | [next] | [standalone]

#1437199

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...

[toc] | [prev] | [next] | [standalone]

#1437237

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

[toc] | [prev] | [next] | [standalone]

#1437563

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...

[toc] | [prev] | [next] | [standalone]

#1437588

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

[toc] | [prev] | [next] | [standalone]

#1437624

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

[toc] | [prev] | [next] | [standalone]

#1437745

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...

[toc] | [prev] | [next] | [standalone]

#1437844

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.

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web