[PATCH 1/2] dt-bindings: arm: idle-states: Add idle-state-disabled property

[PATCH 1/2] dt-bindings: arm: idle-states: Add idle-state-disabled property

[Tushar Nimkar]

This change adds idle-state-disabled property using which certain or all
idle-states can be kept disabled during boot-up. Once boot-up is completed
same can be enabled using below command.

echo N > /sys/devices/system/cpu/cpuX/cpuidle/stateX/disable

Cc: devicetree@vger.kernel.org
Signed-off-by: Tushar Nimkar <quic_tnimkar@quicinc.com>
---
 Documentation/devicetree/bindings/cpu/idle-states.yaml | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/cpu/idle-states.yaml b/Documentation/devicetree/bindings/cpu/idle-states.yaml
index b8cc826c9501..f999bc666bbd 100644
--- a/Documentation/devicetree/bindings/cpu/idle-states.yaml
+++ b/Documentation/devicetree/bindings/cpu/idle-states.yaml
@@ -358,6 +358,13 @@ patternProperties:
           systems entry-latency-us + exit-latency-us will exceed
           wakeup-latency-us by this duration.
 
+      idle-state-disabled:
+        description: |
+          If present the idle state stays disabled. It can be enabled back from
+          shell using below command.
+          echo N > /sys/devices/system/cpu/cpuX/cpuidle/stateX/disable
+        type: boolean
+
       idle-state-name:
         $ref: /schemas/types.yaml#/definitions/string
         description:
@@ -548,6 +555,7 @@ examples:
             CPU_SLEEP_0_0: cpu-sleep-0-0 {
                 compatible = "arm,idle-state";
                 local-timer-stop;
+                idle-state-disabled;
                 arm,psci-suspend-param = <0x0010000>;
                 entry-latency-us = <250>;
                 exit-latency-us = <500>;
-- 
2.17.1

Re: [PATCH 1/2] dt-bindings: arm: idle-states: Add idle-state-disabled property

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2085 bytes --]

Hey Tushar,

On Thu, Jun 08, 2023 at 02:25:42PM +0530, Tushar Nimkar wrote:
> This change adds idle-state-disabled property using which certain or all
> idle-states can be kept disabled during boot-up. Once boot-up is completed
> same can be enabled using below command.
> 
> echo N > /sys/devices/system/cpu/cpuX/cpuidle/stateX/disable
> 
> Cc: devicetree@vger.kernel.org

Firstly, you should CC the dt-bindings maintainers like
get_maintainer.pl would tell you.
Secondly, there are two 1/2 patches in this series.

> Signed-off-by: Tushar Nimkar <quic_tnimkar@quicinc.com>
> ---
>  Documentation/devicetree/bindings/cpu/idle-states.yaml | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/cpu/idle-states.yaml b/Documentation/devicetree/bindings/cpu/idle-states.yaml
> index b8cc826c9501..f999bc666bbd 100644
> --- a/Documentation/devicetree/bindings/cpu/idle-states.yaml
> +++ b/Documentation/devicetree/bindings/cpu/idle-states.yaml
> @@ -358,6 +358,13 @@ patternProperties:
>            systems entry-latency-us + exit-latency-us will exceed
>            wakeup-latency-us by this duration.
>  
> +      idle-state-disabled:
> +        description: |
> +          If present the idle state stays disabled.

> It can be enabled back from
> +          shell using below command.
> +          echo N > /sys/devices/system/cpu/cpuX/cpuidle/stateX/disable

Thirdly, this is operating system specific behaviour, tied to Linux, and
has no place in a binding.

Cheers,
Conor.

> +        type: boolean
> +
>        idle-state-name:
>          $ref: /schemas/types.yaml#/definitions/string
>          description:
> @@ -548,6 +555,7 @@ examples:
>              CPU_SLEEP_0_0: cpu-sleep-0-0 {
>                  compatible = "arm,idle-state";
>                  local-timer-stop;
> +                idle-state-disabled;
>                  arm,psci-suspend-param = <0x0010000>;
>                  entry-latency-us = <250>;
>                  exit-latency-us = <500>;
> -- 
> 2.17.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 1/2] dt-bindings: arm: idle-states: Add idle-state-disabled property

