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