I2c GPIO Recovery with pinctrl strict mode

I2c GPIO Recovery with pinctrl strict mode

[Ryan.Wanner]

Hello,

I came across an issue working on i2c and pinctrl strict mode, with
at91-i2c and at91-pio4 pinctrl driver. I understand that for i2c
recovery the pins under i2c change states into gpio mode for the
recovery then back to their default state when recovery is complete.
With strict mode my goal is to change ownership from the default i2c
into gpio ownership then back to i2c ownership to continue normal
operation.

My main issue is the process of freeing ownership of a pin(s) having
another driver, in this case gpio, to take ownership then free that
ownership back to the default state, in this case it would be back to
i2c.

I have tried calling pinmux_disable_setting() and then claiming the
gpios then enabling them for recovery then disabling them again. This
causes lots of warnings and some cases the full ownership is not
transferred.

It seems that what I am attempting to achieve is not doable currently.
Is this the case or am I missing some extra things needing to prepare
for this action?

Thanks,
Ryan

Re: I2c GPIO Recovery with pinctrl strict mode

[Linus Walleij]

On Tue, Feb 7, 2023 at 6:56 PM <Ryan.Wanner@microchip.com> wrote:

> My main issue is the process of freeing ownership of a pin(s) having
> another driver, in this case gpio, to take ownership then free that
> ownership back to the default state, in this case it would be back to
> i2c.
>
> I have tried calling pinmux_disable_setting() and then claiming the
> gpios then enabling them for recovery then disabling them again. This
> causes lots of warnings and some cases the full ownership is not
> transferred.
>
> It seems that what I am attempting to achieve is not doable currently.
> Is this the case or am I missing some extra things needing to prepare
> for this action?

There are several other i2c bus drivers doing this already, for example
drivers/i2c/busses/i2c-imx.c

The idea is to have some different pinctrl states and move between
them explicitly in the driver to move pins from i2c mode to GPIO
mode and back.

The imx driver depend on the ability of the i.MX pin controller to use
the pins as a certain function and GPIO at the same time.

This is due to the imx pin controller not setting the .strict attribute
on the struct pinmux_ops so that pins can be used in parallel for
i2c and GPIO and gpiod_get() will not fail. But the Atmel driver does
not set this so you should be fine I think.

Yours,
Linus Walleij

Re: I2c GPIO Recovery with pinctrl strict mode

[Michael Walle]

>> My main issue is the process of freeing ownership of a pin(s) having
>> another driver, in this case gpio, to take ownership then free that
>> ownership back to the default state, in this case it would be back to
>> i2c.
>>
>> I have tried calling pinmux_disable_setting() and then claiming the
>> gpios then enabling them for recovery then disabling them again. This
>> causes lots of warnings and some cases the full ownership is not
>> transferred.
>>
>> It seems that what I am attempting to achieve is not doable currently.
>> Is this the case or am I missing some extra things needing to prepare
>> for this action?
>
> There are several other i2c bus drivers doing this already, for example
> drivers/i2c/busses/i2c-imx.c
> 
> The idea is to have some different pinctrl states and move between
> them explicitly in the driver to move pins from i2c mode to GPIO
> mode and back.
> 
> The imx driver depend on the ability of the i.MX pin controller to use
> the pins as a certain function and GPIO at the same time.

But that's because this is a limitation of the imx i2c controller.
Usually, if i2c controllers don't have a hardware bus recovery (which
is broken in most designs..) they usually have an override bit to
bit bang SDA and SCL manually. Do the microchip cores have such bits?

Fun fact: also the layerscape SoCs use the imx i2c cores. It's just that
layerscape SoCs doesn't support dynamic pinmuxing...

-michael

> This is due to the imx pin controller not setting the .strict attribute
> on the struct pinmux_ops so that pins can be used in parallel for
> i2c and GPIO and gpiod_get() will not fail. But the Atmel driver does
> not set this so you should be fine I think.

Re: I2c GPIO Recovery with pinctrl strict mode

[Ryan.Wanner]

On 2/9/23 03:32, Linus Walleij wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Tue, Feb 7, 2023 at 6:56 PM <Ryan.Wanner@microchip.com> wrote:
> 
>> My main issue is the process of freeing ownership of a pin(s) having
>> another driver, in this case gpio, to take ownership then free that
>> ownership back to the default state, in this case it would be back to
>> i2c.
>>
>> I have tried calling pinmux_disable_setting() and then claiming the
>> gpios then enabling them for recovery then disabling them again. This
>> causes lots of warnings and some cases the full ownership is not
>> transferred.
>>
>> It seems that what I am attempting to achieve is not doable currently.
>> Is this the case or am I missing some extra things needing to prepare
>> for this action?
> 
> There are several other i2c bus drivers doing this already, for example
> drivers/i2c/busses/i2c-imx.c
> 
> The idea is to have some different pinctrl states and move between
> them explicitly in the driver to move pins from i2c mode to GPIO
> mode and back.
> 
> The imx driver depend on the ability of the i.MX pin controller to use
> the pins as a certain function and GPIO at the same time.
> 
> This is due to the imx pin controller not setting the .strict attribute
> on the struct pinmux_ops so that pins can be used in parallel for
> i2c and GPIO and gpiod_get() will not fail. But the Atmel driver does
> not set this so you should be fine I think.
> 
I am trying to enable .strict in the Atmel pinctrl driver, and that is
what is causing my issues. I have tried a similar approach to the imx
driver but it seems I cannot transfer ownership cleanly.

Best,
Ryan

Re: I2c GPIO Recovery with pinctrl strict mode

