[En-Nut-Discussion] Cortex ix_xxx_interuptY code duplication

[Ulrich Prinz]

Hi guys,

Am 19.07.2012 13:23, schrieb Ole Reinhardt:
> Hi Uwe,
>
>> at present nearly each nut/arch/cm3/dev/xxx/ih_xxx_*.c file implements it's
>> own xxxIrqCtl(int cmd, void *param) function.  The core of that function is
>> always the same, apart from a different xxxIrqEntry and xxx_IRQn
>> settings. Not a very maintainable situation.
>
> This is a _very_ good point and I thought about this before.
>
> For the LPC every IRQ handler uses exactly the same code, I think for
> the STM it will be the same.
>
> Even the AT91 architecture uses the same code in most cases.
>
Fully true!

>
>> ...
>> Shouldn't we extend above TwoWireIrqCtl() to a IrqCtl() function in
>> e.g. arch/cm3/os, replacing all xxxIrqCtl() functtions.
>
> Yes we could easily write a common IrqCtl function, but would have to
> pass the signal as additional argument. This would need changes in the
> API, but from the user land code this should not be called anyway. So an
> API change here should not hurt, right?

The API is no problem to change but I am not sure if there is no user 
calling NutRegisterIRQ as GPIO IRQs are mostly handled by users themselves.
>
>> I think the additional redirection in the case of the common IrqCtl()
>> doesn't matter. That way, much code duplication could be avoided.

I used to add the IRQ number as a parameter to the IRQ control function 
in the IRQ struct already somewhere in the Cortex port. Sort of a 
sub-peripheral address if you like. Should be no problem to extend this 
to all peripherals.
But isn't it then better to pass the struct itself to the IRQ setup und 
IRQ handle function addressed from the struct? It creates less CPU 
overhead and stack operations. So you have all information at one 
address and ARM/Cortex can address relative without speed loss.
>
> I think there wouldn't be any further redirection needed as long as we
> pass the IRQ_HANDLER instead of a user argument, as the user argument is
> stored in the IRQ_HANDLER struct, isn't it?
>
>> Perhaps even one file for all IRQ_HANDLERs.
>
> Yes, I fully agree. We could stay with one file for all interrupts. The
> only thing shich would have to be defined is the IRQ_HANDLER struct
> instance.

Same idea like mine :)
>
>> NUT_IRQCTL_GETMODE/NUT_IRQCTL_SETMODE might need a special consideration, as
>> Ulrich marked some interrupt handlers as NUT_IRQMODE_LEVEL versus the
>> default NUT_IRQMODE_EDGE everywhere in nxp and mostly in stm32.
>
> AFAIK the CM3 does not even support NUT_IRQMODE_LEVEL as the AT91 does.
>
I am not sure if there is an option to have only one file for all Cortex 
SOCs. I guess that we run into trouble if you setup IRQs. SOmne CPUs 
have problems with level on certain peripherals so you need to set edge, 
others do not support edge and need level.

But you may reach a unified content of all xxx_irq_init.c files and 
adding a new one will result in copy-paste-modify.

> On the AT91, the edge driven interrupts might get lost on some interrupt
> sources (see the errata) so we will need to either define a default
> value for each interrupt (could be placed in the IRQ_HANDLER struct as
> well) or better add a IrqCtl(...,
> NUT_IRQCTL_GETMODE/NUT_IRQCTL_SETMODE, ...) at each driver right after
> IRQ registration where we would need to change the IRQ mode. In my eyes
> this is more comprehensive anyway instead of defining different default
> values for each driver.

As I said.
>
>> However I have not seen any consumer of the
>> NUT_IRQCTL_GETMODE/NUT_IRQCTL_SETMODE function in our codebase yet.
>
> On AT91 is is used by several drivers or (if I'm wrong) there are at
> least differen default values used. See my idea above.
>
>> GPIO is a
>> consumer for sure, but for  NXP Ole just implemented GPIO_SIGNAL redirection,
>> handling that case and I will probably reuse for STM.
>
> Yes, I would appreciate we could go the same way there too. From the
> users point of view I implement the same API as it is used by AT91 as
> well (beside of the fact that AT91 does only support an interrupt
> trigger on both edges).
>
Keep an eye on speed with this redirections. I'd like to design a 
software controlled 300W DC/DC controller and every us counts :)

Ulrich