Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Convert GPIO drivers to new GPIO API #18530

Closed
galak opened this issue Aug 20, 2019 · 12 comments
Closed

Convert GPIO drivers to new GPIO API #18530

galak opened this issue Aug 20, 2019 · 12 comments
Labels
area: GPIO Enhancement Changes/Updates/Additions to existing features
Milestone

Comments

@galak
Copy link
Collaborator

galak commented Aug 20, 2019

This is a tracking issue on process and status for converting GPIO drivers to support new GPIO API.

Driver Status

PR Ready On Topic On Master Driver Point of Contact Details
altera_nios2 @jenmwms Remove #20214
cc13xx_cc26xx @vanti #19626
cc2650 @galak @vanti Deprecated platform, remove, #22012
cc32xx @vanti #19626
cmsdk_ahb @galak #19382
dw @tbursztyka @dcpleung #19631
esp32 @ExtremeGTX #19753
gecko @mnkp #16648
ht16k33 @henrikbrixandersen #19641
imx @stanislav-poboril #19571
intel_apl @dcpleung #20400
litex #18038, added in topic-gpio
lmp90xxx @henrikbrixandersen #22212
mchp_xec @franciscomunoz @albertofloyd @scottwcpg #19555
mcux @MaureenHelm / @mnkp #16648
mcux_igpio @MaureenHelm #19247
mcux_lpc @agansari #19714
mmio32 @galak #20028
nrfx @mnkp #16648
pcal95xx @dcpleung #20268
rv32m1 @MaureenHelm #19664
sam @mnkp #16648
sam0 @pabigot #19785
sifive @nategraff-sifive #19668
stellaris @galak #19384
stm32 @erwango / @mnkp #19579
sx1509b @pabigot #19379

Steps required to convert a GPIO driver to the new API

Convert board .dts files

Before updating the driver source code it's best to start with updating all in-tree board .dts files (located in boards/ folder) the driver depends on. All the GPIO_* flags currently present in the .dts files, such as GPIO_INT_ACTIVE_LOW, GPIO_PUD_PULL_DOWN, GPIO_DIR_OUT are deprecated and should be removed. The newly introduced flags are compatible with Linux DT. They are located (and documented) in include/dt-bindings/gpio/gpio.h. Their naming and purpose is similar, though not identical to the old flags. Rather than mechanically changing old flags to the matching new ones it's better to consult board's schematic. Pin with no flags defaults to active high and, in case it is configured as an output, to a push-pull mode. The dts files that have been converted so far avoid explicit usage of GPIO_ACTIVE_HIGH or GPIO_PUSH_PULL flags.

At present it's not possible to define pin as input/output in DTS. This is done in the application code. Similarly, no interrupt configuration is possible in DTS.

Update the driver

Several drivers have been updated already. Please use any of the commits as an example, e.g.

  • "drivers: gpio_gecko: update to use new GPIO API"
  • "drivers: gpio_sam: update to use new GPIO API"
  • "drivers: gpio_nrfx: update to use new GPIO API"

Adding the implementation of the new port_get_raw, port_set_masked_raw, port_set_bits_raw, port_clear_bits_raw, port_toggle_bits, functions is usually quite straightforward:

  • get_raw(ptr) => *ptr = INPUT;
  • set_masked_raw(mask, value) => OUTPUT = (OUTPUT & ~mask) | (value & mask);
  • set_bits_raw(pins) => OUTPUT |= pins;
  • clear_bits_raw(pins) => OUTPUT &= ~pins;
  • toggle_bits(pins) => OUTPUT ^= pins;

The more complicated part is the implementation of pin_interrupt_configure function as well as updating config to support new flags.

The old GPIO API configures interrupts via the config function. This is not supported by the new API, however to keep backward compatibility the config function will call pin_interrupt_configure. This call will be removed in the future.

Support for all the flags defined in include/dt-bindings/gpio/gpio.h has to be implemented or the driver should return -ENOTSUP. Also GPIO_OUTPUT_INIT_LOW, GPIO_OUTPUT_INIT_HIGH flags have to be implemented since misbehaving driver has a potential of destroying the board.

The button sample located in samples/basic/button/ is a very simple application that is able to test most of the new features. It's quite handy when starting to work on the new code. The app generates an interrupt the moment board's button is pressed (and displays a message on the console). Also, whenever the button is pressed the board's LED should be ON.

The new GPIO API is not specifically designed to be fast, there is another API in the works which aims to achieve that goal. Nevertheless, once the driver is complete it's worth checking the generated assembly code of the new port_* functions. Following table shows typical length of the port_* functions implemented on ARM Cortex-M4, -M7 architecture.