[Linus Walleij]

On Fri, Feb 10, 2023 at 4:21 PM <Ryan.Wanner@microchip.com> wrote:

> I am trying to enable .strict in the Atmel pinctrl driver, and that is
> what is causing my issues.

Strictly speaking (ha!) that flag is for when you *cannot* use a pin
in GPIO mode at the same time as another mode.

Example: if you use the pin in I2C mode, then reading the GPIO
input register will *not* reflect the value on the electrical line,
because it has been decoupled physically. Then .strict should
be true.

The strict mode was not intended for policy, i.e. stopping kernel
developers from doing wrong things. They have enough tools to
do wrong things anyway, one more or less doesn't matter.

Yours,
Linus Walleij

Re: I2c GPIO Recovery with pinctrl strict mode

[Ryan.Wanner]

On 2/13/23 02:29, Linus Walleij wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Fri, Feb 10, 2023 at 4:21 PM <Ryan.Wanner@microchip.com> wrote:
> 
>> I am trying to enable .strict in the Atmel pinctrl driver, and that is
>> what is causing my issues.
> 
> Strictly speaking (ha!) that flag is for when you *cannot* use a pin
> in GPIO mode at the same time as another mode.
> 
> Example: if you use the pin in I2C mode, then reading the GPIO
> input register will *not* reflect the value on the electrical line,
> because it has been decoupled physically. Then .strict should
> be true.
> 
> The strict mode was not intended for policy, i.e. stopping kernel
> developers from doing wrong things. They have enough tools to
> do wrong things anyway, one more or less doesn't matter.

I understand, so .strict keeps the pins mapped to one ownership,
so if I2C has those pins GPIO could not have access to them.

When it comes to I2c recovery, looking at the I2C generic recovery,
the pins are both being used by the I2C and the GPIO at the same time.
This cannot happen with strict mode. So since I am enabling strict mode
for pinctrl-atmel-pio4.c I2C recovery cannot work?

Thanks,
Ryan
> 
> Yours,
> Linus Walleij

Re: I2c GPIO Recovery with pinctrl strict mode

[Linus Walleij]

On Fri, Feb 17, 2023 at 6:36 PM <Ryan.Wanner@microchip.com> wrote:
> On 2/13/23 02:29, Linus Walleij wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> >
> > On Fri, Feb 10, 2023 at 4:21 PM <Ryan.Wanner@microchip.com> wrote:
> >
> >> I am trying to enable .strict in the Atmel pinctrl driver, and that is
> >> what is causing my issues.
> >
> > Strictly speaking (ha!) that flag is for when you *cannot* use a pin
> > in GPIO mode at the same time as another mode.
> >
> > Example: if you use the pin in I2C mode, then reading the GPIO
> > input register will *not* reflect the value on the electrical line,
> > because it has been decoupled physically. Then .strict should
> > be true.
> >
> > The strict mode was not intended for policy, i.e. stopping kernel
> > developers from doing wrong things. They have enough tools to
> > do wrong things anyway, one more or less doesn't matter.
>
> I understand, so .strict keeps the pins mapped to one ownership,
> so if I2C has those pins GPIO could not have access to them.
>
> When it comes to I2c recovery, looking at the I2C generic recovery,
> the pins are both being used by the I2C and the GPIO at the same time.
> This cannot happen with strict mode. So since I am enabling strict mode
> for pinctrl-atmel-pio4.c I2C recovery cannot work?

I think it can, you just have to be more careful.

You can move the pins between different states. E.g. state
"A" and "B", so these states can be "GPIO mode" and "I2C mode".
In "GPIO mode" the pins are muxed to the GPIO block, and in
the "I2C mode" the pins are muxed to the I2C block.

Whether one or other of these modes in practice has an opaque
name like "default" or "init" (etc) is just confusing, the only reason
these named states exist is for convenience. It is perfectly legal
for a pin controller and driver to use states named "foo"
or "bar" and never use any state called "default".
(See further include/linux/pinctrl/pinctrl-state.h)

So what you want in your driver is something like:

struct pinctrl_state *gpio_state;
struct pinctrl_state *i2c_state;

(possibly more)

And before normal operations you issue:

pinctrl_select_state(p, i2c_state);

And before recovery you issue:

pinctrl_select_state(p, gpio_state);

... then you use GPIO abstractions to do the recovery
followed by

pinctrl_select_state(p, i2c_state);

To get back to normal operations.

This is the gist of it. The problem with using GPIO at the
same time as pin control is that some pin controllers
implement a "shortcut" which is the
struct pinmux_ops callbacks
.gpio_request_enable()
.gpio_disable_free()
.gpio_set_direction()

These callbacks will bypass the state selection and mux
pins directly as a byproduct of using gpiod_*() operations.

For example qualcomm does not implement these callbacks,
and all GPIO state setup is done explicitly for every single
GPIO pin. This is quite good, but also a bit tedious for the
common cases which is why the shortcuts exist.

If the pin controller has implemented these operations
you get a problem with recovery because the GPIO
calls may start to conflict with the state-selected muxing.

It can however be made to work also in that case as long
as you think things over, but order of semantics will come
into play: you probably need to get the GPIO *before*
doing pinctrl_select_state(p, i2c_state); the first time,
lest the gpio initialization will unselect the I2C state.

You probably also shouldn't mess with calling any
gpiod_direction_input/output in the recovery code.
Hopefully that can be avoided or replaced by more
explicit pin control states in that case.

This becomes a bit complex, but recovery is a bit
complex and out of the ordinary, so...

Yours,
Linus Walleij