Issue with gpio and pinmux

Issue with gpio and pinmux

[Damien Riegel]

Hi,


In our ARM-based SoC, we can multiplex up to 4 functions on each pin,
but we have an issue when using a pin as a gpio: it can still be muxed
as another function.

In the gpio driver, we delegate calls to "request" to
"pinctrl_request_gpio":

	static int p7gpio_request(struct gpio_chip *chip, unsigned int offset)
	{
		return pinctrl_request_gpio(chip->base + offset);
	}

	static const struct gpio_chip p7gpio_chip_defaults = {
		.request            = p7gpio_request,
		[...]
	};

pinctrl_request_gpio will result in a call to pin_request, which behaves
differently if we request a gpio or another function, so at kernel
level they are not exclusive, but at hardware level they must be.

To avoid conflicts, the workaround I use is to implement "request" and
"gpio_request_enable" as below in a pinctrl driver to make mux and gpio
exclusive. But I really feel like I should not do so and let the kernel
do this for me.

	static int p7ctl_enable_gpio(struct pinctrl_dev* pctl,
					struct pinctrl_gpio_range* range,
					unsigned int gpio)
	{
		struct pin_desc *desc;
		desc = pin_desc_get(pctl, gpio);
		if (desc->mux_owner)
			return -EBUSY;
		
		[...]
	}

	static int p7ctl_request(struct pinctrl_dev* pctl, unsigned int pin)
	{
		struct pin_desc *desc;
		desc = pin_desc_get(pctl, pin);
		if (desc->gpio_owner)
			return -EBUSY;

		return 0;
	}

	static struct pinmux_ops p7ctl_mux_ops = {
		.request                = p7ctl_request,
		.gpio_request_enable    = p7ctl_enable_gpio,
		[...]
	};

Is this solution correct or is there a better way to claim the mux when
using a pin as a gpio ?

Note: we are using a kernel based on 3.4.11, but as far as I can tell,
pinctrl_request_gpio have the same behavior in the upstream kernel.


Thanks,
Damien

Issue with gpio and pinmux

[Stephen Warren]

On 07/17/2013 03:45 AM, Damien Riegel wrote:
> Hi,
> 
> 
> In our ARM-based SoC, we can multiplex up to 4 functions on each pin,
> but we have an issue when using a pin as a gpio: it can still be muxed
> as another function.

It's a good idea to Cc the maintainers of the subsystem you're asking
questions about. See the MAINTAINERS file in the kernel source tree for
clues who that is. I've CC'd LinusW here.

When you say "it can still be muxed as another function", I assume you
mean that SW is allowing someone to do that; it's not that your HW
allows this somehow?

> In the gpio driver, we delegate calls to "request" to
> "pinctrl_request_gpio":
> 
> 	static int p7gpio_request(struct gpio_chip *chip, unsigned int offset)
> 	{
> 		return pinctrl_request_gpio(chip->base + offset);
> 	}
> 
> 	static const struct gpio_chip p7gpio_chip_defaults = {
> 		.request            = p7gpio_request,
> 		[...]
> 	};
> 
> pinctrl_request_gpio will result in a call to pin_request, which behaves
> differently if we request a gpio or another function, so at kernel
> level they are not exclusive, but at hardware level they must be.

Yes, that's how pinctrl is implemented right now.

The issue is that some HW muxes groups of pins at a time, so simply
because that group of pins is "claimed" by a mux function, implies
nothing about which specific pins in that group are actually used; some
may actually still be free for usage as a GPIO.

Now obviously that doesn't make a huge amount of sense if your HW muxes
each pin individually, so there are no groups.

The solution here may be one of:

a) Modify pinctrl /not/ to allow both a GPIO and a mux function to claim
a pin *if* the group that contains the pin only contains a single pin.
This is a bit of a hacky special case though; there are other situations
where "dual claiming" shouldn't be allowed, and this doesn't solve those.

b) Ignore the issue; if the system configuration is correct, the issue
won't cause problems in practice, since nothing will be configured to
"dual claim" any pin.

c) Enhance pinctrl with extra information, so it knows which pins in a
group are usable as GPIO even when the group is claimed by a mux
function. The list of pins could theoretically vary per mux function, so
you'd need a table with lookup key (pin group, mux function) and output
data (list of pins that can be "dual claimed"). This would be a complete
solution. If a list wasn't provided, the pinctrl core could assume that
mean no dual claim was allowed.

Issue with gpio and pinmux

[Linus Walleij]

On Thu, Jul 18, 2013 at 8:11 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:

> The issue is that some HW muxes groups of pins at a time, so simply
> because that group of pins is "claimed" by a mux function, implies
> nothing about which specific pins in that group are actually used; some
> may actually still be free for usage as a GPIO.

This we need to get into the documentation.

> The solution here may be one of:
>
> a) Modify pinctrl /not/ to allow both a GPIO and a mux function to claim
> a pin *if* the group that contains the pin only contains a single pin.
> This is a bit of a hacky special case though; there are other situations
> where "dual claiming" shouldn't be allowed, and this doesn't solve those.
>
> b) Ignore the issue; if the system configuration is correct, the issue
> won't cause problems in practice, since nothing will be configured to
> "dual claim" any pin.
>
> c) Enhance pinctrl with extra information, so it knows which pins in a
> group are usable as GPIO even when the group is claimed by a mux
> function. The list of pins could theoretically vary per mux function, so
> you'd need a table with lookup key (pin group, mux function) and output
> data (list of pins that can be "dual claimed"). This would be a complete
> solution. If a list wasn't provided, the pinctrl core could assume that
> mean no dual claim was allowed.

I guess (b) is what we have.

But it really feels incomplete does it not? IIRC the system was strict
not to allow simultaneous use of pins for GPIO and other functions
at all until the above corner case made it necessary to relax this
restriction.

A simple way to get (a) would be to just add a flag like ".strict"
to struct pinmux_ops but it feels cheap.

What do we need for (c)? I think we can refactor this as the Tegra
and U300 should be the two platforms with such tricky groups so
the impact should be fairly small right now. I'm just worried it would
be a complex and memory-consuming thing :-/

Yours,
Linus Walleij

Issue with gpio and pinmux

[Stephen Warren]

On 07/20/2013 04:44 PM, Linus Walleij wrote:
> On Thu, Jul 18, 2013 at 8:11 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> 
>> The issue is that some HW muxes groups of pins at a time, so simply
>> because that group of pins is "claimed" by a mux function, implies
>> nothing about which specific pins in that group are actually used; some
>> may actually still be free for usage as a GPIO.
> 
> This we need to get into the documentation.
> 
>> The solution here may be one of:
>>
>> a) Modify pinctrl /not/ to allow both a GPIO and a mux function to claim
>> a pin *if* the group that contains the pin only contains a single pin.
>> This is a bit of a hacky special case though; there are other situations
>> where "dual claiming" shouldn't be allowed, and this doesn't solve those.
>>
>> b) Ignore the issue; if the system configuration is correct, the issue
>> won't cause problems in practice, since nothing will be configured to
>> "dual claim" any pin.
>>
>> c) Enhance pinctrl with extra information, so it knows which pins in a
>> group are usable as GPIO even when the group is claimed by a mux
>> function. The list of pins could theoretically vary per mux function, so
>> you'd need a table with lookup key (pin group, mux function) and output
>> data (list of pins that can be "dual claimed"). This would be a complete
>> solution. If a list wasn't provided, the pinctrl core could assume that
>> mean no dual claim was allowed.
> 
> I guess (b) is what we have.
> 
> But it really feels incomplete does it not? IIRC the system was strict
> not to allow simultaneous use of pins for GPIO and other functions
> at all until the above corner case made it necessary to relax this
> restriction.
> 
> A simple way to get (a) would be to just add a flag like ".strict"
> to struct pinmux_ops but it feels cheap.
> 
> What do we need for (c)? I think we can refactor this as the Tegra
> and U300 should be the two platforms with such tricky groups so
> the impact should be fairly small right now. I'm just worried it would
> be a complex and memory-consuming thing :-/

Actually, a better way to do (c) might be to just add an extra "op" to
struct pinmux_ops; something like validate_gpio_mux(), passing in
information such as whether a mux function is in effect on that pin,
whether the pin is already claimed as a GPIO, and whether the function
is being called to check a GPIO-claim or mux-claim operation . If the op
is missing, no "dual claim" is allowed. If the op is present, the code
in the op validates whether the particular GPIO is allowed to be claimed
while the particular mux function is active.

That way, I think any driver that doesn't want to allow dual-claim
doesn't have to do anything, but the few drivers that do can code up
whatever algorithm they want; for Tegra, I'd just allow everything for
simplicity, but we could always enhance that later if we want.

Issue with gpio and pinmux

[Christian Ruppert]

On Sat, Jul 20, 2013 at 09:44:10PM -0600, Stephen Warren wrote:
> On 07/20/2013 04:44 PM, Linus Walleij wrote:
> > On Thu, Jul 18, 2013 at 8:11 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> > 
> >> The issue is that some HW muxes groups of pins at a time, so simply
> >> because that group of pins is "claimed" by a mux function, implies
> >> nothing about which specific pins in that group are actually used; some
> >> may actually still be free for usage as a GPIO.
> > 
> > This we need to get into the documentation.
> > 
> >> The solution here may be one of:
> >>
> >> a) Modify pinctrl /not/ to allow both a GPIO and a mux function to claim
> >> a pin *if* the group that contains the pin only contains a single pin.
> >> This is a bit of a hacky special case though; there are other situations
> >> where "dual claiming" shouldn't be allowed, and this doesn't solve those.
> >>
> >> b) Ignore the issue; if the system configuration is correct, the issue
> >> won't cause problems in practice, since nothing will be configured to
> >> "dual claim" any pin.
> >>
> >> c) Enhance pinctrl with extra information, so it knows which pins in a
> >> group are usable as GPIO even when the group is claimed by a mux
> >> function. The list of pins could theoretically vary per mux function, so
> >> you'd need a table with lookup key (pin group, mux function) and output
> >> data (list of pins that can be "dual claimed"). This would be a complete
> >> solution. If a list wasn't provided, the pinctrl core could assume that
> >> mean no dual claim was allowed.
> > 
> > I guess (b) is what we have.
> > 
> > But it really feels incomplete does it not? IIRC the system was strict
> > not to allow simultaneous use of pins for GPIO and other functions
> > at all until the above corner case made it necessary to relax this
> > restriction.
> > 
> > A simple way to get (a) would be to just add a flag like ".strict"
> > to struct pinmux_ops but it feels cheap.
> > 
> > What do we need for (c)? I think we can refactor this as the Tegra
> > and U300 should be the two platforms with such tricky groups so
> > the impact should be fairly small right now. I'm just worried it would
> > be a complex and memory-consuming thing :-/
> 
> Actually, a better way to do (c) might be to just add an extra "op" to
> struct pinmux_ops; something like validate_gpio_mux(), passing in
> information such as whether a mux function is in effect on that pin,
> whether the pin is already claimed as a GPIO, and whether the function
> is being called to check a GPIO-claim or mux-claim operation . If the op
> is missing, no "dual claim" is allowed. If the op is present, the code
> in the op validates whether the particular GPIO is allowed to be claimed
> while the particular mux function is active.
> 
> That way, I think any driver that doesn't want to allow dual-claim
> doesn't have to do anything, but the few drivers that do can code up
> whatever algorithm they want; for Tegra, I'd just allow everything for
> simplicity, but we could always enhance that later if we want.

[ An introductory note on terminology: in the following, "port" refers
to a set of pins who's mux is controlled through the same bit field in
the same register, "interface" refers to the set of pins comprising a
functional unit, e.g. SCL and SDA of an I2C interface or the four pins
of an SPI interface ]

In my understanding, the problem stems from the fact that we always
claim an entire port instead of just the pins required for a given
functionality. The pinctrl core is already well equipped to manage
claiming interfaces instead of ports and thus a fourth alternative would
be to

d) implement gpio/pin conflict management in the already existing
callbacks inside the driver: Today, both enable and gpio_request_enable
exist (and can refuse a request for a configuration/pin).
If we only claim the pins used by a given interface instead of the
entire port, conflict management becomes natural (see
http://lkml.indiana.edu/hypermail/linux/kernel/1306.2/00749.html and the
following).

IMHO, the pinctrl drivers are the right place for everything but the
most basic pin conflict management since constraints on what can be
muxed at the same time are highly hardware implementation dependent. One
could e.g. imagine an implementation where a GPIO can override and snoop
a pin's output value in one mux configuration, just snoop the pin value
in another and do neither in a third.

I see two (independent) ways to enhance the pinctrl core in order to
support drivers in this task:
a) Add two functions pin_is_claimed and gpio_is_claimed which allow the
pinctrl driver to query if a given pin/gpio is available for muxing.
b) Add four functions lock_gpio/unlock_gpio and lock_pin/unlock_pin
which allow the pinctrl driver to prevent the core from granting
requests on GPIOs/pins.

This solution also has the advantage that it doesn't modify the default
behaviour of the core and it thus won't break existing drivers.

Greetings,
  Christian

Issue with gpio and pinmux

[Stephen Warren]

