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:
dd->clk_ref = of_clk_get(node, 0);
dd->clk_bypass = of_clk_get(node, 1);
Tony, the same problem affects the FAPLL code which copy/pastes some of
the DPLL code.
Thoughts?
Regards,
Mike
* Mike Turquette <[email protected]> [150201 13:27]:
> 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.
Also on at least omap4 like I posted. So sounds like the check for
WARN is wrong but harmless. Paul & Tero, what do you want to do
about that?
> 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:
>
>
> dd->clk_ref = of_clk_get(node, 0);
> dd->clk_bypass = of_clk_get(node, 1);
>
> Tony, the same problem affects the FAPLL code which copy/pastes some of
> the DPLL code.
>
> Thoughts?
Not seeing these warnings with dm186x as fapll.c does not use
dpll3xxx.c. This is because of the way the PLL's child synthesizers
need to also access the PLL registers for power and bypass mode.
Not related to the $subject bug, but to me it seems that we could
possibly have Linux generic PLL code if we add support for
parent_in_bypass_mode in addition to the parent_rate. This is because
the PLL can in theory generate the same rate both in bypass mode and
regular mode so parent_rate is not enough to tell it to the child
synthesizers. Not sure how the PLL registers enabling and disabling
it's children should be handled, maybe regmap would work there.
Regards,
Tony
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 <[email protected]>
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 <[email protected]>
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)
* Tero Kristo <[email protected]> [150202 11:35]:
> On 02/01/2015 11:24 PM, Mike Turquette wrote:
> >Quoting Tomeu Vizoso (2015-01-23 03:03:30)
> >
> >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.
Thanks Tero, looks like your fix fixes all the issues I'm seeing with
commit 59cf3fcf9baf. That is noisy dmesg, dpll_abe_ck not locking
on 4430sdp, and off-idle not working for omap3.
I could not get the patch to apply, below is what I applied manually.
Mike, If possible, maybe fold this into 59cf3fcf9baf? It applies with
some fuzz on that too. And inn that case, please feel also to add the
following for Tomeu's patch:
Tested-by: Tony Lindgren <[email protected]>
8<------------
From: Tero Kristo <[email protected]>
Date: Mon, 2 Feb 2015 12:17:00 -0800
Subject: [PATCH] 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.
Fixes: 59cf3fcf9baf ("clk: Make clk API return per-user struct clk instances")
Signed-off-by: Tero Kristo <[email protected]>
[[email protected]: updated to apply]
Signed-off-by: Tony Lindgren <[email protected]>
--- 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)
On 02/01/15 13:24, 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.
Julia,
Is there a way we can write a coccinelle script to check for this? The
goal being to find all drivers that are comparing struct clk pointers or
attempting to dereference them. There are probably other frameworks that
could use the same type of check (regulator, gpiod, reset, pwm, etc.).
Probably anything that has a get/put API.
-Stephen
> 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:
>
>
> dd->clk_ref = of_clk_get(node, 0);
> dd->clk_bypass = of_clk_get(node, 1);
>
> Tony, the same problem affects the FAPLL code which copy/pastes some of
> the DPLL code.
>
> Thoughts?
>
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
On Mon, 2 Feb 2015, Stephen Boyd wrote:
> On 02/01/15 13:24, 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.
>
> Julia,
>
> Is there a way we can write a coccinelle script to check for this? The
> goal being to find all drivers that are comparing struct clk pointers or
> attempting to dereference them. There are probably other frameworks that
> could use the same type of check (regulator, gpiod, reset, pwm, etc.).
> Probably anything that has a get/put API.
Comparing or dereferencing pointers of a particular type should be
straightforward to check for. Is there an example of how to use the
parent_index value to fix the problem?
julia
>
> -Stephen
>
> > 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:
> >
> >
> > dd->clk_ref = of_clk_get(node, 0);
> > dd->clk_bypass = of_clk_get(node, 1);
> >
> > Tony, the same problem affects the FAPLL code which copy/pastes some of
> > the DPLL code.
> >
> > Thoughts?
> >
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
>
On 02/02/15 13:31, Julia Lawall wrote:
>
> On Mon, 2 Feb 2015, Stephen Boyd wrote:
>
>> Julia,
>>
>> Is there a way we can write a coccinelle script to check for this? The
>> goal being to find all drivers that are comparing struct clk pointers or
>> attempting to dereference them. There are probably other frameworks that
>> could use the same type of check (regulator, gpiod, reset, pwm, etc.).
>> Probably anything that has a get/put API.
> Comparing or dereferencing pointers of a particular type should be
> straightforward to check for. Is there an example of how to use the
> parent_index value to fix the problem?
>
I'm not sure how to fix this case with parent_index values generically.
I imagine it would be highly specific to the surrounding code to the
point where we couldn't fix it automatically. For example, if it's a clk
consumer (not provider like in this case) using an index wouldn't be the
right fix. We would need some sort of clk API that we don't currently
have and I would wonder why clock consumers even care to compare such
pointers in the first place.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
Quoting Tero Kristo (2015-02-02 11:32:01)
> 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.
We can maybe introduce something like of_clk_get_parent_rate, as we have
analogous stuff for getting parent names and indexes. Without
introducing a new helper you could probably just do:
clk_ref = clk_get_parent_by_index(dpll_clk, 0);
ref_rate = clk_get_rate(clk_ref);
clk_bypass = clk_get_parent_by_index(dpll_clk, 1);
bypass_rate = clk_get_rate(clk_bypass);
Currently the semantics around this call are weird. It seems like it
would create a new struct clk pointer but it does not. So don't call
clk_put on clk_ref and clk_bypass yet. That might change in the future
as we iron out this brave new world that we all live in. Probably best
to leave a FIXME in there.
Stephen & Tomeu, let me know if I got any of that wrong.
>
> 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.
Yes, for now that is fine, but feels a bit hacky to me. I don't know
honestly, let me sleep on it. Anyways for 3.20 that is perfectly fine
but we might want to switch to something like the scheme above.
>
> Btw, the rate constraints patch broke boot for me completely, but sounds
> like you reverted it already.
Fixed with Stephen's patch from last week. Thanks for dealing with all
the breakage so promptly. It has helped a lot!
Regards,
Mike
>
> -Tero
>
> --------------------
>
> Author: Tero Kristo <[email protected]>
> 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 <[email protected]>
> 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)
>
>
Quoting Tony Lindgren (2015-02-02 12:44:02)
> * Tero Kristo <[email protected]> [150202 11:35]:
> > On 02/01/2015 11:24 PM, Mike Turquette wrote:
> > >Quoting Tomeu Vizoso (2015-01-23 03:03:30)
> > >
> > >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.
>
> Thanks Tero, looks like your fix fixes all the issues I'm seeing with
> commit 59cf3fcf9baf. That is noisy dmesg, dpll_abe_ck not locking
> on 4430sdp, and off-idle not working for omap3.
>
> I could not get the patch to apply, below is what I applied manually.
>
> Mike, If possible, maybe fold this into 59cf3fcf9baf? It applies with
> some fuzz on that too. And inn that case, please feel also to add the
> following for Tomeu's patch:
>
> Tested-by: Tony Lindgren <[email protected]>
Done and done. Things look good in my testing. I've pushed another
branch out to the mirrors and hopefully the autobuild/autoboot testing
will give us the green light.
This implementation can be revisited probably after 3.19 comes out if
Tero doesn't like using clk_hw directly, or if we provide a better
interface.
Thanks,
Mike
>
> 8<------------
> From: Tero Kristo <[email protected]>
> Date: Mon, 2 Feb 2015 12:17:00 -0800
> Subject: [PATCH] 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.
>
> Fixes: 59cf3fcf9baf ("clk: Make clk API return per-user struct clk instances")
> Signed-off-by: Tero Kristo <[email protected]>
> [[email protected]: updated to apply]
> Signed-off-by: Tony Lindgren <[email protected]>
>
> --- 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)
Quoting Stephen Boyd (2015-02-02 14:35:59)
> On 02/02/15 13:31, Julia Lawall wrote:
> >
> > On Mon, 2 Feb 2015, Stephen Boyd wrote:
> >
> >> Julia,
> >>
> >> Is there a way we can write a coccinelle script to check for this? The
> >> goal being to find all drivers that are comparing struct clk pointers or
> >> attempting to dereference them. There are probably other frameworks that
> >> could use the same type of check (regulator, gpiod, reset, pwm, etc.).
> >> Probably anything that has a get/put API.
> > Comparing or dereferencing pointers of a particular type should be
> > straightforward to check for. Is there an example of how to use the
> > parent_index value to fix the problem?
> >
>
> I'm not sure how to fix this case with parent_index values generically.
> I imagine it would be highly specific to the surrounding code to the
> point where we couldn't fix it automatically. For example, if it's a clk
> consumer (not provider like in this case) using an index wouldn't be the
> right fix. We would need some sort of clk API that we don't currently
> have and I would wonder why clock consumers even care to compare such
> pointers in the first place.
Ack. Is there precedent for a "Don't do that" kind of response?
Regards,
Mike
>
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
On 02/02/15 14:41, Mike Turquette wrote:
> Quoting Tero Kristo (2015-02-02 11:32:01)
>> On 02/01/2015 11:24 PM, Mike Turquette wrote:
>>>
>>> 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.
> We can maybe introduce something like of_clk_get_parent_rate, as we have
> analogous stuff for getting parent names and indexes. Without
> introducing a new helper you could probably just do:
>
> clk_ref = clk_get_parent_by_index(dpll_clk, 0);
> ref_rate = clk_get_rate(clk_ref);
>
> clk_bypass = clk_get_parent_by_index(dpll_clk, 1);
> bypass_rate = clk_get_rate(clk_bypass);
>
> Currently the semantics around this call are weird. It seems like it
> would create a new struct clk pointer but it does not. So don't call
> clk_put on clk_ref and clk_bypass yet. That might change in the future
> as we iron out this brave new world that we all live in. Probably best
> to leave a FIXME in there.
>
> Stephen & Tomeu, let me know if I got any of that wrong.
The plan is to make clk_get_parent_by_index() return a clk_hw pointer
instead of a clk pointer (probably with a new
clk_get_parent_hw_by_index() API). Then drivers that are clk providers
can deal in struct clk_hw and clk consumers can deal in struct clk,
nicely splitting the API between consumers and providers on the
structures they use to interact with the framework.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
* Mike Turquette <[email protected]> [150202 14:51]:
> Quoting Tony Lindgren (2015-02-02 12:44:02)
> >
> > Thanks Tero, looks like your fix fixes all the issues I'm seeing with
> > commit 59cf3fcf9baf. That is noisy dmesg, dpll_abe_ck not locking
> > on 4430sdp, and off-idle not working for omap3.
> >
> > I could not get the patch to apply, below is what I applied manually.
> >
> > Mike, If possible, maybe fold this into 59cf3fcf9baf? It applies with
> > some fuzz on that too. And inn that case, please feel also to add the
> > following for Tomeu's patch:
> >
> > Tested-by: Tony Lindgren <[email protected]>
>
> Done and done. Things look good in my testing. I've pushed another
> branch out to the mirrors and hopefully the autobuild/autoboot testing
> will give us the green light.
Thanks I just checked that your updated branch works for me now.
> This implementation can be revisited probably after 3.19 comes out if
> Tero doesn't like using clk_hw directly, or if we provide a better
> interface.
Sounds like what Tero is saying also relates to knowing if the parent
clock is in bypass mode or not in addition to the parent rate.
Regards,
Tony
On 02/02/2015 11:41 PM, Mike Turquette wrote:
> Quoting Tero Kristo (2015-02-02 11:32:01)
>> 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.
>
> We can maybe introduce something like of_clk_get_parent_rate, as we have
> analogous stuff for getting parent names and indexes. Without
> introducing a new helper you could probably just do:
>
> clk_ref = clk_get_parent_by_index(dpll_clk, 0);
> ref_rate = clk_get_rate(clk_ref);
>
> clk_bypass = clk_get_parent_by_index(dpll_clk, 1);
> bypass_rate = clk_get_rate(clk_bypass);
>
> Currently the semantics around this call are weird. It seems like it
> would create a new struct clk pointer but it does not. So don't call
> clk_put on clk_ref and clk_bypass yet. That might change in the future
> as we iron out this brave new world that we all live in. Probably best
> to leave a FIXME in there.
>
> Stephen & Tomeu, let me know if I got any of that wrong.
I think you got it right, just wanted to mention that we can and
probably should make the clk_get_parent_* calls in the consumer API to
return per-user clk instances but that we need to make sure first that
callers call clk_put afterwards.
This should also allow us to remove the reference to struct clk from
clk_hw, which is at best awkward.
Regards,
Tomeu
>>
>> 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.
>
> Yes, for now that is fine, but feels a bit hacky to me. I don't know
> honestly, let me sleep on it. Anyways for 3.20 that is perfectly fine
> but we might want to switch to something like the scheme above.
>
>>
>> Btw, the rate constraints patch broke boot for me completely, but sounds
>> like you reverted it already.
>
> Fixed with Stephen's patch from last week. Thanks for dealing with all
> the breakage so promptly. It has helped a lot!
>
> Regards,
> Mike
>
>>
>> -Tero
>>
>> --------------------
>>
>> Author: Tero Kristo <[email protected]>
>> 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 <[email protected]>
>> 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)
>>
>>
On 02/03/2015 09:03 AM, Tomeu Vizoso wrote:
> On 02/02/2015 11:41 PM, Mike Turquette wrote:
>> Quoting Tero Kristo (2015-02-02 11:32:01)
>>> 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.
>>
>> We can maybe introduce something like of_clk_get_parent_rate, as we have
>> analogous stuff for getting parent names and indexes. Without
>> introducing a new helper you could probably just do:
>>
>> clk_ref = clk_get_parent_by_index(dpll_clk, 0);
>> ref_rate = clk_get_rate(clk_ref);
>>
>> clk_bypass = clk_get_parent_by_index(dpll_clk, 1);
>> bypass_rate = clk_get_rate(clk_bypass);
>>
>> Currently the semantics around this call are weird. It seems like it
>> would create a new struct clk pointer but it does not. So don't call
>> clk_put on clk_ref and clk_bypass yet. That might change in the future
>> as we iron out this brave new world that we all live in. Probably best
>> to leave a FIXME in there.
>>
>> Stephen & Tomeu, let me know if I got any of that wrong.
>
> I think you got it right, just wanted to mention that we can and
> probably should make the clk_get_parent_* calls in the consumer API to
> return per-user clk instances but that we need to make sure first that
> callers call clk_put afterwards.
>
> This should also allow us to remove the reference to struct clk from
> clk_hw, which is at best awkward.
>
> Regards,
For the DPLL code it should just be fine to be able to get the current
parent index (not parent clock handle), and read a parent clock rate
based on an arbitrary index (not just the current one.) I don't think
there is any other need for having the clk_ref / clk_bypass clock
handles around.
-Tero
>
> Tomeu
>
>>>
>>> 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.
>>
>> Yes, for now that is fine, but feels a bit hacky to me. I don't know
>> honestly, let me sleep on it. Anyways for 3.20 that is perfectly fine
>> but we might want to switch to something like the scheme above.
>>
>>>
>>> Btw, the rate constraints patch broke boot for me completely, but sounds
>>> like you reverted it already.
>>
>> Fixed with Stephen's patch from last week. Thanks for dealing with all
>> the breakage so promptly. It has helped a lot!
>>
>> Regards,
>> Mike
>>
>>>
>>> -Tero
>>>
>>> --------------------
>>>
>>> Author: Tero Kristo <[email protected]>
>>> 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 <[email protected]>
>>> 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)
>>>
>>>
>
* Tero Kristo <[email protected]> [150203 00:49]:
> On 02/03/2015 09:03 AM, Tomeu Vizoso wrote:
> >
> >I think you got it right, just wanted to mention that we can and
> >probably should make the clk_get_parent_* calls in the consumer API to
> >return per-user clk instances but that we need to make sure first that
> >callers call clk_put afterwards.
> >
> >This should also allow us to remove the reference to struct clk from
> >clk_hw, which is at best awkward.
>
> For the DPLL code it should just be fine to be able to get the current
> parent index (not parent clock handle), and read a parent clock rate based
> on an arbitrary index (not just the current one.) I don't think there is any
> other need for having the clk_ref / clk_bypass clock handles around.
I'd like to avoid the situation where the children have know that
certain parent index and parent rate means bypass mode for both
parent and children.
Maybe we can hide that knowledge into some Linux generic PLL code so
the children can get the PLL output as a mux clock. For a PLL, there
can be three mux clock outputs: refclk*multi/div, bypass clock, or no
output.
Regards,
Tony
Hello,
Julia asked me to have a look and see if I can help.
On 02/02/2015 23:50, Mike Turquette wrote:
> Quoting Stephen Boyd (2015-02-02 14:35:59)
>> On 02/02/15 13:31, Julia Lawall wrote:
>>> On Mon, 2 Feb 2015, Stephen Boyd wrote:
>>>
>>>> Julia,
>>>>
>>>> Is there a way we can write a coccinelle script to check for this? The
>>>> goal being to find all drivers that are comparing struct clk pointers or
>>>> attempting to dereference them. There are probably other frameworks that
>>>> could use the same type of check (regulator, gpiod, reset, pwm, etc.).
>>>> Probably anything that has a get/put API.
>>> Comparing or dereferencing pointers of a particular type should be
>>> straightforward to check for. Is there an example of how to use the
>>> parent_index value to fix the problem?
>>>
>> I'm not sure how to fix this case with parent_index values generically.
>> I imagine it would be highly specific to the surrounding code to the
>> point where we couldn't fix it automatically. For example, if it's a clk
>> consumer (not provider like in this case) using an index wouldn't be the
>> right fix. We would need some sort of clk API that we don't currently
>> have and I would wonder why clock consumers even care to compare such
>> pointers in the first place.
> Ack. Is there precedent for a "Don't do that" kind of response?
>
> Regards,
> Mike
>
>> --
>> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
>> a Linux Foundation Collaborative Project
>>
> _______________________________________________
> Cocci mailing list
> [email protected]
> https://systeme.lip6.fr/mailman/listinfo/cocci
I have found these three cases using Coccinnelle in the mach-omap2
directory.
./arch/arm/mach-omap2/clkt_clksel.c
@@ -67,7 +67,6 @@ static const struct clksel *_get_clksel_
return NULL;
for (clks = clk->clksel; clks->parent; clks++)
if (clks->parent == src_clk)
break; /* Found the requested parent */
if (!clks->parent) {
./arch/arm/mach-omap2/dpll3xxx.c
@@ -551,7 +549,6 @@ int omap3_noncore_dpll_set_rate(struct c
if (!dd)
return -EINVAL;
if (__clk_get_parent(hw->clk) != dd->clk_ref)
return -EINVAL;
if (dd->last_rounded_rate == 0)
./arch/arm/mach-omap2/timer.c
@@ -298,7 +298,6 @@ static int __init omap_dm_timer_init_one
if (IS_ERR(src))
return PTR_ERR(src);
if (clk_get_parent(timer->fclk) != src) {
r = clk_set_parent(timer->fclk, src);
if (r < 0) {
pr_warn("%s: %s cannot set source\n", __func__,
Are they instances of your issue?
Do you have any suggestion on how to fix them?
If you want me to I can enlarge the search to other directories.
Quentin
On 02/03/15 08:04, Quentin Lambert wrote:
> Hello,
> Julia asked me to have a look and see if I can help.
>
> I have found these three cases using Coccinnelle in the mach-omap2
> directory.
>
>
>
> ./arch/arm/mach-omap2/clkt_clksel.c
> @@ -67,7 +67,6 @@ static const struct clksel *_get_clksel_
> return NULL;
>
> for (clks = clk->clksel; clks->parent; clks++)
> if (clks->parent == src_clk)
This probably needs to compare hw pointers again given that it's in the
clk-provider code. It would be nice if we could use indices instead though.
> break; /* Found the requested parent */
>
> if (!clks->parent) {
> ./arch/arm/mach-omap2/dpll3xxx.c
> @@ -551,7 +549,6 @@ int omap3_noncore_dpll_set_rate(struct c
> if (!dd)
> return -EINVAL;
>
> if (__clk_get_parent(hw->clk) != dd->clk_ref)
This one is what this thread has started on about. Comparing hw pointers
is ok for now...
> return -EINVAL;
>
> if (dd->last_rounded_rate == 0)
> ./arch/arm/mach-omap2/timer.c
> @@ -298,7 +298,6 @@ static int __init omap_dm_timer_init_one
> if (IS_ERR(src))
> return PTR_ERR(src);
>
> if (clk_get_parent(timer->fclk) != src) {
This one looks like an optimization. We can just always try to set the
parent and if it's already the current parent then the core should bail
out early without an error. So the fix would be to just remove the if
condition.
> r = clk_set_parent(timer->fclk, src);
> if (r < 0) {
> pr_warn("%s: %s cannot set source\n", __func__,
>
>
> Are they instances of your issue?
Yes these all look like instances of the problem.
> If you want me to I can enlarge the search to other directories.
Yes please do. And if you could share the coccinelle patch that would be
great. Thanks.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
On 05/02/2015 00:26, Stephen Boyd wrote:
>> If you want me to I can enlarge the search to other directories.
> Yes please do. And if you could share the coccinelle patch that would be
> great. Thanks.
>
structclk.cocci is the coccinelle patch
structclk-arm.patch is the result I got when applying it to the arch/arm
directory
Is there anything else I can do to help?
Sorry let me do that properly.
On 05/02/2015 16:45, Quentin Lambert wrote:
>
> On 05/02/2015 00:26, Stephen Boyd wrote:
>>> If you want me to I can enlarge the search to other directories.
>> Yes please do. And if you could share the coccinelle patch that would be
>> great. Thanks.
>>
> structclk.cocci is the coccinelle patch
> structclk-arm.patch is the result I got when applying it to the
> arch/arm directory
>
> Is there anything else I can do to help?
>
>
These are the new instances I found by applying the patch to arch/arm
directory:
./arch/arm/mach-imx/mach-imx6q.c
@@ -211,7 +211,6 @@ static void __init imx6q_1588_init(void)
* set bit IOMUXC_GPR1[21]. Or the PTP clock must be from pad
* (external OSC), and we need to clear the bit.
*/
clksel = ptp_clk == enet_ref ? IMX6Q_GPR1_ENET_CLK_SEL_ANATOP :
IMX6Q_GPR1_ENET_CLK_SEL_PAD;
gpr = syscon_regmap_lookup_by_compatible("fsl,imx6q-iomuxc-gpr");
if (!IS_ERR(gpr))
./arch/arm/mach-shmobile/clock-r8a73a4.c
@@ -139,7 +139,6 @@ static int pll_set_parent(struct clk *cl
/* Search the parent */
for (i = 0; i < clk->parent_num; i++)
if (clk->parent_table[i] == parent)
break;
if (i == clk->parent_num)
./arch/arm/mach-shmobile/clock-sh7372.c
@@ -223,7 +223,6 @@ static int pllc2_set_parent(struct clk *
/* Search the parent */
for (i = 0; i < clk->parent_num; i++)
if (clk->parent_table[i] == parent)
break;
if (i == clk->parent_num)
./arch/arm/mach-shmobile/clock-r8a7740.c
@@ -195,7 +195,6 @@ static int usb24s_set_parent(struct clk
/* Search the parent */
for (i = 0; i < clk->parent_num; i++)
if (clk->parent_table[i] == parent)
break;
if (i == clk->parent_num)
On 02/05/15 08:02, Quentin Lambert wrote:
> Sorry let me do that properly.
>
> On 05/02/2015 16:45, Quentin Lambert wrote:
>>
>> On 05/02/2015 00:26, Stephen Boyd wrote:
>>>> If you want me to I can enlarge the search to other directories.
>>> Yes please do. And if you could share the coccinelle patch that
>>> would be
>>> great. Thanks.
>>>
>> structclk.cocci is the coccinelle patch
>> structclk-arm.patch is the result I got when applying it to the
>> arch/arm directory
>>
>> Is there anything else I can do to help?
>>
>>
>
> These are the new instances I found by applying the patch to arch/arm
> directory:
>
> ./arch/arm/mach-imx/mach-imx6q.c
> @@ -211,7 +211,6 @@ static void __init imx6q_1588_init(void)
> * set bit IOMUXC_GPR1[21]. Or the PTP clock must be from pad
> * (external OSC), and we need to clear the bit.
> */
> clksel = ptp_clk == enet_ref ? IMX6Q_GPR1_ENET_CLK_SEL_ANATOP :
> IMX6Q_GPR1_ENET_CLK_SEL_PAD;
> gpr = syscon_regmap_lookup_by_compatible("fsl,imx6q-iomuxc-gpr");
> if (!IS_ERR(gpr))
This one looks like a real problem and it could probably use a
clk_equal(struct clk *a, struct clk *b) function.
>
> ./arch/arm/mach-shmobile/clock-r8a73a4.c
> @@ -139,7 +139,6 @@ static int pll_set_parent(struct clk *cl
>
> /* Search the parent */
> for (i = 0; i < clk->parent_num; i++)
> if (clk->parent_table[i] == parent)
> break;
>
> if (i == clk->parent_num)
>
> ./arch/arm/mach-shmobile/clock-sh7372.c
> @@ -223,7 +223,6 @@ static int pllc2_set_parent(struct clk *
>
> /* Search the parent */
> for (i = 0; i < clk->parent_num; i++)
> if (clk->parent_table[i] == parent)
> break;
>
> if (i == clk->parent_num)
>
> ./arch/arm/mach-shmobile/clock-r8a7740.c
> @@ -195,7 +195,6 @@ static int usb24s_set_parent(struct clk
>
> /* Search the parent */
> for (i = 0; i < clk->parent_num; i++)
> if (clk->parent_table[i] == parent)
> break;
>
> if (i == clk->parent_num)
>
>
>
I don't think shmobile uses the CCF so this should be safe, but we
should probably fix them up to also use a clk_equal() function that just
does pointer comparisons.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
On 02/05/15 07:45, Quentin Lambert wrote:
>
> On 05/02/2015 00:26, Stephen Boyd wrote:
>>> If you want me to I can enlarge the search to other directories.
>> Yes please do. And if you could share the coccinelle patch that would be
>> great. Thanks.
>>
> structclk.cocci is the coccinelle patch
> structclk-arm.patch is the result I got when applying it to the
> arch/arm directory
>
> Is there anything else I can do to help?
>
>
Thanks for the coccinelle patch. Thinking more about it, I don't think
we care if the pointer is dereferenced because that would require a
definition of struct clk and that is most likely not the case outside of
the clock framework. Did you scan the entire kernel? I'm running it now
but it seems to be taking a while.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
On 06/02/2015 03:15, Stephen Boyd wrote:
> Thanks for the coccinelle patch. Thinking more about it, I don't think
> we care if the pointer is dereferenced because that would require a
> definition of struct clk and that is most likely not the case outside of
> the clock framework. Did you scan the entire kernel?
No I haven't.
> I'm running it now
> but it seems to be taking a while.
>
Yes, that's why, as a first step, I chose to limit the scan to the arm
directory.
On Fri, 6 Feb 2015, Quentin Lambert wrote:
>
> On 06/02/2015 03:15, Stephen Boyd wrote:
> > Thanks for the coccinelle patch. Thinking more about it, I don't think
> > we care if the pointer is dereferenced because that would require a
> > definition of struct clk and that is most likely not the case outside of
> > the clock framework. Did you scan the entire kernel?
> No I haven't.
> > I'm running it now
> > but it seems to be taking a while.
> >
> Yes, that's why, as a first step, I chose to limit the scan to the arm
> directory.
Are you sure to be using all of the options provided:
// Options: --recursive-includes --relax-include-path
// Options: --include-headers-for-types
And are you using 1.0.0-rc23 or 1.0.0-rc24? Those should save parsed
header files so that they don't have to be parsed over and over.
If you are using rc24, then you can use the -j option for parallelism.
But then you should also use an option like --chunksize 10 (I don't know
what number would be good), because the default is chunksize 1, and in
that case the saved parsed header files are not reused, because the fies
are all processed individually. In general, it is only the files within a
chunk that will share parsed header files.
julia
On 02/06/15 01:12, Julia Lawall wrote:
>
> On Fri, 6 Feb 2015, Quentin Lambert wrote:
>
>> On 06/02/2015 03:15, Stephen Boyd wrote:
>>> Thanks for the coccinelle patch. Thinking more about it, I don't think
>>> we care if the pointer is dereferenced because that would require a
>>> definition of struct clk and that is most likely not the case outside of
>>> the clock framework. Did you scan the entire kernel?
>> No I haven't.
>>> I'm running it now
>>> but it seems to be taking a while.
>>>
>> Yes, that's why, as a first step, I chose to limit the scan to the arm
>> directory.
> Are you sure to be using all of the options provided:
>
> // Options: --recursive-includes --relax-include-path
> // Options: --include-headers-for-types
>
> And are you using 1.0.0-rc23 or 1.0.0-rc24? Those should save parsed
> header files so that they don't have to be parsed over and over.
>
> If you are using rc24, then you can use the -j option for parallelism.
> But then you should also use an option like --chunksize 10 (I don't know
> what number would be good), because the default is chunksize 1, and in
> that case the saved parsed header files are not reused, because the fies
> are all processed individually. In general, it is only the files within a
> chunk that will share parsed header files.
Thanks for the info.
$ spatch --version
spatch version 1.0.0-rc22 with Python support and with PCRE support
so I guess I need to update. I tried throwing it into
scripts/coccinelle/misc and then did
make O=../obj/ coccicheck
COCCI=../kernel/scripts/coccinelle/misc/structclk.cocci
at the toplevel but it didn't find anything.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
On 02/05/15 18:15, Stephen Boyd wrote:
> On 02/05/15 07:45, Quentin Lambert wrote:
>> On 05/02/2015 00:26, Stephen Boyd wrote:
>>>> If you want me to I can enlarge the search to other directories.
>>> Yes please do. And if you could share the coccinelle patch that would be
>>> great. Thanks.
>>>
>> structclk.cocci is the coccinelle patch
>> structclk-arm.patch is the result I got when applying it to the
>> arch/arm directory
>>
>> Is there anything else I can do to help?
>>
>>
> Thanks for the coccinelle patch. Thinking more about it, I don't think
> we care if the pointer is dereferenced because that would require a
> definition of struct clk and that is most likely not the case outside of
> the clock framework. Did you scan the entire kernel? I'm running it now
> but it seems to be taking a while.
>
I ran the script on all files that include <linux/clk.h>. I've also
trimmed out mips and unicore32 because they're not using the common
clock framework.
diff =
--- arch/arm/mach-imx/mach-imx6q.c
+++ /tmp/cocci-output-11792-b62223-mach-imx6q.c
@@ -211,7 +211,6 @@ static void __init imx6q_1588_init(void)
* set bit IOMUXC_GPR1[21]. Or the PTP clock must be from pad
* (external OSC), and we need to clear the bit.
*/
- clksel = ptp_clk == enet_ref ? IMX6Q_GPR1_ENET_CLK_SEL_ANATOP :
IMX6Q_GPR1_ENET_CLK_SEL_PAD;
gpr = syscon_regmap_lookup_by_compatible("fsl,imx6q-iomuxc-gpr");
if (!IS_ERR(gpr))
diff =
--- drivers/gpu/drm/armada/armada_510.c
+++ /tmp/cocci-output-12321-a5f298-armada_510.c
@@ -53,7 +53,6 @@ static int armada510_crtc_compute_clock(
if (IS_ERR(clk))
return PTR_ERR(clk);
- if (dcrtc->clk != clk) {
ret = clk_prepare_enable(clk);
if (ret)
return ret;
drivers/gpu/drm/armada/armada_510.c:56:5-15: WARNING trying to compare or dereference struct clk pointers.
diff =
--- drivers/pwm/pwm-atmel-hlcdc.c
+++ /tmp/cocci-output-12679-3c5195-pwm-atmel-hlcdc.c
@@ -91,7 +91,6 @@ static int atmel_hlcdc_pwm_config(struct
pwmcfg = ATMEL_HLCDC_PWMPS(pres);
- if (new_clk != chip->cur_clk) {
u32 gencfg = 0;
int ret;
drivers/pwm/pwm-atmel-hlcdc.c:94:5-12: WARNING trying to compare or dereference struct clk pointers.
diff =
--- drivers/tty/serial/samsung.c
+++ /tmp/cocci-output-12827-715e72-samsung.c
@@ -750,7 +750,6 @@ static void s3c24xx_serial_set_termios(s
/* check to see if we need to change clock source */
- if (ourport->baudclk != clk) {
s3c24xx_serial_setsource(port, clk_sel);
if (!IS_ERR(ourport->baudclk)) {
drivers/tty/serial/samsung.c:753:5-21: WARNING trying to compare or dereference struct clk pointers.
diff =
--- sound/soc/fsl/fsl_esai.c
+++ /tmp/cocci-output-13020-d518c3-fsl_esai.c
@@ -269,7 +269,6 @@ static int fsl_esai_set_dai_sysclk(struc
}
/* Only EXTAL source can be output directly without using PSR and PM */
- if (ratio == 1 && clksrc == esai_priv->extalclk) {
/* Bypass all the dividers if not being needed */
ecr |= tx ? ESAI_ECR_ETO : ESAI_ECR_ERO;
goto out;
sound/soc/fsl/fsl_esai.c:272:19-25: WARNING trying to compare or dereference struct clk pointers.
diff =
--- sound/soc/fsl/fsl_spdif.c
+++ /tmp/cocci-output-13024-7acb1d-fsl_spdif.c
@@ -1054,7 +1054,6 @@ static u32 fsl_spdif_txclk_caldiv(struct
enum spdif_txrate index, bool round)
{
const u32 rate[] = { 32000, 44100, 48000, 96000, 192000 };
- bool is_sysclk = clk == spdif_priv->sysclk;
u64 rate_ideal, rate_actual, sub;
u32 sysclk_dfmin, sysclk_dfmax;
u32 txclk_df, sysclk_df, arate;
@@ -1148,7 +1147,6 @@ static int fsl_spdif_probe_txclk(struct
spdif_priv->txclk_src[index], rate[index]);
dev_dbg(&pdev->dev, "use txclk df %d for %dHz sample rate\n",
spdif_priv->txclk_df[index], rate[index]);
- if (spdif_priv->txclk[index] == spdif_priv->sysclk)
dev_dbg(&pdev->dev, "use sysclk df %d for %dHz sample rate\n",
spdif_priv->sysclk_df[index], rate[index]);
dev_dbg(&pdev->dev, "the best rate for %dHz sample rate is %dHz\n",
sound/soc/fsl/fsl_spdif.c:1151:5-29: WARNING trying to compare or dereference struct clk pointers.
sound/soc/fsl/fsl_spdif.c:1057:18-21: WARNING trying to compare or dereference struct clk pointers.
diff =
--- sound/soc/kirkwood/kirkwood-i2s.c
+++ /tmp/cocci-output-13041-3200a6-kirkwood-i2s.c
@@ -579,7 +579,6 @@ static int kirkwood_i2s_dev_probe(struct
if (PTR_ERR(priv->extclk) == -EPROBE_DEFER)
return -EPROBE_DEFER;
} else {
- if (priv->extclk == priv->clk) {
devm_clk_put(&pdev->dev, priv->extclk);
priv->extclk = ERR_PTR(-EINVAL);
} else {
sound/soc/kirkwood/kirkwood-i2s.c:582:6-18: WARNING trying to compare or dereference struct clk pointers.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project