pinctrl - detailed gpio-mode pinconf design

pinctrl - detailed gpio-mode pinconf design

[kevin.bracey at broadcom.com]

I'm working on converting an shmobile-based system that has a lot of custom GPIO/pin manipulation to use pure pinctrl.  I'm a bit stuck on some of the design details.

I'm going to have to extend the sh-pfc pinctrl driver a fair bit to support all the necessary functionality, but I'm not quite sure how some of it should look. I have a first-pass prototype, but I'm worried that the consumer API just isn't right.

Basically, our system has an existing mechanism to get pins into "safe" state for sleep. It uses the legacy SH "gpio_request(FN)" mechanism to switch pinmux between function and GPIO, and then sets GPIO direction through normal GPIO APIs, and pin pulls through a custom API.

Clearly, this can now be replaced with default+sleep pinctrl states. But I'm not seeing a clear model to follow in the upstream code at the level of detail I need.

The pins that we're dealing with basically look like this diagram from pinctrl.txt:

                       pin config
                       logic regs
                       |               +- SPI
     Physical pins --- pad --- pinmux -+- I2C
                               |       +- mmc
                               |       +- GPIO
                               pin
                               multiplex
                               logic regs

The control bits we have in the GPIO function are "input enable", "output enable" and "output data". Those manual controls are only effective when the GPIO mux is selected. The pad config bits are "pull up enable" and "pull down enable", and they're always manually controllable.

So adding a "gpio-mode" function to each pin group as per pinctrl.txt seems the obvious thing to do.

And we need to extend the sh-pfc driver to let us control the GPIO through pinconf once the "gpio-mode" function is selected. But how exactly should this work? The questions are really about the intent of generic pinconf, rather than anything platform-specific.

At present, the sh-pfc driver only supports BIAS_PULL_UP, BIAS_PULL_DOWN, and BIAS_DISABLE, which control only the pad pulls, and don't touch the GPIO bits.

Now, if specifying hi-Z/output for a "gpio-mode" pin, it seems that we should use generic pinconf BIAS_HIGH_IMPEDANCE and OUTPUT(0,1). Yes?

But then how do we describe a pull? It seems most natural for pinctrl consumers to express this as simply MUX("gpio-mode"), CONF(BIAS_PULL_UP).

So, should BIAS_PULL_UP automatically select "input" in gpio-mode? Or does the DT/board have to explicitly say MUX("gpio-mode"), CONF(BIAS_PULL_UP, BIAS_HIGH_IMPEDANCE) to select pull up and input? Or does that just make no sense - it certainly looks odd. What's the intent here in the generic pinconf design?

Looking at it another way - do we model the pinconf as these 2 sets of independent settings - you can always set 1 of the first 3, and you can also set 1 of the second 3 in gpio-mode:

  (BIAS_PULL_UP | BIAS_PULL_DOWN | BIAS_DISABLE)  = (pull=UP | pull=DOWN | pull=OFF)
  (BIAS_HIGH_IMPEDANCE | OUTPUT 0 | OUTPUT 1)     = (IEN=OEN=0 | IEN=0,OEN=1,DATA=0 | IEN=0,OEN=1,DATA=1 ) 

Or we treat it as one set where you can only ever enable 1:

   BIAS_PULL_UP                         pull=UP, IEN=OEN=0
   BIAS_PULL_DOWN                       pull=DOWN, IEN=OEN=0
   BIAS_HIGH_IMPEDANCE                  pull=OFF, IEN=OEN=0
   OUTPUT 0                             pull=OFF, IEN=0, OEN=1, DATA=0
   OUTPUT 1                             pull=OFF, IEN=0, OEN=1, DATA=1

(and I guess BIAS_DISABLE is equivalent then to BIAS_HIGH_IMPEDANCE in gpio-mode; with other functions selected, or a real GPIO request, you would only be able to modify the pulls with pinconf, as with the current driver).

And once you have the answer to that, what about glitch-free handover? What is the canonical ordering for MUX+CONF in state tables/DT? If MUX comes first, should the driver defer the actual GPIO mux switch until the CONF specification, to avoid glitches? (The driver already applies this logic internally for GPIO API requests). Or do we require consumers to specify the CONF /before/ the MUX to get a glitch-free transition? That seems a bit odd, as they'd be requesting a pin config that wouldn't (yet) take effect, ruling out some error-checking.

Any guidance appreciated,

Kevin

Re: pinctrl - detailed gpio-mode pinconf design

[Linus Walleij]

On Thu, Nov 28, 2013 at 11:07 AM,  <kevin.bracey@broadcom.com> wrote:

> I'm working on converting an shmobile-based system that has a lot of custom GPIO/pin manipulation to use
> pure pinctrl.  I'm a bit stuck on some of the design details.

OK I have forwarded this post to Laurent Pinchart who wrote the
shmobile pin control implementation
so we can reach some clarity on how this is supposed to work.

> I'm going to have to extend the sh-pfc pinctrl driver a fair bit to support all the necessary
> functionality, but I'm not quite sure how some of it should look. I have a first-pass prototype,
> but I'm worried that the consumer API just isn't right.

Let's figure this out.

> Basically, our system has an existing mechanism to get pins into "safe" state for sleep.
> It uses the legacy SH "gpio_request(FN)" mechanism to switch pinmux between function
> and GPIO, and then sets GPIO direction through normal GPIO APIs, and pin pulls
> through a custom API.

I cannot really say very much about the legacy stuff as I have no clue how that
works. Paul Mundt and Magnus Damm may know.

> Clearly, this can now be replaced with default+sleep pinctrl states. But I'm not seeing
> a clear model to follow in the upstream code at the level of detail I need.

So this is what we need to discuss with Laurent.

> The pins that we're dealing with basically look like this diagram from pinctrl.txt:
>
>                        pin config
>                        logic regs
>                        |               +- SPI
>      Physical pins --- pad --- pinmux -+- I2C
>                                |       +- mmc
>                                |       +- GPIO
>                                pin
>                                multiplex
>                                logic regs
>
> The control bits we have in the GPIO function are "input enable", "output enable"
> and "output data". Those manual controls are only effective when the GPIO mux
> is selected. The pad config bits are "pull up enable" and "pull down enable",
> and they're always manually controllable.

"Manually" is a very strange word that we use from time to time with a
very fuzzy semantic meaning.

I start to think about Charlie Chaplin in the factory in "modern times"
when I hear this word.

I guess what you mean is that pulls can always be controlled, no matter
if the GPIO block is in use or not, so it corresponds to the diagram above.

> So adding a "gpio-mode" function to each pin group as per pinctrl.txt
> seems the obvious thing to do.

This is *one* way to do it that some implementers have opted for.
The other way is to implement the .gpio_request_enable(),
.gpio_disable_free() functions in struct pinmux_ops so as to
shortcut and simplify matters.

Either way works.

> And we need to extend the sh-pfc driver to let us control the GPIO
> through pinconf once the "gpio-mode" function is selected. But how
> exactly should this work?

Controlling GPIO through pinconf seems to be very twisted.
Usually it is the other way around.

When  a GPIO driver is using pin control as back-end it calls
specific helpers from <linux/pinctrl/consumer.h>:

pinctrl_request_gpio()
pinctrl_free_gpio()
pinctrl_gpio_direction_input()
pinctrl_gpio_direction_output()

> The questions are really about the intent of generic pinconf, rather
> than anything platform-specific.
>
> At present, the sh-pfc driver only supports BIAS_PULL_UP,
> BIAS_PULL_DOWN, and BIAS_DISABLE, which control only the
> pad pulls, and don't touch the GPIO bits.
>
> Now, if specifying hi-Z/output for a "gpio-mode" pin, it seems that
> we should use generic pinconf BIAS_HIGH_IMPEDANCE and
> OUTPUT(0,1). Yes?
>
> But then how do we describe a pull? It seems most natural for
> pinctrl consumers to express this as simply MUX("gpio-mode"),
> CONF(BIAS_PULL_UP).
>
> So, should BIAS_PULL_UP automatically select "input" in
> gpio-mode? Or does the DT/board have to explicitly say
> MUX("gpio-mode"), CONF(BIAS_PULL_UP, BIAS_HIGH_IMPEDANCE)
> to select pull up and input? Or does that just make no sense - it
> certainly looks odd. What's the intent here in the generic pinconf design?

Nominally the pin config and GPIO portions are treated ortogonally,
and the four calls above take care of the interaction between the
two systems.

Note that it is perfectly legal for a pin to be used by
the pin control and GPIO subsystems simultaneously:
e.g. a pin control hog can set up a bias and then the
GPIO subsystem can drive that pin without any
cross-talk.

> Looking at it another way - do we model the pinconf as these
> 2 sets of independent settings - you can always set 1 of the
> first 3, and you can also set 1 of the second 3 in gpio-mode:
>
>   (BIAS_PULL_UP | BIAS_PULL_DOWN | BIAS_DISABLE)  = (pull=UP | pull=DOWN | pull=OFF)
>   (BIAS_HIGH_IMPEDANCE | OUTPUT 0 | OUTPUT 1)     = (IEN=OEN=0 | IEN=0,OEN=1,DATA=0 | IEN=0,OEN=1,DATA=1 )

Yes this makes perfect sense. I would sat the first (bias)
parameter should be handled by the pin control subsystem.

The latter setting should be controlled by the GPIO subsystem
only, right? Because BIAS_HIGH_IMPEDANCE is usually
the same as input. Maybe you want this to happen inside
of the pin control driver as a result of the GPIO driver
calling pinctrl_gpio_direction_input() or
pinctrl_gpio_direction_output(), that is up to the
implementation but it is one way to do it, Laurent can
comment on this I guess.

> Or we treat it as one set where you can only ever enable 1:
>
>    BIAS_PULL_UP                         pull=UP, IEN=OEN=0
>    BIAS_PULL_DOWN                       pull=DOWN, IEN=OEN=0
>    BIAS_HIGH_IMPEDANCE                  pull=OFF, IEN=OEN=0
>    OUTPUT 0                             pull=OFF, IEN=0, OEN=1, DATA=0
>    OUTPUT 1                             pull=OFF, IEN=0, OEN=1, DATA=1
>
> (and I guess BIAS_DISABLE is equivalent then to BIAS_HIGH_IMPEDANCE
> in gpio-mode; with other functions selected, or a real GPIO request, you
> would only be able to modify the pulls with pinconf, as with the current driver).

This looks messy. The intention with the pin config subsystem
is to break things apart and make them understandable that
way, and to handle electronic stuff in pin config whereas driving
a line high/low happens in the GPIO subsystem.

It's the divide-and-conquer design pattern if you like.

> And once you have the answer to that, what about glitch-free handover?

Usually this is a matter for the device driver, for example you can
find interesting glitch workarounds in drivers/pinctrl-nomadik.c.

If you need help from the subsystem to do this let's see what is
needed.

> What is the canonical ordering for MUX+CONF in state tables/DT?
> If MUX comes first, should the driver defer the actual GPIO mux
> switch until the CONF specification, to avoid glitches?

This must be some misunderstanding. DT does not specify
application/ordering semantics. It does not define sequences at
all. The device driver parsing out whatever is in the device tree
has to take care of ordering stuff and driving the hardware.

On all of these problems we need Laurent's input I think.

Yours,
Linus Walleij

pinctrl - detailed gpio-mode pinconf design

[Linus Walleij]

On Thu, Nov 28, 2013 at 11:07 AM,  <kevin.bracey@broadcom.com> wrote:

> I'm working on converting an shmobile-based system that has a lot of custom GPIO/pin manipulation to use
> pure pinctrl.  I'm a bit stuck on some of the design details.

OK I have forwarded this post to Laurent Pinchart who wrote the
shmobile pin control implementation
so we can reach some clarity on how this is supposed to work.

> I'm going to have to extend the sh-pfc pinctrl driver a fair bit to support all the necessary
> functionality, but I'm not quite sure how some of it should look. I have a first-pass prototype,
> but I'm worried that the consumer API just isn't right.

Let's figure this out.

> Basically, our system has an existing mechanism to get pins into "safe" state for sleep.
> It uses the legacy SH "gpio_request(FN)" mechanism to switch pinmux between function
> and GPIO, and then sets GPIO direction through normal GPIO APIs, and pin pulls
> through a custom API.

I cannot really say very much about the legacy stuff as I have no clue how that
works. Paul Mundt and Magnus Damm may know.

> Clearly, this can now be replaced with default+sleep pinctrl states. But I'm not seeing
> a clear model to follow in the upstream code at the level of detail I need.

So this is what we need to discuss with Laurent.

> The pins that we're dealing with basically look like this diagram from pinctrl.txt:
>
>                        pin config
>                        logic regs
>                        |               +- SPI
>      Physical pins --- pad --- pinmux -+- I2C
>                                |       +- mmc
>                                |       +- GPIO
>                                pin
>                                multiplex
>                                logic regs
>
> The control bits we have in the GPIO function are "input enable", "output enable"
> and "output data". Those manual controls are only effective when the GPIO mux
> is selected. The pad config bits are "pull up enable" and "pull down enable",
> and they're always manually controllable.

"Manually" is a very strange word that we use from time to time with a
very fuzzy semantic meaning.

I start to think about Charlie Chaplin in the factory in "modern times"
when I hear this word.

I guess what you mean is that pulls can always be controlled, no matter
if the GPIO block is in use or not, so it corresponds to the diagram above.

> So adding a "gpio-mode" function to each pin group as per pinctrl.txt
> seems the obvious thing to do.

This is *one* way to do it that some implementers have opted for.
The other way is to implement the .gpio_request_enable(),
.gpio_disable_free() functions in struct pinmux_ops so as to
shortcut and simplify matters.

Either way works.

> And we need to extend the sh-pfc driver to let us control the GPIO
> through pinconf once the "gpio-mode" function is selected. But how
> exactly should this work?

Controlling GPIO through pinconf seems to be very twisted.
Usually it is the other way around.

When  a GPIO driver is using pin control as back-end it calls
specific helpers from <linux/pinctrl/consumer.h>:

pinctrl_request_gpio()
pinctrl_free_gpio()
pinctrl_gpio_direction_input()
pinctrl_gpio_direction_output()

> The questions are really about the intent of generic pinconf, rather
> than anything platform-specific.
>
> At present, the sh-pfc driver only supports BIAS_PULL_UP,
> BIAS_PULL_DOWN, and BIAS_DISABLE, which control only the
> pad pulls, and don't touch the GPIO bits.
>
> Now, if specifying hi-Z/output for a "gpio-mode" pin, it seems that
> we should use generic pinconf BIAS_HIGH_IMPEDANCE and
> OUTPUT(0,1). Yes?
>
> But then how do we describe a pull? It seems most natural for
> pinctrl consumers to express this as simply MUX("gpio-mode"),
> CONF(BIAS_PULL_UP).
>
> So, should BIAS_PULL_UP automatically select "input" in
> gpio-mode? Or does the DT/board have to explicitly say
> MUX("gpio-mode"), CONF(BIAS_PULL_UP, BIAS_HIGH_IMPEDANCE)
> to select pull up and input? Or does that just make no sense - it
> certainly looks odd. What's the intent here in the generic pinconf design?

Nominally the pin config and GPIO portions are treated ortogonally,
and the four calls above take care of the interaction between the
two systems.

Note that it is perfectly legal for a pin to be used by
the pin control and GPIO subsystems simultaneously:
e.g. a pin control hog can set up a bias and then the
GPIO subsystem can drive that pin without any
cross-talk.

> Looking at it another way - do we model the pinconf as these
> 2 sets of independent settings - you can always set 1 of the
> first 3, and you can also set 1 of the second 3 in gpio-mode:
>
>   (BIAS_PULL_UP | BIAS_PULL_DOWN | BIAS_DISABLE)  = (pull=UP | pull=DOWN | pull=OFF)
>   (BIAS_HIGH_IMPEDANCE | OUTPUT 0 | OUTPUT 1)     = (IEN=OEN=0 | IEN=0,OEN=1,DATA=0 | IEN=0,OEN=1,DATA=1 )

Yes this makes perfect sense. I would sat the first (bias)
parameter should be handled by the pin control subsystem.

The latter setting should be controlled by the GPIO subsystem
only, right? Because BIAS_HIGH_IMPEDANCE is usually
the same as input. Maybe you want this to happen inside
of the pin control driver as a result of the GPIO driver
calling pinctrl_gpio_direction_input() or
pinctrl_gpio_direction_output(), that is up to the
implementation but it is one way to do it, Laurent can
comment on this I guess.

> Or we treat it as one set where you can only ever enable 1:
>
>    BIAS_PULL_UP                         pull=UP, IEN=OEN=0
>    BIAS_PULL_DOWN                       pull=DOWN, IEN=OEN=0
>    BIAS_HIGH_IMPEDANCE                  pull=OFF, IEN=OEN=0
>    OUTPUT 0                             pull=OFF, IEN=0, OEN=1, DATA=0
>    OUTPUT 1                             pull=OFF, IEN=0, OEN=1, DATA=1
>
> (and I guess BIAS_DISABLE is equivalent then to BIAS_HIGH_IMPEDANCE
> in gpio-mode; with other functions selected, or a real GPIO request, you
> would only be able to modify the pulls with pinconf, as with the current driver).

This looks messy. The intention with the pin config subsystem
is to break things apart and make them understandable that
way, and to handle electronic stuff in pin config whereas driving
a line high/low happens in the GPIO subsystem.

It's the divide-and-conquer design pattern if you like.

> And once you have the answer to that, what about glitch-free handover?

Usually this is a matter for the device driver, for example you can
find interesting glitch workarounds in drivers/pinctrl-nomadik.c.

If you need help from the subsystem to do this let's see what is
needed.

> What is the canonical ordering for MUX+CONF in state tables/DT?
> If MUX comes first, should the driver defer the actual GPIO mux
> switch until the CONF specification, to avoid glitches?

This must be some misunderstanding. DT does not specify
application/ordering semantics. It does not define sequences at
all. The device driver parsing out whatever is in the device tree
has to take care of ordering stuff and driving the hardware.

On all of these problems we need Laurent's input I think.

Yours,
Linus Walleij

Re: pinctrl - detailed gpio-mode pinconf design

[Laurent Pinchart]

Hi Linus and Kevin,

On Tuesday 03 December 2013 11:04:48 Linus Walleij wrote:
> On Thu, Nov 28, 2013 at 11:07 AM,  <kevin.bracey@broadcom.com> wrote:
> > I'm working on converting an shmobile-based system that has a lot of
> > custom GPIO/pin manipulation to use pure pinctrl.  I'm a bit stuck on
> > some of the design details.

Kevin, what SoC do you use ? The GPIO+pinmux hardware architecture varies 
between models (with a single driver handling both GPIO and pinmux versus two 
separate drivers), I'd like to know the exact target before replying to your 
questions below.

> OK I have forwarded this post to Laurent Pinchart who wrote the shmobile pin
> control implementation so we can reach some clarity on how this is supposed
> to work.

By the way, Kevin, feel free to CC linux-sh@vger.kernel.org on Renesas-related 
topics.

> > I'm going to have to extend the sh-pfc pinctrl driver a fair bit to
> > support all the necessary functionality, but I'm not quite sure how some
> > of it should look. I have a first-pass prototype, but I'm worried that
> > the consumer API just isn't right.
> 
> Let's figure this out.
> 
> > Basically, our system has an existing mechanism to get pins into "safe"
> > state for sleep. It uses the legacy SH "gpio_request(FN)" mechanism to
> > switch pinmux between function and GPIO, and then sets GPIO direction
> > through normal GPIO APIs, and pin pulls through a custom API.
> 
> I cannot really say very much about the legacy stuff as I have no clue how
> that works. Paul Mundt and Magnus Damm may know.

The legacy API used fake GPIOs for pinmux and pinconf. We don't really need to 
care about it, but we need to ensure that that pinctrl API covers all the use 
cases.

> > Clearly, this can now be replaced with default+sleep pinctrl states. But
> > I'm not seeing a clear model to follow in the upstream code at the level
> > of detail I need.
>
> So this is what we need to discuss with Laurent.

I'll reply to the rest of the e-mail as soon as Kevin gives me the SoC model 
he's using.

> > The pins that we're dealing with basically look like this diagram from
> > pinctrl.txt:
> >                        pin config
> >                        logic regs
> >                        |               +- SPI
> >      Physical pins --- pad --- pinmux -+- I2C
> >                                |       +- mmc
> >                                |       +- GPIO
> >                                pin
> >                                multiplex
> >                                logic regs
> > 
> > The control bits we have in the GPIO function are "input enable", "output
> > enable" and "output data". Those manual controls are only effective when
> > the GPIO mux is selected. The pad config bits are "pull up enable" and
> > "pull down enable", and they're always manually controllable.
> 
> "Manually" is a very strange word that we use from time to time with a
> very fuzzy semantic meaning.
> 
> I start to think about Charlie Chaplin in the factory in "modern times"
> when I hear this word.
> 
> I guess what you mean is that pulls can always be controlled, no matter
> if the GPIO block is in use or not, so it corresponds to the diagram above.
> 
> > So adding a "gpio-mode" function to each pin group as per pinctrl.txt
> > seems the obvious thing to do.
> 
> This is *one* way to do it that some implementers have opted for. The other
> way is to implement the .gpio_request_enable(), .gpio_disable_free()
> functions in struct pinmux_ops so as to shortcut and simplify matters.
> 
> Either way works.
> 
> > And we need to extend the sh-pfc driver to let us control the GPIO
> > through pinconf once the "gpio-mode" function is selected. But how
> > exactly should this work?
> 
> Controlling GPIO through pinconf seems to be very twisted.
> Usually it is the other way around.
> 
> When  a GPIO driver is using pin control as back-end it calls specific
> helpers from <linux/pinctrl/consumer.h>:
> 
> pinctrl_request_gpio()
> pinctrl_free_gpio()
> pinctrl_gpio_direction_input()
> pinctrl_gpio_direction_output()
> 
> > The questions are really about the intent of generic pinconf, rather
> > than anything platform-specific.
> > 
> > At present, the sh-pfc driver only supports BIAS_PULL_UP,
> > BIAS_PULL_DOWN, and BIAS_DISABLE, which control only the
> > pad pulls, and don't touch the GPIO bits.
> > 
> > Now, if specifying hi-Z/output for a "gpio-mode" pin, it seems that
> > we should use generic pinconf BIAS_HIGH_IMPEDANCE and
> > OUTPUT(0,1). Yes?
> > 
> > But then how do we describe a pull? It seems most natural for
> > pinctrl consumers to express this as simply MUX("gpio-mode"),
> > CONF(BIAS_PULL_UP).
> > 
> > So, should BIAS_PULL_UP automatically select "input" in
> > gpio-mode? Or does the DT/board have to explicitly say
> > MUX("gpio-mode"), CONF(BIAS_PULL_UP, BIAS_HIGH_IMPEDANCE)
> > to select pull up and input? Or does that just make no sense - it
> > certainly looks odd. What's the intent here in the generic pinconf design?
> 
> Nominally the pin config and GPIO portions are treated ortogonally, and the
> four calls above take care of the interaction between the two systems.
> 
> Note that it is perfectly legal for a pin to be used by the pin control and
> GPIO subsystems simultaneously: e.g. a pin control hog can set up a bias and
> then the GPIO subsystem can drive that pin without any cross-talk.
> 
> > Looking at it another way - do we model the pinconf as these
> > 2 sets of independent settings - you can always set 1 of the
> > 
> > first 3, and you can also set 1 of the second 3 in gpio-mode:
> >   (BIAS_PULL_UP | BIAS_PULL_DOWN | BIAS_DISABLE)  = (pull=UP | pull=DOWN |
> >   pull=OFF)
> >   (BIAS_HIGH_IMPEDANCE | OUTPUT 0 | OUTPUT 1)     = (IEN=OEN=0 |
> >   IEN=0,OEN=1,DATA=0 | IEN=0,OEN=1,DATA=1 )
>
> Yes this makes perfect sense. I would sat the first (bias) parameter should
> be handled by the pin control subsystem.
> 
> The latter setting should be controlled by the GPIO subsystem only, right?
> Because BIAS_HIGH_IMPEDANCE is usually the same as input. Maybe you want
> this to happen inside of the pin control driver as a result of the GPIO
> driver calling pinctrl_gpio_direction_input() or
> pinctrl_gpio_direction_output(), that is up to the implementation but it is
> one way to do it, Laurent can comment on this I guess.
> 
> > Or we treat it as one set where you can only ever enable 1:
> >    BIAS_PULL_UP                         pull=UP, IEN=OEN=0
> >    BIAS_PULL_DOWN                       pull=DOWN, IEN=OEN=0
> >    BIAS_HIGH_IMPEDANCE                  pull=OFF, IEN=OEN=0
> >    OUTPUT 0                             pull=OFF, IEN=0, OEN=1, DATA=0
> >    OUTPUT 1                             pull=OFF, IEN=0, OEN=1, DATA=1
> > 
> > (and I guess BIAS_DISABLE is equivalent then to BIAS_HIGH_IMPEDANCE
> > in gpio-mode; with other functions selected, or a real GPIO request, you
> > would only be able to modify the pulls with pinconf, as with the current
> > driver).
>
> This looks messy. The intention with the pin config subsystem is to break
> things apart and make them understandable that way, and to handle electronic
> stuff in pin config whereas driving a line high/low happens in the GPIO
> subsystem.
> 
> It's the divide-and-conquer design pattern if you like.
> 
> > And once you have the answer to that, what about glitch-free handover?
> 
> Usually this is a matter for the device driver, for example you can find
> interesting glitch workarounds in drivers/pinctrl-nomadik.c.
> 
> If you need help from the subsystem to do this let's see what is needed.
> 
> > What is the canonical ordering for MUX+CONF in state tables/DT?
> > If MUX comes first, should the driver defer the actual GPIO mux
> > switch until the CONF specification, to avoid glitches?
> 
> This must be some misunderstanding. DT does not specify application/ordering
> semantics. It does not define sequences at all. The device driver parsing
> out whatever is in the device tree has to take care of ordering stuff and
> driving the hardware.
> 
> On all of these problems we need Laurent's input I think.

-- 
Regards,

Laurent Pinchart

pinctrl - detailed gpio-mode pinconf design

[Laurent Pinchart]

Hi Linus and Kevin,

On Tuesday 03 December 2013 11:04:48 Linus Walleij wrote:
> On Thu, Nov 28, 2013 at 11:07 AM,  <kevin.bracey@broadcom.com> wrote:
> > I'm working on converting an shmobile-based system that has a lot of
> > custom GPIO/pin manipulation to use pure pinctrl.  I'm a bit stuck on
> > some of the design details.

Kevin, what SoC do you use ? The GPIO+pinmux hardware architecture varies 
between models (with a single driver handling both GPIO and pinmux versus two 
separate drivers), I'd like to know the exact target before replying to your 
questions below.

> OK I have forwarded this post to Laurent Pinchart who wrote the shmobile pin
> control implementation so we can reach some clarity on how this is supposed
> to work.

By the way, Kevin, feel free to CC linux-sh at vger.kernel.org on Renesas-related 
topics.

> > I'm going to have to extend the sh-pfc pinctrl driver a fair bit to
> > support all the necessary functionality, but I'm not quite sure how some
> > of it should look. I have a first-pass prototype, but I'm worried that
> > the consumer API just isn't right.
> 
> Let's figure this out.
> 
> > Basically, our system has an existing mechanism to get pins into "safe"
> > state for sleep. It uses the legacy SH "gpio_request(FN)" mechanism to
> > switch pinmux between function and GPIO, and then sets GPIO direction
> > through normal GPIO APIs, and pin pulls through a custom API.
> 
> I cannot really say very much about the legacy stuff as I have no clue how
> that works. Paul Mundt and Magnus Damm may know.

The legacy API used fake GPIOs for pinmux and pinconf. We don't really need to 
care about it, but we need to ensure that that pinctrl API covers all the use 
cases.

> > Clearly, this can now be replaced with default+sleep pinctrl states. But
> > I'm not seeing a clear model to follow in the upstream code at the level
> > of detail I need.
>
> So this is what we need to discuss with Laurent.

I'll reply to the rest of the e-mail as soon as Kevin gives me the SoC model 
he's using.

> > The pins that we're dealing with basically look like this diagram from
> > pinctrl.txt:
> >                        pin config
> >                        logic regs
> >                        |               +- SPI
> >      Physical pins --- pad --- pinmux -+- I2C
> >                                |       +- mmc
> >                                |       +- GPIO
> >                                pin
> >                                multiplex
> >                                logic regs
> > 
> > The control bits we have in the GPIO function are "input enable", "output
> > enable" and "output data". Those manual controls are only effective when
> > the GPIO mux is selected. The pad config bits are "pull up enable" and
> > "pull down enable", and they're always manually controllable.
> 
> "Manually" is a very strange word that we use from time to time with a
> very fuzzy semantic meaning.
> 
> I start to think about Charlie Chaplin in the factory in "modern times"
> when I hear this word.
> 
> I guess what you mean is that pulls can always be controlled, no matter
> if the GPIO block is in use or not, so it corresponds to the diagram above.
> 
> > So adding a "gpio-mode" function to each pin group as per pinctrl.txt
> > seems the obvious thing to do.
> 
> This is *one* way to do it that some implementers have opted for. The other
> way is to implement the .gpio_request_enable(), .gpio_disable_free()
> functions in struct pinmux_ops so as to shortcut and simplify matters.
> 
> Either way works.
> 
> > And we need to extend the sh-pfc driver to let us control the GPIO
> > through pinconf once the "gpio-mode" function is selected. But how
> > exactly should this work?
> 
> Controlling GPIO through pinconf seems to be very twisted.
> Usually it is the other way around.
> 
> When  a GPIO driver is using pin control as back-end it calls specific
> helpers from <linux/pinctrl/consumer.h>:
> 
> pinctrl_request_gpio()
> pinctrl_free_gpio()
> pinctrl_gpio_direction_input()
> pinctrl_gpio_direction_output()
> 
> > The questions are really about the intent of generic pinconf, rather
> > than anything platform-specific.
> > 
> > At present, the sh-pfc driver only supports BIAS_PULL_UP,
> > BIAS_PULL_DOWN, and BIAS_DISABLE, which control only the
> > pad pulls, and don't touch the GPIO bits.
> > 
> > Now, if specifying hi-Z/output for a "gpio-mode" pin, it seems that
> > we should use generic pinconf BIAS_HIGH_IMPEDANCE and
> > OUTPUT(0,1). Yes?
> > 
> > But then how do we describe a pull? It seems most natural for
> > pinctrl consumers to express this as simply MUX("gpio-mode"),
> > CONF(BIAS_PULL_UP).
> > 
> > So, should BIAS_PULL_UP automatically select "input" in
> > gpio-mode? Or does the DT/board have to explicitly say
> > MUX("gpio-mode"), CONF(BIAS_PULL_UP, BIAS_HIGH_IMPEDANCE)
> > to select pull up and input? Or does that just make no sense - it
> > certainly looks odd. What's the intent here in the generic pinconf design?
> 
> Nominally the pin config and GPIO portions are treated ortogonally, and the
> four calls above take care of the interaction between the two systems.
> 
> Note that it is perfectly legal for a pin to be used by the pin control and
> GPIO subsystems simultaneously: e.g. a pin control hog can set up a bias and
> then the GPIO subsystem can drive that pin without any cross-talk.
> 
> > Looking at it another way - do we model the pinconf as these
> > 2 sets of independent settings - you can always set 1 of the
> > 
> > first 3, and you can also set 1 of the second 3 in gpio-mode:
> >   (BIAS_PULL_UP | BIAS_PULL_DOWN | BIAS_DISABLE)  = (pull=UP | pull=DOWN |
> >   pull=OFF)
> >   (BIAS_HIGH_IMPEDANCE | OUTPUT 0 | OUTPUT 1)     = (IEN=OEN=0 |
> >   IEN=0,OEN=1,DATA=0 | IEN=0,OEN=1,DATA=1 )
>
> Yes this makes perfect sense. I would sat the first (bias) parameter should
> be handled by the pin control subsystem.
> 
> The latter setting should be controlled by the GPIO subsystem only, right?
> Because BIAS_HIGH_IMPEDANCE is usually the same as input. Maybe you want
> this to happen inside of the pin control driver as a result of the GPIO
> driver calling pinctrl_gpio_direction_input() or
> pinctrl_gpio_direction_output(), that is up to the implementation but it is
> one way to do it, Laurent can comment on this I guess.
> 
> > Or we treat it as one set where you can only ever enable 1:
> >    BIAS_PULL_UP                         pull=UP, IEN=OEN=0
> >    BIAS_PULL_DOWN                       pull=DOWN, IEN=OEN=0
> >    BIAS_HIGH_IMPEDANCE                  pull=OFF, IEN=OEN=0
> >    OUTPUT 0                             pull=OFF, IEN=0, OEN=1, DATA=0
> >    OUTPUT 1                             pull=OFF, IEN=0, OEN=1, DATA=1
> > 
> > (and I guess BIAS_DISABLE is equivalent then to BIAS_HIGH_IMPEDANCE
> > in gpio-mode; with other functions selected, or a real GPIO request, you
> > would only be able to modify the pulls with pinconf, as with the current
> > driver).
>
> This looks messy. The intention with the pin config subsystem is to break
> things apart and make them understandable that way, and to handle electronic
> stuff in pin config whereas driving a line high/low happens in the GPIO
> subsystem.
> 
> It's the divide-and-conquer design pattern if you like.
> 
> > And once you have the answer to that, what about glitch-free handover?
> 
> Usually this is a matter for the device driver, for example you can find
> interesting glitch workarounds in drivers/pinctrl-nomadik.c.
> 
> If you need help from the subsystem to do this let's see what is needed.
> 
> > What is the canonical ordering for MUX+CONF in state tables/DT?
> > If MUX comes first, should the driver defer the actual GPIO mux
> > switch until the CONF specification, to avoid glitches?
> 
> This must be some misunderstanding. DT does not specify application/ordering
> semantics. It does not define sequences at all. The device driver parsing
> out whatever is in the device tree has to take care of ordering stuff and
> driving the hardware.
> 
> On all of these problems we need Laurent's input I think.

-- 
Regards,

Laurent Pinchart

RE: pinctrl - detailed gpio-mode pinconf design

[kevin.bracey]

Hi Linus & Laurent,

Thanks for taking the time to think about this.

Laurent Pinchart wrote:
> On Tuesday 03 December 2013 11:04:48 Linus Walleij wrote:
> > On Thu, Nov 28, 2013 at 11:07 AM,  <kevin.bracey@broadcom.com> wrote:
> > > I'm working on converting an shmobile-based system that has a lot of 
> > > custom GPIO/pin manipulation to use pure pinctrl.  I'm a bit stuck 
> > > on some of the design details.

> Kevin, what SoC do you use ? The GPIO+pinmux hardware architecture varies
> between models (with a single driver handling both GPIO and pinmux versus
> two separate drivers), I'd like to know the exact target before replying
> to your questions below.

I'm working on the non-upstreamed r8a7373. The closest relative in the PFC area is the r8a73a4, so pfc-r8a7373 is primarily modelled on that, and I'm trying to devise extensions that could be upstreamed in the form of r8a73a4 patches, even if we don't get the r8a7373 itself upstreamed. I do have access to an APE6EVM for testing that if necessary, but I'm not currently working on it.

> > OK I have forwarded this post to Laurent Pinchart who wrote the 
> > shmobile pin control implementation so we can reach some clarity on 
> > how this is supposed to work.
> 
> By the way, Kevin, feel free to CC linux-sh@vger.kernel.org on Renesas-related topics.

Yeah, I'll CC them in from now on. My questions seemed more generic, but obviously there are per-platform design choices to be made here.

> The legacy API used fake GPIOs for pinmux and pinconf. We don't really need to
> care about it, but we need to ensure that that pinctrl API covers all the use cases.

That's exactly my concern. We've got a large mass of code that aggressively uses the legacy APIs, together with some local extensions, and as such we need to get pinctrl/pinconf to cover all of that. And preferably more elegantly. :)

> > > The pins that we're dealing with basically look like this diagram 
> > > from
> > > pinctrl.txt:
> > >                        pin config
> > >                        logic regs
> > >                        |               +- SPI
> > >      Physical pins --- pad --- pinmux -+- I2C
> > >                                |       +- mmc
> > >                                |       +- GPIO
> > >                                pin
> > >                                multiplex
> > >                                logic regs
> > > 
> > > The control bits we have in the GPIO function are "input enable", 
> > > "output enable" and "output data". Those manual controls are only 
> > > effective when the GPIO mux is selected. The pad config bits are 
> > > "pull up enable" and "pull down enable", and they're always manually controllable.
> >
> > I guess what you mean is that pulls can always be controlled, no 
> > matter if the GPIO block is in use or not, so it corresponds to the diagram above.

Correct. The mux is only switching IE+OE+DATA, not PU+PD.


> > > So adding a "gpio-mode" function to each pin group as per 
> > > pinctrl.txt seems the obvious thing to do.
> > 
> > This is *one* way to do it that some implementers have opted for. The 
> > other way is to implement the .gpio_request_enable(), 
> > .gpio_disable_free() functions in struct pinmux_ops so as to shortcut and simplify matters.
> > 
> > Either way works.

Yes, we do have automatic switch to the GPIO mux when someone uses the GPIO API, thanks to gpio_request_enable. Thus there is no need for gpio-mode for the GPIO API. But I think it may still needed for elegant sleep/idle handling.

> Controlling GPIO through pinconf seems to be very twisted.
> Usually it is the other way around.

I refer you to pinctrl.txt. What I'm trying to avoid here is the "GPIO mode pitfall", where we can /only/ usefully manipulate the lines through the Linux gpio API.

I want to be able to drive a function line low in sleep mode, without having to write code that uses the gpio_ API. I want to be able to express that as PIN_CONFIG_OUTPUT 0.

Where we currently stand locally is that we have a load of code that use the legacy function GPIOs thus:

resume:
    gpio_request(GPIO_FN_XXX)  // Set function mux on GPIO36 pin
suspend:
    gpio_request(GPIO_PORT36)  // Set GPIO mux on pin
    gpio_direction_output(GPIO_PORT36, 0);

which is exactly the scenario described in "GPIO mode pitfalls". Some drivers do this manually, and some use a local pinctrl-ish framework; they're passed big platform data tables with lists of suspend+active GPIO numbers, directions, values and pulls, and then pass those tables back to a library mux-change routine in their suspend/resume handlers.

By my understanding of the general pinctrl concept, we should be converting that to the lovely:

resume:
    pinctrl_pm_select_default_state(dev);
suspend:
    pinctrl_pm_select_sleep_state(dev);

But that means we need to be able to specify the line drives for sleep state through pinctrl tables, which leads us to PIN_CONFIG_OUTPUT, which means controlling the GPIO registers to achieve that. We can do this pretty straightforwardly for the r8a73a4-type case of integrated GPIO+pinctrl - for non-integrated ones, I'm less sure how easy it would be to arrange.

> > This looks messy. The intention with the pin config subsystem is to 
> > break things apart and make them understandable that way, and to 
> > handle electronic stuff in pin config whereas driving a line high/low 
> > happens in the GPIO subsystem.

Which is basically an argument against ever using PIN_CONFIG_OUTPUT 0, and the whole of "GPIO mode pitfalls" in pinctrl.txt, isn't it? It means we can't specify idle pinctrl states that drive lines. We have to do the GPIO/function shuffle.

And you're right, it is a little messy in the pin driver implementation, which is what I'm finding, and what leads me here to question the detail. Nevertheless, I've found implementing PIN_CONFIG_OUTPUT on this hardware design is possible, and produces elegant driver+board code.

In my initial prototype, PIN_CONFIG_OUTPUT is only available if the gpio-mode function has been selected. But maybe this could be made hidden and implicit, as per the GPIO API. Eg PIN_CONFIG_OUTPUT/PIN_CONFIG_LOW_POWER_MODE implicitly selects the GPIO mux (for output/input), and PIN_CONFIG_DRIVE_PUSH_PULL/PIN_CONFIG_BIAS_HIGH_IMPEDANCE restores the previously requested?

> > > What is the canonical ordering for MUX+CONF in state tables/DT?
> > > If MUX comes first, should the driver defer the actual GPIO mux 
> > > switch until the CONF specification, to avoid glitches?
> >
> > This must be some misunderstanding. DT does not specify 
> > application/ordering semantics. It does not define sequences at all. 
> > The device driver parsing out whatever is in the device tree has to 
> > take care of ordering stuff and driving the hardware.

Well, the pinctrl core does process its tables in order. So in the case where a pinconf setting may interact with a pinmux setting, the ordering that the pinconf+pinmux entries get processed could be significant. Eg if PIN_CONFIG_OUTPUT requires the "gpio-mode" function selected first.

Kevin

pinctrl - detailed gpio-mode pinconf design

[kevin.bracey at broadcom.com]

Hi Linus & Laurent,

Thanks for taking the time to think about this.

Laurent Pinchart wrote:
> On Tuesday 03 December 2013 11:04:48 Linus Walleij wrote:
> > On Thu, Nov 28, 2013 at 11:07 AM,  <kevin.bracey@broadcom.com> wrote:
> > > I'm working on converting an shmobile-based system that has a lot of 
> > > custom GPIO/pin manipulation to use pure pinctrl.  I'm a bit stuck 
> > > on some of the design details.

> Kevin, what SoC do you use ? The GPIO+pinmux hardware architecture varies
> between models (with a single driver handling both GPIO and pinmux versus
> two separate drivers), I'd like to know the exact target before replying
> to your questions below.

I'm working on the non-upstreamed r8a7373. The closest relative in the PFC area is the r8a73a4, so pfc-r8a7373 is primarily modelled on that, and I'm trying to devise extensions that could be upstreamed in the form of r8a73a4 patches, even if we don't get the r8a7373 itself upstreamed. I do have access to an APE6EVM for testing that if necessary, but I'm not currently working on it.

> > OK I have forwarded this post to Laurent Pinchart who wrote the 
> > shmobile pin control implementation so we can reach some clarity on 
> > how this is supposed to work.
> 
> By the way, Kevin, feel free to CC linux-sh at vger.kernel.org on Renesas-related topics.

Yeah, I'll CC them in from now on. My questions seemed more generic, but obviously there are per-platform design choices to be made here.

> The legacy API used fake GPIOs for pinmux and pinconf. We don't really need to
> care about it, but we need to ensure that that pinctrl API covers all the use cases.

That's exactly my concern. We've got a large mass of code that aggressively uses the legacy APIs, together with some local extensions, and as such we need to get pinctrl/pinconf to cover all of that. And preferably more elegantly. :)

> > > The pins that we're dealing with basically look like this diagram 
> > > from
> > > pinctrl.txt:
> > >                        pin config
> > >                        logic regs
> > >                        |               +- SPI
> > >      Physical pins --- pad --- pinmux -+- I2C
> > >                                |       +- mmc
> > >                                |       +- GPIO
> > >                                pin
> > >                                multiplex
> > >                                logic regs
> > > 
> > > The control bits we have in the GPIO function are "input enable", 
> > > "output enable" and "output data". Those manual controls are only 
> > > effective when the GPIO mux is selected. The pad config bits are 
> > > "pull up enable" and "pull down enable", and they're always manually controllable.
> >
> > I guess what you mean is that pulls can always be controlled, no 
> > matter if the GPIO block is in use or not, so it corresponds to the diagram above.

Correct. The mux is only switching IE+OE+DATA, not PU+PD.


> > > So adding a "gpio-mode" function to each pin group as per 
> > > pinctrl.txt seems the obvious thing to do.
> > 
> > This is *one* way to do it that some implementers have opted for. The 
> > other way is to implement the .gpio_request_enable(), 
> > .gpio_disable_free() functions in struct pinmux_ops so as to shortcut and simplify matters.
> > 
> > Either way works.

Yes, we do have automatic switch to the GPIO mux when someone uses the GPIO API, thanks to gpio_request_enable. Thus there is no need for gpio-mode for the GPIO API. But I think it may still needed for elegant sleep/idle handling.

> Controlling GPIO through pinconf seems to be very twisted.
> Usually it is the other way around.

I refer you to pinctrl.txt. What I'm trying to avoid here is the "GPIO mode pitfall", where we can /only/ usefully manipulate the lines through the Linux gpio API.

I want to be able to drive a function line low in sleep mode, without having to write code that uses the gpio_ API. I want to be able to express that as PIN_CONFIG_OUTPUT 0.

Where we currently stand locally is that we have a load of code that use the legacy function GPIOs thus:

resume:
    gpio_request(GPIO_FN_XXX)  // Set function mux on GPIO36 pin
suspend:
    gpio_request(GPIO_PORT36)  // Set GPIO mux on pin
    gpio_direction_output(GPIO_PORT36, 0);

which is exactly the scenario described in "GPIO mode pitfalls". Some drivers do this manually, and some use a local pinctrl-ish framework; they're passed big platform data tables with lists of suspend+active GPIO numbers, directions, values and pulls, and then pass those tables back to a library mux-change routine in their suspend/resume handlers.

By my understanding of the general pinctrl concept, we should be converting that to the lovely:

resume:
    pinctrl_pm_select_default_state(dev);
suspend:
    pinctrl_pm_select_sleep_state(dev);

But that means we need to be able to specify the line drives for sleep state through pinctrl tables, which leads us to PIN_CONFIG_OUTPUT, which means controlling the GPIO registers to achieve that. We can do this pretty straightforwardly for the r8a73a4-type case of integrated GPIO+pinctrl - for non-integrated ones, I'm less sure how easy it would be to arrange.

> > This looks messy. The intention with the pin config subsystem is to 
> > break things apart and make them understandable that way, and to 
> > handle electronic stuff in pin config whereas driving a line high/low 
> > happens in the GPIO subsystem.

Which is basically an argument against ever using PIN_CONFIG_OUTPUT 0, and the whole of "GPIO mode pitfalls" in pinctrl.txt, isn't it? It means we can't specify idle pinctrl states that drive lines. We have to do the GPIO/function shuffle.

And you're right, it is a little messy in the pin driver implementation, which is what I'm finding, and what leads me here to question the detail. Nevertheless, I've found implementing PIN_CONFIG_OUTPUT on this hardware design is possible, and produces elegant driver+board code.

In my initial prototype, PIN_CONFIG_OUTPUT is only available if the gpio-mode function has been selected. But maybe this could be made hidden and implicit, as per the GPIO API. Eg PIN_CONFIG_OUTPUT/PIN_CONFIG_LOW_POWER_MODE implicitly selects the GPIO mux (for output/input), and PIN_CONFIG_DRIVE_PUSH_PULL/PIN_CONFIG_BIAS_HIGH_IMPEDANCE restores the previously requested?

> > > What is the canonical ordering for MUX+CONF in state tables/DT?
> > > If MUX comes first, should the driver defer the actual GPIO mux 
> > > switch until the CONF specification, to avoid glitches?
> >
> > This must be some misunderstanding. DT does not specify 
> > application/ordering semantics. It does not define sequences at all. 
> > The device driver parsing out whatever is in the device tree has to 
> > take care of ordering stuff and driving the hardware.

Well, the pinctrl core does process its tables in order. So in the case where a pinconf setting may interact with a pinmux setting, the ordering that the pinconf+pinmux entries get processed could be significant. Eg if PIN_CONFIG_OUTPUT requires the "gpio-mode" function selected first.

Kevin

Re: pinctrl - detailed gpio-mode pinconf design

[Linus Walleij]

On Thu, Dec 5, 2013 at 11:15 AM,  <kevin.bracey@broadcom.com> wrote:
> Laurent Pinchart wrote:
>> On Tuesday 03 December 2013 11:04:48 Linus Walleij wrote:

>> Controlling GPIO through pinconf seems to be very twisted.
>> Usually it is the other way around.
>
> I refer you to pinctrl.txt. What I'm trying to avoid here is the "GPIO mode pitfall", where we can /only/ usefully manipulate the lines through the Linux gpio API.
>
> I want to be able to drive a function line low in sleep mode, without having to write code that uses the gpio_ API. I want to be able to express that as PIN_CONFIG_OUTPUT 0.
>
> Where we currently stand locally is that we have a load of code that use the legacy function GPIOs thus:
>
> resume:
>     gpio_request(GPIO_FN_XXX)  // Set function mux on GPIO36 pin
> suspend:
>     gpio_request(GPIO_PORT36)  // Set GPIO mux on pin
>     gpio_direction_output(GPIO_PORT36, 0);
>
> which is exactly the scenario described in "GPIO mode pitfalls". Some drivers do this manually, and some use a local pinctrl-ish framework; they're passed big platform data tables with lists of suspend+active GPIO numbers, directions, values and pulls, and then pass those tables back to a library mux-change routine in their suspend/resume handlers.
>
> By my understanding of the general pinctrl concept, we should be converting that to the lovely:
>
> resume:
>     pinctrl_pm_select_default_state(dev);
> suspend:
>     pinctrl_pm_select_sleep_state(dev);

This observation is correct, and I agree with this reasoning. The PM
states are quite new and may
take some time before they "sink in" in the kernel though, but ideally
people should be using
them like that (atleast IMO).

>> > This looks messy. The intention with the pin config subsystem is to
>> > break things apart and make them understandable that way, and to
>> > handle electronic stuff in pin config whereas driving a line high/low
>> > happens in the GPIO subsystem.
>
> Which is basically an argument against ever using PIN_CONFIG_OUTPUT 0, and the
> whole of "GPIO mode pitfalls" in pinctrl.txt, isn't it? It means we can't specify idle pinctrl
> states that drive lines. We have to do the GPIO/function shuffle.

Well maybe. My point was that it is OK to drive GPIO lines as part of a state
which is only relevant for a device driver, and where using the GPIO API
in top would only bloat and make things hard to follow.

> And you're right, it is a little messy in the pin driver implementation, which is
> what I'm finding, and what leads me here to question the detail. Nevertheless,
> I've found implementing PIN_CONFIG_OUTPUT on this hardware design is
> possible, and produces elegant driver+board code.

I believe you.

> In my initial prototype, PIN_CONFIG_OUTPUT is only available if the gpio-mode
> function has been selected.

That is even more careful code than most drivers write, so sounds
very nice.

> But maybe this could be made hidden and implicit,
> as per the GPIO API. Eg PIN_CONFIG_OUTPUT/PIN_CONFIG_LOW_POWER_MODE
> implicitly selects the GPIO mux (for output/input), and
> PIN_CONFIG_DRIVE_PUSH_PULL/PIN_CONFIG_BIAS_HIGH_IMPEDANCE
> restores the previously requested?

I'd recommend trying to keep as close to the semantics descrived
in <linux/pinctrl/pinconf-generic.h> and not add any implicit semantics.

>> > This must be some misunderstanding. DT does not specify
>> > application/ordering semantics. It does not define sequences at all.
>> > The device driver parsing out whatever is in the device tree has to
>> > take care of ordering stuff and driving the hardware.
>
> Well, the pinctrl core does process its tables in order. So in the case where a
> pinconf setting may interact with a pinmux setting, the ordering that the
> pinconf+pinmux entries get processed could be significant. Eg if
> PIN_CONFIG_OUTPUT requires the "gpio-mode" function selected first.

OK so we solved the similar situation where configs had to be
applied in some certain order for the pin config API by passing
the array of configs down to the driver and let is handle this.

So can we think of an API that gives the drivers the freedom to
express or behave differently here?

We *could* add something like void (* pin_state_commence) ()
to the pinctrl_ops so the driver could cache mux and config ops
and wait for the commence signal before writing anything to the
hardware, and that would be pretty complex.

If it is always the same for every state setting then maybe we
can move this to the core so that we'd add some semantic
flag like

enum state_application_order {
  PINCTRL_ORDER_UNDEFINED,
  PINCTRL_MUX_THEN_CONFIG,
  PINCTRL_CONFIG_THEN_MUX,
};

Then the core needs to alter behaviour on this flag. Any
suggestions welcome, but I won't apply any patches like that
unless there is a driver making use of it.

Yours,
Linus Walleij

pinctrl - detailed gpio-mode pinconf design

[Linus Walleij]

On Thu, Dec 5, 2013 at 11:15 AM,  <kevin.bracey@broadcom.com> wrote:
> Laurent Pinchart wrote:
>> On Tuesday 03 December 2013 11:04:48 Linus Walleij wrote:

>> Controlling GPIO through pinconf seems to be very twisted.
>> Usually it is the other way around.
>
> I refer you to pinctrl.txt. What I'm trying to avoid here is the "GPIO mode pitfall", where we can /only/ usefully manipulate the lines through the Linux gpio API.
>
> I want to be able to drive a function line low in sleep mode, without having to write code that uses the gpio_ API. I want to be able to express that as PIN_CONFIG_OUTPUT 0.
>
> Where we currently stand locally is that we have a load of code that use the legacy function GPIOs thus:
>
> resume:
>     gpio_request(GPIO_FN_XXX)  // Set function mux on GPIO36 pin
> suspend:
>     gpio_request(GPIO_PORT36)  // Set GPIO mux on pin
>     gpio_direction_output(GPIO_PORT36, 0);
>
> which is exactly the scenario described in "GPIO mode pitfalls". Some drivers do this manually, and some use a local pinctrl-ish framework; they're passed big platform data tables with lists of suspend+active GPIO numbers, directions, values and pulls, and then pass those tables back to a library mux-change routine in their suspend/resume handlers.
>
> By my understanding of the general pinctrl concept, we should be converting that to the lovely:
>
> resume:
>     pinctrl_pm_select_default_state(dev);
> suspend:
>     pinctrl_pm_select_sleep_state(dev);

This observation is correct, and I agree with this reasoning. The PM
states are quite new and may
take some time before they "sink in" in the kernel though, but ideally
people should be using
them like that (atleast IMO).

>> > This looks messy. The intention with the pin config subsystem is to
>> > break things apart and make them understandable that way, and to
>> > handle electronic stuff in pin config whereas driving a line high/low
>> > happens in the GPIO subsystem.
>
> Which is basically an argument against ever using PIN_CONFIG_OUTPUT 0, and the
> whole of "GPIO mode pitfalls" in pinctrl.txt, isn't it? It means we can't specify idle pinctrl
> states that drive lines. We have to do the GPIO/function shuffle.

Well maybe. My point was that it is OK to drive GPIO lines as part of a state
which is only relevant for a device driver, and where using the GPIO API
in top would only bloat and make things hard to follow.

> And you're right, it is a little messy in the pin driver implementation, which is
> what I'm finding, and what leads me here to question the detail. Nevertheless,
> I've found implementing PIN_CONFIG_OUTPUT on this hardware design is
> possible, and produces elegant driver+board code.

I believe you.