[Tushar Nimkar]

Thanks Conor for reviewing.

On 6/8/2023 2:49 PM, Conor Dooley wrote:
> Hey Tushar,
> 
> On Thu, Jun 08, 2023 at 02:25:42PM +0530, Tushar Nimkar wrote:

> 
> Firstly, you should CC the dt-bindings maintainers like
> get_maintainer.pl would tell you.
> Secondly, there are two 1/2 patches in this series.
> 
1. Sure, I will include dt maintainer in next version.
2. Yes, one of the Patch 1/2 sent by mistake.
I will remove in next version.

> 
> Thirdly, this is operating system specific behaviour, tied to Linux, and
> has no place in a binding.
> 
3. I will remove [echo N > 
/sys/devices/system/cpu/cpuX/cpuidle/stateX/disable] command from 
bindings document.

> Cheers,
> Conor.
> 

Thanks,
Tushar

Re: [PATCH 1/2] dt-bindings: arm: idle-states: Add idle-state-disabled property

[Krzysztof Kozlowski]

On 08/06/2023 10:55, Tushar Nimkar wrote:
> This change adds idle-state-disabled property using which certain or all

Please do not use "This commit/patch", but imperative mood. See longer
explanation here:
https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95

> idle-states can be kept disabled during boot-up. Once boot-up is completed
> same can be enabled using below command.
> 

I don't understand and you did not explain here, why this is useful and
why this is needed.

> echo N > /sys/devices/system/cpu/cpuX/cpuidle/stateX/disable


> 
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Tushar Nimkar <quic_tnimkar@quicinc.com>

Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC.  It might happen, that command when run on an older
kernel, gives you outdated entries.  Therefore please be sure you base
your patches on recent Linux kernel.

> ---
>  Documentation/devicetree/bindings/cpu/idle-states.yaml | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/cpu/idle-states.yaml b/Documentation/devicetree/bindings/cpu/idle-states.yaml
> index b8cc826c9501..f999bc666bbd 100644
> --- a/Documentation/devicetree/bindings/cpu/idle-states.yaml
> +++ b/Documentation/devicetree/bindings/cpu/idle-states.yaml
> @@ -358,6 +358,13 @@ patternProperties:
>            systems entry-latency-us + exit-latency-us will exceed
>            wakeup-latency-us by this duration.
>  
> +      idle-state-disabled:
> +        description: |
> +          If present the idle state stays disabled. It can be enabled back from
> +          shell using below command.
> +          echo N > /sys/devices/system/cpu/cpuX/cpuidle/stateX/disable

This is Linux specific command, so does not fit the bindings.

Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: arm: idle-states: Add idle-state-disabled property

[Tushar Nimkar]

Thanks Krzysztof for reviewing,

On 6/9/2023 6:44 PM, Krzysztof Kozlowski wrote:
> On 08/06/2023 10:55, Tushar Nimkar wrote:
>> This change adds idle-state-disabled property using which certain or all
> 
> Please do not use "This commit/patch", but imperative mood. See longer
> explanation here:
> https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95
> 
Sure, will update in next version.
>> idle-states can be kept disabled during boot-up. Once boot-up is completed
>> same can be enabled using below command.
>>
> 
> I don't understand and you did not explain here, why this is useful and
> why this is needed.
> 
I will update commit text to why this is useful in new version.
>> echo N > /sys/devices/system/cpu/cpuX/cpuidle/stateX/disable
> 
> 
>>
>> Cc: devicetree@vger.kernel.org
>> Signed-off-by: Tushar Nimkar <quic_tnimkar@quicinc.com>
> 
> Please use scripts/get_maintainers.pl to get a list of necessary people
> and lists to CC.  It might happen, that command when run on an older
> kernel, gives you outdated entries.  Therefore please be sure you base
> your patches on recent Linux kernel.
> 
Yes, In new version will take care.
>> ---

>> +          echo N > /sys/devices/system/cpu/cpuX/cpuidle/stateX/disable
> 
> This is Linux specific command, so does not fit the bindings.
Will remove in new version.
> 
> Best regards,
> Krzysztof
> 

Thanks,
Tushar

Re: [PATCH 1/2] dt-bindings: arm: idle-states: Add idle-state-disabled property

[Sudeep Holla]

On Thu, Jun 08, 2023 at 02:25:42PM +0530, Tushar Nimkar wrote:
> This change adds idle-state-disabled property using which certain or all
> idle-states can be kept disabled during boot-up. Once boot-up is completed
> same can be enabled using below command.
>
> echo N > /sys/devices/system/cpu/cpuX/cpuidle/stateX/disable
>
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Tushar Nimkar <quic_tnimkar@quicinc.com>
> ---
>  Documentation/devicetree/bindings/cpu/idle-states.yaml | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/cpu/idle-states.yaml b/Documentation/devicetree/bindings/cpu/idle-states.yaml
> index b8cc826c9501..f999bc666bbd 100644
> --- a/Documentation/devicetree/bindings/cpu/idle-states.yaml
> +++ b/Documentation/devicetree/bindings/cpu/idle-states.yaml
> @@ -358,6 +358,13 @@ patternProperties:
>            systems entry-latency-us + exit-latency-us will exceed
>            wakeup-latency-us by this duration.
>
> +      idle-state-disabled:
> +        description: |
> +          If present the idle state stays disabled. It can be enabled back from
> +          shell using below command.
> +          echo N > /sys/devices/system/cpu/cpuX/cpuidle/stateX/disable
> +        type: boolean
> +

This is clearly a policy and not a hardware or firmware feature to expose
in the device tree. So NACK, why can't you load it modules if you don't want
idle states in the boot.

It is same as choosing any default governor or performance states, will you
add those next ? It is simply policy not a feature/property to be exposed
in the device tree.

--
Regards,
Sudeep

Re: [PATCH 1/2] dt-bindings: arm: idle-states: Add idle-state-disabled property

[Tushar Nimkar]

Thanks for review Sundeep,

On 6/15/2023 2:26 PM, Sudeep Holla wrote:
> On Thu, Jun 08, 2023 at 02:25:42PM +0530, Tushar Nimkar wrote:
>> +      idle-state-disabled:
>> +        description: |
>> +          If present the idle state stays disabled. It can be enabled back from
>> +          shell using below command.
>> +          echo N > /sys/devices/system/cpu/cpuX/cpuidle/stateX/disable
>> +        type: boolean
>> +
> 
> This is clearly a policy and not a hardware or firmware feature to expose
> in the device tree. So NACK, why can't you load it modules if you don't want
> idle states in the boot.
> 
Attempt of making cpuidle governors to modular was rejected in past [2]

[2] 
https://lore.kernel.org/lkml/1637830481-21709-1-git-send-email-quic_mkshah@quicinc.com/#t

> It is same as choosing any default governor or performance states, will you
> add those next ? It is simply policy not a feature/property to be exposed
> in the device tree.
> 
> --
> Regards,
> Sudeep

Thanks,
Tushar

Re: [PATCH 1/2] dt-bindings: arm: idle-states: Add idle-state-disabled property

[Sudeep Holla]

On Fri, Jun 16, 2023 at 11:26:18AM +0530, Tushar Nimkar wrote:
> 
> Thanks for review Sundeep,
> 
> On 6/15/2023 2:26 PM, Sudeep Holla wrote:
> > On Thu, Jun 08, 2023 at 02:25:42PM +0530, Tushar Nimkar wrote:
> > > +      idle-state-disabled:
> > > +        description: |
> > > +          If present the idle state stays disabled. It can be enabled back from
> > > +          shell using below command.
> > > +          echo N > /sys/devices/system/cpu/cpuX/cpuidle/stateX/disable
> > > +        type: boolean
> > > +
> > 
> > This is clearly a policy and not a hardware or firmware feature to expose
> > in the device tree. So NACK, why can't you load it modules if you don't want
> > idle states in the boot.
> > 
> Attempt of making cpuidle governors to modular was rejected in past [2]
>

OK try command line approach to disable all states(you can't get partial
on/off in that case). I don't think the build config is of any use as we
end up enabling it which will affect other platforms.

> [2] https://lore.kernel.org/lkml/1637830481-21709-1-git-send-email-quic_mkshah@quicinc.com/#t
> 
> > It is same as choosing any default governor or performance states, will you
> > add those next ? It is simply policy not a feature/property to be exposed
> > in the device tree.
> > 

The above still holds, so still NACK. It is a policy and not a
hardware/firmware property or feature.

-- 
Regards,
Sudeep

Re: [PATCH 1/2] dt-bindings: arm: idle-states: Add idle-state-disabled property

[Tushar Nimkar]

Thanks again Sudeep,

On 6/16/2023 9:09 PM, Sudeep Holla wrote:
> On Fri, Jun 16, 2023 at 11:26:18AM +0530, Tushar Nimkar wrote:

> 
> OK try command line approach to disable all states(you can't get partial
> on/off in that case). I don't think the build config is of any use as we
> end up enabling it which will affect other platforms.
> 
Do you mean cpuidle.off=1 ?
It will disable idle states but this will not allow cpuidle_init() and 
governors register to happen which mean no way to re-enable idle states.
Do you mean any other command line approach?

> 
> The above still holds, so still NACK. It is a policy and not a
> hardware/firmware property or feature.
> 
Yes, understood!

Thanks,
Tushar

[PATCH 1/2] dt-bindings: cpu: idle-states: Add idle-state-disabled property

[Tushar Nimkar]

This change adds idle-state-disabled property using which certain or all
idle-states can be kept disabled during boot-up. Once boot-up is completed
same can be enabled using below command.

echo N > /sys/devices/system/cpu/cpuX/cpuidle/stateX/disable

Cc: devicetree@vger.kernel.org
Signed-off-by: Tushar Nimkar <quic_tnimkar@quicinc.com>
---
 Documentation/devicetree/bindings/cpu/idle-states.yaml | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/cpu/idle-states.yaml b/Documentation/devicetree/bindings/cpu/idle-states.yaml
index b8cc826c9501..f999bc666bbd 100644
--- a/Documentation/devicetree/bindings/cpu/idle-states.yaml
+++ b/Documentation/devicetree/bindings/cpu/idle-states.yaml
@@ -358,6 +358,13 @@ patternProperties:
           systems entry-latency-us + exit-latency-us will exceed
           wakeup-latency-us by this duration.
 
+      idle-state-disabled:
+        description: |
+          If present the idle state stays disabled. It can be enabled back from
+          shell using below command.
+          echo N > /sys/devices/system/cpu/cpuX/cpuidle/stateX/disable
+        type: boolean
+
       idle-state-name:
         $ref: /schemas/types.yaml#/definitions/string
         description:
@@ -548,6 +555,7 @@ examples:
             CPU_SLEEP_0_0: cpu-sleep-0-0 {
                 compatible = "arm,idle-state";
                 local-timer-stop;
+                idle-state-disabled;
                 arm,psci-suspend-param = <0x0010000>;
                 entry-latency-us = <250>;
                 exit-latency-us = <500>;
-- 
2.17.1

Re: [PATCH 1/2] dt-bindings: cpu: idle-states: Add idle-state-disabled property

[Krzysztof Kozlowski]

On 08/06/2023 10:55, Tushar Nimkar wrote:
> This change adds idle-state-disabled property using which certain or all
> idle-states can be kept disabled during boot-up. Once boot-up is completed
> same can be enabled using below command.
> 
> echo N > /sys/devices/system/cpu/cpuX/cpuidle/stateX/disab

This is a duplicated patch. I already commented on other so to make it
clear: NAK for this one :)

Best regards,
Krzysztof