[PATCH 0/6] cpufreq: use cpufreq-cpu0 driver for exynos4210 based platforms

[PATCH 0/6] cpufreq: use cpufreq-cpu0 driver for exynos4210 based platforms

[Thomas Abraham]

The patch series removes the use of Exynos4210 specific cpufreq driver
and enables to use the cpufreq-cpu0 driver for Exynos4210 based platforms.
This is being done for few reasons.

(a) The Exynos4210 cpufreq driver reads/writes clock controller registers
    bypassing the Exynos4 CCF driver which is sort of problematic.
(b) Removes the need for having clock controller register definitions
    in the cpufreq driver and also removes the need for statically
    io-remapping clock controller address space (helps in moving towards
    multiplatform kernel).

In order to use cpufreq-cpu0 driver and provide fast cpu clock switching
during dvfs operations, the following apporach has been used.

(a) A new CPU clock provider type has been introduced in Samsung's CCF
    support. This clock provider type can be a compostion of multiple
    clock blocks such as mux, dividers and gates. Typically, in Exynos
    platforms, there are multiple clock blocks in between the output
    of the APLL and the CPU clock domain output. Representing these
    mutiple clock blocks within a opaque CPU clock provider type helps
    in reducing the time taken to perform a CPU clock frequency change
    operation, which is generally required during DVFS operations.
    This approach was suggested by Arnd Bergmann <arnd@arndb.de> during
    LCE-2013.

(b) A new optional safe operating point property has been introduced
    in the cpufreq-cpu0 driver binding. On some platforms such as the
    Samsung Exynos, a change in CPU frequency requires a change in the
    PLL output that drives the CPU clock. A change in PLL output
    requires the PLL output be turned off, which implies that the CPU
    (and other components in the CPU clock domain) be supplied with
    an alternate clock source during the time the PLL output is changed.

    The clock speed of this alternate clock source could be higher
    than the clock speed of the PLL at the time of switching over to
    the alternate clock source. This temporary increase in clock speed
    of the CPU clock domain implies that the blocks within the CPU
    clock domain should also be supplied with an appropriate voltage
    supply level as required to drive the CPU clock domain components
    at the speed of the alternative clock source. This temporary
    voltage level required during switching of CPU clock speed is
    called safe voltage level. And the cpufreq-cpu0 driver has been
    modified to setup the safe voltage levels during the changes
    in CPU clock speed.

(c) The CPU clock supply as been restructured as
    [ Output of APLL -> Opaque CPU clock provider -> CPU clock output ]
    And with the changes in (a) and (b) above, the cpufreq-cpu0 driver
    can now be used and can remove the use of Exynos4210 specific
    cpufreq driver.

This approach is currently tried on Exynos4210 only. The same approach
can be adopted for other Exynos SoCs as well. This patch series is
based on linux-next master branch.

Thomas Abraham (6):
  cpufreq: cpufreq-cpu0: allow optional safe voltage during frequency transitions
  clk: samsung: add infrastructure to register CPU clocks
  clk: samsung: register cpu clock provider for exynos4210 SoC
  cpufreq: exynos: remove Exynos4210 specific cpufreq driver support
  arm: exynos4-dt: statically add platform device for cpufreq-cpu0 platform driver
  arm: dts: add cpu nodes for Exynos4210 SoC

 .../devicetree/bindings/cpufreq/cpufreq-cpu0.txt   |    5 +
 arch/arm/boot/dts/exynos4210-origen.dts            |    6 +
 arch/arm/boot/dts/exynos4210-trats.dts             |    6 +
 arch/arm/boot/dts/exynos4210-universal_c210.dts    |    6 +
 arch/arm/boot/dts/exynos4210.dtsi                  |   22 +++
 arch/arm/mach-exynos/mach-exynos4-dt.c             |    6 +
 drivers/clk/samsung/clk-exynos4.c                  |   96 ++++++++++++-
 drivers/clk/samsung/clk.c                          |   71 +++++++++
 drivers/clk/samsung/clk.h                          |   37 +++++-
 drivers/cpufreq/Kconfig.arm                        |   11 --
 drivers/cpufreq/Makefile                           |    1 -
 drivers/cpufreq/cpufreq-cpu0.c                     |   49 ++++++-
 drivers/cpufreq/exynos-cpufreq.c                   |    4 +-
 drivers/cpufreq/exynos-cpufreq.h                   |    8 -
 drivers/cpufreq/exynos4210-cpufreq.c               |  157 --------------------
 15 files changed, 301 insertions(+), 184 deletions(-)
 delete mode 100644 drivers/cpufreq/exynos4210-cpufreq.c

[PATCH 1/6] cpufreq: cpufreq-cpu0: allow optional safe voltage during frequency transitions

[Thomas Abraham]

On some platforms such as the Samsung Exynos, changing the frequency
of the CPU clock requires changing the frequency of the PLL that is
supplying the CPU clock. To change the frequency of the PLL, the CPU
clock is temporarily reparented to another parent clock.

The clock frequency of this temporary parent clock could be much higher
than the clock frequency of the PLL at the time of reparenting. Due
to the temporary increase in the CPU clock speed, the CPU (and any other
components in the CPU clock domain such as dividers, mux, etc.) have to
to be operated at a higher voltage level, called the safe voltage level.
This patch adds optional support to temporarily switch to a safe voltage
level during CPU frequency transitions.

Cc: Shawn Guo <shawn.guo@linaro.org>
Signed-off-by: Thomas Abraham <thomas.ab@samsung.com>
---
 .../devicetree/bindings/cpufreq/cpufreq-cpu0.txt   |    5 ++
 drivers/cpufreq/cpufreq-cpu0.c                     |   49 +++++++++++++++++++-
 2 files changed, 52 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt b/Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt
index f055515..020d859 100644
--- a/Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt
+++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt
@@ -19,6 +19,10 @@ Optional properties:
 - cooling-min-level:
 - cooling-max-level:
      Please refer to Documentation/devicetree/bindings/thermal/thermal.txt.
+- safe-opp-index: Certain platforms require that during a opp transition,
+  a system should not go below a particular opp level. For such systems,
+  this property specifies the minimum opp to be maintained during the
+  opp transitions.
 
 Examples:
 
@@ -36,6 +40,7 @@ cpus {
 			396000  950000
 			198000  850000
 		>;
+		safe-opp-index = <1>;
 		clock-latency = <61036>; /* two CLK32 periods */
 		#cooling-cells = <2>;
 		cooling-min-level = <0>;
diff --git a/drivers/cpufreq/cpufreq-cpu0.c b/drivers/cpufreq/cpufreq-cpu0.c
index 0c12ffc..dda4b7b 100644
--- a/drivers/cpufreq/cpufreq-cpu0.c
+++ b/drivers/cpufreq/cpufreq-cpu0.c
@@ -27,6 +27,8 @@
 
 static unsigned int transition_latency;
 static unsigned int voltage_tolerance; /* in percentage */
+static unsigned long safe_frequency;
+static unsigned long safe_voltage;
 
 static struct device *cpu_dev;
 static struct clk *cpu_clk;
@@ -69,12 +71,26 @@ static int cpu0_set_target(struct cpufreq_policy *policy, unsigned int index)
 		 new_freq / 1000, volt ? volt / 1000 : -1);
 
 	/* scaling up?  scale voltage before frequency */
-	if (!IS_ERR(cpu_reg) && new_freq > old_freq) {
+	if (!IS_ERR(cpu_reg) && new_freq > old_freq &&
+				new_freq >= safe_frequency) {
 		ret = regulator_set_voltage_tol(cpu_reg, volt, tol);
 		if (ret) {
 			pr_err("failed to scale voltage up: %d\n", ret);
 			return ret;
 		}
+	} else if (!IS_ERR(cpu_reg) && old_freq < safe_frequency) {
+		/*
+		 * the scaled up voltage level for the new_freq is lower
+		 * than the safe voltage level. so set safe_voltage
+		 * as the intermediate voltage level and revert it
+		 * back after the frequency has been changed.
+		 */
+		ret = regulator_set_voltage(cpu_reg, safe_voltage,
+						safe_voltage);
+		if (ret) {
+			pr_err("failed to set safe voltage: %d\n", ret);
+			return ret;
+		}
 	}
 
 	ret = clk_set_rate(cpu_clk, freq_exact);
@@ -94,6 +110,19 @@ static int cpu0_set_target(struct cpufreq_policy *policy, unsigned int index)
 		}
 	}
 
+	/*
+	 * if safe voltage was applied during voltage scale up, then set
+	 * the correct target voltage now.
+	 */
+	if (!IS_ERR(cpu_reg) && new_freq > old_freq &&
+					new_freq < safe_frequency) {
+		ret = regulator_set_voltage_tol(cpu_reg, volt, tol);
+		if (ret) {
+			pr_err("failed to scale voltage up: %d\n", ret);
+			return ret;
+		}
+	}
+
 	return ret;
 }
 
@@ -116,7 +145,9 @@ static struct cpufreq_driver cpu0_cpufreq_driver = {
 
 static int cpu0_cpufreq_probe(struct platform_device *pdev)
 {
+	struct dev_pm_opp *opp;
 	struct device_node *np;
+	unsigned int safe_opp_index;
 	int ret;
 
 	cpu_dev = get_cpu_device(0);
@@ -165,13 +196,27 @@ static int cpu0_cpufreq_probe(struct platform_device *pdev)
 		goto out_put_node;
 	}
 
+	if (!of_property_read_u32(np, "safe-opp-index", &safe_opp_index)) {
+		rcu_read_lock();
+		opp = dev_pm_opp_find_freq_exact(cpu_dev,
+			freq_table[safe_opp_index].frequency * 1000, true);
+		if (IS_ERR(opp)) {
+			rcu_read_unlock();
+			pr_err("safe opp index %d is invalid\n",
+							safe_opp_index);
+			goto out_free_table;
+		}
+		safe_voltage = dev_pm_opp_get_voltage(opp);
+		safe_frequency = freq_table[safe_opp_index].frequency;
+		rcu_read_unlock();
+	}
+
 	of_property_read_u32(np, "voltage-tolerance", &voltage_tolerance);
 
 	if (of_property_read_u32(np, "clock-latency", &transition_latency))
 		transition_latency = CPUFREQ_ETERNAL;
 
 	if (!IS_ERR(cpu_reg)) {
-		struct dev_pm_opp *opp;
 		unsigned long min_uV, max_uV;
 		int i;
 
-- 
1.6.6.rc2

Re: [PATCH 1/6] cpufreq: cpufreq-cpu0: allow optional safe voltage during frequency transitions

[Lukasz Majewski]

Hi Thomas,

> On some platforms such as the Samsung Exynos, changing the frequency
> of the CPU clock requires changing the frequency of the PLL that is
> supplying the CPU clock. To change the frequency of the PLL, the CPU
> clock is temporarily reparented to another parent clock.

Please look into my comments about reparenting at PATCH 3/6.

> 
> The clock frequency of this temporary parent clock could be much
> higher than the clock frequency of the PLL at the time of
> reparenting. Due to the temporary increase in the CPU clock speed,
> the CPU (and any other components in the CPU clock domain such as
> dividers, mux, etc.) have to to be operated at a higher voltage
> level, called the safe voltage level. This patch adds optional
> support to temporarily switch to a safe voltage level during CPU
> frequency transitions.
> 
> Cc: Shawn Guo <shawn.guo@linaro.org>
> Signed-off-by: Thomas Abraham <thomas.ab@samsung.com>
> ---
>  .../devicetree/bindings/cpufreq/cpufreq-cpu0.txt   |    5 ++
>  drivers/cpufreq/cpufreq-cpu0.c                     |   49
> +++++++++++++++++++- 2 files changed, 52 insertions(+), 2 deletions(-)
> 
> diff --git
> a/Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt
> b/Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt index
> f055515..020d859 100644 ---
> a/Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt +++
> b/Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt @@ -19,6
> +19,10 @@ Optional properties:
>  - cooling-min-level:
>  - cooling-max-level:
>       Please refer to
> Documentation/devicetree/bindings/thermal/thermal.txt. +-
> safe-opp-index: Certain platforms require that during a opp
> transition,
> +  a system should not go below a particular opp level. For such
> systems,
> +  this property specifies the minimum opp to be maintained during the
> +  opp transitions.
>  
>  Examples:
>  
> @@ -36,6 +40,7 @@ cpus {
>  			396000  950000
>  			198000  850000
>  		>;
> +		safe-opp-index = <1>;
>  		clock-latency = <61036>; /* two CLK32 periods */
>  		#cooling-cells = <2>;
>  		cooling-min-level = <0>;
> diff --git a/drivers/cpufreq/cpufreq-cpu0.c
> b/drivers/cpufreq/cpufreq-cpu0.c index 0c12ffc..dda4b7b 100644
> --- a/drivers/cpufreq/cpufreq-cpu0.c
> +++ b/drivers/cpufreq/cpufreq-cpu0.c
> @@ -27,6 +27,8 @@
>  
>  static unsigned int transition_latency;
>  static unsigned int voltage_tolerance; /* in percentage */
> +static unsigned long safe_frequency;
> +static unsigned long safe_voltage;
>  
>  static struct device *cpu_dev;
>  static struct clk *cpu_clk;
> @@ -69,12 +71,26 @@ static int cpu0_set_target(struct cpufreq_policy
> *policy, unsigned int index) new_freq / 1000, volt ? volt / 1000 :
> -1); 
>  	/* scaling up?  scale voltage before frequency */
> -	if (!IS_ERR(cpu_reg) && new_freq > old_freq) {
> +	if (!IS_ERR(cpu_reg) && new_freq > old_freq &&
> +				new_freq >= safe_frequency) {
>  		ret = regulator_set_voltage_tol(cpu_reg, volt, tol);
>  		if (ret) {
>  			pr_err("failed to scale voltage up: %d\n",
> ret); return ret;
>  		}
> +	} else if (!IS_ERR(cpu_reg) && old_freq < safe_frequency) {
> +		/*
> +		 * the scaled up voltage level for the new_freq is
> lower
> +		 * than the safe voltage level. so set safe_voltage
> +		 * as the intermediate voltage level and revert it
> +		 * back after the frequency has been changed.
> +		 */
> +		ret = regulator_set_voltage(cpu_reg, safe_voltage,
> +						safe_voltage);
> +		if (ret) {
> +			pr_err("failed to set safe voltage: %d\n",
> ret);
> +			return ret;
> +		}
>  	}
>  
>  	ret = clk_set_rate(cpu_clk, freq_exact);
> @@ -94,6 +110,19 @@ static int cpu0_set_target(struct cpufreq_policy
> *policy, unsigned int index) }
>  	}
>  
> +	/*
> +	 * if safe voltage was applied during voltage scale up, then
> set
> +	 * the correct target voltage now.
> +	 */
> +	if (!IS_ERR(cpu_reg) && new_freq > old_freq &&
> +					new_freq < safe_frequency) {
> +		ret = regulator_set_voltage_tol(cpu_reg, volt, tol);
> +		if (ret) {
> +			pr_err("failed to scale voltage up: %d\n",
> ret);
> +			return ret;
> +		}
> +	}
> +
>  	return ret;
>  }
>  
> @@ -116,7 +145,9 @@ static struct cpufreq_driver cpu0_cpufreq_driver
> = { 
>  static int cpu0_cpufreq_probe(struct platform_device *pdev)
>  {
> +	struct dev_pm_opp *opp;
>  	struct device_node *np;
> +	unsigned int safe_opp_index;
>  	int ret;
>  
>  	cpu_dev = get_cpu_device(0);
> @@ -165,13 +196,27 @@ static int cpu0_cpufreq_probe(struct
> platform_device *pdev) goto out_put_node;
>  	}
>  
> +	if (!of_property_read_u32(np, "safe-opp-index",
> &safe_opp_index)) {
> +		rcu_read_lock();
> +		opp = dev_pm_opp_find_freq_exact(cpu_dev,
> +			freq_table[safe_opp_index].frequency * 1000,
> true);
> +		if (IS_ERR(opp)) {
> +			rcu_read_unlock();
> +			pr_err("safe opp index %d is invalid\n",
> +
> safe_opp_index);
> +			goto out_free_table;
> +		}
> +		safe_voltage = dev_pm_opp_get_voltage(opp);
> +		safe_frequency =
> freq_table[safe_opp_index].frequency;
> +		rcu_read_unlock();
> +	}
> +
>  	of_property_read_u32(np, "voltage-tolerance",
> &voltage_tolerance); 
>  	if (of_property_read_u32(np, "clock-latency",
> &transition_latency)) transition_latency = CPUFREQ_ETERNAL;
>  
>  	if (!IS_ERR(cpu_reg)) {
> -		struct dev_pm_opp *opp;
>  		unsigned long min_uV, max_uV;
>  		int i;
>  

Reviewed-by: Lukasz Majewski <l.majewski@samsung.com>

-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

Re: [PATCH 1/6] cpufreq: cpufreq-cpu0: allow optional safe voltage during frequency transitions

[Tomasz Figa]

Hi Thomas,

Please see my comments inline.

On 09.01.2014 16:59, Thomas Abraham wrote:
[snip]
> @@ -69,12 +71,26 @@ static int cpu0_set_target(struct cpufreq_policy *policy, unsigned int index)
>   		 new_freq / 1000, volt ? volt / 1000 : -1);
>
>   	/* scaling up?  scale voltage before frequency */
> -	if (!IS_ERR(cpu_reg) && new_freq > old_freq) {
> +	if (!IS_ERR(cpu_reg) && new_freq > old_freq &&
> +				new_freq >= safe_frequency) {
>   		ret = regulator_set_voltage_tol(cpu_reg, volt, tol);
>   		if (ret) {
>   			pr_err("failed to scale voltage up: %d\n", ret);
>   			return ret;
>   		}
> +	} else if (!IS_ERR(cpu_reg) && old_freq < safe_frequency) {
> +		/*
> +		 * the scaled up voltage level for the new_freq is lower
> +		 * than the safe voltage level. so set safe_voltage
> +		 * as the intermediate voltage level and revert it
> +		 * back after the frequency has been changed.
> +		 */
> +		ret = regulator_set_voltage(cpu_reg, safe_voltage,
> +						safe_voltage);

Shouldn't the tolerance be used here as well?

> +		if (ret) {
> +			pr_err("failed to set safe voltage: %d\n", ret);
> +			return ret;
> +		}
>   	}
>
>   	ret = clk_set_rate(cpu_clk, freq_exact);
> @@ -94,6 +110,19 @@ static int cpu0_set_target(struct cpufreq_policy *policy, unsigned int index)
>   		}
>   	}
>
> +	/*
> +	 * if safe voltage was applied during voltage scale up, then set
> +	 * the correct target voltage now.
> +	 */
> +	if (!IS_ERR(cpu_reg) && new_freq > old_freq &&
> +					new_freq < safe_frequency) {
> +		ret = regulator_set_voltage_tol(cpu_reg, volt, tol);
> +		if (ret) {
> +			pr_err("failed to scale voltage up: %d\n", ret);
> +			return ret;
> +		}
> +	}
> +

I believe that it would be enough to reuse the if block of scaling down 
instead of repeating the same code, just the condition must be slightly 
modified:

-	/* scaling down?  scale voltage after frequency */
-	if (!IS_ERR(cpu_reg) && new_freq < old_freq) {
+	/*
+	 * scaling down or below safe frequency?
+	 * scale voltage after frequency
+	 */
+	if (!IS_ERR(cpu_reg)
+	    && (new_freq < old_freq || new_freq < safe_frequency)) {

Best regards,
Tomasz

Re: [PATCH 1/6] cpufreq: cpufreq-cpu0: allow optional safe voltage during frequency transitions

[Shawn Guo]

Hi Thomas,

On Thu, Jan 09, 2014 at 09:29:20PM +0530, Thomas Abraham wrote:
> @@ -19,6 +19,10 @@ Optional properties:
>  - cooling-min-level:
>  - cooling-max-level:
>       Please refer to Documentation/devicetree/bindings/thermal/thermal.txt.
> +- safe-opp-index: Certain platforms require that during a opp transition,
> +  a system should not go below a particular opp level. For such systems,
> +  this property specifies the minimum opp to be maintained during the
> +  opp transitions.

<snip>

> @@ -165,13 +196,27 @@ static int cpu0_cpufreq_probe(struct platform_device *pdev)
>  		goto out_put_node;
>  	}
>  
> +	if (!of_property_read_u32(np, "safe-opp-index", &safe_opp_index)) {
> +		rcu_read_lock();
> +		opp = dev_pm_opp_find_freq_exact(cpu_dev,
> +			freq_table[safe_opp_index].frequency * 1000, true);

The property safe_opp_index is directly used to index freq_table here.
It only works when the opp table in device tree is defined in increasing
order.  But we have nothing in opp binding bindings/power/opp.txt to
enforce it.  That said, opp table in device tree could be in arbitrary
order, and thus safe_opp_index gives you nothing reliable.

Shawn

> +		if (IS_ERR(opp)) {
> +			rcu_read_unlock();
> +			pr_err("safe opp index %d is invalid\n",
> +							safe_opp_index);
> +			goto out_free_table;
> +		}
> +		safe_voltage = dev_pm_opp_get_voltage(opp);
> +		safe_frequency = freq_table[safe_opp_index].frequency;
> +		rcu_read_unlock();
> +	}
> +
>  	of_property_read_u32(np, "voltage-tolerance", &voltage_tolerance);
>  
>  	if (of_property_read_u32(np, "clock-latency", &transition_latency))
>  		transition_latency = CPUFREQ_ETERNAL;
>  
>  	if (!IS_ERR(cpu_reg)) {
> -		struct dev_pm_opp *opp;
>  		unsigned long min_uV, max_uV;
>  		int i;
>  
> -- 
> 1.6.6.rc2
> 

Re: [PATCH 1/6] cpufreq: cpufreq-cpu0: allow optional safe voltage during frequency transitions

[Thomas Abraham]

Hi Shawn,

On Mon, Jan 13, 2014 at 8:44 AM, Shawn Guo <shawn.guo@linaro.org> wrote:
> Hi Thomas,
>
> On Thu, Jan 09, 2014 at 09:29:20PM +0530, Thomas Abraham wrote:
>> @@ -19,6 +19,10 @@ Optional properties:
>>  - cooling-min-level:
>>  - cooling-max-level:
>>       Please refer to Documentation/devicetree/bindings/thermal/thermal.txt.
>> +- safe-opp-index: Certain platforms require that during a opp transition,
>> +  a system should not go below a particular opp level. For such systems,
>> +  this property specifies the minimum opp to be maintained during the
>> +  opp transitions.
>
> <snip>
>
>> @@ -165,13 +196,27 @@ static int cpu0_cpufreq_probe(struct platform_device *pdev)
>>               goto out_put_node;
>>       }
>>
>> +     if (!of_property_read_u32(np, "safe-opp-index", &safe_opp_index)) {
>> +             rcu_read_lock();
>> +             opp = dev_pm_opp_find_freq_exact(cpu_dev,
>> +                     freq_table[safe_opp_index].frequency * 1000, true);
>
> The property safe_opp_index is directly used to index freq_table here.
> It only works when the opp table in device tree is defined in increasing
> order.  But we have nothing in opp binding bindings/power/opp.txt to
> enforce it.  That said, opp table in device tree could be in arbitrary
> order, and thus safe_opp_index gives you nothing reliable.

Yes, true. Thanks for pointing out. Instead of using index, will using
a tuple of for safe frequency and safe voltage be fine ?

Thanks,
Thomas.

>
> Shawn
>
>> +             if (IS_ERR(opp)) {
>> +                     rcu_read_unlock();
>> +                     pr_err("safe opp index %d is invalid\n",
>> +                                                     safe_opp_index);
>> +                     goto out_free_table;
>> +             }
>> +             safe_voltage = dev_pm_opp_get_voltage(opp);
>> +             safe_frequency = freq_table[safe_opp_index].frequency;
>> +             rcu_read_unlock();
>> +     }
>> +
>>       of_property_read_u32(np, "voltage-tolerance", &voltage_tolerance);
>>
>>       if (of_property_read_u32(np, "clock-latency", &transition_latency))
>>               transition_latency = CPUFREQ_ETERNAL;
>>
>>       if (!IS_ERR(cpu_reg)) {
>> -             struct dev_pm_opp *opp;
>>               unsigned long min_uV, max_uV;
>>               int i;
>>
>> --
>> 1.6.6.rc2
>>
>

Re: [PATCH 1/6] cpufreq: cpufreq-cpu0: allow optional safe voltage during frequency transitions

[Shawn Guo]

On Mon, Jan 13, 2014 at 07:51:25PM +0530, Thomas Abraham wrote:
> Yes, true. Thanks for pointing out. Instead of using index, will using
> a tuple of for safe frequency and safe voltage be fine ?

I'm fine with it.

Shawn

[PATCH 2/6] clk: samsung: add infrastructure to register CPU clocks

[Thomas Abraham]

The CPU clock provider supplies the clock to the CPU clock domain. The
composition and organization of the CPU clock provider could vary among
Exynos SoCs. A CPU clock provider can be composed of clock mux, dividers
and gates. This patch defines a new clock type for CPU clock provider and
adds infrastructure to register the CPU clock providers for Samsung
platforms.

Signed-off-by: Thomas Abraham <thomas.ab@samsung.com>
---
 drivers/clk/samsung/clk.c |   71 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/clk/samsung/clk.h |   37 +++++++++++++++++++++++-
 2 files changed, 107 insertions(+), 1 deletions(-)

diff --git a/drivers/clk/samsung/clk.c b/drivers/clk/samsung/clk.c
index f503f32..9ac9056 100644
--- a/drivers/clk/samsung/clk.c
+++ b/drivers/clk/samsung/clk.c
@@ -316,3 +316,74 @@ unsigned long _get_rate(const char *clk_name)
 
 	return clk_get_rate(clk);
 }
+
+/*
+ * On most platform, the core clock rate is equal to the clock rate of
+ * parent pll. This is a simple helper function to support recalc_rate
+ * callback for such platforms.
+ */
+unsigned long samsung_core_clock_recalc_rate(struct clk_hw *hw,
+				unsigned long parent_rate)
+{
+	/*
+	 * Assuming that the output of the parent pll is the output clock
+	 * frequency of the core clock.
+	 */
+	return parent_rate;
+}
+
+/* This is a helper function to perform clock rounding for core clocks. */
+long samsung_core_clk_round_rate(struct clk_hw *hw,
+			unsigned long drate, unsigned long *prate)
+{
+	struct samsung_core_clock *core_clk;
+	const struct samsung_core_clock_freq_table *freq_tbl;
+	int i;
+
+	core_clk = container_of(hw, struct samsung_core_clock, hw);
+	freq_tbl = core_clk->freq_table;
+	drate /= 1000;
+
+	for (i = 0; i < freq_tbl->freq_count; i++) {
+		if (drate >= freq_tbl->freq[i])
+			return freq_tbl->freq[i] * 1000;
+	}
+	return freq_tbl->freq[i - 1] * 1000;
+}
+
+/* helper function to register a core clock */
+void __init samsung_coreclk_register(const char *name, const char **parents,
+			unsigned int num_parents, const char *pllclk,
+			const struct clk_ops *clk_ops, unsigned int lookup_id,
+			const struct samsung_core_clock_freq_table *freq_tbl)
+{
+	struct samsung_core_clock *coreclk;
+	struct clk_init_data init;
+	struct clk *clk;
+
+	coreclk = kzalloc(sizeof(*coreclk), GFP_KERNEL);
+	if (!coreclk) {
+		pr_err("%s: could not allocate memory for coreclk %s\n",
+					__func__, name);
+		return;
+	}
+
+	init.name = name;
+	init.flags = CLK_GET_RATE_NOCACHE;
+	init.parent_names = parents;
+	init.num_parents = num_parents;
+	init.ops = clk_ops;
+
+	coreclk->hw.init = &init;
+	coreclk->ctrl_base = reg_base;
+	coreclk->fout_pll = __clk_lookup(pllclk);
+	coreclk->freq_table = freq_tbl;
+
+	clk = clk_register(NULL, &coreclk->hw);
+	if (IS_ERR(clk)) {
+		pr_err("%s: could not register coreclk %s\n", __func__,	name);
+		kfree(coreclk);
+		return;
+	}
+	samsung_clk_add_lookup(clk, lookup_id);
+}
diff --git a/drivers/clk/samsung/clk.h b/drivers/clk/samsung/clk.h
index 31b4174..0e43023 100644
--- a/drivers/clk/samsung/clk.h
+++ b/drivers/clk/samsung/clk.h
@@ -312,6 +312,37 @@ struct samsung_pll_clock {
 	__PLL(_typ, _id, NULL, _name, _pname, CLK_GET_RATE_NOCACHE,	\
 		_lock, _con, _rtable, _alias)
 
+/**
+ * struct samsung_core_clock_freq_table: table of frequency supported by
+ * a core clock and associated data if any.
+ * @freq: points to a table of supported frequencies (in KHz)
+ * @freq_count: number of entries in the frequency table
+ * @data: core clock specific data, if any
+ */
+struct samsung_core_clock_freq_table {
+	const unsigned long	*freq;       /* in KHz */
+	unsigned long		freq_count;
+	const void		*data;
+};
+
+/**
+ * struct samsung_core_clock: information about clock supplied to a CPU core.
+ * @hw: handle between ccf and core clock.
+ * @ctrl_base: base address of the clock controller.
+ * @fout_pll: clock handle representing the clock output of the associated PLL.
+ */
+struct samsung_core_clock {
+	struct clk_hw		hw;
+	void __iomem		*ctrl_base;
+	struct clk		*fout_pll;
+	const struct samsung_core_clock_freq_table *freq_table;
+};
+
+extern unsigned long samsung_core_clock_recalc_rate(struct clk_hw *hw,
+				unsigned long parent_rate);
+extern long samsung_core_clk_round_rate(struct clk_hw *hw,
+			unsigned long drate, unsigned long *prate);
+
 extern void __init samsung_clk_init(struct device_node *np, void __iomem *base,
 		unsigned long nr_clks, unsigned long *rdump,
 		unsigned long nr_rdump, unsigned long *soc_rdump,
@@ -337,7 +368,11 @@ extern void __init samsung_clk_register_gate(
 		struct samsung_gate_clock *clk_list, unsigned int nr_clk);
 extern void __init samsung_clk_register_pll(struct samsung_pll_clock *pll_list,
 		unsigned int nr_clk, void __iomem *base);
-
+extern void __init samsung_coreclk_register(const char *coreclk,
+		const char **parents, unsigned int num_parents,
+		const char *pllclk, const struct clk_ops *clk_ops,
+		unsigned int lookup_id,
+		const struct samsung_core_clock_freq_table *freq_tbl);
 extern unsigned long _get_rate(const char *clk_name);
 
 #endif /* __SAMSUNG_CLK_H */
-- 
1.6.6.rc2

Re: [PATCH 2/6] clk: samsung: add infrastructure to register CPU clocks

[Lukasz Majewski]

Hi Thomas,

> The CPU clock provider supplies the clock to the CPU clock domain. The
> composition and organization of the CPU clock provider could vary
> among Exynos SoCs. A CPU clock provider can be composed of clock mux,
> dividers and gates. This patch defines a new clock type for CPU clock
> provider and adds infrastructure to register the CPU clock providers
> for Samsung platforms.
> 
> Signed-off-by: Thomas Abraham <thomas.ab@samsung.com>
> ---
>  drivers/clk/samsung/clk.c |   71
> +++++++++++++++++++++++++++++++++++++++++++++
> drivers/clk/samsung/clk.h |   37 +++++++++++++++++++++++- 2 files
> changed, 107 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/clk/samsung/clk.c b/drivers/clk/samsung/clk.c
> index f503f32..9ac9056 100644
> --- a/drivers/clk/samsung/clk.c
> +++ b/drivers/clk/samsung/clk.c
> @@ -316,3 +316,74 @@ unsigned long _get_rate(const char *clk_name)
>  
>  	return clk_get_rate(clk);
>  }
> +
> +/*
> + * On most platform, the core clock rate is equal to the clock rate
> of
> + * parent pll. This is a simple helper function to support
> recalc_rate
> + * callback for such platforms.
> + */
> +unsigned long samsung_core_clock_recalc_rate(struct clk_hw *hw,
> +				unsigned long parent_rate)
> +{
> +	/*
> +	 * Assuming that the output of the parent pll is the output
> clock
> +	 * frequency of the core clock.
> +	 */
> +	return parent_rate;
> +}
> +
> +/* This is a helper function to perform clock rounding for core
> clocks. */ +long samsung_core_clk_round_rate(struct clk_hw *hw,
> +			unsigned long drate, unsigned long *prate)
> +{
> +	struct samsung_core_clock *core_clk;
> +	const struct samsung_core_clock_freq_table *freq_tbl;
> +	int i;
> +
> +	core_clk = container_of(hw, struct samsung_core_clock, hw);
> +	freq_tbl = core_clk->freq_table;
> +	drate /= 1000;
> +
> +	for (i = 0; i < freq_tbl->freq_count; i++) {
> +		if (drate >= freq_tbl->freq[i])
> +			return freq_tbl->freq[i] * 1000;
> +	}
> +	return freq_tbl->freq[i - 1] * 1000;
> +}
> +
> +/* helper function to register a core clock */
> +void __init samsung_coreclk_register(const char *name, const char
> **parents,
> +			unsigned int num_parents, const char *pllclk,
> +			const struct clk_ops *clk_ops, unsigned int
> lookup_id,
> +			const struct samsung_core_clock_freq_table
> *freq_tbl) +{
> +	struct samsung_core_clock *coreclk;
> +	struct clk_init_data init;
> +	struct clk *clk;
> +
> +	coreclk = kzalloc(sizeof(*coreclk), GFP_KERNEL);
> +	if (!coreclk) {
> +		pr_err("%s: could not allocate memory for coreclk
> %s\n",
> +					__func__, name);
> +		return;
> +	}
> +
> +	init.name = name;
> +	init.flags = CLK_GET_RATE_NOCACHE;
> +	init.parent_names = parents;
> +	init.num_parents = num_parents;
> +	init.ops = clk_ops;
> +
> +	coreclk->hw.init = &init;
> +	coreclk->ctrl_base = reg_base;
> +	coreclk->fout_pll = __clk_lookup(pllclk);
> +	coreclk->freq_table = freq_tbl;
> +
> +	clk = clk_register(NULL, &coreclk->hw);
> +	if (IS_ERR(clk)) {
> +		pr_err("%s: could not register coreclk %s\n",
> __func__,	name);
> +		kfree(coreclk);
> +		return;
> +	}
> +	samsung_clk_add_lookup(clk, lookup_id);
> +}

I think, that those definitions for samsung_core_clock shall be moved
to a separate file (maybe clk-exynos-cpu.c)?

Moreover, maybe you can choose a shorter name?

> diff --git a/drivers/clk/samsung/clk.h b/drivers/clk/samsung/clk.h
> index 31b4174..0e43023 100644
> --- a/drivers/clk/samsung/clk.h
> +++ b/drivers/clk/samsung/clk.h
> @@ -312,6 +312,37 @@ struct samsung_pll_clock {
>  	__PLL(_typ, _id, NULL, _name, _pname,
> CLK_GET_RATE_NOCACHE,	\ _lock, _con, _rtable, _alias)
>  
> +/**
> + * struct samsung_core_clock_freq_table: table of frequency
> supported by
> + * a core clock and associated data if any.
> + * @freq: points to a table of supported frequencies (in KHz)
> + * @freq_count: number of entries in the frequency table
> + * @data: core clock specific data, if any
> + */
> +struct samsung_core_clock_freq_table {
> +	const unsigned long	*freq;       /* in KHz */
> +	unsigned long		freq_count;
> +	const void		*data;
> +};

This is another instance of a structure, which holds the frequency for
the system (like struct cpufreq_frequency_table). 

Unfortunately, since the clk subsystem starts very early, the
cpufreq_frequency_table is not yet parsed from "operating-points"
attribute.

Maybe we can defer the clock registration?

> +
> +/**
> + * struct samsung_core_clock: information about clock supplied to a
> CPU core.
> + * @hw: handle between ccf and core clock.
> + * @ctrl_base: base address of the clock controller.
> + * @fout_pll: clock handle representing the clock output of the
> associated PLL.
> + */
> +struct samsung_core_clock {
> +	struct clk_hw		hw;
> +	void __iomem		*ctrl_base;
> +	struct clk		*fout_pll;
> +	const struct samsung_core_clock_freq_table *freq_table;
> +};
> +
> +extern unsigned long samsung_core_clock_recalc_rate(struct clk_hw
> *hw,
> +				unsigned long parent_rate);
> +extern long samsung_core_clk_round_rate(struct clk_hw *hw,
> +			unsigned long drate, unsigned long *prate);

IMHO, those methods shall be defined as static in a separate file.

> +
>  extern void __init samsung_clk_init(struct device_node *np, void
> __iomem *base, unsigned long nr_clks, unsigned long *rdump,
>  		unsigned long nr_rdump, unsigned long *soc_rdump,
> @@ -337,7 +368,11 @@ extern void __init samsung_clk_register_gate(
>  		struct samsung_gate_clock *clk_list, unsigned int
> nr_clk); extern void __init samsung_clk_register_pll(struct
> samsung_pll_clock *pll_list, unsigned int nr_clk, void __iomem *base);
> -
> +extern void __init samsung_coreclk_register(const char *coreclk,
> +		const char **parents, unsigned int num_parents,
> +		const char *pllclk, const struct clk_ops *clk_ops,
> +		unsigned int lookup_id,
> +		const struct samsung_core_clock_freq_table
> *freq_tbl); extern unsigned long _get_rate(const char *clk_name);
>  
>  #endif /* __SAMSUNG_CLK_H */



-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

Re: [PATCH 2/6] clk: samsung: add infrastructure to register CPU clocks

[Thomas Abraham]

Hi Lukasz,

On Fri, Jan 10, 2014 at 5:34 PM, Lukasz Majewski <l.majewski@samsung.com> wrote:
> Hi Thomas,
>
>> The CPU clock provider supplies the clock to the CPU clock domain. The
>> composition and organization of the CPU clock provider could vary
>> among Exynos SoCs. A CPU clock provider can be composed of clock mux,
>> dividers and gates. This patch defines a new clock type for CPU clock
>> provider and adds infrastructure to register the CPU clock providers
>> for Samsung platforms.
>>
>> Signed-off-by: Thomas Abraham <thomas.ab@samsung.com>
>> ---
>>  drivers/clk/samsung/clk.c |   71
>> +++++++++++++++++++++++++++++++++++++++++++++
>> drivers/clk/samsung/clk.h |   37 +++++++++++++++++++++++- 2 files
>> changed, 107 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/clk/samsung/clk.c b/drivers/clk/samsung/clk.c
>> index f503f32..9ac9056 100644
>> --- a/drivers/clk/samsung/clk.c
>> +++ b/drivers/clk/samsung/clk.c
>> @@ -316,3 +316,74 @@ unsigned long _get_rate(const char *clk_name)
>>
>>       return clk_get_rate(clk);
>>  }
>> +
>> +/*
>> + * On most platform, the core clock rate is equal to the clock rate
>> of
>> + * parent pll. This is a simple helper function to support
>> recalc_rate
>> + * callback for such platforms.
>> + */
>> +unsigned long samsung_core_clock_recalc_rate(struct clk_hw *hw,
>> +                             unsigned long parent_rate)
>> +{
>> +     /*
>> +      * Assuming that the output of the parent pll is the output
>> clock
>> +      * frequency of the core clock.
>> +      */
>> +     return parent_rate;
>> +}
>> +
>> +/* This is a helper function to perform clock rounding for core
>> clocks. */ +long samsung_core_clk_round_rate(struct clk_hw *hw,
>> +                     unsigned long drate, unsigned long *prate)
>> +{
>> +     struct samsung_core_clock *core_clk;
>> +     const struct samsung_core_clock_freq_table *freq_tbl;
>> +     int i;
>> +
>> +     core_clk = container_of(hw, struct samsung_core_clock, hw);
>> +     freq_tbl = core_clk->freq_table;
>> +     drate /= 1000;
>> +
>> +     for (i = 0; i < freq_tbl->freq_count; i++) {
>> +             if (drate >= freq_tbl->freq[i])
>> +                     return freq_tbl->freq[i] * 1000;
>> +     }
>> +     return freq_tbl->freq[i - 1] * 1000;
>> +}
>> +
>> +/* helper function to register a core clock */
>> +void __init samsung_coreclk_register(const char *name, const char
>> **parents,
>> +                     unsigned int num_parents, const char *pllclk,
>> +                     const struct clk_ops *clk_ops, unsigned int
>> lookup_id,
>> +                     const struct samsung_core_clock_freq_table
>> *freq_tbl) +{
>> +     struct samsung_core_clock *coreclk;
>> +     struct clk_init_data init;
>> +     struct clk *clk;
>> +
>> +     coreclk = kzalloc(sizeof(*coreclk), GFP_KERNEL);
>> +     if (!coreclk) {
>> +             pr_err("%s: could not allocate memory for coreclk
>> %s\n",
>> +                                     __func__, name);
>> +             return;
>> +     }
>> +
>> +     init.name = name;
>> +     init.flags = CLK_GET_RATE_NOCACHE;
>> +     init.parent_names = parents;
>> +     init.num_parents = num_parents;
>> +     init.ops = clk_ops;
>> +
>> +     coreclk->hw.init = &init;
>> +     coreclk->ctrl_base = reg_base;
>> +     coreclk->fout_pll = __clk_lookup(pllclk);
>> +     coreclk->freq_table = freq_tbl;
>> +
>> +     clk = clk_register(NULL, &coreclk->hw);
>> +     if (IS_ERR(clk)) {
>> +             pr_err("%s: could not register coreclk %s\n",
>> __func__,     name);
>> +             kfree(coreclk);
>> +             return;
>> +     }
>> +     samsung_clk_add_lookup(clk, lookup_id);
>> +}
>
> I think, that those definitions for samsung_core_clock shall be moved
> to a separate file (maybe clk-exynos-cpu.c)?

The additions were not really much and the samsung/clk.c file is used
to hold all the common code for Samsung SoCs. So the additions were
put into this existing file itself.

>
> Moreover, maybe you can choose a shorter name?

Ok, I will try to find a shorter name for in the next version.

>
>> diff --git a/drivers/clk/samsung/clk.h b/drivers/clk/samsung/clk.h
>> index 31b4174..0e43023 100644
>> --- a/drivers/clk/samsung/clk.h
>> +++ b/drivers/clk/samsung/clk.h
>> @@ -312,6 +312,37 @@ struct samsung_pll_clock {
>>       __PLL(_typ, _id, NULL, _name, _pname,
>> CLK_GET_RATE_NOCACHE, \ _lock, _con, _rtable, _alias)
>>
>> +/**
>> + * struct samsung_core_clock_freq_table: table of frequency
>> supported by
>> + * a core clock and associated data if any.
>> + * @freq: points to a table of supported frequencies (in KHz)
>> + * @freq_count: number of entries in the frequency table
>> + * @data: core clock specific data, if any
>> + */
>> +struct samsung_core_clock_freq_table {
>> +     const unsigned long     *freq;       /* in KHz */
>> +     unsigned long           freq_count;
>> +     const void              *data;
>> +};
>
> This is another instance of a structure, which holds the frequency for
> the system (like struct cpufreq_frequency_table).
>
> Unfortunately, since the clk subsystem starts very early, the
> cpufreq_frequency_table is not yet parsed from "operating-points"
> attribute.
>
> Maybe we can defer the clock registration?

The core clock is not limited to support only those frequencies which
the CPU operating point specifies. The core clock type can be used
independent of the cpufreq driver and hence has its own set of
supported frequencies. The cpufreq driver, depending the operating
points allowed, can request the core clock to setup the requested
clock speed.

>
>> +
>> +/**
>> + * struct samsung_core_clock: information about clock supplied to a
>> CPU core.
>> + * @hw: handle between ccf and core clock.
>> + * @ctrl_base: base address of the clock controller.
>> + * @fout_pll: clock handle representing the clock output of the
>> associated PLL.
>> + */
>> +struct samsung_core_clock {
>> +     struct clk_hw           hw;
>> +     void __iomem            *ctrl_base;
>> +     struct clk              *fout_pll;
>> +     const struct samsung_core_clock_freq_table *freq_table;
>> +};
>> +
>> +extern unsigned long samsung_core_clock_recalc_rate(struct clk_hw
>> *hw,
>> +                             unsigned long parent_rate);
>> +extern long samsung_core_clk_round_rate(struct clk_hw *hw,
>> +                     unsigned long drate, unsigned long *prate);
>
> IMHO, those methods shall be defined as static in a separate file.

These two functions are generic enough to be reused for core clock
implementations on multiple exynos platforms. Only the set_rate
callback is the one that will be specific to SoC and core clock. The
SoC specific implementation can reuse these two callback funcitons
when registering the core clock type. That is the reason these are not
static functions. The 3/6 patch of this series includes the usage of
these functions.

Thanks,
Thomas.

>
>> +
>>  extern void __init samsung_clk_init(struct device_node *np, void
>> __iomem *base, unsigned long nr_clks, unsigned long *rdump,
>>               unsigned long nr_rdump, unsigned long *soc_rdump,
>> @@ -337,7 +368,11 @@ extern void __init samsung_clk_register_gate(
>>               struct samsung_gate_clock *clk_list, unsigned int
>> nr_clk); extern void __init samsung_clk_register_pll(struct
>> samsung_pll_clock *pll_list, unsigned int nr_clk, void __iomem *base);
>> -
>> +extern void __init samsung_coreclk_register(const char *coreclk,
>> +             const char **parents, unsigned int num_parents,
>> +             const char *pllclk, const struct clk_ops *clk_ops,
>> +             unsigned int lookup_id,
>> +             const struct samsung_core_clock_freq_table
>> *freq_tbl); extern unsigned long _get_rate(const char *clk_name);
>>
>>  #endif /* __SAMSUNG_CLK_H */
>
>
>
> --
> Best regards,
>
> Lukasz Majewski
>
> Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

Re: [PATCH 2/6] clk: samsung: add infrastructure to register CPU clocks

[Lukasz Majewski]

Hi Thomas,

> Hi Lukasz,
> 
> On Fri, Jan 10, 2014 at 5:34 PM, Lukasz Majewski
> <l.majewski@samsung.com> wrote:
> > Hi Thomas,
> >
> >> The CPU clock provider supplies the clock to the CPU clock domain.
> >> The composition and organization of the CPU clock provider could
> >> vary among Exynos SoCs. A CPU clock provider can be composed of
> >> clock mux, dividers and gates. This patch defines a new clock type
> >> for CPU clock provider and adds infrastructure to register the CPU
> >> clock providers for Samsung platforms.
> >>
> >> Signed-off-by: Thomas Abraham <thomas.ab@samsung.com>
> >> ---
> >>  drivers/clk/samsung/clk.c |   71
> >> +++++++++++++++++++++++++++++++++++++++++++++
> >> drivers/clk/samsung/clk.h |   37 +++++++++++++++++++++++- 2 files
> >> changed, 107 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/drivers/clk/samsung/clk.c b/drivers/clk/samsung/clk.c
> >> index f503f32..9ac9056 100644
> >> --- a/drivers/clk/samsung/clk.c
> >> +++ b/drivers/clk/samsung/clk.c
> >> @@ -316,3 +316,74 @@ unsigned long _get_rate(const char *clk_name)
> >>
> >>       return clk_get_rate(clk);
> >>  }
> >> +
> >> +/*
> >> + * On most platform, the core clock rate is equal to the clock
> >> rate of
> >> + * parent pll. This is a simple helper function to support
> >> recalc_rate
> >> + * callback for such platforms.
> >> + */
> >> +unsigned long samsung_core_clock_recalc_rate(struct clk_hw *hw,
> >> +                             unsigned long parent_rate)
> >> +{
> >> +     /*
> >> +      * Assuming that the output of the parent pll is the output
> >> clock
> >> +      * frequency of the core clock.
> >> +      */
> >> +     return parent_rate;
> >> +}
> >> +
> >> +/* This is a helper function to perform clock rounding for core
> >> clocks. */ +long samsung_core_clk_round_rate(struct clk_hw *hw,
> >> +                     unsigned long drate, unsigned long *prate)
> >> +{
> >> +     struct samsung_core_clock *core_clk;
> >> +     const struct samsung_core_clock_freq_table *freq_tbl;
> >> +     int i;
> >> +
> >> +     core_clk = container_of(hw, struct samsung_core_clock, hw);
> >> +     freq_tbl = core_clk->freq_table;
> >> +     drate /= 1000;
> >> +
> >> +     for (i = 0; i < freq_tbl->freq_count; i++) {
> >> +             if (drate >= freq_tbl->freq[i])
> >> +                     return freq_tbl->freq[i] * 1000;
> >> +     }
> >> +     return freq_tbl->freq[i - 1] * 1000;
> >> +}
> >> +
> >> +/* helper function to register a core clock */
> >> +void __init samsung_coreclk_register(const char *name, const char
> >> **parents,
> >> +                     unsigned int num_parents, const char *pllclk,
> >> +                     const struct clk_ops *clk_ops, unsigned int
> >> lookup_id,
> >> +                     const struct samsung_core_clock_freq_table
> >> *freq_tbl) +{
> >> +     struct samsung_core_clock *coreclk;
> >> +     struct clk_init_data init;
> >> +     struct clk *clk;
> >> +
> >> +     coreclk = kzalloc(sizeof(*coreclk), GFP_KERNEL);
> >> +     if (!coreclk) {
> >> +             pr_err("%s: could not allocate memory for coreclk
> >> %s\n",
> >> +                                     __func__, name);
> >> +             return;
> >> +     }
> >> +
> >> +     init.name = name;
> >> +     init.flags = CLK_GET_RATE_NOCACHE;
> >> +     init.parent_names = parents;
> >> +     init.num_parents = num_parents;
> >> +     init.ops = clk_ops;
> >> +
> >> +     coreclk->hw.init = &init;
> >> +     coreclk->ctrl_base = reg_base;
> >> +     coreclk->fout_pll = __clk_lookup(pllclk);
> >> +     coreclk->freq_table = freq_tbl;
> >> +
> >> +     clk = clk_register(NULL, &coreclk->hw);
> >> +     if (IS_ERR(clk)) {
> >> +             pr_err("%s: could not register coreclk %s\n",
> >> __func__,     name);
> >> +             kfree(coreclk);
> >> +             return;
> >> +     }
> >> +     samsung_clk_add_lookup(clk, lookup_id);
> >> +}
> >
> > I think, that those definitions for samsung_core_clock shall be
> > moved to a separate file (maybe clk-exynos-cpu.c)?
> 
> The additions were not really much and the samsung/clk.c file is used
> to hold all the common code for Samsung SoCs. So the additions were
> put into this existing file itself.
> 
> >
> > Moreover, maybe you can choose a shorter name?
> 
> Ok, I will try to find a shorter name for in the next version.
> 
> >
> >> diff --git a/drivers/clk/samsung/clk.h b/drivers/clk/samsung/clk.h
> >> index 31b4174..0e43023 100644
> >> --- a/drivers/clk/samsung/clk.h
> >> +++ b/drivers/clk/samsung/clk.h
> >> @@ -312,6 +312,37 @@ struct samsung_pll_clock {
> >>       __PLL(_typ, _id, NULL, _name, _pname,
> >> CLK_GET_RATE_NOCACHE, \ _lock, _con, _rtable, _alias)
> >>
> >> +/**
> >> + * struct samsung_core_clock_freq_table: table of frequency
> >> supported by
> >> + * a core clock and associated data if any.
> >> + * @freq: points to a table of supported frequencies (in KHz)
> >> + * @freq_count: number of entries in the frequency table
> >> + * @data: core clock specific data, if any
> >> + */
> >> +struct samsung_core_clock_freq_table {
> >> +     const unsigned long     *freq;       /* in KHz */
> >> +     unsigned long           freq_count;
> >> +     const void              *data;
> >> +};
> >
> > This is another instance of a structure, which holds the frequency
> > for the system (like struct cpufreq_frequency_table).
> >
> > Unfortunately, since the clk subsystem starts very early, the
> > cpufreq_frequency_table is not yet parsed from "operating-points"
> > attribute.
> >
> > Maybe we can defer the clock registration?
> 
> The core clock is not limited to support only those frequencies which
> the CPU operating point specifies. The core clock type can be used
> independent of the cpufreq driver and hence has its own set of
> supported frequencies. The cpufreq driver, depending the operating
> points allowed, can request the core clock to setup the requested
> clock speed.

The above would be true if it could have been possible to use this
clock without cpufreq.

Since, justification of this whole change is the clean up of cpufreq for
Exynos, I assume that it will be only used with cpufreq. Is there any
use case to use this clock without cpufreq?

For this reason Exynos SoCs can only change frequency to the one, which
is defined at cpufreq_frequency_table.

And since the cpufreq_frequency_table is dynamically build from
operating-points attribute, then in my opinion we shall refer to only
one place for frequency.

Similar approach - of reusing cpufreq_frequency_table is used at
thermal core driver.

> 
> >
> >> +
> >> +/**
> >> + * struct samsung_core_clock: information about clock supplied to
> >> a CPU core.
> >> + * @hw: handle between ccf and core clock.
> >> + * @ctrl_base: base address of the clock controller.
> >> + * @fout_pll: clock handle representing the clock output of the
> >> associated PLL.
> >> + */
> >> +struct samsung_core_clock {
> >> +     struct clk_hw           hw;
> >> +     void __iomem            *ctrl_base;
> >> +     struct clk              *fout_pll;
> >> +     const struct samsung_core_clock_freq_table *freq_table;
> >> +};
> >> +
> >> +extern unsigned long samsung_core_clock_recalc_rate(struct clk_hw
> >> *hw,
> >> +                             unsigned long parent_rate);
> >> +extern long samsung_core_clk_round_rate(struct clk_hw *hw,
> >> +                     unsigned long drate, unsigned long *prate);
> >
> > IMHO, those methods shall be defined as static in a separate file.
> 
> These two functions are generic enough to be reused for core clock
> implementations on multiple exynos platforms. 

I thought that you are going to implement this clock in a generic way,
so by using the DT we could use it for Exynos4210/4x12 and 5250.

> Only the set_rate
> callback is the one that will be specific to SoC and core clock. The
> SoC specific implementation can reuse these two callback funcitons
> when registering the core clock type. That is the reason these are not
> static functions. The 3/6 patch of this series includes the usage of
> these functions.

In my opinion we shall not duplicate per SoC constructs like for
example:

static struct apll_freq apll_freq_4412[]
static struct apll_freq apll_freq_4212[]
static struct apll_freq apll_freq_4210[]

which would be used at different ->set_rate() callbacks.

I think that the best solution would be to use one ->set_rate() with
data parsed from DT.

> 
> Thanks,
> Thomas.
> 
> >
> >> +
> >>  extern void __init samsung_clk_init(struct device_node *np, void
> >> __iomem *base, unsigned long nr_clks, unsigned long *rdump,
> >>               unsigned long nr_rdump, unsigned long *soc_rdump,
> >> @@ -337,7 +368,11 @@ extern void __init samsung_clk_register_gate(
> >>               struct samsung_gate_clock *clk_list, unsigned int
> >> nr_clk); extern void __init samsung_clk_register_pll(struct
> >> samsung_pll_clock *pll_list, unsigned int nr_clk, void __iomem
> >> *base); -
> >> +extern void __init samsung_coreclk_register(const char *coreclk,
> >> +             const char **parents, unsigned int num_parents,
> >> +             const char *pllclk, const struct clk_ops *clk_ops,
> >> +             unsigned int lookup_id,
> >> +             const struct samsung_core_clock_freq_table
> >> *freq_tbl); extern unsigned long _get_rate(const char *clk_name);
> >>
> >>  #endif /* __SAMSUNG_CLK_H */
> >
> >
> >
> > --
> > Best regards,
> >
> > Lukasz Majewski
> >
> > Samsung R&D Institute Poland (SRPOL) | Linux Platform Group



-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

Re: [PATCH 2/6] clk: samsung: add infrastructure to register CPU clocks

[Thomas Abraham]

On Fri, Jan 10, 2014 at 6:55 PM, Lukasz Majewski <l.majewski@samsung.com> wrote:
> Hi Thomas,
>
>> Hi Lukasz,
>>
>> On Fri, Jan 10, 2014 at 5:34 PM, Lukasz Majewski
>> <l.majewski@samsung.com> wrote:
>> > Hi Thomas,
>> >
>> >> The CPU clock provider supplies the clock to the CPU clock domain.
>> >> The composition and organization of the CPU clock provider could
>> >> vary among Exynos SoCs. A CPU clock provider can be composed of
>> >> clock mux, dividers and gates. This patch defines a new clock type
>> >> for CPU clock provider and adds infrastructure to register the CPU
>> >> clock providers for Samsung platforms.
>> >>
>> >> Signed-off-by: Thomas Abraham <thomas.ab@samsung.com>
>> >> ---
>> >>  drivers/clk/samsung/clk.c |   71
>> >> +++++++++++++++++++++++++++++++++++++++++++++
>> >> drivers/clk/samsung/clk.h |   37 +++++++++++++++++++++++- 2 files
>> >> changed, 107 insertions(+), 1 deletions(-)
>> >>
>> >> diff --git a/drivers/clk/samsung/clk.c b/drivers/clk/samsung/clk.c
>> >> index f503f32..9ac9056 100644
>> >> --- a/drivers/clk/samsung/clk.c
>> >> +++ b/drivers/clk/samsung/clk.c
>> >> @@ -316,3 +316,74 @@ unsigned long _get_rate(const char *clk_name)
>> >>
>> >>       return clk_get_rate(clk);
>> >>  }
>> >> +
>> >> +/*
>> >> + * On most platform, the core clock rate is equal to the clock
>> >> rate of
>> >> + * parent pll. This is a simple helper function to support
>> >> recalc_rate
>> >> + * callback for such platforms.
>> >> + */
>> >> +unsigned long samsung_core_clock_recalc_rate(struct clk_hw *hw,
>> >> +                             unsigned long parent_rate)
>> >> +{
>> >> +     /*
>> >> +      * Assuming that the output of the parent pll is the output
>> >> clock
>> >> +      * frequency of the core clock.
>> >> +      */
>> >> +     return parent_rate;
>> >> +}
>> >> +
>> >> +/* This is a helper function to perform clock rounding for core
>> >> clocks. */ +long samsung_core_clk_round_rate(struct clk_hw *hw,
>> >> +                     unsigned long drate, unsigned long *prate)
>> >> +{
>> >> +     struct samsung_core_clock *core_clk;
>> >> +     const struct samsung_core_clock_freq_table *freq_tbl;
>> >> +     int i;
>> >> +
>> >> +     core_clk = container_of(hw, struct samsung_core_clock, hw);
>> >> +     freq_tbl = core_clk->freq_table;
>> >> +     drate /= 1000;
>> >> +
>> >> +     for (i = 0; i < freq_tbl->freq_count; i++) {
>> >> +             if (drate >= freq_tbl->freq[i])
>> >> +                     return freq_tbl->freq[i] * 1000;
>> >> +     }
>> >> +     return freq_tbl->freq[i - 1] * 1000;
>> >> +}
>> >> +
>> >> +/* helper function to register a core clock */
>> >> +void __init samsung_coreclk_register(const char *name, const char
>> >> **parents,
>> >> +                     unsigned int num_parents, const char *pllclk,
>> >> +                     const struct clk_ops *clk_ops, unsigned int
>> >> lookup_id,
>> >> +                     const struct samsung_core_clock_freq_table
>> >> *freq_tbl) +{
>> >> +     struct samsung_core_clock *coreclk;
>> >> +     struct clk_init_data init;
>> >> +     struct clk *clk;
>> >> +
>> >> +     coreclk = kzalloc(sizeof(*coreclk), GFP_KERNEL);
>> >> +     if (!coreclk) {
>> >> +             pr_err("%s: could not allocate memory for coreclk
>> >> %s\n",
>> >> +                                     __func__, name);
>> >> +             return;
>> >> +     }
>> >> +
>> >> +     init.name = name;
>> >> +     init.flags = CLK_GET_RATE_NOCACHE;
>> >> +     init.parent_names = parents;
>> >> +     init.num_parents = num_parents;
>> >> +     init.ops = clk_ops;
>> >> +
>> >> +     coreclk->hw.init = &init;
>> >> +     coreclk->ctrl_base = reg_base;
>> >> +     coreclk->fout_pll = __clk_lookup(pllclk);
>> >> +     coreclk->freq_table = freq_tbl;
>> >> +
>> >> +     clk = clk_register(NULL, &coreclk->hw);
>> >> +     if (IS_ERR(clk)) {
>> >> +             pr_err("%s: could not register coreclk %s\n",
>> >> __func__,     name);
>> >> +             kfree(coreclk);
>> >> +             return;
>> >> +     }
>> >> +     samsung_clk_add_lookup(clk, lookup_id);
>> >> +}
>> >
>> > I think, that those definitions for samsung_core_clock shall be
>> > moved to a separate file (maybe clk-exynos-cpu.c)?
>>
>> The additions were not really much and the samsung/clk.c file is used
>> to hold all the common code for Samsung SoCs. So the additions were
>> put into this existing file itself.
>>
>> >
>> > Moreover, maybe you can choose a shorter name?
>>
>> Ok, I will try to find a shorter name for in the next version.
>>
>> >
>> >> diff --git a/drivers/clk/samsung/clk.h b/drivers/clk/samsung/clk.h
>> >> index 31b4174..0e43023 100644
>> >> --- a/drivers/clk/samsung/clk.h
>> >> +++ b/drivers/clk/samsung/clk.h
>> >> @@ -312,6 +312,37 @@ struct samsung_pll_clock {
>> >>       __PLL(_typ, _id, NULL, _name, _pname,
>> >> CLK_GET_RATE_NOCACHE, \ _lock, _con, _rtable, _alias)
>> >>
>> >> +/**
>> >> + * struct samsung_core_clock_freq_table: table of frequency
>> >> supported by
>> >> + * a core clock and associated data if any.
>> >> + * @freq: points to a table of supported frequencies (in KHz)
>> >> + * @freq_count: number of entries in the frequency table
>> >> + * @data: core clock specific data, if any
>> >> + */
>> >> +struct samsung_core_clock_freq_table {
>> >> +     const unsigned long     *freq;       /* in KHz */
>> >> +     unsigned long           freq_count;
>> >> +     const void              *data;
>> >> +};
>> >
>> > This is another instance of a structure, which holds the frequency
>> > for the system (like struct cpufreq_frequency_table).
>> >
>> > Unfortunately, since the clk subsystem starts very early, the
>> > cpufreq_frequency_table is not yet parsed from "operating-points"
>> > attribute.
>> >
>> > Maybe we can defer the clock registration?
>>
>> The core clock is not limited to support only those frequencies which
>> the CPU operating point specifies. The core clock type can be used
>> independent of the cpufreq driver and hence has its own set of
>> supported frequencies. The cpufreq driver, depending the operating
>> points allowed, can request the core clock to setup the requested
>> clock speed.
>
> The above would be true if it could have been possible to use this
> clock without cpufreq.
>
> Since, justification of this whole change is the clean up of cpufreq for
> Exynos, I assume that it will be only used with cpufreq. Is there any
> use case to use this clock without cpufreq?
>
> For this reason Exynos SoCs can only change frequency to the one, which
> is defined at cpufreq_frequency_table.
>
> And since the cpufreq_frequency_table is dynamically build from
> operating-points attribute, then in my opinion we shall refer to only
> one place for frequency.
>
> Similar approach - of reusing cpufreq_frequency_table is used at
> thermal core driver.

One of the reasons for keeping the supported frequencies independent
of the cpufreq table was ensure that platforms are free to use only a
subset of the supported clock speed due to certain performance/power
requirements particular to that platform Two or more platforms based
on the same SoC can have different power/performance requirements. The
case of thermal is different, it can only request for the CPU to be
clocked at speeds supported by the cpufreq driver.

Also, the core clock type is not limited to be used only by the
cpufreq driver. It should be possible to use a platform without the
cpufreq driver but still have the flexibility to change the cpu clock
speed.

>
>>
>> >
>> >> +
>> >> +/**
>> >> + * struct samsung_core_clock: information about clock supplied to
>> >> a CPU core.
>> >> + * @hw: handle between ccf and core clock.
>> >> + * @ctrl_base: base address of the clock controller.
>> >> + * @fout_pll: clock handle representing the clock output of the
>> >> associated PLL.
>> >> + */
>> >> +struct samsung_core_clock {
>> >> +     struct clk_hw           hw;
>> >> +     void __iomem            *ctrl_base;
>> >> +     struct clk              *fout_pll;
>> >> +     const struct samsung_core_clock_freq_table *freq_table;
>> >> +};
>> >> +
>> >> +extern unsigned long samsung_core_clock_recalc_rate(struct clk_hw
>> >> *hw,
>> >> +                             unsigned long parent_rate);
>> >> +extern long samsung_core_clk_round_rate(struct clk_hw *hw,
>> >> +                     unsigned long drate, unsigned long *prate);
>> >
>> > IMHO, those methods shall be defined as static in a separate file.
>>
>> These two functions are generic enough to be reused for core clock
>> implementations on multiple exynos platforms.
>
> I thought that you are going to implement this clock in a generic way,
> so by using the DT we could use it for Exynos4210/4x12 and 5250.
>
>> Only the set_rate
>> callback is the one that will be specific to SoC and core clock. The
>> SoC specific implementation can reuse these two callback funcitons
>> when registering the core clock type. That is the reason these are not
>> static functions. The 3/6 patch of this series includes the usage of
>> these functions.
>
> In my opinion we shall not duplicate per SoC constructs like for
> example:
>
> static struct apll_freq apll_freq_4412[]
> static struct apll_freq apll_freq_4212[]
> static struct apll_freq apll_freq_4210[]
>
> which would be used at different ->set_rate() callbacks.
>
> I think that the best solution would be to use one ->set_rate() with
> data parsed from DT.

Yes, I agree with you. The ->set_rate() callback code is not going to
be repeated if the code can be reused for other Exynos SoCs.

And about the values of the registers for set_rate coming in from DT,
these are SoC specific values and are not different for two or more
platforms using the same SoC. So why put this into DT? The same was
the argument about populating all the clocks from DT. Clock blocks are
not going to change from one platform to another platform which use
the same SoC. They are SoC specific and not platform specific and
hence there is little use in putting them into DT.

>
>>
>> Thanks,
>> Thomas.
>>
>> >
>> >> +
>> >>  extern void __init samsung_clk_init(struct device_node *np, void
>> >> __iomem *base, unsigned long nr_clks, unsigned long *rdump,
>> >>               unsigned long nr_rdump, unsigned long *soc_rdump,
>> >> @@ -337,7 +368,11 @@ extern void __init samsung_clk_register_gate(
>> >>               struct samsung_gate_clock *clk_list, unsigned int
>> >> nr_clk); extern void __init samsung_clk_register_pll(struct
>> >> samsung_pll_clock *pll_list, unsigned int nr_clk, void __iomem
>> >> *base); -
>> >> +extern void __init samsung_coreclk_register(const char *coreclk,
>> >> +             const char **parents, unsigned int num_parents,
>> >> +             const char *pllclk, const struct clk_ops *clk_ops,
>> >> +             unsigned int lookup_id,
>> >> +             const struct samsung_core_clock_freq_table
>> >> *freq_tbl); extern unsigned long _get_rate(const char *clk_name);
>> >>
>> >>  #endif /* __SAMSUNG_CLK_H */
>> >
>> >
>> >
>> > --
>> > Best regards,
>> >
>> > Lukasz Majewski
>> >
>> > Samsung R&D Institute Poland (SRPOL) | Linux Platform Group
>
>
>
> --
> Best regards,
>
> Lukasz Majewski
>
> Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

Re: [PATCH 2/6] clk: samsung: add infrastructure to register CPU clocks

[Tomasz Figa]

Hi Thomas, Lukasz,

Let me put my two cents in.

On 11.01.2014 05:43, Thomas Abraham wrote:
> On Fri, Jan 10, 2014 at 6:55 PM, Lukasz Majewski <l.majewski@samsung.com> wrote:
>> Hi Thomas,
>>
>>> Hi Lukasz,
>>>
>>> On Fri, Jan 10, 2014 at 5:34 PM, Lukasz Majewski
>>> <l.majewski@samsung.com> wrote:
[snip]
>>>>> +/* helper function to register a core clock */
>>>>> +void __init samsung_coreclk_register(const char *name, const char
>>>>> **parents,
>>>>> +                     unsigned int num_parents, const char *pllclk,
>>>>> +                     const struct clk_ops *clk_ops, unsigned int
>>>>> lookup_id,
>>>>> +                     const struct samsung_core_clock_freq_table
>>>>> *freq_tbl) +{
>>>>> +     struct samsung_core_clock *coreclk;
>>>>> +     struct clk_init_data init;
>>>>> +     struct clk *clk;
>>>>> +
>>>>> +     coreclk = kzalloc(sizeof(*coreclk), GFP_KERNEL);
>>>>> +     if (!coreclk) {
>>>>> +             pr_err("%s: could not allocate memory for coreclk
>>>>> %s\n",
>>>>> +                                     __func__, name);
>>>>> +             return;
>>>>> +     }
>>>>> +
>>>>> +     init.name = name;
>>>>> +     init.flags = CLK_GET_RATE_NOCACHE;
>>>>> +     init.parent_names = parents;
>>>>> +     init.num_parents = num_parents;
>>>>> +     init.ops = clk_ops;
>>>>> +
>>>>> +     coreclk->hw.init = &init;
>>>>> +     coreclk->ctrl_base = reg_base;
>>>>> +     coreclk->fout_pll = __clk_lookup(pllclk);
>>>>> +     coreclk->freq_table = freq_tbl;
>>>>> +
>>>>> +     clk = clk_register(NULL, &coreclk->hw);
>>>>> +     if (IS_ERR(clk)) {
>>>>> +             pr_err("%s: could not register coreclk %s\n",
>>>>> __func__,     name);
>>>>> +             kfree(coreclk);
>>>>> +             return;
>>>>> +     }
>>>>> +     samsung_clk_add_lookup(clk, lookup_id);
>>>>> +}
>>>>
>>>> I think, that those definitions for samsung_core_clock shall be
>>>> moved to a separate file (maybe clk-exynos-cpu.c)?
>>>
>>> The additions were not really much and the samsung/clk.c file is used
>>> to hold all the common code for Samsung SoCs. So the additions were
>>> put into this existing file itself.

Still, separation between clock drivers and clock providers would be 
nice. clk-exynos4.c and friends doesn't implement any clock handling 
logic, they just define the tree of available clocks. I agree with 
Lukasz that separate file would be better place for such code, just as 
it was done with PLL drivers.

In addition this would allow you to make the generic core clock helpers 
static, as they could be moved to the same file where all the core clock 
driver variants are implemented.

>>>
>>>>
>>>> Moreover, maybe you can choose a shorter name?
>>>
>>> Ok, I will try to find a shorter name for in the next version.

I'm not sure how the name (or which name) could be shorter here, without 
impairing readability. Lukasz, can you elaborate?

>>>
>>>>
>>>>> diff --git a/drivers/clk/samsung/clk.h b/drivers/clk/samsung/clk.h
>>>>> index 31b4174..0e43023 100644
>>>>> --- a/drivers/clk/samsung/clk.h
>>>>> +++ b/drivers/clk/samsung/clk.h
>>>>> @@ -312,6 +312,37 @@ struct samsung_pll_clock {
>>>>>        __PLL(_typ, _id, NULL, _name, _pname,
>>>>> CLK_GET_RATE_NOCACHE, \ _lock, _con, _rtable, _alias)
>>>>>
>>>>> +/**
>>>>> + * struct samsung_core_clock_freq_table: table of frequency
>>>>> supported by
>>>>> + * a core clock and associated data if any.
>>>>> + * @freq: points to a table of supported frequencies (in KHz)
>>>>> + * @freq_count: number of entries in the frequency table
>>>>> + * @data: core clock specific data, if any
>>>>> + */
>>>>> +struct samsung_core_clock_freq_table {
>>>>> +     const unsigned long     *freq;       /* in KHz */
>>>>> +     unsigned long           freq_count;
>>>>> +     const void              *data;
>>>>> +};
>>>>
>>>> This is another instance of a structure, which holds the frequency
>>>> for the system (like struct cpufreq_frequency_table).
>>>>
>>>> Unfortunately, since the clk subsystem starts very early, the
>>>> cpufreq_frequency_table is not yet parsed from "operating-points"
>>>> attribute.
>>>>
>>>> Maybe we can defer the clock registration?
>>>
>>> The core clock is not limited to support only those frequencies which
>>> the CPU operating point specifies. The core clock type can be used
>>> independent of the cpufreq driver and hence has its own set of
>>> supported frequencies. The cpufreq driver, depending the operating
>>> points allowed, can request the core clock to setup the requested
>>> clock speed.
>>
>> The above would be true if it could have been possible to use this
>> clock without cpufreq.
>>
>> Since, justification of this whole change is the clean up of cpufreq for
>> Exynos, I assume that it will be only used with cpufreq. Is there any
>> use case to use this clock without cpufreq?
>>
>> For this reason Exynos SoCs can only change frequency to the one, which
>> is defined at cpufreq_frequency_table.
>>
>> And since the cpufreq_frequency_table is dynamically build from
>> operating-points attribute, then in my opinion we shall refer to only
>> one place for frequency.
>>
>> Similar approach - of reusing cpufreq_frequency_table is used at
>> thermal core driver.
>
> One of the reasons for keeping the supported frequencies independent
> of the cpufreq table was ensure that platforms are free to use only a
> subset of the supported clock speed due to certain performance/power
> requirements particular to that platform Two or more platforms based
> on the same SoC can have different power/performance requirements. The
> case of thermal is different, it can only request for the CPU to be
> clocked at speeds supported by the cpufreq driver.
>
> Also, the core clock type is not limited to be used only by the
> cpufreq driver. It should be possible to use a platform without the
> cpufreq driver but still have the flexibility to change the cpu clock
> speed.
>

I don't think the core clock is going to be used (in reconfigurable way) 
with anything else than cpufreq. I agree with Thomas, though, that clock 
drivers should not have any dependencies on other subsystems. Separation 
is always good.

However, Lukasz has a valid point. Your patch is adding third place 
where list of CPU frequencies is defined. We really shouldn't have 
allowed this to be scattered into two pieces with common clock framework 
implementation, not even saying about three. This is data redundancy 
that is not only error prone, but also a maintenance issue, because 
whenever one wants to change the set of supported frequencies, they must 
modify all two (or three) locations.

My stance on this is that we should get rid of this redundancy and I 
won't Ack any patch that makes us further from this goal, unless you can 
convince me that there is no other option.

As for how I see solution for this, I believe that possible APLL 
settings, core clock settings and cpufreq operating points are all 
board-specific or at least SoC revision-specific. Moreover, they are all 
closely related to themselves:

  - the list of available cpufreq operating points depends on available 
APLL and core clock configurations,

  - each APLL setting has its own, specific, set of configurations of 
core dividers.

This makes me almost sure that what we want is a single place where all 
(or at least all clock-related) data are specified and this is probably 
device tree, since those can be board-specific or at least SoC revision 
specific.

>>
>>>
>>>>
>>>>> +
>>>>> +/**
>>>>> + * struct samsung_core_clock: information about clock supplied to
>>>>> a CPU core.
>>>>> + * @hw: handle between ccf and core clock.
>>>>> + * @ctrl_base: base address of the clock controller.
>>>>> + * @fout_pll: clock handle representing the clock output of the
>>>>> associated PLL.
>>>>> + */
>>>>> +struct samsung_core_clock {
>>>>> +     struct clk_hw           hw;
>>>>> +     void __iomem            *ctrl_base;
>>>>> +     struct clk              *fout_pll;
>>>>> +     const struct samsung_core_clock_freq_table *freq_table;
>>>>> +};
>>>>> +
>>>>> +extern unsigned long samsung_core_clock_recalc_rate(struct clk_hw
>>>>> *hw,
>>>>> +                             unsigned long parent_rate);
>>>>> +extern long samsung_core_clk_round_rate(struct clk_hw *hw,
>>>>> +                     unsigned long drate, unsigned long *prate);
>>>>
>>>> IMHO, those methods shall be defined as static in a separate file.
>>>
>>> These two functions are generic enough to be reused for core clock
>>> implementations on multiple exynos platforms.
>>
>> I thought that you are going to implement this clock in a generic way,
>> so by using the DT we could use it for Exynos4210/4x12 and 5250.
>>
>>> Only the set_rate
>>> callback is the one that will be specific to SoC and core clock. The
>>> SoC specific implementation can reuse these two callback funcitons
>>> when registering the core clock type. That is the reason these are not
>>> static functions. The 3/6 patch of this series includes the usage of
>>> these functions.
>>
>> In my opinion we shall not duplicate per SoC constructs like for
>> example:
>>
>> static struct apll_freq apll_freq_4412[]
>> static struct apll_freq apll_freq_4212[]
>> static struct apll_freq apll_freq_4210[]
>>
>> which would be used at different ->set_rate() callbacks.
>>
>> I think that the best solution would be to use one ->set_rate() with
>> data parsed from DT.
>
> Yes, I agree with you. The ->set_rate() callback code is not going to
> be repeated if the code can be reused for other Exynos SoCs.
>
> And about the values of the registers for set_rate coming in from DT,
> these are SoC specific values and are not different for two or more
> platforms using the same SoC. So why put this into DT? The same was
> the argument about populating all the clocks from DT. Clock blocks are
> not going to change from one platform to another platform which use
> the same SoC. They are SoC specific and not platform specific and
> hence there is little use in putting them into DT.

Those values aren't really such generic as one might think. There exist 
different SoC revisions on which different settings can be used. 
Moreover, I've already seen different sets of settings even for the same 
board in different sources of vendor kernel. This might mean that those 
values are in fact more of configuration, than static values selected 
for given hardware.

Best regards,
Tomasz

Re: [PATCH 2/6] clk: samsung: add infrastructure to register CPU clocks

[Lukasz Majewski]

[-- Attachment #1.1: Type: text/plain, Size: 11999 bytes --]

Dear All,

> Hi Thomas, Lukasz,
> 
> Let me put my two cents in.
> 
> On 11.01.2014 05:43, Thomas Abraham wrote:
> > On Fri, Jan 10, 2014 at 6:55 PM, Lukasz Majewski
> > <l.majewski@samsung.com> wrote:
> >> Hi Thomas,
> >>
> >>> Hi Lukasz,
> >>>
> >>> On Fri, Jan 10, 2014 at 5:34 PM, Lukasz Majewski
> >>> <l.majewski@samsung.com> wrote:
> [snip]
> >>>>> +/* helper function to register a core clock */
> >>>>> +void __init samsung_coreclk_register(const char *name, const
> >>>>> char **parents,
> >>>>> +                     unsigned int num_parents, const char
> >>>>> *pllclk,
> >>>>> +                     const struct clk_ops *clk_ops, unsigned
> >>>>> int lookup_id,
> >>>>> +                     const struct samsung_core_clock_freq_table
> >>>>> *freq_tbl) +{
> >>>>> +     struct samsung_core_clock *coreclk;
> >>>>> +     struct clk_init_data init;
> >>>>> +     struct clk *clk;
> >>>>> +
> >>>>> +     coreclk = kzalloc(sizeof(*coreclk), GFP_KERNEL);
> >>>>> +     if (!coreclk) {
> >>>>> +             pr_err("%s: could not allocate memory for coreclk
> >>>>> %s\n",
> >>>>> +                                     __func__, name);
> >>>>> +             return;
> >>>>> +     }
> >>>>> +
> >>>>> +     init.name = name;
> >>>>> +     init.flags = CLK_GET_RATE_NOCACHE;
> >>>>> +     init.parent_names = parents;
> >>>>> +     init.num_parents = num_parents;
> >>>>> +     init.ops = clk_ops;
> >>>>> +
> >>>>> +     coreclk->hw.init = &init;
> >>>>> +     coreclk->ctrl_base = reg_base;
> >>>>> +     coreclk->fout_pll = __clk_lookup(pllclk);
> >>>>> +     coreclk->freq_table = freq_tbl;
> >>>>> +
> >>>>> +     clk = clk_register(NULL, &coreclk->hw);
> >>>>> +     if (IS_ERR(clk)) {
> >>>>> +             pr_err("%s: could not register coreclk %s\n",
> >>>>> __func__,     name);
> >>>>> +             kfree(coreclk);
> >>>>> +             return;
> >>>>> +     }
> >>>>> +     samsung_clk_add_lookup(clk, lookup_id);
> >>>>> +}
> >>>>
> >>>> I think, that those definitions for samsung_core_clock shall be
> >>>> moved to a separate file (maybe clk-exynos-cpu.c)?
> >>>
> >>> The additions were not really much and the samsung/clk.c file is
> >>> used to hold all the common code for Samsung SoCs. So the
> >>> additions were put into this existing file itself.
> 
> Still, separation between clock drivers and clock providers would be 
> nice. clk-exynos4.c and friends doesn't implement any clock handling 
> logic, they just define the tree of available clocks. I agree with 
> Lukasz that separate file would be better place for such code, just
> as it was done with PLL drivers.
> 
> In addition this would allow you to make the generic core clock
> helpers static, as they could be moved to the same file where all the
> core clock driver variants are implemented.
> 
> >>>
> >>>>
> >>>> Moreover, maybe you can choose a shorter name?
> >>>
> >>> Ok, I will try to find a shorter name for in the next version.
> 
> I'm not sure how the name (or which name) could be shorter here,
> without impairing readability. Lukasz, can you elaborate?

For example samsung_cpu_clk

> 
> >>>
> >>>>
> >>>>> diff --git a/drivers/clk/samsung/clk.h
> >>>>> b/drivers/clk/samsung/clk.h index 31b4174..0e43023 100644
> >>>>> --- a/drivers/clk/samsung/clk.h
> >>>>> +++ b/drivers/clk/samsung/clk.h
> >>>>> @@ -312,6 +312,37 @@ struct samsung_pll_clock {
> >>>>>        __PLL(_typ, _id, NULL, _name, _pname,
> >>>>> CLK_GET_RATE_NOCACHE, \ _lock, _con, _rtable, _alias)
> >>>>>
> >>>>> +/**
> >>>>> + * struct samsung_core_clock_freq_table: table of frequency
> >>>>> supported by
> >>>>> + * a core clock and associated data if any.
> >>>>> + * @freq: points to a table of supported frequencies (in KHz)
> >>>>> + * @freq_count: number of entries in the frequency table
> >>>>> + * @data: core clock specific data, if any
> >>>>> + */
> >>>>> +struct samsung_core_clock_freq_table {
> >>>>> +     const unsigned long     *freq;       /* in KHz */
> >>>>> +     unsigned long           freq_count;
> >>>>> +     const void              *data;
> >>>>> +};
> >>>>
> >>>> This is another instance of a structure, which holds the
> >>>> frequency for the system (like struct cpufreq_frequency_table).
> >>>>
> >>>> Unfortunately, since the clk subsystem starts very early, the
> >>>> cpufreq_frequency_table is not yet parsed from "operating-points"
> >>>> attribute.
> >>>>
> >>>> Maybe we can defer the clock registration?
> >>>
> >>> The core clock is not limited to support only those frequencies
> >>> which the CPU operating point specifies. The core clock type can
> >>> be used independent of the cpufreq driver and hence has its own
> >>> set of supported frequencies. The cpufreq driver, depending the
> >>> operating points allowed, can request the core clock to setup the
> >>> requested clock speed.
> >>
> >> The above would be true if it could have been possible to use this
> >> clock without cpufreq.
> >>
> >> Since, justification of this whole change is the clean up of
> >> cpufreq for Exynos, I assume that it will be only used with
> >> cpufreq. Is there any use case to use this clock without cpufreq?
> >>
> >> For this reason Exynos SoCs can only change frequency to the one,
> >> which is defined at cpufreq_frequency_table.
> >>
> >> And since the cpufreq_frequency_table is dynamically build from
> >> operating-points attribute, then in my opinion we shall refer to
> >> only one place for frequency.
> >>
> >> Similar approach - of reusing cpufreq_frequency_table is used at
> >> thermal core driver.
> >
> > One of the reasons for keeping the supported frequencies independent
> > of the cpufreq table was ensure that platforms are free to use only
> > a subset of the supported clock speed due to certain
> > performance/power requirements particular to that platform Two or
> > more platforms based on the same SoC can have different
> > power/performance requirements. The case of thermal is different,
> > it can only request for the CPU to be clocked at speeds supported
> > by the cpufreq driver.
> >
> > Also, the core clock type is not limited to be used only by the
> > cpufreq driver. It should be possible to use a platform without the
> > cpufreq driver but still have the flexibility to change the cpu
> > clock speed.
> >
> 
> I don't think the core clock is going to be used (in reconfigurable
> way) with anything else than cpufreq. I agree with Thomas, though,
> that clock drivers should not have any dependencies on other
> subsystems. Separation is always good.

One possible use case, which I can predict is the CPU frequency control
from user space (with CPUfreq bypassed).

> 
> However, Lukasz has a valid point. Your patch is adding third place 
> where list of CPU frequencies is defined. We really shouldn't have 
> allowed this to be scattered into two pieces with common clock
> framework implementation, not even saying about three. This is data
> redundancy that is not only error prone, but also a maintenance
> issue, because whenever one wants to change the set of supported
> frequencies, they must modify all two (or three) locations.

+1

> 
> My stance on this is that we should get rid of this redundancy and I 
> won't Ack any patch that makes us further from this goal, unless you
> can convince me that there is no other option.
> 
> As for how I see solution for this, I believe that possible APLL 
> settings, core clock settings and cpufreq operating points are all 
> board-specific or at least SoC revision-specific. Moreover, they are
> all closely related to themselves:
> 
>   - the list of available cpufreq operating points depends on
> available APLL and core clock configurations,
> 
>   - each APLL setting has its own, specific, set of configurations of 
> core dividers.
> 
> This makes me almost sure that what we want is a single place where
> all (or at least all clock-related) data are specified and this is
> probably device tree, since those can be board-specific or at least
> SoC revision specific.
> 
> >>
> >>>
> >>>>
> >>>>> +
> >>>>> +/**
> >>>>> + * struct samsung_core_clock: information about clock supplied
> >>>>> to a CPU core.
> >>>>> + * @hw: handle between ccf and core clock.
> >>>>> + * @ctrl_base: base address of the clock controller.
> >>>>> + * @fout_pll: clock handle representing the clock output of the
> >>>>> associated PLL.
> >>>>> + */
> >>>>> +struct samsung_core_clock {
> >>>>> +     struct clk_hw           hw;
> >>>>> +     void __iomem            *ctrl_base;
> >>>>> +     struct clk              *fout_pll;
> >>>>> +     const struct samsung_core_clock_freq_table *freq_table;
> >>>>> +};
> >>>>> +
> >>>>> +extern unsigned long samsung_core_clock_recalc_rate(struct
> >>>>> clk_hw *hw,
> >>>>> +                             unsigned long parent_rate);
> >>>>> +extern long samsung_core_clk_round_rate(struct clk_hw *hw,
> >>>>> +                     unsigned long drate, unsigned long
> >>>>> *prate);
> >>>>
> >>>> IMHO, those methods shall be defined as static in a separate
> >>>> file.
> >>>
> >>> These two functions are generic enough to be reused for core clock
> >>> implementations on multiple exynos platforms.
> >>
> >> I thought that you are going to implement this clock in a generic
> >> way, so by using the DT we could use it for Exynos4210/4x12 and
> >> 5250.
> >>
> >>> Only the set_rate
> >>> callback is the one that will be specific to SoC and core clock.
> >>> The SoC specific implementation can reuse these two callback
> >>> funcitons when registering the core clock type. That is the
> >>> reason these are not static functions. The 3/6 patch of this
> >>> series includes the usage of these functions.
> >>
> >> In my opinion we shall not duplicate per SoC constructs like for
> >> example:
> >>
> >> static struct apll_freq apll_freq_4412[]
> >> static struct apll_freq apll_freq_4212[]
> >> static struct apll_freq apll_freq_4210[]
> >>
> >> which would be used at different ->set_rate() callbacks.
> >>
> >> I think that the best solution would be to use one ->set_rate()
> >> with data parsed from DT.
> >
> > Yes, I agree with you. The ->set_rate() callback code is not going
> > to be repeated if the code can be reused for other Exynos SoCs.
> >
> > And about the values of the registers for set_rate coming in from
> > DT, these are SoC specific values and are not different for two or
> > more platforms using the same SoC. So why put this into DT? The
> > same was the argument about populating all the clocks from DT.
> > Clock blocks are not going to change from one platform to another
> > platform which use the same SoC. They are SoC specific and not
> > platform specific and hence there is little use in putting them
> > into DT.
> 
> Those values aren't really such generic as one might think. There
> exist different SoC revisions on which different settings can be
> used. Moreover, I've already seen different sets of settings even for
> the same board in different sources of vendor kernel. This might mean
> that those values are in fact more of configuration, than static
> values selected for given hardware.

To give an example here:

Chip vendor produces a SoC with different HW revisions (rev.0, 1, 2,
etc).

For example rev 0. due to some HW bugs is not working correctly with all
freqs listed in the SoC spec.

With frequencies defined at DT and parsed in a generic way I can
redefine the new set of frequencies at board specific DTS file without
changing kernel code.

> 
> Best regards,
> Tomasz
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Best regards,
Lukasz Majewski

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/6] clk: samsung: add infrastructure to register CPU clocks

[Thomas Abraham]

Hi Tomasz,

On Sun, Jan 12, 2014 at 7:17 AM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> Hi Thomas, Lukasz,
>
> Let me put my two cents in.
>
>
> On 11.01.2014 05:43, Thomas Abraham wrote:
>>
>> On Fri, Jan 10, 2014 at 6:55 PM, Lukasz Majewski <l.majewski@samsung.com>
>> wrote:
>>>
>>> Hi Thomas,
>>>
>>>> Hi Lukasz,
>>>>
>>>> On Fri, Jan 10, 2014 at 5:34 PM, Lukasz Majewski
>>>> <l.majewski@samsung.com> wrote:
>
> [snip]
>
>>>>>> +/* helper function to register a core clock */
>>>>>> +void __init samsung_coreclk_register(const char *name, const char
>>>>>> **parents,
>>>>>> +                     unsigned int num_parents, const char *pllclk,
>>>>>> +                     const struct clk_ops *clk_ops, unsigned int
>>>>>> lookup_id,
>>>>>> +                     const struct samsung_core_clock_freq_table
>>>>>> *freq_tbl) +{
>>>>>> +     struct samsung_core_clock *coreclk;
>>>>>> +     struct clk_init_data init;
>>>>>> +     struct clk *clk;
>>>>>> +
>>>>>> +     coreclk = kzalloc(sizeof(*coreclk), GFP_KERNEL);
>>>>>> +     if (!coreclk) {
>>>>>> +             pr_err("%s: could not allocate memory for coreclk
>>>>>> %s\n",
>>>>>> +                                     __func__, name);
>>>>>> +             return;
>>>>>> +     }
>>>>>> +
>>>>>> +     init.name = name;
>>>>>> +     init.flags = CLK_GET_RATE_NOCACHE;
>>>>>> +     init.parent_names = parents;
>>>>>> +     init.num_parents = num_parents;
>>>>>> +     init.ops = clk_ops;
>>>>>> +
>>>>>> +     coreclk->hw.init = &init;
>>>>>> +     coreclk->ctrl_base = reg_base;
>>>>>> +     coreclk->fout_pll = __clk_lookup(pllclk);
>>>>>> +     coreclk->freq_table = freq_tbl;
>>>>>> +
>>>>>> +     clk = clk_register(NULL, &coreclk->hw);
>>>>>> +     if (IS_ERR(clk)) {
>>>>>> +             pr_err("%s: could not register coreclk %s\n",
>>>>>> __func__,     name);
>>>>>> +             kfree(coreclk);
>>>>>> +             return;
>>>>>> +     }
>>>>>> +     samsung_clk_add_lookup(clk, lookup_id);
>>>>>> +}
>>>>>
>>>>>
>>>>> I think, that those definitions for samsung_core_clock shall be
>>>>> moved to a separate file (maybe clk-exynos-cpu.c)?
>>>>
>>>>
>>>> The additions were not really much and the samsung/clk.c file is used
>>>> to hold all the common code for Samsung SoCs. So the additions were
>>>> put into this existing file itself.
>
>
> Still, separation between clock drivers and clock providers would be nice.
> clk-exynos4.c and friends doesn't implement any clock handling logic, they
> just define the tree of available clocks. I agree with Lukasz that separate
> file would be better place for such code, just as it was done with PLL
> drivers.
>
> In addition this would allow you to make the generic core clock helpers
> static, as they could be moved to the same file where all the core clock
> driver variants are implemented.

Ok, I will move them to a new file in the next version.

>
>
>>>>
>>>>>
>>>>> Moreover, maybe you can choose a shorter name?
>>>>
>>>>
>>>> Ok, I will try to find a shorter name for in the next version.
>
>
> I'm not sure how the name (or which name) could be shorter here, without
> impairing readability. Lukasz, can you elaborate?
>
>
>>>>
>>>>>
>>>>>> diff --git a/drivers/clk/samsung/clk.h b/drivers/clk/samsung/clk.h
>>>>>> index 31b4174..0e43023 100644
>>>>>> --- a/drivers/clk/samsung/clk.h
>>>>>> +++ b/drivers/clk/samsung/clk.h
>>>>>> @@ -312,6 +312,37 @@ struct samsung_pll_clock {
>>>>>>        __PLL(_typ, _id, NULL, _name, _pname,
>>>>>> CLK_GET_RATE_NOCACHE, \ _lock, _con, _rtable, _alias)
>>>>>>
>>>>>> +/**
>>>>>> + * struct samsung_core_clock_freq_table: table of frequency
>>>>>> supported by
>>>>>> + * a core clock and associated data if any.
>>>>>> + * @freq: points to a table of supported frequencies (in KHz)
>>>>>> + * @freq_count: number of entries in the frequency table
>>>>>> + * @data: core clock specific data, if any
>>>>>> + */
>>>>>> +struct samsung_core_clock_freq_table {
>>>>>> +     const unsigned long     *freq;       /* in KHz */
>>>>>> +     unsigned long           freq_count;
>>>>>> +     const void              *data;
>>>>>> +};
>>>>>
>>>>>
>>>>> This is another instance of a structure, which holds the frequency
>>>>> for the system (like struct cpufreq_frequency_table).
>>>>>
>>>>> Unfortunately, since the clk subsystem starts very early, the
>>>>> cpufreq_frequency_table is not yet parsed from "operating-points"
>>>>> attribute.
>>>>>
>>>>> Maybe we can defer the clock registration?
>>>>
>>>>
>>>> The core clock is not limited to support only those frequencies which
>>>> the CPU operating point specifies. The core clock type can be used
>>>> independent of the cpufreq driver and hence has its own set of
>>>> supported frequencies. The cpufreq driver, depending the operating
>>>> points allowed, can request the core clock to setup the requested
>>>> clock speed.
>>>
>>>
>>> The above would be true if it could have been possible to use this
>>> clock without cpufreq.
>>>
>>> Since, justification of this whole change is the clean up of cpufreq for
>>> Exynos, I assume that it will be only used with cpufreq. Is there any
>>> use case to use this clock without cpufreq?
>>>
>>> For this reason Exynos SoCs can only change frequency to the one, which
>>> is defined at cpufreq_frequency_table.
>>>
>>> And since the cpufreq_frequency_table is dynamically build from
>>> operating-points attribute, then in my opinion we shall refer to only
>>> one place for frequency.
>>>
>>> Similar approach - of reusing cpufreq_frequency_table is used at
>>> thermal core driver.
>>
>>
>> One of the reasons for keeping the supported frequencies independent
>> of the cpufreq table was ensure that platforms are free to use only a
>> subset of the supported clock speed due to certain performance/power
>> requirements particular to that platform Two or more platforms based
>> on the same SoC can have different power/performance requirements. The
>> case of thermal is different, it can only request for the CPU to be
>> clocked at speeds supported by the cpufreq driver.
>>
>> Also, the core clock type is not limited to be used only by the
>> cpufreq driver. It should be possible to use a platform without the
>> cpufreq driver but still have the flexibility to change the cpu clock
>> speed.
>>
>
> I don't think the core clock is going to be used (in reconfigurable way)
> with anything else than cpufreq. I agree with Thomas, though, that clock
> drivers should not have any dependencies on other subsystems. Separation is
> always good.
>
> However, Lukasz has a valid point. Your patch is adding third place where
> list of CPU frequencies is defined. We really shouldn't have allowed this to
> be scattered into two pieces with common clock framework implementation, not
> even saying about three. This is data redundancy that is not only error
> prone, but also a maintenance issue, because whenever one wants to change
> the set of supported frequencies, they must modify all two (or three)
> locations.
>
> My stance on this is that we should get rid of this redundancy and I won't
> Ack any patch that makes us further from this goal, unless you can convince
> me that there is no other option.

The reasons for having three different tables are.

1. The PLL frequency table is a list of frequencies which the PLL is
capable of generating. A PLL can be used in two or more SoCs but the
frequencies it can generate does not change from one SoC to the other.
A SoC can further enforce limitations on the list of frequencies that
PLL should be programmed for (due to limitations in the hardware).

2. The core clock is derived from the PLL clock output. The list of
frequencies supported by the core clock need not be same as that of
the PLL table. An example of that is, if the minimum frequency
generated by the PLL is 200MHz, and if there are dividers in the core
clock, the core clock can generate frequencies = 200/x where x is the
divider value. Hence, there is no 1:1 relation between PLL clock table
and core clock table.

3. The CPUfreq table is dependent on the platform. There can be
platform level power/performance requirements due to which the number
of frequencies supported on the platform have to be restricted. With
multiple platforms supported in a single binary, there is  a need for
CPUFreq table != core clock table.

>
> As for how I see solution for this, I believe that possible APLL settings,
> core clock settings and cpufreq operating points are all board-specific or
> at least SoC revision-specific. Moreover, they are all closely related to
> themselves:
>
>  - the list of available cpufreq operating points depends on available APLL
> and core clock configurations,

The CPUfreq operating points can be a subset of the core clock outputs
as required for power/performance requirements of a platform.

>
>  - each APLL setting has its own, specific, set of configurations of core
> dividers.

The APLL (P,M,S) values are independent of the core divider values. A
SoC can enforce limitations on the P,M,S values that should be
programmed on the PLL.

Also, there are two core clocks on a big-little system - one for big
cluster and one for little cluster.

>
> This makes me almost sure that what we want is a single place where all (or
> at least all clock-related) data are specified and this is probably device
> tree, since those can be board-specific or at least SoC revision specific.

Thanks,
Thomas.

>
>
>>>
>>>>
>>>>>
>>>>>> +
>>>>>> +/**
>>>>>> + * struct samsung_core_clock: information about clock supplied to
>>>>>> a CPU core.
>>>>>> + * @hw: handle between ccf and core clock.
>>>>>> + * @ctrl_base: base address of the clock controller.
>>>>>> + * @fout_pll: clock handle representing the clock output of the
>>>>>> associated PLL.
>>>>>> + */
>>>>>> +struct samsung_core_clock {
>>>>>> +     struct clk_hw           hw;
>>>>>> +     void __iomem            *ctrl_base;
>>>>>> +     struct clk              *fout_pll;
>>>>>> +     const struct samsung_core_clock_freq_table *freq_table;
>>>>>> +};
>>>>>> +
>>>>>> +extern unsigned long samsung_core_clock_recalc_rate(struct clk_hw
>>>>>> *hw,
>>>>>> +                             unsigned long parent_rate);
>>>>>> +extern long samsung_core_clk_round_rate(struct clk_hw *hw,
>>>>>> +                     unsigned long drate, unsigned long *prate);
>>>>>
>>>>>
>>>>> IMHO, those methods shall be defined as static in a separate file.
>>>>
>>>>
>>>> These two functions are generic enough to be reused for core clock
>>>> implementations on multiple exynos platforms.
>>>
>>>
>>> I thought that you are going to implement this clock in a generic way,
>>> so by using the DT we could use it for Exynos4210/4x12 and 5250.
>>>
>>>> Only the set_rate
>>>> callback is the one that will be specific to SoC and core clock. The
>>>> SoC specific implementation can reuse these two callback funcitons
>>>> when registering the core clock type. That is the reason these are not
>>>> static functions. The 3/6 patch of this series includes the usage of
>>>> these functions.
>>>
>>>
>>> In my opinion we shall not duplicate per SoC constructs like for
>>> example:
>>>
>>> static struct apll_freq apll_freq_4412[]
>>> static struct apll_freq apll_freq_4212[]
>>> static struct apll_freq apll_freq_4210[]
>>>
>>> which would be used at different ->set_rate() callbacks.
>>>
>>> I think that the best solution would be to use one ->set_rate() with
>>> data parsed from DT.
>>
>>
>> Yes, I agree with you. The ->set_rate() callback code is not going to
>> be repeated if the code can be reused for other Exynos SoCs.
>>
>> And about the values of the registers for set_rate coming in from DT,
>> these are SoC specific values and are not different for two or more
>> platforms using the same SoC. So why put this into DT? The same was
>> the argument about populating all the clocks from DT. Clock blocks are
>> not going to change from one platform to another platform which use
>> the same SoC. They are SoC specific and not platform specific and
>> hence there is little use in putting them into DT.
>
>
> Those values aren't really such generic as one might think. There exist
> different SoC revisions on which different settings can be used. Moreover,
> I've already seen different sets of settings even for the same board in
> different sources of vendor kernel. This might mean that those values are in
> fact more of configuration, than static values selected for given hardware.
>
> Best regards,
> Tomasz

[PATCH 3/6] clk: samsung: register cpu clock provider for exynos4210 SoC

[Thomas Abraham]

Add a new clock provider for ARM clock domain. This clock provider
is composed of multiple components which include mux_core, div_core,
div_core2, div_corem0, div_corem1, div_periph, div_atb, div_pclk_dbg,
div_copy and div_hpm. This composition of mutiple components into
a single clock provider helps with faster completion of CPU clock
speed switching during DVFS operations.

Signed-off-by: Thomas Abraham <thomas.ab@samsung.com>
---
 drivers/clk/samsung/clk-exynos4.c |   96 ++++++++++++++++++++++++++++++++++++-
 1 files changed, 95 insertions(+), 1 deletions(-)

diff --git a/drivers/clk/samsung/clk-exynos4.c b/drivers/clk/samsung/clk-exynos4.c
index d967571..4bf2f93 100644
--- a/drivers/clk/samsung/clk-exynos4.c
+++ b/drivers/clk/samsung/clk-exynos4.c
@@ -108,8 +108,11 @@
 #define APLL_CON0		0x14100
 #define E4210_MPLL_CON0		0x14108
 #define SRC_CPU			0x14200
+#define STAT_CPU		0x14400
 #define DIV_CPU0		0x14500
 #define DIV_CPU1		0x14504
+#define DIV_STAT_CPU0		0x14600
+#define DIV_STAT_CPU1		0x14604
 #define GATE_SCLK_CPU		0x14800
 #define GATE_IP_CPU		0x14900
 #define E4X12_DIV_ISP0		0x18300
@@ -289,7 +292,7 @@ static unsigned long exynos4_clk_regs[] __initdata = {
 };
 
 /* list of all parent clock list */
-PNAME(mout_apll_p)	= { "fin_pll", "fout_apll", };
+PNAME(mout_apll_p)	= { "fin_pll", "fout_apll1", };
 PNAME(mout_mpll_p)	= { "fin_pll", "fout_mpll", };
 PNAME(mout_epll_p)	= { "fin_pll", "fout_epll", };
 PNAME(mout_vpllsrc_p)	= { "fin_pll", "sclk_hdmi24m", };
@@ -306,6 +309,7 @@ PNAME(mout_onenand_p)  = {"aclk133", "aclk160", };
 PNAME(mout_onenand1_p) = {"mout_onenand", "sclk_vpll", };
 
 /* Exynos 4210-specific parent groups */
+PNAME(armclk_p) = { "fout_apll", };
 PNAME(sclk_vpll_p4210)	= { "mout_vpllsrc", "fout_vpll", };
 PNAME(mout_core_p4210)	= { "mout_apll", "sclk_mpll", };
 PNAME(sclk_ampll_p4210)	= { "sclk_mpll", "sclk_apll", };
@@ -1089,6 +1093,92 @@ static struct samsung_pll_clock exynos4x12_plls[nr_plls] __initdata = {
 			VPLL_LOCK, VPLL_CON0, NULL),
 };
 
+#define EXYNOS4210_DIV_CPU0(apll, pclk_dbg, atb, periph, corem1, corem0) \
+		((apll << 24) | (pclk_dbg << 20) | (atb << 16) | \
+		(periph << 12) | (corem1 << 8) | (corem0 << 4))
+#define EXYNOS4210_DIV_CPU1(hpm, copy)	\
+		((hpm << 4) | (copy << 0))
+static const unsigned long exynos4210_armclk_data[][2] = {
+	{ EXYNOS4210_DIV_CPU0(7, 1, 4, 3, 7, 3), EXYNOS4210_DIV_CPU1(0, 5), },
+	{ EXYNOS4210_DIV_CPU0(7, 1, 4, 3, 7, 3), EXYNOS4210_DIV_CPU1(0, 4), },
+	{ EXYNOS4210_DIV_CPU0(7, 1, 3, 3, 7, 3), EXYNOS4210_DIV_CPU1(0, 3), },
+	{ EXYNOS4210_DIV_CPU0(7, 1, 3, 3, 7, 3), EXYNOS4210_DIV_CPU1(0, 3), },
+	{ EXYNOS4210_DIV_CPU0(7, 1, 3, 3, 7, 3), EXYNOS4210_DIV_CPU1(0, 3), },
+	{ EXYNOS4210_DIV_CPU0(0, 1, 3, 1, 3, 1), EXYNOS4210_DIV_CPU1(0, 3), },
+};
+
+static const unsigned long exynos4210_armclk_freqs[] = {
+	1200000 , 1000000, 800000, 500000, 400000, 200000,
+};
+
+static const struct samsung_core_clock_freq_table exyno4210_armclk_table = {
+	.freq		= exynos4210_armclk_freqs,
+	.freq_count	= ARRAY_SIZE(exynos4210_armclk_freqs),
+	.data		= (void *)exynos4210_armclk_data,
+};
+
+static int exynos4210_armclk_set_rate(struct clk_hw *hw, unsigned long drate,
+					unsigned long prate)
+{
+	struct samsung_core_clock *armclk;
+	const struct samsung_core_clock_freq_table *freq_tbl;
+	unsigned long *freq_data;
+	unsigned long mux_reg, idx;
+	void __iomem *base;
+
+	if (drate == prate)
+		return 0;
+
+	armclk = container_of(hw, struct samsung_core_clock, hw);
+	freq_tbl = armclk->freq_table;
+	freq_data = (unsigned long *)freq_tbl->data;
+	base = armclk->ctrl_base;
+
+	for (idx = 0; idx < freq_tbl->freq_count; idx++, freq_data += 2)
+		if ((freq_tbl->freq[idx] * 1000) == drate)
+			break;
+
+	if (!armclk->fout_pll)
+		armclk->fout_pll = __clk_lookup("fout_apll");
+
+	if (drate < prate) {
+		mux_reg = readl(base + SRC_CPU);
+		writel(mux_reg | (1 << 16), base + SRC_CPU);
+		while (((readl(base + STAT_CPU) >> 16) & 0x7) != 2)
+			;
+
+		clk_set_rate(armclk->fout_pll, drate);
+	}
+
+	writel(freq_data[0], base + DIV_CPU0);
+	while (readl(base + DIV_STAT_CPU0) != 0)
+		;
+	writel(freq_data[1], base + DIV_CPU1);
+	while (readl(base + DIV_STAT_CPU1) != 0)
+		;
+
+	if (drate > prate) {
+		mux_reg = readl(base + SRC_CPU);
+		writel(mux_reg | (1 << 16), base + SRC_CPU);
+		while (((readl(base + STAT_CPU) >> 16) & 0x7) != 2)
+			;
+
+		clk_set_rate(armclk->fout_pll, drate);
+	}
+
+	mux_reg = readl(base + SRC_CPU);
+	writel(mux_reg & ~(1 << 16), base + SRC_CPU);
+	while (((readl(base + STAT_CPU) >> 16) & 0x7) != 1)
+			;
+	return 0;
+}
+
+static const struct clk_ops exynos4210_armclk_clk_ops = {
+	.recalc_rate = samsung_core_clock_recalc_rate,
+	.round_rate = samsung_core_clk_round_rate,
+	.set_rate = exynos4210_armclk_set_rate,
+};
+
 /* register exynos4 clocks */
 static void __init exynos4_clk_init(struct device_node *np,
 				    enum exynos4_soc exynos4_soc,
@@ -1164,6 +1254,10 @@ 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));
+		samsung_coreclk_register("armclk", armclk_p,
+			ARRAY_SIZE(armclk_p), "fout_apll",
+			&exynos4210_armclk_clk_ops, arm_clk,
+			&exyno4210_armclk_table);
 	} else {
 		samsung_clk_register_mux(exynos4x12_mux_clks,
 			ARRAY_SIZE(exynos4x12_mux_clks));
-- 
1.6.6.rc2

Re: [PATCH 3/6] clk: samsung: register cpu clock provider for exynos4210 SoC

[Lukasz Majewski]

Hi Thomas,

> Add a new clock provider for ARM clock domain. This clock provider
> is composed of multiple components which include mux_core, div_core,
> div_core2, div_corem0, div_corem1, div_periph, div_atb, div_pclk_dbg,
> div_copy and div_hpm. This composition of mutiple components into
> a single clock provider helps with faster completion of CPU clock
> speed switching during DVFS operations.
> 
> Signed-off-by: Thomas Abraham <thomas.ab@samsung.com>
> ---
>  drivers/clk/samsung/clk-exynos4.c |   96
> ++++++++++++++++++++++++++++++++++++- 1 files changed, 95
> insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/clk/samsung/clk-exynos4.c
> b/drivers/clk/samsung/clk-exynos4.c index d967571..4bf2f93 100644
> --- a/drivers/clk/samsung/clk-exynos4.c
> +++ b/drivers/clk/samsung/clk-exynos4.c
> @@ -108,8 +108,11 @@
>  #define APLL_CON0		0x14100
>  #define E4210_MPLL_CON0		0x14108
>  #define SRC_CPU			0x14200
> +#define STAT_CPU		0x14400
>  #define DIV_CPU0		0x14500
>  #define DIV_CPU1		0x14504
> +#define DIV_STAT_CPU0		0x14600
> +#define DIV_STAT_CPU1		0x14604
>  #define GATE_SCLK_CPU		0x14800
>  #define GATE_IP_CPU		0x14900
>  #define E4X12_DIV_ISP0		0x18300
> @@ -289,7 +292,7 @@ static unsigned long exynos4_clk_regs[]
> __initdata = { };
>  
>  /* list of all parent clock list */
> -PNAME(mout_apll_p)	= { "fin_pll", "fout_apll", };
> +PNAME(mout_apll_p)	= { "fin_pll", "fout_apll1", };
>  PNAME(mout_mpll_p)	= { "fin_pll", "fout_mpll", };
>  PNAME(mout_epll_p)	= { "fin_pll", "fout_epll", };
>  PNAME(mout_vpllsrc_p)	= { "fin_pll", "sclk_hdmi24m", };
> @@ -306,6 +309,7 @@ PNAME(mout_onenand_p)  = {"aclk133", "aclk160", };
>  PNAME(mout_onenand1_p) = {"mout_onenand", "sclk_vpll", };
>  
>  /* Exynos 4210-specific parent groups */
> +PNAME(armclk_p) = { "fout_apll", };

Here you only give no parent clock, but at samsung_coreclk_register()
it is expected to provide list of parents.

>  PNAME(sclk_vpll_p4210)	= { "mout_vpllsrc", "fout_vpll", };
>  PNAME(mout_core_p4210)	= { "mout_apll", "sclk_mpll", };
>  PNAME(sclk_ampll_p4210)	= { "sclk_mpll", "sclk_apll", };
> @@ -1089,6 +1093,92 @@ static struct samsung_pll_clock
> exynos4x12_plls[nr_plls] __initdata = { VPLL_LOCK, VPLL_CON0, NULL),
>  };
>  
> +#define EXYNOS4210_DIV_CPU0(apll, pclk_dbg, atb, periph, corem1,
> corem0) \
> +		((apll << 24) | (pclk_dbg << 20) | (atb << 16) | \
> +		(periph << 12) | (corem1 << 8) | (corem0 << 4))
> +#define EXYNOS4210_DIV_CPU1(hpm, copy)	\
> +		((hpm << 4) | (copy << 0))
> +static const unsigned long exynos4210_armclk_data[][2] = {
> +	{ EXYNOS4210_DIV_CPU0(7, 1, 4, 3, 7, 3),
> EXYNOS4210_DIV_CPU1(0, 5), },
> +	{ EXYNOS4210_DIV_CPU0(7, 1, 4, 3, 7, 3),
> EXYNOS4210_DIV_CPU1(0, 4), },
> +	{ EXYNOS4210_DIV_CPU0(7, 1, 3, 3, 7, 3),
> EXYNOS4210_DIV_CPU1(0, 3), },
> +	{ EXYNOS4210_DIV_CPU0(7, 1, 3, 3, 7, 3),
> EXYNOS4210_DIV_CPU1(0, 3), },
> +	{ EXYNOS4210_DIV_CPU0(7, 1, 3, 3, 7, 3),
> EXYNOS4210_DIV_CPU1(0, 3), },
> +	{ EXYNOS4210_DIV_CPU0(0, 1, 3, 1, 3, 1),
> EXYNOS4210_DIV_CPU1(0, 3), }, +};
> +

What do you think about adding those parameters (like CPU dividers) as
an attribute to /cpus/cpu@0 node?

> +static const unsigned long exynos4210_armclk_freqs[] = {
> +	1200000 , 1000000, 800000, 500000, 400000, 200000,
> +};

Those freq's are going to be defined at /cpus/cpu@0 at operating-points
attribute (or if possible took from cpufreq_frequency table).

> +
> +static const struct samsung_core_clock_freq_table
> exyno4210_armclk_table = {
> +	.freq		= exynos4210_armclk_freqs,
> +	.freq_count	= ARRAY_SIZE(exynos4210_armclk_freqs),
> +	.data		= (void *)exynos4210_armclk_data,
> +};
> +
> +static int exynos4210_armclk_set_rate(struct clk_hw *hw, unsigned
> long drate,
> +					unsigned long prate)
> +{
> +	struct samsung_core_clock *armclk;
> +	const struct samsung_core_clock_freq_table *freq_tbl;
> +	unsigned long *freq_data;
> +	unsigned long mux_reg, idx;
> +	void __iomem *base;
> +
> +	if (drate == prate)
> +		return 0;
> +
> +	armclk = container_of(hw, struct samsung_core_clock, hw);
> +	freq_tbl = armclk->freq_table;
> +	freq_data = (unsigned long *)freq_tbl->data;
> +	base = armclk->ctrl_base;
> +
> +	for (idx = 0; idx < freq_tbl->freq_count; idx++, freq_data
> += 2)
> +		if ((freq_tbl->freq[idx] * 1000) == drate)
> +			break;
> +
> +	if (!armclk->fout_pll)
> +		armclk->fout_pll = __clk_lookup("fout_apll");\
		^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^[*]
	
I'm a bit confused here for two reasons. Please correct me if I'm wrong.

1. You go into this ->set_rate() because of calling clk_set_rate at
"arm_clk" clock (numbered as 12 at clk-exynos4.c) at cpufreq-cpu0.c

In a Exynos4210 we have:
XXTI-> APLL -> fout_apll -> mout_apll -> mout_core -> div_core
-> div_core2 -> arm_clk

In the code you call directly the fout_apll which changes
frequency. Then the change shall be propagated to all registered
clocks.
I think, that DIV and DIV1 shall be reduced before PLL change [*],
to reflect the changes at CCF.	


> +
> +	if (drate < prate) {
> +		mux_reg = readl(base + SRC_CPU);
> +		writel(mux_reg | (1 << 16), base + SRC_CPU);
> +		while (((readl(base + STAT_CPU) >> 16) & 0x7) != 2)
> +			;
		^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [**]

2. I think, the above shall be done in a following way:

	clk_set_parent(mout_core, mout_mpll);
	clk_set_rate(armclk->fout_pll, drate);
	clk_set_parent(mout_core, mout_apll);

The direct write to registers [**] doesn't look compliant to CCF.


I'd rather thought about using "mout_core" instead of "arm_clk". Then we
would get access to the parent directly:

	struct clk *parent = clk_get_parent(hw->clk);

so we set the parents explicitly (at clk registration) and call
->recalc_rate for clocks which are lower in the tree (like "div_core",
"div_core2").

> +		clk_set_rate(armclk->fout_pll, drate);
> +	}
> +
> +	writel(freq_data[0], base + DIV_CPU0);
> +	while (readl(base + DIV_STAT_CPU0) != 0)
> +		;
> +	writel(freq_data[1], base + DIV_CPU1);
> +	while (readl(base + DIV_STAT_CPU1) != 0)
> +		;
> +
> +	if (drate > prate) {
> +		mux_reg = readl(base + SRC_CPU);
> +		writel(mux_reg | (1 << 16), base + SRC_CPU);
> +		while (((readl(base + STAT_CPU) >> 16) & 0x7) != 2)
> +			;
> +
> +		clk_set_rate(armclk->fout_pll, drate);
> +	}
> +
> +	mux_reg = readl(base + SRC_CPU);
> +	writel(mux_reg & ~(1 << 16), base + SRC_CPU);
> +	while (((readl(base + STAT_CPU) >> 16) & 0x7) != 1)
> +			;
> +	return 0;
> +}
> +
> +static const struct clk_ops exynos4210_armclk_clk_ops = {
> +	.recalc_rate = samsung_core_clock_recalc_rate,
> +	.round_rate = samsung_core_clk_round_rate,
> +	.set_rate = exynos4210_armclk_set_rate,
> +};
> +
>  /* register exynos4 clocks */
>  static void __init exynos4_clk_init(struct device_node *np,
>  				    enum exynos4_soc exynos4_soc,
> @@ -1164,6 +1254,10 @@ 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));
> +		samsung_coreclk_register("armclk", armclk_p,
> +			ARRAY_SIZE(armclk_p), "fout_apll",
> +			&exynos4210_armclk_clk_ops, arm_clk,
> +			&exyno4210_armclk_table);
>  	} else {
>  		samsung_clk_register_mux(exynos4x12_mux_clks,
>  			ARRAY_SIZE(exynos4x12_mux_clks));



-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

Re: [PATCH 3/6] clk: samsung: register cpu clock provider for exynos4210 SoC

[Thomas Abraham]

Hi Lukasz,

On Fri, Jan 10, 2014 at 5:34 PM, Lukasz Majewski <l.majewski@samsung.com> wrote:
> Hi Thomas,
>
>> Add a new clock provider for ARM clock domain. This clock provider
>> is composed of multiple components which include mux_core, div_core,
>> div_core2, div_corem0, div_corem1, div_periph, div_atb, div_pclk_dbg,
>> div_copy and div_hpm. This composition of mutiple components into
>> a single clock provider helps with faster completion of CPU clock
>> speed switching during DVFS operations.
>>
>> Signed-off-by: Thomas Abraham <thomas.ab@samsung.com>
>> ---
>>  drivers/clk/samsung/clk-exynos4.c |   96
>> ++++++++++++++++++++++++++++++++++++- 1 files changed, 95
>> insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/clk/samsung/clk-exynos4.c
>> b/drivers/clk/samsung/clk-exynos4.c index d967571..4bf2f93 100644
>> --- a/drivers/clk/samsung/clk-exynos4.c
>> +++ b/drivers/clk/samsung/clk-exynos4.c
>> @@ -108,8 +108,11 @@
>>  #define APLL_CON0            0x14100
>>  #define E4210_MPLL_CON0              0x14108
>>  #define SRC_CPU                      0x14200
>> +#define STAT_CPU             0x14400
>>  #define DIV_CPU0             0x14500
>>  #define DIV_CPU1             0x14504
>> +#define DIV_STAT_CPU0                0x14600
>> +#define DIV_STAT_CPU1                0x14604
>>  #define GATE_SCLK_CPU                0x14800
>>  #define GATE_IP_CPU          0x14900
>>  #define E4X12_DIV_ISP0               0x18300
>> @@ -289,7 +292,7 @@ static unsigned long exynos4_clk_regs[]
>> __initdata = { };
>>
>>  /* list of all parent clock list */
>> -PNAME(mout_apll_p)   = { "fin_pll", "fout_apll", };
>> +PNAME(mout_apll_p)   = { "fin_pll", "fout_apll1", };
>>  PNAME(mout_mpll_p)   = { "fin_pll", "fout_mpll", };
>>  PNAME(mout_epll_p)   = { "fin_pll", "fout_epll", };
>>  PNAME(mout_vpllsrc_p)        = { "fin_pll", "sclk_hdmi24m", };
>> @@ -306,6 +309,7 @@ PNAME(mout_onenand_p)  = {"aclk133", "aclk160", };
>>  PNAME(mout_onenand1_p) = {"mout_onenand", "sclk_vpll", };
>>
>>  /* Exynos 4210-specific parent groups */
>> +PNAME(armclk_p) = { "fout_apll", };
>
> Here you only give no parent clock, but at samsung_coreclk_register()
> it is expected to provide list of parents.

Here only one parent is listed, but the core clock type does not limit
the number of parents that can be specified. A specific implementation
can define and use multiple parents.

>
>>  PNAME(sclk_vpll_p4210)       = { "mout_vpllsrc", "fout_vpll", };
>>  PNAME(mout_core_p4210)       = { "mout_apll", "sclk_mpll", };
>>  PNAME(sclk_ampll_p4210)      = { "sclk_mpll", "sclk_apll", };
>> @@ -1089,6 +1093,92 @@ static struct samsung_pll_clock
>> exynos4x12_plls[nr_plls] __initdata = { VPLL_LOCK, VPLL_CON0, NULL),
>>  };
>>
>> +#define EXYNOS4210_DIV_CPU0(apll, pclk_dbg, atb, periph, corem1,
>> corem0) \
>> +             ((apll << 24) | (pclk_dbg << 20) | (atb << 16) | \
>> +             (periph << 12) | (corem1 << 8) | (corem0 << 4))
>> +#define EXYNOS4210_DIV_CPU1(hpm, copy)       \
>> +             ((hpm << 4) | (copy << 0))
>> +static const unsigned long exynos4210_armclk_data[][2] = {
>> +     { EXYNOS4210_DIV_CPU0(7, 1, 4, 3, 7, 3),
>> EXYNOS4210_DIV_CPU1(0, 5), },
>> +     { EXYNOS4210_DIV_CPU0(7, 1, 4, 3, 7, 3),
>> EXYNOS4210_DIV_CPU1(0, 4), },
>> +     { EXYNOS4210_DIV_CPU0(7, 1, 3, 3, 7, 3),
>> EXYNOS4210_DIV_CPU1(0, 3), },
>> +     { EXYNOS4210_DIV_CPU0(7, 1, 3, 3, 7, 3),
>> EXYNOS4210_DIV_CPU1(0, 3), },
>> +     { EXYNOS4210_DIV_CPU0(7, 1, 3, 3, 7, 3),
>> EXYNOS4210_DIV_CPU1(0, 3), },
>> +     { EXYNOS4210_DIV_CPU0(0, 1, 3, 1, 3, 1),
>> EXYNOS4210_DIV_CPU1(0, 3), }, +};
>> +
>
> What do you think about adding those parameters (like CPU dividers) as
> an attribute to /cpus/cpu@0 node?

Not in CPU node but may be in clock controller node since these values
are actually used by the clock controller. But since these values are
Exynos4210 specific and not generic enough to be reused across
multiple Exynos SoCs, there is little benefit in defining bindings and
parsing code for these values. It would be simpler enough to just
embed them in the code.

>
>> +static const unsigned long exynos4210_armclk_freqs[] = {
>> +     1200000 , 1000000, 800000, 500000, 400000, 200000,
>> +};
>
> Those freq's are going to be defined at /cpus/cpu@0 at operating-points
> attribute (or if possible took from cpufreq_frequency table).

These are frequencies supported by the core clock. But the cpufreq
table can use all or subset of the supported frequencies. The core
clock should be usable with the clock api independently and not tied
to be used only by cpufreq driver.

>
>> +
>> +static const struct samsung_core_clock_freq_table
>> exyno4210_armclk_table = {
>> +     .freq           = exynos4210_armclk_freqs,
>> +     .freq_count     = ARRAY_SIZE(exynos4210_armclk_freqs),
>> +     .data           = (void *)exynos4210_armclk_data,
>> +};
>> +
>> +static int exynos4210_armclk_set_rate(struct clk_hw *hw, unsigned
>> long drate,
>> +                                     unsigned long prate)
>> +{
>> +     struct samsung_core_clock *armclk;
>> +     const struct samsung_core_clock_freq_table *freq_tbl;
>> +     unsigned long *freq_data;
>> +     unsigned long mux_reg, idx;
>> +     void __iomem *base;
>> +
>> +     if (drate == prate)
>> +             return 0;
>> +
>> +     armclk = container_of(hw, struct samsung_core_clock, hw);
>> +     freq_tbl = armclk->freq_table;
>> +     freq_data = (unsigned long *)freq_tbl->data;
>> +     base = armclk->ctrl_base;
>> +
>> +     for (idx = 0; idx < freq_tbl->freq_count; idx++, freq_data
>> += 2)
>> +             if ((freq_tbl->freq[idx] * 1000) == drate)
>> +                     break;
>> +
>> +     if (!armclk->fout_pll)
>> +             armclk->fout_pll = __clk_lookup("fout_apll");\
>                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^[*]
>
> I'm a bit confused here for two reasons. Please correct me if I'm wrong.
>
> 1. You go into this ->set_rate() because of calling clk_set_rate at
> "arm_clk" clock (numbered as 12 at clk-exynos4.c) at cpufreq-cpu0.c
>
> In a Exynos4210 we have:
> XXTI-> APLL -> fout_apll -> mout_apll -> mout_core -> div_core
> -> div_core2 -> arm_clk
>
> In the code you call directly the fout_apll which changes
> frequency. Then the change shall be propagated to all registered
> clocks.
> I think, that DIV and DIV1 shall be reduced before PLL change [*],
> to reflect the changes at CCF.

The core clock implementation encapsulates multiple clock blocks (such
as dividers and muxes) which are in between the output of the APLL and
the point that actually is the cpu domain clock output. When a clock
frequency change has to be made, all these clock blocks encapsulated
within the core clock are programmed by pre-determined values. This
approach allows very fast clock speed switching, instead of traversing
the entire CCF clock tree searching for individual clock blocks to be
programmed.

>
>
>> +
>> +     if (drate < prate) {
>> +             mux_reg = readl(base + SRC_CPU);
>> +             writel(mux_reg | (1 << 16), base + SRC_CPU);
>> +             while (((readl(base + STAT_CPU) >> 16) & 0x7) != 2)
>> +                     ;
>                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [**]
>
> 2. I think, the above shall be done in a following way:
>
>         clk_set_parent(mout_core, mout_mpll);
>         clk_set_rate(armclk->fout_pll, drate);
>         clk_set_parent(mout_core, mout_apll);
>
> The direct write to registers [**] doesn't look compliant to CCF.
>

As mentioned above, the clock block encapsulates these clock blocks
into a single clock and only this single encapsulated clock is
registered with CCF. The internal implementation of how the different
clock blocks are managed within this clock is independent of the CCF.

>
> I'd rather thought about using "mout_core" instead of "arm_clk". Then we
> would get access to the parent directly:
>
>         struct clk *parent = clk_get_parent(hw->clk);
>
> so we set the parents explicitly (at clk registration) and call
> ->recalc_rate for clocks which are lower in the tree (like "div_core",
> "div_core2").

That was not the intention as mentioned above.

Thanks,
Thomas.

>
>> +             clk_set_rate(armclk->fout_pll, drate);
>> +     }
>> +
>> +     writel(freq_data[0], base + DIV_CPU0);
>> +     while (readl(base + DIV_STAT_CPU0) != 0)
>> +             ;
>> +     writel(freq_data[1], base + DIV_CPU1);
>> +     while (readl(base + DIV_STAT_CPU1) != 0)
>> +             ;
>> +
>> +     if (drate > prate) {
>> +             mux_reg = readl(base + SRC_CPU);
>> +             writel(mux_reg | (1 << 16), base + SRC_CPU);
>> +             while (((readl(base + STAT_CPU) >> 16) & 0x7) != 2)
>> +                     ;
>> +
>> +             clk_set_rate(armclk->fout_pll, drate);
>> +     }
>> +
>> +     mux_reg = readl(base + SRC_CPU);
>> +     writel(mux_reg & ~(1 << 16), base + SRC_CPU);
>> +     while (((readl(base + STAT_CPU) >> 16) & 0x7) != 1)
>> +                     ;
>> +     return 0;
>> +}
>> +
>> +static const struct clk_ops exynos4210_armclk_clk_ops = {
>> +     .recalc_rate = samsung_core_clock_recalc_rate,
>> +     .round_rate = samsung_core_clk_round_rate,
>> +     .set_rate = exynos4210_armclk_set_rate,
>> +};
>> +
>>  /* register exynos4 clocks */
>>  static void __init exynos4_clk_init(struct device_node *np,
>>                                   enum exynos4_soc exynos4_soc,
>> @@ -1164,6 +1254,10 @@ 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));
>> +             samsung_coreclk_register("armclk", armclk_p,
>> +                     ARRAY_SIZE(armclk_p), "fout_apll",
>> +                     &exynos4210_armclk_clk_ops, arm_clk,
>> +                     &exyno4210_armclk_table);
>>       } else {
>>               samsung_clk_register_mux(exynos4x12_mux_clks,
>>                       ARRAY_SIZE(exynos4x12_mux_clks));
>
>
>
> --
> Best regards,
>
> Lukasz Majewski
>
> Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

Re: [PATCH 3/6] clk: samsung: register cpu clock provider for exynos4210 SoC

[Lukasz Majewski]

Hi Thomas,

> Hi Lukasz,
> 
> On Fri, Jan 10, 2014 at 5:34 PM, Lukasz Majewski
> <l.majewski@samsung.com> wrote:
> > Hi Thomas,
> >
> >> Add a new clock provider for ARM clock domain. This clock provider
> >> is composed of multiple components which include mux_core,
> >> div_core, div_core2, div_corem0, div_corem1, div_periph, div_atb,
> >> div_pclk_dbg, div_copy and div_hpm. This composition of mutiple
> >> components into a single clock provider helps with faster
> >> completion of CPU clock speed switching during DVFS operations.
> >>
> >> Signed-off-by: Thomas Abraham <thomas.ab@samsung.com>
> >> ---
> >>  drivers/clk/samsung/clk-exynos4.c |   96
> >> ++++++++++++++++++++++++++++++++++++- 1 files changed, 95
> >> insertions(+), 1 deletions(-)
> >>
> >> diff --git a/drivers/clk/samsung/clk-exynos4.c
> >> b/drivers/clk/samsung/clk-exynos4.c index d967571..4bf2f93 100644
> >> --- a/drivers/clk/samsung/clk-exynos4.c
> >> +++ b/drivers/clk/samsung/clk-exynos4.c
> >> @@ -108,8 +108,11 @@
> >>  #define APLL_CON0            0x14100
> >>  #define E4210_MPLL_CON0              0x14108
> >>  #define SRC_CPU                      0x14200
> >> +#define STAT_CPU             0x14400
> >>  #define DIV_CPU0             0x14500
> >>  #define DIV_CPU1             0x14504
> >> +#define DIV_STAT_CPU0                0x14600
> >> +#define DIV_STAT_CPU1                0x14604
> >>  #define GATE_SCLK_CPU                0x14800
> >>  #define GATE_IP_CPU          0x14900
> >>  #define E4X12_DIV_ISP0               0x18300
> >> @@ -289,7 +292,7 @@ static unsigned long exynos4_clk_regs[]
> >> __initdata = { };
> >>
> >>  /* list of all parent clock list */
> >> -PNAME(mout_apll_p)   = { "fin_pll", "fout_apll", };
> >> +PNAME(mout_apll_p)   = { "fin_pll", "fout_apll1", };
> >>  PNAME(mout_mpll_p)   = { "fin_pll", "fout_mpll", };
> >>  PNAME(mout_epll_p)   = { "fin_pll", "fout_epll", };
> >>  PNAME(mout_vpllsrc_p)        = { "fin_pll", "sclk_hdmi24m", };
> >> @@ -306,6 +309,7 @@ PNAME(mout_onenand_p)  = {"aclk133",
> >> "aclk160", }; PNAME(mout_onenand1_p) = {"mout_onenand",
> >> "sclk_vpll", };
> >>
> >>  /* Exynos 4210-specific parent groups */
> >> +PNAME(armclk_p) = { "fout_apll", };
> >
> > Here you only give no parent clock, but at
> > samsung_coreclk_register() it is expected to provide list of
> > parents.
> 
> Here only one parent is listed, but the core clock type does not limit
> the number of parents that can be specified. A specific implementation
> can define and use multiple parents.

I only pointed out that the definition of the:

samsung_coreclk_register("armclk", armclk_p,
                    ARRAY_SIZE(armclk_p), "fout_apll",
                    &exynos4210_armclk_clk_ops, arm_clk,
                    &exyno4210_armclk_table);

Could only use parent, especially when you plan to change mux clock
(apll vs. mpll) by writing directly to registers (which I think is bad).

> 
> >
> >>  PNAME(sclk_vpll_p4210)       = { "mout_vpllsrc", "fout_vpll", };
> >>  PNAME(mout_core_p4210)       = { "mout_apll", "sclk_mpll", };
> >>  PNAME(sclk_ampll_p4210)      = { "sclk_mpll", "sclk_apll", };
> >> @@ -1089,6 +1093,92 @@ static struct samsung_pll_clock
> >> exynos4x12_plls[nr_plls] __initdata = { VPLL_LOCK, VPLL_CON0,
> >> NULL), };
> >>
> >> +#define EXYNOS4210_DIV_CPU0(apll, pclk_dbg, atb, periph, corem1,
> >> corem0) \
> >> +             ((apll << 24) | (pclk_dbg << 20) | (atb << 16) | \
> >> +             (periph << 12) | (corem1 << 8) | (corem0 << 4))
> >> +#define EXYNOS4210_DIV_CPU1(hpm, copy)       \
> >> +             ((hpm << 4) | (copy << 0))
> >> +static const unsigned long exynos4210_armclk_data[][2] = {
> >> +     { EXYNOS4210_DIV_CPU0(7, 1, 4, 3, 7, 3),
> >> EXYNOS4210_DIV_CPU1(0, 5), },
> >> +     { EXYNOS4210_DIV_CPU0(7, 1, 4, 3, 7, 3),
> >> EXYNOS4210_DIV_CPU1(0, 4), },
> >> +     { EXYNOS4210_DIV_CPU0(7, 1, 3, 3, 7, 3),
> >> EXYNOS4210_DIV_CPU1(0, 3), },
> >> +     { EXYNOS4210_DIV_CPU0(7, 1, 3, 3, 7, 3),
> >> EXYNOS4210_DIV_CPU1(0, 3), },
> >> +     { EXYNOS4210_DIV_CPU0(7, 1, 3, 3, 7, 3),
> >> EXYNOS4210_DIV_CPU1(0, 3), },
> >> +     { EXYNOS4210_DIV_CPU0(0, 1, 3, 1, 3, 1),
> >> EXYNOS4210_DIV_CPU1(0, 3), }, +};
> >> +
> >
> > What do you think about adding those parameters (like CPU dividers)
> > as an attribute to /cpus/cpu@0 node?
> 
> Not in CPU node but may be in clock controller node since these values
> are actually used by the clock controller.

