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

[<Ludovic.Desroches@xxxxxxxxxxxxx>]

On 3/26/2020 4:55 PM, Alan Ott wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know 
> the content is safe
> 
> On 3/26/20 2:53 AM, Ludovic.Desroches@xxxxxxxxxxxxx wrote:
>> 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@xxxxxxxxxxxxx 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@xxxxxxxxxxxxxxx> 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?
>>
>> 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.

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

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

Regards

Ludovic

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