Re: [PATCH v13 03/10] qcom: spm: Add Subsystem Power Manager driver

[Daniel Lezcano <daniel.lezcano@linaro.org>]

On 12/02/2014 04:35 PM, Lina Iyer wrote:
> On Tue, Dec 02 2014 at 02:53 -0700, Daniel Lezcano wrote:
>> On 12/01/2014 07:50 PM, Lina Iyer wrote:
>>> On Thu, Nov 27 2014 at 01:52 -0700, Daniel Lezcano wrote:
>>>> On 11/27/2014 06:24 AM, Lina Iyer wrote:
>>>
>>>> +    static bool cpuidle_drv_init;
>>>>
>>>>       ^^^^^^^^^
>>>>
>>>> As already said in a previous comment, please find a way to remove
>>>> that.
>>>>
>>> I will look into it. Stephen and I wanted the cpuidle driver to be
>>> probed only after any of the SPMs are ready. And possibly, only for that
>>> cpu, for which the SPM has been probed.
>>>
>>> To achieve the SPM -> CPUIDLE Device relation, I havent found a good way
>>> to do that. Without using CPUIDLE_MULTIPLE_DRIVERS, initializing each
>>> cpuidle device, separate from the cpuidle driver, requires that I update
>>> the cpuidle_driver->cpumask after probing each SPM device, to allow for
>>> only one driver and cpuidle devices only for the probed cpus. Using the
>>> cpuidle_register_driver(), resets the drv->refcnt in
>>> __cpuidle_driver_init.
>>>
>>> I may need something like this in the else clause of
>>> CPUIDLE_MULTIPLE_DRIVERS -
>>>
>>> static inline struct cpuidle_driver *__cpuidle_get_cpu_driver(int cpu)
>>> {
>>>        struct cpuidle_driver *drv = cpuidle_curr_driver;
>>>
>>>        if (drv && !cpumask_test_cpu(cpu, drv->cpumask))
>>>                 drv = NULL;
>>>
>>>        return drv;
>>> }
>>
>> You don't have to deal with the drv->cpumask and
>> CPUIDLE_MULTIPLE_DRIVERS. This field gives the list of the cpus the
>> driver will have to potentially handle.
>>
>> Just register the driver at the first place with the platform device
>> by using cpuidle_register_driver(drv), I suggest somewhere else than
>> the spm code.
>>
>> The underlying code will do:
>>
>>        if (!drv->cpumask)
>>                drv->cpumask = (struct cpumask *)cpu_possible_mask;
>>
>> Until the cpuidle device is not initialized for a specific cpu, the
>> cpuidle code will have no effect for this cpu. If you register the
>> driver but without registering the cpuidle devices that will result in
>> nothing more than a structure initialized but inoperative.
>>
>> For each SPM being probed:
>>
>> struct cpuidle_device *dev = &per_cpu(cpuidle_dev, cpu);
>> dev->cpu = cpu;
>> cpuidle_register_device(dev);
>>
>> Note cpuidle_dev is exported via cpuidle.h, so you don't have to
>> redeclare a per cpu cpuidle device by your own.
>>
>> So rephrasing all that:
>>
>> (1) in cpuidle-qcom register the driver
>> (2) in the spm code register the device for each spm probe
>>
>> (1) being called before (2).
>>
>> If after that in the code you still have a "static bool
>> cpuidle_drv_init", then I guess we have a problem in the init sequence
>> somewhere else.
>>
> Okay, Thanks. Last night I was able to get a patch working with one
> cpuidle_register_driver and multiple cpuidle_register_device. The issue
> using an module_init() was -
>
> 1. Register a device to probe the cpuidle driver.
> 2. Register cpuidle devices for every SPM probed.

That's it.

> What used to happen is that the cpuidle driver probe always happens
> after the cpuidle devices were created.
>
> I have a solution where I register for the cpuidle device to probe a
> driver, at postcore_initcall() at which point, the cpuidle framework
> would also be available.
>
> I am not sure how elegant it looks, but thats the latest initcall I
> could get a consistent driver initialization before the SPM probe
> (therefore the device registration).

I don't have the code in front of me but we can probably find a nice 
place to probe the cpuidle driver. Is there any pm.c file with an init 
function that can be useful ? In any case, regarding the unusual init 
sequence for the cpuidle driver, I think the probe would deserve a comment.

> I will send the patch this morning.

Ok, cool.

[ ... ]

>>>> There is something wrong with the init sequence. Don't you find weird
>>>> you have to backward search for the cpu belonging to the pdev each
>>>> time the probe function is called ?
>>>>
>>>>
>>> Well, it was that or the SPM device node pointing to the CPU that it
>>> references. It seems more common to have an iterator than the doubly
>>> linked device nodes. I dont have a strong preference either way, just
>>> chose the way that made device nodes easier.
>>>
> This one, I havent addressed. How strongly do you feel about this? I
> still have to iterate through all the cpus to find the cpu node that
> matches the handle specified in the SAW node.

IMO, we can live with that if there is no longer any static variable 
around telling us we initialized something.

Thanks !

   -- Daniel


-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog