[PATCH 1/2] RTEMS GPIO API definition and implementation.

[André Marques]

On 24-06-2015 15:03, Gedare Bloom wrote:
> On Tue, Jun 23, 2015 at 6:44 PM, André Marques
> <andre.lousa.marques at gmail.com> wrote:
>> On 23-06-2015 17:16, Gedare Bloom wrote:
>>>>>
>>>>>> +  uint32_t bank;
>>>>>> +  int handled_count;
>>>>>> +  int rv;
>>>>>> +
>>>>>> +  gpio = (gpio_pin*) arg;
>>>>>> +
>>>>>> +  bank = gpio->bank_number;
>>>>> Validate args for errors.
>>>>
>>>> These tasks are only created by the rtems_gpio_enable_interrupt()
>>>> function,
>>>> and all parameters are validated before creating/starting the task.
>>>> Should
>>>> they be validated again here?
>>>>
>>> No, as long as validated in the entry-ways to the module it should be
>>> fine.
>>>
>>>
>>>>>> +
>>>>>> +    AQUIRE_LOCK(bank_lock[bank]);
>>>>> What is the lock protecting?
>>>>
>>>> Since the rtems_gpio_disable_interrupt() kills this task when the
>>>> interrupt
>>>> is disabled, having the lock here (which rtems_gpio_disable_interrupt()
>>>> must
>>>> also acquire before doing anything) ensures that any interrupt disable
>>>> call
>>>> occurring during an interrupt process will have to wait while the
>>>> on-going
>>>> interrupt is processed. Otherwise the handler could leave the system on
>>>> an
>>>> inconsistent state. I shall make this clear with a comment.
>>>>
>>> Do we really want the task to be killed? Anyway, the naming of this
>>> "gpio_disable_interrupt" and "gpio_enable_interrupt" is a bit
>>> confusing to me, since it really is about creating/destroying the
>>> interrupt handling thread? I don't quite understand the design there.
>>
>> When an interrupt is enabled on a pin, a task is created to call its
>> handler/handlers (this task is put to sleep and woken by the bank's ISR each
>> time an interrupt on that pin is sensed). If that interrupt is disabled on
>> that pin, the task is killed because the pin will not be providing more
>> interrupts. An instance of this task is created for every pin with an
>> interrupt enabled.
>>
> Does this mean there is potentially one task for every pin? Why can't
> the task be shared? At least among the banks then you only need
> pins/32 max tasks.

One advantage of a task per pin I think would be on a SMP setup, where 
multiple cores could be processing interrupts from several pins at the 
same time. In pratice the number of tasks can be reduced to one task per 
bank, but they need to receive information of the bank's interrupt line 
status from the ISR (generic_isr) before it is cleared.

>>>> As for the gpio[0], the gpio pointer points to a whole gpio bank, so
>>>> gpio[0]
>>>> is the first pin of this bank. Since all pins in this bank have the same
>>>> bank number, I can use any pin from it to obtain the bank number that
>>>> this
>>>> ISR is listening. Maybe gpio should be renamed to gpio_bank.
>>>>
>>> Ah. Something bugs me about redundantly storing the same bank number
>>> 32 times... Is there a better data structure for you to use to keep
>>> track of all the pins and their metadata / configurations?
>>
>> There may be a few alternatives:
>>
>> 1. With the current bank*bank_pins matrix structure, since I always need to
>> have the bank and pin number to access a GPIO pin I can ommit these values
>> from the pin struct. This, however, requires about two divisions to
>> calculate the bank/pin number combination, but saves 64 bits per pin in
>> memory.
>>
> Generally not worth trading division for memory. Supposing 64 pins,
> we're talking about 512 bytes overhead totally. But, it would be good
> to get a grasp on the memory costs of your structures for tracking
> pins.

As in the memory required to track one pin?

