Hi Stephen,
On Mon, 6 Jul 2015 14:32:10 -0700
Stephen Boyd <sboyd@xxxxxxxxxxxxxx> wrote:
> On 07/06, Boris Brezillon wrote:
> > Clock rates are stored in an unsigned long field, but ->determine_rate()
> > (which returns a rounded rate from a requested one) returns a long
> > value (errors are reported using negative error codes), which can lead
> > to long overflow if the clock rate exceed 2Ghz.
> >
> > Change ->determine_rate() prototype to return 0 or an error code, and pass
> > a pointer to a clk_rate_request structure containing the expected target
> > rate and the rate constraints imposed by clk users.
> >
> > The clk_rate_request structure might be extended in the future to contain
> > other kind of constraints like the rounding policy, the maximum clock
> > inaccuracy or other things that are not yet supported by the CCF
> > (power consumption constraints ?).
> >
> > Signed-off-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxxxxxxx>
>
> Which files did you compile?
>
> drivers/clk/mmp/clk-mix.c: In function ‘mmp_clk_mix_determine_rate’:
> drivers/clk/mmp/clk-mix.c:221:13: error: ‘rate’ undeclared (first use in this function)
>
Hm, I only compile tested the multi_v5 and multi_v7 defconfigs, and
obviously it was a bad idea (just thought all the impacted platforms
were already converted to multiplatform support).
[...]
> > -long omap3_noncore_dpll_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_clk)
> > +int omap3_noncore_dpll_determine_rate(struct clk_hw *hw,
> > + struct clk_rate_request *req)
> > {
> > struct clk_hw_omap *clk = to_clk_hw_omap(hw);
> > struct dpll_data *dd;
> >
> > - if (!hw || !rate)
> > + if (!hw || !req || !req->rate)
>
> Why do we need to check for req? It shouldn't be NULL.
We don't, I'll remove this test.
[...]
> > -long omap4_dpll_regm4xen_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_clk)
> > +int omap4_dpll_regm4xen_determine_rate(struct clk_hw *hw,
> > + struct clk_rate_request *req)
> > {
> > struct clk_hw_omap *clk = to_clk_hw_omap(hw);
> > struct dpll_data *dd;
> >
> > - if (!hw || !rate)
> > + if (!hw || !req || !req->rate)
>
> Same comment here. And why would we care about hw being NULL
> either for that matter.
Yes, but I'm not sure this removal should be done in the same patch.
Let me know if you think otherwise.
> > -static long mmc_clk_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)
> > +static int mmc_clk_determine_rate(struct clk_hw *hw,
> > + struct clk_rate_request *req)
> > {
> > struct clk_mmc *mclk = to_mmc(hw);
> > - unsigned long best = 0;
> >
> > - if ((rate <= 13000000) && (mclk->id == HI3620_MMC_CIUCLK1)) {
> > - rate = 13000000;
> > - best = 26000000;
> > - } else if (rate <= 26000000) {
> > - rate = 25000000;
> > - best = 180000000;
> > - } else if (rate <= 52000000) {
> > - rate = 50000000;
> > - best = 360000000;
> > - } else if (rate <= 100000000) {
> > - rate = 100000000;
> > - best = 720000000;
> > + req->best_parent_hw = __clk_get_hw(__clk_get_parent(hw->clk));
> > +
>
> Where did this come from? We weren't setting the best_parent_p
> pointer before.
It comes from a previous version where I was not assigning the
->best_parent_hw field to the current parent in the core code.
I fixed it in the meantime, but forgot to remove this assignment.
> > -static long
> > -clk_pll_determine_rate(struct clk_hw *hw, unsigned long rate,
> > - unsigned long min_rate, unsigned long max_rate,
> > - unsigned long *p_rate, struct clk_hw **p)
> > +static int
> > +clk_pll_determine_rate(struct clk_hw *hw, struct clk_rate_request *req)
> > {
> > + struct clk *parent = __clk_get_parent(hw->clk);
> > struct clk_pll *pll = to_clk_pll(hw);
> > const struct pll_freq_tbl *f;
> >
> > - f = find_freq(pll->freq_tbl, rate);
> > + req->best_parent_hw = __clk_get_hw(parent);
> > + req->best_parent_rate = __clk_get_rate(parent);
> > +
> > + f = find_freq(pll->freq_tbl, req->rate);
> > if (!f)
> > - return clk_pll_recalc_rate(hw, *p_rate);
> > + req->rate = clk_pll_recalc_rate(hw, req->best_parent_rate);
> > + else
> > + req->rate = f->freq;
> >
> > return f->freq;
>
> return 0?
>
Yes, I'll fix that one too.
Thanks,
Boris
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
