diff mbox

[2/3] clk: samsung: cpu: prepare for adding Exynos5433 CPU clocks

Message ID 1464095957-25851-3-git-send-email-b.zolnierkie@samsung.com (mailing list archive)
State New, archived
Headers show

Commit Message

Bartlomiej Zolnierkiewicz May 24, 2016, 1:19 p.m. UTC
Exynos5433 uses different register layout for CPU clock registers
than earlier SoCs so add new code for handling this layout.  Also
add new CLK_CPU_HAS_E5433_REGS_LAYOUT flag to request using it.

There should be no functional change resulting from this patch.

Cc: Kukjin Kim <kgene@kernel.org>
CC: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
---
 drivers/clk/samsung/clk-cpu.c | 131 +++++++++++++++++++++++++++++++++++++++++-
 drivers/clk/samsung/clk-cpu.h |   4 +-
 2 files changed, 133 insertions(+), 2 deletions(-)

Comments

Krzysztof Kozlowski May 25, 2016, 8:31 a.m. UTC | #1
On 05/24/2016 03:19 PM, Bartlomiej Zolnierkiewicz wrote:
> Exynos5433 uses different register layout for CPU clock registers
> than earlier SoCs so add new code for handling this layout.  Also
> add new CLK_CPU_HAS_E5433_REGS_LAYOUT flag to request using it.
> 
> There should be no functional change resulting from this patch.
> 
> Cc: Kukjin Kim <kgene@kernel.org>
> CC: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> ---
>  drivers/clk/samsung/clk-cpu.c | 131 +++++++++++++++++++++++++++++++++++++++++-
>  drivers/clk/samsung/clk-cpu.h |   4 +-
>  2 files changed, 133 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clk/samsung/clk-cpu.c b/drivers/clk/samsung/clk-cpu.c
> index 813003d..8bf7e80 100644
> --- a/drivers/clk/samsung/clk-cpu.c
> +++ b/drivers/clk/samsung/clk-cpu.c
> @@ -45,6 +45,13 @@
>  #define E4210_DIV_STAT_CPU0	0x400
>  #define E4210_DIV_STAT_CPU1	0x404
>  
> +#define E5433_MUX_SEL2		0x008
> +#define E5433_MUX_STAT2		0x208
> +#define E5433_DIV_CPU0		0x400
> +#define E5433_DIV_CPU1		0x404
> +#define E5433_DIV_STAT_CPU0	0x500
> +#define E5433_DIV_STAT_CPU1	0x504

I got a problem matching it with datasheed. The base is 0x200?

> +
>  #define E4210_DIV0_RATIO0_MASK	0x7
>  #define E4210_DIV1_HPM_MASK	(0x7 << 4)
>  #define E4210_DIV1_COPY_MASK	(0x7 << 0)
> @@ -253,6 +260,102 @@ static int exynos_cpuclk_post_rate_change(struct clk_notifier_data *ndata,
>  }
>  
>  /*
> + * Helper function to set the 'safe' dividers for the CPU clock. The parameters
> + * div and mask contain the divider value and the register bit mask of the
> + * dividers to be programmed.
> + */
> +static void exynos5433_set_safe_div(void __iomem *base, unsigned long div,
> +					unsigned long mask)

Please align the argument.

