[PATCH 0/2] Support for Sigma Designs SMP86xx interrupt controller

[PATCH 0/2] Support for Sigma Designs SMP86xx interrupt controller

[Mans Rullgard]

The Sigma Designs SMP86xx and SMP87xx chips use a secondary interrupt
controller with 64 inputs.  The inputs go to an edge/level detector,
the outputs of which are replicated to three mask blocks, each with a
separate primary IRQ.

These patches add a device tree binding and an irqchip driver for this
controller.

Mans Rullgard (2):
  devicetree: add binding for Sigma Designs SMP86xx interrupt controller
  irqchip: add support for Sigma Designs SMP86xx interrupt controller

 .../interrupt-controller/sigma,smp8642-intc.txt    |  47 +++++
 drivers/irqchip/Kconfig                            |   5 +
 drivers/irqchip/Makefile                           |   1 +
 drivers/irqchip/irq-tangox.c                       | 232 +++++++++++++++++++++
 4 files changed, 285 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/sigma,smp8642-intc.txt
 create mode 100644 drivers/irqchip/irq-tangox.c

-- 
2.6.3

[PATCH 1/2] devicetree: add binding for Sigma Designs SMP86xx interrupt controller

[Mans Rullgard]

This adds a binding for the secondary interrupt controller in
Sigma Designs SMP86xx and SMP87xx chips.

Signed-off-by: Mans Rullgard <mans@mansr.com>
---
 .../interrupt-controller/sigma,smp8642-intc.txt    | 47 ++++++++++++++++++++++
 1 file changed, 47 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/sigma,smp8642-intc.txt

diff --git a/Documentation/devicetree/bindings/interrupt-controller/sigma,smp8642-intc.txt b/Documentation/devicetree/bindings/interrupt-controller/sigma,smp8642-intc.txt
new file mode 100644
index 0000000..f82cddf
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/sigma,smp8642-intc.txt
@@ -0,0 +1,47 @@
+Sigma Designs SMP86xx/SMP87xx secondary interrupt controller
+
+Required properties:
+- compatible: should be "sigma,smp8642-intc"
+- reg: physical address of MMIO region
+- interrupt-parent: phandle of parent interrupt controller
+- interrupt-controller: boolean
+
+One child node per control block with properties:
+- sigma,reg-offset: offset of registers from main base address
+- interrupt-controller: boolean
+- #interrupt-cells: should be <2>, interrupt index and flags per interrupts.txt
+- interrupts: interrupt spec of primary interrupt controller
+- label: (optional) name for display purposes
+
+Example:
+
+intc: interrupt-controller@6e000 {
+	compatible = "sigma,smp8642-intc";
+	reg = <0x6e000 0x400>;
+	interrupt-parent = <&gic>;
+	interrupt-controller;
+
+	irq0: irq0 {
+		sigma,reg-offset = <0x000>;
+		interrupt-controller;
+		#interrupt-cells = <2>;
+		interrupts = <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>;
+		label = "IRQ0";
+	};
+
+	irq1: irq1 {
+		sigma,reg-offset = <0x100>;
+		interrupt-controller;
+		#interrupt-cells = <2>;
+		interrupts = <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>;
+		label = "IRQ1";
+	};
+
+	irq2: irq2 {
+		sigma,reg-offset = <0x300>;
+		interrupt-controller;
+		#interrupt-cells = <2>;
+		interrupts = <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>;
+		label = "IRQ2";
+	};
+};
-- 
2.6.3

[PATCH 2/2] irqchip: add support for Sigma Designs SMP86xx interrupt controller

[Mans Rullgard]

This adds support for the secondary interrupt controller used in Sigma
Designs SMP86xx and SMP87xx chips.

Signed-off-by: Mans Rullgard <mans@mansr.com>
---
 drivers/irqchip/Kconfig      |   5 +
 drivers/irqchip/Makefile     |   1 +
 drivers/irqchip/irq-tangox.c | 232 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 238 insertions(+)
 create mode 100644 drivers/irqchip/irq-tangox.c

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 4d7294e..baf3345 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -193,3 +193,8 @@ config IRQ_MXS
 	def_bool y if MACH_ASM9260 || ARCH_MXS
 	select IRQ_DOMAIN
 	select STMP_DEVICE
+
+config TANGOX_IRQ
+	bool
+	select IRQ_DOMAIN
+	select GENERIC_IRQ_CHIP
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index 177f78f..763715c 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -55,3 +55,4 @@ obj-$(CONFIG_RENESAS_H8S_INTC)		+= irq-renesas-h8s.o
 obj-$(CONFIG_ARCH_SA1100)		+= irq-sa11x0.o
 obj-$(CONFIG_INGENIC_IRQ)		+= irq-ingenic.o
 obj-$(CONFIG_IMX_GPCV2)			+= irq-imx-gpcv2.o
+obj-$(CONFIG_TANGOX_IRQ)		+= irq-tangox.o
diff --git a/drivers/irqchip/irq-tangox.c b/drivers/irqchip/irq-tangox.c
new file mode 100644
index 0000000..630e2b5
--- /dev/null
+++ b/drivers/irqchip/irq-tangox.c
@@ -0,0 +1,232 @@
+/*
+ * 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);
+		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, "sigma,reg-offset", &ctl))
+		panic("%s: failed to get reg base", node->name);
+
+	if (of_property_read_string(node, "label", &name))
+		name = 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);
+
+	for (i = 0; i < 64; i++)
+		irq_create_mapping(dom, i);
+
+	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);
-- 
2.6.3

Re: [PATCH 2/2] irqchip: add support for Sigma Designs SMP86xx interrupt controller

[Marc Zyngier]

On 19/11/15 18:33, Mans Rullgard wrote:
> This adds support for the secondary interrupt controller used in Sigma
> Designs SMP86xx and SMP87xx chips.
> 
> Signed-off-by: Mans Rullgard <mans@mansr.com>
> ---
>  drivers/irqchip/Kconfig      |   5 +
>  drivers/irqchip/Makefile     |   1 +
>  drivers/irqchip/irq-tangox.c | 232 +++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 238 insertions(+)
>  create mode 100644 drivers/irqchip/irq-tangox.c
> 
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index 4d7294e..baf3345 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -193,3 +193,8 @@ config IRQ_MXS
>  	def_bool y if MACH_ASM9260 || ARCH_MXS
>  	select IRQ_DOMAIN
>  	select STMP_DEVICE
> +
> +config TANGOX_IRQ
> +	bool
> +	select IRQ_DOMAIN
> +	select GENERIC_IRQ_CHIP
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index 177f78f..763715c 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -55,3 +55,4 @@ obj-$(CONFIG_RENESAS_H8S_INTC)		+= irq-renesas-h8s.o
>  obj-$(CONFIG_ARCH_SA1100)		+= irq-sa11x0.o
>  obj-$(CONFIG_INGENIC_IRQ)		+= irq-ingenic.o
>  obj-$(CONFIG_IMX_GPCV2)			+= irq-imx-gpcv2.o
> +obj-$(CONFIG_TANGOX_IRQ)		+= irq-tangox.o
> diff --git a/drivers/irqchip/irq-tangox.c b/drivers/irqchip/irq-tangox.c
> new file mode 100644
> index 0000000..630e2b5
> --- /dev/null
> +++ b/drivers/irqchip/irq-tangox.c
> @@ -0,0 +1,232 @@
> +/*
> + * 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);

You may want to check virq in case you get interrupts from unexpected
sources (unlikely, but still).

> +		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, "sigma,reg-offset", &ctl))
> +		panic("%s: failed to get reg base", node->name);

My DT foo is a bit crap, but I'm sure there is ways to express ranges
inside a region that do not require to have vendor-specific properties.
Mark?

> +
> +	if (of_property_read_string(node, "label", &name))
> +		name = node->name;

Do you really need this cosmetic thing? node->name should be enough for
everybody, and the "label" has nothing to do with the HW description.

> +
> +	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);
> +
> +	for (i = 0; i < 64; i++)
> +		irq_create_mapping(dom, i);

/me puzzled. What's that for? You really should never need something
like this.

> +
> +	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);
> 

Thanks,

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

Re: [PATCH 2/2] irqchip: add support for Sigma Designs SMP86xx interrupt controller

[Måns Rullgård]

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

>> +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);
>
> You may want to check virq in case you get interrupts from unexpected
> sources (unlikely, but still).

Sure, never hurts to be safe.

>> +		generic_handle_irq(virq);
>> +		status &= ~BIT(hwirq);
>> +	}
>> +}

[...]

>> +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, "sigma,reg-offset", &ctl))
>> +		panic("%s: failed to get reg base", node->name);
>
> My DT foo is a bit crap, but I'm sure there is ways to express ranges
> inside a region that do not require to have vendor-specific properties.
> Mark?

I wasn't happy about that either.  The usual way is to use a "ranges"
property in the parent node and "reg" in the child node.  That makes it
easy to obtain a mapping of the child range using of_iomap() or
whatever.  The problem is that that's not what I need here.  The type
and ack registers are common while the enable/disable registers are per
sub-block, and the generic irqchip structs use a single base address and
offsets for the various registers, so I need the offset from the common
base to the start of the per-block registers, not the actual full
address.  I could use of_address_to_resource() and subtract one from the
other, I suppose.

>> +
>> +	if (of_property_read_string(node, "label", &name))
>> +		name = node->name;
>
> Do you really need this cosmetic thing? node->name should be enough for
> everybody, and the "label" has nothing to do with the HW description.

No, it's not needed.  I'll get rid of it.

>> +
>> +	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);
>> +
>> +	for (i = 0; i < 64; i++)
>> +		irq_create_mapping(dom, i);
>
> /me puzzled. What's that for? You really should never need something
> like this.

I had some reason for doing when I first wrote this code (MIPS, no DT),
but it's not needed now.

-- 
Måns Rullgård
mans@mansr.com

Re: [PATCH 2/2] irqchip: add support for Sigma Designs SMP86xx interrupt controller

[Måns Rullgård]

Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org> writes:

>> +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);
>
> You may want to check virq in case you get interrupts from unexpected
> sources (unlikely, but still).

Sure, never hurts to be safe.

>> +		generic_handle_irq(virq);
>> +		status &= ~BIT(hwirq);
>> +	}
>> +}

[...]

>> +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, "sigma,reg-offset", &ctl))
>> +		panic("%s: failed to get reg base", node->name);
>
> My DT foo is a bit crap, but I'm sure there is ways to express ranges
> inside a region that do not require to have vendor-specific properties.
> Mark?

I wasn't happy about that either.  The usual way is to use a "ranges"
property in the parent node and "reg" in the child node.  That makes it
easy to obtain a mapping of the child range using of_iomap() or
whatever.  The problem is that that's not what I need here.  The type
and ack registers are common while the enable/disable registers are per
sub-block, and the generic irqchip structs use a single base address and
offsets for the various registers, so I need the offset from the common
base to the start of the per-block registers, not the actual full
address.  I could use of_address_to_resource() and subtract one from the
other, I suppose.

>> +
>> +	if (of_property_read_string(node, "label", &name))
>> +		name = node->name;
>
> Do you really need this cosmetic thing? node->name should be enough for
> everybody, and the "label" has nothing to do with the HW description.

No, it's not needed.  I'll get rid of it.

>> +
>> +	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);
>> +
>> +	for (i = 0; i < 64; i++)
>> +		irq_create_mapping(dom, i);
>
> /me puzzled. What's that for? You really should never need something
> like this.

I had some reason for doing when I first wrote this code (MIPS, no DT),
but it's not needed now.

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

Re: [PATCH 2/2] irqchip: add support for Sigma Designs SMP86xx interrupt controller

[Mason]

On 19/11/2015 19:33, Mans Rullgard wrote:

> This adds support for the secondary interrupt controller used in Sigma
> Designs SMP86xx and SMP87xx chips.
> 
> Signed-off-by: Mans Rullgard <mans@mansr.com>
> ---
>  drivers/irqchip/Kconfig      |   5 +
>  drivers/irqchip/Makefile     |   1 +
>  drivers/irqchip/irq-tangox.c | 232 +++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 238 insertions(+)
>  create mode 100644 drivers/irqchip/irq-tangox.c
> 
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index 4d7294e..baf3345 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -193,3 +193,8 @@ config IRQ_MXS
>  	def_bool y if MACH_ASM9260 || ARCH_MXS
>  	select IRQ_DOMAIN
>  	select STMP_DEVICE
> +
> +config TANGOX_IRQ

Question: Kevin Hilman said I should use tango instead of tangox
for the arch directory. Does that advice extend to Kconfig names?

Regards.

Re: [PATCH 2/2] irqchip: add support for Sigma Designs SMP86xx interrupt controller

[Mason]

On 19/11/2015 19:33, Mans Rullgard wrote:

> This adds support for the secondary interrupt controller used in Sigma
> Designs SMP86xx and SMP87xx chips.
> 
> Signed-off-by: Mans Rullgard <mans-2StjZFpD7GcAvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/irqchip/Kconfig      |   5 +
>  drivers/irqchip/Makefile     |   1 +
>  drivers/irqchip/irq-tangox.c | 232 +++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 238 insertions(+)
>  create mode 100644 drivers/irqchip/irq-tangox.c
> 
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index 4d7294e..baf3345 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -193,3 +193,8 @@ config IRQ_MXS
>  	def_bool y if MACH_ASM9260 || ARCH_MXS
>  	select IRQ_DOMAIN
>  	select STMP_DEVICE
> +
> +config TANGOX_IRQ

Question: Kevin Hilman said I should use tango instead of tangox
for the arch directory. Does that advice extend to Kconfig names?

Regards.

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

Re: [PATCH 2/2] irqchip: add support for Sigma Designs SMP86xx interrupt controller

[Måns Rullgård]

Mason <slash.tmp@free.fr> writes:

> On 19/11/2015 19:33, Mans Rullgard wrote:
>
>> This adds support for the secondary interrupt controller used in Sigma
>> Designs SMP86xx and SMP87xx chips.
>> 
>> Signed-off-by: Mans Rullgard <mans@mansr.com>
>> ---
>>  drivers/irqchip/Kconfig      |   5 +
>>  drivers/irqchip/Makefile     |   1 +
>>  drivers/irqchip/irq-tangox.c | 232 +++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 238 insertions(+)
>>  create mode 100644 drivers/irqchip/irq-tangox.c
>> 
>> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
>> index 4d7294e..baf3345 100644
>> --- a/drivers/irqchip/Kconfig
>> +++ b/drivers/irqchip/Kconfig
>> @@ -193,3 +193,8 @@ config IRQ_MXS
>>  	def_bool y if MACH_ASM9260 || ARCH_MXS
>>  	select IRQ_DOMAIN
>>  	select STMP_DEVICE
>> +
>> +config TANGOX_IRQ
>
> Question: Kevin Hilman said I should use tango instead of tangox
> for the arch directory. Does that advice extend to Kconfig names?

I suppose the same logic applies to all names.

-- 
Måns Rullgård
mans@mansr.com

Re: [PATCH 2/2] irqchip: add support for Sigma Designs SMP86xx interrupt controller

[Måns Rullgård]

Mason <slash.tmp-GANU6spQydw@public.gmane.org> writes:

> On 19/11/2015 19:33, Mans Rullgard wrote:
>
>> This adds support for the secondary interrupt controller used in Sigma
>> Designs SMP86xx and SMP87xx chips.
>> 
>> Signed-off-by: Mans Rullgard <mans-2StjZFpD7GcAvxtiuMwx3w@public.gmane.org>
>> ---
>>  drivers/irqchip/Kconfig      |   5 +
>>  drivers/irqchip/Makefile     |   1 +
>>  drivers/irqchip/irq-tangox.c | 232 +++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 238 insertions(+)
>>  create mode 100644 drivers/irqchip/irq-tangox.c
>> 
>> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
>> index 4d7294e..baf3345 100644
>> --- a/drivers/irqchip/Kconfig
>> +++ b/drivers/irqchip/Kconfig
>> @@ -193,3 +193,8 @@ config IRQ_MXS
>>  	def_bool y if MACH_ASM9260 || ARCH_MXS
>>  	select IRQ_DOMAIN
>>  	select STMP_DEVICE
>> +
>> +config TANGOX_IRQ
>
> Question: Kevin Hilman said I should use tango instead of tangox
> for the arch directory. Does that advice extend to Kconfig names?

I suppose the same logic applies to all names.

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

Re: [PATCH 1/2] devicetree: add binding for Sigma Designs SMP86xx interrupt controller

[Rob Herring]

On Thu, Nov 19, 2015 at 06:33:45PM +0000, Mans Rullgard wrote:
> This adds a binding for the secondary interrupt controller in
> Sigma Designs SMP86xx and SMP87xx chips.
> 
> Signed-off-by: Mans Rullgard <mans@mansr.com>
> ---
>  .../interrupt-controller/sigma,smp8642-intc.txt    | 47 ++++++++++++++++++++++
>  1 file changed, 47 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/sigma,smp8642-intc.txt
> 
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/sigma,smp8642-intc.txt b/Documentation/devicetree/bindings/interrupt-controller/sigma,smp8642-intc.txt
> new file mode 100644
> index 0000000..f82cddf
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/sigma,smp8642-intc.txt
> @@ -0,0 +1,47 @@
> +Sigma Designs SMP86xx/SMP87xx secondary interrupt controller
> +
> +Required properties:
> +- compatible: should be "sigma,smp8642-intc"
> +- reg: physical address of MMIO region
> +- interrupt-parent: phandle of parent interrupt controller
> +- interrupt-controller: boolean
> +
> +One child node per control block with properties:
> +- sigma,reg-offset: offset of registers from main base address

Your driver defines these offsets too.

Do you expect to have many different variations here? If not, I would 
get rid of all the child nodes and just hard code it in the driver.

You could also simply do:

sigma,reg-offset = <0x0 0x100 0x300>;

> +- interrupt-controller: boolean
> +- #interrupt-cells: should be <2>, interrupt index and flags per interrupts.txt
> +- interrupts: interrupt spec of primary interrupt controller
> +- label: (optional) name for display purposes

NAK. Interrupt controllers are not really visible to users. Think of 
physical labels on things like connectors or LEDs.

> +
> +Example:
> +
> +intc: interrupt-controller@6e000 {
> +	compatible = "sigma,smp8642-intc";
> +	reg = <0x6e000 0x400>;
> +	interrupt-parent = <&gic>;
> +	interrupt-controller;
> +
> +	irq0: irq0 {
> +		sigma,reg-offset = <0x000>;
> +		interrupt-controller;
> +		#interrupt-cells = <2>;
> +		interrupts = <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>;
> +		label = "IRQ0";
> +	};
> +
> +	irq1: irq1 {
> +		sigma,reg-offset = <0x100>;
> +		interrupt-controller;
> +		#interrupt-cells = <2>;
> +		interrupts = <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>;
> +		label = "IRQ1";
> +	};
> +
> +	irq2: irq2 {
> +		sigma,reg-offset = <0x300>;
> +		interrupt-controller;
> +		#interrupt-cells = <2>;
> +		interrupts = <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>;
> +		label = "IRQ2";
> +	};
> +};
> -- 
> 2.6.3
> 

Re: [PATCH 1/2] devicetree: add binding for Sigma Designs SMP86xx interrupt controller

[Rob Herring]

On Thu, Nov 19, 2015 at 06:33:45PM +0000, Mans Rullgard wrote:
> This adds a binding for the secondary interrupt controller in
> Sigma Designs SMP86xx and SMP87xx chips.
> 
> Signed-off-by: Mans Rullgard <mans-2StjZFpD7GcAvxtiuMwx3w@public.gmane.org>
> ---
>  .../interrupt-controller/sigma,smp8642-intc.txt    | 47 ++++++++++++++++++++++
>  1 file changed, 47 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/sigma,smp8642-intc.txt
> 
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/sigma,smp8642-intc.txt b/Documentation/devicetree/bindings/interrupt-controller/sigma,smp8642-intc.txt
> new file mode 100644
> index 0000000..f82cddf
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/sigma,smp8642-intc.txt
> @@ -0,0 +1,47 @@
> +Sigma Designs SMP86xx/SMP87xx secondary interrupt controller
> +
> +Required properties:
> +- compatible: should be "sigma,smp8642-intc"
> +- reg: physical address of MMIO region
> +- interrupt-parent: phandle of parent interrupt controller
> +- interrupt-controller: boolean
> +
> +One child node per control block with properties:
> +- sigma,reg-offset: offset of registers from main base address

Your driver defines these offsets too.

Do you expect to have many different variations here? If not, I would 
get rid of all the child nodes and just hard code it in the driver.

You could also simply do:

sigma,reg-offset = <0x0 0x100 0x300>;

> +- interrupt-controller: boolean
> +- #interrupt-cells: should be <2>, interrupt index and flags per interrupts.txt
> +- interrupts: interrupt spec of primary interrupt controller
> +- label: (optional) name for display purposes

NAK. Interrupt controllers are not really visible to users. Think of 
physical labels on things like connectors or LEDs.

> +
> +Example:
> +
> +intc: interrupt-controller@6e000 {
> +	compatible = "sigma,smp8642-intc";
> +	reg = <0x6e000 0x400>;
> +	interrupt-parent = <&gic>;
> +	interrupt-controller;
> +
> +	irq0: irq0 {
> +		sigma,reg-offset = <0x000>;
> +		interrupt-controller;
> +		#interrupt-cells = <2>;
> +		interrupts = <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>;
> +		label = "IRQ0";
> +	};
> +
> +	irq1: irq1 {
> +		sigma,reg-offset = <0x100>;
> +		interrupt-controller;
> +		#interrupt-cells = <2>;
> +		interrupts = <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>;
> +		label = "IRQ1";
> +	};
> +
> +	irq2: irq2 {
> +		sigma,reg-offset = <0x300>;
> +		interrupt-controller;
> +		#interrupt-cells = <2>;
> +		interrupts = <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>;
> +		label = "IRQ2";
> +	};
> +};
> -- 
> 2.6.3
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] devicetree: add binding for Sigma Designs SMP86xx interrupt controller

[Måns Rullgård]

Rob Herring <robh@kernel.org> writes:

> On Thu, Nov 19, 2015 at 06:33:45PM +0000, Mans Rullgard wrote:
>> This adds a binding for the secondary interrupt controller in
>> Sigma Designs SMP86xx and SMP87xx chips.
>> 
>> Signed-off-by: Mans Rullgard <mans@mansr.com>
>> ---
>>  .../interrupt-controller/sigma,smp8642-intc.txt    | 47 ++++++++++++++++++++++
>>  1 file changed, 47 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/sigma,smp8642-intc.txt
>> 
>> diff --git a/Documentation/devicetree/bindings/interrupt-controller/sigma,smp8642-intc.txt b/Documentation/devicetree/bindings/interrupt-controller/sigma,smp8642-intc.txt
>> new file mode 100644
>> index 0000000..f82cddf
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/interrupt-controller/sigma,smp8642-intc.txt
>> @@ -0,0 +1,47 @@
>> +Sigma Designs SMP86xx/SMP87xx secondary interrupt controller
>> +
>> +Required properties:
>> +- compatible: should be "sigma,smp8642-intc"
>> +- reg: physical address of MMIO region
>> +- interrupt-parent: phandle of parent interrupt controller
>> +- interrupt-controller: boolean
>> +
>> +One child node per control block with properties:
>> +- sigma,reg-offset: offset of registers from main base address
>
> Your driver defines these offsets too.
>
> Do you expect to have many different variations here? If not, I would 
> get rid of all the child nodes and just hard code it in the driver.

Then how will other DT nodes reference the correct one?

-- 
Måns Rullgård
mans@mansr.com

Re: [PATCH 1/2] devicetree: add binding for Sigma Designs SMP86xx interrupt controller

[Måns Rullgård]

Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> writes:

> On Thu, Nov 19, 2015 at 06:33:45PM +0000, Mans Rullgard wrote:
>> This adds a binding for the secondary interrupt controller in
>> Sigma Designs SMP86xx and SMP87xx chips.
>> 
>> Signed-off-by: Mans Rullgard <mans-2StjZFpD7GcAvxtiuMwx3w@public.gmane.org>
>> ---
>>  .../interrupt-controller/sigma,smp8642-intc.txt    | 47 ++++++++++++++++++++++
>>  1 file changed, 47 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/sigma,smp8642-intc.txt
>> 
>> diff --git a/Documentation/devicetree/bindings/interrupt-controller/sigma,smp8642-intc.txt b/Documentation/devicetree/bindings/interrupt-controller/sigma,smp8642-intc.txt
>> new file mode 100644
>> index 0000000..f82cddf
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/interrupt-controller/sigma,smp8642-intc.txt
>> @@ -0,0 +1,47 @@
>> +Sigma Designs SMP86xx/SMP87xx secondary interrupt controller
>> +
>> +Required properties:
>> +- compatible: should be "sigma,smp8642-intc"
>> +- reg: physical address of MMIO region
>> +- interrupt-parent: phandle of parent interrupt controller
>> +- interrupt-controller: boolean
>> +
>> +One child node per control block with properties:
>> +- sigma,reg-offset: offset of registers from main base address
>
> Your driver defines these offsets too.
>
> Do you expect to have many different variations here? If not, I would 
> get rid of all the child nodes and just hard code it in the driver.

Then how will other DT nodes reference the correct one?

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

Re: [PATCH 2/2] irqchip: add support for Sigma Designs SMP86xx interrupt controller

[Mason]

[ Trimming CC list ]

On 19/11/2015 19:33, Mans Rullgard wrote:

> +config TANGOX_IRQ

Could you drop the X?
(And perhaps change IRQ to IRQCHIP? What's the current trend?)

> +	bool
> +	select IRQ_DOMAIN
> +	select GENERIC_IRQ_CHIP

Could you sort alphabetically, like the mach Kconfig?

Regards.

Re: [PATCH 2/2] irqchip: add support for Sigma Designs SMP86xx interrupt controller

[Mason]

> +	status_lo = intc_readl(chip, chip->ctl + IRQ_STATUS);
> +	status_hi = intc_readl(chip, chip->ctl + IRQ_CTL_HI + IRQ_STATUS);

In my local branch, I wrote:

#define IRQ_CTL_LO	0

	status_lo = intc_readl(chip, chip->ctl + IRQ_CTL_LO + IRQ_STATUS);
	status_hi = intc_readl(chip, chip->ctl + IRQ_CTL_HI + IRQ_STATUS);

(I'm a sucker for symmetry)

Regards.

Re: [PATCH 2/2] irqchip: add support for Sigma Designs SMP86xx interrupt controller

[Måns Rullgård]

Mason <slash.tmp@free.fr> writes:

> [ Trimming CC list ]
>
> On 19/11/2015 19:33, Mans Rullgard wrote:
>
>> +config TANGOX_IRQ
>
> Could you drop the X?

Sure, if that's what people prefer.

> (And perhaps change IRQ to IRQCHIP? What's the current trend?)

Most of the existing entries say just IRQ or something completely ad
hoc, only 4 IRQCHIP.

>> +	bool
>> +	select IRQ_DOMAIN
>> +	select GENERIC_IRQ_CHIP
>
> Could you sort alphabetically, like the mach Kconfig?

Tell that to all the other ones.  Oh well, whatever.

-- 
Måns Rullgård
mans@mansr.com

Re: [PATCH 2/2] irqchip: add support for Sigma Designs SMP86xx interrupt controller

[Måns Rullgård]

Mason <slash.tmp@free.fr> writes:

>> +	status_lo = intc_readl(chip, chip->ctl + IRQ_STATUS);
>> +	status_hi = intc_readl(chip, chip->ctl + IRQ_CTL_HI + IRQ_STATUS);
>
> In my local branch, I wrote:
>
> #define IRQ_CTL_LO	0
>
> 	status_lo = intc_readl(chip, chip->ctl + IRQ_CTL_LO + IRQ_STATUS);
> 	status_hi = intc_readl(chip, chip->ctl + IRQ_CTL_HI + IRQ_STATUS);
>
> (I'm a sucker for symmetry)

Nothing wrong with a little symmetry, though in this case I think the
extra macro only confuses matters.

-- 
Måns Rullgård
mans@mansr.com

Re: [PATCH 2/2] irqchip: add support for Sigma Designs SMP86xx interrupt controller

[Mason]

On 25/11/2015 13:12, Måns Rullgård wrote:

> Mason writes:
> 
>>> +	status_lo = intc_readl(chip, chip->ctl + IRQ_STATUS);
>>> +	status_hi = intc_readl(chip, chip->ctl + IRQ_CTL_HI + IRQ_STATUS);
>>
>> In my local branch, I wrote:
>>
>> #define IRQ_CTL_LO	0
>>
>> 	status_lo = intc_readl(chip, chip->ctl + IRQ_CTL_LO + IRQ_STATUS);
>> 	status_hi = intc_readl(chip, chip->ctl + IRQ_CTL_HI + IRQ_STATUS);
>>
>> (I'm a sucker for symmetry)
> 
> Nothing wrong with a little symmetry, though in this case I think the
> extra macro only confuses matters.

It's your call :-)

In my mind, the fact that the status_lo register sits at offset 0 is
just an accident. It's just that something has to sit at offset 0.
(Maybe I should tell the HW guys to put nothing at offset 0, and start
the actual register block at offset 4. /That/ would be unexpected.)

Another way to look at it is:

There are two 4-register blocks (LO and HI) each containing registers
{status,rawstat,enableset,enableclr}.

Block LO starts at offset 0x0
Block HI starts at offset 0x18

and then there are the intra offsets for the 4 registers in the block.

There! I got the bike-shedding out of my system ;-)

Regards.

Re: [PATCH 2/2] irqchip: add support for Sigma Designs SMP86xx interrupt controller

[Måns Rullgård]

Mason <slash.tmp@free.fr> writes:

> On 25/11/2015 13:12, Måns Rullgård wrote:
>
>> Mason writes:
>> 
>>>> +	status_lo = intc_readl(chip, chip->ctl + IRQ_STATUS);
>>>> +	status_hi = intc_readl(chip, chip->ctl + IRQ_CTL_HI + IRQ_STATUS);
>>>
>>> In my local branch, I wrote:
>>>
>>> #define IRQ_CTL_LO	0
>>>
>>> 	status_lo = intc_readl(chip, chip->ctl + IRQ_CTL_LO + IRQ_STATUS);
>>> 	status_hi = intc_readl(chip, chip->ctl + IRQ_CTL_HI + IRQ_STATUS);
>>>
>>> (I'm a sucker for symmetry)
>> 
>> Nothing wrong with a little symmetry, though in this case I think the
>> extra macro only confuses matters.
>
> It's your call :-)
>
> In my mind, the fact that the status_lo register sits at offset 0 is
> just an accident. It's just that something has to sit at offset 0.
> (Maybe I should tell the HW guys to put nothing at offset 0, and start
> the actual register block at offset 4. /That/ would be unexpected.)
>
> Another way to look at it is:
>
> There are two 4-register blocks (LO and HI) each containing registers
> {status,rawstat,enableset,enableclr}.
>
> Block LO starts at offset 0x0
> Block HI starts at offset 0x18
>
> and then there are the intra offsets for the 4 registers in the block.

When I wrote it, I was thinking of IRQ_CTL_HI as the offset to add to a
low register to get the corresponding high one.  I think that's what you
said there.

-- 
Måns Rullgård
mans@mansr.com