Re: [PATCH] clk: ti: Fix FAPLL udelay in clk_enable with clk_prepare
* Peter Ujfalusi[150922 23:17]: > Tony, > > On 09/23/2015 12:23 AM, Tony Lindgren wrote: > > As recently pointed out (again) by Thomas and Russell, we must not > > wait in in clk_enable. The wait for PLL to lock needs to happen > > in clk_prepare instead. > > > > It seems this is a common copy paste error with the PLL drivers, > > and similar fixes should be applied to other PLL drivers after > > testing. > > One thing to note: > because of how the hwmod code works, at boot time we prepare all clocks for > the devices and in runtime the hwmod only uses clk_enable/disable, it will > never unprepare the clock(s). This will means that these clocks will be > enabled all the time and will never turned off. Good point, we need to check that for hwmod pm_runtime related functions. Basically the PLLs we can't disable until in pm_runtime_disable. I think the approach we need to take to be that clk_enable/disable is really optional clk_gate_enable/disable. And in many cases PLLs don't have a gate, so it's correct for pm_runtime enable/disable to not do anything for the PLLs. And the reason we don't want to change pm_runtime hooks to use clk_prepare_enable/disable is that then drivers can't use pm_runtime_irq_safe like some do currently. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] clk: ti: Fix FAPLL udelay in clk_enable with clk_prepare
Tony, On 09/23/2015 12:23 AM, Tony Lindgren wrote: > As recently pointed out (again) by Thomas and Russell, we must not > wait in in clk_enable. The wait for PLL to lock needs to happen > in clk_prepare instead. > > It seems this is a common copy paste error with the PLL drivers, > and similar fixes should be applied to other PLL drivers after > testing. One thing to note: because of how the hwmod code works, at boot time we prepare all clocks for the devices and in runtime the hwmod only uses clk_enable/disable, it will never unprepare the clock(s). This will means that these clocks will be enabled all the time and will never turned off. -- Péter > Cc: Brian Hutchinson> Cc: Felipe Balbi > Cc: Grygorii Strashko > Cc: Nishanth Menon > Cc: Russell King - ARM Linux > Cc: Thomas Gleixner > Cc: Sekhar Nori > Signed-off-by: Tony Lindgren > --- > drivers/clk/ti/fapll.c | 18 +- > 1 file changed, 9 insertions(+), 9 deletions(-) > > diff --git a/drivers/clk/ti/fapll.c b/drivers/clk/ti/fapll.c > index f4b2e98..e1db74a 100644 > --- a/drivers/clk/ti/fapll.c > +++ b/drivers/clk/ti/fapll.c > @@ -37,7 +37,7 @@ > #define FAPLL_PWD_OFFSET 4 > > #define MAX_FAPLL_OUTPUTS7 > -#define FAPLL_MAX_RETRIES1000 > +#define FAPLL_MAX_RETRIES5 > > #define to_fapll(_hw)container_of(_hw, struct fapll_data, hw) > #define to_synth(_hw)container_of(_hw, struct fapll_synth, > hw) > @@ -126,7 +126,7 @@ static int ti_fapll_wait_lock(struct fapll_data *fd) > if (retries-- <= 0) > break; > > - udelay(1); > + usleep_range(200, 300); > } > > pr_err("%s failed to lock\n", fd->name); > @@ -134,7 +134,7 @@ static int ti_fapll_wait_lock(struct fapll_data *fd) > return -ETIMEDOUT; > } > > -static int ti_fapll_enable(struct clk_hw *hw) > +static int ti_fapll_prepare(struct clk_hw *hw) > { > struct fapll_data *fd = to_fapll(hw); > u32 v = readl_relaxed(fd->base); > @@ -146,7 +146,7 @@ static int ti_fapll_enable(struct clk_hw *hw) > return 0; > } > > -static void ti_fapll_disable(struct clk_hw *hw) > +static void ti_fapll_unprepare(struct clk_hw *hw) > { > struct fapll_data *fd = to_fapll(hw); > u32 v = readl_relaxed(fd->base); > @@ -155,7 +155,7 @@ static void ti_fapll_disable(struct clk_hw *hw) > writel_relaxed(v, fd->base); > } > > -static int ti_fapll_is_enabled(struct clk_hw *hw) > +static int ti_fapll_is_prepared(struct clk_hw *hw) > { > struct fapll_data *fd = to_fapll(hw); > u32 v = readl_relaxed(fd->base); > @@ -261,7 +261,7 @@ static int ti_fapll_set_rate(struct clk_hw *hw, unsigned > long rate, > v |= pre_div_p << FAPLL_MAIN_DIV_P_SHIFT; > v |= mult_n << FAPLL_MAIN_MULT_N_SHIFT; > writel_relaxed(v, fd->base); > - if (ti_fapll_is_enabled(hw)) > + if (ti_fapll_is_prepared(hw)) > ti_fapll_wait_lock(fd); > ti_fapll_clear_bypass(fd); > > @@ -269,9 +269,9 @@ static int ti_fapll_set_rate(struct clk_hw *hw, unsigned > long rate, > } > > static struct clk_ops ti_fapll_ops = { > - .enable = ti_fapll_enable, > - .disable = ti_fapll_disable, > - .is_enabled = ti_fapll_is_enabled, > + .prepare = ti_fapll_prepare, > + .unprepare = ti_fapll_unprepare, > + .is_prepared = ti_fapll_is_prepared, > .recalc_rate = ti_fapll_recalc_rate, > .get_parent = ti_fapll_get_parent, > .round_rate = ti_fapll_round_rate, > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] clk: ti: Fix FAPLL udelay in clk_enable with clk_prepare
On Tue, Sep 22, 2015 at 02:23:05PM -0700, Tony Lindgren wrote: > As recently pointed out (again) by Thomas and Russell, we must not > wait in in clk_enable. The wait for PLL to lock needs to happen > in clk_prepare instead. > > It seems this is a common copy paste error with the PLL drivers, > and similar fixes should be applied to other PLL drivers after > testing. > > Cc: Brian Hutchinson> Cc: Felipe Balbi > Cc: Grygorii Strashko > Cc: Nishanth Menon > Cc: Russell King - ARM Linux As this moves things in the right direction (and only based on that): Acked-by: Russell King > Cc: Thomas Gleixner > Cc: Sekhar Nori > Signed-off-by: Tony Lindgren > --- > drivers/clk/ti/fapll.c | 18 +- > 1 file changed, 9 insertions(+), 9 deletions(-) > > diff --git a/drivers/clk/ti/fapll.c b/drivers/clk/ti/fapll.c > index f4b2e98..e1db74a 100644 > --- a/drivers/clk/ti/fapll.c > +++ b/drivers/clk/ti/fapll.c > @@ -37,7 +37,7 @@ > #define FAPLL_PWD_OFFSET 4 > > #define MAX_FAPLL_OUTPUTS7 > -#define FAPLL_MAX_RETRIES1000 > +#define FAPLL_MAX_RETRIES5 > > #define to_fapll(_hw)container_of(_hw, struct fapll_data, hw) > #define to_synth(_hw)container_of(_hw, struct fapll_synth, > hw) > @@ -126,7 +126,7 @@ static int ti_fapll_wait_lock(struct fapll_data *fd) > if (retries-- <= 0) > break; > > - udelay(1); > + usleep_range(200, 300); > } > > pr_err("%s failed to lock\n", fd->name); > @@ -134,7 +134,7 @@ static int ti_fapll_wait_lock(struct fapll_data *fd) > return -ETIMEDOUT; > } > > -static int ti_fapll_enable(struct clk_hw *hw) > +static int ti_fapll_prepare(struct clk_hw *hw) > { > struct fapll_data *fd = to_fapll(hw); > u32 v = readl_relaxed(fd->base); > @@ -146,7 +146,7 @@ static int ti_fapll_enable(struct clk_hw *hw) > return 0; > } > > -static void ti_fapll_disable(struct clk_hw *hw) > +static void ti_fapll_unprepare(struct clk_hw *hw) > { > struct fapll_data *fd = to_fapll(hw); > u32 v = readl_relaxed(fd->base); > @@ -155,7 +155,7 @@ static void ti_fapll_disable(struct clk_hw *hw) > writel_relaxed(v, fd->base); > } > > -static int ti_fapll_is_enabled(struct clk_hw *hw) > +static int ti_fapll_is_prepared(struct clk_hw *hw) > { > struct fapll_data *fd = to_fapll(hw); > u32 v = readl_relaxed(fd->base); > @@ -261,7 +261,7 @@ static int ti_fapll_set_rate(struct clk_hw *hw, unsigned > long rate, > v |= pre_div_p << FAPLL_MAIN_DIV_P_SHIFT; > v |= mult_n << FAPLL_MAIN_MULT_N_SHIFT; > writel_relaxed(v, fd->base); > - if (ti_fapll_is_enabled(hw)) > + if (ti_fapll_is_prepared(hw)) > ti_fapll_wait_lock(fd); > ti_fapll_clear_bypass(fd); > > @@ -269,9 +269,9 @@ static int ti_fapll_set_rate(struct clk_hw *hw, unsigned > long rate, > } > > static struct clk_ops ti_fapll_ops = { > - .enable = ti_fapll_enable, > - .disable = ti_fapll_disable, > - .is_enabled = ti_fapll_is_enabled, > + .prepare = ti_fapll_prepare, > + .unprepare = ti_fapll_unprepare, > + .is_prepared = ti_fapll_is_prepared, > .recalc_rate = ti_fapll_recalc_rate, > .get_parent = ti_fapll_get_parent, > .round_rate = ti_fapll_round_rate, > -- > 2.1.4 > -- FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] clk: ti: Fix FAPLL udelay in clk_enable with clk_prepare
As recently pointed out (again) by Thomas and Russell, we must not wait in in clk_enable. The wait for PLL to lock needs to happen in clk_prepare instead. It seems this is a common copy paste error with the PLL drivers, and similar fixes should be applied to other PLL drivers after testing. Cc: Brian HutchinsonCc: Felipe Balbi Cc: Grygorii Strashko Cc: Nishanth Menon Cc: Russell King - ARM Linux Cc: Thomas Gleixner Cc: Sekhar Nori Signed-off-by: Tony Lindgren --- drivers/clk/ti/fapll.c | 18 +- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/drivers/clk/ti/fapll.c b/drivers/clk/ti/fapll.c index f4b2e98..e1db74a 100644 --- a/drivers/clk/ti/fapll.c +++ b/drivers/clk/ti/fapll.c @@ -37,7 +37,7 @@ #define FAPLL_PWD_OFFSET 4 #define MAX_FAPLL_OUTPUTS 7 -#define FAPLL_MAX_RETRIES 1000 +#define FAPLL_MAX_RETRIES 5 #define to_fapll(_hw) container_of(_hw, struct fapll_data, hw) #define to_synth(_hw) container_of(_hw, struct fapll_synth, hw) @@ -126,7 +126,7 @@ static int ti_fapll_wait_lock(struct fapll_data *fd) if (retries-- <= 0) break; - udelay(1); + usleep_range(200, 300); } pr_err("%s failed to lock\n", fd->name); @@ -134,7 +134,7 @@ static int ti_fapll_wait_lock(struct fapll_data *fd) return -ETIMEDOUT; } -static int ti_fapll_enable(struct clk_hw *hw) +static int ti_fapll_prepare(struct clk_hw *hw) { struct fapll_data *fd = to_fapll(hw); u32 v = readl_relaxed(fd->base); @@ -146,7 +146,7 @@ static int ti_fapll_enable(struct clk_hw *hw) return 0; } -static void ti_fapll_disable(struct clk_hw *hw) +static void ti_fapll_unprepare(struct clk_hw *hw) { struct fapll_data *fd = to_fapll(hw); u32 v = readl_relaxed(fd->base); @@ -155,7 +155,7 @@ static void ti_fapll_disable(struct clk_hw *hw) writel_relaxed(v, fd->base); } -static int ti_fapll_is_enabled(struct clk_hw *hw) +static int ti_fapll_is_prepared(struct clk_hw *hw) { struct fapll_data *fd = to_fapll(hw); u32 v = readl_relaxed(fd->base); @@ -261,7 +261,7 @@ static int ti_fapll_set_rate(struct clk_hw *hw, unsigned long rate, v |= pre_div_p << FAPLL_MAIN_DIV_P_SHIFT; v |= mult_n << FAPLL_MAIN_MULT_N_SHIFT; writel_relaxed(v, fd->base); - if (ti_fapll_is_enabled(hw)) + if (ti_fapll_is_prepared(hw)) ti_fapll_wait_lock(fd); ti_fapll_clear_bypass(fd); @@ -269,9 +269,9 @@ static int ti_fapll_set_rate(struct clk_hw *hw, unsigned long rate, } static struct clk_ops ti_fapll_ops = { - .enable = ti_fapll_enable, - .disable = ti_fapll_disable, - .is_enabled = ti_fapll_is_enabled, + .prepare = ti_fapll_prepare, + .unprepare = ti_fapll_unprepare, + .is_prepared = ti_fapll_is_prepared, .recalc_rate = ti_fapll_recalc_rate, .get_parent = ti_fapll_get_parent, .round_rate = ti_fapll_round_rate, -- 2.1.4 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html