diff mbox

[v4,5/8] clk: exynos: use cpu-clock provider type to represent arm clock.

Message ID 1400029876-5830-6-git-send-email-thomas.ab@samsung.com (mailing list archive)
State New, archived
Headers show

Commit Message

Thomas Abraham May 14, 2014, 1:11 a.m. UTC 
From: Thomas Abraham <thomas.ab@samsung.com>

With the addition of the new Samsung specific cpu-clock type, the
arm clock can be represented as a cpu-clock type and the independent
clock blocks that made up the arm clock can be removed.

Cc: Tomasz Figa <t.figa@samsung.com>
Signed-off-by: Thomas Abraham <thomas.ab@samsung.com>
---
 drivers/clk/samsung/clk-exynos4.c      |   25 +++++++++----------------
 drivers/clk/samsung/clk-exynos5250.c   |   12 ++++++------
 include/dt-bindings/clock/exynos5250.h |    1 +
 3 files changed, 16 insertions(+), 22 deletions(-)

Comments

Mike Turquette May 14, 2014, 9:37 p.m. UTC | #1 
Quoting Thomas Abraham (2014-05-13 18:11:13)
> From: Thomas Abraham <thomas.ab@samsung.com>
> 
> With the addition of the new Samsung specific cpu-clock type, the
> arm clock can be represented as a cpu-clock type and the independent
> clock blocks that made up the arm clock can be removed.

<rant>

I am not a fan of this type of "clock hiding". Certainly the design of
the CCF allows for a clock provider to obfuscate it's internals; there
was never a requirement that every clock node be exposed to Linux as a
struct clk. A truly obfuscated system could only expose leaf clocks that
are consumed by Linux device drivers, and never expose any of the
intermediary clocks in between the input clock signal (if it exists) and
the leaf nodes.

However I feel that this patch is more of a workaround to the fact that
the clock framework today does not make DVFS transitions (or
coordinated, multi-clock rate change transitions) easy to control. The
generic "walk up the tree" algorithm might give you the right rate, but
perhaps using a non-validated combination of PLL frequency and
adjustable-rate dividers, or a combination that his higher jitter or is
more likely to unlock at higher temperatures, etc.

Back in the pre-CCF days lots of folks implemented this with "virtual"
clock nodes that simply called "clk_set_rate" or whatever on the
affected clocks, or even worse just banged a bunch of registers.

The cbus clock series for Tegra is also looks a little like this.

</rant>

Thomas,

Would a coordinated clock rate change method solve this problem for you
in place of the cpu-clock provider type? A poorly conceived call graph
for this might look something like:

clk_set_rate(div_arm2, 1000000);
-> if (div_arm2->coordinated == true)
	clk_coordinate_rates(div_arm2, 1000000);
	-> clk->ops->coordinate(div_arm2->hw, 1000000);
	   -> vendor_supplied_magic()

The vendor_supplied_magic() would be a callback that essentially calls
clk_set_rate() on all of the affected (coordinated) clocks. In your case
that looks like mout_core, div_core, div_core2, arm_clk and sclk_apll
for Exynos4.

The trick is that calling clk_set_rate() from any driver would initiate
this coordinated rate change, and then the exynos clock driver would set
up the coordinated clocks itself. No new API is introduced to drivers
(since it still uses clk_set_rate) and now a new clock provider doesn't
have to be invented every time we have a couple of clocks involved in a
DVFS transition.

Does this sound right to you or have I horribly misinterpreted the point
of these clock patches?

Thanks,
Mike

