diff mbox series

[11/16] clk: samsung: Keep register offsets in chip specific structure

Message ID 20240216223245.12273-12-semen.protsenko@linaro.org (mailing list archive)
State New, archived
Headers show
Series clk: samsung: Add CPU clocks for Exynos850 | expand

Commit Message

Sam Protsenko Feb. 16, 2024, 10:32 p.m. UTC 
Abstract CPU clock registers by keeping their offsets in a dedicated
chip specific structure to accommodate for oncoming Exynos850 support,
which has different offsets for cluster 0 and cluster 1. This rework
also makes it possible to use exynos_set_safe_div() for all chips, so
exynos5433_set_safe_div() is removed here to reduce the code
duplication.

No functional change.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
 drivers/clk/samsung/clk-cpu.c | 156 +++++++++++++++++++---------------
 1 file changed, 86 insertions(+), 70 deletions(-)

Comments

Krzysztof Kozlowski Feb. 20, 2024, 11:04 a.m. UTC | #1 
On 16/02/2024 23:32, Sam Protsenko wrote:
> Abstract CPU clock registers by keeping their offsets in a dedicated
> chip specific structure to accommodate for oncoming Exynos850 support,
> which has different offsets for cluster 0 and cluster 1. This rework
> also makes it possible to use exynos_set_safe_div() for all chips, so
> exynos5433_set_safe_div() is removed here to reduce the code
> duplication.
> 

So that's the answer why you could not use flags anymore - you need an
enum, not a bitmap. Such short explanation should be in previous commits
justifying moving reg layout to new property.

