| Message ID | 20230608085544.16211-1-quic_tnimkar@quicinc.com |
|---|---|
| Headers | show |
| Series | Add provision to keep idle state disabled | expand |
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 >
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
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
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
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
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
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