On 3/25/2020 10:09 PM, Alan Ott wrote:

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

On 3/25/20 4:06 PM, Ludovic.Desroches@microchip.com wrote:

quoted

On 3/25/2020 1:42 PM, Alan Ott wrote:

quoted

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

On 2/27/20 11:47 AM, Alan Ott wrote:

quoted

On 12/12/19 7:20 PM, Russell King - ARM Linux admin wrote:

quoted

On Mon, Dec 09, 2019 at 01:20:15AM +0100, Linus Walleij wrote:

quoted

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.

On Fri, Dec 6, 2019 at 6:33 PM Russell King - ARM Linux admin
[off-list ref] wrote:

quoted

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.

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

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.

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?

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

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?

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.

Regards

Ludovic