Linus (W),
Some more thoughts on pinmux:
========== GPIO/pinmux API interactions
I'd like to understand how the gpio and pinmux subsystems are intended
to interact.
Quoting from Documentation/gpio.txt:
Note that requesting a GPIO does NOT cause it to be configured in any
way; it just marks that GPIO as in use. Separate code must handle any
pin setup (e.g. controlling which pin the GPIO uses, pullup/pulldown).
Due to this, the current Tegra GPIO driver contains additional non-
standard functions:
void tegra_gpio_enable(int gpio);
void tegra_gpio_disable(int gpio);
In some cases, those functions are called by board files at boot time,
and in other cases by drivers themselves, right before gpio_request().
Is it your intention that such custom functions be replaced by
pinmux_request_gpio(), and the Tegra pinmux driver's gpio_request_enable
op should call tegra_gpio_enable?
That seems like the right idea to me.
========== pinmux calling GPIO or vice-versa?
Having pinmux call gpio appears to conflict with a couple of things from
your recent pinmux patchset:
> diff --git a/drivers/Kconfig b/drivers/Kconfig
> +# pinctrl before gpio - gpio drivers may need it
> +
> +source "drivers/pinctrl/Kconfig"
> +
> source "drivers/gpio/Kconfig"
>
> diff --git a/drivers/Makefile b/drivers/Makefile
>...
> +# GPIO must come after pinctrl as gpios may need to mux pins etc
> +obj-y += pinctrl/
Don't those patches imply that the GPIO controller code is calling into
the pinmux code to perform muxing, not the other way around?
========== When pins get marked as in-used
There seems to be a discrepancy in the way the client APIs work: For
special functions, the client calls pinmux_get, then later pinmux_enable,
whereas for GPIOs, the client just calls pinmux_request_gpio. I'm not
sure of the benefit of having separate pinmux_get and pinmux_enable
calls; I thought the idea was that a client could:
a = pinmux_get(dev, "funca");
b = pinmux_get(dev, "funcb");
pinmux_enable(a);
...
pinmux_disable(a);
pinmux_enable(b);
...
pinmux_disable(b);
pinmux_put(b);
pinmux_put(a);
However, since the pins are marked as reserved/in-use when pinmux_get
is called rather than pinmux_enable, the code above won't work; it'd
need to be:
a = pinmux_get(dev, "funca");
pinmux_enable(a);
...
pinmux_disable(a);
pinmux_put(a);
b = pinmux_get(dev, "funcb");
pinmux_enable(b);
...
pinmux_disable(b);
pinmux_put(b);
Perhaps pin usage marking should be moved into enable/disable?
========== Matched request/free calls for GPIOs
A couple more comments on your recent pinmux patches in light of this:
>From pin_request():
+ /*
+ * If there is no kind of request function for the pin we just assume
+ * we got it by default and proceed.
+ */
+ if (gpio && ops->gpio_request_enable)
+ /* This requests and enables a single GPIO pin */
+ status = ops->gpio_request_enable(pmxdev,
+ pin - pmxdev->desc->base);
+ else if (ops->request)
+ status = ops->request(pmxdev,
+ pin - pmxdev->desc->base);
+ else
+ status = 0;
a) I feel the default should be to error out, not succeed; the whole
point of the API is for HW where something must be programmed to enable
each function. As such, if you don't provide that ops->request* function,
that seems like an error.
I think the logic above should be to always call ops->gpio_request_enable
if (gpio):
if (gpio)
if (ops->gpio_request_enable)
/* This requests and enables a single GPIO pin */
status = ops->gpio_request_enable(pmxdev,
pin - pmxdev->desc->base);
else
status = ops->request(pmxdev,
pin - pmxdev->desc->base);
(ops->gpio_request_enable could be optional if GPIOs aren't supported on
any pinmux pins, whereas ops->request isn't optional, and should be checked
during pinmux_register)
Also, ops->gpio_request_enable should also be passed the GPIO number,
so the driver can validate that it's the correct one. Often, only
one value would be legal, but perhaps some pinmuxs allow 1 of N different
GPIOs to be mapped to some pin, so selection of which GPIO is on par with
selection of which special function to mux out, yet the driver still
doesn't want to expose every GPIO possibility as a pinmux "function" due
to explosion of the number of functions if it did that.
b) When ops->gpio_request_enable is called to request a pin, it'd be nice
if the core could track this, and specifically call a new ops->gpio_disable
to free it, rather than the generic ops->free. There are two cases in HW:
b1) "GPIO" is just another special function in a list, e.g. SPI, I2C, GPIO.
In this case, free for a GPIO needs to either do nothing (the next call to
request will simply set the desired function), or possibly do something to
disable the pin in the same way as any other function. This works fine with
a single free function.
b2) GPIO enable/disable is a separate concept from the pinmuxing; on Tegra
it'll be implemented by calling tegra_gpio_enable/disable. As such, a
single free function would require the pinmux driver to store state
indicating whether ops->free should call tegra_gpio_disable, or whether it
should do cleanup to the pinmux registers. Since the core is managing GPIO
vs. function allocation, I think the storing that state in the core makes
sense.
Thanks for reading. Hopefully this email didn't jump about too much.
--
nvpublic
On Wed, Jun 15, 2011 at 11:48 PM, Stephen Warren <[email protected]> wrote:
> Some more thoughts on pinmux:
Thanks!
Keep it coming until you get tired of me :-)
> ========== GPIO/pinmux API interactions
>
> I'd like to understand how the gpio and pinmux subsystems are intended
> to interact.
Mainly I'd like Grant to comment on that a bit. It refers to the (long)
reply I wrote in response to his feeback on these issues with some
open questions in it.
> ?Note that requesting a GPIO does NOT cause it to be configured in any
> ?way; it just marks that GPIO as in use. ?Separate code must handle any
> ?pin setup (e.g. controlling which pin the GPIO uses, pullup/pulldown).
>
> Due to this, the current Tegra GPIO driver contains additional non-
> standard functions:
>
> void tegra_gpio_enable(int gpio);
> void tegra_gpio_disable(int gpio);
>
> In some cases, those functions are called by board files at boot time,
> and in other cases by drivers themselves, right before gpio_request().
>
> Is it your intention that such custom functions be replaced by
> pinmux_request_gpio(), and the Tegra pinmux driver's gpio_request_enable
> op should call tegra_gpio_enable?
>
> That seems like the right idea to me.
Yes and no. As you saw in the last iteration I renamed the pinmux
framework "pinctrl" or "pin control". So being such a hopless optimist
I set out to solve all pin controlling in this subsystem. The sales rap
would be something like:
- Do you need to bias your pin to 3.3 V with an 1MOhm resistor?
- Do you want to mux your pin with differetn functions?
- Do you want to set the load capacitance of you pin to 10pF?
- Do you want to drive your pin as open collector?
- Is all or some the above software-controlled in your ASIC?
- Do you despair from the absence of relevant frameworks?
DON'T WORRY!
The pinctrl subsystem is here to save YOU!
However that's a bit of vaporware, since I just implemented the
pin numberspace and pin registration (that is the generic part)
and then the pinmux part. The rest (assorted pin control) is
yet to come.
> ========== pinmux calling GPIO or vice-versa?
>
> Having pinmux call gpio appears to conflict with a couple of things from
> your recent pinmux patchset:
>
>> diff --git a/drivers/Kconfig b/drivers/Kconfig
>> +# pinctrl before gpio - gpio drivers may need it
>> +
>> +source "drivers/pinctrl/Kconfig"
>> +
>> ?source "drivers/gpio/Kconfig"
>>
>> diff --git a/drivers/Makefile b/drivers/Makefile
>>...
>> +# GPIO must come after pinctrl as gpios may need to mux pins etc
>> +obj-y ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?+= pinctrl/
>
> Don't those patches imply that the GPIO controller code is calling into
> the pinmux code to perform muxing, not the other way around?
Yes.
Grant does not seem to like the idea of the gpio subsystem
meddeling with all this stuff anyway, so I intend to take all that
into pinctrl, and then gpio only deal with, well GPIO. Setting
bits quickly and such.
That is, irrespective of the fact that these functions may live
in the same register range, this is a divide between functionality
not hardware blocks.
> ========== When pins get marked as in-used
>
> There seems to be a discrepancy in the way the client APIs work: For
> special functions, the client calls pinmux_get, then later pinmux_enable,
> whereas for GPIOs, the client just calls pinmux_request_gpio. I'm not
> sure of the benefit of having separate pinmux_get and pinmux_enable
> calls; I thought the idea was that a client could:
>
> a = pinmux_get(dev, "funca");
> b = pinmux_get(dev, "funcb");
> pinmux_enable(a);
> ...
> pinmux_disable(a);
> pinmux_enable(b);
> ...
> pinmux_disable(b);
> pinmux_put(b);
> pinmux_put(a);
>
> However, since the pins are marked as reserved/in-use when pinmux_get
> is called rather than pinmux_enable, the code above won't work;
If the two functions share the same pins so they are mutually exclusive,
then yes.
> it'd need to be:
>
> a = pinmux_get(dev, "funca");
> pinmux_enable(a);
> ...
> pinmux_disable(a);
> pinmux_put(a);
> b = pinmux_get(dev, "funcb");
> pinmux_enable(b);
> ...
> pinmux_disable(b);
> pinmux_put(b);
>
> Perhaps pin usage marking should be moved into enable/disable?
Actually no - my idea is that enable/disable shall be fast while
get/put will be allowed to take time. (fastpath vs slowpath if
you like).
The rationale was described I think in the first patch set, that
switching pinmuxes tied to a device will be rare, whereas
enabling/disabling will be common. So get/put will do the
slow lookup of descriptors etc, whereas enable/disable will
typically involve poking some hardware register so it will be fast.
For example enabling/disabling on many systems need to be
done as part of idling the hardware, and thus it should be
fast since you may need to turn it back on quickly.
> ========== Matched request/free calls for GPIOs
>
> A couple more comments on your recent pinmux patches in light of this:
>
> From pin_request():
>
> + ? ? ? /*
> + ? ? ? ?* If there is no kind of request function for the pin we just assume
> + ? ? ? ?* we got it by default and proceed.
> + ? ? ? ?*/
> + ? ? ? if (gpio && ops->gpio_request_enable)
> + ? ? ? ? ? ? ? /* This requests and enables a single GPIO pin */
> + ? ? ? ? ? ? ? status = ops->gpio_request_enable(pmxdev,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? pin - pmxdev->desc->base);
> + ? ? ? else if (ops->request)
> + ? ? ? ? ? ? ? status = ops->request(pmxdev,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? pin - pmxdev->desc->base);
> + ? ? ? else
> + ? ? ? ? ? ? ? status = 0;
>
> a) I feel the default should be to error out, not succeed; the whole
> point of the API is for HW where something must be programmed to enable
> each function. As such, if you don't provide that ops->request* function,
> that seems like an error.
This is designed for the case where the driver avoids to define either
a gpio_request_enable() or a single pin request() callback.
The request() is entirely optional but intended to handle the case
where hardware puts additional restrictons on muxing apart from
pin ovelap (which is already handled by the core). For chips that
do not have any such hardware restrictions you don't implement the
callback and then it succeeds, as there is no restriction.
The issue of HW restrictions on muxing was raised by Grant
only yesterday I think, it's a universal phenomenon, some have
it some don't.
The kerneldoc for request() right now looks like this:
* @request: called by the core to see if a certain pin can be made available
* available for muxing. This is called by the core to acquire the pins
* before selecting any actual mux setting across a function. The driver
* is allowed to answer "no" by returning a negative error code
Maybe it needs some clarification?
> I think the logic above should be to always call ops->gpio_request_enable
> if (gpio):
>
> ? ? ? ?if (gpio)
> ? ? ? ? ? ? ? ?if (ops->gpio_request_enable)
> ? ? ? ? ? ? ? ? ? ? ? ?/* This requests and enables a single GPIO pin */
> ? ? ? ? ? ? ? ? ? ? ? ?status = ops->gpio_request_enable(pmxdev,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?pin - pmxdev->desc->base);
> ? ? ? ?else
> ? ? ? ? ? ? ? ?status = ops->request(pmxdev,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?pin - pmxdev->desc->base);
>
> (ops->gpio_request_enable could be optional if GPIOs aren't supported on
> any pinmux pins, whereas ops->request isn't optional, and should be checked
> during pinmux_register)
Well the idea is that if you want to reserve individual pins for GPIO
but only need to implement one function for all pins on the mux,
you need only implement ops->request(), this is the case for example
with ux500 - any muxable pin works the same way since *all* muxable
pins can also handle GPIO.
> Also, ops->gpio_request_enable should also be passed the GPIO number,
> so the driver can validate that it's the correct one.
Normally I think the pinmux driver should have no knowledge of
such GPIO stuff.
It's two totally separate number spaces here,
one for pins, one for GPIO:s, I definately don't want to create
cross-dependencies between these two.
The basic assumption is that the pin number request coming in
is sane, that the user knows what it is asking for.
> Often, only
> one value would be legal, but perhaps some pinmuxs allow 1 of N different
> GPIOs to be mapped to some pin, so selection of which GPIO is on par with
> selection of which special function to mux out, yet the driver still
> doesn't want to expose every GPIO possibility as a pinmux "function" due
> to explosion of the number of functions if it did that.
>
> b) When ops->gpio_request_enable is called to request a pin, it'd be nice
> if the core could track this, and specifically call a new ops->gpio_disable
> to free it, rather than the generic ops->free. There are two cases in HW:
>
> b1) "GPIO" is just another special function in a list, e.g. SPI, I2C, GPIO.
> In this case, free for a GPIO needs to either do nothing (the next call to
> request will simply set the desired function), or possibly do something to
> disable the pin in the same way as any other function. This works fine with
> a single free function.
>
> b2) GPIO enable/disable is a separate concept from the pinmuxing; on Tegra
> it'll be implemented by calling tegra_gpio_enable/disable. As such, a
> single free function would require the pinmux driver to store state
> indicating whether ops->free should call tegra_gpio_disable, or whether it
> should do cleanup to the pinmux registers. Since the core is managing GPIO
> vs. function allocation, I think the storing that state in the core makes
> sense.
I understand, I think.
But I don't know if passing the gpio number into the pinmux
driver helps with that. To me it sounds like a reason to keep
then even more strictly separated so the pinmux driver have no
clue whatsoever what it is muxing, it just performs the muxing.
I had this similar usecase (I think from Sascha?) where two slightly
different GPIO blocks (different drive strengths etc) would be alternatively
muxed onto the same pins, on a per-pin basis.
In that case I think it was modeled such that say GPIO pins [0-N]
were considered the same pins [0-N] on the other chip too.
In that case I'd say it would be more clever to say that you have
GPIO pins [0..N] and [N+1...2N], then let the physical pins
behind that be a different issue altogether.
For these cases I generally feel we need actual code to get
somewhere, it might get into too much premature upfront design
otherwise. I think it's better to look at patches here.
> Thanks for reading. Hopefully this email didn't jump about too much.
No problem... The more I read the more I learn.
Thanks,
Linus Walleij
Linus Walleij wrote at Thursday, June 16, 2011 9:07 AM:
> On Wed, Jun 15, 2011 at 11:48 PM, Stephen Warren <[email protected]> wrote:
>
> > Some more thoughts on pinmux:
>
> Thanks!
>
> Keep it coming until you get tired of me :-)
>
> > ========== GPIO/pinmux API interactions
> >
> > I'd like to understand how the gpio and pinmux subsystems are intended
> > to interact.
>
> Mainly I'd like Grant to comment on that a bit. It refers to the (long)
> reply I wrote in response to his feeback on these issues with some
> open questions in it.
>
> > Note that requesting a GPIO does NOT cause it to be configured in any
> > way; it just marks that GPIO as in use. Separate code must handle any
> > pin setup (e.g. controlling which pin the GPIO uses, pullup/pulldown).
> >
> > Due to this, the current Tegra GPIO driver contains additional non-
> > standard functions:
> >
> > void tegra_gpio_enable(int gpio);
> > void tegra_gpio_disable(int gpio);
> >
> > In some cases, those functions are called by board files at boot time,
> > and in other cases by drivers themselves, right before gpio_request().
> >
> > Is it your intention that such custom functions be replaced by
> > pinmux_request_gpio(), and the Tegra pinmux driver's gpio_request_enable
> > op should call tegra_gpio_enable?
> >
> > That seems like the right idea to me.
>
> Yes and no. As you saw in the last iteration I renamed the pinmux
> framework "pinctrl" or "pin control". So being such a hopeless optimist
> I set out to solve all pin controlling in this subsystem. The sales rap
> would be something like:
>
> - Do you need to bias your pin to 3.3 V with an 1MOhm resistor?
> - Do you want to mux your pin with differetn functions?
> - Do you want to set the load capacitance of you pin to 10pF?
> - Do you want to drive your pin as open collector?
> - Is all or some the above software-controlled in your ASIC?
> - Do you despair from the absence of relevant frameworks?
>
> DON'T WORRY!
> The pinctrl subsystem is here to save YOU!
>
> However that's a bit of vaporware, since I just implemented the
> pin numberspace and pin registration (that is the generic part)
> and then the pinmux part. The rest (assorted pin control) is
> yet to come.
Just one note on the extra stuff for the future:
* Tegra has say 100 muxable pins.
* GPIO vs. special function is selectable per pin.
* Muxing of which special function (and some other features, such as
pullup/down) is only at a granularity of "group of pins".
* Some other pin features (such as slew rate, drive strength) is also
only at a granularity of "group of pins" **but** the groups are defined
differently for these features than for muxing.
I figure the general model is:
* N pins, each perhaps with some per-pin controls.
* M groups of pins, each allowing muxing, and possibly other controls
and also an arbitrary number of:
* P groups of pins, allowing various other controls
> > ========== pinmux calling GPIO or vice-versa?
> >
> > Having pinmux call gpio appears to conflict with a couple of things from
> > your recent pinmux patchset:
> >
> >> diff --git a/drivers/Kconfig b/drivers/Kconfig
> >> +# pinctrl before gpio - gpio drivers may need it
> >> +
> >> +source "drivers/pinctrl/Kconfig"
> >> +
> >> source "drivers/gpio/Kconfig"
> >>
> >> diff --git a/drivers/Makefile b/drivers/Makefile
> >>...
> >> +# GPIO must come after pinctrl as gpios may need to mux pins etc
> >> +obj-y += pinctrl/
> >
> > Don't those patches imply that the GPIO controller code is calling into
> > the pinmux code to perform muxing, not the other way around?
>
> Yes.
>
> Grant does not seem to like the idea of the gpio subsystem
> meddeling with all this stuff anyway, so I intend to take all that
> into pinctrl, and then gpio only deal with, well GPIO. Setting
> bits quickly and such.
>
> That is, irrespective of the fact that these functions may live
> in the same register range, this is a divide between functionality
> not hardware blocks.
>
> > ========== When pins get marked as in-used
> >
> > There seems to be a discrepancy in the way the client APIs work: For
> > special functions, the client calls pinmux_get, then later pinmux_enable,
> > whereas for GPIOs, the client just calls pinmux_request_gpio. I'm not
> > sure of the benefit of having separate pinmux_get and pinmux_enable
> > calls; I thought the idea was that a client could:
> >
> > a = pinmux_get(dev, "funca");
> > b = pinmux_get(dev, "funcb");
> > pinmux_enable(a);
> > ...
> > pinmux_disable(a);
> > pinmux_enable(b);
> > ...
> > pinmux_disable(b);
> > pinmux_put(b);
> > pinmux_put(a);
> >
> > However, since the pins are marked as reserved/in-use when pinmux_get
> > is called rather than pinmux_enable, the code above won't work;
>
> If the two functions share the same pins so they are mutually exclusive,
> then yes.
>
> > it'd need to be:
> >
> > a = pinmux_get(dev, "funca");
> > pinmux_enable(a);
> > ...
> > pinmux_disable(a);
> > pinmux_put(a);
> > b = pinmux_get(dev, "funcb");
> > pinmux_enable(b);
> > ...
> > pinmux_disable(b);
> > pinmux_put(b);
> >
> > Perhaps pin usage marking should be moved into enable/disable?
>
> Actually no - my idea is that enable/disable shall be fast while
> get/put will be allowed to take time. (fastpath vs slowpath if
> you like).
>
> The rationale was described I think in the first patch set, that
> switching pinmuxes tied to a device will be rare, whereas
> enabling/disabling will be common. So get/put will do the
> slow lookup of descriptors etc, whereas enable/disable will
> typically involve poking some hardware register so it will be fast.
>
> For example enabling/disabling on many systems need to be
> done as part of idling the hardware, and thus it should be
> fast since you may need to turn it back on quickly.
The issue with this model is that one can't fast-switch between two
mappings that do both require the same pin (e.g. say between 2-bit and
4-bit MMC bus). Given this is as you say probably not a very common case,
and actually I don't think the lookup will be *that* slow, it sounds like
everything will work fine.
It might be nice to provide a short example, just like the code I wrote
above, on switching in the documentation just so people have something
canonical to cut/paste into their driver so they get it right without
having to internalize the model.
> > ========== Matched request/free calls for GPIOs
> >
> > A couple more comments on your recent pinmux patches in light of this:
> >
> > From pin_request():
> >
> > + /*
> > + * If there is no kind of request function for the pin we just assume
> > + * we got it by default and proceed.
> > + */
> > + if (gpio && ops->gpio_request_enable)
> > + /* This requests and enables a single GPIO pin */
> > + status = ops->gpio_request_enable(pmxdev,
> > + pin - pmxdev->desc->base);
> > + else if (ops->request)
> > + status = ops->request(pmxdev,
> > + pin - pmxdev->desc->base);
> > + else
> > + status = 0;
> >
> > a) I feel the default should be to error out, not succeed; the whole
> > point of the API is for HW where something must be programmed to enable
> > each function. As such, if you don't provide that ops->request* function,
> > that seems like an error.
>
> This is designed for the case where the driver avoids to define either
> a gpio_request_enable() or a single pin request() callback.
>
> The request() is entirely optional but intended to handle the case
> where hardware puts additional restrictons on muxing apart from
> pin ovelap (which is already handled by the core). For chips that
> do not have any such hardware restrictions you don't implement the
> callback and then it succeeds, as there is no restriction.
>
> The issue of HW restrictions on muxing was raised by Grant
> only yesterday I think, it's a universal phenomenon, some have
> it some don't.
OK. But I'm not sure request is the correct time to implement this, or
at least not the only time it needs to be implemented. The special
requirements that Tegra might implement could only be determined right
when actually programming the pinmux registers, since the checks rely
on which other functions actually end up being activated.
Well, unless you're not allowed to even request conflicting setups. But
Then, the pinmux driver needs to have complete access to the pinmux core
state on what's been *requested*, not just what's actually active in HW
right now...
In particular, the kind of checks Tegra would need are (although exact
details of controller names etc, may be wrong):
* I2C controller 0 can be routed out to two different sets of pins. Only
One set of pins must be connected to the I2C controller at once.
* Foo controller 0 has two different kinds of interface to it; I2C slave
and SPI slave. Only one of these two interfaces should be actually
connected to pins at once.
I think the core pinmux code could implement some of these constraints
if provided with appropriate data from the pinmux drivers. However, I
think it'd be a good idea to defer that discussion until a little later
once we've talked through some of the other data model issues primarily
in the other email I just sent.
> The kerneldoc for request() right now looks like this:
>
> * @request: called by the core to see if a certain pin can be made available
> * available for muxing. This is called by the core to acquire the pins
> * before selecting any actual mux setting across a function. The driver
> * is allowed to answer "no" by returning a negative error code
>
> Maybe it needs some clarification?
>
> > I think the logic above should be to always call ops->gpio_request_enable
> > if (gpio):
> >
> > if (gpio)
> > if (ops->gpio_request_enable)
> > /* This requests and enables a single GPIO pin */
> > status = ops->gpio_request_enable(pmxdev,
> > pin - pmxdev->desc->base);
> > else
> > status = ops->request(pmxdev,
> > pin - pmxdev->desc->base);
> >
> > (ops->gpio_request_enable could be optional if GPIOs aren't supported on
> > any pinmux pins, whereas ops->request isn't optional, and should be checked
> > during pinmux_register)
>
> Well the idea is that if you want to reserve individual pins for GPIO
> but only need to implement one function for all pins on the mux,
> you need only implement ops->request(), this is the case for example
> with ux500 - any muxable pin works the same way since *all* muxable
> pins can also handle GPIO.
If each pin can either be just GPIO or some other function, wouldn't you
only need to implement ops->gpio_request_enable (to set whatever bit was required
for GPIO mode); you wouldn't need ops->request at all, since the non-GPIO case
was always valid?
> > Also, ops->gpio_request_enable should also be passed the GPIO number,
> > so the driver can validate that it's the correct one.
>
> Normally I think the pinmux driver should have no knowledge of
> such GPIO stuff.
>
> It's two totally separate number spaces here,
> one for pins, one for GPIO:s, I definately don't want to create
> cross-dependencies between these two.
>
> The basic assumption is that the pin number request coming in
> is sane, that the user knows what it is asking for.
Hmmm. I'd argue not to pass the GPIO number into the pinmux API at all then;
that way, there's no GPIO-related data that one has to decide whether to
validate or not.
> > Often, only
> > one value would be legal, but perhaps some pinmuxs allow 1 of N different
> > GPIOs to be mapped to some pin, so selection of which GPIO is on par with
> > selection of which special function to mux out, yet the driver still
> > doesn't want to expose every GPIO possibility as a pinmux "function" due
> > to explosion of the number of functions if it did that.
> >
> > b) When ops->gpio_request_enable is called to request a pin, it'd be nice
> > if the core could track this, and specifically call a new ops- >gpio_disable
> > to free it, rather than the generic ops->free. There are two cases in HW:
> >
> > b1) "GPIO" is just another special function in a list, e.g. SPI, I2C, GPIO.
> > In this case, free for a GPIO needs to either do nothing (the next call to
> > request will simply set the desired function), or possibly do something to
> > disable the pin in the same way as any other function. This works fine with
> > a single free function.
> >
> > b2) GPIO enable/disable is a separate concept from the pinmuxing; on Tegra
> > it'll be implemented by calling tegra_gpio_enable/disable. As such, a
> > single free function would require the pinmux driver to store state
> > indicating whether ops->free should call tegra_gpio_disable, or whether it
> > should do cleanup to the pinmux registers. Since the core is managing GPIO
> > vs. function allocation, I think the storing that state in the core makes
> > sense.
>
> I understand, I think.
>
> But I don't know if passing the gpio number into the pinmux
> driver helps with that. To me it sounds like a reason to keep
> then even more strictly separated so the pinmux driver have no
> clue whatsoever what it is muxing, it just performs the muxing.
>
> I had this similar usecase (I think from Sascha?) where two slightly
> different GPIO blocks (different drive strengths etc) would be alternatively
> muxed onto the same pins, on a per-pin basis.
I'd imagine selecting between the two GPIO blocks would be represented as
two different functions for each pin then. Of course, that runs into the
issue of an explosion in the number of functions, which brings me to:
Within a pinmux driver, the definition of a function describes both the
particular pin (or group of pins) it applies to and the functionality to
mux onto those pins.
Hence, if we want to represent "GPIO" as a function, we end up with e.g.
gpio0, gpio1, ..., gpio99 as functions; an explosion.
This is why we ended up with specific pinmux APIs for the "GPIO" case.
But, those APIs break down a little when there are 2 GPIO controllers;
pinmux_request_gpio() doesn't let you say you want to use a pin "for
GPIO controller <n>", but just "as a GPIO".
----------==========----------==========----------==========----------==========
To solve this, I propose that the definition of a function not include
any reference to a pin/pin-group.
Instead, we could have a *single* "gpio" function, that may be applied
to *any* pin/group (that supports it).
Then, to represent two GPIO controllers, you just end up with two
functions; "gpio0" and "gpio1".
This would require a pinmux driver to expose data slightly differently:
* A list of pins, each probably having a name.
Examples might be " A1", " A2", "A3", ...
* A list of functions, each probably having a name, but not list of pins.
Examples might be "I2C 0 SDA", "I2C 0 SCL", "I2C 1 SDA", "UART 0 RX", ...
* A table indicating which pins can support which function.
An example might be:
pin functions
A1 UART 0 RX, I2C 0 SDA
A2 UART 0 TX, I2C 0 SCL
...
(as an aside, this is pretty much the data model in the existing Tegra
pinmux driver; see arch/arm/mach-tegra/include/mach/pinmux*.h and
arch/arm/mach-tegra/pinmux*.c) Perhaps I'm biased, not that I wrote any
of the existing Tegra pinmux code.
Internally, each pinmux driver would need a lookup table that maps from
the tuple (pin, function) to "register value to program into HW", since
selecting a particular function on pin A might be a different register
value than selecting that same logical function on pin B.
An example might be:
pin function register_value
A1 UART 0 RX 0
A1 I2C 0 SDA 1
A2 UART 0 RX 5
...
If pinmux drivers exported this data model, then the pinmux core could
probably implement some of the error-checking I talked about above, such
as ensuring that a particular logical function was not activated on
multiple pins/groups at once.
> In that case I think it was modeled such that say GPIO pins [0-N]
> were considered the same pins [0-N] on the other chip too.
> In that case I'd say it would be more clever to say that you have
> GPIO pins [0..N] and [N+1...2N], then let the physical pins
> behind that be a different issue altogether.
>
> For these cases I generally feel we need actual code to get
> somewhere, it might get into too much premature upfront design
> otherwise. I think it's better to look at patches here.
>
> > Thanks for reading. Hopefully this email didn't jump about too much.
>
> No problem... The more I read the more I learn.
>
> Thanks,
> Linus Walleij
--
nvpublic
On Thu, Jun 16, 2011 at 15:12, Stephen Warren wrote:
> Linus Walleij wrote at Thursday, June 16, 2011 9:07 AM:
>> On Wed, Jun 15, 2011 at 11:48 PM, Stephen Warren wrote:
>> > Some more thoughts on pinmux:
can you guys start cc-ing [email protected] on
future pinmux discussions ? we already have a thin layer in the
Blackfin tree for handling pin muxing that has served us well for a
few years, so we want to keep tabs on where this is going. i just
happened to stumble across this today.
arch/blackfin/include/asm/portmux.h:
peripheral_request()
peripheral_request_list()
peripheral_free()
peripheral_free_list()
this lets you pass in an array of pins and it returns success only if
all were available. we find the array request method gets used the
vast majority of the time, so any proposed API should include that.
> Just one note on the extra stuff for the future:
>
> * Tegra has say 100 muxable pins.
> * GPIO vs. special function is selectable per pin.
> * Muxing of which special function (and some other features, such as
> pullup/down) is only at a granularity of "group of pins".
> * Some other pin features (such as slew rate, drive strength) is also
> only at a granularity of "group of pins" **but** the groups are defined
> differently for these features than for muxing.
>
> I figure the general model is:
>
> * N pins, each perhaps with some per-pin controls.
> * M groups of pins, each allowing muxing, and possibly other controls
> and also an arbitrary number of:
> * P groups of pins, allowing various other controls
some Blackfin parts have ~160 pins, but usually they only have like 16
to 64. we've got pretty much similar functionality as you describe
for Tegra.
we also handle pins that can only be used in peripheral mode. this
allows us to write code that requests UART pins without having to know
whether the pin is muxed or dedicated or somewhere in between.
>> >> diff --git a/drivers/Makefile b/drivers/Makefile
>> >>...
>> >> +# GPIO must come after pinctrl as gpios may need to mux pins etc
>> >> +obj-y += pinctrl/
>> >
>> > Don't those patches imply that the GPIO controller code is calling into
>> > the pinmux code to perform muxing, not the other way around?
>>
>> Yes.
>>
>> Grant does not seem to like the idea of the gpio subsystem
>> meddeling with all this stuff anyway, so I intend to take all that
>> into pinctrl, and then gpio only deal with, well GPIO. Setting
>> bits quickly and such.
but there has to be resource management between the two subsystems
somewhere. on the Blackfin side, if you request a pin as a GPIO using
the GPIO api, and then try to request it in peripheral mode, you get
EBUSY back. you cant have the two blocks stepping on each others
toes.
> In particular, the kind of checks Tegra would need are (although exact
> details of controller names etc, may be wrong):
>
> * I2C controller 0 can be routed out to two different sets of pins. Only
> One set of pins must be connected to the I2C controller at once.
some Blackfin parts have this to ease the conflicts between devices
that customers want. if UART0 and SPI0 share PF0, PF1, and PF2,
sometimes UART0 can be routed to PG3 and PG4 so that SPI0 can be used.
atm we've made this a Kconfig option. obviously that wont fly in the
"build one image to run on all platforms", but that isnt a problem for
Blackfin systems today, and customers have been OK with this minor
limitation.
> Internally, each pinmux driver would need a lookup table that maps from
> the tuple (pin, function) to "register value to program into HW", since
> selecting a particular function on pin A might be a different register
> value than selecting that same logical function on pin B.
>
> An example might be:
>
> pin function register_value
> A1 UART 0 RX 0
> A1 I2C 0 SDA 1
> A2 UART 0 RX 5
> ...
>
> If pinmux drivers exported this data model, then the pinmux core could
> probably implement some of the error-checking I talked about above, such
> as ensuring that a particular logical function was not activated on
> multiple pins/groups at once.
we implemented this on the Blackfin side by encoding things into the
pin data. but that's because we had enough space in u16 to cover all
of our parts so far.
for example, the PA1 pin can act as GPIO, or as a timer pin, or as
part of our high speed peripheral SPORT.
#define P_SPORT2_DTSEC (P_DEFINED | P_IDENT(GPIO_PA1) | P_FUNCT(0))
#define P_TMR4 (P_DEFINED | P_IDENT(GPIO_PA1) | P_FUNCT(1))
when someone does peripheral_request(P_TMR4), the code first reserves
itself with the GPIO core, and then configures the pinmux logic to set
this pin as "function 1" (it's a 2 bit field).
we also have pin grouping logic implemented for the parts which dont
have per-pin muxing so you cant request different pins in the same
group for the same mode.
-mike
On Fri, Jun 24, 2011 at 5:51 AM, Mike Frysinger <[email protected]> wrote:
> can you guys start cc-ing [email protected] on
> future pinmux discussions?
OK!
Please refer to the LKML thread:
[PATCH 1/2] drivers: create a pinmux subsystem v3
For now, I'll include blackfin for the v4 post.
>?we already have a thin layer in the
> Blackfin tree for handling pin muxing that has served us well for a
> few years, so we want to keep tabs on where this is going. ?i just
> happened to stumble across this today.
Aha now it's yet another name, portmux, to add to the happy family
of pinmux, padmux, fingermux, alternate functions and mission mode
terminology soup ;-)
> arch/blackfin/include/asm/portmux.h:
> peripheral_request()
> peripheral_request_list()
> peripheral_free()
> peripheral_free_list()
>
> this lets you pass in an array of pins and it returns success only if
> all were available. ?we find the array request method gets used the
> vast majority of the time, so any proposed API should include that.
I think if you look at the Documentation/pinctrl.txt file from the
last iteration of the framework you find that this is exactly what
the pinmux part of the pinctrl subsystem does. It avoids pin
clashes in a discrete number space.
If I'm not mistaken. (Which happens a lot.)
> we also handle pins that can only be used in peripheral mode. ?this
> allows us to write code that requests UART pins without having to know
> whether the pin is muxed or dedicated or somewhere in between.
This is done with pinmux_get() and pinmux_enable() in this
framework.
>>> Grant does not seem to like the idea of the gpio subsystem
>>> meddeling with all this stuff anyway, so I intend to take all that
>>> into pinctrl, and then gpio only deal with, well GPIO. Setting
>>> bits quickly and such.
>
> but there has to be resource management between the two subsystems
> somewhere. ?on the Blackfin side, if you request a pin as a GPIO using
> the GPIO api, and then try to request it in peripheral mode, you get
> EBUSY back. ?you cant have the two blocks stepping on each others
> toes.
Yes. Avoiding this is exactly the idea behind the
int (*gpio_request_enable) (struct pinmux_dev *pmxdev, unsigned offset);
member in the pinmux_ops vtable. We're discussing the exact
semantics of this call here.
We all agree we need something that can allocate a single pin for
GPIO in the number space, I think.
> some Blackfin parts have this to ease the conflicts between devices
> that customers want. ?if UART0 and SPI0 share PF0, PF1, and PF2,
> sometimes UART0 can be routed to PG3 and PG4 so that SPI0 can be used.
This kind usecase is covered extensively in the documentation
and previous discussion IIUC.
> atm we've made this a Kconfig option. ?obviously that wont fly in the
> "build one image to run on all platforms", but that isnt a problem for
> Blackfin systems today, and customers have been OK with this minor
> limitation.
I think we may be able to fix that limitation, and would like to make sure
we're not engineering in some limitation for Blackfin, so hit us.
> we implemented this on the Blackfin side by encoding things into the
> pin data. ?but that's because we had enough space in u16 to cover all
> of our parts so far.
>
> for example, the PA1 pin can act as GPIO, or as a timer pin, or as
> part of our high speed peripheral SPORT.
> #define P_SPORT2_DTSEC (P_DEFINED | P_IDENT(GPIO_PA1) | P_FUNCT(0))
> #define P_TMR4 (P_DEFINED | P_IDENT(GPIO_PA1) | P_FUNCT(1))
> when someone does peripheral_request(P_TMR4), the code first reserves
> itself with the GPIO core, and then configures the pinmux logic to set
> this pin as "function 1" (it's a 2 bit field).
In ARM we're at the stage now where each mach-foo under arch/arm/*
is running its own show in this regard. For example mach-ux500
magically stuff all its pin configuration into a u32 cleverly reusing bits
here and there.
Everyone is special, just like everybody else...
Basically there are as many pinmux implementations as there are
machines, and then we end up with that other Linus beating us on the
head for not consolidating our stuff into something generic.
So I'm doing this framework using some structs and radix trees
and no magic bitstuffing for now.
> we also have pin grouping logic implemented for the parts which dont
> have per-pin muxing so you cant request different pins in the same
> group for the same mode.
We should be able to solve that I think!
Yours,
Linus Walleij