> In my initial prototype, PIN_CONFIG_OUTPUT is only available if the gpio-mode
> function has been selected.

That is even more careful code than most drivers write, so sounds
very nice.

> But maybe this could be made hidden and implicit,
> as per the GPIO API. Eg PIN_CONFIG_OUTPUT/PIN_CONFIG_LOW_POWER_MODE
> implicitly selects the GPIO mux (for output/input), and
> PIN_CONFIG_DRIVE_PUSH_PULL/PIN_CONFIG_BIAS_HIGH_IMPEDANCE
> restores the previously requested?

I'd recommend trying to keep as close to the semantics descrived
in <linux/pinctrl/pinconf-generic.h> and not add any implicit semantics.

>> > This must be some misunderstanding. DT does not specify
>> > application/ordering semantics. It does not define sequences at all.
>> > The device driver parsing out whatever is in the device tree has to
>> > take care of ordering stuff and driving the hardware.
>
> Well, the pinctrl core does process its tables in order. So in the case where a
> pinconf setting may interact with a pinmux setting, the ordering that the
> pinconf+pinmux entries get processed could be significant. Eg if
> PIN_CONFIG_OUTPUT requires the "gpio-mode" function selected first.

OK so we solved the similar situation where configs had to be
applied in some certain order for the pin config API by passing
the array of configs down to the driver and let is handle this.

So can we think of an API that gives the drivers the freedom to
express or behave differently here?

We *could* add something like void (* pin_state_commence) ()
to the pinctrl_ops so the driver could cache mux and config ops
and wait for the commence signal before writing anything to the
hardware, and that would be pretty complex.

If it is always the same for every state setting then maybe we
can move this to the core so that we'd add some semantic
flag like

enum state_application_order {
  PINCTRL_ORDER_UNDEFINED,
  PINCTRL_MUX_THEN_CONFIG,
  PINCTRL_CONFIG_THEN_MUX,
};

Then the core needs to alter behaviour on this flag. Any
suggestions welcome, but I won't apply any patches like that
unless there is a driver making use of it.

Yours,
Linus Walleij

RE: pinctrl - detailed gpio-mode pinconf design

[kevin.bracey]

Linus Walleij wrote:
> On Thu, Dec 5, 2013 at 11:15 AM,  <kevin.bracey@broadcom.com> wrote:
>> Laurent Pinchart wrote:
>>> On Tuesday 03 December 2013 11:04:48 Linus Walleij wrote:
>
>>> Controlling GPIO through pinconf seems to be very twisted.
>>> Usually it is the other way around.
>>
>> By my understanding of the general pinctrl concept, we should be converting that to the lovely:
>>
>> resume:
>>     pinctrl_pm_select_default_state(dev);
>> suspend:
>>     pinctrl_pm_select_sleep_state(dev);

> This observation is correct, and I agree with this reasoning. The PM states
> are quite new and may take some time before they "sink in" in the kernel though,
> but ideally people should be using them like that (atleast IMO).

I strongly approve of the PM state concept, having seen the alternative. It slots in very nicely. I'm working on 3.10-based systems, but the pinctrl_pm_select is being cherry-picked in because it makes life so much easier.

>>> > This looks messy. The intention with the pin config subsystem is to 
>>> > break things apart and make them understandable that way, and to 
>>> > handle electronic stuff in pin config whereas driving a line 
>>> > high/low happens in the GPIO subsystem.
>>
>> Which is basically an argument against ever using PIN_CONFIG_OUTPUT 0, 
>> and the whole of "GPIO mode pitfalls" in pinctrl.txt, isn't it? It 
>> means we can't specify idle pinctrl states that drive lines. We have to do the GPIO/function shuffle.
>
> Well maybe. My point was that it is OK to drive GPIO lines as part of
> a state which is only relevant for a device driver, and where using the
> GPIO API in top would only bloat and make things hard to follow.

In practice, this is the case for the majority of the code I'm looking at. We have a large number of function drivers which have to be told their pins' GPIO numbers in platform data, solely for their suspend setting. Otherwise the pins would be entirely driven as a function, and no need for the GPIO API.

> > In my initial prototype, PIN_CONFIG_OUTPUT is only available if the 
> > gpio-mode function has been selected.

> That is even more careful code than most drivers write, so sounds very nice.

> > But maybe this could be made hidden and implicit, as per the GPIO API. 
> > Eg PIN_CONFIG_OUTPUT/PIN_CONFIG_LOW_POWER_MODE
> > implicitly selects the GPIO mux (for output/input), and 
> > PIN_CONFIG_DRIVE_PUSH_PULL/PIN_CONFIG_BIAS_HIGH_IMPEDANCE
> > restores the previously requested?
>
> I'd recommend trying to keep as close to the semantics descrived in <linux/pinctrl/pinconf-generic.h> and not add any implicit semantics.

Yeah, but the catch is that the semantics described are on the assumption that pinconf is totally orthogonal to pinmux. That's not true on this platform, and on many others. I think we need some sort of general policy on how to handle conflicting requests.

Personally, I can see the merits of both approaches -

 a) pinconf settings forcing mux to achieve the requested pin state - working to emulate orthogonality on non-orthogonal hardware;

 b) pinconf settings requiring an appropriate mux to be explicitly set for them to be available - machines/DT need to know about the mux/conf interactions.

Main problem with (a) - how do you get back to "normal" mux, and is emulation just obfuscation?
Main problem with (b) - can you guarantee mux/conf ordering?

It sounds like you prefer (b), and that is what my prototype does. It's what I originally decided, but I wasn't totally certain about how much pinconf-generic.h was intended to be a framework/language whose usage is always going to be very pin-driver-dependent, versus trying to make it an API trying to produce consistent behaviour.

>>> > This must be some misunderstanding. DT does not specify 
>>> > application/ordering semantics. It does not define sequences at all.
>>> > The device driver parsing out whatever is in the device tree has to 
>>> > take care of ordering stuff and driving the hardware.
>
>> Well, the pinctrl core does process its tables in order. So in the 
>> case where a pinconf setting may interact with a pinmux setting, the 
>> ordering that the
>> pinconf+pinmux entries get processed could be significant. Eg if
>> PIN_CONFIG_OUTPUT requires the "gpio-mode" function selected first.

> OK so we solved the similar situation where configs had to be applied in some certain order for the pin config API by passing the array of configs down to the driver and let is handle this.

Well, ordering is already preserved by the pinctrl core. As far as I can see, when you're given a table of settings through pinctrl_register_map, you keep the ordering, and when that state is selected, the settings are passed down to the driver one-by-one in original order.

If that can be guaranteed, then we may not need to be do anything else.

> So can we think of an API that gives the drivers the freedom to express or behave differently here?

I don't think it's necessary.

For DT, the driver can control ordering in its parse routines. The sh-pfc driver does already put a node's function request first in its dt_node_to_map(), which is what gpio-mode needs. If the core preserves that order, we're sorted.

For non-DT, I think it's acceptable to require that the tables should be ordered correctly by the board to meet the documented requirements of the driver - so sh-pfc board files would be required to put gpio-mode mux selections before PIN_CONFIG_OUTPUT/PIN_BIAS_CONFIG_HIGH_IMPEDANCE selections.

And either way, for glitch-free-handover, I'm also requiring that the configs always be specified - the mux won't be switched by the driver unless/until a config has been parsed and set. So in the driver - first pinmux "gpio-mode" primes it, and makes the output/high-impedance pinconfs legal. Then the pinconfs set the IE/OE/DATA bits in the GPIO registers and flip to GPIO mux.


Now, going back to the pinconfs. The naming of the configs still throws me a little. Particularly all the "bias" settings. "BIAS_HIGH_IMPEDANCE" doesn't feel like a "bias" to me. What does it mean to "BIAS_DISABLE" a "BIAS_HIGH_IMPEDANCE"? Should "BIAS_HIGH_IMPEDANCE" turn off "BIAS_PULL_UP"?

My feeling is that it would be better named just "CONFIG_HIGH_IMPEDANCE", making it a separate thing from all the other biases, akin to CONFIG_DRIVE_* and CONFIG_OUTPUT.

In summary, I think the two independent exclusive Boolean sets in the configs seem to be:

(BIAS_DISABLE | BIAS_BUS_HOLD | BIAS_PULL_DOWN | BIAS_PULL_UP | BIAS_PULL_PIN_DEFAULT)
([BIAS_]HIGH_IMPEDANCE | DRIVE_PUSH_PULL | DRIVE_OPEN_DRAIN | DRIVE_OPEN_SOURCE | LOW_POWER_MODE | OUTPUT)

And that's effectively how I'm treating them, for the 5 I'm handling for our hardware (BIAS_DISABLE|BIAS_PULL_DOWN|BIAS_PULL_UP)+(BIAS_HIGH_IMPEDANCE|OUTPUT).

Does that make sense? Maybe it's hardware specific, but at least for our case, where the pulls are non-muxed and the drives are muxed, that separation seems logical.

Regards,

Kevin

RE: pinctrl - detailed gpio-mode pinconf design

[kevin.bracey]

Linus Walleij wrote:
> On Thu, Dec 5, 2013 at 11:15 AM,  <kevin.bracey@broadcom.com> wrote:
>> Laurent Pinchart wrote:
>>> On Tuesday 03 December 2013 11:04:48 Linus Walleij wrote:
>
>>> Controlling GPIO through pinconf seems to be very twisted.
>>> Usually it is the other way around.
>>
>> By my understanding of the general pinctrl concept, we should be converting that to the lovely:
>>
>> resume:
>>     pinctrl_pm_select_default_state(dev);
>> suspend:
>>     pinctrl_pm_select_sleep_state(dev);

> This observation is correct, and I agree with this reasoning. The PM states
> are quite new and may take some time before they "sink in" in the kernel though,
> but ideally people should be using them like that (atleast IMO).

I strongly approve of the PM state concept, having seen the alternative. It slots in very nicely. I'm working on 3.10-based systems, but the pinctrl_pm_select is being cherry-picked in because it makes life so much easier.

>>> > This looks messy. The intention with the pin config subsystem is to 
>>> > break things apart and make them understandable that way, and to 
>>> > handle electronic stuff in pin config whereas driving a line 
>>> > high/low happens in the GPIO subsystem.
>>
>> Which is basically an argument against ever using PIN_CONFIG_OUTPUT 0, 
>> and the whole of "GPIO mode pitfalls" in pinctrl.txt, isn't it? It 
>> means we can't specify idle pinctrl states that drive lines. We have to do the GPIO/function shuffle.
>
> Well maybe. My point was that it is OK to drive GPIO lines as part of
> a state which is only relevant for a device driver, and where using the
> GPIO API in top would only bloat and make things hard to follow.

In practice, this is the case for the majority of the code I'm looking at. We have a large number of function drivers which have to be told their pins' GPIO numbers in platform data, solely for their suspend setting. Otherwise the pins would be entirely driven as a function, and no need for the GPIO API.

> > In my initial prototype, PIN_CONFIG_OUTPUT is only available if the 
> > gpio-mode function has been selected.

> That is even more careful code than most drivers write, so sounds very nice.

> > But maybe this could be made hidden and implicit, as per the GPIO API. 
> > Eg PIN_CONFIG_OUTPUT/PIN_CONFIG_LOW_POWER_MODE
> > implicitly selects the GPIO mux (for output/input), and 
> > PIN_CONFIG_DRIVE_PUSH_PULL/PIN_CONFIG_BIAS_HIGH_IMPEDANCE
> > restores the previously requested?
>
> I'd recommend trying to keep as close to the semantics descrived in <linux/pinctrl/pinconf-generic.h> and not add any implicit semantics.

Yeah, but the catch is that the semantics described are on the assumption that pinconf is totally orthogonal to pinmux. That's not true on this platform, and on many others. I think we need some sort of general policy on how to handle conflicting requests.

Personally, I can see the merits of both approaches -

 a) pinconf settings forcing mux to achieve the requested pin state - working to emulate orthogonality on non-orthogonal hardware;

 b) pinconf settings requiring an appropriate mux to be explicitly set for them to be available - machines/DT need to know about the mux/conf interactions.

Main problem with (a) - how do you get back to "normal" mux, and is emulation just obfuscation?
Main problem with (b) - can you guarantee mux/conf ordering?

It sounds like you prefer (b), and that is what my prototype does. It's what I originally decided, but I wasn't totally certain about how much pinconf-generic.h was intended to be a framework/language whose usage is always going to be very pin-driver-dependent, versus trying to make it an API trying to produce consistent behaviour.

>>> > This must be some misunderstanding. DT does not specify 
>>> > application/ordering semantics. It does not define sequences at all.
>>> > The device driver parsing out whatever is in the device tree has to 
>>> > take care of ordering stuff and driving the hardware.
>
>> Well, the pinctrl core does process its tables in order. So in the 
>> case where a pinconf setting may interact with a pinmux setting, the 
>> ordering that the
>> pinconf+pinmux entries get processed could be significant. Eg if
>> PIN_CONFIG_OUTPUT requires the "gpio-mode" function selected first.

> OK so we solved the similar situation where configs had to be applied in some certain order for the pin config API by passing the array of configs down to the driver and let is handle this.

Well, ordering is already preserved by the pinctrl core. As far as I can see, when you're given a table of settings through pinctrl_register_map, you keep the ordering, and when that state is selected, the settings are passed down to the driver one-by-one in original order.

If that can be guaranteed, then we may not need to be do anything else.

> So can we think of an API that gives the drivers the freedom to express or behave differently here?

I don't think it's necessary.

For DT, the driver can control ordering in its parse routines. The sh-pfc driver does already put a node's function request first in its dt_node_to_map(), which is what gpio-mode needs. If the core preserves that order, we're sorted.

For non-DT, I think it's acceptable to require that the tables should be ordered correctly by the board to meet the documented requirements of the driver - so sh-pfc board files would be required to put gpio-mode mux selections before PIN_CONFIG_OUTPUT/PIN_BIAS_CONFIG_HIGH_IMPEDANCE selections.

And either way, for glitch-free-handover, I'm also requiring that the configs always be specified - the mux won't be switched by the driver unless/until a config has been parsed and set. So in the driver - first pinmux "gpio-mode" primes it, and makes the output/high-impedance pinconfs legal. Then the pinconfs set the IE/OE/DATA bits in the GPIO registers and flip to GPIO mux.


Now, going back to the pinconfs. The naming of the configs still throws me a little. Particularly all the "bias" settings. "BIAS_HIGH_IMPEDANCE" doesn't feel like a "bias" to me. What does it mean to "BIAS_DISABLE" a "BIAS_HIGH_IMPEDANCE"? Should "BIAS_HIGH_IMPEDANCE" turn off "BIAS_PULL_UP"?

My feeling is that it would be better named just "CONFIG_HIGH_IMPEDANCE", making it a separate thing from all the other biases, akin to CONFIG_DRIVE_* and CONFIG_OUTPUT.

In summary, I think the two independent exclusive Boolean sets in the configs seem to be:

(BIAS_DISABLE | BIAS_BUS_HOLD | BIAS_PULL_DOWN | BIAS_PULL_UP | BIAS_PULL_PIN_DEFAULT)
([BIAS_]HIGH_IMPEDANCE | DRIVE_PUSH_PULL | DRIVE_OPEN_DRAIN | DRIVE_OPEN_SOURCE | LOW_POWER_MODE | OUTPUT)

And that's effectively how I'm treating them, for the 5 I'm handling for our hardware (BIAS_DISABLE|BIAS_PULL_DOWN|BIAS_PULL_UP)+(BIAS_HIGH_IMPEDANCE|OUTPUT).

Does that make sense? Maybe it's hardware specific, but at least for our case, where the pulls are non-muxed and the drives are muxed, that separation seems logical.

Regards,

Kevin

pinctrl - detailed gpio-mode pinconf design

[kevin.bracey at broadcom.com]

Linus Walleij wrote:
> On Thu, Dec 5, 2013 at 11:15 AM,  <kevin.bracey@broadcom.com> wrote:
>> Laurent Pinchart wrote:
>>> On Tuesday 03 December 2013 11:04:48 Linus Walleij wrote:
>
>>> Controlling GPIO through pinconf seems to be very twisted.
>>> Usually it is the other way around.
>>
>> By my understanding of the general pinctrl concept, we should be converting that to the lovely:
>>
>> resume:
>>     pinctrl_pm_select_default_state(dev);
>> suspend:
>>     pinctrl_pm_select_sleep_state(dev);

> This observation is correct, and I agree with this reasoning. The PM states
> are quite new and may take some time before they "sink in" in the kernel though,
> but ideally people should be using them like that (atleast IMO).

I strongly approve of the PM state concept, having seen the alternative. It slots in very nicely. I'm working on 3.10-based systems, but the pinctrl_pm_select is being cherry-picked in because it makes life so much easier.

>>> > This looks messy. The intention with the pin config subsystem is to 
>>> > break things apart and make them understandable that way, and to 
>>> > handle electronic stuff in pin config whereas driving a line 
>>> > high/low happens in the GPIO subsystem.
>>
>> Which is basically an argument against ever using PIN_CONFIG_OUTPUT 0, 
>> and the whole of "GPIO mode pitfalls" in pinctrl.txt, isn't it? It 
>> means we can't specify idle pinctrl states that drive lines. We have to do the GPIO/function shuffle.
>
> Well maybe. My point was that it is OK to drive GPIO lines as part of
> a state which is only relevant for a device driver, and where using the
> GPIO API in top would only bloat and make things hard to follow.

In practice, this is the case for the majority of the code I'm looking at. We have a large number of function drivers which have to be told their pins' GPIO numbers in platform data, solely for their suspend setting. Otherwise the pins would be entirely driven as a function, and no need for the GPIO API.

> > In my initial prototype, PIN_CONFIG_OUTPUT is only available if the 
> > gpio-mode function has been selected.

> That is even more careful code than most drivers write, so sounds very nice.

> > But maybe this could be made hidden and implicit, as per the GPIO API. 
> > Eg PIN_CONFIG_OUTPUT/PIN_CONFIG_LOW_POWER_MODE
> > implicitly selects the GPIO mux (for output/input), and 
> > PIN_CONFIG_DRIVE_PUSH_PULL/PIN_CONFIG_BIAS_HIGH_IMPEDANCE
> > restores the previously requested?
>
> I'd recommend trying to keep as close to the semantics descrived in <linux/pinctrl/pinconf-generic.h> and not add any implicit semantics.

Yeah, but the catch is that the semantics described are on the assumption that pinconf is totally orthogonal to pinmux. That's not true on this platform, and on many others. I think we need some sort of general policy on how to handle conflicting requests.

Personally, I can see the merits of both approaches -

 a) pinconf settings forcing mux to achieve the requested pin state - working to emulate orthogonality on non-orthogonal hardware;

 b) pinconf settings requiring an appropriate mux to be explicitly set for them to be available - machines/DT need to know about the mux/conf interactions.

Main problem with (a) - how do you get back to "normal" mux, and is emulation just obfuscation?
Main problem with (b) - can you guarantee mux/conf ordering?

It sounds like you prefer (b), and that is what my prototype does. It's what I originally decided, but I wasn't totally certain about how much pinconf-generic.h was intended to be a framework/language whose usage is always going to be very pin-driver-dependent, versus trying to make it an API trying to produce consistent behaviour.

>>> > This must be some misunderstanding. DT does not specify 
>>> > application/ordering semantics. It does not define sequences at all.
>>> > The device driver parsing out whatever is in the device tree has to 
>>> > take care of ordering stuff and driving the hardware.
>
>> Well, the pinctrl core does process its tables in order. So in the 
>> case where a pinconf setting may interact with a pinmux setting, the 
>> ordering that the
>> pinconf+pinmux entries get processed could be significant. Eg if
>> PIN_CONFIG_OUTPUT requires the "gpio-mode" function selected first.

> OK so we solved the similar situation where configs had to be applied in some certain order for the pin config API by passing the array of configs down to the driver and let is handle this.

Well, ordering is already preserved by the pinctrl core. As far as I can see, when you're given a table of settings through pinctrl_register_map, you keep the ordering, and when that state is selected, the settings are passed down to the driver one-by-one in original order.

If that can be guaranteed, then we may not need to be do anything else.

> So can we think of an API that gives the drivers the freedom to express or behave differently here?

I don't think it's necessary.

For DT, the driver can control ordering in its parse routines. The sh-pfc driver does already put a node's function request first in its dt_node_to_map(), which is what gpio-mode needs. If the core preserves that order, we're sorted.

For non-DT, I think it's acceptable to require that the tables should be ordered correctly by the board to meet the documented requirements of the driver - so sh-pfc board files would be required to put gpio-mode mux selections before PIN_CONFIG_OUTPUT/PIN_BIAS_CONFIG_HIGH_IMPEDANCE selections.

And either way, for glitch-free-handover, I'm also requiring that the configs always be specified - the mux won't be switched by the driver unless/until a config has been parsed and set. So in the driver - first pinmux "gpio-mode" primes it, and makes the output/high-impedance pinconfs legal. Then the pinconfs set the IE/OE/DATA bits in the GPIO registers and flip to GPIO mux.


Now, going back to the pinconfs. The naming of the configs still throws me a little. Particularly all the "bias" settings. "BIAS_HIGH_IMPEDANCE" doesn't feel like a "bias" to me. What does it mean to "BIAS_DISABLE" a "BIAS_HIGH_IMPEDANCE"? Should "BIAS_HIGH_IMPEDANCE" turn off "BIAS_PULL_UP"?

My feeling is that it would be better named just "CONFIG_HIGH_IMPEDANCE", making it a separate thing from all the other biases, akin to CONFIG_DRIVE_* and CONFIG_OUTPUT.

In summary, I think the two independent exclusive Boolean sets in the configs seem to be:

(BIAS_DISABLE | BIAS_BUS_HOLD | BIAS_PULL_DOWN | BIAS_PULL_UP | BIAS_PULL_PIN_DEFAULT)
([BIAS_]HIGH_IMPEDANCE | DRIVE_PUSH_PULL | DRIVE_OPEN_DRAIN | DRIVE_OPEN_SOURCE | LOW_POWER_MODE | OUTPUT)

And that's effectively how I'm treating them, for the 5 I'm handling for our hardware (BIAS_DISABLE|BIAS_PULL_DOWN|BIAS_PULL_UP)+(BIAS_HIGH_IMPEDANCE|OUTPUT).

Does that make sense? Maybe it's hardware specific, but at least for our case, where the pulls are non-muxed and the drives are muxed, that separation seems logical.

Regards,

Kevin

Re: pinctrl - detailed gpio-mode pinconf design

[Linus Walleij]

On Fri, Dec 13, 2013 at 9:48 AM,  <kevin.bracey@broadcom.com> wrote:
> Linus Walleij wrote:

>> I'd recommend trying to keep as close to the semantics descrived in
>> <linux/pinctrl/pinconf-generic.h> and not add any implicit semantics.
>
> Yeah, but the catch is that the semantics described are on the assumption
> that pinconf is totally orthogonal to pinmux. That's not true on this platform,
> and on many others. I think we need some sort of general policy on how to
> handle conflicting requests.

Yes currently the kernel does assume these two things are orthogonal.
The implicit assumption is that if they are not, the driver gets to handle
it. It worked so far, but I'm all for a more general solution in the core
if we can come up with something elegant.

> Personally, I can see the merits of both approaches -
>
>  a) pinconf settings forcing mux to achieve the requested pin state -
> working to emulate orthogonality on non-orthogonal hardware;
>  b) pinconf settings requiring an appropriate mux to be explicitly set for
> them to be available - machines/DT need to know about the mux/conf
> interactions.
>
> Main problem with (a) - how do you get back to "normal" mux, and is
>  emulation just obfuscation?
> Main problem with (b) - can you guarantee mux/conf ordering?
>
> It sounds like you prefer (b),

I'm not very fond of either solution really. Can we think of a third
alternative where the pin control core is aware of the constraints and
tries to verify and/or solve the situation?

> and that is what my prototype does. It's what I
> originally decided, but I wasn't totally certain about how much
> pinconf-generic.h was intended to be a framework/language whose
> usage is always going to be very pin-driver-dependent, versus trying
> to make it an API trying to produce consistent behaviour.

The idea is to make the code as reusable as possible with
as many checks for physical constraints as possible, so as
to help the driver writers.

To achieve this the generic pin config tries to establish a
semantic on a best effort basis, sometimes leaving "fill in the blanks"
etc, as we're doing engineering not mathematics.

It currently has no concept of sequences or ordering however,
none whatsoever, all such details are left to the driver to
figure out. I don't know if this is good or bad.

>>> Well, the pinctrl core does process its tables in order. So in the
>>> case where a pinconf setting may interact with a pinmux setting, the
>>> ordering that the
>>> pinconf+pinmux entries get processed could be significant. Eg if
>>> PIN_CONFIG_OUTPUT requires the "gpio-mode" function selected first.
>
>> OK so we solved the similar situation where configs had to be applied
>> in some certain order for the pin config API by passing the array of
>> configs down to the driver and let is handle this.
>
> Well, ordering is already preserved by the pinctrl core. As far as I can
> see, when you're given a table of settings through pinctrl_register_map,
> you keep the ordering, and when that state is selected, the settings are
> passed down to the driver one-by-one in original order.

OK I see...

> If that can be guaranteed, then we may not need to be do anything else.

I suspect people are depending on this already to varying extent,
depending on the sophistication of the system HW.

>> So can we think of an API that gives the drivers the freedom to express
>> or behave differently here?
>
> I don't think it's necessary.
>
> For DT, the driver can control ordering in its parse routines. The sh-pfc
> driver does already put a node's function request first in its
>  dt_node_to_map(), which is what gpio-mode needs. If the core preserves
> that order, we're sorted.

OK I'm sort of aligned on this.

> For non-DT, I think it's acceptable to require that the tables should be
> ordered correctly by the board to meet the documented requirements
> of the driver - so sh-pfc board files would be required to put gpio-mode
> mux selections before
> PIN_CONFIG_OUTPUT/PIN_BIAS_CONFIG_HIGH_IMPEDANCE selections.

This seems like the most invasive action that needs to be taken is
a patch to Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
stating this, and the discussion needs to be with the DT people rather
than me I think. I cannot say much about ordering semantics in the
DTB, but to some extent I think it is already there in arrays and such.

> And either way, for glitch-free-handover, I'm also requiring that the
> configs always be specified - the mux won't be switched by the driver
> unless/until a config has been parsed and set. So in the driver - first
> pinmux "gpio-mode" primes it, and makes the output/high-impedance
> pinconfs legal. Then the pinconfs set the IE/OE/DATA bits in the GPIO
> registers and flip to GPIO mux.

OK if this is a driver detail it's all up to the driver writers.

> Now, going back to the pinconfs. The naming of the configs still throws
> me a little. Particularly all the "bias" settings. "BIAS_HIGH_IMPEDANCE"
> doesn't feel like a "bias" to me. What does it mean to "BIAS_DISABLE"
> a "BIAS_HIGH_IMPEDANCE"?

It is defined in the header file:

 * @PIN_CONFIG_BIAS_DISABLE: disable any pin bias on the pin, a
 *      transition from say pull-up to pull-down implies that you disable
 *      pull-up in the process, this setting disables all biasing.
 * @PIN_CONFIG_BIAS_HIGH_IMPEDANCE: the pin will be set to a high impedance
 *      mode, also know as "third-state" (tristate) or "high-Z" or "floating".
 *      On output pins this effectively disconnects the pin, which is useful
 *      if for example some other pin is going to drive the signal connected
 *      to it for a while. Pins used for input are usually always high
 *      impedance.

Being able to disable biasing on a pin, i.e. by disconnecting
any pull-up/pull-downs through software does not imply that
it is necessarily in high-Z mode after that - this depends on any
additional hardwired electronics, especially if the pin has been
tailored for a specific purpose. It may very often be high-Z
but this is a simplified view of the world and we cannot
guarantee that. Not always are all aspects of a pin under
software control.

> Should "BIAS_HIGH_IMPEDANCE" turn off "BIAS_PULL_UP"?

It is *currently* up to the driver not to allow things that make
no electrical sense, or take evasive action. But I can think of
doing code in the core actually doing this check if it makes
sense to most. Writing and testing patches for this is probably
more fruitful than just discussing it.

> My feeling is that it would be better named just
>  "CONFIG_HIGH_IMPEDANCE", making it a separate thing from
> all the other biases, akin to CONFIG_DRIVE_* and CONFIG_OUTPUT.

The documentation clearly states that this is the same as floating
or tristate.

I named it with the *BIAS* substring since a high-Z bias can
be conceived as a pull-up or pull-down with an infinite resistance.
I don't see any reason to change this, it would just lead to
a massive refactoring of all existing drivers using this, plus it
is already represented in device trees using "bias-high-impedance"
which cannot be changed as DT bindings are perpetual, changing
this would just cause chaos. I suggest to take a deep breath
and live with this terminology. :-)

> In summary, I think the two independent exclusive Boolean sets in the configs seem to be:
>
> (BIAS_DISABLE | BIAS_BUS_HOLD | BIAS_PULL_DOWN | BIAS_PULL_UP | BIAS_PULL_PIN_DEFAULT)
> ([BIAS_]HIGH_IMPEDANCE | DRIVE_PUSH_PULL | DRIVE_OPEN_DRAIN | DRIVE_OPEN_SOURCE | LOW_POWER_MODE | OUTPUT)
>
> And that's effectively how I'm treating them, for the 5 I'm handling for our hardware
> (BIAS_DISABLE|BIAS_PULL_DOWN|BIAS_PULL_UP)+(BIAS_HIGH_IMPEDANCE|OUTPUT).
>
> Does that make sense? Maybe it's hardware specific, but at least for our case,
> where the pulls are non-muxed and the drives are muxed, that separation seems logical.

We have not established any such semantic. It is also mostly
uselesss to do that through documentation or discussion about
it, as this doesn't really have any influence on what people
do in practice.

The only way to have a meaningful discussion around it is
by proposing a patch to drivers/pinctrl/pinconf-generic.c
actually *enforcing* that semantic, and have it tested and
accepted. That is the only way any such distinction would
make sense in practice.

Then we can state the obvious in the documentation...

I would really like to see such a patch, because what
you're saying makes a lot of sense, electrically speaking.
But I expect it to need some discussing and testing
and possibly debugging of existing drivers.

If you want to enforce such a semantic in a certain pin
config driver - i.e. looking for forbidden states in the
set_config() callback when looping over the requested
settings - this is of course perfectly fine.

Yours,
Linus Walleij

Re: pinctrl - detailed gpio-mode pinconf design

[Linus Walleij]

On Fri, Dec 13, 2013 at 9:48 AM,  <kevin.bracey@broadcom.com> wrote:
> Linus Walleij wrote:

>> I'd recommend trying to keep as close to the semantics descrived in
>> <linux/pinctrl/pinconf-generic.h> and not add any implicit semantics.
>
> Yeah, but the catch is that the semantics described are on the assumption
> that pinconf is totally orthogonal to pinmux. That's not true on this platform,
> and on many others. I think we need some sort of general policy on how to
> handle conflicting requests.

Yes currently the kernel does assume these two things are orthogonal.
The implicit assumption is that if they are not, the driver gets to handle
it. It worked so far, but I'm all for a more general solution in the core
if we can come up with something elegant.

> Personally, I can see the merits of both approaches -
>
>  a) pinconf settings forcing mux to achieve the requested pin state -
> working to emulate orthogonality on non-orthogonal hardware;
>  b) pinconf settings requiring an appropriate mux to be explicitly set for
> them to be available - machines/DT need to know about the mux/conf
> interactions.
>
> Main problem with (a) - how do you get back to "normal" mux, and is
>  emulation just obfuscation?
> Main problem with (b) - can you guarantee mux/conf ordering?
>
> It sounds like you prefer (b),

I'm not very fond of either solution really. Can we think of a third
alternative where the pin control core is aware of the constraints and
tries to verify and/or solve the situation?

> and that is what my prototype does. It's what I
> originally decided, but I wasn't totally certain about how much
> pinconf-generic.h was intended to be a framework/language whose
> usage is always going to be very pin-driver-dependent, versus trying
> to make it an API trying to produce consistent behaviour.

The idea is to make the code as reusable as possible with
as many checks for physical constraints as possible, so as
to help the driver writers.

To achieve this the generic pin config tries to establish a
semantic on a best effort basis, sometimes leaving "fill in the blanks"
etc, as we're doing engineering not mathematics.

It currently has no concept of sequences or ordering however,
none whatsoever, all such details are left to the driver to
figure out. I don't know if this is good or bad.

>>> Well, the pinctrl core does process its tables in order. So in the
>>> case where a pinconf setting may interact with a pinmux setting, the
>>> ordering that the
>>> pinconf+pinmux entries get processed could be significant. Eg if
>>> PIN_CONFIG_OUTPUT requires the "gpio-mode" function selected first.
>
>> OK so we solved the similar situation where configs had to be applied
>> in some certain order for the pin config API by passing the array of
>> configs down to the driver and let is handle this.
>
> Well, ordering is already preserved by the pinctrl core. As far as I can
> see, when you're given a table of settings through pinctrl_register_map,
> you keep the ordering, and when that state is selected, the settings are
> passed down to the driver one-by-one in original order.

OK I see...

> If that can be guaranteed, then we may not need to be do anything else.

I suspect people are depending on this already to varying extent,
depending on the sophistication of the system HW.

>> So can we think of an API that gives the drivers the freedom to express
>> or behave differently here?
>
> I don't think it's necessary.
>
> For DT, the driver can control ordering in its parse routines. The sh-pfc
> driver does already put a node's function request first in its
>  dt_node_to_map(), which is what gpio-mode needs. If the core preserves
> that order, we're sorted.

OK I'm sort of aligned on this.

> For non-DT, I think it's acceptable to require that the tables should be
> ordered correctly by the board to meet the documented requirements
> of the driver - so sh-pfc board files would be required to put gpio-mode
> mux selections before
> PIN_CONFIG_OUTPUT/PIN_BIAS_CONFIG_HIGH_IMPEDANCE selections.

This seems like the most invasive action that needs to be taken is
a patch to Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
stating this, and the discussion needs to be with the DT people rather
than me I think. I cannot say much about ordering semantics in the
DTB, but to some extent I think it is already there in arrays and such.

> And either way, for glitch-free-handover, I'm also requiring that the
> configs always be specified - the mux won't be switched by the driver
> unless/until a config has been parsed and set. So in the driver - first
> pinmux "gpio-mode" primes it, and makes the output/high-impedance
> pinconfs legal. Then the pinconfs set the IE/OE/DATA bits in the GPIO
> registers and flip to GPIO mux.

OK if this is a driver detail it's all up to the driver writers.

> Now, going back to the pinconfs. The naming of the configs still throws
> me a little. Particularly all the "bias" settings. "BIAS_HIGH_IMPEDANCE"
> doesn't feel like a "bias" to me. What does it mean to "BIAS_DISABLE"
> a "BIAS_HIGH_IMPEDANCE"?

It is defined in the header file:

 * @PIN_CONFIG_BIAS_DISABLE: disable any pin bias on the pin, a
 *      transition from say pull-up to pull-down implies that you disable
 *      pull-up in the process, this setting disables all biasing.
 * @PIN_CONFIG_BIAS_HIGH_IMPEDANCE: the pin will be set to a high impedance
 *      mode, also know as "third-state" (tristate) or "high-Z" or "floating".
 *      On output pins this effectively disconnects the pin, which is useful
 *      if for example some other pin is going to drive the signal connected
 *      to it for a while. Pins used for input are usually always high
 *      impedance.

Being able to disable biasing on a pin, i.e. by disconnecting
any pull-up/pull-downs through software does not imply that
it is necessarily in high-Z mode after that - this depends on any
additional hardwired electronics, especially if the pin has been
tailored for a specific purpose. It may very often be high-Z
but this is a simplified view of the world and we cannot
guarantee that. Not always are all aspects of a pin under
software control.

> Should "BIAS_HIGH_IMPEDANCE" turn off "BIAS_PULL_UP"?

It is *currently* up to the driver not to allow things that make
no electrical sense, or take evasive action. But I can think of
doing code in the core actually doing this check if it makes
sense to most. Writing and testing patches for this is probably
more fruitful than just discussing it.

> My feeling is that it would be better named just
>  "CONFIG_HIGH_IMPEDANCE", making it a separate thing from
> all the other biases, akin to CONFIG_DRIVE_* and CONFIG_OUTPUT.

The documentation clearly states that this is the same as floating
or tristate.

