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

[Alan Ott <alan@softiron.com>]

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?

Alan.