search

zephyrproject-rtos / zephyr

Primary Git Repository for the Zephyr Project. Zephyr is a new generation, scalable, optimized, secure RTOS for multiple hardware architectures.
https://docs.zephyrproject.org
Apache License 2.0
10.85k stars 6.61k forks source link

gpio/pinctrl: GPIO and introduce PINCTRL API to support gpio, pinctrl DTS nodes #15611

Closed mnkp closed 4 years ago

mnkp commented 5 years ago

Introduction

As a general requirement we want Zephyr DTS to adhere to the specification as well as to use bindings compatible with those of the Linux kernel. To support gpio, pinctrl DTS node bindings the current GPIO driver needs to be reworked. We also need to introduce PINCTRL driver.

A GPIO driver is used to configure and control the pins directly by the application. A PINCTRL driver is used to configure the pins and route the signals to the internal hardware modules on the SoC.

Requirement to support gpio, pinctrl DTS node bindings used by the Linux kernel doesn't imply that we have to provide the same GPIO, PINCTRL API like Linux does. In fact, the APIs can be quite different. We only need to process configuration data provided by the gpio, pinctrl DTS nodes in an easy and convenient manner.

Problem description

Unlike virtually any other driver e.g. I2C, RTC, CAN, UART which functionality is confined to a single hardware module the pin configuration and control functions can be spread among multiple SoC components. The implementation can vary significantly between vendors or even SoC families.

As an example both Intel Quark and NXP/Freescale K64 have separate GPIO and pinctrl modules however functionality assigned to each module by the respective SoC family is quite different. I.e. in one case debouncing is controlled by GPIO in the other by pinctrl module.

Pin functions controlled by

Intel Quark GPIO module: value, direction, interrupts, debouncing, source (pio/ext) Intel Quark pinctrl module: muxing, pullup, slew rate

K64 GPIO module: value, direction K64 pinctrl module: muxing, pullup/pulldown, slew rate, open drain, drive strength, interrupts, debouncing, source (pio/ext)

Proposed change

The driver API should hide implementation details and make it possible to write portable code. I.e. it would be impractical to expect user writing application code to know if they need to use PINCTRL or GPIO driver to configure an output as an open drain. As such both GPIO and PINCTRL API should provide means to fully configure pin properties such as open drain, drive strength, input debounce, pull up or schmitt-trigger mode.

Update GPIO and introduce PINCTRL API to let Zephyr drivers and application code configure pins using data provided by gpio, pinctrl DTS nodes. The API should use naming convention / configuration options which directly relate to the gpio, (gpio.h) and pinctrl bindings defined by Linux DTS. Zephyr implementation should preserve the meaning of Linux defined configuration options. E.g. relation between DTS pinctrl node property drive-open-drain and corresponding feature of the Zephyr API should be straightforward to identify and have the same effect.

Detailed RFC

GPIO/PINCTRL driver should use the following flags as a parameter to *_configure function:

Linux DTS does not specify interrupt configuration within GPIO/PINCTRL node but rather a dedicated IRQ chip node. Zephyr GPIO API shall provide means to directly configure interrupts. Following base flags are proposed:

To make interrupt configuration easier for the end user the following convenience flags are proposed:

Where each of the convenience flags is defined as a combination of a base flag. E.g. 

/** Trigger GPIO pin interrupt on level high. */
#define GPIO_INT_LEVEL_HIGH     (GPIO_INT_ENABLE  \
                 | GPIO_INT_LEVEL \
                 | GPIO_INT_HIGH)

New API functions

Following two functions are added to let the user directly control the pin level ignoring GPIO_ACTIVE_LOW flag. Writing value 1 to a pin will always set it to high output state, writing value 0 will always set it to low output state.

int gpio_pin_write_raw(struct device *port, u32_t pin, u32_t value);
int gpio_pin_read_raw(struct device *port, u32_t pin, u32_t *value);

Pin Controller API: TBD

Dependencies

When a new PINCTRL API is introduced the current PINMUX API can be deprecated.

Concerns and Unresolved Questions

The GPIO and PINCTRL drivers will significantly overlap their functionality since many driver features (e.g. pin drive strength, pull-up, pull-down, ability to configure output as on open source) need to be provided by both drivers. A better choice may be merging the two APIs into one.

pabigot commented 5 years ago

Pull-up and pull-down input configuration flags appear to be missing.

GPIO_DIR_OUT_LOW/HIGH and GPIO_OUTPUT_INIT_LOW/HIGH appear to be redundant.

Some devices (Nordic GPIO, Semtex SX1509B) support drive strength configuration with very different capabilities and defaults. Slew rate was also mentioned. Unless every potential hardware capability can be anticipated with a generic API, there should be some mechanism to pass driver-implementation-specific flags along with the standard ones so vendor extensions can be controlled by applications that are aware of the underlying hardware.

It's unclear whether the flags used in the device-tree gpios composite's flags field are to be the same as the corresponding argument to a GPIO driver configuration function, or if some of the flags may only relevant for either device-tree or dynamic configuration.

The rework should clearly define at what point the flags specified in the device tree will be applied: when the GPIO driver is initialized (e.g. to come up in a low-power mode) or when whatever uses the corresponding GPIO is initialized (which would likely be a very different configuration). It may be useful to provide multiple sets of configuration flags for a specific GPIO, perhaps by leveraging the PINMUX concept.

Functions in the new GPIO and PINCTRL driver API should clearly document exactly what is meant by each flag, and exactly what behavior is expected from drivers that are given flags that represent a configuration that is not supported by the hardware (ignore, error, make up something, ...). The existing solution failed to do this, and the results have been unsatisfactory.

