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