> +{
> +	unsigned long div0;
> +
> +	div0 = readl(base + E5433_DIV_CPU0);
> +	div0 = (div0 & ~mask) | (div & mask);
> +	writel(div0, base + E5433_DIV_CPU0);
> +	wait_until_divider_stable(base + E5433_DIV_STAT_CPU0, mask);
> +}
> +
> +/* handler for pre-rate change notification from parent clock */
> +static int exynos5433_cpuclk_pre_rate_change(struct clk_notifier_data *ndata,
> +			struct exynos_cpuclk *cpuclk, void __iomem *base)
> +{
> +	const struct exynos_cpuclk_cfg_data *cfg_data = cpuclk->cfg;
> +	unsigned long alt_prate = clk_get_rate(cpuclk->alt_parent);
> +	unsigned long alt_div = 0, alt_div_mask = DIV_MASK;
> +	unsigned long div0, div1 = 0, mux_reg;
> +	unsigned long flags;
> +
> +	/* find out the divider values to use for clock data */
> +	while ((cfg_data->prate * 1000) != ndata->new_rate) {
> +		if (cfg_data->prate == 0)
> +			return -EINVAL;
> +		cfg_data++;
> +	}
> +
> +	spin_lock_irqsave(cpuclk->lock, flags);
> +
> +	/*
> +	 * For the selected PLL clock frequency, get the pre-defined divider
> +	 * values.
> +	 */
> +	div0 = cfg_data->div0;
> +	div1 = cfg_data->div1;
> +
> +	/*
> +	 * If the old parent clock speed is less than the clock speed of
> +	 * the alternate parent, then it should be ensured that at no point
> +	 * the armclk speed is more than the old_prate until the dividers are
> +	 * set.  Also workaround the issue of the dividers being set to lower
> +	 * values before the parent clock speed is set to new lower speed
> +	 * (this can result in too high speed of armclk output clocks).

In entire sentence: s/speed/rate/

> +	 */
> +	if (alt_prate > ndata->old_rate || ndata->old_rate > ndata->new_rate) {
> +		unsigned long tmp_rate = min(ndata->old_rate, ndata->new_rate);
> +
> +		alt_div = DIV_ROUND_UP(alt_prate, tmp_rate) - 1;
> +		WARN_ON(alt_div >= MAX_DIV);
> +
> +		exynos5433_set_safe_div(base, alt_div, alt_div_mask);
> +		div0 |= alt_div;
> +	}
> +
> +	/* select the alternate parent */
> +	mux_reg = readl(base + E5433_MUX_SEL2);
> +	writel(mux_reg | 1, base + E5433_MUX_SEL2);
> +	wait_until_mux_stable(base + E5433_MUX_STAT2, 0, 2);
> +
> +	/* alternate parent is active now. set the dividers */
> +	writel(div0, base + E5433_DIV_CPU0);
> +	wait_until_divider_stable(base + E5433_DIV_STAT_CPU0, DIV_MASK_ALL);
> +
> +	writel(div1, base + E5433_DIV_CPU1);
> +	wait_until_divider_stable(base + E5433_DIV_STAT_CPU1, DIV_MASK_ALL);
> +
> +	spin_unlock_irqrestore(cpuclk->lock, flags);

One blank line please.

> +	return 0;
> +}
> +
> +/* handler for post-rate change notification from parent clock */
> +static int exynos5433_cpuclk_post_rate_change(struct clk_notifier_data *ndata,
> +			struct exynos_cpuclk *cpuclk, void __iomem *base)
> +{
> +	unsigned long div = 0, div_mask = DIV_MASK;
> +	unsigned long mux_reg;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(cpuclk->lock, flags);
> +
> +	/* select apll as the alternate parent */
> +	mux_reg = readl(base + E5433_MUX_SEL2);
> +	writel(mux_reg & ~1, base + E5433_MUX_SEL2);
> +	wait_until_mux_stable(base + E5433_MUX_STAT2, 0, 1);
> +
> +	exynos5433_set_safe_div(base, div, div_mask);
> +	spin_unlock_irqrestore(cpuclk->lock, flags);

One blank line please.

> +	return 0;
> +}
> +
> +/*
>   * This notifier function is called for the pre-rate and post-rate change
>   * notifications of the parent clock of cpuclk.
>   */
> @@ -275,6 +378,29 @@ static int exynos_cpuclk_notifier_cb(struct notifier_block *nb,
>  	return notifier_from_errno(err);
>  }
>  
> +/*
> + * This notifier function is called for the pre-rate and post-rate change
> + * notifications of the parent clock of cpuclk.
> + */
> +static int exynos5433_cpuclk_notifier_cb(struct notifier_block *nb,
> +				unsigned long event, void *data)
> +{
> +	struct clk_notifier_data *ndata = data;
> +	struct exynos_cpuclk *cpuclk;
> +	void __iomem *base;
> +	int err = 0;
> +
> +	cpuclk = container_of(nb, struct exynos_cpuclk, clk_nb);
> +	base = cpuclk->ctrl_base;
> +
> +	if (event == PRE_RATE_CHANGE)
> +		err = exynos5433_cpuclk_pre_rate_change(ndata, cpuclk, base);
> +	else if (event == POST_RATE_CHANGE)
> +		err = exynos5433_cpuclk_post_rate_change(ndata, cpuclk, base);
> +
> +	return notifier_from_errno(err);
> +}

