| Message ID | 1492795191-31298-2-git-send-email-s.nawrocki@samsung.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On 04/21, Sylwester Nawrocki wrote: > The existing enable/disable ops for PLL35XX are made more generic > and used also for PLL36XX. This fixes issues in the kernel with > PLL36XX PLLs when the PLL has not been already enabled by bootloader. > > Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com> > --- > drivers/clk/samsung/clk-pll.c | 85 +++++++++++++++++++++++++------------------ > 1 file changed, 49 insertions(+), 36 deletions(-) > > diff --git a/drivers/clk/samsung/clk-pll.c b/drivers/clk/samsung/clk-pll.c > index 5229089..10c76eb 100644 > --- a/drivers/clk/samsung/clk-pll.c > +++ b/drivers/clk/samsung/clk-pll.c > @@ -23,6 +23,10 @@ struct samsung_clk_pll { > struct clk_hw hw; > void __iomem *lock_reg; > void __iomem *con_reg; > + /* PLL enable control bit offset in @con_reg register */ > + unsigned short enable_offs; > + /* PLL lock status bit offset in @con_reg register */ > + unsigned short lock_offs; > enum samsung_pll_type type; > unsigned int rate_count; > const struct samsung_pll_rate_table *rate_table; > @@ -61,6 +65,34 @@ static long samsung_pll_round_rate(struct clk_hw *hw, > return rate_table[i - 1].rate; > } > > +static int samsung_pll3xxx_enable(struct clk_hw *hw) > +{ > + struct samsung_clk_pll *pll = to_clk_pll(hw); > + u32 tmp; > + > + tmp = readl_relaxed(pll->con_reg); > + tmp |= BIT(pll->enable_offs); > + writel_relaxed(tmp, pll->con_reg); > + > + /* wait lock time */ > + do { > + cpu_relax(); > + tmp = readl_relaxed(pll->con_reg); > + } while (!(tmp & BIT(pll->lock_offs))); Not a problem with this patch because we're moving code around, but this is a potential infinite loop that should have some sort of timeout so we don't sit here forever trying to see a bit toggle.
On Fri, Apr 21, 2017 at 07:19:45PM +0200, Sylwester Nawrocki wrote: > The existing enable/disable ops for PLL35XX are made more generic > and used also for PLL36XX. This fixes issues in the kernel with > PLL36XX PLLs when the PLL has not been already enabled by bootloader. > > Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com> > --- > drivers/clk/samsung/clk-pll.c | 85 +++++++++++++++++++++++++------------------ > 1 file changed, 49 insertions(+), 36 deletions(-) > > diff --git a/drivers/clk/samsung/clk-pll.c b/drivers/clk/samsung/clk-pll.c > index 5229089..10c76eb 100644 > --- a/drivers/clk/samsung/clk-pll.c > +++ b/drivers/clk/samsung/clk-pll.c > @@ -23,6 +23,10 @@ struct samsung_clk_pll { > struct clk_hw hw; > void __iomem *lock_reg; > void __iomem *con_reg; > + /* PLL enable control bit offset in @con_reg register */ > + unsigned short enable_offs; > + /* PLL lock status bit offset in @con_reg register */ > + unsigned short lock_offs; > enum samsung_pll_type type; > unsigned int rate_count; > const struct samsung_pll_rate_table *rate_table; > @@ -61,6 +65,34 @@ static long samsung_pll_round_rate(struct clk_hw *hw, > return rate_table[i - 1].rate; > } > > +static int samsung_pll3xxx_enable(struct clk_hw *hw) > +{ > + struct samsung_clk_pll *pll = to_clk_pll(hw); > + u32 tmp; > + > + tmp = readl_relaxed(pll->con_reg); > + tmp |= BIT(pll->enable_offs); > + writel_relaxed(tmp, pll->con_reg); > + > + /* wait lock time */ > + do { > + cpu_relax(); > + tmp = readl_relaxed(pll->con_reg); > + } while (!(tmp & BIT(pll->lock_offs))); > + > + return 0; > +} > + > +static void samsung_pll3xxx_disable(struct clk_hw *hw) > +{ > + struct samsung_clk_pll *pll = to_clk_pll(hw); > + u32 tmp; > + > + tmp = readl_relaxed(pll->con_reg); > + tmp |= BIT(pll->enable_offs); I think you meant here: tmp &= ~BIT() > + writel_relaxed(tmp, pll->con_reg); > +} > + > /* > * PLL2126 Clock Type > */ > @@ -142,34 +174,6 @@ static unsigned long samsung_pll3000_recalc_rate(struct clk_hw *hw, > #define PLL35XX_LOCK_STAT_SHIFT (29) > #define PLL35XX_ENABLE_SHIFT (31) > > -static int samsung_pll35xx_enable(struct clk_hw *hw) > -{ > - struct samsung_clk_pll *pll = to_clk_pll(hw); > - u32 tmp; > - > - tmp = readl_relaxed(pll->con_reg); > - tmp |= BIT(PLL35XX_ENABLE_SHIFT); > - writel_relaxed(tmp, pll->con_reg); > - > - /* wait_lock_time */ > - do { > - cpu_relax(); > - tmp = readl_relaxed(pll->con_reg); > - } while (!(tmp & BIT(PLL35XX_LOCK_STAT_SHIFT))); > - > - return 0; > -} > - > -static void samsung_pll35xx_disable(struct clk_hw *hw) > -{ > - struct samsung_clk_pll *pll = to_clk_pll(hw); > - u32 tmp; > - > - tmp = readl_relaxed(pll->con_reg); > - tmp &= ~BIT(PLL35XX_ENABLE_SHIFT); > - writel_relaxed(tmp, pll->con_reg); > -} > - > static unsigned long samsung_pll35xx_recalc_rate(struct clk_hw *hw, > unsigned long parent_rate) > { > @@ -239,11 +243,11 @@ static int samsung_pll35xx_set_rate(struct clk_hw *hw, unsigned long drate, > writel_relaxed(tmp, pll->con_reg); > > /* wait_lock_time if enabled */ > - if (tmp & BIT(PLL35XX_ENABLE_SHIFT)) { > + if (tmp & BIT(pll->enable_offs)) { > do { > cpu_relax(); > tmp = readl_relaxed(pll->con_reg); > - } while (!(tmp & BIT(PLL35XX_LOCK_STAT_SHIFT))); > + } while (!(tmp & BIT(pll->lock_offs))); > } > return 0; > } > @@ -252,8 +256,8 @@ static int samsung_pll35xx_set_rate(struct clk_hw *hw, unsigned long drate, > .recalc_rate = samsung_pll35xx_recalc_rate, > .round_rate = samsung_pll_round_rate, > .set_rate = samsung_pll35xx_set_rate, > - .enable = samsung_pll35xx_enable, > - .disable = samsung_pll35xx_disable, > + .enable = samsung_pll3xxx_enable, > + .disable = samsung_pll3xxx_disable, > }; > > static const struct clk_ops samsung_pll35xx_clk_min_ops = { > @@ -275,6 +279,7 @@ static int samsung_pll35xx_set_rate(struct clk_hw *hw, unsigned long drate, > #define PLL36XX_SDIV_SHIFT (0) > #define PLL36XX_KDIV_SHIFT (0) > #define PLL36XX_LOCK_STAT_SHIFT (29) > +#define PLL36XX_ENABLE_SHIFT (31) > > static unsigned long samsung_pll36xx_recalc_rate(struct clk_hw *hw, > unsigned long parent_rate) > @@ -354,10 +359,12 @@ static int samsung_pll36xx_set_rate(struct clk_hw *hw, unsigned long drate, > writel_relaxed(pll_con1, pll->con_reg + 4); > > /* wait_lock_time */ > - do { > - cpu_relax(); > - tmp = readl_relaxed(pll->con_reg); > - } while (!(tmp & (1 << PLL36XX_LOCK_STAT_SHIFT))); > + if (pll_con0 & BIT(pll->enable_offs)) { Why this additional if() is needed? > + do { > + cpu_relax(); > + tmp = readl_relaxed(pll->con_reg); > + } while (!(tmp & BIT(PLL36XX_LOCK_STAT_SHIFT))); To be consistent: BIT(pll->lock_offs)? Best regards, Krzysztof -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/22/2017 05:22 PM, Krzysztof Kozlowski wrote: [...] >> +static void samsung_pll3xxx_disable(struct clk_hw *hw) >> +{ >> + struct samsung_clk_pll *pll = to_clk_pll(hw); >> + u32 tmp; >> + >> + tmp = readl_relaxed(pll->con_reg); >> + tmp |= BIT(pll->enable_offs); > > I think you meant here: > tmp &= ~BIT() Yes, I messed it up while copy/pasting from samsung_pll3xxx_enable(). >> + writel_relaxed(tmp, pll->con_reg); >> +} >> static unsigned long samsung_pll36xx_recalc_rate(struct clk_hw *hw, >> unsigned long parent_rate) >> @@ -354,10 +359,12 @@ static int samsung_pll36xx_set_rate(struct clk_hw *hw, unsigned long drate, >> writel_relaxed(pll_con1, pll->con_reg + 4); >> >> /* wait_lock_time */ >> - do { >> - cpu_relax(); >> - tmp = readl_relaxed(pll->con_reg); >> - } while (!(tmp & (1 << PLL36XX_LOCK_STAT_SHIFT))); I will add a comment here like: /* Wait until the PLL is locked if it is enabled. */ >> + if (pll_con0 & BIT(pll->enable_offs)) { > > Why this additional if() is needed? The PLL will never transition to a locked state if it is disabled, without this test we would end up with an indefinite loop below. >> + do { >> + cpu_relax(); >> + tmp = readl_relaxed(pll->con_reg); >> + } while (!(tmp & BIT(PLL36XX_LOCK_STAT_SHIFT))); > > To be consistent: > BIT(pll->lock_offs)? Yes, fixed. Thanks for your review! -- Regards, Sylwester -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/22/2017 04:51 AM, Stephen Boyd wrote: >> +static int samsung_pll3xxx_enable(struct clk_hw *hw) >> +{ >> + struct samsung_clk_pll *pll = to_clk_pll(hw); >> + u32 tmp; >> + >> + tmp = readl_relaxed(pll->con_reg); >> + tmp |= BIT(pll->enable_offs); >> + writel_relaxed(tmp, pll->con_reg); >> + >> + /* wait lock time */ >> + do { >> + cpu_relax(); >> + tmp = readl_relaxed(pll->con_reg); >> + } while (!(tmp & BIT(pll->lock_offs))); > > Not a problem with this patch because we're moving code around, > but this is a potential infinite loop that should have some sort > of timeout so we don't sit here forever trying to see a bit > toggle. Yes, I will add some protection in new patch, like it is done for newer PLLs. Thanks for you review.
On Mon, Apr 24, 2017 at 1:12 PM, Sylwester Nawrocki <s.nawrocki@samsung.com> wrote: > On 04/22/2017 05:22 PM, Krzysztof Kozlowski wrote: >>> @@ -354,10 +359,12 @@ static int samsung_pll36xx_set_rate(struct clk_hw >>> *hw, unsigned long drate, >>> writel_relaxed(pll_con1, pll->con_reg + 4); >>> >>> /* wait_lock_time */ >>> - do { >>> - cpu_relax(); >>> - tmp = readl_relaxed(pll->con_reg); >>> - } while (!(tmp & (1 << PLL36XX_LOCK_STAT_SHIFT))); > > > I will add a comment here like: > /* Wait until the PLL is locked if it is enabled. */ > >>> + if (pll_con0 & BIT(pll->enable_offs)) { >> >> >> Why this additional if() is needed? > > > The PLL will never transition to a locked state if it is disabled, without > this test we would end up with an indefinite loop below. Hmmm... shouldn't this be split from this change? Or maybe this is needed only because you added enable/disable for pl36xx and previously this was not existing? Best regards, Krzysztof -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
(dropping unrelated addresses from Cc list) On 04/24/2017 01:18 PM, Krzysztof Kozlowski wrote: > On Mon, Apr 24, 2017 at 1:12 PM, Sylwester Nawrocki > <s.nawrocki@samsung.com> wrote: >> On 04/22/2017 05:22 PM, Krzysztof Kozlowski wrote: >>>> @@ -354,10 +359,12 @@ static int samsung_pll36xx_set_rate(struct clk_hw >>>> *hw, unsigned long drate, >>>> writel_relaxed(pll_con1, pll->con_reg + 4); >>>> >>>> /* wait_lock_time */ >>>> - do { >>>> - cpu_relax(); >>>> - tmp = readl_relaxed(pll->con_reg); >>>> - } while (!(tmp & (1 << PLL36XX_LOCK_STAT_SHIFT))); >> >> >> I will add a comment here like: >> /* Wait until the PLL is locked if it is enabled. */ >> >>>> + if (pll_con0 & BIT(pll->enable_offs)) { >>> >>> >>> Why this additional if() is needed? >> >> >> The PLL will never transition to a locked state if it is disabled, without >> this test we would end up with an indefinite loop below. > > Hmmm... shouldn't this be split from this change? Or maybe this is > needed only because you added enable/disable for pl36xx and previously > this was not existing? Yes, it looks like previously the PLLs were always on after were enabled. I wouldn't be separating that part, as it's all logically dependent. Just the commit message might need some improvement.
On Mon, Apr 24, 2017 at 1:35 PM, Sylwester Nawrocki <s.nawrocki@samsung.com> wrote: > (dropping unrelated addresses from Cc list) > > On 04/24/2017 01:18 PM, Krzysztof Kozlowski wrote: >> >> On Mon, Apr 24, 2017 at 1:12 PM, Sylwester Nawrocki >> <s.nawrocki@samsung.com> wrote: >>> >>> On 04/22/2017 05:22 PM, Krzysztof Kozlowski wrote: >>>>> >>>>> @@ -354,10 +359,12 @@ static int samsung_pll36xx_set_rate(struct clk_hw >>>>> *hw, unsigned long drate, >>>>> writel_relaxed(pll_con1, pll->con_reg + 4); >>>>> >>>>> /* wait_lock_time */ >>>>> - do { >>>>> - cpu_relax(); >>>>> - tmp = readl_relaxed(pll->con_reg); >>>>> - } while (!(tmp & (1 << PLL36XX_LOCK_STAT_SHIFT))); >>> >>> >>> >>> I will add a comment here like: >>> /* Wait until the PLL is locked if it is enabled. */ >>> >>>>> + if (pll_con0 & BIT(pll->enable_offs)) { >>>> >>>> >>>> >>>> Why this additional if() is needed? >>> >>> >>> >>> The PLL will never transition to a locked state if it is disabled, >>> without >>> this test we would end up with an indefinite loop below. >> >> >> Hmmm... shouldn't this be split from this change? Or maybe this is >> needed only because you added enable/disable for pl36xx and previously >> this was not existing? > > > Yes, it looks like previously the PLLs were always on after were enabled. > I wouldn't be separating that part, as it's all logically dependent. > Just the commit message might need some improvement. Thanks for explanation. Best regards, Krzysztof -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" 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/drivers/clk/samsung/clk-pll.c b/drivers/clk/samsung/clk-pll.c index 5229089..10c76eb 100644 --- a/drivers/clk/samsung/clk-pll.c +++ b/drivers/clk/samsung/clk-pll.c @@ -23,6 +23,10 @@ struct samsung_clk_pll { struct clk_hw hw; void __iomem *lock_reg; void __iomem *con_reg; + /* PLL enable control bit offset in @con_reg register */ + unsigned short enable_offs; + /* PLL lock status bit offset in @con_reg register */ + unsigned short lock_offs; enum samsung_pll_type type; unsigned int rate_count; const struct samsung_pll_rate_table *rate_table; @@ -61,6 +65,34 @@ static long samsung_pll_round_rate(struct clk_hw *hw, return rate_table[i - 1].rate; } +static int samsung_pll3xxx_enable(struct clk_hw *hw) +{ + struct samsung_clk_pll *pll = to_clk_pll(hw); + u32 tmp; + + tmp = readl_relaxed(pll->con_reg); + tmp |= BIT(pll->enable_offs); + writel_relaxed(tmp, pll->con_reg); + + /* wait lock time */ + do { + cpu_relax(); + tmp = readl_relaxed(pll->con_reg); + } while (!(tmp & BIT(pll->lock_offs))); + + return 0; +} + +static void samsung_pll3xxx_disable(struct clk_hw *hw) +{ + struct samsung_clk_pll *pll = to_clk_pll(hw); + u32 tmp; + + tmp = readl_relaxed(pll->con_reg); + tmp |= BIT(pll->enable_offs); + writel_relaxed(tmp, pll->con_reg); +} + /* * PLL2126 Clock Type */ @@ -142,34 +174,6 @@ static unsigned long samsung_pll3000_recalc_rate(struct clk_hw *hw, #define PLL35XX_LOCK_STAT_SHIFT (29) #define PLL35XX_ENABLE_SHIFT (31) -static int samsung_pll35xx_enable(struct clk_hw *hw) -{ - struct samsung_clk_pll *pll = to_clk_pll(hw); - u32 tmp; - - tmp = readl_relaxed(pll->con_reg); - tmp |= BIT(PLL35XX_ENABLE_SHIFT); - writel_relaxed(tmp, pll->con_reg); - - /* wait_lock_time */ - do { - cpu_relax(); - tmp = readl_relaxed(pll->con_reg); - } while (!(tmp & BIT(PLL35XX_LOCK_STAT_SHIFT))); - - return 0; -} - -static void samsung_pll35xx_disable(struct clk_hw *hw) -{ - struct samsung_clk_pll *pll = to_clk_pll(hw); - u32 tmp; - - tmp = readl_relaxed(pll->con_reg); - tmp &= ~BIT(PLL35XX_ENABLE_SHIFT); - writel_relaxed(tmp, pll->con_reg); -} - static unsigned long samsung_pll35xx_recalc_rate(struct clk_hw *hw, unsigned long parent_rate) { @@ -239,11 +243,11 @@ static int samsung_pll35xx_set_rate(struct clk_hw *hw, unsigned long drate, writel_relaxed(tmp, pll->con_reg); /* wait_lock_time if enabled */ - if (tmp & BIT(PLL35XX_ENABLE_SHIFT)) { + if (tmp & BIT(pll->enable_offs)) { do { cpu_relax(); tmp = readl_relaxed(pll->con_reg); - } while (!(tmp & BIT(PLL35XX_LOCK_STAT_SHIFT))); + } while (!(tmp & BIT(pll->lock_offs))); } return 0; } @@ -252,8 +256,8 @@ static int samsung_pll35xx_set_rate(struct clk_hw *hw, unsigned long drate, .recalc_rate = samsung_pll35xx_recalc_rate, .round_rate = samsung_pll_round_rate, .set_rate = samsung_pll35xx_set_rate, - .enable = samsung_pll35xx_enable, - .disable = samsung_pll35xx_disable, + .enable = samsung_pll3xxx_enable, + .disable = samsung_pll3xxx_disable, }; static const struct clk_ops samsung_pll35xx_clk_min_ops = { @@ -275,6 +279,7 @@ static int samsung_pll35xx_set_rate(struct clk_hw *hw, unsigned long drate, #define PLL36XX_SDIV_SHIFT (0) #define PLL36XX_KDIV_SHIFT (0) #define PLL36XX_LOCK_STAT_SHIFT (29) +#define PLL36XX_ENABLE_SHIFT (31) static unsigned long samsung_pll36xx_recalc_rate(struct clk_hw *hw, unsigned long parent_rate) @@ -354,10 +359,12 @@ static int samsung_pll36xx_set_rate(struct clk_hw *hw, unsigned long drate, writel_relaxed(pll_con1, pll->con_reg + 4); /* wait_lock_time */ - do { - cpu_relax(); - tmp = readl_relaxed(pll->con_reg); - } while (!(tmp & (1 << PLL36XX_LOCK_STAT_SHIFT))); + if (pll_con0 & BIT(pll->enable_offs)) { + do { + cpu_relax(); + tmp = readl_relaxed(pll->con_reg); + } while (!(tmp & BIT(PLL36XX_LOCK_STAT_SHIFT))); + } return 0; } @@ -366,6 +373,8 @@ static int samsung_pll36xx_set_rate(struct clk_hw *hw, unsigned long drate, .recalc_rate = samsung_pll36xx_recalc_rate, .set_rate = samsung_pll36xx_set_rate, .round_rate = samsung_pll_round_rate, + .enable = samsung_pll3xxx_enable, + .disable = samsung_pll3xxx_disable, }; static const struct clk_ops samsung_pll36xx_clk_min_ops = { @@ -1288,6 +1297,8 @@ static void __init _samsung_clk_register_pll(struct samsung_clk_provider *ctx, case pll_1450x: case pll_1451x: case pll_1452x: + pll->enable_offs = PLL35XX_ENABLE_SHIFT; + pll->lock_offs = PLL35XX_LOCK_STAT_SHIFT; if (!pll->rate_table) init.ops = &samsung_pll35xx_clk_min_ops; else @@ -1306,6 +1317,8 @@ static void __init _samsung_clk_register_pll(struct samsung_clk_provider *ctx, /* clk_ops for 36xx and 2650 are similar */ case pll_36xx: case pll_2650: + pll->enable_offs = PLL36XX_ENABLE_SHIFT; + pll->lock_offs = PLL36XX_LOCK_STAT_SHIFT; if (!pll->rate_table) init.ops = &samsung_pll36xx_clk_min_ops; else
The existing enable/disable ops for PLL35XX are made more generic and used also for PLL36XX. This fixes issues in the kernel with PLL36XX PLLs when the PLL has not been already enabled by bootloader. Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com> --- drivers/clk/samsung/clk-pll.c | 85 +++++++++++++++++++++++++------------------ 1 file changed, 49 insertions(+), 36 deletions(-)