| Message ID | 20171102231551.16220-3-f.fainelli@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | pinctrl: Allow indicating loss of state across suspend/resume | expand |
On Fri, Nov 3, 2017 at 12:15 AM, Florian Fainelli <f.fainelli@gmail.com> wrote: > Some platforms (e.g: Broadcom STB: BMIPS_GENERIC/ARCH_BRCMSTB) will lose > their register contents when entering their lower power state. In such a > case, the pinctrl-single driver that is used will not be able to restore > the power states without telling the core about it and having > pinctrl_select_state() check for that. > > This patch adds a new optional boolean property that Device Tree can > define in order to obtain exactly that and having the core pinctrl code > take that into account. > > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> Florian, I'm really sorry for losing track of this patch set, it's important stuff and I see why systems are dependent on something like this. Tony: can you look at this from a pinctrl-single point of view? This is the intended consumer: pinctrl-single users that lose the hardware state over suspend/resume. How do you see this working with other pinctrl-single users? 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
* Linus Walleij <linus.walleij@linaro.org> [171129 13:03]: > On Fri, Nov 3, 2017 at 12:15 AM, Florian Fainelli <f.fainelli@gmail.com> wrote: > > > Some platforms (e.g: Broadcom STB: BMIPS_GENERIC/ARCH_BRCMSTB) will lose > > their register contents when entering their lower power state. In such a > > case, the pinctrl-single driver that is used will not be able to restore > > the power states without telling the core about it and having > > pinctrl_select_state() check for that. > > > > This patch adds a new optional boolean property that Device Tree can > > define in order to obtain exactly that and having the core pinctrl code > > take that into account. > > > > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > > Florian, I'm really sorry for losing track of this patch set, it's > important stuff and I see why systems are dependent on something > like this. > > Tony: can you look at this from a pinctrl-single point of view? > This is the intended consumer: pinctrl-single users that lose the > hardware state over suspend/resume. > > How do you see this working with other pinctrl-single users? Hmm well typically a device driver that loses it's context just does save and restore of the registers in runtime PM suspend/resume as needed. In this case it would mean duplicating the state for potentially for hundreds of registers.. So using the existing state in the pinctrl subsystem totally makes sense for the pins. Florian do you have other reasons why this should be done in the pinctrl framework instead of the driver? Might be worth describing the reasoning in the patch descriptions :) So as long as the pinctrl framework state is used to restore the state by the pinctrl driver instead of the pinctrl consumer drivers, I don't have issues with this patchset. So probably just improving the patch messages a bit should do it. FYI, on omaps, the PRCM hardware saves and restores the pinctrl state so this has not been so far an issue. Regards, Tony -- 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 11/29/2017 09:02 AM, Tony Lindgren wrote: > * Linus Walleij <linus.walleij@linaro.org> [171129 13:03]: >> On Fri, Nov 3, 2017 at 12:15 AM, Florian Fainelli <f.fainelli@gmail.com> wrote: >> >>> Some platforms (e.g: Broadcom STB: BMIPS_GENERIC/ARCH_BRCMSTB) will lose >>> their register contents when entering their lower power state. In such a >>> case, the pinctrl-single driver that is used will not be able to restore >>> the power states without telling the core about it and having >>> pinctrl_select_state() check for that. >>> >>> This patch adds a new optional boolean property that Device Tree can >>> define in order to obtain exactly that and having the core pinctrl code >>> take that into account. >>> >>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> >> >> Florian, I'm really sorry for losing track of this patch set, it's >> important stuff and I see why systems are dependent on something >> like this. >> >> Tony: can you look at this from a pinctrl-single point of view? >> This is the intended consumer: pinctrl-single users that lose the >> hardware state over suspend/resume. >> >> How do you see this working with other pinctrl-single users? > > Hmm well typically a device driver that loses it's context just does > save and restore of the registers in runtime PM suspend/resume > as needed. In this case it would mean duplicating the state for > potentially for hundreds of registers.. So using the existing > state in the pinctrl subsystem totally makes sense for the pins. > > Florian do you have other reasons why this should be done in the > pinctrl framework instead of the driver? Might be worth describing > the reasoning in the patch descriptions :) The pinctrl provider driver that I am using is pinctrl-single, which has proper suspend/resume callbacks but those are not causing any HW programming to happen because of the (p->state == state) check, hence this patch series. > > So as long as the pinctrl framework state is used to restore the > state by the pinctrl driver instead of the pinctrl consumer drivers, > I don't have issues with this patchset. So probably just improving > the patch messages a bit should do it. > > FYI, on omaps, the PRCM hardware saves and restores the pinctrl > state so this has not been so far an issue. > > Regards, > > Tony > >
On Wed, Nov 29, 2017 at 6:37 PM, Florian Fainelli <f.fainelli@gmail.com> wrote: > On 11/29/2017 09:02 AM, Tony Lindgren wrote: >> Hmm well typically a device driver that loses it's context just does >> save and restore of the registers in runtime PM suspend/resume >> as needed. In this case it would mean duplicating the state for >> potentially for hundreds of registers.. So using the existing >> state in the pinctrl subsystem totally makes sense for the pins. >> >> Florian do you have other reasons why this should be done in the >> pinctrl framework instead of the driver? Might be worth describing >> the reasoning in the patch descriptions :) > > The pinctrl provider driver that I am using is pinctrl-single, which has > proper suspend/resume callbacks but those are not causing any HW > programming to happen because of the (p->state == state) check, hence > this patch series. So we are talking about these callbacks, correct? #ifdef CONFIG_PM static int pinctrl_single_suspend(struct platform_device *pdev, pm_message_t state) { struct pcs_device *pcs; pcs = platform_get_drvdata(pdev); if (!pcs) return -EINVAL; return pinctrl_force_sleep(pcs->pctl); } static int pinctrl_single_resume(struct platform_device *pdev) { struct pcs_device *pcs; pcs = platform_get_drvdata(pdev); if (!pcs) return -EINVAL; return pinctrl_force_default(pcs->pctl); } #endif Which falls through to this: /** * pinctrl_force_sleep() - turn a given controller device into sleep state * @pctldev: pin controller device */ 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 0; } EXPORT_SYMBOL_GPL(pinctrl_force_sleep); /** * pinctrl_force_default() - turn a given controller device into default state * @pctldev: pin controller device */ 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 0; } EXPORT_SYMBOL_GPL(pinctrl_force_default); So am I right in assuming it is actually the hogs that is your biggest problem, and those are the states that get lost over suspend/resume that are especially problematic? I.e. you don't have any problem with any non-hogged pinctrl handles, those are handled just fine in the suspend/resume paths of the client drivers? If this is the case, it changes the problem scope slightly. It is fair that functions named *force* should actually enforce programming a state. So then I would suggest somethin else: break pinctrl_select_state() into two: pinctrl_select_state() that works just like before, checking if (p->state == state) but which calls a static function pinctrl_select_state_commit() that commits the change unconditonally. Then alter pinctrl_force_sleep() and pinctrl_force_sleep() to call that function. This should solve your problem without having to alter the semantics of pinctrl_select_state() for everyone. If you want I can cook a patch to illustrate what I mean so you can try it. 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 12/02/2017 04:48 AM, Linus Walleij wrote: > On Wed, Nov 29, 2017 at 6:37 PM, Florian Fainelli <f.fainelli@gmail.com> wrote: >> On 11/29/2017 09:02 AM, Tony Lindgren wrote: > >>> Hmm well typically a device driver that loses it's context just does >>> save and restore of the registers in runtime PM suspend/resume >>> as needed. In this case it would mean duplicating the state for >>> potentially for hundreds of registers.. So using the existing >>> state in the pinctrl subsystem totally makes sense for the pins. >>> >>> Florian do you have other reasons why this should be done in the >>> pinctrl framework instead of the driver? Might be worth describing >>> the reasoning in the patch descriptions :) >> >> The pinctrl provider driver that I am using is pinctrl-single, which has >> proper suspend/resume callbacks but those are not causing any HW >> programming to happen because of the (p->state == state) check, hence >> this patch series. > > So we are talking about these callbacks, correct? > > #ifdef CONFIG_PM > static int pinctrl_single_suspend(struct platform_device *pdev, > pm_message_t state) > { > struct pcs_device *pcs; > > pcs = platform_get_drvdata(pdev); > if (!pcs) > return -EINVAL; > > return pinctrl_force_sleep(pcs->pctl); > } > > static int pinctrl_single_resume(struct platform_device *pdev) > { > struct pcs_device *pcs; > > pcs = platform_get_drvdata(pdev); > if (!pcs) > return -EINVAL; > > return pinctrl_force_default(pcs->pctl); > } > #endif > > Which falls through to this: > > /** > * pinctrl_force_sleep() - turn a given controller device into sleep state > * @pctldev: pin controller device > */ > 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 0; > } > EXPORT_SYMBOL_GPL(pinctrl_force_sleep); > > /** > * pinctrl_force_default() - turn a given controller device into default state > * @pctldev: pin controller device > */ > 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 0; > } > EXPORT_SYMBOL_GPL(pinctrl_force_default); > > So am I right in assuming it is actually the hogs that is your biggest > problem, and those are the states that get lost over suspend/resume > that are especially problematic? > > I.e. you don't have any problem with any non-hogged pinctrl > handles, those are handled just fine in the suspend/resume > paths of the client drivers? > > If this is the case, it changes the problem scope slightly. > > It is fair that functions named *force* should actually enforce > programming a state. > > So then I would suggest somethin else: break pinctrl_select_state() > into two: > > pinctrl_select_state() that works just like before, checking if > (p->state == state) but which calls a static function > pinctrl_select_state_commit() that commits the change unconditonally. > Then alter pinctrl_force_sleep() and pinctrl_force_sleep() to call > that function. > > This should solve your problem without having to alter the semantics > of pinctrl_select_state() for everyone. This was exactly what I proposed initially here: http://patchwork.ozlabs.org/patch/734326/ I really want to get this fixed, but I can't do that if we keep losing the context of the discussion (pun intended) :).
On Mon, Dec 11, 2017 at 12:38 AM, Florian Fainelli <f.fainelli@gmail.com> wrote: > On 12/02/2017 04:48 AM, Linus Walleij wrote: >> This should solve your problem without having to alter the semantics >> of pinctrl_select_state() for everyone. > > This was exactly what I proposed initially here: > > http://patchwork.ozlabs.org/patch/734326/ > > I really want to get this fixed, but I can't do that if we keep losing > the context of the discussion (pun intended) :). Oh sorry man. I am clearly too stupid for this job... In accordance with things needing to be intuitive, something named *force_* should of course force the setting into the hardware. The original patch didn't mention the fact that it was hogs and hogs only that was causing the trouble and that is why I got lost. (I guess.) I have been going about this as if it was something generic that affect all states in all devices, and to me hogs is just an abscure corner of pin controlling... I applied the patchwork patch from above, and elaborated a bit on that it pertains to hogs, let's see what happens. For the case where a driver (not hog) needs to handle suspend/resume transitions, proper states can hopefully be used. 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
Le 12/19/17 à 23:24, Linus Walleij a écrit : > On Mon, Dec 11, 2017 at 12:38 AM, Florian Fainelli <f.fainelli@gmail.com> wrote: >> On 12/02/2017 04:48 AM, Linus Walleij wrote: > >>> This should solve your problem without having to alter the semantics >>> of pinctrl_select_state() for everyone. >> >> This was exactly what I proposed initially here: >> >> http://patchwork.ozlabs.org/patch/734326/ >> >> I really want to get this fixed, but I can't do that if we keep losing >> the context of the discussion (pun intended) :). > > Oh sorry man. I am clearly too stupid for this job... No need to slap yourself! > > In accordance with things needing to be intuitive, something named > *force_* should of course force the setting into the hardware. > > The original patch didn't mention the fact that it was hogs > and hogs only that was causing the trouble and that is why I > got lost. (I guess.) I have been going about this as if it was > something generic that affect all states in all devices, and to > me hogs is just an abscure corner of pin controlling... > > I applied the patchwork patch from above, and elaborated > a bit on that it pertains to hogs, let's see what > happens. > > For the case where a driver (not hog) needs to handle > suspend/resume transitions, proper states can hopefully > be used. Your commit message makes that clear now, thanks for applying the patch and gott nytt år!
diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt index ad9bbbba36e9..cc9bae3b7c33 100644 --- a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt @@ -103,6 +103,10 @@ Optional properties: #pinctrl-cells: Number of pin control cells in addition to the index within the pin controller device instance +low-power-state-loss: boolean property which indicates that the pins lose their +state during low power modes and therefore need to be restored upon system +resumption. + Pin controller devices should contain the pin configuration nodes that client devices reference. diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c index c91359d48aa1..3fee457999b5 100644 --- a/drivers/pinctrl/core.c +++ b/drivers/pinctrl/core.c @@ -1978,6 +1978,9 @@ pinctrl_init_controller(struct pinctrl_desc *pctldesc, struct device *dev, pctldev->dev = dev; mutex_init(&pctldev->mutex); + if (of_property_read_bool(dev->of_node, "low-power-state-loss")) + pctldev->flags |= PINCTRL_FLG_FORCE_STATE; + /* check core ops for sanity */ ret = pinctrl_check_ops(pctldev); if (ret) {
Some platforms (e.g: Broadcom STB: BMIPS_GENERIC/ARCH_BRCMSTB) will lose their register contents when entering their lower power state. In such a case, the pinctrl-single driver that is used will not be able to restore the power states without telling the core about it and having pinctrl_select_state() check for that. This patch adds a new optional boolean property that Device Tree can define in order to obtain exactly that and having the core pinctrl code take that into account. Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> --- Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt | 4 ++++ drivers/pinctrl/core.c | 3 +++ 2 files changed, 7 insertions(+)