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

[Alan Ott <alan@softiron.com>]

On 3/25/20 4:06 PM, Ludovic.Desroches@microchip.com wrote:
> On 3/25/2020 1:42 PM, Alan Ott wrote:
>> 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:
>>> 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.
>>>
>>
>> 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?

> 
> 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?

Alan.

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