On 24-06-2015 15:03, Gedare Bloom wrote:
On Tue, Jun 23, 2015 at 6:44 PM, André Marques
<andre.lousa.marq...@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
_______________________________________________
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel
