[Bug 1001787] New: GPIO Interrupt Support for Kinetis

[Bug 1001787] New: GPIO Interrupt Support for Kinetis

[bugzilla-daemon]

Please do not reply to this email, use the link below.

http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001787

            Bug ID: 1001787
           Summary: GPIO Interrupt Support for Kinetis
           Product: eCos
           Version: CVS
            Target: All
  Architecture/Host Cortex-M
                OS:
            Status: UNCONFIRMED
          Severity: enhancement
          Priority: low
         Component: Patches and contributions
          Assignee: unassigned@bugs.ecos.sourceware.org
          Reporter: mjones@linear.com
                CC: ecos-patches@ecos.sourceware.org

Created attachment 2113
  --> http://bugs.ecos.sourceware.org/attachment.cgi?id=2113&action=edit
Support Kinetis GPIO interrupts and add set/get API

This patch adds to the Kinetis GPIO macros and API. It allows code to accept an
interrupt on a port by applying a configuration call, getting the interrupt,
and clearing the interrupt. I also added API for set/get of pins. The interrupt
is well tested on one port and pin. The set/get underlying macro was tested on
a pin before writing the API. The API is coded in a simple manner such that by
inspection if the original macros work, the API will work.

I don't have a mechanism to test interrupts on multiple pins/ports. So strictly
speaking, I can only say it is very robust using the example below. The code is
simple enough I feel save putting this code in CVS. However, if anyone has
hardware that would allow more testing, please do so if you have time.

If anyone is unhappy with the API, I am happy to make changes, retest, and send
a new patch.

An example:

#define ALERT_PORT 0
#define ALERT_PIN 19

cyg_uint32 alert_isr( cyg_vector_t vector,
                              cyg_addrword_t data
)
{
    cyg_interrupt_mask(CYGNUM_HAL_INTERRUPT_PORTA);
    hal_kinetis_gpio_interrupt_acknowledge(ALERT_PORT, ALERT_PIN);

    // Do a little critical work.

    return CYG_ISR_HANDLED | CYG_ISR_CALL_DSR;
}

void alert_dsr( cyg_vector_t vector,
                        cyg_ucount32 count,
                        cyg_addrword_t data
                      )
{
    cyg_flag_setbits(&alert_flag, 0x01);
    cyg_interrupt_unmask(CYGNUM_HAL_INTERRUPT_PORTA);
}



    cyg_interrupt_create(
            CYGNUM_HAL_INTERRUPT_PORTA, // Vector
            0xA0,                         // Priority
            (cyg_addrword_t)0,             // Data
            alert_isr,                     // ISR
            alert_dsr,                     // DSR
            &alert_handle,                 // Handle
            &alert_interrupt);            // INTR

    cyg_interrupt_attach(alert_handle);

    hal_kinetis_gpio_setup_port(ALERT_PORT, ALERT_PIN, 0xA, 0x1, 0x00);
    hal_kinetis_gpio_interrupt_acknowledge(ALERT_PORT, ALERT_PIN);
    cyg_interrupt_unmask(CYGNUM_HAL_INTERRUPT_PORTA);

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug 1001787] GPIO Interrupt Support for Kinetis

[bugzilla-daemon]

Please do not reply to this email, use the link below.

http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001787

Jonathan Larmour <jifl@ecoscentric.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #2113|0                           |1
           is patch|                            |
   Attachment #2113|application/octet-stream    |text/plain
          mime type|                            |

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug 1001787] GPIO Interrupt Support for Kinetis

[bugzilla-daemon]

Please do not reply to this email, use the link below.

http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001787

Ilija Kocho <ilijak@siva.com.mk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |ilijak@siva.com.mk
           Assignee|unassigned@bugs.ecos.source |ilijak@siva.com.mk
                   |ware.org                    |

--- Comment #1 from Ilija Kocho <ilijak@siva.com.mk> ---
Hi Mike

Thank you for your contribution. Now I have a little time to see your macros
and functions, but my ability for testing is limited.

Interrupt support:

To begin with, I should have added __irqc parameter to CYGHWR_HAL_KINETIS_PIN()
from the beginning.
CYGHWR_HAL_KINETIS_SETUP_PIN() corrects my mistake but I would avoid the
assignment operation in this macro in order to allow it's usage in arrays. It
is usually necesary to set several pins at once. Than we can define an array
and call macro HAL_SET_PINS() (See Ethernet, Flexbus and DDRAM drivers for use
cases). Therefore I propose following modification of your macro (then my
original macro will be a special case with __irqc = 0):

// Pin configuration descriptor PinCD
// PinCD is a 32 bit unsigned integer which contains configuration information
// for one pin. This macro produces PinCD based on following parameters:
//     __port - Port name, [A, F]
//     __bit  - Port bit [0, 31]
//     __mux  - Multiplexer index [0, 7]
//     __irqc - Interrupt configuration [0, F]
//     __cnf  - Configuration options

#define CYGHWR_HAL_KINETIS_PINCD(__port, __bit, __mux, __irqc, __cnf) \
    ((CYGHWR_HAL_KINETIS_PORT##__port << 20) | ((__bit) << 27)        \
     | CYGHWR_HAL_KINETIS_PORT_PCR_IRQC(__irqc)                        \
     | CYGHWR_HAL_KINETIS_PORT_PCR_MUX(__mux) | (__cnf))

// PinCD with disabled interrupts. (For backward compatibility)
#define CYGHWR_HAL_KINETIS_PIN(__port, __bit, __mux, __cnf) \
        CYGHWR_HAL_KINETIS_PIN_CNF(__port, __bit, __mux, 0, __cnf)

The interrupt acknowledge function should be designed with efficiency in mind
so I would avoid switch even though it requires direct coding instead of using
macros.

Bit manipulation functions: Having such macros already, bit manipulation
functions seem redundant to me. Is there a use case where we must use function
(rather than macro)?

This is all for now.

Ilija

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug 1001787] GPIO Interrupt Support for Kinetis

[bugzilla-daemon]

Please do not reply to this email, use the link below.

http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001787

--- Comment #3 from Ilija Kocho <ilijak@siva.com.mk> ---
Mike

(In reply to comment #2)
> Ilija,
> 
> I made and tested the following change. I can make a patch later if you
> agree with it. This should allow arrays.
> 
> On Interrupt Acknowledge, I wanted an interface that took an int for port i
> order to be consistent with the overall API, and with the datasheet, and
> with Assertions. However, a user could use the Macro directly like this:
> 
> CYGHWR_HAL_KINETIS_PORT_PCR_ISFR_CLEAR(A, 5);
> 
> This means the user can either choose the slower and safer API, or the Macro
> that already exists. I don't know if that is consistent with eCos principles
> or not, but this is why I made the choice. My feeling was a newbie would see
> the API and use it first. Then if they needed a little more performance,
> they would discover and use the MACRO. This is consistent with my design
> methodology, which is based on late optimization. However, I do recognize
> that embedded community may not operate on that principle as much as large
> system architects, which is my background.
> 
> Let me know what you think. I can make more changes if this is not
> satisfactory.
> 

I am conservative with introduction of new API functions because once API is
released people will start using it and we are stuck with that (Linus
Torvalds).
I think that the macros do the job and are intuitive enough. True we lack
asserts but some errors, such as non existing port are reported by the
compiler.
I would add functions some time to recheck. One problem is that the port
representation is different from macros (i know there's not help there). In
meantime we could focus on macros.

> Mike
> 
> Changes
> -------
> 
> #define CYGHWR_HAL_KINETIS_PIN_CFG(__port, __bit, __irqc, __mux, __cnf) \
>     ((CYGHWR_HAL_KINETIS_PORT##__port << 20) | ((__bit) << 27)        \
>      | CYGHWR_HAL_KINETIS_PORT_PCR_IRQC(__irqc)                        \
>      | CYGHWR_HAL_KINETIS_PORT_PCR_MUX(__mux) | (__cnf))
> 

CFG is probably more intuitive than PINCD - OK. However I would put __mux next
to __port and __bit and __irqc next to __cnf. Rationale: __port, __bit, __mux
triplet represents kind of /addressing parameters/ and __irqc, __cnf are
/configuration options/.

> #define CYGHWR_HAL_KINETIS_PIN(__port, __bit, __mux, __cnf) \
>         CYGHWR_HAL_KINETIS_PIN_CFG(__port, __bit, 0, __mux, __cnf)
> 

Based on your example I expected some code for ISR handling. May, but doesn't
have to be. Please check if your patch is complete.

Ilija

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug 1001787] GPIO Interrupt Support for Kinetis

[bugzilla-daemon]

Please do not reply to this email, use the link below.

http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001787

--- Comment #4 from Mike Jones <mjones@linear.com> ---
Ilija,

I ordered the parameters to correspond to the order in the registers on the
assumption that people will read the register manual to understand what they do
and think of them in that order. But I don't have any strong attachment to the
order, so I have changed to:


#define CYGHWR_HAL_KINETIS_PIN_CFG(__port, __bit, __mux, __irqc, __cnf) \
    ((CYGHWR_HAL_KINETIS_PORT##__port << 20) | ((__bit) << 27)        \
     | CYGHWR_HAL_KINETIS_PORT_PCR_IRQC(__irqc)                        \
     | CYGHWR_HAL_KINETIS_PORT_PCR_MUX(__mux) | (__cnf))

#define CYGHWR_HAL_KINETIS_PIN(__port, __bit, __mux, __cnf) \
        CYGHWR_HAL_KINETIS_PIN_CFG(__port, __bit, __mux, 0, __cnf)

__externC void 
hal_kinetis_gpio_setup_port(cyg_uint32 port, cyg_uint32 pin, cyg_uint8 mux,
cyg_uint8 irqc, cyg_uint8 config)
{
    CYG_ASSERT((port >= 0 && port <= 4), "GPIO ACK PORT must be 0-4");
    CYG_ASSERT((port >= 0 && port <= 31), "GPIO ACK PIN must be 0-31");
    switch(port) {
        case 0: hal_set_pin_function(CYGHWR_HAL_KINETIS_PIN_CFG(A, pin, mux,
irqc, config)); break;
        case 1: hal_set_pin_function(CYGHWR_HAL_KINETIS_PIN_CFG(B, pin, mux,
irqc, config)); break;
        case 2: hal_set_pin_function(CYGHWR_HAL_KINETIS_PIN_CFG(C, pin, mux,
irqc, config)); break;
        case 3: hal_set_pin_function(CYGHWR_HAL_KINETIS_PIN_CFG(D, pin, mux,
irqc, config)); break;
        case 4: hal_set_pin_function(CYGHWR_HAL_KINETIS_PIN_CFG(E, pin, mux,
irqc, config)); break;
        default: break;
    }
}    

The reason I did not recode the interrupt acknowledge was I could see no way to
avoid a conditional without a macro. Some logic has to decide on the register
to use and that has to either be conditional logic in code, or hard coding the
application via multiple API calls for each case, or a macro.

I would prefer not to use macros in application code for a bunch of reasons
that are not pure technical.

I always write application code using these preferences with 1 being the
highest preference:

1) Platform independent C/C++
2) Platform dependent C/C++
3) Macro

This tends to result in the smallest use of flash and in the case of gpio would
cause no performance issues, because I have no code that would ever read/write
gpio in loops, as I would always use SPI, I2C, UART, Ethernet and standard IO
peripherals for communication. If I needed very fast parallel performance, I
would try to find a way to memory map it. Also, gpio interrupts are very rare
events without a strong latency sensitivity.

I don't expect this to be true for every application, but the macros are always
available for those that need more performance. But in my case, memory is the
biggest constraint, not gpio. gpio performance is way done on the list of my
problems.

As for being conservative about new API, I'm with Faust, "be careful what you
ask for in youth, you may receive it in middle age."

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug 1001787] GPIO Interrupt Support for Kinetis

[bugzilla-daemon]

Please do not reply to this email, use the link below.

http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001787

--- Comment #5 from Ilija Kocho <ilijak@siva.com.mk> ---
Mike

If we are going to implement some GPIO API functions than why we don't accept
what's already there. I am looking STM32 where API functions use PIN descriptor
as a paremeyter rather han port/bit pair. It would be good if we have more or
less compatible API. The drawback is that CYGHWR_HAL_KINETIS_PIN_CFG() has more
parameters than we need for GPIO, but we can resolve that by defining another
macro.

Please look at STM32 _hal_stm32_gpio_xxx() functions and tell me what do you
think. In my view if we remove stm32 from name the names may be universal:

_hal_gpio_xxx();

Ilija

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug 1001787] GPIO Interrupt Support for Kinetis

[bugzilla-daemon]

Please do not reply to this email, use the link below.

http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001787

--- Comment #6 from Mike Jones <mjones@linear.com> ---
Ok, I will have a look. I would prefer a common API anyway so my app can
compile on both platforms.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug 1001787] GPIO Interrupt Support for Kinetis

[bugzilla-daemon]

Please do not reply to this email, use the link below.

http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001787

--- Comment #7 from Mike Jones <mjones@linear.com> ---
Ilija,

I will start comparing the existing STM32 API and the patch. Then I will
discuss my reason I made the API in the patch the way it is. Then I will
propose a solution that should satisfy two viewpoints, one the driver
writer/expert, and the other an application writer wanting simple/just-works.

STM32 has three functions:

__externC void hal_stm32_gpio_set( cyg_uint32 pin );
__externC void hal_stm32_gpio_out( cyg_uint32 pin, int val );
__externC void hal_stm32_gpio_in ( cyg_uint32 pin, int *val );

_set causes the driver to "setup" the pin. It calculates a port and bit from a
single number, sets speed, open-drain, etc. The "pin" is a bunch of bit fields,
so there is no structure to help the end user other than to use macros to set
the fields.

_out and _in let you set values on a single pin.

Now if you are a new user, you are immediately thrown into reverse engineering
of the code to figure out how to use it. You have to hunt down the file, and
look through the all the macros until you find the right ones to use and figure
out that "pin" is not a single digit. This is an aspect of GPIO macros I don't
particularly think helps application writers. I don't use the I2C interface
that way. I have generic C code that hopefully will work with all I2C
peripherals on all targets.

As for the API, there are no functions for setting words, which sometimes I
need because I need pins to change together from a single register write. So
this API is missing some capability I need.

Nonetheless, no intent or malice toward the STM32 author. My patch is no better
and was a hack to make it work the same day I needed it to work.

The Kinetis API in the patch is:

__externC void hal_kinetis_gpio_interrupt_acknowledge(cyg_uint32 port,
cyg_uint32 pin);
__externC void hal_kinetis_gpio_set_pin_in(cyg_uint32 port, cyg_uint32 pin);
__externC void hal_kinetis_gpio_set_pin_out(cyg_uint32 port, cyg_uint32 pin);
__externC void hal_kinetis_gpio_set_pin(cyg_uint32 port, cyg_uint32 pin);
__externC void hal_kinetis_gpio_clear_pin(cyg_uint32 port, cyg_uint32 pin);
__externC void hal_kinetis_gpio_get_pin(cyg_uint32 port, cyg_uint32 pin,
cyg_uint32* val);

__externC void hal_kinetis_gpio_put(cyg_uint32 port, cyg_uint32 val);
__externC void hal_kinetis_gpio_get(cyg_uint32 port, cyg_uint32* val);
__externC void hal_kinetis_gpio_clear(cyg_uint32 port, cyg_uint32* val);

__externC void hal_kinetis_gpio_setup_port(cyg_uint32 port, cyg_uint32 pin,
cyg_uint8 mode, cyg_uint8 mux, cyg_uint8 config);

These all take a port and pin. I can use these with no knowledge of macros. The
datasheet and my schematic has ports and pins written on them. I don't need to
look in any code files to know what to do (setup is an exception).

_interrupt_acknowledge is an artifact of the Kinetis unique interrupt
architecture. I think it is ugly because I can't use the standard interrupt
acknowledge. It only got my job done.

_set_pin_in and _set_pin_out set the direction of the pin. I find these names a
bit easy to confuse with _set_pin and I don't like them.

_set_pin, _clear_pin, and get_pin are all for setting and getting single pin
values.

_put, _get, _clear are for setting and getting all pins on a port. These are
also somewhat confusing and might be better as _put_port, etc.

_setup_port sets the attributes and this is ugly because it is Kinetis
specific, and you have to look up the proper values in the datasheet and
recognize the names of the variables in the data sheet.

So the first question I have is whether we should have a pin descriptor, as
asked? As an application writer, I don't see the value in it. I don't care if
my code configuring a port is a little slower. I only execute once, except
perhaps direction. I would prefer a universal API in C that is obvious without
sorting through a big file of macros trying to learn how to use the port. 

When I set pins/ports, I typically look at a schematic that has port/pin
numbers on it. Why would I want to setup a pin descriptor rather than just say
what I want the code to do? There is no value to me in carrying around a
descriptor full of information I don't care about. Even a moniker based API
hides this data behind the API. So please hide it from me. I don't care. I just
want it to work.

On the the other hand, not everyone may think of GPIO as a peripheral like I2C
where some conformity to a higher level API is important. Or perhaps the
expectation is that GPIO coding is really internal driver coding supporting
some other function with API. I have seen bit banged I2C like that. There is an
I2C C API with GPIO bit banging behind it.

I guess my perspective is that there are designs where there are small numbers
of GPIO pins that control things and don't require a "driver" to cover up the
code. For example, a baseboard controller might have some control pins that
turn supplies on or off and enable and disable loads. I see no reason to make a
driver for that. It is very application specific. I don't want my application
code to look like driver code.

So I am attempting to recognize there may be two views on this topic that
really reflect two different personas. A driver writer/orientation, and a
application writer or inexperienced newbie trying to make it work under high
schedule pressure. Enough pressure that if it does not work in a few days, eCos
is booted off the project, justified or not.

If I follow my own stated philosophy, the proposed API could be better and
generic. Perhaps:

// Kinetis specific
__externC void hal_kinetis_gpio_setup_port(cyg_uint32 port, cyg_uint32 pin,
cyg_uint8 mode, cyg_uint8 mux, 
__externC void hal_kinetis_gpio_interrupt_acknowledge(cyg_uint32 port,
cyg_uint32 pin);

// Generic

__externC void hal_gpio_set_pin_dir_in(cyg_uint32 port, cyg_uint32 pin);
__externC void hal_gpio_set_pin_dir_out(cyg_uint32 port, cyg_uint32 pin);

__externC void hal_gpio_set_pin(cyg_uint32 port, cyg_uint32 pin);
__externC void hal_gpio_clear_pin(cyg_uint32 port, cyg_uint32 pin);
__externC void hal_gpio_get_pin(cyg_uint32 port, cyg_uint32 pin, cyg_uint32*
val);

__externC void hal_gpio_port_set(cyg_uint32 port, cyg_uint32 val);
__externC void hal_gpio_port_clear(cyg_uint32 port, cyg_uint32 val);
__externC void hal_gpio_port_get(cyg_uint32 port, cyg_uint32* val);

I have tried here to create some naming symmetry between pin and port versions.
The direction is now more obvious. _setup_port might be better for the CDL and
have the kinetis API for times it has to be dynamic. These settings don't
change often. Interrupt acknowledge is just a problem. I have no idea how to
handle it in a more universal way.

So how to reconcile these two personas?

I want to propose the following:

The GPIO should support two programming models. One API for driver writers, and
one for application writers. The driver API should use macros and pin
descriptors, and be as consistent between targets as is reasonable without any
sacrifice of performance. The application API should be based on C, reflect
what one sees on datasheets and schematics, be universal, and supported by all
targets over time. Anyone that wants speed can always choose to use the macros
in their application and invest the learning time.

I think this implies the following:

1) The STM32 C API and the Kinetis C API need to be worked on so they match and
are universal.
2) The pin descriptor needs to be removed from the STM32 C API and used with
equivalent macros instead.
3) The STM32 and Kinetis macros need to evolve toward consistency over time.

In the short run, I think it is easier to agree on a universal C API and add it
to both platforms.

Ilija, have I convince you that a descriptor free C API has value?

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug 1001787] GPIO Interrupt Support for Kinetis

[bugzilla-daemon]

Please do not reply to this email, use the link below.

http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001787

Mike Jones <mjones@linear.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #2113|0                           |1
        is obsolete|                            |

--- Comment #8 from Mike Jones <mjones@linear.com> ---
Created attachment 2260
  --> http://bugs.ecos.sourceware.org/attachment.cgi?id=2260&action=edit
GPIO Macro Patch

After discussion with Ilija, we agreed it would be best to release just the
macros and follow with C functions at a later date when there was more time to
improve them. The main issue with the original patch is the C functions result
in larger and slower running code.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug 1001787] GPIO Interrupt Support for Kinetis

[bugzilla-daemon]

Please do not reply to this email, use the link below.

http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001787

Ilija Kocho <ilijak@siva.com.mk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|---                         |CURRENTRELEASE

--- Comment #9 from Ilija Kocho <ilijak@siva.com.mk> ---
Checked in.

Thanks to Mike for the contribution.
Ilija

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug 1001787] GPIO Interrupt Support for Kinetis

[bugzilla-daemon]

Please do not reply to this email, use the link below.

http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001787

Ilija Kocho [Илија Кочо] <ilijak@siva.com.mk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |CLOSED

-- 
You are receiving this mail because:
You are on the CC list for the bug.