pinctrl states vs pinmux vs gpio (i2c bus recovery)

One may expect:

        pinctrl_select_state(i2c_imx->pinctrl, i2c_imx->pinctrl_pins_default);

to change them back to the default state, but that would be incorrect. The first thing that pinctrl_select_state() does is check whether

        p->state == state

which it will do, as the pinctrl layer hasn't been informed of the change that has happened behind its back at the pinmux level.

Hi Russell,

very nice description of this dual-mode problem.

I wish I had a simple and elegant way we could make it unambiguous and simple to use ... but it beats me right now.

Quoted 11 lines · Russell King - ARM Linux admin

One may expect:

        pinctrl_select_state(i2c_imx->pinctrl, i2c_imx->pinctrl_pins_default);

to change them back to the default state, but that would be incorrect. The first thing that pinctrl_select_state() does is check whether

        p->state == state

which it will do, as the pinctrl layer hasn't been informed of the change that has happened behind its back at the pinmux level.

Some pin controllers have the .strict property set in their struct pinmux_ops:

* @strict: do not allow simultaneous use of the same pin for GPIO and another
* function. Check both gpio_owner and mux_owner strictly before approving
* the pin request.

The non-strict pin controllers are those that actually allow GPIO and device functions to be used on the same physical line at the same time. In this case there is not special GPIO mode for the line in some muxing registers, they are just physically connected somehow.

One usecase is sort of like how tcpdump work for ethernet interfaces: a GPIO register can "snoop" on a pin while in used by another device.

But it would notably also allow you to drive the line and interfere with the device. Which is exactly what this I2C recovery mechanism does, just that its pin controller is actually strict, will not allow the same line to be used for GPIO and some other function at the same time, so I suppose i.MX should probably explore the strict mode.

Enabling that will sadly make the problem MORE complex for this I2C recovery, requiring a cycle of gpiod_put()/gpiod_get() to get it released from GPIO mode, i.e. we would need to just get the GPIO when this is strictly needed. Using devm_gpiod_get() and keeping a reference descriptor around would not work all of a sudden.

I am thinking whether we can handle the non-strict controllers in a more elegant way, or add some API to explicitly hand over between device function and GPIO function. But I can't really see some obvious solution.

Quoted 53 lines · Linus Walleij

Hi Russell,

very nice description of this dual-mode problem.

I wish I had a simple and elegant way we could make it unambiguous and simple to use ... but it beats me right now.

Quoted 11 lines · Russell King - ARM Linux admin

One may expect:

         pinctrl_select_state(i2c_imx->pinctrl, i2c_imx->pinctrl_pins_default);

to change them back to the default state, but that would be incorrect. The first thing that pinctrl_select_state() does is check whether

         p->state == state

which it will do, as the pinctrl layer hasn't been informed of the change that has happened behind its back at the pinmux level.

Some pin controllers have the .strict property set in their struct pinmux_ops:

* @strict: do not allow simultaneous use of the same pin for GPIO and another
* function. Check both gpio_owner and mux_owner strictly before approving
* the pin request.

The non-strict pin controllers are those that actually allow GPIO and device functions to be used on the same physical line at the same time. In this case there is not special GPIO mode for the line in some muxing registers, they are just physically connected somehow.

One usecase is sort of like how tcpdump work for ethernet interfaces: a GPIO register can "snoop" on a pin while in used by another device.

But it would notably also allow you to drive the line and interfere with the device. Which is exactly what this I2C recovery mechanism does, just that its pin controller is actually strict, will not allow the same line to be used for GPIO and some other function at the same time, so I suppose i.MX should probably explore the strict mode.

Enabling that will sadly make the problem MORE complex for this I2C recovery, requiring a cycle of gpiod_put()/gpiod_get() to get it released from GPIO mode, i.e. we would need to just get the GPIO when this is strictly needed. Using devm_gpiod_get() and keeping a reference descriptor around would not work all of a sudden.

Quoted 4 lines

I am thinking whether we can handle the non-strict controllers in a more elegant way, or add some API to explicitly hand over between device function and GPIO function. But I can't really see some obvious solution.

What I'm currently trying is (error handling removed for brevity):

	struct i2c_bus_recovery_info *bri = &i2c->recovery;

         i2c->pinctrl = devm_pinctrl_get(dev);
         i2c->pinctrl_default = pinctrl_lookup_state(i2c->pinctrl,
                                                     PINCTRL_STATE_DEFAULT);
         i2c->pinctrl_recovery = pinctrl_lookup_state(i2c->pinctrl,
						     "recovery");
         bri->sda_gpiod = devm_gpiod_get(dev, "sda", GPIOD_OUT_HIGH_OPEN_DRAIN);
         bri->scl_gpiod = devm_gpiod_get(dev, "scl", GPIOD_OUT_HIGH_OPEN_DRAIN);

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

which seems good enough to get the pins back into i2c mode after the gpios are obtained. Then we switch the pinctrl state between pinctrl_recovery and pinctrl_default as we have need to.

The problem is, the generic i2c bus recovery code wants the gpiod descriptors to be setup and inplace by the time i2c_init_recovery() is called (which is called when the adapter is registered) so holding off until we need to do recovery doesn't work.

This seems to work for this SoC I'm currently working with, but I think there's more on the horizon - I'm having the same problems on another SoC which also needs bus recovery implemented, and as the problem device is behind an I2C bus mux, when it locks the I2C bus, it kills all I2C buses rooted at that particular SoC I2C controller. However, there's a problem - the pinctrls for that SoC are set by ROM firmware at boot time by reading a table from the boot media. *Unprintables about firmware being too way limiting*. :p

Quoted 83 lines · Russell King - ARM Linux admin

Quoted 53 lines · Linus Walleij

Hi Russell,

very nice description of this dual-mode problem.

I wish I had a simple and elegant way we could make it unambiguous and simple to use ... but it beats me right now.

Quoted 11 lines · Russell King - ARM Linux admin

One may expect:

         pinctrl_select_state(i2c_imx->pinctrl, i2c_imx->pinctrl_pins_default);

to change them back to the default state, but that would be incorrect. The first thing that pinctrl_select_state() does is check whether

         p->state == state

which it will do, as the pinctrl layer hasn't been informed of the change that has happened behind its back at the pinmux level.