I named it with the *BIAS* substring since a high-Z bias can
be conceived as a pull-up or pull-down with an infinite resistance.
I don't see any reason to change this, it would just lead to
a massive refactoring of all existing drivers using this, plus it
is already represented in device trees using "bias-high-impedance"
which cannot be changed as DT bindings are perpetual, changing
this would just cause chaos. I suggest to take a deep breath
and live with this terminology. :-)

> In summary, I think the two independent exclusive Boolean sets in the configs seem to be:
>
> (BIAS_DISABLE | BIAS_BUS_HOLD | BIAS_PULL_DOWN | BIAS_PULL_UP | BIAS_PULL_PIN_DEFAULT)
> ([BIAS_]HIGH_IMPEDANCE | DRIVE_PUSH_PULL | DRIVE_OPEN_DRAIN | DRIVE_OPEN_SOURCE | LOW_POWER_MODE | OUTPUT)
>
> And that's effectively how I'm treating them, for the 5 I'm handling for our hardware
> (BIAS_DISABLE|BIAS_PULL_DOWN|BIAS_PULL_UP)+(BIAS_HIGH_IMPEDANCE|OUTPUT).
>
> Does that make sense? Maybe it's hardware specific, but at least for our case,
> where the pulls are non-muxed and the drives are muxed, that separation seems logical.

We have not established any such semantic. It is also mostly
uselesss to do that through documentation or discussion about
it, as this doesn't really have any influence on what people
do in practice.

The only way to have a meaningful discussion around it is
by proposing a patch to drivers/pinctrl/pinconf-generic.c
actually *enforcing* that semantic, and have it tested and
accepted. That is the only way any such distinction would
make sense in practice.

Then we can state the obvious in the documentation...

I would really like to see such a patch, because what
you're saying makes a lot of sense, electrically speaking.
But I expect it to need some discussing and testing
and possibly debugging of existing drivers.

If you want to enforce such a semantic in a certain pin
config driver - i.e. looking for forbidden states in the
set_config() callback when looping over the requested
settings - this is of course perfectly fine.

Yours,
Linus Walleij

pinctrl - detailed gpio-mode pinconf design

[Linus Walleij]

On Fri, Dec 13, 2013 at 9:48 AM,  <kevin.bracey@broadcom.com> wrote:
> Linus Walleij wrote:

>> I'd recommend trying to keep as close to the semantics descrived in
>> <linux/pinctrl/pinconf-generic.h> and not add any implicit semantics.
>
> Yeah, but the catch is that the semantics described are on the assumption
> that pinconf is totally orthogonal to pinmux. That's not true on this platform,
> and on many others. I think we need some sort of general policy on how to
> handle conflicting requests.

Yes currently the kernel does assume these two things are orthogonal.
The implicit assumption is that if they are not, the driver gets to handle
it. It worked so far, but I'm all for a more general solution in the core
if we can come up with something elegant.

> Personally, I can see the merits of both approaches -
>
>  a) pinconf settings forcing mux to achieve the requested pin state -
> working to emulate orthogonality on non-orthogonal hardware;
>  b) pinconf settings requiring an appropriate mux to be explicitly set for
> them to be available - machines/DT need to know about the mux/conf
> interactions.
>
> Main problem with (a) - how do you get back to "normal" mux, and is
>  emulation just obfuscation?
> Main problem with (b) - can you guarantee mux/conf ordering?
>
> It sounds like you prefer (b),

I'm not very fond of either solution really. Can we think of a third
alternative where the pin control core is aware of the constraints and
tries to verify and/or solve the situation?

> and that is what my prototype does. It's what I
> originally decided, but I wasn't totally certain about how much
> pinconf-generic.h was intended to be a framework/language whose
> usage is always going to be very pin-driver-dependent, versus trying
> to make it an API trying to produce consistent behaviour.

The idea is to make the code as reusable as possible with
as many checks for physical constraints as possible, so as
to help the driver writers.

To achieve this the generic pin config tries to establish a
semantic on a best effort basis, sometimes leaving "fill in the blanks"
etc, as we're doing engineering not mathematics.

It currently has no concept of sequences or ordering however,
none whatsoever, all such details are left to the driver to
figure out. I don't know if this is good or bad.

>>> Well, the pinctrl core does process its tables in order. So in the
>>> case where a pinconf setting may interact with a pinmux setting, the
>>> ordering that the
>>> pinconf+pinmux entries get processed could be significant. Eg if
>>> PIN_CONFIG_OUTPUT requires the "gpio-mode" function selected first.
>
>> OK so we solved the similar situation where configs had to be applied
>> in some certain order for the pin config API by passing the array of
>> configs down to the driver and let is handle this.
>
> Well, ordering is already preserved by the pinctrl core. As far as I can
> see, when you're given a table of settings through pinctrl_register_map,
> you keep the ordering, and when that state is selected, the settings are
> passed down to the driver one-by-one in original order.

OK I see...

> If that can be guaranteed, then we may not need to be do anything else.

I suspect people are depending on this already to varying extent,
depending on the sophistication of the system HW.

>> So can we think of an API that gives the drivers the freedom to express
>> or behave differently here?
>
> I don't think it's necessary.
>
> For DT, the driver can control ordering in its parse routines. The sh-pfc
> driver does already put a node's function request first in its
>  dt_node_to_map(), which is what gpio-mode needs. If the core preserves
> that order, we're sorted.

OK I'm sort of aligned on this.

> For non-DT, I think it's acceptable to require that the tables should be
> ordered correctly by the board to meet the documented requirements
> of the driver - so sh-pfc board files would be required to put gpio-mode
> mux selections before
> PIN_CONFIG_OUTPUT/PIN_BIAS_CONFIG_HIGH_IMPEDANCE selections.

This seems like the most invasive action that needs to be taken is
a patch to Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
stating this, and the discussion needs to be with the DT people rather
than me I think. I cannot say much about ordering semantics in the
DTB, but to some extent I think it is already there in arrays and such.

> And either way, for glitch-free-handover, I'm also requiring that the
> configs always be specified - the mux won't be switched by the driver
> unless/until a config has been parsed and set. So in the driver - first
> pinmux "gpio-mode" primes it, and makes the output/high-impedance
> pinconfs legal. Then the pinconfs set the IE/OE/DATA bits in the GPIO
> registers and flip to GPIO mux.

OK if this is a driver detail it's all up to the driver writers.

> Now, going back to the pinconfs. The naming of the configs still throws
> me a little. Particularly all the "bias" settings. "BIAS_HIGH_IMPEDANCE"
> doesn't feel like a "bias" to me. What does it mean to "BIAS_DISABLE"
> a "BIAS_HIGH_IMPEDANCE"?

It is defined in the header file:

 * @PIN_CONFIG_BIAS_DISABLE: disable any pin bias on the pin, a
 *      transition from say pull-up to pull-down implies that you disable
 *      pull-up in the process, this setting disables all biasing.
 * @PIN_CONFIG_BIAS_HIGH_IMPEDANCE: the pin will be set to a high impedance
 *      mode, also know as "third-state" (tristate) or "high-Z" or "floating".
 *      On output pins this effectively disconnects the pin, which is useful
 *      if for example some other pin is going to drive the signal connected
 *      to it for a while. Pins used for input are usually always high
 *      impedance.

Being able to disable biasing on a pin, i.e. by disconnecting
any pull-up/pull-downs through software does not imply that
it is necessarily in high-Z mode after that - this depends on any
additional hardwired electronics, especially if the pin has been
tailored for a specific purpose. It may very often be high-Z
but this is a simplified view of the world and we cannot
guarantee that. Not always are all aspects of a pin under
software control.

> Should "BIAS_HIGH_IMPEDANCE" turn off "BIAS_PULL_UP"?

It is *currently* up to the driver not to allow things that make
no electrical sense, or take evasive action. But I can think of
doing code in the core actually doing this check if it makes
sense to most. Writing and testing patches for this is probably
more fruitful than just discussing it.

> My feeling is that it would be better named just
>  "CONFIG_HIGH_IMPEDANCE", making it a separate thing from
> all the other biases, akin to CONFIG_DRIVE_* and CONFIG_OUTPUT.

The documentation clearly states that this is the same as floating
or tristate.

I named it with the *BIAS* substring since a high-Z bias can
be conceived as a pull-up or pull-down with an infinite resistance.
I don't see any reason to change this, it would just lead to
a massive refactoring of all existing drivers using this, plus it
is already represented in device trees using "bias-high-impedance"
which cannot be changed as DT bindings are perpetual, changing
this would just cause chaos. I suggest to take a deep breath
and live with this terminology. :-)

> In summary, I think the two independent exclusive Boolean sets in the configs seem to be:
>
> (BIAS_DISABLE | BIAS_BUS_HOLD | BIAS_PULL_DOWN | BIAS_PULL_UP | BIAS_PULL_PIN_DEFAULT)
> ([BIAS_]HIGH_IMPEDANCE | DRIVE_PUSH_PULL | DRIVE_OPEN_DRAIN | DRIVE_OPEN_SOURCE | LOW_POWER_MODE | OUTPUT)
>
> And that's effectively how I'm treating them, for the 5 I'm handling for our hardware
> (BIAS_DISABLE|BIAS_PULL_DOWN|BIAS_PULL_UP)+(BIAS_HIGH_IMPEDANCE|OUTPUT).
>
> Does that make sense? Maybe it's hardware specific, but at least for our case,
> where the pulls are non-muxed and the drives are muxed, that separation seems logical.

We have not established any such semantic. It is also mostly
uselesss to do that through documentation or discussion about
it, as this doesn't really have any influence on what people
do in practice.

The only way to have a meaningful discussion around it is
by proposing a patch to drivers/pinctrl/pinconf-generic.c
actually *enforcing* that semantic, and have it tested and
accepted. That is the only way any such distinction would
make sense in practice.

Then we can state the obvious in the documentation...

I would really like to see such a patch, because what
you're saying makes a lot of sense, electrically speaking.
But I expect it to need some discussing and testing
and possibly debugging of existing drivers.

If you want to enforce such a semantic in a certain pin
config driver - i.e. looking for forbidden states in the
set_config() callback when looping over the requested
settings - this is of course perfectly fine.

Yours,
Linus Walleij

RE: pinctrl - detailed gpio-mode pinconf design

[kevin.bracey]

> Linus Walleij wrote:
> On Fri, Dec 13, 2013 at 9:48 AM,  <kevin.bracey@broadcom.com> wrote:
> > Personally, I can see the merits of both approaches -
> >
> >  a) pinconf settings forcing mux to achieve the requested pin state - 
> > working to emulate orthogonality on non-orthogonal hardware;
> >  b) pinconf settings requiring an appropriate mux to be explicitly set 
> > for them to be available - machines/DT need to know about the mux/conf 
> > interactions.
> >
> > Main problem with (a) - how do you get back to "normal" mux, and is  
> > emulation just obfuscation?
> > Main problem with (b) - can you guarantee mux/conf ordering?
> 
> I'm not very fond of either solution really. Can we think of a third alternative
> where the pin control core is aware of the constraints and tries to verify and/or
> solve the situation?

I've realised there's one existing design issue that complicates this - I'd managed to miss it up until just now, when I tried to add some fixed ties to some pins in my DT.

Pinmux is per-group only, while pinconf is per-pin. So there's no way for consumers to request "gpio-mode" for a loose pin, and the core currently couldn't do it either.

So far this has been not been an issue for all my "sleep" states which manually flip the entirety of a function group to gpio-mode, but it can't handle an odd pin hog. (And the "default no-driver states" that have also been discussed recently in another thread would also need to be applied per-pin to mop up driverless gaps in pins).

That certainly does push me away from option (b) of requiring explicit gpio-mode function selection - it rules out per-pin operation. And this problem also limits what the core could do to solve conf/mux constraints; which leaves just verification.

Maybe we just have to make do with:

c) pinconf settings that would require the driver to internally select GPIO mux can only be selected if no pinmux function (or GPIO API?) is enabled on the pin.

I don't think there's a benefit to that test being enforced by the core - you'd have to have a way for the driver to specify the restrictions, and I don't think that's any easier or more reliable than the driver checking in pinconf_ops::pin_config_set.

That would then make this pair valid for my case:

	default {
		renesas,group="foo";
		renesas,function="bar";
	};
	sleep {
		renesas,group="foo";
		output-high;
	};

as the core will disable the group's function when it selects sleep state, helpfully before processing the new state's configs; and if the pinmux_ops::disable is electrically a no-op, as it currently is for sh-pfc, then that's good for glitch-free handover.

Now, on some other pinctrl driver, where the output isn't behind the mux, they might require a pinconf setting in the default state to undo the output-high; I guess they might need to explicitly put "drive-xxx" or "bias-high-impedance" properties. But in this case, I rely on it being implied/forced by the function select. (And neither property would be accepted anyway - drive-xxx isn't implemented at all, and bias-high-impedance would be refused with a function, as we have no control).

That now feels relatively neat and logical to me.

> > Now, going back to the pinconfs. The naming of the configs still 
> > throws me a little. Particularly all the "bias" settings. "BIAS_HIGH_IMPEDANCE"
> > doesn't feel like a "bias" to me. What does it mean to "BIAS_DISABLE"
> > a "BIAS_HIGH_IMPEDANCE"?
> 
[snip]
> Being able to disable biasing on a pin, i.e. by disconnecting any pull-up/pull-downs
> through software does not imply that it is necessarily in high-Z mode after that -
> this depends on any additional hardwired electronics, especially if the pin has been
> tailored for a specific purpose. It may very often be high-Z but this is a simplified
> view of the world and we cannot guarantee that. Not always are all aspects of a pin
> under software control.

Makes sense, thanks.

> > Should "BIAS_HIGH_IMPEDANCE" turn off "BIAS_PULL_UP"?
>
> It is *currently* up to the driver not to allow things that make no electrical sense, or take evasive action.

But does that make electrical sense or not? Is this a meaningfuly request to internally combine a pull with an otherwise-high-impedance (ie non-output) pin, or nonsense because overall, an external pin can't both be pulling up and high-impedance? Are they orthogonal settings, or exclusive?

Because all these settings are booleans, (mostly) without "disables", they can't be used to toggle between states without a definition of the exclusive sets (at least as of 3.10; 3.12 has changed things - see below). I think that's what needs to be pinned down here. There will some settings that will be mutually exclusive because of hardware limitations. But some of the exclusion has to be by definition. At least before 3.12, where we have to rely on exclusion to turn off the previous state.

I'm still not fully understanding the definitions here. I just don't have a feel for what pinconf state descriptions for the "ideal" maximally-flexible hardware with the "ideal" driver should look like.

Here's the possible implementation of the pinconfs on our hardware:

bias-disable:        PU:=0 PD:=0                                   (PU/D=Pull Up/Down)
bias-pull-up:        PU:=1 PD:=0 (MUX:=GPIO OE:=0 if no func?)     (OE=Output Enable)
bias-pull-down:      PU:=0 PD:=1 (MUX:=GPIO OE:=0 if no func?)
bias-high-impedance: MUX:=GPIO OE:=0         (PU:=0 PD:=0?) 
output-low:          MUX:=GPIO OE:=1 DATA:=0 (PU:=0 PD:=0?)
output-high:         MUX:=GPIO OE:=1 DATA:=1 (PU:=0 PD:=0?)

but I still can't figure out whether setting the controls in parentheses is desirable. Either with or without all the bits in brackets we would get a logically consistent and useable system, but they're two different possible "APIs" for our hardware, and I don't know which API is intended. They lead to two different DT descriptions.

Say I wanted two pinconf-only states that switched between driving low, and weakly pulling up. Should that be expressed in the DT/pinctrl tables like:

	state_a {
		output-low;	 // OE:=1
	};
	state_b {
		bias-pull-up; // OE:=0 implied by bias-pull-up? Pulling implies not driving?
		// Or is OE:=0 implied by lack of output-xxx in this state (possible from 3.12)?
	};

Or should it look like

	state_a {
		bias-disable;  // output-low won't touch the pulls
		output-low;    // OE:=1
	};
	state_b {
		bias-pull-up;        // only sets PU/PD - pulls are orthogonal to OE
		bias-high-impedance; // necessary to set OE:=0
	};

I genuinely have no idea whether combining those two bias settings in state_b is necessary or nonsense. The former feels neater as a consumer, but the latter's orthogonality is also compelling, despite the odd-looking "bias" combo.

> If you want to enforce such a semantic in a certain pin config driver - i.e.
> looking for forbidden states in the set_config() callback when looping over
> the requested settings - this is of course perfectly fine.

Hmm, this is an interesting point.

Up to now I've been viewing configs as being applied one-at-a-time, with no indication to the driver as to when the core has actually started selecting a new state. (Which is the way it is in 3.10, which is where I'm currently working). So there the driver can't tell if seeing "pull-up" followed by "pull-down" is an illegal combination within one state, or if it's a valid switch between two states.

It also means the need for exclusions to be part of the defined API is important, as we have to know how to make sure the configs in state B override all the configs in state A.

However, since commits ad42fc6c8 ("rip out the direct pinconf API") in 3.11 and 03b054e96 ("Pass all configs to driver on pin_config_set()") in 3.12, we now have the possibility that the driver could assume each pin_config_set() call represents a complete, new state.

So implicitly, any array that lacked "output-xxx" could be interpreted to mean "output not required", rather than needing to worry about which configs actually turn off the "output-xxx" from the previous state.

Corollary - given that view, it would seem logical for the core to automatically set an empty config array if the old state had a config array for a pin/group and the new one doesn't, akin to what it does for functions.

Kevin

RE: pinctrl - detailed gpio-mode pinconf design

[kevin.bracey]

> Linus Walleij wrote:
> On Fri, Dec 13, 2013 at 9:48 AM,  <kevin.bracey@broadcom.com> wrote:
> > Personally, I can see the merits of both approaches -
> >
> >  a) pinconf settings forcing mux to achieve the requested pin state - 
> > working to emulate orthogonality on non-orthogonal hardware;
> >  b) pinconf settings requiring an appropriate mux to be explicitly set 
> > for them to be available - machines/DT need to know about the mux/conf 
> > interactions.
> >
> > Main problem with (a) - how do you get back to "normal" mux, and is  
> > emulation just obfuscation?
> > Main problem with (b) - can you guarantee mux/conf ordering?
> 
> I'm not very fond of either solution really. Can we think of a third alternative
> where the pin control core is aware of the constraints and tries to verify and/or
> solve the situation?

