Date:	Fri, 17 Apr 2015 16:28:18 +0200
From:	Tomeu Vizoso <tomeu.vizoso@...labora.com>
To:	Michael Turquette <mturquette@...aro.org>
Cc:	Boris Brezillon <boris.brezillon@...e-electrons.com>,
	Stephen Boyd <sboyd@...eaurora.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: Propagating clock rate constraints

On 13 April 2015 at 01:50, Michael Turquette <mturquette@...aro.org> wrote:
> Quoting Boris Brezillon (2015-03-26 16:40:54)
>> Hello,
>>
>> I recently had a problem with the at91 clk implementation: the
>> programmable clock driver is not forwarding set_rate() requests to its
>> parent, meaning that, if the PLLB is set to 0, it will choose another
>> parent which might be inappropriate to generate the requested clock.
>> ITOH, if we authorize forwarding set_rate requests without taking care
>> of constraints imposed by other users of the PLLB we might end-up
>> with a erroneous rate for the USB clock.
>>
>> I thought using set_rate_range() would be a good idea to address this
>> issue, but apparently rate constraints are not propagated to parent
>> clocks, and PLLB is never exposed to devices (it is accessed through
>> clk muxers, divisors, etc).
>>
>> Is there a plan to support propagating rate constraints to parent
>> clocks ?
>
> Thank for the diff below. Your problem needs to be addressed, though I'm
> not sure if propagating rate constraints up the tree is the right way to
> do it.
>
> Looking through the code I notice that we don't return an error code
> from .determine_rate callbacks in clk_calc_new_rates if the min/max
> constraints are out of bounds. We do this for .round_rate since that
> function does not take min/max rate as inputs. Furthermore
> .determine_returns a long, and we don't check for any kind of error
> here. Sigh. See below snippet:
>
>         /* find the closest rate and parent clk/rate */
>         if (clk->ops->determine_rate) {
>                 parent_hw = parent ? parent->hw : NULL;
>                 new_rate = clk->ops->determine_rate(clk->hw, rate,
>                                                     min_rate,
>                                                     max_rate,
>                                                     &best_parent_rate,
>                                                     &parent_hw);
>                 parent = parent_hw ? parent_hw->core : NULL;
>         } else if (clk->ops->round_rate) {
>                 new_rate = clk->ops->round_rate(clk->hw, rate,
>                                                 &best_parent_rate);
>                 if (new_rate < min_rate || new_rate > max_rate)
>                         return NULL;
>
>
> See that for round_rate we bail out if the rate change operation will go
> outside of our bounds? Seems we don't do that for determine_rate. Maybe
> doing so would at least bail gracefully for you case?
>
> Stephen, Tomeu, any thoughts on this? Completely untested diff below:

Sounds good to me.

> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index fa5a00e..482e906 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -1638,6 +1638,8 @@ static struct clk_core *clk_calc_new_rates(struct clk_core *clk,
>                                                     max_rate,
>                                                     &best_parent_rate,
>                                                     &parent_hw);
> +               if (new_rate < min_rate || new_rate > max_rate)
> +                       return NULL;
>                 parent = parent_hw ? parent_hw->core : NULL;
>         } else if (clk->ops->round_rate) {
>                 new_rate = clk->ops->round_rate(clk->hw, rate,
>
>
>
>>
>> Here is an example where propagating rate constraints would be useful:
>>
>> The USB controller wants a 48MHz clock and the audio controller needs a
>> programmable clock running at 12MHz.
>>
>> 1/ The USB driver calls set_rate() on the USB clock, which in turn
>>    configures the PLLB to generate a 96MHz signal and sets its divisor
>>    to 2.
>>    Then it should call set_rate_range(usb_clk, 48Mhz, 48MHz) to prevent
>>    anyone from messing up with his USB clock.
>>
>> 2/ The audio controller calls set_rate on the prog clock, which in
>>    turn configures the PLLB to generate a 12MHz.
>>    Since nobody explicitly set a constraint on the PLLB, the set_rate()
>>    call should work, right ?
>>    If it works, after this call, the USB clk will generate a 6MHz
>>    signal instead of 48MHz one.
>
> I do not think propagating constraints up the tree is the correct way to
> solve this this. If you propagate a min,max constraint of (48,48) or
> (96,96) up to PLLB it may not be the right thing for all cases.
>
> Additionally, to get the right 12MHz rate that your audio clock wants,
> the clock framework will have to become a LOT smarter and end up being
> able to solve for lots cases at run-time (and it will be non-trivial
> run-time complexity to do this solving).
>
> I don't mean to be hand-wavey, so I'll try to explain a bit more. Today
> the clock framework tries to "solve" for complex rate relationships by
> propagating rates up the tree. This is somewhat fragile:
>
> 1) it only considers the rate for the clock that was passed into
> clk_set_rate, as it pre-dates any rate constraint stuff. This is the
> same he-who-writes-last-wins problem that most clock framework
> implementations have suffered.
>
> 2) there is no downwards traversal of the tree to try different
> combinations. If child_clk propagates a rate request up to parent_clk,
> and parent_clk can't do that rate then we bail. There is no logic where
> parent_clk can try to "suggest" a more optimal scenario, or try to
> arbitrate an agreeable solution between multiple downstream child clocks
> which consume parent_clk's rate. I don't think we should strive for this
> level complexity in the kernel, because it is too ... complex.
>
>>
>>
>> I started to look at how rate constraint propagation could be handled:
>> here [1] is a quick&dirty/untested patch adding several things to deal
>> with such cases.
>> The idea is to declare each clock as a clk user of its parent, and then
>> modify rate range appropriately so that all children clk constraints are
>> taken into account by the parent clock when a rate change is requested.
>>
>> Anyway, just tell me if you're already working on a solution for this
>> problem or if you need any help with that ?
>
> I appreciate the feedback on the constraint propagation and the diff you
> provided. Hopefully Stephen or Tomeu can comment further on it. More
> ideas are welcome for the rate constraint stuff in general.
>
> As for your case of needing to support a sane configuration where
> multiple clocks consume the same parent clock, I am slowly working on a
> clock driver-defined look-up table approach for this which may be
> helpful. Think of it as the configuration file that a system integrator
> is responsible for generating for each device to insure that the various
> use cases can have sane clock configuration. No promises on delivery
> dates though :-(

Can you put an example of how such configuration data would look? Just
to make an idea.

Thanks,

Tomeu

> Regards,
> Mike
>
>>
>> Best Regards,
>>
>> Boris
>>
>> [1]http://code.bulix.org/dcm4c8-88137
>>
>> --
>> Boris Brezillon, Free Electrons
>> Embedded Linux and Kernel engineering
>> http://free-electrons.com
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists