| Message ID | 20220303020108.11453-1-tharvey@gateworks.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | driver: gpio: fix gpio glitch for output-high gpio-hog | expand |
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!
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);
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(+)