[PATCH] gpio: ts4900: Do not set DAT and OE together

[PATCH] gpio: ts4900: Do not set DAT and OE together

[Kris Bahnsen]

From: Mark Featherston <mark@embeddedTS.com>

This works around an issue with the hardware where both OE and
DAT are exposed in the same register. If both are updated
simultaneously, the harware makes no guarantees that OE or DAT
will actually change in any given order and may result in a
glitch of a few ns on a GPIO pin when changing direction and value
in a single write.

Setting direction to input now only affects OE bit. Setting
direction to output updates DAT first, then OE.

Signed-off-by: Mark Featherston <mark@embeddedTS.com>
Signed-off-by: Kris Bahnsen <kris@embeddedTS.com>
---
 drivers/gpio/gpio-ts4900.c | 25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/drivers/gpio/gpio-ts4900.c b/drivers/gpio/gpio-ts4900.c
index d885032cf814..fbabfca030c0 100644
--- a/drivers/gpio/gpio-ts4900.c
+++ b/drivers/gpio/gpio-ts4900.c
@@ -1,7 +1,8 @@
+// SPDX-License-Identifier: GPL-2.0
 /*
  * Digital I/O driver for Technologic Systems I2C FPGA Core
  *
- * Copyright (C) 2015 Technologic Systems
+ * Copyright (C) 2015-2018 Technologic Systems
  * Copyright (C) 2016 Savoir-Faire Linux
  *
  * This program is free software; you can redistribute it and/or
@@ -55,19 +56,33 @@ static int ts4900_gpio_direction_input(struct gpio_chip *chip,
 {
 	struct ts4900_gpio_priv *priv = gpiochip_get_data(chip);
 
-	/*
-	 * This will clear the output enable bit, the other bits are
-	 * dontcare when this is cleared
+	/* Only clear the OE bit here, requires a RMW. Prevents potential issue
+	 * with OE and data getting to the physical pin at different times.
 	 */
