| Message ID | 20200615152054.6819-1-ulf.hansson@linaro.org |
|---|---|
| Headers | show |
| Series | cpuidle: psci: Various improvements for PSCI PM domains | expand |
On Mon, 15 Jun 2020 at 17:20, Ulf Hansson <ulf.hansson@linaro.org> wrote: > > The main change in this series is done in patch 5/5, which implements support > to prevent domain idlestates until all PSCI PM domain consumers are ready for > it. To reach that point the corresponding code for cpuidle-psci and > cpuidle-psci-domain, needed to be converted into platform drivers, which is > done by the earlier changes in the series. > > Additionally, some improvements have been made to the error path, which becomes > easier when the code gets converted to platform drivers. > > Deployment for a Qcom SoC, which actually takes full benefit of these changes > are also in the pipe, but deferring then a bit until $subject series have been > discussed. Sudeep, Lorenzo, Would you mind sharing your opinions on this series please? Kind regards Uffe > > Kind regards > Ulf Hansson > > Ulf Hansson (5): > cpuidle: psci: Fail cpuidle registration if set OSI mode failed > cpuidle: psci: Fix error path via converting to a platform driver > cpuidle: psci: Split into two separate build objects > cpuidle: psci: Convert PM domain to platform driver > cpuidle: psci: Prevent domain idlestates until consumers are ready > > drivers/cpuidle/Kconfig.arm | 10 ++ > drivers/cpuidle/Makefile | 5 +- > drivers/cpuidle/cpuidle-psci-domain.c | 74 +++++++++----- > drivers/cpuidle/cpuidle-psci.c | 141 +++++++++++++++----------- > drivers/cpuidle/cpuidle-psci.h | 11 +- > 5 files changed, 150 insertions(+), 91 deletions(-) > > -- > 2.20.1 >
Hi Ulf, On 6/15/20 4:20 PM, Ulf Hansson wrote: > The main change in this series is done in patch 5/5, which implements support > to prevent domain idlestates until all PSCI PM domain consumers are ready for > it. To reach that point the corresponding code for cpuidle-psci and > cpuidle-psci-domain, needed to be converted into platform drivers, which is > done by the earlier changes in the series. > > Additionally, some improvements have been made to the error path, which becomes > easier when the code gets converted to platform drivers. > > Deployment for a Qcom SoC, which actually takes full benefit of these changes > are also in the pipe, but deferring then a bit until $subject series have been > discussed. > > Kind regards > Ulf Hansson > > Ulf Hansson (5): > cpuidle: psci: Fail cpuidle registration if set OSI mode failed > cpuidle: psci: Fix error path via converting to a platform driver > cpuidle: psci: Split into two separate build objects > cpuidle: psci: Convert PM domain to platform driver > cpuidle: psci: Prevent domain idlestates until consumers are ready > > drivers/cpuidle/Kconfig.arm | 10 ++ > drivers/cpuidle/Makefile | 5 +- > drivers/cpuidle/cpuidle-psci-domain.c | 74 +++++++++----- > drivers/cpuidle/cpuidle-psci.c | 141 +++++++++++++++----------- > drivers/cpuidle/cpuidle-psci.h | 11 +- > 5 files changed, 150 insertions(+), 91 deletions(-) > Since I am interested in some CPU idle statistics (residency, etc), I would like to help you and Sudeep in reviewing the patch set. Could you clarify some bit below? 1. There is Qcom SoC which has dependencies between PSCI PM domain consumers and this CPU idle - thus we cannot register and use CPU idle driver till related drivers fully setup. 2. The proposed solution is to use platform driver and plat. device for this CPU idle driver, to have access to deferred probe mechanism and wait for the consumer drivers fully probed state. 3. Do you have maybe some estimations how long it takes for these consumers to be fully probed? 4. Changing just this CPU idle driver registration point (to late_initcall()) phase in time is not a solution. Regards, Lukasz
Hi Lukaz, On Tue, 30 Jun 2020 at 12:23, Lukasz Luba <lukasz.luba@arm.com> wrote: > > Hi Ulf, > > On 6/15/20 4:20 PM, Ulf Hansson wrote: > > The main change in this series is done in patch 5/5, which implements support > > to prevent domain idlestates until all PSCI PM domain consumers are ready for > > it. To reach that point the corresponding code for cpuidle-psci and > > cpuidle-psci-domain, needed to be converted into platform drivers, which is > > done by the earlier changes in the series. > > > > Additionally, some improvements have been made to the error path, which becomes > > easier when the code gets converted to platform drivers. > > > > Deployment for a Qcom SoC, which actually takes full benefit of these changes > > are also in the pipe, but deferring then a bit until $subject series have been > > discussed. > > > > Kind regards > > Ulf Hansson > > > > Ulf Hansson (5): > > cpuidle: psci: Fail cpuidle registration if set OSI mode failed > > cpuidle: psci: Fix error path via converting to a platform driver > > cpuidle: psci: Split into two separate build objects > > cpuidle: psci: Convert PM domain to platform driver > > cpuidle: psci: Prevent domain idlestates until consumers are ready > > > > drivers/cpuidle/Kconfig.arm | 10 ++ > > drivers/cpuidle/Makefile | 5 +- > > drivers/cpuidle/cpuidle-psci-domain.c | 74 +++++++++----- > > drivers/cpuidle/cpuidle-psci.c | 141 +++++++++++++++----------- > > drivers/cpuidle/cpuidle-psci.h | 11 +- > > 5 files changed, 150 insertions(+), 91 deletions(-) > > > > Since I am interested in some CPU idle statistics (residency, etc), > I would like to help you and Sudeep in reviewing the patch set. Thanks, much appreciated! > > Could you clarify some bit below? > 1. There is Qcom SoC which has dependencies between PSCI PM domain > consumers and this CPU idle - thus we cannot register and use CPU > idle driver till related drivers fully setup. I think you got it right, but let me clarify. On Qcom SoC, PSCI PM domain consumers aren't solely CPU devices, but also the 'qcom,rpmh-rsc' device is a consumer, for example. That doesn't mean the CPU idle driver can't be probed/initialized, but rather that the PSCI PM domain must not be powered off. The power off needs to wait until all the consumers of the PSCI PM domain have been registered/probed. See more details in the commit message of patch5. > 2. The proposed solution is to use platform driver and plat. device > for this CPU idle driver, to have access to deferred probe mechanism and > wait for the consumer drivers fully probed state. Correct, but let me fill in some more. I would like to use the ->sync_state() callback of the PSCI PM domain driver, as a way to understand that all consumers have been probed. > 3. Do you have maybe some estimations how long it takes for these > consumers to be fully probed? I am not sure I understand the reason for the question. Anyway, at this point, I am looking at the qcom,rpmh-rsc device, which is being probed by the drivers/soc/qcom/rpmh-rsc.c driver. Moving forward, in principle it can be any device/driver that is a consumer of the PSCI PM domain. I am not even excluding that drivers can be modules. It should work for all cases. > 4. Changing just this CPU idle driver registration point (to > late_initcall()) phase in time is not a solution. Correct, it doesn't work. Playing with initcalls doesn't guarantee anything when it comes to making sure all consumers are ready. Hope this clarifies things a bit for you, but just tell me if there is anything more I can do to further explain things. Kind regards Uffe
On 7/7/20 12:53 PM, Ulf Hansson wrote: > Hi Lukaz, > > On Tue, 30 Jun 2020 at 12:23, Lukasz Luba <lukasz.luba@arm.com> wrote: >> >> Hi Ulf, >> >> On 6/15/20 4:20 PM, Ulf Hansson wrote: >>> The main change in this series is done in patch 5/5, which implements support >>> to prevent domain idlestates until all PSCI PM domain consumers are ready for >>> it. To reach that point the corresponding code for cpuidle-psci and >>> cpuidle-psci-domain, needed to be converted into platform drivers, which is >>> done by the earlier changes in the series. >>> >>> Additionally, some improvements have been made to the error path, which becomes >>> easier when the code gets converted to platform drivers. >>> >>> Deployment for a Qcom SoC, which actually takes full benefit of these changes >>> are also in the pipe, but deferring then a bit until $subject series have been >>> discussed. >>> >>> Kind regards >>> Ulf Hansson >>> >>> Ulf Hansson (5): >>> cpuidle: psci: Fail cpuidle registration if set OSI mode failed >>> cpuidle: psci: Fix error path via converting to a platform driver >>> cpuidle: psci: Split into two separate build objects >>> cpuidle: psci: Convert PM domain to platform driver >>> cpuidle: psci: Prevent domain idlestates until consumers are ready >>> >>> drivers/cpuidle/Kconfig.arm | 10 ++ >>> drivers/cpuidle/Makefile | 5 +- >>> drivers/cpuidle/cpuidle-psci-domain.c | 74 +++++++++----- >>> drivers/cpuidle/cpuidle-psci.c | 141 +++++++++++++++----------- >>> drivers/cpuidle/cpuidle-psci.h | 11 +- >>> 5 files changed, 150 insertions(+), 91 deletions(-) >>> >> >> Since I am interested in some CPU idle statistics (residency, etc), >> I would like to help you and Sudeep in reviewing the patch set. > > Thanks, much appreciated! > >> >> Could you clarify some bit below? >> 1. There is Qcom SoC which has dependencies between PSCI PM domain >> consumers and this CPU idle - thus we cannot register and use CPU >> idle driver till related drivers fully setup. > > I think you got it right, but let me clarify. > > On Qcom SoC, PSCI PM domain consumers aren't solely CPU devices, but > also the 'qcom,rpmh-rsc' device is a consumer, for example. > > That doesn't mean the CPU idle driver can't be probed/initialized, but > rather that the PSCI PM domain must not be powered off. The power off > needs to wait until all the consumers of the PSCI PM domain have been > registered/probed. > > See more details in the commit message of patch5. > >> 2. The proposed solution is to use platform driver and plat. device >> for this CPU idle driver, to have access to deferred probe mechanism and >> wait for the consumer drivers fully probed state. > > Correct, but let me fill in some more. > > I would like to use the ->sync_state() callback of the PSCI PM domain > driver, as a way to understand that all consumers have been probed. > >> 3. Do you have maybe some estimations how long it takes for these >> consumers to be fully probed? > > I am not sure I understand the reason for the question. I was wondering if this is because of HW issue of long setup, thus we need to wait a bit longer with drivers deferred probing. But this is not the case as I can see now. > > Anyway, at this point, I am looking at the qcom,rpmh-rsc device, which > is being probed by the drivers/soc/qcom/rpmh-rsc.c driver. Moving > forward, in principle it can be any device/driver that is a consumer > of the PSCI PM domain. I am not even excluding that drivers can be > modules. It should work for all cases. The late_initcall won't help, this is a really tough requirement: being a module... > >> 4. Changing just this CPU idle driver registration point (to >> late_initcall()) phase in time is not a solution. > > Correct, it doesn't work. > > Playing with initcalls doesn't guarantee anything when it comes to > making sure all consumers are ready. I agree, especially when modules are involved. > > Hope this clarifies things a bit for you, but just tell me if there is > anything more I can do to further explain things. Yes, thank you. I can see now why you need this explicit ->sync_state() callback. I don't see better solution to what you have proposed here. I will go through the patches once again to check and add some reviewed-by. Regards, Lukasz > > Kind regards > Uffe >
On Tue, 7 Jul 2020 at 14:37, Lukasz Luba <lukasz.luba@arm.com> wrote: > > > > On 7/7/20 12:53 PM, Ulf Hansson wrote: > > Hi Lukaz, > > > > On Tue, 30 Jun 2020 at 12:23, Lukasz Luba <lukasz.luba@arm.com> wrote: > >> > >> Hi Ulf, > >> > >> On 6/15/20 4:20 PM, Ulf Hansson wrote: > >>> The main change in this series is done in patch 5/5, which implements support > >>> to prevent domain idlestates until all PSCI PM domain consumers are ready for > >>> it. To reach that point the corresponding code for cpuidle-psci and > >>> cpuidle-psci-domain, needed to be converted into platform drivers, which is > >>> done by the earlier changes in the series. > >>> > >>> Additionally, some improvements have been made to the error path, which becomes > >>> easier when the code gets converted to platform drivers. > >>> > >>> Deployment for a Qcom SoC, which actually takes full benefit of these changes > >>> are also in the pipe, but deferring then a bit until $subject series have been > >>> discussed. > >>> > >>> Kind regards > >>> Ulf Hansson > >>> > >>> Ulf Hansson (5): > >>> cpuidle: psci: Fail cpuidle registration if set OSI mode failed > >>> cpuidle: psci: Fix error path via converting to a platform driver > >>> cpuidle: psci: Split into two separate build objects > >>> cpuidle: psci: Convert PM domain to platform driver > >>> cpuidle: psci: Prevent domain idlestates until consumers are ready > >>> > >>> drivers/cpuidle/Kconfig.arm | 10 ++ > >>> drivers/cpuidle/Makefile | 5 +- > >>> drivers/cpuidle/cpuidle-psci-domain.c | 74 +++++++++----- > >>> drivers/cpuidle/cpuidle-psci.c | 141 +++++++++++++++----------- > >>> drivers/cpuidle/cpuidle-psci.h | 11 +- > >>> 5 files changed, 150 insertions(+), 91 deletions(-) > >>> > >> > >> Since I am interested in some CPU idle statistics (residency, etc), > >> I would like to help you and Sudeep in reviewing the patch set. > > > > Thanks, much appreciated! > > > >> > >> Could you clarify some bit below? > >> 1. There is Qcom SoC which has dependencies between PSCI PM domain > >> consumers and this CPU idle - thus we cannot register and use CPU > >> idle driver till related drivers fully setup. > > > > I think you got it right, but let me clarify. > > > > On Qcom SoC, PSCI PM domain consumers aren't solely CPU devices, but > > also the 'qcom,rpmh-rsc' device is a consumer, for example. > > > > That doesn't mean the CPU idle driver can't be probed/initialized, but > > rather that the PSCI PM domain must not be powered off. The power off > > needs to wait until all the consumers of the PSCI PM domain have been > > registered/probed. > > > > See more details in the commit message of patch5. > > > >> 2. The proposed solution is to use platform driver and plat. device > >> for this CPU idle driver, to have access to deferred probe mechanism and > >> wait for the consumer drivers fully probed state. > > > > Correct, but let me fill in some more. > > > > I would like to use the ->sync_state() callback of the PSCI PM domain > > driver, as a way to understand that all consumers have been probed. > > > >> 3. Do you have maybe some estimations how long it takes for these > >> consumers to be fully probed? > > > > I am not sure I understand the reason for the question. > > I was wondering if this is because of HW issue of long setup, thus > we need to wait a bit longer with drivers deferred probing. > But this is not the case as I can see now. > > > > > > Anyway, at this point, I am looking at the qcom,rpmh-rsc device, which > > is being probed by the drivers/soc/qcom/rpmh-rsc.c driver. Moving > > forward, in principle it can be any device/driver that is a consumer > > of the PSCI PM domain. I am not even excluding that drivers can be > > modules. It should work for all cases. > > The late_initcall won't help, this is a really tough requirement: > being a module... > > > > > >> 4. Changing just this CPU idle driver registration point (to > >> late_initcall()) phase in time is not a solution. > > > > Correct, it doesn't work. > > > > Playing with initcalls doesn't guarantee anything when it comes to > > making sure all consumers are ready. > > I agree, especially when modules are involved. > > > > > Hope this clarifies things a bit for you, but just tell me if there is > > anything more I can do to further explain things. > > Yes, thank you. I can see now why you need this explicit ->sync_state() > callback. > I don't see better solution to what you have proposed here. > I will go through the patches once again to check and add some > reviewed-by. > Thanks a lot for your time! I am just about to post a v2, to re-order the series so patch 3 comes first, according to suggestions from Lina. Please move over to review that version instead, in a few minutes. Kind regards Uffe
On 7/7/20 1:51 PM, Ulf Hansson wrote: > On Tue, 7 Jul 2020 at 14:37, Lukasz Luba <lukasz.luba@arm.com> wrote: >> >> >> >> On 7/7/20 12:53 PM, Ulf Hansson wrote: >>> Hi Lukaz, >>> >>> On Tue, 30 Jun 2020 at 12:23, Lukasz Luba <lukasz.luba@arm.com> wrote: >>>> >>>> Hi Ulf, >>>> >>>> On 6/15/20 4:20 PM, Ulf Hansson wrote: >>>>> The main change in this series is done in patch 5/5, which implements support >>>>> to prevent domain idlestates until all PSCI PM domain consumers are ready for >>>>> it. To reach that point the corresponding code for cpuidle-psci and >>>>> cpuidle-psci-domain, needed to be converted into platform drivers, which is >>>>> done by the earlier changes in the series. >>>>> >>>>> Additionally, some improvements have been made to the error path, which becomes >>>>> easier when the code gets converted to platform drivers. >>>>> >>>>> Deployment for a Qcom SoC, which actually takes full benefit of these changes >>>>> are also in the pipe, but deferring then a bit until $subject series have been >>>>> discussed. >>>>> >>>>> Kind regards >>>>> Ulf Hansson >>>>> >>>>> Ulf Hansson (5): >>>>> cpuidle: psci: Fail cpuidle registration if set OSI mode failed >>>>> cpuidle: psci: Fix error path via converting to a platform driver >>>>> cpuidle: psci: Split into two separate build objects >>>>> cpuidle: psci: Convert PM domain to platform driver >>>>> cpuidle: psci: Prevent domain idlestates until consumers are ready >>>>> >>>>> drivers/cpuidle/Kconfig.arm | 10 ++ >>>>> drivers/cpuidle/Makefile | 5 +- >>>>> drivers/cpuidle/cpuidle-psci-domain.c | 74 +++++++++----- >>>>> drivers/cpuidle/cpuidle-psci.c | 141 +++++++++++++++----------- >>>>> drivers/cpuidle/cpuidle-psci.h | 11 +- >>>>> 5 files changed, 150 insertions(+), 91 deletions(-) >>>>> >>>> >>>> Since I am interested in some CPU idle statistics (residency, etc), >>>> I would like to help you and Sudeep in reviewing the patch set. >>> >>> Thanks, much appreciated! >>> >>>> >>>> Could you clarify some bit below? >>>> 1. There is Qcom SoC which has dependencies between PSCI PM domain >>>> consumers and this CPU idle - thus we cannot register and use CPU >>>> idle driver till related drivers fully setup. >>> >>> I think you got it right, but let me clarify. >>> >>> On Qcom SoC, PSCI PM domain consumers aren't solely CPU devices, but >>> also the 'qcom,rpmh-rsc' device is a consumer, for example. >>> >>> That doesn't mean the CPU idle driver can't be probed/initialized, but >>> rather that the PSCI PM domain must not be powered off. The power off >>> needs to wait until all the consumers of the PSCI PM domain have been >>> registered/probed. >>> >>> See more details in the commit message of patch5. >>> >>>> 2. The proposed solution is to use platform driver and plat. device >>>> for this CPU idle driver, to have access to deferred probe mechanism and >>>> wait for the consumer drivers fully probed state. >>> >>> Correct, but let me fill in some more. >>> >>> I would like to use the ->sync_state() callback of the PSCI PM domain >>> driver, as a way to understand that all consumers have been probed. >>> >>>> 3. Do you have maybe some estimations how long it takes for these >>>> consumers to be fully probed? >>> >>> I am not sure I understand the reason for the question. >> >> I was wondering if this is because of HW issue of long setup, thus >> we need to wait a bit longer with drivers deferred probing. >> But this is not the case as I can see now. >> >> >>> >>> Anyway, at this point, I am looking at the qcom,rpmh-rsc device, which >>> is being probed by the drivers/soc/qcom/rpmh-rsc.c driver. Moving >>> forward, in principle it can be any device/driver that is a consumer >>> of the PSCI PM domain. I am not even excluding that drivers can be >>> modules. It should work for all cases. >> >> The late_initcall won't help, this is a really tough requirement: >> being a module... >> >> >>> >>>> 4. Changing just this CPU idle driver registration point (to >>>> late_initcall()) phase in time is not a solution. >>> >>> Correct, it doesn't work. >>> >>> Playing with initcalls doesn't guarantee anything when it comes to >>> making sure all consumers are ready. >> >> I agree, especially when modules are involved. >> >>> >>> Hope this clarifies things a bit for you, but just tell me if there is >>> anything more I can do to further explain things. >> >> Yes, thank you. I can see now why you need this explicit ->sync_state() >> callback. >> I don't see better solution to what you have proposed here. >> I will go through the patches once again to check and add some >> reviewed-by. >> > > Thanks a lot for your time! I am just about to post a v2, to re-order > the series so patch 3 comes first, according to suggestions from Lina. > > Please move over to review that version instead, in a few minutes. OK, got it in my mailbox, thanks! Regards, Lukasz