Linux-GPIO Archive on
 help / color / Atom feed
From: <>
To: <>, <>, <>
Cc: <>, <>,
	<>, <>,
Subject: Re: pinctrl states vs pinmux vs gpio (i2c bus recovery)
Date: Thu, 26 Mar 2020 20:39:41 +0000
Message-ID: <> (raw)
In-Reply-To: <>

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, 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, 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
>>>>>>>> <> 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,
>>>>>>>            i2c->pinctrl_recovery = 
>>>>>>> pinctrl_lookup_state(i2c->pinctrl,
>>>>>>>                                "recovery");
>>>>>>>            bri->sda_gpiod = devm_gpiod_get(dev, "sda",
>>>>>>>            bri->scl_gpiod = devm_gpiod_get(dev, "scl",
>>>>>>>       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):
>>>>>> 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.



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

  reply index

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-12-06 17:33 Russell King - ARM Linux admin
2019-12-09  0:20 ` Linus Walleij
2019-12-13  0:20   ` Russell King - ARM Linux admin
2020-02-27 16:47     ` Alan Ott
2020-03-25 12:42       ` Alan Ott
2020-03-25 20:06         ` Ludovic.Desroches
2020-03-25 21:09           ` Alan Ott
2020-03-26  6:53             ` Ludovic.Desroches
2020-03-26 15:55               ` Alan Ott
2020-03-26 20:39                 ` Ludovic.Desroches [this message]
2020-03-27 16:24                   ` Alan Ott
2020-03-27 21:43                     ` Ludovic.Desroches

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-GPIO Archive on

Archives are clonable:
	git clone --mirror linux-gpio/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-gpio linux-gpio/ \
	public-inbox-index linux-gpio

Example config snippet for mirrors

Newsgroup available over NNTP:

AGPL code for this site: git clone