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

[RFC 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>
---
I am aware this probably isnt the correct fix, so I am sending it as RFC
cause I have ran out of ideas.

 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: [RFC PATCH] i2c: core: dont change pinmux state to GPIO during recovery setup

[Robert Marko]

On Fri, Sep 1, 2023 at 1:49 PM Robert Marko <robert.marko@sartura.hr> wrote:
>
> 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>
> ---
> I am aware this probably isnt the correct fix, so I am sending it as RFC
> cause I have ran out of ideas.

CC-ing Russel as well since I forgot him.

Regards,
Robert
>
>  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
>


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

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

[Russell King (Oracle)]

On Wed, Sep 06, 2023 at 04:41:33PM +0200, Robert Marko wrote:
> On Fri, Sep 1, 2023 at 1:49 PM Robert Marko <robert.marko@sartura.hr> wrote:
> >
> > 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>
> > ---
> > I am aware this probably isnt the correct fix, so I am sending it as RFC
> > cause I have ran out of ideas.
> 
> CC-ing Russel as well since I forgot him.

So the generic recovery decided to set the pinmux state before calling
devm_gpiod_get(), where as the driver (and my code) originally did this
after calling devm_gpiod_get():

-       /*
-        * Claiming GPIOs can change the pinmux state, which confuses the
-        * pinctrl since pinctrl's idea of the current setting is unaffected
-        * by the pinmux change caused by claiming the GPIO. Work around that
-        * by switching pinctrl to the GPIO state here. We do it this way to
-        * avoid glitching the I2C bus.
-        */
-       pinctrl_select_state(i2c->pinctrl, i2c->pinctrl_recovery);
-
-       return pinctrl_select_state(i2c->pinctrl, i2c->pinctrl_default);

I'd suggest re-implementing my original scheme in the generic code
because this _does_ work on Armada 3720 hardware.

Removing the pinmux frobbing is likely to break stuff.

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

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

[Robert Marko]

On Wed, Sep 6, 2023 at 6:55 PM Russell King (Oracle)
<linux@armlinux.org.uk> wrote:
>
> On Wed, Sep 06, 2023 at 04:41:33PM +0200, Robert Marko wrote:
> > On Fri, Sep 1, 2023 at 1:49 PM Robert Marko <robert.marko@sartura.hr> wrote:
> > >
> > > 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>
> > > ---
> > > I am aware this probably isnt the correct fix, so I am sending it as RFC
> > > cause I have ran out of ideas.
> >
> > CC-ing Russel as well since I forgot him.
>
> So the generic recovery decided to set the pinmux state before calling
> devm_gpiod_get(), where as the driver (and my code) originally did this
> after calling devm_gpiod_get():
>
> -       /*
> -        * Claiming GPIOs can change the pinmux state, which confuses the
> -        * pinctrl since pinctrl's idea of the current setting is unaffected
> -        * by the pinmux change caused by claiming the GPIO. Work around that
> -        * by switching pinctrl to the GPIO state here. We do it this way to
> -        * avoid glitching the I2C bus.
> -        */
> -       pinctrl_select_state(i2c->pinctrl, i2c->pinctrl_recovery);
> -
> -       return pinctrl_select_state(i2c->pinctrl, i2c->pinctrl_default);
>
> I'd suggest re-implementing my original scheme in the generic code
> because this _does_ work on Armada 3720 hardware.
>
> Removing the pinmux frobbing is likely to break stuff.

Hi Russel,
Sorry for the late reply, its been a busy week.

I have tried doing exactly that and calling:

if (bri->pinctrl)
pinctrl_select_state(bri->pinctrl, bri->pins_gpio);
/* change the state of the pins back to their default state */
if (bri->pinctrl)
pinctrl_select_state(bri->pinctrl, bri->pins_default);

However doing that in the generic code does not work, only thing that has
worked so far is removing the GPIO pinctrl call completely.

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



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