linuxsh-dev

sh: simplify board specific interrupt code for solution engine 7780

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

sh: simplify board specific interrupt code for solution engine 7780

The new intc code handles IRQ3 and IRQ7 in the cpu specific code already,
so there is no reason to duplicate that here.

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

 arch/sh/Kconfig              |    1 -
 arch/sh/boards/se/7780/irq.c |   28 +---------------------------
 2 files changed, 1 insertion(+), 28 deletions(-)

--- 0001/arch/sh/Kconfig
+++ work/arch/sh/Kconfig	2007-07-30 12:18:22.000000000 +0900
@@ -258,7 +258,6 @@ config SH_7780_SOLUTION_ENGINE
 	bool "SolutionEngine7780"
 	select SOLUTION_ENGINE
 	select SYS_SUPPORTS_PCI
-	select CPU_HAS_INTC2_IRQ
 	depends on CPU_SUBTYPE_SH7780
 	help
 	  Select 7780 SolutionEngine if configuring for a Renesas SH7780
--- 0001/arch/sh/boards/se/7780/irq.c
+++ work/arch/sh/boards/se/7780/irq.c	2007-07-30 12:16:31.000000000 +0900
@@ -16,32 +16,6 @@
 #include <asm/io.h>
 #include <asm/se7780.h>
 
-static struct intc2_data intc2_irq_table[] = {
-	{ 2,  0, 31, 0, 31, 3 }, /* daughter board EXTINT1 */
-	{ 4,  0, 30, 0, 30, 3 }, /* daughter board EXTINT2 */
-	{ 6,  0, 29, 0, 29, 3 }, /* daughter board EXTINT3 */
-	{ 8,  0, 28, 0, 28, 3 }, /* SMC 91C111 (LAN) */
-	{ 10, 0, 27, 0, 27, 3 }, /* daughter board EXTINT4 */
-	{ 4,  0, 30, 0, 30, 3 }, /* daughter board EXTINT5 */
-	{ 2,  0, 31, 0, 31, 3 }, /* daughter board EXTINT6 */
-	{ 2,  0, 31, 0, 31, 3 }, /* daughter board EXTINT7 */
-	{ 2,  0, 31, 0, 31, 3 }, /* daughter board EXTINT8 */
-	{ 0 , 0, 24, 0, 24, 3 }, /* SM501 */
-};
-
-static struct intc2_desc intc2_irq_desc __read_mostly = {
-	.prio_base	= 0, /* N/A */
-	.msk_base	= 0xffd00044,
-	.mskclr_base	= 0xffd00064,
-
-	.intc2_data	= intc2_irq_table,
-	.nr_irqs	= ARRAY_SIZE(intc2_irq_table),
-
-	.chip = {
-		.name	= "INTC2-se7780",
-	},
-};
-
 /*
  * Initialize IRQ setting
  */
@@ -68,5 +42,5 @@ void __init init_se7780_IRQ(void)
 	/* FPGA + 0x0A */
 	ctrl_outw((IRQPIN_PCCPW << IRQPOS_PCCPW), FPGA_INTSEL3);
 
-	register_intc2_controller(&intc2_irq_desc);
+	plat_irq_setup_pins(IRQ_MODE_IRQ); /* install handlers for IRQ0-7 */
 }

Re: sh: simplify board specific interrupt code for solution engine 7780

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

On Mon, Jul 30, 2007 at 07:38:06PM +0900, Magnus Damm wrote:
> sh: simplify board specific interrupt code for solution engine 7780
> 
> The new intc code handles IRQ3 and IRQ7 in the cpu specific code already,
> so there is no reason to duplicate that here.
> 
> Signed-off-by: Magnus Damm <da...@ig...>

Looks good, As Iwamatsu-san is probably the only one testing this board
at the moment, it would be nice to know if this causes any regressions
there. I'll add it to the 2.6.24 queue.

Re: sh: simplify board specific interrupt code for solution engine 7780

[Nobuhiro I. <he...@t-...>]

Hi,

On Mon, 30 Jul 2007 19:38:06 +0900
Magnus Damm <mag...@gm...> wrote:

> sh: simplify board specific interrupt code for solution engine 7780
> 
> The new intc code handles IRQ3 and IRQ7 in the cpu specific code already,
> so there is no reason to duplicate that here.
> 
> Signed-off-by: Magnus Damm <da...@ig...>
> ---
Thank you for your patch.

I have some comments.
In mounting your IRQ, the priority level is not change.
(The priority level is not change from 2. )
I thought the interrupt priority level to be to be wanted change from 
the setting of the board side. 

For example ...

