Re: [PATCH] i2c: core: dont change pinmux state to GPIO during recovery setup

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

Hi Robert!

Thanks for getting back on this issue.

On Thu, Nov 9, 2023 at 8:10 PM Robert Marko <robert.marko@xxxxxxxxxx> wrote:

> Yes, I2C recovery is required on this board as otherwise the I2C bus will
> get stuck after a certain number of SFP module plug/unplug events or
> sometimes even just randomly, I2C recovery allows the bus to recover
> and continue working.

OK makes sense.

> Maybe my commit message was confusing, so I will try and explain further.
> I2C recovery did work on Armada 3720 just fine until the driver was converted
> to use the generic I2C recovery which is now part of the I2C core.
>
> After it was converted to it, the I2C bus completely stopped working
> on Armada 3720
> if I2C recovery is enabled by making the recovery pinctrl available in DTS.

Shouldn't we just revert that patch until we can figure this out then?

> I then spent quite a while trying to bisect the exact change that
> causes this issue
> in the conversion as code is almost identical to what the driver was
> doing previously,
> and have bisected it down to pinctrl_select_state(bri->pinctrl,
> bri->pins_gpio) being
> called before SDA and SCL pins are obtained via devm_gpiod_get().
>
> Then I thought that maybe there was a HW bug in the Armada 3720 as the
> pin function
> was being set to GPIO twice via register write so I made sure the
> register was being
> written to only if the desired value is different than the current one
> but that did not help
> at all.
> For whatever reason calling pinctrl_select_state(bri->pinctrl,
> bri->pins_gpio) causes
> the pins to basically lock up, but the weird thing is that calling
> devm_gpiod_get() will
> also end up with the kernel changing the pin function to GPIO and this
> works, hence
> this patch.

That sounds like a race condition... such as if the pin control block is
not clocked when the first pinctrl_select_state() comes in.

Or maybe the i2c hardware needs to be initialized/clocked before
touching it's mux? (Weird but could happen.)

One thing that makes me a bit suspicious is this:
i2c/busses/i2c-pxa.c:
subsys_initcall(i2c_adap_pxa_init);

While:
pinctrl/mvebu/pinctrl-armada-37xx.c
builtin_platform_driver_probe(armada_37xx_pinctrl_driver,
                              armada_37xx_pinctrl_probe);

If there is no probe reordering then the i2c driver will probe before
pin control.

What happens if you move the i2c_adap_pxa_init() call to
module_init() instead of subsys_initcall()?

Yours,
Linus Walleij