| Message ID | 1350551224-12857-3-git-send-email-haojian.zhuang@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Oct 18, 2012 at 11:06 AM, Haojian Zhuang <haojian.zhuang@gmail.com> wrote: > Configure pins by pinctrl driver. > > Signed-off-by: Haojian Zhuang <haojian.zhuang@gmail.com> Acked-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On 10/18/2012 03:06 AM, Haojian Zhuang wrote:
> Configure pins by pinctrl driver.
In general, it seems better to use pinctrl "hogs" if the driver is only
going to statically set up one pinctrl state and never change it. The
alternative is make every single driver execute these pinctrl calls,
which could be tedious.
However, that does raise one question: If all the pinctrl setup is done
by hogs, then how do we ensure that the pinctrl driver is probed first,
and hence sets up the pins before any driver relies on them being set
up? If a driver explicitly enables a pinctrl state (as in this patch),
then deferred probe ensures that, but without any explicit pinctrl
action, it'll only work by luck.
LinusW, what are your thoughts on that?
On Fri, Oct 19, 2012 at 12:20 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 10/18/2012 03:06 AM, Haojian Zhuang wrote: >> Configure pins by pinctrl driver. > > In general, it seems better to use pinctrl "hogs" if the driver is only > going to statically set up one pinctrl state and never change it. The > alternative is make every single driver execute these pinctrl calls, > which could be tedious. True. And each platform has to choose how to go ahead with this. I always imagined that any system of the "power socket in wall" type would just use hogs to configure its pins and be done with it, and there appear to be some platforms like that. (e.g. MIPS and various power-inaware references come to mind). For the Ux500 drivers we have found that all of those that have pin resources actually have to be handled by the driver. The reason is that all of them have idle and/or sleep states that need to be handled at runtime. As an additional complication our drivers are used also by the Versatile and SPEAr family of platforms, so the control need to be tolerant (accept missing pinctrl handles and states) as well. (I can see that other drivers take shortcuts by being less elaborate since there is a 1:1 driver<->platform mapping.) An alternative to embedding the pin handling code into the drivers is to use bus notifiers as is done with the shmobile clocking by drivers/base/power/clock_ops.c I could easily imagine pinctrl_ops.c like that for some platforms. This mandates still binding the pin table entries to a device but avoids adding any code to the drivers. However this latter approach does not work for us (Ux500) - the three resources we have: clocks, pins and power domains are dependent on state switch ordering (sometimes you need to switch off the clock then set pin state, sometimes the other way around) and it is not even the same for all drivers - the notifier approach mandates that all drivers do the clock, power domain and pin handling uniformly. > However, that does raise one question: If all the pinctrl setup is done > by hogs, then how do we ensure that the pinctrl driver is probed first, > and hence sets up the pins before any driver relies on them being set > up? If a driver explicitly enables a pinctrl state (as in this patch), > then deferred probe ensures that, but without any explicit pinctrl > action, it'll only work by luck. Yes, since there are no explicit dependencies with hogs it is implicitly decoupled and you only know that the hogging will happen whenever the pin controller driver is probed. We have many such pieces of code in the kernel for sure, but I agree it's not always very elegant :-/ If using the bus nofifier approach I described above the listener can just listen to BUS_NOTIFY_BIND_DRIVER and BUS_NOTIFY_UNBOUND_DRIVER and hog/unhog the drivers' pins at this point, but as described this approach has other drawbacks. Yours, Linus Walleij
On 10/22/2012 02:45 AM, Linus Walleij wrote: > On Fri, Oct 19, 2012 at 12:20 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: > >> On 10/18/2012 03:06 AM, Haojian Zhuang wrote: >>> Configure pins by pinctrl driver. >> >> In general, it seems better to use pinctrl "hogs" if the driver is only >> going to statically set up one pinctrl state and never change it. The >> alternative is make every single driver execute these pinctrl calls, >> which could be tedious. > > True. And each platform has to choose how to go ahead with this. > > I always imagined that any system of the "power socket in wall" > type would just use hogs to configure its pins and be done with > it, and there appear to be some platforms like that. (e.g. MIPS and > various power-inaware references come to mind). > > For the Ux500 drivers we have found that all of those that have pin > resources actually have to be handled by the driver. The reason is > that all of them have idle and/or sleep states that need to be > handled at runtime. Well, presumably the pinctrl driver itself could have both default/idle states, and system sleep could engage the appropriate system-wide setting. > As an additional complication our drivers are used also by > the Versatile and SPEAr family of platforms, so the control > need to be tolerant (accept missing pinctrl handles and states) > as well. (I can see that other drivers take shortcuts by being less > elaborate since there is a 1:1 driver<->platform mapping.) Isn't the driver (or DT binding) supposed to define what pinctrl states must exist, and those state must always exist? There's no requirement for a pinctrl state definition to always actually contain content that changes the state. That's exactly why PIN_MAP_TYPE_DUMMY_STATE exists. > An alternative to embedding the pin handling code into > the drivers is to use bus notifiers as is done with the > shmobile clocking by drivers/base/power/clock_ops.c > I could easily imagine pinctrl_ops.c like that for some > platforms. This mandates still binding the pin table entries > to a device but avoids adding any code to the drivers. > > However this latter approach does not work for us (Ux500) - > the three resources we have: clocks, pins and power domains > are dependent on state switch ordering (sometimes you need > to switch off the clock then set pin state, sometimes the > other way around) and it is not even > the same for all drivers - the notifier approach mandates > that all drivers do the clock, power domain and pin handling > uniformly. That certainly does imply that individual drivers do need to handle this explicitly. Although I still think that only specific drivers actually affected by this should end up actively managing pinctrl; shouldn't anything that can get away with relying on system hogs do so?
On Mon, Oct 22, 2012 at 10:26 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 10/22/2012 02:45 AM, Linus Walleij wrote: >> For the Ux500 drivers we have found that all of those that have pin >> resources actually have to be handled by the driver. The reason is >> that all of them have idle and/or sleep states that need to be >> handled at runtime. > > Well, presumably the pinctrl driver itself could have both default/idle > states, and system sleep could engage the appropriate system-wide setting. I thought about that, but it does not allow us to control the resource activation/deactivation order. In some drivers the suspend() path could be: - pins to sleep - clk_disable - disable external regulator - release power domain In others: - clk_disable() - disable external regulator - pins to sleep() - release power domain Ulf Hansson ran into this a while back. Just like with the notifier approach, this approach assumes that either the ordering above doesn't matter, or that it is the same for all drivers. >> As an additional complication our drivers are used also by >> the Versatile and SPEAr family of platforms, so the control >> need to be tolerant (accept missing pinctrl handles and states) >> as well. (I can see that other drivers take shortcuts by being less >> elaborate since there is a 1:1 driver<->platform mapping.) > > Isn't the driver (or DT binding) supposed to define what pinctrl states > must exist, and those state must always exist? There's no requirement > for a pinctrl state definition to always actually contain content that > changes the state. That's exactly why PIN_MAP_TYPE_DUMMY_STATE exists. Well, it could also define that e.g. the "sleep" state is optional. I'm sort of scared about imposing too strict usage patterns as it may cause more problems than it solves even if the code does look simpler. >> An alternative to embedding the pin handling code into >> the drivers is to use bus notifiers as is done with the >> shmobile clocking by drivers/base/power/clock_ops.c >> I could easily imagine pinctrl_ops.c like that for some >> platforms. This mandates still binding the pin table entries >> to a device but avoids adding any code to the drivers. >> >> However this latter approach does not work for us (Ux500) - >> the three resources we have: clocks, pins and power domains >> are dependent on state switch ordering (sometimes you need >> to switch off the clock then set pin state, sometimes the >> other way around) and it is not even >> the same for all drivers - the notifier approach mandates >> that all drivers do the clock, power domain and pin handling >> uniformly. > > That certainly does imply that individual drivers do need to handle this > explicitly. Although I still think that only specific drivers actually > affected by this should end up actively managing pinctrl; shouldn't > anything that can get away with relying on system hogs do so? I don't know. Having say regulator, clock and pin handling in the driver itself also gives a nice encapsulated view and sense of control when just reading that one driver. As one developer put it he does not really like that bus notifiers are doing things behind his back. The debugging of the driver can become very hairy, as you trace through bus notifiers etc to find out what is wrong with your pins/clock/voltage. We don't have hogs, bus notifiers and the like for regulators, and IIRC Mark disliked the idea. Basically I think it's better if all resources are atleast handled the same way. Yours, Linus Walleij
On Tue, Oct 23, 2012 at 11:26:40AM +0200, Linus Walleij wrote: > I thought about that, but it does not allow us to control the > resource activation/deactivation order. > In some drivers the suspend() path could be: > - pins to sleep > - clk_disable > - disable external regulator > - release power domain > In others: > - clk_disable() > - disable external regulator > - pins to sleep() > - release power domain > Ulf Hansson ran into this a while back. > Just like with the notifier approach, this approach assumes that either > the ordering above doesn't matter, or that it is the same for all drivers. Can we handle this with power domains - if different devices require different orderings they can be placed in power domains which implement the appropriate orderings for them? > We don't have hogs, bus notifiers and the like for regulators, > and IIRC Mark disliked the idea. Basically I think it's better if > all resources are atleast handled the same way. It'd be OK to have something that was manually activated for specific regulators but it's not sane to try and do something generic as you'll end up powering off all your wakeup sources which tends not to work so well.
On Tue, Oct 23, 2012 at 11:37 AM, Mark Brown <broonie@opensource.wolfsonmicro.com> wrote: > On Tue, Oct 23, 2012 at 11:26:40AM +0200, Linus Walleij wrote: >> I thought about that, but it does not allow us to control the >> resource activation/deactivation order. > >> In some drivers the suspend() path could be: > >> - pins to sleep >> - clk_disable >> - disable external regulator >> - release power domain > >> In others: > >> - clk_disable() >> - disable external regulator >> - pins to sleep() >> - release power domain > >> Ulf Hansson ran into this a while back. > >> Just like with the notifier approach, this approach assumes that either >> the ordering above doesn't matter, or that it is the same for all drivers. > > Can we handle this with power domains - if different devices require > different orderings they can be placed in power domains which implement > the appropriate orderings for them? clocks, regulators, pins and all in power domains? Then we should rename them "device resource domains" or something... I have a strong sense of system-wide all-or-nothing approaches to this problem. - Either we use "power" domains to handle every resource the device has. - Or the device driver manages it's own resources. I find it pretty hard to build consensus around either idea. The problem is that the latter approach (devices drivers themselves take clocks, domain power, etc) is already quite widespread in the kernel so to get the entire world consistent to the former approach would be pretty painful I think. Especially in the case where a device driver is used on more than one SoC. Yours, Linus Walleij
On Tue, Oct 23, 2012 at 11:59:30AM +0200, Linus Walleij wrote: > On Tue, Oct 23, 2012 at 11:37 AM, Mark Brown > > On Tue, Oct 23, 2012 at 11:26:40AM +0200, Linus Walleij wrote: > > Can we handle this with power domains - if different devices require > > different orderings they can be placed in power domains which implement > > the appropriate orderings for them? > clocks, regulators, pins and all in power domains? > Then we should rename them "device resource domains" or > something... We can call them Ichabod for all I care... > I have a strong sense of system-wide all-or-nothing approaches > to this problem. > - Either we use "power" domains to handle every resource the > device has. > - Or the device driver manages it's own resources. > I find it pretty hard to build consensus around either idea. Well, I don't think we want all or nothing approaches here. One problem is that we don't have a home for the SoC integration so we're trying to shove it all into the drivers which just leads to a stack of pointless boilerplate when the driver isn't actually doing anything beyond the basic pattern of turning everything off when it goes idle and turning it on again when it needs to do something. Having to open code that stuff in the drivers and then deal with the stubbing and error handling so the error handling in the drivers is painful. There's also another axis where things aren't part of a SoC but are separate devices so you want to carry things along with the driver rather than have a separate bit of code which is required to glue things into the platform. For example it seems fairly clear to me that things like the pinctrl integration I see in something like sound/soc/fsl/imx-audmux.c shouldn't really be there as we're just setting up a static configration on boot. On the other hand where things are directly involved with the active operation of a device like changing clock rates then clearly the driver needs to know about and manage things.
On Tue, Oct 23, 2012 at 1:58 PM, Mark Brown <broonie@opensource.wolfsonmicro.com> wrote: > One problem > is that we don't have a home for the SoC integration so we're trying to > shove it all into the drivers which just leads to a stack of pointless > boilerplate when the driver isn't actually doing anything beyond the > basic pattern of turning everything off when it goes idle and turning it > on again when it needs to do something. Having to open code that stuff > in the drivers and then deal with the stubbing and error handling so the > error handling in the drivers is painful. There's also another axis > where things aren't part of a SoC but are separate devices so you want > to carry things along with the driver rather than have a separate bit of > code which is required to glue things into the platform. I agree. I'm thinking about the approach of adding helpers into the PM runtime layer so state handling is centralized, while only the event trigger goes into the driver. Basically it's just the implicit triggers from pm_runtime_[get|put][[_sync] that is causing problems. Yours, Linus Walleij
diff --git a/drivers/tty/serial/pxa.c b/drivers/tty/serial/pxa.c index 5847a4b..ee6118a 100644 --- a/drivers/tty/serial/pxa.c +++ b/drivers/tty/serial/pxa.c @@ -37,6 +37,7 @@ #include <linux/delay.h> #include <linux/interrupt.h> #include <linux/of.h> +#include <linux/pinctrl/consumer.h> #include <linux/platform_device.h> #include <linux/tty.h> #include <linux/tty_flip.h> @@ -795,6 +796,7 @@ static int serial_pxa_probe_dt(struct platform_device *pdev, struct uart_pxa_port *sport) { struct device_node *np = pdev->dev.of_node; + struct pinctrl *pinctrl; int ret; if (!np) @@ -805,6 +807,12 @@ static int serial_pxa_probe_dt(struct platform_device *pdev, dev_err(&pdev->dev, "failed to get alias id, errno %d\n", ret); return ret; } + pinctrl = devm_pinctrl_get_select_default(&pdev->dev); + if (IS_ERR(pinctrl)) { + ret = PTR_ERR(pinctrl); + return ret; + } + sport->port.line = ret; return 0; }
Configure pins by pinctrl driver. Signed-off-by: Haojian Zhuang <haojian.zhuang@gmail.com> --- drivers/tty/serial/pxa.c | 8 ++++++++ 1 files changed, 8 insertions(+), 0 deletions(-)