I've realised there's one existing design issue that complicates this - I'd managed to miss it up until just now, when I tried to add some fixed ties to some pins in my DT.

Pinmux is per-group only, while pinconf is per-pin. So there's no way for consumers to request "gpio-mode" for a loose pin, and the core currently couldn't do it either.

So far this has been not been an issue for all my "sleep" states which manually flip the entirety of a function group to gpio-mode, but it can't handle an odd pin hog. (And the "default no-driver states" that have also been discussed recently in another thread would also need to be applied per-pin to mop up driverless gaps in pins).

That certainly does push me away from option (b) of requiring explicit gpio-mode function selection - it rules out per-pin operation. And this problem also limits what the core could do to solve conf/mux constraints; which leaves just verification.

Maybe we just have to make do with:

c) pinconf settings that would require the driver to internally select GPIO mux can only be selected if no pinmux function (or GPIO API?) is enabled on the pin.

I don't think there's a benefit to that test being enforced by the core - you'd have to have a way for the driver to specify the restrictions, and I don't think that's any easier or more reliable than the driver checking in pinconf_ops::pin_config_set.

That would then make this pair valid for my case:

	default {
		renesas,group="foo";
		renesas,function="bar";
	};
	sleep {
		renesas,group="foo";
		output-high;
	};

as the core will disable the group's function when it selects sleep state, helpfully before processing the new state's configs; and if the pinmux_ops::disable is electrically a no-op, as it currently is for sh-pfc, then that's good for glitch-free handover.

Now, on some other pinctrl driver, where the output isn't behind the mux, they might require a pinconf setting in the default state to undo the output-high; I guess they might need to explicitly put "drive-xxx" or "bias-high-impedance" properties. But in this case, I rely on it being implied/forced by the function select. (And neither property would be accepted anyway - drive-xxx isn't implemented at all, and bias-high-impedance would be refused with a function, as we have no control).

That now feels relatively neat and logical to me.

> > Now, going back to the pinconfs. The naming of the configs still 
> > throws me a little. Particularly all the "bias" settings. "BIAS_HIGH_IMPEDANCE"
> > doesn't feel like a "bias" to me. What does it mean to "BIAS_DISABLE"
> > a "BIAS_HIGH_IMPEDANCE"?
> 
[snip]
> Being able to disable biasing on a pin, i.e. by disconnecting any pull-up/pull-downs
> through software does not imply that it is necessarily in high-Z mode after that -
> this depends on any additional hardwired electronics, especially if the pin has been
> tailored for a specific purpose. It may very often be high-Z but this is a simplified
> view of the world and we cannot guarantee that. Not always are all aspects of a pin
> under software control.

Makes sense, thanks.

> > Should "BIAS_HIGH_IMPEDANCE" turn off "BIAS_PULL_UP"?
>
> It is *currently* up to the driver not to allow things that make no electrical sense, or take evasive action.

But does that make electrical sense or not? Is this a meaningfuly request to internally combine a pull with an otherwise-high-impedance (ie non-output) pin, or nonsense because overall, an external pin can't both be pulling up and high-impedance? Are they orthogonal settings, or exclusive?

Because all these settings are booleans, (mostly) without "disables", they can't be used to toggle between states without a definition of the exclusive sets (at least as of 3.10; 3.12 has changed things - see below). I think that's what needs to be pinned down here. There will some settings that will be mutually exclusive because of hardware limitations. But some of the exclusion has to be by definition. At least before 3.12, where we have to rely on exclusion to turn off the previous state.

I'm still not fully understanding the definitions here. I just don't have a feel for what pinconf state descriptions for the "ideal" maximally-flexible hardware with the "ideal" driver should look like.

Here's the possible implementation of the pinconfs on our hardware:

bias-disable:        PU:=0 PD:=0                                   (PU/D=Pull Up/Down)
bias-pull-up:        PU:=1 PD:=0 (MUX:=GPIO OE:=0 if no func?)     (OE=Output Enable)
bias-pull-down:      PU:=0 PD:=1 (MUX:=GPIO OE:=0 if no func?)
bias-high-impedance: MUX:=GPIO OE:=0         (PU:=0 PD:=0?) 
output-low:          MUX:=GPIO OE:=1 DATA:=0 (PU:=0 PD:=0?)
output-high:         MUX:=GPIO OE:=1 DATA:=1 (PU:=0 PD:=0?)

but I still can't figure out whether setting the controls in parentheses is desirable. Either with or without all the bits in brackets we would get a logically consistent and useable system, but they're two different possible "APIs" for our hardware, and I don't know which API is intended. They lead to two different DT descriptions.

Say I wanted two pinconf-only states that switched between driving low, and weakly pulling up. Should that be expressed in the DT/pinctrl tables like:

	state_a {
		output-low;	 // OE:=1
	};
	state_b {
		bias-pull-up; // OE:=0 implied by bias-pull-up? Pulling implies not driving?
		// Or is OE:=0 implied by lack of output-xxx in this state (possible from 3.12)?
	};

Or should it look like

	state_a {
		bias-disable;  // output-low won't touch the pulls
		output-low;    // OE:=1
	};
	state_b {
		bias-pull-up;        // only sets PU/PD - pulls are orthogonal to OE
		bias-high-impedance; // necessary to set OE:=0
	};

I genuinely have no idea whether combining those two bias settings in state_b is necessary or nonsense. The former feels neater as a consumer, but the latter's orthogonality is also compelling, despite the odd-looking "bias" combo.

> If you want to enforce such a semantic in a certain pin config driver - i.e.
> looking for forbidden states in the set_config() callback when looping over
> the requested settings - this is of course perfectly fine.

Hmm, this is an interesting point.

Up to now I've been viewing configs as being applied one-at-a-time, with no indication to the driver as to when the core has actually started selecting a new state. (Which is the way it is in 3.10, which is where I'm currently working). So there the driver can't tell if seeing "pull-up" followed by "pull-down" is an illegal combination within one state, or if it's a valid switch between two states.

It also means the need for exclusions to be part of the defined API is important, as we have to know how to make sure the configs in state B override all the configs in state A.

However, since commits ad42fc6c8 ("rip out the direct pinconf API") in 3.11 and 03b054e96 ("Pass all configs to driver on pin_config_set()") in 3.12, we now have the possibility that the driver could assume each pin_config_set() call represents a complete, new state.

So implicitly, any array that lacked "output-xxx" could be interpreted to mean "output not required", rather than needing to worry about which configs actually turn off the "output-xxx" from the previous state.

Corollary - given that view, it would seem logical for the core to automatically set an empty config array if the old state had a config array for a pin/group and the new one doesn't, akin to what it does for functions.

Kevin

pinctrl - detailed gpio-mode pinconf design

[kevin.bracey at broadcom.com]

> Linus Walleij wrote:
> On Fri, Dec 13, 2013 at 9:48 AM,  <kevin.bracey@broadcom.com> wrote:
> > Personally, I can see the merits of both approaches -
> >
> >  a) pinconf settings forcing mux to achieve the requested pin state - 
> > working to emulate orthogonality on non-orthogonal hardware;
> >  b) pinconf settings requiring an appropriate mux to be explicitly set 
> > for them to be available - machines/DT need to know about the mux/conf 
> > interactions.
> >
> > Main problem with (a) - how do you get back to "normal" mux, and is  
> > emulation just obfuscation?
> > Main problem with (b) - can you guarantee mux/conf ordering?
> 
> I'm not very fond of either solution really. Can we think of a third alternative
> where the pin control core is aware of the constraints and tries to verify and/or
> solve the situation?

I've realised there's one existing design issue that complicates this - I'd managed to miss it up until just now, when I tried to add some fixed ties to some pins in my DT.

Pinmux is per-group only, while pinconf is per-pin. So there's no way for consumers to request "gpio-mode" for a loose pin, and the core currently couldn't do it either.

So far this has been not been an issue for all my "sleep" states which manually flip the entirety of a function group to gpio-mode, but it can't handle an odd pin hog. (And the "default no-driver states" that have also been discussed recently in another thread would also need to be applied per-pin to mop up driverless gaps in pins).

That certainly does push me away from option (b) of requiring explicit gpio-mode function selection - it rules out per-pin operation. And this problem also limits what the core could do to solve conf/mux constraints; which leaves just verification.

Maybe we just have to make do with:

c) pinconf settings that would require the driver to internally select GPIO mux can only be selected if no pinmux function (or GPIO API?) is enabled on the pin.

I don't think there's a benefit to that test being enforced by the core - you'd have to have a way for the driver to specify the restrictions, and I don't think that's any easier or more reliable than the driver checking in pinconf_ops::pin_config_set.

That would then make this pair valid for my case:

	default {
		renesas,group="foo";
		renesas,function="bar";
	};
	sleep {
		renesas,group="foo";
		output-high;
	};

as the core will disable the group's function when it selects sleep state, helpfully before processing the new state's configs; and if the pinmux_ops::disable is electrically a no-op, as it currently is for sh-pfc, then that's good for glitch-free handover.

Now, on some other pinctrl driver, where the output isn't behind the mux, they might require a pinconf setting in the default state to undo the output-high; I guess they might need to explicitly put "drive-xxx" or "bias-high-impedance" properties. But in this case, I rely on it being implied/forced by the function select. (And neither property would be accepted anyway - drive-xxx isn't implemented at all, and bias-high-impedance would be refused with a function, as we have no control).

That now feels relatively neat and logical to me.

> > Now, going back to the pinconfs. The naming of the configs still 
> > throws me a little. Particularly all the "bias" settings. "BIAS_HIGH_IMPEDANCE"
> > doesn't feel like a "bias" to me. What does it mean to "BIAS_DISABLE"
> > a "BIAS_HIGH_IMPEDANCE"?
> 
[snip]
> Being able to disable biasing on a pin, i.e. by disconnecting any pull-up/pull-downs
> through software does not imply that it is necessarily in high-Z mode after that -
> this depends on any additional hardwired electronics, especially if the pin has been
> tailored for a specific purpose. It may very often be high-Z but this is a simplified
> view of the world and we cannot guarantee that. Not always are all aspects of a pin
> under software control.

Makes sense, thanks.

> > Should "BIAS_HIGH_IMPEDANCE" turn off "BIAS_PULL_UP"?
>
> It is *currently* up to the driver not to allow things that make no electrical sense, or take evasive action.

But does that make electrical sense or not? Is this a meaningfuly request to internally combine a pull with an otherwise-high-impedance (ie non-output) pin, or nonsense because overall, an external pin can't both be pulling up and high-impedance? Are they orthogonal settings, or exclusive?

Because all these settings are booleans, (mostly) without "disables", they can't be used to toggle between states without a definition of the exclusive sets (at least as of 3.10; 3.12 has changed things - see below). I think that's what needs to be pinned down here. There will some settings that will be mutually exclusive because of hardware limitations. But some of the exclusion has to be by definition. At least before 3.12, where we have to rely on exclusion to turn off the previous state.

I'm still not fully understanding the definitions here. I just don't have a feel for what pinconf state descriptions for the "ideal" maximally-flexible hardware with the "ideal" driver should look like.

Here's the possible implementation of the pinconfs on our hardware:

bias-disable:        PU:=0 PD:=0                                   (PU/D=Pull Up/Down)
bias-pull-up:        PU:=1 PD:=0 (MUX:=GPIO OE:=0 if no func?)     (OE=Output Enable)
bias-pull-down:      PU:=0 PD:=1 (MUX:=GPIO OE:=0 if no func?)
bias-high-impedance: MUX:=GPIO OE:=0         (PU:=0 PD:=0?) 
output-low:          MUX:=GPIO OE:=1 DATA:=0 (PU:=0 PD:=0?)
output-high:         MUX:=GPIO OE:=1 DATA:=1 (PU:=0 PD:=0?)

but I still can't figure out whether setting the controls in parentheses is desirable. Either with or without all the bits in brackets we would get a logically consistent and useable system, but they're two different possible "APIs" for our hardware, and I don't know which API is intended. They lead to two different DT descriptions.

Say I wanted two pinconf-only states that switched between driving low, and weakly pulling up. Should that be expressed in the DT/pinctrl tables like:

	state_a {
		output-low;	 // OE:=1
	};
	state_b {
		bias-pull-up; // OE:=0 implied by bias-pull-up? Pulling implies not driving?
		// Or is OE:=0 implied by lack of output-xxx in this state (possible from 3.12)?
	};

Or should it look like

	state_a {
		bias-disable;  // output-low won't touch the pulls
		output-low;    // OE:=1
	};
	state_b {
		bias-pull-up;        // only sets PU/PD - pulls are orthogonal to OE
		bias-high-impedance; // necessary to set OE:=0
	};

I genuinely have no idea whether combining those two bias settings in state_b is necessary or nonsense. The former feels neater as a consumer, but the latter's orthogonality is also compelling, despite the odd-looking "bias" combo.

> If you want to enforce such a semantic in a certain pin config driver - i.e.
> looking for forbidden states in the set_config() callback when looping over
> the requested settings - this is of course perfectly fine.

Hmm, this is an interesting point.

Up to now I've been viewing configs as being applied one-at-a-time, with no indication to the driver as to when the core has actually started selecting a new state. (Which is the way it is in 3.10, which is where I'm currently working). So there the driver can't tell if seeing "pull-up" followed by "pull-down" is an illegal combination within one state, or if it's a valid switch between two states.

It also means the need for exclusions to be part of the defined API is important, as we have to know how to make sure the configs in state B override all the configs in state A.

However, since commits ad42fc6c8 ("rip out the direct pinconf API") in 3.11 and 03b054e96 ("Pass all configs to driver on pin_config_set()") in 3.12, we now have the possibility that the driver could assume each pin_config_set() call represents a complete, new state.

So implicitly, any array that lacked "output-xxx" could be interpreted to mean "output not required", rather than needing to worry about which configs actually turn off the "output-xxx" from the previous state.

Corollary - given that view, it would seem logical for the core to automatically set an empty config array if the old state had a config array for a pin/group and the new one doesn't, akin to what it does for functions.

Kevin

Re: pinctrl - detailed gpio-mode pinconf design

[Linus Walleij]

On Thu, Dec 19, 2013 at 11:49 AM,  <kevin.bracey@broadcom.com> wrote:
>> Linus Walleij wrote:

>> > Main problem with (a) - how do you get back to "normal" mux, and is
>> > emulation just obfuscation?
>> > Main problem with (b) - can you guarantee mux/conf ordering?
>>
>> I'm not very fond of either solution really. Can we think of a third alternative
>> where the pin control core is aware of the constraints and tries to verify and/or
>> solve the situation?
>
> I've realised there's one existing design issue that complicates this - I'd managed to miss it up until just now, when I tried to add some fixed ties to some pins in my DT.
>
> Pinmux is per-group only, while pinconf is per-pin. So there's no way for consumers to request "gpio-mode" for a loose pin, and the core currently couldn't do it either.

I don't know what you mean by this. It is possible to use the same pin for
GPIO and a certain device at the same time even, since this may be possible
in hardware (e.g. to use a GPIO to "spy" on a device line).

Also define what you mean by "gpio-mode", as can be seen from
Documentation/pinctrl.txt section "GPIO mode pitfalls" this is a
dangerously ambigous term.

> So far this has been not been an issue for all my "sleep" states which manually

What do you mean by "manually" here? That is a very unclear term that
I prefer that we do not use at all in the kernel. There is no man inside
the system.

> flip the entirety of a function group to gpio-mode, but it can't handle an odd
> pin hog. (And the "default no-driver states" that have also been discussed
> recently in another thread would also need to be applied per-pin to mop
> up driverless gaps in pins).

Hm I might understand what a "default no-driver state" is but ... sorry
I have a hard time following this paragraph :-/

What is an "odd pin hog"?

> Maybe we just have to make do with:
>
> c) pinconf settings that would require the driver to internally select GPIO mux can only be selected if no pinmux function (or GPIO API?) is enabled on the pin.

I don't understand what you mean by this, but I guess you're after
som driver intrinsic detail so let's see the patch so we can see
clearly what you mean here.

>> > Should "BIAS_HIGH_IMPEDANCE" turn off "BIAS_PULL_UP"?
>>
>> It is *currently* up to the driver not to allow things that make no electrical sense, or take evasive action.
>
> But does that make electrical sense or not?

It makes sense if the HW has two knobs: one for setting pull up
and one for setting the pin into High-Z.

If it has only one or the other, there is not much it can do anyway.

> Is this a meaningfuly request to internally combine a pull with an
> otherwise-high-impedance (ie non-output) pin, or nonsense because
> overall, an external pin can't both be pulling up and high-impedance?
> Are they orthogonal settings, or exclusive?

They may be orthogonal in the hardware register sense, i.e. you
can turn on both at the same time by setting two bits, but still they are
not orthogonal in the electrical sense, because the outcome is
ambigous. What happens if you enable both at the same time
must be understood by reading the HW documentation or talking
to the HW engineer who implemented it.

> Because all these settings are booleans, (mostly) without "disables",
> they can't be used to toggle between states without a definition of
> the exclusive sets (at least as of 3.10; 3.12 has changed things -
> see below). I think that's what needs to be pinned down here. There
> will some settings that will be mutually exclusive because of hardware
> limitations. But some of the exclusion has to be by definition. At least
> before 3.12, where we have to rely on exclusion to turn off the previous
> state.

OK I'm following. How do you plan to implement this?

> I'm still not fully understanding the definitions here. I just don't
> have a feel for what pinconf state descriptions for the "ideal"
> maximally-flexible hardware with the "ideal" driver should look like.

As no such hardware exist this is a question relating to the
ideal world.

Bringing such mental figures into the resoning invokes Plato's
metaphor of the Cave and the debate between philosophical idealism
and philisophical realism.
http://en.wikipedia.org/wiki/Idealism
http://en.wikipedia.org/wiki/Philosophical_realism
This relates to mathematical formalism vs. Platonism and whether
our mental models are inventions or discoveries.

I don't know such stuff! All I want to do is manage this subsystem
for the set of pin controllers out there and close the necessary
gaps to make it useful in practice. This is called pragmatism.
http://en.wikipedia.org/wiki/Pragmatism

So let's keep a pragmatic stance - rough consensus and running
code.

