| Message ID | 1352636539-6318-1-git-send-email-linus.walleij@stericsson.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 11/11/2012 05:22 AM, Linus Walleij wrote: > From: Linus Walleij <linus.walleij@linaro.org> > > 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 <broonie@opensource.wolfsonmicro.com> 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
On Thu, Nov 15, 2012 at 3:26 PM, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> 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 > <broonie@opensource.wolfsonmicro.com> 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 > <thomas.petazzoni@free-electrons.com> 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 <swarren@wwwdotorg.org> 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 <swarren@wwwdotorg.org> 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).
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)