[PATCH] Add GPIO, I2C and SPI support for the RPi BSP

[Andre Marques]

Hello Pavel,

On 10/31/14 15:41, Pavel Pisa 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.
>

Yes currently it does not allow an user to specify an handler routine 
with parameters. Will check that.

>> +/**
>> + * @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.

Will definitely check this when possible and report back any questions.

> 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
>
>
>
>