On 07/22/2013 01:35 AM, Christian Ruppert wrote:
> On Sat, Jul 20, 2013 at 09:44:10PM -0600, Stephen Warren wrote:
>> On 07/20/2013 04:44 PM, Linus Walleij wrote:
>>> On Thu, Jul 18, 2013 at 8:11 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>>
>>>> The issue is that some HW muxes groups of pins at a time, so simply
>>>> because that group of pins is "claimed" by a mux function, implies
>>>> nothing about which specific pins in that group are actually used; some
>>>> may actually still be free for usage as a GPIO.
>>>
>>> This we need to get into the documentation.
>>>
>>>> The solution here may be one of:
>>>>
>>>> a) Modify pinctrl /not/ to allow both a GPIO and a mux function to claim
>>>> a pin *if* the group that contains the pin only contains a single pin.
>>>> This is a bit of a hacky special case though; there are other situations
>>>> where "dual claiming" shouldn't be allowed, and this doesn't solve those.
>>>>
>>>> b) Ignore the issue; if the system configuration is correct, the issue
>>>> won't cause problems in practice, since nothing will be configured to
>>>> "dual claim" any pin.
>>>>
>>>> c) Enhance pinctrl with extra information, so it knows which pins in a
>>>> group are usable as GPIO even when the group is claimed by a mux
>>>> function. The list of pins could theoretically vary per mux function, so
>>>> you'd need a table with lookup key (pin group, mux function) and output
>>>> data (list of pins that can be "dual claimed"). This would be a complete
>>>> solution. If a list wasn't provided, the pinctrl core could assume that
>>>> mean no dual claim was allowed.
>>>
>>> I guess (b) is what we have.
>>>
>>> But it really feels incomplete does it not? IIRC the system was strict
>>> not to allow simultaneous use of pins for GPIO and other functions
>>> at all until the above corner case made it necessary to relax this
>>> restriction.
>>>
>>> A simple way to get (a) would be to just add a flag like ".strict"
>>> to struct pinmux_ops but it feels cheap.
>>>
>>> What do we need for (c)? I think we can refactor this as the Tegra
>>> and U300 should be the two platforms with such tricky groups so
>>> the impact should be fairly small right now. I'm just worried it would
>>> be a complex and memory-consuming thing :-/
>>
>> Actually, a better way to do (c) might be to just add an extra "op" to
>> struct pinmux_ops; something like validate_gpio_mux(), passing in
>> information such as whether a mux function is in effect on that pin,
>> whether the pin is already claimed as a GPIO, and whether the function
>> is being called to check a GPIO-claim or mux-claim operation . If the op
>> is missing, no "dual claim" is allowed. If the op is present, the code
>> in the op validates whether the particular GPIO is allowed to be claimed
>> while the particular mux function is active.
>>
>> That way, I think any driver that doesn't want to allow dual-claim
>> doesn't have to do anything, but the few drivers that do can code up
>> whatever algorithm they want; for Tegra, I'd just allow everything for
>> simplicity, but we could always enhance that later if we want.
> 
> [ An introductory note on terminology: in the following, "port" refers
> to a set of pins who's mux is controlled through the same bit field in
> the same register,

I believe that's exactly what a "pingroup" is in pinctrl subsystem
terminology. Where there is already an existing name for something, it'd
be best to use that so that every person doesn't use a different name
for everything.

> "interface" refers to the set of pins comprising a
> functional unit, e.g. SCL and SDA of an I2C interface or the four pins
> of an SPI interface ]
> 
> In my understanding, the problem stems from the fact that we always
> claim an entire port instead of just the pins required for a given
> functionality.

Yes.

> The pinctrl core is already well equipped to manage
> claiming interfaces instead of ports and thus a fourth alternative would
> be to

I would argue that's not the case. pinctrl knows absolutely nothing
about "interfaces"; there is no data structure defines that describes
interfaces at all.

That's not saying such a thing could not be added; I'm simply arguing
that "well equipped" doesn't seem to be true at the moment.

> d) implement gpio/pin conflict management in the already existing
> callbacks inside the driver: Today, both enable and gpio_request_enable
> exist (and can refuse a request for a configuration/pin).
> If we only claim the pins used by a given interface instead of the
> entire port, conflict management becomes natural (see
> http://lkml.indiana.edu/hypermail/linux/kernel/1306.2/00749.html and the
> following).

OK, that's basically the same as my proposal to add a new callback; it's
simply re-using and existing driver callback rather than creating a new
one solely for conflict checking.

Re-using the existing callback(s) seems fine, with appropriate prototype
modification to pass back the list of pins that can-or-can't be
dual-claimed I suppose.

> IMHO, the pinctrl drivers are the right place for everything but the
> most basic pin conflict management since constraints on what can be
> muxed at the same time are highly hardware implementation dependent. One
> could e.g. imagine an implementation where a GPIO can override and snoop
> a pin's output value in one mux configuration, just snoop the pin value
> in another and do neither in a third.
> 
> I see two (independent) ways to enhance the pinctrl core in order to
> support drivers in this task:
> a) Add two functions pin_is_claimed and gpio_is_claimed which allow the
> pinctrl driver to query if a given pin/gpio is available for muxing.
> b) Add four functions lock_gpio/unlock_gpio and lock_pin/unlock_pin
> which allow the pinctrl driver to prevent the core from granting
> requests on GPIOs/pins.

Yes, either of those are basically what I proposed in my followup to the
email you're replying to.

> This solution also has the advantage that it doesn't modify the default
> behaviour of the core and it thus won't break existing drivers.

Issue with gpio and pinmux

[Linus Walleij]

On Sun, Jul 21, 2013 at 5:44 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:

> Actually, a better way to do (c) might be to just add an extra "op" to
> struct pinmux_ops; something like validate_gpio_mux(), passing in
> information such as whether a mux function is in effect on that pin,
> whether the pin is already claimed as a GPIO, and whether the function
> is being called to check a GPIO-claim or mux-claim operation . If the op
> is missing, no "dual claim" is allowed. If the op is present, the code
> in the op validates whether the particular GPIO is allowed to be claimed
> while the particular mux function is active.
>
> That way, I think any driver that doesn't want to allow dual-claim
> doesn't have to do anything, but the few drivers that do can code up
> whatever algorithm they want; for Tegra, I'd just allow everything for
> simplicity, but we could always enhance that later if we want.

