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

[Robert Marko <robert.marko@xxxxxxxxxx>]

On Thu, Sep 28, 2023 at 11:11 PM Linus Walleij <linus.walleij@xxxxxxxxxx> wrote:
>
> On Tue, Sep 26, 2023 at 6:03 PM Robert Marko <robert.marko@xxxxxxxxxx> wrote:
>
> > @@ -359,13 +359,6 @@ static int i2c_gpio_init_generic_recovery(struct i2c_adapter *adap)
> >         if (bri->recover_bus && bri->recover_bus != i2c_generic_scl_recovery)
> >                 return 0;
> >
> > -       /*
> > -        * pins might be taken as GPIO, so we should inform pinctrl about
> > -        * this and move the state to GPIO
> > -        */
> > -       if (bri->pinctrl)
> > -               pinctrl_select_state(bri->pinctrl, bri->pins_gpio);
> > -
>
> But this might be absolutely necessary for other i2c drivers and this is
> set in generic code.
>
> My first question is: why is this platform even defining the "gpio" pin
> control state for this i2c device if it is so dangerous to use?
> If it can't be used, you just give it too much rope, delete the "gpio"
> state for this group from the device tree: problem solved.
>
> (This can be done with the specific /delete-node/ directive if
> necessary, e.g. if you want to use the "gpio" state on other boards.)

Hi,
Sorry for the late reply.

GPIO function is being defined as I2C recovery itself requires it, it
will switch
the pins to GPIO mode and then pulse SCL for a predefined number of pulses
in order to try and recover the bus.
So we cannot remove it from DTS and have I2C recovery working.

Also, GPIO is a fully valid option for this pin group according to the
datasheet,
it is even the default until you switch them to I2C.

>
> Second: do you even want to do recovery on this platform then?
> Is it necessary? What happens electronically in this case, if we don't
> switch the pins to GPIO mode? Is it something akin to the "strict"
> property in struct pinmux: that the GPIO block and the device can
> affect the same pins at the same time? That warrants an explanation
> and a comment.

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.

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.

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.

This is basically the only change in the old driver behavior which would do:

pinctrl_select_state(i2c->pinctrl, i2c->pinctrl_recovery);
return pinctrl_select_state(i2c->pinctrl, i2c->pinctrl_default);

After obtaining the SDA and SCL pins via devm_gpiod_get() and this somehow
works while the generic I2C code will first call:
pinctrl_select_state(bri->pinctrl, bri->pins_gpio)
Then it obtains the SDA and SCL pins via devm_gpiod_get() and
eventually at the end of i2c_gpio_init_generic_recovery() call
pinctrl_select_state(bri->pinctrl, bri->pins_default);
to return the pins back to I2C state.

Hopefully, this all makes more sense.

>
> If you can't delete the "gpio" pin control state, I would add a
> bool pinctrl_stay_in_device_mode;
> to
> struct i2c_bus_recovery_info
> in include/linux/i2c.h
>
> And just:
>
> if (bri->pinctrl && !bri->pinctrl_stay_in_device_mode)
>     pinctrl_select_state(bri->pinctrl, bri->pins_gpio);
>
> (Also the switch to the "default" state further down could be
> contitional !bri->pinctrl_stay_in_device_mode)
>
> But mostly I wonder why the "gpio" pin control state is defined, if it's
> not to be used.

Well, that is the thing, it is being used, it just somehow got broken with
the move to generic I2C recovery.

I hope to have explained the issue and symptoms more clearly now
as I have ran out of ideas why a call to pinctrl_select_state() to
change to GPIO
state breaks things, but then devm_gpio_get() also eventually makes a call to
the pinctrl driver to change the pin function to GPIO but that works.

Regards,
Robert
>
> Yours,
> Linus Walleij



-- 
Robert Marko
Staff Embedded Linux Engineer
Sartura Ltd.
Lendavska ulica 16a
10000 Zagreb, Croatia
Email: robert.marko@xxxxxxxxxx
Web: www.sartura.hr