diff mbox

[7/9] pinctrl: samsung: Add property to mark pad state as suitable for power down

Message ID 1482495889-6201-8-git-send-email-m.szyprowski@samsung.com (mailing list archive)
State Not Applicable
Headers show

Commit Message

Marek Szyprowski Dec. 23, 2016, 12:24 p.m. UTC
Add support for special property "samsung,off-state", which indicates a special
state suitable for device's "sleep" state. Its pin values/properties should
match the configuration in power down mode. It indicates that pin controller
can notify runtime power management subsystem, that it is ready for runtime
suspend if its all pins are configured for such state. This in turn might
allow to turn respective power domain off to reduce power consumption.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt | 8 ++++++++
 drivers/pinctrl/samsung/pinctrl-samsung.c                     | 4 ++++
 drivers/pinctrl/samsung/pinctrl-samsung.h                     | 1 +
 3 files changed, 13 insertions(+)

Comments

Krzysztof Kozlowski Dec. 25, 2016, 7:19 p.m. UTC | #1
On Fri, Dec 23, 2016 at 01:24:47PM +0100, Marek Szyprowski wrote:
> Add support for special property "samsung,off-state", which indicates a special
> state suitable for device's "sleep" state. Its pin values/properties should
> match the configuration in power down mode. It indicates that pin controller
> can notify runtime power management subsystem, that it is ready for runtime
> suspend if its all pins are configured for such state. This in turn might
> allow to turn respective power domain off to reduce power consumption.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt | 8 ++++++++
>  drivers/pinctrl/samsung/pinctrl-samsung.c                     | 4 ++++
>  drivers/pinctrl/samsung/pinctrl-samsung.h                     | 1 +
>  3 files changed, 13 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
> index b7bd2e12a269..354eea0e7798 100644
> --- a/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
> +++ b/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
> @@ -105,6 +105,7 @@ Required Properties:
>    - samsung,pin-drv: Drive strength configuration.
>    - samsung,pin-pud-pdn: Pull up/down configuration in power down mode.
>    - samsung,pin-drv-pdn: Drive strength configuration in power down mode.
> +  - samsung,off-state: Mark this configuration as suitable for bank power off.
>  
>    The values specified by these config properties should be derived from the
>    hardware manual and these values are programmed as-is into the pin
> @@ -113,6 +114,13 @@ Required Properties:
>    Note: A child should include atleast a pin function selection property or
>    pin configuration property (one or more) or both.
>  
> +  Note: Special property "samsung,off-state" indicates that this state can
> +  be used for device's "sleep" pins state. Its pin values/properties should
> +  match the configuration in power down mode.

Why power down values cannot be used for sleep state? Why you need
separate pin control state? If pins values should match power down
configuration, then they could just be added to default state, couldn't
they?

In the patch 2/9, existing configuration:
716         i2s0_bus: i2s0-bus {
(...)
719                 samsung,pin-function = <EXYNOS_PIN_FUNC_2>;
720                 samsung,pin-pud = <EXYNOS_PIN_PULL_NONE>;
721                 samsung,pin-drv = <EXYNOS5420_PIN_DRV_LV1>;
722         };

additional configuration:
+       i2s0_bus_slp: i2s0-bus-slp {
+               samsung,pin-function = <EXYNOS_PIN_FUNC_INPUT>;
+               samsung,pin-pud = <EXYNOS_PIN_PULL_NONE>;
+               samsung,pin-drv = <EXYNOS5420_PIN_DRV_LV1>;
+               samsung,pin-con-pdn = <EXYNOS_PIN_PDN_INPUT>;
+               samsung,pin-pud-pdn = <EXYNOS_PIN_PULL_NONE>;
+               samsung,off-state;
+       };

> It indicates that pin control
> +  can notify runtime power management subsystem, that it is ready for runtime
> +  suspend if its all pins are configured for such state. This in turn might
> +  allow to turn respective power domain off to reduce power consumption.

What do you mean by "notifying RPM subsystem"? Either this is
description of hardware in certain mode (sleep state) or this is not
device tree property.

Best regards,
Krzysztof