diff --git a/arch/sh/boards/se/7780/irq.c b/arch/sh/boards/se/7780/irq.c
index 6bd70da..e412063 100644
--- a/arch/sh/boards/se/7780/irq.c
+++ b/arch/sh/boards/se/7780/irq.c
@@ -16,11 +16,31 @@
 #include <asm/io.h>
 #include <asm/se7780.h>
 
+/* 
+ * FIXME 
+ * copy from arch/sh/kernel/cpu/sh4a/setup-sh7780.c 
+ */
+enum {
+	IRQ0 = 16 , IRQ1, IRQ2, IRQ3, IRQ4, IRQ5, IRQ6, IRQ7,
+};
+
 /*
  * Initialize IRQ setting
  */
 void __init init_se7780_IRQ(void)
 {
+	/* priority level to board */
+	static struct intc_prio plat_prio[] = {
+	        INTC_PRIO(IRQ0, 3),
+	        INTC_PRIO(IRQ1, 3),
+	        INTC_PRIO(IRQ2, 3),
+	        INTC_PRIO(IRQ3, 3),
+	        INTC_PRIO(IRQ4, 3),
+	        INTC_PRIO(IRQ5, 3),
+	        INTC_PRIO(IRQ6, 3),
+	        INTC_PRIO(IRQ7, 3),
+	};
+
 	/* enable all interrupt at FPGA */
 	ctrl_outw(0, FPGA_INTMSK1);
 	/* mask SM501 interrupt */
@@ -42,5 +62,7 @@ void __init init_se7780_IRQ(void)
 	/* FPGA + 0x0A */
 	ctrl_outw((IRQPIN_PCCPW << IRQPOS_PCCPW), FPGA_INTSEL3);
 
-	plat_irq_setup_pins(IRQ_MODE_IRQ); /* install handlers for IRQ0-7 */
+	/* install handlers for IRQ0-7 */
+	plat_irq_setup_pins(IRQ_MODE_IRQ
+		, plat_prio, sizeof(plat_prio)/sizeof(*plat_prio)); 
 }
diff --git a/arch/sh/kernel/cpu/irq/intc.c b/arch/sh/kernel/cpu/irq/intc.c
diff --git a/arch/sh/kernel/cpu/sh4a/setup-sh7780.c b/arch/sh/kernel/cpu/sh4a/setup-sh7780.c
index a4127ec..3ffac0c 100644
--- a/arch/sh/kernel/cpu/sh4a/setup-sh7780.c
+++ b/arch/sh/kernel/cpu/sh4a/setup-sh7780.c
@@ -264,10 +264,15 @@ void __init plat_irq_setup(void)
 	register_intc_controller(&intc_desc);
 }
 