Some pin controllers have the .strict property set in their struct pinmux_ops:

* @strict: do not allow simultaneous use of the same pin for GPIO and another *      function. Check both gpio_owner and mux_owner strictly before approving *      the pin request.

The non-strict pin controllers are those that actually allow GPIO and device functions to be used on the same physical line at the same time. In this case there is not special GPIO mode for the line in some muxing registers, they are just physically connected somehow.

One usecase is sort of like how tcpdump work for ethernet interfaces: a GPIO register can "snoop" on a pin while in used by another device.

But it would notably also allow you to drive the line and interfere with the device. Which is exactly what this I2C recovery mechanism does, just that its pin controller is actually strict, will not allow the same line to be used for GPIO and some other function at the same time, so I suppose i.MX should probably explore the strict mode.

Enabling that will sadly make the problem MORE complex for this I2C recovery, requiring a cycle of gpiod_put()/gpiod_get() to get it released from GPIO mode, i.e. we would need to just get the GPIO when this is strictly needed. Using devm_gpiod_get() and keeping a reference descriptor around would not work all of a sudden.

Quoted 4 lines

I am thinking whether we can handle the non-strict controllers in a more elegant way, or add some API to explicitly hand over between device function and GPIO function. But I can't really see some obvious solution.

What I'm currently trying is (error handling removed for brevity):

    struct i2c_bus_recovery_info *bri = &i2c->recovery;

         i2c->pinctrl = devm_pinctrl_get(dev); i2c->pinctrl_default = pinctrl_lookup_state(i2c->pinctrl,

PINCTRL_STATE_DEFAULT);
         i2c->pinctrl_recovery = pinctrl_lookup_state(i2c->pinctrl, "recovery");
         bri->sda_gpiod = devm_gpiod_get(dev, "sda", GPIOD_OUT_HIGH_OPEN_DRAIN); bri->scl_gpiod = devm_gpiod_get(dev, "scl", GPIOD_OUT_HIGH_OPEN_DRAIN);

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

which seems good enough to get the pins back into i2c mode after the gpios are obtained.  Then we switch the pinctrl state between pinctrl_recovery and pinctrl_default as we have need to.

The problem is, the generic i2c bus recovery code wants the gpiod descriptors to be setup and inplace by the time i2c_init_recovery() is called (which is called when the adapter is registered) so holding off until we need to do recovery doesn't work.

Quoted 9 lines

This seems to work for this SoC I'm currently working with, but I think there's more on the horizon - I'm having the same problems on another SoC which also needs bus recovery implemented, and as the problem device is behind an I2C bus mux, when it locks the I2C bus, it kills all I2C buses rooted at that particular SoC I2C controller.  However, there's a problem - the pinctrls for that SoC are set by ROM firmware at boot time by reading a table from the boot media.  *Unprintables about firmware being too way limiting*. :p

Hi all, what's the current state of this? I can confirm that this is broken with the at91 i2c controller's recovery mode[1], which is implemented exactly the same as other i2c master recovery modes, so I suspect them to be broken as well.

I'm using 5.5.6 with this patch applied (which adds the recovery):
    https://patchwork.kernel.org/cover/11333883/

It worked fine with 5.2, but has now broken, the way Russell describes, in 5.5.6 and also on the latest 5.6-rc3. Russell's suggested workaround of setting the pinctrl to recovery (gpio) and then back to default does make it work.

Alan.

[1] currently the patch for i2c recovery for at91 is accepted to Wolfram Sang's for-next tree.

EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe

Quoted 112 lines · Alan Ott

Quoted 88 lines · Russell King - ARM Linux admin

Quoted 56 lines · Linus Walleij

Hi Russell,

very nice description of this dual-mode problem.

I wish I had a simple and elegant way we could make it unambiguous and simple to use ... but it beats me right now.

Quoted 5 lines · Russell King - ARM Linux admin

One may expect:

         pinctrl_select_state(i2c_imx->pinctrl,
i2c_imx->pinctrl_pins_default);

Quoted 7 lines

to change them back to the default state, but that would be incorrect. The first thing that pinctrl_select_state() does is check whether

         p->state == state

which it will do, as the pinctrl layer hasn't been informed of the change that has happened behind its back at the pinmux level.

Some pin controllers have the .strict property set in their struct pinmux_ops:

* @strict: do not allow simultaneous use of the same pin for GPIO and another
*      function. Check both gpio_owner and mux_owner strictly before approving
*      the pin request.

The non-strict pin controllers are those that actually allow GPIO and device functions to be used on the same physical line at the same time. In this case there is not special GPIO mode for the line in some muxing registers, they are just physically connected somehow.

One usecase is sort of like how tcpdump work for ethernet interfaces: a GPIO register can "snoop" on a pin while in used by another device.

But it would notably also allow you to drive the line and interfere with the device. Which is exactly what this I2C recovery mechanism does, just that its pin controller is actually strict, will not allow the same line to be used for GPIO and some other function at the same time, so I suppose i.MX should probably explore the strict mode.

Enabling that will sadly make the problem MORE complex for this I2C recovery, requiring a cycle of gpiod_put()/gpiod_get() to get it released from GPIO mode, i.e. we would need to just get the GPIO when this is strictly needed. Using devm_gpiod_get() and keeping a reference descriptor around would not work all of a sudden.

Quoted 4 lines

I am thinking whether we can handle the non-strict controllers in a more elegant way, or add some API to explicitly hand over between device function and GPIO function. But I can't really see some obvious solution.

What I'm currently trying is (error handling removed for brevity):

    struct i2c_bus_recovery_info *bri = &i2c->recovery;

         i2c->pinctrl = devm_pinctrl_get(dev); i2c->pinctrl_default = pinctrl_lookup_state(i2c->pinctrl,

PINCTRL_STATE_DEFAULT);
         i2c->pinctrl_recovery = pinctrl_lookup_state(i2c->pinctrl, "recovery");
         bri->sda_gpiod = devm_gpiod_get(dev, "sda", GPIOD_OUT_HIGH_OPEN_DRAIN);
         bri->scl_gpiod = devm_gpiod_get(dev, "scl", GPIOD_OUT_HIGH_OPEN_DRAIN);

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

