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

Groups > linux.kernel > #1304033 > unrolled thread

[PATCH v3 02/14] irqchip: irq-pic32-evic: Add support for PIC32 interrupt controller

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v3 02/14] irqchip: irq-pic32-evic: Add support for PIC32 interrupt controller Joshua Henderson <joshua.henderson@microchip.com> - 2016-01-08 01:00 +0100
    Re: [PATCH v3 02/14] irqchip: irq-pic32-evic: Add support for PIC32  interrupt controller Thomas Gleixner <tglx@linutronix.de> - 2016-01-08 20:10 +0100
      Re: [PATCH v3 02/14] irqchip: irq-pic32-evic: Add support for PIC32  interrupt controller Joshua Henderson <joshua.henderson@microchip.com> - 2016-01-08 23:50 +0100
        Re: [PATCH v3 02/14] irqchip: irq-pic32-evic: Add support for PIC32  interrupt controller Thomas Gleixner <tglx@linutronix.de> - 2016-01-10 11:20 +0100
          Re: [PATCH v3 02/14] irqchip: irq-pic32-evic: Add support for PIC32  interrupt controller Joshua Henderson <joshua.henderson@microchip.com> - 2016-01-12 20:40 +0100

#1304033 — [PATCH v3 02/14] irqchip: irq-pic32-evic: Add support for PIC32 interrupt controller

From: Cristian Birsan <cristian.birsan@microchip.com>

This adds support for the interrupt controller present on PIC32 class
devices.

The following features are supported:
 - DT properties for EVIC and for devices that use interrupt lines
 - Persistent and non-persistent interrupt handling
 - irqdomain support

Signed-off-by: Cristian Birsan <cristian.birsan@microchip.com>
Signed-off-by: Joshua Henderson <joshua.henderson@microchip.com>
Cc: Ralf Baechle <ralf@linux-mips.org>
---
 drivers/irqchip/Makefile           |    1 +
 drivers/irqchip/irq-pic32-evic.c   |  307 ++++++++++++++++++++++++++++++++++++
 include/linux/irqchip/pic32-evic.h |   19 +++
 3 files changed, 327 insertions(+)
 create mode 100644 drivers/irqchip/irq-pic32-evic.c
 create mode 100644 include/linux/irqchip/pic32-evic.h

diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index 177f78f..e3608fc 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_MACH_PIC32)		+= irq-pic32-evic.o
diff --git a/drivers/irqchip/irq-pic32-evic.c b/drivers/irqchip/irq-pic32-evic.c
new file mode 100644
index 0000000..fe7503d
--- /dev/null
+++ b/drivers/irqchip/irq-pic32-evic.c
@@ -0,0 +1,307 @@
+/*
+ * Cristian Birsan <cristian.birsan@microchip.com>
+ * Copyright (C) 2015 Microchip Technology Inc.  All rights reserved.
+ *
+ * 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/kernel.h>
+#include <linux/module.h>
+#include <linux/interrupt.h>
+#include <linux/irqdomain.h>
+#include <linux/of_address.h>
+#include <linux/slab.h>
+#include <linux/io.h>
+#include <linux/irqchip.h>
+#include <linux/irqchip/pic32-evic.h>
+
+#include <asm/irq.h>
+#include <asm/traps.h>
+
+#define CORE_TIMER_INTERRUPT 0
+#define EXTERNAL_INTERRUPT_0 3
+#define EXTERNAL_INTERRUPT_1 8
+#define EXTERNAL_INTERRUPT_2 13
+#define EXTERNAL_INTERRUPT_3 18
+#define EXTERNAL_INTERRUPT_4 23
+
+#define PRI_MASK	0x7	/* 3 bit priority mask */
+#define SUBPRI_MASK	0x3	/* 2 bit subpriority mask */
+#define INT_MASK	0x1F	/* 5 bit pri and subpri mask */
+#define NR_EXT_IRQS	5	/* 5 external interrupts sources */
+
+#define PIC32_INT_PRI(pri, subpri)	\
+	(((pri & PRI_MASK) << 2) | (subpri & SUBPRI_MASK))
+#define DEFAULT_PIC32_INT_PRI PIC32_INT_PRI(2, 0)
+
+static struct irq_domain *evic_irq_domain;
+static struct evic __iomem *evic_base;
+
+static unsigned int *evic_irq_prio;
+
+struct pic_reg {
+	u32 val; /* value register*/
+	u32 clr; /* clear register */
+	u32 set; /* set register */
+	u32 inv; /* inv register */
+} __packed;
+
+struct evic {
+	struct pic_reg intcon;
+	struct pic_reg priss;
+	struct pic_reg intstat;
+	struct pic_reg iptmr;
+	struct pic_reg ifs[6];
+	u32 reserved1[8];
+	struct pic_reg iec[6];
+	u32 reserved2[8];
+	struct pic_reg ipc[48];
+	u32 reserved3[64];
+	u32 off[191];
+} __packed;
+
+static int get_ext_irq_index(irq_hw_number_t hw);
+static void evic_set_ext_irq_polarity(int ext_irq, u32 type);
+
+#define BIT_REG_MASK(bit, reg, mask)		\
+	do {					\
+		reg = bit/32;			\
+		mask = 1 << (bit % 32);		\
+	} while (0)
+
+asmlinkage void __weak plat_irq_dispatch(void)
+{
+	unsigned int irq, hwirq;
+	u32 reg, mask;
+
+	hwirq = readl(&evic_base->intstat.val) & 0xFF;
+
+	/* Check if the interrupt was really triggered by hardware*/
+	BIT_REG_MASK(hwirq, reg, mask);
+	if (likely(readl(&evic_base->ifs[reg].val) &
+			readl(&evic_base->iec[reg].val) & mask)) {
+		irq = irq_linear_revmap(evic_irq_domain, hwirq);
+		do_IRQ(irq);
+	} else
+		spurious_interrupt();
+}
+
+/* mask off an interrupt */
+static inline void mask_pic32_irq(struct irq_data *irqd)
+{
+	u32 reg, mask;
+	unsigned int hwirq = irqd_to_hwirq(irqd);
+
+	BIT_REG_MASK(hwirq, reg, mask);
+	writel(mask, &evic_base->iec[reg].clr);
+}
+
+/* unmask an interrupt */
+static inline void unmask_pic32_irq(struct irq_data *irqd)
+{
+	u32 reg, mask;
+	unsigned int hwirq = irqd_to_hwirq(irqd);
+
+	BIT_REG_MASK(hwirq, reg, mask);
+	writel(mask, &evic_base->iec[reg].set);
+}
+
+/* acknowledge an interrupt */
+static void ack_pic32_irq(struct irq_data *irqd)
+{
+	u32 reg, mask;
+	unsigned int hwirq = irqd_to_hwirq(irqd);
+
+	BIT_REG_MASK(hwirq, reg, mask);
+	writel(mask, &evic_base->ifs[reg].clr);
+}
+
+static int set_type_pic32_irq(struct irq_data *data, unsigned int flow_type)
+{
+	int index;
+
+	switch (flow_type) {
+
+	case IRQ_TYPE_EDGE_RISING:
+	case IRQ_TYPE_EDGE_FALLING:
+		irq_set_handler_locked(data, handle_edge_irq);
+		break;
+
+	case IRQ_TYPE_LEVEL_HIGH:
+	case IRQ_TYPE_LEVEL_LOW:
+		irq_set_handler_locked(data, handle_fasteoi_irq);
+		break;
+
+	default:
+		pr_err("Invalid interrupt type !\n");
+		return -EINVAL;
+	}
+
+	/* set polarity for external interrupts only */
+	index = get_ext_irq_index(data->hwirq);
+	if (index >= 0)
+		evic_set_ext_irq_polarity(index, flow_type);
+
+	return IRQ_SET_MASK_OK;
+}
+
+static void pic32_bind_evic_interrupt(int irq, int set)
+{
+	writel(set, &evic_base->off[irq]);
+}
+
+int pic32_get_c0_compare_int(void)
+{
+	int virq;
+
+	virq = irq_create_mapping(evic_irq_domain, CORE_TIMER_INTERRUPT);
+	irq_set_irq_type(virq, IRQ_TYPE_EDGE_RISING);
+	return virq;
+}
+
+static struct irq_chip pic32_irq_chip = {
+	.name = "PIC32-EVIC",
+	.irq_ack = ack_pic32_irq,
+	.irq_mask = mask_pic32_irq,
+	.irq_unmask = unmask_pic32_irq,
+	.irq_eoi = ack_pic32_irq,
+	.irq_set_type = set_type_pic32_irq,
+};
+
+static void evic_set_irq_priority(int irq, int priority)
+{
+	u32 reg, shift;
+
+	reg = irq / 4;
+	shift = (irq % 4) * 8;
+
+	/* set priority */
+	writel(INT_MASK << shift, &evic_base->ipc[reg].clr);
+	writel(priority << shift, &evic_base->ipc[reg].set);
+}
+
+static void evic_set_ext_irq_polarity(int ext_irq, u32 type)
+{
+	if (WARN_ON(ext_irq >= NR_EXT_IRQS))
+		return;
+	switch (type) {
+	case IRQ_TYPE_EDGE_RISING:
+		writel(1 << ext_irq, &evic_base->intcon.set);
+		break;
+	case IRQ_TYPE_EDGE_FALLING:
+		writel(1 << ext_irq, &evic_base->intcon.clr);
+		break;
+	default:
+		pr_err("Invalid external interrupt polarity !\n");
+	}
+}
+
+static int get_ext_irq_index(irq_hw_number_t hw)
+{
+	switch (hw) {
+	case EXTERNAL_INTERRUPT_0:
+		return 0;
+	case EXTERNAL_INTERRUPT_1:
+		return 1;
+	case EXTERNAL_INTERRUPT_2:
+		return 2;
+	case EXTERNAL_INTERRUPT_3:
+		return 3;
+	case EXTERNAL_INTERRUPT_4:
+		return 4;
+	default:
+		return -1;
+	}
+}
+
+static int evic_intc_map(struct irq_domain *irqd, unsigned int virq,
+			irq_hw_number_t hw)
+{
+	u32 reg, mask;
+
+	irq_set_chip(virq, &pic32_irq_chip);
+
+	BIT_REG_MASK(hw, reg, mask);
+
+	/* disable */
+	writel(mask, &evic_base->iec[reg].clr);
+
+	/* clear flag */
+	writel(mask, &evic_base->ifs[reg].clr);
+
+	evic_set_irq_priority(hw, evic_irq_prio[hw]);
+
+	return 0;
+}
+
+static int evic_irq_domain_xlate(struct irq_domain *d,
+				struct device_node *ctrlr,
+				const u32 *intspec,
+				unsigned int intsize,
+				irq_hw_number_t *out_hwirq,
+				unsigned int *out_type)
+{
+	/* Check for number of params */
+	if (WARN_ON(intsize < 2))
+		return -EINVAL;
+	if (WARN_ON(intspec[0] >= NR_IRQS))
+		return -EINVAL;
+
+	*out_hwirq = intspec[0];
+
+	evic_irq_prio[*out_hwirq] = DEFAULT_PIC32_INT_PRI;
+
+	*out_type = intspec[1];
+
+	return 0;
+}
+
+static const struct irq_domain_ops evic_intc_irq_domain_ops = {
+		.map = evic_intc_map,
+		.xlate = evic_irq_domain_xlate,
+};
+
+#ifdef CONFIG_OF
+static int __init
+microchip_evic_of_init(struct device_node *node, struct device_node *parent)
+{
+	struct resource res;
+
+	if (WARN_ON(!node))
+		return -ENODEV;
+
+	evic_irq_prio = kcalloc(NR_IRQS, sizeof(*evic_irq_prio),
+				GFP_KERNEL);
+	if (!evic_irq_prio)
+		return -ENOMEM;
+
+	evic_irq_prio[CORE_TIMER_INTERRUPT] = DEFAULT_PIC32_INT_PRI;
+
+	if (of_address_to_resource(node, 0, &res))
+		panic("Failed to get evic memory range");
+
+	if (request_mem_region(res.start, resource_size(&res),
+				res.name) == NULL)
+		panic("Failed to request evic memory");
+
+	evic_base = ioremap_nocache(res.start, resource_size(&res));
+	if (!evic_base)
+		panic("Failed to remap evic memory");
+
+	board_bind_eic_interrupt = &pic32_bind_evic_interrupt;
+
+	evic_irq_domain = irq_domain_add_linear(node, NR_IRQS,
+			&evic_intc_irq_domain_ops, NULL);
+	if (!evic_irq_domain)
+		panic("Failed to add linear irqdomain for EVIC");
+
+	irq_set_default_host(evic_irq_domain);
+
+	return 0;
+}
+
+IRQCHIP_DECLARE(microchip_evic, "microchip,pic32mzda-evic",
+		microchip_evic_of_init);
+#endif
diff --git a/include/linux/irqchip/pic32-evic.h b/include/linux/irqchip/pic32-evic.h
new file mode 100644
index 0000000..c514bae
--- /dev/null
+++ b/include/linux/irqchip/pic32-evic.h
@@ -0,0 +1,19 @@
+/*
+ * Joshua Henderson, <joshua.henderson@microchip.com>
+ * Copyright (C) 2015 Microchip Technology Inc.  All rights reserved.
+ *
+ *  This program is free software; you can distribute it and/or modify it
+ *  under the terms of the GNU General Public License (Version 2) as
+ *  published by the Free Software Foundation.
+ *
+ *  This program is distributed in the hope it will be useful, but WITHOUT
+ *  ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ *  FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ *  for more details.
+ */
+#ifndef __LINUX_IRQCHIP_PIC32_EVIC_H
+#define __LINUX_IRQCHIP_PIC32_EVIC_H
+
+extern int pic32_get_c0_compare_int(void);
+
+#endif /* __LINUX_IRQCHIP_PIC32_EVIC_H */
-- 
1.7.9.5

[toc] | [next] | [standalone]

#1304939 — Re: [PATCH v3 02/14] irqchip: irq-pic32-evic: Add support for PIC32 interrupt controller

On Thu, 7 Jan 2016, Joshua Henderson wrote:
> +struct pic_reg {
> +	u32 val; /* value register*/

Just a nit. If you want to document your data structure, then please use
KernelDoc. These tail comments are horrible.

> +	u32 clr; /* clear register */
> +	u32 set; /* set register */
> +	u32 inv; /* inv register */
> +} __packed;
> +
> +struct evic {
> +	struct pic_reg intcon;
> +	struct pic_reg priss;
> +	struct pic_reg intstat;
> +	struct pic_reg iptmr;
> +	struct pic_reg ifs[6];
> +	u32 reserved1[8];
> +	struct pic_reg iec[6];
> +	u32 reserved2[8];
> +	struct pic_reg ipc[48];
> +	u32 reserved3[64];
> +	u32 off[191];

It would be way simpler to parse if you structured it like a table

+	struct pic_reg	intcon;
+	struct pic_reg	priss;
+	struct pic_reg	intstat;
+	struct pic_reg	iptmr;
+	struct pic_reg	ifs[6];
+	u32		reserved1[8];
+	struct pic_reg	iec[6];
+	u32		reserved2[8];
+	struct pic_reg	ipc[48];
+	u32		reserved3[64];
+	u32		off[191];

> +} __packed;
> +
> +static int get_ext_irq_index(irq_hw_number_t hw);
> +static void evic_set_ext_irq_polarity(int ext_irq, u32 type);

If you move the functions right here, then you don't need the forward
declarations.

