Re: I2c GPIO Recovery with pinctrl strict mode

[Linus Walleij]

On Fri, Feb 17, 2023 at 6:36 PM <Ryan.Wanner@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx> 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