which seems good enough to get the pins back into i2c mode after the gpios are obtained.  Then we switch the pinctrl state between pinctrl_recovery and pinctrl_default as we have need to.

The problem is, the generic i2c bus recovery code wants the gpiod descriptors to be setup and inplace by the time i2c_init_recovery() is called (which is called when the adapter is registered) so holding off until we need to do recovery doesn't work.

Quoted 9 lines

This seems to work for this SoC I'm currently working with, but I think there's more on the horizon - I'm having the same problems on another SoC which also needs bus recovery implemented, and as the problem device is behind an I2C bus mux, when it locks the I2C bus, it kills all I2C buses rooted at that particular SoC I2C controller.  However, there's a problem - the pinctrls for that SoC are set by ROM firmware at boot time by reading a table from the boot media.  *Unprintables about firmware being too way limiting*. :p

Hi all, what's the current state of this? I can confirm that this is broken with the at91 i2c controller's recovery mode[1], which is implemented exactly the same as other i2c master recovery modes, so I suspect them to be broken as well.

I'm using 5.5.6 with this patch applied (which adds the recovery):
     https://patchwork.kernel.org/cover/11333883/

It worked fine with 5.2, but has now broken, the way Russell describes, in 5.5.6 and also on the latest 5.6-rc3. Russell's suggested workaround of setting the pinctrl to recovery (gpio) and then back to default does make it work.

Quoted 5 lines

Alan.

[1] currently the patch for i2c recovery for at91 is accepted to Wolfram Sang's for-next tree.

Is there any word on this?

Alan.

Quoted 122 lines · Alan Ott

EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe

Quoted 117 lines · Alan Ott

Quoted 88 lines · Russell King - ARM Linux admin

Quoted 56 lines · Linus Walleij

Hi Russell,

very nice description of this dual-mode problem.

I wish I had a simple and elegant way we could make it unambiguous and simple to use ... but it beats me right now.

Quoted 5 lines · Russell King - ARM Linux admin

One may expect:

         pinctrl_select_state(i2c_imx->pinctrl,
i2c_imx->pinctrl_pins_default);

Quoted 7 lines

to change them back to the default state, but that would be incorrect. The first thing that pinctrl_select_state() does is check whether

         p->state == state

which it will do, as the pinctrl layer hasn't been informed of the change that has happened behind its back at the pinmux level.

Some pin controllers have the .strict property set in their struct pinmux_ops:

* @strict: do not allow simultaneous use of the same pin for GPIO and another
*      function. Check both gpio_owner and mux_owner strictly before approving
*      the pin request.

The non-strict pin controllers are those that actually allow GPIO and device functions to be used on the same physical line at the same time. In this case there is not special GPIO mode for the line in some muxing registers, they are just physically connected somehow.

One usecase is sort of like how tcpdump work for ethernet interfaces: a GPIO register can "snoop" on a pin while in used by another device.

But it would notably also allow you to drive the line and interfere with the device. Which is exactly what this I2C recovery mechanism does, just that its pin controller is actually strict, will not allow the same line to be used for GPIO and some other function at the same time, so I suppose i.MX should probably explore the strict mode.

Enabling that will sadly make the problem MORE complex for this I2C recovery, requiring a cycle of gpiod_put()/gpiod_get() to get it released from GPIO mode, i.e. we would need to just get the GPIO when this is strictly needed. Using devm_gpiod_get() and keeping a reference descriptor around would not work all of a sudden.

Quoted 4 lines

I am thinking whether we can handle the non-strict controllers in a more elegant way, or add some API to explicitly hand over between device function and GPIO function. But I can't really see some obvious solution.

What I'm currently trying is (error handling removed for brevity):

    struct i2c_bus_recovery_info *bri = &i2c->recovery;

         i2c->pinctrl = devm_pinctrl_get(dev); i2c->pinctrl_default = pinctrl_lookup_state(i2c->pinctrl,

PINCTRL_STATE_DEFAULT);
         i2c->pinctrl_recovery = pinctrl_lookup_state(i2c->pinctrl, "recovery");
         bri->sda_gpiod = devm_gpiod_get(dev, "sda",
GPIOD_OUT_HIGH_OPEN_DRAIN);
         bri->scl_gpiod = devm_gpiod_get(dev, "scl",
GPIOD_OUT_HIGH_OPEN_DRAIN);

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

which seems good enough to get the pins back into i2c mode after the gpios are obtained.  Then we switch the pinctrl state between pinctrl_recovery and pinctrl_default as we have need to.

The problem is, the generic i2c bus recovery code wants the gpiod descriptors to be setup and inplace by the time i2c_init_recovery() is called (which is called when the adapter is registered) so holding off until we need to do recovery doesn't work.

Quoted 9 lines

This seems to work for this SoC I'm currently working with, but I think there's more on the horizon - I'm having the same problems on another SoC which also needs bus recovery implemented, and as the problem device is behind an I2C bus mux, when it locks the I2C bus, it kills all I2C buses rooted at that particular SoC I2C controller.  However, there's a problem - the pinctrls for that SoC are set by ROM firmware at boot time by reading a table from the boot media.  *Unprintables about firmware being too way limiting*. :p

Hi all, what's the current state of this? I can confirm that this is broken with the at91 i2c controller's recovery mode[1], which is implemented exactly the same as other i2c master recovery modes, so I suspect them to be broken as well.

I'm using 5.5.6 with this patch applied (which adds the recovery):
     https://patchwork.kernel.org/cover/11333883/

It worked fine with 5.2, but has now broken, the way Russell describes, in 5.5.6 and also on the latest 5.6-rc3. Russell's suggested workaround of setting the pinctrl to recovery (gpio) and then back to default does make it work.

Alan.

[1] currently the patch for i2c recovery for at91 is accepted to Wolfram Sang's for-next tree.

Quoted 2 lines

Is there any word on this?

Internally we have managed it in the same way as the one suggested by Russell.

We wondered if we should mainline it or not as it's really tricky to proceed like this.

In the future, we may declare our pinctrl as strict which should cause another breakage... It's not done yet because when I tried to do it, maybe it has changed now, I was not able to apply the pin configuration to the pin muxed as a gpio.

EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe

Quoted 128 lines · On 3/25/20 4:06 PM, [email protected] wrote:

Quoted 126 lines · Alan Ott

EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe

Quoted 90 lines · Alan Ott

Quoted 89 lines · Russell King - ARM Linux admin

Quoted 57 lines · Linus Walleij

Hi Russell,

very nice description of this dual-mode problem.

I wish I had a simple and elegant way we could make it unambiguous and simple to use ... but it beats me right now.

Quoted 5 lines · Russell King - ARM Linux admin

One may expect:

          pinctrl_select_state(i2c_imx->pinctrl,
i2c_imx->pinctrl_pins_default);

Quoted 8 lines

to change them back to the default state, but that would be incorrect.
The first thing that pinctrl_select_state() does is check whether

          p->state == state

which it will do, as the pinctrl layer hasn't been informed of the change that has happened behind its back at the pinmux level.

Some pin controllers have the .strict property set in their struct pinmux_ops:

* @strict: do not allow simultaneous use of the same pin for GPIO and another
*      function. Check both gpio_owner and mux_owner strictly before approving
*      the pin request.

The non-strict pin controllers are those that actually allow GPIO and device functions to be used on the same physical line at the same time. In this case there is not special GPIO mode for the line in some muxing registers, they are just physically connected somehow.

One usecase is sort of like how tcpdump work for ethernet interfaces: a GPIO register can "snoop" on a pin while in used by another device.

But it would notably also allow you to drive the line and interfere with the device. Which is exactly what this I2C recovery mechanism does, just that its pin controller is actually strict, will not allow the same line to be used for GPIO and some other function at the same time, so I suppose i.MX should probably explore the strict mode.

Enabling that will sadly make the problem MORE complex for this I2C recovery, requiring a cycle of gpiod_put()/gpiod_get() to get it released from GPIO mode, i.e. we would need to just get the GPIO when this is strictly needed. Using devm_gpiod_get() and keeping a reference descriptor around would not work all of a sudden.

Quoted 4 lines

I am thinking whether we can handle the non-strict controllers in a more elegant way, or add some API to explicitly hand over between device function and GPIO function. But I can't really see some obvious solution.

What I'm currently trying is (error handling removed for brevity):

     struct i2c_bus_recovery_info *bri = &i2c->recovery;

          i2c->pinctrl = devm_pinctrl_get(dev); i2c->pinctrl_default = pinctrl_lookup_state(i2c->pinctrl,

PINCTRL_STATE_DEFAULT);
          i2c->pinctrl_recovery = pinctrl_lookup_state(i2c->pinctrl, "recovery");
          bri->sda_gpiod = devm_gpiod_get(dev, "sda", GPIOD_OUT_HIGH_OPEN_DRAIN);
          bri->scl_gpiod = devm_gpiod_get(dev, "scl", GPIOD_OUT_HIGH_OPEN_DRAIN);

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

which seems good enough to get the pins back into i2c mode after the gpios are obtained.  Then we switch the pinctrl state between pinctrl_recovery and pinctrl_default as we have need to.

The problem is, the generic i2c bus recovery code wants the gpiod descriptors to be setup and inplace by the time i2c_init_recovery() is called (which is called when the adapter is registered) so holding off until we need to do recovery doesn't work.

Quoted 9 lines

Quoted 9 lines

This seems to work for this SoC I'm currently working with, but I think there's more on the horizon - I'm having the same problems on another SoC which also needs bus recovery implemented, and as the problem device is behind an I2C bus mux, when it locks the I2C bus, it kills all I2C buses rooted at that particular SoC I2C controller.  However, there's a problem - the pinctrls for that SoC are set by ROM firmware at boot time by reading a table from the boot media.  *Unprintables about firmware being too way limiting*. :p

  >

Quoted 13 lines

Hi all, what's the current state of this? I can confirm that this is broken with the at91 i2c controller's recovery mode[1], which is implemented exactly the same as other i2c master recovery modes, so I suspect them to be broken as well.

I'm using 5.5.6 with this patch applied (which adds the recovery):
      https://patchwork.kernel.org/cover/11333883/

It worked fine with 5.2, but has now broken, the way Russell describes, in 5.5.6 and also on the latest 5.6-rc3. Russell's suggested workaround of setting the pinctrl to recovery (gpio) and then back to default does make it work.

Quoted 6 lines

Alan.

[1] currently the patch for i2c recovery for at91 is accepted to Wolfram
Sang's for-next tree.

Is there any word on this?

Quoted 5 lines

Internally we have managed it in the same way as the one suggested by Russell.

We wondered if we should mainline it or not as it's really tricky to proceed like this.

Certainly it needs to work in mainline though, right? Not just in the linux4sam vendor kernel?

Quoted 7 lines

In the future, we may declare our pinctrl as strict which should cause another breakage... It's not done yet because when I tried to do it, maybe it has changed now, I was not able to apply the pin configuration to the pin muxed as a gpio.

The larger question I think is, is this a breakage in gpio? i2c-at91 is not the only i2c driver which uses gpio-based bus recovery, and many of them use nearly the exact same code as i2c-at91. Are they all broken with this kernel update too?

Quoted 138 lines · Alan Ott

EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe

Quoted 133 lines · On 3/25/20 4:06 PM, [email protected] wrote:

Quoted 117 lines · Alan Ott

EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe

Quoted 90 lines · Alan Ott

Quoted 89 lines · Russell King - ARM Linux admin

Quoted 57 lines · Linus Walleij

Hi Russell,

very nice description of this dual-mode problem.

I wish I had a simple and elegant way we could make it unambiguous and simple to use ... but it beats me right now.

Quoted 5 lines · Russell King - ARM Linux admin

One may expect:

          pinctrl_select_state(i2c_imx->pinctrl,
i2c_imx->pinctrl_pins_default);

Quoted 8 lines

to change them back to the default state, but that would be incorrect.
The first thing that pinctrl_select_state() does is check whether

          p->state == state

which it will do, as the pinctrl layer hasn't been informed of the change that has happened behind its back at the pinmux level.

Some pin controllers have the .strict property set in their struct pinmux_ops:

* @strict: do not allow simultaneous use of the same pin for GPIO and another
*      function. Check both gpio_owner and mux_owner strictly before approving
*      the pin request.

The non-strict pin controllers are those that actually allow GPIO and device functions to be used on the same physical line at the same time. In this case there is not special GPIO mode for the line in some muxing registers, they are just physically connected somehow.

