PowerPC exceptions and interrupts

Tue May 20 08:53:25 UTC 2008

On Tuesday 20 May 2008 09:22, Sebastian Huber wrote:
> Hi,
> here is my proposal for a unified interrupt API and the support code.
> The aim was to provide a clean interface and convenience for the BSP
> developer for a wide range of boards. You may have a look at
> bsp-irq/index.html in the documentation:
>
> http://rapidshare.com/files/116211100/bsp-irq.tgz.html
>
> I'am sorry for this download link.
>
> Thanks,
>     Sebastian

Hello Sebastian and all others,

I think, that trying to unite and clean RTEMS interrupt
support is a needed and honorable think to do.

There are some comments based on my personal experience
and use of similar routines in our system less designs
and work with Linux and some time ago even with RT-Linux
and following these systems development and changes.

> static rtems_status_code rtems_interrupt_handler_install( 
> 	rtems_vector_number vector,
> 	rtems_interrupt_handler handler, 
> 	void *arg
> 	)

I think personally, that additional function
 rtems_interrupt_handler_unique_install() 
for unique handler install is not good idea.
I would prefer to add some mode/flags field into
regular rtems_interrupt_handler_install() function.
You implementation uses this approach anyway.
I would suggest to use masks provided by defines
for these flags. You need sometimes specific modifications/
modes for interrupts, not only RTEMS_IRQ_SHARE_ALLOWED.
The Linux generic IRQ support converged to allow specify
if IRQ is triggered on rising, falling edge or level directly
in request_irq() function for GPIO IRQ sources for example.

The Linux adds ability to provide some user recognizable name
for handler as well. This is not mandatory for RTEMS, but
it could be of great help, if you need to check what is
registered in the system and to display IRQ counters.

So my proposal is

static rtems_status_code rtems_interrupt_handler_install( 
	rtems_vector_number vector,
	rtems_interrupt_handler handler, 
	rtems_interrupt_flags   flags, 
	char *informative_name,
	void *arg
	)

> static rtems_status_code rtems_interrupt_handler_remove( 
> 	rtems_vector_number vector,
> 	rtems_interrupt_handler handler 
> 	)

As for the remove, I would again suggest to follow proven
way, which is used in Linux. It has significant rationale.
If you have shared interrupt source (consider PCI for example)
and you can have multiple instances of same hardware (multiple
ETHERNET cards for example) sharing same IRQ line, then
you cannot control, which of multiple registration
with same handler would be unregisterred. This could lead
to fatal problems, that state structure for some card is
destroyed, but handler for different one is unregisterred
=> handler with arg pointing to the free/reused memory
would be used => disaster situation.

On the other hand, if device context structure pointer
("arg") is used for selection which shared registration
should be removed, than it finds correct registration instance.

This means, that I would propose with Linux and other OS
proven prototype

static rtems_status_code rtems_interrupt_handler_remove( 
	rtems_vector_number vector,
	void *arg
	)

If you ensure safety even more, handler parameter could
be left there as well. In such case, the functionality
could be extended next way

   handler == NULL .. then only match for arg is checked
   arg == NULL .. then only match for handler is checked
                  and all instances with this handler are removed
   (handler == NULL) && (arg == NULL) .. all registration
                  for given vector are removed

As for low level stuff and relation to the rtems_irq_connect_data,
I have feeling, that your approach is upside down.
It think, that rtems_irq_connect_data and IRQ controllers
low level stuff should be base on which user friendly
 rtems_interrupt_handler_install() and
 rtems_status_code rtems_interrupt_handler_remove()
should be based upon and not vice versa.

The IRQ controller support can gain from ability to have direct
and fast aces to some (even architecture extendable) information
context for each interrupt source
  typedef void (*rtems_irq_enable) (const struct __rtems_irq_connect_data__*);
  typedef void (*rtems_irq_disable) (const struct __rtems_irq_connect_data__*);
  typedef int  (*rtems_irq_is_enabled) (const struct __rtems_irq_connect_data__*);

I agree, that your approach could be less intrusive
for code compatibility, but I think, that for long time
maintainability the vice versa approach is more correct.
On the other hand, low level stuff can be based on structure
named different then "rtems_irq_connect_data".
Basically, rtems_irq_connect_data should provide information
about handler, arg, informative_name, next rtems_irq_connect_data
and pointer to the new rtems_interrupt_source_data.
The rtems_interrupt_source_data should provide common
chip specific routines for rtems_irq_enable, rtems_irq_disable,
rtems_irq_is_enabled and dispatch support. These do not belong
to the rtems_irq_connect_data. They are common for shared
IRQs for single source.

> typedef void(*rtems_interrupt_handler )(void *)

If we speak about generic support, then to allow
correct sharing for all obscure controllers and
even for edge triggered sources, then
  rtems_interrupt_handler
prototype has to be modified to return information,
if the even has been recognized by device driver or not
  typedef int rtems_interrupt_handled_state;
  typedef rtems_interrupt_handled_state (*rtems_interrupt_handler )(void *)

The next algorithm is required to handle correctly shared edge
triggered sources

   rtems_interrupt_handled_state again;
   rtems_interrupt_handled_state res;
   do {
     again = 0;
     for_each(vector->rtems_irq_connect_data_list, connect) {
       res=connect->hndl(connect->handle);
       if(res == RTEMS_INTERRUPT_HANDLED)
         again = 1;
     }
   } while(again);

I consider above thoughts as minimal requirements for
extendable IRQ handling. You can look into Linux
kernel for much elaborated solution.

Best wishes

                Pavel Pisa

==================================================
 PiKRON s.r.o.       Phone/Fax: +420 2 84684676
 Kankovskeho 1235    Phone:     +420 2 96781671
 182 00 Praha 8      WWW:   http://www.pikron.com/
 Czech Republic      e-mail:  pikron at pikron.com
==================================================