You are right, this would be an elegant solution.

Christian, are you interested in coding this up?

Yours,
Linus Walleij

Issue with gpio and pinmux

[Damien Riegel]

I don't know about pinctrl background but I don't really get why there
is such a distinction between gpio and other functions in most cases.

I understand that on some arch a pin can be muxed and used as a gpio at
the same time. But on most arch, isn't gpio just another function that
can be muxed on a pin, and therefore we should check that a pin can be
requested as a gpio just by checking desc->mux_owner ?

Issue with gpio and pinmux

[Linus Walleij]

On Thu, Jul 25, 2013 at 3:20 PM, Damien Riegel
<damien.riegel.ext@parrot.com> wrote:

> I don't know about pinctrl background but I don't really get why there
> is such a distinction between gpio and other functions in most cases.
>
> I understand that on some arch a pin can be muxed and used as a gpio at
> the same time. But on most arch, isn't gpio just another function that
> can be muxed on a pin, and therefore we should check that a pin can be
> requested as a gpio just by checking desc->mux_owner ?

Does this recently submitted documentation patch help in explaining
some of your questions?
http://marc.info/?l=linux-kernel&m=137442899829271&w=2

Yours,
Linus Walleij

Issue with gpio and pinmux

[Damien Riegel]

> Does this recently submitted documentation patch help in explaining
> some of your questions?
> http://marc.info/?l=linux-kernel&m=137442899829271&w=2
This is a very helpful addition to the existing documentation.

My reflection rather was: when gpio is not orthogonal, we could just
check mux_owner to see if pin is available; and when it is, use the
current implementation.

But well, we obviously need to way to tell if gpios are orthogonal or
not, and adding a function to struct pinmux_ops is an elegant solution
to do so since it even allows a pin granularity.

I guess I sent my previous mail a bit too quickly.

Issue with gpio and pinmux

[Linus Walleij]

On Thu, Jul 25, 2013 at 4:42 PM, Damien Riegel
<damien.riegel.ext@parrot.com> wrote:

> My reflection rather was: when gpio is not orthogonal, we could just
> check mux_owner to see if pin is available; and when it is, use the
> current implementation.
>
> But well, we obviously need to way to tell if gpios are orthogonal or
> not, and adding a function to struct pinmux_ops is an elegant solution
> to do so since it even allows a pin granularity.

Yep, I'm just waiting for the patches now :-)

Yours,
Linus Walleij

Issue with gpio and pinmux

[Christian Ruppert]

On Tue, Jul 23, 2013 at 08:47:22AM -0700, Stephen Warren wrote:
> On 07/22/2013 01:35 AM, Christian Ruppert wrote:
> > On Sat, Jul 20, 2013 at 09:44:10PM -0600, Stephen Warren wrote:
> >> On 07/20/2013 04:44 PM, Linus Walleij wrote:
> > [...]
> > [ An introductory note on terminology: in the following, "port" refers
> > to a set of pins who's mux is controlled through the same bit field in
> > the same register,
> 
> I believe that's exactly what a "pingroup" is in pinctrl subsystem
> terminology. Where there is already an existing name for something, it'd
> be best to use that so that every person doesn't use a different name
> for everything.

Actually, I use this terminology because in my head a pingroup is "just"
an abstract data structure (i.e. a named list of pins). I think one of
our points of discussion is if this data structure should be used only
to represent what I call ports or if it can also represent things like
interfaces, gpio-ranges etc. Thus, we need a terminology to talk about
the "physical equivalents"...

> > "interface" refers to the set of pins comprising a
> > functional unit, e.g. SCL and SDA of an I2C interface or the four pins
> > of an SPI interface ]
> > 
> > In my understanding, the problem stems from the fact that we always
> > claim an entire port instead of just the pins required for a given
> > functionality.
> 
> Yes.
> 
> > The pinctrl core is already well equipped to manage
> > claiming interfaces instead of ports and thus a fourth alternative would
> > be to
> 
> I would argue that's not the case. pinctrl knows absolutely nothing
> about "interfaces"; there is no data structure defines that describes
> interfaces at all.

Well, pin groups are perfectly capable of representing interfaces
instead of ports (see my remark above).

> That's not saying such a thing could not be added; I'm simply arguing
> that "well equipped" doesn't seem to be true at the moment.

Intuitively, I would implement this by adding a second level of
semantics on top of pin groups (at the same level as the "functions" we
already have).

Alternatively, one could add the notion of interfaces to functions (a
function provides one or more interfaces) but I'm a bit afraid that this
encodes tb10x-specific logic into the core...