Function name Assembly instructions no.
port_get_raw 7
port_set_masked_raw 10
port_set_bits_raw 6
port_clear_bits_raw 6
port_toggle_bits 6
port_toggle_bits (no toggle reg) 8

Update *.c files in the board directory

A few boards require initial configuration of GPIO pins. This is done by a *.c file placed in the board directory. If the file contains any calls to the deprecated GPIO functions, e.g. gpio_pin_write or use any of the deprecated flags e.g. GPIO_DIR_OUT they have to be replaced by the corresponding feature of the new API.

Test the driver code

The initial driver development can be guided by samples/basic/button/ sample project. Once the driver is working it should be tested with tests/drivers/gpio/gpio_api_1pin/ testcase. The testcase is using the pin where the board's LED0 is connected to run battery of tests. No hardware modifications to the board are required. However, it relies on the ability of the driver to configure the pin in input/output mode. If this is not supported by the driver the driver should return -ENOTSUP. The testcase should still pass but is pretty useless in assessing quality of the code.

The testcase which is using two (or more) different pins is not ready yet.

Steps required to convert samples / application code to the new API

@mnkp please fill in

List of users to convert: #20017

@galak galak added Enhancement Changes/Updates/Additions to existing features area: GPIO labels Aug 20, 2019
@pabigot pabigot added the area: API Changes to public APIs label Aug 20, 2019
@pabigot pabigot removed the area: API Changes to public APIs label Aug 20, 2019
@galak
Copy link
Collaborator Author

galak commented Aug 20, 2019

@nategraff-sifive I assume you are good with looking to convert the sifive gpio driver to the new GPIO api?

@galak
Copy link
Collaborator Author

galak commented Aug 20, 2019

@henrikbrixandersen I assume you are good with looking to convert the ht16k33 to the new GPIO api?

@henrikbrixandersen
Copy link
Member

@henrikbrixandersen I assume you are good with looking to convert the ht16k33 to the new GPIO api?

@galak Sure.

@pabigot
Copy link
Collaborator

pabigot commented Sep 4, 2019

Proposed Roadmap to GPIO Integration for #15611

(Review the comment history for details of the changes to this process based on API meeting discussions.)

This is a slightly edited description of stages originally proposed on slack. The goal is to do development and review work on a topic branch, but to merge pieces to master as soon as they are sufficiently ready that their presence will not break any tests or applications. At all points master is expected to build and pass CI/shippable; however in certain stages the topic branch may have deprecation warnings and build or runtime failures resulting from platforms and drivers that have not been converted. Content on the topic branch is expected to be refactored, reordered, and cherry-picked as necessary to ensure material merged to master has clean history.

There are seven stages:

  • (1) Add the new API and test code as in Update GPIO API to support flags used by Linux DT bindings #16648 plus pieces of [TOPIC: GPIO] documentation refinements, test conversion, and sx1509b conversion for new API #18689, and enough additional drivers to be convinced the approach is sound. These merge to the topic branch when ready. Existing code uses old API and old implementation and should work correctly. Old API will not be deprecated at this stage.

    It is not a goal of this stage to convert all drivers, only the ones that have an active responsible codeowner who commits to doing the conversion in a timely manner (e.g. within the first 2-3 weeks of 2.1 development). We must expose the work to the entire community quickly so we get feedback on issues before significant effort is wasted. There will be time to make corrections.

  • (2) Extract the initial capability from the topic branch into a PR to master. All tests should pass in this PR.

    The topic branch will be rebased on the resulting master retaining any commits such as samples that would not pass on master. A commit that adds deprecation warnings for old API may be added to the topic branch, or delayed until later.

  • (3) Re-implement the old API using the new basis functions in a single commit on the topic branch. All tests should pass for applications where all GPIO peripherals have been converted, if deprecation warnings are disabled. Tests for systems that have not implemented the new API will fail.

  • (4) Implement the new API on remaining platforms, each in a single commit for the peripheral driver and all target-specific devicetree source. These are reviewed and tested on the topic branch. When one is deemed stable it gets extracted into a PR to master to join the others.

    Simultaneously any driver that uses GPIO may be updated to the new API; examples include sensors and networking hardware. These commits stay on the topic branch until anything that might use them has been converted to the new implementation, at which point they can merge to master.

  • (5) When all SOC/external GPIO peripherals have been converted the old API implementation functions are removed. This is validated on the topic branch, then merged to master.

  • (6) Any remaining GPIO uses can be cleaned up by commits targeting master. This can begin as of stage (4) and proceed independently as long as they do not introduce failures on unconverted platforms. Issues may be opened identifying particular sensors/drivers/applications where conversion must be done and validated by the codeowner.

  • (7) Once all such uses are converted the ability to control deprecation of the old API is removed on master and the integration for GPIO changes is complete.

Material is merged to master at/during stages (2) and (4) through (6). During those stages the topic branch works for converted systems/applications, but may not build for others. Master should pass sanitycheck at all times and be bisectable except possibly between commits within a merge, just as is true for existing PRs. The topic branch is no longer used after stage (5).

This approach requires active management: cherry-picking pieces from the topic branch into PRs that target master, and frequent rebasing of the topic branch. This is not technically difficult to do, and if this approach is approved I can take on that task. Success requires responsiveness from stakeholders who approve the topic-branch merge to quickly approve the master merge.

It also requires responsiveness from stakeholders outside the API meeting participants: vendors to convert GPIO peripheral drivers in (4) and other codeowners to update and validate code that uses the GPIO API in (6).

I would like to see us reach stage (4) no later than the end of September. I believe this is achievable.

#18689 has been a prototype of this: it contains material that is similar to master at stage (4). sanitycheck will pass in that situation (as long as CI doesn't break, as it did in my last push). I've also got commits that verify subsequent when restricted to converted devices; they are currently not in that PR.

@galak
Copy link
Collaborator Author

galak commented Sep 4, 2019

Would be good to get a summary of changes that need to be made to the driver and dts files.

@pabigot
Copy link
Collaborator

pabigot commented Sep 4, 2019

IMO the best approach is to use the five existing conversions as examples, but:

Essentially for the driver it's "implement the new functions in the API table, and make the tests pass".

For the devicetree files it's minimally replace the deprecated macros and update the flags to be correct for the corresponding device (generally leave at 0 for active high, or change to GPIO_ACTIVE_LOW if appropriate) so the updated LED and button examples will work and driver code can use gpio_pin_set without worrying about logical/physical conversions.

@mnkp
Copy link
Member

mnkp commented Sep 9, 2019

I've added "Steps required to convert a GPIO driver to the new API" section directly to the issue description so it's easy to spot. Please let me know if there are parts which are not clear / should be extended.

@pabigot
Copy link
Collaborator

pabigot commented Sep 11, 2019

Proposed GPIO policies for interrupt use/safety, atomicity, etc. related to #18970

Copied from slack, edited here based on feedback

  • All functions must be invokable from an ISR. Functions that cannot accomplish their roles in that context may return -EWOULDBLOCK.
  • It must be possible to disable a GPIO pin interrupt from a GPIO interrupt callback.
  • All pin configuration functions (GPIO, interrupt, handler) must be atomic.
  • All functions that change port outputs must be atomic. (For example, gpio_port_toggle() must lock in any system that doesn't have a hardware toggle register since it's a read-modify-write operation.)

@pabigot
Copy link
Collaborator

pabigot commented Sep 11, 2019

The potential issue with the existing API is that the way to disable interrupts is gpio_pin_configure_interrupt(). The requirements in #18530 (comment) are easily met for SOC GPIO drivers. For an external GPIO chip like SX1509B disabling a specific pin interrupt would require an I2C transaction, which cannot be done from an ISR.

Currently SX1509B doesn't support interrupts. If it were made to do so, it could still satisfy the above requirements because I carefully worded them to allow an implementation that flagged the interrupt, but didn't invoke the corresponding callback within the ISR. This could still cause functional problems: the situation where I usually want to disable interrupts in the callback is where I'm using a level trigger, and the callback can't perform the actions necessary to clear or change the input level.

The questions are (a) do we care about interrupts from external GPIO devices, and (b) if we do is there any API change that would resolve this, e.g. defining gpio_pin_interrupt_{enable,disable}() distinct from the interrupt configuration?

I think the answer should be (a) yes, and (b) no, which leads to (c) we need to document that callbacks for some GPIO drivers may be invoked from a helper thread rather than within the ISR itself.

@galak
Copy link
Collaborator Author

galak commented Oct 29, 2019

@jenmwms any update on the intel-apl or pcal9535a drivers?

@jenmwms
Copy link
Collaborator

jenmwms commented Oct 29, 2019 via email

@pabigot
Copy link
Collaborator

pabigot commented Jan 28, 2020

All drivers are converted on the topic branch.

@pabigot pabigot closed this as completed Jan 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment