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

[Pavel Pisa]

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