linuxsh-dev

sh: intc - rework core code

[Magnus D. <mag...@gm...>]

sh: intc - rework core code

This patch reworks the intc core, implementing the following features:
- Support dual priority registers - one set and one clear register
- All 8/16/32 bit register combinations are now supported
- Both single mask and single enable bitmap register are supported
- Add code to set interrupt priority
- Speedup sense and priority configuration code
- Allocate data using bootmem, allows intc data structures to be __initdata
- Save memory - allocated memory footprint is smaller than intc structures

Signed-off-by: Magnus Damm <da...@ig...>
---

 Boot tested on r2d, r7780, r7785 and x3.
 More serious testing performed on r2d with fpga and voyager interrupts.

 arch/sh/cchips/voyagergx/irq.c |    2 
 arch/sh/kernel/cpu/irq/intc.c  |  560 ++++++++++++++++++++++++----------------
 include/asm-sh/hw_irq.h        |    4 
 3 files changed, 343 insertions(+), 223 deletions(-)

--- 0001/arch/sh/cchips/voyagergx/irq.c
+++ work/arch/sh/cchips/voyagergx/irq.c	2007-08-09 19:42:23.000000000 +0900
@@ -50,7 +50,7 @@ static struct intc_vect vectors[] = {
 };
 
 static struct intc_mask_reg mask_registers[] = {
-	{ VOYAGER_INT_MASK, 1, 32, /* "Interrupt Mask", MMIO_base + 0x30 */
+	{ VOYAGER_INT_MASK, 0, 32, /* "Interrupt Mask", MMIO_base + 0x30 */
 	  { UP, G54, G53, G52, G51, G50, G49, G48,
 	    I2C, PW, 0, DMA, PCI, I2S, AC, US,
 	    0, 0, U1, U0, CV, MC, S1, S0,
--- 0003/arch/sh/kernel/cpu/irq/intc.c
+++ work/arch/sh/kernel/cpu/irq/intc.c	2007-08-09 19:42:23.000000000 +0900
@@ -20,176 +20,228 @@
 #include <linux/module.h>
 #include <linux/io.h>
 #include <linux/interrupt.h>
+#include <linux/bootmem.h>
 
-#define _INTC_MK(fn, idx, bit, value) \
-	((fn) << 24 | ((value) << 16) | ((idx) << 8) | (bit))
-#define _INTC_FN(h) (h >> 24)
-#define _INTC_VALUE(h) ((h >> 16) & 0xff)
-#define _INTC_IDX(h) ((h >> 8) & 0xff)
-#define _INTC_BIT(h) (h & 0xff)
+#define _INTC_MK(fn, mode, addr_e, addr_d, width, shift) \
+	((shift) | ((width) << 5) | ((fn) << 9) | ((mode) << 13) | \
+         ((addr_e) << 16) | ((addr_d << 24)))
+
+#define _INTC_SHIFT(h) (h & 0x1f)
+#define _INTC_WIDTH(h) ((h >> 5) & 0xf)
+#define _INTC_FN(h) ((h >> 9) & 0xf)
+#define _INTC_MODE(h) ((h >> 13) & 0x7)
+#define _INTC_ADDR_E(h) ((h >> 16) & 0xff)
+#define _INTC_ADDR_D(h) ((h >> 24) & 0xff)
+
+struct intc_handle_int {
+	unsigned int irq;
+	unsigned long handle;
+};
+
+struct intc_desc_int {
+	unsigned long *reg;
+	unsigned int nr_reg;
+	struct intc_handle_int *prio;
+	unsigned int nr_prio;
+	struct intc_handle_int *sense;
+	unsigned int nr_sense;
+	struct irq_chip chip;
+};
 
-#define _INTC_PTR(desc, member, data) \
-	(desc->member + _INTC_IDX(data))
+static unsigned int intc_prio_level[NR_IRQS]; /* for now */
 
-static inline struct intc_desc *get_intc_desc(unsigned int irq)
+static inline struct intc_desc_int *get_intc_desc(unsigned int irq)
 {
 	struct irq_chip *chip = get_irq_chip(irq);
-	return (void *)((char *)chip - offsetof(struct intc_desc, chip));
+	return (void *)((char *)chip - offsetof(struct intc_desc_int, chip));
 }
 
 static inline unsigned int set_field(unsigned int value,
 				     unsigned int field_value,
-				     unsigned int width,
-				     unsigned int shift)
+				     unsigned int handle)
 {
+	unsigned int width = _INTC_WIDTH(handle);
+	unsigned int shift = _INTC_SHIFT(handle);
+
 	value &= ~(((1 << width) - 1) << shift);
 	value |= field_value << shift;
 	return value;
 }
 
-static inline unsigned int set_prio_field(struct intc_desc *desc,
-					  unsigned int value,
-					  unsigned int priority,
-					  unsigned int data)
+static void write_8(unsigned long addr, unsigned long h, unsigned long data)
 {
-	unsigned int width = _INTC_PTR(desc, prio_regs, data)->field_width;
-
-	return set_field(value, priority, width, _INTC_BIT(data));
+	ctrl_outb(set_field(0, data, h), addr);
 }
 
-static void disable_prio_16(struct intc_desc *desc, unsigned int data)
+static void write_16(unsigned long addr, unsigned long h, unsigned long data)
 {
-	unsigned long addr = _INTC_PTR(desc, prio_regs, data)->set_reg;
-
-	ctrl_outw(set_prio_field(desc, ctrl_inw(addr), 0, data), addr);
+	ctrl_outw(set_field(0, data, h), addr);
 }
 
-static void enable_prio_16(struct intc_desc *desc, unsigned int data)
+static void write_32(unsigned long addr, unsigned long h, unsigned long data)
 {
-	unsigned long addr = _INTC_PTR(desc, prio_regs, data)->set_reg;
-	unsigned int prio = _INTC_VALUE(data);
-
-	ctrl_outw(set_prio_field(desc, ctrl_inw(addr), prio, data), addr);
+	ctrl_outl(set_field(0, data, h), addr);
 }
 
-static void disable_prio_32(struct intc_desc *desc, unsigned int data)
+static void modify_8(unsigned long addr, unsigned long h, unsigned long data)
 {
-	unsigned long addr = _INTC_PTR(desc, prio_regs, data)->set_reg;
-
-	ctrl_outl(set_prio_field(desc, ctrl_inl(addr), 0, data), addr);
+	ctrl_outb(set_field(ctrl_inb(addr), data, h), addr);
 }
 
-static void enable_prio_32(struct intc_desc *desc, unsigned int data)
+static void modify_16(unsigned long addr, unsigned long h, unsigned long data)
 {
-	unsigned long addr = _INTC_PTR(desc, prio_regs, data)->set_reg;
-	unsigned int prio = _INTC_VALUE(data);
-
-	ctrl_outl(set_prio_field(desc, ctrl_inl(addr), prio, data), addr);
+	ctrl_outw(set_field(ctrl_inw(addr), data, h), addr);
 }
 
-static void write_set_reg_8(struct intc_desc *desc, unsigned int data)
+static void modify_32(unsigned long addr, unsigned long h, unsigned long data)
 {
-	ctrl_outb(1 << _INTC_BIT(data),
-		  _INTC_PTR(desc, mask_regs, data)->set_reg);
+	ctrl_outl(set_field(ctrl_inl(addr), data, h), addr);
 }
 
-static void write_clr_reg_8(struct intc_desc *desc, unsigned int data)
-{
-	ctrl_outb(1 << _INTC_BIT(data),
-		  _INTC_PTR(desc, mask_regs, data)->clr_reg);
-}
+enum {	REG_FN_ERR = 0, REG_FN_WRITE_BASE = 1, REG_FN_MODIFY_BASE = 5 };
 
-static void write_set_reg_32(struct intc_desc *desc, unsigned int data)
-{
-	ctrl_outl(1 << _INTC_BIT(data),
-		  _INTC_PTR(desc, mask_regs, data)->set_reg);
-}
+static void (*intc_reg_fns[])(unsigned long addr,
+			      unsigned long h,
+			      unsigned long data) = {
+	[REG_FN_WRITE_BASE + 0] = write_8,
+	[REG_FN_WRITE_BASE + 1] = write_16,
+	[REG_FN_WRITE_BASE + 3] = write_32,
+	[REG_FN_MODIFY_BASE + 0] = modify_8,
+	[REG_FN_MODIFY_BASE + 1] = modify_16,
+	[REG_FN_MODIFY_BASE + 3] = modify_32,
+};
 
-static void write_clr_reg_32(struct intc_desc *desc, unsigned int data)
-{
-	ctrl_outl(1 << _INTC_BIT(data),
-		  _INTC_PTR(desc, mask_regs, data)->clr_reg);
-}
+enum {	MODE_ENABLE_REG = 0, /* Bit(s) set -> interrupt enabled */
+	MODE_MASK_REG,       /* Bit(s) set -> interrupt disabled */
+	MODE_DUAL_REG,       /* Two registers, set bit to enable / disable */
+	MODE_PRIO_REG,       /* Priority value written to enable interrupt */
+	MODE_PCLR_REG,       /* Above plus all bits set to disable interrupt */
+};
 
-static void or_set_reg_16(struct intc_desc *desc, unsigned int data)
-{
-	unsigned long addr = _INTC_PTR(desc, mask_regs, data)->set_reg;
+static void intc_mode_field(unsigned long addr,
+			    unsigned long handle,
+			    void (*fn)(unsigned long,
+				       unsigned long,
+				       unsigned long),
+			    unsigned int irq)
+{
+	fn(addr, handle, ((1 << _INTC_WIDTH(handle)) - 1));
+}
+
+static void intc_mode_zero(unsigned long addr,
+			   unsigned long handle,
+			   void (*fn)(unsigned long,
+				       unsigned long,
+				       unsigned long),
+			   unsigned int irq)
+{
+	fn(addr, handle, 0);
+}
+
+static void intc_mode_prio(unsigned long addr,
+			   unsigned long handle,
+			   void (*fn)(unsigned long,
+				       unsigned long,
+				       unsigned long),
+			   unsigned int irq)
+{
+	fn(addr, handle, intc_prio_level[irq]);
+}
+
+static void (*intc_enable_fns[])(unsigned long addr, 
+				 unsigned long handle,
+				 void (*fn)(unsigned long,
+					    unsigned long,
+					    unsigned long),
+				 unsigned int irq) = {
+	[MODE_ENABLE_REG] = intc_mode_field,
+	[MODE_MASK_REG] = intc_mode_zero,
+	[MODE_DUAL_REG] = intc_mode_field,
+	[MODE_PRIO_REG] = intc_mode_prio,
+	[MODE_PCLR_REG] = intc_mode_prio,
+};
 
-	ctrl_outw(ctrl_inw(addr) | 1 << _INTC_BIT(data), addr);
-}
+static void (*intc_disable_fns[])(unsigned long addr,
+				  unsigned long handle,
+				  void (*fn)(unsigned long,
+					     unsigned long,
+					     unsigned long),
+				  unsigned int irq) = {
+	[MODE_ENABLE_REG] = intc_mode_zero,
+	[MODE_MASK_REG] = intc_mode_field,
+	[MODE_DUAL_REG] = intc_mode_field,
+	[MODE_PRIO_REG] = intc_mode_zero,
+	[MODE_PCLR_REG] = intc_mode_field,
+};
 
-static void and_set_reg_16(struct intc_desc *desc, unsigned int data)
+static inline void _intc_enable(unsigned int irq, unsigned long handle)
 {
-	unsigned long addr = _INTC_PTR(desc, mask_regs, data)->set_reg;
+	struct intc_desc_int *d = get_intc_desc(irq);
+	unsigned long addr = d->reg[_INTC_ADDR_E(handle)];
 
-	ctrl_outw(ctrl_inw(addr) & ~(1 << _INTC_BIT(data)), addr);
+	intc_enable_fns[_INTC_MODE(handle)](addr, handle,
+					    intc_reg_fns[_INTC_FN(handle)],
+					    irq);
 }
+			    
 
-static void or_set_reg_32(struct intc_desc *desc, unsigned int data)
+static void intc_enable(unsigned int irq)
 {
-	unsigned long addr = _INTC_PTR(desc, mask_regs, data)->set_reg;
-
-	ctrl_outl(ctrl_inl(addr) | 1 << _INTC_BIT(data), addr);
+	_intc_enable(irq, (unsigned long)get_irq_chip_data(irq));
 }
 
-static void and_set_reg_32(struct intc_desc *desc, unsigned int data)
+static void intc_disable(unsigned int irq)
 {
-	unsigned long addr = _INTC_PTR(desc, mask_regs, data)->set_reg;
-
-	ctrl_outl(ctrl_inl(addr) & ~(1 << _INTC_BIT(data)), addr);
-}
-
-enum {	REG_FN_ERROR=0,
-	REG_FN_DUAL_8, REG_FN_DUAL_32,
-	REG_FN_ENA_16, REG_FN_ENA_32,
-	REG_FN_PRIO_16, REG_FN_PRIO_32 };
-
-static struct {
-	void (*enable)(struct intc_desc *, unsigned int);
-	void (*disable)(struct intc_desc *, unsigned int);
-} intc_reg_fns[] = {
-	[REG_FN_DUAL_8] = { write_clr_reg_8, write_set_reg_8 },
-	[REG_FN_DUAL_32] = { write_clr_reg_32, write_set_reg_32 },
-	[REG_FN_ENA_16] = { or_set_reg_16, and_set_reg_16 },
-	[REG_FN_ENA_32] = { or_set_reg_32, and_set_reg_32 },
-	[REG_FN_PRIO_16] = { enable_prio_16, disable_prio_16 },
-	[REG_FN_PRIO_32] = { enable_prio_32, disable_prio_32 },
-};
-
-static void intc_enable(unsigned int irq)
+	struct intc_desc_int *desc = get_intc_desc(irq);
+	unsigned long handle = (unsigned long) get_irq_chip_data(irq);
+	unsigned long addr = desc->reg[_INTC_ADDR_D(handle)];
+	
+	intc_disable_fns[_INTC_MODE(handle)](addr, handle,
+					     intc_reg_fns[_INTC_FN(handle)],
+					     irq);
+}
+
+static struct intc_handle_int *intc_find_irq(struct intc_handle_int *hp,
+					     unsigned int nr_hp,
+					     unsigned int irq)
 {
-	struct intc_desc *desc = get_intc_desc(irq);
-	unsigned int data = (unsigned int) get_irq_chip_data(irq);
+	int i;
 
-	intc_reg_fns[_INTC_FN(data)].enable(desc, data);
-}
+	for (i = 0; i < nr_hp; i++) {
+		if ((hp + i)->irq != irq)
+			continue;
 
-static void intc_disable(unsigned int irq)
-{
-	struct intc_desc *desc = get_intc_desc(irq);
-	unsigned int data = (unsigned int) get_irq_chip_data(irq);
+		return hp + i;
+	}
 
-	intc_reg_fns[_INTC_FN(data)].disable(desc, data);
+	return NULL;
 }
 
-static void set_sense_16(struct intc_desc *desc, unsigned int data)
+int intc_set_priority(unsigned int irq, unsigned int prio)
 {
-	unsigned long addr = _INTC_PTR(desc, sense_regs, data)->reg;
-	unsigned int width = _INTC_PTR(desc, sense_regs, data)->field_width;
-	unsigned int bit = _INTC_BIT(data);
-	unsigned int value = _INTC_VALUE(data);
+	struct intc_desc_int *d = get_intc_desc(irq);
+	struct intc_handle_int *ihp;
 
-	ctrl_outw(set_field(ctrl_inw(addr), value, width, bit), addr);
-}
+	if (!intc_prio_level[irq] || prio <= 1)
+		return -EINVAL;
 
-static void set_sense_32(struct intc_desc *desc, unsigned int data)
-{
-	unsigned long addr = _INTC_PTR(desc, sense_regs, data)->reg;
-	unsigned int width = _INTC_PTR(desc, sense_regs, data)->field_width;
-	unsigned int bit = _INTC_BIT(data);
-	unsigned int value = _INTC_VALUE(data);
+	ihp = intc_find_irq(d->prio, d->nr_prio, irq);
+	if (ihp) {
+		if (prio >= ((1 << _INTC_WIDTH(ihp->handle)) - 1))
+			return -EINVAL;
+
+		intc_prio_level[irq] = prio;
+
+		/*
+		 * only set secondary masking method directly
+		 * primary masking method is using intc_prio_level[irq]
+		 * priority level will be set during next enable()
+		 */
 
-	ctrl_outl(set_field(ctrl_inl(addr), value, width, bit), addr);
+		if (ihp->handle)
+			_intc_enable(irq, ihp->handle);
+	}
+	return 0;
 }
 
 #define VALID(x) (x | 0x80)
@@ -203,92 +255,38 @@ static unsigned char intc_irq_sense_tabl
 
 static int intc_set_sense(unsigned int irq, unsigned int type)
 {
-	struct intc_desc *desc = get_intc_desc(irq);
+	struct intc_desc_int *d = get_intc_desc(irq);
 	unsigned char value = intc_irq_sense_table[type & IRQ_TYPE_SENSE_MASK];
-	unsigned int i, j, data, bit;
-	intc_enum enum_id = 0;
-
-	for (i = 0; i < desc->nr_vectors; i++) {
-		struct intc_vect *vect = desc->vectors + i;
-
-		if (evt2irq(vect->vect) != irq)
-			continue;
+	struct intc_handle_int *ihp;
+	unsigned long addr;
 
-		enum_id = vect->enum_id;
-		break;
-	}
-
-	if (!enum_id || !value || !desc->sense_regs)
+	if (!value)
 		return -EINVAL;
 
-	value ^= VALID(0);
-
-	for (i = 0; i < desc->nr_sense_regs; i++) {
-		struct intc_sense_reg *sr = desc->sense_regs + i;
-
-		for (j = 0; j < ARRAY_SIZE(sr->enum_ids); j++) {
-			if (sr->enum_ids[j] != enum_id)
-				continue;
-
-			bit = sr->reg_width - ((j + 1) * sr->field_width);
-			data = _INTC_MK(0, i, bit, value);
-
-			switch(sr->reg_width) {
-			case 16:
-				set_sense_16(desc, data);
-				break;
-			case 32:
-				set_sense_32(desc, data);
-				break;
-			}
-
-			return 0;
-		}
-	}
-
-	return -EINVAL;
-}
-
-static unsigned int __init intc_find_dual_handler(unsigned int width)
-{
-	switch (width) {
-	case 8:
-		return REG_FN_DUAL_8;
-	case 32:
-		return REG_FN_DUAL_32;
+	ihp = intc_find_irq(d->sense, d->nr_sense, irq);
+	if (ihp) {
+		addr = d->reg[_INTC_ADDR_E(ihp->handle)];
+		intc_reg_fns[_INTC_FN(ihp->handle)](addr, ihp->handle, value);
 	}
-
-	BUG();
-	return REG_FN_ERROR;
+	return 0;
 }
 
-static unsigned int __init intc_find_prio_handler(unsigned int width)
+static unsigned int __init intc_get_reg(struct intc_desc_int *d,
+				 unsigned long address)
 {
-	switch (width) {
-	case 16:
-		return REG_FN_PRIO_16;
-	case 32:
-		return REG_FN_PRIO_32;
-	}
-
-	BUG();
-	return REG_FN_ERROR;
-}
+	unsigned int k;
 
-static unsigned int __init intc_find_ena_handler(unsigned int width)
-{
-	switch (width) {
-	case 16:
-		return REG_FN_ENA_16;
-	case 32:
-		return REG_FN_ENA_32;
+	for (k = 0; k < d->nr_reg; k++) {
+		if (d->reg[k] == address)
+			return k;
 	}
 
 	BUG();
-	return REG_FN_ERROR;
+	return 0;
 }
 
-static intc_enum __init intc_grp_id(struct intc_desc *desc, intc_enum enum_id)
+static intc_enum __init intc_grp_id(struct intc_desc *desc,
+				    intc_enum enum_id)
 {
 	struct intc_group *g = desc->groups;
 	unsigned int i, j;
@@ -333,10 +331,12 @@ static unsigned int __init intc_prio_val
 }
 
 static unsigned int __init intc_mask_data(struct intc_desc *desc,
+					  struct intc_desc_int *d,
 					  intc_enum enum_id, int do_grps)
 {
 	struct intc_mask_reg *mr = desc->mask_regs;
-	unsigned int i, j, fn;
+	unsigned int i, j, fn, mode;
+	unsigned long reg_e, reg_d;
 
 	for (i = 0; mr && enum_id && i < desc->nr_mask_regs; i++) {
 		mr = desc->mask_regs + i;
@@ -345,32 +345,48 @@ static unsigned int __init intc_mask_dat
 			if (mr->enum_ids[j] != enum_id)
 				continue;
 
-			switch (mr->clr_reg) {
-			case 1: /* 1 = enabled interrupt - "enable" register */
-				fn = intc_find_ena_handler(mr->reg_width);
-				break;
-			default:
-				fn = intc_find_dual_handler(mr->reg_width);
+			if (mr->set_reg && mr->clr_reg) {
+				fn = REG_FN_WRITE_BASE;
+				mode = MODE_DUAL_REG;
+				reg_e = mr->clr_reg;
+				reg_d = mr->set_reg;
+			}
+			else {
+				fn = REG_FN_MODIFY_BASE;
+				if (mr->set_reg) {
+					mode = MODE_ENABLE_REG;
+					reg_e = mr->set_reg;
+					reg_d = mr->set_reg;
+				}
+				else {
+					mode = MODE_MASK_REG;
+					reg_e = mr->clr_reg;
+					reg_d = mr->clr_reg;
+				}
 			}
 
-			if (fn == REG_FN_ERROR)
-				return 0;
-
-			return _INTC_MK(fn, i, (mr->reg_width - 1) - j, 0);
+			fn += (mr->reg_width >> 3) - 1;
+			return _INTC_MK(fn, mode,
+					intc_get_reg(d, reg_e),
+					intc_get_reg(d, reg_d),
+					1,
+					(mr->reg_width - 1) - j);
 		}
 	}
 
 	if (do_grps)
-		return intc_mask_data(desc, intc_grp_id(desc, enum_id), 0);
+		return intc_mask_data(desc, d, intc_grp_id(desc, enum_id), 0);
 
 	return 0;
 }
 
 static unsigned int __init intc_prio_data(struct intc_desc *desc,
+					  struct intc_desc_int *d,
 					  intc_enum enum_id, int do_grps)
 {
 	struct intc_prio_reg *pr = desc->prio_regs;
-	unsigned int i, j, fn, bit, prio;
+	unsigned int i, j, fn, mode, bit;
+	unsigned long reg_e, reg_d;
 
 	for (i = 0; pr && enum_id && i < desc->nr_prio_regs; i++) {
 		pr = desc->prio_regs + i;
@@ -379,26 +395,70 @@ static unsigned int __init intc_prio_dat
 			if (pr->enum_ids[j] != enum_id)
 				continue;
 
-			fn = intc_find_prio_handler(pr->reg_width);
-			if (fn == REG_FN_ERROR)
-				return 0;
+			if (pr->set_reg && pr->clr_reg) {
+				fn = REG_FN_WRITE_BASE;
+				mode = MODE_PCLR_REG;
+				reg_e = pr->set_reg;
+				reg_d = pr->clr_reg;
+			}
+			else {
+				fn = REG_FN_MODIFY_BASE;
+				mode = MODE_PRIO_REG;
+				if (!pr->set_reg)
+					BUG();
+				reg_e = pr->set_reg;
+				reg_d = pr->set_reg;
+			}
 
-			prio = intc_prio_value(desc, enum_id, 1);
+			fn += (pr->reg_width >> 3) - 1;
 			bit = pr->reg_width - ((j + 1) * pr->field_width);
 
 			BUG_ON(bit < 0);
 
-			return _INTC_MK(fn, i, bit, prio);
+			return _INTC_MK(fn, mode,
+					intc_get_reg(d, reg_e),
+					intc_get_reg(d, reg_d),
+					pr->field_width, bit);
 		}
 	}
 
 	if (do_grps)
-		return intc_prio_data(desc, intc_grp_id(desc, enum_id), 0);
+		return intc_prio_data(desc, d, intc_grp_id(desc, enum_id), 0);
+
+	return 0;
+}
+
+static unsigned int __init intc_sense_data(struct intc_desc *desc,
+					   struct intc_desc_int *d,
+					   intc_enum enum_id)
+{
+	struct intc_sense_reg *sr = desc->sense_regs;
+	unsigned int i, j, fn, bit;
+
+	for (i = 0; sr && enum_id && i < desc->nr_sense_regs; i++) {
+		sr = desc->sense_regs + i;
+
+		for (j = 0; j < ARRAY_SIZE(sr->enum_ids); j++) {
+			if (sr->enum_ids[j] != enum_id)
+				continue;
+
+			fn = REG_FN_MODIFY_BASE;
+			fn += (sr->reg_width >> 3) - 1;
+			bit = sr->reg_width - ((j + 1) * sr->field_width);
+
+			BUG_ON(bit < 0);
+
+			return _INTC_MK(fn, 0, intc_get_reg(d, sr->reg),
+					0, sr->field_width, bit);
+		}
+	}
 
 	return 0;
 }
 
-static void __init intc_register_irq(struct intc_desc *desc, intc_enum enum_id,
+static void __init intc_register_irq(struct intc_desc *desc,
+				     struct intc_desc_int *d,
+				     intc_enum enum_id,
 				     unsigned int irq)
 {
 	unsigned int data[2], primary;
@@ -410,15 +470,15 @@ static void __init intc_register_irq(str
 	 * 4. priority, multiple interrupt sources (groups)
 	 */
 
-	data[0] = intc_mask_data(desc, enum_id, 0);
-	data[1] = intc_prio_data(desc, enum_id, 0);
+	data[0] = intc_mask_data(desc, d, enum_id, 0);
+	data[1] = intc_prio_data(desc, d, enum_id, 0);
 
 	primary = 0;
 	if (!data[0] && data[1])
 		primary = 1;
 
-	data[0] = data[0] ? data[0] : intc_mask_data(desc, enum_id, 1);
-	data[1] = data[1] ? data[1] : intc_prio_data(desc, enum_id, 1);
+	data[0] = data[0] ? data[0] : intc_mask_data(desc, d, enum_id, 1);
+	data[1] = data[1] ? data[1] : intc_prio_data(desc, d, enum_id, 1);
 
 	if (!data[primary])
 		primary ^= 1;
@@ -426,31 +486,91 @@ static void __init intc_register_irq(str
 	BUG_ON(!data[primary]); /* must have primary masking method */
 
 	disable_irq_nosync(irq);
-	set_irq_chip_and_handler_name(irq, &desc->chip,
+	set_irq_chip_and_handler_name(irq, &d->chip,
 				      handle_level_irq, "level");
 	set_irq_chip_data(irq, (void *)data[primary]);
 
+	/* record the desired priority level */
+	intc_prio_level[irq] = intc_prio_value(desc, enum_id, 1);
+
 	/* enable secondary masking method if present */
 	if (data[!primary])
-		intc_reg_fns[_INTC_FN(data[!primary])].enable(desc,
-							      data[!primary]);
+		_intc_enable(irq, data[!primary]);
+
+	/* add irq to d->prio list if priority is available */
+	if (data[1]) {
+		(d->prio + d->nr_prio)->irq = irq;
+		if (!primary) /* only secondary priority can access regs */
+			(d->prio + d->nr_prio)->handle = data[1];
+		d->nr_prio++;
+	}
+
+	/* add irq to d->sense list if sense is available */
+	data[0] = intc_sense_data(desc, d, enum_id);
+	if (data[0]) {
+		(d->sense + d->nr_sense)->irq = irq;
+		(d->sense + d->nr_sense)->handle = data[0];
+		d->nr_sense++;
+	}
 
 	/* irq should be disabled by default */
-	desc->chip.mask(irq);
+	d->chip.mask(irq);
 }
 
 void __init register_intc_controller(struct intc_desc *desc)
 {
-	unsigned int i;
+	unsigned int i, k;
+	struct intc_desc_int *d;
+
+	d = alloc_bootmem(sizeof(*d));
+
+	d->nr_reg = desc->mask_regs ? desc->nr_mask_regs * 2 : 0;
+	d->nr_reg += desc->prio_regs ? desc->nr_prio_regs * 2 : 0;
+	d->nr_reg += desc->sense_regs ? desc->nr_sense_regs : 0;
+
+	d->reg = alloc_bootmem(d->nr_reg * sizeof(*d->reg));
+	k = 0;
+
+	if (desc->mask_regs) {
+		for (i = 0; i < desc->nr_mask_regs; i++) {
+			if (desc->mask_regs[i].set_reg)
+				d->reg[k++] = desc->mask_regs[i].set_reg;
+			if (desc->mask_regs[i].clr_reg)
+				d->reg[k++] = desc->mask_regs[i].clr_reg;
+		}
+	}
+
+	if (desc->prio_regs) {
+		d->prio = alloc_bootmem(desc->nr_vectors * sizeof(*d->prio));
+
+		for (i = 0; i < desc->nr_prio_regs; i++) {
+			if (desc->prio_regs[i].set_reg)
+				d->reg[k++] = desc->prio_regs[i].set_reg;
+			if (desc->prio_regs[i].clr_reg)
+				d->reg[k++] = desc->prio_regs[i].clr_reg;
+		}
+	}
+
+	if (desc->sense_regs) {
+		d->sense = alloc_bootmem(desc->nr_vectors * sizeof(*d->sense));
+
+		for (i = 0; i < desc->nr_sense_regs; i++) {
+			if (desc->sense_regs[i].reg)
+				d->reg[k++] = desc->sense_regs[i].reg;
+		}
+	}
+
+	BUG_ON(k > 256); /* _INTC_ADDR_E() and _INTC_ADDR_D() are 8 bits */
 
-	desc->chip.mask = intc_disable;
-	desc->chip.unmask = intc_enable;
-	desc->chip.mask_ack = intc_disable;
-	desc->chip.set_type = intc_set_sense;
+	d->chip.name = desc->name;
+	d->chip.mask = intc_disable;
+	d->chip.unmask = intc_enable;
+	d->chip.mask_ack = intc_disable;
+	d->chip.set_type = intc_set_sense;
 
 	for (i = 0; i < desc->nr_vectors; i++) {
 		struct intc_vect *vect = desc->vectors + i;
 
-		intc_register_irq(desc, vect->enum_id, evt2irq(vect->vect));
+		intc_register_irq(desc, d, vect->enum_id, evt2irq(vect->vect));
 	}
 }
--- 0003/include/asm-sh/hw_irq.h
+++ work/include/asm-sh/hw_irq.h	2007-08-09 19:42:23.000000000 +0900
@@ -75,7 +75,7 @@ struct intc_desc {
 	unsigned int nr_prio_regs;
 	struct intc_sense_reg *sense_regs;
 	unsigned int nr_sense_regs;
-	struct irq_chip chip;
+	char *name;
 };
 
 #define _INTC_ARRAY(a) a, sizeof(a)/sizeof(*a)
@@ -86,7 +86,7 @@ struct intc_desc symbol = {						\
 	_INTC_ARRAY(priorities),					\
 	_INTC_ARRAY(mask_regs), _INTC_ARRAY(prio_regs),			\
 	_INTC_ARRAY(sense_regs),					\
-	.chip.name = chipname,						\
+	chipname,							\
 }
 
 void __init register_intc_controller(struct intc_desc *desc);

Re: intc - rework core code

[EXTERNAL B. M. (P. ST-FIR/Eng) <ext...@de...>]

Hi Magnus,

thanks for the new priority feature.
There are 3 things I noticed.
First:=20
The declaration of the new function should probably go to hw_irq.h.

Second:
Is it forbidden to set the highest possible priority? If not there is a
bug in set_priority.

if (prio >=3D ((1 << _INTC_WIDTH(ihp->handle)) - 1))


Third:
The code doesn't work for processors without mask registers.

If the processor only has priority registers the "handle" will not be
set in intc_register_irq (because of "if(!primary)")
This leads to wrong results from _INTC_WIDTH, which causes
intc_set_priority fail with EINVAL

When I comment out the "if(!primary)" condition all priorities will be
set immediatelly, which also enables them.
When I hardcode the _INTC_WIDTH to the correct size in set_priority, the
priority will be set as soon as the irq is requested.

Regards

Markus

Re: intc - rework core code

[Magnus D. <mag...@gm...>]

Hi Markus!

On 8/14/07, EXTERNAL Brunner Markus (Praktikant; ST-FIR/Eng)
<ext...@de...> wrote:
> Hi Magnus,
>
> thanks for the new priority feature.

Hehe, you noticed. =)

> There are 3 things I noticed.
> First:
> The declaration of the new function should probably go to hw_irq.h.

I wanted to keep it semi-secret waiting to tie it in with some generic
code, but I think you are right that a prototype should be put in
hw_irq.h.

> Second:
> Is it forbidden to set the highest possible priority? If not there is a
> bug in set_priority.
>
> if (prio >= ((1 << _INTC_WIDTH(ihp->handle)) - 1))

You are right, we should have a ">" instead of ">=". Thanks.

> Third:
> The code doesn't work for processors without mask registers.

Is that so? I've tried the code on sh7750 among other cpus without any
problems. But I may of course have missed something...

Just to clarify (you probably know this already, but anyway):

The intc code requires at least one way to mask an interrupt. So intc
can for instance not be used in the cpu specific code for certain IRL
vectors on sh processors that don't provide any mask registers. For
sh7705 that means that the cpu specific code can use intc in IRQ mode
but not for IRL mode. So with IRL mode you need board specific
interrupt code that masks interrupts in some external register (FPGA
maybe) that handles the IRL pins. And that board specific code can of
course use intc... A good example where intc can be used in IRL mode
in the cpu specific code is sh7785 which provides bitmap support for
both IRQ mode and IRL.

> If the processor only has priority registers the "handle" will not be
> set in intc_register_irq (because of "if(!primary)")
> This leads to wrong results from _INTC_WIDTH, which causes
> intc_set_priority fail with EINVAL

You cannot set priorities for interrupt source with priority as
primary masking method, right? It sure looks like a bug. Thank you for
spending time on tracking this down.

> When I comment out the "if(!primary)" condition all priorities will be
> set immediatelly, which also enables them.
> When I hardcode the _INTC_WIDTH to the correct size in set_priority, the
> priority will be set as soon as the irq is requested.

Right. The correct fix is probably to setup the handle properly but
use a _INTC_FN(h) set to REG_FN_ERR to indicate that there is no point
in writing the priority value to hardware.

I'll post fixes in a little while. Thank you!

/ magnus

AW: intc - rework core code

[EXTERNAL B. M. (P. ST-FIR/Eng) <ext...@de...>]

Greetings

...
> I wanted to keep it semi-secret ...

That reminds me on your way you access arrays ;-)
I've never seen things like
data[!primary]
before and was really surprised it works. What about=20
data[1 - primary]
or
data[1 ^ primary]
If you ask me this is easier to understand and more important this
complies with any C implementation.
The C standard defines false as 0 and any other value is defined as
true. So !0 could be anything.

>> The code doesn't work for processors without mask registers.
>=20
> Is that so? I've tried the code on sh7750 among other cpus without any
> problems. But I may of course have missed something...
>=20
> Just to clarify (you probably know this already, but anyway):
>=20
> The intc code requires at least one way to mask an interrupt.=20

Yes I already ran into BUG_ON(!data[primary]) ;-)

> You cannot set priorities for interrupt source with priority as
> primary masking method, right? It sure looks like a bug. Thank you for
> spending time on tracking this down.
>=20
Right!

> I'll post fixes in a little while. Thank you!

Great! I'm looking forward to your fix.=20
I'm already cleaning up my SH7720 patch, so I will send it out the next
days.

Regards

Markus

Re: intc - rework core code

[Magnus D. <mag...@gm...>]

Hi Markus!

On 8/14/07, EXTERNAL Brunner Markus (Praktikant; ST-FIR/Eng)
<ext...@de...> wrote:
> Greetings
>
> ...
> > I wanted to keep it semi-secret ...
>
> That reminds me on your way you access arrays ;-)

=)

> I've never seen things like
> data[!primary]
> before and was really surprised it works. What about
> data[1 - primary]
> or
> data[1 ^ primary]
> If you ask me this is easier to understand and more important this
> complies with any C implementation.
> The C standard defines false as 0 and any other value is defined as
> true. So !0 could be anything.

Does this mean that you dislike the idea of "!!" as well? =)
Grep the kernel sources and you'll find that here and there...

I think it's important that people easily can read and understand the
code, but I'm not sure if your proposal makes it any more readable.
Feel free to cook up a patch and if other people prefer that as well
then I'm all for it. =)

> > I'll post fixes in a little while. Thank you!
>
> Great! I'm looking forward to your fix.

Ok, I've just posted the patch. Please let me know if it solves your problem.

> I'm already cleaning up my SH7720 patch, so I will send it out the next
> days.

Excellent. Having support for sh7720 in 2.6.24 would be nice.

Thanks!

/ magnus

Re: intc - rework core code

[Paul M. <le...@li...>]

On Wed, Aug 15, 2007 at 12:03:54PM +0900, Magnus Damm wrote:
> > I've never seen things like
> > data[!primary]
> > before and was really surprised it works. What about
> > data[1 - primary]
> > or
> > data[1 ^ primary]
> > If you ask me this is easier to understand and more important this
> > complies with any C implementation.
> > The C standard defines false as 0 and any other value is defined as
> > true. So !0 could be anything.
> 
> Does this mean that you dislike the idea of "!!" as well? =)
> Grep the kernel sources and you'll find that here and there...
> 
You can find a lot of stupid things in the kernel, most of drivers/ for
example. Just because it's in the kernel doesn't mean you should blindly
follow all of its silly conventions. !! does end up being pretty
expensive on platforms that have trivial test-and-branch opcodes, even if
it's more obvious when used in the code. The main area where it ends up
being useful is for places where you need to test a value but do not want
to have to emit the shift and other things for it (especially for
platforms that can't do large immediates relative to the bit position),
but it is not a blanket win. ;-)

> I think it's important that people easily can read and understand the
> code, but I'm not sure if your proposal makes it any more readable.
> Feel free to cook up a patch and if other people prefer that as well
> then I'm all for it. =)
> 
More comments or additional variables with some more descriptive names
might deal with whatever confusion arises from it. Patches to rework the
ambiguity are always a good thing.