-void __init plat_irq_setup_pins(int mode)
+void __init plat_irq_setup_pins(int mode , struct intc_prio *plat_prio , int plat_size)
 {
+
 	switch (mode) {
 	case IRQ_MODE_IRQ:
+        	if(plat_prio != NULL){
+                	intc_irq_desc.priorities = plat_prio;
+                	intc_irq_desc.nr_priorities = plat_size;
+        	}
 		register_intc_controller(&intc_irq_desc);
 		break;
 	case IRQ_MODE_IRL7654:
diff --git a/include/asm-sh/hw_irq.h b/include/asm-sh/hw_irq.h
index a4086ea..0d74f30 100644
--- a/include/asm-sh/hw_irq.h
+++ b/include/asm-sh/hw_irq.h
@@ -95,6 +95,6 @@ void __init plat_irq_setup(void);
 
 enum { IRQ_MODE_IRQ, IRQ_MODE_IRQ7654, IRQ_MODE_IRQ3210,
        IRQ_MODE_IRL7654, IRQ_MODE_IRL3210 };
-void __init plat_irq_setup_pins(int mode);
+void __init plat_irq_setup_pins(int mode , struct intc_prio *plat_prio, int plat_size);
 
 #endif /* __ASM_SH_HW_IRQ_H */

Thanks,
 Nobuhiro
-- 
Nobuhiro Iwamatsu
	he...@t-...
	iwa...@de...

	GPG ID : 3170EBE9

Re: sh: simplify board specific interrupt code for solution engine 7780

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

Hi everyone,

Thanks for your comments Iwamatsu-san!

On 8/4/07, Nobuhiro Iwamatsu <he...@t-...> wrote:
> Hi,
>
> On Mon, 30 Jul 2007 19:38:06 +0900
> Magnus Damm <mag...@gm...> wrote:
>
> > sh: simplify board specific interrupt code for solution engine 7780
> >
> > The new intc code handles IRQ3 and IRQ7 in the cpu specific code already,
> > so there is no reason to duplicate that here.
> >
> > Signed-off-by: Magnus Damm <da...@ig...>
> > ---
> Thank you for your patch.
>
> I have some comments.
> In mounting your IRQ, the priority level is not change.
> (The priority level is not change from 2. )

Uhm, yes - the priorities are lost with this patch.

> I thought the interrupt priority level to be to be wanted change from
> the setting of the board side.

I think we should stop and zoom out a bit here. I believe that
adjustable interrupt priorities are a good idea, and so far we've kept
them in the board specific code for board specific interrupts. But we
may want to setup the priorities for other interrupts too - cpu
specific interrupts for instance - and there is no good way to do that
now.

> For example ...
>
> diff --git a/arch/sh/boards/se/7780/irq.c b/arch/sh/boards/se/7780/irq.c
> index 6bd70da..e412063 100644
> --- a/arch/sh/boards/se/7780/irq.c
> +++ b/arch/sh/boards/se/7780/irq.c
> @@ -16,11 +16,31 @@
>  #include <asm/io.h>
>  #include <asm/se7780.h>
>
> +/*
> + * FIXME
> + * copy from arch/sh/kernel/cpu/sh4a/setup-sh7780.c
> + */
> +enum {
> +       IRQ0 = 16 , IRQ1, IRQ2, IRQ3, IRQ4, IRQ5, IRQ6, IRQ7,
> +};

I understand that you want to setup your priorities somehow, but I
think there must exist better and cleaner ways to do it. I know Paul
was thinking about adding some function to set interrupt priority
similar to set_irq_type(), but I wonder if it may be better to allow
the user to select the priorities from sysfs or something. Probably
not, at least not as a first step.

So if some generic function like set_irq_type() could be used that
would be great - otherwise I think just adding some intc specific
function is good enough. So this is what I would prefer something like
this:

void board_irq_setup()
{
  /* first register the board specific interrupts */
  register_intc_controller(&intc_desc);

  /* then setup interrupt priorities for this board */
  set_irq_priority(STNIC_IRQ, 12);
  set_irq_priority(CF_IRQ, 3);
}

Does that solve what you are want to do?

Speaking about interrupt priorities, the irq code in
arch/sh/boards/se/770x/irq.c seem to set a priority that is very
similar to the IRQ value. Do you know anything about this? Is this
priority value similar by pure coincidence, or is this related to the
IRL interrupt number?

Thanks!

/ magnus

Re: sh: simplify board specific interrupt code for solution engine 7780

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

On Mon, Aug 06, 2007 at 06:33:10PM +0900, Magnus Damm wrote:
> I understand that you want to setup your priorities somehow, but I
> think there must exist better and cleaner ways to do it. I know Paul
> was thinking about adding some function to set interrupt priority
> similar to set_irq_type(), but I wonder if it may be better to allow
> the user to select the priorities from sysfs or something. Probably
> not, at least not as a first step.
> 
> So if some generic function like set_irq_type() could be used that
> would be great - otherwise I think just adding some intc specific
> function is good enough. So this is what I would prefer something like
> this:
> 
> void board_irq_setup()
> {
>   /* first register the board specific interrupts */
>   register_intc_controller(&intc_desc);
> 
>   /* then setup interrupt priorities for this board */
>   set_irq_priority(STNIC_IRQ, 12);
>   set_irq_priority(CF_IRQ, 3);
> }
> 
> Does that solve what you are want to do?
> 
> Speaking about interrupt priorities, the irq code in
> arch/sh/boards/se/770x/irq.c seem to set a priority that is very
> similar to the IRQ value. Do you know anything about this? Is this
> priority value similar by pure coincidence, or is this related to the
> IRL interrupt number?
> 
Yes, I was thinking of something like the following.

This does restrict the range of allowable priorities at request_irq()
time, but if someone has a use case for an explicit priority,
set_irq_prio() can be used for kicking that down to the chip handler.

I would wager that 99.99999% of all use cases will fit in with
default/high/low priorities, though. If someone has a non-academic use
case that won't fit in this model, I'd be interested in hearing it.

--

diff --git a/include/linux/irq.h b/include/linux/irq.h
index 1695054..1f7578e 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -45,6 +45,11 @@ typedef	void fastcall (*irq_flow_handler_t)(unsigned int irq,
 #define IRQ_TYPE_SENSE_MASK	0x0000000f	/* Mask of the above */
 #define IRQ_TYPE_PROBE		0x00000010	/* Probing in progress */
 
+/* IRQ priorities */
+#define IRQ_PRIO_NONE		0x00000000	/* Default priority */
+#define IRQ_PRIO_HIGH		0x00000001	/* High priority */
+#define IRQ_PRIO_LOW		0x00000002	/* Low priority */
+
 /* Internal flags */
 #define IRQ_INPROGRESS		0x00000100	/* IRQ handler active - do not enter! */
 #define IRQ_DISABLED		0x00000200	/* IRQ disabled - do not enter! */
@@ -91,6 +96,7 @@ struct msi_desc;
  * @retrigger:		resend an IRQ to the CPU
  * @set_type:		set the flow type (IRQ_TYPE_LEVEL/etc.) of an IRQ
  * @set_wake:		enable/disable power-management wake-on of an IRQ
+ * @set_prio:		set the priority of an IRQ
  *
  * @release:		release function solely used by UML
  * @typename:		obsoleted by name, kept as migration helper
@@ -113,6 +119,7 @@ struct irq_chip {
 	int		(*retrigger)(unsigned int irq);
 	int		(*set_type)(unsigned int irq, unsigned int flow_type);
 	int		(*set_wake)(unsigned int irq, unsigned int on);
+	int		(*set_prio)(unsigned int irq, unsigned int prio);
 
 	/* Currently used only by UML, might disappear one day.*/
 #ifdef CONFIG_IRQ_RELEASE_METHOD
@@ -379,6 +386,7 @@ extern int set_irq_data(unsigned int irq, void *data);
 extern int set_irq_chip_data(unsigned int irq, void *data);
 extern int set_irq_type(unsigned int irq, unsigned int type);
 extern int set_irq_msi(unsigned int irq, struct msi_desc *entry);
+extern int set_irq_prio(unsigned int irq, unsigned int prio);
 
 #define get_irq_chip(irq)	(irq_desc[irq].chip)
 #define get_irq_chip_data(irq)	(irq_desc[irq].chip_data)
diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index 615ce97..fa7df4d 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -141,6 +141,32 @@ int set_irq_type(unsigned int irq, unsigned int type)
 EXPORT_SYMBOL(set_irq_type);
 
 /**
+ *	set_irq_prio - set the irq priority for an irq
+ *	@irq:	irq number
+ *	@prio:	interrupt priority - see include/linux/irq.h
+ */
+int set_irq_prio(unsigned int irq, unsigned int prio)
+{
+	struct irq_desc *desc;
+	unsigned long flags;
+	int ret = -ENXIO;
+
+	if (irq >= NR_IRQS) {
+		printk(KERN_ERR "Trying to set irq priority for IRQ%d\n", irq);
+		return -ENODEV;
+	}
+
+	desc = irq_desc + irq;
+	if (desc->chip->set_prio) {
+		spin_lock_irqsave(&desc->lock, flags);
+		ret = desc->chip->set_prio(irq, prio);
+		spin_unlock_irqrestore(&desc->lock, flags);
+	}
+	return ret;
+}
+EXPORT_SYMBOL(set_irq_prio);
+
+/**
  *	set_irq_data - set irq type data for an irq
  *	@irq:	Interrupt number
  *	@data:	Pointer to interrupt specific data

Re: sh: simplify board specific interrupt code for solution engine 7780

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

On Mon, Aug 06, 2007 at 06:47:14PM +0900, Paul Mundt wrote:
> On Mon, Aug 06, 2007 at 06:33:10PM +0900, Magnus Damm wrote:
> > I understand that you want to setup your priorities somehow, but I
> > think there must exist better and cleaner ways to do it. I know Paul
> > was thinking about adding some function to set interrupt priority
> > similar to set_irq_type(), but I wonder if it may be better to allow
> > the user to select the priorities from sysfs or something. Probably
> > not, at least not as a first step.
> > 
> > So if some generic function like set_irq_type() could be used that
> > would be great - otherwise I think just adding some intc specific
> > function is good enough. So this is what I would prefer something like
> > this:
> > 
> > void board_irq_setup()
> > {
> >   /* first register the board specific interrupts */
> >   register_intc_controller(&intc_desc);
> > 
> >   /* then setup interrupt priorities for this board */
> >   set_irq_priority(STNIC_IRQ, 12);
> >   set_irq_priority(CF_IRQ, 3);
> > }
> > 
> > Does that solve what you are want to do?
> > 
> > Speaking about interrupt priorities, the irq code in
> > arch/sh/boards/se/770x/irq.c seem to set a priority that is very
> > similar to the IRQ value. Do you know anything about this? Is this
> > priority value similar by pure coincidence, or is this related to the
> > IRL interrupt number?
> > 
> Yes, I was thinking of something like the following.
> 
> This does restrict the range of allowable priorities at request_irq()
> time, but if someone has a use case for an explicit priority,
> set_irq_prio() can be used for kicking that down to the chip handler.
> 
> I would wager that 99.99999% of all use cases will fit in with
> default/high/low priorities, though. If someone has a non-academic use
> case that won't fit in this model, I'd be interested in hearing it.
> 
Reworked it a bit:

	http://marc.info/?l=linux-kernel&m=118639637304482&w=2