On Fri, Jul 24, 2015 at 11:54:18AM +0800, Chen-Yu Tsai wrote:
Hi,
On Sat, May 2, 2015 at 12:06 AM, Nicolas Pitre nicolas.pitre@linaro.org wrote:
This concerns the following helpers:
__mcpm_cpu_going_down() __mcpm_cpu_down() __mcpm_outbound_enter_critical() __mcpm_outbound_leave_critical() __mcpm_cluster_state()They are and should only be used by the core code now. Therefore their declarations are removed from mcpm.h and their definitions are made static, hence the need to move them before their users which accounts for the bulk of this patch.
I'm looking for some advice. On the Allwinner A80, at least on mainline, there is no external PMU or embedded controller in charge of power controls. What this means is that I'm doing power sequencing in the kernel as part of the MCPM calls, specifically powering down cores and clusters in the .wait_for_powerdown callback. (I don't think it's reasonable or even possible to power down stuff in .*_powerdown_prepare)
Previously I was using __mcpm_cluster_state() to check if the last core in a cluster was to be powered off, and thus the whole cluster could be turned off as well. I could also check if the individual power gates or resets are asserted, but if a core was already scheduled to be brought up, and MCPM common framework didn't call .cluster_powerup, there might be a problem.
It's been a while since I looked at this stuff in detail, so Nico may want to put me right ... but here's my 2 cents:
__mcpm_cluster_state() should be considered an implementation detail of mcpm. If you feel you need to call it from your driver code, that's a warning that your code is probably racy -- unless you can explain otherwise.
When you say you have no external power controller, does this mean that you need to poll in Linux for the affected cpus/clusters to reach WFI and then hit the relevant clamps, clocks and/or regulators yourself?
If so you're effectively implementing a multithreaded power controller in software, suggesting that you need some state tracking and locking between the various mcpm methods in your code. However, you can use the normal locking and atomics APIs for this.
Since the MCPM lock is held when calling all the relevant methods except for wait_for_powerdown(), this nearly satisfies the locking requirement by itself. You are correct that the hardware operations associated with physically powering things down will need to be done in wait_for_powerdown() though, and the MCPM lock is not held when calling that.
This would motivate two solutions that I can see:
a) Expose the mcpm lock so that driver code can lock/unlock it during critical regions in wait_for_powerdown().
b) Refactor wait_for_powerdown() so that the locking, sleeping and timeout code is moved into the mcpm generic code, and the wait_for_powerdown() method is replaced with something like "cpu_is_powered_down()".
Further to this, this "software power controller" really just maps one power sequencing model onto another, so there's a chance it could be a generic mcpm driver than can be reused across multiple platforms.
Cheers ---Dave