/cpus/cpu@0 seems like a good place for them (since those DIVs are
related to core)
.
However, we can choose any better DT node to add it.

> But since these values are
> Exynos4210 specific and not generic enough to be reused across
> multiple Exynos SoCs, there is little benefit in defining bindings and
> parsing code for these values. It would be simpler enough to just
> embed them in the code.

It would be less to code, but isn't it the same ugly code, which we
have now at exynos4xxx-cpufreq.c?

With those values parsed from DT we can write generic code for the
"arm_clk" clock. One clock implementation for cpufreq-cpu0.c (and maybe
for arm_big_little.c) reused by Exynos4/5.

> 
> >
> >> +static const unsigned long exynos4210_armclk_freqs[] = {
> >> +     1200000 , 1000000, 800000, 500000, 400000, 200000,
> >> +};
> >
> > Those freq's are going to be defined at /cpus/cpu@0 at
> > operating-points attribute (or if possible took from
> > cpufreq_frequency table).
> 
> These are frequencies supported by the core clock. But the cpufreq
> table can use all or subset of the supported frequencies.

I see your point, but I find this distinction here a bit superfluous. 

> The core
> clock should be usable with the clock api independently and not tied
> to be used only by cpufreq driver.

But then still for Exynos it will use PLL's M P S coefficients which
only corresponds to values defined at cpufreq's frequency table.

