Fix GPIO framework to prevent glitch when requesting an output GPIO #7478

tbourgoi · 2025-07-09T15:15:30Z

Hello,

During internal test I discovered that a voltage glitch appeared on boards with discrete regulators.
After further investigation I find out that when a GPIO is retrieved from the device tree it's configured as input and then the function gpio_set_direction() is called to configure the GPIO as output.
This intermediate configuration creates a glitch on the voltage line, we should only configure once a GPIO. Especially since the configuration might already be set by the previous bootloader.

When discussing the issue with @patrickdelaunay we found another bug in the regulator_gpio and regulator_fixed drivers.
The properties enable-active-high, gpio-open-drain from the device tree are not taken into account. The device tree is parsed and variable gpio->dt_flags is updated but the configuration of the GPIO with the device tree flags is done in function gpio_dt_get_by_index() which is called before parsing the custom options.

So this PR does the following :

Update gpio_dt_get_by_index() to search for properties with and without suffixes
Add .configure() ops in GPIO framework configure a GPIO after the parsing of custom properties.
Add function gpio_dt_cfg_by_index() to request a GPIO from the DT and configure it.

Thomas

Align the OP-TEE GPIO driver on the Linux GPIO lib behavior for the gpio properties in device tree: search "gpios" or "gpio" when gpio_name is omitted and "foo-gpios" or "foo-gpio" when and if the gpio_name="foo". This patch allows use of the function gpio_dt_get_by_index() by GPIO consumers for all supported bindings. Signed-off-by: Patrick Delaunay <[email protected]> Signed-off-by: Thomas Bourgoin <[email protected]>

Use gpio_dt_get_by_index() to get the properties in device tree with all the possible variant name: "gpio" and "gpios". Signed-off-by: Patrick Delaunay <[email protected]> Signed-off-by: Thomas Bourgoin <[email protected]>

Align the OP-TEE GPIO driver with GPIO lib (devm_gpiod_get_index with enum gpiod_flags) and add a way to select the GPIO configuration directly during request with the new function, gpio_dt_cfg_by_index(). This patch remove assumption on GPIO direction (gpio input was assumed in GPIO driver) or output level when the GPIO is requested by consumer. with this patch it must be requested explicitly with: - GPIO_IN: configure the input GPIO - GPIO_ASIS: direction is managed by caller later with gpio_set_value() call. - GPIO_OUT_LOW / GPIO_OUT_HIGH: output GPIO at expected level, the sequence for GPIO have the correct order to avoid glitch by using the gpio function at the correct order. This patch is a preliminary step for the introduction of ops configure. The API gpio_dt_get_by_index() is keep for backward compatibility but should be used with gpio_configure() or replaced by gpio_dt_cfg_by_index(). Now gpio_configure() is only used in the 2 drivers, regulator_fixed and regulator_gpio, because in these drivers the flags gpio->dt_flags are modified after call of gpio_dt_get_by_index(). For get_voltage_level_gpio(), GPIO_OUT_HIGH is used by default as it is done in Linux function of_get_gpio_regulator_config(). Signed-off-by: Patrick Delaunay <[email protected]> Signed-off-by: Thomas Bourgoin <[email protected]>

Add the new ops configure to initialize the GPIO resource based on dt_flags. This new ops is require to support the modification of dt_flag done by caller after gpio_dt_init_by_index() and before call of gpio_configure(). It is done by example in driver of regulator fixed and gpio. The new ops is called only in gpio_dt_cfg_by_index() or in gpio_configure() so usage of one of this API is manadatory after this patch. Signed-off-by: Patrick Delaunay <[email protected]> Signed-off-by: Thomas Bourgoin <[email protected]>

Move the GPIO configuration based on dt_flags previously done in get_dt ops in the new gpio configure ops. To avoid glitch on GPIO line, the write in register GPIO_MODER is only kept in the ops stm32_gpio_set_direction(), called by gpio framework function gpio_configure() after the call of configure ops, so we have no more transient GPIO input configuration. Moreover, this patch allows to correctly apply the dt_flags when they are modify after the call of stm32_gpio_get_dt(), for example in drivers regulator fixed and gpio. Signed-off-by: Patrick Delaunay <[email protected]> Signed-off-by: Thomas Bourgoin <[email protected]>

etienne-lms

Some preliminary comments...

etienne-lms · 2025-07-09T16:06:03Z

core/drivers/gpio/gpio.c

