Hi Thomas, Mike
> Hi Mike,
>
> On Tue, Jan 28, 2014 at 1:55 AM, Mike Turquette
> <mturquette@xxxxxxxxxx> wrote:
> > Quoting Thomas Abraham (2014-01-18 04:10:51)
> >> From: Thomas Abraham <thomas.ab@xxxxxxxxxxx>
> >>
> >> On some platforms such as the Samsung Exynos, changing the
> >> frequency of the CPU clock requires changing the frequency of the
> >> PLL that is supplying the CPU clock. To change the frequency of
> >> the PLL, the CPU clock is temporarily reparented to another parent
> >> clock.
> >>
> >> The clock frequency of this temporary parent clock could be much
> >> higher than the clock frequency of the PLL at the time of
> >> reparenting. Due to the temporary increase in the CPU clock speed,
> >> the CPU (and any other components in the CPU clock domain such as
> >> dividers, mux, etc.) have to to be operated at a higher voltage
> >> level, called the safe voltage level. This patch adds optional
> >> support to temporarily switch to a safe voltage level during CPU
> >> frequency transitions.
> >>
> >> Cc: Shawn Guo <shawn.guo@xxxxxxxxxx>
> >> Signed-off-by: Thomas Abraham <thomas.ab@xxxxxxxxxxx>
> >
> > I'm not a fan of this change. This corner case should be abstracted
> > away somehow. I had talked to Chander Kayshap previously about
> > handling voltage changes in clock notifier callbacks, which then
> > renders any voltage change as a trivial part of the clock rate
> > transition. That means that this "safe voltage" thing could be
> > handled automagically without any additional code in the CPUfreq
> > driver.
> >
> > There are two nice ways to do this with the clock framework. First
> > is explicit re-parenting with voltage scaling done in the clock
> > rate-change notifiers:
> >
> > clk_set_parent(cpu_clk, temp_parent);
> > /* implicit voltage scaling to "safe voltage" happens above */
> > clk_set_rate(pll, some_rate);
> > clk_set_parent(cpu_clk, pll);
> > /* implicit voltage scaling to nominal OPP voltage happens above */
> >
I must agree with Mike here. In my opinion the above approach is more
compliant with CCF (as I've pointed it out in my other comment - the
cpu_clk has more than one parent and we could switch between them when
needed).
> > The above sequence would require a separate exnyos CPUfreq driver,
> > due to the added clk_set_parent logic.
> >
> > The second way to do this is to abstract the clk re-muxing logic out
> > into the clk driver, which would allow cpufreq-cpu0 to be used for
> > the exynos chips.
>
> This is the approach this patch series takes (patch 2/7). The clock
> re-muxing logic is handled by a clock driver code. The difference from
> what you suggested is that the safe voltage (that may be optionally)
> required before doing the re-muxing is handled here in cpufreq-cpu0
> driver.
>
> The safe voltage setup can be done in the notifier as you suggested.
If the clk_set_parent() approach is not suitable, then cannot we
consider using the one from highbank-cpufreq.c?
Here we have cpufreq-cpu0.c which sets voltage of the cpu_clk.
In the highbank-cpufreq.c there are clock notifiers to change the
voltage.
Cannot Exynos reuse such approach? Why shall we pollute cpufreq-cpu0.c
with another solution?
> But, doing that in cpufreq-cpu0 driver will help other platforms reuse
> this feature if required. Also, if done here, the regulator handling
> is localized in this driver which otherwise would need to be handled
> in two places, cpufreq-cpu0 driver and the clock notifier.
I think that there is a logical distinction between setting voltage for
cpufreq-cpu0 related clock and increasing voltage of reparented clock.
The former fits naturally to cpufreq-cpu0, when the latter seems like
some corner case (as Mike pointed out) for Exynos.
>
> So I tend to prefer the approach in this patch but I am willing to
> consider any suggestions.
Thomas, what do you think about highbank-cpufreq.c approach (with
using clock notifiers)?
Do you think, that it is feasible to reuse it with Exynos?
> Shawn, it would be helpful if you could let
> us know your thoughts on this. I am almost done with testing the v3 of
> this series and want to post it so if there are any objections to the
> changes in this patch, please let me know.
>
> Thanks,
> Thomas.
>
> >
> > I'm more a fan of explicitly listing the Exact Steps for the cpu opp
> > transition in a separate exynos-specific CPUfreq driver, but that's
> > probably an unpopular view.
> >
> > Regards,
> > Mike
> >
> >> ---
> >> .../devicetree/bindings/cpufreq/cpufreq-cpu0.txt | 7 ++++
> >> drivers/cpufreq/cpufreq-cpu0.c | 37
> >> +++++++++++++++++-- 2 files changed, 40 insertions(+), 4
> >> deletions(-)
> >>
> >> diff --git
> >> a/Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt
> >> b/Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt index
> >> f055515..37453ab 100644 ---
> >> a/Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt +++
> >> b/Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt @@
> >> -19,6 +19,12 @@ Optional properties:
> >> - cooling-min-level:
> >> - cooling-max-level:
> >> Please refer to
> >> Documentation/devicetree/bindings/thermal/thermal.txt. +-
> >> safe-opp: Certain platforms require that during a opp transition,
> >> + a system should not go below a particular opp level. For such
> >> systems,
> >> + this property specifies the minimum opp to be maintained during
> >> the
> >> + opp transitions. The safe-opp value is a tuple with first
> >> element
> >> + representing the safe frequency and the second element
> >> representing the
> >> + safe voltage.
> >>
> >> Examples:
> >>
> >> @@ -36,6 +42,7 @@ cpus {
> >> 396000 950000
> >> 198000 850000
> >> >;
> >> + safe-opp = <396000 950000>
> >> clock-latency = <61036>; /* two CLK32 periods */
> >> #cooling-cells = <2>;
> >> cooling-min-level = <0>;
> >> diff --git a/drivers/cpufreq/cpufreq-cpu0.c
> >> b/drivers/cpufreq/cpufreq-cpu0.c index 0c12ffc..075d3d1 100644
> >> --- a/drivers/cpufreq/cpufreq-cpu0.c
> >> +++ b/drivers/cpufreq/cpufreq-cpu0.c
> >> @@ -27,6 +27,8 @@
> >>
> >> static unsigned int transition_latency;
> >> static unsigned int voltage_tolerance; /* in percentage */
> >> +static unsigned long safe_frequency;
> >> +static unsigned long safe_voltage;
> >>
> >> static struct device *cpu_dev;
> >> static struct clk *cpu_clk;
> >> @@ -64,17 +66,30 @@ static int cpu0_set_target(struct
> >> cpufreq_policy *policy, unsigned int index) volt_old =
> >> regulator_get_voltage(cpu_reg); }
> >>
> >> - pr_debug("%u MHz, %ld mV --> %u MHz, %ld mV\n",
> >> + pr_debug("\n\n%u MHz, %ld mV --> %u MHz, %ld mV\n",
> >> old_freq / 1000, volt_old ? volt_old / 1000 : -1,
> >> new_freq / 1000, volt ? volt / 1000 : -1);
> >>
> >> /* scaling up? scale voltage before frequency */
> >> - if (!IS_ERR(cpu_reg) && new_freq > old_freq) {
> >> + if (!IS_ERR(cpu_reg) && new_freq > old_freq &&
> >> + new_freq >= safe_frequency) {
> >> ret = regulator_set_voltage_tol(cpu_reg, volt,
> >> tol); if (ret) {
> >> pr_err("failed to scale voltage up: %d\n",
> >> ret); return ret;
> >> }
> >> + } else if (!IS_ERR(cpu_reg) && old_freq < safe_frequency) {
> >> + /*
> >> + * the scaled up voltage level for the new_freq is
> >> lower
> >> + * than the safe voltage level. so set safe_voltage
> >> + * as the intermediate voltage level and revert it
> >> + * back after the frequency has been changed.
> >> + */
> >> + ret = regulator_set_voltage_tol(cpu_reg,
> >> safe_voltage, tol);
> >> + if (ret) {
> >> + pr_err("failed to set safe voltage: %d\n",
> >> ret);
> >> + return ret;
> >> + }
> >> }
> >>
> >> ret = clk_set_rate(cpu_clk, freq_exact);
> >> @@ -86,7 +101,8 @@ static int cpu0_set_target(struct
> >> cpufreq_policy *policy, unsigned int index) }
> >>
> >> /* scaling down? scale voltage after frequency */
> >> - if (!IS_ERR(cpu_reg) && new_freq < old_freq) {
> >> + if (!IS_ERR(cpu_reg) &&
> >> + (new_freq < old_freq || new_freq <
> >> safe_frequency)) { ret = regulator_set_voltage_tol(cpu_reg, volt,
> >> tol); if (ret) {
> >> pr_err("failed to scale voltage down:
> >> %d\n", ret); @@ -116,6 +132,8 @@ static struct cpufreq_driver
> >> cpu0_cpufreq_driver = {
> >>
> >> static int cpu0_cpufreq_probe(struct platform_device *pdev)
> >> {
> >> + const struct property *prop;
> >> + struct dev_pm_opp *opp;
> >> struct device_node *np;
> >> int ret;
> >>
> >> @@ -165,13 +183,24 @@ static int cpu0_cpufreq_probe(struct
> >> platform_device *pdev) goto out_put_node;
> >> }
> >>
> >> + prop = of_find_property(np, "safe-opp", NULL);
> >> + if (prop) {
> >> + if (prop->value && (prop->length / sizeof(u32)) ==
> >> 2) {
> >> + const __be32 *val;
> >> + val = prop->value;
> >> + safe_frequency = be32_to_cpup(val++);
> >> + safe_voltage = be32_to_cpup(val);
> >> + } else {
> >> + pr_err("invalid safe-opp level
> >> specified\n");
> >> + }
> >> + }
> >> +
> >> of_property_read_u32(np, "voltage-tolerance",
> >> &voltage_tolerance);
> >>
> >> if (of_property_read_u32(np, "clock-latency",
> >> &transition_latency)) transition_latency = CPUFREQ_ETERNAL;
> >>
> >> if (!IS_ERR(cpu_reg)) {
> >> - struct dev_pm_opp *opp;
> >> unsigned long min_uV, max_uV;
> >> int i;
> >>
> >> --
> >> 1.6.6.rc2
> >>
--
Best regards,
Lukasz Majewski
Samsung R&D Institute Poland (SRPOL) | Linux Platform Group
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
