On 04/15/2015 12:06 AM, Michael Turquette wrote:
Quoting Mikko Perttunen (2015-04-14 12:40:36)
On 04/14/2015 08:21 PM, Boris Brezillon wrote:
Hi Mikko,
On Tue, 14 Apr 2015 14:25:59 +0300
Mikko Perttunen <mikko.perttunen@xxxxxxxx> wrote:
On 04/11/2015 12:11 AM, Michael Turquette wrote:
Quoting Thierry Reding (2015-03-11 03:07:43)
Hi Mike,
Have you had a chance to look at these changes to the Tegra clock
driver? If you're fine with it, I'd like to take these patches through
the Tegra tree because the rest of the series depends on them. I can
provide a stable branch in case we need to base other Tegra clock
changes on top of this.
Hi Thierry,
Clock patches (and corresponding DT binding descriptions and changes to
DTS) look fine to me. Please add:
Acked-by: Michael Turquette <mturquette@xxxxxxxxxx>
I did have a question about the beahvior of clk_put in one of Mikko's
patches but it should not gate this series. I'm just trying to find out
if we have a bug in the framework or if the Tegra driver is a special
case.
Also I do not think a stable branch is necessary.
Regards,
Mike
Looks like in the meantime, this has been partially broken by
03bc10ab5b0f "clk: check ->determine/round_rate() return value in
clk_calc_new_rates". The highest rates supported by the DFLL clock have
1 in the MSB, so those cannot be entered after the aforementioned patch,
as the return value of round_rate is interpreted as an error. Avenues
that I can see: 1) revert the above patch 2) restrict the cpu clock rate
to those with 0 in the MSB 3) move to 64-bit clock rates.
How about changing ->determine_rate() and ->round_rate() prototypes so
that they always return 0 or an error code and passing the adjusted_rate
as an argument ?
Something like that:
int (*round_rate)(struct clk_hw *hw, unsigned long *rate,
unsigned long *parent_rate);
int (*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_hw);
I know this implies a lot of changes (in all clock drivers and in the
core infrastructure), but I really think we should not mix error codes
and clock frequencies (even if we decide to move to a 64 bits rate
approach).
This sounds like a good idea, too.
I've had this idea as well, which is to never return rates but only
error codes, and rates are passed by reference like in your example
above. Clearly the *best_parent_rate stuff already functions this way.
Would be cool to use a programming language that supported complex
return types ;-)
Algebraic data type pipe dreams.. :)
Anyway, IMHO the only alternative to this solution is solution #3,
because #1 implies re-introducing another bug where
->round_rate()/->determine_rate() are silently ignored, and #2 implies
lying about your DFLL capabilities.
Yeah, #1 and #2 weren't really meant as realistic options to end up with :)
Yes, seems that we're heading towards #3. In the mean time option #1.5
(the one where we change the round_rate/determine_rate semantics) is
probably a good idea and can resolve this issue in the shorter term
compared to signed 64-bit rates (and will be necessary anyhow if we use
unsigned 64-bit rates).
I'll add this to the high priority todo list since the Tegra EMC stuff
won't go for 4.1 but will very likely go for 4.2.
Wonderful :)
Thanks,
Mikko
Regards,
Mike
Mike, what's your opinion ?
Best Regards,
Boris
Cheers,
Mikko.
--
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