> > d) implement gpio/pin conflict management in the already existing
> > callbacks inside the driver: Today, both enable and gpio_request_enable
> > exist (and can refuse a request for a configuration/pin).
> > If we only claim the pins used by a given interface instead of the
> > entire port, conflict management becomes natural (see
> > http://lkml.indiana.edu/hypermail/linux/kernel/1306.2/00749.html and the
> > following).
> 
> OK, that's basically the same as my proposal to add a new callback; it's
> simply re-using and existing driver callback rather than creating a new
> one solely for conflict checking.
> 
> Re-using the existing callback(s) seems fine, with appropriate prototype
> modification to pass back the list of pins that can-or-can't be
> dual-claimed I suppose.

I like this proposal but I would rather add a set of functions to the
core than modifying the prototype (more details in a separate email in
response to LinusW).

> > IMHO, the pinctrl drivers are the right place for everything but the
> > most basic pin conflict management since constraints on what can be
> > muxed at the same time are highly hardware implementation dependent. One
> > could e.g. imagine an implementation where a GPIO can override and snoop
> > a pin's output value in one mux configuration, just snoop the pin value
> > in another and do neither in a third.
> > 
> > I see two (independent) ways to enhance the pinctrl core in order to
> > support drivers in this task:
> > a) Add two functions pin_is_claimed and gpio_is_claimed which allow the
> > pinctrl driver to query if a given pin/gpio is available for muxing.
> > b) Add four functions lock_gpio/unlock_gpio and lock_pin/unlock_pin
> > which allow the pinctrl driver to prevent the core from granting
> > requests on GPIOs/pins.
> 
> Yes, either of those are basically what I proposed in my followup to the
> email you're replying to.

Actually what I was thinking of was the inverse of your suggestion: In
your suggestion the *_is_claimed functions were part of each driver, in
mine they were part of the core.

Greetings,
  Christian

-- 
  Christian Ruppert              ,          <christian.ruppert@abilis.com>
                                /|
  Tel: +41/(0)22 816 19-42     //|                 3, Chemin du Pr?-Fleuri
                             _// | bilis Systems   CH-1228 Plan-les-Ouates

Issue with gpio and pinmux

[Christian Ruppert]

On Thu, Jul 25, 2013 at 11:37:52AM +0200, Linus Walleij wrote:
> On Sun, Jul 21, 2013 at 5:44 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> 
> > Actually, a better way to do (c) might be to just add an extra "op" to
> > struct pinmux_ops; something like validate_gpio_mux(), passing in
> > information such as whether a mux function is in effect on that pin,
> > whether the pin is already claimed as a GPIO, and whether the function
> > is being called to check a GPIO-claim or mux-claim operation . If the op
> > is missing, no "dual claim" is allowed. If the op is present, the code
> > in the op validates whether the particular GPIO is allowed to be claimed
> > while the particular mux function is active.
> >
> > That way, I think any driver that doesn't want to allow dual-claim
> > doesn't have to do anything, but the few drivers that do can code up
> > whatever algorithm they want; for Tegra, I'd just allow everything for
> > simplicity, but we could always enhance that later if we want.
> 
> You are right, this would be an elegant solution.
> 
> Christian, are you interested in coding this up?

When thinking about it, we require the same mechanism the other way
round: If a GPIO is claimed first we need a mechanism to forbid
incompatible reconfiguration of other functionalities until that GPIO is
free again.

I like Stephen's proposal (in another mail) to claim GPIOs from the
pinctrl drivers but instead of modifying the callback prototypes I
suggest to add the following functions:

/*
 * Claim GPIO pin (in the sense: forbid acutal GPIO usage)
 */
int claim_gpios(struct pinctrl_dev *pctl, unsigned *pins
                unsigned port_selector, unsigned func_selector);

/* allow GPIO usage of the given pins */
void unclaim_gpios(struct pinctrl_dev *pctl, unsigned *pins);

Claiming could be fully dynamic. This way, driver authors don't need to
implement additional callbacks or modify their existing ones. In the
simplest case they do nothing. The entire conflict-checking logic is
confined inside the core.

Alternatively, a similar set of functions could make the use of GPIOs
exclusive with other functions for a given set of pins. This would work
very well with the proposal below and would be easier to manage for most
driver authors: The configuration would be static in most cases.
(We just have to discuss about the default mode, do we want to keep the
backward-compatible inclusive default or change everything to exclusive
by default?).

A last point I would like to address before passing on to implementation
is to introduce the notion of interfaces: I think it is important to
introduce some orthogonality between unrelated interfaces which happen
to be muxed on the same port and ideally we implement this coherently
with the above.

I would like to know your opinion if the following proposal is generic
enough or if it encodes too much chip-specific logic into the core:

- We extend the notion of pin groups to represent either ports (as it is
  currently the case) or interfaces. No need to use name pin groups in
  both cases, I'm open to proposals for better terminology.
- Functions are associated either to one port or a set of interfaces.
- Functions continue to apply to an entire port but they only claim the
  pins of the interfaces they are associated with.