b0661 commented 5 years ago

GPIO_ACTIVE_LOW, GPIO_ACTIVE_HIGH: indicate pin active state (logical value '1' in low state or logical value '1' in high state)

If this is the output configuration it should be renamed to GPIO_OUTPUT_ACTIVE_LOW/HIGH to avoid any misinterpretation.

GPIO_OPEN_DRAIN, GPIO_OPEN_SOURCE: configure single ended pin driving mode

How is push/pull operation configured?

GPIO_INPUT_DEBOUNCE: enable pin debounce circuitry (the debounce parameters, if any, should be set by a SoC specific DTS)

Why not have a set of generic debounce times that may fit a lot of SoCs (e.g. DEBOUNCE_NONE, DEBOUNCE_SHORT, DEBOUNCE_MEDIUM, DEBOUNCE_LONG). What short/ medium/ long means is dependent on the SoC and may even be specified in the DTS.

Linux DTS does not specify interrupt configuration within GPIO/PINCTRL node but rather a dedicated IRQ chip node. Zephyr GPIO API shall provide means to directly configure interrupts. Following flags are proposed:

GPIO_INT_EDGE_RISING
GPIO_INT_EDGE_FALLING
GPIO_INT_EDGE_BOTH
GPIO_INT_LEVEL_LOW
GPIO_INT_LEVEL_HIGH

GPIO_INT_EDGE_BOTH is just a waste of configuration space, it should be the combination of the GPIO_INT_EDGE_RISING and GPIO_INT_EDGE_FALLING flags.

mnkp commented 5 years ago

Pull-up and pull-down input configuration flags appear to be missing.

Added to the issue description.

GPIO_DIR_OUT_LOW/HIGH and GPIO_OUTPUT_INIT_LOW/HIGH appear to be redundant.

I'm not sure how they made it there. Removed.

Some devices (Nordic GPIO, Semtex SX1509B) support drive strength configuration with very different capabilities and defaults. Slew rate was also mentioned. Unless every potential hardware capability can be anticipated with a generic API, there should be some mechanism to pass driver-implementation-specific flags along with the standard ones so vendor extensions can be controlled by applications that are aware of the underlying hardware.

I think we need to discuss drive strength, slew rate and debouncing configuration a bit more. In Linux DTS these are configured with a parameter.

If we want to stay true to the promise that Zephyr DTS description is compatible with Linux DTS then we would need to configure these parameters via a dedicated functions taking respected parameter as an argument. The GPIO_INPUT_DEBOUNCE flag would need to be removed. Setting drive strength in mA doesn't seem to be very practical, on the other hand that's probably the only way to do it in a generic way. We could provide some SoC specific convenience macros that would encode available drive strengths in mA as an easy to understand name.

It's unclear whether the flags used in the device-tree gpios composite's flags field are to be the same as the corresponding argument to a GPIO driver configuration function, or if some of the flags may only relevant for either device-tree or dynamic configuration.

The DTS configuration has to follow the rules specified in GPIO bindings. These are documented in Linux DTS GPIO bindings.

The rework should clearly define at what point the flags specified in the device tree will be applied: when the GPIO driver is initialized (e.g. to come up in a low-power mode) or when whatever uses the corresponding GPIO is initialized (which would likely be a very different configuration). It may be useful to provide multiple sets of configuration flags for a specific GPIO, perhaps by leveraging the PINMUX concept.

According to the Linux DTS automatic configuration of GPIO pins is achieved by means of gpio-hog property. 

The GPIO chip may contain GPIO hog definitions. GPIO hogging is a mechanism
providing automatic GPIO request and configuration as part of the
gpio-controller's driver probe function.

That's independent from GPIO API but surly needs to be clarified.

Functions in the new GPIO and PINCTRL driver API should clearly document exactly what is meant by each flag, and exactly what behavior is expected from drivers that are given flags that represent a configuration that is not supported by the hardware (ignore, error, make up something, ...). The existing solution failed to do this, and the results have been unsatisfactory.

Yes, fully agree. This should be taken care of in the PR.

mnkp commented 5 years ago

GPIO_ACTIVE_LOW, GPIO_ACTIVE_HIGH: indicate pin active state (logical value '1' in low state or logical value '1' in high state)

If this is the output configuration it should be renamed to GPIO_OUTPUT_ACTIVE_LOW/HIGH to avoid any misinterpretation.

The flag is honored by gpio_pin_write and gpio_pin_read functions. I've clarified the description.

GPIO_OPEN_DRAIN, GPIO_OPEN_SOURCE: configure single ended pin driving mode

How is push/pull operation configured?

By default (no flags) output is configured in push/pull mode. I've updated the description.

Why not have a set of generic debounce times that may fit a lot of SoCs (e.g. DEBOUNCE_NONE, DEBOUNCE_SHORT, DEBOUNCE_MEDIUM, DEBOUNCE_LONG). What short/ medium/ long means is dependent on the SoC and may even be specified in the DTS.

I discuss the configuration of debounce time in the previous comment.

Linux DTS does not specify interrupt configuration within GPIO/PINCTRL node but rather a dedicated IRQ chip node. Zephyr GPIO API shall provide means to directly configure interrupts. Following flags are proposed:

GPIO_INT_EDGE_RISING
GPIO_INT_EDGE_FALLING
GPIO_INT_EDGE_BOTH
GPIO_INT_LEVEL_LOW
GPIO_INT_LEVEL_HIGH

GPIO_INT_EDGE_BOTH is just a waste of configuration space, it should be the combination of the GPIO_INT_EDGE_RISING and GPIO_INT_EDGE_FALLING flags.

