[PATCH 0/2] pinctrl: Allow indicating loss of state across suspend/resume

[PATCH 0/2] pinctrl: Allow indicating loss of state across suspend/resume

[Florian Fainelli]

Hello Linus,

It's me again, so I have been thinking about the problem originally
reported in: [PATCH fixes v3] pinctrl: Really force states during suspend/resume

and other similar patches a while ago, and this new version allows a platform
using pinctrl-single to specify whether its pins are going to lose their state
during a system deep sleep.

Note that this is still checked at the pinctrl_select_state() because consumers
of the pinctrl API might be calling this from their suspend/resume functions
and should not have to know whether the provider does lose its pin states.

Thanks!

Florian Fainelli (2):
  pinctrl: Allow a device to indicate when to force a state
  pinctrl: single: Allow indicating loss of pin states during low-power

 .../devicetree/bindings/pinctrl/pinctrl-single.txt        |  4 ++++
 drivers/pinctrl/core.c                                    | 15 +++++++++++++++
 drivers/pinctrl/core.h                                    |  4 ++++
 drivers/pinctrl/pinctrl-single.c                          |  3 +++
 4 files changed, 26 insertions(+)

-- 
2.14.1

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

[Florian Fainelli]

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 | 15 +++++++++++++++
 drivers/pinctrl/core.h |  4 ++++
 2 files changed, 19 insertions(+)

diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index 56fbe4c3e800..c450a97de88f 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -1197,11 +1197,26 @@ 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)
 		return 0;
 
+	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) {
 		/*
 		 * For each pinmux setting in the old state, forget SW's record
diff --git a/drivers/pinctrl/core.h b/drivers/pinctrl/core.h
index 7880c3adc450..5fbf4dd1fa76 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
-- 
2.14.1

Re: [PATCH 1/2] pinctrl: Allow a device to indicate when to force a state

[Linus Walleij]

On Thu, Sep 21, 2017 at 3:04 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>

But I'm uncertain about the design.

A while back, I applied this patch to the GPIO subsystem:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/include/linux/gpio/machine.h?id=05f479bf7d239f01ff6546f2bdeb14ad0fe65601

commit 05f479bf7d239f01ff6546f2bdeb14ad0fe65601
Author: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
Date:   Tue May 23 15:47:29 2017 +0100

    gpio: Add new flags to control sleep status of GPIOs

    Add new flags to allow users to specify that they are not concerned with
    the status of GPIOs whilst in a sleep/low power state.

    Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
    Acked-by: Rob Herring <robh@kernel.org>
    Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

diff --git a/include/linux/gpio/machine.h b/include/linux/gpio/machine.h
index c0d712d22b07..13adadf53c09 100644
--- a/include/linux/gpio/machine.h
+++ b/include/linux/gpio/machine.h
@@ -9,6 +9,8 @@ enum gpio_lookup_flags {
        GPIO_ACTIVE_LOW = (1 << 0),
        GPIO_OPEN_DRAIN = (1 << 1),
        GPIO_OPEN_SOURCE = (1 << 2),
+       GPIO_SLEEP_MAINTAIN_VALUE = (0 << 3),
+       GPIO_SLEEP_MAY_LOOSE_VALUE = (1 << 3),
 };

Charles is also working on pin control and will probably have some
input on design.

Maybe we could do the same: a flag to maintain the value and
a flag to allow it to be lost across suspend/resume, which is the
core issue here.

Your case is analogous to GPIO_SLEEP_MAY_LOOSE_VALUE
and you restore the state when you come back from sleep.

Note how gpiolib does this:

bool gpiochip_line_is_persistent(struct gpio_chip *chip, unsigned int offset)
{
        if (offset >= chip->ngpio)
                return false;

        return !test_bit(FLAG_SLEEP_MAY_LOOSE_VALUE,
                         &chip->gpiodev->descs[offset].flags);
}
EXPORT_SYMBOL_GPL(gpiochip_line_is_persistent);

(Notice ! in front of test_bit(), the name of the function makes sense.)

Then the driver in drivers/gpio/gpio-arizona.c does some
special quirks like this:

static int arizona_gpio_direction_in(struct gpio_chip *chip, unsigned offset)
{
        struct arizona_gpio *arizona_gpio = gpiochip_get_data(chip);
        struct arizona *arizona = arizona_gpio->arizona;
        bool persistent = gpiochip_line_is_persistent(chip, offset);
        bool change;
        int ret;

        ret = regmap_update_bits_check(arizona->regmap,
                                       ARIZONA_GPIO1_CTRL + offset,
                                       ARIZONA_GPN_DIR, ARIZONA_GPN_DIR,
                                       &change);
        if (ret < 0)
                return ret;

        if (change && persistent) {
                pm_runtime_mark_last_busy(chip->parent);
                pm_runtime_put_autosuspend(chip->parent);
        }

        return 0;
}

So what this driver does is allow the GPIO block to loose power
using runtime PM if and only if the line is flagged as persistent, i.e. it
will not loose its state when sleeping, so let it sleep.

Next point, this commit from Baolin:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt?id=6606bc9dee63ad8cda2cc310d2ad5992673a785a

output-low - set the pin to output mode with low level
output-high - set the pin to output mode with high level
+sleep-hardware-state - indicate this is sleep related state which
will be programmed
+ into the registers for the sleep state.
slew-rate - set the slew rate

This is another thing: here we are defining a state that will be managed
by autonomous hardware. The state settings will be poked into some
special registers that will automatically take effect when the system
goes into sleep.

This is a hardware-induced state: the SLEEP line for the entire SoC
is asserted.

What you want is something different: a flag like:

sleep-restore-state - indicate that this state needs to be restored after sleep
  as the hardware loose states when sleeping.

The driver could look for this in a more granular per-state manner, instead
of all states of the pin controller being restored, we mark what
states need to be restored on the way up after sleep.

What are your thoughts about this?

I do not rule out a global flag for the whole controller, it is just a bit
confusing if we start to have per-state, per-pin and per-controller
sleep behaviour. If we have to have this we have to, but I want to
fully understand the problem first.

Yours,
Linus Walleij

Re: [PATCH 1/2] pinctrl: Allow a device to indicate when to force a state

[Charles Keepax]

On Fri, Sep 22, 2017 at 01:55:22PM +0200, Linus Walleij wrote:
> On Thu, Sep 21, 2017 at 3:04 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.
> >

Is the intention here to have this as a hint to the driver that
it needs to restore the state, or as something the core will act
upon and restore the state for you?

> > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> 
> But I'm uncertain about the design.
> 
> A while back, I applied this patch to the GPIO subsystem:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/include/linux/gpio/machine.h?id=05f479bf7d239f01ff6546f2bdeb14ad0fe65601
> 
> commit 05f479bf7d239f01ff6546f2bdeb14ad0fe65601
> Author: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> Date:   Tue May 23 15:47:29 2017 +0100
> 
>     gpio: Add new flags to control sleep status of GPIOs
> 
>     Add new flags to allow users to specify that they are not concerned with
>     the status of GPIOs whilst in a sleep/low power state.
> 
>     Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
>     Acked-by: Rob Herring <robh@kernel.org>
>     Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> 
> diff --git a/include/linux/gpio/machine.h b/include/linux/gpio/machine.h
> index c0d712d22b07..13adadf53c09 100644
> --- a/include/linux/gpio/machine.h
> +++ b/include/linux/gpio/machine.h
> @@ -9,6 +9,8 @@ enum gpio_lookup_flags {
>         GPIO_ACTIVE_LOW = (1 << 0),
>         GPIO_OPEN_DRAIN = (1 << 1),
>         GPIO_OPEN_SOURCE = (1 << 2),
> +       GPIO_SLEEP_MAINTAIN_VALUE = (0 << 3),
> +       GPIO_SLEEP_MAY_LOOSE_VALUE = (1 << 3),
>  };
> 
> Charles is also working on pin control and will probably have some
> input on design.
> 
> Maybe we could do the same: a flag to maintain the value and
> a flag to allow it to be lost across suspend/resume, which is the
> core issue here.
> 
> Your case is analogous to GPIO_SLEEP_MAY_LOOSE_VALUE
> and you restore the state when you come back from sleep.

I am not sure these are strictly analogous, my patch is about
the consumer of the GPIO specifying if it can tolerate the state
being dropped whilst in suspend. Whereas this patch appears to be
concerned with whether the state of the controller needs restored
on resume.

I guess in our case we didn't really consider the restore aspect
because we essentially get that for free from regmap. Regmap
cache's all our register state and provides a mechanism to sync
that back to the hardware, so we simply invoke that on resume and
all our GPIO/pinctrl state is restored.

> Next point, this commit from Baolin:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt?id=6606bc9dee63ad8cda2cc310d2ad5992673a785a
> 
> output-low - set the pin to output mode with low level
> output-high - set the pin to output mode with high level
> +sleep-hardware-state - indicate this is sleep related state which
> will be programmed
> + into the registers for the sleep state.
> slew-rate - set the slew rate
> 
> This is another thing: here we are defining a state that will be managed
> by autonomous hardware. The state settings will be poked into some
> special registers that will automatically take effect when the system
> goes into sleep.
> 
> This is a hardware-induced state: the SLEEP line for the entire SoC
> is asserted.
> 

Just to make sure I understand this property is used to specify a
pinctrl state that will be automatically applied by the hardware when
entering suspend? Kind of an odd one, feels like something you
could just have the software apply as part of the suspend
process. Almost would have wondered should this be a driver
specific binding rather than a generic pinctrl one?

I guess from looking at the driver using this I assume that said
hardware also automatically replies the non-sleep settings on
resume?

> What you want is something different: a flag like:
> 
> sleep-restore-state - indicate that this state needs to be restored after sleep
>   as the hardware loose states when sleeping.
> 
> The driver could look for this in a more granular per-state manner, instead
> of all states of the pin controller being restored, we mark what
> states need to be restored on the way up after sleep.
> 
> What are your thoughts about this?
> 
> I do not rule out a global flag for the whole controller, it is just a bit
> confusing if we start to have per-state, per-pin and per-controller
> sleep behaviour. If we have to have this we have to, but I want to
> fully understand the problem first.
> 
> Yours,
> Linus Walleij

Thanks,
Charles

Re: [PATCH 1/2] pinctrl: Allow a device to indicate when to force a state

[Linus Walleij]

On Fri, Sep 22, 2017 at 3:20 PM, Charles Keepax
<ckeepax@opensource.cirrus.com> wrote:
> On Fri, Sep 22, 2017 at 01:55:22PM +0200, Linus Walleij wrote:

>> Next point, this commit from Baolin:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt?id=6606bc9dee63ad8cda2cc310d2ad5992673a785a
>>
>> output-low - set the pin to output mode with low level
>> output-high - set the pin to output mode with high level
>> +sleep-hardware-state - indicate this is sleep related state which
>> will be programmed
>> + into the registers for the sleep state.
>> slew-rate - set the slew rate
>>
>> This is another thing: here we are defining a state that will be managed
>> by autonomous hardware. The state settings will be poked into some
>> special registers that will automatically take effect when the system
>> goes into sleep.
>>
>> This is a hardware-induced state: the SLEEP line for the entire SoC
>> is asserted.
>>
>
> Just to make sure I understand this property is used to specify a
> pinctrl state that will be automatically applied by the hardware when
> entering suspend?

Yes. It is quite common in SoCs, we just never supported it properly.

> Kind of an odd one, feels like something you
> could just have the software apply as part of the suspend
> process.

Not really. It has special registers just for this purpose,
and the driver is completely unaware that sleep is happening,
instead it is driven to the hardware by special hardware sleep
lines inside the SoC. So it needs to be set up when the default
state is programmed.

> Almost would have wondered should this be a driver
> specific binding rather than a generic pinctrl one?

No, I've seen it in several hardwares. (The Nomadik pin controller
has this too.)

> I guess from looking at the driver using this I assume that said
> hardware also automatically replies the non-sleep settings on
> resume?

Yep.

Yours,
Linus Walleij

Re: [PATCH 1/2] pinctrl: Allow a device to indicate when to force a state

[Florian Fainelli]

On 09/22/2017 06:57 AM, Linus Walleij wrote:
> On Fri, Sep 22, 2017 at 3:20 PM, Charles Keepax
> <ckeepax@opensource.cirrus.com> wrote:
>> On Fri, Sep 22, 2017 at 01:55:22PM +0200, Linus Walleij wrote:
> 
>>> Next point, this commit from Baolin:
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt?id=6606bc9dee63ad8cda2cc310d2ad5992673a785a
>>>
>>> output-low - set the pin to output mode with low level
>>> output-high - set the pin to output mode with high level
>>> +sleep-hardware-state - indicate this is sleep related state which
>>> will be programmed
>>> + into the registers for the sleep state.
>>> slew-rate - set the slew rate
>>>
>>> This is another thing: here we are defining a state that will be managed
>>> by autonomous hardware. The state settings will be poked into some
>>> special registers that will automatically take effect when the system
>>> goes into sleep.
>>>
>>> This is a hardware-induced state: the SLEEP line for the entire SoC
>>> is asserted.
>>>
>>
>> Just to make sure I understand this property is used to specify a
>> pinctrl state that will be automatically applied by the hardware when
>> entering suspend?
> 
> Yes. It is quite common in SoCs, we just never supported it properly.

This appears to be solving another possible problem/feature with pin
controllers during suspend, which is not exactly what I am after here.

Unless we generalize this into a state of some kind, which would
de-facto force a state transition in pinctrl_select_state() because
p->default != state, then I am not sure this how that is related to the
problem space exposed earlier.

> 
>> Kind of an odd one, feels like something you
>> could just have the software apply as part of the suspend
>> process.
> 
> Not really. It has special registers just for this purpose,
> and the driver is completely unaware that sleep is happening,
> instead it is driven to the hardware by special hardware sleep
> lines inside the SoC. So it needs to be set up when the default
> state is programmed.
> 
>> Almost would have wondered should this be a driver
>> specific binding rather than a generic pinctrl one?
> 
> No, I've seen it in several hardwares. (The Nomadik pin controller
> has this too.)
> 
>> I guess from looking at the driver using this I assume that said
>> hardware also automatically replies the non-sleep settings on
>> resume?
> 
> Yep.
> 
> Yours,
> Linus Walleij
> 


-- 
Florian

Re: [PATCH 1/2] pinctrl: Allow a device to indicate when to force a state

[Florian Fainelli]

On 09/22/2017 06:20 AM, Charles Keepax wrote:
> On Fri, Sep 22, 2017 at 01:55:22PM +0200, Linus Walleij wrote:
>> On Thu, Sep 21, 2017 at 3:04 AM, Florian Fainelli <f.fainelli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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.
>>>
> 
> Is the intention here to have this as a hint to the driver that
> it needs to restore the state, or as something the core will act
> upon and restore the state for you?

The intention here is to tell the pinctrl core code that a pinctrl
provider hardware will lose its hardware state upon entering a system
wide deep sleep. This is mainly because this specific pinctrl provider
is no an ON/OFF power island that is powered off on suspend, and resumes
with its default pin state.

> 
>>> Signed-off-by: Florian Fainelli <f.fainelli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>
>> But I'm uncertain about the design.
>>
>> A while back, I applied this patch to the GPIO subsystem:
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/include/linux/gpio/machine.h?id=05f479bf7d239f01ff6546f2bdeb14ad0fe65601
>>
>> commit 05f479bf7d239f01ff6546f2bdeb14ad0fe65601
>> Author: Charles Keepax <ckeepax-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org>
>> Date:   Tue May 23 15:47:29 2017 +0100
>>
>>     gpio: Add new flags to control sleep status of GPIOs
>>
>>     Add new flags to allow users to specify that they are not concerned with
>>     the status of GPIOs whilst in a sleep/low power state.
>>
>>     Signed-off-by: Charles Keepax <ckeepax-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org>
>>     Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>>     Signed-off-by: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>>
>> diff --git a/include/linux/gpio/machine.h b/include/linux/gpio/machine.h
>> index c0d712d22b07..13adadf53c09 100644
>> --- a/include/linux/gpio/machine.h
>> +++ b/include/linux/gpio/machine.h
>> @@ -9,6 +9,8 @@ enum gpio_lookup_flags {
>>         GPIO_ACTIVE_LOW = (1 << 0),
>>         GPIO_OPEN_DRAIN = (1 << 1),
>>         GPIO_OPEN_SOURCE = (1 << 2),
>> +       GPIO_SLEEP_MAINTAIN_VALUE = (0 << 3),
>> +       GPIO_SLEEP_MAY_LOOSE_VALUE = (1 << 3),
>>  };
>>
>> Charles is also working on pin control and will probably have some
>> input on design.
>>
>> Maybe we could do the same: a flag to maintain the value and
>> a flag to allow it to be lost across suspend/resume, which is the
>> core issue here.
>>
>> Your case is analogous to GPIO_SLEEP_MAY_LOOSE_VALUE
>> and you restore the state when you come back from sleep.
> 
> I am not sure these are strictly analogous, my patch is about
> the consumer of the GPIO specifying if it can tolerate the state
> being dropped whilst in suspend. Whereas this patch appears to be
> concerned with whether the state of the controller needs restored
> on resume.

Your patch does seem to be tackling a different angle indeed.

> 
> I guess in our case we didn't really consider the restore aspect
> because we essentially get that for free from regmap. Regmap
> cache's all our register state and provides a mechanism to sync
> that back to the hardware, so we simply invoke that on resume and
> all our GPIO/pinctrl state is restored.

As you may see, the problem in my case is that the hardware has only onw
pinctrl state: "default", and it loses its hardware register contents,
and because of early check in pinctrl_select_state(), we do nothing (the
state has not changed per-se), so we are left with SW thinking we
applied the "default" state again, while in fact we did not.

The approach taken here was to move this to the core pinctrl code
because this is not something a pinctrl consumer should be aware of,
when it calls pinctrl_select_state(), it should do what it asked for.

I also decided to make this a per-provider property as opposed to a
per-group property because chances are that the state retention is on a
per-controller basis, and not per-bank/group, although I may be wrong.

> 
>> Next point, this commit from Baolin:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt?id=6606bc9dee63ad8cda2cc310d2ad5992673a785a
>>
>> output-low - set the pin to output mode with low level
>> output-high - set the pin to output mode with high level
>> +sleep-hardware-state - indicate this is sleep related state which
>> will be programmed
>> + into the registers for the sleep state.
>> slew-rate - set the slew rate
>>
>> This is another thing: here we are defining a state that will be managed
>> by autonomous hardware. The state settings will be poked into some
>> special registers that will automatically take effect when the system
>> goes into sleep.
>>
>> This is a hardware-induced state: the SLEEP line for the entire SoC
>> is asserted.
>>
> 
> Just to make sure I understand this property is used to specify a
> pinctrl state that will be automatically applied by the hardware when
> entering suspend? Kind of an odd one, feels like something you
> could just have the software apply as part of the suspend
> process. Almost would have wondered should this be a driver
> specific binding rather than a generic pinctrl one?
> 
> I guess from looking at the driver using this I assume that said
> hardware also automatically replies the non-sleep settings on
> resume?
> 
>> What you want is something different: a flag like:
>>
>> sleep-restore-state - indicate that this state needs to be restored after sleep
>>   as the hardware loose states when sleeping.
>>
>> The driver could look for this in a more granular per-state manner, instead
>> of all states of the pin controller being restored, we mark what
>> states need to be restored on the way up after sleep.
>>
>> What are your thoughts about this?
>>
>> I do not rule out a global flag for the whole controller, it is just a bit
>> confusing if we start to have per-state, per-pin and per-controller
>> sleep behaviour. If we have to have this we have to, but I want to
>> fully understand the problem first.
>>
>> Yours,
>> Linus Walleij
> 
> Thanks,
> Charles
> 


-- 
Florian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] pinctrl: Allow a device to indicate when to force a state

[Charles Keepax]

On Fri, Sep 22, 2017 at 09:57:22AM -0700, Florian Fainelli wrote:
> On 09/22/2017 06:20 AM, Charles Keepax wrote:
> > On Fri, Sep 22, 2017 at 01:55:22PM +0200, Linus Walleij wrote:
> >> On Thu, Sep 21, 2017 at 3:04 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> > I guess in our case we didn't really consider the restore aspect
> > because we essentially get that for free from regmap. Regmap
> > cache's all our register state and provides a mechanism to sync
> > that back to the hardware, so we simply invoke that on resume and
> > all our GPIO/pinctrl state is restored.
> 
> As you may see, the problem in my case is that the hardware has only onw
> pinctrl state: "default", and it loses its hardware register contents,
> and because of early check in pinctrl_select_state(), we do nothing (the
> state has not changed per-se), so we are left with SW thinking we
> applied the "default" state again, while in fact we did not.
> 

That is exactly the situation we have on the CODECs when they go
into runtime suspend, power is removed, and everything is back at
defaults when we resume. Just in our case we re-apply the state
as part of the CODEC resume using a regmap sync.

> The approach taken here was to move this to the core pinctrl code
> because this is not something a pinctrl consumer should be aware of,
> when it calls pinctrl_select_state(), it should do what it asked for.
> 

Apologies if I have missed something here, but does the consumer
not still to some extent need to be aware with this solution
since it needs to re-request the pin state in resume?

I think that is really my only reservation here, is it feels
like this should be something that is purely implemented on the
provider, and be invisible to the consumer, and I am not clear
this is.

> I also decided to make this a per-provider property as opposed to a
> per-group property because chances are that the state retention is on a
> per-controller basis, and not per-bank/group, although I may be wrong.
> 

It seems quite likely that this property would mostly be
per-provider to me as well.

Thanks,
Charles

Re: [PATCH 1/2] pinctrl: Allow a device to indicate when to force a state

[Florian Fainelli]

On 09/26/2017 07:16 AM, Charles Keepax wrote:
> On Fri, Sep 22, 2017 at 09:57:22AM -0700, Florian Fainelli wrote:
>> On 09/22/2017 06:20 AM, Charles Keepax wrote:
>>> On Fri, Sep 22, 2017 at 01:55:22PM +0200, Linus Walleij wrote:
>>>> On Thu, Sep 21, 2017 at 3:04 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>> I guess in our case we didn't really consider the restore aspect
>>> because we essentially get that for free from regmap. Regmap
>>> cache's all our register state and provides a mechanism to sync
>>> that back to the hardware, so we simply invoke that on resume and
>>> all our GPIO/pinctrl state is restored.
>>
>> As you may see, the problem in my case is that the hardware has only onw
>> pinctrl state: "default", and it loses its hardware register contents,
>> and because of early check in pinctrl_select_state(), we do nothing (the
>> state has not changed per-se), so we are left with SW thinking we
>> applied the "default" state again, while in fact we did not.
>>
> 
> That is exactly the situation we have on the CODECs when they go
> into runtime suspend, power is removed, and everything is back at
> defaults when we resume. Just in our case we re-apply the state
> as part of the CODEC resume using a regmap sync.

Do you just re-apply the previous state, or do you force a "fake" state
by moving to a state different than the current during suspend, just to
force a transition during resume?

> 
>> The approach taken here was to move this to the core pinctrl code
>> because this is not something a pinctrl consumer should be aware of,
>> when it calls pinctrl_select_state(), it should do what it asked for.
>>
> 
> Apologies if I have missed something here, but does the consumer
> not still to some extent need to be aware with this solution
> since it needs to re-request the pin state in resume?

Consumers may indeed have to call pinctrl_select_state() but because of
the current check that does:

if (p->state == state)

this is not happening, but you are absolutely right, consumers that wish
to see their pin state be (re)configured during driver resume absolutely
need to tell the core about it, I am not thinking about any of this
happening "under the hood", this absolutely would not be right.

> 
> I think that is really my only reservation here, is it feels
> like this should be something that is purely implemented on the
> provider, and be invisible to the consumer, and I am not clear
> this is.
> 
>> I also decided to make this a per-provider property as opposed to a
>> per-group property because chances are that the state retention is on a
>> per-controller basis, and not per-bank/group, although I may be wrong.
>>
> 
> It seems quite likely that this property would mostly be
> per-provider to me as well.
> 
> Thanks,
> Charles
> 


-- 
Florian

Re: [PATCH 1/2] pinctrl: Allow a device to indicate when to force a state

[Charles Keepax]

On Tue, Sep 26, 2017 at 10:51:14AM -0700, Florian Fainelli wrote:
> On 09/26/2017 07:16 AM, Charles Keepax wrote:
> > On Fri, Sep 22, 2017 at 09:57:22AM -0700, Florian Fainelli wrote:
> >> On 09/22/2017 06:20 AM, Charles Keepax wrote:
> >>> On Fri, Sep 22, 2017 at 01:55:22PM +0200, Linus Walleij wrote:
> >>>> On Thu, Sep 21, 2017 at 3:04 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> >>> I guess in our case we didn't really consider the restore aspect
> >>> because we essentially get that for free from regmap. Regmap
> >>> cache's all our register state and provides a mechanism to sync
> >>> that back to the hardware, so we simply invoke that on resume and
> >>> all our GPIO/pinctrl state is restored.
> >>
> >> As you may see, the problem in my case is that the hardware has only onw
> >> pinctrl state: "default", and it loses its hardware register contents,
> >> and because of early check in pinctrl_select_state(), we do nothing (the
> >> state has not changed per-se), so we are left with SW thinking we
> >> applied the "default" state again, while in fact we did not.
> >>
> > 
> > That is exactly the situation we have on the CODECs when they go
> > into runtime suspend, power is removed, and everything is back at
> > defaults when we resume. Just in our case we re-apply the state
> > as part of the CODEC resume using a regmap sync.
> 
> Do you just re-apply the previous state, or do you force a "fake" state
> by moving to a state different than the current during suspend, just to
> force a transition during resume?
> 

Yes we just reapply the state, I guess the important thing here
is we arn't doing that through pinctrl we are just returning
the registers to the appropriate values (which regmap handily
provides) as part of resume.

It looks like there are other pinctrl drivers also doing this,
although often hardcoding it rather than using regmap. See either
pinctrl-exynos-arm.c or pinctrl-at91-pio4.c

> > 
> >> The approach taken here was to move this to the core pinctrl code
> >> because this is not something a pinctrl consumer should be aware of,
> >> when it calls pinctrl_select_state(), it should do what it asked for.
> >>
> > 
> > Apologies if I have missed something here, but does the consumer
> > not still to some extent need to be aware with this solution
> > since it needs to re-request the pin state in resume?
> 
> Consumers may indeed have to call pinctrl_select_state() but because of
> the current check that does:
> 
> if (p->state == state)
> 
> this is not happening, but you are absolutely right, consumers that wish
> to see their pin state be (re)configured during driver resume absolutely
> need to tell the core about it, I am not thinking about any of this
> happening "under the hood", this absolutely would not be right.
> 

See that's the bit that is concerning me, shouldn't the default
position be you get your pin state reconfigured during resume,
without having to do anything. Anything else seems to violate the
principle of least surprise.

It also feels to me the provider driver should be handling that,
rather than expecting the consumer to call pinctrl_select_state
again. So for example from the consumer I should be fine just to
call pinctrl_select_state in probe and have that state remain
until someone changes it.

Thanks,
Charles

Re: [PATCH 1/2] pinctrl: Allow a device to indicate when to force a state

[Charles Keepax]

On Wed, Sep 20, 2017 at 06:04:20PM -0700, Florian Fainelli 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>
> ---
>  drivers/pinctrl/core.c | 15 +++++++++++++++
>  drivers/pinctrl/core.h |  4 ++++
>  2 files changed, 19 insertions(+)
> 
> diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
> index 56fbe4c3e800..c450a97de88f 100644
> --- a/drivers/pinctrl/core.c
> +++ b/drivers/pinctrl/core.c
> @@ -1197,11 +1197,26 @@ 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)
>  		return 0;

I am guessing you probably intended to remove these two lines.

>  
> +	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;
> +

Thanks,
Charles

Re: [PATCH 1/2] pinctrl: Allow a device to indicate when to force a state

[Florian Fainelli]

On 09/22/2017 05:47 AM, Charles Keepax wrote:
> On Wed, Sep 20, 2017 at 06:04:20PM -0700, Florian Fainelli 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>
>> ---
>>  drivers/pinctrl/core.c | 15 +++++++++++++++
>>  drivers/pinctrl/core.h |  4 ++++
>>  2 files changed, 19 insertions(+)
>>
>> diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
>> index 56fbe4c3e800..c450a97de88f 100644
>> --- a/drivers/pinctrl/core.c
>> +++ b/drivers/pinctrl/core.c
>> @@ -1197,11 +1197,26 @@ 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)
>>  		return 0;
> 
> I am guessing you probably intended to remove these two lines.

Argh right, I cherry-picked what I done on an earlier kernel version and
this resolved to this, thanks for noticing!

> 
>>  
>> +	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;
>> +
> 
> Thanks,
> Charles
> 


-- 
Florian

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

[Florian Fainelli]

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-single.txt | 4 ++++
 drivers/pinctrl/pinctrl-single.c                             | 3 +++
 2 files changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt
index e705acd3612c..e71967f6a1a7 100644
--- a/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt
+++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt
@@ -82,6 +82,10 @@ Optional properties:
 		/* pin base, nr pins & gpio function */
 		pinctrl-single,gpio-range = <&range 0 3 0 &range 3 9 1>;
 
+- pinctrl-single,low-power-state-loss : indicates that the pins lose their
+  state during low power modes and therefore need to be restored upon
+  system resumption.
+
 - interrupt-controller : standard interrupt controller binding if using
   interrupts for wake-up events for example. In this case pinctrl-single
   is set up as a chained interrupt controller and the wake-up interrupts
diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c
index b8b3d932cd73..d69d20b8247a 100644
--- a/drivers/pinctrl/pinctrl-single.c
+++ b/drivers/pinctrl/pinctrl-single.c
@@ -1749,6 +1749,9 @@ static int pcs_probe(struct platform_device *pdev)
 		goto free;
 	}
 
+	if (of_property_read_bool(np, "pinctrl-single,low-power-state-loss"))
+		pcs->pctl->flags |= PINCTRL_FLG_FORCE_STATE;
+
 	ret = pcs_add_gpio_func(np, pcs);
 	if (ret < 0)
 		goto free;
-- 
2.14.1

Re: [PATCH 2/2] pinctrl: single: Allow indicating loss of pin states during low-power

[Linus Walleij]

On Thu, Sep 21, 2017 at 3:04 AM, Florian Fainelli <f.fainelli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

If we use this per-controller approach rather than the per-state approach
I discuss in reply to patch 1/2, we should probably make it a generic
property for pin controllers and not just a pinctrl-single business.

So patch pinctrl-bindings.txt and put the code somewhere in
core.

But that is more of a detail, first we need to figure out how to
handle this business in general.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] pinctrl: single: Allow indicating loss of pin states during low-power

[Florian Fainelli]

On 09/22/2017 06:03 AM, Linus Walleij wrote:
> On Thu, Sep 21, 2017 at 3:04 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>
> 
> If we use this per-controller approach rather than the per-state approach
> I discuss in reply to patch 1/2, we should probably make it a generic
> property for pin controllers and not just a pinctrl-single business.

I suppose it makes sense to make this a generic pinctrl property then.
drivers/pinctrl/core.c does not appear to be trying to fetch any
properties for a pinctrl device, but that is probably not too hard to add.

> 
> So patch pinctrl-bindings.txt and put the code somewhere in
> core.
> 
> But that is more of a detail, first we need to figure out how to
> handle this business in general.

Fair enough.
-- 
Florian