Entire function is a duplication of exynos_cpuclk_notifier_cb(), except
the {pre,post}_rate_change. How about checking the flags here and using
only one notifier? Overall the code should be smaller...

Another, more robust solution would be to define new members in:
struct exynos_cpuclk {
	int (pre_rate_change) *(....);
	int (post_rate_change) *(....);
}
This way the notifier_cb would be exactly the same for all implementations.

Best regards,
Krzysztof

> +
>  /* helper function to register a CPU clock */
>  int __init exynos_register_cpu_clock(struct samsung_clk_provider *ctx,
>  		unsigned int lookup_id, const char *name, const char *parent,
> @@ -301,7 +427,10 @@ int __init exynos_register_cpu_clock(struct samsung_clk_provider *ctx,
>  	cpuclk->ctrl_base = ctx->reg_base + offset;
>  	cpuclk->lock = &ctx->lock;
>  	cpuclk->flags = flags;
> -	cpuclk->clk_nb.notifier_call = exynos_cpuclk_notifier_cb;
> +	if (flags & CLK_CPU_HAS_E5433_REGS_LAYOUT)
> +		cpuclk->clk_nb.notifier_call = exynos5433_cpuclk_notifier_cb;
> +	else
> +		cpuclk->clk_nb.notifier_call = exynos_cpuclk_notifier_cb;
>  
>  	cpuclk->alt_parent = __clk_lookup(alt_parent);
>  	if (!cpuclk->alt_parent) {
> diff --git a/drivers/clk/samsung/clk-cpu.h b/drivers/clk/samsung/clk-cpu.h
> index 37874d3..d4b6b51 100644
> --- a/drivers/clk/samsung/clk-cpu.h
> +++ b/drivers/clk/samsung/clk-cpu.h
> @@ -57,10 +57,12 @@ struct exynos_cpuclk {
>  	struct notifier_block			clk_nb;
>  	unsigned long				flags;
>  
> -/* The CPU clock registers has DIV1 configuration register */
> +/* The CPU clock registers have DIV1 configuration register */
>  #define CLK_CPU_HAS_DIV1		(1 << 0)
>  /* When ALT parent is active, debug clocks need safe divider values */
>  #define CLK_CPU_NEEDS_DEBUG_ALT_DIV	(1 << 1)
> +/* The CPU clock registers have Exynos5433-compatible layout */
> +#define CLK_CPU_HAS_E5433_REGS_LAYOUT	(1 << 2)
>  };
>  
>  extern int __init exynos_register_cpu_clock(struct samsung_clk_provider *ctx,
>
Tomasz Figa June 18, 2016, 2:53 p.m. UTC | #2
Hi Bart,

2016-05-24 22:19 GMT+09:00 Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>:
> Exynos5433 uses different register layout for CPU clock registers
> than earlier SoCs so add new code for handling this layout.  Also
> add new CLK_CPU_HAS_E5433_REGS_LAYOUT flag to request using it.

Have you considered abstracting this? Comparing existing code with
newly added one, the differences don't really seem to be that huge and
it looks like there is more common code than different, except maybe
some numeric constants that could be put into a struct.

Best regards,
Tomasz
Bartlomiej Zolnierkiewicz June 20, 2016, 1:54 p.m. UTC | #3
On Saturday, June 18, 2016 11:53:36 PM Tomasz Figa wrote:
> Hi Bart,

Hi Tomek,

> 2016-05-24 22:19 GMT+09:00 Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>:
> > Exynos5433 uses different register layout for CPU clock registers
> > than earlier SoCs so add new code for handling this layout.  Also
> > add new CLK_CPU_HAS_E5433_REGS_LAYOUT flag to request using it.
> 
> Have you considered abstracting this? Comparing existing code with

Yes, I have considered abstracting this but I've decided that there is
sufficient number of differences to justify adding new code.

> newly added one, the differences don't really seem to be that huge and
> it looks like there is more common code than different, except maybe
> some numeric constants that could be put into a struct.

It is not only that, HPM clock sourcing checking would also need to be
abstracted somehow.  Anyway there is not a lot of new code and it so
much easier to follow by not being shared that I would prefer to keep
it that way.

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
diff mbox

Patch

diff --git a/drivers/clk/samsung/clk-cpu.c b/drivers/clk/samsung/clk-cpu.c
index 813003d..8bf7e80 100644
--- a/drivers/clk/samsung/clk-cpu.c
+++ b/drivers/clk/samsung/clk-cpu.c
@@ -45,6 +45,13 @@ 
 #define E4210_DIV_STAT_CPU0	0x400
 #define E4210_DIV_STAT_CPU1	0x404
 
+#define E5433_MUX_SEL2		0x008
+#define E5433_MUX_STAT2		0x208
+#define E5433_DIV_CPU0		0x400
+#define E5433_DIV_CPU1		0x404
+#define E5433_DIV_STAT_CPU0	0x500
+#define E5433_DIV_STAT_CPU1	0x504
+
 #define E4210_DIV0_RATIO0_MASK	0x7
 #define E4210_DIV1_HPM_MASK	(0x7 << 4)
 #define E4210_DIV1_COPY_MASK	(0x7 << 0)
@@ -253,6 +260,102 @@  static int exynos_cpuclk_post_rate_change(struct clk_notifier_data *ndata,
 }
 
 /*
+ * Helper function to set the 'safe' dividers for the CPU clock. The parameters
+ * div and mask contain the divider value and the register bit mask of the
+ * dividers to be programmed.
+ */
+static void exynos5433_set_safe_div(void __iomem *base, unsigned long div,
+					unsigned long mask)
+{
+	unsigned long div0;
+
+	div0 = readl(base + E5433_DIV_CPU0);
+	div0 = (div0 & ~mask) | (div & mask);
+	writel(div0, base + E5433_DIV_CPU0);
+	wait_until_divider_stable(base + E5433_DIV_STAT_CPU0, mask);
+}
+
+/* handler for pre-rate change notification from parent clock */
+static int exynos5433_cpuclk_pre_rate_change(struct clk_notifier_data *ndata,
+			struct exynos_cpuclk *cpuclk, void __iomem *base)
+{
+	const struct exynos_cpuclk_cfg_data *cfg_data = cpuclk->cfg;
+	unsigned long alt_prate = clk_get_rate(cpuclk->alt_parent);
+	unsigned long alt_div = 0, alt_div_mask = DIV_MASK;
+	unsigned long div0, div1 = 0, mux_reg;
+	unsigned long flags;
+
+	/* find out the divider values to use for clock data */
+	while ((cfg_data->prate * 1000) != ndata->new_rate) {
+		if (cfg_data->prate == 0)
+			return -EINVAL;
+		cfg_data++;
+	}
+
+	spin_lock_irqsave(cpuclk->lock, flags);
+
+	/*
+	 * For the selected PLL clock frequency, get the pre-defined divider
+	 * values.
+	 */
+	div0 = cfg_data->div0;
+	div1 = cfg_data->div1;
+
+	/*
+	 * If the old parent clock speed is less than the clock speed of
+	 * the alternate parent, then it should be ensured that at no point
+	 * the armclk speed is more than the old_prate until the dividers are
+	 * set.  Also workaround the issue of the dividers being set to lower
+	 * values before the parent clock speed is set to new lower speed
+	 * (this can result in too high speed of armclk output clocks).
+	 */
+	if (alt_prate > ndata->old_rate || ndata->old_rate > ndata->new_rate) {
+		unsigned long tmp_rate = min(ndata->old_rate, ndata->new_rate);
+
+		alt_div = DIV_ROUND_UP(alt_prate, tmp_rate) - 1;
+		WARN_ON(alt_div >= MAX_DIV);
+
+		exynos5433_set_safe_div(base, alt_div, alt_div_mask);
+		div0 |= alt_div;
+	}
+
+	/* select the alternate parent */
+	mux_reg = readl(base + E5433_MUX_SEL2);
+	writel(mux_reg | 1, base + E5433_MUX_SEL2);
+	wait_until_mux_stable(base + E5433_MUX_STAT2, 0, 2);
+
+	/* alternate parent is active now. set the dividers */
+	writel(div0, base + E5433_DIV_CPU0);
+	wait_until_divider_stable(base + E5433_DIV_STAT_CPU0, DIV_MASK_ALL);
+
+	writel(div1, base + E5433_DIV_CPU1);
+	wait_until_divider_stable(base + E5433_DIV_STAT_CPU1, DIV_MASK_ALL);
+
+	spin_unlock_irqrestore(cpuclk->lock, flags);
+	return 0;
+}
+
+/* handler for post-rate change notification from parent clock */
+static int exynos5433_cpuclk_post_rate_change(struct clk_notifier_data *ndata,
+			struct exynos_cpuclk *cpuclk, void __iomem *base)
+{
+	unsigned long div = 0, div_mask = DIV_MASK;
+	unsigned long mux_reg;
+	unsigned long flags;
+
+	spin_lock_irqsave(cpuclk->lock, flags);
+
+	/* select apll as the alternate parent */
+	mux_reg = readl(base + E5433_MUX_SEL2);
+	writel(mux_reg & ~1, base + E5433_MUX_SEL2);
+	wait_until_mux_stable(base + E5433_MUX_STAT2, 0, 1);
+
+	exynos5433_set_safe_div(base, div, div_mask);
+	spin_unlock_irqrestore(cpuclk->lock, flags);
+	return 0;
+}
+
+/*
  * This notifier function is called for the pre-rate and post-rate change
  * notifications of the parent clock of cpuclk.
  */
@@ -275,6 +378,29 @@  static int exynos_cpuclk_notifier_cb(struct notifier_block *nb,
 	return notifier_from_errno(err);
 }
 
+/*
+ * This notifier function is called for the pre-rate and post-rate change
+ * notifications of the parent clock of cpuclk.
+ */
+static int exynos5433_cpuclk_notifier_cb(struct notifier_block *nb,
+				unsigned long event, void *data)
+{
+	struct clk_notifier_data *ndata = data;
+	struct exynos_cpuclk *cpuclk;
+	void __iomem *base;
+	int err = 0;
+
+	cpuclk = container_of(nb, struct exynos_cpuclk, clk_nb);
+	base = cpuclk->ctrl_base;
+
+	if (event == PRE_RATE_CHANGE)
+		err = exynos5433_cpuclk_pre_rate_change(ndata, cpuclk, base);
+	else if (event == POST_RATE_CHANGE)
+		err = exynos5433_cpuclk_post_rate_change(ndata, cpuclk, base);
+
+	return notifier_from_errno(err);
+}
+
 /* helper function to register a CPU clock */
 int __init exynos_register_cpu_clock(struct samsung_clk_provider *ctx,
 		unsigned int lookup_id, const char *name, const char *parent,
@@ -301,7 +427,10 @@  int __init exynos_register_cpu_clock(struct samsung_clk_provider *ctx,
 	cpuclk->ctrl_base = ctx->reg_base + offset;
 	cpuclk->lock = &ctx->lock;
 	cpuclk->flags = flags;
-	cpuclk->clk_nb.notifier_call = exynos_cpuclk_notifier_cb;
+	if (flags & CLK_CPU_HAS_E5433_REGS_LAYOUT)
+		cpuclk->clk_nb.notifier_call = exynos5433_cpuclk_notifier_cb;
+	else
+		cpuclk->clk_nb.notifier_call = exynos_cpuclk_notifier_cb;
 
 	cpuclk->alt_parent = __clk_lookup(alt_parent);
 	if (!cpuclk->alt_parent) {
diff --git a/drivers/clk/samsung/clk-cpu.h b/drivers/clk/samsung/clk-cpu.h
index 37874d3..d4b6b51 100644
--- a/drivers/clk/samsung/clk-cpu.h
+++ b/drivers/clk/samsung/clk-cpu.h
@@ -57,10 +57,12 @@  struct exynos_cpuclk {
 	struct notifier_block			clk_nb;
 	unsigned long				flags;
 
-/* The CPU clock registers has DIV1 configuration register */
+/* The CPU clock registers have DIV1 configuration register */
 #define CLK_CPU_HAS_DIV1		(1 << 0)
 /* When ALT parent is active, debug clocks need safe divider values */
 #define CLK_CPU_NEEDS_DEBUG_ALT_DIV	(1 << 1)
+/* The CPU clock registers have Exynos5433-compatible layout */
+#define CLK_CPU_HAS_E5433_REGS_LAYOUT	(1 << 2)
 };
 
 extern int __init exynos_register_cpu_clock(struct samsung_clk_provider *ctx,