Indeed, that was not well explained. I propose to have to have two sets of flags. The base set used by the drivers, fully sufficient to configure an interrupt. Since their usage may not be intuitive to the end user / may involve too much typing the second set are convenience flags. This is a single identifier with a straightforward name which combines and may be used in place of a set of base flags. I've updated the description.

anangl commented 5 years ago

@mnkp I am still concerned about the flags GPIO_ACTIVE_* and their influence on gpio_pin_write and gpio_pin_read functions. As I already mentioned in https://github.com/zephyrproject-rtos/zephyr/pull/12651#discussion_r254328542, I think it would be better to keep the current behavior of gpio_pin_write and gpio_pin_read, and to handle these flags outside of GPIO drivers. My main concern is the inconsistency between reading/writing the state of pins and configuring interrupts for them (I assume from your previous comment in this topic that interrupt triggers should still operate only on physical input levels). I think this will be just confusing for people - one will configure an interrupt to be generated on the raising edge on a pin, and the state reported for this pin in the interrupt handler will be either 1 or 0, depending on the GPIO_ACTIVE_* flag used in this pin configuration. And to properly configure the interrupt to occur when the input changes its state to active, one would still need to use the same GPIO_ACTIVE_* flag that was used when the pin was configured (and a piece of code that will handle it, so probably it would be convenient to provide a wrapper for this in GPIO API).

You replied to my comment:

I provided a bit more elaborate answer in my comment in #11880. In short GPIO_ACTIVE_LOW is just a different name for GPIO_POL_INV flag which controls the value returned by gpio_pin_write and gpio_pin_read functions.

I am not convinced that GPIO_ACTIVE_LOW in its current form is just a replacement for GPIO_POL_INV. As I understand it, the inversion of polarity should affect a given pin in all aspects, including the configuration of interrupts, just as if there was a hardware inverter connected to the pin. I tried to look at how the polarity inversion is handled in hardware currently supported by Zephyr's GPIO drivers:

And now I'm really puzzled regarding what would be the best way to handle GPIOs polarity inversion. 🙂 Could anyone provide other examples of hardware supporting this feature, or provide any other hints?

Anyway, I still think that the influence of the GPIO_ACTIVE_LOW flag on the behavior of gpio_pin_write and gpio_pin_read functions should be reconsidered.

mbolivar commented 5 years ago

Hi @mnkp, I wanted to clarify my remarks about fast access functions during today's meeting.

I think that the _raw variants are good and should be included. The issue I am considering is a separate one that would be an enhancement to your existing work.

My main point is that it would also be useful for other device drivers that need fast access to GPIOs built into the SoC to have a generic API for accessing the output registers for these devices.

In other words, something like this:

struct device *gpio_port = device_get_binding(...);
u32_t *output_register;
u32_t my_bit = 3U;

gpio_get_output_register(gpio_port, &output_register);
*output_register |= (1U << my_bit);

To be equivalent to:

struct device *gpio_port = device_get_binding(...);
gpio_pin_write_raw(gpio_port, 3U, 1U);

except without the overhead, so that the bit manipulation on output_register could be done in a tight loop.

The inspiration is from the FastLED library's pin abstraction (https://github.com/FastLED/FastLED/blob/master/fastpin.h; also see PORTING.md in the same repository), which works on a variety of Arm SoCs, ESPs, and AVRs at the very least.

Of course, there are problems to consider:

Your thoughts are welcome. Thanks.

mnkp commented 5 years ago

@mbolivar thanks for the proposal.

I was hoping that removing the access_op from GPIO API would improve things significantly. It does improve things but there is still a fair share of pointer dereferencing going on. To test it I've implemented gpio_pin_write_raw on SAM E70 (ARM Cortex-M7) SoC which is a very friendly chip to work with. 

static int gpio_sam_pin_write_raw(struct device *dev, u32_t pin, u32_t value)
{
    const struct gpio_sam_config * const cfg = DEV_CFG(dev);
    Pio *const pio = cfg->regs;

    if (value) {
        /* Set the pin. */
        pio->PIO_SODR = 1 << pin;
    } else {
        /* Clear the pin. */
        pio->PIO_CODR = 1 << pin;
    }
    return 0;
}