> 
> Cc: Tomasz Figa <t.figa@samsung.com>
> Signed-off-by: Thomas Abraham <thomas.ab@samsung.com>
> ---
>  drivers/clk/samsung/clk-exynos4.c      |   25 +++++++++----------------
>  drivers/clk/samsung/clk-exynos5250.c   |   12 ++++++------
>  include/dt-bindings/clock/exynos5250.h |    1 +
>  3 files changed, 16 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/clk/samsung/clk-exynos4.c b/drivers/clk/samsung/clk-exynos4.c
> index b4f9672..7e3bb16c 100644
> --- a/drivers/clk/samsung/clk-exynos4.c
> +++ b/drivers/clk/samsung/clk-exynos4.c
> @@ -471,7 +471,6 @@ static struct samsung_mux_clock exynos4210_mux_clks[] __initdata = {
>         MUX(0, "mout_fimd1", group1_p4210, E4210_SRC_LCD1, 0, 4),
>         MUX(0, "mout_mipi1", group1_p4210, E4210_SRC_LCD1, 12, 4),
>         MUX(CLK_SCLK_MPLL, "sclk_mpll", mout_mpll_p, SRC_CPU, 8, 1),
> -       MUX(CLK_MOUT_CORE, "mout_core", mout_core_p4210, SRC_CPU, 16, 1),
>         MUX(CLK_SCLK_VPLL, "sclk_vpll", sclk_vpll_p4210, SRC_TOP0, 8, 1),
>         MUX(CLK_MOUT_FIMC0, "mout_fimc0", group1_p4210, SRC_CAM, 0, 4),
>         MUX(CLK_MOUT_FIMC1, "mout_fimc1", group1_p4210, SRC_CAM, 4, 4),
> @@ -530,7 +529,6 @@ static struct samsung_mux_clock exynos4x12_mux_clks[] __initdata = {
>         MUX(0, "mout_jpeg", mout_jpeg_p, E4X12_SRC_CAM1, 8, 1),
>         MUX(CLK_SCLK_MPLL, "sclk_mpll", mout_mpll_p, SRC_DMC, 12, 1),
>         MUX(CLK_SCLK_VPLL, "sclk_vpll", mout_vpll_p, SRC_TOP0, 8, 1),
> -       MUX(CLK_MOUT_CORE, "mout_core", mout_core_p4x12, SRC_CPU, 16, 1),
>         MUX(CLK_MOUT_FIMC0, "mout_fimc0", group1_p4x12, SRC_CAM, 0, 4),
>         MUX(CLK_MOUT_FIMC1, "mout_fimc1", group1_p4x12, SRC_CAM, 4, 4),
>         MUX(CLK_MOUT_FIMC2, "mout_fimc2", group1_p4x12, SRC_CAM, 8, 4),
> @@ -572,8 +570,6 @@ static struct samsung_mux_clock exynos4x12_mux_clks[] __initdata = {
>  
>  /* list of divider clocks supported in all exynos4 soc's */
>  static struct samsung_div_clock exynos4_div_clks[] __initdata = {
> -       DIV(0, "div_core", "mout_core", DIV_CPU0, 0, 3),
> -       DIV(0, "div_core2", "div_core", DIV_CPU0, 28, 3),
>         DIV(0, "div_fimc0", "mout_fimc0", DIV_CAM, 0, 4),
>         DIV(0, "div_fimc1", "mout_fimc1", DIV_CAM, 4, 4),
>         DIV(0, "div_fimc2", "mout_fimc2", DIV_CAM, 8, 4),
> @@ -619,8 +615,8 @@ static struct samsung_div_clock exynos4_div_clks[] __initdata = {
>         DIV(0, "div_spi_pre2", "div_spi2", DIV_PERIL2, 8, 8),
>         DIV(0, "div_audio1", "mout_audio1", DIV_PERIL4, 0, 4),
>         DIV(0, "div_audio2", "mout_audio2", DIV_PERIL4, 16, 4),
> -       DIV(CLK_ARM_CLK, "arm_clk", "div_core2", DIV_CPU0, 28, 3),
> -       DIV(CLK_SCLK_APLL, "sclk_apll", "mout_apll", DIV_CPU0, 24, 3),
> +       DIV_F(CLK_SCLK_APLL, "sclk_apll", "mout_apll", DIV_CPU0, 24, 3,
> +                       CLK_GET_RATE_NOCACHE, 0),
>         DIV_F(0, "div_mipi_pre0", "div_mipi0", DIV_LCD0, 20, 4,
>                         CLK_SET_RATE_PARENT, 0),
>         DIV_F(0, "div_mmc_pre0", "div_mmc0", DIV_FSYS1, 8, 8,
> @@ -1003,12 +999,6 @@ static struct samsung_gate_clock exynos4x12_gate_clks[] __initdata = {
>                 0),
>  };
>  
> -static struct samsung_clock_alias exynos4_aliases[] __initdata = {
> -       ALIAS(CLK_MOUT_CORE, NULL, "moutcore"),
> -       ALIAS(CLK_ARM_CLK, NULL, "armclk"),
> -       ALIAS(CLK_SCLK_APLL, NULL, "mout_apll"),
> -};
> -
>  static struct samsung_clock_alias exynos4210_aliases[] __initdata = {
>         ALIAS(CLK_SCLK_MPLL, NULL, "mout_mpll"),
>  };
> @@ -1241,6 +1231,9 @@ static void __init exynos4_clk_init(struct device_node *np,
>                         ARRAY_SIZE(exynos4210_gate_clks));
>                 samsung_clk_register_alias(exynos4210_aliases,
>                         ARRAY_SIZE(exynos4210_aliases));
> +               exynos_register_arm_clock(CLK_ARM_CLK, mout_core_p4210,
> +                       ARRAY_SIZE(mout_core_p4210), reg_base, np, NULL,
> +                       &samsung_clk_lock);
>         } else {
>                 samsung_clk_register_mux(exynos4x12_mux_clks,
>                         ARRAY_SIZE(exynos4x12_mux_clks));
> @@ -1250,11 +1243,11 @@ static void __init exynos4_clk_init(struct device_node *np,
>                         ARRAY_SIZE(exynos4x12_gate_clks));
>                 samsung_clk_register_alias(exynos4x12_aliases,
>                         ARRAY_SIZE(exynos4x12_aliases));
> +               exynos_register_arm_clock(CLK_ARM_CLK, mout_core_p4x12,
> +                       ARRAY_SIZE(mout_core_p4x12), reg_base, np, NULL,
> +                       &samsung_clk_lock);
>         }
>  
> -       samsung_clk_register_alias(exynos4_aliases,
> -                       ARRAY_SIZE(exynos4_aliases));
> -
>         exynos4_clk_sleep_init();
>  
>         pr_info("%s clocks: sclk_apll = %ld, sclk_mpll = %ld\n"
> @@ -1262,7 +1255,7 @@ static void __init exynos4_clk_init(struct device_node *np,
>                 exynos4_soc == EXYNOS4210 ? "Exynos4210" : "Exynos4x12",
>                 _get_rate("sclk_apll"), _get_rate("sclk_mpll"),
>                 _get_rate("sclk_epll"), _get_rate("sclk_vpll"),
> -               _get_rate("arm_clk"));
> +               _get_rate("armclk"));
>  }
>  
>  
> diff --git a/drivers/clk/samsung/clk-exynos5250.c b/drivers/clk/samsung/clk-exynos5250.c
> index e7ee442..3fe1ca0 100644
> --- a/drivers/clk/samsung/clk-exynos5250.c
> +++ b/drivers/clk/samsung/clk-exynos5250.c
> @@ -260,8 +260,6 @@ static struct samsung_mux_clock exynos5250_mux_clks[] __initdata = {
>          */
>         MUX_FA(0, "mout_apll", mout_apll_p, SRC_CPU, 0, 1,
>                                         CLK_SET_RATE_PARENT, 0, "mout_apll"),
> -       MUX_A(0, "mout_cpu", mout_cpu_p, SRC_CPU, 16, 1, "mout_cpu"),
> -
>         /*
>          * CMU_CORE
>          */
> @@ -339,9 +337,8 @@ static struct samsung_div_clock exynos5250_div_clks[] __initdata = {
>         /*
>          * CMU_CPU
>          */
> -       DIV(0, "div_arm", "mout_cpu", DIV_CPU0, 0, 3),
> -       DIV(0, "div_apll", "mout_apll", DIV_CPU0, 24, 3),
> -       DIV_A(0, "div_arm2", "div_arm", DIV_CPU0, 28, 3, "armclk"),
> +       DIV_F(0, "div_apll", "mout_apll", DIV_CPU0, 24, 3,
> +                       CLK_GET_RATE_NOCACHE, 0),
>  
>         /*
>          * CMU_TOP
> @@ -721,10 +718,13 @@ static void __init exynos5250_clk_init(struct device_node *np)
>                         ARRAY_SIZE(exynos5250_div_clks));
>         samsung_clk_register_gate(exynos5250_gate_clks,
>                         ARRAY_SIZE(exynos5250_gate_clks));
> +       exynos_register_arm_clock(CLK_ARM_CLK, mout_cpu_p,
> +                       ARRAY_SIZE(mout_cpu_p), reg_base, np, NULL,
> +                       &samsung_clk_lock);
>  
>         exynos5250_clk_sleep_init();
>  
>         pr_info("Exynos5250: clock setup completed, armclk=%ld\n",
> -                       _get_rate("div_arm2"));
> +                       _get_rate("armclk"));
>  }
>  CLK_OF_DECLARE(exynos5250_clk, "samsung,exynos5250-clock", exynos5250_clk_init);
> diff --git a/include/dt-bindings/clock/exynos5250.h b/include/dt-bindings/clock/exynos5250.h
> index 922f2dc..59a10fb 100644
> --- a/include/dt-bindings/clock/exynos5250.h
> +++ b/include/dt-bindings/clock/exynos5250.h
> @@ -21,6 +21,7 @@
>  #define CLK_FOUT_CPLL          6
>  #define CLK_FOUT_EPLL          7
>  #define CLK_FOUT_VPLL          8
> +#define CLK_ARM_CLK            12
>  
>  /* gate for special clocks (sclk) */
>  #define CLK_SCLK_CAM_BAYER     128
> -- 
> 1.7.4.4
>
Thomas Abraham May 15, 2014, 7:48 a.m. UTC | #2 
On Thu, May 15, 2014 at 3:07 AM, Mike Turquette <mturquette@linaro.org> wrote:
> Quoting Thomas Abraham (2014-05-13 18:11:13)
>> From: Thomas Abraham <thomas.ab@samsung.com>
>>
>> With the addition of the new Samsung specific cpu-clock type, the
>> arm clock can be represented as a cpu-clock type and the independent
>> clock blocks that made up the arm clock can be removed.
>
> <rant>
>
> I am not a fan of this type of "clock hiding". Certainly the design of
> the CCF allows for a clock provider to obfuscate it's internals; there
> was never a requirement that every clock node be exposed to Linux as a
> struct clk. A truly obfuscated system could only expose leaf clocks that
> are consumed by Linux device drivers, and never expose any of the
> intermediary clocks in between the input clock signal (if it exists) and
> the leaf nodes.
>
> However I feel that this patch is more of a workaround to the fact that
> the clock framework today does not make DVFS transitions (or
> coordinated, multi-clock rate change transitions) easy to control. The
> generic "walk up the tree" algorithm might give you the right rate, but
> perhaps using a non-validated combination of PLL frequency and
> adjustable-rate dividers, or a combination that his higher jitter or is
> more likely to unlock at higher temperatures, etc.
>
> Back in the pre-CCF days lots of folks implemented this with "virtual"
> clock nodes that simply called "clk_set_rate" or whatever on the
> affected clocks, or even worse just banged a bunch of registers.
>
> The cbus clock series for Tegra is also looks a little like this.
>
> </rant>
>
> Thomas,
>
> Would a coordinated clock rate change method solve this problem for you
> in place of the cpu-clock provider type? A poorly conceived call graph
> for this might look something like:
>
> clk_set_rate(div_arm2, 1000000);
> -> if (div_arm2->coordinated == true)
>         clk_coordinate_rates(div_arm2, 1000000);
>         -> clk->ops->coordinate(div_arm2->hw, 1000000);
>            -> vendor_supplied_magic()
>
> The vendor_supplied_magic() would be a callback that essentially calls
> clk_set_rate() on all of the affected (coordinated) clocks. In your case
> that looks like mout_core, div_core, div_core2, arm_clk and sclk_apll
> for Exynos4.
>
> The trick is that calling clk_set_rate() from any driver would initiate
> this coordinated rate change, and then the exynos clock driver would set
> up the coordinated clocks itself. No new API is introduced to drivers
> (since it still uses clk_set_rate) and now a new clock provider doesn't
> have to be invented every time we have a couple of clocks involved in a
> DVFS transition.
>
> Does this sound right to you or have I horribly misinterpreted the point
> of these clock patches?

Mike,

Thanks for your comments. If I have understood the coordinated clock
as described above, I believe this patch series also attempts to do
almost the same thing. The advantage of using coordinated clock over
the approach in this series is that each platform need not define a
new clock type.

In this patch series, for all exynos SoC's, a new cpu clock type was
introduced which is reusable for multi-cluster exynos SoCs as well.
The new clock type is part of the clock driver and all drivers can use
standard clock API to access this clock type instance.

So probably, all the logic for the new cpu_clk clock type introduced
in this patch would go into the vendor_supplied_magic() callback. So
it sounds like the coordinated clock approach would be helpful. Is
this something that you are already working on?

Thanks,
Thomas.

>
> Thanks,
> Mike
>
>>
>> Cc: Tomasz Figa <t.figa@samsung.com>
>> Signed-off-by: Thomas Abraham <thomas.ab@samsung.com>
>> ---
>>  drivers/clk/samsung/clk-exynos4.c      |   25 +++++++++----------------
>>  drivers/clk/samsung/clk-exynos5250.c   |   12 ++++++------
>>  include/dt-bindings/clock/exynos5250.h |    1 +
>>  3 files changed, 16 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/clk/samsung/clk-exynos4.c b/drivers/clk/samsung/clk-exynos4.c
>> index b4f9672..7e3bb16c 100644
>> --- a/drivers/clk/samsung/clk-exynos4.c
>> +++ b/drivers/clk/samsung/clk-exynos4.c
>> @@ -471,7 +471,6 @@ static struct samsung_mux_clock exynos4210_mux_clks[] __initdata = {
>>         MUX(0, "mout_fimd1", group1_p4210, E4210_SRC_LCD1, 0, 4),
>>         MUX(0, "mout_mipi1", group1_p4210, E4210_SRC_LCD1, 12, 4),
>>         MUX(CLK_SCLK_MPLL, "sclk_mpll", mout_mpll_p, SRC_CPU, 8, 1),
>> -       MUX(CLK_MOUT_CORE, "mout_core", mout_core_p4210, SRC_CPU, 16, 1),
>>         MUX(CLK_SCLK_VPLL, "sclk_vpll", sclk_vpll_p4210, SRC_TOP0, 8, 1),
>>         MUX(CLK_MOUT_FIMC0, "mout_fimc0", group1_p4210, SRC_CAM, 0, 4),
>>         MUX(CLK_MOUT_FIMC1, "mout_fimc1", group1_p4210, SRC_CAM, 4, 4),
>> @@ -530,7 +529,6 @@ static struct samsung_mux_clock exynos4x12_mux_clks[] __initdata = {
>>         MUX(0, "mout_jpeg", mout_jpeg_p, E4X12_SRC_CAM1, 8, 1),
>>         MUX(CLK_SCLK_MPLL, "sclk_mpll", mout_mpll_p, SRC_DMC, 12, 1),
>>         MUX(CLK_SCLK_VPLL, "sclk_vpll", mout_vpll_p, SRC_TOP0, 8, 1),
>> -       MUX(CLK_MOUT_CORE, "mout_core", mout_core_p4x12, SRC_CPU, 16, 1),
>>         MUX(CLK_MOUT_FIMC0, "mout_fimc0", group1_p4x12, SRC_CAM, 0, 4),
>>         MUX(CLK_MOUT_FIMC1, "mout_fimc1", group1_p4x12, SRC_CAM, 4, 4),
>>         MUX(CLK_MOUT_FIMC2, "mout_fimc2", group1_p4x12, SRC_CAM, 8, 4),
>> @@ -572,8 +570,6 @@ static struct samsung_mux_clock exynos4x12_mux_clks[] __initdata = {
>>
>>  /* list of divider clocks supported in all exynos4 soc's */
>>  static struct samsung_div_clock exynos4_div_clks[] __initdata = {
>> -       DIV(0, "div_core", "mout_core", DIV_CPU0, 0, 3),
>> -       DIV(0, "div_core2", "div_core", DIV_CPU0, 28, 3),
>>         DIV(0, "div_fimc0", "mout_fimc0", DIV_CAM, 0, 4),
>>         DIV(0, "div_fimc1", "mout_fimc1", DIV_CAM, 4, 4),
>>         DIV(0, "div_fimc2", "mout_fimc2", DIV_CAM, 8, 4),
>> @@ -619,8 +615,8 @@ static struct samsung_div_clock exynos4_div_clks[] __initdata = {
>>         DIV(0, "div_spi_pre2", "div_spi2", DIV_PERIL2, 8, 8),
>>         DIV(0, "div_audio1", "mout_audio1", DIV_PERIL4, 0, 4),
>>         DIV(0, "div_audio2", "mout_audio2", DIV_PERIL4, 16, 4),
>> -       DIV(CLK_ARM_CLK, "arm_clk", "div_core2", DIV_CPU0, 28, 3),
>> -       DIV(CLK_SCLK_APLL, "sclk_apll", "mout_apll", DIV_CPU0, 24, 3),
>> +       DIV_F(CLK_SCLK_APLL, "sclk_apll", "mout_apll", DIV_CPU0, 24, 3,
>> +                       CLK_GET_RATE_NOCACHE, 0),
>>         DIV_F(0, "div_mipi_pre0", "div_mipi0", DIV_LCD0, 20, 4,
>>                         CLK_SET_RATE_PARENT, 0),
>>         DIV_F(0, "div_mmc_pre0", "div_mmc0", DIV_FSYS1, 8, 8,
>> @@ -1003,12 +999,6 @@ static struct samsung_gate_clock exynos4x12_gate_clks[] __initdata = {
>>                 0),
>>  };
>>
>> -static struct samsung_clock_alias exynos4_aliases[] __initdata = {
>> -       ALIAS(CLK_MOUT_CORE, NULL, "moutcore"),
>> -       ALIAS(CLK_ARM_CLK, NULL, "armclk"),
>> -       ALIAS(CLK_SCLK_APLL, NULL, "mout_apll"),
>> -};
>> -
>>  static struct samsung_clock_alias exynos4210_aliases[] __initdata = {
>>         ALIAS(CLK_SCLK_MPLL, NULL, "mout_mpll"),
>>  };
>> @@ -1241,6 +1231,9 @@ static void __init exynos4_clk_init(struct device_node *np,
>>                         ARRAY_SIZE(exynos4210_gate_clks));
>>                 samsung_clk_register_alias(exynos4210_aliases,
>>                         ARRAY_SIZE(exynos4210_aliases));
>> +               exynos_register_arm_clock(CLK_ARM_CLK, mout_core_p4210,
>> +                       ARRAY_SIZE(mout_core_p4210), reg_base, np, NULL,
>> +                       &samsung_clk_lock);
>>         } else {
>>                 samsung_clk_register_mux(exynos4x12_mux_clks,
>>                         ARRAY_SIZE(exynos4x12_mux_clks));
>> @@ -1250,11 +1243,11 @@ static void __init exynos4_clk_init(struct device_node *np,
>>                         ARRAY_SIZE(exynos4x12_gate_clks));
>>                 samsung_clk_register_alias(exynos4x12_aliases,
>>                         ARRAY_SIZE(exynos4x12_aliases));
>> +               exynos_register_arm_clock(CLK_ARM_CLK, mout_core_p4x12,
>> +                       ARRAY_SIZE(mout_core_p4x12), reg_base, np, NULL,
>> +                       &samsung_clk_lock);
>>         }
>>
>> -       samsung_clk_register_alias(exynos4_aliases,
>> -                       ARRAY_SIZE(exynos4_aliases));
>> -
>>         exynos4_clk_sleep_init();
>>
>>         pr_info("%s clocks: sclk_apll = %ld, sclk_mpll = %ld\n"
>> @@ -1262,7 +1255,7 @@ static void __init exynos4_clk_init(struct device_node *np,
>>                 exynos4_soc == EXYNOS4210 ? "Exynos4210" : "Exynos4x12",
>>                 _get_rate("sclk_apll"), _get_rate("sclk_mpll"),
>>                 _get_rate("sclk_epll"), _get_rate("sclk_vpll"),
>> -               _get_rate("arm_clk"));
>> +               _get_rate("armclk"));
>>  }
>>
>>
>> diff --git a/drivers/clk/samsung/clk-exynos5250.c b/drivers/clk/samsung/clk-exynos5250.c
>> index e7ee442..3fe1ca0 100644
>> --- a/drivers/clk/samsung/clk-exynos5250.c
>> +++ b/drivers/clk/samsung/clk-exynos5250.c
>> @@ -260,8 +260,6 @@ static struct samsung_mux_clock exynos5250_mux_clks[] __initdata = {
>>          */
>>         MUX_FA(0, "mout_apll", mout_apll_p, SRC_CPU, 0, 1,
>>                                         CLK_SET_RATE_PARENT, 0, "mout_apll"),
>> -       MUX_A(0, "mout_cpu", mout_cpu_p, SRC_CPU, 16, 1, "mout_cpu"),
>> -
>>         /*
>>          * CMU_CORE
>>          */
>> @@ -339,9 +337,8 @@ static struct samsung_div_clock exynos5250_div_clks[] __initdata = {
>>         /*
>>          * CMU_CPU
>>          */
>> -       DIV(0, "div_arm", "mout_cpu", DIV_CPU0, 0, 3),
>> -       DIV(0, "div_apll", "mout_apll", DIV_CPU0, 24, 3),
>> -       DIV_A(0, "div_arm2", "div_arm", DIV_CPU0, 28, 3, "armclk"),
>> +       DIV_F(0, "div_apll", "mout_apll", DIV_CPU0, 24, 3,
>> +                       CLK_GET_RATE_NOCACHE, 0),
>>
>>         /*
>>          * CMU_TOP
>> @@ -721,10 +718,13 @@ static void __init exynos5250_clk_init(struct device_node *np)
>>                         ARRAY_SIZE(exynos5250_div_clks));
>>         samsung_clk_register_gate(exynos5250_gate_clks,
>>                         ARRAY_SIZE(exynos5250_gate_clks));
>> +       exynos_register_arm_clock(CLK_ARM_CLK, mout_cpu_p,
>> +                       ARRAY_SIZE(mout_cpu_p), reg_base, np, NULL,
>> +                       &samsung_clk_lock);
>>
>>         exynos5250_clk_sleep_init();
>>
>>         pr_info("Exynos5250: clock setup completed, armclk=%ld\n",
>> -                       _get_rate("div_arm2"));
>> +                       _get_rate("armclk"));
>>  }
>>  CLK_OF_DECLARE(exynos5250_clk, "samsung,exynos5250-clock", exynos5250_clk_init);
>> diff --git a/include/dt-bindings/clock/exynos5250.h b/include/dt-bindings/clock/exynos5250.h
>> index 922f2dc..59a10fb 100644
>> --- a/include/dt-bindings/clock/exynos5250.h
>> +++ b/include/dt-bindings/clock/exynos5250.h
>> @@ -21,6 +21,7 @@
>>  #define CLK_FOUT_CPLL          6
>>  #define CLK_FOUT_EPLL          7
>>  #define CLK_FOUT_VPLL          8
>> +#define CLK_ARM_CLK            12
>>
>>  /* gate for special clocks (sclk) */
>>  #define CLK_SCLK_CAM_BAYER     128
>> --
>> 1.7.4.4
>>
Lukasz Majewski May 15, 2014, 8:10 a.m. UTC | #3 
Hi Thomas,

> On Thu, May 15, 2014 at 3:07 AM, Mike Turquette
> <mturquette@linaro.org> wrote:
> > Quoting Thomas Abraham (2014-05-13 18:11:13)
> >> From: Thomas Abraham <thomas.ab@samsung.com>
> >>
> >> With the addition of the new Samsung specific cpu-clock type, the
> >> arm clock can be represented as a cpu-clock type and the
> >> independent clock blocks that made up the arm clock can be removed.
> >
> > <rant>
> >
> > I am not a fan of this type of "clock hiding". Certainly the design
> > of the CCF allows for a clock provider to obfuscate it's internals;
> > there was never a requirement that every clock node be exposed to
> > Linux as a struct clk. A truly obfuscated system could only expose
> > leaf clocks that are consumed by Linux device drivers, and never
> > expose any of the intermediary clocks in between the input clock
> > signal (if it exists) and the leaf nodes.
> >
> > However I feel that this patch is more of a workaround to the fact
> > that the clock framework today does not make DVFS transitions (or
> > coordinated, multi-clock rate change transitions) easy to control.
> > The generic "walk up the tree" algorithm might give you the right
> > rate, but perhaps using a non-validated combination of PLL
> > frequency and adjustable-rate dividers, or a combination that his
> > higher jitter or is more likely to unlock at higher temperatures,
> > etc.
> >
> > Back in the pre-CCF days lots of folks implemented this with
> > "virtual" clock nodes that simply called "clk_set_rate" or whatever
> > on the affected clocks, or even worse just banged a bunch of
> > registers.
> >
> > The cbus clock series for Tegra is also looks a little like this.
> >
> > </rant>
> >
> > Thomas,
> >
> > Would a coordinated clock rate change method solve this problem for
> > you in place of the cpu-clock provider type? A poorly conceived
> > call graph for this might look something like:
> >
> > clk_set_rate(div_arm2, 1000000);
> > -> if (div_arm2->coordinated == true)
> >         clk_coordinate_rates(div_arm2, 1000000);
> >         -> clk->ops->coordinate(div_arm2->hw, 1000000);
> >            -> vendor_supplied_magic()
> >
> > The vendor_supplied_magic() would be a callback that essentially
> > calls clk_set_rate() on all of the affected (coordinated) clocks.
> > In your case that looks like mout_core, div_core, div_core2,
> > arm_clk and sclk_apll for Exynos4.

I might misinterpret the idea here, but is clk_set_rate() function
ready to handle atomic change for several clocks? Especially setting
rate of dividers located in the same register, represented as different
clocks to CCF.

As fair as I remember it was not possible to serialize operations on
clocks with calling clk_set_rate() several times. Those had to be done
instantly, otherwise platform hanged.

Am I missing something, or a major improvement had I overlooked?

> >
> > The trick is that calling clk_set_rate() from any driver would
> > initiate this coordinated rate change, and then the exynos clock
> > driver would set up the coordinated clocks itself. No new API is
> > introduced to drivers (since it still uses clk_set_rate) and now a
> > new clock provider doesn't have to be invented every time we have a
> > couple of clocks involved in a DVFS transition.
> >
> > Does this sound right to you or have I horribly misinterpreted the
> > point of these clock patches?
> 
> Mike,
> 
> Thanks for your comments. If I have understood the coordinated clock
> as described above, I believe this patch series also attempts to do
> almost the same thing. The advantage of using coordinated clock over
> the approach in this series is that each platform need not define a
> new clock type.
> 
> In this patch series, for all exynos SoC's, a new cpu clock type was
> introduced which is reusable for multi-cluster exynos SoCs as well.
> The new clock type is part of the clock driver and all drivers can use
> standard clock API to access this clock type instance.
> 
> So probably, all the logic for the new cpu_clk clock type introduced
> in this patch would go into the vendor_supplied_magic() callback. So
> it sounds like the coordinated clock approach would be helpful. Is
> this something that you are already working on?
> 
> Thanks,
> Thomas.
> 
> >
> > Thanks,
> > Mike
> >
> >>
> >> Cc: Tomasz Figa <t.figa@samsung.com>
> >> Signed-off-by: Thomas Abraham <thomas.ab@samsung.com>
> >> ---
> >>  drivers/clk/samsung/clk-exynos4.c      |   25
> >> +++++++++---------------- drivers/clk/samsung/clk-exynos5250.c
> >> |   12 ++++++------ include/dt-bindings/clock/exynos5250.h |    1 +
> >>  3 files changed, 16 insertions(+), 22 deletions(-)
> >>
> >> diff --git a/drivers/clk/samsung/clk-exynos4.c
> >> b/drivers/clk/samsung/clk-exynos4.c index b4f9672..7e3bb16c 100644
> >> --- a/drivers/clk/samsung/clk-exynos4.c
> >> +++ b/drivers/clk/samsung/clk-exynos4.c
> >> @@ -471,7 +471,6 @@ static struct samsung_mux_clock
> >> exynos4210_mux_clks[] __initdata = { MUX(0, "mout_fimd1",
> >> group1_p4210, E4210_SRC_LCD1, 0, 4), MUX(0, "mout_mipi1",
> >> group1_p4210, E4210_SRC_LCD1, 12, 4), MUX(CLK_SCLK_MPLL,
> >> "sclk_mpll", mout_mpll_p, SRC_CPU, 8, 1),
> >> -       MUX(CLK_MOUT_CORE, "mout_core", mout_core_p4210, SRC_CPU,
> >> 16, 1), MUX(CLK_SCLK_VPLL, "sclk_vpll", sclk_vpll_p4210, SRC_TOP0,
> >> 8, 1), MUX(CLK_MOUT_FIMC0, "mout_fimc0", group1_p4210, SRC_CAM, 0,
> >> 4), MUX(CLK_MOUT_FIMC1, "mout_fimc1", group1_p4210, SRC_CAM, 4, 4),
> >> @@ -530,7 +529,6 @@ static struct samsung_mux_clock
> >> exynos4x12_mux_clks[] __initdata = { MUX(0, "mout_jpeg",
> >> mout_jpeg_p, E4X12_SRC_CAM1, 8, 1), MUX(CLK_SCLK_MPLL,
> >> "sclk_mpll", mout_mpll_p, SRC_DMC, 12, 1), MUX(CLK_SCLK_VPLL,
> >> "sclk_vpll", mout_vpll_p, SRC_TOP0, 8, 1),
> >> -       MUX(CLK_MOUT_CORE, "mout_core", mout_core_p4x12, SRC_CPU,
> >> 16, 1), MUX(CLK_MOUT_FIMC0, "mout_fimc0", group1_p4x12, SRC_CAM,
> >> 0, 4), MUX(CLK_MOUT_FIMC1, "mout_fimc1", group1_p4x12, SRC_CAM, 4,
> >> 4), MUX(CLK_MOUT_FIMC2, "mout_fimc2", group1_p4x12, SRC_CAM, 8, 4),
> >> @@ -572,8 +570,6 @@ static struct samsung_mux_clock
> >> exynos4x12_mux_clks[] __initdata = {
> >>
> >>  /* list of divider clocks supported in all exynos4 soc's */
> >>  static struct samsung_div_clock exynos4_div_clks[] __initdata = {
> >> -       DIV(0, "div_core", "mout_core", DIV_CPU0, 0, 3),
> >> -       DIV(0, "div_core2", "div_core", DIV_CPU0, 28, 3),
> >>         DIV(0, "div_fimc0", "mout_fimc0", DIV_CAM, 0, 4),
> >>         DIV(0, "div_fimc1", "mout_fimc1", DIV_CAM, 4, 4),
> >>         DIV(0, "div_fimc2", "mout_fimc2", DIV_CAM, 8, 4),
> >> @@ -619,8 +615,8 @@ static struct samsung_div_clock
> >> exynos4_div_clks[] __initdata = { DIV(0, "div_spi_pre2",
> >> "div_spi2", DIV_PERIL2, 8, 8), DIV(0, "div_audio1", "mout_audio1",
> >> DIV_PERIL4, 0, 4), DIV(0, "div_audio2", "mout_audio2", DIV_PERIL4,
> >> 16, 4),
> >> -       DIV(CLK_ARM_CLK, "arm_clk", "div_core2", DIV_CPU0, 28, 3),
> >> -       DIV(CLK_SCLK_APLL, "sclk_apll", "mout_apll", DIV_CPU0, 24,
> >> 3),
> >> +       DIV_F(CLK_SCLK_APLL, "sclk_apll", "mout_apll", DIV_CPU0,
> >> 24, 3,
> >> +                       CLK_GET_RATE_NOCACHE, 0),
> >>         DIV_F(0, "div_mipi_pre0", "div_mipi0", DIV_LCD0, 20, 4,
> >>                         CLK_SET_RATE_PARENT, 0),
> >>         DIV_F(0, "div_mmc_pre0", "div_mmc0", DIV_FSYS1, 8, 8,
> >> @@ -1003,12 +999,6 @@ static struct samsung_gate_clock
> >> exynos4x12_gate_clks[] __initdata = { 0),
> >>  };
> >>
> >> -static struct samsung_clock_alias exynos4_aliases[] __initdata = {
> >> -       ALIAS(CLK_MOUT_CORE, NULL, "moutcore"),
> >> -       ALIAS(CLK_ARM_CLK, NULL, "armclk"),
> >> -       ALIAS(CLK_SCLK_APLL, NULL, "mout_apll"),
> >> -};
> >> -
> >>  static struct samsung_clock_alias exynos4210_aliases[] __initdata
> >> = { ALIAS(CLK_SCLK_MPLL, NULL, "mout_mpll"),
> >>  };
> >> @@ -1241,6 +1231,9 @@ static void __init exynos4_clk_init(struct
> >> device_node *np, ARRAY_SIZE(exynos4210_gate_clks));
> >>                 samsung_clk_register_alias(exynos4210_aliases,
> >>                         ARRAY_SIZE(exynos4210_aliases));
> >> +               exynos_register_arm_clock(CLK_ARM_CLK,
> >> mout_core_p4210,
> >> +                       ARRAY_SIZE(mout_core_p4210), reg_base, np,
> >> NULL,
> >> +                       &samsung_clk_lock);
> >>         } else {
> >>                 samsung_clk_register_mux(exynos4x12_mux_clks,
> >>                         ARRAY_SIZE(exynos4x12_mux_clks));
> >> @@ -1250,11 +1243,11 @@ static void __init exynos4_clk_init(struct
> >> device_node *np, ARRAY_SIZE(exynos4x12_gate_clks));
> >>                 samsung_clk_register_alias(exynos4x12_aliases,
> >>                         ARRAY_SIZE(exynos4x12_aliases));
> >> +               exynos_register_arm_clock(CLK_ARM_CLK,
> >> mout_core_p4x12,
> >> +                       ARRAY_SIZE(mout_core_p4x12), reg_base, np,
> >> NULL,
> >> +                       &samsung_clk_lock);
> >>         }
> >>
> >> -       samsung_clk_register_alias(exynos4_aliases,
> >> -                       ARRAY_SIZE(exynos4_aliases));
> >> -
> >>         exynos4_clk_sleep_init();
> >>
> >>         pr_info("%s clocks: sclk_apll = %ld, sclk_mpll = %ld\n"
> >> @@ -1262,7 +1255,7 @@ static void __init exynos4_clk_init(struct
> >> device_node *np, exynos4_soc == EXYNOS4210 ? "Exynos4210" :
> >> "Exynos4x12", _get_rate("sclk_apll"), _get_rate("sclk_mpll"),
> >>                 _get_rate("sclk_epll"), _get_rate("sclk_vpll"),
> >> -               _get_rate("arm_clk"));
> >> +               _get_rate("armclk"));
> >>  }
> >>
> >>
> >> diff --git a/drivers/clk/samsung/clk-exynos5250.c
> >> b/drivers/clk/samsung/clk-exynos5250.c index e7ee442..3fe1ca0
> >> 100644 --- a/drivers/clk/samsung/clk-exynos5250.c
> >> +++ b/drivers/clk/samsung/clk-exynos5250.c
> >> @@ -260,8 +260,6 @@ static struct samsung_mux_clock
> >> exynos5250_mux_clks[] __initdata = { */
> >>         MUX_FA(0, "mout_apll", mout_apll_p, SRC_CPU, 0, 1,
> >>                                         CLK_SET_RATE_PARENT, 0,
> >> "mout_apll"),
> >> -       MUX_A(0, "mout_cpu", mout_cpu_p, SRC_CPU, 16, 1,
> >> "mout_cpu"), -
> >>         /*
> >>          * CMU_CORE
> >>          */
> >> @@ -339,9 +337,8 @@ static struct samsung_div_clock
> >> exynos5250_div_clks[] __initdata = { /*
> >>          * CMU_CPU
> >>          */
> >> -       DIV(0, "div_arm", "mout_cpu", DIV_CPU0, 0, 3),
> >> -       DIV(0, "div_apll", "mout_apll", DIV_CPU0, 24, 3),
> >> -       DIV_A(0, "div_arm2", "div_arm", DIV_CPU0, 28, 3, "armclk"),
> >> +       DIV_F(0, "div_apll", "mout_apll", DIV_CPU0, 24, 3,
> >> +                       CLK_GET_RATE_NOCACHE, 0),
> >>
> >>         /*
> >>          * CMU_TOP
> >> @@ -721,10 +718,13 @@ static void __init
> >> exynos5250_clk_init(struct device_node *np)
> >> ARRAY_SIZE(exynos5250_div_clks));
> >> samsung_clk_register_gate(exynos5250_gate_clks,
> >> ARRAY_SIZE(exynos5250_gate_clks));
> >> +       exynos_register_arm_clock(CLK_ARM_CLK, mout_cpu_p,
> >> +                       ARRAY_SIZE(mout_cpu_p), reg_base, np, NULL,
> >> +                       &samsung_clk_lock);
> >>
> >>         exynos5250_clk_sleep_init();
> >>
> >>         pr_info("Exynos5250: clock setup completed, armclk=%ld\n",
> >> -                       _get_rate("div_arm2"));
> >> +                       _get_rate("armclk"));
> >>  }
> >>  CLK_OF_DECLARE(exynos5250_clk, "samsung,exynos5250-clock",
> >> exynos5250_clk_init); diff --git
> >> a/include/dt-bindings/clock/exynos5250.h
> >> b/include/dt-bindings/clock/exynos5250.h index 922f2dc..59a10fb
> >> 100644 --- a/include/dt-bindings/clock/exynos5250.h +++
> >> b/include/dt-bindings/clock/exynos5250.h @@ -21,6 +21,7 @@
> >>  #define CLK_FOUT_CPLL          6
> >>  #define CLK_FOUT_EPLL          7
> >>  #define CLK_FOUT_VPLL          8
> >> +#define CLK_ARM_CLK            12
> >>
> >>  /* gate for special clocks (sclk) */
> >>  #define CLK_SCLK_CAM_BAYER     128
> >> --
> >> 1.7.4.4
> >>
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Thomas Abraham May 15, 2014, 9:59 a.m. UTC | #4 
On Thu, May 15, 2014 at 1:40 PM, Lukasz Majewski <l.majewski@samsung.com> wrote:
> Hi Thomas,
>
>> On Thu, May 15, 2014 at 3:07 AM, Mike Turquette
>> <mturquette@linaro.org> wrote:
>> > Quoting Thomas Abraham (2014-05-13 18:11:13)
>> >> From: Thomas Abraham <thomas.ab@samsung.com>
>> >>
>> >> With the addition of the new Samsung specific cpu-clock type, the
>> >> arm clock can be represented as a cpu-clock type and the
>> >> independent clock blocks that made up the arm clock can be removed.
>> >
>> > <rant>
>> >
>> > I am not a fan of this type of "clock hiding". Certainly the design
>> > of the CCF allows for a clock provider to obfuscate it's internals;
>> > there was never a requirement that every clock node be exposed to
>> > Linux as a struct clk. A truly obfuscated system could only expose
>> > leaf clocks that are consumed by Linux device drivers, and never
>> > expose any of the intermediary clocks in between the input clock
>> > signal (if it exists) and the leaf nodes.
>> >
>> > However I feel that this patch is more of a workaround to the fact
>> > that the clock framework today does not make DVFS transitions (or
>> > coordinated, multi-clock rate change transitions) easy to control.
>> > The generic "walk up the tree" algorithm might give you the right
>> > rate, but perhaps using a non-validated combination of PLL
>> > frequency and adjustable-rate dividers, or a combination that his
>> > higher jitter or is more likely to unlock at higher temperatures,
>> > etc.
>> >
>> > Back in the pre-CCF days lots of folks implemented this with
>> > "virtual" clock nodes that simply called "clk_set_rate" or whatever
>> > on the affected clocks, or even worse just banged a bunch of
>> > registers.
>> >
>> > The cbus clock series for Tegra is also looks a little like this.
>> >
>> > </rant>
>> >
>> > Thomas,
>> >
>> > Would a coordinated clock rate change method solve this problem for
>> > you in place of the cpu-clock provider type? A poorly conceived
>> > call graph for this might look something like:
>> >
>> > clk_set_rate(div_arm2, 1000000);
>> > -> if (div_arm2->coordinated == true)
>> >         clk_coordinate_rates(div_arm2, 1000000);
>> >         -> clk->ops->coordinate(div_arm2->hw, 1000000);
>> >            -> vendor_supplied_magic()
>> >
>> > The vendor_supplied_magic() would be a callback that essentially
>> > calls clk_set_rate() on all of the affected (coordinated) clocks.
>> > In your case that looks like mout_core, div_core, div_core2,
>> > arm_clk and sclk_apll for Exynos4.
>
> I might misinterpret the idea here, but is clk_set_rate() function
> ready to handle atomic change for several clocks? Especially setting
> rate of dividers located in the same register, represented as different
> clocks to CCF.
>
> As fair as I remember it was not possible to serialize operations on
> clocks with calling clk_set_rate() several times. Those had to be done
> instantly, otherwise platform hanged.
>
> Am I missing something, or a major improvement had I overlooked?

Hi Lukasz,

Not sure what Mike would suggest here, but probably the clk_set_rate()
itself might not be called for coordinated clocks. It could be some
internal clock framework function that takes care of setting rate of
coordinated clocks. I don't have a good understanding of how the
implementation for this will look like.

Thanks,
Thomas.

>
>> >
>> > The trick is that calling clk_set_rate() from any driver would
>> > initiate this coordinated rate change, and then the exynos clock
>> > driver would set up the coordinated clocks itself. No new API is
>> > introduced to drivers (since it still uses clk_set_rate) and now a
>> > new clock provider doesn't have to be invented every time we have a
>> > couple of clocks involved in a DVFS transition.
>> >
>> > Does this sound right to you or have I horribly misinterpreted the
>> > point of these clock patches?
>>
>> Mike,
>>
>> Thanks for your comments. If I have understood the coordinated clock
>> as described above, I believe this patch series also attempts to do
>> almost the same thing. The advantage of using coordinated clock over
>> the approach in this series is that each platform need not define a
>> new clock type.
>>
>> In this patch series, for all exynos SoC's, a new cpu clock type was
>> introduced which is reusable for multi-cluster exynos SoCs as well.
>> The new clock type is part of the clock driver and all drivers can use
>> standard clock API to access this clock type instance.
>>
>> So probably, all the logic for the new cpu_clk clock type introduced
>> in this patch would go into the vendor_supplied_magic() callback. So
>> it sounds like the coordinated clock approach would be helpful. Is
>> this something that you are already working on?
>>
>> Thanks,
>> Thomas.
>>
>> >
>> > Thanks,
>> > Mike
>> >
>> >>
>> >> Cc: Tomasz Figa <t.figa@samsung.com>
>> >> Signed-off-by: Thomas Abraham <thomas.ab@samsung.com>
>> >> ---
>> >>  drivers/clk/samsung/clk-exynos4.c      |   25
>> >> +++++++++---------------- drivers/clk/samsung/clk-exynos5250.c
>> >> |   12 ++++++------ include/dt-bindings/clock/exynos5250.h |    1 +
>> >>  3 files changed, 16 insertions(+), 22 deletions(-)
>> >>
>> >> diff --git a/drivers/clk/samsung/clk-exynos4.c
>> >> b/drivers/clk/samsung/clk-exynos4.c index b4f9672..7e3bb16c 100644
>> >> --- a/drivers/clk/samsung/clk-exynos4.c
>> >> +++ b/drivers/clk/samsung/clk-exynos4.c
>> >> @@ -471,7 +471,6 @@ static struct samsung_mux_clock
>> >> exynos4210_mux_clks[] __initdata = { MUX(0, "mout_fimd1",
>> >> group1_p4210, E4210_SRC_LCD1, 0, 4), MUX(0, "mout_mipi1",
>> >> group1_p4210, E4210_SRC_LCD1, 12, 4), MUX(CLK_SCLK_MPLL,
>> >> "sclk_mpll", mout_mpll_p, SRC_CPU, 8, 1),
>> >> -       MUX(CLK_MOUT_CORE, "mout_core", mout_core_p4210, SRC_CPU,
>> >> 16, 1), MUX(CLK_SCLK_VPLL, "sclk_vpll", sclk_vpll_p4210, SRC_TOP0,
>> >> 8, 1), MUX(CLK_MOUT_FIMC0, "mout_fimc0", group1_p4210, SRC_CAM, 0,
>> >> 4), MUX(CLK_MOUT_FIMC1, "mout_fimc1", group1_p4210, SRC_CAM, 4, 4),
>> >> @@ -530,7 +529,6 @@ static struct samsung_mux_clock
>> >> exynos4x12_mux_clks[] __initdata = { MUX(0, "mout_jpeg",
>> >> mout_jpeg_p, E4X12_SRC_CAM1, 8, 1), MUX(CLK_SCLK_MPLL,
>> >> "sclk_mpll", mout_mpll_p, SRC_DMC, 12, 1), MUX(CLK_SCLK_VPLL,
>> >> "sclk_vpll", mout_vpll_p, SRC_TOP0, 8, 1),
>> >> -       MUX(CLK_MOUT_CORE, "mout_core", mout_core_p4x12, SRC_CPU,
>> >> 16, 1), MUX(CLK_MOUT_FIMC0, "mout_fimc0", group1_p4x12, SRC_CAM,
>> >> 0, 4), MUX(CLK_MOUT_FIMC1, "mout_fimc1", group1_p4x12, SRC_CAM, 4,
>> >> 4), MUX(CLK_MOUT_FIMC2, "mout_fimc2", group1_p4x12, SRC_CAM, 8, 4),
>> >> @@ -572,8 +570,6 @@ static struct samsung_mux_clock
>> >> exynos4x12_mux_clks[] __initdata = {
>> >>
>> >>  /* list of divider clocks supported in all exynos4 soc's */
>> >>  static struct samsung_div_clock exynos4_div_clks[] __initdata = {
>> >> -       DIV(0, "div_core", "mout_core", DIV_CPU0, 0, 3),
>> >> -       DIV(0, "div_core2", "div_core", DIV_CPU0, 28, 3),
>> >>         DIV(0, "div_fimc0", "mout_fimc0", DIV_CAM, 0, 4),
>> >>         DIV(0, "div_fimc1", "mout_fimc1", DIV_CAM, 4, 4),
>> >>         DIV(0, "div_fimc2", "mout_fimc2", DIV_CAM, 8, 4),
>> >> @@ -619,8 +615,8 @@ static struct samsung_div_clock
>> >> exynos4_div_clks[] __initdata = { DIV(0, "div_spi_pre2",
>> >> "div_spi2", DIV_PERIL2, 8, 8), DIV(0, "div_audio1", "mout_audio1",
>> >> DIV_PERIL4, 0, 4), DIV(0, "div_audio2", "mout_audio2", DIV_PERIL4,
>> >> 16, 4),
>> >> -       DIV(CLK_ARM_CLK, "arm_clk", "div_core2", DIV_CPU0, 28, 3),
>> >> -       DIV(CLK_SCLK_APLL, "sclk_apll", "mout_apll", DIV_CPU0, 24,
>> >> 3),
>> >> +       DIV_F(CLK_SCLK_APLL, "sclk_apll", "mout_apll", DIV_CPU0,
>> >> 24, 3,
>> >> +                       CLK_GET_RATE_NOCACHE, 0),
>> >>         DIV_F(0, "div_mipi_pre0", "div_mipi0", DIV_LCD0, 20, 4,
>> >>                         CLK_SET_RATE_PARENT, 0),
>> >>         DIV_F(0, "div_mmc_pre0", "div_mmc0", DIV_FSYS1, 8, 8,
>> >> @@ -1003,12 +999,6 @@ static struct samsung_gate_clock
>> >> exynos4x12_gate_clks[] __initdata = { 0),
>> >>  };
>> >>
>> >> -static struct samsung_clock_alias exynos4_aliases[] __initdata = {
>> >> -       ALIAS(CLK_MOUT_CORE, NULL, "moutcore"),
>> >> -       ALIAS(CLK_ARM_CLK, NULL, "armclk"),
>> >> -       ALIAS(CLK_SCLK_APLL, NULL, "mout_apll"),
>> >> -};
>> >> -
>> >>  static struct samsung_clock_alias exynos4210_aliases[] __initdata
>> >> = { ALIAS(CLK_SCLK_MPLL, NULL, "mout_mpll"),
>> >>  };
>> >> @@ -1241,6 +1231,9 @@ static void __init exynos4_clk_init(struct
>> >> device_node *np, ARRAY_SIZE(exynos4210_gate_clks));
>> >>                 samsung_clk_register_alias(exynos4210_aliases,
>> >>                         ARRAY_SIZE(exynos4210_aliases));
>> >> +               exynos_register_arm_clock(CLK_ARM_CLK,
>> >> mout_core_p4210,
>> >> +                       ARRAY_SIZE(mout_core_p4210), reg_base, np,
>> >> NULL,
>> >> +                       &samsung_clk_lock);
>> >>         } else {
>> >>                 samsung_clk_register_mux(exynos4x12_mux_clks,
>> >>                         ARRAY_SIZE(exynos4x12_mux_clks));
>> >> @@ -1250,11 +1243,11 @@ static void __init exynos4_clk_init(struct
>> >> device_node *np, ARRAY_SIZE(exynos4x12_gate_clks));
>> >>                 samsung_clk_register_alias(exynos4x12_aliases,
>> >>                         ARRAY_SIZE(exynos4x12_aliases));
>> >> +               exynos_register_arm_clock(CLK_ARM_CLK,
>> >> mout_core_p4x12,
>> >> +                       ARRAY_SIZE(mout_core_p4x12), reg_base, np,
>> >> NULL,
>> >> +                       &samsung_clk_lock);
>> >>         }
>> >>
>> >> -       samsung_clk_register_alias(exynos4_aliases,
>> >> -                       ARRAY_SIZE(exynos4_aliases));
>> >> -
>> >>         exynos4_clk_sleep_init();
>> >>
>> >>         pr_info("%s clocks: sclk_apll = %ld, sclk_mpll = %ld\n"
>> >> @@ -1262,7 +1255,7 @@ static void __init exynos4_clk_init(struct
>> >> device_node *np, exynos4_soc == EXYNOS4210 ? "Exynos4210" :
>> >> "Exynos4x12", _get_rate("sclk_apll"), _get_rate("sclk_mpll"),
>> >>                 _get_rate("sclk_epll"), _get_rate("sclk_vpll"),
>> >> -               _get_rate("arm_clk"));
>> >> +               _get_rate("armclk"));
>> >>  }
>> >>
>> >>
>> >> diff --git a/drivers/clk/samsung/clk-exynos5250.c
>> >> b/drivers/clk/samsung/clk-exynos5250.c index e7ee442..3fe1ca0
>> >> 100644 --- a/drivers/clk/samsung/clk-exynos5250.c
>> >> +++ b/drivers/clk/samsung/clk-exynos5250.c
>> >> @@ -260,8 +260,6 @@ static struct samsung_mux_clock
>> >> exynos5250_mux_clks[] __initdata = { */
>> >>         MUX_FA(0, "mout_apll", mout_apll_p, SRC_CPU, 0, 1,
>> >>                                         CLK_SET_RATE_PARENT, 0,
>> >> "mout_apll"),
>> >> -       MUX_A(0, "mout_cpu", mout_cpu_p, SRC_CPU, 16, 1,
>> >> "mout_cpu"), -
>> >>         /*
>> >>          * CMU_CORE
>> >>          */
>> >> @@ -339,9 +337,8 @@ static struct samsung_div_clock
>> >> exynos5250_div_clks[] __initdata = { /*
>> >>          * CMU_CPU
>> >>          */
>> >> -       DIV(0, "div_arm", "mout_cpu", DIV_CPU0, 0, 3),
>> >> -       DIV(0, "div_apll", "mout_apll", DIV_CPU0, 24, 3),
>> >> -       DIV_A(0, "div_arm2", "div_arm", DIV_CPU0, 28, 3, "armclk"),
>> >> +       DIV_F(0, "div_apll", "mout_apll", DIV_CPU0, 24, 3,
>> >> +                       CLK_GET_RATE_NOCACHE, 0),
>> >>
>> >>         /*
>> >>          * CMU_TOP
>> >> @@ -721,10 +718,13 @@ static void __init
>> >> exynos5250_clk_init(struct device_node *np)
>> >> ARRAY_SIZE(exynos5250_div_clks));
>> >> samsung_clk_register_gate(exynos5250_gate_clks,
>> >> ARRAY_SIZE(exynos5250_gate_clks));
>> >> +       exynos_register_arm_clock(CLK_ARM_CLK, mout_cpu_p,
>> >> +                       ARRAY_SIZE(mout_cpu_p), reg_base, np, NULL,
>> >> +                       &samsung_clk_lock);
>> >>
>> >>         exynos5250_clk_sleep_init();
>> >>
>> >>         pr_info("Exynos5250: clock setup completed, armclk=%ld\n",
>> >> -                       _get_rate("div_arm2"));
>> >> +                       _get_rate("armclk"));
>> >>  }
>> >>  CLK_OF_DECLARE(exynos5250_clk, "samsung,exynos5250-clock",
>> >> exynos5250_clk_init); diff --git
>> >> a/include/dt-bindings/clock/exynos5250.h
>> >> b/include/dt-bindings/clock/exynos5250.h index 922f2dc..59a10fb
>> >> 100644 --- a/include/dt-bindings/clock/exynos5250.h +++
>> >> b/include/dt-bindings/clock/exynos5250.h @@ -21,6 +21,7 @@
>> >>  #define CLK_FOUT_CPLL          6
>> >>  #define CLK_FOUT_EPLL          7
>> >>  #define CLK_FOUT_VPLL          8
>> >> +#define CLK_ARM_CLK            12
>> >>
>> >>  /* gate for special clocks (sclk) */
>> >>  #define CLK_SCLK_CAM_BAYER     128
>> >> --
>> >> 1.7.4.4
>> >>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
>
>
> --
> Best regards,
>
> Lukasz Majewski
>
> Samsung R&D Institute Poland (SRPOL) | Linux Platform Group
Thomas Abraham May 16, 2014, 5:14 a.m. UTC | #5 
On Thu, May 15, 2014 at 3:07 AM, Mike Turquette <mturquette@linaro.org> wrote:
> Quoting Thomas Abraham (2014-05-13 18:11:13)
>> From: Thomas Abraham <thomas.ab@samsung.com>
>>
>> With the addition of the new Samsung specific cpu-clock type, the
>> arm clock can be represented as a cpu-clock type and the independent
>> clock blocks that made up the arm clock can be removed.
>
> <rant>
>
> I am not a fan of this type of "clock hiding". Certainly the design of
> the CCF allows for a clock provider to obfuscate it's internals; there
> was never a requirement that every clock node be exposed to Linux as a
> struct clk. A truly obfuscated system could only expose leaf clocks that
> are consumed by Linux device drivers, and never expose any of the
> intermediary clocks in between the input clock signal (if it exists) and
> the leaf nodes.
>
> However I feel that this patch is more of a workaround to the fact that
> the clock framework today does not make DVFS transitions (or
> coordinated, multi-clock rate change transitions) easy to control. The
> generic "walk up the tree" algorithm might give you the right rate, but
> perhaps using a non-validated combination of PLL frequency and
> adjustable-rate dividers, or a combination that his higher jitter or is
> more likely to unlock at higher temperatures, etc.
>
> Back in the pre-CCF days lots of folks implemented this with "virtual"
> clock nodes that simply called "clk_set_rate" or whatever on the
> affected clocks, or even worse just banged a bunch of registers.
>
> The cbus clock series for Tegra is also looks a little like this.
>
> </rant>
>
> Thomas,
>
> Would a coordinated clock rate change method solve this problem for you
> in place of the cpu-clock provider type? A poorly conceived call graph
> for this might look something like:
>
> clk_set_rate(div_arm2, 1000000);
> -> if (div_arm2->coordinated == true)
>         clk_coordinate_rates(div_arm2, 1000000);
>         -> clk->ops->coordinate(div_arm2->hw, 1000000);
>            -> vendor_supplied_magic()
>
> The vendor_supplied_magic() would be a callback that essentially calls
> clk_set_rate() on all of the affected (coordinated) clocks. In your case
> that looks like mout_core, div_core, div_core2, arm_clk and sclk_apll
> for Exynos4.
>
> The trick is that calling clk_set_rate() from any driver would initiate
> this coordinated rate change, and then the exynos clock driver would set
> up the coordinated clocks itself. No new API is introduced to drivers
> (since it still uses clk_set_rate) and now a new clock provider doesn't
> have to be invented every time we have a couple of clocks involved in a
> DVFS transition.
>
> Does this sound right to you or have I horribly misinterpreted the point
> of these clock patches?
>
> Thanks,
> Mike

Hi Mike,

I have been testing this patch series on three exynos platforms for
about five days now and do not see any issues. There are some review
comments which I need to incorporate. So before that, I wanted to
check with you about the grouping of clocks approach in this patch
series. Is this approach something that we could use until the changes
for coordinated clocks are in place? This patch series also helps with
multi-platform support for exynos as well.

Tomasz, I have addressed most of your comments from v3. If you could
have a look at the v4 series as well, it would be helpful in making
this better.

Thanks,
Thomas.

>
>>
>> Cc: Tomasz Figa <t.figa@samsung.com>
>> Signed-off-by: Thomas Abraham <thomas.ab@samsung.com>
>> ---
>>  drivers/clk/samsung/clk-exynos4.c      |   25 +++++++++----------------
>>  drivers/clk/samsung/clk-exynos5250.c   |   12 ++++++------
>>  include/dt-bindings/clock/exynos5250.h |    1 +
>>  3 files changed, 16 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/clk/samsung/clk-exynos4.c b/drivers/clk/samsung/clk-exynos4.c
>> index b4f9672..7e3bb16c 100644
>> --- a/drivers/clk/samsung/clk-exynos4.c
>> +++ b/drivers/clk/samsung/clk-exynos4.c
>> @@ -471,7 +471,6 @@ static struct samsung_mux_clock exynos4210_mux_clks[] __initdata = {
>>         MUX(0, "mout_fimd1", group1_p4210, E4210_SRC_LCD1, 0, 4),
>>         MUX(0, "mout_mipi1", group1_p4210, E4210_SRC_LCD1, 12, 4),
>>         MUX(CLK_SCLK_MPLL, "sclk_mpll", mout_mpll_p, SRC_CPU, 8, 1),
>> -       MUX(CLK_MOUT_CORE, "mout_core", mout_core_p4210, SRC_CPU, 16, 1),
>>         MUX(CLK_SCLK_VPLL, "sclk_vpll", sclk_vpll_p4210, SRC_TOP0, 8, 1),
>>         MUX(CLK_MOUT_FIMC0, "mout_fimc0", group1_p4210, SRC_CAM, 0, 4),
>>         MUX(CLK_MOUT_FIMC1, "mout_fimc1", group1_p4210, SRC_CAM, 4, 4),
>> @@ -530,7 +529,6 @@ static struct samsung_mux_clock exynos4x12_mux_clks[] __initdata = {
>>         MUX(0, "mout_jpeg", mout_jpeg_p, E4X12_SRC_CAM1, 8, 1),
>>         MUX(CLK_SCLK_MPLL, "sclk_mpll", mout_mpll_p, SRC_DMC, 12, 1),
>>         MUX(CLK_SCLK_VPLL, "sclk_vpll", mout_vpll_p, SRC_TOP0, 8, 1),
>> -       MUX(CLK_MOUT_CORE, "mout_core", mout_core_p4x12, SRC_CPU, 16, 1),
>>         MUX(CLK_MOUT_FIMC0, "mout_fimc0", group1_p4x12, SRC_CAM, 0, 4),
>>         MUX(CLK_MOUT_FIMC1, "mout_fimc1", group1_p4x12, SRC_CAM, 4, 4),
>>         MUX(CLK_MOUT_FIMC2, "mout_fimc2", group1_p4x12, SRC_CAM, 8, 4),
>> @@ -572,8 +570,6 @@ static struct samsung_mux_clock exynos4x12_mux_clks[] __initdata = {
>>
>>  /* list of divider clocks supported in all exynos4 soc's */
>>  static struct samsung_div_clock exynos4_div_clks[] __initdata = {
>> -       DIV(0, "div_core", "mout_core", DIV_CPU0, 0, 3),
>> -       DIV(0, "div_core2", "div_core", DIV_CPU0, 28, 3),
>>         DIV(0, "div_fimc0", "mout_fimc0", DIV_CAM, 0, 4),
>>         DIV(0, "div_fimc1", "mout_fimc1", DIV_CAM, 4, 4),
>>         DIV(0, "div_fimc2", "mout_fimc2", DIV_CAM, 8, 4),
>> @@ -619,8 +615,8 @@ static struct samsung_div_clock exynos4_div_clks[] __initdata = {
>>         DIV(0, "div_spi_pre2", "div_spi2", DIV_PERIL2, 8, 8),
>>         DIV(0, "div_audio1", "mout_audio1", DIV_PERIL4, 0, 4),
>>         DIV(0, "div_audio2", "mout_audio2", DIV_PERIL4, 16, 4),
>> -       DIV(CLK_ARM_CLK, "arm_clk", "div_core2", DIV_CPU0, 28, 3),
>> -       DIV(CLK_SCLK_APLL, "sclk_apll", "mout_apll", DIV_CPU0, 24, 3),
>> +       DIV_F(CLK_SCLK_APLL, "sclk_apll", "mout_apll", DIV_CPU0, 24, 3,
>> +                       CLK_GET_RATE_NOCACHE, 0),
>>         DIV_F(0, "div_mipi_pre0", "div_mipi0", DIV_LCD0, 20, 4,
>>                         CLK_SET_RATE_PARENT, 0),
>>         DIV_F(0, "div_mmc_pre0", "div_mmc0", DIV_FSYS1, 8, 8,
>> @@ -1003,12 +999,6 @@ static struct samsung_gate_clock exynos4x12_gate_clks[] __initdata = {
>>                 0),
>>  };
>>
>> -static struct samsung_clock_alias exynos4_aliases[] __initdata = {
>> -       ALIAS(CLK_MOUT_CORE, NULL, "moutcore"),
>> -       ALIAS(CLK_ARM_CLK, NULL, "armclk"),
>> -       ALIAS(CLK_SCLK_APLL, NULL, "mout_apll"),
>> -};
>> -
>>  static struct samsung_clock_alias exynos4210_aliases[] __initdata = {
>>         ALIAS(CLK_SCLK_MPLL, NULL, "mout_mpll"),
>>  };
>> @@ -1241,6 +1231,9 @@ static void __init exynos4_clk_init(struct device_node *np,
>>                         ARRAY_SIZE(exynos4210_gate_clks));
>>                 samsung_clk_register_alias(exynos4210_aliases,
>>                         ARRAY_SIZE(exynos4210_aliases));
>> +               exynos_register_arm_clock(CLK_ARM_CLK, mout_core_p4210,
>> +                       ARRAY_SIZE(mout_core_p4210), reg_base, np, NULL,
>> +                       &samsung_clk_lock);
>>         } else {
>>                 samsung_clk_register_mux(exynos4x12_mux_clks,
>>                         ARRAY_SIZE(exynos4x12_mux_clks));
>> @@ -1250,11 +1243,11 @@ static void __init exynos4_clk_init(struct device_node *np,
>>                         ARRAY_SIZE(exynos4x12_gate_clks));
>>                 samsung_clk_register_alias(exynos4x12_aliases,
>>                         ARRAY_SIZE(exynos4x12_aliases));
>> +               exynos_register_arm_clock(CLK_ARM_CLK, mout_core_p4x12,
>> +                       ARRAY_SIZE(mout_core_p4x12), reg_base, np, NULL,
>> +                       &samsung_clk_lock);
>>         }
>>
>> -       samsung_clk_register_alias(exynos4_aliases,
>> -                       ARRAY_SIZE(exynos4_aliases));
>> -
>>         exynos4_clk_sleep_init();
>>
>>         pr_info("%s clocks: sclk_apll = %ld, sclk_mpll = %ld\n"
>> @@ -1262,7 +1255,7 @@ static void __init exynos4_clk_init(struct device_node *np,
>>                 exynos4_soc == EXYNOS4210 ? "Exynos4210" : "Exynos4x12",
>>                 _get_rate("sclk_apll"), _get_rate("sclk_mpll"),
>>                 _get_rate("sclk_epll"), _get_rate("sclk_vpll"),
>> -               _get_rate("arm_clk"));
>> +               _get_rate("armclk"));
>>  }
>>
>>
>> diff --git a/drivers/clk/samsung/clk-exynos5250.c b/drivers/clk/samsung/clk-exynos5250.c
>> index e7ee442..3fe1ca0 100644
>> --- a/drivers/clk/samsung/clk-exynos5250.c
>> +++ b/drivers/clk/samsung/clk-exynos5250.c
>> @@ -260,8 +260,6 @@ static struct samsung_mux_clock exynos5250_mux_clks[] __initdata = {
>>          */
>>         MUX_FA(0, "mout_apll", mout_apll_p, SRC_CPU, 0, 1,
>>                                         CLK_SET_RATE_PARENT, 0, "mout_apll"),
>> -       MUX_A(0, "mout_cpu", mout_cpu_p, SRC_CPU, 16, 1, "mout_cpu"),
>> -
>>         /*
>>          * CMU_CORE
>>          */
>> @@ -339,9 +337,8 @@ static struct samsung_div_clock exynos5250_div_clks[] __initdata = {
>>         /*
>>          * CMU_CPU
>>          */
>> -       DIV(0, "div_arm", "mout_cpu", DIV_CPU0, 0, 3),
>> -       DIV(0, "div_apll", "mout_apll", DIV_CPU0, 24, 3),
>> -       DIV_A(0, "div_arm2", "div_arm", DIV_CPU0, 28, 3, "armclk"),
>> +       DIV_F(0, "div_apll", "mout_apll", DIV_CPU0, 24, 3,
>> +                       CLK_GET_RATE_NOCACHE, 0),
>>
>>         /*
>>          * CMU_TOP
>> @@ -721,10 +718,13 @@ static void __init exynos5250_clk_init(struct device_node *np)
>>                         ARRAY_SIZE(exynos5250_div_clks));
>>         samsung_clk_register_gate(exynos5250_gate_clks,
>>                         ARRAY_SIZE(exynos5250_gate_clks));
>> +       exynos_register_arm_clock(CLK_ARM_CLK, mout_cpu_p,
>> +                       ARRAY_SIZE(mout_cpu_p), reg_base, np, NULL,
>> +                       &samsung_clk_lock);
>>
>>         exynos5250_clk_sleep_init();
>>
>>         pr_info("Exynos5250: clock setup completed, armclk=%ld\n",
>> -                       _get_rate("div_arm2"));
>> +                       _get_rate("armclk"));
>>  }
>>  CLK_OF_DECLARE(exynos5250_clk, "samsung,exynos5250-clock", exynos5250_clk_init);
>> diff --git a/include/dt-bindings/clock/exynos5250.h b/include/dt-bindings/clock/exynos5250.h
>> index 922f2dc..59a10fb 100644
>> --- a/include/dt-bindings/clock/exynos5250.h
>> +++ b/include/dt-bindings/clock/exynos5250.h
>> @@ -21,6 +21,7 @@
>>  #define CLK_FOUT_CPLL          6
>>  #define CLK_FOUT_EPLL          7
>>  #define CLK_FOUT_VPLL          8
>> +#define CLK_ARM_CLK            12
>>
>>  /* gate for special clocks (sclk) */
>>  #define CLK_SCLK_CAM_BAYER     128
>> --
>> 1.7.4.4
>>
Tomasz Figa May 16, 2014, 11:57 p.m. UTC | #6 
Hi Thomas,

On 14.05.2014 03:11, Thomas Abraham wrote:
> From: Thomas Abraham <thomas.ab@samsung.com>
> 
> With the addition of the new Samsung specific cpu-clock type, the
> arm clock can be represented as a cpu-clock type and the independent
> clock blocks that made up the arm clock can be removed.
> 
> Cc: Tomasz Figa <t.figa@samsung.com>
> Signed-off-by: Thomas Abraham <thomas.ab@samsung.com>
> ---
>  drivers/clk/samsung/clk-exynos4.c      |   25 +++++++++----------------
>  drivers/clk/samsung/clk-exynos5250.c   |   12 ++++++------
>  include/dt-bindings/clock/exynos5250.h |    1 +
>  3 files changed, 16 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/clk/samsung/clk-exynos4.c b/drivers/clk/samsung/clk-exynos4.c
> index b4f9672..7e3bb16c 100644
> --- a/drivers/clk/samsung/clk-exynos4.c
> +++ b/drivers/clk/samsung/clk-exynos4.c
> @@ -471,7 +471,6 @@ static struct samsung_mux_clock exynos4210_mux_clks[] __initdata = {
>  	MUX(0, "mout_fimd1", group1_p4210, E4210_SRC_LCD1, 0, 4),
>  	MUX(0, "mout_mipi1", group1_p4210, E4210_SRC_LCD1, 12, 4),
>  	MUX(CLK_SCLK_MPLL, "sclk_mpll", mout_mpll_p, SRC_CPU, 8, 1),
> -	MUX(CLK_MOUT_CORE, "mout_core", mout_core_p4210, SRC_CPU, 16, 1),
>  	MUX(CLK_SCLK_VPLL, "sclk_vpll", sclk_vpll_p4210, SRC_TOP0, 8, 1),
>  	MUX(CLK_MOUT_FIMC0, "mout_fimc0", group1_p4210, SRC_CAM, 0, 4),
>  	MUX(CLK_MOUT_FIMC1, "mout_fimc1", group1_p4210, SRC_CAM, 4, 4),
> @@ -530,7 +529,6 @@ static struct samsung_mux_clock exynos4x12_mux_clks[] __initdata = {
>  	MUX(0, "mout_jpeg", mout_jpeg_p, E4X12_SRC_CAM1, 8, 1),
>  	MUX(CLK_SCLK_MPLL, "sclk_mpll", mout_mpll_p, SRC_DMC, 12, 1),
>  	MUX(CLK_SCLK_VPLL, "sclk_vpll", mout_vpll_p, SRC_TOP0, 8, 1),
> -	MUX(CLK_MOUT_CORE, "mout_core", mout_core_p4x12, SRC_CPU, 16, 1),
>  	MUX(CLK_MOUT_FIMC0, "mout_fimc0", group1_p4x12, SRC_CAM, 0, 4),
>  	MUX(CLK_MOUT_FIMC1, "mout_fimc1", group1_p4x12, SRC_CAM, 4, 4),
>  	MUX(CLK_MOUT_FIMC2, "mout_fimc2", group1_p4x12, SRC_CAM, 8, 4),
> @@ -572,8 +570,6 @@ static struct samsung_mux_clock exynos4x12_mux_clks[] __initdata = {
>  
>  /* list of divider clocks supported in all exynos4 soc's */
>  static struct samsung_div_clock exynos4_div_clks[] __initdata = {
> -	DIV(0, "div_core", "mout_core", DIV_CPU0, 0, 3),
> -	DIV(0, "div_core2", "div_core", DIV_CPU0, 28, 3),

Please don't remove these clocks, as they will be necessary to define
proper hierarchy CMU_CPU clock output block. In particular, access to
following clocks will be required:

div_corem0
div_corem1
div_cores
div_atb
div_periph
div_pclk_dbg
div_hpm

They might be implemented using normal dividers, but with
CLK_DIVIDER_READ_ONLY [1] and CLK_GET_RATE_NOCACHE flags.

[1] http://thread.gmane.org/gmane.linux.ports.arm.kernel/322977/focus=322978

This also means keeping mout_core clock defined, but with
CLK_MUX_READ_ONLY flag it should be fine.

>  	DIV(0, "div_fimc0", "mout_fimc0", DIV_CAM, 0, 4),
>  	DIV(0, "div_fimc1", "mout_fimc1", DIV_CAM, 4, 4),
>  	DIV(0, "div_fimc2", "mout_fimc2", DIV_CAM, 8, 4),
> @@ -619,8 +615,8 @@ static struct samsung_div_clock exynos4_div_clks[] __initdata = {
>  	DIV(0, "div_spi_pre2", "div_spi2", DIV_PERIL2, 8, 8),
>  	DIV(0, "div_audio1", "mout_audio1", DIV_PERIL4, 0, 4),
>  	DIV(0, "div_audio2", "mout_audio2", DIV_PERIL4, 16, 4),
> -	DIV(CLK_ARM_CLK, "arm_clk", "div_core2", DIV_CPU0, 28, 3),
> -	DIV(CLK_SCLK_APLL, "sclk_apll", "mout_apll", DIV_CPU0, 24, 3),
> +	DIV_F(CLK_SCLK_APLL, "sclk_apll", "mout_apll", DIV_CPU0, 24, 3,
> +			CLK_GET_RATE_NOCACHE, 0),

Do you need CLK_GET_RATE_NOCACHE here? AFAIK whenever rate is changed on
APLL the clock core will recalculate rate of this clock.

>  	DIV_F(0, "div_mipi_pre0", "div_mipi0", DIV_LCD0, 20, 4,
>  			CLK_SET_RATE_PARENT, 0),
>  	DIV_F(0, "div_mmc_pre0", "div_mmc0", DIV_FSYS1, 8, 8,
> @@ -1003,12 +999,6 @@ static struct samsung_gate_clock exynos4x12_gate_clks[] __initdata = {
>  		0),
>  };
>  
> -static struct samsung_clock_alias exynos4_aliases[] __initdata = {
> -	ALIAS(CLK_MOUT_CORE, NULL, "moutcore"),
> -	ALIAS(CLK_ARM_CLK, NULL, "armclk"),
> -	ALIAS(CLK_SCLK_APLL, NULL, "mout_apll"),
> -};
> -
>  static struct samsung_clock_alias exynos4210_aliases[] __initdata = {
>  	ALIAS(CLK_SCLK_MPLL, NULL, "mout_mpll"),
>  };
> @@ -1241,6 +1231,9 @@ static void __init exynos4_clk_init(struct device_node *np,
>  			ARRAY_SIZE(exynos4210_gate_clks));
>  		samsung_clk_register_alias(exynos4210_aliases,
>  			ARRAY_SIZE(exynos4210_aliases));
> +		exynos_register_arm_clock(CLK_ARM_CLK, mout_core_p4210,
> +			ARRAY_SIZE(mout_core_p4210), reg_base, np, NULL,
> +			&samsung_clk_lock);
>  	} else {
>  		samsung_clk_register_mux(exynos4x12_mux_clks,
>  			ARRAY_SIZE(exynos4x12_mux_clks));
> @@ -1250,11 +1243,11 @@ static void __init exynos4_clk_init(struct device_node *np,
>  			ARRAY_SIZE(exynos4x12_gate_clks));
>  		samsung_clk_register_alias(exynos4x12_aliases,
>  			ARRAY_SIZE(exynos4x12_aliases));
> +		exynos_register_arm_clock(CLK_ARM_CLK, mout_core_p4x12,
> +			ARRAY_SIZE(mout_core_p4x12), reg_base, np, NULL,
> +			&samsung_clk_lock);
>  	}
>  
> -	samsung_clk_register_alias(exynos4_aliases,
> -			ARRAY_SIZE(exynos4_aliases));
> -
>  	exynos4_clk_sleep_init();
>  
>  	pr_info("%s clocks: sclk_apll = %ld, sclk_mpll = %ld\n"
> @@ -1262,7 +1255,7 @@ static void __init exynos4_clk_init(struct device_node *np,
>  		exynos4_soc == EXYNOS4210 ? "Exynos4210" : "Exynos4x12",
>  		_get_rate("sclk_apll"),	_get_rate("sclk_mpll"),
>  		_get_rate("sclk_epll"), _get_rate("sclk_vpll"),
> -		_get_rate("arm_clk"));
> +		_get_rate("armclk"));

I'd prefer name of this clock to be not changed to not cause conflicts
with other patches.

Similar comments apply to clk-exynos5250.

Best regards,
Tomasz
diff mbox

Patch

diff --git a/drivers/clk/samsung/clk-exynos4.c b/drivers/clk/samsung/clk-exynos4.c
index b4f9672..7e3bb16c 100644
--- a/drivers/clk/samsung/clk-exynos4.c
+++ b/drivers/clk/samsung/clk-exynos4.c
@@ -471,7 +471,6 @@  static struct samsung_mux_clock exynos4210_mux_clks[] __initdata = {
 	MUX(0, "mout_fimd1", group1_p4210, E4210_SRC_LCD1, 0, 4),
 	MUX(0, "mout_mipi1", group1_p4210, E4210_SRC_LCD1, 12, 4),
 	MUX(CLK_SCLK_MPLL, "sclk_mpll", mout_mpll_p, SRC_CPU, 8, 1),
-	MUX(CLK_MOUT_CORE, "mout_core", mout_core_p4210, SRC_CPU, 16, 1),
 	MUX(CLK_SCLK_VPLL, "sclk_vpll", sclk_vpll_p4210, SRC_TOP0, 8, 1),
 	MUX(CLK_MOUT_FIMC0, "mout_fimc0", group1_p4210, SRC_CAM, 0, 4),
 	MUX(CLK_MOUT_FIMC1, "mout_fimc1", group1_p4210, SRC_CAM, 4, 4),
@@ -530,7 +529,6 @@  static struct samsung_mux_clock exynos4x12_mux_clks[] __initdata = {
 	MUX(0, "mout_jpeg", mout_jpeg_p, E4X12_SRC_CAM1, 8, 1),
 	MUX(CLK_SCLK_MPLL, "sclk_mpll", mout_mpll_p, SRC_DMC, 12, 1),
 	MUX(CLK_SCLK_VPLL, "sclk_vpll", mout_vpll_p, SRC_TOP0, 8, 1),
-	MUX(CLK_MOUT_CORE, "mout_core", mout_core_p4x12, SRC_CPU, 16, 1),
 	MUX(CLK_MOUT_FIMC0, "mout_fimc0", group1_p4x12, SRC_CAM, 0, 4),
 	MUX(CLK_MOUT_FIMC1, "mout_fimc1", group1_p4x12, SRC_CAM, 4, 4),
 	MUX(CLK_MOUT_FIMC2, "mout_fimc2", group1_p4x12, SRC_CAM, 8, 4),
@@ -572,8 +570,6 @@  static struct samsung_mux_clock exynos4x12_mux_clks[] __initdata = {
 
 /* list of divider clocks supported in all exynos4 soc's */
 static struct samsung_div_clock exynos4_div_clks[] __initdata = {
-	DIV(0, "div_core", "mout_core", DIV_CPU0, 0, 3),
-	DIV(0, "div_core2", "div_core", DIV_CPU0, 28, 3),
 	DIV(0, "div_fimc0", "mout_fimc0", DIV_CAM, 0, 4),
 	DIV(0, "div_fimc1", "mout_fimc1", DIV_CAM, 4, 4),
 	DIV(0, "div_fimc2", "mout_fimc2", DIV_CAM, 8, 4),
@@ -619,8 +615,8 @@  static struct samsung_div_clock exynos4_div_clks[] __initdata = {
 	DIV(0, "div_spi_pre2", "div_spi2", DIV_PERIL2, 8, 8),
 	DIV(0, "div_audio1", "mout_audio1", DIV_PERIL4, 0, 4),
 	DIV(0, "div_audio2", "mout_audio2", DIV_PERIL4, 16, 4),
-	DIV(CLK_ARM_CLK, "arm_clk", "div_core2", DIV_CPU0, 28, 3),
-	DIV(CLK_SCLK_APLL, "sclk_apll", "mout_apll", DIV_CPU0, 24, 3),
+	DIV_F(CLK_SCLK_APLL, "sclk_apll", "mout_apll", DIV_CPU0, 24, 3,
+			CLK_GET_RATE_NOCACHE, 0),
 	DIV_F(0, "div_mipi_pre0", "div_mipi0", DIV_LCD0, 20, 4,
 			CLK_SET_RATE_PARENT, 0),
 	DIV_F(0, "div_mmc_pre0", "div_mmc0", DIV_FSYS1, 8, 8,
@@ -1003,12 +999,6 @@  static struct samsung_gate_clock exynos4x12_gate_clks[] __initdata = {
 		0),
 };
 
-static struct samsung_clock_alias exynos4_aliases[] __initdata = {
-	ALIAS(CLK_MOUT_CORE, NULL, "moutcore"),
-	ALIAS(CLK_ARM_CLK, NULL, "armclk"),
-	ALIAS(CLK_SCLK_APLL, NULL, "mout_apll"),
-};
-
 static struct samsung_clock_alias exynos4210_aliases[] __initdata = {
 	ALIAS(CLK_SCLK_MPLL, NULL, "mout_mpll"),
 };
@@ -1241,6 +1231,9 @@  static void __init exynos4_clk_init(struct device_node *np,
 			ARRAY_SIZE(exynos4210_gate_clks));
 		samsung_clk_register_alias(exynos4210_aliases,
 			ARRAY_SIZE(exynos4210_aliases));
+		exynos_register_arm_clock(CLK_ARM_CLK, mout_core_p4210,
+			ARRAY_SIZE(mout_core_p4210), reg_base, np, NULL,
+			&samsung_clk_lock);
 	} else {
 		samsung_clk_register_mux(exynos4x12_mux_clks,
 			ARRAY_SIZE(exynos4x12_mux_clks));
@@ -1250,11 +1243,11 @@  static void __init exynos4_clk_init(struct device_node *np,
 			ARRAY_SIZE(exynos4x12_gate_clks));
 		samsung_clk_register_alias(exynos4x12_aliases,
 			ARRAY_SIZE(exynos4x12_aliases));
+		exynos_register_arm_clock(CLK_ARM_CLK, mout_core_p4x12,
+			ARRAY_SIZE(mout_core_p4x12), reg_base, np, NULL,
+			&samsung_clk_lock);
 	}
 
-	samsung_clk_register_alias(exynos4_aliases,
-			ARRAY_SIZE(exynos4_aliases));
-
 	exynos4_clk_sleep_init();
 
 	pr_info("%s clocks: sclk_apll = %ld, sclk_mpll = %ld\n"
@@ -1262,7 +1255,7 @@  static void __init exynos4_clk_init(struct device_node *np,
 		exynos4_soc == EXYNOS4210 ? "Exynos4210" : "Exynos4x12",
 		_get_rate("sclk_apll"),	_get_rate("sclk_mpll"),
 		_get_rate("sclk_epll"), _get_rate("sclk_vpll"),
-		_get_rate("arm_clk"));
+		_get_rate("armclk"));
 }
 
 
diff --git a/drivers/clk/samsung/clk-exynos5250.c b/drivers/clk/samsung/clk-exynos5250.c
index e7ee442..3fe1ca0 100644
--- a/drivers/clk/samsung/clk-exynos5250.c
+++ b/drivers/clk/samsung/clk-exynos5250.c
@@ -260,8 +260,6 @@  static struct samsung_mux_clock exynos5250_mux_clks[] __initdata = {
 	 */
 	MUX_FA(0, "mout_apll", mout_apll_p, SRC_CPU, 0, 1,
 					CLK_SET_RATE_PARENT, 0, "mout_apll"),
-	MUX_A(0, "mout_cpu", mout_cpu_p, SRC_CPU, 16, 1, "mout_cpu"),
-
 	/*
 	 * CMU_CORE
 	 */
@@ -339,9 +337,8 @@  static struct samsung_div_clock exynos5250_div_clks[] __initdata = {
 	/*
 	 * CMU_CPU
 	 */
-	DIV(0, "div_arm", "mout_cpu", DIV_CPU0, 0, 3),
-	DIV(0, "div_apll", "mout_apll", DIV_CPU0, 24, 3),
-	DIV_A(0, "div_arm2", "div_arm", DIV_CPU0, 28, 3, "armclk"),
+	DIV_F(0, "div_apll", "mout_apll", DIV_CPU0, 24, 3,
+			CLK_GET_RATE_NOCACHE, 0),
 
 	/*
 	 * CMU_TOP
@@ -721,10 +718,13 @@  static void __init exynos5250_clk_init(struct device_node *np)
 			ARRAY_SIZE(exynos5250_div_clks));
 	samsung_clk_register_gate(exynos5250_gate_clks,
 			ARRAY_SIZE(exynos5250_gate_clks));
+	exynos_register_arm_clock(CLK_ARM_CLK, mout_cpu_p,
+			ARRAY_SIZE(mout_cpu_p), reg_base, np, NULL,
+			&samsung_clk_lock);
 
 	exynos5250_clk_sleep_init();
 
 	pr_info("Exynos5250: clock setup completed, armclk=%ld\n",
-			_get_rate("div_arm2"));
+			_get_rate("armclk"));
 }
 CLK_OF_DECLARE(exynos5250_clk, "samsung,exynos5250-clock", exynos5250_clk_init);
diff --git a/include/dt-bindings/clock/exynos5250.h b/include/dt-bindings/clock/exynos5250.h
index 922f2dc..59a10fb 100644
--- a/include/dt-bindings/clock/exynos5250.h
+++ b/include/dt-bindings/clock/exynos5250.h
@@ -21,6 +21,7 @@ 
 #define CLK_FOUT_CPLL		6
 #define CLK_FOUT_EPLL		7
 #define CLK_FOUT_VPLL		8
+#define CLK_ARM_CLK		12
 
 /* gate for special clocks (sclk) */
 #define CLK_SCLK_CAM_BAYER	128