Heiko, On Thu, May 15, 2014 at 12:17 PM, Heiko Stübner <heiko@xxxxxxxxx> wrote: > Am Donnerstag, 15. Mai 2014, 11:18:44 schrieb Doug Anderson: >> Thomas, >> >> On Tue, May 13, 2014 at 6:11 PM, Thomas Abraham <ta.omasab@xxxxxxxxx> wrote: >> > From: Thomas Abraham <thomas.ab@xxxxxxxxxxx> >> > +static int exynos4210_armclk_pre_rate_change(struct clk_notifier_data >> > *ndata, + struct exynos_cpuclk *armclk, void >> > __iomem *base) +{ >> > + struct exynos4210_armclk_data *armclk_data = armclk->data; >> > + unsigned long alt_prate = clk_get_rate(armclk->alt_parent); >> > + unsigned long alt_div, div0, div1, tdiv0, mux_reg; >> > + unsigned long cur_armclk_rate, timeout; >> > + unsigned long flags; >> > + >> > + /* find out the divider values to use for clock data */ >> > + while (armclk_data->prate != ndata->new_rate) { >> > + if (armclk_data->prate == 0) >> > + return -EINVAL; >> > + armclk_data++; >> > + } >> > + >> > + div0 = armclk_data->div0; >> > + div1 = armclk_data->div1; >> > + if (readl(base + SRC_CPU) & EXYNOS4210_MUX_HPM_MASK) { >> > + div1 = readl(base + DIV_CPU1) & EXYNOS4210_DIV1_HPM_MASK; >> > + div1 |= ((armclk_data->div1) & ~EXYNOS4210_DIV1_HPM_MASK); >> > + } >> > + >> > + /* >> > + * if the new and old parent clock speed is less than the clock >> > speed + * of the alternate parent, then it should be ensured that >> > at no point + * the armclk speed is more than the old_prate until >> > the dividers are + * set. >> > + */ >> > + tdiv0 = readl(base + DIV_CPU0); >> > + cur_armclk_rate = ndata->old_rate / EXYNOS4210_ARM_DIV1(tdiv0) / >> > + EXYNOS4210_ARM_DIV2(tdiv0); >> > + if (alt_prate > cur_armclk_rate) { >> > + alt_div = _calc_div(alt_prate, cur_armclk_rate); >> > + _exynos4210_set_armclk_div(base, alt_div); >> > + div0 |= alt_div; >> >> Don't you need to up the voltage here, too? ...I haven't reviewed >> this whole patch (so perhaps it's elsewhere in the patch or in the >> series), but I stumbled upon this while trying to solve a different >> problem and figured I'd check... > > setting the voltage should be done by the cpufreq driver like cpufreq-cpu0 - > whose usage this series intents to allow. > > As I've hijacked Thomas' concept for my current rockchip clock work, I've > already seen this working nicely :-) . I guess I should have been more clear. I was talking more specifically about upping the voltage as part of the mux switch in the case that alt_prate > cur_armclk_rate. ...if you're switching from 200MHz to 300MHz and the alt_prate is 800MHz, you need to account for that fact. The code here accounts for the fact in setting the "armclk_div", but (I don't think) it accounts for the fact that 800MHz will need a higher voltage. As per a separate discussion, a clean solution might be to move the mux switching to the core of CPU_FREQ. That would have the side effect of also making it very easy to send notifications. -Doug -- 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