On Tue, May 26, 2020 at 10:59:48AM +0200, Stephan Gerhold wrote: > > Considering that CPR is not an actual power domain, CPR gives > > adjustments to VDD_APC, but I don't know of any other device > > connected to VDD_APC, other than the CPU, so in hindsight the CPR > > driver probably should have been implemented using .target_index(), > > rather than as a power domain provider using performance states. > > I suppose having CPR, MEMACC etc as power domain providers is a bit > overkill, given there is just one consumer. However, at least the > "performance state" part fits quite well in my opinion. At the end > all these requirements represent some performance state that must be > set when the CPU frequency is changed. > For MX, it makes sense to model it as a power domain provider, and for it to have its own OPP table, since this actually is a power domain. For CPR, I think that the target_index() model of just giving an index in a frequency table is much better, the OPP library can still be used to get the frequencies/frequency_table. Since at least for Qualcom CPU's, the corner (opp-level) is defined as an increasing number 1,2,3,4, without skips. Even if it wasn't always without skips, we could just put opp-level in the CPU opp table, and get it from there. The only thing that the corner is used for really, is to use it as an index the local drv->corner array, which is where the (current) VDD_APC voltage is stored for each index/corner. For CPR, the .target_index() in cpufreq-dt.c gets called, which is supplied with an index, but the index gets converted to a frequency. This frequency is then sent to the OPP library, and is then converted back to an index of the same value (just increased by one), before cpr_set_performance_state() is called (which then has to subtract one). In this case, all the extra overhead of going via genpd is totally unnecessary. This is totally correct when setting a performance state on a power domain like MX, since for an actual power domain you might have multiple consumers, so you need to go via genpd. Considering that CPR is not a power domain, I wish the driver wasn't designed around performance states, which, _for the CPR case_, is misleading, unnecessary, and adds extra overhead for no reason. I realize the irony of me criticizing my own code. I simply know better now, and wish I had designed it differently :) Kind regards, Niklas