The set of frequencies for PLL, cpufreq and this clock is the same, so
I think that we shall not define them in three different places.

Could you give any example supporting your point of view?

> 
> >
> >> +
> >> +static const struct samsung_core_clock_freq_table
> >> exyno4210_armclk_table = {
> >> +     .freq           = exynos4210_armclk_freqs,
> >> +     .freq_count     = ARRAY_SIZE(exynos4210_armclk_freqs),
> >> +     .data           = (void *)exynos4210_armclk_data,
> >> +};
> >> +
> >> +static int exynos4210_armclk_set_rate(struct clk_hw *hw, unsigned
> >> long drate,
> >> +                                     unsigned long prate)
> >> +{
> >> +     struct samsung_core_clock *armclk;
> >> +     const struct samsung_core_clock_freq_table *freq_tbl;
> >> +     unsigned long *freq_data;
> >> +     unsigned long mux_reg, idx;
> >> +     void __iomem *base;
> >> +
> >> +     if (drate == prate)
> >> +             return 0;
> >> +
> >> +     armclk = container_of(hw, struct samsung_core_clock, hw);
> >> +     freq_tbl = armclk->freq_table;
> >> +     freq_data = (unsigned long *)freq_tbl->data;
> >> +     base = armclk->ctrl_base;
> >> +
> >> +     for (idx = 0; idx < freq_tbl->freq_count; idx++, freq_data
> >> += 2)
> >> +             if ((freq_tbl->freq[idx] * 1000) == drate)
> >> +                     break;
> >> +
> >> +     if (!armclk->fout_pll)
> >> +             armclk->fout_pll = __clk_lookup("fout_apll");\
> >                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^[*]
> >
> > I'm a bit confused here for two reasons. Please correct me if I'm
> > wrong.
> >
> > 1. You go into this ->set_rate() because of calling clk_set_rate at
> > "arm_clk" clock (numbered as 12 at clk-exynos4.c) at cpufreq-cpu0.c
> >
> > In a Exynos4210 we have:
> > XXTI-> APLL -> fout_apll -> mout_apll -> mout_core -> div_core
> > -> div_core2 -> arm_clk
> >
> > In the code you call directly the fout_apll which changes
> > frequency. Then the change shall be propagated to all registered
> > clocks.
> > I think, that DIV and DIV1 shall be reduced before PLL change [*],
> > to reflect the changes at CCF.
> 
> The core clock implementation encapsulates multiple clock blocks (such
> as dividers and muxes) which are in between the output of the APLL and
> the point that actually is the cpu domain clock output. 

No problem with that. I mostly agree...

> When a clock
> frequency change has to be made, all these clock blocks encapsulated
> within the core clock are programmed by pre-determined values. 

And what about the situation with already defined clocks (like
"div_core" and "div_core2"). Those will not be updated when you first
call clk_set_rate() and change by hand DIV and DIV1.

What if you would like to have the PCLK_DBG clock used in the future?
You would add it to CCF and the change will not propagate.

> This
> approach allows very fast clock speed switching, instead of traversing
> the entire CCF clock tree searching for individual clock blocks to be
> programmed.

Those are mostly DIV and MUXes. Recalculation shouldn't be time
consuming.

> 
> >
> >
> >> +
> >> +     if (drate < prate) {
> >> +             mux_reg = readl(base + SRC_CPU);
> >> +             writel(mux_reg | (1 << 16), base + SRC_CPU);
> >> +             while (((readl(base + STAT_CPU) >> 16) & 0x7) != 2)
> >> +                     ;
> >                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [**]
> >
> > 2. I think, the above shall be done in a following way:
> >
> >         clk_set_parent(mout_core, mout_mpll);
> >         clk_set_rate(armclk->fout_pll, drate);
> >         clk_set_parent(mout_core, mout_apll);
> >
> > The direct write to registers [**] doesn't look compliant to CCF.
> >
> 
> As mentioned above, the clock block encapsulates these clock blocks
> into a single clock and only this single encapsulated clock is
> registered with CCF. The internal implementation of how the different
> clock blocks are managed within this clock is independent of the CCF.

I agree, that the CPU_DIV and CPU_DIV1 shall be changed atomically
(without CCF).

But on the situation [**] the MUX can be changed by clk_set_parent() as
it is now done at exynosXXXX-cpufreq.c code.


> 
> >
> > I'd rather thought about using "mout_core" instead of "arm_clk".
> > Then we would get access to the parent directly:
> >
> >         struct clk *parent = clk_get_parent(hw->clk);
> >
> > so we set the parents explicitly (at clk registration) and call
> > ->recalc_rate for clocks which are lower in the tree (like
> > "div_core", "div_core2").
> 
> That was not the intention as mentioned above.

This is just another possible solution to the problem. 

> 
> Thanks,
> Thomas.
> 
> >
> >> +             clk_set_rate(armclk->fout_pll, drate);
> >> +     }
> >> +
> >> +     writel(freq_data[0], base + DIV_CPU0);
> >> +     while (readl(base + DIV_STAT_CPU0) != 0)
> >> +             ;
> >> +     writel(freq_data[1], base + DIV_CPU1);
> >> +     while (readl(base + DIV_STAT_CPU1) != 0)
> >> +             ;
> >> +
> >> +     if (drate > prate) {
> >> +             mux_reg = readl(base + SRC_CPU);
> >> +             writel(mux_reg | (1 << 16), base + SRC_CPU);
> >> +             while (((readl(base + STAT_CPU) >> 16) & 0x7) != 2)
> >> +                     ;
> >> +
> >> +             clk_set_rate(armclk->fout_pll, drate);
> >> +     }
> >> +
> >> +     mux_reg = readl(base + SRC_CPU);
> >> +     writel(mux_reg & ~(1 << 16), base + SRC_CPU);
> >> +     while (((readl(base + STAT_CPU) >> 16) & 0x7) != 1)
> >> +                     ;
> >> +     return 0;
> >> +}
> >> +
> >> +static const struct clk_ops exynos4210_armclk_clk_ops = {
> >> +     .recalc_rate = samsung_core_clock_recalc_rate,
> >> +     .round_rate = samsung_core_clk_round_rate,
> >> +     .set_rate = exynos4210_armclk_set_rate,
> >> +};
> >> +
> >>  /* register exynos4 clocks */
> >>  static void __init exynos4_clk_init(struct device_node *np,
> >>                                   enum exynos4_soc exynos4_soc,
> >> @@ -1164,6 +1254,10 @@ 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));
> >> +             samsung_coreclk_register("armclk", armclk_p,
> >> +                     ARRAY_SIZE(armclk_p), "fout_apll",
> >> +                     &exynos4210_armclk_clk_ops, arm_clk,
> >> +                     &exyno4210_armclk_table);
> >>       } else {
> >>               samsung_clk_register_mux(exynos4x12_mux_clks,
> >>                       ARRAY_SIZE(exynos4x12_mux_clks));
> >
> >
> >
> > --
> > Best regards,
> >
> > Lukasz Majewski
> >
> > Samsung R&D Institute Poland (SRPOL) | Linux Platform Group



-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

Re: [PATCH 3/6] clk: samsung: register cpu clock provider for exynos4210 SoC

[Thomas Abraham]

On Fri, Jan 10, 2014 at 7:48 PM, Lukasz Majewski <l.majewski@samsung.com> wrote:
> Hi Thomas,
>
>> Hi Lukasz,
>>
>> On Fri, Jan 10, 2014 at 5:34 PM, Lukasz Majewski
>> <l.majewski@samsung.com> wrote:
>> > Hi Thomas,
>> >
>> >> Add a new clock provider for ARM clock domain. This clock provider
>> >> is composed of multiple components which include mux_core,
>> >> div_core, div_core2, div_corem0, div_corem1, div_periph, div_atb,
>> >> div_pclk_dbg, div_copy and div_hpm. This composition of mutiple
>> >> components into a single clock provider helps with faster
>> >> completion of CPU clock speed switching during DVFS operations.
>> >>
>> >> Signed-off-by: Thomas Abraham <thomas.ab@samsung.com>
>> >> ---
>> >>  drivers/clk/samsung/clk-exynos4.c |   96
>> >> ++++++++++++++++++++++++++++++++++++- 1 files changed, 95
>> >> insertions(+), 1 deletions(-)
>> >>
>> >> diff --git a/drivers/clk/samsung/clk-exynos4.c
>> >> b/drivers/clk/samsung/clk-exynos4.c index d967571..4bf2f93 100644
>> >> --- a/drivers/clk/samsung/clk-exynos4.c
>> >> +++ b/drivers/clk/samsung/clk-exynos4.c
>> >> @@ -108,8 +108,11 @@
>> >>  #define APLL_CON0            0x14100
>> >>  #define E4210_MPLL_CON0              0x14108
>> >>  #define SRC_CPU                      0x14200
>> >> +#define STAT_CPU             0x14400
>> >>  #define DIV_CPU0             0x14500
>> >>  #define DIV_CPU1             0x14504
>> >> +#define DIV_STAT_CPU0                0x14600
>> >> +#define DIV_STAT_CPU1                0x14604
>> >>  #define GATE_SCLK_CPU                0x14800
>> >>  #define GATE_IP_CPU          0x14900
>> >>  #define E4X12_DIV_ISP0               0x18300
>> >> @@ -289,7 +292,7 @@ static unsigned long exynos4_clk_regs[]
>> >> __initdata = { };
>> >>
>> >>  /* list of all parent clock list */
>> >> -PNAME(mout_apll_p)   = { "fin_pll", "fout_apll", };
>> >> +PNAME(mout_apll_p)   = { "fin_pll", "fout_apll1", };
>> >>  PNAME(mout_mpll_p)   = { "fin_pll", "fout_mpll", };
>> >>  PNAME(mout_epll_p)   = { "fin_pll", "fout_epll", };
>> >>  PNAME(mout_vpllsrc_p)        = { "fin_pll", "sclk_hdmi24m", };
>> >> @@ -306,6 +309,7 @@ PNAME(mout_onenand_p)  = {"aclk133",
>> >> "aclk160", }; PNAME(mout_onenand1_p) = {"mout_onenand",
>> >> "sclk_vpll", };
>> >>
>> >>  /* Exynos 4210-specific parent groups */
>> >> +PNAME(armclk_p) = { "fout_apll", };
>> >
>> > Here you only give no parent clock, but at
>> > samsung_coreclk_register() it is expected to provide list of
>> > parents.
>>
>> Here only one parent is listed, but the core clock type does not limit
>> the number of parents that can be specified. A specific implementation
>> can define and use multiple parents.
>
> I only pointed out that the definition of the:
>
> samsung_coreclk_register("armclk", armclk_p,
>                     ARRAY_SIZE(armclk_p), "fout_apll",
>                     &exynos4210_armclk_clk_ops, arm_clk,
>                     &exyno4210_armclk_table);
>
> Could only use parent, especially when you plan to change mux clock
> (apll vs. mpll) by writing directly to registers (which I think is bad).

This definition is not limited to be used only on Exynos4210. This is
a generic core clock registration helper function intended to be
reusable across multiple Samsung SoCs.

>
>>
>> >
>> >>  PNAME(sclk_vpll_p4210)       = { "mout_vpllsrc", "fout_vpll", };
>> >>  PNAME(mout_core_p4210)       = { "mout_apll", "sclk_mpll", };
>> >>  PNAME(sclk_ampll_p4210)      = { "sclk_mpll", "sclk_apll", };
>> >> @@ -1089,6 +1093,92 @@ static struct samsung_pll_clock
>> >> exynos4x12_plls[nr_plls] __initdata = { VPLL_LOCK, VPLL_CON0,
>> >> NULL), };
>> >>
>> >> +#define EXYNOS4210_DIV_CPU0(apll, pclk_dbg, atb, periph, corem1,
>> >> corem0) \
>> >> +             ((apll << 24) | (pclk_dbg << 20) | (atb << 16) | \
>> >> +             (periph << 12) | (corem1 << 8) | (corem0 << 4))
>> >> +#define EXYNOS4210_DIV_CPU1(hpm, copy)       \
>> >> +             ((hpm << 4) | (copy << 0))
>> >> +static const unsigned long exynos4210_armclk_data[][2] = {
>> >> +     { EXYNOS4210_DIV_CPU0(7, 1, 4, 3, 7, 3),
>> >> EXYNOS4210_DIV_CPU1(0, 5), },
>> >> +     { EXYNOS4210_DIV_CPU0(7, 1, 4, 3, 7, 3),
>> >> EXYNOS4210_DIV_CPU1(0, 4), },
>> >> +     { EXYNOS4210_DIV_CPU0(7, 1, 3, 3, 7, 3),
>> >> EXYNOS4210_DIV_CPU1(0, 3), },
>> >> +     { EXYNOS4210_DIV_CPU0(7, 1, 3, 3, 7, 3),
>> >> EXYNOS4210_DIV_CPU1(0, 3), },
>> >> +     { EXYNOS4210_DIV_CPU0(7, 1, 3, 3, 7, 3),
>> >> EXYNOS4210_DIV_CPU1(0, 3), },
>> >> +     { EXYNOS4210_DIV_CPU0(0, 1, 3, 1, 3, 1),
>> >> EXYNOS4210_DIV_CPU1(0, 3), }, +};
>> >> +
>> >
>> > What do you think about adding those parameters (like CPU dividers)
>> > as an attribute to /cpus/cpu@0 node?
>>
>> Not in CPU node but may be in clock controller node since these values
>> are actually used by the clock controller.
>
> /cpus/cpu@0 seems like a good place for them (since those DIVs are
> related to core)

DIVs belong to the clock controller, not the CPU, and are addressed
from the clock controller address space.

> .
> However, we can choose any better DT node to add it.
>
>> But since these values are
>> Exynos4210 specific and not generic enough to be reused across
>> multiple Exynos SoCs, there is little benefit in defining bindings and
>> parsing code for these values. It would be simpler enough to just
>> embed them in the code.
>
> It would be less to code, but isn't it the same ugly code, which we
> have now at exynos4xxx-cpufreq.c?
>
> With those values parsed from DT we can write generic code for the
> "arm_clk" clock. One clock implementation for cpufreq-cpu0.c (and maybe
> for arm_big_little.c) reused by Exynos4/5.

As replied in the 2/3 patch, if these values would change across
multiple platforms based on the same SoC, it makes sense to put them
into DT. Any data that is purely SoC specific and not going to change
across platforms can be embedded into the code itself.

>
>>
>> >
>> >> +static const unsigned long exynos4210_armclk_freqs[] = {
>> >> +     1200000 , 1000000, 800000, 500000, 400000, 200000,
>> >> +};
>> >
>> > Those freq's are going to be defined at /cpus/cpu@0 at
>> > operating-points attribute (or if possible took from
>> > cpufreq_frequency table).
>>
>> These are frequencies supported by the core clock. But the cpufreq
>> table can use all or subset of the supported frequencies.
>
> I see your point, but I find this distinction here a bit superfluous.