> No functional change.
> 
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> ---
>  drivers/clk/samsung/clk-cpu.c | 156 +++++++++++++++++++---------------
>  1 file changed, 86 insertions(+), 70 deletions(-)
> 
> diff --git a/drivers/clk/samsung/clk-cpu.c b/drivers/clk/samsung/clk-cpu.c
> index 04394d2166c9..744b609c222d 100644
> --- a/drivers/clk/samsung/clk-cpu.c
> +++ b/drivers/clk/samsung/clk-cpu.c
> @@ -44,12 +44,14 @@ typedef int (*exynos_rate_change_fn_t)(struct clk_notifier_data *ndata,
>  
>  /**
>   * struct exynos_cpuclk_chip - Chip specific data for CPU clock
> + * @regs: register offsets for CPU related clocks
>   * @pre_rate_cb: callback to run before CPU clock rate change
>   * @post_rate_cb: callback to run after CPU clock rate change
>   */
>  struct exynos_cpuclk_chip {
> -	exynos_rate_change_fn_t	pre_rate_cb;
> -	exynos_rate_change_fn_t	post_rate_cb;
> +	const void				* const regs;

Why this is void?

> +	exynos_rate_change_fn_t			pre_rate_cb;
> +	exynos_rate_change_fn_t			post_rate_cb;
>  };
>  



Best regards,
Krzysztof
Sam Protsenko Feb. 22, 2024, 12:42 a.m. UTC | #2 
On Tue, Feb 20, 2024 at 5:04 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 16/02/2024 23:32, Sam Protsenko wrote:
> > Abstract CPU clock registers by keeping their offsets in a dedicated
> > chip specific structure to accommodate for oncoming Exynos850 support,
> > which has different offsets for cluster 0 and cluster 1. This rework
> > also makes it possible to use exynos_set_safe_div() for all chips, so
> > exynos5433_set_safe_div() is removed here to reduce the code
> > duplication.
> >
>
> So that's the answer why you could not use flags anymore - you need an
> enum, not a bitmap. Such short explanation should be in previous commits
> justifying moving reg layout to new property.

Will do, thanks.

>
> > No functional change.
> >
> > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> > ---
> >  drivers/clk/samsung/clk-cpu.c | 156 +++++++++++++++++++---------------
> >  1 file changed, 86 insertions(+), 70 deletions(-)
> >
> > diff --git a/drivers/clk/samsung/clk-cpu.c b/drivers/clk/samsung/clk-cpu.c
> > index 04394d2166c9..744b609c222d 100644
> > --- a/drivers/clk/samsung/clk-cpu.c
> > +++ b/drivers/clk/samsung/clk-cpu.c
> > @@ -44,12 +44,14 @@ typedef int (*exynos_rate_change_fn_t)(struct clk_notifier_data *ndata,
> >
> >  /**
> >   * struct exynos_cpuclk_chip - Chip specific data for CPU clock
> > + * @regs: register offsets for CPU related clocks
> >   * @pre_rate_cb: callback to run before CPU clock rate change
> >   * @post_rate_cb: callback to run after CPU clock rate change
> >   */
> >  struct exynos_cpuclk_chip {
> > -     exynos_rate_change_fn_t pre_rate_cb;
> > -     exynos_rate_change_fn_t post_rate_cb;
> > +     const void                              * const regs;
>
> Why this is void?
>

Different chips can have very different register layout. For example,
older Exynos chips usually keep multiple CPU divider ratios in one
single register, whereas more modern chips have a dedicated register
for each divider clock. Also, old chips usually split divider ratio vs
DIV clock status between different registers, but in modern chips they
both live in one single register. Having (void *) makes it possible to
keep pointers to different structures, and each function for the
particular chip can "know" which exactly structure is stored there,
casting (void *) to a needed type. Another way to do that would be to
have "one-size-fits-all" structure with all possible registers for all
possible chips. I don't know, I just didn't like that for a couple of
reasons, so decided to go with (void *).

I'll add some explanation in the commit message in v2.

> > +     exynos_rate_change_fn_t                 pre_rate_cb;
> > +     exynos_rate_change_fn_t                 post_rate_cb;
> >  };
> >
>
>
>
> Best regards,
> Krzysztof
>
Krzysztof Kozlowski Feb. 22, 2024, 7:47 a.m. UTC | #3 
On 22/02/2024 01:42, Sam Protsenko wrote:
> On Tue, Feb 20, 2024 at 5:04 AM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 16/02/2024 23:32, Sam Protsenko wrote:
>>> Abstract CPU clock registers by keeping their offsets in a dedicated
>>> chip specific structure to accommodate for oncoming Exynos850 support,
>>> which has different offsets for cluster 0 and cluster 1. This rework
>>> also makes it possible to use exynos_set_safe_div() for all chips, so
>>> exynos5433_set_safe_div() is removed here to reduce the code
>>> duplication.
>>>
>>
>> So that's the answer why you could not use flags anymore - you need an
>> enum, not a bitmap. Such short explanation should be in previous commits
>> justifying moving reg layout to new property.
> 
> Will do, thanks.
> 
>>
>>> No functional change.
>>>
>>> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
>>> ---
>>>  drivers/clk/samsung/clk-cpu.c | 156 +++++++++++++++++++---------------
>>>  1 file changed, 86 insertions(+), 70 deletions(-)
>>>
>>> diff --git a/drivers/clk/samsung/clk-cpu.c b/drivers/clk/samsung/clk-cpu.c
>>> index 04394d2166c9..744b609c222d 100644
>>> --- a/drivers/clk/samsung/clk-cpu.c
>>> +++ b/drivers/clk/samsung/clk-cpu.c
>>> @@ -44,12 +44,14 @@ typedef int (*exynos_rate_change_fn_t)(struct clk_notifier_data *ndata,
>>>
>>>  /**
>>>   * struct exynos_cpuclk_chip - Chip specific data for CPU clock
>>> + * @regs: register offsets for CPU related clocks
>>>   * @pre_rate_cb: callback to run before CPU clock rate change
>>>   * @post_rate_cb: callback to run after CPU clock rate change
>>>   */
>>>  struct exynos_cpuclk_chip {
>>> -     exynos_rate_change_fn_t pre_rate_cb;
>>> -     exynos_rate_change_fn_t post_rate_cb;
>>> +     const void                              * const regs;
>>
>> Why this is void?
>>
> 
> Different chips can have very different register layout. For example,
> older Exynos chips usually keep multiple CPU divider ratios in one
> single register, whereas more modern chips have a dedicated register
> for each divider clock. Also, old chips usually split divider ratio vs
> DIV clock status between different registers, but in modern chips they
> both live in one single register. Having (void *) makes it possible to
> keep pointers to different structures, and each function for the
> particular chip can "know" which exactly structure is stored there,
> casting (void *) to a needed type. Another way to do that would be to
> have "one-size-fits-all" structure with all possible registers for all
> possible chips. I don't know, I just didn't like that for a couple of
> reasons, so decided to go with (void *).
> 
> I'll add some explanation in the commit message in v2.

Currently the one-size-fits-all seems feasible, even if few fields are
not matching, so I would prefer to go this approach.

Best regards,
Krzysztof
Sam Protsenko Feb. 24, 2024, 7:57 p.m. UTC | #4 
On Thu, Feb 22, 2024 at 1:47 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 22/02/2024 01:42, Sam Protsenko wrote:
> > On Tue, Feb 20, 2024 at 5:04 AM Krzysztof Kozlowski
> > <krzysztof.kozlowski@linaro.org> wrote:
> >>
> >> On 16/02/2024 23:32, Sam Protsenko wrote:
> >>> Abstract CPU clock registers by keeping their offsets in a dedicated
> >>> chip specific structure to accommodate for oncoming Exynos850 support,
> >>> which has different offsets for cluster 0 and cluster 1. This rework
> >>> also makes it possible to use exynos_set_safe_div() for all chips, so
> >>> exynos5433_set_safe_div() is removed here to reduce the code
> >>> duplication.
> >>>
> >>
> >> So that's the answer why you could not use flags anymore - you need an
> >> enum, not a bitmap. Such short explanation should be in previous commits
> >> justifying moving reg layout to new property.
> >
> > Will do, thanks.
> >
> >>
> >>> No functional change.
> >>>
> >>> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> >>> ---
> >>>  drivers/clk/samsung/clk-cpu.c | 156 +++++++++++++++++++---------------
> >>>  1 file changed, 86 insertions(+), 70 deletions(-)
> >>>
> >>> diff --git a/drivers/clk/samsung/clk-cpu.c b/drivers/clk/samsung/clk-cpu.c
> >>> index 04394d2166c9..744b609c222d 100644
> >>> --- a/drivers/clk/samsung/clk-cpu.c
> >>> +++ b/drivers/clk/samsung/clk-cpu.c
> >>> @@ -44,12 +44,14 @@ typedef int (*exynos_rate_change_fn_t)(struct clk_notifier_data *ndata,
> >>>
> >>>  /**
> >>>   * struct exynos_cpuclk_chip - Chip specific data for CPU clock
> >>> + * @regs: register offsets for CPU related clocks
> >>>   * @pre_rate_cb: callback to run before CPU clock rate change
> >>>   * @post_rate_cb: callback to run after CPU clock rate change
> >>>   */
> >>>  struct exynos_cpuclk_chip {
> >>> -     exynos_rate_change_fn_t pre_rate_cb;
> >>> -     exynos_rate_change_fn_t post_rate_cb;
> >>> +     const void                              * const regs;
> >>
> >> Why this is void?
> >>
> >
> > Different chips can have very different register layout. For example,
> > older Exynos chips usually keep multiple CPU divider ratios in one
> > single register, whereas more modern chips have a dedicated register
> > for each divider clock. Also, old chips usually split divider ratio vs
> > DIV clock status between different registers, but in modern chips they
> > both live in one single register. Having (void *) makes it possible to
> > keep pointers to different structures, and each function for the
> > particular chip can "know" which exactly structure is stored there,
> > casting (void *) to a needed type. Another way to do that would be to
> > have "one-size-fits-all" structure with all possible registers for all
> > possible chips. I don't know, I just didn't like that for a couple of
> > reasons, so decided to go with (void *).
> >
> > I'll add some explanation in the commit message in v2.
>
> Currently the one-size-fits-all seems feasible, even if few fields are
> not matching, so I would prefer to go this approach.
>

Sure, no problem. Will fix it in v3.

> Best regards,
> Krzysztof
>
diff mbox series

Patch

diff --git a/drivers/clk/samsung/clk-cpu.c b/drivers/clk/samsung/clk-cpu.c
index 04394d2166c9..744b609c222d 100644
--- a/drivers/clk/samsung/clk-cpu.c
+++ b/drivers/clk/samsung/clk-cpu.c
@@ -44,12 +44,14 @@  typedef int (*exynos_rate_change_fn_t)(struct clk_notifier_data *ndata,
 
 /**
  * struct exynos_cpuclk_chip - Chip specific data for CPU clock
+ * @regs: register offsets for CPU related clocks
  * @pre_rate_cb: callback to run before CPU clock rate change
  * @post_rate_cb: callback to run after CPU clock rate change
  */
 struct exynos_cpuclk_chip {
-	exynos_rate_change_fn_t	pre_rate_cb;
-	exynos_rate_change_fn_t	post_rate_cb;
+	const void				* const regs;
+	exynos_rate_change_fn_t			pre_rate_cb;
+	exynos_rate_change_fn_t			post_rate_cb;
 };
 
 /**
@@ -90,6 +92,24 @@  struct exynos_cpuclk {
 #define DIV_MASK_ALL		GENMASK(31, 0)
 #define MUX_MASK		GENMASK(2, 0)
 
+/**
+ * struct exynos_cpuclk_regs - Register offsets for CPU related clocks
+ * @mux_sel: offset of CPU MUX_SEL register (for selecting MUX clock parent)
+ * @mux_stat: offset of CPU MUX_STAT register (for checking MUX clock status)
+ * @div_cpu0: offset of CPU DIV0 register (for modifying divider values)
+ * @div_cpu1: offset of CPU DIV1 register (for modifying divider values)
+ * @div_stat_cpu0: offset of CPU DIV0_STAT register (for checking DIV status)
+ * @div_stat_cpu1: offset of CPU DIV1_STAT register (for checking DIV status)
+ */
+struct exynos_cpuclk_regs {
+	u32 mux_sel;
+	u32 mux_stat;
+	u32 div_cpu0;
+	u32 div_cpu1;
+	u32 div_stat_cpu0;
+	u32 div_stat_cpu1;
+};
+
 /*
  * Helper function to wait until divider(s) have stabilized after the divider
  * value has changed.
@@ -129,43 +149,48 @@  static void wait_until_mux_stable(void __iomem *mux_reg, u32 mux_pos,
 	pr_err("%s: re-parenting mux timed-out\n", __func__);
 }
 
-/* ---- Exynos 3/4/5 -------------------------------------------------------- */
-
-#define E4210_SRC_CPU		0x200
-#define E4210_STAT_CPU		0x400
-#define E4210_DIV_CPU0		0x500
-#define E4210_DIV_CPU1		0x504
-#define E4210_DIV_STAT_CPU0	0x600
-#define E4210_DIV_STAT_CPU1	0x604
-
-#define E4210_DIV0_RATIO0_MASK	GENMASK(2, 0)
-#define E4210_DIV1_HPM_MASK	GENMASK(6, 4)
-#define E4210_DIV1_COPY_MASK	GENMASK(2, 0)
-#define E4210_MUX_HPM_MASK	BIT(20)
-#define E4210_DIV0_ATB_SHIFT	16
-#define E4210_DIV0_ATB_MASK	(DIV_MASK << E4210_DIV0_ATB_SHIFT)
-
 /*
  * 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 exynos_set_safe_div(void __iomem *base, unsigned long div,
+static void exynos_set_safe_div(struct exynos_cpuclk *cpuclk, unsigned long div,
 				unsigned long mask)
 {
+	const struct exynos_cpuclk_regs * const regs = cpuclk->chip->regs;
+	void __iomem *base = cpuclk->ctrl_base;
 	unsigned long div0;
 
-	div0 = readl(base + E4210_DIV_CPU0);
+	div0 = readl(base + regs->div_cpu0);
 	div0 = (div0 & ~mask) | (div & mask);
-	writel(div0, base + E4210_DIV_CPU0);
-	wait_until_divider_stable(base + E4210_DIV_STAT_CPU0, mask);
+	writel(div0, base + regs->div_cpu0);
+	wait_until_divider_stable(base + regs->div_stat_cpu0, mask);
 }
 
+/* ---- Exynos 3/4/5 -------------------------------------------------------- */
+
+#define E4210_DIV0_RATIO0_MASK	GENMASK(2, 0)
+#define E4210_DIV1_HPM_MASK	GENMASK(6, 4)
+#define E4210_DIV1_COPY_MASK	GENMASK(2, 0)
+#define E4210_MUX_HPM_MASK	BIT(20)
+#define E4210_DIV0_ATB_SHIFT	16
+#define E4210_DIV0_ATB_MASK	(DIV_MASK << E4210_DIV0_ATB_SHIFT)
+
+static const struct exynos_cpuclk_regs e4210_cpuclk_regs = {
+	.mux_sel	= 0x200,
+	.mux_stat	= 0x400,
+	.div_cpu0	= 0x500,
+	.div_cpu1	= 0x504,
+	.div_stat_cpu0	= 0x600,
+	.div_stat_cpu1	= 0x604,
+};
+
 /* handler for pre-rate change notification from parent clock */
 static int exynos_cpuclk_pre_rate_change(struct clk_notifier_data *ndata,
 					 struct exynos_cpuclk *cpuclk)
 {
 	const struct exynos_cpuclk_cfg_data *cfg_data = cpuclk->cfg;
+	const struct exynos_cpuclk_regs * const regs = cpuclk->chip->regs;
 	void __iomem *base = cpuclk->ctrl_base;
 	unsigned long alt_prate = clk_hw_get_rate(cpuclk->alt_parent);
 	unsigned long div0, div1 = 0, mux_reg;
@@ -188,8 +213,8 @@  static int exynos_cpuclk_pre_rate_change(struct clk_notifier_data *ndata,
 	div0 = cfg_data->div0;
 	if (cpuclk->flags & CLK_CPU_HAS_DIV1) {
 		div1 = cfg_data->div1;
-		if (readl(base + E4210_SRC_CPU) & E4210_MUX_HPM_MASK)
-			div1 = readl(base + E4210_DIV_CPU1) &
+		if (readl(base + regs->mux_sel) & E4210_MUX_HPM_MASK)
+			div1 = readl(base + regs->div_cpu1) &
 				(E4210_DIV1_HPM_MASK | E4210_DIV1_COPY_MASK);
 	}
 
@@ -216,22 +241,22 @@  static int exynos_cpuclk_pre_rate_change(struct clk_notifier_data *ndata,
 			alt_div |= E4210_DIV0_ATB_MASK;
 			alt_div_mask |= E4210_DIV0_ATB_MASK;
 		}
-		exynos_set_safe_div(base, alt_div, alt_div_mask);
+		exynos_set_safe_div(cpuclk, alt_div, alt_div_mask);
 		div0 |= alt_div;
 	}
 
 	/* select sclk_mpll as the alternate parent */
-	mux_reg = readl(base + E4210_SRC_CPU);
-	writel(mux_reg | (1 << 16), base + E4210_SRC_CPU);
-	wait_until_mux_stable(base + E4210_STAT_CPU, 16, 2);
+	mux_reg = readl(base + regs->mux_sel);
+	writel(mux_reg | (1 << 16), base + regs->mux_sel);
+	wait_until_mux_stable(base + regs->mux_stat, 16, 2);
 
 	/* alternate parent is active now. set the dividers */
-	writel(div0, base + E4210_DIV_CPU0);
-	wait_until_divider_stable(base + E4210_DIV_STAT_CPU0, DIV_MASK_ALL);
+	writel(div0, base + regs->div_cpu0);
+	wait_until_divider_stable(base + regs->div_stat_cpu0, DIV_MASK_ALL);
 
 	if (cpuclk->flags & CLK_CPU_HAS_DIV1) {
-		writel(div1, base + E4210_DIV_CPU1);
-		wait_until_divider_stable(base + E4210_DIV_STAT_CPU1,
+		writel(div1, base + regs->div_cpu1);
+		wait_until_divider_stable(base + regs->div_stat_cpu1,
 					  DIV_MASK_ALL);
 	}
 
@@ -244,6 +269,7 @@  static int exynos_cpuclk_post_rate_change(struct clk_notifier_data *ndata,
 					  struct exynos_cpuclk *cpuclk)
 {
 	const struct exynos_cpuclk_cfg_data *cfg_data = cpuclk->cfg;
+	const struct exynos_cpuclk_regs * const regs = cpuclk->chip->regs;
 	void __iomem *base = cpuclk->ctrl_base;
 	unsigned long div = 0, div_mask = DIV_MASK;
 	unsigned long mux_reg;
@@ -261,50 +287,37 @@  static int exynos_cpuclk_post_rate_change(struct clk_notifier_data *ndata,
 	spin_lock_irqsave(cpuclk->lock, flags);
 
 	/* select mout_apll as the alternate parent */
-	mux_reg = readl(base + E4210_SRC_CPU);
-	writel(mux_reg & ~(1 << 16), base + E4210_SRC_CPU);
-	wait_until_mux_stable(base + E4210_STAT_CPU, 16, 1);
+	mux_reg = readl(base + regs->mux_sel);
+	writel(mux_reg & ~(1 << 16), base + regs->mux_sel);
+	wait_until_mux_stable(base + regs->mux_stat, 16, 1);
 
 	if (cpuclk->flags & CLK_CPU_NEEDS_DEBUG_ALT_DIV) {
 		div |= (cfg_data->div0 & E4210_DIV0_ATB_MASK);
 		div_mask |= E4210_DIV0_ATB_MASK;
 	}
 
-	exynos_set_safe_div(base, div, div_mask);
+	exynos_set_safe_div(cpuclk, div, div_mask);
 	spin_unlock_irqrestore(cpuclk->lock, flags);
 	return 0;
 }
 
 /* ---- Exynos5433 ---------------------------------------------------------- */
 
-#define E5433_MUX_SEL2		0x208
-#define E5433_MUX_STAT2		0x408
-#define E5433_DIV_CPU0		0x600
-#define E5433_DIV_CPU1		0x604
-#define E5433_DIV_STAT_CPU0	0x700
-#define E5433_DIV_STAT_CPU1	0x704
-
-/*
- * 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);
-}
+static const struct exynos_cpuclk_regs e5433_cpuclk_regs = {
+	.mux_sel	= 0x208,
+	.mux_stat	= 0x408,
+	.div_cpu0	= 0x600,
+	.div_cpu1	= 0x604,
+	.div_stat_cpu0	= 0x700,
+	.div_stat_cpu1	= 0x704,
+};
 
 /* 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)
 {
 	const struct exynos_cpuclk_cfg_data *cfg_data = cpuclk->cfg;
+	const struct exynos_cpuclk_regs * const regs = cpuclk->chip->regs;
 	void __iomem *base = cpuclk->ctrl_base;
 	unsigned long alt_prate = clk_hw_get_rate(cpuclk->alt_parent);
 	unsigned long div0, div1 = 0, mux_reg;
@@ -341,21 +354,21 @@  static int exynos5433_cpuclk_pre_rate_change(struct clk_notifier_data *ndata,
 		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);
+		exynos_set_safe_div(cpuclk, 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);
+	mux_reg = readl(base + regs->mux_sel);
+	writel(mux_reg | 1, base + regs->mux_sel);
+	wait_until_mux_stable(base + regs->mux_stat, 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(div0, base + regs->div_cpu0);
+	wait_until_divider_stable(base + regs->div_stat_cpu0, DIV_MASK_ALL);
 
-	writel(div1, base + E5433_DIV_CPU1);
-	wait_until_divider_stable(base + E5433_DIV_STAT_CPU1, DIV_MASK_ALL);
+	writel(div1, base + regs->div_cpu1);
+	wait_until_divider_stable(base + regs->div_stat_cpu1, DIV_MASK_ALL);
 
 	spin_unlock_irqrestore(cpuclk->lock, flags);
 	return 0;
@@ -365,6 +378,7 @@  static int exynos5433_cpuclk_pre_rate_change(struct clk_notifier_data *ndata,
 static int exynos5433_cpuclk_post_rate_change(struct clk_notifier_data *ndata,
 					      struct exynos_cpuclk *cpuclk)
 {
+	const struct exynos_cpuclk_regs * const regs = cpuclk->chip->regs;
 	void __iomem *base = cpuclk->ctrl_base;
 	unsigned long div = 0, div_mask = DIV_MASK;
 	unsigned long mux_reg;
@@ -373,11 +387,11 @@  static int exynos5433_cpuclk_post_rate_change(struct clk_notifier_data *ndata,
 	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);
+	mux_reg = readl(base + regs->mux_sel);
+	writel(mux_reg & ~1, base + regs->mux_sel);
+	wait_until_mux_stable(base + regs->mux_stat, 0, 1);
 
-	exynos5433_set_safe_div(base, div, div_mask);
+	exynos_set_safe_div(cpuclk, div, div_mask);
 	spin_unlock_irqrestore(cpuclk->lock, flags);
 	return 0;
 }
@@ -435,10 +449,12 @@  static int exynos_cpuclk_notifier_cb(struct notifier_block *nb,
 
 static const struct exynos_cpuclk_chip exynos_clkcpu_chips[] = {
 	[CPUCLK_LAYOUT_E4210] = {
+		.regs		= &e4210_cpuclk_regs,
 		.pre_rate_cb	= exynos_cpuclk_pre_rate_change,
 		.post_rate_cb	= exynos_cpuclk_post_rate_change,
 	},
 	[CPUCLK_LAYOUT_E5433] = {
+		.regs		= &e5433_cpuclk_regs,
 		.pre_rate_cb	= exynos5433_cpuclk_pre_rate_change,
 		.post_rate_cb	= exynos5433_cpuclk_post_rate_change,
 	},