Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1431064 > unrolled thread

[PATCH v2 06/14] ARM: sun8i: clk: Add clk-factor rate application method

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v2 06/14] ARM: sun8i: clk: Add clk-factor rate application method megous@megous.com - 2016-06-25 05:50 +0200
    Re: [PATCH v2 06/14] ARM: sun8i: clk: Add clk-factor rate  application method Maxime Ripard <maxime.ripard@free-electrons.com> - 2016-06-30 22:50 +0200
      Re: [PATCH v2 06/14] ARM: sun8i: clk: Add clk-factor rate application  method Ondřej Jirman <megous@megous.com> - 2016-07-01 03:00 +0200
        Re: [PATCH v2 06/14] ARM: sun8i: clk: Add clk-factor rate  application method Jean-Francois Moine <moinejf@free.fr> - 2016-07-01 07:40 +0200
          Re: [PATCH v2 06/14] ARM: sun8i: clk: Add clk-factor rate application  method Ondřej Jirman <megous@megous.com> - 2016-07-01 08:50 +0200
            Re: [PATCH v2 06/14] ARM: sun8i: clk: Add clk-factor rate  application method Jean-Francois Moine <moinejf@free.fr> - 2016-07-01 09:50 +0200
      Re: [PATCH v2 06/14] ARM: sun8i: clk: Add clk-factor rate application  method Ondřej Jirman <megous@megous.com> - 2016-07-01 03:00 +0200

#1431064 — [PATCH v2 06/14] ARM: sun8i: clk: Add clk-factor rate application method

From: Ondrej Jirman <megous@megous.com>

PLL1 on H3 requires special factors application algorithm,
when the rate is changed. This algorithm was extracted
from the arisc code that handles frequency scaling
in the BSP kernel.

This commit adds optional apply function to
struct factors_data, that can implement non-trivial
factors application method, when necessary.

Also struct clk_factors_config is extended with position
of the PLL lock flag.

Signed-off-by: Ondrej Jirman <megous@megous.com>
---
 Documentation/devicetree/bindings/clock/sunxi.txt |  1 +
 drivers/clk/sunxi/clk-factors.c                   | 34 +++++------
 drivers/clk/sunxi/clk-factors.h                   | 12 ++++
 drivers/clk/sunxi/clk-sunxi.c                     | 72 ++++++++++++++++++++++-
 4 files changed, 98 insertions(+), 21 deletions(-)

diff --git a/Documentation/devicetree/bindings/clock/sunxi.txt b/Documentation/devicetree/bindings/clock/sunxi.txt
index 5faae05..774500c 100644
--- a/Documentation/devicetree/bindings/clock/sunxi.txt
+++ b/Documentation/devicetree/bindings/clock/sunxi.txt
@@ -10,6 +10,7 @@ Required properties:
 	"allwinner,sun4i-a10-pll1-clk" - for the main PLL clock and PLL4
 	"allwinner,sun6i-a31-pll1-clk" - for the main PLL clock on A31
 	"allwinner,sun8i-a23-pll1-clk" - for the main PLL clock on A23
+	"allwinner,sun8i-h3-pll1-clk" - for the main PLL clock on H3
 	"allwinner,sun4i-a10-pll3-clk" - for the video PLL clock on A10
 	"allwinner,sun9i-a80-pll4-clk" - for the peripheral PLLs on A80
 	"allwinner,sun4i-a10-pll5-clk" - for the PLL5 clock
diff --git a/drivers/clk/sunxi/clk-factors.c b/drivers/clk/sunxi/clk-factors.c
index ddefe96..7c165db 100644
--- a/drivers/clk/sunxi/clk-factors.c
+++ b/drivers/clk/sunxi/clk-factors.c
@@ -34,13 +34,6 @@
 
 #define FACTORS_MAX_PARENTS		5
 