Replied to similar comment in 2/3 patch.

>
>> The core
>> clock should be usable with the clock api independently and not tied
>> to be used only by cpufreq driver.
>
> But then still for Exynos it will use PLL's M P S coefficients which
> only corresponds to values defined at cpufreq's frequency table.

The PLL clocks are now separated out as PLL clock types in
samsung/clk-pll.c file. The P,M,S values of the PLLs are now handled
over there. So now the PLL is independent of the cpufreq driver and
can support any number of clock speeds not limited to the ones needed
by cpufreq.

>
> The set of frequencies for PLL, cpufreq and this clock is the same, so
> I think that we shall not define them in three different places.
>
> Could you give any example supporting your point of view?

A PLL is a hardware component that can be reused in multiple SoCs. A
PLL can generate and support 'x' number of clock speeds but a SoC
using that PLL might use only 'y' (a subset of 'x')  number of clock
speeds of the PLL due to certain hardware limitations. Then there are
platforms using a SoC which might use only 'z' (a subset of 'y') clock
speeds due to the power/performance requirements of the platform.

>
>>
>> >
>> >> +
>> >> +static const struct samsung_core_clock_freq_table
>> >> exyno4210_armclk_table = {
>> >> +     .freq           = exynos4210_armclk_freqs,
>> >> +     .freq_count     = ARRAY_SIZE(exynos4210_armclk_freqs),
>> >> +     .data           = (void *)exynos4210_armclk_data,
>> >> +};
>> >> +
>> >> +static int exynos4210_armclk_set_rate(struct clk_hw *hw, unsigned
>> >> long drate,
>> >> +                                     unsigned long prate)
>> >> +{
>> >> +     struct samsung_core_clock *armclk;
>> >> +     const struct samsung_core_clock_freq_table *freq_tbl;
>> >> +     unsigned long *freq_data;
>> >> +     unsigned long mux_reg, idx;
>> >> +     void __iomem *base;
>> >> +
>> >> +     if (drate == prate)
>> >> +             return 0;
>> >> +
>> >> +     armclk = container_of(hw, struct samsung_core_clock, hw);
>> >> +     freq_tbl = armclk->freq_table;
>> >> +     freq_data = (unsigned long *)freq_tbl->data;
>> >> +     base = armclk->ctrl_base;
>> >> +
>> >> +     for (idx = 0; idx < freq_tbl->freq_count; idx++, freq_data
>> >> += 2)
>> >> +             if ((freq_tbl->freq[idx] * 1000) == drate)
>> >> +                     break;
>> >> +
>> >> +     if (!armclk->fout_pll)
>> >> +             armclk->fout_pll = __clk_lookup("fout_apll");\
>> >                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^[*]
>> >
>> > I'm a bit confused here for two reasons. Please correct me if I'm
>> > wrong.
>> >
>> > 1. You go into this ->set_rate() because of calling clk_set_rate at
>> > "arm_clk" clock (numbered as 12 at clk-exynos4.c) at cpufreq-cpu0.c
>> >
>> > In a Exynos4210 we have:
>> > XXTI-> APLL -> fout_apll -> mout_apll -> mout_core -> div_core
>> > -> div_core2 -> arm_clk
>> >
>> > In the code you call directly the fout_apll which changes
>> > frequency. Then the change shall be propagated to all registered
>> > clocks.
>> > I think, that DIV and DIV1 shall be reduced before PLL change [*],
>> > to reflect the changes at CCF.
>>
>> The core clock implementation encapsulates multiple clock blocks (such
>> as dividers and muxes) which are in between the output of the APLL and
>> the point that actually is the cpu domain clock output.
>
> No problem with that. I mostly agree...
>
>> When a clock
>> frequency change has to be made, all these clock blocks encapsulated
>> within the core clock are programmed by pre-determined values.
>
> And what about the situation with already defined clocks (like
> "div_core" and "div_core2"). Those will not be updated when you first
> call clk_set_rate() and change by hand DIV and DIV1.
>
> What if you would like to have the PCLK_DBG clock used in the future?
> You would add it to CCF and the change will not propagate.

I did intend to remove individual clock blocks which are now
encapsulated within the core clock type from the clock driver file. I
missed doing that in this patch series.

>
>> This
>> approach allows very fast clock speed switching, instead of traversing
>> the entire CCF clock tree searching for individual clock blocks to be
>> programmed.
>
> Those are mostly DIV and MUXes. Recalculation shouldn't be time
> consuming.

I was mainly referring to the time taken to search the clock tree for
these individual clock blocks.

>
>>
>> >
>> >
>> >> +
>> >> +     if (drate < prate) {
>> >> +             mux_reg = readl(base + SRC_CPU);
>> >> +             writel(mux_reg | (1 << 16), base + SRC_CPU);
>> >> +             while (((readl(base + STAT_CPU) >> 16) & 0x7) != 2)
>> >> +                     ;
>> >                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [**]
>> >
>> > 2. I think, the above shall be done in a following way:
>> >
>> >         clk_set_parent(mout_core, mout_mpll);
>> >         clk_set_rate(armclk->fout_pll, drate);
>> >         clk_set_parent(mout_core, mout_apll);
>> >
>> > The direct write to registers [**] doesn't look compliant to CCF.
>> >
>>
>> As mentioned above, the clock block encapsulates these clock blocks
>> into a single clock and only this single encapsulated clock is
>> registered with CCF. The internal implementation of how the different
>> clock blocks are managed within this clock is independent of the CCF.
>
> I agree, that the CPU_DIV and CPU_DIV1 shall be changed atomically
> (without CCF).
>
> But on the situation [**] the MUX can be changed by clk_set_parent() as
> it is now done at exynosXXXX-cpufreq.c code.

The mux is also encapsulated into a larger clock type and this new
clock type know how the mux has to be configured.

>
>
>>
>> >
>> > I'd rather thought about using "mout_core" instead of "arm_clk".
>> > Then we would get access to the parent directly:
>> >
>> >         struct clk *parent = clk_get_parent(hw->clk);
>> >
>> > so we set the parents explicitly (at clk registration) and call
>> > ->recalc_rate for clocks which are lower in the tree (like
>> > "div_core", "div_core2").
>>
>> That was not the intention as mentioned above.
>
> This is just another possible solution to the problem.
>
>>
>> Thanks,
>> Thomas.
>>
>> >
>> >> +             clk_set_rate(armclk->fout_pll, drate);
>> >> +     }
>> >> +
>> >> +     writel(freq_data[0], base + DIV_CPU0);
>> >> +     while (readl(base + DIV_STAT_CPU0) != 0)
>> >> +             ;
>> >> +     writel(freq_data[1], base + DIV_CPU1);
>> >> +     while (readl(base + DIV_STAT_CPU1) != 0)
>> >> +             ;
>> >> +
>> >> +     if (drate > prate) {
>> >> +             mux_reg = readl(base + SRC_CPU);
>> >> +             writel(mux_reg | (1 << 16), base + SRC_CPU);
>> >> +             while (((readl(base + STAT_CPU) >> 16) & 0x7) != 2)
>> >> +                     ;
>> >> +
>> >> +             clk_set_rate(armclk->fout_pll, drate);
>> >> +     }
>> >> +
>> >> +     mux_reg = readl(base + SRC_CPU);
>> >> +     writel(mux_reg & ~(1 << 16), base + SRC_CPU);
>> >> +     while (((readl(base + STAT_CPU) >> 16) & 0x7) != 1)
>> >> +                     ;
>> >> +     return 0;
>> >> +}
>> >> +
>> >> +static const struct clk_ops exynos4210_armclk_clk_ops = {
>> >> +     .recalc_rate = samsung_core_clock_recalc_rate,
>> >> +     .round_rate = samsung_core_clk_round_rate,
>> >> +     .set_rate = exynos4210_armclk_set_rate,
>> >> +};
>> >> +
>> >>  /* register exynos4 clocks */
>> >>  static void __init exynos4_clk_init(struct device_node *np,
>> >>                                   enum exynos4_soc exynos4_soc,
>> >> @@ -1164,6 +1254,10 @@ 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));
>> >> +             samsung_coreclk_register("armclk", armclk_p,
>> >> +                     ARRAY_SIZE(armclk_p), "fout_apll",
>> >> +                     &exynos4210_armclk_clk_ops, arm_clk,
>> >> +                     &exyno4210_armclk_table);
>> >>       } else {
>> >>               samsung_clk_register_mux(exynos4x12_mux_clks,
>> >>                       ARRAY_SIZE(exynos4x12_mux_clks));
>> >
>> >
>> >
>> > --
>> > Best regards,
>> >
>> > Lukasz Majewski
>> >
>> > Samsung R&D Institute Poland (SRPOL) | Linux Platform Group
>
>
>
> --
> Best regards,
>
> Lukasz Majewski
>
> Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

Re: [PATCH 3/6] clk: samsung: register cpu clock provider for exynos4210 SoC

[Tomasz Figa]

On 11.01.2014 06:25, Thomas Abraham wrote:
> On Fri, Jan 10, 2014 at 7:48 PM, Lukasz Majewski <l.majewski@samsung.com> wrote:
>> Hi Thomas,
>>
>>> Hi Lukasz,
>>>
>>> On Fri, Jan 10, 2014 at 5:34 PM, Lukasz Majewski
>>> <l.majewski@samsung.com> wrote:
>>>> Hi Thomas,
>>>>
>>>>> Add a new clock provider for ARM clock domain. This clock provider
>>>>> is composed of multiple components which include mux_core,
>>>>> div_core, div_core2, div_corem0, div_corem1, div_periph, div_atb,
>>>>> div_pclk_dbg, div_copy and div_hpm. This composition of mutiple
>>>>> components into a single clock provider helps with faster
>>>>> completion of CPU clock speed switching during DVFS operations.
>>>>>
>>>>> Signed-off-by: Thomas Abraham <thomas.ab@samsung.com>
>>>>> ---
>>>>>   drivers/clk/samsung/clk-exynos4.c |   96
>>>>> ++++++++++++++++++++++++++++++++++++- 1 files changed, 95
>>>>> insertions(+), 1 deletions(-)
>>>>>
>>>>> diff --git a/drivers/clk/samsung/clk-exynos4.c
>>>>> b/drivers/clk/samsung/clk-exynos4.c index d967571..4bf2f93 100644
>>>>> --- a/drivers/clk/samsung/clk-exynos4.c
>>>>> +++ b/drivers/clk/samsung/clk-exynos4.c
>>>>> @@ -108,8 +108,11 @@
>>>>>   #define APLL_CON0            0x14100
>>>>>   #define E4210_MPLL_CON0              0x14108
>>>>>   #define SRC_CPU                      0x14200
>>>>> +#define STAT_CPU             0x14400
>>>>>   #define DIV_CPU0             0x14500
>>>>>   #define DIV_CPU1             0x14504
>>>>> +#define DIV_STAT_CPU0                0x14600
>>>>> +#define DIV_STAT_CPU1                0x14604
>>>>>   #define GATE_SCLK_CPU                0x14800
>>>>>   #define GATE_IP_CPU          0x14900
>>>>>   #define E4X12_DIV_ISP0               0x18300
>>>>> @@ -289,7 +292,7 @@ static unsigned long exynos4_clk_regs[]
>>>>> __initdata = { };
>>>>>
>>>>>   /* list of all parent clock list */
>>>>> -PNAME(mout_apll_p)   = { "fin_pll", "fout_apll", };
>>>>> +PNAME(mout_apll_p)   = { "fin_pll", "fout_apll1", };
>>>>>   PNAME(mout_mpll_p)   = { "fin_pll", "fout_mpll", };
>>>>>   PNAME(mout_epll_p)   = { "fin_pll", "fout_epll", };
>>>>>   PNAME(mout_vpllsrc_p)        = { "fin_pll", "sclk_hdmi24m", };
>>>>> @@ -306,6 +309,7 @@ PNAME(mout_onenand_p)  = {"aclk133",
>>>>> "aclk160", }; PNAME(mout_onenand1_p) = {"mout_onenand",
>>>>> "sclk_vpll", };
>>>>>
>>>>>   /* Exynos 4210-specific parent groups */
>>>>> +PNAME(armclk_p) = { "fout_apll", };
>>>>
>>>> Here you only give no parent clock, but at
>>>> samsung_coreclk_register() it is expected to provide list of
>>>> parents.
>>>
>>> Here only one parent is listed, but the core clock type does not limit
>>> the number of parents that can be specified. A specific implementation
>>> can define and use multiple parents.
>>
>> I only pointed out that the definition of the:
>>
>> samsung_coreclk_register("armclk", armclk_p,
>>                      ARRAY_SIZE(armclk_p), "fout_apll",
>>                      &exynos4210_armclk_clk_ops, arm_clk,
>>                      &exyno4210_armclk_table);
>>
>> Could only use parent, especially when you plan to change mux clock
>> (apll vs. mpll) by writing directly to registers (which I think is bad).
>
> This definition is not limited to be used only on Exynos4210. This is
> a generic core clock registration helper function intended to be
> reusable across multiple Samsung SoCs.
>

I think Lukasz meant that you should rather use parent list to pass any 
input clocks of the core clock block, instead of hardcoded clock look-up 
inside clock ops.

>>
>>>
>>>>
>>>>>   PNAME(sclk_vpll_p4210)       = { "mout_vpllsrc", "fout_vpll", };
>>>>>   PNAME(mout_core_p4210)       = { "mout_apll", "sclk_mpll", };
>>>>>   PNAME(sclk_ampll_p4210)      = { "sclk_mpll", "sclk_apll", };
>>>>> @@ -1089,6 +1093,92 @@ static struct samsung_pll_clock
>>>>> exynos4x12_plls[nr_plls] __initdata = { VPLL_LOCK, VPLL_CON0,
>>>>> NULL), };
>>>>>
>>>>> +#define EXYNOS4210_DIV_CPU0(apll, pclk_dbg, atb, periph, corem1,
>>>>> corem0) \
>>>>> +             ((apll << 24) | (pclk_dbg << 20) | (atb << 16) | \
>>>>> +             (periph << 12) | (corem1 << 8) | (corem0 << 4))
>>>>> +#define EXYNOS4210_DIV_CPU1(hpm, copy)       \
>>>>> +             ((hpm << 4) | (copy << 0))
>>>>> +static const unsigned long exynos4210_armclk_data[][2] = {
>>>>> +     { EXYNOS4210_DIV_CPU0(7, 1, 4, 3, 7, 3),
>>>>> EXYNOS4210_DIV_CPU1(0, 5), },
>>>>> +     { EXYNOS4210_DIV_CPU0(7, 1, 4, 3, 7, 3),
>>>>> EXYNOS4210_DIV_CPU1(0, 4), },
>>>>> +     { EXYNOS4210_DIV_CPU0(7, 1, 3, 3, 7, 3),
>>>>> EXYNOS4210_DIV_CPU1(0, 3), },
>>>>> +     { EXYNOS4210_DIV_CPU0(7, 1, 3, 3, 7, 3),
>>>>> EXYNOS4210_DIV_CPU1(0, 3), },
>>>>> +     { EXYNOS4210_DIV_CPU0(7, 1, 3, 3, 7, 3),
>>>>> EXYNOS4210_DIV_CPU1(0, 3), },
>>>>> +     { EXYNOS4210_DIV_CPU0(0, 1, 3, 1, 3, 1),
>>>>> EXYNOS4210_DIV_CPU1(0, 3), }, +};
>>>>> +
>>>>
>>>> What do you think about adding those parameters (like CPU dividers)
>>>> as an attribute to /cpus/cpu@0 node?
>>>
>>> Not in CPU node but may be in clock controller node since these values
>>> are actually used by the clock controller.
>>
>> /cpus/cpu@0 seems like a good place for them (since those DIVs are
>> related to core)
>
> DIVs belong to the clock controller, not the CPU, and are addressed
> from the clock controller address space.

I also think that the contents of cpu@0 node should be limited only to 
CPU specific data. Personally I don't even like the idea of having 
operating points there - I would rather see frequency limits there, i.e. 
maximum frequency allowed at given voltage; specific frequency values 
could be inferred from available APLL/core clock configurations.

>
>> .
>> However, we can choose any better DT node to add it.
>>
>>> But since these values are
>>> Exynos4210 specific and not generic enough to be reused across
>>> multiple Exynos SoCs, there is little benefit in defining bindings and
>>> parsing code for these values. It would be simpler enough to just
>>> embed them in the code.
>>
>> It would be less to code, but isn't it the same ugly code, which we
>> have now at exynos4xxx-cpufreq.c?
>>
>> With those values parsed from DT we can write generic code for the
>> "arm_clk" clock. One clock implementation for cpufreq-cpu0.c (and maybe
>> for arm_big_little.c) reused by Exynos4/5.
>
> As replied in the 2/3 patch, if these values would change across
> multiple platforms based on the same SoC, it makes sense to put them
> into DT. Any data that is purely SoC specific and not going to change
> across platforms can be embedded into the code itself.

Well, they do change. If not on per board basis, then at least with SoC 
revisions.

Anyway, the biggest problem is that the same data needs to be duplicated 
(well, triplicated) for each driver that needs them. If you can find a 
reasonable way to avoid redundancy, without having DT involved, I will 
probably be fine with it.

>>>
>>> These are frequencies supported by the core clock. But the cpufreq
>>> table can use all or subset of the supported frequencies.
>>
>> I see your point, but I find this distinction here a bit superfluous.
>
> Replied to similar comment in 2/3 patch.
>
>>
>>> The core
>>> clock should be usable with the clock api independently and not tied
>>> to be used only by cpufreq driver.
>>
>> But then still for Exynos it will use PLL's M P S coefficients which
>> only corresponds to values defined at cpufreq's frequency table.
>
> The PLL clocks are now separated out as PLL clock types in
> samsung/clk-pll.c file. The P,M,S values of the PLLs are now handled
> over there. So now the PLL is independent of the cpufreq driver and
> can support any number of clock speeds not limited to the ones needed
> by cpufreq.

Don't forget about opposite side of this relation. The APLL needs to 
support at least those that are required by cpufreq.

>
>>
>> The set of frequencies for PLL, cpufreq and this clock is the same, so
>> I think that we shall not define them in three different places.
>>
>> Could you give any example supporting your point of view?
>
> A PLL is a hardware component that can be reused in multiple SoCs. A
> PLL can generate and support 'x' number of clock speeds but a SoC
> using that PLL might use only 'y' (a subset of 'x')  number of clock
> speeds of the PLL due to certain hardware limitations. Then there are
> platforms using a SoC which might use only 'z' (a subset of 'y') clock
> speeds due to the power/performance requirements of the platform.
>

As observed with our platforms, 'x' is usually SoC or SoC-revision 
specific and equal to 'y' and 'z' on respective platforms.

>>
>>>
>>>>
>>>>> +
>>>>> +static const struct samsung_core_clock_freq_table
>>>>> exyno4210_armclk_table = {
>>>>> +     .freq           = exynos4210_armclk_freqs,
>>>>> +     .freq_count     = ARRAY_SIZE(exynos4210_armclk_freqs),
>>>>> +     .data           = (void *)exynos4210_armclk_data,
>>>>> +};
>>>>> +
>>>>> +static int exynos4210_armclk_set_rate(struct clk_hw *hw, unsigned
>>>>> long drate,
>>>>> +                                     unsigned long prate)
>>>>> +{
>>>>> +     struct samsung_core_clock *armclk;
>>>>> +     const struct samsung_core_clock_freq_table *freq_tbl;
>>>>> +     unsigned long *freq_data;
>>>>> +     unsigned long mux_reg, idx;
>>>>> +     void __iomem *base;
>>>>> +
>>>>> +     if (drate == prate)
>>>>> +             return 0;
>>>>> +
>>>>> +     armclk = container_of(hw, struct samsung_core_clock, hw);
>>>>> +     freq_tbl = armclk->freq_table;
>>>>> +     freq_data = (unsigned long *)freq_tbl->data;
>>>>> +     base = armclk->ctrl_base;
>>>>> +
>>>>> +     for (idx = 0; idx < freq_tbl->freq_count; idx++, freq_data
>>>>> += 2)
>>>>> +             if ((freq_tbl->freq[idx] * 1000) == drate)
>>>>> +                     break;
>>>>> +
>>>>> +     if (!armclk->fout_pll)
>>>>> +             armclk->fout_pll = __clk_lookup("fout_apll");\
>>>>                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^[*]
>>>>
>>>> I'm a bit confused here for two reasons. Please correct me if I'm
>>>> wrong.
>>>>
>>>> 1. You go into this ->set_rate() because of calling clk_set_rate at
>>>> "arm_clk" clock (numbered as 12 at clk-exynos4.c) at cpufreq-cpu0.c
>>>>
>>>> In a Exynos4210 we have:
>>>> XXTI-> APLL -> fout_apll -> mout_apll -> mout_core -> div_core
>>>> -> div_core2 -> arm_clk
>>>>
>>>> In the code you call directly the fout_apll which changes
>>>> frequency. Then the change shall be propagated to all registered
>>>> clocks.
>>>> I think, that DIV and DIV1 shall be reduced before PLL change [*],
>>>> to reflect the changes at CCF.
>>>
>>> The core clock implementation encapsulates multiple clock blocks (such
>>> as dividers and muxes) which are in between the output of the APLL and
>>> the point that actually is the cpu domain clock output.
>>
>> No problem with that. I mostly agree...
>>
>>> When a clock
>>> frequency change has to be made, all these clock blocks encapsulated
>>> within the core clock are programmed by pre-determined values.
>>
>> And what about the situation with already defined clocks (like
>> "div_core" and "div_core2"). Those will not be updated when you first
>> call clk_set_rate() and change by hand DIV and DIV1.
>>
>> What if you would like to have the PCLK_DBG clock used in the future?
>> You would add it to CCF and the change will not propagate.
>
> I did intend to remove individual clock blocks which are now
> encapsulated within the core clock type from the clock driver file. I
> missed doing that in this patch series.

Yes, they should be removed, since they are encapsulated inside the core 
clock now.

>
>>
>>> This
>>> approach allows very fast clock speed switching, instead of traversing
>>> the entire CCF clock tree searching for individual clock blocks to be
>>> programmed.
>>
>> Those are mostly DIV and MUXes. Recalculation shouldn't be time
>> consuming.
>
> I was mainly referring to the time taken to search the clock tree for
> these individual clock blocks.

Hmm, why couldn't you simply look-up all the needed clock at 
initialization and keep references to them?

Still, I think it is fine to directly program registers of encapsulated 
clocks, since they are not visible outside anymore and there is no need 
for them to be visible.

>
>>
>>>
>>>>
>>>>
>>>>> +
>>>>> +     if (drate < prate) {
>>>>> +             mux_reg = readl(base + SRC_CPU);
>>>>> +             writel(mux_reg | (1 << 16), base + SRC_CPU);
>>>>> +             while (((readl(base + STAT_CPU) >> 16) & 0x7) != 2)
>>>>> +                     ;
>>>>                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [**]
>>>>
>>>> 2. I think, the above shall be done in a following way:
>>>>
>>>>          clk_set_parent(mout_core, mout_mpll);
>>>>          clk_set_rate(armclk->fout_pll, drate);
>>>>          clk_set_parent(mout_core, mout_apll);
>>>>
>>>> The direct write to registers [**] doesn't look compliant to CCF.
>>>>
>>>
>>> As mentioned above, the clock block encapsulates these clock blocks
>>> into a single clock and only this single encapsulated clock is
>>> registered with CCF. The internal implementation of how the different
>>> clock blocks are managed within this clock is independent of the CCF.
>>
>> I agree, that the CPU_DIV and CPU_DIV1 shall be changed atomically
>> (without CCF).
>>
>> But on the situation [**] the MUX can be changed by clk_set_parent() as
>> it is now done at exynosXXXX-cpufreq.c code.
>
> The mux is also encapsulated into a larger clock type and this new
> clock type know how the mux has to be configured.

IMHO it's fine to encapsulate the mux as well. There are no users of it 
other than the core clock.

>
>>
>>
>>>
>>>>
>>>> I'd rather thought about using "mout_core" instead of "arm_clk".
>>>> Then we would get access to the parent directly:
>>>>
>>>>          struct clk *parent = clk_get_parent(hw->clk);
>>>>
>>>> so we set the parents explicitly (at clk registration) and call
>>>> ->recalc_rate for clocks which are lower in the tree (like
>>>> "div_core", "div_core2").
>>>
>>> That was not the intention as mentioned above.
>>
>> This is just another possible solution to the problem.

Those clocks should not be dealt with separately and this was the 
intention of this composite clock. I agree with Thomas here.

Best regards,
Tomasz

Re: [PATCH 3/6] clk: samsung: register cpu clock provider for exynos4210 SoC

[Lukasz Majewski]

[-- Attachment #1.1: Type: text/plain, Size: 16779 bytes --]

Dear All,

> On 11.01.2014 06:25, Thomas Abraham wrote:
> > On Fri, Jan 10, 2014 at 7:48 PM, Lukasz Majewski
> > <l.majewski@samsung.com> wrote:
> >> Hi Thomas,
> >>
> >>> Hi Lukasz,
> >>>
> >>> On Fri, Jan 10, 2014 at 5:34 PM, Lukasz Majewski
> >>> <l.majewski@samsung.com> wrote:
> >>>> Hi Thomas,
> >>>>
> >>>>> Add a new clock provider for ARM clock domain. This clock
> >>>>> provider is composed of multiple components which include
> >>>>> mux_core, div_core, div_core2, div_corem0, div_corem1,
> >>>>> div_periph, div_atb, div_pclk_dbg, div_copy and div_hpm. This
> >>>>> composition of mutiple components into a single clock provider
> >>>>> helps with faster completion of CPU clock speed switching
> >>>>> during DVFS operations.
> >>>>>
> >>>>> Signed-off-by: Thomas Abraham <thomas.ab@samsung.com>
> >>>>> ---
> >>>>>   drivers/clk/samsung/clk-exynos4.c |   96
> >>>>> ++++++++++++++++++++++++++++++++++++- 1 files changed, 95
> >>>>> insertions(+), 1 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/clk/samsung/clk-exynos4.c
> >>>>> b/drivers/clk/samsung/clk-exynos4.c index d967571..4bf2f93
> >>>>> 100644 --- a/drivers/clk/samsung/clk-exynos4.c
> >>>>> +++ b/drivers/clk/samsung/clk-exynos4.c
> >>>>> @@ -108,8 +108,11 @@
> >>>>>   #define APLL_CON0            0x14100
> >>>>>   #define E4210_MPLL_CON0              0x14108
> >>>>>   #define SRC_CPU                      0x14200
> >>>>> +#define STAT_CPU             0x14400
> >>>>>   #define DIV_CPU0             0x14500
> >>>>>   #define DIV_CPU1             0x14504
> >>>>> +#define DIV_STAT_CPU0                0x14600
> >>>>> +#define DIV_STAT_CPU1                0x14604
> >>>>>   #define GATE_SCLK_CPU                0x14800
> >>>>>   #define GATE_IP_CPU          0x14900
> >>>>>   #define E4X12_DIV_ISP0               0x18300
> >>>>> @@ -289,7 +292,7 @@ static unsigned long exynos4_clk_regs[]
> >>>>> __initdata = { };
> >>>>>
> >>>>>   /* list of all parent clock list */
> >>>>> -PNAME(mout_apll_p)   = { "fin_pll", "fout_apll", };
> >>>>> +PNAME(mout_apll_p)   = { "fin_pll", "fout_apll1", };
> >>>>>   PNAME(mout_mpll_p)   = { "fin_pll", "fout_mpll", };
> >>>>>   PNAME(mout_epll_p)   = { "fin_pll", "fout_epll", };
> >>>>>   PNAME(mout_vpllsrc_p)        = { "fin_pll", "sclk_hdmi24m", };
> >>>>> @@ -306,6 +309,7 @@ PNAME(mout_onenand_p)  = {"aclk133",
> >>>>> "aclk160", }; PNAME(mout_onenand1_p) = {"mout_onenand",
> >>>>> "sclk_vpll", };
> >>>>>
> >>>>>   /* Exynos 4210-specific parent groups */
> >>>>> +PNAME(armclk_p) = { "fout_apll", };
> >>>>
> >>>> Here you only give no parent clock, but at
> >>>> samsung_coreclk_register() it is expected to provide list of
> >>>> parents.
> >>>
> >>> Here only one parent is listed, but the core clock type does not
> >>> limit the number of parents that can be specified. A specific
> >>> implementation can define and use multiple parents.
> >>
> >> I only pointed out that the definition of the:
> >>
> >> samsung_coreclk_register("armclk", armclk_p,
> >>                      ARRAY_SIZE(armclk_p), "fout_apll",
> >>                      &exynos4210_armclk_clk_ops, arm_clk,
> >>                      &exyno4210_armclk_table);
> >>
> >> Could only use parent, especially when you plan to change mux clock
> >> (apll vs. mpll) by writing directly to registers (which I think is
> >> bad).
> >
> > This definition is not limited to be used only on Exynos4210. This
> > is a generic core clock registration helper function intended to be
> > reusable across multiple Samsung SoCs.
> >
> 
> I think Lukasz meant that you should rather use parent list to pass
> any input clocks of the core clock block, instead of hardcoded clock
> look-up inside clock ops.

Yes, that was my point.

> 
> >>
> >>>
> >>>>
> >>>>>   PNAME(sclk_vpll_p4210)       = { "mout_vpllsrc",
> >>>>> "fout_vpll", }; PNAME(mout_core_p4210)       = { "mout_apll",
> >>>>> "sclk_mpll", }; PNAME(sclk_ampll_p4210)      = { "sclk_mpll",
> >>>>> "sclk_apll", }; @@ -1089,6 +1093,92 @@ static struct
> >>>>> samsung_pll_clock exynos4x12_plls[nr_plls] __initdata =
> >>>>> { VPLL_LOCK, VPLL_CON0, NULL), };
> >>>>>
> >>>>> +#define EXYNOS4210_DIV_CPU0(apll, pclk_dbg, atb, periph,
> >>>>> corem1, corem0) \
> >>>>> +             ((apll << 24) | (pclk_dbg << 20) | (atb << 16) | \
> >>>>> +             (periph << 12) | (corem1 << 8) | (corem0 << 4))
> >>>>> +#define EXYNOS4210_DIV_CPU1(hpm, copy)       \
> >>>>> +             ((hpm << 4) | (copy << 0))
> >>>>> +static const unsigned long exynos4210_armclk_data[][2] = {
> >>>>> +     { EXYNOS4210_DIV_CPU0(7, 1, 4, 3, 7, 3),
> >>>>> EXYNOS4210_DIV_CPU1(0, 5), },
> >>>>> +     { EXYNOS4210_DIV_CPU0(7, 1, 4, 3, 7, 3),
> >>>>> EXYNOS4210_DIV_CPU1(0, 4), },
> >>>>> +     { EXYNOS4210_DIV_CPU0(7, 1, 3, 3, 7, 3),
> >>>>> EXYNOS4210_DIV_CPU1(0, 3), },
> >>>>> +     { EXYNOS4210_DIV_CPU0(7, 1, 3, 3, 7, 3),
> >>>>> EXYNOS4210_DIV_CPU1(0, 3), },
> >>>>> +     { EXYNOS4210_DIV_CPU0(7, 1, 3, 3, 7, 3),
> >>>>> EXYNOS4210_DIV_CPU1(0, 3), },
> >>>>> +     { EXYNOS4210_DIV_CPU0(0, 1, 3, 1, 3, 1),
> >>>>> EXYNOS4210_DIV_CPU1(0, 3), }, +};
> >>>>> +
> >>>>
> >>>> What do you think about adding those parameters (like CPU
> >>>> dividers) as an attribute to /cpus/cpu@0 node?
> >>>
> >>> Not in CPU node but may be in clock controller node since these
> >>> values are actually used by the clock controller.
> >>
> >> /cpus/cpu@0 seems like a good place for them (since those DIVs are
> >> related to core)
> >
> > DIVs belong to the clock controller, not the CPU, and are addressed
> > from the clock controller address space.
> 
> I also think that the contents of cpu@0 node should be limited only
> to CPU specific data. Personally I don't even like the idea of having 
> operating points there - I would rather see frequency limits there,
> i.e. maximum frequency allowed at given voltage; specific frequency
> values could be inferred from available APLL/core clock
> configurations.
> 
> >
> >> .
> >> However, we can choose any better DT node to add it.
> >>
> >>> But since these values are
> >>> Exynos4210 specific and not generic enough to be reused across
> >>> multiple Exynos SoCs, there is little benefit in defining
> >>> bindings and parsing code for these values. It would be simpler
> >>> enough to just embed them in the code.
> >>
> >> It would be less to code, but isn't it the same ugly code, which we
> >> have now at exynos4xxx-cpufreq.c?
> >>
> >> With those values parsed from DT we can write generic code for the
> >> "arm_clk" clock. One clock implementation for cpufreq-cpu0.c (and
> >> maybe for arm_big_little.c) reused by Exynos4/5.
> >
> > As replied in the 2/3 patch, if these values would change across
> > multiple platforms based on the same SoC, it makes sense to put them
> > into DT. Any data that is purely SoC specific and not going to
> > change across platforms can be embedded into the code itself.
> 
> Well, they do change. If not on per board basis, then at least with
> SoC revisions.

I've added a real life example at my reply to patch 2/6

> 
> Anyway, the biggest problem is that the same data needs to be
> duplicated (well, triplicated) for each driver that needs them. If
> you can find a reasonable way to avoid redundancy, without having DT
> involved, I will probably be fine with it.
> 
> >>>
> >>> These are frequencies supported by the core clock. But the cpufreq
> >>> table can use all or subset of the supported frequencies.
> >>
> >> I see your point, but I find this distinction here a bit
> >> superfluous.
> >
> > Replied to similar comment in 2/3 patch.
> >
> >>
> >>> The core
> >>> clock should be usable with the clock api independently and not
> >>> tied to be used only by cpufreq driver.
> >>
> >> But then still for Exynos it will use PLL's M P S coefficients
> >> which only corresponds to values defined at cpufreq's frequency
> >> table.
> >
> > The PLL clocks are now separated out as PLL clock types in
> > samsung/clk-pll.c file. The P,M,S values of the PLLs are now handled
> > over there. So now the PLL is independent of the cpufreq driver and
> > can support any number of clock speeds not limited to the ones
> > needed by cpufreq.
> 
> Don't forget about opposite side of this relation. The APLL needs to 
> support at least those that are required by cpufreq.
> 
> >
> >>
> >> The set of frequencies for PLL, cpufreq and this clock is the
> >> same, so I think that we shall not define them in three different
> >> places.
> >>
> >> Could you give any example supporting your point of view?
> >
> > A PLL is a hardware component that can be reused in multiple SoCs. A
> > PLL can generate and support 'x' number of clock speeds but a SoC
> > using that PLL might use only 'y' (a subset of 'x')  number of clock
> > speeds of the PLL due to certain hardware limitations. Then there
> > are platforms using a SoC which might use only 'z' (a subset of
> > 'y') clock speeds due to the power/performance requirements of the
> > platform.
> >
> 
> As observed with our platforms, 'x' is usually SoC or SoC-revision 
> specific and equal to 'y' and 'z' on respective platforms.
> 
> >>
> >>>
> >>>>
> >>>>> +
> >>>>> +static const struct samsung_core_clock_freq_table
> >>>>> exyno4210_armclk_table = {
> >>>>> +     .freq           = exynos4210_armclk_freqs,
> >>>>> +     .freq_count     = ARRAY_SIZE(exynos4210_armclk_freqs),
> >>>>> +     .data           = (void *)exynos4210_armclk_data,
> >>>>> +};
> >>>>> +
> >>>>> +static int exynos4210_armclk_set_rate(struct clk_hw *hw,
> >>>>> unsigned long drate,
> >>>>> +                                     unsigned long prate)
> >>>>> +{
> >>>>> +     struct samsung_core_clock *armclk;
> >>>>> +     const struct samsung_core_clock_freq_table *freq_tbl;
> >>>>> +     unsigned long *freq_data;
> >>>>> +     unsigned long mux_reg, idx;
> >>>>> +     void __iomem *base;
> >>>>> +
> >>>>> +     if (drate == prate)
> >>>>> +             return 0;
> >>>>> +
> >>>>> +     armclk = container_of(hw, struct samsung_core_clock, hw);
> >>>>> +     freq_tbl = armclk->freq_table;
> >>>>> +     freq_data = (unsigned long *)freq_tbl->data;
> >>>>> +     base = armclk->ctrl_base;
> >>>>> +
> >>>>> +     for (idx = 0; idx < freq_tbl->freq_count; idx++, freq_data
> >>>>> += 2)
> >>>>> +             if ((freq_tbl->freq[idx] * 1000) == drate)
> >>>>> +                     break;
> >>>>> +
> >>>>> +     if (!armclk->fout_pll)
> >>>>> +             armclk->fout_pll = __clk_lookup("fout_apll");\
> >>>>                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^[*]
> >>>>
> >>>> I'm a bit confused here for two reasons. Please correct me if I'm
> >>>> wrong.
> >>>>
> >>>> 1. You go into this ->set_rate() because of calling clk_set_rate
> >>>> at "arm_clk" clock (numbered as 12 at clk-exynos4.c) at
> >>>> cpufreq-cpu0.c
> >>>>
> >>>> In a Exynos4210 we have:
> >>>> XXTI-> APLL -> fout_apll -> mout_apll -> mout_core -> div_core
> >>>> -> div_core2 -> arm_clk
> >>>>
> >>>> In the code you call directly the fout_apll which changes
> >>>> frequency. Then the change shall be propagated to all registered
> >>>> clocks.
> >>>> I think, that DIV and DIV1 shall be reduced before PLL change
> >>>> [*], to reflect the changes at CCF.
> >>>
> >>> The core clock implementation encapsulates multiple clock blocks
> >>> (such as dividers and muxes) which are in between the output of
> >>> the APLL and the point that actually is the cpu domain clock
> >>> output.
> >>
> >> No problem with that. I mostly agree...
> >>
> >>> When a clock
> >>> frequency change has to be made, all these clock blocks
> >>> encapsulated within the core clock are programmed by
> >>> pre-determined values.
> >>
> >> And what about the situation with already defined clocks (like
> >> "div_core" and "div_core2"). Those will not be updated when you
> >> first call clk_set_rate() and change by hand DIV and DIV1.
> >>
> >> What if you would like to have the PCLK_DBG clock used in the
> >> future? You would add it to CCF and the change will not propagate.
> >
> > I did intend to remove individual clock blocks which are now
> > encapsulated within the core clock type from the clock driver file.
> > I missed doing that in this patch series.
> 
> Yes, they should be removed, since they are encapsulated inside the
> core clock now.

This was not my point here. 

You are creating an opaque clock for cpu (like
"samsung_clock_cpu" [*]) , which encapsulates many other clocks
(mostly DIVs and MUXes to be more precise).

I don't think, that those clocks shall be removed from CCF. 

I do believe, that they shall be passed as the argument to [*] and
recalculated.

Why the advent of [*] clock forbids me from creating and using clocks
based on "div_core", "div_core2" and PCLK_DBG?

What if I would like to see output of those particular clocks
at /sys/kernel/debug/ or use them at a custom driver?

> 
> >
> >>
> >>> This
> >>> approach allows very fast clock speed switching, instead of
> >>> traversing the entire CCF clock tree searching for individual
> >>> clock blocks to be programmed.
> >>
> >> Those are mostly DIV and MUXes. Recalculation shouldn't be time
> >> consuming.
> >
> > I was mainly referring to the time taken to search the clock tree
> > for these individual clock blocks.
> 
> Hmm, why couldn't you simply look-up all the needed clock at 
> initialization and keep references to them?

+1

> 
> Still, I think it is fine to directly program registers of
> encapsulated clocks, 

And then recalculate values of relevant clocks build on top of those
registers.

> since they are not visible outside anymore and
> there is no need for them to be visible.

Why do you assume, that those clocks won't be needed anymore? For me
clocks like [*] are valid clocks from CCF/user point of view.

> 
> >
> >>
> >>>
> >>>>
> >>>>
> >>>>> +
> >>>>> +     if (drate < prate) {
> >>>>> +             mux_reg = readl(base + SRC_CPU);
> >>>>> +             writel(mux_reg | (1 << 16), base + SRC_CPU);
> >>>>> +             while (((readl(base + STAT_CPU) >> 16) & 0x7) !=
> >>>>> 2)
> >>>>> +                     ;
> >>>>                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [**]
> >>>>
> >>>> 2. I think, the above shall be done in a following way:
> >>>>
> >>>>          clk_set_parent(mout_core, mout_mpll);
> >>>>          clk_set_rate(armclk->fout_pll, drate);
> >>>>          clk_set_parent(mout_core, mout_apll);
> >>>>
> >>>> The direct write to registers [**] doesn't look compliant to CCF.
> >>>>
> >>>
> >>> As mentioned above, the clock block encapsulates these clock
> >>> blocks into a single clock and only this single encapsulated
> >>> clock is registered with CCF. The internal implementation of how
> >>> the different clock blocks are managed within this clock is
> >>> independent of the CCF.
> >>
> >> I agree, that the CPU_DIV and CPU_DIV1 shall be changed atomically
> >> (without CCF).
> >>
> >> But on the situation [**] the MUX can be changed by
> >> clk_set_parent() as it is now done at exynosXXXX-cpufreq.c code.
> >
> > The mux is also encapsulated into a larger clock type and this new
> > clock type know how the mux has to be configured.
> 
> IMHO it's fine to encapsulate the mux as well. There are no users of
> it other than the core clock.

In spite of the above comment, it looks far more better to change clocks
with CCF API (like clk_set_parent), not by writing directly to
registers.

> 
> >
> >>
> >>
> >>>
> >>>>
> >>>> I'd rather thought about using "mout_core" instead of "arm_clk".
> >>>> Then we would get access to the parent directly:
> >>>>
> >>>>          struct clk *parent = clk_get_parent(hw->clk);
> >>>>
> >>>> so we set the parents explicitly (at clk registration) and call
> >>>> ->recalc_rate for clocks which are lower in the tree (like
> >>>> "div_core", "div_core2").
> >>>
> >>> That was not the intention as mentioned above.
> >>
> >> This is just another possible solution to the problem.
> 
> Those clocks should not be dealt with separately and this was the 
> intention of this composite clock. I agree with Thomas here.

I cannot agree here, as stated at the above comment.

> 
> Best regards,
> Tomasz
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Best regards,
Lukasz Majewski

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/6] clk: samsung: register cpu clock provider for exynos4210 SoC

[Tomasz Figa]

On 12.01.2014 09:23, Lukasz Majewski wrote:
> Dear All,
>
>> On 11.01.2014 06:25, Thomas Abraham wrote:
>>> On Fri, Jan 10, 2014 at 7:48 PM, Lukasz Majewski
>>> <l.majewski@samsung.com> wrote:
>>>>> On Fri, Jan 10, 2014 at 5:34 PM, Lukasz Majewski
>>>>> <l.majewski@samsung.com> wrote:
[snip]
>>>>>>> +
>>>>>>> +static const struct samsung_core_clock_freq_table
>>>>>>> exyno4210_armclk_table = {
>>>>>>> +     .freq           = exynos4210_armclk_freqs,
>>>>>>> +     .freq_count     = ARRAY_SIZE(exynos4210_armclk_freqs),
>>>>>>> +     .data           = (void *)exynos4210_armclk_data,
>>>>>>> +};
>>>>>>> +
>>>>>>> +static int exynos4210_armclk_set_rate(struct clk_hw *hw,
>>>>>>> unsigned long drate,
>>>>>>> +                                     unsigned long prate)
>>>>>>> +{
>>>>>>> +     struct samsung_core_clock *armclk;
>>>>>>> +     const struct samsung_core_clock_freq_table *freq_tbl;
>>>>>>> +     unsigned long *freq_data;
>>>>>>> +     unsigned long mux_reg, idx;
>>>>>>> +     void __iomem *base;
>>>>>>> +
>>>>>>> +     if (drate == prate)
>>>>>>> +             return 0;
>>>>>>> +
>>>>>>> +     armclk = container_of(hw, struct samsung_core_clock, hw);
>>>>>>> +     freq_tbl = armclk->freq_table;
>>>>>>> +     freq_data = (unsigned long *)freq_tbl->data;
>>>>>>> +     base = armclk->ctrl_base;
>>>>>>> +
>>>>>>> +     for (idx = 0; idx < freq_tbl->freq_count; idx++, freq_data
>>>>>>> += 2)
>>>>>>> +             if ((freq_tbl->freq[idx] * 1000) == drate)
>>>>>>> +                     break;
>>>>>>> +
>>>>>>> +     if (!armclk->fout_pll)
>>>>>>> +             armclk->fout_pll = __clk_lookup("fout_apll");\
>>>>>>                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^[*]
>>>>>>
>>>>>> I'm a bit confused here for two reasons. Please correct me if I'm
>>>>>> wrong.
>>>>>>
>>>>>> 1. You go into this ->set_rate() because of calling clk_set_rate
>>>>>> at "arm_clk" clock (numbered as 12 at clk-exynos4.c) at
>>>>>> cpufreq-cpu0.c
>>>>>>
>>>>>> In a Exynos4210 we have:
>>>>>> XXTI-> APLL -> fout_apll -> mout_apll -> mout_core -> div_core
>>>>>> -> div_core2 -> arm_clk
>>>>>>
>>>>>> In the code you call directly the fout_apll which changes
>>>>>> frequency. Then the change shall be propagated to all registered
>>>>>> clocks.
>>>>>> I think, that DIV and DIV1 shall be reduced before PLL change
>>>>>> [*], to reflect the changes at CCF.
>>>>>
>>>>> The core clock implementation encapsulates multiple clock blocks
>>>>> (such as dividers and muxes) which are in between the output of
>>>>> the APLL and the point that actually is the cpu domain clock
>>>>> output.
>>>>
>>>> No problem with that. I mostly agree...
>>>>
>>>>> When a clock
>>>>> frequency change has to be made, all these clock blocks
>>>>> encapsulated within the core clock are programmed by
>>>>> pre-determined values.
>>>>
>>>> And what about the situation with already defined clocks (like
>>>> "div_core" and "div_core2"). Those will not be updated when you
>>>> first call clk_set_rate() and change by hand DIV and DIV1.
>>>>
>>>> What if you would like to have the PCLK_DBG clock used in the
>>>> future? You would add it to CCF and the change will not propagate.
>>>
>>> I did intend to remove individual clock blocks which are now
>>> encapsulated within the core clock type from the clock driver file.
>>> I missed doing that in this patch series.
>>
>> Yes, they should be removed, since they are encapsulated inside the
>> core clock now.
>
> This was not my point here.
>
> You are creating an opaque clock for cpu (like
> "samsung_clock_cpu" [*]) , which encapsulates many other clocks
> (mostly DIVs and MUXes to be more precise).
>
> I don't think, that those clocks shall be removed from CCF.
>
> I do believe, that they shall be passed as the argument to [*] and
> recalculated.
>
> Why the advent of [*] clock forbids me from creating and using clocks
> based on "div_core", "div_core2" and PCLK_DBG?
>
> What if I would like to see output of those particular clocks
> at /sys/kernel/debug/ or use them at a custom driver?

Having those clocks defined as separate entities in CCF would make them 
accessible outside the core clock, including changing their 
configuration, but they should be only changed on APLL change and 
atomically. By registering them as standard CCF clocks you are providing 
an interface to something that is not allowed.

If there was a _real_ need to have them defined as separate clocks, you 
would have to implement a separate clock type that would only allow 
retrieving the rate from it. Maybe a read-only flag for the divider 
clock could do, as mux clock already has such.

>
>>
>>>
>>>>
>>>>> This
>>>>> approach allows very fast clock speed switching, instead of
>>>>> traversing the entire CCF clock tree searching for individual
>>>>> clock blocks to be programmed.
>>>>
>>>> Those are mostly DIV and MUXes. Recalculation shouldn't be time
>>>> consuming.
>>>
>>> I was mainly referring to the time taken to search the clock tree
>>> for these individual clock blocks.
>>
>> Hmm, why couldn't you simply look-up all the needed clock at
>> initialization and keep references to them?
>
> +1
>
>>
>> Still, I think it is fine to directly program registers of
>> encapsulated clocks,
>
> And then recalculate values of relevant clocks build on top of those
> registers.
>
>> since they are not visible outside anymore and
>> there is no need for them to be visible.
>
> Why do you assume, that those clocks won't be needed anymore? For me
> clocks like [*] are valid clocks from CCF/user point of view.

I fail to see any use cases for them. Even now from 7 of core clock 
dividers of Exynos4210, in clk-exynos4.c only div_core and div_core2 are 
defined.

Then as I noted above, the values set to those internal blocks are 
completely tied to used APLL setting and you can't change them 
separately. Any potential use case would be limited to reading the 
frequency.

>
>>
>>>
>>>>
>>>>>
>>>>>>
>>>>>>
>>>>>>> +
>>>>>>> +     if (drate < prate) {
>>>>>>> +             mux_reg = readl(base + SRC_CPU);
>>>>>>> +             writel(mux_reg | (1 << 16), base + SRC_CPU);
>>>>>>> +             while (((readl(base + STAT_CPU) >> 16) & 0x7) !=
>>>>>>> 2)
>>>>>>> +                     ;
>>>>>>                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [**]
>>>>>>
>>>>>> 2. I think, the above shall be done in a following way:
>>>>>>
>>>>>>           clk_set_parent(mout_core, mout_mpll);
>>>>>>           clk_set_rate(armclk->fout_pll, drate);
>>>>>>           clk_set_parent(mout_core, mout_apll);
>>>>>>
>>>>>> The direct write to registers [**] doesn't look compliant to CCF.
>>>>>>
>>>>>
>>>>> As mentioned above, the clock block encapsulates these clock
>>>>> blocks into a single clock and only this single encapsulated
>>>>> clock is registered with CCF. The internal implementation of how
>>>>> the different clock blocks are managed within this clock is
>>>>> independent of the CCF.
>>>>
>>>> I agree, that the CPU_DIV and CPU_DIV1 shall be changed atomically
>>>> (without CCF).
>>>>
>>>> But on the situation [**] the MUX can be changed by
>>>> clk_set_parent() as it is now done at exynosXXXX-cpufreq.c code.
>>>
>>> The mux is also encapsulated into a larger clock type and this new
>>> clock type know how the mux has to be configured.
>>
>> IMHO it's fine to encapsulate the mux as well. There are no users of
>> it other than the core clock.
>
> In spite of the above comment, it looks far more better to change clocks
> with CCF API (like clk_set_parent), not by writing directly to
> registers.

This also depends on whether other entities should be able to 
reconfigure the mux in question. If it needs to be switched only when 
the APLL is reconfigured by core clock, then I don't think there is a 
need to provide access to it outside.

Best regards,
Tomasz

Re: [PATCH 3/6] clk: samsung: register cpu clock provider for exynos4210 SoC

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 8957 bytes --]

On Sun, 12 Jan 2014 13:05:47 +0100
Tomasz Figa <tomasz.figa@gmail.com> wrote:

> On 12.01.2014 09:23, Lukasz Majewski wrote:
> > Dear All,
> >
> >> On 11.01.2014 06:25, Thomas Abraham wrote:
> >>> On Fri, Jan 10, 2014 at 7:48 PM, Lukasz Majewski
> >>> <l.majewski@samsung.com> wrote:
> >>>>> On Fri, Jan 10, 2014 at 5:34 PM, Lukasz Majewski
> >>>>> <l.majewski@samsung.com> wrote:
> [snip]
> >>>>>>> +
> >>>>>>> +static const struct samsung_core_clock_freq_table
> >>>>>>> exyno4210_armclk_table = {
> >>>>>>> +     .freq           = exynos4210_armclk_freqs,
> >>>>>>> +     .freq_count     = ARRAY_SIZE(exynos4210_armclk_freqs),
> >>>>>>> +     .data           = (void *)exynos4210_armclk_data,
> >>>>>>> +};
> >>>>>>> +
> >>>>>>> +static int exynos4210_armclk_set_rate(struct clk_hw *hw,
> >>>>>>> unsigned long drate,
> >>>>>>> +                                     unsigned long prate)
> >>>>>>> +{
> >>>>>>> +     struct samsung_core_clock *armclk;
> >>>>>>> +     const struct samsung_core_clock_freq_table *freq_tbl;
> >>>>>>> +     unsigned long *freq_data;
> >>>>>>> +     unsigned long mux_reg, idx;
> >>>>>>> +     void __iomem *base;
> >>>>>>> +
> >>>>>>> +     if (drate == prate)
> >>>>>>> +             return 0;
> >>>>>>> +
> >>>>>>> +     armclk = container_of(hw, struct samsung_core_clock,
> >>>>>>> hw);
> >>>>>>> +     freq_tbl = armclk->freq_table;
> >>>>>>> +     freq_data = (unsigned long *)freq_tbl->data;
> >>>>>>> +     base = armclk->ctrl_base;
> >>>>>>> +
> >>>>>>> +     for (idx = 0; idx < freq_tbl->freq_count; idx++,
> >>>>>>> freq_data += 2)
> >>>>>>> +             if ((freq_tbl->freq[idx] * 1000) == drate)
> >>>>>>> +                     break;
> >>>>>>> +
> >>>>>>> +     if (!armclk->fout_pll)
> >>>>>>> +             armclk->fout_pll = __clk_lookup("fout_apll");\
> >>>>>>                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^[*]
> >>>>>>
> >>>>>> I'm a bit confused here for two reasons. Please correct me if
> >>>>>> I'm wrong.
> >>>>>>
> >>>>>> 1. You go into this ->set_rate() because of calling
> >>>>>> clk_set_rate at "arm_clk" clock (numbered as 12 at
> >>>>>> clk-exynos4.c) at cpufreq-cpu0.c
> >>>>>>
> >>>>>> In a Exynos4210 we have:
> >>>>>> XXTI-> APLL -> fout_apll -> mout_apll -> mout_core -> div_core
> >>>>>> -> div_core2 -> arm_clk
> >>>>>>
> >>>>>> In the code you call directly the fout_apll which changes
> >>>>>> frequency. Then the change shall be propagated to all
> >>>>>> registered clocks.
> >>>>>> I think, that DIV and DIV1 shall be reduced before PLL change
> >>>>>> [*], to reflect the changes at CCF.
> >>>>>
> >>>>> The core clock implementation encapsulates multiple clock blocks
> >>>>> (such as dividers and muxes) which are in between the output of
> >>>>> the APLL and the point that actually is the cpu domain clock
> >>>>> output.
> >>>>
> >>>> No problem with that. I mostly agree...
> >>>>
> >>>>> When a clock
> >>>>> frequency change has to be made, all these clock blocks
> >>>>> encapsulated within the core clock are programmed by
> >>>>> pre-determined values.
> >>>>
> >>>> And what about the situation with already defined clocks (like
> >>>> "div_core" and "div_core2"). Those will not be updated when you
> >>>> first call clk_set_rate() and change by hand DIV and DIV1.
> >>>>
> >>>> What if you would like to have the PCLK_DBG clock used in the
> >>>> future? You would add it to CCF and the change will not
> >>>> propagate.
> >>>
> >>> I did intend to remove individual clock blocks which are now
> >>> encapsulated within the core clock type from the clock driver
> >>> file. I missed doing that in this patch series.
> >>
> >> Yes, they should be removed, since they are encapsulated inside the
> >> core clock now.
> >
> > This was not my point here.
> >
> > You are creating an opaque clock for cpu (like
> > "samsung_clock_cpu" [*]) , which encapsulates many other clocks
> > (mostly DIVs and MUXes to be more precise).
> >
> > I don't think, that those clocks shall be removed from CCF.
> >
> > I do believe, that they shall be passed as the argument to [*] and
> > recalculated.
> >
> > Why the advent of [*] clock forbids me from creating and using
> > clocks based on "div_core", "div_core2" and PCLK_DBG?
> >
> > What if I would like to see output of those particular clocks
> > at /sys/kernel/debug/ or use them at a custom driver?
> 
> Having those clocks defined as separate entities in CCF would make
> them accessible outside the core clock, including changing their 
> configuration, but they should be only changed on APLL change and 
> atomically. By registering them as standard CCF clocks you are
> providing an interface to something that is not allowed.
> 
> If there was a _real_ need to have them defined as separate clocks,
> you would have to implement a separate clock type that would only
> allow retrieving the rate from it. Maybe a read-only flag for the
> divider clock could do, as mux clock already has such.

To not create unrealistic use cases, we can export those clocks as read
only (to allow them to be visible at /sys/kernel/debug).

Also we can design the code with future write extension in back of our
heads.

> 
> >
> >>
> >>>
> >>>>
> >>>>> This
> >>>>> approach allows very fast clock speed switching, instead of
> >>>>> traversing the entire CCF clock tree searching for individual
> >>>>> clock blocks to be programmed.
> >>>>
> >>>> Those are mostly DIV and MUXes. Recalculation shouldn't be time
> >>>> consuming.
> >>>
> >>> I was mainly referring to the time taken to search the clock tree
> >>> for these individual clock blocks.
> >>
> >> Hmm, why couldn't you simply look-up all the needed clock at
> >> initialization and keep references to them?
> >
> > +1
> >
> >>
> >> Still, I think it is fine to directly program registers of
> >> encapsulated clocks,
> >
> > And then recalculate values of relevant clocks build on top of those
> > registers.
> >
> >> since they are not visible outside anymore and
> >> there is no need for them to be visible.
> >
> > Why do you assume, that those clocks won't be needed anymore? For me
> > clocks like [*] are valid clocks from CCF/user point of view.
> 
> I fail to see any use cases for them. Even now from 7 of core clock 
> dividers of Exynos4210, in clk-exynos4.c only div_core and div_core2
> are defined.
> 
> Then as I noted above, the values set to those internal blocks are 
> completely tied to used APLL setting and you can't change them 
> separately. Any potential use case would be limited to reading the 
> frequency.
> 
> >
> >>
> >>>
> >>>>
> >>>>>
> >>>>>>
> >>>>>>
> >>>>>>> +
> >>>>>>> +     if (drate < prate) {
> >>>>>>> +             mux_reg = readl(base + SRC_CPU);
> >>>>>>> +             writel(mux_reg | (1 << 16), base + SRC_CPU);
> >>>>>>> +             while (((readl(base + STAT_CPU) >> 16) & 0x7) !=
> >>>>>>> 2)
> >>>>>>> +                     ;
> >>>>>>                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [**]
> >>>>>>
> >>>>>> 2. I think, the above shall be done in a following way:
> >>>>>>
> >>>>>>           clk_set_parent(mout_core, mout_mpll);
> >>>>>>           clk_set_rate(armclk->fout_pll, drate);
> >>>>>>           clk_set_parent(mout_core, mout_apll);
> >>>>>>
> >>>>>> The direct write to registers [**] doesn't look compliant to
> >>>>>> CCF.
> >>>>>>
> >>>>>
> >>>>> As mentioned above, the clock block encapsulates these clock
> >>>>> blocks into a single clock and only this single encapsulated
> >>>>> clock is registered with CCF. The internal implementation of how
> >>>>> the different clock blocks are managed within this clock is
> >>>>> independent of the CCF.
> >>>>
> >>>> I agree, that the CPU_DIV and CPU_DIV1 shall be changed
> >>>> atomically (without CCF).
> >>>>
> >>>> But on the situation [**] the MUX can be changed by
> >>>> clk_set_parent() as it is now done at exynosXXXX-cpufreq.c code.
> >>>
> >>> The mux is also encapsulated into a larger clock type and this new
> >>> clock type know how the mux has to be configured.
> >>
> >> IMHO it's fine to encapsulate the mux as well. There are no users
> >> of it other than the core clock.
> >
> > In spite of the above comment, it looks far more better to change
> > clocks with CCF API (like clk_set_parent), not by writing directly
> > to registers.
> 
> This also depends on whether other entities should be able to 
> reconfigure the mux in question. If it needs to be switched only when 
> the APLL is reconfigured by core clock, then I don't think there is a 
> need to provide access to it outside.

The mout_apll and mout_mpll are important clocks for the platform. For
this reason I'd prefer them to be exported and up to date.

> 
> Best regards,
> Tomasz

Best regards,
Lukasz Majewski

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 3/6] clk: samsung: register cpu clock provider for exynos4210 SoC

[Tomasz Figa]

On 12.01.2014 13:41, Lukasz Majewski wrote:
> On Sun, 12 Jan 2014 13:05:47 +0100
> Tomasz Figa <tomasz.figa@gmail.com> wrote:
>
>> On 12.01.2014 09:23, Lukasz Majewski wrote:
>>> Dear All,
>>>
>>>> On 11.01.2014 06:25, Thomas Abraham wrote:
>>>>> On Fri, Jan 10, 2014 at 7:48 PM, Lukasz Majewski
>>>>> <l.majewski@samsung.com> wrote:
>>>>>>> On Fri, Jan 10, 2014 at 5:34 PM, Lukasz Majewski
>>>>>>> <l.majewski@samsung.com> wrote:
>> [snip]
>>>>>>>>> +
>>>>>>>>> +static const struct samsung_core_clock_freq_table
>>>>>>>>> exyno4210_armclk_table = {
>>>>>>>>> +     .freq           = exynos4210_armclk_freqs,
>>>>>>>>> +     .freq_count     = ARRAY_SIZE(exynos4210_armclk_freqs),
>>>>>>>>> +     .data           = (void *)exynos4210_armclk_data,
>>>>>>>>> +};
>>>>>>>>> +
>>>>>>>>> +static int exynos4210_armclk_set_rate(struct clk_hw *hw,
>>>>>>>>> unsigned long drate,
>>>>>>>>> +                                     unsigned long prate)
>>>>>>>>> +{
>>>>>>>>> +     struct samsung_core_clock *armclk;
>>>>>>>>> +     const struct samsung_core_clock_freq_table *freq_tbl;
>>>>>>>>> +     unsigned long *freq_data;
>>>>>>>>> +     unsigned long mux_reg, idx;
>>>>>>>>> +     void __iomem *base;
>>>>>>>>> +
>>>>>>>>> +     if (drate == prate)
>>>>>>>>> +             return 0;
>>>>>>>>> +
>>>>>>>>> +     armclk = container_of(hw, struct samsung_core_clock,
>>>>>>>>> hw);
>>>>>>>>> +     freq_tbl = armclk->freq_table;
>>>>>>>>> +     freq_data = (unsigned long *)freq_tbl->data;
>>>>>>>>> +     base = armclk->ctrl_base;
>>>>>>>>> +
>>>>>>>>> +     for (idx = 0; idx < freq_tbl->freq_count; idx++,
>>>>>>>>> freq_data += 2)
>>>>>>>>> +             if ((freq_tbl->freq[idx] * 1000) == drate)
>>>>>>>>> +                     break;
>>>>>>>>> +
>>>>>>>>> +     if (!armclk->fout_pll)
>>>>>>>>> +             armclk->fout_pll = __clk_lookup("fout_apll");\
>>>>>>>>                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^[*]
>>>>>>>>
>>>>>>>> I'm a bit confused here for two reasons. Please correct me if
>>>>>>>> I'm wrong.
>>>>>>>>
>>>>>>>> 1. You go into this ->set_rate() because of calling
>>>>>>>> clk_set_rate at "arm_clk" clock (numbered as 12 at
>>>>>>>> clk-exynos4.c) at cpufreq-cpu0.c
>>>>>>>>
>>>>>>>> In a Exynos4210 we have:
>>>>>>>> XXTI-> APLL -> fout_apll -> mout_apll -> mout_core -> div_core
>>>>>>>> -> div_core2 -> arm_clk
>>>>>>>>
>>>>>>>> In the code you call directly the fout_apll which changes
>>>>>>>> frequency. Then the change shall be propagated to all
>>>>>>>> registered clocks.
>>>>>>>> I think, that DIV and DIV1 shall be reduced before PLL change
>>>>>>>> [*], to reflect the changes at CCF.
>>>>>>>
>>>>>>> The core clock implementation encapsulates multiple clock blocks
>>>>>>> (such as dividers and muxes) which are in between the output of
>>>>>>> the APLL and the point that actually is the cpu domain clock
>>>>>>> output.
>>>>>>
>>>>>> No problem with that. I mostly agree...
>>>>>>
>>>>>>> When a clock
>>>>>>> frequency change has to be made, all these clock blocks
>>>>>>> encapsulated within the core clock are programmed by
>>>>>>> pre-determined values.
>>>>>>
>>>>>> And what about the situation with already defined clocks (like
>>>>>> "div_core" and "div_core2"). Those will not be updated when you
>>>>>> first call clk_set_rate() and change by hand DIV and DIV1.
>>>>>>
>>>>>> What if you would like to have the PCLK_DBG clock used in the
>>>>>> future? You would add it to CCF and the change will not
>>>>>> propagate.
>>>>>
>>>>> I did intend to remove individual clock blocks which are now
>>>>> encapsulated within the core clock type from the clock driver
>>>>> file. I missed doing that in this patch series.
>>>>
>>>> Yes, they should be removed, since they are encapsulated inside the
>>>> core clock now.
>>>
>>> This was not my point here.
>>>
>>> You are creating an opaque clock for cpu (like
>>> "samsung_clock_cpu" [*]) , which encapsulates many other clocks
>>> (mostly DIVs and MUXes to be more precise).
>>>
>>> I don't think, that those clocks shall be removed from CCF.
>>>
>>> I do believe, that they shall be passed as the argument to [*] and
>>> recalculated.
>>>
>>> Why the advent of [*] clock forbids me from creating and using
>>> clocks based on "div_core", "div_core2" and PCLK_DBG?
>>>
>>> What if I would like to see output of those particular clocks
>>> at /sys/kernel/debug/ or use them at a custom driver?
>>
>> Having those clocks defined as separate entities in CCF would make
>> them accessible outside the core clock, including changing their
>> configuration, but they should be only changed on APLL change and
>> atomically. By registering them as standard CCF clocks you are
>> providing an interface to something that is not allowed.
>>
>> If there was a _real_ need to have them defined as separate clocks,
>> you would have to implement a separate clock type that would only
>> allow retrieving the rate from it. Maybe a read-only flag for the
>> divider clock could do, as mux clock already has such.
>
> To not create unrealistic use cases, we can export those clocks as read
> only (to allow them to be visible at /sys/kernel/debug).

What functionality would that provide?

>
> Also we can design the code with future write extension in back of our
> heads.

As I already explained above, you _must_ _not_ reconfigure the clocks 
beyond div_core2 separately, because they are supposed to be set atomically.

In case of mout_core, div_core and div_core2 they probably can be 
reconfigured in a non-atomic fashion, but this is still a bad idea, 
because this invalidates the settings done to those internal dividers. 
Remember that whenever you change the input frequency of core block you 
need to reconfigure the internal dividers properly.

Of course there is a number of complex solutions involving adding some 
synchronization code, clock notifications and so on to keep those clocks 
at CCF and still meet the requirements for core block clocks (not even 
saying that you could even extend CCF to provide support for changing 
clocks atomically), but would this complexity be justified without any 
_real_ need?

>>>>>>>>
[snip]
>>>>>>>>
>>>>>>>>> +
>>>>>>>>> +     if (drate < prate) {
>>>>>>>>> +             mux_reg = readl(base + SRC_CPU);
>>>>>>>>> +             writel(mux_reg | (1 << 16), base + SRC_CPU);
>>>>>>>>> +             while (((readl(base + STAT_CPU) >> 16) & 0x7) !=
>>>>>>>>> 2)
>>>>>>>>> +                     ;
>>>>>>>>                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [**]
>>>>>>>>
>>>>>>>> 2. I think, the above shall be done in a following way:
>>>>>>>>
>>>>>>>>            clk_set_parent(mout_core, mout_mpll);
>>>>>>>>            clk_set_rate(armclk->fout_pll, drate);
>>>>>>>>            clk_set_parent(mout_core, mout_apll);
>>>>>>>>
>>>>>>>> The direct write to registers [**] doesn't look compliant to
>>>>>>>> CCF.
>>>>>>>>
>>>>>>>
>>>>>>> As mentioned above, the clock block encapsulates these clock
>>>>>>> blocks into a single clock and only this single encapsulated
>>>>>>> clock is registered with CCF. The internal implementation of how
>>>>>>> the different clock blocks are managed within this clock is
>>>>>>> independent of the CCF.
>>>>>>
>>>>>> I agree, that the CPU_DIV and CPU_DIV1 shall be changed
>>>>>> atomically (without CCF).
>>>>>>
>>>>>> But on the situation [**] the MUX can be changed by
>>>>>> clk_set_parent() as it is now done at exynosXXXX-cpufreq.c code.
>>>>>
>>>>> The mux is also encapsulated into a larger clock type and this new
>>>>> clock type know how the mux has to be configured.
>>>>
>>>> IMHO it's fine to encapsulate the mux as well. There are no users
>>>> of it other than the core clock.
>>>
>>> In spite of the above comment, it looks far more better to change
>>> clocks with CCF API (like clk_set_parent), not by writing directly
>>> to registers.
>>
>> This also depends on whether other entities should be able to
>> reconfigure the mux in question. If it needs to be switched only when
>> the APLL is reconfigured by core clock, then I don't think there is a
>> need to provide access to it outside.
>
> The mout_apll and mout_mpll are important clocks for the platform. For
> this reason I'd prefer them to be exported and up to date.

I haven't said anything about mout_apll and mout_mpll. They have 
multiple users other than the core block and they should be left intact. 
We are talking here about mout_core which is only used to select the 
input of core block.

Best regards,
Tomasz

Re: [PATCH 3/6] clk: samsung: register cpu clock provider for exynos4210 SoC

[Thomas Abraham]

On Sun, Jan 12, 2014 at 6:28 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
>
>
> On 12.01.2014 13:41, Lukasz Majewski wrote:
>>
>> On Sun, 12 Jan 2014 13:05:47 +0100
>> Tomasz Figa <tomasz.figa@gmail.com> wrote:
>>
>>> On 12.01.2014 09:23, Lukasz Majewski wrote:
>>>>
>>>> Dear All,
>>>>
>>>>> On 11.01.2014 06:25, Thomas Abraham wrote:
>>>>>>
>>>>>> On Fri, Jan 10, 2014 at 7:48 PM, Lukasz Majewski
>>>>>> <l.majewski@samsung.com> wrote:
>>>>>>>>
>>>>>>>> On Fri, Jan 10, 2014 at 5:34 PM, Lukasz Majewski
>>>>>>>> <l.majewski@samsung.com> wrote:
>>>
>>> [snip]
>>>>>>>>>>
>>>>>>>>>> +
>>>>>>>>>> +static const struct samsung_core_clock_freq_table
>>>>>>>>>> exyno4210_armclk_table = {
>>>>>>>>>> +     .freq           = exynos4210_armclk_freqs,
>>>>>>>>>> +     .freq_count     = ARRAY_SIZE(exynos4210_armclk_freqs),
>>>>>>>>>> +     .data           = (void *)exynos4210_armclk_data,
>>>>>>>>>> +};
>>>>>>>>>> +
>>>>>>>>>> +static int exynos4210_armclk_set_rate(struct clk_hw *hw,
>>>>>>>>>> unsigned long drate,
>>>>>>>>>> +                                     unsigned long prate)
>>>>>>>>>> +{
>>>>>>>>>> +     struct samsung_core_clock *armclk;
>>>>>>>>>> +     const struct samsung_core_clock_freq_table *freq_tbl;
>>>>>>>>>> +     unsigned long *freq_data;
>>>>>>>>>> +     unsigned long mux_reg, idx;
>>>>>>>>>> +     void __iomem *base;
>>>>>>>>>> +
>>>>>>>>>> +     if (drate == prate)
>>>>>>>>>> +             return 0;
>>>>>>>>>> +
>>>>>>>>>> +     armclk = container_of(hw, struct samsung_core_clock,
>>>>>>>>>> hw);
>>>>>>>>>> +     freq_tbl = armclk->freq_table;
>>>>>>>>>> +     freq_data = (unsigned long *)freq_tbl->data;
>>>>>>>>>> +     base = armclk->ctrl_base;
>>>>>>>>>> +
>>>>>>>>>> +     for (idx = 0; idx < freq_tbl->freq_count; idx++,
>>>>>>>>>> freq_data += 2)
>>>>>>>>>> +             if ((freq_tbl->freq[idx] * 1000) == drate)
>>>>>>>>>> +                     break;
>>>>>>>>>> +
>>>>>>>>>> +     if (!armclk->fout_pll)
>>>>>>>>>> +             armclk->fout_pll = __clk_lookup("fout_apll");\
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^[*]
>>>>>>>>>
>>>>>>>>> I'm a bit confused here for two reasons. Please correct me if
>>>>>>>>> I'm wrong.
>>>>>>>>>
>>>>>>>>> 1. You go into this ->set_rate() because of calling
>>>>>>>>> clk_set_rate at "arm_clk" clock (numbered as 12 at
>>>>>>>>> clk-exynos4.c) at cpufreq-cpu0.c
>>>>>>>>>
>>>>>>>>> In a Exynos4210 we have:
>>>>>>>>> XXTI-> APLL -> fout_apll -> mout_apll -> mout_core -> div_core
>>>>>>>>> -> div_core2 -> arm_clk
>>>>>>>>>
>>>>>>>>> In the code you call directly the fout_apll which changes
>>>>>>>>> frequency. Then the change shall be propagated to all
>>>>>>>>> registered clocks.
>>>>>>>>> I think, that DIV and DIV1 shall be reduced before PLL change
>>>>>>>>> [*], to reflect the changes at CCF.
>>>>>>>>
>>>>>>>>
>>>>>>>> The core clock implementation encapsulates multiple clock blocks
>>>>>>>> (such as dividers and muxes) which are in between the output of
>>>>>>>> the APLL and the point that actually is the cpu domain clock
>>>>>>>> output.
>>>>>>>
>>>>>>>
>>>>>>> No problem with that. I mostly agree...
>>>>>>>
>>>>>>>> When a clock
>>>>>>>> frequency change has to be made, all these clock blocks
>>>>>>>> encapsulated within the core clock are programmed by
>>>>>>>> pre-determined values.
>>>>>>>
>>>>>>>
>>>>>>> And what about the situation with already defined clocks (like
>>>>>>> "div_core" and "div_core2"). Those will not be updated when you
>>>>>>> first call clk_set_rate() and change by hand DIV and DIV1.
>>>>>>>
>>>>>>> What if you would like to have the PCLK_DBG clock used in the
>>>>>>> future? You would add it to CCF and the change will not
>>>>>>> propagate.
>>>>>>
>>>>>>
>>>>>> I did intend to remove individual clock blocks which are now
>>>>>> encapsulated within the core clock type from the clock driver
>>>>>> file. I missed doing that in this patch series.
>>>>>
>>>>>
>>>>> Yes, they should be removed, since they are encapsulated inside the
>>>>> core clock now.
>>>>
>>>>
>>>> This was not my point here.
>>>>
>>>> You are creating an opaque clock for cpu (like
>>>> "samsung_clock_cpu" [*]) , which encapsulates many other clocks
>>>> (mostly DIVs and MUXes to be more precise).
>>>>
>>>> I don't think, that those clocks shall be removed from CCF.
>>>>
>>>> I do believe, that they shall be passed as the argument to [*] and
>>>> recalculated.
>>>>
>>>> Why the advent of [*] clock forbids me from creating and using
>>>> clocks based on "div_core", "div_core2" and PCLK_DBG?
>>>>
>>>> What if I would like to see output of those particular clocks
>>>> at /sys/kernel/debug/ or use them at a custom driver?
>>>
>>>
>>> Having those clocks defined as separate entities in CCF would make
>>> them accessible outside the core clock, including changing their
>>> configuration, but they should be only changed on APLL change and
>>> atomically. By registering them as standard CCF clocks you are
>>> providing an interface to something that is not allowed.
>>>
>>> If there was a _real_ need to have them defined as separate clocks,
>>> you would have to implement a separate clock type that would only
>>> allow retrieving the rate from it. Maybe a read-only flag for the
>>> divider clock could do, as mux clock already has such.
>>
>>
>> To not create unrealistic use cases, we can export those clocks as read
>> only (to allow them to be visible at /sys/kernel/debug).
>
>
> What functionality would that provide?
>
>
>>
>> Also we can design the code with future write extension in back of our
>> heads.
>
>
> As I already explained above, you _must_ _not_ reconfigure the clocks beyond
> div_core2 separately, because they are supposed to be set atomically.
>
> In case of mout_core, div_core and div_core2 they probably can be
> reconfigured in a non-atomic fashion, but this is still a bad idea, because
> this invalidates the settings done to those internal dividers. Remember that
> whenever you change the input frequency of core block you need to
> reconfigure the internal dividers properly.
>
> Of course there is a number of complex solutions involving adding some
> synchronization code, clock notifications and so on to keep those clocks at
> CCF and still meet the requirements for core block clocks (not even saying
> that you could even extend CCF to provide support for changing clocks
> atomically), but would this complexity be justified without any _real_ need?
>
>>>>>>>>>
> [snip]
>
>>>>>>>>>
>>>>>>>>>> +
>>>>>>>>>> +     if (drate < prate) {
>>>>>>>>>> +             mux_reg = readl(base + SRC_CPU);
>>>>>>>>>> +             writel(mux_reg | (1 << 16), base + SRC_CPU);
>>>>>>>>>> +             while (((readl(base + STAT_CPU) >> 16) & 0x7) !=
>>>>>>>>>> 2)
>>>>>>>>>> +                     ;
>>>>>>>>>
>>>>>>>>>                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [**]
>>>>>>>>>
>>>>>>>>> 2. I think, the above shall be done in a following way:
>>>>>>>>>
>>>>>>>>>            clk_set_parent(mout_core, mout_mpll);
>>>>>>>>>            clk_set_rate(armclk->fout_pll, drate);
>>>>>>>>>            clk_set_parent(mout_core, mout_apll);
>>>>>>>>>
>>>>>>>>> The direct write to registers [**] doesn't look compliant to
>>>>>>>>> CCF.
>>>>>>>>>
>>>>>>>>
>>>>>>>> As mentioned above, the clock block encapsulates these clock
>>>>>>>> blocks into a single clock and only this single encapsulated
>>>>>>>> clock is registered with CCF. The internal implementation of how
>>>>>>>> the different clock blocks are managed within this clock is
>>>>>>>> independent of the CCF.
>>>>>>>
>>>>>>>
>>>>>>> I agree, that the CPU_DIV and CPU_DIV1 shall be changed
>>>>>>> atomically (without CCF).
>>>>>>>
>>>>>>> But on the situation [**] the MUX can be changed by
>>>>>>> clk_set_parent() as it is now done at exynosXXXX-cpufreq.c code.
>>>>>>
>>>>>>
>>>>>> The mux is also encapsulated into a larger clock type and this new
>>>>>> clock type know how the mux has to be configured.
>>>>>
>>>>>
>>>>> IMHO it's fine to encapsulate the mux as well. There are no users
>>>>> of it other than the core clock.
>>>>
>>>>
>>>> In spite of the above comment, it looks far more better to change
>>>> clocks with CCF API (like clk_set_parent), not by writing directly
>>>> to registers.
>>>
>>>
>>> This also depends on whether other entities should be able to
>>> reconfigure the mux in question. If it needs to be switched only when
>>> the APLL is reconfigured by core clock, then I don't think there is a
>>> need to provide access to it outside.
>>
>>
>> The mout_apll and mout_mpll are important clocks for the platform. For
>> this reason I'd prefer them to be exported and up to date.
>
>
> I haven't said anything about mout_apll and mout_mpll. They have multiple
> users other than the core block and they should be left intact. We are
> talking here about mout_core which is only used to select the input of core
> block.

Hi Lukasz, Tomasz,

Thanks for your comments. As per this discussion, we remove the clocks
encapsulated within the core clock. If there are any more points to be
considered, please let me know.

Thanks,
Thomas.

>
> Best regards,
> Tomasz

Re: [PATCH 3/6] clk: samsung: register cpu clock provider for exynos4210 SoC

[Thomas Abraham]

On Sun, Jan 12, 2014 at 7:49 AM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> On 11.01.2014 06:25, Thomas Abraham wrote:
>>
>> On Fri, Jan 10, 2014 at 7:48 PM, Lukasz Majewski <l.majewski@samsung.com>
>> wrote:
>>>
>>> Hi Thomas,
>>>
>>>> Hi Lukasz,
>>>>
>>>> On Fri, Jan 10, 2014 at 5:34 PM, Lukasz Majewski
>>>> <l.majewski@samsung.com> wrote:
>>>>>
>>>>> Hi Thomas,
>>>>>
>>>>>> Add a new clock provider for ARM clock domain. This clock provider
>>>>>> is composed of multiple components which include mux_core,
>>>>>> div_core, div_core2, div_corem0, div_corem1, div_periph, div_atb,
>>>>>> div_pclk_dbg, div_copy and div_hpm. This composition of mutiple
>>>>>> components into a single clock provider helps with faster
>>>>>> completion of CPU clock speed switching during DVFS operations.
>>>>>>
>>>>>> Signed-off-by: Thomas Abraham <thomas.ab@samsung.com>
>>>>>> ---
>>>>>>   drivers/clk/samsung/clk-exynos4.c |   96
>>>>>> ++++++++++++++++++++++++++++++++++++- 1 files changed, 95
>>>>>> insertions(+), 1 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/clk/samsung/clk-exynos4.c
>>>>>> b/drivers/clk/samsung/clk-exynos4.c index d967571..4bf2f93 100644
>>>>>> --- a/drivers/clk/samsung/clk-exynos4.c
>>>>>> +++ b/drivers/clk/samsung/clk-exynos4.c
>>>>>> @@ -108,8 +108,11 @@
>>>>>>   #define APLL_CON0            0x14100
>>>>>>   #define E4210_MPLL_CON0              0x14108
>>>>>>   #define SRC_CPU                      0x14200
>>>>>> +#define STAT_CPU             0x14400
>>>>>>   #define DIV_CPU0             0x14500
>>>>>>   #define DIV_CPU1             0x14504
>>>>>> +#define DIV_STAT_CPU0                0x14600
>>>>>> +#define DIV_STAT_CPU1                0x14604
>>>>>>   #define GATE_SCLK_CPU                0x14800
>>>>>>   #define GATE_IP_CPU          0x14900
>>>>>>   #define E4X12_DIV_ISP0               0x18300
>>>>>> @@ -289,7 +292,7 @@ static unsigned long exynos4_clk_regs[]
>>>>>> __initdata = { };
>>>>>>
>>>>>>   /* list of all parent clock list */
>>>>>> -PNAME(mout_apll_p)   = { "fin_pll", "fout_apll", };
>>>>>> +PNAME(mout_apll_p)   = { "fin_pll", "fout_apll1", };
>>>>>>   PNAME(mout_mpll_p)   = { "fin_pll", "fout_mpll", };
>>>>>>   PNAME(mout_epll_p)   = { "fin_pll", "fout_epll", };
>>>>>>   PNAME(mout_vpllsrc_p)        = { "fin_pll", "sclk_hdmi24m", };
>>>>>> @@ -306,6 +309,7 @@ PNAME(mout_onenand_p)  = {"aclk133",
>>>>>> "aclk160", }; PNAME(mout_onenand1_p) = {"mout_onenand",
>>>>>> "sclk_vpll", };
>>>>>>
>>>>>>   /* Exynos 4210-specific parent groups */
>>>>>> +PNAME(armclk_p) = { "fout_apll", };
>>>>>
>>>>>
>>>>> Here you only give no parent clock, but at
>>>>> samsung_coreclk_register() it is expected to provide list of
>>>>> parents.
>>>>
>>>>
>>>> Here only one parent is listed, but the core clock type does not limit
>>>> the number of parents that can be specified. A specific implementation
>>>> can define and use multiple parents.
>>>
>>>
>>> I only pointed out that the definition of the:
>>>
>>> samsung_coreclk_register("armclk", armclk_p,
>>>                      ARRAY_SIZE(armclk_p), "fout_apll",
>>>                      &exynos4210_armclk_clk_ops, arm_clk,
>>>                      &exyno4210_armclk_table);
>>>
>>> Could only use parent, especially when you plan to change mux clock
>>> (apll vs. mpll) by writing directly to registers (which I think is bad).
>>
>>
>> This definition is not limited to be used only on Exynos4210. This is
>> a generic core clock registration helper function intended to be
>> reusable across multiple Samsung SoCs.
>>
>
> I think Lukasz meant that you should rather use parent list to pass any
> input clocks of the core clock block, instead of hardcoded clock look-up
> inside clock ops.

Ok.

>
>
>>>
>>>>
>>>>>
>>>>>>   PNAME(sclk_vpll_p4210)       = { "mout_vpllsrc", "fout_vpll", };
>>>>>>   PNAME(mout_core_p4210)       = { "mout_apll", "sclk_mpll", };
>>>>>>   PNAME(sclk_ampll_p4210)      = { "sclk_mpll", "sclk_apll", };
>>>>>> @@ -1089,6 +1093,92 @@ static struct samsung_pll_clock
>>>>>> exynos4x12_plls[nr_plls] __initdata = { VPLL_LOCK, VPLL_CON0,
>>>>>> NULL), };
>>>>>>
>>>>>> +#define EXYNOS4210_DIV_CPU0(apll, pclk_dbg, atb, periph, corem1,
>>>>>> corem0) \
>>>>>> +             ((apll << 24) | (pclk_dbg << 20) | (atb << 16) | \
>>>>>> +             (periph << 12) | (corem1 << 8) | (corem0 << 4))
>>>>>> +#define EXYNOS4210_DIV_CPU1(hpm, copy)       \
>>>>>> +             ((hpm << 4) | (copy << 0))
>>>>>> +static const unsigned long exynos4210_armclk_data[][2] = {
>>>>>> +     { EXYNOS4210_DIV_CPU0(7, 1, 4, 3, 7, 3),
>>>>>> EXYNOS4210_DIV_CPU1(0, 5), },
>>>>>> +     { EXYNOS4210_DIV_CPU0(7, 1, 4, 3, 7, 3),
>>>>>> EXYNOS4210_DIV_CPU1(0, 4), },
>>>>>> +     { EXYNOS4210_DIV_CPU0(7, 1, 3, 3, 7, 3),
>>>>>> EXYNOS4210_DIV_CPU1(0, 3), },
>>>>>> +     { EXYNOS4210_DIV_CPU0(7, 1, 3, 3, 7, 3),
>>>>>> EXYNOS4210_DIV_CPU1(0, 3), },
>>>>>> +     { EXYNOS4210_DIV_CPU0(7, 1, 3, 3, 7, 3),
>>>>>> EXYNOS4210_DIV_CPU1(0, 3), },
>>>>>> +     { EXYNOS4210_DIV_CPU0(0, 1, 3, 1, 3, 1),
>>>>>> EXYNOS4210_DIV_CPU1(0, 3), }, +};
>>>>>> +
>>>>>
>>>>>
>>>>> What do you think about adding those parameters (like CPU dividers)
>>>>> as an attribute to /cpus/cpu@0 node?
>>>>
>>>>
>>>> Not in CPU node but may be in clock controller node since these values
>>>> are actually used by the clock controller.
>>>
>>>
>>> /cpus/cpu@0 seems like a good place for them (since those DIVs are
>>> related to core)
>>
>>
>> DIVs belong to the clock controller, not the CPU, and are addressed
>> from the clock controller address space.
>
>
> I also think that the contents of cpu@0 node should be limited only to CPU
> specific data. Personally I don't even like the idea of having operating
> points there - I would rather see frequency limits there, i.e. maximum
> frequency allowed at given voltage; specific frequency values could be
> inferred from available APLL/core clock configurations.
>
>
>>
>>> .
>>> However, we can choose any better DT node to add it.
>>>
>>>> But since these values are
>>>> Exynos4210 specific and not generic enough to be reused across
>>>> multiple Exynos SoCs, there is little benefit in defining bindings and
>>>> parsing code for these values. It would be simpler enough to just
>>>> embed them in the code.
>>>
>>>
>>> It would be less to code, but isn't it the same ugly code, which we
>>> have now at exynos4xxx-cpufreq.c?
>>>
>>> With those values parsed from DT we can write generic code for the
>>> "arm_clk" clock. One clock implementation for cpufreq-cpu0.c (and maybe
>>> for arm_big_little.c) reused by Exynos4/5.
>>
>>
>> As replied in the 2/3 patch, if these values would change across
>> multiple platforms based on the same SoC, it makes sense to put them
>> into DT. Any data that is purely SoC specific and not going to change
>> across platforms can be embedded into the code itself.
>
>
> Well, they do change. If not on per board basis, then at least with SoC
> revisions.
>
> Anyway, the biggest problem is that the same data needs to be duplicated
> (well, triplicated) for each driver that needs them. If you can find a
> reasonable way to avoid redundancy, without having DT involved, I will
> probably be fine with it.

I have listed down the need for the three tables in the reply to your
comments in 2/6 patch.

>
>
>>>>
>>>> These are frequencies supported by the core clock. But the cpufreq
>>>> table can use all or subset of the supported frequencies.
>>>
>>>
>>> I see your point, but I find this distinction here a bit superfluous.
>>
>>
>> Replied to similar comment in 2/3 patch.
>>
>>>
>>>> The core
>>>> clock should be usable with the clock api independently and not tied
>>>> to be used only by cpufreq driver.
>>>
>>>
>>> But then still for Exynos it will use PLL's M P S coefficients which
>>> only corresponds to values defined at cpufreq's frequency table.
>>
>>
>> The PLL clocks are now separated out as PLL clock types in
>> samsung/clk-pll.c file. The P,M,S values of the PLLs are now handled
>> over there. So now the PLL is independent of the cpufreq driver and
>> can support any number of clock speeds not limited to the ones needed
>> by cpufreq.
>
>
> Don't forget about opposite side of this relation. The APLL needs to support
> at least those that are required by cpufreq.

The core clock can round the rate requested by the cpufreq driver and
provide an output with least delta (but always less then the frequency
requested by cpufreq).

>
>
>>
>>>
>>> The set of frequencies for PLL, cpufreq and this clock is the same, so
>>> I think that we shall not define them in three different places.
>>>
>>> Could you give any example supporting your point of view?
>>
>>
>> A PLL is a hardware component that can be reused in multiple SoCs. A
>> PLL can generate and support 'x' number of clock speeds but a SoC
>> using that PLL might use only 'y' (a subset of 'x')  number of clock
>> speeds of the PLL due to certain hardware limitations. Then there are
>> platforms using a SoC which might use only 'z' (a subset of 'y') clock
>> speeds due to the power/performance requirements of the platform.
>>
>
> As observed with our platforms, 'x' is usually SoC or SoC-revision specific
> and equal to 'y' and 'z' on respective platforms.

That is not a hard rule. When used in products, 'z' is influenced by
the power/performance needs of the product and 'y' tends to be
superset of 'z'. 'x' is SoC specific and 'y' can a derived set of 'x'.


>
>
>>>
>>>>
>>>>>
>>>>>> +
>>>>>> +static const struct samsung_core_clock_freq_table
>>>>>> exyno4210_armclk_table = {
>>>>>> +     .freq           = exynos4210_armclk_freqs,
>>>>>> +     .freq_count     = ARRAY_SIZE(exynos4210_armclk_freqs),
>>>>>> +     .data           = (void *)exynos4210_armclk_data,
>>>>>> +};
>>>>>> +
>>>>>> +static int exynos4210_armclk_set_rate(struct clk_hw *hw, unsigned
>>>>>> long drate,
>>>>>> +                                     unsigned long prate)
>>>>>> +{
>>>>>> +     struct samsung_core_clock *armclk;
>>>>>> +     const struct samsung_core_clock_freq_table *freq_tbl;
>>>>>> +     unsigned long *freq_data;
>>>>>> +     unsigned long mux_reg, idx;
>>>>>> +     void __iomem *base;
>>>>>> +
>>>>>> +     if (drate == prate)
>>>>>> +             return 0;
>>>>>> +
>>>>>> +     armclk = container_of(hw, struct samsung_core_clock, hw);
>>>>>> +     freq_tbl = armclk->freq_table;
>>>>>> +     freq_data = (unsigned long *)freq_tbl->data;
>>>>>> +     base = armclk->ctrl_base;
>>>>>> +
>>>>>> +     for (idx = 0; idx < freq_tbl->freq_count; idx++, freq_data
>>>>>> += 2)
>>>>>> +             if ((freq_tbl->freq[idx] * 1000) == drate)
>>>>>> +                     break;
>>>>>> +
>>>>>> +     if (!armclk->fout_pll)
>>>>>> +             armclk->fout_pll = __clk_lookup("fout_apll");\
>>>>>
>>>>>                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^[*]
>>>>>
>>>>> I'm a bit confused here for two reasons. Please correct me if I'm
>>>>> wrong.
>>>>>
>>>>> 1. You go into this ->set_rate() because of calling clk_set_rate at
>>>>> "arm_clk" clock (numbered as 12 at clk-exynos4.c) at cpufreq-cpu0.c
>>>>>
>>>>> In a Exynos4210 we have:
>>>>> XXTI-> APLL -> fout_apll -> mout_apll -> mout_core -> div_core
>>>>> -> div_core2 -> arm_clk
>>>>>
>>>>> In the code you call directly the fout_apll which changes
>>>>> frequency. Then the change shall be propagated to all registered
>>>>> clocks.
>>>>> I think, that DIV and DIV1 shall be reduced before PLL change [*],
>>>>> to reflect the changes at CCF.
>>>>
>>>>
>>>> The core clock implementation encapsulates multiple clock blocks (such
>>>> as dividers and muxes) which are in between the output of the APLL and
>>>> the point that actually is the cpu domain clock output.
>>>
>>>
>>> No problem with that. I mostly agree...
>>>
>>>> When a clock
>>>> frequency change has to be made, all these clock blocks encapsulated
>>>> within the core clock are programmed by pre-determined values.
>>>
>>>
>>> And what about the situation with already defined clocks (like
>>> "div_core" and "div_core2"). Those will not be updated when you first
>>> call clk_set_rate() and change by hand DIV and DIV1.
>>>
>>> What if you would like to have the PCLK_DBG clock used in the future?
>>> You would add it to CCF and the change will not propagate.
>>
>>
>> I did intend to remove individual clock blocks which are now
>> encapsulated within the core clock type from the clock driver file. I
>> missed doing that in this patch series.
>
>
> Yes, they should be removed, since they are encapsulated inside the core
> clock now.
>
>
>>
>>>
>>>> This
>>>> approach allows very fast clock speed switching, instead of traversing
>>>> the entire CCF clock tree searching for individual clock blocks to be
>>>> programmed.
>>>
>>>
>>> Those are mostly DIV and MUXes. Recalculation shouldn't be time
>>> consuming.
>>
>>
>> I was mainly referring to the time taken to search the clock tree for
>> these individual clock blocks.
>
>
> Hmm, why couldn't you simply look-up all the needed clock at initialization
> and keep references to them?
>
> Still, I think it is fine to directly program registers of encapsulated
> clocks, since they are not visible outside anymore and there is no need for
> them to be visible.
>
>
>>
>>>
>>>>
>>>>>
>>>>>
>>>>>> +
>>>>>> +     if (drate < prate) {
>>>>>> +             mux_reg = readl(base + SRC_CPU);
>>>>>> +             writel(mux_reg | (1 << 16), base + SRC_CPU);
>>>>>> +             while (((readl(base + STAT_CPU) >> 16) & 0x7) != 2)
>>>>>> +                     ;
>>>>>
>>>>>                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [**]
>>>>>
>>>>> 2. I think, the above shall be done in a following way:
>>>>>
>>>>>          clk_set_parent(mout_core, mout_mpll);
>>>>>          clk_set_rate(armclk->fout_pll, drate);
>>>>>          clk_set_parent(mout_core, mout_apll);
>>>>>
>>>>> The direct write to registers [**] doesn't look compliant to CCF.
>>>>>
>>>>
>>>> As mentioned above, the clock block encapsulates these clock blocks
>>>> into a single clock and only this single encapsulated clock is
>>>> registered with CCF. The internal implementation of how the different
>>>> clock blocks are managed within this clock is independent of the CCF.
>>>
>>>
>>> I agree, that the CPU_DIV and CPU_DIV1 shall be changed atomically
>>> (without CCF).
>>>
>>> But on the situation [**] the MUX can be changed by clk_set_parent() as
>>> it is now done at exynosXXXX-cpufreq.c code.
>>
>>
>> The mux is also encapsulated into a larger clock type and this new
>> clock type know how the mux has to be configured.
>
>
> IMHO it's fine to encapsulate the mux as well. There are no users of it
> other than the core clock.
>
>
>>
>>>
>>>
>>>>
>>>>>
>>>>> I'd rather thought about using "mout_core" instead of "arm_clk".
>>>>> Then we would get access to the parent directly:
>>>>>
>>>>>          struct clk *parent = clk_get_parent(hw->clk);
>>>>>
>>>>> so we set the parents explicitly (at clk registration) and call
>>>>> ->recalc_rate for clocks which are lower in the tree (like
>>>>> "div_core", "div_core2").
>>>>
>>>>
>>>> That was not the intention as mentioned above.
>>>
>>>
>>> This is just another possible solution to the problem.
>
>
> Those clocks should not be dealt with separately and this was the intention
> of this composite clock. I agree with Thomas here.
>
> Best regards,
> Tomasz

Thanks for your comments Tomasz.

Thanks,
Thomas.

[PATCH 4/6] cpufreq: exynos: remove Exynos4210 specific cpufreq driver support

[Thomas Abraham]

The Exynos4210 specific cpufreq driver performs read/write operations
of clock controller registers bypassing the Exynos common clock
framework driver. This could lead to potential issues if CCF and
cpufreq driver modify the clock registers independently of each other.
In addition to this, the generic cpufreq-cpu0 driver is sufficient
for Exynos4210 based platforms since this SoC uses a single clock
and voltage line for both the ARM cores. So remove the support for
Exynos4210 specific cpufreq driver and use cpufreq-cpu0 driver
for Exynos4210 platforms

Signed-off-by: Thomas Abraham <thomas.ab@samsung.com>
---
 drivers/cpufreq/Kconfig.arm          |   11 ---
 drivers/cpufreq/Makefile             |    1 -
 drivers/cpufreq/exynos-cpufreq.c     |    4 +-
 drivers/cpufreq/exynos-cpufreq.h     |    8 --
 drivers/cpufreq/exynos4210-cpufreq.c |  157 ----------------------------------
 5 files changed, 1 insertions(+), 180 deletions(-)
 delete mode 100644 drivers/cpufreq/exynos4210-cpufreq.c

diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
index 0468ad1..0a2a589 100644
--- a/drivers/cpufreq/Kconfig.arm
+++ b/drivers/cpufreq/Kconfig.arm
@@ -19,17 +19,6 @@ config ARM_DT_BL_CPUFREQ
 config ARM_EXYNOS_CPUFREQ
 	bool
 
-config ARM_EXYNOS4210_CPUFREQ
-	bool "SAMSUNG EXYNOS4210"
-	depends on CPU_EXYNOS4210
-	default y
-	select ARM_EXYNOS_CPUFREQ
-	help
-	  This adds the CPUFreq driver for Samsung EXYNOS4210
-	  SoC (S5PV310 or S5PC210).
-
-	  If in doubt, say N.
-
 config ARM_EXYNOS4X12_CPUFREQ
 	bool "SAMSUNG EXYNOS4x12"
 	depends on (SOC_EXYNOS4212 || SOC_EXYNOS4412)
diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
index 7494565..ce2abf9 100644
--- a/drivers/cpufreq/Makefile
+++ b/drivers/cpufreq/Makefile
@@ -50,7 +50,6 @@ obj-$(CONFIG_ARM_DT_BL_CPUFREQ)		+= arm_big_little_dt.o
 obj-$(CONFIG_ARCH_DAVINCI_DA850)	+= davinci-cpufreq.o
 obj-$(CONFIG_UX500_SOC_DB8500)		+= dbx500-cpufreq.o
 obj-$(CONFIG_ARM_EXYNOS_CPUFREQ)	+= exynos-cpufreq.o
-obj-$(CONFIG_ARM_EXYNOS4210_CPUFREQ)	+= exynos4210-cpufreq.o
 obj-$(CONFIG_ARM_EXYNOS4X12_CPUFREQ)	+= exynos4x12-cpufreq.o
 obj-$(CONFIG_ARM_EXYNOS5250_CPUFREQ)	+= exynos5250-cpufreq.o
 obj-$(CONFIG_ARM_EXYNOS5440_CPUFREQ)	+= exynos5440-cpufreq.o
diff --git a/drivers/cpufreq/exynos-cpufreq.c b/drivers/cpufreq/exynos-cpufreq.c
index 4ee3804..eacc7eb 100644
--- a/drivers/cpufreq/exynos-cpufreq.c
+++ b/drivers/cpufreq/exynos-cpufreq.c
@@ -237,9 +237,7 @@ static int exynos_cpufreq_probe(struct platform_device *pdev)
 	if (!exynos_info)
 		return -ENOMEM;
 
-	if (soc_is_exynos4210())
-		ret = exynos4210_cpufreq_init(exynos_info);
-	else if (soc_is_exynos4212() || soc_is_exynos4412())
+	if (soc_is_exynos4212() || soc_is_exynos4412())
 		ret = exynos4x12_cpufreq_init(exynos_info);
 	else if (soc_is_exynos5250())
 		ret = exynos5250_cpufreq_init(exynos_info);
diff --git a/drivers/cpufreq/exynos-cpufreq.h b/drivers/cpufreq/exynos-cpufreq.h
index 3ddade8..1e0afbe 100644
--- a/drivers/cpufreq/exynos-cpufreq.h
+++ b/drivers/cpufreq/exynos-cpufreq.h
@@ -43,14 +43,6 @@ struct exynos_dvfs_info {
 	bool (*need_apll_change)(unsigned int, unsigned int);
 };
 
-#ifdef CONFIG_ARM_EXYNOS4210_CPUFREQ
-extern int exynos4210_cpufreq_init(struct exynos_dvfs_info *);
-#else
-static inline int exynos4210_cpufreq_init(struct exynos_dvfs_info *info)
-{
-	return -EOPNOTSUPP;
-}
-#endif
 #ifdef CONFIG_ARM_EXYNOS4X12_CPUFREQ
 extern int exynos4x12_cpufreq_init(struct exynos_dvfs_info *);
 #else
diff --git a/drivers/cpufreq/exynos4210-cpufreq.c b/drivers/cpufreq/exynos4210-cpufreq.c
deleted file mode 100644
index 40d84c4..0000000
--- a/drivers/cpufreq/exynos4210-cpufreq.c
+++ /dev/null
@@ -1,157 +0,0 @@
-/*
- * Copyright (c) 2010-2011 Samsung Electronics Co., Ltd.
- *		http://www.samsung.com
- *
- * EXYNOS4210 - CPU frequency scaling support
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 as
- * published by the Free Software Foundation.
-*/
-
-#include <linux/module.h>
-#include <linux/kernel.h>
-#include <linux/err.h>
-#include <linux/clk.h>
-#include <linux/io.h>
-#include <linux/slab.h>
-#include <linux/cpufreq.h>
-
-#include "exynos-cpufreq.h"
-
-static struct clk *cpu_clk;
-static struct clk *moutcore;
-static struct clk *mout_mpll;
-static struct clk *mout_apll;
-
-static unsigned int exynos4210_volt_table[] = {
-	1250000, 1150000, 1050000, 975000, 950000,
-};
-
-static struct cpufreq_frequency_table exynos4210_freq_table[] = {
-	{L0, 1200 * 1000},
-	{L1, 1000 * 1000},
-	{L2,  800 * 1000},
-	{L3,  500 * 1000},
-	{L4,  200 * 1000},
-	{0, CPUFREQ_TABLE_END},
-};
-
-static struct apll_freq apll_freq_4210[] = {
-	/*
-	 * values:
-	 * freq
-	 * clock divider for CORE, COREM0, COREM1, PERIPH, ATB, PCLK_DBG, APLL, RESERVED
-	 * clock divider for COPY, HPM, RESERVED
-	 * PLL M, P, S
-	 */
-	APLL_FREQ(1200, 0, 3, 7, 3, 4, 1, 7, 0, 5, 0, 0, 150, 3, 1),
-	APLL_FREQ(1000, 0, 3, 7, 3, 4, 1, 7, 0, 4, 0, 0, 250, 6, 1),
-	APLL_FREQ(800,  0, 3, 7, 3, 3, 1, 7, 0, 3, 0, 0, 200, 6, 1),
-	APLL_FREQ(500,  0, 3, 7, 3, 3, 1, 7, 0, 3, 0, 0, 250, 6, 2),
-	APLL_FREQ(200,  0, 1, 3, 1, 3, 1, 0, 0, 3, 0, 0, 200, 6, 3),
-};
-
-static void exynos4210_set_clkdiv(unsigned int div_index)
-{
-	unsigned int tmp;
-
-	/* Change Divider - CPU0 */
-
-	tmp = apll_freq_4210[div_index].clk_div_cpu0;
-
-	__raw_writel(tmp, EXYNOS4_CLKDIV_CPU);
-
-	do {
-		tmp = __raw_readl(EXYNOS4_CLKDIV_STATCPU);
-	} while (tmp & 0x1111111);
-
-	/* Change Divider - CPU1 */
-
-	tmp = apll_freq_4210[div_index].clk_div_cpu1;
-
-	__raw_writel(tmp, EXYNOS4_CLKDIV_CPU1);
-
-	do {
-		tmp = __raw_readl(EXYNOS4_CLKDIV_STATCPU1);
-	} while (tmp & 0x11);
-}
-
-static void exynos4210_set_apll(unsigned int index)
-{
-	unsigned int tmp, freq = apll_freq_4210[index].freq;
-
-	/* MUX_CORE_SEL = MPLL, ARMCLK uses MPLL for lock time */
-	clk_set_parent(moutcore, mout_mpll);
-
-	do {
-		tmp = (__raw_readl(EXYNOS4_CLKMUX_STATCPU)
-			>> EXYNOS4_CLKSRC_CPU_MUXCORE_SHIFT);
-		tmp &= 0x7;
-	} while (tmp != 0x2);
-
-	clk_set_rate(mout_apll, freq * 1000);
-
-	/* MUX_CORE_SEL = APLL */
-	clk_set_parent(moutcore, mout_apll);
-
-	do {
-		tmp = __raw_readl(EXYNOS4_CLKMUX_STATCPU);
-		tmp &= EXYNOS4_CLKMUX_STATCPU_MUXCORE_MASK;
-	} while (tmp != (0x1 << EXYNOS4_CLKSRC_CPU_MUXCORE_SHIFT));
-}
-
-static void exynos4210_set_frequency(unsigned int old_index,
-				     unsigned int new_index)
-{
-	if (old_index > new_index) {
-		exynos4210_set_clkdiv(new_index);
-		exynos4210_set_apll(new_index);
-	} else if (old_index < new_index) {
-		exynos4210_set_apll(new_index);
-		exynos4210_set_clkdiv(new_index);
-	}
-}
-
-int exynos4210_cpufreq_init(struct exynos_dvfs_info *info)
-{
-	unsigned long rate;
-
-	cpu_clk = clk_get(NULL, "armclk");
-	if (IS_ERR(cpu_clk))
-		return PTR_ERR(cpu_clk);
-
-	moutcore = clk_get(NULL, "moutcore");
-	if (IS_ERR(moutcore))
-		goto err_moutcore;
-
-	mout_mpll = clk_get(NULL, "mout_mpll");
-	if (IS_ERR(mout_mpll))
-		goto err_mout_mpll;
-
-	rate = clk_get_rate(mout_mpll) / 1000;
-
-	mout_apll = clk_get(NULL, "mout_apll");
-	if (IS_ERR(mout_apll))
-		goto err_mout_apll;
-
-	info->mpll_freq_khz = rate;
-	/* 800Mhz */
-	info->pll_safe_idx = L2;
-	info->cpu_clk = cpu_clk;
-	info->volt_table = exynos4210_volt_table;
-	info->freq_table = exynos4210_freq_table;
-	info->set_freq = exynos4210_set_frequency;
-
-	return 0;
-
-err_mout_apll:
-	clk_put(mout_mpll);
-err_mout_mpll:
-	clk_put(moutcore);
-err_moutcore:
-	clk_put(cpu_clk);
-
-	pr_debug("%s: failed initialization\n", __func__);
-	return -EINVAL;
-}
-- 
1.6.6.rc2

Re: [PATCH 4/6] cpufreq: exynos: remove Exynos4210 specific cpufreq driver support

[Lukasz Majewski]

Hi Thomas,

> The Exynos4210 specific cpufreq driver performs read/write operations
> of clock controller registers bypassing the Exynos common clock
> framework driver. This could lead to potential issues if CCF and
> cpufreq driver modify the clock registers independently of each other.
> In addition to this, the generic cpufreq-cpu0 driver is sufficient
> for Exynos4210 based platforms since this SoC uses a single clock
> and voltage line for both the ARM cores. So remove the support for
> Exynos4210 specific cpufreq driver and use cpufreq-cpu0 driver
> for Exynos4210 platforms
> 
> Signed-off-by: Thomas Abraham <thomas.ab@samsung.com>
> ---
>  drivers/cpufreq/Kconfig.arm          |   11 ---
>  drivers/cpufreq/Makefile             |    1 -
>  drivers/cpufreq/exynos-cpufreq.c     |    4 +-
>  drivers/cpufreq/exynos-cpufreq.h     |    8 --
>  drivers/cpufreq/exynos4210-cpufreq.c |  157
> ---------------------------------- 5 files changed, 1 insertions(+),
> 180 deletions(-) delete mode 100644
> drivers/cpufreq/exynos4210-cpufreq.c
> 
> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> index 0468ad1..0a2a589 100644
> --- a/drivers/cpufreq/Kconfig.arm
> +++ b/drivers/cpufreq/Kconfig.arm
> @@ -19,17 +19,6 @@ config ARM_DT_BL_CPUFREQ
>  config ARM_EXYNOS_CPUFREQ
>  	bool
>  
> -config ARM_EXYNOS4210_CPUFREQ
> -	bool "SAMSUNG EXYNOS4210"
> -	depends on CPU_EXYNOS4210
> -	default y
> -	select ARM_EXYNOS_CPUFREQ
> -	help
> -	  This adds the CPUFreq driver for Samsung EXYNOS4210
> -	  SoC (S5PV310 or S5PC210).
> -
> -	  If in doubt, say N.
> -
>  config ARM_EXYNOS4X12_CPUFREQ
>  	bool "SAMSUNG EXYNOS4x12"
>  	depends on (SOC_EXYNOS4212 || SOC_EXYNOS4412)
> diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
> index 7494565..ce2abf9 100644
> --- a/drivers/cpufreq/Makefile
> +++ b/drivers/cpufreq/Makefile
> @@ -50,7 +50,6 @@ obj-$(CONFIG_ARM_DT_BL_CPUFREQ)		+=
> arm_big_little_dt.o obj-$(CONFIG_ARCH_DAVINCI_DA850)	+=
> davinci-cpufreq.o obj-$(CONFIG_UX500_SOC_DB8500)		+=
> dbx500-cpufreq.o obj-$(CONFIG_ARM_EXYNOS_CPUFREQ)	+=
> exynos-cpufreq.o -obj-$(CONFIG_ARM_EXYNOS4210_CPUFREQ)	+=
> exynos4210-cpufreq.o obj-$(CONFIG_ARM_EXYNOS4X12_CPUFREQ)	+=
> exynos4x12-cpufreq.o obj-$(CONFIG_ARM_EXYNOS5250_CPUFREQ)	+=
> exynos5250-cpufreq.o obj-$(CONFIG_ARM_EXYNOS5440_CPUFREQ)	+=
> exynos5440-cpufreq.o diff --git a/drivers/cpufreq/exynos-cpufreq.c
> b/drivers/cpufreq/exynos-cpufreq.c index 4ee3804..eacc7eb 100644
> --- a/drivers/cpufreq/exynos-cpufreq.c
> +++ b/drivers/cpufreq/exynos-cpufreq.c
> @@ -237,9 +237,7 @@ static int exynos_cpufreq_probe(struct
> platform_device *pdev) if (!exynos_info)
>  		return -ENOMEM;
>  
> -	if (soc_is_exynos4210())
> -		ret = exynos4210_cpufreq_init(exynos_info);
> -	else if (soc_is_exynos4212() || soc_is_exynos4412())
> +	if (soc_is_exynos4212() || soc_is_exynos4412())
>  		ret = exynos4x12_cpufreq_init(exynos_info);
>  	else if (soc_is_exynos5250())
>  		ret = exynos5250_cpufreq_init(exynos_info);
> diff --git a/drivers/cpufreq/exynos-cpufreq.h
> b/drivers/cpufreq/exynos-cpufreq.h index 3ddade8..1e0afbe 100644
> --- a/drivers/cpufreq/exynos-cpufreq.h
> +++ b/drivers/cpufreq/exynos-cpufreq.h
> @@ -43,14 +43,6 @@ struct exynos_dvfs_info {
>  	bool (*need_apll_change)(unsigned int, unsigned int);
>  };
>  
> -#ifdef CONFIG_ARM_EXYNOS4210_CPUFREQ
> -extern int exynos4210_cpufreq_init(struct exynos_dvfs_info *);
> -#else
> -static inline int exynos4210_cpufreq_init(struct exynos_dvfs_info
> *info) -{
> -	return -EOPNOTSUPP;
> -}
> -#endif
>  #ifdef CONFIG_ARM_EXYNOS4X12_CPUFREQ
>  extern int exynos4x12_cpufreq_init(struct exynos_dvfs_info *);
>  #else
> diff --git a/drivers/cpufreq/exynos4210-cpufreq.c
> b/drivers/cpufreq/exynos4210-cpufreq.c deleted file mode 100644
> index 40d84c4..0000000
> --- a/drivers/cpufreq/exynos4210-cpufreq.c
> +++ /dev/null
> @@ -1,157 +0,0 @@
> -/*
> - * Copyright (c) 2010-2011 Samsung Electronics Co., Ltd.
> - *		http://www.samsung.com
> - *
> - * EXYNOS4210 - CPU frequency scaling support
> - *
> - * This program is free software; you can redistribute it and/or
> modify
> - * it under the terms of the GNU General Public License version 2 as
> - * published by the Free Software Foundation.
> -*/
> -
> -#include <linux/module.h>
> -#include <linux/kernel.h>
> -#include <linux/err.h>
> -#include <linux/clk.h>
> -#include <linux/io.h>
> -#include <linux/slab.h>
> -#include <linux/cpufreq.h>
> -
> -#include "exynos-cpufreq.h"
> -
> -static struct clk *cpu_clk;
> -static struct clk *moutcore;
> -static struct clk *mout_mpll;
> -static struct clk *mout_apll;
> -
> -static unsigned int exynos4210_volt_table[] = {
> -	1250000, 1150000, 1050000, 975000, 950000,
> -};
> -
> -static struct cpufreq_frequency_table exynos4210_freq_table[] = {
> -	{L0, 1200 * 1000},
> -	{L1, 1000 * 1000},
> -	{L2,  800 * 1000},
> -	{L3,  500 * 1000},
> -	{L4,  200 * 1000},
> -	{0, CPUFREQ_TABLE_END},
> -};
> -
> -static struct apll_freq apll_freq_4210[] = {
> -	/*
> -	 * values:
> -	 * freq
> -	 * clock divider for CORE, COREM0, COREM1, PERIPH, ATB,
> PCLK_DBG, APLL, RESERVED
> -	 * clock divider for COPY, HPM, RESERVED
> -	 * PLL M, P, S
> -	 */
> -	APLL_FREQ(1200, 0, 3, 7, 3, 4, 1, 7, 0, 5, 0, 0, 150, 3, 1),
> -	APLL_FREQ(1000, 0, 3, 7, 3, 4, 1, 7, 0, 4, 0, 0, 250, 6, 1),
> -	APLL_FREQ(800,  0, 3, 7, 3, 3, 1, 7, 0, 3, 0, 0, 200, 6, 1),
> -	APLL_FREQ(500,  0, 3, 7, 3, 3, 1, 7, 0, 3, 0, 0, 250, 6, 2),
> -	APLL_FREQ(200,  0, 1, 3, 1, 3, 1, 0, 0, 3, 0, 0, 200, 6, 3),
> -};
> -
> -static void exynos4210_set_clkdiv(unsigned int div_index)
> -{
> -	unsigned int tmp;
> -
> -	/* Change Divider - CPU0 */
> -
> -	tmp = apll_freq_4210[div_index].clk_div_cpu0;
> -
> -	__raw_writel(tmp, EXYNOS4_CLKDIV_CPU);
> -
> -	do {
> -		tmp = __raw_readl(EXYNOS4_CLKDIV_STATCPU);
> -	} while (tmp & 0x1111111);
> -
> -	/* Change Divider - CPU1 */
> -
> -	tmp = apll_freq_4210[div_index].clk_div_cpu1;
> -
> -	__raw_writel(tmp, EXYNOS4_CLKDIV_CPU1);
> -
> -	do {
> -		tmp = __raw_readl(EXYNOS4_CLKDIV_STATCPU1);
> -	} while (tmp & 0x11);
> -}
> -
> -static void exynos4210_set_apll(unsigned int index)
> -{
> -	unsigned int tmp, freq = apll_freq_4210[index].freq;
> -
> -	/* MUX_CORE_SEL = MPLL, ARMCLK uses MPLL for lock time */
> -	clk_set_parent(moutcore, mout_mpll);
> -
> -	do {
> -		tmp = (__raw_readl(EXYNOS4_CLKMUX_STATCPU)
> -			>> EXYNOS4_CLKSRC_CPU_MUXCORE_SHIFT);
> -		tmp &= 0x7;
> -	} while (tmp != 0x2);
> -
> -	clk_set_rate(mout_apll, freq * 1000);
> -
> -	/* MUX_CORE_SEL = APLL */
> -	clk_set_parent(moutcore, mout_apll);
> -
> -	do {
> -		tmp = __raw_readl(EXYNOS4_CLKMUX_STATCPU);
> -		tmp &= EXYNOS4_CLKMUX_STATCPU_MUXCORE_MASK;
> -	} while (tmp != (0x1 << EXYNOS4_CLKSRC_CPU_MUXCORE_SHIFT));
> -}
> -
> -static void exynos4210_set_frequency(unsigned int old_index,
> -				     unsigned int new_index)
> -{
> -	if (old_index > new_index) {
> -		exynos4210_set_clkdiv(new_index);
> -		exynos4210_set_apll(new_index);
> -	} else if (old_index < new_index) {
> -		exynos4210_set_apll(new_index);
> -		exynos4210_set_clkdiv(new_index);
> -	}
> -}
> -
> -int exynos4210_cpufreq_init(struct exynos_dvfs_info *info)
> -{
> -	unsigned long rate;
> -
> -	cpu_clk = clk_get(NULL, "armclk");
> -	if (IS_ERR(cpu_clk))
> -		return PTR_ERR(cpu_clk);
> -
> -	moutcore = clk_get(NULL, "moutcore");
> -	if (IS_ERR(moutcore))
> -		goto err_moutcore;
> -
> -	mout_mpll = clk_get(NULL, "mout_mpll");
> -	if (IS_ERR(mout_mpll))
> -		goto err_mout_mpll;
> -
> -	rate = clk_get_rate(mout_mpll) / 1000;
> -
> -	mout_apll = clk_get(NULL, "mout_apll");
> -	if (IS_ERR(mout_apll))
> -		goto err_mout_apll;
> -
> -	info->mpll_freq_khz = rate;
> -	/* 800Mhz */
> -	info->pll_safe_idx = L2;
> -	info->cpu_clk = cpu_clk;
> -	info->volt_table = exynos4210_volt_table;
> -	info->freq_table = exynos4210_freq_table;
> -	info->set_freq = exynos4210_set_frequency;
> -
> -	return 0;
> -
> -err_mout_apll:
> -	clk_put(mout_mpll);
> -err_mout_mpll:
> -	clk_put(moutcore);
> -err_moutcore:
> -	clk_put(cpu_clk);
> -
> -	pr_debug("%s: failed initialization\n", __func__);
> -	return -EINVAL;
> -}

Reviewed-by: Lukasz Majewski <l.majewski@samsung.com>

-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

[PATCH 5/6] arm: exynos4-dt: statically add platform device for cpufreq-cpu0 platform driver

[Thomas Abraham]

In order to use the cpufreq-cpu0 driver on Exynos4 based platforms,
statically add the platform device for cpufreq-cpu0 platform driver.

Signed-off-by: Thomas Abraham <thomas.ab@samsung.com>
---
 arch/arm/mach-exynos/mach-exynos4-dt.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-exynos/mach-exynos4-dt.c b/arch/arm/mach-exynos/mach-exynos4-dt.c
index d3e54b7..8b8ad41 100644
--- a/arch/arm/mach-exynos/mach-exynos4-dt.c
+++ b/arch/arm/mach-exynos/mach-exynos4-dt.c
@@ -19,11 +19,17 @@
 
 #include "common.h"
 
+static struct platform_device cpufreq_device = {
+	.name		= "cpufreq-cpu0",
+	.id		= -1,
+};
+
 static void __init exynos4_dt_machine_init(void)
 {
 	exynos_cpuidle_init();
 	exynos_cpufreq_init();
 
+	platform_device_register(&cpufreq_device);
 	of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
 }
 
-- 
1.6.6.rc2

Re: [PATCH 5/6] arm: exynos4-dt: statically add platform device for cpufreq-cpu0 platform driver

[Lukasz Majewski]

Hi Thomas,

> In order to use the cpufreq-cpu0 driver on Exynos4 based platforms,
> statically add the platform device for cpufreq-cpu0 platform driver.
> 
> Signed-off-by: Thomas Abraham <thomas.ab@samsung.com>
> ---
>  arch/arm/mach-exynos/mach-exynos4-dt.c |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/mach-exynos/mach-exynos4-dt.c
> b/arch/arm/mach-exynos/mach-exynos4-dt.c index d3e54b7..8b8ad41 100644
> --- a/arch/arm/mach-exynos/mach-exynos4-dt.c
> +++ b/arch/arm/mach-exynos/mach-exynos4-dt.c
> @@ -19,11 +19,17 @@
>  
>  #include "common.h"
>  
> +static struct platform_device cpufreq_device = {
> +	.name		= "cpufreq-cpu0",
> +	.id		= -1,
> +};
> +
>  static void __init exynos4_dt_machine_init(void)
>  {
>  	exynos_cpuidle_init();
>  	exynos_cpufreq_init();
>  
> +	platform_device_register(&cpufreq_device);
>  	of_platform_populate(NULL, of_default_bus_match_table, NULL,
> NULL); }
>  

Reviewed-by: Lukasz Majewski <l.majewski@samsung.com>

-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

Re: [PATCH 5/6] arm: exynos4-dt: statically add platform device for cpufreq-cpu0 platform driver

[Shawn Guo]

On Thu, Jan 09, 2014 at 09:29:24PM +0530, Thomas Abraham wrote:
> +static struct platform_device cpufreq_device = {
> +	.name		= "cpufreq-cpu0",
> +	.id		= -1,
> +};
> +
>  static void __init exynos4_dt_machine_init(void)
>  {
>  	exynos_cpuidle_init();
>  	exynos_cpufreq_init();
>  
> +	platform_device_register(&cpufreq_device);

It can just be:

	platform_device_register_simple("cpufreq-cpu0", -1, NULL, 0);

Shawn

>  	of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
>  }
>  
> -- 
> 1.6.6.rc2
> 

[PATCH 6/6] arm: dts: add cpu nodes for Exynos4210 SoC

[Thomas Abraham]

Add CPU nodes for Exynos4210 SoC and also properties required by the
cpufreq-cpu0 driver.

Signed-off-by: Thomas Abraham <thomas.ab@samsung.com>
---
 arch/arm/boot/dts/exynos4210-origen.dts         |    6 ++++++
 arch/arm/boot/dts/exynos4210-trats.dts          |    6 ++++++
 arch/arm/boot/dts/exynos4210-universal_c210.dts |    6 ++++++
 arch/arm/boot/dts/exynos4210.dtsi               |   22 ++++++++++++++++++++++
 4 files changed, 40 insertions(+), 0 deletions(-)

diff --git a/arch/arm/boot/dts/exynos4210-origen.dts b/arch/arm/boot/dts/exynos4210-origen.dts
index 2aa13cb..afecd8f 100644
--- a/arch/arm/boot/dts/exynos4210-origen.dts
+++ b/arch/arm/boot/dts/exynos4210-origen.dts
@@ -32,6 +32,12 @@
 		bootargs ="root=/dev/ram0 rw ramdisk=8192 initrd=0x41000000,8M console=ttySAC2,115200 init=/linuxrc";
 	};
 
+	cpus {
+		cpu: cpu@0 {
+			cpu0-supply = <&buck1_reg>;
+		};
+	};
+
 	regulators {
 		compatible = "simple-bus";
 		#address-cells = <1>;
diff --git a/arch/arm/boot/dts/exynos4210-trats.dts b/arch/arm/boot/dts/exynos4210-trats.dts
index 63cc571..25487d7 100644
--- a/arch/arm/boot/dts/exynos4210-trats.dts
+++ b/arch/arm/boot/dts/exynos4210-trats.dts
@@ -30,6 +30,12 @@
 		bootargs = "console=ttySAC2,115200N8 root=/dev/mmcblk0p5 rootwait earlyprintk panic=5";
 	};
 
+	cpus {
+		cpu: cpu@0 {
+			cpu0-supply = <&varm_breg>;
+		};
+	};
+
 	regulators {
 		compatible = "simple-bus";
 
diff --git a/arch/arm/boot/dts/exynos4210-universal_c210.dts b/arch/arm/boot/dts/exynos4210-universal_c210.dts
index d2e3f5f..74d5a70 100644
--- a/arch/arm/boot/dts/exynos4210-universal_c210.dts
+++ b/arch/arm/boot/dts/exynos4210-universal_c210.dts
@@ -28,6 +28,12 @@
 		bootargs = "console=ttySAC2,115200N8 root=/dev/mmcblk0p5 rw rootwait earlyprintk panic=5 maxcpus=1";
 	};
 
+	cpus {
+		cpu: cpu@0 {
+			cpu0-supply = <&vdd_arm_reg>;
+		};
+	};
+
 	mct@10050000 {
 		compatible = "none";
 	};
diff --git a/arch/arm/boot/dts/exynos4210.dtsi b/arch/arm/boot/dts/exynos4210.dtsi
index 48ecd7a..3db2da8 100644
--- a/arch/arm/boot/dts/exynos4210.dtsi
+++ b/arch/arm/boot/dts/exynos4210.dtsi
@@ -36,6 +36,28 @@
 		reg = <0x10023CA0 0x20>;
 	};
 
+	cpus {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		cpu: cpu@0 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a9";
+			reg = <0>;
+			clocks = <&clock 12>;
+			clock-names = "cpu";
+
+			operating-points = <
+				200000	950000
+				400000	975000
+				500000	975000
+				800000	1075000
+				1000000	1150000
+				1200000	1250000
+			>;
+			safe-opp-index = <3>;
+		};
+	};
+
 	gic: interrupt-controller@10490000 {
 		cpu-offset = <0x8000>;
 	};
-- 
1.6.6.rc2

Re: [PATCH 6/6] arm: dts: add cpu nodes for Exynos4210 SoC

[Lukasz Majewski]

Hi Thomas,

> Add CPU nodes for Exynos4210 SoC and also properties required by the
> cpufreq-cpu0 driver.
> 
> Signed-off-by: Thomas Abraham <thomas.ab@samsung.com>
> ---
>  arch/arm/boot/dts/exynos4210-origen.dts         |    6 ++++++
>  arch/arm/boot/dts/exynos4210-trats.dts          |    6 ++++++
>  arch/arm/boot/dts/exynos4210-universal_c210.dts |    6 ++++++
>  arch/arm/boot/dts/exynos4210.dtsi               |   22
> ++++++++++++++++++++++ 4 files changed, 40 insertions(+), 0
> deletions(-)
> 
> diff --git a/arch/arm/boot/dts/exynos4210-origen.dts
> b/arch/arm/boot/dts/exynos4210-origen.dts index 2aa13cb..afecd8f
> 100644 --- a/arch/arm/boot/dts/exynos4210-origen.dts
> +++ b/arch/arm/boot/dts/exynos4210-origen.dts
> @@ -32,6 +32,12 @@
>  		bootargs ="root=/dev/ram0 rw ramdisk=8192
> initrd=0x41000000,8M console=ttySAC2,115200 init=/linuxrc"; };
>  
> +	cpus {
> +		cpu: cpu@0 {
> +			cpu0-supply = <&buck1_reg>;
> +		};
> +	};
> +
>  	regulators {
>  		compatible = "simple-bus";
>  		#address-cells = <1>;
> diff --git a/arch/arm/boot/dts/exynos4210-trats.dts
> b/arch/arm/boot/dts/exynos4210-trats.dts index 63cc571..25487d7 100644
> --- a/arch/arm/boot/dts/exynos4210-trats.dts
> +++ b/arch/arm/boot/dts/exynos4210-trats.dts
> @@ -30,6 +30,12 @@
>  		bootargs = "console=ttySAC2,115200N8
> root=/dev/mmcblk0p5 rootwait earlyprintk panic=5"; };
>  
> +	cpus {
> +		cpu: cpu@0 {
> +			cpu0-supply = <&varm_breg>;
> +		};
> +	};
> +
>  	regulators {
>  		compatible = "simple-bus";
>  
> diff --git a/arch/arm/boot/dts/exynos4210-universal_c210.dts
> b/arch/arm/boot/dts/exynos4210-universal_c210.dts index
> d2e3f5f..74d5a70 100644 ---
> a/arch/arm/boot/dts/exynos4210-universal_c210.dts +++
> b/arch/arm/boot/dts/exynos4210-universal_c210.dts @@ -28,6 +28,12 @@
>  		bootargs = "console=ttySAC2,115200N8
> root=/dev/mmcblk0p5 rw rootwait earlyprintk panic=5 maxcpus=1"; };
>  
> +	cpus {
> +		cpu: cpu@0 {
> +			cpu0-supply = <&vdd_arm_reg>;
> +		};
> +	};
> +
>  	mct@10050000 {
>  		compatible = "none";
>  	};
> diff --git a/arch/arm/boot/dts/exynos4210.dtsi
> b/arch/arm/boot/dts/exynos4210.dtsi index 48ecd7a..3db2da8 100644
> --- a/arch/arm/boot/dts/exynos4210.dtsi
> +++ b/arch/arm/boot/dts/exynos4210.dtsi
> @@ -36,6 +36,28 @@
>  		reg = <0x10023CA0 0x20>;
>  	};
>  
> +	cpus {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		cpu: cpu@0 {
> +			device_type = "cpu";
> +			compatible = "arm,cortex-a9";
> +			reg = <0>;
> +			clocks = <&clock 12>;
> +			clock-names = "cpu";
> +
> +			operating-points = <
> +				200000	950000
> +				400000	975000
> +				500000	975000
> +				800000	1075000
> +				1000000	1150000
> +				1200000	1250000
> +			>;

On the ./arch/arm/boot/dts/highbank.dts the oop's order is decreasing. 

That shouldn't be a problem for the cpufreq core, however in my opinion
we shall stick to one convention.

> +			safe-opp-index = <3>;
> +		};
> +	};
> +
>  	gic: interrupt-controller@10490000 {
>  		cpu-offset = <0x8000>;
>  	};



-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

Re: [PATCH 6/6] arm: dts: add cpu nodes for Exynos4210 SoC

[Thomas Abraham]

Hi Lukasz,

On Fri, Jan 10, 2014 at 4:02 PM, Lukasz Majewski <l.majewski@samsung.com> wrote:
> Hi Thomas,
>
>> Add CPU nodes for Exynos4210 SoC and also properties required by the
>> cpufreq-cpu0 driver.
>>
>> Signed-off-by: Thomas Abraham <thomas.ab@samsung.com>
>> ---
>>  arch/arm/boot/dts/exynos4210-origen.dts         |    6 ++++++
>>  arch/arm/boot/dts/exynos4210-trats.dts          |    6 ++++++
>>  arch/arm/boot/dts/exynos4210-universal_c210.dts |    6 ++++++
>>  arch/arm/boot/dts/exynos4210.dtsi               |   22
>> ++++++++++++++++++++++ 4 files changed, 40 insertions(+), 0
>> deletions(-)
>>
>> diff --git a/arch/arm/boot/dts/exynos4210-origen.dts
>> b/arch/arm/boot/dts/exynos4210-origen.dts index 2aa13cb..afecd8f
>> 100644 --- a/arch/arm/boot/dts/exynos4210-origen.dts
>> +++ b/arch/arm/boot/dts/exynos4210-origen.dts
>> @@ -32,6 +32,12 @@
>>               bootargs ="root=/dev/ram0 rw ramdisk=8192
>> initrd=0x41000000,8M console=ttySAC2,115200 init=/linuxrc"; };
>>
>> +     cpus {
>> +             cpu: cpu@0 {
>> +                     cpu0-supply = <&buck1_reg>;
>> +             };
>> +     };
>> +
>>       regulators {
>>               compatible = "simple-bus";
>>               #address-cells = <1>;
>> diff --git a/arch/arm/boot/dts/exynos4210-trats.dts
>> b/arch/arm/boot/dts/exynos4210-trats.dts index 63cc571..25487d7 100644
>> --- a/arch/arm/boot/dts/exynos4210-trats.dts
>> +++ b/arch/arm/boot/dts/exynos4210-trats.dts
>> @@ -30,6 +30,12 @@
>>               bootargs = "console=ttySAC2,115200N8
>> root=/dev/mmcblk0p5 rootwait earlyprintk panic=5"; };
>>
>> +     cpus {
>> +             cpu: cpu@0 {
>> +                     cpu0-supply = <&varm_breg>;
>> +             };
>> +     };
>> +
>>       regulators {
>>               compatible = "simple-bus";
>>
>> diff --git a/arch/arm/boot/dts/exynos4210-universal_c210.dts
>> b/arch/arm/boot/dts/exynos4210-universal_c210.dts index
>> d2e3f5f..74d5a70 100644 ---
>> a/arch/arm/boot/dts/exynos4210-universal_c210.dts +++
>> b/arch/arm/boot/dts/exynos4210-universal_c210.dts @@ -28,6 +28,12 @@
>>               bootargs = "console=ttySAC2,115200N8
>> root=/dev/mmcblk0p5 rw rootwait earlyprintk panic=5 maxcpus=1"; };
>>
>> +     cpus {
>> +             cpu: cpu@0 {
>> +                     cpu0-supply = <&vdd_arm_reg>;
>> +             };
>> +     };
>> +
>>       mct@10050000 {
>>               compatible = "none";
>>       };
>> diff --git a/arch/arm/boot/dts/exynos4210.dtsi
>> b/arch/arm/boot/dts/exynos4210.dtsi index 48ecd7a..3db2da8 100644
>> --- a/arch/arm/boot/dts/exynos4210.dtsi
>> +++ b/arch/arm/boot/dts/exynos4210.dtsi
>> @@ -36,6 +36,28 @@
>>               reg = <0x10023CA0 0x20>;
>>       };
>>
>> +     cpus {
>> +             #address-cells = <1>;
>> +             #size-cells = <0>;
>> +             cpu: cpu@0 {
>> +                     device_type = "cpu";
>> +                     compatible = "arm,cortex-a9";
>> +                     reg = <0>;
>> +                     clocks = <&clock 12>;
>> +                     clock-names = "cpu";
>> +
>> +                     operating-points = <
>> +                             200000  950000
>> +                             400000  975000
>> +                             500000  975000
>> +                             800000  1075000
>> +                             1000000 1150000
>> +                             1200000 1250000
>> +                     >;
>
> On the ./arch/arm/boot/dts/highbank.dts the oop's order is decreasing.
>
> That shouldn't be a problem for the cpufreq core, however in my opinion
> we shall stick to one convention.

There are other platforms like the OMAP which use increasing order but
the decreasing order seems to be the most prevalent. I will change the
order in the next version of this patch series.

Thanks,
Thomas.

>
>> +                     safe-opp-index = <3>;
>> +             };
>> +     };
>> +
>>       gic: interrupt-controller@10490000 {
>>               cpu-offset = <0x8000>;
>>       };
>
>
>
> --
> Best regards,
>
> Lukasz Majewski
>
> Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

Re: [PATCH 0/6] cpufreq: use cpufreq-cpu0 driver for exynos4210 based platforms

[Lukasz Majewski]

Hi Thomas,

I've investigated the topic for a while, so if you don't mind I will
share my thoughts :-)

> The patch series removes the use of Exynos4210 specific cpufreq driver
> and enables to use the cpufreq-cpu0 driver for Exynos4210 based
> platforms. This is being done for few reasons.
> 
> (a) The Exynos4210 cpufreq driver reads/writes clock controller
> registers bypassing the Exynos4 CCF driver which is sort of
> problematic.

Also other, already supported devices suffer from it. Namely -
Exynos4x12 and Exynos5250.

That's why the solution shall be as generic as possible to also work
with those two SoCs.

> (b) Removes the need for having clock controller
> register definitions in the cpufreq driver and also removes the need
> for statically io-remapping clock controller address space (helps in
> moving towards multiplatform kernel).
> 
> In order to use cpufreq-cpu0 driver and provide fast cpu clock
> switching during dvfs operations, the following apporach has been
> used.
> 
> (a) A new CPU clock provider type has been introduced in Samsung's CCF
>     support. This clock provider type can be a compostion of multiple
>     clock blocks such as mux, dividers and gates. Typically, in Exynos
>     platforms, there are multiple clock blocks in between the output
>     of the APLL and the CPU clock domain output. Representing these
>     mutiple clock blocks within a opaque CPU clock provider type helps
>     in reducing the time taken to perform a CPU clock frequency change
>     operation, which is generally required during DVFS operations.
>     This approach was suggested by Arnd Bergmann <arnd@arndb.de>
> during LCE-2013.
> 
> (b) A new optional safe operating point property has been introduced
>     in the cpufreq-cpu0 driver binding. On some platforms such as the
>     Samsung Exynos, a change in CPU frequency requires a change in the
>     PLL output that drives the CPU clock. A change in PLL output
>     requires the PLL output be turned off, which implies that the CPU
>     (and other components in the CPU clock domain) be supplied with
>     an alternate clock source during the time the PLL output is
> changed.
> 
>     The clock speed of this alternate clock source could be higher
>     than the clock speed of the PLL at the time of switching over to
>     the alternate clock source. This temporary increase in clock speed
>     of the CPU clock domain implies that the blocks within the CPU
>     clock domain should also be supplied with an appropriate voltage
>     supply level as required to drive the CPU clock domain components
>     at the speed of the alternative clock source. This temporary
>     voltage level required during switching of CPU clock speed is
>     called safe voltage level. And the cpufreq-cpu0 driver has been
>     modified to setup the safe voltage levels during the changes
>     in CPU clock speed.
> 
> (c) The CPU clock supply as been restructured as
>     [ Output of APLL -> Opaque CPU clock provider -> CPU clock output
> ] And with the changes in (a) and (b) above, the cpufreq-cpu0 driver
>     can now be used and can remove the use of Exynos4210 specific
>     cpufreq driver.
> 
> This approach is currently tried on Exynos4210 only. The same approach
> can be adopted for other Exynos SoCs as well. This patch series is
> based on linux-next master branch.
> 
> Thomas Abraham (6):
>   cpufreq: cpufreq-cpu0: allow optional safe voltage during frequency
> transitions clk: samsung: add infrastructure to register CPU clocks
>   clk: samsung: register cpu clock provider for exynos4210 SoC
>   cpufreq: exynos: remove Exynos4210 specific cpufreq driver support
>   arm: exynos4-dt: statically add platform device for cpufreq-cpu0
> platform driver arm: dts: add cpu nodes for Exynos4210 SoC
> 
>  .../devicetree/bindings/cpufreq/cpufreq-cpu0.txt   |    5 +
>  arch/arm/boot/dts/exynos4210-origen.dts            |    6 +
>  arch/arm/boot/dts/exynos4210-trats.dts             |    6 +
>  arch/arm/boot/dts/exynos4210-universal_c210.dts    |    6 +
>  arch/arm/boot/dts/exynos4210.dtsi                  |   22 +++
>  arch/arm/mach-exynos/mach-exynos4-dt.c             |    6 +
>  drivers/clk/samsung/clk-exynos4.c                  |   96
> ++++++++++++- drivers/clk/samsung/clk.c                          |
> 71 +++++++++ drivers/clk/samsung/clk.h                          |
> 37 +++++- drivers/cpufreq/Kconfig.arm                        |   11 --
>  drivers/cpufreq/Makefile                           |    1 -
>  drivers/cpufreq/cpufreq-cpu0.c                     |   49 ++++++-
>  drivers/cpufreq/exynos-cpufreq.c                   |    4 +-
>  drivers/cpufreq/exynos-cpufreq.h                   |    8 -
>  drivers/cpufreq/exynos4210-cpufreq.c               |  157
> -------------------- 15 files changed, 301 insertions(+), 184
> deletions(-) delete mode 100644 drivers/cpufreq/exynos4210-cpufreq.c
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe cpufreq" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

Re: [PATCH 0/6] cpufreq: use cpufreq-cpu0 driver for exynos4210 based platforms

[Thomas Abraham]

Hi Lukasz,

On Fri, Jan 10, 2014 at 4:02 PM, Lukasz Majewski <l.majewski@samsung.com> wrote:
> Hi Thomas,
>
> I've investigated the topic for a while, so if you don't mind I will
> share my thoughts :-)

Sure. Thanks for having a look at this patch series.

>
>> The patch series removes the use of Exynos4210 specific cpufreq driver
>> and enables to use the cpufreq-cpu0 driver for Exynos4210 based
>> platforms. This is being done for few reasons.
>>
>> (a) The Exynos4210 cpufreq driver reads/writes clock controller
>> registers bypassing the Exynos4 CCF driver which is sort of
>> problematic.
>
> Also other, already supported devices suffer from it. Namely -
> Exynos4x12 and Exynos5250.
>

Right, I did check the cpufreq driver of these SoCs.

> That's why the solution shall be as generic as possible to also work
> with those two SoCs.

The approach taken here is to encapsulate the source of the CPU domain
clock in a single logical clock. The implementation of this logical
clock can vary for different Exynos SoCs but the interface to register
and use this clock is common for all Exynos SoCs. If the changes in
the cpufreq-cpu0 driver are accepted, the combination of cpufreq-cpu0
and the logical clock representing the CPU clock is sufficient to
provide a generic solution for Exynos4x12 and Exynos5250 SoCs.

Thanks,
Thomas.

>
>> (b) Removes the need for having clock controller
>> register definitions in the cpufreq driver and also removes the need
>> for statically io-remapping clock controller address space (helps in
>> moving towards multiplatform kernel).
>>
>> In order to use cpufreq-cpu0 driver and provide fast cpu clock
>> switching during dvfs operations, the following apporach has been
>> used.
>>
>> (a) A new CPU clock provider type has been introduced in Samsung's CCF
>>     support. This clock provider type can be a compostion of multiple
>>     clock blocks such as mux, dividers and gates. Typically, in Exynos
>>     platforms, there are multiple clock blocks in between the output
>>     of the APLL and the CPU clock domain output. Representing these
>>     mutiple clock blocks within a opaque CPU clock provider type helps
>>     in reducing the time taken to perform a CPU clock frequency change
>>     operation, which is generally required during DVFS operations.
>>     This approach was suggested by Arnd Bergmann <arnd@arndb.de>
>> during LCE-2013.
>>
>> (b) A new optional safe operating point property has been introduced
>>     in the cpufreq-cpu0 driver binding. On some platforms such as the
>>     Samsung Exynos, a change in CPU frequency requires a change in the
>>     PLL output that drives the CPU clock. A change in PLL output
>>     requires the PLL output be turned off, which implies that the CPU
>>     (and other components in the CPU clock domain) be supplied with
>>     an alternate clock source during the time the PLL output is
>> changed.
>>
>>     The clock speed of this alternate clock source could be higher
>>     than the clock speed of the PLL at the time of switching over to
>>     the alternate clock source. This temporary increase in clock speed
>>     of the CPU clock domain implies that the blocks within the CPU
>>     clock domain should also be supplied with an appropriate voltage
>>     supply level as required to drive the CPU clock domain components
>>     at the speed of the alternative clock source. This temporary
>>     voltage level required during switching of CPU clock speed is
>>     called safe voltage level. And the cpufreq-cpu0 driver has been
>>     modified to setup the safe voltage levels during the changes
>>     in CPU clock speed.
>>
>> (c) The CPU clock supply as been restructured as
>>     [ Output of APLL -> Opaque CPU clock provider -> CPU clock output
>> ] And with the changes in (a) and (b) above, the cpufreq-cpu0 driver
>>     can now be used and can remove the use of Exynos4210 specific
>>     cpufreq driver.
>>
>> This approach is currently tried on Exynos4210 only. The same approach
>> can be adopted for other Exynos SoCs as well. This patch series is
>> based on linux-next master branch.
>>
>> Thomas Abraham (6):
>>   cpufreq: cpufreq-cpu0: allow optional safe voltage during frequency
>> transitions clk: samsung: add infrastructure to register CPU clocks
>>   clk: samsung: register cpu clock provider for exynos4210 SoC
>>   cpufreq: exynos: remove Exynos4210 specific cpufreq driver support
>>   arm: exynos4-dt: statically add platform device for cpufreq-cpu0
>> platform driver arm: dts: add cpu nodes for Exynos4210 SoC
>>
>>  .../devicetree/bindings/cpufreq/cpufreq-cpu0.txt   |    5 +
>>  arch/arm/boot/dts/exynos4210-origen.dts            |    6 +
>>  arch/arm/boot/dts/exynos4210-trats.dts             |    6 +
>>  arch/arm/boot/dts/exynos4210-universal_c210.dts    |    6 +
>>  arch/arm/boot/dts/exynos4210.dtsi                  |   22 +++
>>  arch/arm/mach-exynos/mach-exynos4-dt.c             |    6 +
>>  drivers/clk/samsung/clk-exynos4.c                  |   96
>> ++++++++++++- drivers/clk/samsung/clk.c                          |
>> 71 +++++++++ drivers/clk/samsung/clk.h                          |
>> 37 +++++- drivers/cpufreq/Kconfig.arm                        |   11 --
>>  drivers/cpufreq/Makefile                           |    1 -
>>  drivers/cpufreq/cpufreq-cpu0.c                     |   49 ++++++-
>>  drivers/cpufreq/exynos-cpufreq.c                   |    4 +-
>>  drivers/cpufreq/exynos-cpufreq.h                   |    8 -
>>  drivers/cpufreq/exynos4210-cpufreq.c               |  157
>> -------------------- 15 files changed, 301 insertions(+), 184
>> deletions(-) delete mode 100644 drivers/cpufreq/exynos4210-cpufreq.c
>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe cpufreq" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>
> --
> Best regards,
>
> Lukasz Majewski
>
> Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

Re: [PATCH 0/6] cpufreq: use cpufreq-cpu0 driver for exynos4210 based platforms

[Tomasz Figa]

On 10.01.2014 12:59, Thomas Abraham wrote:
> Hi Lukasz,
>
> On Fri, Jan 10, 2014 at 4:02 PM, Lukasz Majewski <l.majewski@samsung.com> wrote:
>> Hi Thomas,
>>
>> I've investigated the topic for a while, so if you don't mind I will
>> share my thoughts :-)
>
> Sure. Thanks for having a look at this patch series.
>
>>
>>> The patch series removes the use of Exynos4210 specific cpufreq driver
>>> and enables to use the cpufreq-cpu0 driver for Exynos4210 based
>>> platforms. This is being done for few reasons.
>>>
>>> (a) The Exynos4210 cpufreq driver reads/writes clock controller
>>> registers bypassing the Exynos4 CCF driver which is sort of
>>> problematic.
>>
>> Also other, already supported devices suffer from it. Namely -
>> Exynos4x12 and Exynos5250.
>>
>
> Right, I did check the cpufreq driver of these SoCs.
>
>> That's why the solution shall be as generic as possible to also work
>> with those two SoCs.
>
> The approach taken here is to encapsulate the source of the CPU domain
> clock in a single logical clock. The implementation of this logical
> clock can vary for different Exynos SoCs but the interface to register
> and use this clock is common for all Exynos SoCs. If the changes in
> the cpufreq-cpu0 driver are accepted, the combination of cpufreq-cpu0
> and the logical clock representing the CPU clock is sufficient to
> provide a generic solution for Exynos4x12 and Exynos5250 SoCs.

Well, looking at Exynos SoCs, the structure of core block is very 
similar on all of them. You have a single mux to select between MPLL and 
APLL and then a number of dividers that need to be configured 
quasi-atomically when CPU frequency is to be changed or, looking at 
higher level, a set of registers, which values need to be changed 
(possibly masked).

I'd say it wouldn't require much more effort to make this common at 
least for all currently supported Exynos SoCs (except 5440, which has 
completely different clock logic).

Best regards,
Tomasz

Re: [PATCH 0/6] cpufreq: use cpufreq-cpu0 driver for exynos4210 based platforms

[Thomas Abraham]

On Sun, Jan 12, 2014 at 7:56 AM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> On 10.01.2014 12:59, Thomas Abraham wrote:
>>
>> Hi Lukasz,
>>
>> On Fri, Jan 10, 2014 at 4:02 PM, Lukasz Majewski <l.majewski@samsung.com>
>> wrote:
>>>
>>> Hi Thomas,
>>>
>>> I've investigated the topic for a while, so if you don't mind I will
>>> share my thoughts :-)
>>
>>
>> Sure. Thanks for having a look at this patch series.
>>
>>>
>>>> The patch series removes the use of Exynos4210 specific cpufreq driver
>>>> and enables to use the cpufreq-cpu0 driver for Exynos4210 based
>>>> platforms. This is being done for few reasons.
>>>>
>>>> (a) The Exynos4210 cpufreq driver reads/writes clock controller
>>>> registers bypassing the Exynos4 CCF driver which is sort of
>>>> problematic.
>>>
>>>
>>> Also other, already supported devices suffer from it. Namely -
>>> Exynos4x12 and Exynos5250.
>>>
>>
>> Right, I did check the cpufreq driver of these SoCs.
>>
>>> That's why the solution shall be as generic as possible to also work
>>> with those two SoCs.
>>
>>
>> The approach taken here is to encapsulate the source of the CPU domain
>> clock in a single logical clock. The implementation of this logical
>> clock can vary for different Exynos SoCs but the interface to register
>> and use this clock is common for all Exynos SoCs. If the changes in
>> the cpufreq-cpu0 driver are accepted, the combination of cpufreq-cpu0
>> and the logical clock representing the CPU clock is sufficient to
>> provide a generic solution for Exynos4x12 and Exynos5250 SoCs.
>
>
> Well, looking at Exynos SoCs, the structure of core block is very similar on
> all of them. You have a single mux to select between MPLL and APLL and then
> a number of dividers that need to be configured quasi-atomically when CPU
> frequency is to be changed or, looking at higher level, a set of registers,
> which values need to be changed (possibly masked).
>
> I'd say it wouldn't require much more effort to make this common at least
> for all currently supported Exynos SoCs (except 5440, which has completely
> different clock logic).

Hi Tomasz,

Sorry, I did not get your point here. The core clock is meant to be
generic for all Samsung SoCs. There are simple get_rate and round_rate
generic callbacks provide which a core implementation can optionally
use. Only the set_rate callback tend to be specific for a core clock.
And that will also be reused across SoC as applicable. For instance,
the clock blocks that make up the ARM domain core clock of Exynos5420
is different from that in Exynos4210.

Thanks,
Thomas.

>
> Best regards,
> Tomasz