with default Zephyr compiler optimization options the implementation takes 11 assembly instructions (for comparison the gpio_sam_pin_write function takes 32 assembly instructions. Since there is also some overhead for calling the function to have truly fast GPIO access we definitely need to considered the type of API you mentioned.

I believe though that letting an application directly access the output_register will not be possible. There are SoCs that provide only output_set, output_clear registers. There are SoCs that provide only output_write register. There are SoCs that provide both. The internal layout of the registers is also not always consistent. We would probably need a set of inline functions which would present a consistent API but be implemented individually by the SoC. Similar to your proposal but writing to the register would happen via an inline function. We would need to rely on CMake to include the right header (and in this case also implementation) file.

mbolivar commented 5 years ago

I believe though that letting an application directly access the output_register will not be possible. There are SoCs that provide only output_set, output_clear registers. There are SoCs that provide only output_write register. There are SoCs that provide both. The internal layout of the registers is also not always consistent.

Yes, I understand this is not a universally supported register type. However, I believe that the existence of the fastled library shows that there is a small set of "types" of GPIO output registers, which we can support through a common API. E.g. if gpio_get_output_register errors out, the driver could fall back on gpio_get_output_set_register(), gpio_get_output_clear_register(), etc., with coverage level depending on the driver. We could consider a query routine where the GPIO driver returns a bitmask of supported types.

We would probably need a set of inline functions which would present a consistent API but be implemented individually by the SoC. Similar to your proposal but writing to the register would happen via an inline function. We would need to rely on CMake to include the right header (and in this case also implementation) file

I'm not sure I can see clearly how some of these details would work in practice, but I look forward to seeing more details if you have them.

Thanks for your response!

anangl commented 5 years ago

@mnkp Regarding the question I was trying to signal yesterday on slack. It's about configuring interrupts for handling a button, for instance. An application will get the flags defined for the button in DTS through a a generated macro, like SW0_GPIO_FLAGS. Ideally, it would just pass the value to the gpio_pin_configure function to get the pin properly configured, just like in the case of the LED that you presented in your PR. But the flags controlling the interrupts generation are application specific - some applications will need to react on the input becoming active (button pressed), others on the input becoming inactive (button released), and in some cases, perhaps not buttons, there may be a need to react on the signal level, not an edge. Thus, I think that the flags related to interrupts should not be specified in DTS (like in your PR), but instead the GPIO API should provide some means to get the proper interrupt configuration based on the GPIO_ACTIVE_* flag used in DTS. Hence, I thought that we would need to add some macros or inline functions that would handle the translation. Or do you have some other vision of achieving this?

anangl commented 5 years ago

After taking another look, I've got one more doubt. Why do we change the required behavior of gpio_pin_{read|write} functions so that they now are to operate on the logic level and then we introduce gpio_pin_{read|write}_raw functions that are to behave just like gpio_pin_{read|write} were supposed to so far? Wouldn't it be clearer/safer to only introduce new functions that take into account the GPIO_ACTIVE_LOW flag? Possibly with names clearly indicating this, for instance gpio_pin_{activate|deactivate|is_active} or gpio_pin_{assert|deassert|is_asserted} as @pabigot proposed earlier in his PR.

mnkp commented 5 years ago

I think that the flags related to interrupts should not be specified in DTS (like in your PR), but instead the GPIO API should provide some means to get the proper interrupt configuration based on the GPIOACTIVE* flag used in DTS.

The current interrupt configuration done in the Zephyr version of the DTS is not standard and indeed it may be best to remove it. We can always add it back in the future, possibly in a standard form, if the need arises. The easiest way to have the application configure interrupts in a portable way is to define them in terms of pin active, inactive state as in: LEVEL_INACTIVE, LEVEL_ACTIVE, EDGE_TO_ACTIVE, EDGE_TO_INACTIVE.

A general remark regarding DTS: According to the documentation DTS is meant to provide information about hardware the OS runs on as well as its initial configuration for the boot process. Thus uart DTS node contains register addresses of the UART module as well as the initial baud rate. The application is free to change the configuration later. It's actually OK to add application specific bits to the DTS.

mnkp commented 5 years ago

After taking another look, I've got one more doubt. Why do we change the required behavior of gpio_pin_{read|write} functions so that they now are to operate on the logic level and then we introduce gpio_pin_{read|write}_raw functions that are to behave just like gpio_pin_{read|write} were supposed to so far?

I'll copy paste my answer from #12651.

I provided a bit more elaborate answer in my comment in #11880. In short GPIO_ACTIVE_LOW is just a different name for GPIO_POL_INV flag which controls the value returned by gpio_pin_write and gpio_pin_read functions. Unfortunately, the documentation of these functions did not state it explicitly. The only reason we rename the GPIO_POL_INV flag to GPIO_ACTIVE_LOW is to be compatible with the definition of DTS gpio node in Linux.

In short we don't change the behavior of gpio_pin_{read|write} functions. We rename the GPIO_POL_INV flag to match Linux DTS and clarify its usage.

anangl commented 5 years ago

In short we don't change the behavior of gpio_pin_{read|write} functions. We rename the GPIO_POL_INV flag to match Linux DTS and clarify its usage.

I cannot entirely agree with that. As I recall, one of the reasons for making this GPIO flags cleanup was the need to replace GPIO_INT_ACTIVE_{HIGH|LOW} flags used in DTS files to define the active state of buttons and LEDs with something more appropriate (see #10339 and its predecessor #10300). In this context, when we replace GPIO_INT_ACTIVE_LOW with GPIO_ACTIVE_LOW, we change the behavior of gpio_pin_{read|write} functions. See for instance the basic button sample. It takes the flags defined in DTS for the button with sw0 alias and passes them directly to gpio_pin_configure. After GPIO_INT_ACTIVE_LOW is replaced with GPIO_ACTIVE_LOW, this call to gpio_pin_read will give the opposite result than it does currently. The read value is only printed here, so it is not a big deal, but I'm afraid that we may encounter cases with more serious consequences of such change (related to incorrect driving of CS pins, for example). Do you think it is possible to catch them all while applying the proposed changes in GPIO flags?

carlescufi commented 5 years ago

@mnkp @anangl My suggestion here would be to:

anangl commented 5 years ago

@carlescufi I was thinking about similar names for these new functions, but I wasn't sure if _get and _set would not be too general, so I thought that maybe _state_{get|set} could work, but actually these didn't convince me fully either. And so far I couldn't come up with anything better. Anyway, your proposal looks good to me. With one correction, gpio_port_set should take 3 parameters: (*port, pin_mask, value), so that one could set states of pins from the group specified by pin_mask to desired states (active or inactive) as defined by the bits in value.

pabigot commented 5 years ago

@anangl @carlescufi gpio_pin_{assert,deassert} as with https://github.com/zephyrproject-rtos/zephyr/pull/11880#issuecomment-459641859? I believe it's understood that "assert" means logic true and "deassert" logic false.

In any case, I think you're running into the same problems that killed #11880: we can be backward compatible, or we can match Linux/DTS API. Because GPIO_POL_INV wasn't clearly defined and implemented consistently across implementations we can't do both.

Or so I believe. I'm not sure everybody believes they're incompatible, and if they are I don't know that there's agreement on which requirement should take priority. If not, that should be clearly settled first.

carlescufi commented 5 years ago

@pabigot @anangl @mnkp Now we're getting into personal preference territory, but I honestly think that assert,deassert is less clear than get,set. Looking at other APIs I can also propose:

The advantage with get_level is that we could actually reuse it for physical or logical level depending on what you pass to gpio_configure(), which would allow users to use the new signatures (returning bool and u32_t and no pointer:

that way we match the classical "active level low/high` way of describing the logical levels. EDIT: A couple more choices below

mnkp commented 5 years ago

Thanks @carlescufi for the proposal. Nice selection!

Since we are getting into personal preference territory regarding naming I'll make my statement. I like very much the first proposal: gpio_pin_get, gpio_pin_set, gpio_port_get, gpio_port_set. The function names are short and to the point. They should become the most used GPIO API functions so it's better to keep them short. IMHO the level variants all read well but there is no added value, we only make the name longer.

anangl commented 5 years ago

While I agree with @pabigot that gpio_pin_{assert,deassert} would indicate the purpose of these functions in perhaps the clearest way, I see a disadvantage of such approach. We'd have separate functions for setting active and inactive pin levels, while set handles both cases. I think it matters especially for ports, where it will be convenient to have the possibility of setting a selected group of pins to different levels in one call. And I agree with @mnkp that the get,set proposal seems to be the best one. But still, shouldn't the gpio_port_set function take the third parameter with values to set for the pins specified by the mask (as I already tried to signal above)? Or do I misunderstand something?

mnkp commented 5 years ago

shouldn't the gpio_port_set function take the third parameter with values to set for the pins specified by the mask (as I already tried to signal above)? Or do I misunderstand something?

Yes, the port functions should include an extra parameter

anangl commented 5 years ago

Yes, the port functions should include an extra parameter

I'm not sure about gpio_port_get. Callers of the function can mask the obtained value on their own, if needed. Do you see some additional benefit of masking the value inside the driver?

mnkp commented 5 years ago

Yes, the port functions should include an extra parameter

I'm not sure about gpio_port_get. Callers of the function can mask the obtained value on their own, if needed. Do you see some additional benefit of masking the value inside the driver?

I believe it will make the API more consistent and easier to use.

carlescufi commented 5 years ago

Yes, the port functions should include an extra parameter

I'm not sure about gpio_port_get. Callers of the function can mask the obtained value on their own, if needed. Do you see some additional benefit of masking the value inside the driver?

I believe it will make the API more consistent and easier to use.

I've updated port_set() to take a value. I have no preference regarding port_get().

anangl commented 5 years ago

I believe it will make the API more consistent and easier to use.

I just thought it might be considered an unnecessary overhead when there would be no need of any masking at this point (for example, when there are more groups of pins in this port which need to be later on masked separately). But that's fine for me. Consistency is important, too.

pabigot commented 5 years ago

If you do:

you don't need need the mask on the read, and you get a slightly more powerful operation that I personally think is more clear than "set to this value, oh, but ignore everything but these bits". (I use this in my APIs, with the semantics that a bit set in both set_bits and clear_bits is inverted from whatever its previous value was. Very useful for flashing LEDs.)

Just another opinion.

carlescufi commented 5 years ago
  • u32_t gpio_port_get(*port)
  • void gpio_port_set(*port, set_bits, clear_bits)

I like this proposal for the port, thoughts @mnkp and @anangl ?

anangl commented 5 years ago
  • u32_t gpio_port_get(*port)
  • void gpio_port_set(*port, set_bits, clear_bits)

I like this proposal for the port, thoughts @mnkp and @anangl ?

Looks reasonable. These parameters will actually directly provide values that will be written to hardware registers in many drivers. And we still can provide the functionality of setting (*port, mask, value) with some wrapper if it turns out to be useful for cases that utilized gpio_port_write previously, like this one. I like it, too.

mnkp commented 5 years ago
  • u32_t gpio_port_get(*port)
  • void gpio_port_set(*port, set_bits, clear_bits)

I like this proposal for the port, thoughts @mnkp and @anangl ?

Interesting. I'll need to have a look at how such an API plays with bit-banging implementations on the application level. There should be a few in Zephyr codebase. For now in the PR I'm preparing I'll implement pin versions only. It's going to be a lot of work anyway. Now that we've chosen the names we can add port versions later.

mbolivar commented 5 years ago

My main point is that it would also be useful for other device drivers that need fast access to GPIOs built into the SoC to have a generic API for accessing the output registers for these devices.

@mnkp @pabigot to follow up on our brief discussion today, I thought I would look at all the existing GPIO drivers and see what types of registers they use. From there, I think we can see more clearly what this would look like in practice.

Edit: I've moved this comment to https://github.com/zephyrproject-rtos/zephyr/issues/11917#issuecomment-503318404 which is the correct issue for this subject.

mnkp commented 5 years ago

I would like to shortly revisit our decision on naming functions that read/write pin logical level. Currently we have gpio_pin_get, gpio_pin_set and I argued to keep it this way. However, to implement fast access GPIO functions as proposed by @mbolivar we will likely need to add a new function to the API, something like gpio_pin_get_register or gpio_pin_get_descriptor. And in this case maybe it will be better to use the _level variants of pin_get, pin_set functions (to easier distinguish the two) as proposed by @carlescufi i.e. use gpio_pin_get_level, gpio_pin_set_level functions. Linux API is using gpiod_get_value, gpiod_set_value names.

pabigot commented 5 years ago

And in this case maybe it will be better to use the _level variants of pin_get, pin_set functions (to easier distinguish the two) as proposed by @carlescufi i.e. use gpio_pin_get_level, gpio_pin_set_level functions.

I would prefer to avoid using the term "level" in the API identifiers. AFAICT the API in #16648 has the traditional gpio_pin_read which operates on the line (electrical/physical) level and new gpio_pin_get which operates on the logical level. Both of them are levels, so adding _level does not help the user to understand what the function does.

If this is for the "fast" version of the API I'd rather see that as a separate, parallel API with a different prefix, e.g. gpiof_pin_read(), just as Linux gpiod_get_value(&gpio_desc) is for a descriptor based API that replaces the legacy gpio_get_value(unsigned).

MaureenHelm commented 5 years ago

If you do:

  • u32_t gpio_port_get(*port)
  • void gpio_port_set(*port, set_bits, clear_bits)

you don't need need the mask on the read, and you get a slightly more powerful operation that I personally think is more clear than "set to this value, oh, but ignore everything but these bits". (I use this in my APIs, with the semantics that a bit set in both set_bits and clear_bits is inverted from whatever its previous value was. Very useful for flashing LEDs.)

How is the masked write not clear? It corresponds directly to hardware implementations with explicit set and clear port registers, which allow you to change the state of a pin without a read-modify-write cycle.

I don't see how a bit set in both set_bits and clear_bits should mean to invert it.

I think I'd rather see something like:

void gpio_port_set(*port, set_bits)
void gpio_port_clear(*port, clear_bits)

And if you really want an invert, you can add:

void gpio_port_invert(*port, invert_bits)
pabigot commented 5 years ago

So that'd be:

u32_t gpio_port_read(port); // line level
u32_t gpio_port_get(port);  // logic level
void gpio_port_write(port, value); // line level, all bits
void gpio_port_set1(port, value);  // logic level, all bits
void gpio_port_set(port, set_bits); // logic level, only specified bits
void gpio_port_clear(port, clear_bits); // logic level, only specified bits
void gpio_port_invert(port, invert_bits); // logic level, only specified bits

Other than the naming conflict when both line and logic level operations are supported I don't have a big problem with this. The original motivation for the API I proposed was to resolve the conflict when having a mask parameter to the set operation suggested there should be a corresponding mask in the get operation.

carlescufi commented 5 years ago

Here is my proposal based on everything that has been said so far, and taking into account the following:

Standard APIs:

/** Pin access **/
/* Logical */
int gpio_pin_get(*port, pin);
int gpio_pin_set(*port, pin);
/* Physical */
int gpio_pin_get_raw(*port, pin);
int gpio_pin_set_raw(*port, pin);

/** Port access **/
/* Logical */
int gpio_port_get(*port, *value);
int gpio_port_get_masked(*port, mask *value);
int gpio_port_set(*port, value); // writes the value to the port
int gpio_port_op(*port, set_bits, clear_bits, invert_bits);

/* Physical */
int gpio_port_get_raw(*port, *value);
int gpio_port_get_masked_raw(*port, mask *value);
int gpio_port_set_raw(*port, value); // writes the value to the port
int gpio_port_op_raw(*port, set_bits, clear_bits, invert_bits);

Fast/descriptor-based APIs:

/* Get a descriptor */
struct gpiod_t* gpiod_get(*port);

The rest of APIs should be exactly the same as the standard APIs but:
- Replacing gpio_ with gpiod_ as a prefix
- Replacing the first port parameter with struct gpiod_t*
pabigot commented 5 years ago

I don't believe we have consensus on the port set API. There have been three proposals:

// API 1
int gpio_port_set_masked(*port, value, mask); // writes the value to the port, only non-masked bits
// API 2
int gpio_port_set_sct(*port, set_bits, clear_bits); // sets some bits, clears some bits, toggles if both requested
// API 3
int gpio_port_set_bits(*port, set_bits); // sets specified bits
int gpio_port_clear_bits(*port, clear_bits); // clears specified bits
int gpio_port_invert_bits(*port, toggle_bits); // TBD: changes level of specified bits

(1) is existing practice, so should be the fallback if we can't agree on a change. (2) seemed to get some approval and interest, but one rejection. (3) hasn't really been discussed.

I prefer (2), because the set-clear-toggle bit mutation operation has general applicability for flags and register values outside of GPIO, and I am going to use it in other situations where it's more clear. I could live with the others.

So how about we vote? I'm going to post separate comments for each choice; sorry for the blitz. Click thumbs-up to prefer this API. Click thumbs-down if you think it's an absolutely horrible idea and would want to block a PR that includes it. Vote up on one choice, and down on any of them you want. We'll figure out how to tally the votes when we have some.

pabigot commented 5 years ago

Click thumbs-up to prefer this API. Click thumbs-down if you think it's an absolutely horrible idea.

// Multi-pin mutate operation API 1
int gpio_port_set_masked(*port, value, mask); // writes the value to the port, only non-masked bits
pabigot commented 5 years ago

Click thumbs-up to prefer this API. Click thumbs-down if you think it's an absolutely horrible idea.

// Multi-pin mutate operation API 2
int gpio_port_set_sct(*port, set_bits, clear_bits); // sets some bits, clears some bits, toggles if both requested
pabigot commented 5 years ago

Click thumbs-up to prefer this API. Click thumbs-down if you think it's an absolutely horrible idea.

// Multi-pin mutate operation API 3
int gpio_port_set_bits(*port, set_bits); // sets specified bits
int gpio_port_clear_bits(*port, clear_bits); // clears specified bits
int gpio_port_invert_bits(*port, toggle_bits); // TBD: changes level of specified bits
mnkp commented 5 years ago

Previously we agreed that the pin set / get functions should be defined as

bool gpio_pin_get(*port, pin);
void gpio_pin_set(*port, pin, value);

i.e. they were not returning error codes. That's fine for the SoC GPIOs but as @pabigot mentioned during API meeting we need to support also external GPIOs, e.g. connected via I2C bus. So it looks like we do need to add error codes to set/get functions. That's easy for the pin version of the functions, as proposed by @carlescufi.

int gpio_pin_get(*port, pin);
int gpio_pin_set(*port, pin, value);

However, we can't rely on the assert for the port version of the functions as we're talking here about allowed runtime errors. E.g. an error on the I2C bus that prevents the gpio_port_get_masked from being correctly executed. I'm afraid in this case we will need to return the port value by a parameter, as in

int gpio_port_get_masked(*port, mask, *value);

Carles: Added this to the main description

pabigot commented 5 years ago

Click thumbs-up to prefer an API that combines AP1 and API3. Click thumbs-down if you think it's an absolutely horrible idea.

// Multi-pin mutate operation API 1
int gpio_port_set_masked(*port, value, mask); // writes the value to the port, only non-masked bits
// Multi-pin mutate operation API 3
int gpio_port_set_bits(*port, set_bits); // sets specified bits
int gpio_port_clear_bits(*port, clear_bits); // clears specified bits
int gpio_port_invert_bits(*port, toggle_bits); // TBD: changes level of specified bits
pabigot commented 5 years ago

Reviewing some of the material above there's another consensus decision that needs to be summarized: handling of line vs logic level taking into account GPIO and interrupt configuration and the impact of adding line/logic level API on the use of existing GPIO_POL_* flags. Every bulleted item below is intended to be either a definition or a requirement that together determine whether an implementation of the API is conforming.

A clarified terminology summary from the pin/port access API summary above:

Logic level derives from line level based on a configured active level:

I.e. active level is a binary state. Informally, if a pin is configured active high the value representations of logic level and line level are equivalent; otherwise the logic level is the inverse of the line level. In support of formalizing this, define these two functions for any pin p within the device:

where standard C conversion between integer and boolean value spaces is assumed and XOR is boolean exclusive or.

We have also introduced a distinction between GPIO mode and interrupt mode for a pin. Using the proposed API of #16648:

This summarizes my understanding of what we've decided; again, this needs to be validated.

The effect of active level on pin mode is specified by this rule:

My recollection is that in early discussions this was not true (possibly because it was mixed with GPIO_POL_INV), so we need to confirm or reject this interpretation.

Abstracting away from public API names we have an inspect operation get and a mutate operation set, where the unadorned name operations on logic levels and a suffix _raw operates on line levels. The fundamental get_raw function defines the behavior of an internal inspection of pin state within the target, regardless of whether the pin is GPIO or interrupt controlled. For GPIO-controlled it is equivalent to the exposed GPIO function; for interrupt-controlled its defined behavior is used to configure the interrupt conditions.

Proposed detailed semantics for get_raw(p) for any pin p are:

NOTE: The requirements above are carefully worded to specify the behavior when a pin is configured for both input and output. There are devices (e.g. SX1509B) where bidirectional configuration is supported and the input signal may be different from the configured output signal. This proposed behavior must be validated, because it fails to provide a way to read the configured output of a signal that is bidirectional. Also note that this requires that a driver be able to provide the last configured output value; it's possible some peripherals don't provide a way to read that back.

With the above, we then have:

Where "all situations" covers GPIO and interrupt configuration, direction, and active level.

For mutation operations:

Finally we need the following to deal with the legacy parts of the API:

Without this people may continue to use these flags to enable peripheral-specific inversion capabilities, resulting in cross-platform inconsistencies. (I would rather retain these flags and specify that their use has a target-specific impact on the behavior of getraw() and setraw(), but I don't think that's the consensus position.)

anangl commented 5 years ago

@pabigot Two doubts from my side.

  1. Regarding get_raw(p). Is this bullet:
    • If a pin is configured to enable output, get_raw(p) returns the line output level configured for p in the GPIO peripheral; otherwise

really necessary, will such functionality be useful? Maybe we could remove it and hence simplify the API by allowing using get_raw(p) only for pins with enabled input?

  1. Regarding GPIO- vs interrupt-controlled pins.
    • A pin that was last configured by gpio_pin_configure() is a GPIO-controlled pin.
    • A pin that was last configured by gpio_pin_interrupt_configure() is an interrupt-controlled pin.

I am not convinced that we should distinct such two modes for pins, and basing on them disallow parts of API. I'd rather instead set a requirement for set_raw(p) to return an error code (-EINVAL perhaps) if it is called for a pin that was not configured to enable output, and a similar one for get_raw(p). I understand the introduced gpio_pin_interrupt_configure as an additional to gpio_pin_configure way of configuring GPIO interrupts, to be used in cases when a pin is not to be controlled via GPIO driver (like the one signaled by @b0661). And I thought this function could be used also for changing only the interrupt configuration for a pin configured previously by gpio_pin_configure.

pabigot commented 5 years ago

@anangl Thanks for the careful reading.

For the semantics of interrupt versus GPIO configuration I represented my understanding based on the goal of moving towards PINMUX functionality where interrupt functions are separated (to some degree) from GPIO functions. From earlier discussions I understood that on some hardware a pin configured to be in interrupt mode may not support GPIO operations. I haven't encountered that myself, but I defer to @mnkp to precisely define the two roles and the functions used to control them.

For reading the configured output state of a pin: if it is to be possible to toggle a pin state, as with flashing an LED, you need to be able to read the current setting so you know what value to write to change that state. That could be done by:

In my APIs I usually go with (B), but (C) is OK too and simplifies GPIO.

Thumbs up here to go with (C), removing explicit toggle support from the GPIO API.

anangl commented 5 years ago

@pabigot Both (B) and (C) are OK to me, my point was just to not go with (A). (B) seems to be a bit better option, since there are pieces of hardware providing dedicated registers for toggling (no need to read the output state then). It shouldn't be that hard to agree on how to support toggling in the GPIO API.

pabigot commented 5 years ago

It shouldn't be that hard to agree on how to support toggling in the GPIO API.

You are an optimist.

A minimal impact way to do it is to define toggle as the behavior specified when a bit is set in both the set_bits and clear_bits parameters to the fundamental set-clear operation. ~As there were reasonable objections to that based on it being obscure, and~ I think adding an explicit toggle operation is over-engineering. ~I still prefer (C)~

Edit: Actually, if we specify the behavior of set_bits to be "Pins selected by this mask are changed to high/active if they were not already at that state", and clear_bits similarly, then the toggle behavior comes out naturally. So I'm totally fine with (B), and would take (C) as an option. I'm not supportive of a separate API function just to toggle.

Edit2: In fact this specification also defines the behavior when the driver knows the operation will not change the current configuration (viz., it must not appear to occur). This is relevant to drivers like SX1509B where an I2C transaction can be eliminated. That tips my vote to (B) provided though this mechanism.

anangl commented 5 years ago

A minimal impact way to do it is to define toggle as the behavior specified when a bit is set in both the set_bits and clear_bits parameters to the fundamental set-clear operation.

I agree and I think this is a good way to go. Such semantics may seem a bit "exotic" at first glance, but after you think of it a bit, you realize that it is a good way of utilizing a parameter combination that has to be checked anyway, but instead of returning an error, providing a nice feature for it. To me this is mainly a question of describing it properly in the documentation, so that it is clear to users. What you propose in your first edit seems reasonable.

Edit2: In fact this specification also defines the behavior when the driver knows the operation will not change the current configuration (viz., it must not appear to occur). This is relevant to drivers like SX1509B where an I2C transaction can be eliminated. That tips my vote to (B) provided though this mechanism.

Yes. That convinces me even more that this option is a better choice.

carlescufi commented 5 years ago

API meeting:

Standard APIs:

/** Pin access **/
/* Logical level */
int gpio_pin_get(*port, pin);
int gpio_pin_set(*port, pin, value);
/* Line level */
int gpio_pin_get_raw(*port, pin);
int gpio_pin_set_raw(*port, pin);

/** Port access **/
/* Logical level */
int gpio_port_get(*port, u32_t *value);
int gpio_port_set(*port, u32_t value); // writes the value to the port
int gpio_port_set_masked(*port, u32_t mask, u32_t value); // writes the value to the port
int gpio_port_set_clr_bits(*port, u32_t set_bits, u32_t clear_bits);
int gpio_port_set_bits(*port, u32_t bits);
int gpio_port_clear_bits(*port, u32_t bits);
int gpio_port_toggle_bits(*port, u32_t bits);

/* Line level */
int gpio_port_get_raw(*port, u32_t *value);
int gpio_port_set_raw(*port, u32_t value); // writes the value to the port
int gpio_port_set_masked_raw(*port, u32_t mask, u32_t value); // writes the value to the port
int gpio_port_set_clr_bits_raw(*port, u32_t set_bits, u32_t clear_bits);
int gpio_port_set_bits_raw(*port, u32_t bits);
int gpio_port_clear_bits_raw(*port, u32_t bits);
int gpio_port_toggle_bits_raw(*port, u32_t bits);

Fast/descriptor-based APIs:

/* Get a descriptor */
gpiod_t gpiod_get(*port);

The rest of APIs should be exactly the same as the standard APIs but:
- Replacing gpio_ with gpiod_ as a prefix
- Replacing the first port parameter with gpiod_t
- Only _raw versions of the APIs would be available
- Implemented as static inline functions

@mnkp the above is from the API meeting, are you happy with that?

galak commented 5 years ago

int gpio_port_sc_bits(*port, u32_t set_bits, u32_t clear_bits);

I'd suggest:

int gpio_port_set_clr_bits(*port, u32_t set_bits, u32_t clear_bits);

lawrencek52 commented 5 years ago

All of the APIs above have a baseline assumption that you are working on a 32-bit machine, and can only set or clear up to 32 bits at a time. what happens when you move to a 64-bit machine? What happens if you are working on a 16-bit native machine, and it has to extend all of the parameters up to 32-bits? You are passing in 32-bit ints, but getting back native machine ints (which may or may not be 32-bits).

pabigot commented 5 years ago

The baseline assumption is that no GPIO device has more than 32 pins. There's one existing device that has more, and it has to be split into separate devices.

Yes, if there's a 16-bit target this would cause overhead, but Zephyr pretty strongly expects a minimum 32-bit system. I generally prefer native int types but for a cross-platform system they introduce complexity.

Plain int returns are (or should be) always zero, a boolean, or a negative error code. Pin sets should always be returned via out parameters. If that isn't happening, it needs to be fixed.