-#define SETMASK(len, pos)		(((1U << (len)) - 1) << (pos))
-#define CLRMASK(len, pos)		(~(SETMASK(len, pos)))
-#define FACTOR_GET(bit, len, reg)	(((reg) & SETMASK(len, bit)) >> (bit))
-
-#define FACTOR_SET(bit, len, reg, val) \
-	(((reg) & CLRMASK(len, bit)) | (val << (bit)))
-
 static unsigned long clk_factors_recalc_rate(struct clk_hw *hw,
 					     unsigned long parent_rate)
 {
@@ -150,20 +143,24 @@ static int clk_factors_set_rate(struct clk_hw *hw, unsigned long rate,
 	if (factors->lock)
 		spin_lock_irqsave(factors->lock, flags);
 
-	/* Fetch the register value */
-	reg = readl(factors->reg);
+	if (factors->apply) {
+		factors->apply(factors, &req);
+	} else {
+		/* Fetch the register value */
+		reg = readl(factors->reg);
 
-	/* Set up the new factors - macros do not do anything if width is 0 */
-	reg = FACTOR_SET(config->nshift, config->nwidth, reg, req.n);
-	reg = FACTOR_SET(config->kshift, config->kwidth, reg, req.k);
-	reg = FACTOR_SET(config->mshift, config->mwidth, reg, req.m);
-	reg = FACTOR_SET(config->pshift, config->pwidth, reg, req.p);
+		/* Set up the new factors - macros do not do anything if width is 0 */
+		reg = FACTOR_SET(config->nshift, config->nwidth, reg, req.n);
+		reg = FACTOR_SET(config->kshift, config->kwidth, reg, req.k);
+		reg = FACTOR_SET(config->mshift, config->mwidth, reg, req.m);
+		reg = FACTOR_SET(config->pshift, config->pwidth, reg, req.p);
 
-	/* Apply them now */
-	writel(reg, factors->reg);
+		/* Apply them now */
+		writel(reg, factors->reg);
 
-	/* delay 500us so pll stabilizes */
-	__delay((rate >> 20) * 500 / 2);
+		/* delay 500us so pll stabilizes */
+		__delay((rate >> 20) * 500 / 2);
+	}
 
 	if (factors->lock)
 		spin_unlock_irqrestore(factors->lock, flags);
@@ -213,6 +210,7 @@ struct clk *sunxi_factors_register(struct device_node *node,
 	factors->config = data->table;
 	factors->get_factors = data->getter;
 	factors->recalc = data->recalc;
+	factors->apply = data->apply;
 	factors->lock = lock;
 
 	/* Add a gate if this factor clock can be gated */
diff --git a/drivers/clk/sunxi/clk-factors.h b/drivers/clk/sunxi/clk-factors.h
index 1e63c5b..661a45a 100644
--- a/drivers/clk/sunxi/clk-factors.h
+++ b/drivers/clk/sunxi/clk-factors.h
@@ -6,6 +6,13 @@
 
 #define SUNXI_FACTORS_NOT_APPLICABLE	(0)
 
+#define SETMASK(len, pos)		(((1U << (len)) - 1) << (pos))
+#define CLRMASK(len, pos)		(~(SETMASK(len, pos)))
+#define FACTOR_GET(bit, len, reg)	(((reg) & SETMASK(len, bit)) >> (bit))
+
+#define FACTOR_SET(bit, len, reg, val) \
+	(((reg) & CLRMASK(len, bit)) | (val << (bit)))
+
 struct clk_factors_config {
 	u8 nshift;
 	u8 nwidth;
@@ -16,6 +23,7 @@ struct clk_factors_config {
 	u8 pshift;
 	u8 pwidth;
 	u8 n_start;
+	u8 lock;
 };
 
 struct factors_request {
@@ -28,6 +36,8 @@ struct factors_request {
 	u8 p;
 };
 
+struct clk_factors;
+
 struct factors_data {
 	int enable;
 	int mux;
@@ -35,6 +45,7 @@ struct factors_data {
 	const struct clk_factors_config *table;
 	void (*getter)(struct factors_request *req);
 	void (*recalc)(struct factors_request *req);
+	void (*apply)(struct clk_factors *factors, struct factors_request *req);
 	const char *name;
 };
 
@@ -44,6 +55,7 @@ struct clk_factors {
 	const struct clk_factors_config *config;
 	void (*get_factors)(struct factors_request *req);
 	void (*recalc)(struct factors_request *req);
+	void (*apply)(struct clk_factors *factors, struct factors_request *req);
 	spinlock_t *lock;
 	/* for cleanup */
 	struct clk_mux *mux;
diff --git a/drivers/clk/sunxi/clk-sunxi.c b/drivers/clk/sunxi/clk-sunxi.c
index 838b22a..e4bb908 100644
--- a/drivers/clk/sunxi/clk-sunxi.c
+++ b/drivers/clk/sunxi/clk-sunxi.c
@@ -23,6 +23,7 @@
 #include <linux/slab.h>
 #include <linux/spinlock.h>
 #include <linux/log2.h>
+#include <linux/delay.h>
 
 #include "clk-factors.h"
 
@@ -200,6 +201,56 @@ static void sun8i_a23_get_pll1_factors(struct factors_request *req)
 }
 
 /**
+ * sun8i_h3_apply_pll1_factors() - applies n, k, m, p factors to the
+ * register using an algorithm that tries to reserve the PLL lock
+ */
+
+static void sun8i_h3_apply_pll1_factors(struct clk_factors *factors, struct factors_request *req)
+{
+	const struct clk_factors_config *config = factors->config;
+	u32 reg;
+
+	/* Fetch the register value */
+	reg = readl(factors->reg);
+
+	if (FACTOR_GET(config->pshift, config->pwidth, reg) < req->p) {
+		reg = FACTOR_SET(config->pshift, config->pwidth, reg, req->p);
+
+		writel(reg, factors->reg);
+		__delay(2000);
+	}
+
+	if (FACTOR_GET(config->mshift, config->mwidth, reg) < req->m) {
+		reg = FACTOR_SET(config->mshift, config->mwidth, reg, req->m);
+
+		writel(reg, factors->reg);
+		__delay(2000);
+	}
+
+	reg = FACTOR_SET(config->nshift, config->nwidth, reg, req->n);
+	reg = FACTOR_SET(config->kshift, config->kwidth, reg, req->k);
+
+	writel(reg, factors->reg);
+	__delay(20);
+
+	while (!(readl(factors->reg) & (1 << config->lock)));
+
+	if (FACTOR_GET(config->mshift, config->mwidth, reg) > req->m) {
+		reg = FACTOR_SET(config->mshift, config->mwidth, reg, req->m);
+
+		writel(reg, factors->reg);
+		__delay(2000);
+	}
+
+	if (FACTOR_GET(config->pshift, config->pwidth, reg) > req->p) {
+		reg = FACTOR_SET(config->pshift, config->pwidth, reg, req->p);
+
+		writel(reg, factors->reg);
+		__delay(2000);
+	}
+}
+
+/**
  * sun4i_get_pll5_factors() - calculates n, k factors for PLL5
  * PLL5 rate is calculated as follows
  * rate = parent_rate * n * (k + 1)
@@ -451,6 +502,7 @@ static const struct clk_factors_config sun8i_a23_pll1_config = {
 	.pshift = 16,
 	.pwidth = 2,
 	.n_start = 1,
+	.lock = 28
 };
 
 static const struct clk_factors_config sun4i_pll5_config = {
@@ -513,6 +565,13 @@ static const struct factors_data sun8i_a23_pll1_data __initconst = {
 	.getter = sun8i_a23_get_pll1_factors,
 };
 
+static const struct factors_data sun8i_h3_pll1_data __initconst = {
+	.enable = 31,
+	.table = &sun8i_a23_pll1_config,
+	.getter = sun8i_a23_get_pll1_factors,
+	.apply = sun8i_h3_apply_pll1_factors,
+};
+
 static const struct factors_data sun7i_a20_pll4_data __initconst = {
 	.enable = 31,
 	.table = &sun4i_pll5_config,
@@ -590,12 +649,19 @@ static void __init sun6i_pll1_clk_setup(struct device_node *node)
 CLK_OF_DECLARE(sun6i_pll1, "allwinner,sun6i-a31-pll1-clk",
 	       sun6i_pll1_clk_setup);
 
-static void __init sun8i_pll1_clk_setup(struct device_node *node)
+static void __init sun8i_a23_pll1_clk_setup(struct device_node *node)
 {
 	sunxi_factors_clk_setup(node, &sun8i_a23_pll1_data);
 }
-CLK_OF_DECLARE(sun8i_pll1, "allwinner,sun8i-a23-pll1-clk",
-	       sun8i_pll1_clk_setup);
+CLK_OF_DECLARE(sun8i_a23_pll1, "allwinner,sun8i-a23-pll1-clk",
+	       sun8i_a23_pll1_clk_setup);
+
+static void __init sun8i_h3_pll1_clk_setup(struct device_node *node)
+{
+	sunxi_factors_clk_setup(node, &sun8i_h3_pll1_data);
+}
+CLK_OF_DECLARE(sun8i_h3_pll1, "allwinner,sun8i-h3-pll1-clk",
+	       sun8i_h3_pll1_clk_setup);
 
 static void __init sun7i_pll4_clk_setup(struct device_node *node)
 {
-- 
2.9.0

[toc] | [next] | [standalone]

#1434837 — Re: [PATCH v2 06/14] ARM: sun8i: clk: Add clk-factor rate application method

[Multipart message — attachments visible in raw view] — view raw

Hi,

On Sat, Jun 25, 2016 at 05:45:03AM +0200, megous@megous.com wrote:
> From: Ondrej Jirman <megous@megous.com>
> 
> PLL1 on H3 requires special factors application algorithm,
> when the rate is changed. This algorithm was extracted
> from the arisc code that handles frequency scaling
> in the BSP kernel.
> 
> This commit adds optional apply function to
> struct factors_data, that can implement non-trivial
> factors application method, when necessary.
> 
> Also struct clk_factors_config is extended with position
> of the PLL lock flag.

Have you tested the current implementation, and found that it was not
working, or did you duplicate the arisc code directly?

>  /**
> + * sun8i_h3_apply_pll1_factors() - applies n, k, m, p factors to the
> + * register using an algorithm that tries to reserve the PLL lock
> + */
> +
> +static void sun8i_h3_apply_pll1_factors(struct clk_factors *factors, struct factors_request *req)
> +{
> +	const struct clk_factors_config *config = factors->config;
> +	u32 reg;
> +
> +	/* Fetch the register value */
> +	reg = readl(factors->reg);
> +
> +	if (FACTOR_GET(config->pshift, config->pwidth, reg) < req->p) {
> +		reg = FACTOR_SET(config->pshift, config->pwidth, reg, req->p);
> +
> +		writel(reg, factors->reg);
> +		__delay(2000);
> +	}

So there was some doubts about the fact that P was being used, or at
least that it was useful.

> +	if (FACTOR_GET(config->mshift, config->mwidth, reg) < req->m) {
> +		reg = FACTOR_SET(config->mshift, config->mwidth, reg, req->m);
> +
> +		writel(reg, factors->reg);
> +		__delay(2000);
> +	}
> +
> +	reg = FACTOR_SET(config->nshift, config->nwidth, reg, req->n);
> +	reg = FACTOR_SET(config->kshift, config->kwidth, reg, req->k);
> +
> +	writel(reg, factors->reg);
> +	__delay(20);
> +
> +	while (!(readl(factors->reg) & (1 << config->lock)));

So, they are applying the dividers first, and then applying the
multipliers, and then wait for the PLL to stabilize.

> +
> +	if (FACTOR_GET(config->mshift, config->mwidth, reg) > req->m) {
> +		reg = FACTOR_SET(config->mshift, config->mwidth, reg, req->m);
> +
> +		writel(reg, factors->reg);
> +		__delay(2000);
> +	}
> +
> +	if (FACTOR_GET(config->pshift, config->pwidth, reg) > req->p) {
> +		reg = FACTOR_SET(config->pshift, config->pwidth, reg, req->p);
> +
> +		writel(reg, factors->reg);
> +		__delay(2000);
> +	}

However, this is kind of weird, why would you need to re-apply the
dividers? Nothing really changes. Have you tried without that part?

Since this is really specific, I guess you could simply make the
clk_ops for the nkmp clocks public, and just re-implement set_rate
using that logic.

You might also need to set an upper limit on P, since the last value
(4) is not a valid one.

I guess you could do that by adding a max field in the __ccu_div
structure.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[toc] | [prev] | [next] | [standalone]

#1434934 — Re: [PATCH v2 06/14] ARM: sun8i: clk: Add clk-factor rate application method

[Multipart message — attachments visible in raw view] — view raw

Hi,

On 30.6.2016 22:40, Maxime Ripard wrote:
> Hi,
> 
> On Sat, Jun 25, 2016 at 05:45:03AM +0200, megous@megous.com wrote:
>> From: Ondrej Jirman <megous@megous.com>
>>
>> PLL1 on H3 requires special factors application algorithm,
>> when the rate is changed. This algorithm was extracted
>> from the arisc code that handles frequency scaling
>> in the BSP kernel.
>>
>> This commit adds optional apply function to
>> struct factors_data, that can implement non-trivial
>> factors application method, when necessary.
>>
>> Also struct clk_factors_config is extended with position
>> of the PLL lock flag.
> 
> Have you tested the current implementation, and found that it was not
> working, or did you duplicate the arisc code directly?

I have tested the current implementation, and it was not working. It
depended on some other factors, like the initial setup done by u-boot.
It didn't work reliably.

Then I reverse engineered arisc, in an effort to see what's the
difference, between mainline and BSP code.

>>  /**
>> + * sun8i_h3_apply_pll1_factors() - applies n, k, m, p factors to the
>> + * register using an algorithm that tries to reserve the PLL lock
>> + */
>> +
>> +static void sun8i_h3_apply_pll1_factors(struct clk_factors *factors, struct factors_request *req)
>> +{
>> +	const struct clk_factors_config *config = factors->config;
>> +	u32 reg;
>> +
>> +	/* Fetch the register value */
>> +	reg = readl(factors->reg);
>> +
>> +	if (FACTOR_GET(config->pshift, config->pwidth, reg) < req->p) {
>> +		reg = FACTOR_SET(config->pshift, config->pwidth, reg, req->p);
>> +
>> +		writel(reg, factors->reg);
>> +		__delay(2000);
>> +	}
> 
> So there was some doubts about the fact that P was being used, or at
> least that it was useful.

p is necessary to reduce frequencies below 288 MHz according to the
datasheet.

>> +	if (FACTOR_GET(config->mshift, config->mwidth, reg) < req->m) {
>> +		reg = FACTOR_SET(config->mshift, config->mwidth, reg, req->m);
>> +
>> +		writel(reg, factors->reg);
>> +		__delay(2000);
>> +	}
>> +
>> +	reg = FACTOR_SET(config->nshift, config->nwidth, reg, req->n);
>> +	reg = FACTOR_SET(config->kshift, config->kwidth, reg, req->k);
>> +
>> +	writel(reg, factors->reg);
>> +	__delay(20);
>> +
>> +	while (!(readl(factors->reg) & (1 << config->lock)));
> 
> So, they are applying the dividers first, and then applying the
> multipliers, and then wait for the PLL to stabilize.

Not exactly, first we are increasing dividers if the new dividers are
higher that that what's already set. This ensures that because
application of dividers is immediate by the design of the PLL, the
application of multipliers isn't. So the VCO would still run at the same
frequency for a while gradually rising to a new value for example,
while the dividers would be reduced immediately. Leading to crash.

PLL
--------------------------
PRE DIV(f0) -> VCO(f1) -> POST DIV(f2)
   P             K,N           M

Example: (we set all factors at once, reducing dividers and multipliers
at the same time at 0ms - this should lead to no change in the output
frequency, but...)

-1ms: f0 = 24MHz, f1 = 2GHz,   f2 = 1GHz
 0ms: f0 = 24MHz, f1 = 2GHz,   f2 = 2GHz       - boom
 1ms: f0 = 24MHz, f1 = 1.5GHz, f2 = 1.5GHz
 2ms: f0 = 24MHz, f1 = 1GHz,   f2 = 1GHz

The current code crashes exactly at boom, you don't get any more
instructions to execute.

See.

So this patch first increases dividers (only if necessary), changes
multipliers and waits for change to happen (takes around 2000 cycles),
and then decreases dividers (only if necessary).

So we get:

-1ms: f0 = 24MHz, f1 = 2GHz,   f2 = 1GHz
 0ms: f0 = 24MHz, f1 = 2GHz,   f2 = 1GHz   - no boom, multiplier
                                             reduced
 1ms: f0 = 24MHz, f1 = 1.5GHz, f2 = 0.75GHz
1.9ms: f0 = 24MHz, f1 = 1GHz,   f2 = 0.5GHz - we got PLL sync
 2ms: f0 = 24MHz, f1 = 1GHz,   f2 = 1GHz   - and here we reduce divider
at last

>> +
>> +	if (FACTOR_GET(config->mshift, config->mwidth, reg) > req->m) {
>> +		reg = FACTOR_SET(config->mshift, config->mwidth, reg, req->m);
>> +
>> +		writel(reg, factors->reg);
>> +		__delay(2000);
>> +	}
>> +
>> +	if (FACTOR_GET(config->pshift, config->pwidth, reg) > req->p) {
>> +		reg = FACTOR_SET(config->pshift, config->pwidth, reg, req->p);
>> +
>> +		writel(reg, factors->reg);
>> +		__delay(2000);
>> +	}
> 
> However, this is kind of weird, why would you need to re-apply the
> dividers? Nothing really changes. Have you tried without that part?

See above, we either increase before PLL change, or reduce dividers
after the change. Nothing is re-applied.

> Since this is really specific, I guess you could simply make the
> clk_ops for the nkmp clocks public, and just re-implement set_rate
> using that logic.

I would argue that this may be necessary for other PLL clocks too, if
you can get out of bounds output frequency, by changing the dividers too
early or too late. So perhaps this code should be generalized for other
PLL clocks too, instead.

> 
> You might also need to set an upper limit on P, since the last value
> (4) is not a valid one.

I think, that should be done by the factors calculation function already.

> I guess you could do that by adding a max field in the __ccu_div
> structure.
> 
> Maxime
> 

regards,
  Ondrej

[toc] | [prev] | [next] | [standalone]

#1435026 — Re: [PATCH v2 06/14] ARM: sun8i: clk: Add clk-factor rate application method

On Fri, 1 Jul 2016 02:50:57 +0200
Ondřej Jirman <megous@megous.com> wrote:

> > Since this is really specific, I guess you could simply make the
> > clk_ops for the nkmp clocks public, and just re-implement set_rate
> > using that logic.
> 
> I would argue that this may be necessary for other PLL clocks too, if
> you can get out of bounds output frequency, by changing the dividers too
> early or too late. So perhaps this code should be generalized for other
> PLL clocks too, instead.

The documentation says that only the CPU and DDR PLLs can be dynamically
changed after boot.

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

[toc] | [prev] | [next] | [standalone]

#1435048 — Re: [PATCH v2 06/14] ARM: sun8i: clk: Add clk-factor rate application method

[Multipart message — attachments visible in raw view] — view raw

On 1.7.2016 07:37, Jean-Francois Moine wrote:
> On Fri, 1 Jul 2016 02:50:57 +0200
> Ondřej Jirman <megous@megous.com> wrote:
> 
>>> Since this is really specific, I guess you could simply make the
>>> clk_ops for the nkmp clocks public, and just re-implement set_rate
>>> using that logic.
>>
>> I would argue that this may be necessary for other PLL clocks too, if
>> you can get out of bounds output frequency, by changing the dividers too
>> early or too late. So perhaps this code should be generalized for other
>> PLL clocks too, instead.
> 
> The documentation says that only the CPU and DDR PLLs can be dynamically
> changed after boot.

The question is what exactly is meant by after boot. :) Anyway, if the
kernel has no business changing some other PLLs, if there's code for
changing them, should it be dropped?

regards,
  Ondrej

[toc] | [prev] | [next] | [standalone]

#1435092 — Re: [PATCH v2 06/14] ARM: sun8i: clk: Add clk-factor rate application method

On Fri, 1 Jul 2016 08:34:21 +0200
Ondřej Jirman <megous@megous.com> wrote:

> > The documentation says that only the CPU and DDR PLLs can be dynamically
> > changed after boot.
> 
> The question is what exactly is meant by after boot. :) Anyway, if the
> kernel has no business changing some other PLLs, if there's code for
> changing them, should it be dropped?

No, because all the other PLLs may not be initialized by the U-boot
(audio, video, gpu...), and also, their rate may be changed safely by
stopping them (gate).

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

[toc] | [prev] | [next] | [standalone]

#1434935 — Re: [PATCH v2 06/14] ARM: sun8i: clk: Add clk-factor rate application method

[Multipart message — attachments visible in raw view] — view raw

On 30.6.2016 22:40, Maxime Ripard wrote:
> Hi,
> 
> On Sat, Jun 25, 2016 at 05:45:03AM +0200, megous@megous.com wrote:
>> From: Ondrej Jirman <megous@megous.com>
>>
>> PLL1 on H3 requires special factors application algorithm,
>> when the rate is changed. This algorithm was extracted
>> from the arisc code that handles frequency scaling
>> in the BSP kernel.
>>
>> This commit adds optional apply function to
>> struct factors_data, that can implement non-trivial
>> factors application method, when necessary.
>>
>> Also struct clk_factors_config is extended with position
>> of the PLL lock flag.
> 
> Have you tested the current implementation, and found that it was not
> working, or did you duplicate the arisc code directly?

Also of note is that similar code probably doesn't crash in u-boot,
because there, before changing the PLL1 clock, the cpu is switched to
24MHz osc, so it is not overclocked, even if factors align in such a way
that you'd get the behavior I described in the other email.

>>  /**
>> + * sun8i_h3_apply_pll1_factors() - applies n, k, m, p factors to the
>> + * register using an algorithm that tries to reserve the PLL lock
>> + */
>> +
>> +static void sun8i_h3_apply_pll1_factors(struct clk_factors *factors, struct factors_request *req)
>> +{
>> +	const struct clk_factors_config *config = factors->config;
>> +	u32 reg;
>> +
>> +	/* Fetch the register value */
>> +	reg = readl(factors->reg);
>> +
>> +	if (FACTOR_GET(config->pshift, config->pwidth, reg) < req->p) {
>> +		reg = FACTOR_SET(config->pshift, config->pwidth, reg, req->p);
>> +
>> +		writel(reg, factors->reg);
>> +		__delay(2000);
>> +	}
> 
> So there was some doubts about the fact that P was being used, or at
> least that it was useful.
> 
>> +	if (FACTOR_GET(config->mshift, config->mwidth, reg) < req->m) {
>> +		reg = FACTOR_SET(config->mshift, config->mwidth, reg, req->m);
>> +
>> +		writel(reg, factors->reg);
>> +		__delay(2000);
>> +	}
>> +
>> +	reg = FACTOR_SET(config->nshift, config->nwidth, reg, req->n);
>> +	reg = FACTOR_SET(config->kshift, config->kwidth, reg, req->k);
>> +
>> +	writel(reg, factors->reg);
>> +	__delay(20);
>> +
>> +	while (!(readl(factors->reg) & (1 << config->lock)));
> 
> So, they are applying the dividers first, and then applying the
> multipliers, and then wait for the PLL to stabilize.
> 
>> +
>> +	if (FACTOR_GET(config->mshift, config->mwidth, reg) > req->m) {
>> +		reg = FACTOR_SET(config->mshift, config->mwidth, reg, req->m);
>> +
>> +		writel(reg, factors->reg);
>> +		__delay(2000);
>> +	}
>> +
>> +	if (FACTOR_GET(config->pshift, config->pwidth, reg) > req->p) {
>> +		reg = FACTOR_SET(config->pshift, config->pwidth, reg, req->p);
>> +
>> +		writel(reg, factors->reg);
>> +		__delay(2000);
>> +	}
> 
> However, this is kind of weird, why would you need to re-apply the
> dividers? Nothing really changes. Have you tried without that part?
> 
> Since this is really specific, I guess you could simply make the
> clk_ops for the nkmp clocks public, and just re-implement set_rate
> using that logic.
> 
> You might also need to set an upper limit on P, since the last value
> (4) is not a valid one.
> 
> I guess you could do that by adding a max field in the __ccu_div
> structure.
> 
> Maxime
> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web