Re: [PATCH v2 2/3] ARM: keystone: pm: switch to use generic pm domains

[Ulf Hansson]

Hi Grygorii,

[...]

>>
>> The solution that I propose is to "manually" enable your PM clks
>> during the probe sequence. We can do that as a part of pm_clk_add() or
>
> Done in patch set 3 - but only if !CONFIG_PM_RUNTIME
>
>> we invoke pm_clk_resume() separately, but more important no matter of
>> CONFIG_PM_RUNTIME.
>>
> Why? What benefits will be doing this if CONFIG_PM_RUNTIME=y?
> For Keystone 2 CONFIG_PM_RUNTIME=y is intended to be normal operational mode and
> all devices belongs to Platform bus.
>
> Also, device's resuming operation is usually heavy operation and, taking into
> account deferred probing mechanism and usual implementation of .probe() function,
> your proposition will lead to runtime overhead at least for Platform devices.

I don't quite follow. Why should there be an overhead?

>
> What is usually done in probe:
> <- here you propose to resume device

I didn't suggest we should resume the device here.

I said we should enable the device's PM clocks during ->probe().
That's a different thing.

>
> 1) get resources (IO, IRQ, regulators, GPIO, phys, ..) - for each
> resource -EPROBE_DEFER can be returned.
>
> 2) allocate and fill device context - can fail.

For your information, writing/reading to the device's registers might
not be safe, unless its PM domain is powered.

>
> 3) configure resources (set gpio, enable regulators or phys,..) - can fail
>
> 4) [now] resume device
>
> 5) configure device
>
> 6) setup irq
>
> 7) [optional] suspend device
>
> As you can see from above, the Platform devices aren't need to be enabled before step 5 and,
> if your proposition will be accepted, it will lead to few additional resume/suspend
> cycles per-device. It's not good as for me. Is it?

Why will it lead to a few additional resume/suspend cycles per device?
I don't follow.

>
>
>> The driver could then be responsible to invoke pm_runtime_set_active()
>> to reflect that all runtime PM resources are enabled.
>
> [runtime_pm.txt] - this is recovery function and caller should be very careful.

What? I couldn't find this stated anywhere in the documentation.

This is what's stated though:

5. Runtime PM Initialization, Device Probing and Removal
[...]
In addition to that, the initial runtime PM status of all devices is
'suspended', but it need not reflect the actual physical state of the device.
Thus, if the device is initially active (i.e. it is able to process I/O), its
runtime PM status must be changed to 'active', with the help of
pm_runtime_set_active(), before pm_runtime_enable() is called for the device.
[...]

>
> Again, from implementation point of view:
> -- how it's done now:
>     .probe()
>         pm_runtime_enable(&pdev->dev);
>         pm_runtime_get_sync(&pdev->dev);
>
>     .remove()
>         pm_runtime_put_sync(&pdev->dev);
>         pm_runtime_disable(&pdev->dev);
>
> -- how it will be:
>      .probe()
>         //pm_runtime_enable(&pdev->dev);
>         //pm_runtime_get_sync(&pdev->dev);
>
>         [optional] call .runtime_resume();
>
>         pm_runtime_set_active(dev);
>         pm_runtime_enable(dev);
>         [optional, to keep device active] pm_runtime_get_sync()
>
>      .remove()
>         [optional] pm_runtime_put_sync(&pdev->dev);
>         pm_runtime_disable(&pdev->dev);
>
>         call .runtime_suspend();
>         pm_runtime_set_suspended(dev);
>
> And that would need to be done for all drivers.

I can't tell the number of drivers you are referring to and how big
impact it would have. Could you maybe summarize which drivers you are
concerned about?

I guess, if we have screwed things up regarding the runtime PM support
for some drivers, we need to fix them. I am also happy to help.

[...]

>> This is wrong!
>>
>> 1) It will break the driver for !CONFIG_PM_RUNTIME.
>
> Hm. It should work. In your driver you have (for the case !CONFIG_PM_RUNTIME):
>         pm_runtime_enable(dev); ------------------------ NOP
>         ret = pm_runtime_get_sync(&pdev->dev); --------- NOP
>         if (ret < 0)
>                 goto err_m2m;
> so, if you add:
>         if (!pm_runtime_enabled(dev)) { ---------------- always FALSE
>                gsc_runtime_resume(dev);
>                /* ^ is the same as
>                 gsc_hw_set_sw_reset(gsc);
>                 gsc_wait_reset(gsc);
>                 gsc_m2m_resume(gsc);
>                */
>        }
> it will work in both cases, because pm_runtime_enabled() == true
> when  CONFIG_PM_RUNTIME=y.

So this might work for some cases and for some not. As stated earlier,
it won't work if the device is runtime PM active.

The better solution is the follow my proposal and thus also conform to
the runtime PM documentation for how ->probe() should be done.

>
>>
>> 2) It would also be broken for CONFIG_PM_RUNTIME for the scenario
>> where a bus also handles runtime PM resources.
>> Typically from the bus' ->probe() this is done:
>> pm_runtime_get_noresume()
>> pm_runtime_set_active()
>
> So, Has your device been enabled by bus?

Yes we have examples of that. drivers/amba/bus.c.

Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/