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

Groups > linux.kernel > #1311527 > unrolled thread

[RFC PATCH v2] irqchip: Add support for Tango interrupt controller

Back to article view | Back to linux.kernel

Contents

  [RFC PATCH v2] irqchip: Add support for Tango interrupt controller Marc Gonzalez <marc_gonzalez@sigmadesigns.com> - 2016-01-18 14:10 +0100
    Re: [RFC PATCH v2] irqchip: Add support for Tango interrupt controller Måns Rullgård <mans@mansr.com> - 2016-01-18 14:10 +0100
    Re: [RFC PATCH v2] irqchip: Add support for Tango interrupt  controller Marc Gonzalez <marc_gonzalez@sigmadesigns.com> - 2016-01-18 17:10 +0100
      [RFC PATCH v3] irqchip: Add support for Tango interrupt controller Marc Gonzalez <marc_gonzalez@sigmadesigns.com> - 2016-01-18 17:50 +0100
        Re: [RFC PATCH v3] irqchip: Add support for Tango interrupt  controller Marc Zyngier <marc.zyngier@arm.com> - 2016-01-20 17:10 +0100
          Re: [RFC PATCH v3] irqchip: Add support for Tango interrupt controller Måns Rullgård <mans@mansr.com> - 2016-01-20 17:20 +0100
            Re: [RFC PATCH v3] irqchip: Add support for Tango interrupt controller Måns Rullgård <mans@mansr.com> - 2016-01-20 17:30 +0100
              Re: [RFC PATCH v3] irqchip: Add support for Tango interrupt  controller Marc Gonzalez <marc_gonzalez@sigmadesigns.com> - 2016-01-20 17:40 +0100
                Re: [RFC PATCH v3] irqchip: Add support for Tango interrupt controller Måns Rullgård <mans@mansr.com> - 2016-01-20 17:40 +0100
                  Re: [RFC PATCH v3] irqchip: Add support for Tango interrupt  controller Marc Gonzalez <marc_gonzalez@sigmadesigns.com> - 2016-01-20 17:50 +0100
                    Re: [RFC PATCH v3] irqchip: Add support for Tango interrupt controller Måns Rullgård <mans@mansr.com> - 2016-01-20 19:10 +0100
                      Re: [RFC PATCH v3] irqchip: Add support for Tango interrupt  controller Marc Gonzalez <marc_gonzalez@sigmadesigns.com> - 2016-01-22 17:00 +0100
                        Re: [RFC PATCH v3] irqchip: Add support for Tango interrupt controller Måns Rullgård <mans@mansr.com> - 2016-01-22 17:40 +0100
                          Re: [RFC PATCH v3] irqchip: Add support for Tango interrupt  controller Marc Gonzalez <marc_gonzalez@sigmadesigns.com> - 2016-01-22 17:40 +0100
                            Re: [RFC PATCH v3] irqchip: Add support for Tango interrupt controller Måns Rullgård <mans@mansr.com> - 2016-01-22 17:40 +0100
                              Re: [RFC PATCH v3] irqchip: Add support for Tango interrupt  controller Marc Gonzalez <marc_gonzalez@sigmadesigns.com> - 2016-01-22 17:50 +0100
                                Re: [RFC PATCH v3] irqchip: Add support for Tango interrupt controller Måns Rullgård <mans@mansr.com> - 2016-01-22 17:50 +0100
            Re: [RFC PATCH v3] irqchip: Add support for Tango interrupt  controller Marc Zyngier <marc.zyngier@arm.com> - 2016-01-20 17:30 +0100
            Re: [RFC PATCH v3] irqchip: Add support for Tango interrupt controller Måns Rullgård <mans@mansr.com> - 2016-01-20 17:30 +0100
            Re: [RFC PATCH v3] irqchip: Add support for Tango interrupt  controller Marc Gonzalez <marc_gonzalez@sigmadesigns.com> - 2016-01-20 17:30 +0100
              Re: [RFC PATCH v3] irqchip: Add support for Tango interrupt  controller Marc Gonzalez <marc_gonzalez@sigmadesigns.com> - 2016-01-20 18:10 +0100
                Re: [RFC PATCH v3] irqchip: Add support for Tango interrupt controller Måns Rullgård <mans@mansr.com> - 2016-01-20 18:10 +0100
              Re: [RFC PATCH v3] irqchip: Add support for Tango interrupt  controller Mark Rutland <mark.rutland@arm.com> - 2016-01-20 18:10 +0100
                Re: [RFC PATCH v3] irqchip: Add support for Tango interrupt controller Måns Rullgård <mans@mansr.com> - 2016-01-20 18:10 +0100

Page 1 of 2  [1] 2  Next page →

#1311527 — [RFC PATCH v2] irqchip: Add support for Tango interrupt controller

From: Mans Rullgard <mans@mansr.com>

Add support for the interrupt controller used in Sigma Designs
SMP86xx (Tango3) and SMP87xx (Tango4) chips.

Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
---
For the arch/arm/mach-tango port, I'm using this driver written
by Mans. Version 1 of the patch was submitted two months ago:
http://thread.gmane.org/gmane.linux.kernel/2089471

I've tried to address Marc's remarks.

