| Message ID | 1370439873-30053-3-git-send-email-linus.walleij@stericsson.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Linus Walleij <linus.walleij@stericsson.com> writes: > From: Linus Walleij <linus.walleij@linaro.org> > > This utilize the new pinctrl core PM helpers to transition > the driver to "sleep" and "idle" states, cutting away some > boilerplate code. > > Cc: Hebbar Gururaja <gururaja.hebbar@ti.com> > Cc: Mark Brown <broonie@kernel.org> > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> > Cc: Kevin Hilman <khilman@linaro.org> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: Stephen Warren <swarren@wwwdotorg.org> > Cc: Wolfram Sang <wsa@the-dreams.de> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> I have some questions on the interaction with runtime PM here... > --- > I'm seeking Wolfram's ACK on this to take it through the > pinctrl tree in the end. > --- > drivers/i2c/busses/i2c-nomadik.c | 90 +++++----------------------------------- > 1 file changed, 10 insertions(+), 80 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c > index 650293f..c7e3b0c 100644 > --- a/drivers/i2c/busses/i2c-nomadik.c > +++ b/drivers/i2c/busses/i2c-nomadik.c > @@ -148,10 +148,6 @@ struct i2c_nmk_client { > * @stop: stop condition. > * @xfer_complete: acknowledge completion for a I2C message. > * @result: controller propogated result. > - * @pinctrl: pinctrl handle. > - * @pins_default: default state for the pins. > - * @pins_idle: idle state for the pins. > - * @pins_sleep: sleep state for the pins. > * @busy: Busy doing transfer. > */ > struct nmk_i2c_dev { > @@ -165,11 +161,6 @@ struct nmk_i2c_dev { > int stop; > struct completion xfer_complete; > int result; > - /* Three pin states - default, idle & sleep */ > - struct pinctrl *pinctrl; > - struct pinctrl_state *pins_default; > - struct pinctrl_state *pins_idle; > - struct pinctrl_state *pins_sleep; > bool busy; > }; > > @@ -645,13 +636,7 @@ static int nmk_i2c_xfer(struct i2c_adapter *i2c_adap, > } > > /* Optionaly enable pins to be muxed in and configured */ > - if (!IS_ERR(dev->pins_default)) { > - status = pinctrl_select_state(dev->pinctrl, > - dev->pins_default); > - if (status) > - dev_err(&dev->adev->dev, > - "could not set default pins\n"); > - } > + pinctrl_pm_select_default_state(&dev->adev->dev); Shouldn't this be in the ->runtime_resume() callback of the driver (the original code should've as well.) IOW, the pinctrl changes only need to happen when the runtime PM usecount goes from zero to 1. For any additional users, the device will already be active and pins already in default state. I'm not familiar with this HW, and maybe the driver already prevents multiple users, but for correctness sake (and because others will copy this), the (re)muxing should be in the runtime PM callback. Also, IMO, that's further evidence that the pinctrl stuff could (and probably should) be handled by the PM core. > status = init_hw(dev); > if (status) > @@ -681,13 +666,7 @@ out: > clk_disable_unprepare(dev->clk); > out_clk: > /* Optionally let pins go into idle state */ > - if (!IS_ERR(dev->pins_idle)) { > - status = pinctrl_select_state(dev->pinctrl, > - dev->pins_idle); > - if (status) > - dev_err(&dev->adev->dev, > - "could not set pins to idle state\n"); > - } > + pinctrl_pm_select_idle_state(&dev->adev->dev); Again, if there are multiple users, you're changing the pin states without knowing if you're the last user or not (again, the problem existed in the original code as well.) Runtime PM is doing the usecouning, so this should be in the ->runtime_suspend() callback. > pm_runtime_put_sync(&dev->adev->dev); > Kevin
On Wed, Jun 5, 2013 at 6:34 PM, Kevin Hilman <khilman@linaro.org> wrote: >> From: Linus Walleij <linus.walleij@linaro.org> >> >> This utilize the new pinctrl core PM helpers to transition >> the driver to "sleep" and "idle" states, cutting away some >> boilerplate code. >> >> Cc: Hebbar Gururaja <gururaja.hebbar@ti.com> >> Cc: Mark Brown <broonie@kernel.org> >> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> >> Cc: Kevin Hilman <khilman@linaro.org> >> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >> Cc: Stephen Warren <swarren@wwwdotorg.org> >> Cc: Wolfram Sang <wsa@the-dreams.de> >> Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > > I have some questions on the interaction with runtime PM here... OK, to get the basic infrastructure in place I have merged these patches with the I2C maintainers ACK, since it is doing one thing, i.e. moving the functionality out of the driver and into the pinctrl core. I am considering semantic changes related to runtime PM in addition to this as a separate patch, so let's start talking about that here. It would be inappropriate to try to create a patch that is changing these two things at once, but let's see where we end up by the merge window. >> @@ -645,13 +636,7 @@ static int nmk_i2c_xfer(struct i2c_adapter *i2c_adap, >> } >> >> /* Optionaly enable pins to be muxed in and configured */ >> - if (!IS_ERR(dev->pins_default)) { >> - status = pinctrl_select_state(dev->pinctrl, >> - dev->pins_default); >> - if (status) >> - dev_err(&dev->adev->dev, >> - "could not set default pins\n"); >> - } >> + pinctrl_pm_select_default_state(&dev->adev->dev); > > Shouldn't this be in the ->runtime_resume() callback of the driver (the > original code should've as well.) > > IOW, the pinctrl changes only need to happen when the runtime PM > usecount goes from zero to 1. For any additional users, the device will > already be active and pins already in default state. > > I'm not familiar with this HW, and maybe the driver already prevents > multiple users, but for correctness sake (and because others will copy > this), the (re)muxing should be in the runtime PM callback. I2C message are serialized/marshalled by nature so it's actually not causing a concurrency problem: this xfer function will not be called from two places for the same driver. What is true however is that we're hammering the pins from active to idle for every transfer, instead of letting runtime PM provide some nice hysteresis (autosuspend) around that. Notice though that: - This driver has no driver-local runtime PM callbacks, so the runtime PM calls are intended to inform the rest of the system, such as the bus, that the device is idle. - The bus used is the AMBA (PrimeCell) bus, drivers/amba/bus.c > Also, IMO, that's further evidence that the pinctrl stuff could (and > probably should) be handled by the PM core. So I'm now thinking about how to achieve this. What happens for this driver when the usecount goes from 1->0 is (the other way is very similar): drivers/base/power/runtime.c if (dev->pm_domain) callback = dev->pm_domain->ops.runtime_suspend; else if (dev->type && dev->type->pm) callback = dev->type->pm->runtime_suspend; else if (dev->class && dev->class->pm) callback = dev->class->pm->runtime_suspend; else if (dev->bus && dev->bus->pm) callback = dev->bus->pm->runtime_suspend; else callback = NULL; if (!callback && dev->driver && dev->driver->pm) callback = dev->driver->pm->runtime_suspend; retval = rpm_callback(callback, dev); This platform will currently hit dev->bus->pm->runtime_suspend to drivers/amba/bus.c: static int amba_pm_runtime_suspend(struct device *dev) { struct amba_device *pcdev = to_amba_device(dev); int ret = pm_generic_runtime_suspend(dev); if (ret == 0 && dev->driver) clk_disable(pcdev->pclk); return ret; } The pm_generic_runtime_suspend will call the driver callbacks (none in this case). Then the bus core proceeds to gate off the block clock and we're done. I could make a patch adding runtime PM ops to the driver itself to set the pinctrl state from there, which would be a nice improvement in itself. But we're discussing handling it all in the PM core, so let's think bigger. If we're making this all generic, were in this chain do you suggest that I set the pins to idle? drivers/base/power/runtime.c? One thing in particular that worries me here is the ordering of things, because that has been a severe issue for us. For example: maybe on a certain platform pins need to be idled/defaulted *before* calling the PM domain or bus callbacks are executed, because of transient IRQs and whatnot. So I put my pinctrl_pm_select_idle_state() *before* the chain of calls. But sometimes you may want to execute the pinctrl_pm_select_idle_state() *after* all other things have been done, including the calls to the domain/bus/driver. And this is only for the runtime suspend/resume path. For the common suspend/resume path things get more complex still. Users may need to call pinctrl_pm_select_sleep_state() in the middle of the code sending the platform done, and will not survive it being called by the PM core, and we'd need to add a flag for this etc. To sum up I am afraid of a can of worms of corner cases on something that looks simple here. Thus I cannot really make a patch moving pinctrl state selection to the PM core, I don't know the business there well enough, I just know there are tigers in there :-/ Yours, Linus Walleij
Linus Walleij <linus.walleij@linaro.org> writes: > On Wed, Jun 5, 2013 at 6:34 PM, Kevin Hilman <khilman@linaro.org> wrote: > >>> From: Linus Walleij <linus.walleij@linaro.org> >>> >>> This utilize the new pinctrl core PM helpers to transition >>> the driver to "sleep" and "idle" states, cutting away some >>> boilerplate code. >>> >>> Cc: Hebbar Gururaja <gururaja.hebbar@ti.com> >>> Cc: Mark Brown <broonie@kernel.org> >>> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> >>> Cc: Kevin Hilman <khilman@linaro.org> >>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >>> Cc: Stephen Warren <swarren@wwwdotorg.org> >>> Cc: Wolfram Sang <wsa@the-dreams.de> >>> Signed-off-by: Linus Walleij <linus.walleij@linaro.org> >> >> I have some questions on the interaction with runtime PM here... > > OK, to get the basic infrastructure in place I have merged these > patches with the I2C maintainers ACK, since it is doing one thing, > i.e. moving the functionality out of the driver and into the pinctrl > core. > > I am considering semantic changes related to runtime PM in > addition to this as a separate patch, so let's start talking about > that here. > > It would be inappropriate to try to create a patch that is > changing these two things at once, but let's see where we end > up by the merge window. > >>> @@ -645,13 +636,7 @@ static int nmk_i2c_xfer(struct i2c_adapter *i2c_adap, >>> } >>> >>> /* Optionaly enable pins to be muxed in and configured */ >>> - if (!IS_ERR(dev->pins_default)) { >>> - status = pinctrl_select_state(dev->pinctrl, >>> - dev->pins_default); >>> - if (status) >>> - dev_err(&dev->adev->dev, >>> - "could not set default pins\n"); >>> - } >>> + pinctrl_pm_select_default_state(&dev->adev->dev); >> >> Shouldn't this be in the ->runtime_resume() callback of the driver (the >> original code should've as well.) >> >> IOW, the pinctrl changes only need to happen when the runtime PM >> usecount goes from zero to 1. For any additional users, the device will >> already be active and pins already in default state. >> >> I'm not familiar with this HW, and maybe the driver already prevents >> multiple users, but for correctness sake (and because others will copy >> this), the (re)muxing should be in the runtime PM callback. > > I2C message are serialized/marshalled by nature so it's actually > not causing a concurrency problem: this xfer function will not be > called from two places for the same driver. > > What is true however is that we're hammering the pins from > active to idle for every transfer, instead of letting runtime PM > provide some nice hysteresis (autosuspend) around that. > > Notice though that: > > - This driver has no driver-local runtime PM callbacks, so the > runtime PM calls are intended to inform the rest of the system, > such as the bus, that the device is idle. > > - The bus used is the AMBA (PrimeCell) bus, > drivers/amba/bus.c > >> Also, IMO, that's further evidence that the pinctrl stuff could (and >> probably should) be handled by the PM core. > > So I'm now thinking about how to achieve this. > > What happens for this driver when the usecount goes from > 1->0 is (the other way is very similar): > > drivers/base/power/runtime.c > > if (dev->pm_domain) > callback = dev->pm_domain->ops.runtime_suspend; > else if (dev->type && dev->type->pm) > callback = dev->type->pm->runtime_suspend; > else if (dev->class && dev->class->pm) > callback = dev->class->pm->runtime_suspend; > else if (dev->bus && dev->bus->pm) > callback = dev->bus->pm->runtime_suspend; > else > callback = NULL; > > if (!callback && dev->driver && dev->driver->pm) > callback = dev->driver->pm->runtime_suspend; > > retval = rpm_callback(callback, dev); > > This platform will currently hit dev->bus->pm->runtime_suspend > to drivers/amba/bus.c: > > static int amba_pm_runtime_suspend(struct device *dev) > { > struct amba_device *pcdev = to_amba_device(dev); > int ret = pm_generic_runtime_suspend(dev); > > if (ret == 0 && dev->driver) > clk_disable(pcdev->pclk); > > return ret; > } > > The pm_generic_runtime_suspend will call the driver callbacks > (none in this case). > > Then the bus core proceeds to gate off the block clock and > we're done. > > I could make a patch adding runtime PM ops to the > driver itself to set the pinctrl state from there, which would > be a nice improvement in itself. > > But we're discussing handling it all in the PM core, so > let's think bigger. > > If we're making this all generic, were in this chain do you > suggest that I set the pins to idle? > drivers/base/power/runtime.c? Yes, that's where I was thinking. > One thing in particular that worries me here is the ordering > of things, because that has been a severe issue for us. In the original series from Hebbar Gururaja, Grygorii Strashko pointed out that we may have some of the ordering issues on OMAP as well. I hadn't thought about that (enough) yet. > For example: maybe on a certain platform pins need to > be idled/defaulted *before* calling the PM domain or > bus callbacks are executed, because of transient IRQs > and whatnot. So I put my pinctrl_pm_select_idle_state() > *before* the chain of calls. Right, that corresponds to the runtime PM core calling the drivers ->runtime_suspend() callback before the subsystem/bus/domain has done its thing. > But sometimes you may want to execute the > pinctrl_pm_select_idle_state() *after* all other things have > been done, including the calls to the domain/bus/driver. Whether it's in the PM core or not, with runtime PM today, there is no easy way to do this from the driver ($SUBJECT patch assumes a single user, which is not true in general.) The only way a driver truly knows that the domain/bus/subsystem calls have been done is when its own callback is called, and for suspend this only happens *before* the device is actually idled. This is effectively a pre-runtime_suspend callback. We don't currently have a post-runtime_suspend callback (or a pre-runtime_resume callback.) Might be we need something like that to do this in a generic way. > And this is only for the runtime suspend/resume path. > > For the common suspend/resume path things get more > complex still. Users may need to call > pinctrl_pm_select_sleep_state() in the middle of the > code sending the platform done, and will not survive it > being called by the PM core, and we'd need to add a flag > for this etc. > > To sum up I am afraid of a can of worms of corner cases > on something that looks simple here. Thus I cannot really > make a patch moving pinctrl state selection to the PM > core, I don't know the business there well enough, I just know > there are tigers in there :-/ Yeah, the static PM case is definitely a can of worms, especially in the case where many devices are already runtime suspended. That defnitely needs some serious thought and testing before handling in the PM core. So for now, it's probably not a good idea to move things to the PM core until we see a strong pattern emerging. That being said, maybe we could handle this in the subsystem/bus/pm_domain level though (in your case AMBA, in OMAP case the pm_domain) to still avoid sprinkling this across all the drivers? Kevin
diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c index 650293f..c7e3b0c 100644 --- a/drivers/i2c/busses/i2c-nomadik.c +++ b/drivers/i2c/busses/i2c-nomadik.c @@ -148,10 +148,6 @@ struct i2c_nmk_client { * @stop: stop condition. * @xfer_complete: acknowledge completion for a I2C message. * @result: controller propogated result. - * @pinctrl: pinctrl handle. - * @pins_default: default state for the pins. - * @pins_idle: idle state for the pins. - * @pins_sleep: sleep state for the pins. * @busy: Busy doing transfer. */ struct nmk_i2c_dev { @@ -165,11 +161,6 @@ struct nmk_i2c_dev { int stop; struct completion xfer_complete; int result; - /* Three pin states - default, idle & sleep */ - struct pinctrl *pinctrl; - struct pinctrl_state *pins_default; - struct pinctrl_state *pins_idle; - struct pinctrl_state *pins_sleep; bool busy; }; @@ -645,13 +636,7 @@ static int nmk_i2c_xfer(struct i2c_adapter *i2c_adap, } /* Optionaly enable pins to be muxed in and configured */ - if (!IS_ERR(dev->pins_default)) { - status = pinctrl_select_state(dev->pinctrl, - dev->pins_default); - if (status) - dev_err(&dev->adev->dev, - "could not set default pins\n"); - } + pinctrl_pm_select_default_state(&dev->adev->dev); status = init_hw(dev); if (status) @@ -681,13 +666,7 @@ out: clk_disable_unprepare(dev->clk); out_clk: /* Optionally let pins go into idle state */ - if (!IS_ERR(dev->pins_idle)) { - status = pinctrl_select_state(dev->pinctrl, - dev->pins_idle); - if (status) - dev_err(&dev->adev->dev, - "could not set pins to idle state\n"); - } + pinctrl_pm_select_idle_state(&dev->adev->dev); pm_runtime_put_sync(&dev->adev->dev); @@ -882,41 +861,22 @@ static int nmk_i2c_suspend(struct device *dev) { struct amba_device *adev = to_amba_device(dev); struct nmk_i2c_dev *nmk_i2c = amba_get_drvdata(adev); - int ret; if (nmk_i2c->busy) return -EBUSY; - if (!IS_ERR(nmk_i2c->pins_sleep)) { - ret = pinctrl_select_state(nmk_i2c->pinctrl, - nmk_i2c->pins_sleep); - if (ret) - dev_err(dev, "could not set pins to sleep state\n"); - } + pinctrl_pm_select_sleep_state(dev); return 0; } static int nmk_i2c_resume(struct device *dev) { - struct amba_device *adev = to_amba_device(dev); - struct nmk_i2c_dev *nmk_i2c = amba_get_drvdata(adev); - int ret; - /* First go to the default state */ - if (!IS_ERR(nmk_i2c->pins_default)) { - ret = pinctrl_select_state(nmk_i2c->pinctrl, - nmk_i2c->pins_default); - if (ret) - dev_err(dev, "could not set pins to default state\n"); - } + pinctrl_pm_select_default_state(dev); /* Then let's idle the pins until the next transfer happens */ - if (!IS_ERR(nmk_i2c->pins_idle)) { - ret = pinctrl_select_state(nmk_i2c->pinctrl, - nmk_i2c->pins_idle); - if (ret) - dev_err(dev, "could not set pins to idle state\n"); - } + pinctrl_pm_select_idle_state(dev); + return 0; } #else @@ -1004,39 +964,10 @@ static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id) dev->adev = adev; amba_set_drvdata(adev, dev); - dev->pinctrl = devm_pinctrl_get(&adev->dev); - if (IS_ERR(dev->pinctrl)) { - ret = PTR_ERR(dev->pinctrl); - goto err_pinctrl; - } - - dev->pins_default = pinctrl_lookup_state(dev->pinctrl, - PINCTRL_STATE_DEFAULT); - if (IS_ERR(dev->pins_default)) { - dev_err(&adev->dev, "could not get default pinstate\n"); - } else { - ret = pinctrl_select_state(dev->pinctrl, - dev->pins_default); - if (ret) - dev_dbg(&adev->dev, "could not set default pinstate\n"); - } - - dev->pins_idle = pinctrl_lookup_state(dev->pinctrl, - PINCTRL_STATE_IDLE); - if (IS_ERR(dev->pins_idle)) { - dev_dbg(&adev->dev, "could not get idle pinstate\n"); - } else { - /* If possible, let's go to idle until the first transfer */ - ret = pinctrl_select_state(dev->pinctrl, - dev->pins_idle); - if (ret) - dev_dbg(&adev->dev, "could not set idle pinstate\n"); - } - - dev->pins_sleep = pinctrl_lookup_state(dev->pinctrl, - PINCTRL_STATE_SLEEP); - if (IS_ERR(dev->pins_sleep)) - dev_dbg(&adev->dev, "could not get sleep pinstate\n"); + /* Select default pin state */ + pinctrl_pm_select_default_state(&adev->dev); + /* If possible, let's go to idle until the first transfer */ + pinctrl_pm_select_idle_state(&adev->dev); dev->virtbase = ioremap(adev->res.start, resource_size(&adev->res)); if (!dev->virtbase) { @@ -1106,7 +1037,6 @@ static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id) iounmap(dev->virtbase); err_no_ioremap: kfree(dev); - err_pinctrl: err_no_mem: return ret;