> +
>    The client nodes that require a particular pin function selection and/or
>    pin configuration should use the bindings listed in the "pinctrl-bindings.txt"
>    file.
> diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.c b/drivers/pinctrl/samsung/pinctrl-samsung.c
> index a7b7d75373f2..301169d2b6e1 100644
> --- a/drivers/pinctrl/samsung/pinctrl-samsung.c
> +++ b/drivers/pinctrl/samsung/pinctrl-samsung.c
> @@ -692,6 +692,10 @@ static int samsung_pinctrl_create_function(struct device *dev,
>  	}
>  
>  	func->name = func_np->full_name;
> +	if (of_property_read_bool(func_np, "samsung,off-state"))
> +		func->rpm_active = false;
> +	else
> +		func->rpm_active = true;
>  
>  	func->groups = devm_kzalloc(dev, npins * sizeof(char *), GFP_KERNEL);
>  	if (!func->groups)
> diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.h b/drivers/pinctrl/samsung/pinctrl-samsung.h
> index 32b949e2a89b..edeafa00abd3 100644
> --- a/drivers/pinctrl/samsung/pinctrl-samsung.h
> +++ b/drivers/pinctrl/samsung/pinctrl-samsung.h
> @@ -280,6 +280,7 @@ struct samsung_pmx_func {
>  	const char		**groups;
>  	u8			num_groups;
>  	u32			val;
> +	bool			rpm_active;
>  };
>  
>  /* list of all exported SoC specific data */
> -- 
> 1.9.1
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tomasz Figa Dec. 26, 2016, 6:02 a.m. UTC | #2
2016-12-26 4:19 GMT+09:00 Krzysztof Kozlowski <krzk@kernel.org>:
> On Fri, Dec 23, 2016 at 01:24:47PM +0100, Marek Szyprowski wrote:
>> Add support for special property "samsung,off-state", which indicates a special
>> state suitable for device's "sleep" state. Its pin values/properties should
>> match the configuration in power down mode. It indicates that pin controller
>> can notify runtime power management subsystem, that it is ready for runtime
>> suspend if its all pins are configured for such state. This in turn might
>> allow to turn respective power domain off to reduce power consumption.
>>
>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> ---
>>  Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt | 8 ++++++++
>>  drivers/pinctrl/samsung/pinctrl-samsung.c                     | 4 ++++
>>  drivers/pinctrl/samsung/pinctrl-samsung.h                     | 1 +
>>  3 files changed, 13 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
>> index b7bd2e12a269..354eea0e7798 100644
>> --- a/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
>> +++ b/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
>> @@ -105,6 +105,7 @@ Required Properties:
>>    - samsung,pin-drv: Drive strength configuration.
>>    - samsung,pin-pud-pdn: Pull up/down configuration in power down mode.
>>    - samsung,pin-drv-pdn: Drive strength configuration in power down mode.
>> +  - samsung,off-state: Mark this configuration as suitable for bank power off.
>>
>>    The values specified by these config properties should be derived from the
>>    hardware manual and these values are programmed as-is into the pin
>> @@ -113,6 +114,13 @@ Required Properties:
>>    Note: A child should include atleast a pin function selection property or
>>    pin configuration property (one or more) or both.
>>
>> +  Note: Special property "samsung,off-state" indicates that this state can
>> +  be used for device's "sleep" pins state. Its pin values/properties should
>> +  match the configuration in power down mode.
>
> Why power down values cannot be used for sleep state? Why you need
> separate pin control state? If pins values should match power down
> configuration, then they could just be added to default state, couldn't
> they?
>
> In the patch 2/9, existing configuration:
> 716         i2s0_bus: i2s0-bus {
> (...)
> 719                 samsung,pin-function = <EXYNOS_PIN_FUNC_2>;
> 720                 samsung,pin-pud = <EXYNOS_PIN_PULL_NONE>;
> 721                 samsung,pin-drv = <EXYNOS5420_PIN_DRV_LV1>;
> 722         };
>
> additional configuration:
> +       i2s0_bus_slp: i2s0-bus-slp {
> +               samsung,pin-function = <EXYNOS_PIN_FUNC_INPUT>;
> +               samsung,pin-pud = <EXYNOS_PIN_PULL_NONE>;
> +               samsung,pin-drv = <EXYNOS5420_PIN_DRV_LV1>;
> +               samsung,pin-con-pdn = <EXYNOS_PIN_PDN_INPUT>;
> +               samsung,pin-pud-pdn = <EXYNOS_PIN_PULL_NONE>;
> +               samsung,off-state;
> +       };