+	for (i = 0; i < ARRAY_SIZE(gpio_suffixes); i++) {
+		if (gpio_name)
+			snprintf(prop_name, sizeof(prop_name), "%s-%s",
+				 gpio_name, gpio_suffixes[i]);


snprintf() return value to check? I could not find where in the DT spec property names shall be at most 32bytes (including terminal \0 char?)

The limit of 32 bytes depends on the devicetree specifications :
https://devicetree-specification.readthedocs.io/en/v0.3/devicetree-basics.html#properties
2.2.4.1.
In linux the function of_find_gpio() has the following comment :
char propname[32]; /* 32 is max size of property name */

Ok, clear. Thanks.

For consistency and help maintenance, could you introduce a DT_PROP_NAME_SIZE_MAX macro in core/kernel/dt.h through a dedicated commit (which commit message could refer to URL you pointed above)?

I still think snprintf() return value should be checked, at least asserted.

I'll search for a commit in Linux,

I cannot see why it's mandatory to check the error of snprintf(), The function cannot overflow and if the property name is too long the search in the DT will fail. It seems to be overprotecting the code.

You're right. Checking the return value is not needed.
Discard my comment. Sorry for the noise.

core/include/drivers/gpio.h

core/drivers/gpio/gpio.c

remove gpio_configure(); since it's always define independantyly of CFG_DT. Restore inline gpio_dt_get_by_index() stub definition. handle return code of gpio_configure() in gpio_dt_cfg_by_index(). Update comment of gpio_dt_cfg_by_index() to explicitly mention 'gpios', 'gpio', '-gpios' and '-gpio'. Signed-off-by: Thomas Bourgoin <[email protected]>

Do not accept to configure a NULL gpio. Signed-off-by: Thomas Bourgoin <[email protected]>

Update comment of gpio_dt_get_by_index() to explicitly mention 'gpios', 'gpio', '-gpios' and '-gpio'. Signed-off-by: Thomas Bourgoin <[email protected]>

tbourgoi · 2025-07-10T11:49:21Z

Hello,

I've update the series with the most of the comments. Mainly to fix the CI compiling error ASAP.

etienne-lms

Thanks for the updates. I'm fine you squash any fixup commits.

etienne-lms · 2025-07-15T12:24:35Z

core/include/drivers/gpio.h

+ * gpio_configure() - Configure a GPIO controller
+ *
+ * @gpio: GPIO pin reference upon success
+ * @flags: requester flags of GPIO


s/flags/mode/
Or maybe the a more consistent change would be to rename the argument from mode to flags.

Ditto in gpio_dt_cfg_by_index() inline description comment.

etienne-lms · 2025-07-15T12:28:32Z

core/drivers/gpio/gpio.c

+		}
+	}
+
+	return TEE_SUCCESS;


If always successful, return value is not needed.
That said, I think it would be better to allow configure ops handler to return a status.

etienne-lms · 2025-07-15T12:28:57Z

core/drivers/regulator/regulator_fixed.c

@@ -114,6 +108,10 @@ static TEE_Result get_enable_gpio(const void *fdt, int node,
 	if (cuint)
 		regu->off_on_delay = fdt32_to_cpu(*cuint);

+	/* Low level initialisation with updated dt_flags */
+	gpio_configure(gpio, GPIO_ASIS);


If gpio_configure() returns a status, it should be checked.

patrickdelaunay added 5 commits July 9, 2025 16:41

drivers: regulator: simplify device tree parsing

1a2380d

Use gpio_dt_get_by_index() to get the properties in device tree with all the possible variant name: "gpio" and "gpios". Signed-off-by: Patrick Delaunay <[email protected]> Signed-off-by: Thomas Bourgoin <[email protected]>

etienne-lms reviewed Jul 9, 2025

View reviewed changes

tbourgoi commented Jul 10, 2025

View reviewed changes

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

tbourgoi added 3 commits July 10, 2025 13:29

fixup! drivers: gpio: add new ops configure to initialize gpio

f148093

Do not accept to configure a NULL gpio. Signed-off-by: Thomas Bourgoin <[email protected]>

fixup! drivers: gpio: support gpio suffix in device tree

fa5fe2c

Update comment of gpio_dt_get_by_index() to explicitly mention 'gpios', 'gpio', '-gpios' and '-gpio'. Signed-off-by: Thomas Bourgoin <[email protected]>

etienne-lms reviewed Jul 15, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix GPIO framework to prevent glitch when requesting an output GPIO #7478

Fix GPIO framework to prevent glitch when requesting an output GPIO #7478

Uh oh!

tbourgoi commented Jul 9, 2025

Uh oh!

etienne-lms left a comment

Uh oh!

etienne-lms Jul 9, 2025

Uh oh!

tbourgoi Jul 10, 2025

Uh oh!

etienne-lms Jul 10, 2025

Uh oh!

tbourgoi Jul 10, 2025

Uh oh!

etienne-lms Jul 15, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tbourgoi commented Jul 10, 2025

Uh oh!

etienne-lms left a comment

Uh oh!

etienne-lms Jul 15, 2025

Uh oh!

etienne-lms Jul 15, 2025

Uh oh!

etienne-lms Jul 15, 2025

Uh oh!

Uh oh!

Fix GPIO framework to prevent glitch when requesting an output GPIO #7478

Are you sure you want to change the base?

Fix GPIO framework to prevent glitch when requesting an output GPIO #7478

Uh oh!

Conversation

tbourgoi commented Jul 9, 2025

Uh oh!

etienne-lms left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tbourgoi commented Jul 10, 2025

Uh oh!

etienne-lms left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!