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

[Lukasz Luba]

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.

Regards,
Lukasz

> 
> Kind regards
> Uffe
>