[TOPIC-GPIO] drivers: gpio_sifive: Update for new GPIO API #19668

nategraff-sifive · 2019-10-07T22:51:55Z

Update the SiFive GPIO driver for the new GPIO API, including replacing deprecated flag names, implementing the port_* API surface, and moving interrupt configuration to pin_interrupt_configure.

One of the drivers tracked in #18530

zephyrbot · 2019-10-07T22:55:25Z

All checks are passing now.

checkpatch (informational only, not a failure)

-:308: WARNING:LONG_LINE_COMMENT: line over 80 characters
#308: FILE: drivers/gpio/gpio_sifive.c:340:
+		/* TODO: The interrupt functionality of this driver is incomplete,

-:310: WARNING:LONG_LINE_COMMENT: line over 80 characters
#310: FILE: drivers/gpio/gpio_sifive.c:342:
+		 * I'm just returning -ENOTSUP until we can track down the issue.

-:329: WARNING:LONG_LINE_COMMENT: line over 80 characters
#329: FILE: drivers/gpio/gpio_sifive.c:361:
+		/* TODO: The interrupt functionality of this driver is incomplete,

-:331: WARNING:LONG_LINE_COMMENT: line over 80 characters
#331: FILE: drivers/gpio/gpio_sifive.c:363:
+		 * I'm just returning -ENOTSUP until we can track down the issue.

- total: 0 errors, 4 warnings, 404 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

Your patch has style problems, please review.

NOTE: Ignored message types: AVOID_EXTERNS BRACES CONFIG_EXPERIMENTAL CONST_STRUCT DATE_TIME FILE_PATH_CHANGES MINMAX NETWORKING_BLOCK_COMMENT_STYLE PRINTK_WITHOUT_KERN_LEVEL SPLIT_STRING VOLATILE

NOTE: If any of the errors are false positives, please report
      them to the maintainers.

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

carlescufi · 2019-10-10T14:02:47Z

@nategraff-sifive
The topic branch has been rebased. Could you please rebase this PR against it?

nategraff-sifive · 2019-10-10T17:16:47Z

@carlescufi rebased 👍

drivers/gpio/gpio_sifive.c

nategraff-sifive · 2019-10-14T19:54:44Z

@mnkp, thanks for your feedback! I've updated the PR based on your suggestions. What do you think of the new configure? I'm setting/clearing all of the bits with the WRITE_BIT macro now.

mnkp

Thanks! LGTM with one remark.

It would be great if you could verify that the driver is passing tests/drivers/gpio/gpio_api_1pin/ testcase. You need to run it on the board which has LED0 defined in the board DTS file.

drivers/gpio/gpio_sifive.c

nategraff-sifive · 2019-10-14T21:47:25Z

I'm getting some test failures on tests/drivers/gpio/gpio_api_1pin/ on hifive1_revb

***** Booting Zephyr OS build zephyr-v2.0.0-1178-gfb2acd99d9d8 *****
Running test suite gpio_api_1pin_test
===================================================================
starting test - test_gpio_pin_configure_push_pull
Running test on port=gpio_0, pin=19
PASS - test_gpio_pin_configure_push_pull
===================================================================
starting test - test_gpio_pin_configure_single_ended
Running test on port=gpio_0, pin=19
Pull Up / Pull Down pin bias is not supported
SKIP - test_gpio_pin_configure_single_ended
===================================================================
starting test - test_gpio_pin_set_get_raw
Running test on port=gpio_0, pin=19
PASS - test_gpio_pin_set_get_raw
===================================================================
starting test - test_gpio_pin_set_get
Running test on port=gpio_0, pin=19
PASS - test_gpio_pin_set_get
===================================================================
starting test - test_gpio_pin_set_get_active_high
Running test on port=gpio_0, pin=19
Step 1: Set logical, get logical and physical pin value
Step 2: Set physical, get logical and physical pin value
PASS - test_gpio_pin_set_get_active_high
===================================================================
starting test - test_gpio_pin_set_get_active_low
Running test on port=gpio_0, pin=19
Step 1: Set logical, get logical and physical pin value
Step 2: Set physical, get logical and physical pin value
PASS - test_gpio_pin_set_get_active_low
===================================================================
starting test - test_gpio_pin_toggle
Running test on port=gpio_0, pin=19
PASS - test_gpio_pin_toggle
===================================================================
starting test - test_gpio_port_set_masked_get_raw
Running test on port=gpio_0, pin=19
PASS - test_gpio_port_set_masked_get_raw
===================================================================
starting test - test_gpio_port_set_masked_get
Running test on port=gpio_0, pin=19
PASS - test_gpio_port_set_masked_get
===================================================================
starting test - test_gpio_port_set_masked_get_active_high
Running test on port=gpio_0, pin=19
Step 1: Set logical, get logical and physical port value
Step 2: Set physical, get logical and physical port value
PASS - test_gpio_port_set_masked_get_active_high
===================================================================
starting test - test_gpio_port_set_masked_get_active_low
Running test on port=gpio_0, pin=19
Step 1: Set logical, get logical and physical port value
Step 2: Set physical, get logical and physical port value
PASS - test_gpio_port_set_masked_get_active_low
===================================================================
starting test - test_gpio_port_set_bits_clear_bits_raw
Running test on port=gpio_0, pin=19
PASS - test_gpio_port_set_bits_clear_bits_raw
===================================================================
starting test - test_gpio_port_set_bits_clear_bits
Running test on port=gpio_0, pin=19
PASS - test_gpio_port_set_bits_clear_bits
===================================================================
starting test - test_gpio_port_set_clr_bits_raw
Running test on port=gpio_0, pin=19
PASS - test_gpio_port_set_clr_bits_raw
===================================================================
starting test - test_gpio_port_set_clr_bits
Running test on port=gpio_0, pin=19
PASS - test_gpio_port_set_clr_bits
===================================================================
starting test - test_gpio_port_toggle
Running test on port=gpio_0, pin=19
PASS - test_gpio_port_toggle
===================================================================
starting test - test_gpio_int_edge_rising
Running test on port=gpio_0, pin=19

    Assertion failed at ZEPHYR_BASE/tests/drivers/gpio/gpio_api_1pin/src/test_pin_interrupt.c:105: test_gpio_pin_interrupt_edge: (cb_count not equal to cb_count_expected)
Test point 0: Pin interrupt triggered invalid number of times on rising/to active edge
FAIL - test_gpio_int_edge_rising
===================================================================
starting test - test_gpio_int_edge_falling
Running test on port=gpio_0, pin=19

    Assertion failed at ZEPHYR_BASE/tests/drivers/gpio/gpio_api_1pin/src/test_pin_interrupt.c:113: test_gpio_pin_interrupt_edge: (cb_count not equal to cb_count_expected)
Test point 0: Pin interrupt triggered invalid number of times on falling/to inactive edge
FAIL - test_gpio_int_edge_falling
===================================================================
starting test - test_gpio_int_edge_both
Running test on port=gpio_0, pin=19

    Assertion failed at ZEPHYR_BASE/tests/drivers/gpio/gpio_api_1pin/src/test_pin_interrupt.c:105: test_gpio_pin_interrupt_edge: (cb_count not equal to cb_count_expected)
Test point 0: Pin interrupt triggered invalid number of times on rising/to active edge
FAIL - test_gpio_int_edge_both
===================================================================
starting test - test_gpio_int_edge_to_active
Step 1: Configure pin as active high
Running test on port=gpio_0, pin=19

    Assertion failed at ZEPHYR_BASE/tests/drivers/gpio/gpio_api_1pin/src/test_pin_interrupt.c:105: test_gpio_pin_interrupt_edge: (cb_count not equal to cb_count_expected)
Test point 0: Pin interrupt triggered invalid number of times on rising/to active edge
FAIL - test_gpio_int_edge_to_active
===================================================================
starting test - test_gpio_int_edge_to_inactive
Step 1: Configure pin as active high
Running test on port=gpio_0, pin=19

    Assertion failed at ZEPHYR_BASE/tests/drivers/gpio/gpio_api_1pin/src/test_pin_interrupt.c:113: test_gpio_pin_interrupt_edge: (cb_count not equal to cb_count_expected)
Test point 0: Pin interrupt triggered invalid number of times on falling/to inactive edge
FAIL - test_gpio_int_edge_to_inactive
===================================================================
starting test - test_gpio_int_level_high
Running test on port=gpio_0, pin=19

    Assertion failed at ZEPHYR_BASE/tests/drivers/gpio/gpio_api_1pin/src/test_pin_interrupt.c:186: test_gpio_pin_interrupt_level: (cb_count not equal to cb_count_expected)
Test point 0: Pin interrupt triggered invalid number of times on level 1
FAIL - test_gpio_int_level_high
===================================================================
starting test - test_gpio_int_level_low
Running test on port=gpio_0, pin=19

    Assertion failed at ZEPHYR_BASE/tests/drivers/gpio/gpio_api_1pin/src/test_pin_interrupt.c:186: test_gpio_pin_interrupt_level: (cb_count not equal to cb_count_expected)
Test point 0: Pin interrupt triggered invalid number of times on level 0
FAIL - test_gpio_int_level_low
===================================================================
starting test - test_gpio_int_level_active
Step 1: Configure pin as active high
Running test on port=gpio_0, pin=19

    Assertion failed at ZEPHYR_BASE/tests/drivers/gpio/gpio_api_1pin/src/test_pin_interrupt.c:186: test_gpio_pin_interrupt_level: (cb_count not equal to cb_count_expected)
Test point 0: Pin interrupt triggered invalid number of times on level 1
FAIL - test_gpio_int_level_active
===================================================================
starting test - test_gpio_int_level_inactive
Step 1: Configure pin as active high
Running test on port=gpio_0, pin=19

    Assertion failed at ZEPHYR_BASE/tests/drivers/gpio/gpio_api_1pin/src/test_pin_interrupt.c:186: test_gpio_pin_interrupt_level: (cb_count not equal to cb_count_expected)
Test point 0: Pin interrupt triggered invalid number of times on level 0
FAIL - test_gpio_int_level_inactive
===================================================================
starting test - test_gpio_pin_toggle_visual
Running test on port=gpio_0, pin=19
LED ON
LED OFF
LED ON
LED OFF
PASS - test_gpio_pin_toggle_visual
===================================================================
Test suite gpio_api_1pin_test failed.
===================================================================
PROJECT EXECUTION FAILED

nategraff-sifive · 2019-10-14T21:59:07Z

One other thing I noticed while running the test is that the LED polarity is inverted from what the test thinks. The LED0 GPIO pin is on the cathode of the LED, so when the GPIO is driven low the LED is turned on. During test_gpio_pin_toggle_visual, the LED is ON when the test says it's OFF.

Schematic for the HiFive1 Rev B LEDs (Go to page 3)

What are the odds this is the cause of the interrupt failures in the test?

mnkp · 2019-10-14T22:04:46Z

According to the testcase log the interrupts are not triggered at all. Since test for GPIO_INT_EDGE_BOTH also fails, so it's not polarity related. Does the button sample samples/basic/button/ work?

The trace messages printed by the testcase are not very good at the moment since they don't print actual and expected values. I didn't do it explicitly in the testcase hoping that this issue will be solved globally. The assert macros should print actual and expected values automatically, always. Unfortunately, that's not so easy to implement.

mnkp · 2019-10-14T22:09:10Z

To fix the LED polarity you need to add GPIO_ACTIVE_LOW to the board dts file boards/riscv/hifive1_revb/hifive1_revb.dts. Look e.g. at boards/arm/sam_e70_xplained/sam_e70_xplained.dts as an example.

mnkp · 2019-10-14T22:17:24Z

There is one more issue I overlooked, in the gpio_sifive_irq_handler function the interrupt status has to be cleared first, before gpio_fire_callbacks function is called. Otherwise a second edge interrupt that happens when the callback is being executed will be missed.

mnkp · 2019-10-14T22:19:01Z

I will re-review the code when the pending issues are solved.

carlescufi · 2019-10-15T16:26:16Z

@nategraff-sifive could you address the remaining issues please?

nategraff-sifive · 2019-10-15T16:50:39Z

@carlescufi I'm working on it!

carlescufi · 2019-10-15T16:51:56Z

@carlescufi I'm working on it!

Thanks Nate.

mnkp · 2019-10-15T19:14:26Z

It would be probably best if you ask a question on #devicetree channel in Slack.

Just to double check, you could run a quick test and modify IRQ_CONNECT macro in gpio_sifive.c to something like.

-IRQ_CONNECT(DT_INST_0_SIFIVE_GPIO0_IRQ_##n,    \
+IRQ_CONNECT((8 + n),   \

and verify that after the modification the interrupts are firing correctly.

nategraff-sifive · 2019-10-15T20:26:13Z

The devicetree channel is a good idea, I'll ask there.

So the PLIC IRQ numbers are actually offset by 12, so the change to IRQ_CONNECT is

-IRQ_CONNECT(DT_INST_0_SIFIVE_GPIO0_IRQ_##n,    \
+IRQ_CONNECT((12 + 8 + n),   \

...which doesn't work, even though the generated _sw_isr_table has the SiFive GPIO irq_handlers at the right offsets 20-51.

nategraff-sifive · 2019-10-15T20:40:23Z

The weird IRQ numbers are said to be a result of multi-level IRQ handling, so the issue still lies elsewhere.

nategraff-sifive · 2019-10-15T21:11:31Z

I found it!

gpio_sifive_enable_callback calls irq_enable(cfg->gpio_irq_base + pin);, which no longer works because of the multi-level IRQ numbers.

Edit: lots of stuff in the driver suffers from the assumption that gpio_irq_base does what it used to. Anyway, I have some work to do. :)

mnkp · 2019-10-15T21:36:00Z

Nice! Good job! :)

But it reminds me that there is one more thing to fix. The gpio_sifive_pin_interrupt_configure function needs to enable / disable interrupt line as instructed by the mode parameter. So it needs to call irq_enable / irq_disable. The gpio_sifive_enable_callback / gpio_sifive_disable_callback functions will be deprecated.

nategraff-sifive · 2019-10-21T18:21:49Z

Progress update:

I just pushed a new commit with my work so far to update the driver for multi-level interrupts.

It still fails all of the interrupt tests in tests/drivers/gpio/gpio_api_1pin, except it gets to step 2 on test_gpio_int_edge_to_active

===================================================================
starting test - test_gpio_int_edge_to_active
Step 1: Configure pin as active high
Running test on port=gpio_0, pin=19
Step 2: Configure pin as active low
Running test on port=gpio_0, pin=19

    Assertion failed at ZEPHYR_BASE/tests/drivers/gpio/gpio_api_1pin/src/test_pin_interrupt.c:105: test_gpio_pin_interrupt_edge: (cb_count not equal to cb_count_expected)
Test point 0: Pin interrupt triggered invalid number of times on rising/to active edge
FAIL - test_gpio_int_edge_to_active

drivers/gpio/gpio_sifive.c

carlescufi · 2019-10-22T16:26:45Z

@nategraff-sifive could you fix the CI issues and reply to @mnkp 's comment above about the interrupt handling?

nategraff-sifive · 2019-10-22T23:37:36Z

Clearing the pending register before enabling the GPIO interrupt didn't fix it. I'm still digging around. The problem is definitely spurious interrupts, I can confirm that the callbacks are firing too much, not too little.

galak · 2019-10-25T12:59:44Z

We rebased the topic-gpio branch against master, so this PR needs to be rebased on the updated topic-gpio branch.

nategraff-sifive · 2019-10-25T17:23:05Z

Rebased on topic-gpio, still getting some spurious interrupts, still digging.

galak · 2019-10-29T05:33:22Z

Rebased on topic-gpio, still getting some spurious interrupts, still digging.

Any luck? This is one of the few remaining GPIO drivers that needs to get converted. Thanks.

nategraff-sifive · 2019-10-30T16:57:32Z

@galak I have to admit I'm pretty stuck. Is it worth just disabling interrupts for this driver (which I'm pretty sure were already broken) to get the API change merged?

pabigot · 2019-10-30T17:05:57Z

I think that'd be appropriate. We can continue to work on it after everything's merged to master.

Update the Sifive GPIO driver for new API Signed-off-by: Nathaniel Graff <[email protected]>

The interrupt functionality of this driver is incomplete, but for the sake of not slowing down the GPIO API refactor, I'm just returning -ENOTSUP until we can track down the issue. Signed-off-by: Nathaniel Graff <[email protected]>

nategraff-sifive · 2019-10-31T17:23:08Z

@galak, @pabigot, @mnkp tests/drivers/gpio/gpio_api_1pin passes now with the interrupt tests skipped

pabigot · 2019-10-31T20:54:15Z

@nategraff-sifive Have you tried tests/drivers/gpio/gpio_basic_api (the 2-pin test)?

mnkp

LGTM with a minor comment.

mnkp · 2019-11-03T22:59:01Z

drivers/gpio/gpio_sifive.c

+}
+
+/* Given gpio_irq_base and the pin number, return the IRQ number for the pin */
+static inline unsigned int gpio_sifive_pin_irq(unsigned int base_irq, int pin)