One usecase is sort of like how tcpdump work for ethernet interfaces: a GPIO register can "snoop" on a pin while in used by another device.

But it would notably also allow you to drive the line and interfere with the device. Which is exactly what this I2C recovery mechanism does, just that its pin controller is actually strict, will not allow the same line to be used for GPIO and some other function at the same time, so I suppose i.MX should probably explore the strict mode.

Enabling that will sadly make the problem MORE complex for this I2C recovery, requiring a cycle of gpiod_put()/gpiod_get() to get it released from GPIO mode, i.e. we would need to just get the GPIO when this is strictly needed. Using devm_gpiod_get() and keeping a reference descriptor around would not work all of a sudden.

Quoted 4 lines

I am thinking whether we can handle the non-strict controllers in a more elegant way, or add some API to explicitly hand over between device function and GPIO function. But I can't really see some obvious solution.

What I'm currently trying is (error handling removed for brevity):

     struct i2c_bus_recovery_info *bri = &i2c->recovery;

          i2c->pinctrl = devm_pinctrl_get(dev); i2c->pinctrl_default = pinctrl_lookup_state(i2c->pinctrl,

PINCTRL_STATE_DEFAULT);
          i2c->pinctrl_recovery = pinctrl_lookup_state(i2c->pinctrl, "recovery");
          bri->sda_gpiod = devm_gpiod_get(dev, "sda",
GPIOD_OUT_HIGH_OPEN_DRAIN);
          bri->scl_gpiod = devm_gpiod_get(dev, "scl",
GPIOD_OUT_HIGH_OPEN_DRAIN);

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

which seems good enough to get the pins back into i2c mode after the gpios are obtained.  Then we switch the pinctrl state between pinctrl_recovery and pinctrl_default as we have need to.

The problem is, the generic i2c bus recovery code wants the gpiod descriptors to be setup and inplace by the time i2c_init_recovery() is called (which is called when the adapter is registered) so holding off until we need to do recovery doesn't work.

Quoted 9 lines

Quoted 9 lines

This seems to work for this SoC I'm currently working with, but I think there's more on the horizon - I'm having the same problems on another SoC which also needs bus recovery implemented, and as the problem device is behind an I2C bus mux, when it locks the I2C bus, it kills all I2C buses rooted at that particular SoC I2C controller.  However, there's a problem - the pinctrls for that SoC are set by ROM firmware at boot time by reading a table from the boot media.  *Unprintables about firmware being too way limiting*. :p

  >

Quoted 13 lines

Hi all, what's the current state of this? I can confirm that this is broken with the at91 i2c controller's recovery mode[1], which is implemented exactly the same as other i2c master recovery modes, so I suspect them to be broken as well.

I'm using 5.5.6 with this patch applied (which adds the recovery):
      https://patchwork.kernel.org/cover/11333883/

It worked fine with 5.2, but has now broken, the way Russell describes, in 5.5.6 and also on the latest 5.6-rc3. Russell's suggested workaround of setting the pinctrl to recovery (gpio) and then back to default does make it work.

Quoted 9 lines

Quoted 6 lines

Alan.

[1] currently the patch for i2c recovery for at91 is accepted to Wolfram
Sang's for-next tree.

Is there any word on this?

Internally we have managed it in the same way as the one suggested by Russell.

We wondered if we should mainline it or not as it's really tricky to proceed like this.

Quoted 2 lines

Certainly it needs to work in mainline though, right? Not just in the linux4sam vendor kernel?

It has been fixed two days ago. We'll send it and see if it will be accepted.

By the way, with which SoC have you encountered this issue? It is of intereset as we have two different pin controllers.

Quoted 9 lines

Quoted 8 lines

In the future, we may declare our pinctrl as strict which should cause another breakage... It's not done yet because when I tried to do it, maybe it has changed now, I was not able to apply the pin configuration to the pin muxed as a gpio.

Quoted 5 lines

The larger question I think is, is this a breakage in gpio? i2c-at91 is not the only i2c driver which uses gpio-based bus recovery, and many of them use nearly the exact same code as i2c-at91. Are they all broken with this kernel update too?

I don't know what changed in gpio or pinctrl. Thinking more about it I'am surprised it had worked. In my mind, gpiod_get has always ended with a call to the gpio_request_enable operation so changing the mux to a gpio function.

The pinctrl-at91 defines the gpio_request_enable op while the pinctrl-at91-pio4 not yet. That's why I asked for the SoC you use.

EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe

Quoted 151 lines · On 3/26/20 2:53 AM, [email protected] wrote:

Quoted 149 lines · Alan Ott

EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe

Quoted 134 lines · On 3/25/20 4:06 PM, [email protected] wrote:

Quoted 133 lines · Alan Ott

EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe

Quoted 95 lines · Alan Ott

Quoted 94 lines · Russell King - ARM Linux admin

Quoted 60 lines · Linus Walleij

Hi Russell,

very nice description of this dual-mode problem.

I wish I had a simple and elegant way we could make it unambiguous and simple to use ... but it beats me right now.

Quoted 5 lines · Russell King - ARM Linux admin

One may expect:

           pinctrl_select_state(i2c_imx->pinctrl,
i2c_imx->pinctrl_pins_default);

Quoted 8 lines

to change them back to the default state, but that would be incorrect.
The first thing that pinctrl_select_state() does is check whether

           p->state == state

which it will do, as the pinctrl layer hasn't been informed of the change that has happened behind its back at the pinmux level.

Some pin controllers have the .strict property set in their struct pinmux_ops:

* @strict: do not allow simultaneous use of the same pin for GPIO and
another
*      function. Check both gpio_owner and mux_owner strictly before
approving
*      the pin request.

The non-strict pin controllers are those that actually allow GPIO and device functions to be used on the same physical line at the same time. In this case there is not special GPIO mode for the line in some muxing registers, they are just physically connected somehow.

One usecase is sort of like how tcpdump work for ethernet interfaces: a GPIO register can "snoop" on a pin while in used by another device.

But it would notably also allow you to drive the line and interfere with the device. Which is exactly what this I2C recovery mechanism does, just that its pin controller is actually strict, will not allow
the same line to be used for GPIO and some other function at the same time, so I suppose i.MX should probably explore the strict mode.

Enabling that will sadly make the problem MORE complex for this I2C recovery, requiring a cycle of gpiod_put()/gpiod_get() to get it released from GPIO mode, i.e. we would need to just get the GPIO when this is strictly needed. Using devm_gpiod_get() and keeping a reference descriptor around would not work all of a sudden.

Quoted 4 lines

I am thinking whether we can handle the non-strict controllers in a more elegant way, or add some API to explicitly hand over between device function and GPIO function. But I can't really see some obvious solution.

What I'm currently trying is (error handling removed for brevity):

      struct i2c_bus_recovery_info *bri = &i2c->recovery;

           i2c->pinctrl = devm_pinctrl_get(dev); i2c->pinctrl_default = pinctrl_lookup_state(i2c->pinctrl,

PINCTRL_STATE_DEFAULT);
           i2c->pinctrl_recovery = pinctrl_lookup_state(i2c->pinctrl, "recovery"); bri->sda_gpiod = devm_gpiod_get(dev, "sda", GPIOD_OUT_HIGH_OPEN_DRAIN);
           bri->scl_gpiod = devm_gpiod_get(dev, "scl", GPIOD_OUT_HIGH_OPEN_DRAIN);

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

which seems good enough to get the pins back into i2c mode after the gpios are obtained.  Then we switch the pinctrl state between pinctrl_recovery and pinctrl_default as we have need to.

The problem is, the generic i2c bus recovery code wants the gpiod descriptors to be setup and inplace by the time i2c_init_recovery() is called (which is called when the adapter is registered) so holding off until we need to do recovery doesn't work.

Quoted 10 lines

Quoted 10 lines

This seems to work for this SoC I'm currently working with, but I think there's more on the horizon - I'm having the same problems on another SoC which also needs bus recovery implemented, and as the problem device is behind an I2C bus mux, when it locks the I2C bus, it kills all I2C buses rooted at that particular SoC I2C controller.  However, there's a problem - the pinctrls for that SoC are set by ROM firmware at boot time by reading a table from the boot media.  *Unprintables about firmware being too way limiting*. :p

   >

Quoted 22 lines

Hi all, what's the current state of this? I can confirm that this is broken with the at91 i2c controller's recovery mode[1], which is implemented exactly the same as other i2c master recovery modes, so I suspect them to be broken as well.

I'm using 5.5.6 with this patch applied (which adds the recovery):
       https://patchwork.kernel.org/cover/11333883/

It worked fine with 5.2, but has now broken, the way Russell describes,
in 5.5.6 and also on the latest 5.6-rc3. Russell's suggested workaround
of setting the pinctrl to recovery (gpio) and then back to default does
make it work.

Alan.

[1] currently the patch for i2c recovery for at91 is accepted to Wolfram
Sang's for-next tree.

Quoted 8 lines

Quoted 2 lines

Is there any word on this?

Internally we have managed it in the same way as the one suggested by Russell.

We wondered if we should mainline it or not as it's really tricky to proceed like this.

Certainly it needs to work in mainline though, right? Not just in the linux4sam vendor kernel?

Quoted 5 lines

It has been fixed two days ago. We'll send it and see if it will be accepted.

By the way, with which SoC have you encountered this issue? It is of intereset as we have two different pin controllers.

SAMA5D33

Quoted 15 lines

Quoted 14 lines

Quoted 8 lines

In the future, we may declare our pinctrl as strict which should cause another breakage... It's not done yet because when I tried to do it, maybe it has changed now, I was not able to apply the pin configuration to the pin muxed as a gpio.

The larger question I think is, is this a breakage in gpio? i2c-at91 is not the only i2c driver which uses gpio-based bus recovery, and many of them use nearly the exact same code as i2c-at91. Are they all broken with this kernel update too?

Quoted 4 lines

I don't know what changed in gpio or pinctrl. Thinking more about it I'am surprised it had worked. In my mind, gpiod_get has always ended with a call to the gpio_request_enable operation so changing the mux to a gpio function.

I definitely did an A/B test with 5.2 and 5.5 before writing the email, and I also definitely traced it down to devm_gpiod_get() (ie: leave all the rest and take those two lines out in 5.5, and that made it not fail).

Again, other i2c controllers are built the exact same way, with a handful of them using the same copied/pasted code. I suspect they are broken too (when gpio bus recovery is enabled). If they're not, then does it mean devm_gpiod_get() works differently on different CPUs?

Quoted 4 lines

The pinctrl-at91 defines the gpio_request_enable op while the pinctrl-at91-pio4 not yet. That's why I asked for the SoC you use.

Ok.

Alan.

Quoted 155 lines · Alan Ott

EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe

Quoted 151 lines · On 3/26/20 2:53 AM, [email protected] wrote:

Quoted 149 lines · Alan Ott

EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe

Quoted 134 lines · On 3/25/20 4:06 PM, [email protected] wrote:

Quoted 133 lines · Alan Ott

EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe

Quoted 95 lines · Alan Ott

Quoted 94 lines · Russell King - ARM Linux admin

Quoted 60 lines · Linus Walleij

Hi Russell,

very nice description of this dual-mode problem.

I wish I had a simple and elegant way we could make it unambiguous and simple to use ... but it beats me right now.

Quoted 5 lines · Russell King - ARM Linux admin

One may expect:

           pinctrl_select_state(i2c_imx->pinctrl,
i2c_imx->pinctrl_pins_default);

Quoted 8 lines

to change them back to the default state, but that would be incorrect.
The first thing that pinctrl_select_state() does is check whether

           p->state == state

which it will do, as the pinctrl layer hasn't been informed of the change that has happened behind its back at the pinmux level.

Some pin controllers have the .strict property set in their struct pinmux_ops:

* @strict: do not allow simultaneous use of the same pin for GPIO and
another
*      function. Check both gpio_owner and mux_owner strictly before
approving
*      the pin request.

The non-strict pin controllers are those that actually allow GPIO and device functions to be used on the same physical line at the same time. In this case there is not special GPIO mode for the line in some muxing registers, they are just physically connected somehow.

One usecase is sort of like how tcpdump work for ethernet interfaces: a GPIO register can "snoop" on a pin while in used by another device.

