[v2,1/2] pinctrl: Allow a device to indicate when to force a state

Message ID 20171102231551.16220-2-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.
It may happen that a device needs to force applying a state, e.g:
because it only defines one state of pin states (default) but loses
power/register contents when entering low power modes. Add a
pinctrl_dev::flags bitmask to help describe future quirks and define
PINCTRL_FLG_FORCE_STATE as such a settable flag.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/pinctrl/core.c | 14 +++++++++++++-
 drivers/pinctrl/core.h |  4 ++++
 2 files changed, 17 insertions(+), 1 deletion(-)

Comments

Linus Walleij Nov. 29, 2017, 1:06 p.m. | #1
On Fri, Nov 3, 2017 at 12:15 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:

> It may happen that a device needs to force applying a state, e.g:
> because it only defines one state of pin states (default) but loses
> power/register contents when entering low power modes. Add a
> pinctrl_dev::flags bitmask to help describe future quirks and define
> PINCTRL_FLG_FORCE_STATE as such a settable flag.
>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>

So if I understand correctly, the state is lost across
suspend/resume, correct?

Or are we even talking runtime PM runtime_suspend
and runtime_resume here?

> @@ -1197,9 +1197,21 @@ int pinctrl_select_state(struct pinctrl *p, struct pinctrl_state *state)
>  {
>         struct pinctrl_setting *setting, *setting2;
>         struct pinctrl_state *old_state = p->state;
> +       bool force = false;
>         int ret;
>
> -       if (p->state == state)
> +       if (p->state) {
> +               list_for_each_entry(setting, &p->state->settings, node) {
> +                       if (setting->pctldev->flags & PINCTRL_FLG_FORCE_STATE)
> +                               force = true;
> +               }
> +       }
> +
> +       /* Some controllers may want to force this operation when they define
> +        * only one set of functions and lose power state, e.g: pinctrl-single
> +        * with its pinctrl-single,low-power-state-loss property.
> +        */
> +       if (p->state == state && !force)
>                 return 0;

So the idea is we go and change the state even if we are in the right
state already, I understand that much.

But how is pinctrl_select_state() being called in the first place under
these circumstances?

If this comes from the resume() callback in .pm of the device driver,
would not the same thing be achived if you just set some mock
"sleep" state in suspend()? It could even have exactly the same settings
as the "default" state, as long as it is another state, the register
will be reprogrammed.

See further include/linux/pinctrl/pinctrl-state.h

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:01 p.m. | #2
* Florian Fainelli <f.fainelli@gmail.com> [171102 23:18]:
> It may happen that a device needs to force applying a state, e.g:
> because it only defines one state of pin states (default) but loses
> power/register contents when entering low power modes. Add a
> pinctrl_dev::flags bitmask to help describe future quirks and define
> PINCTRL_FLG_FORCE_STATE as such a settable flag.

It makes sense to tag the existing state with the context loss
information as otherwise we'll be duplicating the state in the
pinctrl driver potentially for hundreds of pins.

Maybe this patch description should clarify that it's the
pinctrl device restoring the pin state, not the pinctrl
consumer devices?

So maybe just "a pinctrl device needs to force apply a state"
instead of just device above?

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:35 p.m. | #3
On 11/29/2017 09:01 AM, Tony Lindgren wrote:
> * Florian Fainelli <f.fainelli@gmail.com> [171102 23:18]:
>> It may happen that a device needs to force applying a state, e.g:
>> because it only defines one state of pin states (default) but loses
>> power/register contents when entering low power modes. Add a
>> pinctrl_dev::flags bitmask to help describe future quirks and define
>> PINCTRL_FLG_FORCE_STATE as such a settable flag.
> 
> It makes sense to tag the existing state with the context loss
> information as otherwise we'll be duplicating the state in the
> pinctrl driver potentially for hundreds of pins.
> 
> Maybe this patch description should clarify that it's the
> pinctrl device restoring the pin state, not the pinctrl
> consumer devices?
> 
> So maybe just "a pinctrl device needs to force apply a state"
> instead of just device above?

It's a bit more involved than that, the pinctrl consumer device might
want to restore a particular state by calling pinctrl_select_state(),
however, because of the (p->state == state)check, the pinctrl provider
driver has no chance of making that call do the actual HW programming.
Tony Lindgren Nov. 29, 2017, 5:45 p.m. | #4
* Florian Fainelli <f.fainelli@gmail.com> [171129 17:37]:
> On 11/29/2017 09:01 AM, Tony Lindgren wrote:
> > * Florian Fainelli <f.fainelli@gmail.com> [171102 23:18]:
> >> It may happen that a device needs to force applying a state, e.g:
> >> because it only defines one state of pin states (default) but loses
> >> power/register contents when entering low power modes. Add a
> >> pinctrl_dev::flags bitmask to help describe future quirks and define
> >> PINCTRL_FLG_FORCE_STATE as such a settable flag.
> > 
> > It makes sense to tag the existing state with the context loss
> > information as otherwise we'll be duplicating the state in the
> > pinctrl driver potentially for hundreds of pins.
> > 
> > Maybe this patch description should clarify that it's the
> > pinctrl device restoring the pin state, not the pinctrl
> > consumer devices?
> > 
> > So maybe just "a pinctrl device needs to force apply a state"
> > instead of just device above?
> 
> It's a bit more involved than that, the pinctrl consumer device might
> want to restore a particular state by calling pinctrl_select_state(),
> however, because of the (p->state == state)check, the pinctrl provider
> driver has no chance of making that call do the actual HW programming.

Hmm but isn't it the pinctrl provider device losing context here?
I think the restore of the pin state should somehow happen automatically
by the pinctrl provider driver without a need for the pinctrl consumer
drivers to do anything.

Or what's the use case for pinctrl consumer driver wanting to store
a pin?

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, 6:15 p.m. | #5
On 11/29/2017 09:45 AM, Tony Lindgren wrote:
> * Florian Fainelli <f.fainelli@gmail.com> [171129 17:37]:
>> On 11/29/2017 09:01 AM, Tony Lindgren wrote:
>>> * Florian Fainelli <f.fainelli@gmail.com> [171102 23:18]:
>>>> It may happen that a device needs to force applying a state, e.g:
>>>> because it only defines one state of pin states (default) but loses
>>>> power/register contents when entering low power modes. Add a
>>>> pinctrl_dev::flags bitmask to help describe future quirks and define
>>>> PINCTRL_FLG_FORCE_STATE as such a settable flag.
>>>
>>> It makes sense to tag the existing state with the context loss
>>> information as otherwise we'll be duplicating the state in the
>>> pinctrl driver potentially for hundreds of pins.
>>>
>>> Maybe this patch description should clarify that it's the
>>> pinctrl device restoring the pin state, not the pinctrl
>>> consumer devices?
>>>
>>> So maybe just "a pinctrl device needs to force apply a state"
>>> instead of just device above?
>>
>> It's a bit more involved than that, the pinctrl consumer device might
>> want to restore a particular state by calling pinctrl_select_state(),
>> however, because of the (p->state == state)check, the pinctrl provider
>> driver has no chance of making that call do the actual HW programming.
> 
> Hmm but isn't it the pinctrl provider device losing context here?

It is the pinctrl provider indeed.

> I think the restore of the pin state should somehow happen automatically
> by the pinctrl provider driver without a need for the pinctrl consumer
> drivers to do anything.

Correct.

> 
> Or what's the use case for pinctrl consumer driver wanting to store
> a pin?

I actually meant that a consumer driver could aalso call
pinctrl_select_state() in one of its resume callback for instance, but
if the pinctrl provider driver does nothing (or rather the core, on
behalf of the provider), this would be an issue. This was not super
clear, so I will stop using that example from now on :)
Tony Lindgren Nov. 29, 2017, 6:27 p.m. | #6
* Florian Fainelli <f.fainelli@gmail.com> [171129 18:17]:
> On 11/29/2017 09:45 AM, Tony Lindgren wrote:
> > * Florian Fainelli <f.fainelli@gmail.com> [171129 17:37]:
> >> On 11/29/2017 09:01 AM, Tony Lindgren wrote:
> >>> * Florian Fainelli <f.fainelli@gmail.com> [171102 23:18]:
> >>>> It may happen that a device needs to force applying a state, e.g:
> >>>> because it only defines one state of pin states (default) but loses
> >>>> power/register contents when entering low power modes. Add a
> >>>> pinctrl_dev::flags bitmask to help describe future quirks and define
> >>>> PINCTRL_FLG_FORCE_STATE as such a settable flag.
> >>>
> >>> It makes sense to tag the existing state with the context loss
> >>> information as otherwise we'll be duplicating the state in the
> >>> pinctrl driver potentially for hundreds of pins.
> >>>
> >>> Maybe this patch description should clarify that it's the
> >>> pinctrl device restoring the pin state, not the pinctrl
> >>> consumer devices?
> >>>
> >>> So maybe just "a pinctrl device needs to force apply a state"
> >>> instead of just device above?
> >>
> >> It's a bit more involved than that, the pinctrl consumer device might
> >> want to restore a particular state by calling pinctrl_select_state(),
> >> however, because of the (p->state == state)check, the pinctrl provider
> >> driver has no chance of making that call do the actual HW programming.
> > 
> > Hmm but isn't it the pinctrl provider device losing context here?
> 
> It is the pinctrl provider indeed.
> 
> > I think the restore of the pin state should somehow happen automatically
> > by the pinctrl provider driver without a need for the pinctrl consumer
> > drivers to do anything.
> 
> Correct.