I kept Mans as the commit author, but I removed his Signed-off-by
(because I might have introduced errors that he didn't sign off on).
Is that the right thing to do?

I didn't rename tangox to tango inside irq-tango.c because I didn't
want to change the code too much, but I'm OK either way.

The matching DT description currently is

intc: interrupt-controller@6e000 {
	compatible = "sigma,smp8642-intc";
	reg = <0x6e000 0x400>;
	ranges = <0 0x6e000 0x400>;
	interrupt-parent = <&gic>;
	interrupt-controller;
	#address-cells = <1>;
	#size-cells = <1>;

	irq0: irq0@000 {
		reg = <0x000 0x100>;
		interrupt-controller;
		#interrupt-cells = <2>;
		interrupts = <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>;
	};

	irq1: irq1@100 {
		reg = <0x100 0x100>;
		interrupt-controller;
		#interrupt-cells = <2>;
		interrupts = <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>;
	};

	irq2: irq2@300 {
		reg = <0x300 0x100>;
		interrupt-controller;
		#interrupt-cells = <2>;
		interrupts = <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>;
	};
};

---
 drivers/irqchip/Kconfig     |   5 +
 drivers/irqchip/Makefile    |   1 +
 drivers/irqchip/irq-tango.c | 227 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 233 insertions(+)
 create mode 100644 drivers/irqchip/irq-tango.c

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 4d7294e5d982..399eda54b860 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -133,6 +133,11 @@ config ST_IRQCHIP
 	help
 	  Enables SysCfg Controlled IRQs on STi based platforms.
 
+config TANGO_IRQ
+	bool
+	select IRQ_DOMAIN
+	select GENERIC_IRQ_CHIP
+
 config TB10X_IRQC
 	bool
 	select IRQ_DOMAIN
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index 177f78f6e6d6..74512452cc8b 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -38,6 +38,7 @@ obj-$(CONFIG_VERSATILE_FPGA_IRQ)	+= irq-versatile-fpga.o
 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_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.c b/drivers/irqchip/irq-tango.c
new file mode 100644
index 000000000000..5e2383699be0
--- /dev/null
+++ b/drivers/irqchip/irq-tango.c
@@ -0,0 +1,227 @@
+/*
+ * Copyright (C) 2014 Mans Rullgard <mans@mansr.com>
+ *
+ * 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 IRQ0_CTL_BASE		0x0000
+#define IRQ1_CTL_BASE		0x0100
+#define EDGE_CTL_BASE		0x0200
+#define IRQ2_CTL_BASE		0x0300
+
+#define IRQ_CTL_HI		0x18
+#define EDGE_CTL_HI		0x20
+
+#define IRQ_STATUS		0x00
+#define IRQ_RAWSTAT		0x04
+#define IRQ_EN_SET		0x08
+#define IRQ_EN_CLR		0x0c
+#define IRQ_SOFT_SET		0x10
+#define IRQ_SOFT_CLR		0x14
+
+#define EDGE_STATUS		0x00
+#define EDGE_RAWSTAT		0x04
+#define EDGE_CFG_RISE		0x08
+#define EDGE_CFG_FALL		0x0c
+#define EDGE_CFG_RISE_SET	0x10
+#define EDGE_CFG_RISE_CLR	0x14
+#define EDGE_CFG_FALL_SET	0x18
+#define EDGE_CFG_FALL_CLR	0x1c
+
+struct tangox_irq_chip {
+	void __iomem *base;
+	unsigned long ctl;
+};
+
+static inline u32 intc_readl(struct tangox_irq_chip *chip, int reg)
+{
+	return readl_relaxed(chip->base + reg);
+}
+
+static inline void intc_writel(struct tangox_irq_chip *chip, int reg, u32 val)
+{
+	writel_relaxed(val, chip->base + reg);
+}
+
+static void tangox_dispatch_irqs(struct irq_domain *dom, unsigned int status,
+				 int base)
+{
+	unsigned int hwirq;
+	unsigned int virq;
+
+	while (status) {
+		hwirq = __ffs(status);
+		virq = irq_find_mapping(dom, base + hwirq);
+		if (virq) generic_handle_irq(virq);
+		status &= ~BIT(hwirq);
+	}
+}
+
+static void tangox_irq_handler(struct irq_desc *desc)
+{
+	struct irq_domain *dom = irq_desc_get_handler_data(desc);
+	struct irq_chip *host_chip = irq_desc_get_chip(desc);
+	struct tangox_irq_chip *chip = dom->host_data;
+	unsigned int status_lo, status_hi;
+
+	chained_irq_enter(host_chip, desc);
+
+	status_lo = intc_readl(chip, chip->ctl + IRQ_STATUS);
+	status_hi = intc_readl(chip, chip->ctl + IRQ_CTL_HI + IRQ_STATUS);
+
+	tangox_dispatch_irqs(dom, status_lo, 0);
+	tangox_dispatch_irqs(dom, status_hi, 32);
+
+	chained_irq_exit(host_chip, desc);
+}
+
+static int tangox_irq_set_type(struct irq_data *d, unsigned int flow_type)
+{
+	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+	struct tangox_irq_chip *chip = gc->domain->host_data;
+	struct irq_chip_regs *regs = &gc->chip_types[0].regs;
+
+	switch (flow_type & IRQ_TYPE_SENSE_MASK) {
+	case IRQ_TYPE_EDGE_RISING:
+		intc_writel(chip, regs->type + EDGE_CFG_RISE_SET, d->mask);
+		intc_writel(chip, regs->type + EDGE_CFG_FALL_CLR, d->mask);
+		break;
+
+	case IRQ_TYPE_EDGE_FALLING:
+		intc_writel(chip, regs->type + EDGE_CFG_RISE_CLR, d->mask);
+		intc_writel(chip, regs->type + EDGE_CFG_FALL_SET, d->mask);
+		break;
+
+	case IRQ_TYPE_LEVEL_HIGH:
+		intc_writel(chip, regs->type + EDGE_CFG_RISE_CLR, d->mask);
+		intc_writel(chip, regs->type + EDGE_CFG_FALL_CLR, d->mask);
+		break;
+
+	case IRQ_TYPE_LEVEL_LOW:
+		intc_writel(chip, regs->type + EDGE_CFG_RISE_SET, d->mask);
+		intc_writel(chip, regs->type + EDGE_CFG_FALL_SET, d->mask);
+		break;
+
+	default:
+		pr_err("Invalid trigger mode %x for IRQ %d\n",
+		       flow_type, d->irq);
+		return -EINVAL;
+	}
+
+	return irq_setup_alt_chip(d, flow_type);
+}
+
+static void __init tangox_irq_init_chip(struct irq_chip_generic *gc,
+					unsigned long ctl_offs,
+					unsigned long edge_offs)
+{
+	struct tangox_irq_chip *chip = gc->domain->host_data;
+	struct irq_chip_type *ct = gc->chip_types;
+	unsigned long ctl_base = chip->ctl + ctl_offs;
+	unsigned long edge_base = EDGE_CTL_BASE + edge_offs;
+	int i;
+
+	gc->reg_base = chip->base;
+	gc->unused = 0;
+
+	for (i = 0; i < 2; i++) {
+		ct[i].chip.irq_ack = irq_gc_ack_set_bit;
+		ct[i].chip.irq_mask = irq_gc_mask_disable_reg;
+		ct[i].chip.irq_mask_ack = irq_gc_mask_disable_reg_and_ack;
+		ct[i].chip.irq_unmask = irq_gc_unmask_enable_reg;
+		ct[i].chip.irq_set_type = tangox_irq_set_type;
+		ct[i].chip.name = gc->domain->name;
+
+		ct[i].regs.enable = ctl_base + IRQ_EN_SET;
+		ct[i].regs.disable = ctl_base + IRQ_EN_CLR;
+		ct[i].regs.ack = edge_base + EDGE_RAWSTAT;
+		ct[i].regs.type = edge_base;
+	}
+
+	ct[0].type = IRQ_TYPE_LEVEL_MASK;
+	ct[0].handler = handle_level_irq;
+
+	ct[1].type = IRQ_TYPE_EDGE_BOTH;
+	ct[1].handler = handle_edge_irq;
+
+	intc_writel(chip, ct->regs.disable, 0xffffffff);
+	intc_writel(chip, ct->regs.ack, 0xffffffff);
+}
+
+static void __init tangox_irq_domain_init(struct irq_domain *dom)
+{
+	struct irq_chip_generic *gc;
+	int i;
+
+	for (i = 0; i < 2; i++) {
+		gc = irq_get_domain_generic_chip(dom, i * 32);
+		tangox_irq_init_chip(gc, i * IRQ_CTL_HI, i * EDGE_CTL_HI);
+	}
+}
+
+static int __init tangox_irq_init(void __iomem *base, struct device_node *node)
+{
+	struct tangox_irq_chip *chip;
+	struct irq_domain *dom;
+	const char *name;
+	u32 ctl;
+	int irq;
+	int err;
+	int i;
+
+	irq = irq_of_parse_and_map(node, 0);
+	if (!irq)
+		panic("%s: failed to get IRQ", node->name);
+
+	if (of_property_read_u32(node, "reg", &ctl))
+		panic("%s: failed to get reg base", node->name);
+
+	chip = kzalloc(sizeof(*chip), GFP_KERNEL);
+	chip->ctl = ctl;
+	chip->base = base;
+
+	dom = irq_domain_add_linear(node, 64, &irq_generic_chip_ops, chip);
+	if (!dom)
+		panic("%s: failed to create irqdomain", node->name);
+
+	err = irq_alloc_domain_generic_chips(dom, 32, 2, name, handle_level_irq,
+					     0, 0, 0);
+	if (err)
+		panic("%s: failed to allocate irqchip", node->name);
+
+	tangox_irq_domain_init(dom);
+
+	irq_set_chained_handler(irq, tangox_irq_handler);
+	irq_set_handler_data(irq, dom);
+
+	return 0;
+}
+
+static int __init tangox_of_irq_init(struct device_node *node,
+				     struct device_node *parent)
+{
+	struct device_node *c;
+	void __iomem *base;
+
+	base = of_iomap(node, 0);
+
+	for_each_child_of_node(node, c)
+		tangox_irq_init(base, c);
+
+	return 0;
+}
+
+IRQCHIP_DECLARE(tangox_intc, "sigma,smp8642-intc", tangox_of_irq_init);

[toc] | [next] | [standalone]

#1311531

Marc Gonzalez <marc_gonzalez@sigmadesigns.com> writes:

> +	if (of_property_read_u32(node, "reg", &ctl))
> +		panic("%s: failed to get reg base", node->name);
> +
> +	chip = kzalloc(sizeof(*chip), GFP_KERNEL);
> +	chip->ctl = ctl;
> +	chip->base = base;

I think it might be better to let the OF core convert the "reg" property
to a struct resource, then get the offset by subtracting from the base
(physical) address.  Otherwise it will break badly if someone thinks
it's safe to drop "ranges" from the outer node and use absolute
addresses in the child nodes (that's how the "reg" property typically
behaves).

Anyway, thanks for revisiting this.  I never got a straight answer (or
any answer for that matter) when I asked which approach would be
preferred, and then I had better things to do than keep trying patches
until people got happy.

-- 
Måns Rullgård

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

#1311648 — Re: [RFC PATCH v2] irqchip: Add support for Tango interrupt controller

On 18/01/2016 13:56, Marc Gonzalez wrote:

> For the arch/arm/mach-tango port, I'm using this driver written
> by Mans. Version 1 of the patch was submitted two months ago:
> http://thread.gmane.org/gmane.linux.kernel/2089471
> 
> I've tried to address Marc Zyngier's remarks.

And my patch is incomplete, v3 coming right up.

drivers/irqchip/irq-tango.c: In function 'tangox_irq_init':
drivers/irqchip/irq-tango.c:183:6: warning: unused variable 'i' [-Wunused-variable]
  int i;
      ^
drivers/irqchip/irq-tango.c: In function 'tangox_of_irq_init':
drivers/irqchip/irq-tango.c:200:6: warning: 'name' may be used uninitialized in this function [-Wmaybe-uninitialized]
  err = irq_alloc_domain_generic_chips(dom, 32, 2, name, handle_level_irq,
      ^
drivers/irqchip/irq-tango.c:179:14: note: 'name' was declared here
  const char *name;
              ^

Regards.

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

#1311665 — [RFC PATCH v3] irqchip: Add support for Tango interrupt controller

From: Mans Rullgard <mans@mansr.com>

Add support for the interrupt controller used in Sigma Designs
SMP86xx (Tango3) and SMP87xx (Tango4) chips.

Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
---
For the arch/arm/mach-tango port, I'm using this driver written
by Mans. Version 1 of the patch was submitted two months ago:
http://thread.gmane.org/gmane.linux.kernel/2089471

I've tried to address Marc Zyngier's remarks.

I kept Mans as the commit author, but I removed his Signed-off-by
(because I might have introduced errors that he didn't sign off on).
Is that the right thing to do?

I didn't rename tangox to tango inside irq-tango.c because I didn't
want to change the code too much, but I'm OK either way.

The matching DT description currently is

intc: interrupt-controller@6e000 {
	compatible = "sigma,smp8642-intc";
	reg = <0x6e000 0x400>;
	ranges = <0 0x6e000 0x400>;
	interrupt-parent = <&gic>;
	interrupt-controller;
	#address-cells = <1>;
	#size-cells = <1>;

	irq0: irq0@000 {
		reg = <0x000 0x100>;
		interrupt-controller;
		#interrupt-cells = <2>;
		interrupts = <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>;
	};

	irq1: irq1@100 {
		reg = <0x100 0x100>;
		interrupt-controller;
		#interrupt-cells = <2>;
		interrupts = <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>;
	};

	irq2: irq2@300 {
		reg = <0x300 0x100>;
		interrupt-controller;
		#interrupt-cells = <2>;
		interrupts = <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>;
	};
};

---
 drivers/irqchip/Kconfig     |   5 +
 drivers/irqchip/Makefile    |   1 +
 drivers/irqchip/irq-tango.c | 225 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 231 insertions(+)
 create mode 100644 drivers/irqchip/irq-tango.c

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 4d7294e5d982..399eda54b860 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -133,6 +133,11 @@ config ST_IRQCHIP
 	help
 	  Enables SysCfg Controlled IRQs on STi based platforms.
 
+config TANGO_IRQ
+	bool
+	select IRQ_DOMAIN
+	select GENERIC_IRQ_CHIP
+
 config TB10X_IRQC
 	bool
 	select IRQ_DOMAIN
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index 177f78f6e6d6..74512452cc8b 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -38,6 +38,7 @@ obj-$(CONFIG_VERSATILE_FPGA_IRQ)	+= irq-versatile-fpga.o
 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_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.c b/drivers/irqchip/irq-tango.c
new file mode 100644
index 000000000000..ace9bd50be6b
--- /dev/null
+++ b/drivers/irqchip/irq-tango.c
@@ -0,0 +1,225 @@
+/*
+ * Copyright (C) 2014 Mans Rullgard <mans@mansr.com>
+ *
+ * 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 IRQ0_CTL_BASE		0x0000
+#define IRQ1_CTL_BASE		0x0100
+#define EDGE_CTL_BASE		0x0200
+#define IRQ2_CTL_BASE		0x0300
+
+#define IRQ_CTL_HI		0x18
+#define EDGE_CTL_HI		0x20
+
+#define IRQ_STATUS		0x00
+#define IRQ_RAWSTAT		0x04
+#define IRQ_EN_SET		0x08
+#define IRQ_EN_CLR		0x0c
+#define IRQ_SOFT_SET		0x10
+#define IRQ_SOFT_CLR		0x14
+
+#define EDGE_STATUS		0x00
+#define EDGE_RAWSTAT		0x04
+#define EDGE_CFG_RISE		0x08
+#define EDGE_CFG_FALL		0x0c
+#define EDGE_CFG_RISE_SET	0x10
+#define EDGE_CFG_RISE_CLR	0x14
+#define EDGE_CFG_FALL_SET	0x18
+#define EDGE_CFG_FALL_CLR	0x1c
+
+struct tangox_irq_chip {
+	void __iomem *base;
+	unsigned long ctl;
+};
+
+static inline u32 intc_readl(struct tangox_irq_chip *chip, int reg)
+{
+	return readl_relaxed(chip->base + reg);
+}
+
+static inline void intc_writel(struct tangox_irq_chip *chip, int reg, u32 val)
+{
+	writel_relaxed(val, chip->base + reg);
+}
+
+static void tangox_dispatch_irqs(struct irq_domain *dom, unsigned int status,
+				 int base)
+{
+	unsigned int hwirq;
+	unsigned int virq;
+
+	while (status) {
+		hwirq = __ffs(status);
+		virq = irq_find_mapping(dom, base + hwirq);
+		if (virq) generic_handle_irq(virq);
+		status &= ~BIT(hwirq);
+	}
+}
+
+static void tangox_irq_handler(struct irq_desc *desc)
+{
+	struct irq_domain *dom = irq_desc_get_handler_data(desc);
+	struct irq_chip *host_chip = irq_desc_get_chip(desc);
+	struct tangox_irq_chip *chip = dom->host_data;
+	unsigned int status_lo, status_hi;
+
+	chained_irq_enter(host_chip, desc);
+
+	status_lo = intc_readl(chip, chip->ctl + IRQ_STATUS);
+	status_hi = intc_readl(chip, chip->ctl + IRQ_CTL_HI + IRQ_STATUS);
+
+	tangox_dispatch_irqs(dom, status_lo, 0);
+	tangox_dispatch_irqs(dom, status_hi, 32);
+
+	chained_irq_exit(host_chip, desc);
+}
+
+static int tangox_irq_set_type(struct irq_data *d, unsigned int flow_type)
+{
+	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+	struct tangox_irq_chip *chip = gc->domain->host_data;
+	struct irq_chip_regs *regs = &gc->chip_types[0].regs;
+
+	switch (flow_type & IRQ_TYPE_SENSE_MASK) {
+	case IRQ_TYPE_EDGE_RISING:
+		intc_writel(chip, regs->type + EDGE_CFG_RISE_SET, d->mask);
+		intc_writel(chip, regs->type + EDGE_CFG_FALL_CLR, d->mask);
+		break;
+
+	case IRQ_TYPE_EDGE_FALLING:
+		intc_writel(chip, regs->type + EDGE_CFG_RISE_CLR, d->mask);
+		intc_writel(chip, regs->type + EDGE_CFG_FALL_SET, d->mask);
+		break;
+
+	case IRQ_TYPE_LEVEL_HIGH:
+		intc_writel(chip, regs->type + EDGE_CFG_RISE_CLR, d->mask);
+		intc_writel(chip, regs->type + EDGE_CFG_FALL_CLR, d->mask);
+		break;
+
+	case IRQ_TYPE_LEVEL_LOW:
+		intc_writel(chip, regs->type + EDGE_CFG_RISE_SET, d->mask);
+		intc_writel(chip, regs->type + EDGE_CFG_FALL_SET, d->mask);
+		break;
+
+	default:
+		pr_err("Invalid trigger mode %x for IRQ %d\n",
+		       flow_type, d->irq);
+		return -EINVAL;
+	}
+
+	return irq_setup_alt_chip(d, flow_type);
+}
+
+static void __init tangox_irq_init_chip(struct irq_chip_generic *gc,
+					unsigned long ctl_offs,
+					unsigned long edge_offs)
+{
+	struct tangox_irq_chip *chip = gc->domain->host_data;
+	struct irq_chip_type *ct = gc->chip_types;
+	unsigned long ctl_base = chip->ctl + ctl_offs;
+	unsigned long edge_base = EDGE_CTL_BASE + edge_offs;
+	int i;
+
+	gc->reg_base = chip->base;
+	gc->unused = 0;
+
+	for (i = 0; i < 2; i++) {
+		ct[i].chip.irq_ack = irq_gc_ack_set_bit;
+		ct[i].chip.irq_mask = irq_gc_mask_disable_reg;
+		ct[i].chip.irq_mask_ack = irq_gc_mask_disable_reg_and_ack;
+		ct[i].chip.irq_unmask = irq_gc_unmask_enable_reg;
+		ct[i].chip.irq_set_type = tangox_irq_set_type;
+		ct[i].chip.name = gc->domain->name;
+
+		ct[i].regs.enable = ctl_base + IRQ_EN_SET;
+		ct[i].regs.disable = ctl_base + IRQ_EN_CLR;
+		ct[i].regs.ack = edge_base + EDGE_RAWSTAT;
+		ct[i].regs.type = edge_base;
+	}
+
+	ct[0].type = IRQ_TYPE_LEVEL_MASK;
+	ct[0].handler = handle_level_irq;
+
+	ct[1].type = IRQ_TYPE_EDGE_BOTH;
+	ct[1].handler = handle_edge_irq;
+
+	intc_writel(chip, ct->regs.disable, 0xffffffff);
+	intc_writel(chip, ct->regs.ack, 0xffffffff);
+}
+
+static void __init tangox_irq_domain_init(struct irq_domain *dom)
+{
+	struct irq_chip_generic *gc;
+	int i;
+
+	for (i = 0; i < 2; i++) {
+		gc = irq_get_domain_generic_chip(dom, i * 32);
+		tangox_irq_init_chip(gc, i * IRQ_CTL_HI, i * EDGE_CTL_HI);
+	}
+}
+
+static int __init tangox_irq_init(void __iomem *base, struct device_node *node)
+{
+	struct tangox_irq_chip *chip;
+	struct irq_domain *dom;
+	u32 ctl;
+	int irq;
+	int err;
+
+	irq = irq_of_parse_and_map(node, 0);
+	if (!irq)
+		panic("%s: failed to get IRQ", node->name);
+
+	if (of_property_read_u32(node, "reg", &ctl))
+		panic("%s: failed to get reg base", node->name);
+
+	chip = kzalloc(sizeof(*chip), GFP_KERNEL);
+	chip->ctl = ctl;
+	chip->base = base;
+
+	dom = irq_domain_add_linear(node, 64, &irq_generic_chip_ops, chip);
+	if (!dom)
+		panic("%s: failed to create irqdomain", node->name);
+
+	err = irq_alloc_domain_generic_chips(dom, 32, 2, node->name,
+			handle_level_irq, 0, 0, 0);
+	if (err)
+		panic("%s: failed to allocate irqchip", node->name);
+
+	tangox_irq_domain_init(dom);
+
+	irq_set_chained_handler(irq, tangox_irq_handler);
+	irq_set_handler_data(irq, dom);
+
+	return 0;
+}
+
+static int __init tangox_of_irq_init(struct device_node *node,
+				     struct device_node *parent)
+{
+	struct device_node *c;
+	void __iomem *base;
+
+	base = of_iomap(node, 0);
+
+	for_each_child_of_node(node, c)
+		tangox_irq_init(base, c);
+
+	return 0;
+}
+
+IRQCHIP_DECLARE(tangox_intc, "sigma,smp8642-intc", tangox_of_irq_init);

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

#1313315 — Re: [RFC PATCH v3] irqchip: Add support for Tango interrupt controller

On 18/01/16 16:44, Marc Gonzalez wrote:
> From: Mans Rullgard <mans@mansr.com>
> 
> Add support for the interrupt controller used in Sigma Designs
> SMP86xx (Tango3) and SMP87xx (Tango4) chips.
> 
> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
> ---
> For the arch/arm/mach-tango port, I'm using this driver written
> by Mans. Version 1 of the patch was submitted two months ago:
> http://thread.gmane.org/gmane.linux.kernel/2089471
> 
> I've tried to address Marc Zyngier's remarks.
> 
> I kept Mans as the commit author, but I removed his Signed-off-by
> (because I might have introduced errors that he didn't sign off on).
> Is that the right thing to do?
> 
> I didn't rename tangox to tango inside irq-tango.c because I didn't
> want to change the code too much, but I'm OK either way.
> 
> The matching DT description currently is
> 
> intc: interrupt-controller@6e000 {
> 	compatible = "sigma,smp8642-intc";
> 	reg = <0x6e000 0x400>;
> 	ranges = <0 0x6e000 0x400>;
> 	interrupt-parent = <&gic>;
> 	interrupt-controller;
> 	#address-cells = <1>;
> 	#size-cells = <1>;
> 
> 	irq0: irq0@000 {
> 		reg = <0x000 0x100>;
> 		interrupt-controller;
> 		#interrupt-cells = <2>;
> 		interrupts = <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>;
> 	};
> 
> 	irq1: irq1@100 {
> 		reg = <0x100 0x100>;
> 		interrupt-controller;
> 		#interrupt-cells = <2>;
> 		interrupts = <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>;
> 	};
> 
> 	irq2: irq2@300 {
> 		reg = <0x300 0x100>;
> 		interrupt-controller;
> 		#interrupt-cells = <2>;
> 		interrupts = <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>;
> 	};
> };
> 
> ---
>  drivers/irqchip/Kconfig     |   5 +
>  drivers/irqchip/Makefile    |   1 +
>  drivers/irqchip/irq-tango.c | 225 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 231 insertions(+)
>  create mode 100644 drivers/irqchip/irq-tango.c
> 
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index 4d7294e5d982..399eda54b860 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -133,6 +133,11 @@ config ST_IRQCHIP
>  	help
>  	  Enables SysCfg Controlled IRQs on STi based platforms.
>  
> +config TANGO_IRQ
> +	bool
> +	select IRQ_DOMAIN
> +	select GENERIC_IRQ_CHIP
> +
>  config TB10X_IRQC
>  	bool
>  	select IRQ_DOMAIN
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index 177f78f6e6d6..74512452cc8b 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -38,6 +38,7 @@ obj-$(CONFIG_VERSATILE_FPGA_IRQ)	+= irq-versatile-fpga.o
>  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_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.c b/drivers/irqchip/irq-tango.c
> new file mode 100644
> index 000000000000..ace9bd50be6b
> --- /dev/null
> +++ b/drivers/irqchip/irq-tango.c
> @@ -0,0 +1,225 @@
> +/*
> + * Copyright (C) 2014 Mans Rullgard <mans@mansr.com>
> + *
> + * 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 IRQ0_CTL_BASE		0x0000
> +#define IRQ1_CTL_BASE		0x0100
> +#define EDGE_CTL_BASE		0x0200
> +#define IRQ2_CTL_BASE		0x0300
> +
> +#define IRQ_CTL_HI		0x18
> +#define EDGE_CTL_HI		0x20
> +
> +#define IRQ_STATUS		0x00
> +#define IRQ_RAWSTAT		0x04
> +#define IRQ_EN_SET		0x08
> +#define IRQ_EN_CLR		0x0c
> +#define IRQ_SOFT_SET		0x10
> +#define IRQ_SOFT_CLR		0x14
> +
> +#define EDGE_STATUS		0x00
> +#define EDGE_RAWSTAT		0x04
> +#define EDGE_CFG_RISE		0x08
> +#define EDGE_CFG_FALL		0x0c
> +#define EDGE_CFG_RISE_SET	0x10
> +#define EDGE_CFG_RISE_CLR	0x14
> +#define EDGE_CFG_FALL_SET	0x18
> +#define EDGE_CFG_FALL_CLR	0x1c
> +
> +struct tangox_irq_chip {
> +	void __iomem *base;
> +	unsigned long ctl;
> +};
> +
> +static inline u32 intc_readl(struct tangox_irq_chip *chip, int reg)
> +{
> +	return readl_relaxed(chip->base + reg);
> +}
> +
> +static inline void intc_writel(struct tangox_irq_chip *chip, int reg, u32 val)
> +{
> +	writel_relaxed(val, chip->base + reg);
> +}
> +
> +static void tangox_dispatch_irqs(struct irq_domain *dom, unsigned int status,
> +				 int base)
> +{
> +	unsigned int hwirq;
> +	unsigned int virq;
> +
> +	while (status) {
> +		hwirq = __ffs(status);
> +		virq = irq_find_mapping(dom, base + hwirq);
> +		if (virq) generic_handle_irq(virq);
> +		status &= ~BIT(hwirq);
> +	}
> +}
> +
> +static void tangox_irq_handler(struct irq_desc *desc)
> +{
> +	struct irq_domain *dom = irq_desc_get_handler_data(desc);
> +	struct irq_chip *host_chip = irq_desc_get_chip(desc);
> +	struct tangox_irq_chip *chip = dom->host_data;
> +	unsigned int status_lo, status_hi;
> +
> +	chained_irq_enter(host_chip, desc);
> +
> +	status_lo = intc_readl(chip, chip->ctl + IRQ_STATUS);
> +	status_hi = intc_readl(chip, chip->ctl + IRQ_CTL_HI + IRQ_STATUS);
> +
> +	tangox_dispatch_irqs(dom, status_lo, 0);
> +	tangox_dispatch_irqs(dom, status_hi, 32);
> +
> +	chained_irq_exit(host_chip, desc);
> +}
> +
> +static int tangox_irq_set_type(struct irq_data *d, unsigned int flow_type)
> +{
> +	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
> +	struct tangox_irq_chip *chip = gc->domain->host_data;
> +	struct irq_chip_regs *regs = &gc->chip_types[0].regs;
> +
> +	switch (flow_type & IRQ_TYPE_SENSE_MASK) {
> +	case IRQ_TYPE_EDGE_RISING:
> +		intc_writel(chip, regs->type + EDGE_CFG_RISE_SET, d->mask);
> +		intc_writel(chip, regs->type + EDGE_CFG_FALL_CLR, d->mask);
> +		break;
> +
> +	case IRQ_TYPE_EDGE_FALLING:
> +		intc_writel(chip, regs->type + EDGE_CFG_RISE_CLR, d->mask);
> +		intc_writel(chip, regs->type + EDGE_CFG_FALL_SET, d->mask);
> +		break;
> +
> +	case IRQ_TYPE_LEVEL_HIGH:
> +		intc_writel(chip, regs->type + EDGE_CFG_RISE_CLR, d->mask);
> +		intc_writel(chip, regs->type + EDGE_CFG_FALL_CLR, d->mask);
> +		break;
> +
> +	case IRQ_TYPE_LEVEL_LOW:
> +		intc_writel(chip, regs->type + EDGE_CFG_RISE_SET, d->mask);
> +		intc_writel(chip, regs->type + EDGE_CFG_FALL_SET, d->mask);
> +		break;
> +
> +	default:
> +		pr_err("Invalid trigger mode %x for IRQ %d\n",
> +		       flow_type, d->irq);
> +		return -EINVAL;
> +	}
> +
> +	return irq_setup_alt_chip(d, flow_type);
> +}
> +
> +static void __init tangox_irq_init_chip(struct irq_chip_generic *gc,
> +					unsigned long ctl_offs,
> +					unsigned long edge_offs)
> +{
> +	struct tangox_irq_chip *chip = gc->domain->host_data;
> +	struct irq_chip_type *ct = gc->chip_types;
> +	unsigned long ctl_base = chip->ctl + ctl_offs;
> +	unsigned long edge_base = EDGE_CTL_BASE + edge_offs;
> +	int i;
> +
> +	gc->reg_base = chip->base;
> +	gc->unused = 0;
> +
> +	for (i = 0; i < 2; i++) {
> +		ct[i].chip.irq_ack = irq_gc_ack_set_bit;
> +		ct[i].chip.irq_mask = irq_gc_mask_disable_reg;
> +		ct[i].chip.irq_mask_ack = irq_gc_mask_disable_reg_and_ack;
> +		ct[i].chip.irq_unmask = irq_gc_unmask_enable_reg;
> +		ct[i].chip.irq_set_type = tangox_irq_set_type;
> +		ct[i].chip.name = gc->domain->name;
> +
> +		ct[i].regs.enable = ctl_base + IRQ_EN_SET;
> +		ct[i].regs.disable = ctl_base + IRQ_EN_CLR;
> +		ct[i].regs.ack = edge_base + EDGE_RAWSTAT;
> +		ct[i].regs.type = edge_base;
> +	}
> +
> +	ct[0].type = IRQ_TYPE_LEVEL_MASK;
> +	ct[0].handler = handle_level_irq;
> +
> +	ct[1].type = IRQ_TYPE_EDGE_BOTH;
> +	ct[1].handler = handle_edge_irq;
> +
> +	intc_writel(chip, ct->regs.disable, 0xffffffff);
> +	intc_writel(chip, ct->regs.ack, 0xffffffff);
> +}
> +
> +static void __init tangox_irq_domain_init(struct irq_domain *dom)
> +{
> +	struct irq_chip_generic *gc;
> +	int i;
> +
> +	for (i = 0; i < 2; i++) {
> +		gc = irq_get_domain_generic_chip(dom, i * 32);
> +		tangox_irq_init_chip(gc, i * IRQ_CTL_HI, i * EDGE_CTL_HI);
> +	}
> +}
> +
> +static int __init tangox_irq_init(void __iomem *base, struct device_node *node)
> +{
> +	struct tangox_irq_chip *chip;
> +	struct irq_domain *dom;
> +	u32 ctl;
> +	int irq;
> +	int err;
> +
> +	irq = irq_of_parse_and_map(node, 0);
> +	if (!irq)
> +		panic("%s: failed to get IRQ", node->name);
> +
> +	if (of_property_read_u32(node, "reg", &ctl))
> +		panic("%s: failed to get reg base", node->name);
> +
> +	chip = kzalloc(sizeof(*chip), GFP_KERNEL);
> +	chip->ctl = ctl;
> +	chip->base = base;
> +
> +	dom = irq_domain_add_linear(node, 64, &irq_generic_chip_ops, chip);
> +	if (!dom)
> +		panic("%s: failed to create irqdomain", node->name);
> +
> +	err = irq_alloc_domain_generic_chips(dom, 32, 2, node->name,
> +			handle_level_irq, 0, 0, 0);
> +	if (err)
> +		panic("%s: failed to allocate irqchip", node->name);
> +
> +	tangox_irq_domain_init(dom);
> +
> +	irq_set_chained_handler(irq, tangox_irq_handler);
> +	irq_set_handler_data(irq, dom);
> +
> +	return 0;
> +}
> +
> +static int __init tangox_of_irq_init(struct device_node *node,
> +				     struct device_node *parent)
> +{
> +	struct device_node *c;
> +	void __iomem *base;
> +
> +	base = of_iomap(node, 0);
> +
> +	for_each_child_of_node(node, c)
> +		tangox_irq_init(base, c);
> +
> +	return 0;
> +}
> +
> +IRQCHIP_DECLARE(tangox_intc, "sigma,smp8642-intc", tangox_of_irq_init);
> 

So the whole thing seems sound. For this patch:

Acked-by: Marc Zyngier <marc.zyngier@arm.com>

But you definitely need another patch documenting the DT bindings.

Thanks,

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

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

#1313326 — Re: [RFC PATCH v3] irqchip: Add support for Tango interrupt controller

Marc Zyngier <marc.zyngier@arm.com> writes:

>> +	if (of_property_read_u32(node, "reg", &ctl))
>> +		panic("%s: failed to get reg base", node->name);
>> +
>> +	chip = kzalloc(sizeof(*chip), GFP_KERNEL);
>> +	chip->ctl = ctl;
>> +	chip->base = base;

As I said before, this assumes the outer DT node uses a ranges
property.  Normally reg properties work the same whether they specify an
offset within an outer "ranges" or have a full address directly.  It
would be easy enough to make this work with either, so I don't see any
reason not to.

-- 
Måns Rullgård

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

#1313333 — Re: [RFC PATCH v3] irqchip: Add support for Tango interrupt controller

Marc Zyngier <marc.zyngier@arm.com> writes:

> On 20/01/16 16:10, Måns Rullgård wrote:
>> Marc Zyngier <marc.zyngier@arm.com> writes:
>> 
>>>> +	if (of_property_read_u32(node, "reg", &ctl))
>>>> +		panic("%s: failed to get reg base", node->name);
>>>> +
>>>> +	chip = kzalloc(sizeof(*chip), GFP_KERNEL);
>>>> +	chip->ctl = ctl;
>>>> +	chip->base = base;
>> 
>> As I said before, this assumes the outer DT node uses a ranges
>> property.  Normally reg properties work the same whether they specify an
>> offset within an outer "ranges" or have a full address directly.  It
>> would be easy enough to make this work with either, so I don't see any
>> reason not to.
>
> Yup, that is a good point. I guess Marc can address this in the next
> round, since we need a DT binding anyway.

I'd suggest using of_address_to_resource() on both nodes and subtracting
the start addresses returned.

-- 
Måns Rullgård

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

#1313340 — Re: [RFC PATCH v3] irqchip: Add support for Tango interrupt controller

On 20/01/2016 17:25, Måns Rullgård wrote:

> Marc Zyngier <marc.zyngier@arm.com> writes:
> 
>> On 20/01/16 16:10, Måns Rullgård wrote:
>>
>>> Marc Zyngier <marc.zyngier@arm.com> writes:
>>>
>>>>> +	if (of_property_read_u32(node, "reg", &ctl))
>>>>> +		panic("%s: failed to get reg base", node->name);
>>>>> +
>>>>> +	chip = kzalloc(sizeof(*chip), GFP_KERNEL);
>>>>> +	chip->ctl = ctl;
>>>>> +	chip->base = base;
>>>
>>> As I said before, this assumes the outer DT node uses a ranges
>>> property.  Normally reg properties work the same whether they specify an
>>> offset within an outer "ranges" or have a full address directly.  It
>>> would be easy enough to make this work with either, so I don't see any
>>> reason not to.
>>
>> Yup, that is a good point. I guess Marc can address this in the next
>> round, since we need a DT binding anyway.
> 
> I'd suggest using of_address_to_resource() on both nodes and subtracting
> the start addresses returned.

For my own reference, Marc Zyngier suggested:
"you should use of_iomap to map the child nodes, and not mess with
the parent one."

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

#1313348 — Re: [RFC PATCH v3] irqchip: Add support for Tango interrupt controller

Marc Gonzalez <marc_gonzalez@sigmadesigns.com> writes:

> On 20/01/2016 17:25, Måns Rullgård wrote:
>
>> Marc Zyngier <marc.zyngier@arm.com> writes:
>> 
>>> On 20/01/16 16:10, Måns Rullgård wrote:
>>>
>>>> Marc Zyngier <marc.zyngier@arm.com> writes:
>>>>
>>>>>> +	if (of_property_read_u32(node, "reg", &ctl))
>>>>>> +		panic("%s: failed to get reg base", node->name);
>>>>>> +
>>>>>> +	chip = kzalloc(sizeof(*chip), GFP_KERNEL);
>>>>>> +	chip->ctl = ctl;
>>>>>> +	chip->base = base;
>>>>
>>>> As I said before, this assumes the outer DT node uses a ranges
>>>> property.  Normally reg properties work the same whether they specify an
>>>> offset within an outer "ranges" or have a full address directly.  It
>>>> would be easy enough to make this work with either, so I don't see any
>>>> reason not to.
>>>
>>> Yup, that is a good point. I guess Marc can address this in the next
>>> round, since we need a DT binding anyway.
>> 
>> I'd suggest using of_address_to_resource() on both nodes and subtracting
>> the start addresses returned.
>
> For my own reference, Marc Zyngier suggested:
> "you should use of_iomap to map the child nodes, and not mess with
> the parent one."

That's going to get very messy since the generic irqchip code needs all
the registers as offsets from a common base address.

-- 
Måns Rullgård

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

#1313362 — Re: [RFC PATCH v3] irqchip: Add support for Tango interrupt controller

On 20/01/2016 17:38, Måns Rullgård wrote:

> Marc Gonzalez <marc_gonzalez@sigmadesigns.com> writes:
> 
>> On 20/01/2016 17:25, Måns Rullgård wrote:
>>
>>> Marc Zyngier <marc.zyngier@arm.com> writes:
>>>
>>>> On 20/01/16 16:10, Måns Rullgård wrote:
>>>>
>>>>> Marc Zyngier <marc.zyngier@arm.com> writes:
>>>>>
>>>>>>> +	if (of_property_read_u32(node, "reg", &ctl))
>>>>>>> +		panic("%s: failed to get reg base", node->name);
>>>>>>> +
>>>>>>> +	chip = kzalloc(sizeof(*chip), GFP_KERNEL);
>>>>>>> +	chip->ctl = ctl;
>>>>>>> +	chip->base = base;
>>>>>
>>>>> As I said before, this assumes the outer DT node uses a ranges
>>>>> property.  Normally reg properties work the same whether they specify an
>>>>> offset within an outer "ranges" or have a full address directly.  It
>>>>> would be easy enough to make this work with either, so I don't see any
>>>>> reason not to.
>>>>
>>>> Yup, that is a good point. I guess Marc can address this in the next
>>>> round, since we need a DT binding anyway.
>>>
>>> I'd suggest using of_address_to_resource() on both nodes and subtracting
>>> the start addresses returned.
>>
>> For my own reference, Marc Zyngier suggested:
>> "you should use of_iomap to map the child nodes, and not mess with
>> the parent one."
> 
> That's going to get very messy since the generic irqchip code needs all
> the registers as offsets from a common base address.

The two suggestions are over my head at the moment.

Do you want to submit v4 and have Marc Z take a look?

Regards.

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

#1313422 — Re: [RFC PATCH v3] irqchip: Add support for Tango interrupt controller

Marc Gonzalez <marc_gonzalez@sigmadesigns.com> writes:

> On 20/01/2016 17:38, Måns Rullgård wrote:
>
>> Marc Gonzalez <marc_gonzalez@sigmadesigns.com> writes:
>> 
>>> On 20/01/2016 17:25, Måns Rullgård wrote:
>>>
>>>> Marc Zyngier <marc.zyngier@arm.com> writes:
>>>>
>>>>> On 20/01/16 16:10, Måns Rullgård wrote:
>>>>>
>>>>>> Marc Zyngier <marc.zyngier@arm.com> writes:
>>>>>>
>>>>>>>> +	if (of_property_read_u32(node, "reg", &ctl))
>>>>>>>> +		panic("%s: failed to get reg base", node->name);
>>>>>>>> +
>>>>>>>> +	chip = kzalloc(sizeof(*chip), GFP_KERNEL);
>>>>>>>> +	chip->ctl = ctl;
>>>>>>>> +	chip->base = base;
>>>>>>
>>>>>> As I said before, this assumes the outer DT node uses a ranges
>>>>>> property.  Normally reg properties work the same whether they specify an
>>>>>> offset within an outer "ranges" or have a full address directly.  It
>>>>>> would be easy enough to make this work with either, so I don't see any
>>>>>> reason not to.
>>>>>
>>>>> Yup, that is a good point. I guess Marc can address this in the next
>>>>> round, since we need a DT binding anyway.
>>>>
>>>> I'd suggest using of_address_to_resource() on both nodes and subtracting
>>>> the start addresses returned.
>>>
>>> For my own reference, Marc Zyngier suggested:
>>> "you should use of_iomap to map the child nodes, and not mess with
>>> the parent one."
>> 
>> That's going to get very messy since the generic irqchip code needs all
>> the registers as offsets from a common base address.
>
> The two suggestions are over my head at the moment.
>
> Do you want to submit v4 and have Marc Z take a look?

Done.  If this isn't acceptable either, I'm out of ideas that don't end
up being far uglier than anything suggested so far.

-- 
Måns Rullgård

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

#1315094 — Re: [RFC PATCH v3] irqchip: Add support for Tango interrupt controller

On 20/01/2016 19:09, Måns Rullgård wrote:

> Marc Gonzalez <marc_gonzalez@sigmadesigns.com> writes:
> 
>> On 20/01/2016 17:38, Måns Rullgård wrote:
>>
>>> Marc Gonzalez <marc_gonzalez@sigmadesigns.com> writes:
>>>
>>>> On 20/01/2016 17:25, Måns Rullgård wrote:
>>>>
>>>>> Marc Zyngier <marc.zyngier@arm.com> writes:
>>>>>
>>>>>> On 20/01/16 16:10, Måns Rullgård wrote:
>>>>>>
>>>>>>> Marc Zyngier <marc.zyngier@arm.com> writes:
>>>>>>>
>>>>>>>>> +	if (of_property_read_u32(node, "reg", &ctl))
>>>>>>>>> +		panic("%s: failed to get reg base", node->name);
>>>>>>>>> +
>>>>>>>>> +	chip = kzalloc(sizeof(*chip), GFP_KERNEL);
>>>>>>>>> +	chip->ctl = ctl;
>>>>>>>>> +	chip->base = base;
>>>>>>>
>>>>>>> As I said before, this assumes the outer DT node uses a ranges
>>>>>>> property.  Normally reg properties work the same whether they specify an
>>>>>>> offset within an outer "ranges" or have a full address directly.  It
>>>>>>> would be easy enough to make this work with either, so I don't see any
>>>>>>> reason not to.
>>>>>>
>>>>>> Yup, that is a good point. I guess Marc can address this in the next
>>>>>> round, since we need a DT binding anyway.
>>>>>
>>>>> I'd suggest using of_address_to_resource() on both nodes and subtracting
>>>>> the start addresses returned.
>>>>
>>>> For my own reference, Marc Zyngier suggested:
>>>> "you should use of_iomap to map the child nodes, and not mess with
>>>> the parent one."
>>>
>>> That's going to get very messy since the generic irqchip code needs all
>>> the registers as offsets from a common base address.
>>
>> The two suggestions are over my head at the moment.
>>
>> Do you want to submit v4 and have Marc Z take a look?
> 
> Done.  If this isn't acceptable either, I'm out of ideas that don't end
> up being far uglier than anything suggested so far.

With your latest patch, can I drop the ranges property?

Regards.

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

#1315115 — Re: [RFC PATCH v3] irqchip: Add support for Tango interrupt controller

Marc Gonzalez <marc_gonzalez@sigmadesigns.com> writes:

> On 20/01/2016 19:09, Måns Rullgård wrote:
>
>> Marc Gonzalez <marc_gonzalez@sigmadesigns.com> writes:
>> 
>>> On 20/01/2016 17:38, Måns Rullgård wrote:
>>>
>>>> Marc Gonzalez <marc_gonzalez@sigmadesigns.com> writes:
>>>>
>>>>> On 20/01/2016 17:25, Måns Rullgård wrote:
>>>>>
>>>>>> Marc Zyngier <marc.zyngier@arm.com> writes:
>>>>>>
>>>>>>> On 20/01/16 16:10, Måns Rullgård wrote:
>>>>>>>
>>>>>>>> Marc Zyngier <marc.zyngier@arm.com> writes:
>>>>>>>>
>>>>>>>>>> +	if (of_property_read_u32(node, "reg", &ctl))
>>>>>>>>>> +		panic("%s: failed to get reg base", node->name);
>>>>>>>>>> +
>>>>>>>>>> +	chip = kzalloc(sizeof(*chip), GFP_KERNEL);
>>>>>>>>>> +	chip->ctl = ctl;
>>>>>>>>>> +	chip->base = base;
>>>>>>>>
>>>>>>>> As I said before, this assumes the outer DT node uses a ranges
>>>>>>>> property.  Normally reg properties work the same whether they specify an
>>>>>>>> offset within an outer "ranges" or have a full address directly.  It
>>>>>>>> would be easy enough to make this work with either, so I don't see any
>>>>>>>> reason not to.
>>>>>>>
>>>>>>> Yup, that is a good point. I guess Marc can address this in the next
>>>>>>> round, since we need a DT binding anyway.
>>>>>>
>>>>>> I'd suggest using of_address_to_resource() on both nodes and subtracting
>>>>>> the start addresses returned.
>>>>>
>>>>> For my own reference, Marc Zyngier suggested:
>>>>> "you should use of_iomap to map the child nodes, and not mess with
>>>>> the parent one."
>>>>
>>>> That's going to get very messy since the generic irqchip code needs all
>>>> the registers as offsets from a common base address.
>>>
>>> The two suggestions are over my head at the moment.
>>>
>>> Do you want to submit v4 and have Marc Z take a look?
>> 
>> Done.  If this isn't acceptable either, I'm out of ideas that don't end
>> up being far uglier than anything suggested so far.
>
> With your latest patch, can I drop the ranges property?

Why would you want to do that?

-- 
Måns Rullgård

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

#1315117 — Re: [RFC PATCH v3] irqchip: Add support for Tango interrupt controller

On 22/01/2016 17:35, Måns Rullgård wrote:
> Marc Gonzalez <marc_gonzalez@sigmadesigns.com> writes:
> 
>> On 20/01/2016 19:09, Måns Rullgård wrote:
>>
>>> Marc Gonzalez <marc_gonzalez@sigmadesigns.com> writes:
>>>
>>>> On 20/01/2016 17:38, Måns Rullgård wrote:
>>>>
>>>>> Marc Gonzalez <marc_gonzalez@sigmadesigns.com> writes:
>>>>>
>>>>>> On 20/01/2016 17:25, Måns Rullgård wrote:
>>>>>>
>>>>>>> Marc Zyngier <marc.zyngier@arm.com> writes:
>>>>>>>
>>>>>>>> On 20/01/16 16:10, Måns Rullgård wrote:
>>>>>>>>
>>>>>>>>> Marc Zyngier <marc.zyngier@arm.com> writes:
>>>>>>>>>
>>>>>>>>>>> +	if (of_property_read_u32(node, "reg", &ctl))
>>>>>>>>>>> +		panic("%s: failed to get reg base", node->name);
>>>>>>>>>>> +
>>>>>>>>>>> +	chip = kzalloc(sizeof(*chip), GFP_KERNEL);
>>>>>>>>>>> +	chip->ctl = ctl;
>>>>>>>>>>> +	chip->base = base;
>>>>>>>>>
>>>>>>>>> As I said before, this assumes the outer DT node uses a ranges
>>>>>>>>> property.  Normally reg properties work the same whether they specify an
>>>>>>>>> offset within an outer "ranges" or have a full address directly.  It
>>>>>>>>> would be easy enough to make this work with either, so I don't see any
>>>>>>>>> reason not to.
>>>>>>>>
>>>>>>>> Yup, that is a good point. I guess Marc can address this in the next
>>>>>>>> round, since we need a DT binding anyway.
>>>>>>>
>>>>>>> I'd suggest using of_address_to_resource() on both nodes and subtracting
>>>>>>> the start addresses returned.
>>>>>>
>>>>>> For my own reference, Marc Zyngier suggested:
>>>>>> "you should use of_iomap to map the child nodes, and not mess with
>>>>>> the parent one."
>>>>>
>>>>> That's going to get very messy since the generic irqchip code needs all
>>>>> the registers as offsets from a common base address.
>>>>
>>>> The two suggestions are over my head at the moment.
>>>>
>>>> Do you want to submit v4 and have Marc Z take a look?
>>>
>>> Done.  If this isn't acceptable either, I'm out of ideas that don't end
>>> up being far uglier than anything suggested so far.
>>
>> With your latest patch, can I drop the ranges property?
> 
> Why would you want to do that?

<confused> I thought that was the whole point of the v4 improvement?

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

#1315118 — Re: [RFC PATCH v3] irqchip: Add support for Tango interrupt controller

Marc Gonzalez <marc_gonzalez@sigmadesigns.com> writes:

> On 22/01/2016 17:35, Måns Rullgård wrote:
>> Marc Gonzalez <marc_gonzalez@sigmadesigns.com> writes:
>> 
>>> On 20/01/2016 19:09, Måns Rullgård wrote:
>>>
>>>> Marc Gonzalez <marc_gonzalez@sigmadesigns.com> writes:
>>>>
>>>>> On 20/01/2016 17:38, Måns Rullgård wrote:
>>>>>
>>>>>> Marc Gonzalez <marc_gonzalez@sigmadesigns.com> writes:
>>>>>>
>>>>>>> On 20/01/2016 17:25, Måns Rullgård wrote:
>>>>>>>
>>>>>>>> Marc Zyngier <marc.zyngier@arm.com> writes:
>>>>>>>>
>>>>>>>>> On 20/01/16 16:10, Måns Rullgård wrote:
>>>>>>>>>
>>>>>>>>>> Marc Zyngier <marc.zyngier@arm.com> writes:
>>>>>>>>>>
>>>>>>>>>>>> +	if (of_property_read_u32(node, "reg", &ctl))
>>>>>>>>>>>> +		panic("%s: failed to get reg base", node->name);
>>>>>>>>>>>> +
>>>>>>>>>>>> +	chip = kzalloc(sizeof(*chip), GFP_KERNEL);
>>>>>>>>>>>> +	chip->ctl = ctl;
>>>>>>>>>>>> +	chip->base = base;
>>>>>>>>>>
>>>>>>>>>> As I said before, this assumes the outer DT node uses a ranges
>>>>>>>>>> property.  Normally reg properties work the same whether they specify an
>>>>>>>>>> offset within an outer "ranges" or have a full address directly.  It
>>>>>>>>>> would be easy enough to make this work with either, so I don't see any
>>>>>>>>>> reason not to.
>>>>>>>>>
>>>>>>>>> Yup, that is a good point. I guess Marc can address this in the next
>>>>>>>>> round, since we need a DT binding anyway.
>>>>>>>>
>>>>>>>> I'd suggest using of_address_to_resource() on both nodes and subtracting
>>>>>>>> the start addresses returned.
>>>>>>>
>>>>>>> For my own reference, Marc Zyngier suggested:
>>>>>>> "you should use of_iomap to map the child nodes, and not mess with
>>>>>>> the parent one."
>>>>>>
>>>>>> That's going to get very messy since the generic irqchip code needs all
>>>>>> the registers as offsets from a common base address.
>>>>>
>>>>> The two suggestions are over my head at the moment.
>>>>>
>>>>> Do you want to submit v4 and have Marc Z take a look?
>>>>
>>>> Done.  If this isn't acceptable either, I'm out of ideas that don't end
>>>> up being far uglier than anything suggested so far.
>>>
>>> With your latest patch, can I drop the ranges property?
>> 
>> Why would you want to do that?
>
> <confused> I thought that was the whole point of the v4 improvement?

The point was to make it work right *if* someone were to do that even
though having it there better reflects what the hardware actually looks
like.

-- 
Måns Rullgård

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

#1315121 — Re: [RFC PATCH v3] irqchip: Add support for Tango interrupt controller

On 22/01/2016 17:39, Måns Rullgård wrote:

> Marc Gonzalez <marc_gonzalez@sigmadesigns.com> writes:
> 
>> On 22/01/2016 17:35, Måns Rullgård wrote:
>>
>>> Marc Gonzalez <marc_gonzalez@sigmadesigns.com> writes:
>>>
>>>> With your latest patch, can I drop the ranges property?
>>>
>>> Why would you want to do that?
>>
>> <confused> I thought that was the whole point of the v4 improvement?
> 
> The point was to make it work right *if* someone were to do that even
> though having it there better reflects what the hardware actually looks
> like.

That's the part I'm struggling to understand. Who other than you or me
would write a device tree from scratch for this interrupt controller?

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

#1315125 — Re: [RFC PATCH v3] irqchip: Add support for Tango interrupt controller

Marc Gonzalez <marc_gonzalez@sigmadesigns.com> writes:

> On 22/01/2016 17:39, Måns Rullgård wrote:
>
>> Marc Gonzalez <marc_gonzalez@sigmadesigns.com> writes:
>> 
>>> On 22/01/2016 17:35, Måns Rullgård wrote:
>>>
>>>> Marc Gonzalez <marc_gonzalez@sigmadesigns.com> writes:
>>>>
>>>>> With your latest patch, can I drop the ranges property?
>>>>
>>>> Why would you want to do that?
>>>
>>> <confused> I thought that was the whole point of the v4 improvement?
>> 
>> The point was to make it work right *if* someone were to do that even
>> though having it there better reflects what the hardware actually looks
>> like.
>
> That's the part I'm struggling to understand. Who other than you or me
> would write a device tree from scratch for this interrupt controller?

I wouldn't discount the possibility.

-- 
Måns Rullgård

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

#1313334 — Re: [RFC PATCH v3] irqchip: Add support for Tango interrupt controller

On 20/01/16 16:10, Måns Rullgård wrote:
> Marc Zyngier <marc.zyngier@arm.com> writes:
> 
>>> +	if (of_property_read_u32(node, "reg", &ctl))
>>> +		panic("%s: failed to get reg base", node->name);
>>> +
>>> +	chip = kzalloc(sizeof(*chip), GFP_KERNEL);
>>> +	chip->ctl = ctl;
>>> +	chip->base = base;
> 
> As I said before, this assumes the outer DT node uses a ranges
> property.  Normally reg properties work the same whether they specify an
> offset within an outer "ranges" or have a full address directly.  It
> would be easy enough to make this work with either, so I don't see any
> reason not to.

Yup, that is a good point. I guess Marc can address this in the next
round, since we need a DT binding anyway.

Thanks,

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

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

#1313336 — Re: [RFC PATCH v3] irqchip: Add support for Tango interrupt controller

Marc Gonzalez <marc_gonzalez@sigmadesigns.com> writes:

> On 20/01/2016 17:10, Måns Rullgård wrote:
>
>> Marc Zyngier wrote:
>> 
>>>> +	if (of_property_read_u32(node, "reg", &ctl))
>>>> +		panic("%s: failed to get reg base", node->name);
>>>> +
>>>> +	chip = kzalloc(sizeof(*chip), GFP_KERNEL);
>>>> +	chip->ctl = ctl;
>>>> +	chip->base = base;
>> 
>> As I said before, this assumes the outer DT node uses a ranges
>> property.  Normally reg properties work the same whether they specify an
>> offset within an outer "ranges" or have a full address directly.  It
>> would be easy enough to make this work with either, so I don't see any
>> reason not to.
>
> IIRC, I was told very early in the review process that the ranges prop
> was mandatory. Lemme look for it... It was Arnd:
>
> http://thread.gmane.org/gmane.linux.ports.arm.kernel/444131/focus=444207
>
>> You are missing a ranges property that describes what address
>> space these addresses are in.
>>
>> 'ranges;' would be wrong here, as the interrupt controller is
>> not a bus. If you have no ranges property, the bus is interpreted
>> as having its own address space with no relation to the parent bus
>> (e.g. an I2C bus uses addresses that are not memory mapped).
>> 
>> Just list the addresses that are actually decoded by child
>> devices here.
>
> Did I misunderstand?

It's still possible to create such a device tree, and that will fail in
very hard to debug ways.  Better to be a bit robust.

-- 
Måns Rullgård

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

#1313339 — Re: [RFC PATCH v3] irqchip: Add support for Tango interrupt controller

On 20/01/2016 17:10, Måns Rullgård wrote:

> Marc Zyngier wrote:
> 
>>> +	if (of_property_read_u32(node, "reg", &ctl))
>>> +		panic("%s: failed to get reg base", node->name);
>>> +
>>> +	chip = kzalloc(sizeof(*chip), GFP_KERNEL);
>>> +	chip->ctl = ctl;
>>> +	chip->base = base;
> 
> As I said before, this assumes the outer DT node uses a ranges
> property.  Normally reg properties work the same whether they specify an
> offset within an outer "ranges" or have a full address directly.  It
> would be easy enough to make this work with either, so I don't see any
> reason not to.

IIRC, I was told very early in the review process that the ranges prop
was mandatory. Lemme look for it... It was Arnd:

http://thread.gmane.org/gmane.linux.ports.arm.kernel/444131/focus=444207

> You are missing a ranges property that describes what address
> space these addresses are in.
>
> 'ranges;' would be wrong here, as the interrupt controller is
> not a bus. If you have no ranges property, the bus is interpreted
> as having its own address space with no relation to the parent bus
> (e.g. an I2C bus uses addresses that are not memory mapped).
> 
> Just list the addresses that are actually decoded by child
> devices here.

Did I misunderstand?

Regards.

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

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web