[PATCH 1/2] RTEMS GPIO API definition and implementation.
André Marques
andre.lousa.marques at gmail.com
Tue Jun 23 22:44:38 UTC 2015
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.
>
>>>> + if ( handled_count == 0 ) {
>>>> + bsp_interrupt_handler_default(rtems_bsp_gpio_get_vector(bank));
>>>> + }
>>>> +
>>>> + RELEASE_LOCK(bank_lock[bank]);
>>>> + }
>>>> +}
>>>> +
>>>> +static void generic_isr(void* arg)
>>>> +{
>>>> + rtems_vector_number vector;
>>>> + uint32_t event_status;
>>>> + uint32_t bank_pin_count;
>>>> + gpio_pin *gpio;
>>>> + uint32_t bank_number;
>>>> + int i;
>>>> +
>>>> + gpio = (gpio_pin *) arg;
>>>> +
>>>> + /* Get the bank/vector number of a pin (e.g.: 0) from this bank. */
>>>> + bank_number = gpio[0].bank_number;
>>>> +
>>> Again, validate args. Why is it gpio[0] here? And not above?
>>
>> As with generic_handler_task, these ISRs are only created by
>> rtems_gpio_enable_interrupt() and rtems_gpio_disable_interrupt(), which
>> validate the arguments sent here. Should they be re-validated here?
> This is a little different, in that the code path gets reached
> indirectly via interrupt handling. I recommand always verifying these
> inputs, even if it is with debug asserts only.
Noted.
>> 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.
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.
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.
>
>>>> +
>>>> + /* Obtains a 32-bit bitmask, with the pins currently reporting
>>>> interrupts
>>>> + * signaled with 1. */
>>>> + event_status = rtems_bsp_gpio_interrupt_line(vector);
>>>> +
>>> Is it ever the case there are more than 32 pins in a bank? (If not,
>>> this should be assert()ed somewhere in the code.)
>>
>> There may be bigger (up to 64?). But yes it would help to define a max pin
>> number for a single bank, and have that asserted. Would it be best to set a
>> max pin number per bank of 64 pins, and change this to a 64-bit bitmask to
>> account for any number in-between? The reasoning behind the pin bitmask is
>> that most platforms should be able to directly (or after some bitwise
>> operations) write it to the registers.
>>
> Stick to 32.
>
>>>> + /* Iterates through the bitmask and calls the corresponding handler
>>>> + * for active interrupts. */
>>>> + for ( i = 0; i < bank_pin_count; ++i ) {
>>>> + /* If active, wake the corresponding pin's ISR task. */
>>>> + if ( event_status & (1 << i) ) {
>>>> + rtems_event_send(gpio[i].task_id, RTEMS_EVENT_1);
>>> It may improve readability to #define something as RTEMS_EVENT_1. like
>>> #define GPIO_INTERRUPT_ACTIVE RTEMS_EVENT_1.
>>
>> maybe GPIO_INTERRUPT_EVENT?
>>
> Yes.
>
>>>> + }
>>>> +
>>>> + bitmask = 0;
>>>> +
>>>> + for ( i = 0; i < count; ++i ) {
>>>> + pin_number = va_arg(args, uint32_t);
>>>> +
>>>> + if ( pin_number < 0 || pin_number >= gpio_count ) {
>>>> + *sc = RTEMS_INVALID_ID;
>>>> +
>>>> + return 0;
>>>> + }
>>>> +
>>>> + bank = BANK_NUMBER(pin_number);
>>> If you already know the bank_number, you can replace this with
>>> if(BANK_NUMBER(pin_number) != bank_number) return RTEMS_UNSATISFIED;
>>
>> I never know the bank number until the first iteration ( the i == 0 case
>> bellow).
>>
> You can pre-compute this before the loop.
>
>
>
>
>>>> +
>>>> + return RTEMS_UNSATISFIED;
>>>> + }
>>>> +
>>>> + sc = rtems_gpio_resistor_mode(conf->pin_number, conf->pull_mode);
>>>> +
>>>> + if ( sc != RTEMS_SUCCESSFUL ) {
>>>> + RTEMS_SYSLOG_ERROR("rtems_gpio_resistor_mode failed with status code
>>>> %d\n", sc);
>>>> +
>>>> + return RTEMS_UNSATISFIED;
>>>> + }
>>>> +
>>>> + interrupt_conf = (rtems_gpio_interrupt_conf*) conf->interrupt;
>>>> +
>>>> + if ( interrupt_conf != NULL ) {
>>>> + bank = BANK_NUMBER(conf->pin_number);
>>>> + pin = PIN_NUMBER(conf->pin_number);
>>> Why doesn't the conf store these, anyway?
>>
>> The API calculates the pin and bank numbers from the "global" pin number as
>> a protection mechanism against a change in the GPIO layout. On a second
>> thougth, I do not actually see a reason for it to change, unless there is an
>> application where I would like to work with banks instead of individual
>> pins, and would change the GPIO layout accordingly. But even in that case I
>> could just create a separate conf file with those specific requirements (and
>> maybe it can allow to define multiple pins with the same conf, to form a
>> group that is to be treated as a single entity, if that can be a thing?).
> Using configuration structs to abstract the pins/banks/groups-of-pins
> makes sense to me. Having an efficient way to map from the pin number
> to its configuration could solve the dilemma you face about wanting to
> keep things simple. A linear index (array) works fine for 1:1
> pin-to-conf. A tree (rbtree) works for sparse mappings. Or rely on the
> application to keep track of the confs itself.
>
>> One issue that arises with storing the bank and pin numbers directly is that
>> it becomes more hardcoded. With a global pin number a bsp can define a pin
>> with
>>
>> #define GPIO_40 40
>>
>> and use that to refer to that pin. By having to explicitly use the bank and
>> pin numbers, I would have two numbers that would be hardcoded (and the
>> separate conf file would apply here).
> Or the application passes the configuration instead of the numbers.
> Then the configuration can store things like pin/bank numbers.
>
>> So yes, probably the bank and pin numbers can be directly stored. The other
>> API calls can also receive the two numbers, or at least the
>> rtems_gpio_request_pin could. If this function (and rtems_gpio_request_conf)
>> would return a gpio_pin_id var/struct with the bank and pin number
>> combination, the following calls would only use this gpio_pin_id, and could
>> even avoid the bank/pin check if this type is opaque.
>>
>>>> +
>>>> + AQUIRE_LOCK(bank_lock[bank]);
>>> And also store its own lock field?
>>
>> The lock field is per bank. Each conf struct only refer to an individual
>> pin.
>>
> OK, or possibly a group of pins, fine. It could store a pointer to the
> lock maybe, but this global array of locks is I suppose acceptable.
>
>
>>>> +int rtems_gpio_get_value(uint32_t pin_number)
>>>> +{
>>>> + uint32_t bank;
>>>> + uint32_t pin;
>>>> + int rv;
>>>> +
>>>> + if ( pin_number < 0 || pin_number >= gpio_count ) {
>>>> + return -1;
>>>> + }
>>>> +
>>>> + bank = BANK_NUMBER(pin_number);
>>>> + pin = PIN_NUMBER(pin_number);
>>>> +
>>>> + AQUIRE_LOCK(bank_lock[bank]);
>>>> +
>>>> + if ( gpio_pin_state[bank][pin].pin_function != DIGITAL_INPUT) {
>>>> + RELEASE_LOCK(bank_lock[bank]);
>>>> +
>>>> + RTEMS_SYSLOG_ERROR("Can only read digital input pins\n");
>>>> +
>>>> + return -1;
>>>> + }
>>>> +
>>>> + rv = rtems_bsp_gpio_get_value(bank, pin);
>>>> +
>>>> + if ( gpio_pin_state[bank][pin].logic_invert && rv > 0 ) {
>>> why rv > 0?
>>
>> The rtems_bsp_gpio_get_value can return -1 if for some reason it failed to
>> get a value. With the rv > 0 condition that would be sent to an application
>> through the last return.
>>
> So in case of an error, the lock remains held? What does rv==0 signify?
If rtems_bsp_gpio_get_value() has an error it should return -1 and then
the lock is released below. In success that function should return 0 (rv
== 0) if the pin level is currently logical low, or a number higher than
0 if the pin level is a logical high.
>>>> + RELEASE_LOCK(bank_lock[bank]);
>>>> +
>>>> + return !rv;
>>>> + }
>>>> +
>>>> + RELEASE_LOCK(bank_lock[bank]);
>>>> +
>>>> + return ( rv > 0 ) ? 1 : rv;
>>>> +}
>>>> +
>>>> +rtems_status_code rtems_gpio_request_pin(uint32_t pin_number,
>>>> rtems_gpio_function function, bool output_enabled, bool logic_invert, void*
>>>> bsp_specific)
>>>> +{
>>>> + rtems_gpio_specific_data* bsp_data;
>>>> + rtems_status_code sc = RTEMS_SUCCESSFUL;
>>>> + 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);
>>>> +
>>>> + AQUIRE_LOCK(bank_lock[bank]);
>>>> +
>>>> + /* If the pin is already being used returns with an error. */
>>>> + if ( gpio_pin_state[bank][pin].pin_function != NOT_USED ) {
>>>> + RELEASE_LOCK(bank_lock[bank]);
>>>> +
>>>> + return RTEMS_RESOURCE_IN_USE;
>>>> + }
>>>> +
>>>> + switch ( function ) {
>>>> + case DIGITAL_INPUT:
>>>> + sc = rtems_bsp_gpio_select_input(bank, pin, bsp_specific);
>>>> + break;
>>>> + case DIGITAL_OUTPUT:
>>>> + sc = rtems_bsp_gpio_select_output(bank, pin, bsp_specific);
>>>> + break;
>>>> + case BSP_SPECIFIC:
>>>> + bsp_data = (rtems_gpio_specific_data*) bsp_specific;
>>>> +
>>>> + if ( bsp_data == NULL ) {
>>>> + RELEASE_LOCK(bank_lock[bank]);
>>>> +
>>>> + return RTEMS_UNSATISFIED;
>>>> + }
>>>> +
>>>> + sc = rtems_bsp_select_specific_io(bank, pin,
>>>> bsp_data->io_function, bsp_data->pin_data);
>>>> + break;
>>>> + case NOT_USED:
>>>> + default:
>>>> + RELEASE_LOCK(bank_lock[bank]);
>>>> +
>>>> + return RTEMS_NOT_DEFINED;
>>>> + }
>>>> +
>>>> + if ( sc != RTEMS_SUCCESSFUL ) {
>>>> + RELEASE_LOCK(bank_lock[bank]);
>>>> +
>>>> + return sc;
>>>> + }
>>>> +
>>>> + /* 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).
>
>>>> +rtems_status_code rtems_gpio_interrupt_handler_install(
>>>> +uint32_t pin_number,
>>>> +rtems_gpio_irq_state (*handler) (void *arg),
>>>> +void *arg
>>>> +)
>>>> +{
>>>> + gpio_handler_list *isr_node;
>>>> + 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);
>>>> +
>>>> + AQUIRE_LOCK(bank_lock[bank]);
>>>> +
>>>> + gpio = &gpio_pin_state[bank][pin];
>>>> +
>>>> + /* If the current pin has no interrupt enabled
>>>> + * then it does not need an handler. */
>>>> + if ( gpio->enabled_interrupt == NONE ) {
>>>> + RELEASE_LOCK(bank_lock[bank]);
>>>> +
>>>> + return RTEMS_NOT_CONFIGURED;
>>>> + }
>>>> + /* If the pin already has an enabled interrupt but the installed
>>>> handler
>>>> + * is set as unique. */
>>>> + else if ( gpio->handler_flag == UNIQUE_HANDLER && gpio->handler_list
>>>> != NULL ) {
>>>> + RELEASE_LOCK(bank_lock[bank]);
>>>> +
>>>> + return RTEMS_RESOURCE_IN_USE;
>>>> + }
>>>> +
>>>> + /* Update the pin's ISR list. */
>>>> + isr_node = (gpio_handler_list *) malloc(sizeof(gpio_handler_list));
>>>> +
>>>> + if ( isr_node == NULL ) {
>>>> + RELEASE_LOCK(bank_lock[bank]);
>>>> +
>>>> + return RTEMS_NO_MEMORY;
>>>> + }
>>>> +
>>>> + isr_node->handler = handler;
>>>> + isr_node->arg = arg;
>>>> +
>>>> + if ( gpio->handler_flag == SHARED_HANDLER ) {
>>>> + isr_node->next_isr = gpio->handler_list;
>>>> + }
>>>> + else {
>>>> + isr_node->next_isr = NULL;
>>> Isn't this the same thing as gpio->handler_list in case of
>>> !SHARED_HANDLER?
>>
>> Indeed the gpio->handler_list is always NULL if empty, so yes this if block
>> can be replaced with
>>
>> isr_node->next_isr = gpio->handler_list;
>>
>> Not sure if that would be more readable.
>>
> Realistically you should have helper functions for manipulating these
> lists, which would make the code eminently more readable. Or use RTEMS
> chains.
Yes, rtems chains should be better.
>>>> +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.
>>>> +
>>>> +/**
>>>> + * @brief Object containing relevant information for assigning a BSP
>>>> specific
>>>> + * function to a pin.
>>>> + *
>>>> + * Encapsulates relevant data for a BSP specific GPIO function.
>>>> + */
>>>> +typedef struct
>>>> +{
>>>> + /* The bsp defined function code. */
>>>> + uint32_t io_function;
>>>> +
>>>> + void* pin_data;
>>>> +} rtems_gpio_specific_data;
>>>> +
>>>> +/**
>>>> + * @brief Object containing relevant information about a BSP's GPIO
>>>> layout.
>>>> + */
>>>> +typedef struct
>>>> +{
>>>> + /* Total number of GPIO pins. */
>>>> + uint32_t pin_count;
>>>> +
>>>> + /* Number of pins per bank. The last bank may be smaller,
>>>> + * depending on the total number of pins. */
>>>> + uint32_t pins_per_bank;
>>> Maybe use an array instead. This can also reduce the need for all the
>>> arithmetic computing bank and pin from pin_number.. if you can just
>>> index directly into a struct containing all that information? What
>>> would be the expected cost in size to do it like this?
>>
>> You mean to replace the rtems_gpio_layout with an array of structs
>> statically filled with the bank/pin combinations by each BSP? That does not
>> seem very clean, specially since each all banks have the same number of pins
>> (the last is the only possible exception), and the layout calculation is
>> pretty easy/cheap.
>>
>> One way to improve the bank/pin situation would, maybe, be as follows:
>>
>> Replace the gpio_pin **gpio_pin_state matrix with an uni-dimensional array,
>> such that (suposing pins_per_bank = 32) from the index 0 to 31 it would
>> refer to pins in the first bank, from index 32 to 63 to pins from the second
>> bank, and so on. This would allow to fetch the right GPIO pin using the
>> pin_number directly.
>>
>> For the cases where I need to send a whole bank of pins, I could simply send
>> a pointer to the index of the first pin of that bank. Then since I know how
>> many pins each bank has I always know where the bank ends.
>>
> That might make sense. I'm just trying to think through the useful
> options here. Before we get stuck with something in the API layer that
> is inefficient to use.
>
> Gedare
More information about the devel
mailing list