On 02/26/2014 11:00 PM, Mike Turquette wrote:

Quoting Nishanth Menon (2014-02-26 18:34:55)

quoted

+/**
+ * pm_runtime_get_rate() - Returns the device operational frequency
+ * @dev:       Device to handle
+ * @rate:      Returns rate in Hz.
+ *
+ * Returns appropriate error value in case of error conditions, else
+ * returns 0 and rate is updated. The pm_domain logic does all the necessary
+ * operation (which may consider magic hardware stuff) to provide the rate.
+ *
+ * NOTE: the rate returned is a snapshot and in many cases just a bypass
+ * to clk api to set the rate.
+ */
+int pm_runtime_get_rate(struct device *dev, unsigned long *rate)

Instead of "rate", how about we use "level" and leave it undefined as to
what that means? It would be equally valid for level to represent a
clock rate, or an opp from a table of opp's, or a p-state, or some value
passed to a PM microcontroller.

IMHO, from a driver which already exists for multiple SoCs/
architectures, we cannot have variant definitions that a generic
driver will be unable to depend upon. what should such a driver
expect? level == rate OR level == index to p-state or magic PM
controller value?

Today the definitions are very clear to such a driver, pm_runtime APIs
are used for device specific idle management, but for active
management, use clk and regulator code as needed - ofcourse, that
machine specificity triggers the need for the 50 odd cpufreq drivers
we have today - intent was to be able to abstract it enough for a
generic logic to exist.

Code that is tightly coupled to the hardware would simply know what
value to use with no extra sugar.

agreed on the machine specific implementation, but the logic at driver
level will then get tied down to machine specific implementation as well

Generic code would need to get the various supported "levels" populated
at run time, but a DT binding could do that, or a query to the ACPI
tables, or whatever.

then we'd also have to introduce a translator API for drivers that
need frequency -> we go back to the old world of having specific
functions depending on usage in the frequency world, ACPI world, PM
controller world.

That completely breaks the concept of having a higher level function,
right?

quoted

+{
+       unsigned long flags;
+       int error = -ENOSYS;
+
+       if (!rate || !dev)
+               return -EINVAL;
+
+       spin_lock_irqsave(&dev->power.lock, flags);
+       if (!pm_runtime_active(dev)) {
+               error = -EINVAL;
+               goto out;
+       }
+
+       if (dev->pm_domain && dev->pm_domain->active_ops.get_rate)
+               error = dev->pm_domain->active_ops.get_rate(dev, rate);
+out:
+       spin_unlock_irqrestore(&dev->power.lock, flags);
+
+       return error;
+}
+
+/**
+ * pm_runtime_set_rate() - Set a specific rate for the device operation
+ * @dev:       Device to handle
+ * @rate:      Rate to set in Hz
+ *
+ * Returns appropriate error value in case of error conditions, else
+ * returns 0. The pm_domain logic does all the necessary operation (which
+ * may include voltage scale operations or other magic hardware stuff) to
+ * achieve the operation. It is guarenteed that the requested rate is achieved
+ * on returning from this function if return value is 0.
+ */
+int pm_runtime_set_rate(struct device *dev, unsigned long rate)

Additionally I wonder if the function signature should include a way to
specify the sub-unit of a device that we are operating on? This is a way
to tackle the issues you raised regarding multiple clocks per device,
etc. Two approaches come to mind:

int pm_runtime_set_rate(struct device *dev, int index,
				unsigned long rate);

Where index is a sub-unit of struct device *dev.

Here the problem is trying to define what that index is. should it be
clk index? how again would a generic driver be able to consistently
function? lets, for a moment replace that with a string - "clk_name"
to uniquely identify the clk -> but then, it still, in concept makes
it no better than a clk_set_rate since we are uniquely identifying the
clk to operate upon -> and we can definitely add "magic dvfs" stuff on
existing clock framework without a need for additional wrappers (which
what the original $subject series does).

The second approach is
to create a publicly declared structure representing the sub-unit. Some
variations on that theme:

int pm_runtime_set_rate(struct perf_domain *perfdm, unsigned long rate);

or,

int pm_runtime_set_rate(struct generic_power_domain *gpd,
				unsigned long rate);

or whatever that sub-unit looks like. The gpd thing might be a total
layering violation, I don't know. Or perhaps it's a decent idea but it
shouldn't be as a PM runtime call. Again, I dunno.

Again, we goes back to the same question, right? which clock in a
power_domain/perf_domain are we intending with the rate? a device
belongs to a perf domain -> Taking drivers/cpufreq/imx6q-cpufreq.c as
an example.
Clocks needed: arm_clk, pll1_sys_clk, pll1_sw_clk, step_clk,
pll2_pfd2_396m_clk.
regulators needed: arm_reg, pu_reg, soc_reg.

The device we want to set a frequency is the ARM processor. by
describing "perf_domain" or "generic power domain" as a single clock
entity, we might as well use clk_set_rate instead to be specific,
instead of writing one wrapper on top of the entire thing.


I apologize, more I think in this angle, less I think such a generic
api seems feasible.

-- 
Regards,
Nishanth Menon