Re: GPIO regression on mvebu and/or max310x since 2ab73c6d8323

[Thierry Reding <treding@nvidia.com>]

[-- Attachment #1: Type: text/plain, Size: 5239 bytes --]

On Wed, Jun 10, 2020 at 02:39:22AM +0200, Jan Kundrát wrote:
> Hi,
> after upgrading from v5.6.7 to v5.7.1, my bit-banged I2C bus no longer
> works. I've run a bisection and it led me to commit 2ab73c6d8323 (gpio:
> Support GPIO controllers without pin-ranges). If I make
> gpiochip_generic_request() call pinctrl_gpio_request() unconditionally
> again, stuff gets back to working.
> 
> My HW setup is "unusual" as there's also an analog switch in the path of the
> bit-banged bus, and that one is controlled by another GPIO from MAX14830
> (drivers/tty/serial/max310x.c). The I2C bit-banging is driven by a Solidrun
> ClearFog Base (mvebu, Armada AM385) pins MPP24 and MPP25 which are
> configured as GPIOs. In terms of a DTS snippet, here's how it looks like:
> 
>        /* Bit-banged I2C instead of the default UART function from the SoC
> */
>        &uart1_pins {
>                status = "disabled";
>        };
>        &uart1 {
>                status = "disabled";
>        };
>        gpio_i2c {
>                compatible = "i2c-gpio";
>                sda-gpios = <&gpio0 25 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
>                scl-gpios = <&gpio0 24 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
>                i2c-gpio.delay-us = <1>;
>                #address-cells = <1>;
>                #size-cells = <0>;
>        };
>                /* Analog switch control via a GPIO hog. This drives that
> analog switch
>           so that the signals from the SoC actually reach the I2C slaves */
>        max14830@2 {
>                // ...
>                i2c_bitbang_enable {
>                        gpio-hog;
>                        gpios = <7 GPIO_ACTIVE_HIGH>;
>                        output-high;
>                        line-name = "I2C bitbang bus";
>                };
>        };
> 
> This means that for my bit-banged I2C to work, I need both the mvebu's
> gpio+pinctrl and the MAX14830 GPIOs. I hope that the max310x.c path is OK
> (max310x_gpio_direction_output() and friends are being called when kernel
> sets up GPIO hogs). According to some dev_dbg()s in i2c/busses/i2c-gpio.c
> and i2c/algos/i2c-algo-bit.c, the I2C slaves never respond with an ACK. The
> machine is remote so I have not had a chance to attach a logical analyzer
> yet -- but I *think* that the root cause is somewhere in pinconf. When stuff
> doesn't work, I get this:
> 
> # grep -e mpp24 -e mpp25
> /sys/kernel/debug/pinctrl/f1018000.pinctrl/pinconf-groups
> 24 (mpp24): current: ua1(rxd), available = [ gpio(io) spi0(miso) ua0(cts)
> sd0(d4) dev(ready) ]
> 25 (mpp25): current: ua1(txd), available = [ gpio(io) spi0(cs0) ua0(rts)
> sd0(d5) dev(cs0) ]
> 
> ...whereas on the old kernel, it looks like this:
> 
> # grep -e mpp24 -e mpp25
> /sys/kernel/debug/pinctrl/f1018000.pinctrl/pinconf-groups 24 (mpp24):
> current: gpio(io), available = [ spi0(miso) ua0(cts) ua1(rxd) sd0(d4)
> dev(ready) ]
> 25 (mpp25): current: gpio(io), available = [ spi0(cs0) ua0(rts) ua1(txd)
> sd0(d5) dev(cs0) ]
> 
> There's also some difference in
> /sys/kernel/debug/pinctrl/f1018000.pinctrl/pinmux-pins says, for example:
> 
> pin 19 (PIN19): f1072004.mdio (GPIO UNCLAIMED) function gpio group mpp19
> 
> ...whereas in the old kernel it is:
> 
> pin 19 (PIN19): f1072004.mdio mvebu-gpio:19 function gpio group mpp19
> 
> ...and the same for 22, 24, 29, 44, 54, all of these are supposed to be
> GPIOs I think. At this time I'm deep in the bowels of pinconf/pinmux that I
> do not understand.
> 
> TL;DR: 2ab73c6d8323 breaks GPIOs on mvebu on my system. I'll be happy to
> test patches which fix this in some better way than just a revert.

All of the above seems to indicate that there are no GPIO ranges
associated with the pinmux controller, because that's the only reason
why gpiochip_generic_request() wouldn't call into pinctrl to request
the pin for GPIO.

However, the reason why I sent that patch was because the absence of
GPIO ranges would actually cause pinctrl_gpio_request() to crash, so in
order to be able to use gpiochip_generic_request() on chips that don't
have GPIO ranges we have to call pinctrl_gpio_request() conditionally.

So I don't understand how this could've worked for you before. Unless
perhaps if there's some other difference between v5.6.7 and v5.7.1 that
introduces the crash that I was seeing.

I think a good first step would be to try that reverting the change
would actually fix the issue for you. Something like the below should do
the trick. Also, can you point out which exact DTS file it is that you
see this problem with?

--- >8 ---
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index c14f0784274a..2befc4dba7f3 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2783,11 +2783,6 @@ static inline void gpiochip_irqchip_free_valid_mask(struct gpio_chip *gc)
  */
 int gpiochip_generic_request(struct gpio_chip *gc, unsigned offset)
 {
-#ifdef CONFIG_PINCTRL
- if (list_empty(&gc->gpiodev->pin_ranges))
-  return 0;
-#endif
-
  return pinctrl_gpio_request(gc->gpiodev->base + offset);
 }
 EXPORT_SYMBOL_GPL(gpiochip_generic_request);
--- >8 ---

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]