Re: [PATCH] clk: mediatek: Export CPU mux clocks for CPU frequency control

[Sascha Hauer <s.hauer@pengutronix.de>]

+Cc Viresh Kumar

Viresh, this is the patch for the underlying clocks for the Mediatek
cpufreq driver.

On Thu, Mar 05, 2015 at 10:43:21AM +0800, Pi-Cheng Chen wrote:
> Hi Sascha,
> 
> On 4 March 2015 at 19:21, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > On Wed, Mar 04, 2015 at 06:49:11PM +0800, pi-cheng.chen wrote:
> >> This patch adds CPU mux clocks which are used by Mediatek cpufreq driver
> >> for intermediate clock source switching. This patch is based on Mediatek
> >> clock driver patches[1].
> >>
> >> [1] http://thread.gmane.org/gmane.linux.kernel/1892436
> >>
> >> Signed-off-by: pi-cheng.chen <pi-cheng.chen@linaro.org>
> >> ---
> >> +static long clk_cpumux_determine_rate(struct clk_hw *hw, unsigned long rate,
> >> +                                   unsigned long min_rate,
> >> +                                   unsigned long max_rate,
> >> +                                   unsigned long *best_parent_rate,
> >> +                                   struct clk_hw **best_parent_p)
> >> +{
> >> +     struct clk *clk = hw->clk, *parent;
> >> +     unsigned long parent_rate;
> >> +     int i;
> >> +
> >> +     for (i = MAINPLL_INDEX; i >= ARMPLL_INDEX; i--) {
> >> +             parent = clk_get_parent_by_index(clk, i);
> >> +             if (!parent)
> >> +                     return 0;
> >> +
> >> +             if (i == MAINPLL_INDEX) {
> >> +                     parent_rate = __clk_get_rate(parent);
> >> +                     if (parent_rate == rate)
> >> +                             break;
> >> +             }
> >> +
> >> +             parent_rate = __clk_round_rate(parent, rate);
> >> +     }
> >> +
> >> +     *best_parent_rate = parent_rate;
> >> +     *best_parent_p = __clk_get_hw(parent);
> >> +     return parent_rate;
> >> +}
> >
> > Why this determine_rate hook? If you want to switch the clock to some
> > intermediate parent I would assume you do this explicitly by setting the
> > parent and not implicitly by setting a rate.
> >
> 
> I use determine_rate hook here because I am using generic cpufreq-dt
> driver and I want to make clock switching transparent to cpufreq-dt.
> I.e. when I am trying to switch the clock from ARMPLL to MAINPLL, I
> call clk_set_rate() with the rate of MAINPLL, and determine_rate will
> select MAINPLL as the new parent.

We have clk_set_parent for changing the parent and clk_set_rate to
change the rate. Use the former for changing the parent and the latter
for changing the rate. What you are interested in is changing the
parent, so use clk_set_parent for this and not abuse a side effect
of clk_set_rate.

My suggestion is to take another approach. Implement clk_set_rate for
these muxes and in the set_rate hook:

- switch mux to intermediate PLL parent
- call clk_set_rate() for the real parent PLL
- switch mux back to real parent PLL

This way the things happening behind the scenes are completely transparent
to the cpufreq driver and you can use cpufreq-dt as is without changes.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |