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