But it would notably also allow you to drive the line and interfere with the device. Which is exactly what this I2C recovery mechanism does, just that its pin controller is actually strict, will not allow
the same line to be used for GPIO and some other function at the same time, so I suppose i.MX should probably explore the strict mode.

Enabling that will sadly make the problem MORE complex for this I2C recovery, requiring a cycle of gpiod_put()/gpiod_get() to get it released from GPIO mode, i.e. we would need to just get the GPIO when this is strictly needed. Using devm_gpiod_get() and keeping a reference descriptor around would not work all of a sudden.

Quoted 4 lines

I am thinking whether we can handle the non-strict controllers in a more elegant way, or add some API to explicitly hand over between device function and GPIO function. But I can't really see some obvious solution.

What I'm currently trying is (error handling removed for brevity):

      struct i2c_bus_recovery_info *bri = &i2c->recovery;

           i2c->pinctrl = devm_pinctrl_get(dev); i2c->pinctrl_default = pinctrl_lookup_state(i2c->pinctrl,

PINCTRL_STATE_DEFAULT);
           i2c->pinctrl_recovery =
pinctrl_lookup_state(i2c->pinctrl,
                               "recovery"); bri->sda_gpiod = devm_gpiod_get(dev, "sda",
GPIOD_OUT_HIGH_OPEN_DRAIN);
           bri->scl_gpiod = devm_gpiod_get(dev, "scl",
GPIOD_OUT_HIGH_OPEN_DRAIN);

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

which seems good enough to get the pins back into i2c mode after the gpios are obtained.  Then we switch the pinctrl state between pinctrl_recovery and pinctrl_default as we have need to.

The problem is, the generic i2c bus recovery code wants the gpiod descriptors to be setup and inplace by the time i2c_init_recovery() is called (which is called when the adapter is registered) so holding off until we need to do recovery doesn't work.

Quoted 10 lines

Quoted 10 lines

This seems to work for this SoC I'm currently working with, but I think there's more on the horizon - I'm having the same problems on another SoC which also needs bus recovery implemented, and as the problem device is behind an I2C bus mux, when it locks the I2C bus, it kills all I2C buses rooted at that particular SoC I2C controller.  However, there's a problem - the pinctrls for that SoC are set by ROM firmware at boot time by reading a table from the boot media.  *Unprintables about firmware being too way limiting*. :p

   >

Quoted 22 lines

Hi all, what's the current state of this? I can confirm that this is broken with the at91 i2c controller's recovery mode[1], which is implemented exactly the same as other i2c master recovery modes, so I suspect them to be broken as well.

I'm using 5.5.6 with this patch applied (which adds the recovery):
       https://patchwork.kernel.org/cover/11333883/

It worked fine with 5.2, but has now broken, the way Russell describes,
in 5.5.6 and also on the latest 5.6-rc3. Russell's suggested workaround
of setting the pinctrl to recovery (gpio) and then back to default does
make it work.

Alan.

[1] currently the patch for i2c recovery for at91 is accepted to Wolfram
Sang's for-next tree.

Quoted 8 lines

Quoted 2 lines

Is there any word on this?

Internally we have managed it in the same way as the one suggested by Russell.

We wondered if we should mainline it or not as it's really tricky to proceed like this.

Certainly it needs to work in mainline though, right? Not just in the linux4sam vendor kernel?

Quoted 8 lines

Quoted 5 lines

It has been fixed two days ago. We'll send it and see if it will be accepted.

By the way, with which SoC have you encountered this issue? It is of intereset as we have two different pin controllers.

SAMA5D33

Ok, thanks.

Quoted 20 lines

Quoted 19 lines

Quoted 9 lines

Quoted 8 lines

In the future, we may declare our pinctrl as strict which should cause another breakage... It's not done yet because when I tried to do it, maybe it has changed now, I was not able to apply the pin configuration to the pin muxed as a gpio.

Quoted 5 lines

The larger question I think is, is this a breakage in gpio? i2c-at91 is not the only i2c driver which uses gpio-based bus recovery, and many of them use nearly the exact same code as i2c-at91. Are they all broken with this kernel update too?

I don't know what changed in gpio or pinctrl. Thinking more about it I'am surprised it had worked. In my mind, gpiod_get has always ended with a call to the gpio_request_enable operation so changing the mux to a gpio function.

Quoted 4 lines

I definitely did an A/B test with 5.2 and 5.5 before writing the email, and I also definitely traced it down to devm_gpiod_get() (ie: leave all the rest and take those two lines out in 5.5, and that made it not fail).

If I have well understood, in 5.5 you kept devm_gpiod_get() calls without the recovery stuff and it works. Isn't it?

Quoted 6 lines

Again, other i2c controllers are built the exact same way, with a handful of them using the same copied/pasted code. I suspect they are broken too (when gpio bus recovery is enabled). If they're not, then does it mean devm_gpiod_get() works differently on different CPUs?

devm_gpiod_get() calls gpio_request or gpio_request_enable ops which are implemented in the pin controller so the behavior can be different.

EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe

Quoted 202 lines · On 3/26/20 4:39 PM, [email protected] wrote:

Quoted 166 lines · Alan Ott

EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe

Quoted 162 lines · On 3/26/20 2:53 AM, [email protected] wrote:

Quoted 160 lines · Alan Ott

EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe

Quoted 145 lines · On 3/25/20 4:06 PM, [email protected] wrote:

Quoted 144 lines · Alan Ott

EXTERNAL EMAIL: Do not click links or open attachments unless you know
the content is safe

Quoted 102 lines · Alan Ott

Quoted 101 lines · Russell King - ARM Linux admin

Quoted 64 lines · Linus Walleij

Hi Russell,

very nice description of this dual-mode problem.

I wish I had a simple and elegant way we could make it unambiguous and simple to use ... but it beats me right now.

Quoted 10 lines · Russell King - ARM Linux admin

One may expect:

            pinctrl_select_state(i2c_imx->pinctrl, i2c_imx->pinctrl_pins_default);

to change them back to the default state, but that would be incorrect.
The first thing that pinctrl_select_state() does is check whether

Quoted 5 lines

            p->state == state

which it will do, as the pinctrl layer hasn't been informed of the
change that has happened behind its back at the pinmux level.

Some pin controllers have the .strict property set in their struct pinmux_ops:

