From: Magnus D. <mag...@gm...> - 2007-08-15 02:45:27
|
sh: intc - primary priority masking fixes This patch contains various intc fixes for problems reported by Markus Brunner on the linuxsh-dev mailing list: http://marc.info/?l=linuxsh-dev&m=118701948224991&w=1 Apart from added comments, the fixes are: - add intc_set_priority() function prototype to hw_irq.h - fix off-by-one error in intc_set_priority() - make sure _INTC_WIDTH() is set for primary priority masking Big thanks to Markus for finding these problems. Signed-off-by: Magnus Damm <da...@ig...> --- Markus, could you please check if this patch solves your problems and if so reply with an Acked-by? arch/sh/kernel/cpu/irq/intc.c | 33 ++++++++++++++++++++++++++++----- include/asm-sh/hw_irq.h | 1 + 2 files changed, 29 insertions(+), 5 deletions(-) --- 0001/arch/sh/kernel/cpu/irq/intc.c +++ work/arch/sh/kernel/cpu/irq/intc.c 2007-08-14 14:04:12.000000000 +0900 @@ -206,6 +206,18 @@ static struct intc_handle_int *intc_find { int i; + /* this doesn't scale well, but... + * + * this function should only be used for cerain uncommon + * operations such as intc_set_priority() and intc_set_sense() + * and in those rare cases performance doesn't matter that much. + * keeping the memory footprint low is more important. + * + * one rather simple way to speed this up and still keep the + * memory footprint down is to make sure the array is sorted + * and then perform a bisect to lookup the irq. + */ + for (i = 0; i < nr_hp; i++) { if ((hp + i)->irq != irq) continue; @@ -226,7 +238,7 @@ int intc_set_priority(unsigned int irq, ihp = intc_find_irq(d->prio, d->nr_prio, irq); if (ihp) { - if (prio >= ((1 << _INTC_WIDTH(ihp->handle)) - 1)) + if (prio >= (1 << _INTC_WIDTH(ihp->handle)) return -EINVAL; intc_prio_level[irq] = prio; @@ -237,7 +249,7 @@ int intc_set_priority(unsigned int irq, * priority level will be set during next enable() */ - if (ihp->handle) + if (_INTC_FN(ihp->handle) != REG_FN_ERR) _intc_enable(irq, ihp->handle); } return 0; @@ -457,6 +469,7 @@ static void __init intc_register_irq(str intc_enum enum_id, unsigned int irq) { + struct intc_handle_int *hp; unsigned int data[2], primary; /* Prefer single interrupt source bitmap over other combinations: @@ -495,9 +508,19 @@ static void __init intc_register_irq(str /* 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]; + hp = d->prio + d->nr_prio; + hp->irq = irq; + hp->handle = data[1]; + + if (!primary) { + /* + * only secondary priority should access registers, so + * set _INTC_FN(h) = REG_FN_ERR for intc_set_priority() + */ + + hp->handle &= ~_INTC_MK(0x0f, 0, 0, 0, 0, 0); + hp->handle |= _INTC_MK(REG_FN_ERR, 0, 0, 0, 0, 0); + } d->nr_prio++; } --- 0002/include/asm-sh/hw_irq.h +++ work/include/asm-sh/hw_irq.h 2007-08-14 14:05:34.000000000 +0900 @@ -90,6 +90,7 @@ struct intc_desc symbol __initdata = { } void __init register_intc_controller(struct intc_desc *desc); +int intc_set_priority(unsigned int irq, unsigned int prio); void __init plat_irq_setup(void); |
From: Magnus D. <mag...@gm...> - 2007-08-15 04:53:32
|
On 8/15/07, Magnus Damm <mag...@gm...> wrote: > sh: intc - primary priority masking fixes Uhm, this patch doesn't compile... Sorry about that. > --- 0001/arch/sh/kernel/cpu/irq/intc.c > +++ work/arch/sh/kernel/cpu/irq/intc.c 2007-08-14 14:04:12.000000000 +0900 > @@ -226,7 +238,7 @@ int intc_set_priority(unsigned int irq, > > ihp = intc_find_irq(d->prio, d->nr_prio, irq); > if (ihp) { > - if (prio >= ((1 << _INTC_WIDTH(ihp->handle)) - 1)) > + if (prio >= (1 << _INTC_WIDTH(ihp->handle)) > return -EINVAL; > > intc_prio_level[irq] = prio; I'm missing a ")" in the if statement above. Apart from that the patch should be ok. Markus, could you please fix this by hand and check if the patch is ok? / magnus |
From: EXTERNAL B. M. (P. ST-FIR/Eng) <ext...@de...> - 2007-08-16 08:26:09
|
Hi, I didn't have access to the hardware yesterday. > Markus, could you please fix this by hand and check if the patch is ok? The patch needed another fix. It was my old friend "!" =3D) After that it worked. - if (!primary) { + if (primary) { /* * only secondary priority should access registers, so * set _INTC_FN(h) =3D REG_FN_ERR for intc_set_priority() */ I don't have problems with "!" in conditions, because it's a logical operator in a logical condition and it's not a not not in a numerical calculation ;) I think the way you check handle against REG_FN_ERR is problematic. You set it with a binary or, + hp->handle &=3D ~_INTC_MK(0x0f, 0, 0, 0, 0, 0); + hp->handle |=3D _INTC_MK(REG_FN_ERR, 0, 0, 0, 0, 0); but you check it directly. if (_INTC_FN(ihp->handle) !=3D REG_FN_ERR) I'm already hunting the next bug.=20 It seems to be without influence on the functionality. The value of "handle" for the sense is also wrong. So I better find the bug before it finds us ;) Markus |
From: EXTERNAL B. M. (P. ST-FIR/Eng) <ext...@de...> - 2007-08-16 08:38:00
|
> I'm already hunting the next bug.=20 I found the bug. It was in my printk debug messages, which lead me to wrong decisions. So your code is right. Regards Markus |