-	return regmap_write(priv->regmap, offset, 0);
+	return regmap_update_bits(priv->regmap, offset, TS4900_GPIO_OE, 0);
 }
 
 static int ts4900_gpio_direction_output(struct gpio_chip *chip,
 					unsigned int offset, int value)
 {
 	struct ts4900_gpio_priv *priv = gpiochip_get_data(chip);
+	unsigned int reg;
 	int ret;
 
+	/* If changing from an input to an output, we need to first set the
+	 * proper data bit to what is requested and then set OE bit. This
+	 * prevents a glitch that can occur on the IO line
+	 */
+	regmap_read(priv->regmap, offset, &reg);
+	if (!(reg & TS4900_GPIO_OE)) {
+		if (value)
+			reg = TS4900_GPIO_OUT;
+		else
+			reg &= ~TS4900_GPIO_OUT;
+
+		regmap_write(priv->regmap, offset, reg);
+	}
+
 	if (value)
 		ret = regmap_write(priv->regmap, offset, TS4900_GPIO_OE |
 							 TS4900_GPIO_OUT);
-- 
2.11.0

Re: [PATCH] gpio: ts4900: Do not set DAT and OE together

[Bartosz Golaszewski]

On Fri, Mar 4, 2022 at 11:15 PM Kris Bahnsen <kris@embeddedts.com> wrote:
>
> From: Mark Featherston <mark@embeddedTS.com>
>
> This works around an issue with the hardware where both OE and
> DAT are exposed in the same register. If both are updated
> simultaneously, the harware makes no guarantees that OE or DAT
> will actually change in any given order and may result in a
> glitch of a few ns on a GPIO pin when changing direction and value
> in a single write.
>
> Setting direction to input now only affects OE bit. Setting
> direction to output updates DAT first, then OE.
>
> Signed-off-by: Mark Featherston <mark@embeddedTS.com>
> Signed-off-by: Kris Bahnsen <kris@embeddedTS.com>
> ---
>  drivers/gpio/gpio-ts4900.c | 25 ++++++++++++++++++++-----
>  1 file changed, 20 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpio/gpio-ts4900.c b/drivers/gpio/gpio-ts4900.c
> index d885032cf814..fbabfca030c0 100644
> --- a/drivers/gpio/gpio-ts4900.c
> +++ b/drivers/gpio/gpio-ts4900.c
> @@ -1,7 +1,8 @@
> +// SPDX-License-Identifier: GPL-2.0
>  /*
>   * Digital I/O driver for Technologic Systems I2C FPGA Core
>   *
> - * Copyright (C) 2015 Technologic Systems
> + * Copyright (C) 2015-2018 Technologic Systems
>   * Copyright (C) 2016 Savoir-Faire Linux
>   *
>   * This program is free software; you can redistribute it and/or
> @@ -55,19 +56,33 @@ static int ts4900_gpio_direction_input(struct gpio_chip *chip,
>  {
>         struct ts4900_gpio_priv *priv = gpiochip_get_data(chip);
>
> -       /*
> -        * This will clear the output enable bit, the other bits are
> -        * dontcare when this is cleared
> +       /* Only clear the OE bit here, requires a RMW. Prevents potential issue
> +        * with OE and data getting to the physical pin at different times.
>          */
> -       return regmap_write(priv->regmap, offset, 0);
> +       return regmap_update_bits(priv->regmap, offset, TS4900_GPIO_OE, 0);
>  }
>
>  static int ts4900_gpio_direction_output(struct gpio_chip *chip,
>                                         unsigned int offset, int value)
>  {
>         struct ts4900_gpio_priv *priv = gpiochip_get_data(chip);
> +       unsigned int reg;
>         int ret;
>
> +       /* If changing from an input to an output, we need to first set the
> +        * proper data bit to what is requested and then set OE bit. This
> +        * prevents a glitch that can occur on the IO line
> +        */
> +       regmap_read(priv->regmap, offset, &reg);
> +       if (!(reg & TS4900_GPIO_OE)) {
> +               if (value)
> +                       reg = TS4900_GPIO_OUT;
> +               else
> +                       reg &= ~TS4900_GPIO_OUT;
> +
> +               regmap_write(priv->regmap, offset, reg);
> +       }
> +
>         if (value)
>                 ret = regmap_write(priv->regmap, offset, TS4900_GPIO_OE |
>                                                          TS4900_GPIO_OUT);
> --
> 2.11.0
>

This looks like a fix, can you add a Fixes tag?

Bart

Re: [PATCH] gpio: ts4900: Do not set DAT and OE together

[Kris Bahnsen]

On Mon, 2022-03-07 at 10:13 +0100, Bartosz Golaszewski wrote:
> On Fri, Mar 4, 2022 at 11:15 PM Kris Bahnsen <kris@embeddedts.com> wrote:
> > 
> > From: Mark Featherston <mark@embeddedTS.com>
> > 
> > This works around an issue with the hardware where both OE and
> > DAT are exposed in the same register. If both are updated
> > simultaneously, the harware makes no guarantees that OE or DAT
> > will actually change in any given order and may result in a
> > glitch of a few ns on a GPIO pin when changing direction and value
> > in a single write.
> > 
> > Setting direction to input now only affects OE bit. Setting
> > direction to output updates DAT first, then OE.
> > 
> > Signed-off-by: Mark Featherston <mark@embeddedTS.com>
> > Signed-off-by: Kris Bahnsen <kris@embeddedTS.com>
> > ---
> >  drivers/gpio/gpio-ts4900.c | 25 ++++++++++++++++++++-----
> >  1 file changed, 20 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpio/gpio-ts4900.c b/drivers/gpio/gpio-ts4900.c
> > index d885032cf814..fbabfca030c0 100644
> > --- a/drivers/gpio/gpio-ts4900.c
> > +++ b/drivers/gpio/gpio-ts4900.c
> > @@ -1,7 +1,8 @@
> > +// SPDX-License-Identifier: GPL-2.0
> >  /*
> >   * Digital I/O driver for Technologic Systems I2C FPGA Core
> >   *
> > - * Copyright (C) 2015 Technologic Systems
> > + * Copyright (C) 2015-2018 Technologic Systems
> >   * Copyright (C) 2016 Savoir-Faire Linux
> >   *
> >   * This program is free software; you can redistribute it and/or
> > @@ -55,19 +56,33 @@ static int ts4900_gpio_direction_input(struct gpio_chip *chip,
> >  {
> >         struct ts4900_gpio_priv *priv = gpiochip_get_data(chip);
> > 
> > -       /*
> > -        * This will clear the output enable bit, the other bits are
> > -        * dontcare when this is cleared
> > +       /* Only clear the OE bit here, requires a RMW. Prevents potential issue
> > +        * with OE and data getting to the physical pin at different times.
> >          */
> > -       return regmap_write(priv->regmap, offset, 0);
> > +       return regmap_update_bits(priv->regmap, offset, TS4900_GPIO_OE, 0);
> >  }
> > 
> >  static int ts4900_gpio_direction_output(struct gpio_chip *chip,
> >                                         unsigned int offset, int value)
> >  {
> >         struct ts4900_gpio_priv *priv = gpiochip_get_data(chip);
> > +       unsigned int reg;
> >         int ret;
> > 
> > +       /* If changing from an input to an output, we need to first set the
> > +        * proper data bit to what is requested and then set OE bit. This
> > +        * prevents a glitch that can occur on the IO line
> > +        */
> > +       regmap_read(priv->regmap, offset, &reg);
> > +       if (!(reg & TS4900_GPIO_OE)) {
> > +               if (value)
> > +                       reg = TS4900_GPIO_OUT;
> > +               else
> > +                       reg &= ~TS4900_GPIO_OUT;
> > +
> > +               regmap_write(priv->regmap, offset, reg);
> > +       }
> > +
> >         if (value)
> >                 ret = regmap_write(priv->regmap, offset, TS4900_GPIO_OE |
> >                                                          TS4900_GPIO_OUT);
> > --
> > 2.11.0
> > 
> 
> This looks like a fix, can you add a Fixes tag?
> 
> Bart
> 

Please excuse my ignorance (and email client issues) I am still learning the
submission process. I'm not sure what kind of Fixes tag to add in this scenario.

This GPIO issue has existed since the driver's inception. It is a quirk of
hardware that has always existed on this platform. The driver was originally
implemented by Savoir-faire Linux. We discovered the issue and have had it
patched in our trees for years and wanted to push it upstream.

There is no public discussion on it, it was found and patched. And, aside from
the first commit of this driver, there is no commit that introduced any issue.

Can you please advise what kind of Fixes tag is appropriate in this situation?

Additionally, if I do add a Fixes tag, would that warrant a v2 patch? Or would
it just need to be an email response that includes it?

Kris

Re: [PATCH] gpio: ts4900: Do not set DAT and OE together

[Bartosz Golaszewski]

On Mon, Mar 7, 2022 at 8:40 PM Kris Bahnsen <kris@embeddedts.com> wrote:
>
> On Mon, 2022-03-07 at 10:13 +0100, Bartosz Golaszewski wrote:
> > On Fri, Mar 4, 2022 at 11:15 PM Kris Bahnsen <kris@embeddedts.com> wrote:
> > >
> > > From: Mark Featherston <mark@embeddedTS.com>
> > >
> > > This works around an issue with the hardware where both OE and
> > > DAT are exposed in the same register. If both are updated
> > > simultaneously, the harware makes no guarantees that OE or DAT
> > > will actually change in any given order and may result in a
> > > glitch of a few ns on a GPIO pin when changing direction and value
> > > in a single write.
> > >
> > > Setting direction to input now only affects OE bit. Setting
> > > direction to output updates DAT first, then OE.
> > >
> > > Signed-off-by: Mark Featherston <mark@embeddedTS.com>
> > > Signed-off-by: Kris Bahnsen <kris@embeddedTS.com>
> > > ---
> > >  drivers/gpio/gpio-ts4900.c | 25 ++++++++++++++++++++-----
> > >  1 file changed, 20 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/gpio/gpio-ts4900.c b/drivers/gpio/gpio-ts4900.c
> > > index d885032cf814..fbabfca030c0 100644
> > > --- a/drivers/gpio/gpio-ts4900.c
> > > +++ b/drivers/gpio/gpio-ts4900.c
> > > @@ -1,7 +1,8 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > >  /*
> > >   * Digital I/O driver for Technologic Systems I2C FPGA Core
> > >   *
> > > - * Copyright (C) 2015 Technologic Systems
> > > + * Copyright (C) 2015-2018 Technologic Systems
> > >   * Copyright (C) 2016 Savoir-Faire Linux
> > >   *
> > >   * This program is free software; you can redistribute it and/or
> > > @@ -55,19 +56,33 @@ static int ts4900_gpio_direction_input(struct gpio_chip *chip,
> > >  {
> > >         struct ts4900_gpio_priv *priv = gpiochip_get_data(chip);
> > >
> > > -       /*
> > > -        * This will clear the output enable bit, the other bits are
> > > -        * dontcare when this is cleared
> > > +       /* Only clear the OE bit here, requires a RMW. Prevents potential issue
> > > +        * with OE and data getting to the physical pin at different times.
> > >          */
> > > -       return regmap_write(priv->regmap, offset, 0);
> > > +       return regmap_update_bits(priv->regmap, offset, TS4900_GPIO_OE, 0);
> > >  }
> > >
> > >  static int ts4900_gpio_direction_output(struct gpio_chip *chip,
> > >                                         unsigned int offset, int value)
> > >  {
> > >         struct ts4900_gpio_priv *priv = gpiochip_get_data(chip);
> > > +       unsigned int reg;
> > >         int ret;
> > >
> > > +       /* If changing from an input to an output, we need to first set the
> > > +        * proper data bit to what is requested and then set OE bit. This
> > > +        * prevents a glitch that can occur on the IO line
> > > +        */
> > > +       regmap_read(priv->regmap, offset, &reg);
> > > +       if (!(reg & TS4900_GPIO_OE)) {
> > > +               if (value)
> > > +                       reg = TS4900_GPIO_OUT;
> > > +               else
> > > +                       reg &= ~TS4900_GPIO_OUT;
> > > +
> > > +               regmap_write(priv->regmap, offset, reg);
> > > +       }
> > > +
> > >         if (value)
> > >                 ret = regmap_write(priv->regmap, offset, TS4900_GPIO_OE |
> > >                                                          TS4900_GPIO_OUT);
> > > --
> > > 2.11.0
> > >
> >
> > This looks like a fix, can you add a Fixes tag?
> >
> > Bart
> >
>
> Please excuse my ignorance (and email client issues) I am still learning the
> submission process. I'm not sure what kind of Fixes tag to add in this scenario.
>
> This GPIO issue has existed since the driver's inception. It is a quirk of
> hardware that has always existed on this platform. The driver was originally
> implemented by Savoir-faire Linux. We discovered the issue and have had it
> patched in our trees for years and wanted to push it upstream.
>
> There is no public discussion on it, it was found and patched. And, aside from
> the first commit of this driver, there is no commit that introduced any issue.
>
> Can you please advise what kind of Fixes tag is appropriate in this situation?
>
> Additionally, if I do add a Fixes tag, would that warrant a v2 patch? Or would
> it just need to be an email response that includes it?
>
> Kris

Yeah, just send a v2 with "Fixes: <short commit hash> ("<commit
title>")" referring to the original patch that added the driver.

We'll send it for stable.

Bart