| Message ID | 522F1BF9.5050006@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Tero Kristo (2013-09-10 06:17:45) > On 09/10/2013 03:40 PM, Tomi Valkeinen wrote: > > On 10/09/13 15:24, Tero Kristo wrote: > >> On 09/10/2013 03:19 PM, Tomi Valkeinen wrote: > >>> On 10/09/13 15:12, Tero Kristo wrote: > >>> > >>>> If it claims it is not locked, it means the DPLL itself is disabled. You > >>>> could try clk_enable for the clock before doing clk_set_rate. > >>> > >>> Hmm, so is it required to enable the clock before setting the rate? If > >>> so, I think I'm using the clocks wrong in all the places =). > >> > >> In generic case, it is not. But DPLLs behave strangely if they go to low > >> power stop mode. If there is any downstream clock enabled for a specific > >> DPLL it is enabled and things work okay. > >> > >> One could also argue that the API behavior in OMAP is wrong currently, > >> as the bypass rate is something you are most likely never actually going > >> to use for anything.... > >> > >> Just try the change and check the results. > > > > Ok, so as Stefan said, enabling the clock fixes the issue. > > > > How do you suggest we fix this? Changing omapdss to enable the clock > > before changing its rate is not very difficult, so it can be used as a > > quick fix. But it doesn't sound like a proper fix if this is not > > normally required. > > The quick fix sounds good to me. > > > And, maybe I'm missing something as I don't have good understanding of > > the PRCM's PLLs, but the current behavior sounds odd. So the DPLL is > > off, and in bypass mode. When we try to change the rate of the clock > > provided by the PLL, shouldn't it fail, as bypass mode's rate cannot be > > changed? Or better, change the non-bypass rate. > > In theory, DPLLs can also be used in their bypass mode to feed customer > nodes clocks. I just think the check in the clkoutx2_recalc is wrong, > and should be enhanced to actually check what is the target mode for the > clock once it is enabled. Maybe something like this would work properly: > > diff --git a/arch/arm/mach-omap2/dpll3xxx.c b/arch/arm/mach-omap2/dpll3xxx.c > index 3a0296c..ba218fb 100644 > --- a/arch/arm/mach-omap2/dpll3xxx.c > +++ b/arch/arm/mach-omap2/dpll3xxx.c > @@ -658,14 +658,12 @@ unsigned long omap3_clkoutx2_recalc(struct clk_hw *hw, > > dd = pclk->dpll_data; > > - WARN_ON(!dd->enable_mask); > - > - v = __raw_readl(dd->control_reg) & dd->enable_mask; > - v >>= __ffs(dd->enable_mask); > - if ((v != OMAP3XXX_EN_DPLL_LOCKED) || (dd->flags & DPLL_J_TYPE)) > + if ((dd->flags & DPLL_J_TYPE) || > + __clk_get_rate(dd->clk_bypass) == __clk_get_rate(pclk)) Two quick asides here: 1) might be nice if the J_TYPE pll was it's own clock type 2) would be nice if the check for bypass used something like: if (clk_get_parent(hw->clk) == dd->bypass_clk) rate = parent_rate; Which implies that the DPLLs become proper mux clocks. Regards, Mike > rate = parent_rate; > else > rate = parent_rate * 2; > + > return rate; > } > > > > > > How is the DPLL4's clock rate 432000000 anyway in bypass mode. Isn't > > bypass mode usually plain sys-clock, or such? > > This again reflects the rate that the clock has once it is enabled, the > clkoutx2 does not. > > Getting comment from someone like Paul would probably help here. > > -Tero -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Sep 10, 2013 at 2:17 PM, Mike Turquette <mturquette@linaro.org> wrote: > Quoting Tero Kristo (2013-09-10 06:17:45) >> On 09/10/2013 03:40 PM, Tomi Valkeinen wrote: >> > On 10/09/13 15:24, Tero Kristo wrote: >> >> On 09/10/2013 03:19 PM, Tomi Valkeinen wrote: >> >>> On 10/09/13 15:12, Tero Kristo wrote: >> >>> >> >>>> If it claims it is not locked, it means the DPLL itself is disabled. You >> >>>> could try clk_enable for the clock before doing clk_set_rate. >> >>> >> >>> Hmm, so is it required to enable the clock before setting the rate? If >> >>> so, I think I'm using the clocks wrong in all the places =). >> >> >> >> In generic case, it is not. But DPLLs behave strangely if they go to low >> >> power stop mode. If there is any downstream clock enabled for a specific >> >> DPLL it is enabled and things work okay. >> >> >> >> One could also argue that the API behavior in OMAP is wrong currently, >> >> as the bypass rate is something you are most likely never actually going >> >> to use for anything.... >> >> >> >> Just try the change and check the results. >> > >> > Ok, so as Stefan said, enabling the clock fixes the issue. >> > >> > How do you suggest we fix this? Changing omapdss to enable the clock >> > before changing its rate is not very difficult, so it can be used as a >> > quick fix. But it doesn't sound like a proper fix if this is not >> > normally required. >> >> The quick fix sounds good to me. >> >> > And, maybe I'm missing something as I don't have good understanding of >> > the PRCM's PLLs, but the current behavior sounds odd. So the DPLL is >> > off, and in bypass mode. When we try to change the rate of the clock >> > provided by the PLL, shouldn't it fail, as bypass mode's rate cannot be >> > changed? Or better, change the non-bypass rate. >> >> In theory, DPLLs can also be used in their bypass mode to feed customer >> nodes clocks. I just think the check in the clkoutx2_recalc is wrong, >> and should be enhanced to actually check what is the target mode for the >> clock once it is enabled. Maybe something like this would work properly: >> >> diff --git a/arch/arm/mach-omap2/dpll3xxx.c b/arch/arm/mach-omap2/dpll3xxx.c >> index 3a0296c..ba218fb 100644 >> --- a/arch/arm/mach-omap2/dpll3xxx.c >> +++ b/arch/arm/mach-omap2/dpll3xxx.c >> @@ -658,14 +658,12 @@ unsigned long omap3_clkoutx2_recalc(struct clk_hw *hw, >> >> dd = pclk->dpll_data; >> >> - WARN_ON(!dd->enable_mask); >> - >> - v = __raw_readl(dd->control_reg) & dd->enable_mask; >> - v >>= __ffs(dd->enable_mask); >> - if ((v != OMAP3XXX_EN_DPLL_LOCKED) || (dd->flags & DPLL_J_TYPE)) >> + if ((dd->flags & DPLL_J_TYPE) || >> + __clk_get_rate(dd->clk_bypass) == __clk_get_rate(pclk)) > > Two quick asides here: > > 1) might be nice if the J_TYPE pll was it's own clock type > > 2) would be nice if the check for bypass used something like: > > if (clk_get_parent(hw->clk) == dd->bypass_clk) > rate = parent_rate; > > Which implies that the DPLLs become proper mux clocks. Please disregard the above! I'm reviewing the OMAP v6 CCF series and does these things already. Very cool. Regards, Mike > > Regards, > Mike > >> rate = parent_rate; >> else >> rate = parent_rate * 2; >> + >> return rate; >> } >> >> >> > >> > How is the DPLL4's clock rate 432000000 anyway in bypass mode. Isn't >> > bypass mode usually plain sys-clock, or such? >> >> This again reflects the rate that the clock has once it is enabled, the >> clkoutx2 does not. >> >> Getting comment from someone like Paul would probably help here. >> >> -Tero -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/09/13 16:17, Tero Kristo wrote: > In theory, DPLLs can also be used in their bypass mode to feed customer > nodes clocks. I just think the check in the clkoutx2_recalc is wrong, > and should be enhanced to actually check what is the target mode for the > clock once it is enabled. Maybe something like this would work properly: > > diff --git a/arch/arm/mach-omap2/dpll3xxx.c > b/arch/arm/mach-omap2/dpll3xxx.c > index 3a0296c..ba218fb 100644 > --- a/arch/arm/mach-omap2/dpll3xxx.c > +++ b/arch/arm/mach-omap2/dpll3xxx.c > @@ -658,14 +658,12 @@ unsigned long omap3_clkoutx2_recalc(struct clk_hw > *hw, > > dd = pclk->dpll_data; > > - WARN_ON(!dd->enable_mask); > - > - v = __raw_readl(dd->control_reg) & dd->enable_mask; > - v >>= __ffs(dd->enable_mask); > - if ((v != OMAP3XXX_EN_DPLL_LOCKED) || (dd->flags & DPLL_J_TYPE)) > + if ((dd->flags & DPLL_J_TYPE) || > + __clk_get_rate(dd->clk_bypass) == __clk_get_rate(pclk)) > rate = parent_rate; > else > rate = parent_rate * 2; > + > return rate; > } Stefan, are you able to test the above? I'd rather have a proper fix for this, than hack omapdss =). >> How is the DPLL4's clock rate 432000000 anyway in bypass mode. Isn't >> bypass mode usually plain sys-clock, or such? > > This again reflects the rate that the clock has once it is enabled, the > clkoutx2 does not. Ok. Then it does sound like the clkoutx2 is calculated wrong, as you say. Tomi
On 11.09.2013 09:21, Tomi Valkeinen wrote: > On 10/09/13 16:17, Tero Kristo wrote: > >> In theory, DPLLs can also be used in their bypass mode to feed customer >> nodes clocks. I just think the check in the clkoutx2_recalc is wrong, >> and should be enhanced to actually check what is the target mode for the >> clock once it is enabled. Maybe something like this would work properly: >> >> diff --git a/arch/arm/mach-omap2/dpll3xxx.c >> b/arch/arm/mach-omap2/dpll3xxx.c >> index 3a0296c..ba218fb 100644 >> --- a/arch/arm/mach-omap2/dpll3xxx.c >> +++ b/arch/arm/mach-omap2/dpll3xxx.c >> @@ -658,14 +658,12 @@ unsigned long omap3_clkoutx2_recalc(struct clk_hw >> *hw, >> >> dd = pclk->dpll_data; >> >> - WARN_ON(!dd->enable_mask); >> - >> - v = __raw_readl(dd->control_reg) & dd->enable_mask; >> - v >>= __ffs(dd->enable_mask); >> - if ((v != OMAP3XXX_EN_DPLL_LOCKED) || (dd->flags & DPLL_J_TYPE)) >> + if ((dd->flags & DPLL_J_TYPE) || >> + __clk_get_rate(dd->clk_bypass) == __clk_get_rate(pclk)) >> rate = parent_rate; >> else >> rate = parent_rate * 2; >> + >> return rate; >> } > > Stefan, are you able to test the above? > > I'd rather have a proper fix for this, than hack omapdss =). Okay, I finally found some time to test this. The patch above generates this warning: arch/arm/mach-omap2/dpll3xxx.c: In function 'omap3_clkoutx2_recalc': arch/arm/mach-omap2/dpll3xxx.c:663:6: warning: passing argument 1 of '__clk_get_rate' from incompatible pointer type [enabled by default] include/linux/clk-provider.h:423:15: note: expected 'struct clk *' but argument is of type 'struct clk_hw_omap *' I then changed it (not 100% sure if correctly) to this version: + if ((dd->flags & DPLL_J_TYPE) || + __clk_get_rate(dd->clk_bypass) == __clk_get_rate(pclk->hw.clk)) And this seems to work. At least the clock rate mismatch warning does not appear with this patch applied (and without the clk_enable) in the bootlog any more. Thanks, Stefan -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/13/2013 10:51 AM, Stefan Roese wrote: > On 11.09.2013 09:21, Tomi Valkeinen wrote: >> On 10/09/13 16:17, Tero Kristo wrote: >> >>> In theory, DPLLs can also be used in their bypass mode to feed customer >>> nodes clocks. I just think the check in the clkoutx2_recalc is wrong, >>> and should be enhanced to actually check what is the target mode for the >>> clock once it is enabled. Maybe something like this would work properly: >>> >>> diff --git a/arch/arm/mach-omap2/dpll3xxx.c >>> b/arch/arm/mach-omap2/dpll3xxx.c >>> index 3a0296c..ba218fb 100644 >>> --- a/arch/arm/mach-omap2/dpll3xxx.c >>> +++ b/arch/arm/mach-omap2/dpll3xxx.c >>> @@ -658,14 +658,12 @@ unsigned long omap3_clkoutx2_recalc(struct clk_hw >>> *hw, >>> >>> dd = pclk->dpll_data; >>> >>> - WARN_ON(!dd->enable_mask); >>> - >>> - v = __raw_readl(dd->control_reg) & dd->enable_mask; >>> - v >>= __ffs(dd->enable_mask); >>> - if ((v != OMAP3XXX_EN_DPLL_LOCKED) || (dd->flags & DPLL_J_TYPE)) >>> + if ((dd->flags & DPLL_J_TYPE) || >>> + __clk_get_rate(dd->clk_bypass) == __clk_get_rate(pclk)) >>> rate = parent_rate; >>> else >>> rate = parent_rate * 2; >>> + >>> return rate; >>> } >> >> Stefan, are you able to test the above? >> >> I'd rather have a proper fix for this, than hack omapdss =). > > Okay, I finally found some time to test this. The patch above generates > this warning: > > arch/arm/mach-omap2/dpll3xxx.c: In function 'omap3_clkoutx2_recalc': > arch/arm/mach-omap2/dpll3xxx.c:663:6: warning: passing argument 1 of '__clk_get_rate' from incompatible pointer type [enabled by default] > include/linux/clk-provider.h:423:15: note: expected 'struct clk *' but argument is of type 'struct clk_hw_omap *' Yea sorry about that, I just quickly hacked the patch together without testing it at all. :P > > I then changed it (not 100% sure if correctly) to this version: > > + if ((dd->flags & DPLL_J_TYPE) || > + __clk_get_rate(dd->clk_bypass) == __clk_get_rate(pclk->hw.clk)) > > And this seems to work. At least the clock rate mismatch warning does not > appear with this patch applied (and without the clk_enable) in the > bootlog any more. Yea, looks good to me, however I guess I would like second opinion on this also. -Tero -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoting Tero Kristo (2013-09-13 04:34:54) > On 09/13/2013 10:51 AM, Stefan Roese wrote: > > On 11.09.2013 09:21, Tomi Valkeinen wrote: > >> On 10/09/13 16:17, Tero Kristo wrote: > >> > >>> In theory, DPLLs can also be used in their bypass mode to feed customer > >>> nodes clocks. I just think the check in the clkoutx2_recalc is wrong, > >>> and should be enhanced to actually check what is the target mode for the > >>> clock once it is enabled. Maybe something like this would work properly: > >>> > >>> diff --git a/arch/arm/mach-omap2/dpll3xxx.c > >>> b/arch/arm/mach-omap2/dpll3xxx.c > >>> index 3a0296c..ba218fb 100644 > >>> --- a/arch/arm/mach-omap2/dpll3xxx.c > >>> +++ b/arch/arm/mach-omap2/dpll3xxx.c > >>> @@ -658,14 +658,12 @@ unsigned long omap3_clkoutx2_recalc(struct clk_hw > >>> *hw, > >>> > >>> dd = pclk->dpll_data; > >>> > >>> - WARN_ON(!dd->enable_mask); > >>> - > >>> - v = __raw_readl(dd->control_reg) & dd->enable_mask; > >>> - v >>= __ffs(dd->enable_mask); > >>> - if ((v != OMAP3XXX_EN_DPLL_LOCKED) || (dd->flags & DPLL_J_TYPE)) > >>> + if ((dd->flags & DPLL_J_TYPE) || > >>> + __clk_get_rate(dd->clk_bypass) == __clk_get_rate(pclk)) > >>> rate = parent_rate; > >>> else > >>> rate = parent_rate * 2; > >>> + > >>> return rate; > >>> } > >> > >> Stefan, are you able to test the above? > >> > >> I'd rather have a proper fix for this, than hack omapdss =). > > > > Okay, I finally found some time to test this. The patch above generates > > this warning: > > > > arch/arm/mach-omap2/dpll3xxx.c: In function 'omap3_clkoutx2_recalc': > > arch/arm/mach-omap2/dpll3xxx.c:663:6: warning: passing argument 1 of '__clk_get_rate' from incompatible pointer type [enabled by default] > > include/linux/clk-provider.h:423:15: note: expected 'struct clk *' but argument is of type 'struct clk_hw_omap *' > > Yea sorry about that, I just quickly hacked the patch together without > testing it at all. :P > > > > > I then changed it (not 100% sure if correctly) to this version: > > > > + if ((dd->flags & DPLL_J_TYPE) || > > + __clk_get_rate(dd->clk_bypass) == __clk_get_rate(pclk->hw.clk)) > > > > And this seems to work. At least the clock rate mismatch warning does not > > appear with this patch applied (and without the clk_enable) in the > > bootlog any more. > > Yea, looks good to me, however I guess I would like second opinion on > this also. Looks right to me. Regards, Mike > > -Tero -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 13/09/13 14:34, Kristo, Tero wrote: > On 09/13/2013 10:51 AM, Stefan Roese wrote: >> On 11.09.2013 09:21, Tomi Valkeinen wrote: >>> On 10/09/13 16:17, Tero Kristo wrote: >>> >>>> In theory, DPLLs can also be used in their bypass mode to feed customer >>>> nodes clocks. I just think the check in the clkoutx2_recalc is wrong, >>>> and should be enhanced to actually check what is the target mode for the >>>> clock once it is enabled. Maybe something like this would work properly: >>>> >>>> diff --git a/arch/arm/mach-omap2/dpll3xxx.c >>>> b/arch/arm/mach-omap2/dpll3xxx.c >>>> index 3a0296c..ba218fb 100644 >>>> --- a/arch/arm/mach-omap2/dpll3xxx.c >>>> +++ b/arch/arm/mach-omap2/dpll3xxx.c >>>> @@ -658,14 +658,12 @@ unsigned long omap3_clkoutx2_recalc(struct clk_hw >>>> *hw, >>>> >>>> dd = pclk->dpll_data; >>>> >>>> - WARN_ON(!dd->enable_mask); >>>> - >>>> - v = __raw_readl(dd->control_reg) & dd->enable_mask; >>>> - v >>= __ffs(dd->enable_mask); >>>> - if ((v != OMAP3XXX_EN_DPLL_LOCKED) || (dd->flags & DPLL_J_TYPE)) >>>> + if ((dd->flags & DPLL_J_TYPE) || >>>> + __clk_get_rate(dd->clk_bypass) == __clk_get_rate(pclk)) >>>> rate = parent_rate; >>>> else >>>> rate = parent_rate * 2; >>>> + >>>> return rate; >>>> } >>> >>> Stefan, are you able to test the above? >>> >>> I'd rather have a proper fix for this, than hack omapdss =). >> >> Okay, I finally found some time to test this. The patch above generates >> this warning: >> >> arch/arm/mach-omap2/dpll3xxx.c: In function 'omap3_clkoutx2_recalc': >> arch/arm/mach-omap2/dpll3xxx.c:663:6: warning: passing argument 1 of '__clk_get_rate' from incompatible pointer type [enabled by default] >> include/linux/clk-provider.h:423:15: note: expected 'struct clk *' but argument is of type 'struct clk_hw_omap *' > > Yea sorry about that, I just quickly hacked the patch together without > testing it at all. :P > >> >> I then changed it (not 100% sure if correctly) to this version: >> >> + if ((dd->flags & DPLL_J_TYPE) || >> + __clk_get_rate(dd->clk_bypass) == __clk_get_rate(pclk->hw.clk)) >> >> And this seems to work. At least the clock rate mismatch warning does not >> appear with this patch applied (and without the clk_enable) in the >> bootlog any more. > > Yea, looks good to me, however I guess I would like second opinion on > this also. Tero, can you queue this patch? Or who should handle this? Tomi
On 09/27/2013 11:41 AM, Tomi Valkeinen wrote: > On 13/09/13 14:34, Kristo, Tero wrote: >> On 09/13/2013 10:51 AM, Stefan Roese wrote: >>> On 11.09.2013 09:21, Tomi Valkeinen wrote: >>>> On 10/09/13 16:17, Tero Kristo wrote: >>>> >>>>> In theory, DPLLs can also be used in their bypass mode to feed customer >>>>> nodes clocks. I just think the check in the clkoutx2_recalc is wrong, >>>>> and should be enhanced to actually check what is the target mode for the >>>>> clock once it is enabled. Maybe something like this would work properly: >>>>> >>>>> diff --git a/arch/arm/mach-omap2/dpll3xxx.c >>>>> b/arch/arm/mach-omap2/dpll3xxx.c >>>>> index 3a0296c..ba218fb 100644 >>>>> --- a/arch/arm/mach-omap2/dpll3xxx.c >>>>> +++ b/arch/arm/mach-omap2/dpll3xxx.c >>>>> @@ -658,14 +658,12 @@ unsigned long omap3_clkoutx2_recalc(struct clk_hw >>>>> *hw, >>>>> >>>>> dd = pclk->dpll_data; >>>>> >>>>> - WARN_ON(!dd->enable_mask); >>>>> - >>>>> - v = __raw_readl(dd->control_reg) & dd->enable_mask; >>>>> - v >>= __ffs(dd->enable_mask); >>>>> - if ((v != OMAP3XXX_EN_DPLL_LOCKED) || (dd->flags & DPLL_J_TYPE)) >>>>> + if ((dd->flags & DPLL_J_TYPE) || >>>>> + __clk_get_rate(dd->clk_bypass) == __clk_get_rate(pclk)) >>>>> rate = parent_rate; >>>>> else >>>>> rate = parent_rate * 2; >>>>> + >>>>> return rate; >>>>> } >>>> >>>> Stefan, are you able to test the above? >>>> >>>> I'd rather have a proper fix for this, than hack omapdss =). >>> >>> Okay, I finally found some time to test this. The patch above generates >>> this warning: >>> >>> arch/arm/mach-omap2/dpll3xxx.c: In function 'omap3_clkoutx2_recalc': >>> arch/arm/mach-omap2/dpll3xxx.c:663:6: warning: passing argument 1 of '__clk_get_rate' from incompatible pointer type [enabled by default] >>> include/linux/clk-provider.h:423:15: note: expected 'struct clk *' but argument is of type 'struct clk_hw_omap *' >> >> Yea sorry about that, I just quickly hacked the patch together without >> testing it at all. :P >> >>> >>> I then changed it (not 100% sure if correctly) to this version: >>> >>> + if ((dd->flags & DPLL_J_TYPE) || >>> + __clk_get_rate(dd->clk_bypass) == __clk_get_rate(pclk->hw.clk)) >>> >>> And this seems to work. At least the clock rate mismatch warning does not >>> appear with this patch applied (and without the clk_enable) in the >>> bootlog any more. >> >> Yea, looks good to me, however I guess I would like second opinion on >> this also. > > Tero, can you queue this patch? Or who should handle this? I can't queue anything as I don't have maintainership on any of this stuff. Paul / Tony should go with this. -Tero -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 27/09/13 14:24, Tero Kristo wrote: >> Tero, can you queue this patch? Or who should handle this? > > I can't queue anything as I don't have maintainership on any of this > stuff. Paul / Tony should go with this. Well, I mainly meant preparing a patch with proper description and sending to relevant maintainers. For some reason I can't reproduce the issue with 3.12-rc1. I guess some other driver enables the clock now, which wasn't there in 3.11. So as far as DSS is concerned, things look fine now without the patch. But if the clock code is wrong, as I understood, maybe it's still good to have the patch applied. Tomi
On Tue, 10 Sep 2013, Tero Kristo wrote: > In theory, DPLLs can also be used in their bypass mode to feed customer nodes > clocks. I just think the check in the clkoutx2_recalc is wrong, and should be > enhanced to actually check what is the target mode for the clock once it is > enabled. Maybe something like this would work properly: > > diff --git a/arch/arm/mach-omap2/dpll3xxx.c b/arch/arm/mach-omap2/dpll3xxx.c > index 3a0296c..ba218fb 100644 > --- a/arch/arm/mach-omap2/dpll3xxx.c > +++ b/arch/arm/mach-omap2/dpll3xxx.c > @@ -658,14 +658,12 @@ unsigned long omap3_clkoutx2_recalc(struct clk_hw *hw, > > dd = pclk->dpll_data; > > - WARN_ON(!dd->enable_mask); > - > - v = __raw_readl(dd->control_reg) & dd->enable_mask; > - v >>= __ffs(dd->enable_mask); > - if ((v != OMAP3XXX_EN_DPLL_LOCKED) || (dd->flags & DPLL_J_TYPE)) > + if ((dd->flags & DPLL_J_TYPE) || > + __clk_get_rate(dd->clk_bypass) == __clk_get_rate(pclk)) > rate = parent_rate; > else > rate = parent_rate * 2; > + > return rate; > } > [...] > Getting comment from someone like Paul would probably help here. Looks like this is a regression from the CCF port. Seems to me that the code above assumes that when the DPLL's rate is set to the same rate as the bypass clock's rate, we can assume that the DPLL is in bypass. I wonder if that's valid in a case where a previous OS or bootloader may have programmed the DPLL. Sounds to me like the best way to fix it would be to test whether this code is intended to return the "target rate" (when the struct clk representing the DPLL is disabled), versus the "current rate" (when the struct clk representing the DPLL is enabled). If it's the target rate, then there's no point checking the current state of the hardware. A check similar to the above would probably be fine. Otherwise, seems best to use the existing code that does test the PLL state. - Paul -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoting Paul Walmsley (2013-10-07 01:21:16) > On Tue, 10 Sep 2013, Tero Kristo wrote: > > > In theory, DPLLs can also be used in their bypass mode to feed customer nodes > > clocks. I just think the check in the clkoutx2_recalc is wrong, and should be > > enhanced to actually check what is the target mode for the clock once it is > > enabled. Maybe something like this would work properly: > > > > diff --git a/arch/arm/mach-omap2/dpll3xxx.c b/arch/arm/mach-omap2/dpll3xxx.c > > index 3a0296c..ba218fb 100644 > > --- a/arch/arm/mach-omap2/dpll3xxx.c > > +++ b/arch/arm/mach-omap2/dpll3xxx.c > > @@ -658,14 +658,12 @@ unsigned long omap3_clkoutx2_recalc(struct clk_hw *hw, > > > > dd = pclk->dpll_data; > > > > - WARN_ON(!dd->enable_mask); > > - > > - v = __raw_readl(dd->control_reg) & dd->enable_mask; > > - v >>= __ffs(dd->enable_mask); > > - if ((v != OMAP3XXX_EN_DPLL_LOCKED) || (dd->flags & DPLL_J_TYPE)) > > + if ((dd->flags & DPLL_J_TYPE) || > > + __clk_get_rate(dd->clk_bypass) == __clk_get_rate(pclk)) > > rate = parent_rate; > > else > > rate = parent_rate * 2; > > + > > return rate; > > } > > > > [...] > > > Getting comment from someone like Paul would probably help here. > > Looks like this is a regression from the CCF port. > > Seems to me that the code above assumes that when the DPLL's rate is set > to the same rate as the bypass clock's rate, we can assume that the DPLL > is in bypass. I wonder if that's valid in a case where a previous OS or > bootloader may have programmed the DPLL. Well it used to be that calling clk_set_rate(dpll, bypass_rate) would put it in bypass, I don't know if that is still the case. But you are right that having the implicit assumption that 'bypass rate' == 'DPLL in bypass' is not safe since a bootloader could lock this PLL to the same rate as it's bypass rate. I hope that the bypass rate stuff does not get swept away in the changes since it is an interesting way to save a little power. Regards, Mike > > Sounds to me like the best way to fix it would be to test whether this > code is intended to return the "target rate" (when the struct clk > representing the DPLL is disabled), versus the "current rate" (when the > struct clk representing the DPLL is enabled). If it's the target rate, > then there's no point checking the current state of the hardware. A check > similar to the above would probably be fine. Otherwise, seems best to use > the existing code that does test the PLL state. > > > > - Paul -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 7 Oct 2013, Mike Turquette wrote: > Well it used to be that calling clk_set_rate(dpll, bypass_rate) would > put it in bypass, I don't know if that is still the case. But you are > right that having the implicit assumption that 'bypass rate' == 'DPLL in > bypass' is not safe since a bootloader could lock this PLL to the same > rate as it's bypass rate. > > I hope that the bypass rate stuff does not get swept away in the changes > since it is an interesting way to save a little power. Yeah, I don't think anyone's proposing to remove it, as far as I know. Am more concerned about any similar lurking problems in low-level clock code that use the current state of the clock hardware to determine a possible future rate. Am hoping that this particular issue is simply due to the state dependency between the CLKOUTX2 node and the PLL node. Considering this issue, in retrospect, it probably would have been better to merge the CLKOUTX2 node with the PLL. - Paul -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/arm/mach-omap2/dpll3xxx.c b/arch/arm/mach-omap2/dpll3xxx.c index 3a0296c..ba218fb 100644 --- a/arch/arm/mach-omap2/dpll3xxx.c +++ b/arch/arm/mach-omap2/dpll3xxx.c @@ -658,14 +658,12 @@ unsigned long omap3_clkoutx2_recalc(struct clk_hw *hw, dd = pclk->dpll_data; - WARN_ON(!dd->enable_mask); - - v = __raw_readl(dd->control_reg) & dd->enable_mask; - v >>= __ffs(dd->enable_mask); - if ((v != OMAP3XXX_EN_DPLL_LOCKED) || (dd->flags & DPLL_J_TYPE)) + if ((dd->flags & DPLL_J_TYPE) || + __clk_get_rate(dd->clk_bypass) == __clk_get_rate(pclk)) rate = parent_rate; else rate = parent_rate * 2; + return rate; }