Date:   Mon, 31 May 2021 12:02:31 +0200
From:   Michael Walle <michael@...le.cc>
To:     Sander Vanheule <sander@...nheule.net>
Cc:     Andy Shevchenko <andy.shevchenko@...il.com>,
        Hans de Goede <hdegoede@...hat.com>,
        Andrew Lunn <andrew@...n.ch>, Pavel Machek <pavel@....cz>,
        Rob Herring <robh+dt@...nel.org>,
        Lee Jones <lee.jones@...aro.org>,
        Mark Brown <broonie@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "Rafael J . Wysocki" <rafael@...nel.org>,
        Linus Walleij <linus.walleij@...aro.org>,
        Bartosz Golaszewski <bgolaszewski@...libre.com>,
        Linux LED Subsystem <linux-leds@...r.kernel.org>,
        devicetree <devicetree@...r.kernel.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 0/6] RTL8231 GPIO expander support

Am 2021-05-31 10:36, schrieb Sander Vanheule:
> On Sun, 2021-05-30 at 23:22 +0200, Michael Walle wrote:
>> Am 2021-05-30 20:16, schrieb Andy Shevchenko:
>> > On Sun, May 30, 2021 at 7:51 PM Hans de Goede <hdegoede@...hat.com>
>> > wrote:
>> > > On 5/30/21 6:19 PM, Sander Vanheule wrote:
>> > > > As Michael suggested, I tried raw register reads and writes, to eliminate
>> > > > any
>> > > > side effects of the intermediate code. I didn't use the ioctls (this isn't
>> > > > a
>> > > > netdev), but I found regmap's debugfs write functionality, which allowed
>> > > > me to
>> > > > do the same.
>> > > >
>> > > > I was trying to reproduce the behaviour I reported earlier, but couldn't.
>> > > > The
>> > > > output levels were always the intended ones. At some point I realised that
>> > > > the
>> > > > regmap_update_bits function does a read-modify-write, which might shadow
>> > > > the
>> > > > actual current output value.
>> > > > For example:
>> > > >  * Set output low: current out is low
>> > > >  * Change to input with pull-up: current out is still low, but DATAx reads
>> > > > high
>> > > >  * Set output high: RMW reads a high value (the input), so assumes a write
>> > > > is
>> > > >    not necessary, leaving the old output value (low).
>> > > >
>> > > > Currently, I see two options:
>> > > >  * Use regmap_update_bits_base to avoid the lazy RMW behaviour
>> > > >  * Add a cache for the output data values to the driver, and only use
>> > > > these
>> > > >    values to write to the output registers. This would allow keeping lazy
>> > > > RMW
>> > > >    behaviour, which may be a benefit on slow busses.
>> > > >
>> > > > With either of these implemented, if I set the output value before the
>> > > > direction, everything works! :-)
>> > > >
>> > > > Would you like this to be added to regmap-gpio, or should I revert back to
>> > > > a
>> > > > device-specific implementation?
>> > >
>> > > Regmap allows you to mark certain ranges as volatile, so that they
>> > > will not
>> > > be cached, these GPIO registers containing the current pin value seems
>> > > like
>> > > a good candidate for this. This is also necessary to make reading the
>> > > GPIO
>> > > work without getting back a stale, cached value.
>> >
>> > After all it seems a simple missed proper register configuration in
>> > the driver for regmap.
>> > Oh, as usual something easy-to-solve requires tons of time to find it.
>> > :-)
>> >
>> > Sander, I think you may look at gpio-pca953x.c to understand how it
>> > works (volatility of registers).
>> 
>> But as far as I see is the regmap instantiated without a cache?
> 
> That's correct, there currently is no cache, although I could add one.
> 
> The data register rather appears to be implemented as a read-only (pin 
> inputs)
> register and a write-only (pin outputs) register, aliased on the same 
> register
> address.

Ahh so this makes more sense. If the data register is really write only
regardless of the direction mode, then RMW doesn't make any sense at 
all.
Please note, that even if regmap caches values, it might be marked as 
dirty
and it will re-read the values from hardware. So I don't know if that 
will
help you.

So a possible quirk could be
  GPIO_REGMAP_WRITE_ONLY_DATA_REG (or something like that)

I'm not sure if regmap can cache the value for us or if we have to do it
ourselves.

> As I understand, marking the DATA registers as volatile wouldn't help. 
> With a
> cache this would force reads to not use the cache, which is indeed 
> required for
> the pin input values (DATA register reads). However, the output values 
> (DATA
> register writes) can in fact be cached.
> Looking at _regmap_update_bits(), marking a register as volatile would 
> only make
> a difference if regmap.reg_update_bits is implemented. On an MDIO bus, 
> this
> would also be emulated with a lazy RMW (see mdiobus_modify()), which is 
> why I
> chose not to implement it for regmap-mdio.
> 
> So, I still think the issue lies with the lazy RMW behaviour. The patch 
> below
> would force a register update when reg_set_base (the data output 
> register) and
> reg_dat_base (the data input register) are identical. Otherwise the two
> registers are assumed to have conventional RW behaviour. I'm just not 
> entirely
> sure gpio-regmap.c is the right place for this.
> 
> ---8<---
> 
> diff --git a/drivers/gpio/gpio-regmap.c b/drivers/gpio/gpio-regmap.c
> index 95553734e169..c2fccd19548a 100644
> --- a/drivers/gpio/gpio-regmap.c
> +++ b/drivers/gpio/gpio-regmap.c
> @@ -81,13 +81,16 @@ static void gpio_regmap_set(struct gpio_chip *chip, 
> unsigned
> int offset,
>  {
>         struct gpio_regmap *gpio = gpiochip_get_data(chip);
>         unsigned int base = gpio_regmap_addr(gpio->reg_set_base);
> +       bool force = gpio->reg_set_base == gpio->reg_dat_base;

Ha I've thought of the same thing, but there might be hardware which
actually mux the data in and data out register. Thus I think we have
to distiguish between:

  (1) write only data registers
  (2) muxed data in/data out according to the direction

for (1) we'd have to cache the value (ourselves (?))
for (2) we'd only need to drop a cached value if we switch directions

>         unsigned int reg, mask;
> 
>         gpio->reg_mask_xlate(gpio, base, offset, &reg, &mask);
>         if (val)
> -               regmap_update_bits(gpio->regmap, reg, mask, mask);
> +               regmap_update_bits_base(gpio->regmap, reg, mask, mask, 
> NULL,
> +                                       false, force);

mh, I don't see how this will work with a write only register. I seems
that you might accidentially change the values of the other GPIOs in
this registers (that is depending on the input of them, because you
are still doing a RMW).

>         else
> -               regmap_update_bits(gpio->regmap, reg, mask, 0);
> +               regmap_update_bits_base(gpio->regmap, reg, mask, 0, 
> NULL,
> +                                       false, force);
>  }
> 
>  static void gpio_regmap_set_with_clear(struct gpio_chip *chip,

-michael

Powered by blists - more mailing lists