Level triggered GPIO support

[JordanYates]

It has recently been raised (in #59459) that a substantial portion of GPIO drivers (46/76) do not support level triggered GPIO interrupts as defined in zephyr/drivers/gpio.h. Some variation on:

zephyr/drivers/gpio/gpio_stm32.c

Lines 614 to 617 in 732dd48

	if (mode == GPIO_INT_MODE_LEVEL) {
	err = -ENOTSUP;
	goto exit;
	}

This raises tensions when the semantics of level triggered interrupts are correct for a driver, especially when existing implementations use edge triggered interrupts. Do you leave bugs in the driver in order to continue supporting all vendors, or fix bugs that then causes previously supported platform regression.

In my opinion there are two possible actions to take:

1. Disallow usage of level triggered interrupts in-tree, as they are not sufficiently supported
2. Develop an emulation layer for hardware that does not support level triggered interrupts through a combination of edge interrupts and gpio_read

My preference is for 2, as the semantics of level triggered interrupts are obviously useful for certain drivers.

[erwango]

Adding GPIO collaborators: @henrikbrixandersen, @cfriedt, @mnkp

[cfriedt]

I could see option 2 working.

The tricky part would be when the user disables edge interrupts but enables level interrupts; internally, the driver would have to enable edge interrupts, not report that edge interrupts were enabled, etc.

Callbacks would need to trigger as long as the level was hi / lo, etc.

Yeah, it would be tricky

[savent404]

I have a proposal for that: First improve the gpio_callback fire routine, then we add a level-trigger simulator as a internal gpio callback.

The internal/external fire routine for gpio_callback could be like this below:

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 90a3065658..07ae664605 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -68,6 +68,12 @@ config GPIO_ENABLE_DISABLE_INTERRUPT
 	  on/off the interrupt signal without changing other registers, such as
 	  pending register, etc. The driver must implement it to work.
 
+config GPIO_INTERNAL_INTERRUPT
+	bool "Support for internal interrupt [EXPERIMENTAL]"
+	select EXPERIMENTAL
+	help
+	  This option enables the support for internal interrupt. The internal interrupt
+	  is the interrupt that is generated by the GPIO controller itself, not from user.
 
 source "drivers/gpio/Kconfig.b91"
 
diff --git a/include/zephyr/drivers/gpio.h b/include/zephyr/drivers/gpio.h
index 112a448e05..1475b6104e 100644
--- a/include/zephyr/drivers/gpio.h
+++ b/include/zephyr/drivers/gpio.h
@@ -508,6 +508,10 @@ struct gpio_callback {
 	 * an interrupt.
 	 */
 	gpio_port_pins_t pin_mask;
+
+#if CONFIG_GPIO_INTERNAL_INTERRUPT
+	bool is_internal;
+#endif
 };
 
 /**
@@ -1446,6 +1450,7 @@ static inline void gpio_init_callback(struct gpio_callback *callback,
 
 	callback->handler = handler;
 	callback->pin_mask = pin_mask;
+	callback->is_internal = false;
 }
 
 /**
diff --git a/include/zephyr/drivers/gpio/gpio_utils.h b/include/zephyr/drivers/gpio/gpio_utils.h
index 753640f4fb..f470e9d23a 100644
--- a/include/zephyr/drivers/gpio/gpio_utils.h
+++ b/include/zephyr/drivers/gpio/gpio_utils.h
@@ -66,7 +66,7 @@ static inline int gpio_manage_callback(sys_slist_t *callbacks,
  * @param port A pointer on the gpio driver instance
  * @param pins The actual pin mask that triggered the interrupt
  */
-static inline void gpio_fire_callbacks(sys_slist_t *list,
+static inline void gpio_fire_callbacks_general(sys_slist_t *list,
 					const struct device *port,
 					uint32_t pins)
 {
@@ -80,4 +80,80 @@ static inline void gpio_fire_callbacks(sys_slist_t *list,
 	}
 }
 
+#if CONFIG_GPIO_INTERNAL_INTERRUPT
+
+/**
+ * @brief Generic function to go through and fire callback from a callback list(but prefer internal callback)
+ *
+ * @param list A pointer on the gpio callback list
+ * @param port A pointer on the gpio driver instance
+ * @param pins The actual pin mask that triggered the interrupt
+ *
+ * @note if has internal callback, only fire internal callback
+ */
+static inline void _gpio_fire_callbacks_prefer_internal(sys_slist_t *list,
+					const struct device *port,
+					uint32_t pins)
+{
+	struct gpio_callback *cb, *tmp;
+	bool has_internal = false;
+
+	// check if has any internal callback for this pins
+	SYS_SLIST_FOR_EACH_CONTAINER_SAFE(list, cb, tmp, node) {
+		if (cb->pin_mask & pins && cb->is_internal) {
+			has_internal = true;
+			break;
+		}
+	}
+
+	SYS_SLIST_FOR_EACH_CONTAINER_SAFE(list, cb, tmp, node) {
+		if (cb->pin_mask & pins && (!has_internal || cb->is_internal)) {
+			__ASSERT(cb->handler, "No callback handler!");
+			cb->handler(port, cb, cb->pin_mask & pins);
+		}
+	}
+}
+
+/**
+ * @brief Generic function for fire external callbacks
+ *
+ * @param list A pointer on the gpio callback list
+ * @param port A pointer on the gpio driver instance
+ * @param pins The actual pin mask that triggered the interrupt
+ *
+ * @note this function will called in the internal callback when want to fire external callback
+ */
+static inline void gpio_fire_callbacks_force_external(sys_slist_t *list,
+					const struct device *port,
+					uint32_t pins)
+{
+	struct gpio_callback *cb, *tmp;
+
+	SYS_SLIST_FOR_EACH_CONTAINER_SAFE(list, cb, tmp, node) {
+		if (cb->pin_mask & pins && !cb->is_internal) {
+			__ASSERT(cb->handler, "No callback handler!");
+			cb->handler(port, cb, cb->pin_mask & pins);
+		}
+	}
+}
+#endif
+
+/**
+ * @brief Generic function to go through and fire callback from a callback list
+ *
+ * @param list A pointer on the gpio callback list
+ * @param port A pointer on the gpio driver instance
+ * @param pins The actual pin mask that triggered the interrupt
+ */
+static inline void gpio_fire_callbacks(sys_slist_t *list,
+					const struct device *port,
+					uint32_t pins)
+{
+#if CONFIG_GPIO_INTERNAL_INTERRUPT
+	gpio_fire_callbacks_prefer_internal(list, port, pins);
+#else
+	gpio_fire_callbacks_general(list, port, pins);
+#endif
+}
+
 #endif /* ZEPHYR_INCLUDE_DRIVERS_GPIO_GPIO_UTILS_H_ */

[cfriedt]

Why not make a PR? Please don't forget to add coverage to the testsuite. The GPIO emulator likely already has this feature, but that code could be externalized and shared with other drivers in e.g. gpio_common.c

[savent404]

great! try to do it

[GilDev]

Any news about this?

[savent404]

Forgot to work on it... Sorry for that.

I'll try it in this week. Let's find out what's new...

[GilDev]

Thanks! We just had to modify the MCP251xFD’s driver code with a colleague after much debugging because our target (STM32G4) doesn’t support level interrupts and the driver enforces it… There’s definitely a need for this!

[henrikbrixandersen]

• Disallow usage of level triggered interrupts in-tree, as they are not sufficiently supported
• Develop an emulation layer for hardware that does not support level triggered interrupts through a combination of edge interrupts and gpio_read

Isn't this just a matter of trying to interconnect two pieces of incompatible hardware? I mean, if a peripheral uses level-triggered interrupts via a GPIO, it needs to be connected to a GPIO controller with support for level-triggered interrupts.

I am not sold on neither of the two options presented above.

[GilDev]

In my specific use case, changing from level-triggered to edge-triggered interrupt didn’t change anything functionnaly.
It’s wired to a digital interrupt pin on my CAN controller. What would be the reason why level-triggered interrupts are enforced here?

[henrikbrixandersen]

In my specific use case, changing from level-triggered to edge-triggered interrupt didn’t change anything functionnaly. It’s wired to a digital interrupt pin on my CAN controller. What would be the reason why level-triggered interrupts are enforced here?

Do you know the difference between edge-triggered and level-triggered? Edge-triggered caused an edge (signal level change) to occur when an interrupt is triggered. Level-triggered causes the level of the signal to be asserted as long as an interrupt is active.

Handling these two types of interrupt requires support in the interrupt controller (here: GPIO controller acting as interrupt controller). If setting up the interrupt controller for the wrong type of interrupt, you risk either missing interrupts or an interrupt storm, depending on the combination.

[GilDev]

Indeed I’m not familiar with the difference, I only supposed how it worked according to the name, thanks for explaining.
I can imagine some use cases where this could be useful, but then is there any way to make my peripheral work on targets that do not implement level-triggered interrupts (as it is the case with a majority of GPIO controllers as reported by OP)? I’m not sure what the physical limitation is here: I can make it work outside of Zephyr without issues, other libraries don’t require this (it’s not even a subject)… I’m not sure what I am supposed to do other than changing the interrupt type in the driver…

[savent404]

PR created : #85401