In general we use static inline only for the functions placed in a header. If the function is internal to the source file it's usually better to leave the decision weather to inline a function (or not) to the compiler. If the function is small, used once the compiler will inline a function even without the inline keyword.

mnkp · 2019-11-03T23:03:57Z

drivers/gpio/gpio_sifive.c

 #define DEV_GPIO_DATA(dev)				\
 	((struct gpio_sifive_data *)(dev)->driver_data)

+/* _irq_level and _level2_irq are copied from


Let's prefix this paragraph with TODO:.

galak · 2019-11-04T14:51:42Z

Going ahead and merging this and will open some issues for the TODO items.

nategraff-sifive added area: Drivers area: GPIO labels Oct 7, 2019

nategraff-sifive requested a review from galak October 7, 2019 22:51

nategraff-sifive force-pushed the sifive-gpio-api-update branch from a064fb7 to 3c75671 Compare October 7, 2019 22:56

galak mentioned this pull request Oct 8, 2019

Convert GPIO drivers to new GPIO API #18530

Closed

pabigot force-pushed the topic-gpio branch from 67daa80 to 2b556e1 Compare October 10, 2019 13:57

nategraff-sifive force-pushed the sifive-gpio-api-update branch from 3c75671 to 693149e Compare October 10, 2019 17:15

mnkp self-requested a review October 13, 2019 17:51

mnkp requested changes Oct 13, 2019

View reviewed changes

nategraff-sifive force-pushed the sifive-gpio-api-update branch from 693149e to 9306d93 Compare October 14, 2019 19:53

nategraff-sifive requested a review from pabigot as a code owner October 14, 2019 19:53

mnkp reviewed Oct 14, 2019

View reviewed changes

drivers/gpio/gpio_sifive.c Outdated Show resolved Hide resolved

nategraff-sifive force-pushed the sifive-gpio-api-update branch from 9306d93 to fb2acd9 Compare October 14, 2019 21:36

mnkp approved these changes Oct 14, 2019

View reviewed changes

mnkp self-requested a review October 14, 2019 22:18

nategraff-sifive force-pushed the sifive-gpio-api-update branch from fb2acd9 to 1dcb681 Compare October 15, 2019 17:37

nategraff-sifive requested a review from kgugala as a code owner October 15, 2019 17:37

nategraff-sifive force-pushed the sifive-gpio-api-update branch from 1dcb681 to 7b7e019 Compare October 21, 2019 18:18

zephyrbot added the area: Boards label Oct 21, 2019

mnkp reviewed Oct 21, 2019

View reviewed changes

drivers/gpio/gpio_sifive.c Outdated Show resolved Hide resolved

drivers/gpio/gpio_sifive.c Outdated Show resolved Hide resolved

drivers/gpio/gpio_sifive.c Outdated Show resolved Hide resolved

nategraff-sifive force-pushed the sifive-gpio-api-update branch from 7b7e019 to 2927c86 Compare October 22, 2019 23:36

galak force-pushed the topic-gpio branch from 315246d to 653e004 Compare October 25, 2019 12:55

galak force-pushed the topic-gpio branch from 653e004 to 2222ac6 Compare October 25, 2019 16:16

nategraff-sifive force-pushed the sifive-gpio-api-update branch from 2927c86 to b2492b7 Compare October 25, 2019 17:20

nategraff-sifive force-pushed the sifive-gpio-api-update branch from b2492b7 to ef8ef1f Compare October 30, 2019 17:20

nategraff-sifive added 2 commits October 30, 2019 14:18

drivers: gpio_sifive: Update for new GPIO API

37e7116

Update the Sifive GPIO driver for new API Signed-off-by: Nathaniel Graff <[email protected]>

drivers: gpio_sifive: Disable interrupts

93d47fb

The interrupt functionality of this driver is incomplete, but for the sake of not slowing down the GPIO API refactor, I'm just returning -ENOTSUP until we can track down the issue. Signed-off-by: Nathaniel Graff <[email protected]>

nategraff-sifive force-pushed the sifive-gpio-api-update branch from ef8ef1f to 93d47fb Compare October 30, 2019 21:19

mnkp reviewed Nov 3, 2019

View reviewed changes

galak merged commit 1097995 into zephyrproject-rtos:topic-gpio Nov 4, 2019

nategraff-sifive deleted the sifive-gpio-api-update branch November 4, 2019 17:52

[TOPIC-GPIO] drivers: gpio_sifive: Update for new GPIO API #19668

[TOPIC-GPIO] drivers: gpio_sifive: Update for new GPIO API #19668

Uh oh!

Conversation

nategraff-sifive commented Oct 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zephyrbot commented Oct 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

checkpatch (informational only, not a failure)

Uh oh!

carlescufi commented Oct 10, 2019

Uh oh!

nategraff-sifive commented Oct 10, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nategraff-sifive commented Oct 14, 2019

Uh oh!

mnkp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nategraff-sifive commented Oct 14, 2019

Uh oh!

nategraff-sifive commented Oct 14, 2019

Uh oh!

mnkp commented Oct 14, 2019

Uh oh!

mnkp commented Oct 14, 2019

Uh oh!

mnkp commented Oct 14, 2019

Uh oh!

mnkp commented Oct 14, 2019

Uh oh!

carlescufi commented Oct 15, 2019

Uh oh!

nategraff-sifive commented Oct 15, 2019

Uh oh!

carlescufi commented Oct 15, 2019

Uh oh!

mnkp commented Oct 15, 2019

Uh oh!

nategraff-sifive commented Oct 15, 2019

Uh oh!

nategraff-sifive commented Oct 15, 2019

Uh oh!

nategraff-sifive commented Oct 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mnkp commented Oct 15, 2019

Uh oh!

nategraff-sifive commented Oct 21, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

carlescufi commented Oct 22, 2019

Uh oh!

nategraff-sifive commented Oct 22, 2019

Uh oh!

galak commented Oct 25, 2019

Uh oh!

nategraff-sifive commented Oct 25, 2019

Uh oh!

galak commented Oct 29, 2019

Uh oh!

nategraff-sifive commented Oct 30, 2019

Uh oh!

pabigot commented Oct 30, 2019

Uh oh!

nategraff-sifive commented Oct 31, 2019

Uh oh!

pabigot commented Oct 31, 2019

Uh oh!

nategraff-sifive commented Oct 7, 2019 •

edited

Loading

zephyrbot commented Oct 7, 2019 •

edited

Loading

nategraff-sifive commented Oct 15, 2019 •

edited

Loading