+1

If I'm not missing something, it should be reasonably easy to
determine if a state is suitable for power off by its configuration,
by comparing pin-function and pin-pud with pin-con-pdn and
pin-pud-pdn.

Best regards,
Tomasz
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Marek Szyprowski Dec. 27, 2016, 10:30 a.m. UTC | #3
Hi Krzysztof,


On 2016-12-25 20:19, Krzysztof Kozlowski wrote:
> On Fri, Dec 23, 2016 at 01:24:47PM +0100, Marek Szyprowski wrote:
>> Add support for special property "samsung,off-state", which indicates a special
>> state suitable for device's "sleep" state. Its pin values/properties should
>> match the configuration in power down mode. It indicates that pin controller
>> can notify runtime power management subsystem, that it is ready for runtime
>> suspend if its all pins are configured for such state. This in turn might
>> allow to turn respective power domain off to reduce power consumption.
>>
>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> ---
>>   Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt | 8 ++++++++
>>   drivers/pinctrl/samsung/pinctrl-samsung.c                     | 4 ++++
>>   drivers/pinctrl/samsung/pinctrl-samsung.h                     | 1 +
>>   3 files changed, 13 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
>> index b7bd2e12a269..354eea0e7798 100644
>> --- a/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
>> +++ b/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
>> @@ -105,6 +105,7 @@ Required Properties:
>>     - samsung,pin-drv: Drive strength configuration.
>>     - samsung,pin-pud-pdn: Pull up/down configuration in power down mode.
>>     - samsung,pin-drv-pdn: Drive strength configuration in power down mode.
>> +  - samsung,off-state: Mark this configuration as suitable for bank power off.
>>   
>>     The values specified by these config properties should be derived from the
>>     hardware manual and these values are programmed as-is into the pin
>> @@ -113,6 +114,13 @@ Required Properties:
>>     Note: A child should include atleast a pin function selection property or
>>     pin configuration property (one or more) or both.
>>   
>> +  Note: Special property "samsung,off-state" indicates that this state can
>> +  be used for device's "sleep" pins state. Its pin values/properties should
>> +  match the configuration in power down mode.
> Why power down values cannot be used for sleep state? Why you need
> separate pin control state? If pins values should match power down
> configuration, then they could just be added to default state, couldn't
> they?

Separate sleep state is needed because of the pin control bindings and 
design.

A separate sleep state is required to let pin control client driver (in 
this case
Exynos I2S driver) let to choose when it is okay to switch pads into sleep
state and I see no other way to achieve this.

> In the patch 2/9, existing configuration:
> 716         i2s0_bus: i2s0-bus {
> (...)
> 719                 samsung,pin-function = <EXYNOS_PIN_FUNC_2>;
> 720                 samsung,pin-pud = <EXYNOS_PIN_PULL_NONE>;
> 721                 samsung,pin-drv = <EXYNOS5420_PIN_DRV_LV1>;
> 722         };
>
> additional configuration:
> +       i2s0_bus_slp: i2s0-bus-slp {
> +               samsung,pin-function = <EXYNOS_PIN_FUNC_INPUT>;
> +               samsung,pin-pud = <EXYNOS_PIN_PULL_NONE>;
> +               samsung,pin-drv = <EXYNOS5420_PIN_DRV_LV1>;
> +               samsung,pin-con-pdn = <EXYNOS_PIN_PDN_INPUT>;
> +               samsung,pin-pud-pdn = <EXYNOS_PIN_PULL_NONE>;
> +               samsung,off-state;
> +       };

I agree that it would be possible to get rid of "samsung,off-state" 
property and
just detect off state when func/pud pair matches power down func/pud.

>> It indicates that pin control
>> +  can notify runtime power management subsystem, that it is ready for runtime
>> +  suspend if its all pins are configured for such state. This in turn might
>> +  allow to turn respective power domain off to reduce power consumption.
> What do you mean by "notifying RPM subsystem"? Either this is
> description of hardware in certain mode (sleep state) or this is not
> device tree property.

Maybe I wrote the description with a view too limited to the kernel 
operation
perspective, but if we remove the need to mark state as off, the above 
description
will not be needed.