OK thanks for confirming that.

> > Or what's the use case for pinctrl consumer driver wanting to store
> > a pin?
> 
> I actually meant that a consumer driver could aalso call
> pinctrl_select_state() in one of its resume callback for instance, but
> if the pinctrl provider driver does nothing (or rather the core, on
> behalf of the provider), this would be an issue. This was not super
> clear, so I will stop using that example from now on :)

OK yeah that's probably where the confusion comes from :)

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

Patch

diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index 4c8d5b23e4d0..c91359d48aa1 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -1197,9 +1197,21 @@  int pinctrl_select_state(struct pinctrl *p, struct pinctrl_state *state)
 {
 	struct pinctrl_setting *setting, *setting2;
 	struct pinctrl_state *old_state = p->state;
+	bool force = false;
 	int ret;
 
-	if (p->state == state)
+	if (p->state) {
+		list_for_each_entry(setting, &p->state->settings, node) {
+			if (setting->pctldev->flags & PINCTRL_FLG_FORCE_STATE)
+				force = true;
+		}
+	}
+
+	/* Some controllers may want to force this operation when they define
+	 * only one set of functions and lose power state, e.g: pinctrl-single
+	 * with its pinctrl-single,low-power-state-loss property.
+	 */
+	if (p->state == state && !force)
 		return 0;
 
 	if (p->state) {
diff --git a/drivers/pinctrl/core.h b/drivers/pinctrl/core.h
index 8cf2eba17c8c..8f900e152295 100644
--- a/drivers/pinctrl/core.h
+++ b/drivers/pinctrl/core.h
@@ -39,6 +39,7 @@  struct pinctrl_gpio_range;
  * @hog_sleep: sleep state for pins hogged by this device
  * @mutex: mutex taken on each pin controller specific action
  * @device_root: debugfs root for this device
+ * @flags: feature/quirk flags
  */
 struct pinctrl_dev {
 	struct list_head node;
@@ -63,8 +64,11 @@  struct pinctrl_dev {
 #ifdef CONFIG_DEBUG_FS
 	struct dentry *device_root;
 #endif
+	unsigned long flags;
 };
 
+#define PINCTRL_FLG_FORCE_STATE	(1 << 0)
+
 /**
  * struct pinctrl - per-device pin control state holder
  * @node: global list node