[PATCH 0/5] cpuidle: psci: Various improvements for PSCI PM domains

[Lukasz Luba]

On 7/7/20 1:51 PM, Ulf Hansson wrote:
> On Tue, 7 Jul 2020 at 14:37, Lukasz Luba <lukasz.luba at 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 at 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