pinctrl states vs pinmux vs gpio (i2c bus recovery)

pinctrl states vs pinmux vs gpio (i2c bus recovery)

[Russell King - ARM Linux admin]

Hi Linus,

There seems to be a problem in pinctrl land when pinctrls interact with
GPIOs.

If, for example, an I2C driver wants to attempt bus recovery, and needs
to use GPIO mode to control the I2C bus pins to do this, then one may
expect code such as found in i2c_imx_init_recovery_info() to work.

However, consider that devm_gpiod_get() may invoke
pinctrl_gpio_request(), which changes the pinmux settings independently
of the pinctrl layer.

The result is that after devm_gpiod_get() has completed, the I2C bus
pins are in GPIO mode.

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.

The result is, one needs to switch the state to gpio mode, and then
back to default to ensure that the pins are muxed back to the I2C
controller while the I2C controller is supposed to be operational.

GPIO mode is only used to recover the bus when a bus hang has happened
due to a slave device holding SDA low - and the only way that can be
recovered is by toggling the SCL signal. If the controller provides no
means to do this other than switching to GPIO mode, the above is
necessary.

I spotted this while trying out this method of i2c bus recovery on a
different I2C controller.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

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

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

[Linus Walleij]

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.

Yours,
Linus Walleij

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

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

[Russell King - ARM Linux admin]

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

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

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

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

[Alan Ott]

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.


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

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

[Alan Ott]

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.

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

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

[Ludovic.Desroches]

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.

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.

Regards

Ludovic

> Alan.

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

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

[Alan Ott]

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

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

[Ludovic.Desroches]

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.

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

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.

Regards

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

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

[Alan Ott]

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

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

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?

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

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

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

[Ludovic.Desroches]

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.

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

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

[Alan Ott]

On 3/26/20 4:39 PM, Ludovic.Desroches@microchip.com wrote:
> 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?

No, it's what I said higher up in the email:

* 5.2 plus the recovery patch linked above does work.
* 5.5 plus the recovery patch linked above does not work[1].
* 5.5 plus the recovery patch linked above plus a "fix" similar to 
Russell's does work.

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

Are you asserting that, based on the controller,:
* Sometimes gpiod_get() adjusts the pinmux, and
* Sometimes gpiod_get() doesn't adjust the pinmux?

Whatever it is, it appears that the behavior has changed for SAMA5D3 
between 5.2 and 5.5, and it makes the patch set (linked above) that's 
currently in Wolfram's for-next tree no longer work properly.

Alan.

[1] And "doesn't work" means, nothing in the i2c controller works 
because the pinctrl is set to gpio rather than to the i2c controller.

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

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

[Ludovic.Desroches]

On 3/27/2020 5:24 PM, Alan Ott wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know 
> the content is safe
> 
> On 3/26/20 4:39 PM, Ludovic.Desroches@microchip.com wrote:
>> 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?
> 
> No, it's what I said higher up in the email:
> 
> * 5.2 plus the recovery patch linked above does work.
> * 5.5 plus the recovery patch linked above does not work[1].
> * 5.5 plus the recovery patch linked above plus a "fix" similar to
> Russell's does work.

Thanks, understood.

> 
>>
>>> 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.
>>
> 
> Are you asserting that, based on the controller,:
> * Sometimes gpiod_get() adjusts the pinmux, and
> * Sometimes gpiod_get() doesn't adjust the pinmux?
> 

Yes. On our side we have encountered it when implementing the 
gpio_request_enable() operation for the pinctrl-at91-pio4 driver 
(SAMA5D2 family). If it's not implemented, nothing happens at the pin 
controller level, but gpiod_get() assume that the pin can be used as a 
GPIO. Once implemented, the pin controller modifies the muxing of the 
pin and we ended in this situation where i2c no longer works. The 
workaround discussed before fix the problem by remuxing the pin to the 
device after gpiod_get().

> Whatever it is, it appears that the behavior has changed for SAMA5D3
> between 5.2 and 5.5, and it makes the patch set (linked above) that's
> currently in Wolfram's for-next tree no longer work properly.
> 

I tend to think that it should have be broken in 5.2. I2C controller 
should or the recovery mode should not work. I don't see how both can 
work. Whatever, as you said something changed but maybe the breakage 
comes from a fix!

Regards

Ludovic

> Alan.
> 
> [1] And "doesn't work" means, nothing in the i2c controller works
> because the pinctrl is set to gpio rather than to the i2c controller.

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