| Message ID | 20240730134610.80986-1-skmr537@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: exar set value handling for hw with gpio pull-up or pull-down | expand |
Hi all, I worked with Sai on this. We discovered this problem when we upgraded our kernel from 5.10 to 6.1. The behavior changed with the switch to regmap in 5.11. commit 36fb7218e87833b17e3042e77f3b102c75129e8f Author: Bartosz Golaszewski <brgl@bgdev.pl> Date: Mon Sep 28 17:00:26 2020 +0200 gpio: exar: switch to using regmap We can simplify the code in gpio-exar by using regmap. This allows us to drop the mutex (regmap provides its own locking) and we can also reuse regmap's bit operations instead of implementing our own update function. Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> We noticed because we had some gpios tied to reset pins and when we set direction to high, value went to 0, and put devices in reset. Thanks, Matthew On 7/30/24 08:46, Sai Kumar Cholleti wrote: > Setting gpio direction = high, sometimes results in gpio value = 0. > > If a gpio is pulled high, the following construction results in the > value being 0 when the desired value is 1: > > $ echo "high" > /sys/class/gpio/gpio336/direction > $ cat /sys/class/gpio/gpio336/value > 0 > > Before the gpio direction is changed from input to output, > exar_set_value is set to 1, but since direction is input when > exar_set_value is called, _regmap_update_bits reads a 1 due to an > external pull-up. When force_write is not set (regmap_set_bits has > force_write = false), the value is not written. When the direction is > then changed, the gpio becomes an output with the value of 0 (the > hardware default). > > regmap_write_bits sets force_write = true, so the value is always > written by exar_set_value and an external pull-up doesn't affect the > outcome of setting direction = high. > > > The same can happen when a gpio is pulled low, but the scenario is a > little more complicated. > > $ echo high > /sys/class/gpio/gpio351/direction > $ cat /sys/class/gpio/gpio351/value > 1 > > $ echo in > /sys/class/gpio/gpio351/direction > $ cat /sys/class/gpio/gpio351/value > 0 > > $ echo low > /sys/class/gpio/gpio351/direction > $ cat /sys/class/gpio/gpio351/value > 1 > > Signed-off-by: Sai Kumar Cholleti <skmr537@gmail.com> > --- > drivers/gpio/gpio-exar.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpio/gpio-exar.c b/drivers/gpio/gpio-exar.c > index 482f678c893e..de5ce73159cb 100644 > --- a/drivers/gpio/gpio-exar.c > +++ b/drivers/gpio/gpio-exar.c > @@ -99,11 +99,13 @@ static void exar_set_value(struct gpio_chip *chip, unsigned int offset, > struct exar_gpio_chip *exar_gpio = gpiochip_get_data(chip); > unsigned int addr = exar_offset_to_lvl_addr(exar_gpio, offset); > unsigned int bit = exar_offset_to_bit(exar_gpio, offset); > + unsigned int bit_value = value ? BIT(bit) : 0; > > - if (value) > - regmap_set_bits(exar_gpio->regmap, addr, BIT(bit)); > - else > - regmap_clear_bits(exar_gpio->regmap, addr, BIT(bit)); > + /* > + * regmap_write_bits forces value to be written when an external > + * pull up/down might otherwise indicate value was already set > + */ > + regmap_write_bits(exar_gpio->regmap, addr, BIT(bit), bit_value); > } > > static int exar_direction_output(struct gpio_chip *chip, unsigned int offset,
On Tue, Jul 30, 2024 at 07:16:10PM +0530, Sai Kumar Cholleti wrote: Please, refer to the functions in the text as func(), e.g. exar_set_value(). Use proper acronym, i.e. GPIO (capitalised). > Setting gpio direction = high, sometimes results in gpio value = 0. > > If a gpio is pulled high, the following construction results in the > value being 0 when the desired value is 1: > > $ echo "high" > /sys/class/gpio/gpio336/direction > $ cat /sys/class/gpio/gpio336/value > 0 > > Before the gpio direction is changed from input to output, > exar_set_value is set to 1, but since direction is input when > exar_set_value is called, _regmap_update_bits reads a 1 due to an > external pull-up. When force_write is not set (regmap_set_bits has > force_write = false), the value is not written. When the direction is > then changed, the gpio becomes an output with the value of 0 (the > hardware default). > > regmap_write_bits sets force_write = true, so the value is always > written by exar_set_value and an external pull-up doesn't affect the > outcome of setting direction = high. > > > The same can happen when a gpio is pulled low, but the scenario is a > little more complicated. > > $ echo high > /sys/class/gpio/gpio351/direction > $ cat /sys/class/gpio/gpio351/value > 1 > > $ echo in > /sys/class/gpio/gpio351/direction > $ cat /sys/class/gpio/gpio351/value > 0 > > $ echo low > /sys/class/gpio/gpio351/direction > $ cat /sys/class/gpio/gpio351/value > 1 Okay, shouldn't you instead mark the respective registers as volatile or so? I believe regmap has some settings for this case. But I haven't checked myself.
On 8/12/24 12:22, Andy Shevchenko wrote: > On Tue, Jul 30, 2024 at 07:16:10PM +0530, Sai Kumar Cholleti wrote: > > Please, refer to the functions in the text as func(), e.g. exar_set_value(). > Use proper acronym, i.e. GPIO (capitalised). We will update the patch and send a new version out if the current approach is acceptable. >> Before the gpio direction is changed from input to output, >> exar_set_value is set to 1, but since direction is input when >> exar_set_value is called, _regmap_update_bits reads a 1 due to an >> external pull-up. When force_write is not set (regmap_set_bits has >> force_write = false), the value is not written. When the direction is >> then changed, the gpio becomes an output with the value of 0 (the >> hardware default). >> >> regmap_write_bits sets force_write = true, so the value is always >> written by exar_set_value and an external pull-up doesn't affect the >> outcome of setting direction = high. > Okay, shouldn't you instead mark the respective registers as volatile or so? > I believe regmap has some settings for this case. But I haven't checked myself. Unfortunately, in addition to marking the regmap volatile, we'd need to define reg_update_bits which means we'd be partially undoing the work from 36fb7218e87833b17e3042e77f3b102c75129e8f to reuse regmap locking and update functions. Below is the relevant section of _regmap_update_bits(). static int _regmap_update_bits(struct regmap *map, unsigned int reg, unsigned int mask, unsigned int val, bool *change, bool force_write) ... if (regmap_volatile(map, reg) && map->reg_update_bits) { ... } else { ... if (force_write || (tmp != orig) || map->force_write_field) { ret = _regmap_write(map, reg, tmp); if (ret == 0 && change) *change = true; ... I suspect this might be a common problem with other GPIO drivers as well. Thank you, Matthew
diff --git a/drivers/gpio/gpio-exar.c b/drivers/gpio/gpio-exar.c index 482f678c893e..de5ce73159cb 100644 --- a/drivers/gpio/gpio-exar.c +++ b/drivers/gpio/gpio-exar.c @@ -99,11 +99,13 @@ static void exar_set_value(struct gpio_chip *chip, unsigned int offset, struct exar_gpio_chip *exar_gpio = gpiochip_get_data(chip); unsigned int addr = exar_offset_to_lvl_addr(exar_gpio, offset); unsigned int bit = exar_offset_to_bit(exar_gpio, offset); + unsigned int bit_value = value ? BIT(bit) : 0; - if (value) - regmap_set_bits(exar_gpio->regmap, addr, BIT(bit)); - else - regmap_clear_bits(exar_gpio->regmap, addr, BIT(bit)); + /* + * regmap_write_bits forces value to be written when an external + * pull up/down might otherwise indicate value was already set + */ + regmap_write_bits(exar_gpio->regmap, addr, BIT(bit), bit_value); } static int exar_direction_output(struct gpio_chip *chip, unsigned int offset,
Setting gpio direction = high, sometimes results in gpio value = 0. If a gpio is pulled high, the following construction results in the value being 0 when the desired value is 1: $ echo "high" > /sys/class/gpio/gpio336/direction $ cat /sys/class/gpio/gpio336/value 0 Before the gpio direction is changed from input to output, exar_set_value is set to 1, but since direction is input when exar_set_value is called, _regmap_update_bits reads a 1 due to an external pull-up. When force_write is not set (regmap_set_bits has force_write = false), the value is not written. When the direction is then changed, the gpio becomes an output with the value of 0 (the hardware default). regmap_write_bits sets force_write = true, so the value is always written by exar_set_value and an external pull-up doesn't affect the outcome of setting direction = high. The same can happen when a gpio is pulled low, but the scenario is a little more complicated. $ echo high > /sys/class/gpio/gpio351/direction $ cat /sys/class/gpio/gpio351/value 1 $ echo in > /sys/class/gpio/gpio351/direction $ cat /sys/class/gpio/gpio351/value 0 $ echo low > /sys/class/gpio/gpio351/direction $ cat /sys/class/gpio/gpio351/value 1 Signed-off-by: Sai Kumar Cholleti <skmr537@gmail.com> --- drivers/gpio/gpio-exar.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)