- A client can request either a function or an interface from the
  pinctrl core. In the second case, the core will figure out if the
  request is coherent with previous requests (i.e. if there is a
  function providing all requested interfaces) and call the driver
  accordingly.

We'll have to figure out a few details but I would like to have an
agreement on fundamentals before spending time on implementation.

Greetings,
  Christian

-- 
  Christian Ruppert              ,          <christian.ruppert@abilis.com>
                                /|
  Tel: +41/(0)22 816 19-42     //|                 3, Chemin du Pr?-Fleuri
                             _// | bilis Systems   CH-1228 Plan-les-Ouates

Issue with gpio and pinmux

[Stephen Warren]

On 07/26/2013 03:26 AM, Christian Ruppert wrote:
> On Thu, Jul 25, 2013 at 11:37:52AM +0200, Linus Walleij wrote:
>> On Sun, Jul 21, 2013 at 5:44 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>
>>> Actually, a better way to do (c) might be to just add an extra "op" to
>>> struct pinmux_ops; something like validate_gpio_mux(), passing in
>>> information such as whether a mux function is in effect on that pin,
>>> whether the pin is already claimed as a GPIO, and whether the function
>>> is being called to check a GPIO-claim or mux-claim operation . If the op
>>> is missing, no "dual claim" is allowed. If the op is present, the code
>>> in the op validates whether the particular GPIO is allowed to be claimed
>>> while the particular mux function is active.
>>>
>>> That way, I think any driver that doesn't want to allow dual-claim
>>> doesn't have to do anything, but the few drivers that do can code up
>>> whatever algorithm they want; for Tegra, I'd just allow everything for
>>> simplicity, but we could always enhance that later if we want.
>>
>> You are right, this would be an elegant solution.
>>
>> Christian, are you interested in coding this up?
> 
> When thinking about it, we require the same mechanism the other way
> round: If a GPIO is claimed first we need a mechanism to forbid
> incompatible reconfiguration of other functionalities until that GPIO is
> free again.
> 
> I like Stephen's proposal (in another mail) to claim GPIOs from the
> pinctrl drivers but instead of modifying the callback prototypes I
> suggest to add the following functions:
> 
> /*
>  * Claim GPIO pin (in the sense: forbid acutal GPIO usage)
>  */
> int claim_gpios(struct pinctrl_dev *pctl, unsigned *pins
>                 unsigned port_selector, unsigned func_selector);

I don't think that interface is quite right.

It assumes there's a concept of ports in pinctrl which there isn't. I
know you're arguing for that below, but dual-claiming and ports seem
like two related but separate issues, and I believe we should keep the
discussions somewhat separate.

How many entries are there in *pins? If just one, no need for a pointer.
If multiple, there needs to be a count parameter, and the
port/func_selector would also need to be arrays, since presumably
there's no reason to expect all the GPIOs to be in the same physical pin
group, if the HW is muxed at a pingroup level.

I think both a claim_gpios() and claim_mux_function() API would be
needed. If this function is intended to cover both, then I'd rather name
it claim_pins, since in the context of pinctrl, "GPIO" is a particular
usage of a pin, rather than a noun meaning the pin/ball/pad itself.

> /* allow GPIO usage of the given pins */
> void unclaim_gpios(struct pinctrl_dev *pctl, unsigned *pins);
> 
> Claiming could be fully dynamic. This way, driver authors don't need to
> implement additional callbacks or modify their existing ones. In the
> simplest case they do nothing. The entire conflict-checking logic is
> confined inside the core.
> 
> Alternatively, a similar set of functions could make the use of GPIOs
> exclusive with other functions for a given set of pins. This would work
> very well with the proposal below and would be easier to manage for most
> driver authors: The configuration would be static in most cases.
> (We just have to discuss about the default mode, do we want to keep the
> backward-compatible inclusive default or change everything to exclusive
> by default?).
> 
> A last point I would like to address before passing on to implementation
> is to introduce the notion of interfaces: I think it is important to
> introduce some orthogonality between unrelated interfaces which happen
> to be muxed on the same port and ideally we implement this coherently
> with the above.
> 
> I would like to know your opinion if the following proposal is generic
> enough or if it encodes too much chip-specific logic into the core:
> 
> - We extend the notion of pin groups to represent either ports (as it is
>   currently the case) or interfaces. No need to use name pin groups in
>   both cases, I'm open to proposals for better terminology.

I think if we start doing this, we really need to come up with unique
names for the usages for each type of "group of pins". Overloading "pin
group" for all the different uses will become rather confusing. It's bad
enough with some pinctrl drivers using them for real HW pingroups and
others for arbitrary SW groups of logically related pins. Introducing
new usage for "ports" and so on without actually calling them "ports" is
going to get even worse.

Perhaps each pin group needs a type associated with it, and we'll have:

PIN_GROUP_TYPE_PHYSICAL (only exists for HW that muxes groups of pins).
PIN_GROUP_TYPE_VIRTUAL (SW fake versions of _PHYSICAL)
PIN_GROUP_TYPE_PORT
PIN_GROUP_TYPE_...

