Hi, On 5/30/21 6:19 PM, Sander Vanheule wrote: > Hi Michael, Andy, > > On Fri, 2021-05-28 at 08:37 +0200, Michael Walle wrote: >> Am 2021-05-24 13:41, schrieb Sander Vanheule: >>> Hi Andy, Andrew, >>> >>> On Mon, 2021-05-24 at 10:53 +0300, Andy Shevchenko wrote: >>>> On Mon, May 24, 2021 at 4:11 AM Andrew Lunn <andrew@xxxxxxx> wrote: >>>>> >>>>>> Changes since v2: >>>>>> - MDIO regmap support was merged, so patch is dropped here >>>>> >>>>> Do you have any idea how this will get merged. It sounds like one of >>>>> the Maintainers will need a stable branch of regmap. >>>> >>>> This is not a problem if Mark provides an immutable branch to pull >>>> from. >>> >>> Mark has a tag (regmap-mdio) for this patch: >>> https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git/tag/?h=regmap-mdio >>> >>>> >>>>>> - Introduce GPIO regmap quirks to set output direction first >>>>> >>>>> I thought you had determined it was possible to set output before >>>>> direction? >>>> >>>> Same thoughts when I saw an updated version of that patch. My >>>> anticipation was to not see it at all. >>> >>> The two devices I've been trying to test the behaviour on are: >>> * Netgear GS110TPP: has an RTL8231 with three LEDs, each driven via a >>> pin >>> configured as (active-low) GPIO. The LEDs are easy for a quick >>> visual check. >>> * Zyxel GS1900-8: RTL8231 used for the front panel button, and an >>> active-low >>> GPIO used to hard reset the main SoC (an RTL8380). I've modified >>> this board >>> to change some of the strapping pin values, but testing with the >>> jumpers and >>> pull-up/down resistors is a bit more tedious. >>> >>> On the Netgear, I tested the following with and without the quirk: >>> >>> # Set as OUT-LOW twice, to avoid the quirk. Always turns the LED on >>> gpioset 1 32=0; gpioset 1 32=0 >>> # Get value to change to input, turns the LED off (high impedance) >>> # Will return 1 due to (weak) internal pull-up >>> gpioget 1 32 >>> # Set as OUT-HIGH, should result in LED off >>> # When the quirk is disabled, the LED turns on (i.e. old OUT-LOW >>> value) >>> # When the quirk is enabled, the LED remains off (i.e. correct >>> OUT-HIGH value) >>> gpioset 1 32=1 >>> >>> Now, what's confusing (to me) is that the inverse doesn't depend on the >>> quirk: >>> >>> # Set as OUT-HIGH twice >>> gpioset 1 32=1; gpioset 1 32=1 >>> # Change to high-Z >>> gpioget 1 32 >>> # Set to OUT-LOW, always results in LED on, with or without quirk >>> gpioset 1 32=0 >>> >>> Any idea why this would be (or appear) broken on the former case, but >>> not on the >>> latter? >> >> Before reading this, I'd have guessed that they switch the internal >> register >> depending on the GPIO direction; I mean there is only one register >> address >> for both the input and the output register. Hm. >> >> Did you try playing around with raw register accesses and see if the >> value >> of the GPIO data register is changing when you switch GPIOs to >> input/output. >> >> Eg. you could try https://github.com/kontron/miitool to access the >> registers >> from userspace (your ethernet controller has to have support for the >> ioctl's >> though, see commit a613bafec516 ("enetc: add ioctl() support for >> PHY-related >> ops") for an example). > > I think I found a solution! > > 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. Regards, Hans