>> 2. Use the uni-dimensional array with direct access with pin_number (see the
>> end of this e-mail), and keep the bank/pin data for each pin struct, but now
>> I do not have to calculate the bank/pin from pin_number.
>>
>> 3. The application could use the bank/pin number directly to refer to a pin,
>> instead of the global pin number. However, if the bsp wants to define a
>> series of constants for each pin, it would become more difficult since it
>> would have to store two numbers (#define GPIO_40 40 would not be possible),
>> so each platform can either not define any pin reference (each application
>> hardcodes the pin reference on their configuration), or it would have to
>> resort to a table of structs, probably facing this same problem.
>>
> May be better for the application interface to be opaque by using some
> kind of descriptor, either a pin number or an opaque type.
>
>> Maybe option 3 is better with the hardcoded reference to the bank/pin
>> combination, otherwise I feel like the only option becomes a choice between
>> 1 or 2, or moving the problem around. The usefulness of having platform
>> defined references to the pins may also be arguable.
>>
> Somehow an application has to know what pins to use. BSPs can provide
> some specific information, such as known interfaces (LEDs are a simple
> example). Ultimately though, it may be simplest for BSP writers to
> provide a document that maps between global pin numbers and pins on
> the board, so that users know where to attach wires.  Sorting this
> problem out is worthwhile, and might be good to discuss in a separate
> email where others might catch it.
>

Yes that is a good idea. I am also having some weird problems with the 
generic_isr that arised when I changed the pin struct (may be related to 
the struct size, but changing the logic_invert field from int to bool 
for instance causes the generic_isr to block everything, but if I do not 
use interrupts everything works fine), and in the way I seem to have 
found some problems with the rpi clock driver, so I may be posting some 
emails later today.

>>>>>> +
>>>>>> +  /* If the function was successfuly assigned to the pin,
>>>>>> +   * record that information on the gpio_pin_state structure. */
>>>>>> +  gpio_pin_state[bank][pin].pin_function = function;
>>>>>> +  gpio_pin_state[bank][pin].logic_invert = logic_invert;
>>>>>> +
>>>>>> +  if ( function == DIGITAL_OUTPUT ) {
>>>>>> +    if ( output_enabled == true ) {
>>>>>> +      sc = rtems_bsp_gpio_set(bank, pin);
>>>>> Shouldn't this also check logic_invert?
>>>>
>>>> If the application indicates that this pin is to be enabled upon request,
>>>> it
>>>> should be enabled (set) regardless of the behavior of the next set/clears
>>>> on
>>>> that pin (which may be inverted, depending on the logic_invert flag).
>>>> That
>>>> is how I see it.
>>>>
>>> I don't know. It's a bit funny. If the pin is low-out
>>> (logic_invert==true), then output_enabled means you want it pulled
>>> low, no?
>>
>> Since the API does not have platform code, if output_enabled is true then it
>> will always call gpio_set to have it logical high. If that platform uses
>> inverted logic then their implementation will do that. This logic_invert
>> would be a flag to switch the set and clear calls (a set would become a
>> clear, and vice-versa).
>>
> Can you give me an example of how this is used then? If the platform
> already inverts logic, why should the API layer?

The idea would be to invert the behavior of a pin on an applicational 
level. For instance I may have a switch connected to my system, and 
would like it to be always closed except when I am pressing it, or 
invert a specific led light pattern.

>>>>>> +rtems_status_code rtems_gpio_disable_interrupt(uint32_t pin_number)
>>>>>> +{
>>>>>> +  gpio_handler_list *isr_node;
>>>>>> +  rtems_vector_number vector;
>>>>>> +  rtems_status_code sc;
>>>>>> +  gpio_pin* gpio;
>>>>>> +  uint32_t bank;
>>>>>> +  uint32_t pin;
>>>>>> +
>>>>>> +  if ( pin_number < 0 || pin_number >= gpio_count ) {
>>>>>> +    return RTEMS_INVALID_ID;
>>>>>> +  }
>>>>>> +
>>>>>> +  bank = BANK_NUMBER(pin_number);
>>>>>> +  pin = PIN_NUMBER(pin_number);
>>>>>> +
>>>>>> +  vector = rtems_bsp_gpio_get_vector(bank);
>>>>>> +
>>>>>> +  AQUIRE_LOCK(bank_lock[bank]);
>>>>>> +
>>>>>> +  gpio = &gpio_pin_state[bank][pin];
>>>>>> +
>>>>>> +  if ( interrupt_counter[bank] == 0 || gpio->enabled_interrupt == NONE
>>>>>> )
>>>>>> {
>>>>>> +    RELEASE_LOCK(bank_lock[bank]);
>>>>>> +
>>>>>> +    return RTEMS_SUCCESSFUL;
>>>>>> +  }
>>>>>> +
>>>>>> +  sc = rtems_bsp_disable_interrupt(bank, pin,
>>>>>> gpio->enabled_interrupt);
>>>>>> +
>>>>>> +  if ( sc != RTEMS_SUCCESSFUL ) {
>>>>>> +    RELEASE_LOCK(bank_lock[bank]);
>>>>>> +
>>>>>> +    return RTEMS_UNSATISFIED;
>>>>>> +  }
>>>>>> +
>>>>>> +  gpio->enabled_interrupt = NONE;
>>>>>> +
>>>>>> +  while ( gpio->handler_list != NULL ) {
>>>>>> +    isr_node = gpio->handler_list;
>>>>>> +
>>>>>> +    gpio->handler_list = isr_node->next_isr;
>>>>>> +
>>>>>> +    free(isr_node);
>>>>>> +  }
>>>>>> +
>>>>>> +  sc = rtems_task_delete(gpio->task_id);
>>>>> So the handler task cannot be shared by more than one pin? Is this
>>>>> requirement explicit somewhere? Is this the intent?
>>>>
>>>> The handler task is to handle the (possible) many handlers an individual
>>>> pin
>>>> can have, if it acts as a shared IRQ line. What is shared across multiple
>>>> pins is the generic_isr, which is as actual ISR routine (shared with all
>>>> pins of a given bank).
>>>>
>>> I need to understand this handler task and generic_isr framework better.
>>
>> Each bank has a generic_isr, while each pin (with an active interrupt) has
>> an handler_task. Every time an interrupt is sensed on a bank, generic_isr
>> checks which pins from that bank have pending interrupts, wakes the
>> corresponding pin handler task, clears the interrupt line on that bank and
>> finishes. The actual action fired by the interrupt is done by the
>> handler_task, outside of ISR context.
>>
> Maybe each bank can also have a handler_task? As mentioned above.
> Having to configure tasks for all the possible pins in use seems
> excessive to me.
>
> Gedare