?

I'm still not convinced that pinctrl should care about interfaces at all
though. Perhaps it might be useful if pinctrl wants to be really anal
about validating every detail of whether GPIOs and mux functions can
share pins. However, if the pinctrl configuration is correct, this isn't
necessary anyway, and hence just seems to bring additional complexity
for little benefit.

> - Functions are associated either to one port or a set of interfaces.

> - Functions continue to apply to an entire port but they only claim the
>   pins of the interfaces they are associated with.

This will be rather difficult to model in 100% accurate detail. Some
signals may be optional on some HW modules.

Consider an SDHCI controller which supports either 1-, 4-, or 8-bit
operation. Will a new function value be created for each of those
options, even though they all represent connecting the pins to the same
HW module, and hence the HW probably needs the same value programmed
into it for the relevant pins/groups irrespective of the bus width?

What about the pinctrl DT bindings that already exist, where the set of
available functions has already been defined to match HW. We need to
maintain compatibility with the old DT bindings, and hence can't just
create new function values out of thin air.

> - A client can request either a function or an interface from the
>   pinctrl core. In the second case, the core will figure out if the
>   request is coherent with previous requests (i.e. if there is a
>   function providing all requested interfaces) and call the driver
>   accordingly.

Requesting an interface doesn't make sense. The definition of an
interface implies zero knowledge, in general, about which pins that
interface will be routed to; in general HW can be designed to allow any
interface to be routed to/from different sets of pins. While it's true
that some SoCs don't actually allow this, I think it's best to keep the
pinctrl API model fully general, rather than allowing some drivers to
tailor their usage to more specific configurations. After all, the whole
point of pinctrl is to provide common semantics to drivers, so that if
the HW block they control are re-used across SoCs with different pinctrl
units, the drivers don't have to change.

> We'll have to figure out a few details but I would like to have an
> agreement on fundamentals before spending time on implementation.

Issue with gpio and pinmux

[Linus Walleij]

On Fri, Jul 26, 2013 at 11:26 AM, Christian Ruppert
<christian.ruppert@abilis.com> wrote:

> I like Stephen's proposal (in another mail) to claim GPIOs from the
> pinctrl drivers but instead of modifying the callback prototypes I
> suggest to add the following functions:
>
> /*
>  * Claim GPIO pin (in the sense: forbid acutal GPIO usage)
>  */
> int claim_gpios(struct pinctrl_dev *pctl, unsigned *pins
>                 unsigned port_selector, unsigned func_selector);
>
> /* allow GPIO usage of the given pins */
> void unclaim_gpios(struct pinctrl_dev *pctl, unsigned *pins);

I would keep ports out of this discussion to begin with,
that can be discussed and patched separately. Let's focus on
GPIO exclusivity WRT other functions.

And these should not be separate functions right? It should
be new members of struct pinmux_ops in <linux/pinctrl/pinmux.h>
right?

And there we already have these:

 * @gpio_request_enable: requests and enables GPIO on a certain pin.
 *      Implement this only if you can mux every pin individually as GPIO. The
 *      affected GPIO range is passed along with an offset(pin number) into that
 *      specific GPIO range - function selectors and pin groups are orthogonal
 *      to this, the core will however make sure the pins do not collide.
 * @gpio_disable_free: free up GPIO muxing on a certain pin, the reverse of
 *      @gpio_request_enable


        int (*gpio_request_enable) (struct pinctrl_dev *pctldev,
                                    struct pinctrl_gpio_range *range,
                                    unsigned offset);
        void (*gpio_disable_free) (struct pinctrl_dev *pctldev,
                                   struct pinctrl_gpio_range *range,
                                   unsigned offset);


> Claiming could be fully dynamic. This way, driver authors don't need to
> implement additional callbacks or modify their existing ones. In the
> simplest case they do nothing. The entire conflict-checking logic is
> confined inside the core.

This is the idea.

The core can already do this, basically, it is just that it's
pin_request() from drivers/pinctrl/pinmux.c.

As you can see if it is passed a gpio range, the GPIO request is
handled orhogonal to any other pin request, and calling the above
functions. What we want to do is basically selectively remove the
orthogonality. It should be very simple at this point.

> Alternatively, a similar set of functions could make the use of GPIOs
> exclusive with other functions for a given set of pins. This would work
> very well with the proposal below and would be easier to manage for most
> driver authors: The configuration would be static in most cases.
> (We just have to discuss about the default mode, do we want to keep the
> backward-compatible inclusive default or change everything to exclusive
> by default?).

I'm not following this, sorry.

> A last point I would like to address before passing on to implementation
> is to introduce the notion of interfaces: I think it is important to
> introduce some orthogonality between unrelated interfaces which happen
> to be muxed on the same port and ideally we implement this coherently
> with the above.

The ports concept should be handled separately.

Yours,
Linus Walleij