On 02/26/2014 11:00 PM, Mike Turquette wrote:
> Quoting Nishanth Menon (2014-02-26 18:34:55)
>> +/**
>> + * 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?
>
>> +{
>> + 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
--
To unsubscribe from this list: send the line "unsubscribe cpufreq" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
