[PATCH RFC v2 09/16] arm: domain: Add platform callbacks for domain power on/off

[lina.iyer@linaro.org (Lina Iyer)]

On Fri, Jul 03 2015 at 05:36 -0600, Geert Uytterhoeven wrote:
>Hi Lina,
>
>On Thu, Jul 2, 2015 at 9:38 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
>> On Tue, Jun 30 2015 at 09:10 -0600, Geert Uytterhoeven wrote:
>>> On Mon, Jun 29, 2015 at 6:32 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
>>>> On Mon, Jun 29 2015 at 07:36 -0600, Geert Uytterhoeven wrote:

[...]

>> With what you have suggested, the platform driver creates the genpd and
>> would pass the CPU genpd to the common code for common operations. (This
>> was what was done in [1]). The platform driver would set the power_on()
>> and power_off() callbacks and that would have to be overriden in order
>> handle common CPU domain suspend/resume activities. Overwriting members
>> of an object allocated by the platform driver, is something we should
>> avoid.
>
>Instead of letting the generic code override the .power_{on,off}() callbacks,
>the platform code could call the generic CPU-related methods from its own
>.power_{on,off}() callbacks?
>
We dont want to impose on platform drivers to call back into the CPU
domain core code to do common domain power down activities. On a generic
ARM CPU domain, we may not even need a platform specific domain
callbacks. IMHO, this is not desirable.

>struct rmobile_pm_domain already has .suspend() and .resume() methods.
>The former is used to e.g. prevent the PM domains containing CPUs to be
>powered down (in the absence of cpuidle integration). That requires scanning
>the DT for CPUs, and it would indeed be good to have that scanning support
>in generic code.
>The latter became unused after the removal of sh7372 support, which did have
>some cpuidle integration.
>
The CPUs may still be attached to domain, even if they are not powered
off by CPUIdle. The code increases the reference count for every online
CPU and any CPUs that may come online. The reference count, only goes
down when the CPU powers down through CPUIdle or hotplug. So, if a CPU
is not integrated with CPUIdle, it would remain in use (unless powered
off by hotplug) and the domain would not be powered down.

>> Or instead of allocating the memory in your platform driver for the CPU
>> genpd, you could query and get the genpd and then add your additions on
>> top. You could add other flags like GENPD_FLAG_PM_CLK, but *not*
>> overwrite the power on/off callbacks in the genpd. They still have to be
>> registered separately like in this patch. Again, not every elegant, IMO.
>
>Just wondering, can I set up the .attach_dev() and .detach_dev()?
>
I am presuming, you are concerned with the fore mentioned case of CPUs
not participating in CPUIdle. With pm_runtime_put_sync() not happening
for CPUs that do not power down, you should not have to worry about the
domain being powered down.

Please correct me if I misunderstood your point.

>Actually, for R-Mobile hardware I don't have a need to set GENPD_FLAG_PM_CLK
>or .attach_dev() and .detach_dev() for CPU PM domains, as they are only
>needed for devices with MSTP clocks. CPU and L2 cache don't have these,
>and there are no other devices in e.g. a3sm and a2sl.
>
>The GIC has an MSTP clock, but it's part of a different power domain.
>
Okay.

[...]

>> This the common code would get its own callbacks and when that genpd
>> powers off, the platform genpd would power down. But no new code is
>> needed in your platform driver. The benefit is that platform code and
>> generic CPU domain code may exist and act in parallel and may only be
>> related if specified in the DT and the problem with that approach is
>> that this virtual PM domain is not a h/w domain, hence specifying in DT
>> is questionable.
>
>Indeed, I don't like this option, as the DT would no longer describe HW,
>but the Linux implementation.
>
>So let's continue with your approach, and see how it turns out. We can
>always change and improvide code, while changing DT is more complicated.
>
Agreed.

Thanks for your time, Geert.

-- Lina