Re: pinctrl states vs pinmux vs gpio (i2c bus recovery)

[Alan Ott <alan@softiron.com>]

On 12/12/19 7:20 PM, Russell King - ARM Linux admin wrote:
> On Mon, Dec 09, 2019 at 01:20:15AM +0100, Linus Walleij wrote:
>> 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
>> <linux@armlinux.org.uk> wrote:
>>
>>> 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
>
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.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel