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

[topic-gpio] drivers: gpio_stm32: update to use new GPIO API #19579

Merged
merged 15 commits into from
Oct 16, 2019

Conversation

erwango
Copy link
Member

@erwango erwango commented Oct 3, 2019

Update driver code to use new configuration flags such as GPIO_ACTIVE_LOW.
STM32F1 series not taken into account.
Only one board converted.
No backward compatibility: gpio_configure don't configure interrupts anymore

Based on:

TODO:

EDIT:
PR description rework after push on 09-09-19

@erwango erwango requested a review from galak October 3, 2019 13:06
@erwango erwango added the platform: STM32 ST Micro STM32 label Oct 3, 2019
@erwango
Copy link
Member Author

erwango commented Oct 3, 2019

@mniestroj, FYI

@galak galak changed the title drivers: gpio_stm32: update to use new GPIO API [topic-gpio] drivers: gpio_stm32: update to use new GPIO API Oct 3, 2019
@erwango
Copy link
Member Author

erwango commented Oct 3, 2019

Tested ok on gpio_basic_api but FATAL error when test is run w/o wire.
So this requires some more analysis.

@erwango
Copy link
Member Author

erwango commented Oct 3, 2019

Tested ok on gpio_basic_api but FATAL error when test is run w/o wire.
So this requires some more analysis.

Seems to be intended behavior.

@galak
Copy link
Collaborator

galak commented Oct 3, 2019

Tested ok on gpio_basic_api but FATAL error when test is run w/o wire.
So this requires some more analysis.

Seems to be intended behavior.

Yeah, seen the same thing myself

@galak
Copy link
Collaborator

galak commented Oct 3, 2019

  • Get all STM32 based boards converted to the new API

While maybe not the most efficient, here's an example to help convert the dts files over:

git grep -l \<st boards/arm | grep dts | xargs sed -i 's/GPIO_INT_ACTIVE_HIGH/GPIO_ACTIVE_LOW/g'

@galak galak added area: GPIO DNM This PR should not be merged (Do Not Merge) labels Oct 3, 2019
@galak
Copy link
Collaborator

galak commented Oct 3, 2019

Marking dnm since there are some todo items.

@zephyrbot zephyrbot added area: Boards area: Tests Issues related to a particular existing or missing test labels Oct 3, 2019
@zephyrbot
Copy link
Collaborator

zephyrbot commented Oct 3, 2019

All checks are passing now.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

@erwango
Copy link
Member Author

erwango commented Oct 3, 2019

recheck

Copy link
Collaborator

@pabigot pabigot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Observed one flag test using the zero-valued variant, which needs to be fixed. One or two other comments.

Checked on nucleo_l476rg, things passed.

drivers/gpio/gpio_stm32.c Show resolved Hide resolved
drivers/gpio/gpio_stm32.c Outdated Show resolved Hide resolved
drivers/gpio/gpio_stm32.c Outdated Show resolved Hide resolved
Copy link
Member

@mnkp mnkp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe it would be better to have all stm32 changes in one PR instead of two. It makes it difficult to do the review since both PRs modify driver code. Also by not running checks on all the boards we may miss some issues. In case of Shippable timeout it's better to add a temporary patch from @galak that fixes the issue.

drivers/gpio/gpio_stm32.c Outdated Show resolved Hide resolved
drivers/gpio/gpio_stm32.c Outdated Show resolved Hide resolved
drivers/gpio/gpio_stm32.c Outdated Show resolved Hide resolved
drivers/gpio/gpio_stm32.c Outdated Show resolved Hide resolved
@erwango
Copy link
Member Author

erwango commented Oct 7, 2019

@mnkp Thanks for review!

It makes it difficult to do the review since both PRs modify driver code.

This should not be the case PR #19067 only adds commit 717de74 which impact is limited to boards

Also by not running checks on all the boards we may miss some issues. In case of Shippable timeout it's better to add a temporary patch from @galak that fixes the issue.

Agree it's better to actually get the results on all boards. @galak how can we make this happen?

@pabigot
Copy link
Collaborator

pabigot commented Oct 7, 2019

@erwango As suggested to me by @carlescufi:

From e888dbf355ca8a63ec17ed725368ebb2ec79a200 Mon Sep 17 00:00:00 2001
From: Peter Bigot <peter.bigot@nordicsemi.no>
Date: Sun, 6 Oct 2019 15:05:28 -0500
Subject: [PATCH] DNM shippable.yml: increase builds to avoid timeout failures

Signed-off-by: Peter Bigot <peter.bigot@nordicsemi.no>
---
 .shippable.yml | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/.shippable.yml b/.shippable.yml
index 605fd41be8..b50342a5ca 100644
--- a/.shippable.yml
+++ b/.shippable.yml
@@ -6,13 +6,20 @@ env:
     global:
         - ZEPHYR_SDK_INSTALL_DIR=/opt/sdk/zephyr-sdk-0.10.3
         - ZEPHYR_TOOLCHAIN_VARIANT=zephyr
-        - MATRIX_BUILDS="5"
+        - MATRIX_BUILDS="12"
     matrix:
         - MATRIX_BUILD="1"
         - MATRIX_BUILD="2"
         - MATRIX_BUILD="3"
         - MATRIX_BUILD="4"
         - MATRIX_BUILD="5"
+        - MATRIX_BUILD="6"
+        - MATRIX_BUILD="7"
+        - MATRIX_BUILD="8"
+        - MATRIX_BUILD="9"
+        - MATRIX_BUILD="10"
+        - MATRIX_BUILD="11"
+        - MATRIX_BUILD="12"
 
 build:
     cache: false
-- 
2.20.1

Also #19562 (comment)

@erwango
Copy link
Member Author

erwango commented Oct 7, 2019

@pabigot

@erwango As suggested to me by @carlescufi:

Thanks, will use that.

@erwango
Copy link
Member Author

erwango commented Oct 11, 2019

Status following last push:

Tested on all STM32 series.

Test status:

  • gpio_basic_api: Passed on all available boards except stm32h747_disco_m7, cf below
  • gpio_api_1pin: Passed on available boards except stm32h747_disco_m7, and stm32mp157c_dk2 cf below
  • button: Passed on all available boards

Test failures on stm32h747_disco_m7:

  • gpio_basic_api:
    Playing with other boards, I've seen several times that this specific failure can happen when pin selected is in conflict with other usage. I've not been able to identify the conflict yet, but I heavily suspsect this issue is purely a configuration issue.
  • gpio_api_1pin:
    Can be PASSED if changing pin.

STM32H7 port is still very basic for now (only GPIO, UART) and globally a wip.
Given this status and board complexity that might explain itself tests failures, I think it is safe to enable new GPIO API on this series.

Test failures on stm32mp157c_dk2:

  • gpio_api_1pin: Failing point relates to port testing. Due to use of Linux on Cortex-A, lot of pins are configured for various purposes (ethernet, display, ...), there is no empty port available for testing matching pins available on arduino connector. I'll have a new try once [TOPIC-GPIO] gpi_api_1pin test failures #19692 is fixed.

Given board complexity that might explain tests failures, I think it is safe to enable new GPIO API on this series.

Summary

Unless reviewers comment, I consider the work done on this PR.

@erwango erwango removed the DNM This PR should not be merged (Do Not Merge) label Oct 11, 2019
@carlescufi
Copy link
Member

@erwango could you please rebase?

mniestroj and others added 15 commits October 16, 2019 08:51
This allows compiler to inline function body and reduce overall code
size.

Signed-off-by: Marcin Niestroj <m.niestroj@grinn-global.com>
This patch doesn't change functionality, but is only related to improved
readability and reusability.