Best regards
Krzysztof Kozlowski Dec. 27, 2016, 3:33 p.m. UTC | #4
On Tue, Dec 27, 2016 at 11:30:57AM +0100, Marek Szyprowski wrote:
> Hi Krzysztof,
> 
> 
> On 2016-12-25 20:19, Krzysztof Kozlowski wrote:
> > On Fri, Dec 23, 2016 at 01:24:47PM +0100, Marek Szyprowski wrote:
> > > Add support for special property "samsung,off-state", which indicates a special
> > > state suitable for device's "sleep" state. Its pin values/properties should
> > > match the configuration in power down mode. It indicates that pin controller
> > > can notify runtime power management subsystem, that it is ready for runtime
> > > suspend if its all pins are configured for such state. This in turn might
> > > allow to turn respective power domain off to reduce power consumption.
> > > 
> > > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> > > ---
> > >   Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt | 8 ++++++++
> > >   drivers/pinctrl/samsung/pinctrl-samsung.c                     | 4 ++++
> > >   drivers/pinctrl/samsung/pinctrl-samsung.h                     | 1 +
> > >   3 files changed, 13 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
> > > index b7bd2e12a269..354eea0e7798 100644
> > > --- a/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
> > > +++ b/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
> > > @@ -105,6 +105,7 @@ Required Properties:
> > >     - samsung,pin-drv: Drive strength configuration.
> > >     - samsung,pin-pud-pdn: Pull up/down configuration in power down mode.
> > >     - samsung,pin-drv-pdn: Drive strength configuration in power down mode.
> > > +  - samsung,off-state: Mark this configuration as suitable for bank power off.
> > >     The values specified by these config properties should be derived from the
> > >     hardware manual and these values are programmed as-is into the pin
> > > @@ -113,6 +114,13 @@ Required Properties:
> > >     Note: A child should include atleast a pin function selection property or
> > >     pin configuration property (one or more) or both.
> > > +  Note: Special property "samsung,off-state" indicates that this state can
> > > +  be used for device's "sleep" pins state. Its pin values/properties should
> > > +  match the configuration in power down mode.
> > Why power down values cannot be used for sleep state? Why you need
> > separate pin control state? If pins values should match power down
> > configuration, then they could just be added to default state, couldn't
> > they?
> 
> Separate sleep state is needed because of the pin control bindings and
> design.
> 
> A separate sleep state is required to let pin control client driver (in this
> case
> Exynos I2S driver) let to choose when it is okay to switch pads into sleep
> state and I see no other way to achieve this.

Maybe the pinctrl API should be extended for this? Doing this in DTS
just for purpose of passing information between drivers (consumer and
provider) looks odd.

Anyway, you are proposing a new binding so please Cc devicetree mailing
list and device tree maintainers.

> > In the patch 2/9, existing configuration:
> > 716         i2s0_bus: i2s0-bus {
> > (...)
> > 719                 samsung,pin-function = <EXYNOS_PIN_FUNC_2>;
> > 720                 samsung,pin-pud = <EXYNOS_PIN_PULL_NONE>;
> > 721                 samsung,pin-drv = <EXYNOS5420_PIN_DRV_LV1>;
> > 722         };
> > 
> > additional configuration:
> > +       i2s0_bus_slp: i2s0-bus-slp {
> > +               samsung,pin-function = <EXYNOS_PIN_FUNC_INPUT>;
> > +               samsung,pin-pud = <EXYNOS_PIN_PULL_NONE>;
> > +               samsung,pin-drv = <EXYNOS5420_PIN_DRV_LV1>;
> > +               samsung,pin-con-pdn = <EXYNOS_PIN_PDN_INPUT>;
> > +               samsung,pin-pud-pdn = <EXYNOS_PIN_PULL_NONE>;
> > +               samsung,off-state;
> > +       };
> 
> I agree that it would be possible to get rid of "samsung,off-state" property
> and
> just detect off state when func/pud pair matches power down func/pud.
> 
> > > It indicates that pin control
> > > +  can notify runtime power management subsystem, that it is ready for runtime
> > > +  suspend if its all pins are configured for such state. This in turn might
> > > +  allow to turn respective power domain off to reduce power consumption.
> > What do you mean by "notifying RPM subsystem"? Either this is
> > description of hardware in certain mode (sleep state) or this is not
> > device tree property.
> 
> Maybe I wrote the description with a view too limited to the kernel
> operation
> perspective, but if we remove the need to mark state as off, the above
> description
> will not be needed.

