From: <Ludovic.Desroches@microchip.com>
To: <alan@softiron.com>, <linux@armlinux.org.uk>, <linus.walleij@linaro.org>
Cc: <kamel.bouhara@bootlin.com>, <wsa@the-dreams.de>,
<linux-gpio@vger.kernel.org>, <Codrin.Ciubotariu@microchip.com>,
<linux-arm-kernel@lists.infradead.org>
Subject: Re: pinctrl states vs pinmux vs gpio (i2c bus recovery)
Date: Thu, 26 Mar 2020 20:39:41 +0000 [thread overview]
Message-ID: <4ad49369-ec70-4452-2149-85b877a1c371@microchip.com> (raw)
In-Reply-To: <ae952fa3-4b20-5571-875c-408408d7ecb1@softiron.com>
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@microchip.com 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@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?
>>
>> 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.
next prev parent reply other threads:[~2020-03-26 20:39 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-12-06 17:33 pinctrl states vs pinmux vs gpio (i2c bus recovery) 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:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4ad49369-ec70-4452-2149-85b877a1c371@microchip.com \
--to=ludovic.desroches@microchip.com \
--cc=Codrin.Ciubotariu@microchip.com \
--cc=alan@softiron.com \
--cc=kamel.bouhara@bootlin.com \
--cc=linus.walleij@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-gpio@vger.kernel.org \
--cc=linux@armlinux.org.uk \
--cc=wsa@the-dreams.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).