Signed-off-by: Marcin Niestroj <m.niestroj@grinn-global.com>
Up to now interrupts could be only configured once, with no way to
disable them in runtime.

Allow interrupts to be disabled in runtime and then properly reenabled
on user request. This allows to ignore interrupts when software is not
expecting them.

The improvement over previously reverted patch [1] is that we disable
interrupts only when we configure port for which interrupt line was
previously selected. This for example prevents to disable interrupts
line 2 in case PA2 was previously configured as interrupt source, but we
are currently configuring PB2 as output.

[1] 0951ce2 ("gpio: stm32: support disabling and reenabling
  interrupts on pin")

Signed-off-by: Marcin Niestroj <m.niestroj@grinn-global.com>
Update STM32 GPIO driver to support new GPIO API.

Signed-off-by: Erwan Gouriou <erwan.gouriou@linaro.org>
Perform few clean up in stm32 gpio driver:
*Clean up uint32_t occurrences left over.
*Move function gpio_stm32_flags_to_conf from const to static
and remove useless parameter check
*Rework error handling in functoin gpio_stm32_config
*Remove gpio_stm32_int_enabled_port function and have direct call
to gpio_stm32_get_exti_source

Signed-off-by: Erwan Gouriou <erwan.gouriou@linaro.org>
Since it is now possible to disable/re-enable interrupts and
also to reconfigure an already configured interrupt, it is
now required to clear non requested triggers.
While it is not strictly requested, triggers are also cleared
when interrupt is disabled (assuming trigger should be configured
when interrupt is enabled).

Signed-off-by: Erwan Gouriou <erwan.gouriou@linaro.org>

fixup exti
Move GPIO_ACTIVE_INT_HIGH/LOW to GPIO_ACTIVE_HIGH/LOW.

Signed-off-by: Erwan Gouriou <erwan.gouriou@linaro.org>
Add boards overlay to enable gpio_basic tests.
Selected boards should reflect various stm32 series.

Signed-off-by: Erwan Gouriou <erwan.gouriou@linaro.org>
On some stm32 based boards buttons and leds configuration
was wrong. Fix that.

Signed-off-by: Erwan Gouriou <erwan.gouriou@linaro.org>
Add arduino connection dts description and update target
yaml files.

Signed-off-by: Erwan Gouriou <erwan.gouriou@linaro.org>
Argument 'port' in stm32_exti_set_callback function is not
used, remove it.

Signed-off-by: Erwan Gouriou <erwan.gouriou@linaro.org>
stm32_exti_enable was returning errors on line > 32 or line pointing
to non implemented line. Both conditions are hard-coded, hence there
is no use to detect them dynamically in the code.
Check them with assert. As a consequence, function could now be void.

Additionally, enable exti irq line only if both checks are passed.

Signed-off-by: Erwan Gouriou <erwan.gouriou@linaro.org>
Add led and button to board description to enable basic gpio
tests.

Signed-off-by: Erwan Gouriou <erwan.gouriou@linaro.org>
Test generates lengthy log.

Signed-off-by: Erwan Gouriou <erwan.gouriou@linaro.org>
Change codeowner following driver indeep rework.


Signed-off-by: Erwan Gouriou <erwan.gouriou@linaro.org>
@erwango
Copy link
Member Author

erwango commented Oct 16, 2019

@erwango could you please rebase?

@carlescufi, done.

@MaureenHelm MaureenHelm merged commit d170e2e into zephyrproject-rtos:topic-gpio Oct 16, 2019
/* trigger on rising edge */
STM32_EXTI_TRIG_RISING = 0x1,
/* trigger on falling endge */
STM32_EXTI_TRIG_FALLING = 0x2,
/* trigger on falling endge */
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

endge?

@erwango erwango deleted the gpio_stm32 branch June 5, 2020 10:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Boards area: GPIO area: Tests Issues related to a particular existing or missing test platform: STM32 ST Micro STM32
Projects
None yet
Development

Successfully merging this pull request may close these issues.