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

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

[Robert Marko]

Ever since PXA I2C driver was moved to the generic I2C recovery, I2C has
stopped working completely on Armada 3720 if the pins are specified in DTS.

After a while it was traced down to the only difference being that PXA
driver did not change the pinmux state to GPIO before trying to acquire the
GPIO pins.
And indeed as soon as this call is removed I2C starts working.

To me it seems that this call is not required at all as devm_gpiod_get()
will result in the pinmux state being changed to GPIO via the pinmux
set_mux() op.

Fixes: 0b01392c18b9 ("i2c: pxa: move to generic GPIO recovery")
Signed-off-by: Robert Marko <robert.marko@sartura.hr>
---
 drivers/i2c/i2c-core-base.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 60746652fd52..b34d939078a1 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -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);
-
 	/*
 	 * if there is incomplete or no recovery information, see if generic
 	 * GPIO recovery is available
-- 
2.41.0

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

[Linus Walleij]

On Tue, Sep 26, 2023 at 6:03 PM Robert Marko <robert.marko@sartura.hr> 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.)

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.

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.

Yours,
Linus Walleij

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

[Robert Marko]

On Thu, Sep 28, 2023 at 11:11 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Tue, Sep 26, 2023 at 6:03 PM Robert Marko <robert.marko@sartura.hr> 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@sartura.hr
Web: www.sartura.hr

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

[Linus Walleij]

Hi Robert!

Thanks for getting back on this issue.

On Thu, Nov 9, 2023 at 8:10 PM Robert Marko <robert.marko@sartura.hr> 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

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

[Russell King (Oracle)]

On Thu, Nov 09, 2023 at 09:04:29PM +0100, Linus Walleij wrote:
> Hi Robert!
> 
> Thanks for getting back on this issue.
> 
> On Thu, Nov 9, 2023 at 8:10 PM Robert Marko <robert.marko@sartura.hr> 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?

Note that when I wrote the i2c-pxa recovery code (which was developed
and tested on Armada 3720 - the uDPU) it had to work... when the
suggestion came up to implement generic recovery, I stated:

http://archive.lwn.net:8080/linux-kernel/20200705210942.GA1055@kunai/T/#mf7f862fcd53245f14fb650d33c29cf139d41039d

> > 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().

Yes, indeed. That's because the pinctrl internals get confused. I sent
you an email about it on 6th December 2019

"pinctrl states vs pinmux vs gpio (i2c bus recovery)"

which is why i2c-pxa did things the way it did in my commit
"i2c: pxa: implement generic i2c bus recovery".

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

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

[Linus Walleij]

On Thu, Nov 9, 2023 at 9:30 PM Russell King (Oracle)
<linux@armlinux.org.uk> wrote:
> On Thu, Nov 09, 2023 at 09:04:29PM +0100, Linus Walleij wrote:

> > > 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?
>
> Note that when I wrote the i2c-pxa recovery code (which was developed
> and tested on Armada 3720 - the uDPU) it had to work... when the
> suggestion came up to implement generic recovery, I stated:
>
> http://archive.lwn.net:8080/linux-kernel/20200705210942.GA1055@kunai/T/#mf7f862fcd53245f14fb650d33c29cf139d41039d

Makes me even more convinced that we should just revert this. i.e.
commit 0b01392c18b9993a584f36ace1d61118772ad0ca
i2c: pxa: move to generic GPIO recovery

There is even:
https://lore.kernel.org/linux-i2c/20201209204645.GF3499@kunai/

"In case we missed a glitch, we can still revert the patch later."
Well this is later.

Robert can you see if it possible to revert, that things work after a
revert and send a revert patch?

> > > 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().
>
> Yes, indeed. That's because the pinctrl internals get confused. I sent
> you an email about it on 6th December 2019
>
> "pinctrl states vs pinmux vs gpio (i2c bus recovery)"

I found it:
https://lore.kernel.org/all/20191206173343.GX25745@shell.armlinux.org.uk/

Sadly I had no good advice for any simple elegant solutions
to the problem, but the more complicated solution does
work so let's go for that.

> which is why i2c-pxa did things the way it did in my commit
> "i2c: pxa: implement generic i2c bus recovery".

I think we need to go back to this.

It's nice with the ambition to create generic code of course, but
sometimes it is better to just roll something IP-unique.

Yours,
Linus Walleij

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

[Robert Marko]

On Thu, Nov 9, 2023 at 10:02 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Thu, Nov 9, 2023 at 9:30 PM Russell King (Oracle)
> <linux@armlinux.org.uk> wrote:
> > On Thu, Nov 09, 2023 at 09:04:29PM +0100, Linus Walleij wrote:
>
> > > > 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?
> >
> > Note that when I wrote the i2c-pxa recovery code (which was developed
> > and tested on Armada 3720 - the uDPU) it had to work... when the
> > suggestion came up to implement generic recovery, I stated:
> >
> > http://archive.lwn.net:8080/linux-kernel/20200705210942.GA1055@kunai/T/#mf7f862fcd53245f14fb650d33c29cf139d41039d
>
> Makes me even more convinced that we should just revert this. i.e.
> commit 0b01392c18b9993a584f36ace1d61118772ad0ca
> i2c: pxa: move to generic GPIO recovery
>
> There is even:
> https://lore.kernel.org/linux-i2c/20201209204645.GF3499@kunai/
>
> "In case we missed a glitch, we can still revert the patch later."
> Well this is later.
>
> Robert can you see if it possible to revert, that things work after a
> revert and send a revert patch?

Hi,
Yes, a revert still applies and "fixes" things so I2C starts working as before.

I can send the revert tomorrow, I was just hoping that there was an bug
that could be fixed instead of reverting, but seems its more complicated.

Regards,
Robert
>
> > > > 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().
> >
> > Yes, indeed. That's because the pinctrl internals get confused. I sent
> > you an email about it on 6th December 2019
> >
> > "pinctrl states vs pinmux vs gpio (i2c bus recovery)"
>
> I found it:
> https://lore.kernel.org/all/20191206173343.GX25745@shell.armlinux.org.uk/
>
> Sadly I had no good advice for any simple elegant solutions
> to the problem, but the more complicated solution does
> work so let's go for that.
>
> > which is why i2c-pxa did things the way it did in my commit
> > "i2c: pxa: implement generic i2c bus recovery".
>
> I think we need to go back to this.
>
> It's nice with the ambition to create generic code of course, but
> sometimes it is better to just roll something IP-unique.
>
> Yours,
> Linus Walleij



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