* [PATCH] ARM: OMAP: dpll: enable bypass clock only when attempting dpll bypass
@ 2013-03-28 10:59 Rajendra Nayak
2013-03-28 16:32 ` Mike Turquette
2013-04-01 10:27 ` Paul Walmsley
0 siblings, 2 replies; 5+ messages in thread
From: Rajendra Nayak @ 2013-03-28 10:59 UTC (permalink / raw)
To: linux-arm-kernel
omap3_noncore_dpll_set_rate() attempts an enable of bypass clk as well
as ref clk for every .set_rate attempt on a noncore DPLL, regardless of
whether the .set_rate results in the DPLL being locked or put in bypass.
Early at boot, while some of these DPLLs are programmed and locked
(using .set_rate for the DPLL), this causes an ordering issue.
For instance, on OMAP5, the USB DPLL derives its bypass clk from ABE DPLL.
If a .set_rate of USB DPLL which programmes the M,N and locks it is called
before the one for ABE, the enable of USB bypass clk (derived from ABE DPLL)
then attempts to lock the ABE DPLL and fails as the M,N values for ABE
are yet to be programmed.
To get rid of this ordering needs, enable bypass clk for a DPLL as part
of its .set_rate only when its being put in bypass, and only enable the
ref clk when its locked.
Reported-by: Roger Quadros <rogerq@ti.com>
Signed-off-by: Rajendra Nayak <rnayak@ti.com>
---
arch/arm/mach-omap2/dpll3xxx.c | 19 +++++++++----------
1 file changed, 9 insertions(+), 10 deletions(-)
diff --git a/arch/arm/mach-omap2/dpll3xxx.c b/arch/arm/mach-omap2/dpll3xxx.c
index 3aed4b0..6e9873f 100644
--- a/arch/arm/mach-omap2/dpll3xxx.c
+++ b/arch/arm/mach-omap2/dpll3xxx.c
@@ -480,20 +480,22 @@ int omap3_noncore_dpll_set_rate(struct clk_hw *hw, unsigned long rate,
if (!dd)
return -EINVAL;
- __clk_prepare(dd->clk_bypass);
- clk_enable(dd->clk_bypass);
- __clk_prepare(dd->clk_ref);
- clk_enable(dd->clk_ref);
-
if (__clk_get_rate(dd->clk_bypass) == rate &&
(dd->modes & (1 << DPLL_LOW_POWER_BYPASS))) {
pr_debug("%s: %s: set rate: entering bypass.\n",
__func__, __clk_get_name(hw->clk));
+ __clk_prepare(dd->clk_bypass);
+ clk_enable(dd->clk_bypass);
ret = _omap3_noncore_dpll_bypass(clk);
if (!ret)
new_parent = dd->clk_bypass;
+ clk_disable(dd->clk_bypass);
+ __clk_unprepare(dd->clk_bypass);
} else {
+ __clk_prepare(dd->clk_ref);
+ clk_enable(dd->clk_ref);
+
if (dd->last_rounded_rate != rate)
rate = __clk_round_rate(hw->clk, rate);
@@ -514,6 +516,8 @@ int omap3_noncore_dpll_set_rate(struct clk_hw *hw, unsigned long rate,
ret = omap3_noncore_dpll_program(clk, freqsel);
if (!ret)
new_parent = dd->clk_ref;
+ clk_disable(dd->clk_ref);
+ __clk_unprepare(dd->clk_ref);
}
/*
* FIXME - this is all wrong. common code handles reparenting and
@@ -525,11 +529,6 @@ int omap3_noncore_dpll_set_rate(struct clk_hw *hw, unsigned long rate,
if (!ret)
__clk_reparent(hw->clk, new_parent);
- clk_disable(dd->clk_ref);
- __clk_unprepare(dd->clk_ref);
- clk_disable(dd->clk_bypass);
- __clk_unprepare(dd->clk_bypass);
-
return 0;
}
--
1.7.9.5
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH] ARM: OMAP: dpll: enable bypass clock only when attempting dpll bypass
2013-03-28 10:59 [PATCH] ARM: OMAP: dpll: enable bypass clock only when attempting dpll bypass Rajendra Nayak
@ 2013-03-28 16:32 ` Mike Turquette
2013-03-29 5:26 ` Rajendra Nayak
2013-04-01 10:27 ` Paul Walmsley
1 sibling, 1 reply; 5+ messages in thread
From: Mike Turquette @ 2013-03-28 16:32 UTC (permalink / raw)
To: linux-arm-kernel
Quoting Rajendra Nayak (2013-03-28 03:59:41)
> omap3_noncore_dpll_set_rate() attempts an enable of bypass clk as well
> as ref clk for every .set_rate attempt on a noncore DPLL, regardless of
> whether the .set_rate results in the DPLL being locked or put in bypass.
> Early at boot, while some of these DPLLs are programmed and locked
> (using .set_rate for the DPLL), this causes an ordering issue.
>
> For instance, on OMAP5, the USB DPLL derives its bypass clk from ABE DPLL.
> If a .set_rate of USB DPLL which programmes the M,N and locks it is called
> before the one for ABE, the enable of USB bypass clk (derived from ABE DPLL)
> then attempts to lock the ABE DPLL and fails as the M,N values for ABE
> are yet to be programmed.
>
> To get rid of this ordering needs, enable bypass clk for a DPLL as part
> of its .set_rate only when its being put in bypass, and only enable the
> ref clk when its locked.
>
Hi Rajendra,
Another way to solve this problem would be to model the DPLLs as mux
clocks with a list of possible parents (e.g. clk->parents[2]). Then set
the CLK_SET_RATE_PARENT flag on the USB DPLL which will allow to
propagate the rate request up to the ABE DPLL. This should set the MN
dividers appropriately.
> Reported-by: Roger Quadros <rogerq@ti.com>
> Signed-off-by: Rajendra Nayak <rnayak@ti.com>
> ---
> arch/arm/mach-omap2/dpll3xxx.c | 19 +++++++++----------
> 1 file changed, 9 insertions(+), 10 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/dpll3xxx.c b/arch/arm/mach-omap2/dpll3xxx.c
> index 3aed4b0..6e9873f 100644
> --- a/arch/arm/mach-omap2/dpll3xxx.c
> +++ b/arch/arm/mach-omap2/dpll3xxx.c
> @@ -480,20 +480,22 @@ int omap3_noncore_dpll_set_rate(struct clk_hw *hw, unsigned long rate,
> if (!dd)
> return -EINVAL;
>
> - __clk_prepare(dd->clk_bypass);
> - clk_enable(dd->clk_bypass);
> - __clk_prepare(dd->clk_ref);
> - clk_enable(dd->clk_ref);
Is this safe? I always thought that the programming sequence in the TRM
was to enable both the ref and bypass clocks during DPLL reprogramming.
Maybe I am misremembering or assuming that the code strictly followed
the TRM.
Regards,
Mike
> -
> if (__clk_get_rate(dd->clk_bypass) == rate &&
> (dd->modes & (1 << DPLL_LOW_POWER_BYPASS))) {
> pr_debug("%s: %s: set rate: entering bypass.\n",
> __func__, __clk_get_name(hw->clk));
>
> + __clk_prepare(dd->clk_bypass);
> + clk_enable(dd->clk_bypass);
> ret = _omap3_noncore_dpll_bypass(clk);
> if (!ret)
> new_parent = dd->clk_bypass;
> + clk_disable(dd->clk_bypass);
> + __clk_unprepare(dd->clk_bypass);
> } else {
> + __clk_prepare(dd->clk_ref);
> + clk_enable(dd->clk_ref);
> +
> if (dd->last_rounded_rate != rate)
> rate = __clk_round_rate(hw->clk, rate);
>
> @@ -514,6 +516,8 @@ int omap3_noncore_dpll_set_rate(struct clk_hw *hw, unsigned long rate,
> ret = omap3_noncore_dpll_program(clk, freqsel);
> if (!ret)
> new_parent = dd->clk_ref;
> + clk_disable(dd->clk_ref);
> + __clk_unprepare(dd->clk_ref);
> }
> /*
> * FIXME - this is all wrong. common code handles reparenting and
> @@ -525,11 +529,6 @@ int omap3_noncore_dpll_set_rate(struct clk_hw *hw, unsigned long rate,
> if (!ret)
> __clk_reparent(hw->clk, new_parent);
>
> - clk_disable(dd->clk_ref);
> - __clk_unprepare(dd->clk_ref);
> - clk_disable(dd->clk_bypass);
> - __clk_unprepare(dd->clk_bypass);
> -
> return 0;
> }
>
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 5+ messages in thread* [PATCH] ARM: OMAP: dpll: enable bypass clock only when attempting dpll bypass
2013-03-28 16:32 ` Mike Turquette
@ 2013-03-29 5:26 ` Rajendra Nayak
2013-03-29 17:24 ` Mike Turquette
0 siblings, 1 reply; 5+ messages in thread
From: Rajendra Nayak @ 2013-03-29 5:26 UTC (permalink / raw)
To: linux-arm-kernel
On Thursday 28 March 2013 10:02 PM, Mike Turquette wrote:
> Quoting Rajendra Nayak (2013-03-28 03:59:41)
>> omap3_noncore_dpll_set_rate() attempts an enable of bypass clk as well
>> as ref clk for every .set_rate attempt on a noncore DPLL, regardless of
>> whether the .set_rate results in the DPLL being locked or put in bypass.
>> Early at boot, while some of these DPLLs are programmed and locked
>> (using .set_rate for the DPLL), this causes an ordering issue.
>>
>> For instance, on OMAP5, the USB DPLL derives its bypass clk from ABE DPLL.
>> If a .set_rate of USB DPLL which programmes the M,N and locks it is called
>> before the one for ABE, the enable of USB bypass clk (derived from ABE DPLL)
>> then attempts to lock the ABE DPLL and fails as the M,N values for ABE
>> are yet to be programmed.
>>
>> To get rid of this ordering needs, enable bypass clk for a DPLL as part
>> of its .set_rate only when its being put in bypass, and only enable the
>> ref clk when its locked.
>>
>
> Hi Rajendra,
>
> Another way to solve this problem would be to model the DPLLs as mux
> clocks with a list of possible parents (e.g. clk->parents[2]). Then set
> the CLK_SET_RATE_PARENT flag on the USB DPLL which will allow to
But isn't this applicable only to the _current_ parent? The bypass_clk isn't
the parent of DPLL USB when I try a .set_rate on it. Its the ref_clk.
> propagate the rate request up to the ABE DPLL. This should set the MN
> dividers appropriately.
>
>> Reported-by: Roger Quadros <rogerq@ti.com>
>> Signed-off-by: Rajendra Nayak <rnayak@ti.com>
>> ---
>> arch/arm/mach-omap2/dpll3xxx.c | 19 +++++++++----------
>> 1 file changed, 9 insertions(+), 10 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap2/dpll3xxx.c b/arch/arm/mach-omap2/dpll3xxx.c
>> index 3aed4b0..6e9873f 100644
>> --- a/arch/arm/mach-omap2/dpll3xxx.c
>> +++ b/arch/arm/mach-omap2/dpll3xxx.c
>> @@ -480,20 +480,22 @@ int omap3_noncore_dpll_set_rate(struct clk_hw *hw, unsigned long rate,
>> if (!dd)
>> return -EINVAL;
>>
>> - __clk_prepare(dd->clk_bypass);
>> - clk_enable(dd->clk_bypass);
>> - __clk_prepare(dd->clk_ref);
>> - clk_enable(dd->clk_ref);
>
> Is this safe? I always thought that the programming sequence in the TRM
> was to enable both the ref and bypass clocks during DPLL reprogramming.
> Maybe I am misremembering or assuming that the code strictly followed
> the TRM.
hmm, I didn't bother to check either. Will check, it would be strange though
if such a sequence is indeed needed.
regards,
Rajendra
>
> Regards,
> Mike
>
>> -
>> if (__clk_get_rate(dd->clk_bypass) == rate &&
>> (dd->modes & (1 << DPLL_LOW_POWER_BYPASS))) {
>> pr_debug("%s: %s: set rate: entering bypass.\n",
>> __func__, __clk_get_name(hw->clk));
>>
>> + __clk_prepare(dd->clk_bypass);
>> + clk_enable(dd->clk_bypass);
>> ret = _omap3_noncore_dpll_bypass(clk);
>> if (!ret)
>> new_parent = dd->clk_bypass;
>> + clk_disable(dd->clk_bypass);
>> + __clk_unprepare(dd->clk_bypass);
>> } else {
>> + __clk_prepare(dd->clk_ref);
>> + clk_enable(dd->clk_ref);
>> +
>> if (dd->last_rounded_rate != rate)
>> rate = __clk_round_rate(hw->clk, rate);
>>
>> @@ -514,6 +516,8 @@ int omap3_noncore_dpll_set_rate(struct clk_hw *hw, unsigned long rate,
>> ret = omap3_noncore_dpll_program(clk, freqsel);
>> if (!ret)
>> new_parent = dd->clk_ref;
>> + clk_disable(dd->clk_ref);
>> + __clk_unprepare(dd->clk_ref);
>> }
>> /*
>> * FIXME - this is all wrong. common code handles reparenting and
>> @@ -525,11 +529,6 @@ int omap3_noncore_dpll_set_rate(struct clk_hw *hw, unsigned long rate,
>> if (!ret)
>> __clk_reparent(hw->clk, new_parent);
>>
>> - clk_disable(dd->clk_ref);
>> - __clk_unprepare(dd->clk_ref);
>> - clk_disable(dd->clk_bypass);
>> - __clk_unprepare(dd->clk_bypass);
>> -
>> return 0;
>> }
>>
>> --
>> 1.7.9.5
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
>> the body of a message to majordomo at vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 5+ messages in thread* [PATCH] ARM: OMAP: dpll: enable bypass clock only when attempting dpll bypass
2013-03-29 5:26 ` Rajendra Nayak
@ 2013-03-29 17:24 ` Mike Turquette
0 siblings, 0 replies; 5+ messages in thread
From: Mike Turquette @ 2013-03-29 17:24 UTC (permalink / raw)
To: linux-arm-kernel
Quoting Rajendra Nayak (2013-03-28 22:26:05)
> On Thursday 28 March 2013 10:02 PM, Mike Turquette wrote:
> > Quoting Rajendra Nayak (2013-03-28 03:59:41)
> >> omap3_noncore_dpll_set_rate() attempts an enable of bypass clk as well
> >> as ref clk for every .set_rate attempt on a noncore DPLL, regardless of
> >> whether the .set_rate results in the DPLL being locked or put in bypass.
> >> Early at boot, while some of these DPLLs are programmed and locked
> >> (using .set_rate for the DPLL), this causes an ordering issue.
> >>
> >> For instance, on OMAP5, the USB DPLL derives its bypass clk from ABE DPLL.
> >> If a .set_rate of USB DPLL which programmes the M,N and locks it is called
> >> before the one for ABE, the enable of USB bypass clk (derived from ABE DPLL)
> >> then attempts to lock the ABE DPLL and fails as the M,N values for ABE
> >> are yet to be programmed.
> >>
> >> To get rid of this ordering needs, enable bypass clk for a DPLL as part
> >> of its .set_rate only when its being put in bypass, and only enable the
> >> ref clk when its locked.
> >>
> >
> > Hi Rajendra,
> >
> > Another way to solve this problem would be to model the DPLLs as mux
> > clocks with a list of possible parents (e.g. clk->parents[2]). Then set
> > the CLK_SET_RATE_PARENT flag on the USB DPLL which will allow to
>
> But isn't this applicable only to the _current_ parent? The bypass_clk isn't
> the parent of DPLL USB when I try a .set_rate on it. Its the ref_clk.
>
Oops. Yeah, I got that one mixed up.
Another interesting idea is for the .prepare of a DPLL to check to see
if M/N values have been populated or not, and if they are not populated
then to put some known-good defaults in there.
But this is just kicking around ideas really and I'm not nacking this
patch based on the above idea.
Regards,
Mike
> > propagate the rate request up to the ABE DPLL. This should set the MN
> > dividers appropriately.
> >
> >> Reported-by: Roger Quadros <rogerq@ti.com>
> >> Signed-off-by: Rajendra Nayak <rnayak@ti.com>
> >> ---
> >> arch/arm/mach-omap2/dpll3xxx.c | 19 +++++++++----------
> >> 1 file changed, 9 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/arch/arm/mach-omap2/dpll3xxx.c b/arch/arm/mach-omap2/dpll3xxx.c
> >> index 3aed4b0..6e9873f 100644
> >> --- a/arch/arm/mach-omap2/dpll3xxx.c
> >> +++ b/arch/arm/mach-omap2/dpll3xxx.c
> >> @@ -480,20 +480,22 @@ int omap3_noncore_dpll_set_rate(struct clk_hw *hw, unsigned long rate,
> >> if (!dd)
> >> return -EINVAL;
> >>
> >> - __clk_prepare(dd->clk_bypass);
> >> - clk_enable(dd->clk_bypass);
> >> - __clk_prepare(dd->clk_ref);
> >> - clk_enable(dd->clk_ref);
> >
> > Is this safe? I always thought that the programming sequence in the TRM
> > was to enable both the ref and bypass clocks during DPLL reprogramming.
> > Maybe I am misremembering or assuming that the code strictly followed
> > the TRM.
>
> hmm, I didn't bother to check either. Will check, it would be strange though
> if such a sequence is indeed needed.
>
> regards,
> Rajendra
>
> >
> > Regards,
> > Mike
> >
> >> -
> >> if (__clk_get_rate(dd->clk_bypass) == rate &&
> >> (dd->modes & (1 << DPLL_LOW_POWER_BYPASS))) {
> >> pr_debug("%s: %s: set rate: entering bypass.\n",
> >> __func__, __clk_get_name(hw->clk));
> >>
> >> + __clk_prepare(dd->clk_bypass);
> >> + clk_enable(dd->clk_bypass);
> >> ret = _omap3_noncore_dpll_bypass(clk);
> >> if (!ret)
> >> new_parent = dd->clk_bypass;
> >> + clk_disable(dd->clk_bypass);
> >> + __clk_unprepare(dd->clk_bypass);
> >> } else {
> >> + __clk_prepare(dd->clk_ref);
> >> + clk_enable(dd->clk_ref);
> >> +
> >> if (dd->last_rounded_rate != rate)
> >> rate = __clk_round_rate(hw->clk, rate);
> >>
> >> @@ -514,6 +516,8 @@ int omap3_noncore_dpll_set_rate(struct clk_hw *hw, unsigned long rate,
> >> ret = omap3_noncore_dpll_program(clk, freqsel);
> >> if (!ret)
> >> new_parent = dd->clk_ref;
> >> + clk_disable(dd->clk_ref);
> >> + __clk_unprepare(dd->clk_ref);
> >> }
> >> /*
> >> * FIXME - this is all wrong. common code handles reparenting and
> >> @@ -525,11 +529,6 @@ int omap3_noncore_dpll_set_rate(struct clk_hw *hw, unsigned long rate,
> >> if (!ret)
> >> __clk_reparent(hw->clk, new_parent);
> >>
> >> - clk_disable(dd->clk_ref);
> >> - __clk_unprepare(dd->clk_ref);
> >> - clk_disable(dd->clk_bypass);
> >> - __clk_unprepare(dd->clk_bypass);
> >> -
> >> return 0;
> >> }
> >>
> >> --
> >> 1.7.9.5
> >>
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> >> the body of a message to majordomo at vger.kernel.org
> >> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] ARM: OMAP: dpll: enable bypass clock only when attempting dpll bypass
2013-03-28 10:59 [PATCH] ARM: OMAP: dpll: enable bypass clock only when attempting dpll bypass Rajendra Nayak
2013-03-28 16:32 ` Mike Turquette
@ 2013-04-01 10:27 ` Paul Walmsley
1 sibling, 0 replies; 5+ messages in thread
From: Paul Walmsley @ 2013-04-01 10:27 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, 28 Mar 2013, Rajendra Nayak wrote:
> omap3_noncore_dpll_set_rate() attempts an enable of bypass clk as well
> as ref clk for every .set_rate attempt on a noncore DPLL, regardless of
> whether the .set_rate results in the DPLL being locked or put in bypass.
> Early at boot, while some of these DPLLs are programmed and locked
> (using .set_rate for the DPLL), this causes an ordering issue.
>
> For instance, on OMAP5, the USB DPLL derives its bypass clk from ABE DPLL.
> If a .set_rate of USB DPLL which programmes the M,N and locks it is called
> before the one for ABE, the enable of USB bypass clk (derived from ABE DPLL)
> then attempts to lock the ABE DPLL and fails as the M,N values for ABE
> are yet to be programmed.
>
> To get rid of this ordering needs, enable bypass clk for a DPLL as part
> of its .set_rate only when its being put in bypass, and only enable the
> ref clk when its locked.
>
> Reported-by: Roger Quadros <rogerq@ti.com>
> Signed-off-by: Rajendra Nayak <rnayak@ti.com>
Thanks, queued for 3.10.
- Paul
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-04-01 10:27 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-03-28 10:59 [PATCH] ARM: OMAP: dpll: enable bypass clock only when attempting dpll bypass Rajendra Nayak
2013-03-28 16:32 ` Mike Turquette
2013-03-29 5:26 ` Rajendra Nayak
2013-03-29 17:24 ` Mike Turquette
2013-04-01 10:27 ` Paul Walmsley
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).