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