[PATCH] PM / Domains: Power on the PM domain right after attach completes

[Grygorii Strashko]

On 11/19/2014 10:54 AM, Ulf Hansson wrote:
> [...]
> 
>>>
>>> Scenario 5), a platform driver with/without runtime PM callbacks.
>>> ->probe()
>>> - do some initialization
>>> - may fetch handles to runtime PM resources
>>> - pm_runtime_enable()
>>
>> Well, and now how the driver knows if the device is "on" before accessing it?
> 
> In this case the driver don't need to access the device during
> ->probe(). That's postponed until sometime later.
> 
>>
>>> Note 1)
>>> Scenario 1) and 2), both relies on the approach to power on the PM
>>> domain by using pm_runtime_get_sync(). That approach didn't work when
>>> CONFIG_PM_RUNTIME was unset, but we recently decided to fixed that by
>>> the below patch, so that's good!
>>> "[PATCH] PM / domains: Kconfig: always enable PM_RUNTIME when genpd enabled"
>>>
>>> Note 2)
>>> Scenario 3) and 4) use the same principles for managing runtime PM.
>>> These scenarios needs a way to power on the generic PM domain prior
>>> probing the device. The call to pm_runtime_set_active(), prevents an
>>> already powered PM domain from power off until after probe, but that's
>>> not enough.
>>>
>>> Note 3)
>>> The $subject patch, tried to address the issues for scenario 3) and
>>> 4). It does so, but will affect scenario 5) which was working nicely
>>> before. In scenario 5), the $subject patch will cause the generic PM
>>> domain to potentially stay powered after ->probe() even if the device
>>> is runtime PM suspended.
>>
>> Why would it?  If the device is runtime-suspended, the domain will know
>> that, because its callbacks will be used for that.  At least, that's
>> what I'd expect to happen, so is there a problem here?
> 
> Genpd do knows about the device but it doesn’t get a "notification" to
> power off. There are no issues whatsoever for driver.
> 
> This is a somewhat special case. Let's go through an example.
> 
> 1. The PM domain is initially in powered off state.
> 2. The bus ->probe() invokes dev_pm_domain_attach() and then the PM
> domain gets attached to the device.
> 3. $subject patch causes the PM domain to power on.
> 4. A driver ->probe() sequence start, following the Scenario 5).
> 5. The device is initially in runtime PM suspended state and it will
> remain so during ->probe().
> 6. The pm_request_idle() invoked after really_probe() in driver core,
> won't trigger a runtime PM suspend callback to be invoked. In other
> words, genpd don't get a "notification" that it may power off.
> 
> In this state, genpd will either power off from the late_initcall,
> genpd_poweroff_unused() (depending on when the driver was probed) or
> wait until some device's runtime PM suspend callback is invoked at any
> later point.

if I understand things right (thanks to Russell), the Power domain may not
 be powered off not only in above case, but also in some cases when
driver is unloaded.

AMBA bus for example:
static int amba_remove(struct device *dev)
{
	pm_runtime_get_sync(dev); <---------- GPD=on, dev is active, usage_count >= 1
	ret = drv->remove(pcdev); <----------- GPD=on, should do balancing put() to compensate all get() made by driver, usage_count == 1
				  <----------- GPD=on, should do balancing get() to compensate put() in probe, usage_count == 2
	pm_runtime_put_noidle(dev); <---------- GPD=on, dev is active, usage_count == 1

	/* Undo the runtime PM settings in amba_probe() */
	pm_runtime_disable(dev);  <---------- GPD=on, dev is active, usage_count == 1
	pm_runtime_set_suspended(dev); <---------- GPD=on, dev is suspended, usage_count == 1
	pm_runtime_put_noidle(dev); <---------- GPD=on, dev is suspended, usage_count == 0

	amba_put_disable_pclk(pcdev);
	dev_pm_domain_detach(dev, true); <---------- GPD=on, dev is suspended, usage_count == 0

also:
 i2c-qup.c
 i2c-hix5hd2.c
 exynos_drm_gsc.c
 exynos_drm_fimc.c
 ab8500-gpadc.c
 ...

Is it?

> 
>>
>>> I see three options going forward.
>>>
>>> Option 1)
>>> Convert scenario 3) and 4) into using the pm_runtime_get_sync()
>>> approach. There are no theoretical obstacles to do this, but pure
>>> practical. There are a lot of drivers that needs to be converted and
>>> we also need to teach driver authors future wise to not use
>>> pm_runtime_set_active() in this manner.
>>
>> I'd say we need to do something like this anyway.  That is, standardize on
>> *one* approach.  I'm actually not sure what approach is the most useful,
>> but the pm_runtime_get_sync() one seems to be the most popular to me.
>>
>>> Option 2)
>>> Add some kind of get/put API for PM domains. The buses invokes it to
>>> control the power to the PM domain. From what I understand, that's
>>> also what Dmitry think is needed!?
>>> Anyway, that somehow means to proceed with the approach I took in the
>>> below patchset.
>>> [PATCH v3 0/9] PM / Domains: Fix race conditions during boot
>>> http://marc.info/?t=141320907000003&r=1&w=2
>>
>> I don't like that.  The API is already quite complicated in my view and
>> adding even more complexity to it is not going to help in the long run.
> 
> I absolutely agree that we shouldn't add unnecessary APIs and keep
> APIs as simple as possible.
> 
> In that context, I think the effect from proceeding with Option 2)
> also means there are no need for the below APIs any more.
> pm_genpd_poweron()
> pm_genpd_name_poweron() (requires some additional work though)
> pm_genpd_poweroff_unused()
> pm_genpd_dev_need_restore()
> 
> I guess you figured out that I am in favour of Option 2). :-)
> Especially since it cover all scenarios and we don't have to go a fix
> a vast amount of drivers.
> 
>>
>>> Option 3)
>>> Live we the limitation this $subject patch introduces for scenario 5).
>>
>> I'd say, 3) for now and 1) going forward.
> 
[...]

regards,
-grygorii