* @strict: do not allow simultaneous use of the same pin for GPIO and
another
*      function. Check both gpio_owner and mux_owner strictly before
approving
*      the pin request.

The non-strict pin controllers are those that actually allow GPIO and device functions to be used on the same physical line at the same time. In this case there is not special GPIO mode for the line in some muxing registers, they are just physically connected somehow.

One usecase is sort of like how tcpdump work for ethernet interfaces: a GPIO register can "snoop" on a pin while in used by another device.

But it would notably also allow you to drive the line and interfere
with the device. Which is exactly what this I2C recovery mechanism
does, just that its pin controller is actually strict, will not allow
the same line to be used for GPIO and some other function at the same time, so I suppose i.MX should probably explore the strict mode.

Enabling that will sadly make the problem MORE complex for this I2C recovery, requiring a cycle of gpiod_put()/gpiod_get() to get it released from GPIO mode, i.e. we would need to just get the GPIO when this is strictly needed. Using devm_gpiod_get() and keeping a reference descriptor around would not work all of a sudden.

Quoted 4 lines

I am thinking whether we can handle the non-strict controllers in a more elegant way, or add some API to explicitly hand over between device function and GPIO function. But I can't really see some obvious solution.

What I'm currently trying is (error handling removed for brevity):

       struct i2c_bus_recovery_info *bri = &i2c->recovery;

            i2c->pinctrl = devm_pinctrl_get(dev);
            i2c->pinctrl_default =
pinctrl_lookup_state(i2c->pinctrl,

PINCTRL_STATE_DEFAULT);
            i2c->pinctrl_recovery = pinctrl_lookup_state(i2c->pinctrl, "recovery"); bri->sda_gpiod = devm_gpiod_get(dev, "sda", GPIOD_OUT_HIGH_OPEN_DRAIN);
            bri->scl_gpiod = devm_gpiod_get(dev, "scl", GPIOD_OUT_HIGH_OPEN_DRAIN);

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

which seems good enough to get the pins back into i2c mode after the
gpios are obtained.  Then we switch the pinctrl state between pinctrl_recovery and pinctrl_default as we have need to.

The problem is, the generic i2c bus recovery code wants the gpiod descriptors to be setup and inplace by the time i2c_init_recovery()
is called (which is called when the adapter is registered) so holding off until we need to do recovery doesn't work.

Quoted 11 lines

Quoted 11 lines

This seems to work for this SoC I'm currently working with, but I think there's more on the horizon - I'm having the same problems on another SoC which also needs bus recovery implemented, and as the problem device is behind an I2C bus mux, when it locks the I2C bus, it kills all I2C buses rooted at that particular SoC I2C controller.  However, there's a problem - the pinctrls for that SoC
are set by ROM firmware at boot time by reading a table from the boot media.  *Unprintables about firmware being too way limiting*. :p

    >

Quoted 24 lines

Hi all, what's the current state of this? I can confirm that this is
broken with the at91 i2c controller's recovery mode[1], which is implemented exactly the same as other i2c master recovery modes, so I
suspect them to be broken as well.

I'm using 5.5.6 with this patch applied (which adds the recovery):
        https://patchwork.kernel.org/cover/11333883/

It worked fine with 5.2, but has now broken, the way Russell describes,
in 5.5.6 and also on the latest 5.6-rc3. Russell's suggested workaround
of setting the pinctrl to recovery (gpio) and then back to default does
make it work.

Alan.

[1] currently the patch for i2c recovery for at91 is accepted to Wolfram
Sang's for-next tree.

Quoted 8 lines

Quoted 2 lines

Is there any word on this?

Internally we have managed it in the same way as the one suggested by Russell.

We wondered if we should mainline it or not as it's really tricky to proceed like this.

Certainly it needs to work in mainline though, right? Not just in the linux4sam vendor kernel?

Quoted 8 lines

Quoted 5 lines

It has been fixed two days ago. We'll send it and see if it will be accepted.

By the way, with which SoC have you encountered this issue? It is of intereset as we have two different pin controllers.

SAMA5D33

Ok, thanks.

Quoted 24 lines

Quoted 23 lines

Quoted 11 lines

Quoted 10 lines

In the future, we may declare our pinctrl as strict which should cause
another breakage... It's not done yet because when I tried to do it, maybe it has changed now, I was not able to apply the pin configuration
to the pin muxed as a gpio.

Quoted 7 lines

The larger question I think is, is this a breakage in gpio? i2c-at91 is
not the only i2c driver which uses gpio-based bus recovery, and many of
them use nearly the exact same code as i2c-at91. Are they all broken with this kernel update too?

I don't know what changed in gpio or pinctrl. Thinking more about it I'am surprised it had worked. In my mind, gpiod_get has always ended with a call to the gpio_request_enable operation so changing the mux to a gpio function.

Quoted 8 lines

Quoted 5 lines

I definitely did an A/B test with 5.2 and 5.5 before writing the email, and I also definitely traced it down to devm_gpiod_get() (ie: leave all the rest and take those two lines out in 5.5, and that made it not fail).

If I have well understood, in 5.5 you kept devm_gpiod_get() calls without the recovery stuff and it works. Isn't it?

No, it's what I said higher up in the email:

* 5.2 plus the recovery patch linked above does work.
* 5.5 plus the recovery patch linked above does not work[1].
* 5.5 plus the recovery patch linked above plus a "fix" similar to Russell's does work.

Quoted 8 lines

Quoted 7 lines

Again, other i2c controllers are built the exact same way, with a handful of them using the same copied/pasted code. I suspect they are broken too (when gpio bus recovery is enabled). If they're not, then does it mean devm_gpiod_get() works differently on different CPUs?

Quoted 3 lines

devm_gpiod_get() calls gpio_request or gpio_request_enable ops which are implemented in the pin controller so the behavior can be different.

Are you asserting that, based on the controller,:
* Sometimes gpiod_get() adjusts the pinmux, and
* Sometimes gpiod_get() doesn't adjust the pinmux?

Whatever it is, it appears that the behavior has changed for SAMA5D3 between 5.2 and 5.5, and it makes the patch set (linked above) that's currently in Wolfram's for-next tree no longer work properly.

Alan.

[1] And "doesn't work" means, nothing in the i2c controller works because the pinctrl is set to gpio rather than to the i2c controller.