[PATCH 0/2] Add provision to keep idle state disabled

[PATCH 0/2] Add provision to keep idle state disabled

[Tushar Nimkar]

CPUidle already has CPUIDLE_FLAG_OFF to keep idle state disabled,
Lets extend the support to set this flag using device tree.

This allows to keep an idle state disabled and they can be enabled back using
sysfs after certain point using below command.

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

This helps in cases where vendors want to keep cpuidle off until home
screen comes up. In the past attempt was done at [1] but it was not considered
safe option to export cpu_idle_poll_ctrl().

[1] https://patchwork.kernel.org/project/linux-arm-msm/patch/1637831676-32737-1-git-send-email-quic_mkshah@quicinc.com/

Thanks,
Tushar Nimkar.

Tushar Nimkar (2):
  dt-bindings: cpu: idle-states: Add idle-state-disabled property
  cpuidle: dt: Add support to keep idle state disabled

 Documentation/devicetree/bindings/cpu/idle-states.yaml | 8 ++++++++
 drivers/cpuidle/dt_idle_states.c                       | 3 +++
 2 files changed, 11 insertions(+)

-- 
2.17.1

[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

[PATCH 2/2] cpuidle: dt: Add support to keep idle state disabled

[Tushar Nimkar]

Mark the idle state as disabled using CPUIDLE_FLAG_OFF when
idle-state-disabled property is present.

Such idle states stays disabled and can be enabled using below command.

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

Signed-off-by: Tushar Nimkar <quic_tnimkar@quicinc.com>
---
 drivers/cpuidle/dt_idle_states.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/cpuidle/dt_idle_states.c b/drivers/cpuidle/dt_idle_states.c
index 12fec92a85fd..3d50181512a3 100644
--- a/drivers/cpuidle/dt_idle_states.c
+++ b/drivers/cpuidle/dt_idle_states.c
@@ -79,6 +79,9 @@ static int init_state_node(struct cpuidle_state *idle_state,
 	idle_state->flags = CPUIDLE_FLAG_RCU_IDLE;
 	if (of_property_read_bool(state_node, "local-timer-stop"))
 		idle_state->flags |= CPUIDLE_FLAG_TIMER_STOP;
+
+	if (of_property_read_bool(state_node, "idle-state-disabled"))
+		idle_state->flags |= CPUIDLE_FLAG_OFF;
 	/*
 	 * TODO:
 	 *	replace with kstrdup and pointer assignment when name
-- 
2.17.1

Re: [PATCH 0/2] Add provision to keep idle state disabled

[Ulf Hansson]

On Thu, 8 Jun 2023 at 10:56, Tushar Nimkar <quic_tnimkar@quicinc.com> wrote:
>
> CPUidle already has CPUIDLE_FLAG_OFF to keep idle state disabled,
> Lets extend the support to set this flag using device tree.
>
> This allows to keep an idle state disabled and they can be enabled back using
> sysfs after certain point using below command.
>
> echo N > /sys/devices/system/cpu/cpuX/cpuidle/stateX/disable
>
> This helps in cases where vendors want to keep cpuidle off until home
> screen comes up. In the past attempt was done at [1] but it was not considered
> safe option to export cpu_idle_poll_ctrl().
>
> [1] https://patchwork.kernel.org/project/linux-arm-msm/patch/1637831676-32737-1-git-send-email-quic_mkshah@quicinc.com/

Before considering this, I don't recall if I ever received a reply to
my earlier suggestion to the above thread. So, let me repeat my
question.

I am wondering if a similar improvement can be achieved by
modularizing the cpuidle-psci driver. If insmodding it after the
homescreen, we allow only ARM WFI during boot. This should achieve the
similar results as we get with $subject series, right?

[...]

>
> Thanks,
> Tushar Nimkar.
>
> Tushar Nimkar (2):
>   dt-bindings: cpu: idle-states: Add idle-state-disabled property
>   cpuidle: dt: Add support to keep idle state disabled
>
>  Documentation/devicetree/bindings/cpu/idle-states.yaml | 8 ++++++++
>  drivers/cpuidle/dt_idle_states.c                       | 3 +++
>  2 files changed, 11 insertions(+)
>

Kind regards
Uffe

Re: [PATCH 0/2] Add provision to keep idle state disabled

[Tushar Nimkar]

Thanks for response Ulf.

On 6/12/2023 3:56 PM, Ulf Hansson wrote:
> On Thu, 8 Jun 2023 at 10:56, Tushar Nimkar <quic_tnimkar@quicinc.com> wrote:
>>
>> CPUidle already has CPUIDLE_FLAG_OFF to keep idle state disabled,
>> Lets extend the support to set this flag using device tree.
>>
>> This allows to keep an idle state disabled and they can be enabled back using
>> sysfs after certain point using below command.
>>
>> echo N > /sys/devices/system/cpu/cpuX/cpuidle/stateX/disable
>>
>> This helps in cases where vendors want to keep cpuidle off until home
>> screen comes up. In the past attempt was done at [1] but it was not considered
>> safe option to export cpu_idle_poll_ctrl().
>>
>> [1] https://patchwork.kernel.org/project/linux-arm-msm/patch/1637831676-32737-1-git-send-email-quic_mkshah@quicinc.com/
> 
> Before considering this, I don't recall if I ever received a reply to
> my earlier suggestion to the above thread. So, let me repeat my
> question.
> 
> I am wondering if a similar improvement can be achieved by
> modularizing the cpuidle-psci driver. If insmodding it after the
> homescreen, we allow only ARM WFI during boot. This should achieve the
> similar results as we get with $subject series, right?
> 
> [...]
> 
This will not work for targets which does not have DLKM support.

>>
>> Thanks,
>> Tushar Nimkar.
>>
>> Tushar Nimkar (2):
>>    dt-bindings: cpu: idle-states: Add idle-state-disabled property
>>    cpuidle: dt: Add support to keep idle state disabled
>>
>>   Documentation/devicetree/bindings/cpu/idle-states.yaml | 8 ++++++++
>>   drivers/cpuidle/dt_idle_states.c                       | 3 +++
>>   2 files changed, 11 insertions(+)
>>
> 
> Kind regards
> Uffe

Thanks,
Tushar

Re: [PATCH 0/2] Add provision to keep idle state disabled

[Ulf Hansson]

On Wed, 14 Jun 2023 at 08:43, Tushar Nimkar <quic_tnimkar@quicinc.com> wrote:
>
> Thanks for response Ulf.
>
> On 6/12/2023 3:56 PM, Ulf Hansson wrote:
> > On Thu, 8 Jun 2023 at 10:56, Tushar Nimkar <quic_tnimkar@quicinc.com> wrote:
> >>
> >> CPUidle already has CPUIDLE_FLAG_OFF to keep idle state disabled,
> >> Lets extend the support to set this flag using device tree.
> >>
> >> This allows to keep an idle state disabled and they can be enabled back using
> >> sysfs after certain point using below command.
> >>
> >> echo N > /sys/devices/system/cpu/cpuX/cpuidle/stateX/disable
> >>
> >> This helps in cases where vendors want to keep cpuidle off until home
> >> screen comes up. In the past attempt was done at [1] but it was not considered
> >> safe option to export cpu_idle_poll_ctrl().
> >>
> >> [1] https://patchwork.kernel.org/project/linux-arm-msm/patch/1637831676-32737-1-git-send-email-quic_mkshah@quicinc.com/
> >
> > Before considering this, I don't recall if I ever received a reply to
> > my earlier suggestion to the above thread. So, let me repeat my
> > question.
> >
> > I am wondering if a similar improvement can be achieved by
> > modularizing the cpuidle-psci driver. If insmodding it after the
> > homescreen, we allow only ARM WFI during boot. This should achieve the
> > similar results as we get with $subject series, right?
> >
> > [...]
> >
> This will not work for targets which does not have DLKM support.

Right. I am not saying it's the perfect solution, but it seems like it
could potentially solve the problem for many cases.

If you want some help to turn the cpuidle-psci driver into a loadable
module, just reach out, I am happy to help.

[...]

Kind regards
Uffe

Re: [PATCH 0/2] Add provision to keep idle state disabled

[Tushar Nimkar]

Many thanks again,

On 6/16/2023 4:25 PM, Ulf Hansson wrote:
> On Wed, 14 Jun 2023 at 08:43, Tushar Nimkar <quic_tnimkar@quicinc.com> wrote:

> 
> Right. I am not saying it's the perfect solution, but it seems like it
> could potentially solve the problem for many cases.
> 
> If you want some help to turn the cpuidle-psci driver into a loadable
> module, just reach out, I am happy to help.
> 
Thanks :)
Making cpuidle-psci as loadable does not hold good for target does not 
support DLKM, in addition to it rpmh driver has dependency on 
cpuidle-psci for pm-domain and rpmh probe will get defer, their are 
driver which depends on rpmh probe like interconnect, clk etc. And 
eventually dependent driver probe defers which are essential for Linux 
boot-up.
Hope you got scenario for getting probe defer if we make cpuidle-psci as 
loadable.

I have below options as well
[A]: Can we think of making "governor/param_governor" 
module_param_string, string named governor only to load. In that way 
need to remove check [3]. Let's say string passed as "teo" then it will 
not load "menu" and loads "teo" once comes-up.

[B]: Can we think of making cpuidle.off as writable, let governors to 
register (i.e remove check [4]) and allow cpuidle_init() to happen (i.e 
remove check [5])
So in this way cpuidle.off=1, your idle state can not be selected 
because [6] and later we can write off=0 to let same check [6] to fail.

[C]: Coming to this series approach...What is best way to utilize 
already present Flag-CPUIDLE_FLAG_OFF ?
Since we can not add new DT property to take decision in driver as it's 
not HW feature to be expose in device tree [7]. Can we introduce new 
module_param() for making idle-state disable default and utilize 
CPUIDLE_FLAG_OFF? maybe similar to [8]

happy to hear your thoughts!


[3] 
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/cpuidle/governor.c?h=next-20230620#n93

[4] 
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/cpuidle/governor.c?h=next-20230620#n86

[5] 
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/cpuidle/cpuidle.c?h=next-20230620#n808

[6] 
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/kernel/sched/idle.c?h=next-20230620#n167

[7] 
https://lore.kernel.org/lkml/20230608085544.16211-1-quic_tnimkar@quicinc.com/T/#m5d6012b0dfcff700f48c0efbba629382f18ee33b

[8] 
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/idle/intel_idle.c?h=next-20230620#n2160
> [...]
> 
> Kind regards
> Uffe


Thanks,
Tushar

Re: [PATCH 0/2] Add provision to keep idle state disabled

[Ulf Hansson]

On Wed, 21 Jun 2023 at 08:21, Tushar Nimkar <quic_tnimkar@quicinc.com> wrote:
>
> Many thanks again,
>
> On 6/16/2023 4:25 PM, Ulf Hansson wrote:
> > On Wed, 14 Jun 2023 at 08:43, Tushar Nimkar <quic_tnimkar@quicinc.com> wrote:
>
> >
> > Right. I am not saying it's the perfect solution, but it seems like it
> > could potentially solve the problem for many cases.
> >
> > If you want some help to turn the cpuidle-psci driver into a loadable
> > module, just reach out, I am happy to help.
> >
> Thanks :)

Np!

> Making cpuidle-psci as loadable does not hold good for target does not
> support DLKM, in addition to it rpmh driver has dependency on
> cpuidle-psci for pm-domain and rpmh probe will get defer, their are
> driver which depends on rpmh probe like interconnect, clk etc. And
> eventually dependent driver probe defers which are essential for Linux
> boot-up.
> Hope you got scenario for getting probe defer if we make cpuidle-psci as
> loadable.

I understand your concern, but you have got my idea wrong.

I was suggesting turning the cpuidle-psci driver into a loadable
module - not the cpuidle-psci-domain driver. The latter is the genpd
provider, which consumers like rpmh need to probe.

>
> I have below options as well
> [A]: Can we think of making "governor/param_governor"
> module_param_string, string named governor only to load. In that way
> need to remove check [3]. Let's say string passed as "teo" then it will
> not load "menu" and loads "teo" once comes-up.
>
> [B]: Can we think of making cpuidle.off as writable, let governors to
> register (i.e remove check [4]) and allow cpuidle_init() to happen (i.e
> remove check [5])
> So in this way cpuidle.off=1, your idle state can not be selected
> because [6] and later we can write off=0 to let same check [6] to fail.
>
> [C]: Coming to this series approach...What is best way to utilize
> already present Flag-CPUIDLE_FLAG_OFF ?
> Since we can not add new DT property to take decision in driver as it's
> not HW feature to be expose in device tree [7]. Can we introduce new
> module_param() for making idle-state disable default and utilize
> CPUIDLE_FLAG_OFF? maybe similar to [8]
>
> happy to hear your thoughts!

In general I am not in favor of module parameters, but maybe it's the
best option to solve this problem. We need Rafael's and Daniel's
opinion to conclude.

However, to me, I still think the easiest approach would be to turn
the cpuidle-psci driver into a loadable module. Let me hack on that
and post a few patches that you can test for this.

>
>
> [3]
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/cpuidle/governor.c?h=next-20230620#n93
>
> [4]
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/cpuidle/governor.c?h=next-20230620#n86
>
> [5]
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/cpuidle/cpuidle.c?h=next-20230620#n808
>
> [6]
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/kernel/sched/idle.c?h=next-20230620#n167
>
> [7]
> https://lore.kernel.org/lkml/20230608085544.16211-1-quic_tnimkar@quicinc.com/T/#m5d6012b0dfcff700f48c0efbba629382f18ee33b
>
> [8]
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/idle/intel_idle.c?h=next-20230620#n2160
> > [...]
> >

Kind regards
Uffe

Re: [PATCH 0/2] Add provision to keep idle state disabled

[Tushar Nimkar]

Thank you for checking this Ulf,

On 6/22/2023 6:31 PM, Ulf Hansson wrote:
> On Wed, 21 Jun 2023 at 08:21, Tushar Nimkar <quic_tnimkar@quicinc.com> wrote:
>>
>> Many thanks again,
>>
>> On 6/16/2023 4:25 PM, Ulf Hansson wrote:
>>> On Wed, 14 Jun 2023 at 08:43, Tushar Nimkar <quic_tnimkar@quicinc.com> wrote:
>>
>>>
>>> Right. I am not saying it's the perfect solution, but it seems like it
>>> could potentially solve the problem for many cases.
>>>
>>> If you want some help to turn the cpuidle-psci driver into a loadable
>>> module, just reach out, I am happy to help.
>>>
>> Thanks :)
> 
> Np!
> 
>> Making cpuidle-psci as loadable does not hold good for target does not
>> support DLKM, in addition to it rpmh driver has dependency on
>> cpuidle-psci for pm-domain and rpmh probe will get defer, their are
>> driver which depends on rpmh probe like interconnect, clk etc. And
>> eventually dependent driver probe defers which are essential for Linux
>> boot-up.
>> Hope you got scenario for getting probe defer if we make cpuidle-psci as
>> loadable.
> 
> I understand your concern, but you have got my idea wrong.
> 
> I was suggesting turning the cpuidle-psci driver into a loadable
> module - not the cpuidle-psci-domain driver. The latter is the genpd
> provider, which consumers like rpmh need to probe.
> 
hmm, I need to revisit.
>>
>> I have below options as well
>> [A]: Can we think of making "governor/param_governor"
>> module_param_string, string named governor only to load. In that way
>> need to remove check [3]. Let's say string passed as "teo" then it will
>> not load "menu" and loads "teo" once comes-up.
>>
>> [B]: Can we think of making cpuidle.off as writable, let governors to
>> register (i.e remove check [4]) and allow cpuidle_init() to happen (i.e
>> remove check [5])
>> So in this way cpuidle.off=1, your idle state can not be selected
>> because [6] and later we can write off=0 to let same check [6] to fail.
>>
>> [C]: Coming to this series approach...What is best way to utilize
>> already present Flag-CPUIDLE_FLAG_OFF ?
>> Since we can not add new DT property to take decision in driver as it's
>> not HW feature to be expose in device tree [7]. Can we introduce new
>> module_param() for making idle-state disable default and utilize
>> CPUIDLE_FLAG_OFF? maybe similar to [8]
>>
>> happy to hear your thoughts!
> 
> In general I am not in favor of module parameters, but maybe it's the
> best option to solve this problem. We need Rafael's and Daniel's
> opinion to conclude.
> 
Yea, I double thought before proposing module parameters options. But on 
other hand to justify CPUIDLE_FLAG_OFF looks good.
Will wait for Rafael's and Daniel's opinion.

> However, to me, I still think the easiest approach would be to turn
> the cpuidle-psci driver into a loadable module. Let me hack on that
> and post a few patches that you can test for this.
> 
Sure,
>>
>>
>> [3]
>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/cpuidle/governor.c?h=next-20230620#n93
>>
>> [4]
>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/cpuidle/governor.c?h=next-20230620#n86
>>
>> [5]
>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/cpuidle/cpuidle.c?h=next-20230620#n808
>>
>> [6]
>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/kernel/sched/idle.c?h=next-20230620#n167
>>
>> [7]
>> https://lore.kernel.org/lkml/20230608085544.16211-1-quic_tnimkar@quicinc.com/T/#m5d6012b0dfcff700f48c0efbba629382f18ee33b
>>
>> [8]
>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/idle/intel_idle.c?h=next-20230620#n2160
>>> [...]
>>>
> 
> Kind regards
> Uffe

Thanks,
Tushar