From: Tero Kristo <t-kristo@ti.com>
To: Mike Turquette <mturquette@linaro.org>,
Tomeu Vizoso <tomeu.vizoso@collabora.com>,
<linux-kernel@vger.kernel.org>,
Stephen Boyd <sboyd@codeaurora.org>
Cc: Paul Walmsley <paul@pwsan.com>, Tony Lindgren <tony@atomide.com>,
<linux-omap@vger.kernel.org>,
<linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
Date: Mon, 2 Feb 2015 21:32:01 +0200 [thread overview]
Message-ID: <54CFD0B1.2000003@ti.com> (raw)
In-Reply-To: <20150201212432.22722.70917@quantum>
On 02/01/2015 11:24 PM, Mike Turquette wrote:
> Quoting Tomeu Vizoso (2015-01-23 03:03:30)
>> Moves clock state to struct clk_core, but takes care to change as little API as
>> possible.
>>
>> struct clk_hw still has a pointer to a struct clk, which is the
>> implementation's per-user clk instance, for backwards compatibility.
>>
>> The struct clk that clk_get_parent() returns isn't owned by the caller, but by
>> the clock implementation, so the former shouldn't call clk_put() on it.
>>
>> Because some boards in mach-omap2 still register clocks statically, their clock
>> registration had to be updated to take into account that the clock information
>> is stored in struct clk_core now.
>
> Tero, Paul & Tony,
>
> Tomeu's patch unveils a problem with omap3_noncore_dpll_enable and
> struct dpll_data, namely this snippet from
> arch/arm/mach-omap2/dpll3xxx.c:
>
> parent = __clk_get_parent(hw->clk);
>
> if (__clk_get_rate(hw->clk) == __clk_get_rate(dd->clk_bypass)) {
> WARN(parent != dd->clk_bypass,
> "here0, parent name is %s, bypass name is %s\n",
> __clk_get_name(parent), __clk_get_name(dd->clk_bypass));
> r = _omap3_noncore_dpll_bypass(clk);
> } else {
> WARN(parent != dd->clk_ref,
> "here1, parent name is %s, ref name is %s\n",
> __clk_get_name(parent), __clk_get_name(dd->clk_ref));
> r = _omap3_noncore_dpll_lock(clk);
> }
>
> struct dpll_data has members clk_ref and clk_bypass which are struct clk
> pointers. This was always a bit of a violation of the clk.h contract
> since drivers are not supposed to deref struct clk pointers. Now that we
> generate unique pointers for each call to clk_get (clk_ref & clk_bypass
> are populated by of_clk_get in ti_clk_register_dpll) then the pointer
> comparisons above will never be equal (even if they resolve down to the
> same struct clk_core). I added the verbose traces to the WARNs above to
> illustrate the point: the names are always the same but the pointers
> differ.
>
> AFAICT this doesn't break anything, but booting on OMAP3+ results in
> noisy WARNs.
>
> I think the correct fix is to replace clk_bypass and clk_ref pointers
> with a simple integer parent_index. In fact we already have this index.
> See how the pointers are populated in ti_clk_register_dpll:
The problem is we still need to be able to get runtime parent clock
rates (the parent rate may change also), so simple index value is not
sufficient. We need a handle of some sort to the bypass/ref clocks. The
DPLL code generally requires knowledge of the bypass + reference clock
rates to work properly, as it calculates the M/N values based on these.
Shall I change the DPLL code to check against clk_hw pointers or what is
the preferred approach here? The patch at the end does this and fixes
the dpll related warnings.
Btw, the rate constraints patch broke boot for me completely, but sounds
like you reverted it already.
-Tero
--------------------
Author: Tero Kristo <t-kristo@ti.com>
Date: Mon Feb 2 17:19:17 2015 +0200
ARM: OMAP3+: clock: dpll: fix logic for comparing parent clocks
DPLL code uses reference and bypass clock pointers for determining
runtime
properties for these clocks, like parent clock rates.
As clock API now returns per-user clock structs, using a global handle
in the clock driver code does not work properly anymore. Fix this by
using the clk_hw instead, and comparing this against the parents.
Signed-off-by: Tero Kristo <t-kristo@ti.com>
Fixes: 59cf3fcf9baf ("clk: Make clk API return per-user struct clk
instances")
diff --git a/arch/arm/mach-omap2/dpll3xxx.c b/arch/arm/mach-omap2/dpll3xxx.c
index c2da2a0..49752d7 100644
--- a/arch/arm/mach-omap2/dpll3xxx.c
+++ b/arch/arm/mach-omap2/dpll3xxx.c
@@ -410,7 +410,7 @@ int omap3_noncore_dpll_enable(struct clk_hw *hw)
struct clk_hw_omap *clk = to_clk_hw_omap(hw);
int r;
struct dpll_data *dd;
- struct clk *parent;
+ struct clk_hw *parent;
dd = clk->dpll_data;
if (!dd)
@@ -427,13 +427,13 @@ int omap3_noncore_dpll_enable(struct clk_hw *hw)
}
}
- parent = __clk_get_parent(hw->clk);
+ parent = __clk_get_hw(__clk_get_parent(hw->clk));
if (__clk_get_rate(hw->clk) == __clk_get_rate(dd->clk_bypass)) {
- WARN_ON(parent != dd->clk_bypass);
+ WARN_ON(parent != __clk_get_hw(dd->clk_bypass));
r = _omap3_noncore_dpll_bypass(clk);
} else {
- WARN_ON(parent != dd->clk_ref);
+ WARN_ON(parent != __clk_get_hw(dd->clk_ref));
r = _omap3_noncore_dpll_lock(clk);
}
@@ -549,7 +549,8 @@ int omap3_noncore_dpll_set_rate(struct clk_hw *hw,
unsigned long rate,
if (!dd)
return -EINVAL;
- if (__clk_get_parent(hw->clk) != dd->clk_ref)
+ if (__clk_get_hw(__clk_get_parent(hw->clk)) !=
+ __clk_get_hw(dd->clk_ref))
return -EINVAL;
if (dd->last_rounded_rate == 0)
next prev parent reply other threads:[~2015-02-02 19:32 UTC|newest]
Thread overview: 68+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-01-23 11:03 [PATCH v13 0/6] Per-user clock constraints Tomeu Vizoso
2015-01-23 11:03 ` [PATCH v13 1/6] clk: Remove unneeded NULL checks Tomeu Vizoso
2015-01-23 11:03 ` [PATCH v13 2/6] clk: Remove __clk_register Tomeu Vizoso
2015-01-23 11:03 ` [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances Tomeu Vizoso
2015-02-01 21:24 ` Mike Turquette
2015-02-02 17:04 ` Tony Lindgren
2015-02-02 19:32 ` Tero Kristo [this message]
2015-02-02 20:44 ` Tony Lindgren
2015-02-02 22:48 ` Mike Turquette
2015-02-02 23:11 ` Tony Lindgren
2015-02-02 22:41 ` Mike Turquette
2015-02-02 22:52 ` Stephen Boyd
2015-02-03 7:03 ` Tomeu Vizoso
2015-02-03 8:46 ` Tero Kristo
2015-02-03 15:22 ` Tony Lindgren
2015-02-02 20:45 ` Stephen Boyd
2015-02-02 21:31 ` Julia Lawall
2015-02-02 22:35 ` Stephen Boyd
2015-02-02 22:50 ` Mike Turquette
2015-02-03 16:04 ` [Cocci] " Quentin Lambert
2015-02-04 23:26 ` Stephen Boyd
2015-02-05 15:45 ` Quentin Lambert
2015-02-05 16:02 ` Quentin Lambert
2015-02-06 1:49 ` Stephen Boyd
2015-02-06 2:15 ` Stephen Boyd
2015-02-06 9:01 ` Quentin Lambert
2015-02-06 9:12 ` Julia Lawall
2015-02-06 17:15 ` Stephen Boyd
2015-02-17 22:01 ` Stephen Boyd
2015-03-12 17:20 ` Sebastian Andrzej Siewior
2015-03-12 19:43 ` Stephen Boyd
2015-03-13 3:29 ` Shawn Guo
2015-03-13 8:20 ` Sebastian Andrzej Siewior
2015-03-13 13:42 ` Shawn Guo
2015-03-13 17:42 ` Stephen Boyd
2015-02-05 19:44 ` Sylwester Nawrocki
2015-02-05 20:06 ` Sylwester Nawrocki
2015-02-05 20:07 ` Stephen Boyd
2015-02-05 22:14 ` Stephen Boyd
2015-02-06 0:42 ` Russell King - ARM Linux
2015-02-06 1:35 ` Stephen Boyd
2015-02-06 13:39 ` Russell King - ARM Linux
2015-02-06 19:30 ` Stephen Boyd
2015-02-06 19:37 ` Russell King - ARM Linux
2015-02-06 19:41 ` Stephen Boyd
2015-02-19 21:32 ` Mike Turquette
2015-02-24 14:08 ` Russell King - ARM Linux
2015-01-23 11:03 ` [PATCH v13 4/6] clk: Add rate constraints to clocks Tomeu Vizoso
2015-01-29 13:31 ` Geert Uytterhoeven
2015-01-29 19:13 ` Stephen Boyd
2015-01-31 1:31 ` Stephen Boyd
2015-01-31 18:36 ` Tomeu Vizoso
2015-02-01 22:18 ` Mike Turquette
2015-02-02 7:59 ` Geert Uytterhoeven
2015-02-02 16:12 ` Tony Lindgren
2015-02-02 17:46 ` Mike Turquette
2015-02-02 17:49 ` Russell King - ARM Linux
2015-02-02 19:21 ` Tony Lindgren
2015-02-02 20:47 ` Tony Lindgren
2015-01-23 11:03 ` [PATCH v13 5/6] clkdev: Export clk_register_clkdev Tomeu Vizoso
2015-02-03 17:35 ` Andy Shevchenko
2015-02-03 17:43 ` Andy Shevchenko
2015-01-23 11:03 ` [PATCH v13 6/6] clk: Add module for unit tests Tomeu Vizoso
2015-01-27 0:55 ` [PATCH v13 0/6] Per-user clock constraints Stephen Boyd
2015-01-27 6:29 ` Tomeu Vizoso
2015-01-28 6:59 ` Tomeu Vizoso
[not found] ` <20150129022633.22722.78592@quantum>
2015-01-29 6:41 ` Tomeu Vizoso
2015-01-29 14:29 ` Mike Turquette
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=54CFD0B1.2000003@ti.com \
--to=t-kristo@ti.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-omap@vger.kernel.org \
--cc=mturquette@linaro.org \
--cc=paul@pwsan.com \
--cc=sboyd@codeaurora.org \
--cc=tomeu.vizoso@collabora.com \
--cc=tony@atomide.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).