2015-05-26 22:09:40

by Ezequiel Garcia

[permalink] [raw]
Subject: [PATCH 0/3] clk: pistachio: Assorted fixes

This patchset contains a few fixes for the Pistachio
clock driver.

This is based on the series "clk: pistachio: Assorted changes"
(http://permalink.gmane.org/gmane.linux.kernel/1960107). I've dropped
the patches related to the fractional and integer PLLs rate change
and left only the fixes, as the rest might need some polishing.

Here's a brief summary of the patches:

Patches 1 and 2 clean up the PLL lock handling.

Patch 3 adds some very useful sanity checks on integer and fractions PLL
set_rate(), to make sure the parameters are modified only when it's legal
to do so.

None of these are really urgent fixes so this is all v4.2 material.
Hope we are still in time!

Ezequiel Garcia (2):
clk: pistachio: Add a pll_lock() helper for clarity
clk: pistachio: Lock the PLL when enabled upon rate change

Kevin Cernekee (1):
clk: pistachio: Add sanity checks on PLL configuration

drivers/clk/pistachio/clk-pll.c | 115 ++++++++++++++++++++++++++++++++--------
1 file changed, 93 insertions(+), 22 deletions(-)

--
2.3.3


2015-05-26 22:08:55

by Ezequiel Garcia

[permalink] [raw]
Subject: [PATCH 1/3] clk: pistachio: Add a pll_lock() helper for clarity

This commit adds a pll_lock() helper making the code more readable.
Cosmetic change only, no functionality changes.

Signed-off-by: Andrew Bresticker <[email protected]>
Signed-off-by: Ezequiel Garcia <[email protected]>
---
drivers/clk/pistachio/clk-pll.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/pistachio/clk-pll.c b/drivers/clk/pistachio/clk-pll.c
index de53756..9ce1be7 100644
--- a/drivers/clk/pistachio/clk-pll.c
+++ b/drivers/clk/pistachio/clk-pll.c
@@ -67,6 +67,12 @@ static inline void pll_writel(struct pistachio_clk_pll *pll, u32 val, u32 reg)
writel(val, pll->base + reg);
}

