[v2,2/2] pinctrl: Allow indicating loss of pin states during low-power

Message ID 20171102231551.16220-3-f.fainelli@gmail.com
State New
Headers show
Series
  • pinctrl: Allow indicating loss of state across suspend/resume
Related show

Commit Message

Florian Fainelli Nov. 2, 2017, 11:15 p.m.
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(+)

Comments

Linus Walleij Nov. 29, 2017, 1:01 p.m. | #1
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
Tony Lindgren Nov. 29, 2017, 5:02 p.m. | #2
* 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
Florian Fainelli Nov. 29, 2017, 5:37 p.m. | #3
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
> 
>
Linus Walleij Dec. 2, 2017, 12:48 p.m. | #4
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
Florian Fainelli Dec. 10, 2017, 11:38 p.m. | #5
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) :).
Linus Walleij Dec. 20, 2017, 7:24 a.m. | #6
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
Florian Fainelli Dec. 30, 2017, 7:31 p.m. | #7
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!

Patch

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) {