From: Daniel Lezcano <daniel.lezcano@linaro.org>
To: Lina Iyer <lina.iyer@linaro.org>
Cc: khilman@linaro.org, sboyd@codeaurora.org, galak@codeaurora.org,
linux-arm-msm@vger.kernel.org, linux-pm@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, lorenzo.pieralisi@arm.com,
msivasub@codeaurora.org, devicetree@vger.kernel.org
Subject: Re: [PATCH v13 03/10] qcom: spm: Add Subsystem Power Manager driver
Date: Tue, 02 Dec 2014 16:47:05 +0100 [thread overview]
Message-ID: <547DDEF9.8020105@linaro.org> (raw)
In-Reply-To: <20141202153548.GE499@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
next prev parent reply other threads:[~2014-12-02 15:47 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-11-27 5:24 [PATCH v13 00/10] cpuidle driver for QCOM SoCs: 8064, 8074, 8084 Lina Iyer
2014-11-27 5:24 ` [PATCH v13 01/10] qcom: scm: Move scm-boot files to drivers/soc/qcom/ and include/soc/qcom Lina Iyer
2014-11-27 5:24 ` [PATCH v13 02/10] qcom: scm: Add SCM warmboot support for quad core SoCs Lina Iyer
2014-11-27 5:24 ` [PATCH v13 03/10] qcom: spm: Add Subsystem Power Manager driver Lina Iyer
2014-11-27 8:44 ` Ivan T. Ivanov
2014-12-01 17:57 ` Lina Iyer
2014-11-27 8:52 ` Daniel Lezcano
2014-12-01 18:50 ` Lina Iyer
2014-12-02 9:53 ` Daniel Lezcano
2014-12-02 15:35 ` Lina Iyer
2014-12-02 15:47 ` Daniel Lezcano [this message]
2014-11-27 15:01 ` Lorenzo Pieralisi
2014-12-01 18:57 ` Lina Iyer
2014-12-02 11:10 ` Catalin Marinas
2014-12-02 15:52 ` Lina Iyer
2014-11-27 5:24 ` [PATCH v13 04/10] arm: dts: qcom: Add power-controller device node for 8074 Krait CPUs Lina Iyer
2014-11-27 5:24 ` [PATCH v13 05/10] arm: dts: qcom: Add power-controller device node for 8084 " Lina Iyer
2014-11-27 5:24 ` [PATCH v13 06/10] arm: dts: qcom: Update power-controller device node for 8064 " Lina Iyer
2014-11-27 5:24 ` [PATCH v13 07/10] qcom: cpuidle: Add cpuidle driver for QCOM cpus Lina Iyer
2014-11-27 5:24 ` [PATCH v13 08/10] arm: dts: qcom: Add idle states device nodes for 8074 Lina Iyer
2014-11-27 5:24 ` [PATCH v13 09/10] arm: dts: qcom: Add idle states device nodes for 8084 Lina Iyer
2014-11-27 5:24 ` [PATCH v13 10/10] arm: dts: qcom: Add idle state device nodes for 8064 Lina Iyer
2014-11-27 8:53 ` [PATCH v13 00/10] cpuidle driver for QCOM SoCs: 8064, 8074, 8084 Daniel Lezcano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=547DDEF9.8020105@linaro.org \
--to=daniel.lezcano@linaro.org \
--cc=devicetree@vger.kernel.org \
--cc=galak@codeaurora.org \
--cc=khilman@linaro.org \
--cc=lina.iyer@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=lorenzo.pieralisi@arm.com \
--cc=msivasub@codeaurora.org \
--cc=sboyd@codeaurora.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).