From: Linus Walleij <[email protected]>
This makes the pinctrl subsystem auto-grab the pinctrl handle and
set the "default" (PINCTRL_STATE_DEFAULT) state for every device
that is present on the platform or AMBA (PrimeCell) bus right
before probe. This will account for the lion's share of embedded
silicon devcies. The behaviour is achieved using bus notifiers
on the platform and AMBA (PrimeCell) busses.
A modification of the semantics for pinctrl_get() is also done:
previously if the pinctrl handle for a certain device was already
taken, the pinctrl core would return an error. Now, since the
core may have already default-grabbed the handle and set its
state to "default", if the handle was already taken, this will
be disregarded and the located, previously instanitated handle
will be returned to the caller.
This way all code in drivers explicitly requesting their pinctrl
handlers will still be functional, and drivers that want to
explicitly retrieve and switch their handles can still do that.
But if the desired functionality is just boilerplate of this
type in the probe() function:
struct pinctrl *p;
p = devm_pinctrl_get_select_default(&dev);
if (IS_ERR(p)) {
if (PTR_ERR(p) == -EPROBE_DEFER)
return -EPROBE_DEFER;
dev_warn(&dev, "no pinctrl handle\n");
}
The discussion began with the addition of such boilerplate
to the omap4 keypad driver:
http://marc.info/?l=linux-input&m=135091157719300&w=2
Notice that the patch disregards deferral as per above:
if the pin controller returns -EPROBE_DEFER, there is no
way to send that back to the device core using notifiers,
so in cases where the probe ordering is to be controlled,
drivers still need to add the above pin control handling
code. So this mechanism will basically be optimistic, and
to be really sure that you don't have a deferred probe
on your pins, you still need to explicitly request the
pins.
This patch alone does not solve the entire dilemma faced:
whether code should be distributed into the drivers or
if it should be centralized to e.g. a PM domain. But it
solves the immediate issue of the addition of boilerplate
to a lot of drivers that just want to grab the default
state. As mentioned, they can later explicitly retrieve
the handle and set different states, and this could as
well be done by e.g. PM domains as it is only related
to a certain struct device * pointer.
Another solution that was discussed was whether to move
the default pinctrl handle and state grab to the device
core as an optional field in struct device itself, but
I'd like to first propose this less intrusive mechanism.
CC: Felipe Balbi <[email protected]>
Cc: Benoit Cousson <[email protected]>
Cc: Dmitry Torokhov <[email protected]>
Cc: Thomas Petazzoni <[email protected]>
Cc: Mitch Bradley <[email protected]>
Cc: Mark Brown <[email protected]>
Cc: Ulf Hansson <[email protected]>
Cc: Rafael J. Wysocki <[email protected]>
Cc: Kevin Hilman <[email protected]>
Cc: Jean-Christophe PLAGNIOL-VILLARD <[email protected]>
Cc: Rickard Andersson <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Russell King <[email protected]>
Signed-off-by: Linus Walleij <[email protected]>
---
Documentation/pinctrl.txt | 24 +++++++++++++++++--
drivers/pinctrl/core.c | 60 +++++++++++++++++++++++++++++++++++++++++++++--
2 files changed, 80 insertions(+), 4 deletions(-)
diff --git a/Documentation/pinctrl.txt b/Documentation/pinctrl.txt
index da40efb..b6c27b1 100644
--- a/Documentation/pinctrl.txt
+++ b/Documentation/pinctrl.txt
@@ -972,6 +972,19 @@ pinmux core.
Pin control requests from drivers
=================================
+When a device driver is about to probe on the platform or AMBA (PrimeCell)
+bus, the pinctrl core will automatically attempt to issue
+pinctrl_get_select_default() on these devices using the bus notification
+mechanism. This way driver writers do not need to add any of the
+boilerplate code of the type found below. However when doing fine-grained
+state selection and not using the "default" state, you may have to do
+some device driver handling of the pinctrl handles and states.
+
+So if you just want to put the pins for a certain platform or AMBA device
+into the default state and be done with it, there is nothing you need to
+do besides providing the proper mapping table. The device core will take
+care of the rest.
+
Generally it is discouraged to let individual drivers get and enable pin
control. So if possible, handle the pin control in platform code or some other
place where you have access to all the affected struct device * pointers. In
@@ -1097,8 +1110,8 @@ situations that can be electrically unpleasant, you will certainly want to
mux in and bias pins in a certain way before the GPIO subsystems starts to
deal with them.
-The above can be hidden: using pinctrl hogs, the pin control driver may be
-setting up the config and muxing for the pins when it is probing,
+The above can be hidden: using device notifiers, the pinctrl core may be
+setting up the config and muxing for the pins when the device is probing,
nevertheless orthogonal to the GPIO subsystem.
But there are also situations where it makes sense for the GPIO subsystem
@@ -1144,6 +1157,13 @@ PIN_MAP_MUX_GROUP_HOG_DEFAULT("pinctrl-foo", NULL /* group */, "power_func")
This gives the exact same result as the above construction.
+This should not be used for any kind of device which is represented in
+the device model. For platform and AMBA (PrimeCell) devices, such as found
+in many SoC:s, the pinctrl core will listen to notifications from these
+buses and attempt to do pinctrl_get_select_default() for these devices
+right before their device drivers are probed, so hogging these will just
+make the model look strange.
+
Runtime pinmuxing
=================
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index 71db586..bdb2300 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -26,6 +26,9 @@
#include <linux/pinctrl/consumer.h>
#include <linux/pinctrl/pinctrl.h>
#include <linux/pinctrl/machine.h>
+#include <linux/notifier.h>
+#include <linux/platform_device.h>
+#include <linux/amba/bus.h>
#include "core.h"
#include "devicetree.h"
#include "pinmux.h"
@@ -684,9 +687,16 @@ static struct pinctrl *pinctrl_get_locked(struct device *dev)
if (WARN_ON(!dev))
return ERR_PTR(-EINVAL);
+ /*
+ * See if somebody else (such as the pinctrl core, using the
+ * notifiers) has already obtained a handle to the pinctrl for
+ * this device. In that case, return another pointer to it.
+ */
p = find_pinctrl(dev);
- if (p != NULL)
- return ERR_PTR(-EBUSY);
+ if (p != NULL) {
+ dev_dbg(dev, "obtain a copy of previously claimed pinctrl\n");
+ return p;
+ }
return create_pinctrl(dev);
}
@@ -1026,6 +1036,52 @@ void pinctrl_unregister_map(struct pinctrl_map const *map)
}
}
+static int pinctrl_device_notify(struct notifier_block *nb,
+ unsigned long action, void *data)
+{
+ struct device *dev = data;
+ struct pinctrl *p;
+
+ switch (action) {
+ case BUS_NOTIFY_BIND_DRIVER:
+ /*
+ * Hog pins right before a driver is about to be bound to
+ * the device
+ */
+ p = devm_pinctrl_get_select_default(dev);
+ if (IS_ERR_OR_NULL(p)) {
+ dev_dbg(dev, "no pinctrl handle or default state\n");
+ return 0;
+ }
+ dev_dbg(dev, "pinctrl core obtained default pinctrl\n");
+ break;
+ case BUS_NOTIFY_UNBOUND_DRIVER:
+ break;
+ }
+
+ return 0;
+}
+
+static struct notifier_block pinctrl_device_nb = {
+ .notifier_call = pinctrl_device_notify,
+};
+
+static int __init pinctrl_notifiers_init(void)
+{
+ bus_register_notifier(&platform_bus_type, &pinctrl_device_nb);
+#ifdef CONFIG_ARM_AMBA
+ bus_register_notifier(&amba_bustype, &pinctrl_device_nb);
+#endif
+ return 0;
+}
+
+/*
+ * The AMBA bus registers in the postcore_initcall() so we need to
+ * register our notifiers right after that so we can fetch pins for
+ * e.g. TTY drivers that will be registered in the arch_initcall() next.
+ */
+postcore_initcall_sync(pinctrl_notifiers_init);
+
#ifdef CONFIG_DEBUG_FS
static int pinctrl_pins_show(struct seq_file *s, void *what)
--
1.7.11.3
On 11/11/2012 05:22 AM, Linus Walleij wrote:
> From: Linus Walleij <[email protected]>
>
> This makes the pinctrl subsystem auto-grab the pinctrl handle and
> set the "default" (PINCTRL_STATE_DEFAULT) state for every device
> that is present on the platform or AMBA (PrimeCell) bus right
> before probe. This will account for the lion's share of embedded
> silicon devcies. The behaviour is achieved using bus notifiers
> on the platform and AMBA (PrimeCell) busses.
Doing this seems reasonable.
> Notice that the patch disregards deferral as per above:
...
> Another solution that was discussed was whether to move
> the default pinctrl handle and state grab to the device
> core as an optional field in struct device itself, but
> I'd like to first propose this less intrusive mechanism.
I think doing that approach makes a lot more sense; wouldn't it
completely avoid the issues with deferred probe that this notifier-based
method can't solve? It would also be very much in line with e.g.
dev_get_regmap() - if every resource that a driver required were handled
like that, then deferred probe could be significantly isolated into the
driver core rather than in every driver...
> diff --git a/Documentation/pinctrl.txt b/Documentation/pinctrl.txt
> index da40efb..b6c27b1 100644
> --- a/Documentation/pinctrl.txt
> +++ b/Documentation/pinctrl.txt
> @@ -972,6 +972,19 @@ pinmux core.
> Pin control requests from drivers
> =================================
>
> +When a device driver is about to probe on the platform or AMBA (PrimeCell)
> +bus,
Why not make this apply to every device? I don't think the specific bus
type impacts whether pinctrl would be useful?
> @@ -1097,8 +1110,8 @@ situations that can be electrically unpleasant, you will certainly want to
> -The above can be hidden: using pinctrl hogs, the pin control driver may be
> -setting up the config and muxing for the pins when it is probing,
> +The above can be hidden: using device notifiers, the pinctrl core may be
> +setting up the config and muxing for the pins when the device is probing,
> nevertheless orthogonal to the GPIO subsystem.
Why removing "using pinctrl hogs"; can't "it" (although I didn't check
what "it" is...) be hidden using either pinctrl hogs or device notifiers?
> But there are also situations where it makes sense for the GPIO subsystem
> @@ -1144,6 +1157,13 @@ PIN_MAP_MUX_GROUP_HOG_DEFAULT("pinctrl-foo", NULL /* group */, "power_func")
>
> This gives the exact same result as the above construction.
>
> +This should not be used for any kind of device which is represented in
> +the device model. For platform and AMBA (PrimeCell) devices, such as found
> +in many SoC:s, the pinctrl core will listen to notifications from these
> +buses and attempt to do pinctrl_get_select_default() for these devices
> +right before their device drivers are probed, so hogging these will just
> +make the model look strange.
(re: the first sentence:) Why not? For the device tree case, that's
forcing DT authors to sprinkle the device tree across all devices
throughout the tree, rather than having a simple single "hog" table in
the pin controller itself. At least where there's no dynamic muxing
required, I don't think it makes any semantic difference whether the
pinctrl configuration is represented as a single "pinmux HW"
configuration, or split up per-device, and writing the single
centralized configuration is easier.
> diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
> @@ -684,9 +687,16 @@ static struct pinctrl *pinctrl_get_locked(struct device *dev)
> if (WARN_ON(!dev))
> return ERR_PTR(-EINVAL);
>
> + /*
> + * See if somebody else (such as the pinctrl core, using the
> + * notifiers) has already obtained a handle to the pinctrl for
> + * this device. In that case, return another pointer to it.
> + */
> p = find_pinctrl(dev);
> - if (p != NULL)
> - return ERR_PTR(-EBUSY);
> + if (p != NULL) {
> + dev_dbg(dev, "obtain a copy of previously claimed pinctrl\n");
> + return p;
You'd want to implement full reference-counting then; IIRC, that's why I
banned pinctrl_get() running twice in the original code.
On Mon, Nov 12, 2012 at 01:21:40PM -0700, Stephen Warren wrote:
> On 11/11/2012 05:22 AM, Linus Walleij wrote:
> > Another solution that was discussed was whether to move
> > the default pinctrl handle and state grab to the device
> > core as an optional field in struct device itself, but
> > I'd like to first propose this less intrusive mechanism.
> I think doing that approach makes a lot more sense; wouldn't it
> completely avoid the issues with deferred probe that this notifier-based
> method can't solve? It would also be very much in line with e.g.
> dev_get_regmap() - if every resource that a driver required were handled
> like that, then deferred probe could be significantly isolated into the
> driver core rather than in every driver...
I have to say that I agree with this, notifiers seem to make life more
complicated for limited gain. Otherwise I guess we could enhance
notifiers so that they're able to trigger deferrals?
On Tue, Nov 13, 2012 at 7:35 AM, Mark Brown
<[email protected]> wrote:
> On Mon, Nov 12, 2012 at 01:21:40PM -0700, Stephen Warren wrote:
>> On 11/11/2012 05:22 AM, Linus Walleij wrote:
>
>> > Another solution that was discussed was whether to move
>> > the default pinctrl handle and state grab to the device
>> > core as an optional field in struct device itself, but
>> > I'd like to first propose this less intrusive mechanism.
>
>> I think doing that approach makes a lot more sense; wouldn't it
>> completely avoid the issues with deferred probe that this notifier-based
>> method can't solve? It would also be very much in line with e.g.
>> dev_get_regmap() - if every resource that a driver required were handled
>> like that, then deferred probe could be significantly isolated into the
>> driver core rather than in every driver...
>
> I have to say that I agree with this, notifiers seem to make life more
> complicated for limited gain. Otherwise I guess we could enhance
> notifiers so that they're able to trigger deferrals?
OK I'll have to come up with a patch to the device core
instead... it'll be much simpler anyway and if both of you guys
can back it I guess Greg might be OK with it too.
Yours,
Linus Walleij
Linus,
On Thu, 15 Nov 2012 15:03:40 +0100, Linus Walleij wrote:
> > I have to say that I agree with this, notifiers seem to make life more
> > complicated for limited gain. Otherwise I guess we could enhance
> > notifiers so that they're able to trigger deferrals?
>
> OK I'll have to come up with a patch to the device core
> instead... it'll be much simpler anyway and if both of you guys
> can back it I guess Greg might be OK with it too.
At ELCE, I've discussed with one person having an interesting use case:
they are using the same pin for two different purposes during the
system operation.
At boot time, this pin is muxed as a SPI pin and is used to program the
bitstream of a FPGA. This is done in Linux, through a userspace
application.
Then, once it is done, this pin is remuxed as a UART pin, and used to
communicate with another device on the board.
If the pinctrl configuration is done at probe() time inside the driver
core, then how can this kind of use case be supported? If each driver
does its own muxing, we could think of letting the SPI and UART driver
only do their muxing when the device is opened, and remove their muxing
when the device is closed, so that such sharing of pins for two
separate functions could be supported. Is this something we want to
support?
Best regards,
Thomas
--
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
On Thu, Nov 15, 2012 at 3:26 PM, Thomas Petazzoni
<[email protected]> wrote:
> At ELCE, I've discussed with one person having an interesting use case:
> they are using the same pin for two different purposes during the
> system operation.
>
> At boot time, this pin is muxed as a SPI pin and is used to program the
> bitstream of a FPGA. This is done in Linux, through a userspace
> application.
>
> Then, once it is done, this pin is remuxed as a UART pin, and used to
> communicate with another device on the board.
This is not related to the current discussion but interesting
anyway...
> If the pinctrl configuration is done at probe() time inside the driver
> core, then how can this kind of use case be supported? If each driver
> does its own muxing, we could think of letting the SPI and UART driver
> only do their muxing when the device is opened, and remove their muxing
> when the device is closed, so that such sharing of pins for two
> separate functions could be supported. Is this something we want to
> support?
This is trivial to support after this commit in the current patch
queue:
commit 1a78958dc212f3698fdc543857af80155cb30f7f
pinctrl: reserve pins when states are activated
Just define function maps for both devices using the same
pins. Since the devices need to actively release their
pins they need to go to a state which does not mux in
any pins, so that the pins will be free:ed.
This state can be any arbitrary string, but if so desired
for consistency we can define something like
#define PINCTRL_STATE_DECOUPLED "decoupled"
in include/linux/pinctrl/pinctrl-state.h to clearly
mark that this is a state where the device is not
using any pins at all.
Yours,
Linus Walleij
Yours,
Linus Walleij
On 11/15/2012 07:03 AM, Linus Walleij wrote:
> On Tue, Nov 13, 2012 at 7:35 AM, Mark Brown
> <[email protected]> wrote:
>> On Mon, Nov 12, 2012 at 01:21:40PM -0700, Stephen Warren wrote:
>>> On 11/11/2012 05:22 AM, Linus Walleij wrote:
>>
>>>> Another solution that was discussed was whether to move
>>>> the default pinctrl handle and state grab to the device
>>>> core as an optional field in struct device itself, but
>>>> I'd like to first propose this less intrusive mechanism.
>>
>>> I think doing that approach makes a lot more sense; wouldn't it
>>> completely avoid the issues with deferred probe that this notifier-based
>>> method can't solve? It would also be very much in line with e.g.
>>> dev_get_regmap() - if every resource that a driver required were handled
>>> like that, then deferred probe could be significantly isolated into the
>>> driver core rather than in every driver...
>>
>> I have to say that I agree with this, notifiers seem to make life more
>> complicated for limited gain. Otherwise I guess we could enhance
>> notifiers so that they're able to trigger deferrals?
>
> OK I'll have to come up with a patch to the device core
> instead... it'll be much simpler anyway and if both of you guys
> can back it I guess Greg might be OK with it too.
I did have one thought here; how will this interact with hogs? If a
device's pinctrl configuration must be pinctrl_get()'d before the device
is probed, then a pinctrl device with hogs will never get probed because
it won't be registered to provide the pinctrl node parsing. Solutions
might include:
a) Some special case where if the pinctrl driver only can't probe due to
missing pinctrl from its own node, don't defer the probe, but defer the
pinctrl_get().
b) Separate out DT node parsing from device instantiation, so that the
driver can always parse the DT, without needing the context of a
specific pinctrl device to do so.
I haven't thought through this in any detail though.
On 11/15/2012 10:24 AM, Linus Walleij wrote:
> On Thu, Nov 15, 2012 at 3:26 PM, Thomas Petazzoni
> <[email protected]> wrote:
>
>> At ELCE, I've discussed with one person having an interesting use case:
>> they are using the same pin for two different purposes during the
>> system operation.
>>
>> At boot time, this pin is muxed as a SPI pin and is used to program the
>> bitstream of a FPGA. This is done in Linux, through a userspace
>> application.
>>
>> Then, once it is done, this pin is remuxed as a UART pin, and used to
>> communicate with another device on the board.
>
> This is not related to the current discussion but interesting
> anyway...
>
>> If the pinctrl configuration is done at probe() time inside the driver
>> core, then how can this kind of use case be supported? If each driver
>> does its own muxing, we could think of letting the SPI and UART driver
>> only do their muxing when the device is opened, and remove their muxing
>> when the device is closed, so that such sharing of pins for two
>> separate functions could be supported. Is this something we want to
>> support?
>
> This is trivial to support after this commit in the current patch
> queue:
> commit 1a78958dc212f3698fdc543857af80155cb30f7f
> pinctrl: reserve pins when states are activated
>
> Just define function maps for both devices using the same
> pins. Since the devices need to actively release their
> pins they need to go to a state which does not mux in
> any pins, so that the pins will be free:ed.
>
> This state can be any arbitrary string, but if so desired
> for consistency we can define something like
> #define PINCTRL_STATE_DECOUPLED "decoupled"
> in include/linux/pinctrl/pinctrl-state.h to clearly
> mark that this is a state where the device is not
> using any pins at all.
Well, you'd also need to explicitly code the pinmux logic into the UART
driver, so that the UART's default state didn't prevent the SPI driver
from temporarily acquiring the pin to program the FPGA. It's rather
nasty to put that into the UART driver, which would presumably otherwise
be completely generic and not need runtime pinmux state changes. I
wonder if interjecting a mux driver between the UART/SPI and the pins
would help here, although presumably the UART driver would still need
some way of interacting with the mux driver which wouldn't be any better...
On Thu, Nov 15, 2012 at 7:23 PM, Stephen Warren <[email protected]> wrote:
> On 11/15/2012 07:03 AM, Linus Walleij wrote:
>> OK I'll have to come up with a patch to the device core
>> instead... it'll be much simpler anyway and if both of you guys
>> can back it I guess Greg might be OK with it too.
>
> I did have one thought here; how will this interact with hogs? If a
> device's pinctrl configuration must be pinctrl_get()'d before the device
> is probed, then a pinctrl device with hogs will never get probed because
> it won't be registered to provide the pinctrl node parsing.
Catch 22 :-(
Yeah we need to come up with something there.
> Solutions might include:
>
> a) Some special case where if the pinctrl driver only can't probe due to
> missing pinctrl from its own node, don't defer the probe, but defer the
> pinctrl_get().
>
> b) Separate out DT node parsing from device instantiation, so that the
> driver can always parse the DT, without needing the context of a
> specific pinctrl device to do so.
But this mechanism can't be device tree-specific, we have some
of olschool pdata users and ACPI probing around the corner.
I will likely just cook up something like seeing if the
dev_name() for provider and consumer is the same and
in that case avoid deferral.
Oh well, I'll try to actually code this now...
Yours,
Linus Walleij
On 11/16/2012 04:36 AM, Linus Walleij wrote:
> On Thu, Nov 15, 2012 at 7:23 PM, Stephen Warren <[email protected]> wrote:
>> On 11/15/2012 07:03 AM, Linus Walleij wrote:
>
>>> OK I'll have to come up with a patch to the device core
>>> instead... it'll be much simpler anyway and if both of you guys
>>> can back it I guess Greg might be OK with it too.
>>
>> I did have one thought here; how will this interact with hogs? If a
>> device's pinctrl configuration must be pinctrl_get()'d before the device
>> is probed, then a pinctrl device with hogs will never get probed because
>> it won't be registered to provide the pinctrl node parsing.
>
> Catch 22 :-(
>
> Yeah we need to come up with something there.
>
>> Solutions might include:
>>
>> a) Some special case where if the pinctrl driver only can't probe due to
>> missing pinctrl from its own node, don't defer the probe, but defer the
>> pinctrl_get().
>>
>> b) Separate out DT node parsing from device instantiation, so that the
>> driver can always parse the DT, without needing the context of a
>> specific pinctrl device to do so.
>
> But this mechanism can't be device tree-specific, we have some
> of olschool pdata users and ACPI probing around the corner.
>
> I will likely just cook up something like seeing if the
> dev_name() for provider and consumer is the same and
> in that case avoid deferral.
Well, the DT parsing is all about creating the mapping table from the
DT. Without DT, this doesn't need to happen, since the board file
already contains the complete mapping table. So, this isn't so much a
special case for DT, but rather simply a step we don't need to take for
DT, if you get what I mean (and that's all true irrespective of the
change we're discussing here).