> Here's the possible implementation of the pinconfs on our hardware:
>
> bias-disable:        PU:=0 PD:=0                                   (PU/D=Pull Up/Down)
> bias-pull-up:        PU:=1 PD:=0 (MUX:=GPIO OE:=0 if no func?)     (OE=Output Enable)
> bias-pull-down:      PU:=0 PD:=1 (MUX:=GPIO OE:=0 if no func?)
> bias-high-impedance: MUX:=GPIO OE:=0         (PU:=0 PD:=0?)
> output-low:          MUX:=GPIO OE:=1 DATA:=0 (PU:=0 PD:=0?)
> output-high:         MUX:=GPIO OE:=1 DATA:=1 (PU:=0 PD:=0?)
>
> but I still can't figure out whether setting the controls in parentheses is desirable.

The documentation should say, else I think you should ask a renesas
HW engineer about that. I don't know how that hardware will react
to some of these settings...

> Either with or without all the bits in brackets we would get a logically
> consistent and useable system, but they're two different possible
> "APIs" for our hardware, and I don't know which API is intended.

What are the two APIs you're referring to here? Pinctrl and
GPIO?

Since both aspects are joined in the subsystem and even in the
same driver file (I think) it is perfectly possible to handle this
in the PFC driver.

> They lead to two different DT descriptions.
>
> Say I wanted two pinconf-only states that switched between driving low, and weakly pulling up. Should that be expressed in the DT/pinctrl tables like:
>
>         state_a {
>                 output-low;      // OE:=1
>         };
>         state_b {
>                 bias-pull-up; // OE:=0 implied by bias-pull-up? Pulling implies not driving?
>                 // Or is OE:=0 implied by lack of output-xxx in this state (possible from 3.12)?
>         };
>
> Or should it look like
>
>         state_a {
>                 bias-disable;  // output-low won't touch the pulls
>                 output-low;    // OE:=1
>         };
>         state_b {
>                 bias-pull-up;        // only sets PU/PD - pulls are orthogonal to OE
>                 bias-high-impedance; // necessary to set OE:=0
>         };

From a DT code parsing point of view I think both are OK?

I think the best approach is that the driver check both current
and requested state in the .pin_config_set() callback.

> I genuinely have no idea whether combining those two bias settings in state_b is
> necessary or nonsense.

You need to know this first before developing code for
driving the hardware.

> The former feels neater as a consumer, but the latter's orthogonality is also
> compelling, despite the odd-looking "bias" combo.

The framework does not constrain which version you use, and
any hard constraints should probably be in the driver.

Maybe the generic pinconf core could print warnings about
strange configurations though, maybe as a debug option?

> Up to now I've been viewing configs as being applied one-at-a-time,

They are *not* applied one-at-a-time. The driver is given an
array of configs and can inspect this array and take decisions
on whether it is possible to satisfy this config and in case it
cannot it may return an error code.

> with no indication to the driver as to when the core has actually
> started selecting a new state. (Which is the way it is in 3.10, which
> is where I'm currently working).

So don't use such ancient codebase, I'm developing v3.14 now.

> It also means the need for exclusions to be part of the defined
> API is important, as we have to know how to make sure the
> configs in state B override all the configs in state A.

As mentioned this is for the driver to handle.

> Corollary - given that view, it would seem logical for the core to
> automatically set an empty config array if the old state had a
> config array for a pin/group and the new one doesn't, akin to
> what it does for functions.

Please elaborate with an example what you mean by this.

Yours,
Linus Walleij

Re: pinctrl - detailed gpio-mode pinconf design

[Linus Walleij]

On Thu, Dec 19, 2013 at 11:49 AM,  <kevin.bracey@broadcom.com> wrote:
>> Linus Walleij wrote:

>> > Main problem with (a) - how do you get back to "normal" mux, and is
>> > emulation just obfuscation?
>> > Main problem with (b) - can you guarantee mux/conf ordering?
>>
>> I'm not very fond of either solution really. Can we think of a third alternative
>> where the pin control core is aware of the constraints and tries to verify and/or
>> solve the situation?
>
> I've realised there's one existing design issue that complicates this - I'd managed to miss it up until just now, when I tried to add some fixed ties to some pins in my DT.
>
> Pinmux is per-group only, while pinconf is per-pin. So there's no way for consumers to request "gpio-mode" for a loose pin, and the core currently couldn't do it either.

I don't know what you mean by this. It is possible to use the same pin for
GPIO and a certain device at the same time even, since this may be possible
in hardware (e.g. to use a GPIO to "spy" on a device line).

Also define what you mean by "gpio-mode", as can be seen from
Documentation/pinctrl.txt section "GPIO mode pitfalls" this is a
dangerously ambigous term.

> So far this has been not been an issue for all my "sleep" states which manually

What do you mean by "manually" here? That is a very unclear term that
I prefer that we do not use at all in the kernel. There is no man inside
the system.

> flip the entirety of a function group to gpio-mode, but it can't handle an odd
> pin hog. (And the "default no-driver states" that have also been discussed
> recently in another thread would also need to be applied per-pin to mop
> up driverless gaps in pins).

Hm I might understand what a "default no-driver state" is but ... sorry
I have a hard time following this paragraph :-/

What is an "odd pin hog"?

> Maybe we just have to make do with:
>
> c) pinconf settings that would require the driver to internally select GPIO mux can only be selected if no pinmux function (or GPIO API?) is enabled on the pin.

I don't understand what you mean by this, but I guess you're after
som driver intrinsic detail so let's see the patch so we can see
clearly what you mean here.

>> > Should "BIAS_HIGH_IMPEDANCE" turn off "BIAS_PULL_UP"?
>>
>> It is *currently* up to the driver not to allow things that make no electrical sense, or take evasive action.
>
> But does that make electrical sense or not?

It makes sense if the HW has two knobs: one for setting pull up
and one for setting the pin into High-Z.

If it has only one or the other, there is not much it can do anyway.

> Is this a meaningfuly request to internally combine a pull with an
> otherwise-high-impedance (ie non-output) pin, or nonsense because
> overall, an external pin can't both be pulling up and high-impedance?
> Are they orthogonal settings, or exclusive?

They may be orthogonal in the hardware register sense, i.e. you
can turn on both at the same time by setting two bits, but still they are
not orthogonal in the electrical sense, because the outcome is
ambigous. What happens if you enable both at the same time
must be understood by reading the HW documentation or talking
to the HW engineer who implemented it.

> Because all these settings are booleans, (mostly) without "disables",
> they can't be used to toggle between states without a definition of
> the exclusive sets (at least as of 3.10; 3.12 has changed things -
> see below). I think that's what needs to be pinned down here. There
> will some settings that will be mutually exclusive because of hardware
> limitations. But some of the exclusion has to be by definition. At least
> before 3.12, where we have to rely on exclusion to turn off the previous
> state.

OK I'm following. How do you plan to implement this?

> I'm still not fully understanding the definitions here. I just don't
> have a feel for what pinconf state descriptions for the "ideal"
> maximally-flexible hardware with the "ideal" driver should look like.

As no such hardware exist this is a question relating to the
ideal world.

Bringing such mental figures into the resoning invokes Plato's
metaphor of the Cave and the debate between philosophical idealism
and philisophical realism.
http://en.wikipedia.org/wiki/Idealism
http://en.wikipedia.org/wiki/Philosophical_realism
This relates to mathematical formalism vs. Platonism and whether
our mental models are inventions or discoveries.

I don't know such stuff! All I want to do is manage this subsystem
for the set of pin controllers out there and close the necessary
gaps to make it useful in practice. This is called pragmatism.
http://en.wikipedia.org/wiki/Pragmatism

So let's keep a pragmatic stance - rough consensus and running
code.

> Here's the possible implementation of the pinconfs on our hardware:
>
> bias-disable:        PU:=0 PD:=0                                   (PU/D=Pull Up/Down)
> bias-pull-up:        PU:=1 PD:=0 (MUX:=GPIO OE:=0 if no func?)     (OE=Output Enable)
> bias-pull-down:      PU:=0 PD:=1 (MUX:=GPIO OE:=0 if no func?)
> bias-high-impedance: MUX:=GPIO OE:=0         (PU:=0 PD:=0?)
> output-low:          MUX:=GPIO OE:=1 DATA:=0 (PU:=0 PD:=0?)
> output-high:         MUX:=GPIO OE:=1 DATA:=1 (PU:=0 PD:=0?)
>
> but I still can't figure out whether setting the controls in parentheses is desirable.

The documentation should say, else I think you should ask a renesas
HW engineer about that. I don't know how that hardware will react
to some of these settings...

> Either with or without all the bits in brackets we would get a logically
> consistent and useable system, but they're two different possible
> "APIs" for our hardware, and I don't know which API is intended.

What are the two APIs you're referring to here? Pinctrl and
GPIO?

Since both aspects are joined in the subsystem and even in the
same driver file (I think) it is perfectly possible to handle this
in the PFC driver.

> They lead to two different DT descriptions.
>
> Say I wanted two pinconf-only states that switched between driving low, and weakly pulling up. Should that be expressed in the DT/pinctrl tables like:
>
>         state_a {
>                 output-low;      // OE:=1
>         };
>         state_b {
>                 bias-pull-up; // OE:=0 implied by bias-pull-up? Pulling implies not driving?
>                 // Or is OE:=0 implied by lack of output-xxx in this state (possible from 3.12)?
>         };
>
> Or should it look like
>
>         state_a {
>                 bias-disable;  // output-low won't touch the pulls
>                 output-low;    // OE:=1
>         };
>         state_b {
>                 bias-pull-up;        // only sets PU/PD - pulls are orthogonal to OE
>                 bias-high-impedance; // necessary to set OE:=0
>         };

>From a DT code parsing point of view I think both are OK?

I think the best approach is that the driver check both current
and requested state in the .pin_config_set() callback.

> I genuinely have no idea whether combining those two bias settings in state_b is
> necessary or nonsense.

You need to know this first before developing code for
driving the hardware.

> The former feels neater as a consumer, but the latter's orthogonality is also
> compelling, despite the odd-looking "bias" combo.

The framework does not constrain which version you use, and
any hard constraints should probably be in the driver.

Maybe the generic pinconf core could print warnings about
strange configurations though, maybe as a debug option?

> Up to now I've been viewing configs as being applied one-at-a-time,

They are *not* applied one-at-a-time. The driver is given an
array of configs and can inspect this array and take decisions
on whether it is possible to satisfy this config and in case it
cannot it may return an error code.

> with no indication to the driver as to when the core has actually
> started selecting a new state. (Which is the way it is in 3.10, which
> is where I'm currently working).

So don't use such ancient codebase, I'm developing v3.14 now.

> It also means the need for exclusions to be part of the defined
> API is important, as we have to know how to make sure the
> configs in state B override all the configs in state A.

As mentioned this is for the driver to handle.

> Corollary - given that view, it would seem logical for the core to
> automatically set an empty config array if the old state had a
> config array for a pin/group and the new one doesn't, akin to
> what it does for functions.

Please elaborate with an example what you mean by this.

Yours,
Linus Walleij

pinctrl - detailed gpio-mode pinconf design

[Linus Walleij]

On Thu, Dec 19, 2013 at 11:49 AM,  <kevin.bracey@broadcom.com> wrote:
>> Linus Walleij wrote:

>> > Main problem with (a) - how do you get back to "normal" mux, and is
>> > emulation just obfuscation?
>> > Main problem with (b) - can you guarantee mux/conf ordering?
>>
>> I'm not very fond of either solution really. Can we think of a third alternative
>> where the pin control core is aware of the constraints and tries to verify and/or
>> solve the situation?
>
> I've realised there's one existing design issue that complicates this - I'd managed to miss it up until just now, when I tried to add some fixed ties to some pins in my DT.
>
> Pinmux is per-group only, while pinconf is per-pin. So there's no way for consumers to request "gpio-mode" for a loose pin, and the core currently couldn't do it either.

I don't know what you mean by this. It is possible to use the same pin for
GPIO and a certain device at the same time even, since this may be possible
in hardware (e.g. to use a GPIO to "spy" on a device line).

Also define what you mean by "gpio-mode", as can be seen from
Documentation/pinctrl.txt section "GPIO mode pitfalls" this is a
dangerously ambigous term.

> So far this has been not been an issue for all my "sleep" states which manually

What do you mean by "manually" here? That is a very unclear term that
I prefer that we do not use at all in the kernel. There is no man inside
the system.

> flip the entirety of a function group to gpio-mode, but it can't handle an odd
> pin hog. (And the "default no-driver states" that have also been discussed
> recently in another thread would also need to be applied per-pin to mop
> up driverless gaps in pins).

Hm I might understand what a "default no-driver state" is but ... sorry
I have a hard time following this paragraph :-/

What is an "odd pin hog"?

> Maybe we just have to make do with:
>
> c) pinconf settings that would require the driver to internally select GPIO mux can only be selected if no pinmux function (or GPIO API?) is enabled on the pin.

I don't understand what you mean by this, but I guess you're after
som driver intrinsic detail so let's see the patch so we can see
clearly what you mean here.

>> > Should "BIAS_HIGH_IMPEDANCE" turn off "BIAS_PULL_UP"?
>>
>> It is *currently* up to the driver not to allow things that make no electrical sense, or take evasive action.
>
> But does that make electrical sense or not?

It makes sense if the HW has two knobs: one for setting pull up
and one for setting the pin into High-Z.

If it has only one or the other, there is not much it can do anyway.

> Is this a meaningfuly request to internally combine a pull with an
> otherwise-high-impedance (ie non-output) pin, or nonsense because
> overall, an external pin can't both be pulling up and high-impedance?
> Are they orthogonal settings, or exclusive?

They may be orthogonal in the hardware register sense, i.e. you
can turn on both at the same time by setting two bits, but still they are
not orthogonal in the electrical sense, because the outcome is
ambigous. What happens if you enable both at the same time
must be understood by reading the HW documentation or talking
to the HW engineer who implemented it.

> Because all these settings are booleans, (mostly) without "disables",
> they can't be used to toggle between states without a definition of
> the exclusive sets (at least as of 3.10; 3.12 has changed things -
> see below). I think that's what needs to be pinned down here. There
> will some settings that will be mutually exclusive because of hardware
> limitations. But some of the exclusion has to be by definition. At least
> before 3.12, where we have to rely on exclusion to turn off the previous
> state.

OK I'm following. How do you plan to implement this?

> I'm still not fully understanding the definitions here. I just don't
> have a feel for what pinconf state descriptions for the "ideal"
> maximally-flexible hardware with the "ideal" driver should look like.

As no such hardware exist this is a question relating to the
ideal world.

Bringing such mental figures into the resoning invokes Plato's
metaphor of the Cave and the debate between philosophical idealism
and philisophical realism.
http://en.wikipedia.org/wiki/Idealism
http://en.wikipedia.org/wiki/Philosophical_realism
This relates to mathematical formalism vs. Platonism and whether
our mental models are inventions or discoveries.

I don't know such stuff! All I want to do is manage this subsystem
for the set of pin controllers out there and close the necessary
gaps to make it useful in practice. This is called pragmatism.
http://en.wikipedia.org/wiki/Pragmatism

So let's keep a pragmatic stance - rough consensus and running
code.

> Here's the possible implementation of the pinconfs on our hardware:
>
> bias-disable:        PU:=0 PD:=0                                   (PU/D=Pull Up/Down)
> bias-pull-up:        PU:=1 PD:=0 (MUX:=GPIO OE:=0 if no func?)     (OE=Output Enable)
> bias-pull-down:      PU:=0 PD:=1 (MUX:=GPIO OE:=0 if no func?)
> bias-high-impedance: MUX:=GPIO OE:=0         (PU:=0 PD:=0?)
> output-low:          MUX:=GPIO OE:=1 DATA:=0 (PU:=0 PD:=0?)
> output-high:         MUX:=GPIO OE:=1 DATA:=1 (PU:=0 PD:=0?)
>
> but I still can't figure out whether setting the controls in parentheses is desirable.

The documentation should say, else I think you should ask a renesas
HW engineer about that. I don't know how that hardware will react
to some of these settings...

> Either with or without all the bits in brackets we would get a logically
> consistent and useable system, but they're two different possible
> "APIs" for our hardware, and I don't know which API is intended.

What are the two APIs you're referring to here? Pinctrl and
GPIO?

Since both aspects are joined in the subsystem and even in the
same driver file (I think) it is perfectly possible to handle this
in the PFC driver.

> They lead to two different DT descriptions.
>
> Say I wanted two pinconf-only states that switched between driving low, and weakly pulling up. Should that be expressed in the DT/pinctrl tables like:
>
>         state_a {
>                 output-low;      // OE:=1
>         };
>         state_b {
>                 bias-pull-up; // OE:=0 implied by bias-pull-up? Pulling implies not driving?
>                 // Or is OE:=0 implied by lack of output-xxx in this state (possible from 3.12)?
>         };
>
> Or should it look like
>
>         state_a {
>                 bias-disable;  // output-low won't touch the pulls
>                 output-low;    // OE:=1
>         };
>         state_b {
>                 bias-pull-up;        // only sets PU/PD - pulls are orthogonal to OE
>                 bias-high-impedance; // necessary to set OE:=0
>         };

>From a DT code parsing point of view I think both are OK?

I think the best approach is that the driver check both current
and requested state in the .pin_config_set() callback.

> I genuinely have no idea whether combining those two bias settings in state_b is
> necessary or nonsense.

You need to know this first before developing code for
driving the hardware.

> The former feels neater as a consumer, but the latter's orthogonality is also
> compelling, despite the odd-looking "bias" combo.

The framework does not constrain which version you use, and
any hard constraints should probably be in the driver.

Maybe the generic pinconf core could print warnings about
strange configurations though, maybe as a debug option?

> Up to now I've been viewing configs as being applied one-at-a-time,

They are *not* applied one-at-a-time. The driver is given an
array of configs and can inspect this array and take decisions
on whether it is possible to satisfy this config and in case it
cannot it may return an error code.

> with no indication to the driver as to when the core has actually
> started selecting a new state. (Which is the way it is in 3.10, which
> is where I'm currently working).

So don't use such ancient codebase, I'm developing v3.14 now.

> It also means the need for exclusions to be part of the defined
> API is important, as we have to know how to make sure the
> configs in state B override all the configs in state A.

As mentioned this is for the driver to handle.

> Corollary - given that view, it would seem logical for the core to
> automatically set an empty config array if the old state had a
> config array for a pin/group and the new one doesn't, akin to
> what it does for functions.

Please elaborate with an example what you mean by this.

Yours,
Linus Walleij