+static inline void pll_lock(struct pistachio_clk_pll *pll)
+{
+ while (!(pll_readl(pll, PLL_STATUS) & PLL_STATUS_LOCK))
+ cpu_relax();
+}
+
static inline u32 do_div_round_closest(u64 dividend, u32 divisor)
{
dividend += divisor / 2;
@@ -178,8 +184,7 @@ static int pll_gf40lp_frac_set_rate(struct clk_hw *hw, unsigned long rate,
(params->postdiv2 << PLL_FRAC_CTRL2_POSTDIV2_SHIFT);
pll_writel(pll, val, PLL_CTRL2);

- while (!(pll_readl(pll, PLL_STATUS) & PLL_STATUS_LOCK))
- cpu_relax();
+ pll_lock(pll);

if (!was_enabled)
pll_gf40lp_frac_disable(hw);
@@ -288,8 +293,7 @@ static int pll_gf40lp_laint_set_rate(struct clk_hw *hw, unsigned long rate,
(params->postdiv2 << PLL_INT_CTRL1_POSTDIV2_SHIFT);
pll_writel(pll, val, PLL_CTRL1);

- while (!(pll_readl(pll, PLL_STATUS) & PLL_STATUS_LOCK))
- cpu_relax();
+ pll_lock(pll);

if (!was_enabled)
pll_gf40lp_laint_disable(hw);
--
2.3.3

2015-05-26 22:08:24

by Ezequiel Garcia

[permalink] [raw]
Subject: [PATCH 2/3] clk: pistachio: Lock the PLL when enabled upon rate change

Currently, when the rate is changed, the driver makes sure the
PLL is enabled before doing so. This is done because the PLL
cannot be locked while disabled. Once locked, the drivers
returns the PLL to its previous enable/disable state.

This is a bit cumbersome, and can be simplified.

This commit reworks the .set_rate() functions for the integer
and fractional PLLs. Upon rate change, the PLL is now locked
only if it's already enabled.

Also, the driver locks the PLL on .enable(). This makes sure
the PLL is locked when enabled, and not locked when disabled.

Signed-off-by: Andrew Bresticker <[email protected]>
Signed-off-by: Ezequiel Garcia <[email protected]>
---
drivers/clk/pistachio/clk-pll.c | 28 ++++++++++------------------
1 file changed, 10 insertions(+), 18 deletions(-)

diff --git a/drivers/clk/pistachio/clk-pll.c b/drivers/clk/pistachio/clk-pll.c
index 9ce1be7..f12d520 100644
--- a/drivers/clk/pistachio/clk-pll.c
+++ b/drivers/clk/pistachio/clk-pll.c
@@ -130,6 +130,8 @@ static int pll_gf40lp_frac_enable(struct clk_hw *hw)
val &= ~PLL_FRAC_CTRL4_BYPASS;
pll_writel(pll, val, PLL_CTRL4);

+ pll_lock(pll);
+
return 0;
}

@@ -155,17 +157,13 @@ static int pll_gf40lp_frac_set_rate(struct clk_hw *hw, unsigned long rate,
{
struct pistachio_clk_pll *pll = to_pistachio_pll(hw);
struct pistachio_pll_rate_table *params;
- bool was_enabled;
+ int enabled = pll_gf40lp_frac_is_enabled(hw);
u32 val;

params = pll_get_params(pll, parent_rate, rate);
if (!params)
return -EINVAL;

- was_enabled = pll_gf40lp_frac_is_enabled(hw);
- if (!was_enabled)
- pll_gf40lp_frac_enable(hw);
-
val = pll_readl(pll, PLL_CTRL1);
val &= ~((PLL_CTRL1_REFDIV_MASK << PLL_CTRL1_REFDIV_SHIFT) |
(PLL_CTRL1_FBDIV_MASK << PLL_CTRL1_FBDIV_SHIFT));
@@ -184,10 +182,8 @@ static int pll_gf40lp_frac_set_rate(struct clk_hw *hw, unsigned long rate,
(params->postdiv2 << PLL_FRAC_CTRL2_POSTDIV2_SHIFT);
pll_writel(pll, val, PLL_CTRL2);

- pll_lock(pll);
-
- if (!was_enabled)
- pll_gf40lp_frac_disable(hw);
+ if (enabled)
+ pll_lock(pll);

return 0;
}
@@ -246,6 +242,8 @@ static int pll_gf40lp_laint_enable(struct clk_hw *hw)
val &= ~PLL_INT_CTRL2_BYPASS;
pll_writel(pll, val, PLL_CTRL2);

+ pll_lock(pll);
+
return 0;
}

@@ -271,17 +269,13 @@ static int pll_gf40lp_laint_set_rate(struct clk_hw *hw, unsigned long rate,
{
struct pistachio_clk_pll *pll = to_pistachio_pll(hw);
struct pistachio_pll_rate_table *params;
- bool was_enabled;
+ int enabled = pll_gf40lp_laint_is_enabled(hw);
u32 val;

params = pll_get_params(pll, parent_rate, rate);
if (!params)
return -EINVAL;

- was_enabled = pll_gf40lp_laint_is_enabled(hw);
- if (!was_enabled)
- pll_gf40lp_laint_enable(hw);
-
val = pll_readl(pll, PLL_CTRL1);
val &= ~((PLL_CTRL1_REFDIV_MASK << PLL_CTRL1_REFDIV_SHIFT) |
(PLL_CTRL1_FBDIV_MASK << PLL_CTRL1_FBDIV_SHIFT) |
@@ -293,10 +287,8 @@ static int pll_gf40lp_laint_set_rate(struct clk_hw *hw, unsigned long rate,
(params->postdiv2 << PLL_INT_CTRL1_POSTDIV2_SHIFT);
pll_writel(pll, val, PLL_CTRL1);

- pll_lock(pll);
-
- if (!was_enabled)
- pll_gf40lp_laint_disable(hw);
+ if (enabled)
+ pll_lock(pll);

return 0;
}
--
2.3.3

2015-05-26 22:06:56

by Ezequiel Garcia

[permalink] [raw]
Subject: [PATCH 3/3] clk: pistachio: Add sanity checks on PLL configuration

From: Kevin Cernekee <[email protected]>

When setting the PLL rates, check that:

- VCO is within range
- PFD is within range
- PLL is disabled when postdiv is changed
- postdiv2 <= postdiv1

Reviewed-by: Andrew Bresticker <[email protected]>
Signed-off-by: Kevin Cernekee <[email protected]>
Signed-off-by: Ezequiel Garcia <[email protected]>
---
drivers/clk/pistachio/clk-pll.c | 83 +++++++++++++++++++++++++++++++++++++++--
1 file changed, 79 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/pistachio/clk-pll.c b/drivers/clk/pistachio/clk-pll.c
index f12d520..e17dada 100644
--- a/drivers/clk/pistachio/clk-pll.c
+++ b/drivers/clk/pistachio/clk-pll.c
@@ -6,9 +6,12 @@
* version 2, as published by the Free Software Foundation.
*/

+#define pr_fmt(fmt) "%s: " fmt, __func__
+
#include <linux/clk-provider.h>
#include <linux/io.h>
#include <linux/kernel.h>
+#include <linux/printk.h>
#include <linux/slab.h>

#include "clk.h"
@@ -50,6 +53,18 @@
#define PLL_CTRL4 0x10
#define PLL_FRAC_CTRL4_BYPASS BIT(28)

+#define MIN_PFD 9600000UL
+#define MIN_VCO_LA 400000000UL
+#define MAX_VCO_LA 1600000000UL
+#define MIN_VCO_FRAC_INT 600000000UL
+#define MAX_VCO_FRAC_INT 1600000000UL
+#define MIN_VCO_FRAC_FRAC 600000000UL
+#define MAX_VCO_FRAC_FRAC 2400000000UL
+#define MIN_OUTPUT_LA 8000000UL
+#define MAX_OUTPUT_LA 1600000000UL
+#define MIN_OUTPUT_FRAC 12000000UL
+#define MAX_OUTPUT_FRAC 1600000000UL
+
struct pistachio_clk_pll {
struct clk_hw hw;
void __iomem *base;
@@ -158,12 +173,29 @@ static int pll_gf40lp_frac_set_rate(struct clk_hw *hw, unsigned long rate,
struct pistachio_clk_pll *pll = to_pistachio_pll(hw);
struct pistachio_pll_rate_table *params;
int enabled = pll_gf40lp_frac_is_enabled(hw);
- u32 val;
+ u32 val, vco, old_postdiv1, old_postdiv2;
+ const char *name = __clk_get_name(hw->clk);
+
+ if (rate < MIN_OUTPUT_FRAC || rate > MAX_OUTPUT_FRAC)
+ return -EINVAL;

params = pll_get_params(pll, parent_rate, rate);
- if (!params)
+ if (!params || !params->refdiv)
return -EINVAL;

+ vco = params->fref * params->fbdiv / params->refdiv;
+ if (vco < MIN_VCO_FRAC_FRAC || vco > MAX_VCO_FRAC_FRAC)
+ pr_warn("%s: VCO %u is out of range %lu..%lu\n", name, vco,
+ MIN_VCO_FRAC_FRAC, MAX_VCO_FRAC_FRAC);
+
+ val = params->fref / params->refdiv;
+ if (val < MIN_PFD)
+ pr_warn("%s: PFD %u is too low (min %lu)\n",
+ name, val, MIN_PFD);
+ if (val > vco / 16)
+ pr_warn("%s: PFD %u is too high (max %u)\n",
+ name, val, vco / 16);
+
val = pll_readl(pll, PLL_CTRL1);
val &= ~((PLL_CTRL1_REFDIV_MASK << PLL_CTRL1_REFDIV_SHIFT) |
(PLL_CTRL1_FBDIV_MASK << PLL_CTRL1_FBDIV_SHIFT));
@@ -172,6 +204,19 @@ static int pll_gf40lp_frac_set_rate(struct clk_hw *hw, unsigned long rate,
pll_writel(pll, val, PLL_CTRL1);

val = pll_readl(pll, PLL_CTRL2);
+
+ old_postdiv1 = (val >> PLL_FRAC_CTRL2_POSTDIV1_SHIFT) &
+ PLL_FRAC_CTRL2_POSTDIV1_MASK;
+ old_postdiv2 = (val >> PLL_FRAC_CTRL2_POSTDIV2_SHIFT) &
+ PLL_FRAC_CTRL2_POSTDIV2_MASK;
+ if (enabled &&
+ (params->postdiv1 != old_postdiv1 ||
+ params->postdiv2 != old_postdiv2))
+ pr_warn("%s: changing postdiv while PLL is enabled\n", name);
+
+ if (params->postdiv2 > params->postdiv1)
+ pr_warn("%s: postdiv2 should not exceed postdiv1\n", name);
+
val &= ~((PLL_FRAC_CTRL2_FRAC_MASK << PLL_FRAC_CTRL2_FRAC_SHIFT) |
(PLL_FRAC_CTRL2_POSTDIV1_MASK <<
PLL_FRAC_CTRL2_POSTDIV1_SHIFT) |
@@ -270,13 +315,43 @@ static int pll_gf40lp_laint_set_rate(struct clk_hw *hw, unsigned long rate,
struct pistachio_clk_pll *pll = to_pistachio_pll(hw);
struct pistachio_pll_rate_table *params;
int enabled = pll_gf40lp_laint_is_enabled(hw);
- u32 val;
+ u32 val, vco, old_postdiv1, old_postdiv2;
+ const char *name = __clk_get_name(hw->clk);
+
+ if (rate < MIN_OUTPUT_LA || rate > MAX_OUTPUT_LA)
+ return -EINVAL;

params = pll_get_params(pll, parent_rate, rate);
- if (!params)
+ if (!params || !params->refdiv)
return -EINVAL;

+ vco = params->fref * params->fbdiv / params->refdiv;
+ if (vco < MIN_VCO_LA || vco > MAX_VCO_LA)
+ pr_warn("%s: VCO %u is out of range %lu..%lu\n", name, vco,
+ MIN_VCO_LA, MAX_VCO_LA);
+
+ val = params->fref / params->refdiv;
+ if (val < MIN_PFD)
+ pr_warn("%s: PFD %u is too low (min %lu)\n",
+ name, val, MIN_PFD);
+ if (val > vco / 16)
+ pr_warn("%s: PFD %u is too high (max %u)\n",
+ name, val, vco / 16);
+
val = pll_readl(pll, PLL_CTRL1);
+
+ old_postdiv1 = (val >> PLL_INT_CTRL1_POSTDIV1_SHIFT) &
+ PLL_INT_CTRL1_POSTDIV1_MASK;
+ old_postdiv2 = (val >> PLL_INT_CTRL1_POSTDIV2_SHIFT) &
+ PLL_INT_CTRL1_POSTDIV2_MASK;
+ if (enabled &&
+ (params->postdiv1 != old_postdiv1 ||
+ params->postdiv2 != old_postdiv2))
+ pr_warn("%s: changing postdiv while PLL is enabled\n", name);
+
+ if (params->postdiv2 > params->postdiv1)
+ pr_warn("%s: postdiv2 should not exceed postdiv1\n", name);
+
val &= ~((PLL_CTRL1_REFDIV_MASK << PLL_CTRL1_REFDIV_SHIFT) |
(PLL_CTRL1_FBDIV_MASK << PLL_CTRL1_FBDIV_SHIFT) |
(PLL_INT_CTRL1_POSTDIV1_MASK << PLL_INT_CTRL1_POSTDIV1_SHIFT) |
--
2.3.3

2015-06-04 19:44:27

by Stephen Boyd

[permalink] [raw]
Subject: Re: [PATCH 1/3] clk: pistachio: Add a pll_lock() helper for clarity

On 05/26, Ezequiel Garcia wrote:
> This commit adds a pll_lock() helper making the code more readable.
> Cosmetic change only, no functionality changes.
>
> Signed-off-by: Andrew Bresticker <[email protected]>
> Signed-off-by: Ezequiel Garcia <[email protected]>
> ---

Applied to clk-next.

> drivers/clk/pistachio/clk-pll.c | 12 ++++++++----
> 1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/clk/pistachio/clk-pll.c b/drivers/clk/pistachio/clk-pll.c
> index de53756..9ce1be7 100644
> --- a/drivers/clk/pistachio/clk-pll.c
> +++ b/drivers/clk/pistachio/clk-pll.c
> @@ -67,6 +67,12 @@ static inline void pll_writel(struct pistachio_clk_pll *pll, u32 val, u32 reg)
> writel(val, pll->base + reg);
> }
>
> +static inline void pll_lock(struct pistachio_clk_pll *pll)
> +{
> + while (!(pll_readl(pll, PLL_STATUS) & PLL_STATUS_LOCK))
> + cpu_relax();

The patch is fine, but I wonder if we shouldn't have a timeout
here in case the PLL fails to lock.

--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

2015-06-04 19:47:42

by Stephen Boyd

[permalink] [raw]
Subject: Re: [PATCH 2/3] clk: pistachio: Lock the PLL when enabled upon rate change

On 05/26, Ezequiel Garcia wrote:
> Currently, when the rate is changed, the driver makes sure the
> PLL is enabled before doing so. This is done because the PLL
> cannot be locked while disabled. Once locked, the drivers
> returns the PLL to its previous enable/disable state.
>
> This is a bit cumbersome, and can be simplified.
>
> This commit reworks the .set_rate() functions for the integer
> and fractional PLLs. Upon rate change, the PLL is now locked
> only if it's already enabled.
>
> Also, the driver locks the PLL on .enable(). This makes sure
> the PLL is locked when enabled, and not locked when disabled.
>
> Signed-off-by: Andrew Bresticker <[email protected]>
> Signed-off-by: Ezequiel Garcia <[email protected]>
> ---
> drivers/clk/pistachio/clk-pll.c | 28 ++++++++++------------------
> 1 file changed, 10 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/clk/pistachio/clk-pll.c b/drivers/clk/pistachio/clk-pll.c
> index 9ce1be7..f12d520 100644
> --- a/drivers/clk/pistachio/clk-pll.c
> +++ b/drivers/clk/pistachio/clk-pll.c
> @@ -130,6 +130,8 @@ static int pll_gf40lp_frac_enable(struct clk_hw *hw)
> val &= ~PLL_FRAC_CTRL4_BYPASS;
> pll_writel(pll, val, PLL_CTRL4);
>
> + pll_lock(pll);
> +
> return 0;
> }
>
> @@ -155,17 +157,13 @@ static int pll_gf40lp_frac_set_rate(struct clk_hw *hw, unsigned long rate,
> {
> struct pistachio_clk_pll *pll = to_pistachio_pll(hw);
> struct pistachio_pll_rate_table *params;
> - bool was_enabled;
> + int enabled = pll_gf40lp_frac_is_enabled(hw);

Is there any sort of spinlock here so that we protect the
sleeping set_rate() path against the non-sleeping enable/disable
path? There should be a spinlock of some kind to prevent that,
or the enable/disable for the PLL should move to
prepare/unprepare so that we can't disable the PLL in the middle
of a rate switch.

This is an existing problem though, so I applied this to clk-next
anyway.

--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

2015-06-04 19:48:47

by Stephen Boyd

[permalink] [raw]
Subject: Re: [PATCH 3/3] clk: pistachio: Add sanity checks on PLL configuration

On 05/26, Ezequiel Garcia wrote:
> From: Kevin Cernekee <[email protected]>
>
> When setting the PLL rates, check that:
>
> - VCO is within range
> - PFD is within range
> - PLL is disabled when postdiv is changed
> - postdiv2 <= postdiv1
>
> Reviewed-by: Andrew Bresticker <[email protected]>
> Signed-off-by: Kevin Cernekee <[email protected]>
> Signed-off-by: Ezequiel Garcia <[email protected]>
> ---

Applied to clk-next

--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project