> +/* mask off an interrupt */
> +static inline void mask_pic32_irq(struct irq_data *irqd)
> +{
> +	u32 reg, mask;
> +	unsigned int hwirq = irqd_to_hwirq(irqd);
> +
> +	BIT_REG_MASK(hwirq, reg, mask);
> +	writel(mask, &evic_base->iec[reg].clr);
> +}
> +
> +/* unmask an interrupt */
> +static inline void unmask_pic32_irq(struct irq_data *irqd)
> +{
> +	u32 reg, mask;
> +	unsigned int hwirq = irqd_to_hwirq(irqd);
> +
> +	BIT_REG_MASK(hwirq, reg, mask);
> +	writel(mask, &evic_base->iec[reg].set);
> +}
> +
> +/* acknowledge an interrupt */
> +static void ack_pic32_irq(struct irq_data *irqd)
> +{
> +	u32 reg, mask;
> +	unsigned int hwirq = irqd_to_hwirq(irqd);
> +
> +	BIT_REG_MASK(hwirq, reg, mask);
> +	writel(mask, &evic_base->ifs[reg].clr);

So you invented an open coded variant of the generic irq chip. Just with the
difference that the generic chip caches the mask and the register offsets ....

> +}
> +
> +static int set_type_pic32_irq(struct irq_data *data, unsigned int flow_type)
> +{
> +	int index;
> +
> +	switch (flow_type) {
> +
> +	case IRQ_TYPE_EDGE_RISING:
> +	case IRQ_TYPE_EDGE_FALLING:
> +		irq_set_handler_locked(data, handle_edge_irq);
> +		break;
> +
> +	case IRQ_TYPE_LEVEL_HIGH:
> +	case IRQ_TYPE_LEVEL_LOW:
> +		irq_set_handler_locked(data, handle_fasteoi_irq);
> +		break;
> +
> +	default:
> +		pr_err("Invalid interrupt type !\n");
> +		return -EINVAL;
> +	}
> +
> +	/* set polarity for external interrupts only */
> +	index = get_ext_irq_index(data->hwirq);
> +	if (index >= 0)
> +		evic_set_ext_irq_polarity(index, flow_type);

So for the non external interrupts you set a different handler and be
done. How is that supposed to work? They switch magically from one mode to the
other?

> +static void pic32_bind_evic_interrupt(int irq, int set)
> +{
> +	writel(set, &evic_base->off[irq]);
> +}
> +
> +int pic32_get_c0_compare_int(void)
> +{
> +	int virq;
> +
> +	virq = irq_create_mapping(evic_irq_domain, CORE_TIMER_INTERRUPT);
> +	irq_set_irq_type(virq, IRQ_TYPE_EDGE_RISING);
> +	return virq;

Why isn't that information retrieved via device tree?

> +}
> +
> +static struct irq_chip pic32_irq_chip = {
> +	.name = "PIC32-EVIC",
> +	.irq_ack = ack_pic32_irq,
> +	.irq_mask = mask_pic32_irq,
> +	.irq_unmask = unmask_pic32_irq,
> +	.irq_eoi = ack_pic32_irq,
> +	.irq_set_type = set_type_pic32_irq,

Again, this want's to be in tabular form, if at all.

> +};
> +
> +static void evic_set_irq_priority(int irq, int priority)
> +{
> +	u32 reg, shift;
> +
> +	reg = irq / 4;
> +	shift = (irq % 4) * 8;
> +
> +	/* set priority */
> +	writel(INT_MASK << shift, &evic_base->ipc[reg].clr);
> +	writel(priority << shift, &evic_base->ipc[reg].set);
> +}
> +
> +static void evic_set_ext_irq_polarity(int ext_irq, u32 type)
> +{
> +	if (WARN_ON(ext_irq >= NR_EXT_IRQS))
> +		return;

That WARN_ON is really helpful because you already made sure not to call it
for non EXT irqs.

> +	switch (type) {
> +	case IRQ_TYPE_EDGE_RISING:
> +		writel(1 << ext_irq, &evic_base->intcon.set);
> +		break;
> +	case IRQ_TYPE_EDGE_FALLING:
> +		writel(1 << ext_irq, &evic_base->intcon.clr);
> +		break;
> +	default:
> +		pr_err("Invalid external interrupt polarity !\n");
> +	}
> +}
> +
> +static int get_ext_irq_index(irq_hw_number_t hw)
> +{
> +	switch (hw) {
> +	case EXTERNAL_INTERRUPT_0:
> +		return 0;
> +	case EXTERNAL_INTERRUPT_1:
> +		return 1;
> +	case EXTERNAL_INTERRUPT_2:
> +		return 2;
> +	case EXTERNAL_INTERRUPT_3:
> +		return 3;
> +	case EXTERNAL_INTERRUPT_4:
> +		return 4;
> +	default:
> +		return -1;
> +	}
> +}

Why don't you use a seperate irq chip for the ext irqs? You can do that with
the generic chip as well.

    irq_alloc_domain_generic_chips(domain, 32, 2, ....)

And then you assign the alternate chip (2) to your ext irqs and have the set
type function only for the alternate chip.

Thanks,

	tglx

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

#1305045 — Re: [PATCH v3 02/14] irqchip: irq-pic32-evic: Add support for PIC32 interrupt controller

Thomas,

On 01/08/2016 12:04 PM, Thomas Gleixner wrote:
> On Thu, 7 Jan 2016, Joshua Henderson wrote:
>> +struct pic_reg {
>> +	u32 val; /* value register*/
> 
> Just a nit. If you want to document your data structure, then please use
> KernelDoc. These tail comments are horrible.
> 

Consider it done.

>> +	u32 clr; /* clear register */
>> +	u32 set; /* set register */
>> +	u32 inv; /* inv register */
>> +} __packed;
>> +
>> +struct evic {
>> +	struct pic_reg intcon;
>> +	struct pic_reg priss;
>> +	struct pic_reg intstat;
>> +	struct pic_reg iptmr;
>> +	struct pic_reg ifs[6];
>> +	u32 reserved1[8];
>> +	struct pic_reg iec[6];
>> +	u32 reserved2[8];
>> +	struct pic_reg ipc[48];
>> +	u32 reserved3[64];
>> +	u32 off[191];
> 
> It would be way simpler to parse if you structured it like a table
> 

Ack

> +	struct pic_reg	intcon;
> +	struct pic_reg	priss;
> +	struct pic_reg	intstat;
> +	struct pic_reg	iptmr;
> +	struct pic_reg	ifs[6];
> +	u32		reserved1[8];
> +	struct pic_reg	iec[6];
> +	u32		reserved2[8];
> +	struct pic_reg	ipc[48];
> +	u32		reserved3[64];
> +	u32		off[191];
> 
>> +} __packed;
>> +
>> +static int get_ext_irq_index(irq_hw_number_t hw);
>> +static void evic_set_ext_irq_polarity(int ext_irq, u32 type);
> 
> If you move the functions right here, then you don't need the forward
> declarations.
> 

Ack

>> +/* mask off an interrupt */
>> +static inline void mask_pic32_irq(struct irq_data *irqd)
>> +{
>> +	u32 reg, mask;
>> +	unsigned int hwirq = irqd_to_hwirq(irqd);
>> +
>> +	BIT_REG_MASK(hwirq, reg, mask);
>> +	writel(mask, &evic_base->iec[reg].clr);
>> +}
>> +
>> +/* unmask an interrupt */
>> +static inline void unmask_pic32_irq(struct irq_data *irqd)
>> +{
>> +	u32 reg, mask;
>> +	unsigned int hwirq = irqd_to_hwirq(irqd);
>> +
>> +	BIT_REG_MASK(hwirq, reg, mask);
>> +	writel(mask, &evic_base->iec[reg].set);
>> +}
>> +
>> +/* acknowledge an interrupt */
>> +static void ack_pic32_irq(struct irq_data *irqd)
>> +{
>> +	u32 reg, mask;
>> +	unsigned int hwirq = irqd_to_hwirq(irqd);
>> +
>> +	BIT_REG_MASK(hwirq, reg, mask);
>> +	writel(mask, &evic_base->ifs[reg].clr);
> 
> So you invented an open coded variant of the generic irq chip. Just with the
> difference that the generic chip caches the mask and the register offsets ....
> 

On PIC32 we have 4 different register offsets in many cases, including the interrupt
controller registers, to write to one hardware register.  The PIC32 has special
write only registers for set/clear/invert and which one is used is dependent on
the logic at the time of writel(). Point being, there is no obvious value in
caching when using these registers.  We don't have to perform a readl() at any
time beforehand to write a mask to a register to update it atomically.

>> +}
>> +
>> +static int set_type_pic32_irq(struct irq_data *data, unsigned int flow_type)
>> +{
>> +	int index;
>> +
>> +	switch (flow_type) {
>> +
>> +	case IRQ_TYPE_EDGE_RISING:
>> +	case IRQ_TYPE_EDGE_FALLING:
>> +		irq_set_handler_locked(data, handle_edge_irq);
>> +		break;
>> +
>> +	case IRQ_TYPE_LEVEL_HIGH:
>> +	case IRQ_TYPE_LEVEL_LOW:
>> +		irq_set_handler_locked(data, handle_fasteoi_irq);
>> +		break;
>> +
>> +	default:
>> +		pr_err("Invalid interrupt type !\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	/* set polarity for external interrupts only */
>> +	index = get_ext_irq_index(data->hwirq);
>> +	if (index >= 0)
>> +		evic_set_ext_irq_polarity(index, flow_type);
> 
> So for the non external interrupts you set a different handler and be
> done. How is that supposed to work? They switch magically from one mode to the
> other?
> 

It's all the same handlers (depending on whether it's persistent or
non-persistent) irrelevant of it being an external interrupt or not.  It's all
the same hardware interrupt controller.  Some pins on the chip can be configured
as an interrupt source through pin configuration and those have dedicated
interrupts associated with them.  The only thing "special" about these external
interrupts is they must be explicitly configured as edge rising or edge falling
in hardware- which is what is being handled here.  Non-external interrupts don't
need this configuration.

>> +static void pic32_bind_evic_interrupt(int irq, int set)
>> +{
>> +	writel(set, &evic_base->off[irq]);
>> +}
>> +
>> +int pic32_get_c0_compare_int(void)
>> +{
>> +	int virq;
>> +
>> +	virq = irq_create_mapping(evic_irq_domain, CORE_TIMER_INTERRUPT);
>> +	irq_set_irq_type(virq, IRQ_TYPE_EDGE_RISING);
>> +	return virq;
> 
> Why isn't that information retrieved via device tree?
> 

I suppose it could be.  We took a lesson from irq-mips-gic.c on this one.

>> +}
>> +
>> +static struct irq_chip pic32_irq_chip = {
>> +	.name = "PIC32-EVIC",
>> +	.irq_ack = ack_pic32_irq,
>> +	.irq_mask = mask_pic32_irq,
>> +	.irq_unmask = unmask_pic32_irq,
>> +	.irq_eoi = ack_pic32_irq,
>> +	.irq_set_type = set_type_pic32_irq,
> 
> Again, this want's to be in tabular form, if at all.
> 

Ack

>> +};
>> +
>> +static void evic_set_irq_priority(int irq, int priority)
>> +{
>> +	u32 reg, shift;
>> +
>> +	reg = irq / 4;
>> +	shift = (irq % 4) * 8;
>> +
>> +	/* set priority */
>> +	writel(INT_MASK << shift, &evic_base->ipc[reg].clr);
>> +	writel(priority << shift, &evic_base->ipc[reg].set);
>> +}
>> +
>> +static void evic_set_ext_irq_polarity(int ext_irq, u32 type)
>> +{
>> +	if (WARN_ON(ext_irq >= NR_EXT_IRQS))
>> +		return;
> 
> That WARN_ON is really helpful because you already made sure not to call it
> for non EXT irqs.
> 

It is indeed redundant.

>> +	switch (type) {
>> +	case IRQ_TYPE_EDGE_RISING:
>> +		writel(1 << ext_irq, &evic_base->intcon.set);
>> +		break;
>> +	case IRQ_TYPE_EDGE_FALLING:
>> +		writel(1 << ext_irq, &evic_base->intcon.clr);
>> +		break;
>> +	default:
>> +		pr_err("Invalid external interrupt polarity !\n");
>> +	}
>> +}
>> +
>> +static int get_ext_irq_index(irq_hw_number_t hw)
>> +{
>> +	switch (hw) {
>> +	case EXTERNAL_INTERRUPT_0:
>> +		return 0;
>> +	case EXTERNAL_INTERRUPT_1:
>> +		return 1;
>> +	case EXTERNAL_INTERRUPT_2:
>> +		return 2;
>> +	case EXTERNAL_INTERRUPT_3:
>> +		return 3;
>> +	case EXTERNAL_INTERRUPT_4:
>> +		return 4;
>> +	default:
>> +		return -1;
>> +	}
>> +}
> 
> Why don't you use a seperate irq chip for the ext irqs? You can do that with
> the generic chip as well.
> 
>     irq_alloc_domain_generic_chips(domain, 32, 2, ....)
> 
> And then you assign the alternate chip (2) to your ext irqs and have the set
> type function only for the alternate chip.
> 

We have one interrupt controller.  All interrupts have a hardware hard-coded
flow type (edge, level) with the exception of what we are calling "external
interrupts".  These are essentially gpio interrupts that can be software
configured as edge rising or edge falling.  Otherwise, there is no difference
between any of the interrupts.  Does setting edge parity for these interrupts
really warrant a new irq domain and irq_chip?

> Thanks,
> 
> 	tglx
> 

Josh

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

#1305526 — Re: [PATCH v3 02/14] irqchip: irq-pic32-evic: Add support for PIC32 interrupt controller

Joshua,

On Fri, 8 Jan 2016, Joshua Henderson wrote:
> On 01/08/2016 12:04 PM, Thomas Gleixner wrote:
> > On Thu, 7 Jan 2016, Joshua Henderson wrote:
> >> +/* acknowledge an interrupt */
> >> +static void ack_pic32_irq(struct irq_data *irqd)
> >> +{
> >> +	u32 reg, mask;
> >> +	unsigned int hwirq = irqd_to_hwirq(irqd);
> >> +
> >> +	BIT_REG_MASK(hwirq, reg, mask);
> >> +	writel(mask, &evic_base->ifs[reg].clr);
> > 
> > So you invented an open coded variant of the generic irq chip. Just with the
> > difference that the generic chip caches the mask and the register offsets ....
> > 
> 
> On PIC32 we have 4 different register offsets in many cases, including the interrupt
> controller registers, to write to one hardware register.  The PIC32 has special
> write only registers for set/clear/invert and which one is used is dependent on
> the logic at the time of writel(). Point being, there is no obvious value in
> caching when using these registers.  We don't have to perform a readl() at any
> time beforehand to write a mask to a register to update it atomically.

The generic chip has functions which handle the seperate register case.

void irq_gc_mask_disable_reg(struct irq_data *d);
void irq_gc_unmask_enable_reg(struct irq_data *d);
void irq_gc_mask_disable_reg_and_ack(struct irq_data *d);
void irq_gc_eoi(struct irq_data *d);

> >> +static int set_type_pic32_irq(struct irq_data *data, unsigned int flow_type)
> >> +{
> >> +	int index;
> >> +
> >> +	switch (flow_type) {
> >> +
> >> +	case IRQ_TYPE_EDGE_RISING:
> >> +	case IRQ_TYPE_EDGE_FALLING:
> >> +		irq_set_handler_locked(data, handle_edge_irq);
> >> +		break;
> >> +
> >> +	case IRQ_TYPE_LEVEL_HIGH:
> >> +	case IRQ_TYPE_LEVEL_LOW:
> >> +		irq_set_handler_locked(data, handle_fasteoi_irq);
> >> +		break;
> >> +
> >> +	default:
> >> +		pr_err("Invalid interrupt type !\n");
> >> +		return -EINVAL;
> >> +	}
> >> +
> >> +	/* set polarity for external interrupts only */
> >> +	index = get_ext_irq_index(data->hwirq);
> >> +	if (index >= 0)
> >> +		evic_set_ext_irq_polarity(index, flow_type);
> > 
> > So for the non external interrupts you set a different handler and be
> > done. How is that supposed to work? They switch magically from one mode to the
> > other?
> > 
> 
> It's all the same handlers (depending on whether it's persistent or
> non-persistent) irrelevant of it being an external interrupt or not.  It's all
> the same hardware interrupt controller.  Some pins on the chip can be configured
> as an interrupt source through pin configuration and those have dedicated
> interrupts associated with them.  The only thing "special" about these external
> interrupts is they must be explicitly configured as edge rising or edge falling
> in hardware- which is what is being handled here.  Non-external interrupts don't
> need this configuration.

I really cannot follow here. The code tells me that I can set
EDGE_RISING/FALLING/LEVEL_HIGH/LOW for any of those interrupts.

So that makes two questions:

   1) Can the non-external mode handle all type variants automagically? I
      seriously doubt that. If the type cannot be set, then it makes no sense
      to pretend that it can and allow to switch the handler from fasteoi to
      edge mode.

   2) The external irqs do not support level according to your
      evic_set_ext_irq_polarity() function. But you return success if set_type
      is called with a level type and gladly switch the handler. You merily
      pr_warn in evic_set_ext_irq_polarity().

That's just crap.
 
> >> +int pic32_get_c0_compare_int(void)
> >> +{
> >> +	int virq;
> >> +
> >> +	virq = irq_create_mapping(evic_irq_domain, CORE_TIMER_INTERRUPT);
> >> +	irq_set_irq_type(virq, IRQ_TYPE_EDGE_RISING);
> >> +	return virq;
> > 
> > Why isn't that information retrieved via device tree?
> > 
> 
> I suppose it could be.  We took a lesson from irq-mips-gic.c on this one.

You copied it, right? That does not make it any better.
 
> > Why don't you use a seperate irq chip for the ext irqs? You can do that with
> > the generic chip as well.
> > 
> >     irq_alloc_domain_generic_chips(domain, 32, 2, ....)
> > 
> > And then you assign the alternate chip (2) to your ext irqs and have the set
> > type function only for the alternate chip.
> > 
> 
> We have one interrupt controller.  All interrupts have a hardware hard-coded
> flow type (edge, level) ...

And that's exactly the point. They are hardcoded, but you still allow any
random driver to change the type and therefor the handler. How is that
supposed to work?

> ...  with the exception of what we are calling "external interrupts".
> These are essentially gpio interrupts that can be software
> configured as edge rising or edge falling.  Otherwise, there is no difference
> between any of the interrupts.

And again. Here you can change the type, but only edge rising and falling are
supported. And you still allow setting a level type.

So for both types you allow the driver/DT writer to get it wrong. And of
course this happens without a single line of comment which explains the
oddities of your driver, so a causual reader will stumble over it and ask
exactly the questions I'm asking. We want understandable and maintainable code
and not some 'work for me' hackery.

Thanks,

	tglx

	

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

#1307766 — Re: [PATCH v3 02/14] irqchip: irq-pic32-evic: Add support for PIC32 interrupt controller

Thomas,

On 01/10/2016 03:09 AM, Thomas Gleixner wrote:
> Joshua,
> 
> On Fri, 8 Jan 2016, Joshua Henderson wrote:
>> On 01/08/2016 12:04 PM, Thomas Gleixner wrote:
>>> On Thu, 7 Jan 2016, Joshua Henderson wrote:
>>>> +/* acknowledge an interrupt */
>>>> +static void ack_pic32_irq(struct irq_data *irqd)
>>>> +{
>>>> +	u32 reg, mask;
>>>> +	unsigned int hwirq = irqd_to_hwirq(irqd);
>>>> +
>>>> +	BIT_REG_MASK(hwirq, reg, mask);
>>>> +	writel(mask, &evic_base->ifs[reg].clr);
>>>
>>> So you invented an open coded variant of the generic irq chip. Just with the
>>> difference that the generic chip caches the mask and the register offsets ....
>>>
>>
>> On PIC32 we have 4 different register offsets in many cases, including the interrupt
>> controller registers, to write to one hardware register.  The PIC32 has special
>> write only registers for set/clear/invert and which one is used is dependent on
>> the logic at the time of writel(). Point being, there is no obvious value in
>> caching when using these registers.  We don't have to perform a readl() at any
>> time beforehand to write a mask to a register to update it atomically.
> 
> The generic chip has functions which handle the seperate register case.
> 
> void irq_gc_mask_disable_reg(struct irq_data *d);
> void irq_gc_unmask_enable_reg(struct irq_data *d);
> void irq_gc_mask_disable_reg_and_ack(struct irq_data *d);
> void irq_gc_eoi(struct irq_data *d);
> 

This makes sense now.  Using these is a natural result of moving to using generic chip as suggested below.

>>>> +static int set_type_pic32_irq(struct irq_data *data, unsigned int flow_type)
>>>> +{
>>>> +	int index;
>>>> +
>>>> +	switch (flow_type) {
>>>> +
>>>> +	case IRQ_TYPE_EDGE_RISING:
>>>> +	case IRQ_TYPE_EDGE_FALLING:
>>>> +		irq_set_handler_locked(data, handle_edge_irq);
>>>> +		break;
>>>> +
>>>> +	case IRQ_TYPE_LEVEL_HIGH:
>>>> +	case IRQ_TYPE_LEVEL_LOW:
>>>> +		irq_set_handler_locked(data, handle_fasteoi_irq);
>>>> +		break;
>>>> +
>>>> +	default:
>>>> +		pr_err("Invalid interrupt type !\n");
>>>> +		return -EINVAL;
>>>> +	}
>>>> +
>>>> +	/* set polarity for external interrupts only */
>>>> +	index = get_ext_irq_index(data->hwirq);
>>>> +	if (index >= 0)
>>>> +		evic_set_ext_irq_polarity(index, flow_type);
>>>
>>> So for the non external interrupts you set a different handler and be
>>> done. How is that supposed to work? They switch magically from one mode to the
>>> other?
>>>
>>
>> It's all the same handlers (depending on whether it's persistent or
>> non-persistent) irrelevant of it being an external interrupt or not.  It's all
>> the same hardware interrupt controller.  Some pins on the chip can be configured
>> as an interrupt source through pin configuration and those have dedicated
>> interrupts associated with them.  The only thing "special" about these external
>> interrupts is they must be explicitly configured as edge rising or edge falling
>> in hardware- which is what is being handled here.  Non-external interrupts don't
>> need this configuration.
> 
> I really cannot follow here. The code tells me that I can set
> EDGE_RISING/FALLING/LEVEL_HIGH/LOW for any of those interrupts.
> 
> So that makes two questions:
> 
>    1) Can the non-external mode handle all type variants automagically? I
>       seriously doubt that. If the type cannot be set, then it makes no sense
>       to pretend that it can and allow to switch the handler from fasteoi to
>       edge mode.
> 
>    2) The external irqs do not support level according to your
>       evic_set_ext_irq_polarity() function. But you return success if set_type
>       is called with a level type and gladly switch the handler. You merily
>       pr_warn in evic_set_ext_irq_polarity().
> 
> That's just crap.
> 

Got it.  The plan is for .irq_set_type to only exist for edge interrupts, and properly fail if it is not an external interrupt (meaning, there is no actual hardware change otherwise).  External interrupts will configured through a custom DT property in the evic node.  So, this means all non external interrupts will be "locked" to the DT specified irq type at mapping.  This should cleanly address these two issues.  Comments will be added to explain this.
 
>>>> +int pic32_get_c0_compare_int(void)
>>>> +{
>>>> +	int virq;
>>>> +
>>>> +	virq = irq_create_mapping(evic_irq_domain, CORE_TIMER_INTERRUPT);
>>>> +	irq_set_irq_type(virq, IRQ_TYPE_EDGE_RISING);
>>>> +	return virq;
>>>
>>> Why isn't that information retrieved via device tree?
>>>
>>
>> I suppose it could be.  We took a lesson from irq-mips-gic.c on this one.
> 
> You copied it, right? That does not make it any better.
>  

Typically, this CPU core interrupt number can be read from the CP0 registers.  However, the interrupt controller on PIC32 is the interface/arbiter for all interrupts, peripheral and CPU, and therefore we have to specify it.  Because this is not a unique scenario for probably various reasons, the arch/mips/kernel/cevt-r4k.c provides get_c0_compare_int() as a __weak symbol which is currently overwritten by ~11 MIPS platforms in the exact same non-DTS way.  This is what I meant by saying PIC32 implemented this like irq-mips-gic.c.  So, I see 2 options to address this:

1) Move pic32_get_c0_compare_int() to platform code as get_c0_compare_int().  Load a fixed mapping from DT in the irqchip driver and find that mapping with get_c0_compare_int().
2) Add DT support to arch cevt-r4k.c or cpu_probe.c read an arch common property from DT to get the core timer interrupt number.

I think 1) satisfies your feedback on this.  Does this make sense?

>>> Why don't you use a seperate irq chip for the ext irqs? You can do that with
>>> the generic chip as well.
>>>
>>>     irq_alloc_domain_generic_chips(domain, 32, 2, ....)
>>>
>>> And then you assign the alternate chip (2) to your ext irqs and have the set
>>> type function only for the alternate chip.
>>>
>>
>> We have one interrupt controller.  All interrupts have a hardware hard-coded
>> flow type (edge, level) ...
> 
> And that's exactly the point. They are hardcoded, but you still allow any
> random driver to change the type and therefor the handler. How is that
> supposed to work?
> 

To clarify, by hardcoded, I mean the hardware peripheral the interrupt controller is arbitrating for has a defined irq type.  We specify this in a DT standard way for each peripheral, seeing that it varies by peripheral and there is no rhyme or reason to it in the linear IRQ mapping for > 200 interrupts at the evic level:

	uart1: serial@1f822000 {
                compatible = "microchip,pic32mzda-uart";
		...
                interrupts = <112 IRQ_TYPE_LEVEL_HIGH>,
                        <113 IRQ_TYPE_LEVEL_HIGH>,
                        <114 IRQ_TYPE_LEVEL_HIGH>;
		...
        };

	PBTIMER1:pbtimer1 {
		compatible = "microchip,pic32mzda-timerA";
		...
                interrupts = <4 IRQ_TYPE_EDGE_RISING>;
		...
        };

The shortcoming of allowing a change to the flow handler will be addressed.  It does make sense as you are suggesting to use different irq chips for the different flow handlers.  irq_alloc_domain_generic_chips() will work as suggested and there will be alternate chips for the different scenarios.

>> ...  with the exception of what we are calling "external interrupts".
>> These are essentially gpio interrupts that can be software
>> configured as edge rising or edge falling.  Otherwise, there is no difference
>> between any of the interrupts.
> 
> And again. Here you can change the type, but only edge rising and falling are
> supported. And you still allow setting a level type.
> 

This will be addressed.

> So for both types you allow the driver/DT writer to get it wrong. And of
> course this happens without a single line of comment which explains the
> oddities of your driver, so a causual reader will stumble over it and ask
> exactly the questions I'm asking. We want understandable and maintainable code
> and not some 'work for me' hackery.

Make no mistake, I'm here to do this the right way.  A new irqchip driver is in the oven.

Thanks,
Josh

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web