<div dir="ltr">I'm going to try to look at the Pi BSP warnings soon. I can try to help address some of the issues with this patch as well.<div><br></div><div>What is the desired cut off date for 4.11? </div><div><br></div><div>Alan</div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Oct 31, 2014 at 11:41 AM, Pavel Pisa <span dir="ltr"><<a href="mailto:ppisa4lists@pikron.com" target="_blank">ppisa4lists@pikron.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hello Andre,<br>
<div><div class="h5"><br>
On Friday 31 of October 2014 12:56:40 Andre Marques wrote:<br>
<br>
> +/**<br>
> + * @brief Generic ISR that clears the event register on the Raspberry Pi<br>
> and calls + * an user defined ISR.<br>
> + *<br>
> + * @param[in] arg Void pointer to a handler_arguments structure.<br>
> + */<br>
> +static void generic_handler(void* arg)<br>
> +{<br>
> + handler_arguments* handler_args;<br>
> + int rv = 0;<br>
> + int pin = 0;<br>
> +<br>
> + handler_args = (handler_arguments*) arg;<br>
> +<br>
> + pin = handler_args->pin_number;<br>
> +<br>
> + /* If the interrupt was generated by the pin attached to this ISR<br>
> clears it. */ + if ( BCM2835_REG(BCM2835_GPIO_GPEDS0) & (1 << pin) )<br>
> + BCM2835_REG(BCM2835_GPIO_GPEDS0) &= (1 << pin);<br>
> +<br>
> + /* If not lets the next ISR process the interrupt. */<br>
> + else<br>
> + return;<br>
> +<br>
> + /* If this pin has the deboucing function attached, call it. */<br>
> + if ( handler_args->debouncing_tick_count > 0 ) {<br>
> + rv = debounce_switch(pin);<br>
> +<br>
> + if ( rv < 0 )<br>
> + return;<br>
> + }<br>
> +<br>
> + /* Call the user's ISR. */<br>
</div></div>> + (handler_args->handler) (handler_args->handler_arg);<br>
> +}<br>
<br>
There has to be mechanism (array of pointers) for registering args<br>
for each pin handler routine.<br>
<span class=""><br>
> +/**<br>
> + * @brief Enables interrupts to be generated on a given GPIO pin.<br>
> + * When fired that interrupt will call the given handler.<br>
> + *<br>
> + * @param[in] dev_pin Raspberry Pi GPIO pin label number (not its position<br>
> + * on the header).<br>
> + * @param[in] interrupt Type of interrupt to enable for the pin.<br>
> + * @param[in] handler Pointer to a function that will be called every time<br>
> + * @var interrupt is generated. This function must have<br>
> + * no receiving parameters and return void.<br>
</span> @param[in] arg or context<br>
<span class=""><br>
> + *<br>
> + * @retval 0 Interrupt successfully enabled for this pin.<br>
> + * @retval -1 Could not replace the currently active interrupt on this<br>
> pin, or + * unknown @var interrupt.<br>
> + */<br>
> +int gpio_enable_interrupt(int dev_pin, gpio_interrupt interrupt, void<br>
> (*handler)(void)) +{<br>
<br>
<br>
</span><span class="">int gpio_enable_interrupt(int dev_pin, gpio_interrupt interrupt, void<br>
</span> (*handler)(void *arg), void *arg)<br>
<br>
It would worth to be able to register mutiple handlers for single<br>
pin for case o emulating shared IRQ line - i.e. ISA bus, then information<br>
about IRQ handled/unhandled state has to be returned by handler - but that<br>
is not critical. Argument is critical as I have already stated.<br>
<br>
The handler function prototype should be same as for RTEMS ISR registration<br>
<br>
typedef void (*rtems_interrupt_handler)(void *);<br>
<br>
This allows to use drivers for some generic chips to be connected<br>
to some external bus same way on different CPU and boards architectures<br>
with same ISR handler when directly connected to some external interrupt<br>
input and use same driver+handler when interrupt is connected<br>
to GPIO ISR multiplexor.<br>
<br>
I have already raised this concern for previous patch version<br>
- see mail thread<br>
<br>
<a href="http://article.gmane.org/gmane.os.rtems.user/13211" target="_blank">http://article.gmane.org/gmane.os.rtems.user/13211</a><br>
<span class=""><br>
> + rtems_status_code sc;<br>
> + rpi_gpio_pin *pin;<br>
> +<br>
> + /* Only consider GPIO pins up to 31. */<br>
> + if ( dev_pin > 31 )<br>
> + return -1;<br>
> +<br>
> + pin = &gpio_pin[dev_pin-1];<br>
> +<br>
> + /* If the pin already has an enabled interrupt removes it first,<br>
> + * as well as its handler. */<br>
> + if ( pin->enabled_interrupt != NONE ) {<br>
> + sc = gpio_disable_interrupt(dev_pin);<br>
> +<br>
> + if ( sc != RTEMS_SUCCESSFUL )<br>
> + return -1;<br>
> + }<br>
> +<br>
> + pin->h_args.pin_number = dev_pin;<br>
> + pin->h_args.handler = handler;<br>
<br>
</span> + pin->h_args.handler_arg = arg;<br>
<span class=""><br>
<br>
> +<br>
> + pin->h_args.last_isr_tick = rtems_clock_get_ticks_since_boot();<br>
> +<br>
> + /* Installs the generic_handler, which will call the user handler<br>
> received + * a parameter. */<br>
> + sc = rtems_interrupt_handler_install(BCM2835_IRQ_ID_GPIO_0,<br>
> + NULL,<br>
> + RTEMS_INTERRUPT_SHARED,<br>
> + (rtems_interrupt_handler)<br>
> generic_handler, + &(pin->h_args));<br>
<br>
</span>The use of RTEMS_INTERRUPT_SHARED and registering multiple handlers<br>
for single pin group is significant overhead. It would be much faster<br>
to register only one handler (when gpio_enable_interrupt is called first time).<br>
Then read interrupt status register (if before HW masking, apply mask)<br>
and use ffs (find first bit set) in loop to process all pending events.<br>
Each time ffs gives the bit, clear the bit in the local copy,<br>
retrieve pin description from the array (ffs gives the index)<br>
and process user provided handler with corresponding user argument.<br>
<div><div class="h5"><br>
> + if ( sc != RTEMS_SUCCESSFUL )<br>
> + return -1;<br>
> +<br>
> + switch ( interrupt ) {<br>
> + case FALLING_EDGE:<br>
> +<br>
> + /* Enables asynchronous falling edge detection. */<br>
> + BCM2835_REG(BCM2835_GPIO_GPAFEN0) |= (1 << dev_pin);<br>
> +<br>
> + break;<br>
> +<br>
> + case RISING_EDGE:<br>
> +<br>
> + /* Enables asynchronous rising edge detection. */<br>
> + BCM2835_REG(BCM2835_GPIO_GPAREN0) |= (1 << dev_pin);<br>
> +<br>
> + break;<br>
> +<br>
> + case BOTH_EDGES:<br>
> +<br>
> + /* Enables asynchronous falling edge detection. */<br>
> + BCM2835_REG(BCM2835_GPIO_GPAFEN0) |= (1 << dev_pin);<br>
> +<br>
> + /* Enables asynchronous rising edge detection. */<br>
> + BCM2835_REG(BCM2835_GPIO_GPAREN0) |= (1 << dev_pin);<br>
> +<br>
> + break;<br>
> +<br>
> + case LOW_LEVEL:<br>
> +<br>
> + /* Enables pin low level detection. */<br>
> + BCM2835_REG(BCM2835_GPIO_GPLEN0) |= (1 << dev_pin);<br>
> +<br>
> + break;<br>
> +<br>
> + case HIGH_LEVEL:<br>
> +<br>
> + /* Enables pin high level detection. */<br>
> + BCM2835_REG(BCM2835_GPIO_GPHEN0) |= (1 << dev_pin);<br>
> +<br>
> + break;<br>
> +<br>
> + case BOTH_LEVELS:<br>
> +<br>
> + /* Enables pin low level detection. */<br>
> + BCM2835_REG(BCM2835_GPIO_GPLEN0) |= (1 << dev_pin);<br>
> +<br>
> + /* Enables pin high level detection. */<br>
> + BCM2835_REG(BCM2835_GPIO_GPHEN0) |= (1 << dev_pin);<br>
> +<br>
> + break;<br>
> +<br>
> + case NONE:<br>
> + return 0;<br>
> +<br>
> + default:<br>
> + return -1;<br>
> + }<br>
> +<br>
> + pin->enabled_interrupt = interrupt;<br>
> +<br>
> + return 0;<br>
> +}<br>
<br>
</div></div>The whole function and many others require to define some locking<br>
to can be used from paralley running threads.<br>
<br>
There should be functions which do plain enable and disable of IRQ<br>
generation and then functions to register and unregister handler for pin.<br>
Register can/should probably call enable at the end to be consistent with<br>
rtems_interrupt_handler_install(). The enable and disable functions<br>
should and in case of RPi can be written (separates set/reset registers and<br>
single CPU core so CPU level IRQ disable is cheap) such way that<br>
they can be called from ISR context (no mutex locking). On the other hand<br>
<br>
gpio_enable_interrupt/gpio_interrupt_handler_install has quite little<br>
other options than to use mutex. Use of CPU level IRQ disable<br>
is not good for latencies.<br>
<br>
There are probably some compiler (SMP for case of multiple cores)<br>
barriers to ensure that structure is updated before enable<br>
of IRQ in hardware.<br>
<div><div class="h5"><br>
> +/**<br>
> + * @brief Stops interrupts from being generated from a given GPIO pin<br>
> + * and removes the corresponding handler.<br>
> + *<br>
> + * @param[in] dev_pin Raspberry Pi GPIO pin label number (not its position<br>
> + * on the header).<br>
> + *<br>
> + * @retval 0 Interrupt successfully disabled for this pin.<br>
> + * @retval -1 Could not remove the current interrupt handler or could not<br>
> + * recognise the current active interrupt on this pin.<br>
> + */<br>
> +int gpio_disable_interrupt(int dev_pin)<br>
> +{<br>
> + rtems_status_code sc;<br>
> + rpi_gpio_pin *pin;<br>
> +<br>
> + pin = &gpio_pin[dev_pin-1];<br>
> +<br>
> + switch ( pin->enabled_interrupt ) {<br>
> + case FALLING_EDGE:<br>
> +<br>
> + /* Disables asynchronous falling edge detection. */<br>
> + BCM2835_REG(BCM2835_GPIO_GPAFEN0) &= ~(1 << dev_pin);<br>
> +<br>
> + break;<br>
> +<br>
> + case RISING_EDGE:<br>
> +<br>
> + /* Disables asynchronous rising edge detection. */<br>
> + BCM2835_REG(BCM2835_GPIO_GPAREN0) &= ~(1 << dev_pin);<br>
> +<br>
> + break;<br>
> +<br>
> + case BOTH_EDGES:<br>
> +<br>
> + /* Disables asynchronous falling edge detection. */<br>
> + BCM2835_REG(BCM2835_GPIO_GPAFEN0) &= ~(1 << dev_pin);<br>
> +<br>
> + /* Disables asynchronous rising edge detection. */<br>
> + BCM2835_REG(BCM2835_GPIO_GPAREN0) &= ~(1 << dev_pin);<br>
> +<br>
> + break;<br>
> +<br>
> + case LOW_LEVEL:<br>
> +<br>
> + /* Disables pin low level detection. */<br>
> + BCM2835_REG(BCM2835_GPIO_GPLEN0) &= ~(1 << dev_pin);<br>
> +<br>
> + break;<br>
> +<br>
> + case HIGH_LEVEL:<br>
> +<br>
> + /* Disables pin high level detection. */<br>
> + BCM2835_REG(BCM2835_GPIO_GPHEN0) &= ~(1 << dev_pin);<br>
> +<br>
> + break;<br>
> +<br>
> + case BOTH_LEVELS:<br>
> +<br>
> + /* Disables pin low level detection. */<br>
> + BCM2835_REG(BCM2835_GPIO_GPLEN0) &= ~(1 << dev_pin);<br>
> +<br>
> + /* Disables pin high level detection. */<br>
> + BCM2835_REG(BCM2835_GPIO_GPHEN0) &= ~(1 << dev_pin);<br>
> +<br>
> + break;<br>
> +<br>
> + case NONE:<br>
> + return 0;<br>
> +<br>
> + default:<br>
> + return -1;<br>
> + }<br>
> +<br>
> + /* Removes the handler. */<br>
> + sc = rtems_interrupt_handler_remove(BCM2835_IRQ_ID_GPIO_0,<br>
> + (rtems_interrupt_handler)<br>
> generic_handler, + &(pin->h_args));<br>
> +<br>
> + if ( sc != RTEMS_SUCCESSFUL )<br>
> + return -1;<br>
> +<br>
> + pin->enabled_interrupt = NONE;<br>
> +<br>
> + return 0;<br>
> +}<br>
<br>
<br>
</div></div>You can look how interrupts are used in real-time RPi demo application<br>
to control DC motor with incremental encoder signals detections by software.<br>
We have succeed to process 25000 interrupts/second with fully preemptive<br>
Linux kernel where actual IRQ handlers are run in kernel threads/this means<br>
than each ISR results in kernel context switch.<br>
<br>
<a href="http://lintarget.sourceforge.net/rpi-motor-control/index.html" target="_blank">http://lintarget.sourceforge.net/rpi-motor-control/index.html</a><br>
<br>
<a href="https://github.com/ppisa/rpi-rt-control/blob/master/kernel/modules/rpi_gpio_irc_module.c" target="_blank">https://github.com/ppisa/rpi-rt-control/blob/master/kernel/modules/rpi_gpio_irc_module.c</a><br>
<br>
RTEMS should be in much better situation there because its context switch<br>
is much lighter - no MMU consideration etc. So same task should<br>
be portable for RTEMS easily. Even quadrature decoding can be done in<br>
single handler for both pins because there is no latency between<br>
HW ISR and its handler running in the kernel thread. So simple<br>
state decoding could be used by reading values from GPIO input register.<br>
<br>
Please, try to make API and functions robust and (re)usable.<br>
If you have problem to see all consequences then I can help with<br>
some advises or if others agree I would prepare some follow up<br>
patches to clean the code as my time allows ....<br>
I have not so big problem with time to rewrite code but time<br>
for testing is much bigger problem.<br>
<br>
Best wishes,<br>
<br>
Pavel<br>
<div class="HOEnZb"><div class="h5"><br>
<br>
<br>
<br>
_______________________________________________<br>
devel mailing list<br>
<a href="mailto:devel@rtems.org">devel@rtems.org</a><br>
<a href="http://lists.rtems.org/mailman/listinfo/devel" target="_blank">http://lists.rtems.org/mailman/listinfo/devel</a><br>
</div></div></blockquote></div><br></div></div>