[PATCH] driver: gpio: fix gpio glitch for output-high gpio-hog
A gpio-hog can be specified as output-low, output-high, or input where output-low means 'de-asserted' and 'output-high' means asserted vs voltage levels.
When a hog is probed gpio_request_tail() calls dm_gpio_set_dir_flags() which ends up setting the GPIO as an output with a driven 'de-asserted' value prior to setting the desired value the hog was configured for. While I'm not sure it makes sense to set the output level while simply 'requesting' a GPIO the result of this is that if the hog is configured for output-high the request call sets it first as output low before gpio_hog_probe() sets it to the configured value causing the gpio to 'glitch' which may be undesired for certain applications.
Fix this by setting the GPIOD_IS_OUT_ACTIVE flag for hogs configured as output-high.
This was tested with the following hogs:
/* active-high output-low (de-asserted) GPIO should drive 0 */ gpio1 { gpio-hog; output-low; gpios = <1 GPIO_ACTIVE_HIGH>; line-name = "gpio1"; };
/* active-high output-high (asserted) GPIO should drive 1 */ /* before patch this would first drive 0 then 1 */ gpio2 { gpio-hog; output-high; gpios = <2 GPIO_ACTIVE_HIGH>; line-name = "gpio2"; };
/* active-low output-low (de-asserted) GPIO should drive 1 */ gpio3 { gpio-hog; output-low; gpios = <3 GPIO_ACTIVE_LOW>; line-name = "gpio3#"; };
/* active-low output-high (asserted) GPIO should drive 0 */ /* before patch this would first drive 0 then 1 */ gpio4 { gpio-hog; output-high; gpios = <4 GPIO_ACTIVE_LOW>; line-name = "gpio4#"; };
Cc: Sean Anderson sean.anderson@seco.com Signed-off-by: Tim Harvey tharvey@gateworks.com --- drivers/gpio/gpio-uclass.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/gpio/gpio-uclass.c b/drivers/gpio/gpio-uclass.c index 125ae53d612f..baf9d451c19d 100644 --- a/drivers/gpio/gpio-uclass.c +++ b/drivers/gpio/gpio-uclass.c @@ -294,6 +294,8 @@ static int gpio_hog_probe(struct udevice *dev) struct gpio_hog_priv *priv = dev_get_priv(dev); int ret;
+ if (plat->gpiod_flags == GPIOD_IS_OUT && plat->value) + plat->gpiod_flags |= GPIOD_IS_OUT_ACTIVE; ret = gpio_dev_request_index(dev->parent, dev->name, "gpio-hog", plat->val[0], plat->gpiod_flags, plat->val[1], &priv->gpiod);
On Wed, Mar 2, 2022 at 6:01 PM Tim Harvey tharvey@gateworks.com wrote:
A gpio-hog can be specified as output-low, output-high, or input where output-low means 'de-asserted' and 'output-high' means asserted vs voltage levels.
When a hog is probed gpio_request_tail() calls dm_gpio_set_dir_flags() which ends up setting the GPIO as an output with a driven 'de-asserted' value prior to setting the desired value the hog was configured for. While I'm not sure it makes sense to set the output level while simply 'requesting' a GPIO the result of this is that if the hog is configured for output-high the request call sets it first as output low before gpio_hog_probe() sets it to the configured value causing the gpio to 'glitch' which may be undesired for certain applications.
Fix this by setting the GPIOD_IS_OUT_ACTIVE flag for hogs configured as output-high.
This was tested with the following hogs:
/* active-high output-low (de-asserted) GPIO should drive 0 */ gpio1 { gpio-hog; output-low; gpios = <1 GPIO_ACTIVE_HIGH>; line-name = "gpio1"; }; /* active-high output-high (asserted) GPIO should drive 1 */ /* before patch this would first drive 0 then 1 */ gpio2 { gpio-hog; output-high; gpios = <2 GPIO_ACTIVE_HIGH>; line-name = "gpio2"; }; /* active-low output-low (de-asserted) GPIO should drive 1 */ gpio3 { gpio-hog; output-low; gpios = <3 GPIO_ACTIVE_LOW>; line-name = "gpio3#"; }; /* active-low output-high (asserted) GPIO should drive 0 */ /* before patch this would first drive 0 then 1 */ gpio4 { gpio-hog; output-high; gpios = <4 GPIO_ACTIVE_LOW>; line-name = "gpio4#"; };Cc: Sean Anderson sean.anderson@seco.com Signed-off-by: Tim Harvey tharvey@gateworks.com
drivers/gpio/gpio-uclass.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/gpio/gpio-uclass.c b/drivers/gpio/gpio-uclass.c index 125ae53d612f..baf9d451c19d 100644 --- a/drivers/gpio/gpio-uclass.c +++ b/drivers/gpio/gpio-uclass.c @@ -294,6 +294,8 @@ static int gpio_hog_probe(struct udevice *dev) struct gpio_hog_priv *priv = dev_get_priv(dev); int ret;
if (plat->gpiod_flags == GPIOD_IS_OUT && plat->value)plat->gpiod_flags |= GPIOD_IS_OUT_ACTIVE; ret = gpio_dev_request_index(dev->parent, dev->name, "gpio-hog", plat->val[0], plat->gpiod_flags, plat->val[1], &priv->gpiod);-- 2.17.1
Widening the net here to those who were involved in the gpio-hog support:
Cc: Heiko Schocher hs@denx.de Cc: Michal Simek michal.simek@xilinx.com Cc: Patrick Delaunay patrick.delaunay@st.com
Best Regards,
Tim
On Wed, Mar 02, 2022 at 06:01:08PM -0800, Tim Harvey wrote:
A gpio-hog can be specified as output-low, output-high, or input where output-low means 'de-asserted' and 'output-high' means asserted vs voltage levels.
When a hog is probed gpio_request_tail() calls dm_gpio_set_dir_flags() which ends up setting the GPIO as an output with a driven 'de-asserted' value prior to setting the desired value the hog was configured for. While I'm not sure it makes sense to set the output level while simply 'requesting' a GPIO the result of this is that if the hog is configured for output-high the request call sets it first as output low before gpio_hog_probe() sets it to the configured value causing the gpio to 'glitch' which may be undesired for certain applications.
Fix this by setting the GPIOD_IS_OUT_ACTIVE flag for hogs configured as output-high.
This was tested with the following hogs:
/* active-high output-low (de-asserted) GPIO should drive 0 */ gpio1 { gpio-hog; output-low; gpios = <1 GPIO_ACTIVE_HIGH>; line-name = "gpio1"; }; /* active-high output-high (asserted) GPIO should drive 1 *//* before patch this would first drive 0 then 1 */ gpio2 { gpio-hog; output-high; gpios = <2 GPIO_ACTIVE_HIGH>; line-name = "gpio2"; };
/* active-low output-low (de-asserted) GPIO should drive 1 */ gpio3 { gpio-hog; output-low; gpios = <3 GPIO_ACTIVE_LOW>; line-name = "gpio3#"; }; /* active-low output-high (asserted) GPIO should drive 0 *//* before patch this would first drive 0 then 1 */ gpio4 { gpio-hog; output-high; gpios = <4 GPIO_ACTIVE_LOW>; line-name = "gpio4#"; };
Cc: Sean Anderson sean.anderson@seco.com Signed-off-by: Tim Harvey tharvey@gateworks.com
This breaks the ut_dm_dm_test_gpio test which I guess needs to be updated.
On Wed, Apr 6, 2022 at 5:27 AM Tom Rini trini@konsulko.com wrote:
On Wed, Mar 02, 2022 at 06:01:08PM -0800, Tim Harvey wrote:
A gpio-hog can be specified as output-low, output-high, or input where output-low means 'de-asserted' and 'output-high' means asserted vs voltage levels.
When a hog is probed gpio_request_tail() calls dm_gpio_set_dir_flags() which ends up setting the GPIO as an output with a driven 'de-asserted' value prior to setting the desired value the hog was configured for. While I'm not sure it makes sense to set the output level while simply 'requesting' a GPIO the result of this is that if the hog is configured for output-high the request call sets it first as output low before gpio_hog_probe() sets it to the configured value causing the gpio to 'glitch' which may be undesired for certain applications.
Fix this by setting the GPIOD_IS_OUT_ACTIVE flag for hogs configured as output-high.
This was tested with the following hogs:
/* active-high output-low (de-asserted) GPIO should drive 0 */ gpio1 { gpio-hog; output-low; gpios = <1 GPIO_ACTIVE_HIGH>; line-name = "gpio1"; }; /* active-high output-high (asserted) GPIO should drive 1 */ /* before patch this would first drive 0 then 1 */ gpio2 { gpio-hog; output-high; gpios = <2 GPIO_ACTIVE_HIGH>; line-name = "gpio2"; }; /* active-low output-low (de-asserted) GPIO should drive 1 */ gpio3 { gpio-hog; output-low; gpios = <3 GPIO_ACTIVE_LOW>; line-name = "gpio3#"; }; /* active-low output-high (asserted) GPIO should drive 0 */ /* before patch this would first drive 0 then 1 */ gpio4 { gpio-hog; output-high; gpios = <4 GPIO_ACTIVE_LOW>; line-name = "gpio4#"; };Cc: Sean Anderson sean.anderson@seco.com Signed-off-by: Tim Harvey tharvey@gateworks.com
This breaks the ut_dm_dm_test_gpio test which I guess needs to be updated.
Tom,
That's kind of funny I suppose because the behavior is clearly wrong.
Is fixing the test something that needs to be done for the patch to be applied, or needs to be done by me? I'm not familiar with the test framework or how to execute it.
Tim
On Wed, Apr 06, 2022 at 08:30:31AM -0700, Tim Harvey wrote:
On Wed, Apr 6, 2022 at 5:27 AM Tom Rini trini@konsulko.com wrote:
On Wed, Mar 02, 2022 at 06:01:08PM -0800, Tim Harvey wrote:
A gpio-hog can be specified as output-low, output-high, or input where output-low means 'de-asserted' and 'output-high' means asserted vs voltage levels.
When a hog is probed gpio_request_tail() calls dm_gpio_set_dir_flags() which ends up setting the GPIO as an output with a driven 'de-asserted' value prior to setting the desired value the hog was configured for. While I'm not sure it makes sense to set the output level while simply 'requesting' a GPIO the result of this is that if the hog is configured for output-high the request call sets it first as output low before gpio_hog_probe() sets it to the configured value causing the gpio to 'glitch' which may be undesired for certain applications.
Fix this by setting the GPIOD_IS_OUT_ACTIVE flag for hogs configured as output-high.
This was tested with the following hogs:
/* active-high output-low (de-asserted) GPIO should drive 0 */ gpio1 { gpio-hog; output-low; gpios = <1 GPIO_ACTIVE_HIGH>; line-name = "gpio1"; }; /* active-high output-high (asserted) GPIO should drive 1 */ /* before patch this would first drive 0 then 1 */ gpio2 { gpio-hog; output-high; gpios = <2 GPIO_ACTIVE_HIGH>; line-name = "gpio2"; }; /* active-low output-low (de-asserted) GPIO should drive 1 */ gpio3 { gpio-hog; output-low; gpios = <3 GPIO_ACTIVE_LOW>; line-name = "gpio3#"; }; /* active-low output-high (asserted) GPIO should drive 0 */ /* before patch this would first drive 0 then 1 */ gpio4 { gpio-hog; output-high; gpios = <4 GPIO_ACTIVE_LOW>; line-name = "gpio4#"; };Cc: Sean Anderson sean.anderson@seco.com Signed-off-by: Tim Harvey tharvey@gateworks.com
This breaks the ut_dm_dm_test_gpio test which I guess needs to be updated.
Tom,
That's kind of funny I suppose because the behavior is clearly wrong.
Is fixing the test something that needs to be done for the patch to be applied, or needs to be done by me? I'm not familiar with the test framework or how to execute it.
For running the tests, please see: https://u-boot.readthedocs.io/en/latest/develop/testing.html and you're going to need to take a look at test/dm/gpio.c as where to fix what. And yes, a v2 that addresses the test as well is what I need. Thanks!
participants (2)
-
Tim Harvey -
Tom Rini