But still it wouldn't be describing any hardware property, would it?

Best regards,
Krzysztof

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij Dec. 30, 2016, 9:23 a.m. UTC | #5
On Tue, Dec 27, 2016 at 4:33 PM, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> On Tue, Dec 27, 2016 at 11:30:57AM +0100, Marek Szyprowski wrote:

>> Separate sleep state is needed because of the pin control bindings and
>> design.
>>
>> A separate sleep state is required to let pin control client driver (in this
>> case
>> Exynos I2S driver) let to choose when it is okay to switch pads into sleep
>> state and I see no other way to achieve this.
>
> Maybe the pinctrl API should be extended for this? Doing this in DTS
> just for purpose of passing information between drivers (consumer and
> provider) looks odd.

I don't understand what is going on but you are all smart people so I
guess you're right :)

Tell me if I can help clarifying something...

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Marek Szyprowski Dec. 30, 2016, 11:55 a.m. UTC | #6
Hi Krzysztof,

On 2016-12-27 16:33, Krzysztof Kozlowski wrote:
> On Tue, Dec 27, 2016 at 11:30:57AM +0100, Marek Szyprowski wrote:
>> On 2016-12-25 20:19, Krzysztof Kozlowski wrote:
>>> On Fri, Dec 23, 2016 at 01:24:47PM +0100, Marek Szyprowski wrote:
>>>> Add support for special property "samsung,off-state", which indicates a special
>>>> state suitable for device's "sleep" state. Its pin values/properties should
>>>> match the configuration in power down mode. It indicates that pin controller
>>>> can notify runtime power management subsystem, that it is ready for runtime
>>>> suspend if its all pins are configured for such state. This in turn might
>>>> allow to turn respective power domain off to reduce power consumption.
>>>>
>>>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>>>> ---
>>>>    Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt | 8 ++++++++
>>>>    drivers/pinctrl/samsung/pinctrl-samsung.c                     | 4 ++++
>>>>    drivers/pinctrl/samsung/pinctrl-samsung.h                     | 1 +
>>>>    3 files changed, 13 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
>>>> index b7bd2e12a269..354eea0e7798 100644
>>>> --- a/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
>>>> +++ b/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
>>>> @@ -105,6 +105,7 @@ Required Properties:
>>>>      - samsung,pin-drv: Drive strength configuration.
>>>>      - samsung,pin-pud-pdn: Pull up/down configuration in power down mode.
>>>>      - samsung,pin-drv-pdn: Drive strength configuration in power down mode.
>>>> +  - samsung,off-state: Mark this configuration as suitable for bank power off.
>>>>      The values specified by these config properties should be derived from the
>>>>      hardware manual and these values are programmed as-is into the pin
>>>> @@ -113,6 +114,13 @@ Required Properties:
>>>>      Note: A child should include atleast a pin function selection property or
>>>>      pin configuration property (one or more) or both.
>>>> +  Note: Special property "samsung,off-state" indicates that this state can
>>>> +  be used for device's "sleep" pins state. Its pin values/properties should
>>>> +  match the configuration in power down mode.
>>> Why power down values cannot be used for sleep state? Why you need
>>> separate pin control state? If pins values should match power down
>>> configuration, then they could just be added to default state, couldn't
>>> they?
>> Separate sleep state is needed because of the pin control bindings and
>> design.
>>
>> A separate sleep state is required to let pin control client driver (in this
>> case
>> Exynos I2S driver) let to choose when it is okay to switch pads into sleep
>> state and I see no other way to achieve this.
> Maybe the pinctrl API should be extended for this? Doing this in DTS
> just for purpose of passing information between drivers (consumer and
> provider) looks odd.

Well, I don't know if it is odd or not, but that's how it is used now 
and I see
no reason to reinvent wheel. Please check it yourself:
$ git grep \"sleep\" arch/arm/boot/dts | wc -l
101

> Anyway, you are proposing a new binding so please Cc devicetree mailing
> list and device tree maintainers.

I'm just using the generic pinctrl bindings, but it might make some sense to
add a note to Exynos i2s driver that a sleep pin control state is needed if
one wants to get power domain to be turned off.

>>> In the patch 2/9, existing configuration:
>>> 716         i2s0_bus: i2s0-bus {
>>> (...)
>>> 719                 samsung,pin-function = <EXYNOS_PIN_FUNC_2>;
>>> 720                 samsung,pin-pud = <EXYNOS_PIN_PULL_NONE>;
>>> 721                 samsung,pin-drv = <EXYNOS5420_PIN_DRV_LV1>;
>>> 722         };
>>>
>>> additional configuration:
>>> +       i2s0_bus_slp: i2s0-bus-slp {
>>> +               samsung,pin-function = <EXYNOS_PIN_FUNC_INPUT>;
>>> +               samsung,pin-pud = <EXYNOS_PIN_PULL_NONE>;
>>> +               samsung,pin-drv = <EXYNOS5420_PIN_DRV_LV1>;
>>> +               samsung,pin-con-pdn = <EXYNOS_PIN_PDN_INPUT>;
>>> +               samsung,pin-pud-pdn = <EXYNOS_PIN_PULL_NONE>;
>>> +               samsung,off-state;
>>> +       };
>> I agree that it would be possible to get rid of "samsung,off-state" property
>> and
>> just detect off state when func/pud pair matches power down func/pud.
>>
>>>> It indicates that pin control
>>>> +  can notify runtime power management subsystem, that it is ready for runtime
>>>> +  suspend if its all pins are configured for such state. This in turn might
>>>> +  allow to turn respective power domain off to reduce power consumption.
>>> What do you mean by "notifying RPM subsystem"? Either this is
>>> description of hardware in certain mode (sleep state) or this is not
>>> device tree property.
>> Maybe I wrote the description with a view too limited to the kernel
>> operation
>> perspective, but if we remove the need to mark state as off, the above
>> description
>> will not be needed.
> But still it wouldn't be describing any hardware property, would it?

Well, it somehow describes the hardware, because the pin state when it 
is allowed
to turn off the power domain is board specific. I should move that part 
to the
Odroid dts, because there might be a board which require other pin 
values in power
down mode.

Best regards
Krzysztof Kozlowski Dec. 30, 2016, 3:05 p.m. UTC | #7
On Fri, Dec 30, 2016 at 12:55:27PM +0100, Marek Szyprowski wrote:
> Hi Krzysztof,
> 
> On 2016-12-27 16:33, Krzysztof Kozlowski wrote:
> > On Tue, Dec 27, 2016 at 11:30:57AM +0100, Marek Szyprowski wrote:
> > > On 2016-12-25 20:19, Krzysztof Kozlowski wrote:
> > > > On Fri, Dec 23, 2016 at 01:24:47PM +0100, Marek Szyprowski wrote:
> > > > > Add support for special property "samsung,off-state", which indicates a special
> > > > > state suitable for device's "sleep" state. Its pin values/properties should
> > > > > match the configuration in power down mode. It indicates that pin controller
> > > > > can notify runtime power management subsystem, that it is ready for runtime
> > > > > suspend if its all pins are configured for such state. This in turn might
> > > > > allow to turn respective power domain off to reduce power consumption.
> > > > > 
> > > > > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> > > > > ---
> > > > >    Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt | 8 ++++++++
> > > > >    drivers/pinctrl/samsung/pinctrl-samsung.c                     | 4 ++++
> > > > >    drivers/pinctrl/samsung/pinctrl-samsung.h                     | 1 +
> > > > >    3 files changed, 13 insertions(+)
> > > > > 
> > > > > diff --git a/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
> > > > > index b7bd2e12a269..354eea0e7798 100644
> > > > > --- a/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
> > > > > +++ b/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
> > > > > @@ -105,6 +105,7 @@ Required Properties:
> > > > >      - samsung,pin-drv: Drive strength configuration.
> > > > >      - samsung,pin-pud-pdn: Pull up/down configuration in power down mode.
> > > > >      - samsung,pin-drv-pdn: Drive strength configuration in power down mode.
> > > > > +  - samsung,off-state: Mark this configuration as suitable for bank power off.
> > > > >      The values specified by these config properties should be derived from the
> > > > >      hardware manual and these values are programmed as-is into the pin
> > > > > @@ -113,6 +114,13 @@ Required Properties:
> > > > >      Note: A child should include atleast a pin function selection property or
> > > > >      pin configuration property (one or more) or both.
> > > > > +  Note: Special property "samsung,off-state" indicates that this state can
> > > > > +  be used for device's "sleep" pins state. Its pin values/properties should
> > > > > +  match the configuration in power down mode.
> > > > Why power down values cannot be used for sleep state? Why you need
> > > > separate pin control state? If pins values should match power down
> > > > configuration, then they could just be added to default state, couldn't
> > > > they?
> > > Separate sleep state is needed because of the pin control bindings and
> > > design.
> > > 
> > > A separate sleep state is required to let pin control client driver (in this
> > > case
> > > Exynos I2S driver) let to choose when it is okay to switch pads into sleep
> > > state and I see no other way to achieve this.
> > Maybe the pinctrl API should be extended for this? Doing this in DTS
> > just for purpose of passing information between drivers (consumer and
> > provider) looks odd.
> 
> Well, I don't know if it is odd or not, but that's how it is used now and I
> see
> no reason to reinvent wheel. Please check it yourself:
> $ git grep \"sleep\" arch/arm/boot/dts | wc -l
> 101

These drivers, at least not all of them, are not using the existence of
sleep mode configuration as a indication of possible runtime sleep. You
are mixing here different ideas.

> 
> > Anyway, you are proposing a new binding so please Cc devicetree mailing
> > list and device tree maintainers.
> 
> I'm just using the generic pinctrl bindings, but it might make some sense to
> add a note to Exynos i2s driver that a sleep pin control state is needed if
> one wants to get power domain to be turned off.

The samsung,off-state is a extension of the existing binding, so please
Cc the devicetree and maintainers. Why you see a problem in it?

Best regards,
Krzysztof

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
index b7bd2e12a269..354eea0e7798 100644
--- a/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
+++ b/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
@@ -105,6 +105,7 @@  Required Properties:
   - samsung,pin-drv: Drive strength configuration.
   - samsung,pin-pud-pdn: Pull up/down configuration in power down mode.
   - samsung,pin-drv-pdn: Drive strength configuration in power down mode.
+  - samsung,off-state: Mark this configuration as suitable for bank power off.
 
   The values specified by these config properties should be derived from the
   hardware manual and these values are programmed as-is into the pin
@@ -113,6 +114,13 @@  Required Properties:
   Note: A child should include atleast a pin function selection property or
   pin configuration property (one or more) or both.
 
+  Note: Special property "samsung,off-state" indicates that this state can
+  be used for device's "sleep" pins state. Its pin values/properties should
+  match the configuration in power down mode. It indicates that pin control
+  can notify runtime power management subsystem, that it is ready for runtime
+  suspend if its all pins are configured for such state. This in turn might
+  allow to turn respective power domain off to reduce power consumption.
+
   The client nodes that require a particular pin function selection and/or
   pin configuration should use the bindings listed in the "pinctrl-bindings.txt"
   file.
diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.c b/drivers/pinctrl/samsung/pinctrl-samsung.c
index a7b7d75373f2..301169d2b6e1 100644
--- a/drivers/pinctrl/samsung/pinctrl-samsung.c
+++ b/drivers/pinctrl/samsung/pinctrl-samsung.c
@@ -692,6 +692,10 @@  static int samsung_pinctrl_create_function(struct device *dev,
 	}
 
 	func->name = func_np->full_name;
+	if (of_property_read_bool(func_np, "samsung,off-state"))
+		func->rpm_active = false;
+	else
+		func->rpm_active = true;
 
 	func->groups = devm_kzalloc(dev, npins * sizeof(char *), GFP_KERNEL);
 	if (!func->groups)
diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.h b/drivers/pinctrl/samsung/pinctrl-samsung.h
index 32b949e2a89b..edeafa00abd3 100644
--- a/drivers/pinctrl/samsung/pinctrl-samsung.h
+++ b/drivers/pinctrl/samsung/pinctrl-samsung.h
@@ -280,6 +280,7 @@  struct samsung_pmx_func {
 	const char		**groups;
 	u8			num_groups;
 	u32			val;
+	bool			rpm_active;
 };
 
 /* list of all exported SoC specific data */