| Message ID | 20170301183257.6554-1-f.fainelli@gmail.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Mar 1, 2017 at 8:32 PM, Florian Fainelli <f.fainelli@gmail.com> wrote: > In case a platform only defaults a "default" set of pins, but not a > "sleep" set of pins, and this particular platform suspends and resumes > in a way that the pin states are not preserved by the hardware, when we > resume, we would call pinctrl_single_resume() -> pinctrl_force_default() > -> pinctrl_select_state() and the first thing we do is check that the > pins state is the same as before, and do nothing. > > In order to fix this, decouple the actual state change from > pinctrl_select_state() and move it pinctrl_commit_state(), while keeping > the p->state == state check in pinctrl_select_state() not to change the > caller assumptions. pinctrl_force_sleep() and pinctrl_force_default() > are updated to bypass the state check by calling pinctrl_commit_state(). > > Fixes: 6e5e959dde0d ("pinctrl: API changes to support multiple states per device") > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> This change is indeed beautiful and clean. FWIW: Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > --- > Changes in v3: > > - move the state check to pinctrl_select_state > > Changes in v2: > > - rename __pinctrl_select_state to pinctrl_commit_state > > drivers/pinctrl/core.c | 24 +++++++++++++++++------- > 1 file changed, 17 insertions(+), 7 deletions(-) > > diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c > index fb38e208f32d..735d8f7f9d71 100644 > --- a/drivers/pinctrl/core.c > +++ b/drivers/pinctrl/core.c > @@ -992,19 +992,16 @@ struct pinctrl_state *pinctrl_lookup_state(struct pinctrl *p, > EXPORT_SYMBOL_GPL(pinctrl_lookup_state); > > /** > - * pinctrl_select_state() - select/activate/program a pinctrl state to HW > + * pinctrl_commit_state() - select/activate/program a pinctrl state to HW > * @p: the pinctrl handle for the device that requests configuration > * @state: the state handle to select/activate/program > */ > -int pinctrl_select_state(struct pinctrl *p, struct pinctrl_state *state) > +static int pinctrl_commit_state(struct pinctrl *p, struct pinctrl_state *state) > { > struct pinctrl_setting *setting, *setting2; > struct pinctrl_state *old_state = p->state; > int ret; > > - if (p->state == state) > - return 0; > - > if (p->state) { > /* > * For each pinmux setting in the old state, forget SW's record > @@ -1068,6 +1065,19 @@ int pinctrl_select_state(struct pinctrl *p, struct pinctrl_state *state) > > return ret; > } > + > +/** > + * pinctrl_select_state() - select/activate/program a pinctrl state to HW > + * @p: the pinctrl handle for the device that requests configuration > + * @state: the state handle to select/activate/program > + */ > +int pinctrl_select_state(struct pinctrl *p, struct pinctrl_state *state) > +{ > + if (p->state == state) > + return 0; > + > + return pinctrl_commit_state(p, state); > +} > EXPORT_SYMBOL_GPL(pinctrl_select_state); > > static void devm_pinctrl_release(struct device *dev, void *res) > @@ -1236,7 +1246,7 @@ void pinctrl_unregister_map(struct pinctrl_map const *map) > int pinctrl_force_sleep(struct pinctrl_dev *pctldev) > { > if (!IS_ERR(pctldev->p) && !IS_ERR(pctldev->hog_sleep)) > - return pinctrl_select_state(pctldev->p, pctldev->hog_sleep); > + return pinctrl_commit_state(pctldev->p, pctldev->hog_sleep); > return 0; > } > EXPORT_SYMBOL_GPL(pinctrl_force_sleep); > @@ -1248,7 +1258,7 @@ EXPORT_SYMBOL_GPL(pinctrl_force_sleep); > int pinctrl_force_default(struct pinctrl_dev *pctldev) > { > if (!IS_ERR(pctldev->p) && !IS_ERR(pctldev->hog_default)) > - return pinctrl_select_state(pctldev->p, pctldev->hog_default); > + return pinctrl_commit_state(pctldev->p, pctldev->hog_default); > return 0; > } > EXPORT_SYMBOL_GPL(pinctrl_force_default); > -- > 2.9.3 >
On 03/02/2017 12:54 AM, Andy Shevchenko wrote: > On Wed, Mar 1, 2017 at 8:32 PM, Florian Fainelli <f.fainelli@gmail.com> wrote: >> In case a platform only defaults a "default" set of pins, but not a >> "sleep" set of pins, and this particular platform suspends and resumes >> in a way that the pin states are not preserved by the hardware, when we >> resume, we would call pinctrl_single_resume() -> pinctrl_force_default() >> -> pinctrl_select_state() and the first thing we do is check that the >> pins state is the same as before, and do nothing. >> >> In order to fix this, decouple the actual state change from >> pinctrl_select_state() and move it pinctrl_commit_state(), while keeping >> the p->state == state check in pinctrl_select_state() not to change the >> caller assumptions. pinctrl_force_sleep() and pinctrl_force_default() >> are updated to bypass the state check by calling pinctrl_commit_state(). >> >> Fixes: 6e5e959dde0d ("pinctrl: API changes to support multiple states per device") >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > > This change is indeed beautiful and clean. > > FWIW: > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> Thanks Andy! So actually, I have been thinking about this some more, and while this definitively fixes my original problem with pinctrl-single, I just had to make another driver (sdhci-brcmstb.c) pinctrl aware. And then again, same thing, it has got just one "default" state, so when we call pinctrl_select_state() during driver resume, nothing happens. So, with that in mind, it seems to me like we may actually just want to remove the p->state == state statement entirely, even though that's an optimization.... ... or, what we could do is, expose a version of pinctrl_force_default that takes a struct pinctrl reference instead of a struct pinctrl_dev reference and named differently of course. Thoughts? > > >> --- >> Changes in v3: >> >> - move the state check to pinctrl_select_state >> >> Changes in v2: >> >> - rename __pinctrl_select_state to pinctrl_commit_state >> >> drivers/pinctrl/core.c | 24 +++++++++++++++++------- >> 1 file changed, 17 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c >> index fb38e208f32d..735d8f7f9d71 100644 >> --- a/drivers/pinctrl/core.c >> +++ b/drivers/pinctrl/core.c >> @@ -992,19 +992,16 @@ struct pinctrl_state *pinctrl_lookup_state(struct pinctrl *p, >> EXPORT_SYMBOL_GPL(pinctrl_lookup_state); >> >> /** >> - * pinctrl_select_state() - select/activate/program a pinctrl state to HW >> + * pinctrl_commit_state() - select/activate/program a pinctrl state to HW >> * @p: the pinctrl handle for the device that requests configuration >> * @state: the state handle to select/activate/program >> */ >> -int pinctrl_select_state(struct pinctrl *p, struct pinctrl_state *state) >> +static int pinctrl_commit_state(struct pinctrl *p, struct pinctrl_state *state) >> { >> struct pinctrl_setting *setting, *setting2; >> struct pinctrl_state *old_state = p->state; >> int ret; >> >> - if (p->state == state) >> - return 0; >> - >> if (p->state) { >> /* >> * For each pinmux setting in the old state, forget SW's record >> @@ -1068,6 +1065,19 @@ int pinctrl_select_state(struct pinctrl *p, struct pinctrl_state *state) >> >> return ret; >> } >> + >> +/** >> + * pinctrl_select_state() - select/activate/program a pinctrl state to HW >> + * @p: the pinctrl handle for the device that requests configuration >> + * @state: the state handle to select/activate/program >> + */ >> +int pinctrl_select_state(struct pinctrl *p, struct pinctrl_state *state) >> +{ >> + if (p->state == state) >> + return 0; >> + >> + return pinctrl_commit_state(p, state); >> +} >> EXPORT_SYMBOL_GPL(pinctrl_select_state); >> >> static void devm_pinctrl_release(struct device *dev, void *res) >> @@ -1236,7 +1246,7 @@ void pinctrl_unregister_map(struct pinctrl_map const *map) >> int pinctrl_force_sleep(struct pinctrl_dev *pctldev) >> { >> if (!IS_ERR(pctldev->p) && !IS_ERR(pctldev->hog_sleep)) >> - return pinctrl_select_state(pctldev->p, pctldev->hog_sleep); >> + return pinctrl_commit_state(pctldev->p, pctldev->hog_sleep); >> return 0; >> } >> EXPORT_SYMBOL_GPL(pinctrl_force_sleep); >> @@ -1248,7 +1258,7 @@ EXPORT_SYMBOL_GPL(pinctrl_force_sleep); >> int pinctrl_force_default(struct pinctrl_dev *pctldev) >> { >> if (!IS_ERR(pctldev->p) && !IS_ERR(pctldev->hog_default)) >> - return pinctrl_select_state(pctldev->p, pctldev->hog_default); >> + return pinctrl_commit_state(pctldev->p, pctldev->hog_default); >> return 0; >> } >> EXPORT_SYMBOL_GPL(pinctrl_force_default); >> -- >> 2.9.3 >> > > >
On Thu, Mar 2, 2017 at 11:19 PM, Florian Fainelli <f.fainelli@gmail.com> wrote: > So actually, I have been thinking about this some more, and while this > definitively fixes my original problem with pinctrl-single, I just had > to make another driver (sdhci-brcmstb.c) pinctrl aware. And then again, > same thing, it has got just one "default" state, so when we call > pinctrl_select_state() during driver resume, nothing happens. > > So, with that in mind, it seems to me like we may actually just want to > remove the p->state == state statement entirely, even though that's an > optimization.... > > ... or, what we could do is, expose a version of pinctrl_force_default > that takes a struct pinctrl reference instead of a struct pinctrl_dev > reference and named differently of course. > > Thoughts? The root problem that the patch is trying to solve is that the hardware loose state when going to sleep, without the pin control core being informed about this, correct? And that is why the state is then "forced" with this patch, when setting default state: the core think we are already in "default" state, and thus the hack force it to be written down to the hardware again. But it is IMO just papering over the real bug: that the core does not know that the state is lost. What I think is the proper solution is to add a callback to switch the state in the core. The most obvious would be to use the API as many already do: define "sleep" states in the core, and switch to these before going to sleep. If CONFIG_PM is available simply by calling pinctrl_pm_select_sleep_state() in the driver suspend() callback. I think this is the most intuitive and clean solution. Alternatively we would add a function to set the pinctrl handle to an "unknown" state, so that when we resume, the pinctrl core at least knows that we are not in "default" state anymore, so that "default" is applied. So then we would add: /** * pinctrl_set_unknown_state() - tell the pinctrl core that we lost the state */ void pinctrl_set_unknown_state(struct device *dev) { struct dev_pin_info *pins = dev->pins; if (!pins) return 0; pins->p->state = NULL; } I imagine this would give the right results on the suspend path. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03/14/2017 03:16 AM, Linus Walleij wrote: > On Thu, Mar 2, 2017 at 11:19 PM, Florian Fainelli <f.fainelli@gmail.com> wrote: > >> So actually, I have been thinking about this some more, and while this >> definitively fixes my original problem with pinctrl-single, I just had >> to make another driver (sdhci-brcmstb.c) pinctrl aware. And then again, >> same thing, it has got just one "default" state, so when we call >> pinctrl_select_state() during driver resume, nothing happens. >> >> So, with that in mind, it seems to me like we may actually just want to >> remove the p->state == state statement entirely, even though that's an >> optimization.... >> >> ... or, what we could do is, expose a version of pinctrl_force_default >> that takes a struct pinctrl reference instead of a struct pinctrl_dev >> reference and named differently of course. >> >> Thoughts? > > The root problem that the patch is trying to solve is that the hardware > loose state when going to sleep, without the pin control core > being informed about this, correct? That is exactly what is happening. > > And that is why the state is then "forced" with this patch, when > setting default state: the core think we are already in "default" > state, and thus the hack force it to be written down to the hardware > again. Correct. > > But it is IMO just papering over the real bug: that the core does > not know that the state is lost. Yes, agree with that statement. > > What I think is the proper solution is to add a callback to switch > the state in the core. > > The most obvious would be to use the API as many already do: > define "sleep" states in the core, and switch to these before > going to sleep. If CONFIG_PM is available simply by calling > pinctrl_pm_select_sleep_state() in the driver suspend() callback. Well, the difficulty for our platforms is that S2 does not make the HW lose pin states, only S3 does and drivers should be agnostic of S2 vs. S3. There is not really a "sleep" and "default" state defined for these platforms just the "default" state. I initially even considered adding a fake "sleep" state just to satisfy the state transition condition, but that does not accurately represent the HW. > > I think this is the most intuitive and clean solution. > > Alternatively we would add a function to set the pinctrl handle to > an "unknown" state, so that when we resume, the pinctrl core at > least knows that we are not in "default" state anymore, so that > "default" is applied. And such a function would be called during driver suspend? Would not we still end-up with the drivers having to know about the fact that there is a) only one pin state defined, and b) these pins potentially lose their states in some deep sleep mode? > > So then we would add: > > /** > * pinctrl_set_unknown_state() - tell the pinctrl core that we lost the state > */ > void pinctrl_set_unknown_state(struct device *dev) { > struct dev_pin_info *pins = dev->pins; > > if (!pins) > return 0; > > pins->p->state = NULL; > } > > I imagine this would give the right results on the suspend path. Humm, why not, but I am not clear how I would call that nor under which conditions from both the pinctrl-single driver and another one that we use (sdhci-brcmstb.c for instance, but it could be any pinctrl consumer really)? Thanks! > > Yours, > Linus Walleij >
On Wed, Mar 15, 2017 at 3:18 AM, Florian Fainelli <f.fainelli@gmail.com> wrote: > On 03/14/2017 03:16 AM, Linus Walleij wrote: >> The most obvious would be to use the API as many already do: >> define "sleep" states in the core, and switch to these before >> going to sleep. If CONFIG_PM is available simply by calling >> pinctrl_pm_select_sleep_state() in the driver suspend() callback. > > Well, the difficulty for our platforms is that S2 does not make the HW > lose pin states, only S3 does and drivers should be agnostic of S2 vs. S3. > > There is not really a "sleep" and "default" state defined for these > platforms just the "default" state. I initially even considered adding a > fake "sleep" state just to satisfy the state transition condition, but > that does not accurately represent the HW. Do you mean that on the way up, on the resume path, you know whether the setting was lost or not? Or you don't know it anywhere? It is not less elegant to uncessesarily switch to a sleep state than to unnecessarily program the default state when you only went into S2 in that case. I guess then it is better to assume we will loose the state, or push for more granular handling of S2/3 etc states in the PM core (I guess these states comes from ACPI or similar). >> Alternatively we would add a function to set the pinctrl handle to >> an "unknown" state, so that when we resume, the pinctrl core at >> least knows that we are not in "default" state anymore, so that >> "default" is applied. > > And such a function would be called during driver suspend? Would not we > still end-up with the drivers having to know about the fact that there > is a) only one pin state defined, and b) these pins potentially lose > their states in some deep sleep mode? Again, the proposal to switch to default state twice just because we do not know how deep sleep we went into isn't any more elegant. Then it is better to just assume we lost the state at all times. Alternatively develop the PM core. Is it really impossible for PM hooks to know which state it went into/came from? Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
(sorry for the lag) On 03/16/2017 07:08 AM, Linus Walleij wrote: > On Wed, Mar 15, 2017 at 3:18 AM, Florian Fainelli <f.fainelli@gmail.com> wrote: >> On 03/14/2017 03:16 AM, Linus Walleij wrote: > >>> The most obvious would be to use the API as many already do: >>> define "sleep" states in the core, and switch to these before >>> going to sleep. If CONFIG_PM is available simply by calling >>> pinctrl_pm_select_sleep_state() in the driver suspend() callback. >> >> Well, the difficulty for our platforms is that S2 does not make the HW >> lose pin states, only S3 does and drivers should be agnostic of S2 vs. S3. >> >> There is not really a "sleep" and "default" state defined for these >> platforms just the "default" state. I initially even considered adding a >> fake "sleep" state just to satisfy the state transition condition, but >> that does not accurately represent the HW. > > Do you mean that on the way up, on the resume path, you know> whether the setting was lost or not? In S3 we loose the hardware contents, and in S2 we do not. A platform device driver has no way (currently) in its suspend/resume callback to know which state was entered/exited. > > Or you don't know it anywhere? > > It is not less elegant to uncessesarily switch to a sleep state > than to unnecessarily program the default state when you only > went into S2 in that case. Agreed, but defining a sleep state that does not exist just to force a transition to the default state upon resumption is not really elegant. > > I guess then it is better to assume we will loose the state, or > push for more granular handling of S2/3 etc states in the > PM core (I guess these states comes from ACPI or similar). I expected to see pm_message_t reflect which state we were entering into (PM_SUSPEND_STANDBY vs. PM_SUSPEND_MEM), but that is not the case. > >>> Alternatively we would add a function to set the pinctrl handle to >>> an "unknown" state, so that when we resume, the pinctrl core at >>> least knows that we are not in "default" state anymore, so that >>> "default" is applied. >> >> And such a function would be called during driver suspend? Would not we >> still end-up with the drivers having to know about the fact that there >> is a) only one pin state defined, and b) these pins potentially lose >> their states in some deep sleep mode? > > Again, the proposal to switch to default state twice just because > we do not know how deep sleep we went into isn't any more > elegant. Then it is better to just assume we lost the state at > all times. > > Alternatively develop the PM core. Is it really impossible for > PM hooks to know which state it went into/came from? I don't think I liked Rafael's suggestion of putting that kind of detail into the platform_suspend_ops routine as he seems to suggest here: https://www.spinics.net/lists/arm-kernel/msg587311.html and here is my response: https://www.spinics.net/lists/arm-kernel/msg589844.html
Sorry for slowness... On Wed, Jun 21, 2017 at 11:23 PM, Florian Fainelli <f.fainelli@gmail.com> wrote: > On 03/16/2017 07:08 AM, Linus Walleij wrote: >> I guess then it is better to assume we will loose the state, or >> push for more granular handling of S2/3 etc states in the >> PM core (I guess these states comes from ACPI or similar). > > I expected to see pm_message_t reflect which state we were entering into > (PM_SUSPEND_STANDBY vs. PM_SUSPEND_MEM), but that is not the case. Can we fix it? >> Alternatively develop the PM core. Is it really impossible for >> PM hooks to know which state it went into/came from? > > I don't think I liked Rafael's suggestion of putting that kind of detail > into the platform_suspend_ops routine as he seems to suggest here: > > https://www.spinics.net/lists/arm-kernel/msg587311.html He is suggesting: > The cleanest way would be to run that code from one of the platform > suspend hooks that receive information on what sleep state is to be > entered. But what I suggest is more the inverse: that it receive information on what state it is coming from, rather than which state it is going to. But I guess it would be logical that suspend() get to know what state it is going to and resume() get to know which state it is coming from. So Rafael seem to be aligned with that idea. > and here is my response: > > https://www.spinics.net/lists/arm-kernel/msg589844.html So if it is not desireable to have every driver know which exact state it came from like S3 this or S2 that and on this laptop we have S2' which is slightly different and such mess (that you predict IIUC) what we really need to know is pretty simple: did the hardware loose its state or not? That is the information we want the PM core to provide to the resume() callback, somehow. A simple bool is fine. Any platform specifics or simplifications pertaining to certain states and whether S5 or S7 looses the context should not be the concern of a driver, what it wants to know is simply whether its device has been powered off and lost its hardware context. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/29/2017 02:17 AM, Linus Walleij wrote: > Sorry for slowness... > > On Wed, Jun 21, 2017 at 11:23 PM, Florian Fainelli <f.fainelli@gmail.com> wrote: >> On 03/16/2017 07:08 AM, Linus Walleij wrote: > >>> I guess then it is better to assume we will loose the state, or >>> push for more granular handling of S2/3 etc states in the >>> PM core (I guess these states comes from ACPI or similar). >> >> I expected to see pm_message_t reflect which state we were entering into >> (PM_SUSPEND_STANDBY vs. PM_SUSPEND_MEM), but that is not the case. > > Can we fix it? Yes, I proposed this and got no feedback so far: https://www.spinics.net/lists/arm-kernel/msg590135.html > >>> Alternatively develop the PM core. Is it really impossible for >>> PM hooks to know which state it went into/came from? >> >> I don't think I liked Rafael's suggestion of putting that kind of detail >> into the platform_suspend_ops routine as he seems to suggest here: >> >> https://www.spinics.net/lists/arm-kernel/msg587311.html > > He is suggesting: > >> The cleanest way would be to run that code from one of the platform >> suspend hooks that receive information on what sleep state is to be >> entered. > > But what I suggest is more the inverse: that it receive information > on what state it is coming from, rather than which state it is > going to. The same information is available and it won't change from one suspend cycle to resume, since in between these calls you are supposed to be... suspended. > > But I guess it would be logical that suspend() get to know what state > it is going to and resume() get to know which state it is coming from.> > So Rafael seem to be aligned with that idea. > >> and here is my response: >> >> https://www.spinics.net/lists/arm-kernel/msg589844.html > > So if it is not desireable to have every driver know which exact > state it came from like S3 this or S2 that and on this laptop > we have S2' which is slightly different and such mess (that you > predict IIUC) what we really need to know is pretty simple: > did the hardware loose its state or not? That information is inherently platform specific though, so on platforms where pinctrl-single is used, you won't necessarily know whether the state should be restored (conversely saved) so maybe that means we should have the possibility for a platform to define a wrapper around pinctrl-single whose purpose is to implement platform specific suspend/r/resume functions and just that really? Is there such a driver already that uses pinctrl-single more as a "library" than anything else? > > That is the information we want the PM core to provide to > the resume() callback, somehow. A simple bool is fine. > > Any platform specifics or simplifications pertaining to certain > states and whether S5 or S7 looses the context should not > be the concern of a driver, what it wants to know is simply > whether its device has been powered off and lost its hardware > context. > > Yours, > Linus Walleij >
On Thu, Jun 29, 2017 at 9:38 PM, Florian Fainelli <f.fainelli@gmail.com> wrote: > On 06/29/2017 02:17 AM, Linus Walleij wrote: >> So if it is not desireable to have every driver know which exact >> state it came from like S3 this or S2 that and on this laptop >> we have S2' which is slightly different and such mess (that you >> predict IIUC) what we really need to know is pretty simple: >> did the hardware loose its state or not? > > That information is inherently platform specific though, so on platforms > where pinctrl-single is used, you won't necessarily know whether the > state should be restored (conversely saved) so maybe that means we > should have the possibility for a platform to define a wrapper around > pinctrl-single whose purpose is to implement platform specific > suspend/r/resume functions and just that really? Is there such a driver > already that uses pinctrl-single more as a "library" than anything else? pinctrl-single maye not be the easiest in cases like this, but Tony et al have bolted in a few OMAP specifics to pinctrl-single in the past so I don't see why this would be a problem. commit 02e483f66deb6bd8df6af450726574614eb53be3 "pinctrl: single: Prepare for supporting SoC specific features" etc. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi all, On 2017-03-01 18:32, Florian Fainelli wrote: > In case a platform only defaults a "default" set of pins, but not a > "sleep" set of pins, and this particular platform suspends and > resumes > in a way that the pin states are not preserved by the hardware, when > we > resume, we would call pinctrl_single_resume() -> > pinctrl_force_default() > -> pinctrl_select_state() and the first thing we do is check that the > pins state is the same as before, and do nothing. > > In order to fix this, decouple the actual state change from > pinctrl_select_state() and move it pinctrl_commit_state(), while > keeping > the p->state == state check in pinctrl_select_state() not to change > the > caller assumptions. pinctrl_force_sleep() and pinctrl_force_default() > are updated to bypass the state check by calling > pinctrl_commit_state(). > > Fixes: 6e5e959dde0d ("pinctrl: API changes to support multiple states > per device") > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > --- > Changes in v3: > > - move the state check to pinctrl_select_state > > Changes in v2: > > - rename __pinctrl_select_state to pinctrl_commit_state > > drivers/pinctrl/core.c | 24 +++++++++++++++++------- > 1 file changed, 17 insertions(+), 7 deletions(-) > > diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c > index fb38e208f32d..735d8f7f9d71 100644 > --- a/drivers/pinctrl/core.c > +++ b/drivers/pinctrl/core.c > @@ -992,19 +992,16 @@ struct pinctrl_state > *pinctrl_lookup_state(struct pinctrl *p, > EXPORT_SYMBOL_GPL(pinctrl_lookup_state); > > /** > - * pinctrl_select_state() - select/activate/program a pinctrl state > to HW > + * pinctrl_commit_state() - select/activate/program a pinctrl state > to HW > * @p: the pinctrl handle for the device that requests configuration > * @state: the state handle to select/activate/program > */ > -int pinctrl_select_state(struct pinctrl *p, struct pinctrl_state > *state) > +static int pinctrl_commit_state(struct pinctrl *p, struct > pinctrl_state *state) > { > struct pinctrl_setting *setting, *setting2; > struct pinctrl_state *old_state = p->state; > int ret; > > - if (p->state == state) > - return 0; > - > if (p->state) { > /* > * For each pinmux setting in the old state, forget SW's record > @@ -1068,6 +1065,19 @@ int pinctrl_select_state(struct pinctrl *p, > struct pinctrl_state *state) > > return ret; > } > + > +/** > + * pinctrl_select_state() - select/activate/program a pinctrl state > to HW > + * @p: the pinctrl handle for the device that requests configuration > + * @state: the state handle to select/activate/program > + */ > +int pinctrl_select_state(struct pinctrl *p, struct pinctrl_state > *state) > +{ > + if (p->state == state) > + return 0; > + > + return pinctrl_commit_state(p, state); > +} > EXPORT_SYMBOL_GPL(pinctrl_select_state); > > static void devm_pinctrl_release(struct device *dev, void *res) > @@ -1236,7 +1246,7 @@ void pinctrl_unregister_map(struct pinctrl_map > const *map) > int pinctrl_force_sleep(struct pinctrl_dev *pctldev) > { > if (!IS_ERR(pctldev->p) && !IS_ERR(pctldev->hog_sleep)) > - return pinctrl_select_state(pctldev->p, pctldev->hog_sleep); > + return pinctrl_commit_state(pctldev->p, pctldev->hog_sleep); > return 0; > } > EXPORT_SYMBOL_GPL(pinctrl_force_sleep); > @@ -1248,7 +1258,7 @@ EXPORT_SYMBOL_GPL(pinctrl_force_sleep); > int pinctrl_force_default(struct pinctrl_dev *pctldev) > { > if (!IS_ERR(pctldev->p) && !IS_ERR(pctldev->hog_default)) > - return pinctrl_select_state(pctldev->p, pctldev->hog_default); > + return pinctrl_commit_state(pctldev->p, pctldev->hog_default); > return 0; > } > EXPORT_SYMBOL_GPL(pinctrl_force_default); I don't often go back over a year worth of LKML, but since this patch recently landed in mainline as 981ed1bfbc6c, I though I'd use it as an anchor to report the following: It turns out that this patch completely breaks resume on my rk3399-based Chromebook. Most things are timing out, the box is unusable. And since this is my everyday tool, I'm mildly grumpy. Please don't break my toys! ;-) Reverting this patch on top of 4.16-rc2 makes me productive again... More seriously, I have no idea what's wrong here. It could be a SoC-related issue, hence Heiko on Cc. I'm happy to test any idea you could have. Thanks, M.
On February 19, 2018 9:25:26 AM PST, Marc Zyngier <maz@misterjones.org> wrote: >Hi all, > >On 2017-03-01 18:32, Florian Fainelli wrote: >> In case a platform only defaults a "default" set of pins, but not a >> "sleep" set of pins, and this particular platform suspends and >> resumes >> in a way that the pin states are not preserved by the hardware, when >> we >> resume, we would call pinctrl_single_resume() -> >> pinctrl_force_default() >> -> pinctrl_select_state() and the first thing we do is check that the >> pins state is the same as before, and do nothing. >> >> In order to fix this, decouple the actual state change from >> pinctrl_select_state() and move it pinctrl_commit_state(), while >> keeping >> the p->state == state check in pinctrl_select_state() not to change >> the >> caller assumptions. pinctrl_force_sleep() and pinctrl_force_default() >> are updated to bypass the state check by calling >> pinctrl_commit_state(). >> >> Fixes: 6e5e959dde0d ("pinctrl: API changes to support multiple states >> per device") >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> >> --- >> Changes in v3: >> >> - move the state check to pinctrl_select_state >> >> Changes in v2: >> >> - rename __pinctrl_select_state to pinctrl_commit_state >> >> drivers/pinctrl/core.c | 24 +++++++++++++++++------- >> 1 file changed, 17 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c >> index fb38e208f32d..735d8f7f9d71 100644 >> --- a/drivers/pinctrl/core.c >> +++ b/drivers/pinctrl/core.c >> @@ -992,19 +992,16 @@ struct pinctrl_state >> *pinctrl_lookup_state(struct pinctrl *p, >> EXPORT_SYMBOL_GPL(pinctrl_lookup_state); >> >> /** >> - * pinctrl_select_state() - select/activate/program a pinctrl state >> to HW >> + * pinctrl_commit_state() - select/activate/program a pinctrl state >> to HW >> * @p: the pinctrl handle for the device that requests configuration >> * @state: the state handle to select/activate/program >> */ >> -int pinctrl_select_state(struct pinctrl *p, struct pinctrl_state >> *state) >> +static int pinctrl_commit_state(struct pinctrl *p, struct >> pinctrl_state *state) >> { >> struct pinctrl_setting *setting, *setting2; >> struct pinctrl_state *old_state = p->state; >> int ret; >> >> - if (p->state == state) >> - return 0; >> - >> if (p->state) { >> /* >> * For each pinmux setting in the old state, forget SW's record >> @@ -1068,6 +1065,19 @@ int pinctrl_select_state(struct pinctrl *p, >> struct pinctrl_state *state) >> >> return ret; >> } >> + >> +/** >> + * pinctrl_select_state() - select/activate/program a pinctrl state >> to HW >> + * @p: the pinctrl handle for the device that requests configuration >> + * @state: the state handle to select/activate/program >> + */ >> +int pinctrl_select_state(struct pinctrl *p, struct pinctrl_state >> *state) >> +{ >> + if (p->state == state) >> + return 0; >> + >> + return pinctrl_commit_state(p, state); >> +} >> EXPORT_SYMBOL_GPL(pinctrl_select_state); >> >> static void devm_pinctrl_release(struct device *dev, void *res) >> @@ -1236,7 +1246,7 @@ void pinctrl_unregister_map(struct pinctrl_map >> const *map) >> int pinctrl_force_sleep(struct pinctrl_dev *pctldev) >> { >> if (!IS_ERR(pctldev->p) && !IS_ERR(pctldev->hog_sleep)) >> - return pinctrl_select_state(pctldev->p, pctldev->hog_sleep); >> + return pinctrl_commit_state(pctldev->p, pctldev->hog_sleep); >> return 0; >> } >> EXPORT_SYMBOL_GPL(pinctrl_force_sleep); >> @@ -1248,7 +1258,7 @@ EXPORT_SYMBOL_GPL(pinctrl_force_sleep); >> int pinctrl_force_default(struct pinctrl_dev *pctldev) >> { >> if (!IS_ERR(pctldev->p) && !IS_ERR(pctldev->hog_default)) >> - return pinctrl_select_state(pctldev->p, pctldev->hog_default); >> + return pinctrl_commit_state(pctldev->p, pctldev->hog_default); >> return 0; >> } >> EXPORT_SYMBOL_GPL(pinctrl_force_default); Hey Marc, > >I don't often go back over a year worth of LKML, but since this patch >recently landed in mainline as 981ed1bfbc6c, I though I'd use it as an >anchor to report the following: > >It turns out that this patch completely breaks resume on my >rk3399-based Chromebook. Most things are timing out, the box is >unusable. And since this is my everyday tool, I'm mildly grumpy. Please > >don't break my toys! ;-) Reverting this patch on top of 4.16-rc2 makes >me productive again... > >More seriously, I have no idea what's wrong here. It could be a >SoC-related issue, hence Heiko on Cc. I'm happy to test any idea you >could have. Can you indicate which DTS file is used for your Chromebook model? Sorry about the breakage.
Am Montag, 19. Februar 2018, 19:03:27 CET schrieb Florian Fainelli: > On February 19, 2018 9:25:26 AM PST, Marc Zyngier <maz@misterjones.org> wrote: > >Hi all, > > > >On 2017-03-01 18:32, Florian Fainelli wrote: > >> In case a platform only defaults a "default" set of pins, but not a > >> "sleep" set of pins, and this particular platform suspends and > >> resumes > >> in a way that the pin states are not preserved by the hardware, when > >> we > >> resume, we would call pinctrl_single_resume() -> > >> pinctrl_force_default() > >> -> pinctrl_select_state() and the first thing we do is check that the > >> pins state is the same as before, and do nothing. > >> > >> In order to fix this, decouple the actual state change from > >> pinctrl_select_state() and move it pinctrl_commit_state(), while > >> keeping > >> the p->state == state check in pinctrl_select_state() not to change > >> the > >> caller assumptions. pinctrl_force_sleep() and pinctrl_force_default() > >> are updated to bypass the state check by calling > >> pinctrl_commit_state(). > >> > >> Fixes: 6e5e959dde0d ("pinctrl: API changes to support multiple states > >> per device") > >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > >> --- > >> Changes in v3: > >> > >> - move the state check to pinctrl_select_state > >> > >> Changes in v2: > >> > >> - rename __pinctrl_select_state to pinctrl_commit_state > >> > >> drivers/pinctrl/core.c | 24 +++++++++++++++++------- > >> 1 file changed, 17 insertions(+), 7 deletions(-) > >> > >> diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c > >> index fb38e208f32d..735d8f7f9d71 100644 > >> --- a/drivers/pinctrl/core.c > >> +++ b/drivers/pinctrl/core.c > >> @@ -992,19 +992,16 @@ struct pinctrl_state > >> *pinctrl_lookup_state(struct pinctrl *p, > >> EXPORT_SYMBOL_GPL(pinctrl_lookup_state); > >> > >> /** > >> - * pinctrl_select_state() - select/activate/program a pinctrl state > >> to HW > >> + * pinctrl_commit_state() - select/activate/program a pinctrl state > >> to HW > >> * @p: the pinctrl handle for the device that requests configuration > >> * @state: the state handle to select/activate/program > >> */ > >> -int pinctrl_select_state(struct pinctrl *p, struct pinctrl_state > >> *state) > >> +static int pinctrl_commit_state(struct pinctrl *p, struct > >> pinctrl_state *state) > >> { > >> struct pinctrl_setting *setting, *setting2; > >> struct pinctrl_state *old_state = p->state; > >> int ret; > >> > >> - if (p->state == state) > >> - return 0; > >> - > >> if (p->state) { > >> /* > >> * For each pinmux setting in the old state, forget SW's record > >> @@ -1068,6 +1065,19 @@ int pinctrl_select_state(struct pinctrl *p, > >> struct pinctrl_state *state) > >> > >> return ret; > >> } > >> + > >> +/** > >> + * pinctrl_select_state() - select/activate/program a pinctrl state > >> to HW > >> + * @p: the pinctrl handle for the device that requests configuration > >> + * @state: the state handle to select/activate/program > >> + */ > >> +int pinctrl_select_state(struct pinctrl *p, struct pinctrl_state > >> *state) > >> +{ > >> + if (p->state == state) > >> + return 0; > >> + > >> + return pinctrl_commit_state(p, state); > >> +} > >> EXPORT_SYMBOL_GPL(pinctrl_select_state); > >> > >> static void devm_pinctrl_release(struct device *dev, void *res) > >> @@ -1236,7 +1246,7 @@ void pinctrl_unregister_map(struct pinctrl_map > >> const *map) > >> int pinctrl_force_sleep(struct pinctrl_dev *pctldev) > >> { > >> if (!IS_ERR(pctldev->p) && !IS_ERR(pctldev->hog_sleep)) > >> - return pinctrl_select_state(pctldev->p, pctldev->hog_sleep); > >> + return pinctrl_commit_state(pctldev->p, pctldev->hog_sleep); > >> return 0; > >> } > >> EXPORT_SYMBOL_GPL(pinctrl_force_sleep); > >> @@ -1248,7 +1258,7 @@ EXPORT_SYMBOL_GPL(pinctrl_force_sleep); > >> int pinctrl_force_default(struct pinctrl_dev *pctldev) > >> { > >> if (!IS_ERR(pctldev->p) && !IS_ERR(pctldev->hog_default)) > >> - return pinctrl_select_state(pctldev->p, pctldev->hog_default); > >> + return pinctrl_commit_state(pctldev->p, pctldev->hog_default); > >> return 0; > >> } > >> EXPORT_SYMBOL_GPL(pinctrl_force_default); > > Hey Marc, > > > > >I don't often go back over a year worth of LKML, but since this patch > >recently landed in mainline as 981ed1bfbc6c, I though I'd use it as an > >anchor to report the following: > > > >It turns out that this patch completely breaks resume on my > >rk3399-based Chromebook. Most things are timing out, the box is > >unusable. And since this is my everyday tool, I'm mildly grumpy. Please > > > >don't break my toys! ;-) Reverting this patch on top of 4.16-rc2 makes > >me productive again... > > > >More seriously, I have no idea what's wrong here. It could be a > >SoC-related issue, hence Heiko on Cc. I'm happy to test any idea you > >could have. > > Can you indicate which DTS file is used for your Chromebook model? Sorry about the breakage. that should be https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts I'm vacationing right now, so don't think I'll find time to dive into Rockchip pinctrl this week. But I'd guess it could be somehow related to the ATF touching pins during suspend/resume? -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 19 Feb 2018 18:03:27 +0000, Florian Fainelli wrote: > > On February 19, 2018 9:25:26 AM PST, Marc Zyngier <maz@misterjones.org> wrote: > >Hi all, > > > >On 2017-03-01 18:32, Florian Fainelli wrote: > >> In case a platform only defaults a "default" set of pins, but not a > >> "sleep" set of pins, and this particular platform suspends and > >> resumes > >> in a way that the pin states are not preserved by the hardware, when > >> we > >> resume, we would call pinctrl_single_resume() -> > >> pinctrl_force_default() > >> -> pinctrl_select_state() and the first thing we do is check that the > >> pins state is the same as before, and do nothing. > >> > >> In order to fix this, decouple the actual state change from > >> pinctrl_select_state() and move it pinctrl_commit_state(), while > >> keeping > >> the p->state == state check in pinctrl_select_state() not to change > >> the > >> caller assumptions. pinctrl_force_sleep() and pinctrl_force_default() > >> are updated to bypass the state check by calling > >> pinctrl_commit_state(). > >> > >> Fixes: 6e5e959dde0d ("pinctrl: API changes to support multiple states > >> per device") > >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> [back to using my ARM address] Hey Florian, > Hey Marc, > > > > >I don't often go back over a year worth of LKML, but since this patch > >recently landed in mainline as 981ed1bfbc6c, I though I'd use it as an > >anchor to report the following: > > > >It turns out that this patch completely breaks resume on my > >rk3399-based Chromebook. Most things are timing out, the box is > >unusable. And since this is my everyday tool, I'm mildly > >grumpy. Please don't break my toys! ;-) Reverting this patch on top > >of 4.16-rc2 makes me productive again... > > > >More seriously, I have no idea what's wrong here. It could be a > >SoC-related issue, hence Heiko on Cc. I'm happy to test any idea you > >could have. > > Can you indicate which DTS file is used for your Chromebook model? Sure. That's arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts, with a couple of fixes on top (some clocks and big-little idiosyncrasies). > Sorry about the breakage. No worries. M.
On Mon, 19 Feb 2018 18:57:23 +0000, Heiko Stuebner wrote: > > Am Montag, 19. Februar 2018, 19:03:27 CET schrieb Florian Fainelli: > > On February 19, 2018 9:25:26 AM PST, Marc Zyngier <maz@misterjones.org> wrote: > > >Hi all, > > > > > >On 2017-03-01 18:32, Florian Fainelli wrote: > > >> In case a platform only defaults a "default" set of pins, but not a > > >> "sleep" set of pins, and this particular platform suspends and > > >> resumes > > >> in a way that the pin states are not preserved by the hardware, when > > >> we > > >> resume, we would call pinctrl_single_resume() -> > > >> pinctrl_force_default() > > >> -> pinctrl_select_state() and the first thing we do is check that the > > >> pins state is the same as before, and do nothing. > > >> > > >> In order to fix this, decouple the actual state change from > > >> pinctrl_select_state() and move it pinctrl_commit_state(), while > > >> keeping > > >> the p->state == state check in pinctrl_select_state() not to change > > >> the > > >> caller assumptions. pinctrl_force_sleep() and pinctrl_force_default() > > >> are updated to bypass the state check by calling > > >> pinctrl_commit_state(). > > >> > > >> Fixes: 6e5e959dde0d ("pinctrl: API changes to support multiple states > > >> per device") > > >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > > >> --- > > >> Changes in v3: > > >> > > >> - move the state check to pinctrl_select_state > > >> > > >> Changes in v2: > > >> > > >> - rename __pinctrl_select_state to pinctrl_commit_state > > >> > > >> drivers/pinctrl/core.c | 24 +++++++++++++++++------- > > >> 1 file changed, 17 insertions(+), 7 deletions(-) > > >> Hey Heiko, > > Hey Marc, > > > > > > > >I don't often go back over a year worth of LKML, but since this patch > > >recently landed in mainline as 981ed1bfbc6c, I though I'd use it as an > > >anchor to report the following: > > > > > >It turns out that this patch completely breaks resume on my > > >rk3399-based Chromebook. Most things are timing out, the box is > > >unusable. And since this is my everyday tool, I'm mildly grumpy. Please > > > > > >don't break my toys! ;-) Reverting this patch on top of 4.16-rc2 makes > > >me productive again... > > > > > >More seriously, I have no idea what's wrong here. It could be a > > >SoC-related issue, hence Heiko on Cc. I'm happy to test any idea you > > >could have. > > > > Can you indicate which DTS file is used for your Chromebook model? Sorry about the breakage. > > that should be > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts > > I'm vacationing right now, so don't think I'll find time to dive into > Rockchip pinctrl this week. But I'd guess it could be somehow > related to the ATF touching pins during suspend/resume? That'd be really unfortunate. I would have assumed that ATF would leave things as they were instead of re-configuring them to whatever default. The most annoying thing is that if that's indeed the case, we need to find a solution that will cope with the current state of the firmware. I guess that'd mean eagerly saving/restoring the pin state across suspend/resume, irrespective of what firmware could do? Enjoy your holiday! M.
On Mon, Feb 19, 2018 at 8:23 PM, Marc Zyngier <marc.zyngier@arm.com> wrote: >> Am Montag, 19. Februar 2018, 19:03:27 CET schrieb Florian Fainelli: >> > Can you indicate which DTS file is used for your Chromebook model? Sorry about the breakage. >> >> that should be >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts >> >> I'm vacationing right now, so don't think I'll find time to dive into >> Rockchip pinctrl this week. But I'd guess it could be somehow >> related to the ATF touching pins during suspend/resume? > > That'd be really unfortunate. I would have assumed that ATF would > leave things as they were instead of re-configuring them to whatever > default. > > The most annoying thing is that if that's indeed the case, we need to > find a solution that will cope with the current state of the > firmware. I guess that'd mean eagerly saving/restoring the pin state > across suspend/resume, irrespective of what firmware could do? What is ATF? Asus Touch Firmware? Does it in effect mean that when the Rockchip pinctrl driver says pinctrl_force_sleep() and pinctrl_force_default() it expects those to be a noop? Then the real patch to apply is something deleting the pinctrl_force* calls from the pinctrl-rockchip driver, is it not? Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Linus, On 22/02/18 15:30, Linus Walleij wrote: > On Mon, Feb 19, 2018 at 8:23 PM, Marc Zyngier <marc.zyngier@arm.com> wrote: >>> Am Montag, 19. Februar 2018, 19:03:27 CET schrieb Florian Fainelli: > >>>> Can you indicate which DTS file is used for your Chromebook model? Sorry about the breakage. >>> >>> that should be >>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dts >>> >>> I'm vacationing right now, so don't think I'll find time to dive into >>> Rockchip pinctrl this week. But I'd guess it could be somehow >>> related to the ATF touching pins during suspend/resume? >> >> That'd be really unfortunate. I would have assumed that ATF would >> leave things as they were instead of re-configuring them to whatever >> default. >> >> The most annoying thing is that if that's indeed the case, we need to >> find a solution that will cope with the current state of the >> firmware. I guess that'd mean eagerly saving/restoring the pin state >> across suspend/resume, irrespective of what firmware could do? > > What is ATF? Asus Touch Firmware? More like "ARM Trusted Firmware", aka the firmware that runs on the secure side of most 64bit ARM systems. > Does it in effect mean that when the Rockchip pinctrl driver > says pinctrl_force_sleep() and pinctrl_force_default() > it expects those to be a noop? > > Then the real patch to apply is something deleting the > pinctrl_force* calls from the pinctrl-rockchip driver, > is it not? Quite possibly. Unravelling this bit of code, I came to a similar conclusion. The question is then: what is this call supposed to be used for? I'm pretty sure it is not completely gratuitous, but it makes no sense on my platform. I'll give it a go later today. M.
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c index fb38e208f32d..735d8f7f9d71 100644 --- a/drivers/pinctrl/core.c +++ b/drivers/pinctrl/core.c @@ -992,19 +992,16 @@ struct pinctrl_state *pinctrl_lookup_state(struct pinctrl *p, EXPORT_SYMBOL_GPL(pinctrl_lookup_state); /** - * pinctrl_select_state() - select/activate/program a pinctrl state to HW + * pinctrl_commit_state() - select/activate/program a pinctrl state to HW * @p: the pinctrl handle for the device that requests configuration * @state: the state handle to select/activate/program */ -int pinctrl_select_state(struct pinctrl *p, struct pinctrl_state *state) +static int pinctrl_commit_state(struct pinctrl *p, struct pinctrl_state *state) { struct pinctrl_setting *setting, *setting2; struct pinctrl_state *old_state = p->state; int ret; - if (p->state == state) - return 0; - if (p->state) { /* * For each pinmux setting in the old state, forget SW's record @@ -1068,6 +1065,19 @@ int pinctrl_select_state(struct pinctrl *p, struct pinctrl_state *state) return ret; } + +/** + * pinctrl_select_state() - select/activate/program a pinctrl state to HW + * @p: the pinctrl handle for the device that requests configuration + * @state: the state handle to select/activate/program + */ +int pinctrl_select_state(struct pinctrl *p, struct pinctrl_state *state) +{ + if (p->state == state) + return 0; + + return pinctrl_commit_state(p, state); +} EXPORT_SYMBOL_GPL(pinctrl_select_state); static void devm_pinctrl_release(struct device *dev, void *res) @@ -1236,7 +1246,7 @@ void pinctrl_unregister_map(struct pinctrl_map const *map) int pinctrl_force_sleep(struct pinctrl_dev *pctldev) { if (!IS_ERR(pctldev->p) && !IS_ERR(pctldev->hog_sleep)) - return pinctrl_select_state(pctldev->p, pctldev->hog_sleep); + return pinctrl_commit_state(pctldev->p, pctldev->hog_sleep); return 0; } EXPORT_SYMBOL_GPL(pinctrl_force_sleep); @@ -1248,7 +1258,7 @@ EXPORT_SYMBOL_GPL(pinctrl_force_sleep); int pinctrl_force_default(struct pinctrl_dev *pctldev) { if (!IS_ERR(pctldev->p) && !IS_ERR(pctldev->hog_default)) - return pinctrl_select_state(pctldev->p, pctldev->hog_default); + return pinctrl_commit_state(pctldev->p, pctldev->hog_default); return 0; } EXPORT_SYMBOL_GPL(pinctrl_force_default);
In case a platform only defaults a "default" set of pins, but not a "sleep" set of pins, and this particular platform suspends and resumes in a way that the pin states are not preserved by the hardware, when we resume, we would call pinctrl_single_resume() -> pinctrl_force_default() -> pinctrl_select_state() and the first thing we do is check that the pins state is the same as before, and do nothing. In order to fix this, decouple the actual state change from pinctrl_select_state() and move it pinctrl_commit_state(), while keeping the p->state == state check in pinctrl_select_state() not to change the caller assumptions. pinctrl_force_sleep() and pinctrl_force_default() are updated to bypass the state check by calling pinctrl_commit_state(). Fixes: 6e5e959dde0d ("pinctrl: API changes to support multiple states per device") Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> --- Changes in v3: - move the state check to pinctrl_select_state Changes in v2: - rename __pinctrl_select_state to pinctrl_commit_state drivers/pinctrl/core.c | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-)