From: Thomas Abraham <hidden> Date: 2014-05-14 01:11:08
Changes since v3:
- Addressed comments from Tomasz Figa [off-list ref]
[http://www.spinics.net/lists/cpufreq/msg09290.html]
- Rebased to v3.15-rc4
Changes since v2:
- Safe operating voltage is not required while switching APLL frequency
since the temporary parent's clock is divided down to keep armclk within
permissible limits. Thanks to Heiko St?bner [off-list ref] for this
suggesting this.
- Rob had suggested to use max frequency for each of the divider clock
outputs instead of divider values. But due to certain SoC specific
characteristics, the divider values corresponding to the input clock
frequency for the CMU_CPU clock blocks have to be used.
Changes since v1:
- Removes Exynos4x12 and Exynos5250 cpufreq driver also.
- Device tree based clock configuration lookup as suggested by Lukasz
Majewski and Tomasz Figa.
- safe operating point binding reworked as suggested by Shawn Guo.
The patch series removes the use of Exynos specific cpufreq driver and enables
the use of cpufreq-cpu0 driver for Exynos4210, Exynos4x12 and Exynos5250 based
platforms. This is being done for few reasons.
(a) The Exynos cpufreq driver reads/writes clock controller registers
bypassing the Exynos 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).
Thomas Abraham (8):
cpufreq: cpufreq-cpu0: allow use of optional boost mode frequencies
clk: samsung: change scope of samsung clock lock to global
clk: samsung: add infrastructure to register cpu clocks
Documentation: devicetree: add cpu clock configuration data binding for Exynos4/5
clk: exynos: use cpu-clock provider type to represent arm clock
ARM: dts: Exynos: add cpu nodes, opp and cpu clock configuration data
ARM: Exynos: switch to using generic cpufreq-cpu0 driver
cpufreq: exynos: remove all exynos specific cpufreq driver support
.../devicetree/bindings/clock/exynos4-clock.txt | 37 ++
.../devicetree/bindings/clock/exynos5250-clock.txt | 36 ++
.../devicetree/bindings/cpufreq/cpufreq-cpu0.txt | 2 +
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 | 35 ++
arch/arm/boot/dts/exynos4212.dtsi | 18 +
arch/arm/boot/dts/exynos4412-odroidx.dts | 6 +
arch/arm/boot/dts/exynos4412-origen.dts | 6 +
arch/arm/boot/dts/exynos4412-trats2.dts | 6 +
arch/arm/boot/dts/exynos4412.dtsi | 31 ++
arch/arm/boot/dts/exynos4x12.dtsi | 36 ++
arch/arm/boot/dts/exynos5250-arndale.dts | 6 +
arch/arm/boot/dts/exynos5250-cros-common.dtsi | 6 +
arch/arm/boot/dts/exynos5250-smdk5250.dts | 6 +
arch/arm/boot/dts/exynos5250.dtsi | 41 ++
arch/arm/mach-exynos/exynos.c | 4 +-
drivers/clk/samsung/Makefile | 2 +-
drivers/clk/samsung/clk-cpu.c | 458 ++++++++++++++++++++
drivers/clk/samsung/clk-exynos4.c | 25 +-
drivers/clk/samsung/clk-exynos5250.c | 12 +-
drivers/clk/samsung/clk.c | 13 +-
drivers/clk/samsung/clk.h | 7 +
drivers/cpufreq/Kconfig | 11 +
drivers/cpufreq/Kconfig.arm | 52 ---
drivers/cpufreq/Makefile | 4 -
drivers/cpufreq/cpufreq-cpu0.c | 5 +
drivers/cpufreq/exynos-cpufreq.c | 209 ---------
drivers/cpufreq/exynos-cpufreq.h | 91 ----
drivers/cpufreq/exynos4210-cpufreq.c | 157 -------
drivers/cpufreq/exynos4x12-cpufreq.c | 211 ---------
drivers/cpufreq/exynos5250-cpufreq.c | 183 --------
include/dt-bindings/clock/exynos5250.h | 1 +
34 files changed, 799 insertions(+), 936 deletions(-)
create mode 100644 drivers/clk/samsung/clk-cpu.c
delete mode 100644 drivers/cpufreq/exynos-cpufreq.c
delete mode 100644 drivers/cpufreq/exynos-cpufreq.h
delete mode 100644 drivers/cpufreq/exynos4210-cpufreq.c
delete mode 100644 drivers/cpufreq/exynos4x12-cpufreq.c
delete mode 100644 drivers/cpufreq/exynos5250-cpufreq.c
--
1.7.4.4
From: Thomas Abraham <hidden> Date: 2014-05-14 01:11:09
From: Thomas Abraham <redacted>
Lookup for the optional boost-frequency property in cpu0 node and if
available, enable support for boost mode frequencies. The frequencies
usable in boost mode are determined while preparing the cpufreq table
from the list of operating points available.
Cc: Shawn Guo <redacted>
Cc: Lukasz Majewski <redacted>
Signed-off-by: Thomas Abraham <redacted>
---
.../devicetree/bindings/cpufreq/cpufreq-cpu0.txt | 2 ++
drivers/cpufreq/Kconfig | 11 +++++++++++
drivers/cpufreq/cpufreq-cpu0.c | 5 +++++
3 files changed, 18 insertions(+), 0 deletions(-)
@@ -194,6 +194,17 @@ config GENERIC_CPUFREQ_CPU0Ifindoubt,sayN.+configGENERIC_CPUFREQ_CPU0_BOOST+bool"Boost frequency support for generic CPU0 cpufreq driver"+depends onGENERIC_CPUFREQ_CPU0+selectCPU_FREQ_BOOST_SW+help+Thisenablessupportforsoftwaremanagedoverclocking(BOOST).It+allowsusageofspecialfrequencies(thosebeyondthenormaloperating+frequencies).++Ifindoubt,sayN.+menu"x86 CPU frequency scaling drivers"depends onX86source"drivers/cpufreq/Kconfig.x86"
On 14 May 2014 06:41, Thomas Abraham [off-list ref] wrote:
quoted hunk
From: Thomas Abraham <redacted>
Lookup for the optional boost-frequency property in cpu0 node and if
available, enable support for boost mode frequencies. The frequencies
usable in boost mode are determined while preparing the cpufreq table
from the list of operating points available.
Cc: Shawn Guo <redacted>
Cc: Lukasz Majewski <redacted>
Signed-off-by: Thomas Abraham <redacted>
---
.../devicetree/bindings/cpufreq/cpufreq-cpu0.txt | 2 ++
drivers/cpufreq/Kconfig | 11 +++++++++++
drivers/cpufreq/cpufreq-cpu0.c | 5 +++++
3 files changed, 18 insertions(+), 0 deletions(-)
+ bool "Boost frequency support for generic CPU0 cpufreq driver"
+ depends on GENERIC_CPUFREQ_CPU0
+ select CPU_FREQ_BOOST_SW
+ help
+ This enables support for software managed overclocking (BOOST). It
+ allows usage of special frequencies (those beyond the normal operating
+ frequencies).
+
+ If in doubt, say N.
+
menu "x86 CPU frequency scaling drivers"
depends on X86
source "drivers/cpufreq/Kconfig.x86"
On 14 May 2014 06:41, Thomas Abraham [off-list ref] wrote:
quoted
From: Thomas Abraham <redacted>
Lookup for the optional boost-frequency property in cpu0 node and if
available, enable support for boost mode frequencies. The
frequencies usable in boost mode are determined while preparing the
cpufreq table from the list of operating points available.
Cc: Shawn Guo <redacted>
Cc: Lukasz Majewski <redacted>
Signed-off-by: Thomas Abraham <redacted>
---
.../devicetree/bindings/cpufreq/cpufreq-cpu0.txt | 2 ++
drivers/cpufreq/Kconfig | 11
+++++++++++ drivers/cpufreq/cpufreq-cpu0.c |
5 +++++ 3 files changed, 18 insertions(+), 0 deletions(-)
diff --git
a/Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt
b/Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt index
f055515..60f321a 100644 ---
a/Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt +++
b/Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt @@
-19,6 +19,8 @@ Optional properties:
- cooling-min-level:
- cooling-max-level:
Please refer to
Documentation/devicetree/bindings/thermal/thermal.txt. +-
boost-frequency:
+ Please refer to
Documentation/devicetree/bindings/cpufreq/cpufreq-boost.txt
Examples:
I think that you can rely solely on the CPU_FREQ_BOOST_SW flag if the
original author of cpufreq-cpu0.c don't mind.
quoted
+ bool "Boost frequency support for generic CPU0 cpufreq
driver"
+ depends on GENERIC_CPUFREQ_CPU0
+ select CPU_FREQ_BOOST_SW
+ help
+ This enables support for software managed overclocking
(BOOST). It
+ allows usage of special frequencies (those beyond the
normal operating
+ frequencies).
+
+ If in doubt, say N.
+
menu "x86 CPU frequency scaling drivers"
depends on X86
source "drivers/cpufreq/Kconfig.x86"
diff --git a/drivers/cpufreq/cpufreq-cpu0.c
b/drivers/cpufreq/cpufreq-cpu0.c index 1bf6bba..e6c21a8 100644
From: Thomas Abraham <hidden> Date: 2014-05-14 13:43:21
Hi Lukasz, Viresh,
On Wed, May 14, 2014 at 11:50 AM, Viresh Kumar [off-list ref] wrote:
On 14 May 2014 11:47, Lukasz Majewski [off-list ref] wrote:
quoted
I think that you can rely solely on the CPU_FREQ_BOOST_SW flag if the
original author of cpufreq-cpu0.c don't mind.
We don't have to. struct cpufreq_driver: boost_supported is always available.
Just update it without any ugly #ifdefs ..
Okay, so you are suggesting that Kconfig entry used to select
cpufreq-cpu0 should also enable CPU_FREQ_BOOST_SW config as well.
Considering that exynos4412 is the only user of this feature in
mainline, should we do this?
From: Thomas Abraham <hidden> Date: 2014-05-14 14:18:06
On Wed, May 14, 2014 at 7:20 PM, Viresh Kumar [off-list ref] wrote:
On 14 May 2014 19:13, Thomas Abraham [off-list ref] wrote:
quoted
Okay, so you are suggesting that Kconfig entry used to select
cpufreq-cpu0 should also enable CPU_FREQ_BOOST_SW config as well.
I never said that... That should be enabled by your defconfig instead.
What I said was, don't put code within any #ifdefs in cpufreq-cpu0.
Okay, I get the point about #ifdef in cpufreq-cpu0. What about your
earlier comment about the new Kconfig entry ("Get rid of this.. just
not required.")? The new Kconfig entry would be needed in that case.
On 14 May 2014 19:48, Thomas Abraham [off-list ref] wrote:
Okay, I get the point about #ifdef in cpufreq-cpu0. What about your
earlier comment about the new Kconfig entry ("Get rid of this.. just
not required.")? The new Kconfig entry would be needed in that case.
The only place you used that ifdef was in cpufreq-cpu0, which we
already decided should stay without any ifdefs..
Now, you must enable CPU_FREQ_BOOST_SW from your defconfig.
Did I make it clear this time ?
--
viresh
From: Thomas Abraham <hidden> Date: 2014-05-14 01:11:10
From: Thomas Abraham <redacted>
Access to samsung clock lock is required to support newer samsung specific
clock types. So change the scope of the samsung clock lock to global. And
prefix 'samsung_clk_' to the existing name of the lock to prevent name space
pollution.
Cc: Tomasz Figa <redacted>
Signed-off-by: Thomas Abraham <redacted>
---
drivers/clk/samsung/clk.c | 13 ++++++++-----
drivers/clk/samsung/clk.h | 2 ++
2 files changed, 10 insertions(+), 5 deletions(-)
On 14 May 2014 06:41, Thomas Abraham [off-list ref] wrote:
From: Thomas Abraham <redacted>
Access to samsung clock lock is required to support newer samsung specific
clock types. So change the scope of the samsung clock lock to global. And
Maybe bit more detail on why exactly it should be available outside this file.
From: Thomas Abraham <hidden> Date: 2014-05-14 13:26:43
On Wed, May 14, 2014 at 9:20 AM, Viresh Kumar [off-list ref] wrote:
On 14 May 2014 06:41, Thomas Abraham [off-list ref] wrote:
quoted
From: Thomas Abraham <redacted>
Access to samsung clock lock is required to support newer samsung specific
clock types. So change the scope of the samsung clock lock to global. And
Maybe bit more detail on why exactly it should be available outside this file.
From: Tomasz Figa <hidden> Date: 2014-05-16 12:30:55
Hi Thomas,
On 14.05.2014 03:11, Thomas Abraham wrote:
quoted hunk
From: Thomas Abraham <redacted>
Access to samsung clock lock is required to support newer samsung specific
clock types. So change the scope of the samsung clock lock to global. And
prefix 'samsung_clk_' to the existing name of the lock to prevent name space
pollution.
Cc: Tomasz Figa <redacted>
Signed-off-by: Thomas Abraham <redacted>
---
drivers/clk/samsung/clk.c | 13 ++++++++-----
drivers/clk/samsung/clk.h | 2 ++
2 files changed, 10 insertions(+), 5 deletions(-)
I'm afraid this series needs to be rebased on top of samsung-clk-next
branch of Samsung clock tree:
git://git.kernel.org/pub/scm/linux/kernel/git/tfiga/samsung-clk.git
You can find there patches adding Samsung clock context struct, which
should be passed to Samsung clock helper functions and which replaces
the global variables above.
So changes introduced by this patch will be no longer necessary, as you
will be able to just pass the context struct to registration function of
your clock type.
Best regards,
Tomasz
From: Thomas Abraham <hidden> Date: 2014-05-14 01:11:11
From: Thomas Abraham <redacted>
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.
Cc: Tomasz Figa <redacted>
Signed-off-by: Thomas Abraham <redacted>
---
drivers/clk/samsung/Makefile | 2 +-
drivers/clk/samsung/clk-cpu.c | 458 +++++++++++++++++++++++++++++++++++++++++
drivers/clk/samsung/clk.h | 5 +
3 files changed, 464 insertions(+), 1 deletions(-)
create mode 100644 drivers/clk/samsung/clk-cpu.c
@@ -0,0 +1,458 @@+/*+*Copyright(c)2014SamsungElectronicsCo.,Ltd.+*Author:ThomasAbraham<thomas.ab@samsung.com>+*+*Thisprogramisfreesoftware;youcanredistributeitand/ormodify+*itunderthetermsoftheGNUGeneralPublicLicenseversion2as+*publishedbytheFreeSoftwareFoundation.+*+*Thisfilecontainstheutilityfunctionstoregisterthecpuclocks+*forsamsungplatforms.+*/++#include<linux/errno.h>+#include"clk.h"++#define SRC_CPU 0x0+#define STAT_CPU 0x200+#define DIV_CPU0 0x300+#define DIV_CPU1 0x304+#define DIV_STAT_CPU0 0x400+#define DIV_STAT_CPU1 0x404++#define MAX_DIV 8++#define EXYNOS4210_ARM_DIV1(div) ((div & 0x7) + 1)+#define EXYNOS4210_ARM_DIV2(div) (((div >> 28) & 0x7) + 1)++#define EXYNOS4210_DIV_CPU0(d5, d4, d3, d2, d1, d0) \+((d5<<24)|(d4<<20)|(d3<<16)|(d2<<12)|\+(d1<<8)|(d0<<4))+#define EXYNOS4210_DIV_CPU1(d2, d1, d0) \+((d2<<8)|(d1<<4)|(d0<<0))++#define EXYNOS4210_DIV1_HPM_MASK ((0x7 << 0) | (0x7 << 4))+#define EXYNOS4210_MUX_HPM_MASK (1 << 20)++/**+*structexynos4210_armclk_data:configdatatosetupexynos4210cpuclocks.+*@prate:frequencyoftheparentclock.+*@div0:valuetobeprogrammedinthediv_cpu0register.+*@div1:valuetobeprogrammedinthediv_cpu1register.+*+*Thisstructureholdsthedividerconfigurationdatafordividerclocks+*belongingtotheCMU_CPUclockdomain.Theparentfrequency@whichthese+*dividervaluesarevaildisspecifiedin@prate.+*/+structexynos4210_armclk_data{+unsignedlongprate;+unsignedintdiv0;+unsignedintdiv1;+};++/**+*structexynos_cpuclk:informationaboutclocksuppliedtoaCPUcore.+*@hw:handlebetweenccfandcpuclock.+*@alt_parent:alternateparentclocktousewhenswitchingthespeed+*oftheprimaryparentclock.+*@ctrl_base:baseaddressoftheclockcontroller.+*@offset:offsetfromthectrl_baseaddresswherethecpuclockdiv/mux+*registerscanbeaccessed.+*@clk_nb:clocknotifierregisteredforchangesinclockspeedofthe+*primaryparentclock.+*@lock:registeraccesslock.+*@data:optionaldatawhichtheacutalinstantiationofthisclock+*canuse.+*/+structexynos_cpuclk{+structclk_hwhw;+structclk*alt_parent;+void__iomem*ctrl_base;+unsignedlongoffset;+structnotifier_blockclk_nb;+spinlock_t*lock;+void*data;+};++#define to_exynos_cpuclk_hw(hw) container_of(hw, struct exynos_cpuclk, hw)+#define to_exynos_cpuclk_nb(nb) container_of(nb, struct exynos_cpuclk, clk_nb)++/**+*structexynos_cpuclk_soc_data:socspecificdataforcpuclocks.+*@parser:pointertoafunctionthatcanparseSoCspecificdata.+*@ops:clockoperationstobeusedforthisclock.+*@offset:optionaloffsetfrombaseofclockcontrollerregisterbase,to+*beusedwhenaccessingclockcontrollerregistersrelatedtothe+*cpuclock.+*@clk_cb:theclocknotifiercallbacktobecalledforchangesinthe+*clockrateoftheprimaryparentclock.+*+*ThisstructureprovidesSoCspecificdataforARMclocks.Basedon+*thecompatiblevalueoftheclockcontrollernode,thevalueofthe+*fieldsinthisstructurecanbepopulated.+*/+structexynos_cpuclk_soc_data{+int(*parser)(structdevice_node*,void**);+conststructclk_ops*ops;+unsignedintoffset;+int(*clk_cb)(structnotifier_block*nb,unsignedlongevt,void*data);+};++/* common round rate callback useable for all types of cpu clocks */+staticlongexynos_cpuclk_round_rate(structclk_hw*hw,+unsignedlongdrate,unsignedlong*prate)+{+structclk*parent=__clk_get_parent(hw->clk);+unsignedlongmax_prate=__clk_round_rate(parent,UINT_MAX);+unsignedlongt_prate,div=1,best_div=1;+unsignedlongdelta,min_delta=UINT_MAX;++do{+t_prate=__clk_round_rate(parent,drate*div);+delta=drate-(t_prate/div);+if(delta<min_delta){+*prate=t_prate;+best_div=div;+min_delta=delta;+}+if(!delta)+break;+div++;+}while((drate*div)<max_prate&&div<=MAX_DIV);++return*prate/best_div;+}++staticunsignedlong_calc_div(unsignedlongprate,unsignedlongdrate)+{+unsignedlongdiv=prate/drate;++WARN_ON(div>=MAX_DIV);+return(!(prate%drate))?div--:div;+}++/* helper function to register a cpu clock */+staticint__initexynos_cpuclk_register(unsignedintlookup_id,+constchar*name,constchar**parents,+unsignedintnum_parents,void__iomem*base,+conststructexynos_cpuclk_soc_data*soc_data,+structdevice_node*np,conststructclk_ops*ops,+spinlock_t*lock)+{+structexynos_cpuclk*cpuclk;+structclk_init_datainit;+structclk*clk;+intret;++cpuclk=kzalloc(sizeof(*cpuclk),GFP_KERNEL);+if(!cpuclk){+pr_err("%s: could not allocate memory for %s clock\n",+__func__,name);+return-ENOMEM;+}++init.name=name;+init.flags=CLK_SET_RATE_PARENT;+init.parent_names=parents;+init.num_parents=1;+init.ops=ops;++cpuclk->hw.init=&init;+cpuclk->ctrl_base=base;+cpuclk->lock=lock;++ret=soc_data->parser(np,&cpuclk->data);+if(ret){+pr_err("%s: error %d in parsing %s clock data",+__func__,ret,name);+ret=-EINVAL;+gotofree_cpuclk;+}+cpuclk->offset=soc_data->offset;+init.ops=soc_data->ops;++cpuclk->clk_nb.notifier_call=soc_data->clk_cb;+if(clk_notifier_register(__clk_lookup(parents[0]),&cpuclk->clk_nb)){+pr_err("%s: failed to register clock notifier for %s\n",+__func__,name);+gotofree_cpuclk_data;+}++cpuclk->alt_parent=__clk_lookup(parents[1]);+if(!cpuclk->alt_parent){+pr_err("%s: could not lookup alternate parent %s\n",+__func__,parents[1]);+ret=-EINVAL;+gotofree_cpuclk_data;+}++clk=clk_register(NULL,&cpuclk->hw);+if(IS_ERR(clk)){+pr_err("%s: could not register cpuclk %s\n",__func__,name);+ret=PTR_ERR(clk);+gotofree_cpuclk_data;+}++samsung_clk_add_lookup(clk,lookup_id);+return0;++free_cpuclk_data:+kfree(cpuclk->data);+free_cpuclk:+kfree(cpuclk);+returnret;+}++staticvoid_exynos4210_set_armclk_div(void__iomem*base,unsignedlongdiv)+{+unsignedlongtimeout=jiffies+msecs_to_jiffies(10);++writel((readl(base+DIV_CPU0)&~0x7)|div,base+DIV_CPU0);+while(time_before(jiffies,timeout))+if(!readl(base+DIV_STAT_CPU0))+return;+pr_err("%s: timeout in divider stablization\n",__func__);+}++staticunsignedlongexynos4210_armclk_recalc_rate(structclk_hw*hw,+unsignedlongparent_rate)+{+structexynos_cpuclk*armclk=to_exynos_cpuclk_hw(hw);+void__iomem*base=armclk->ctrl_base+armclk->offset;+unsignedlongdiv0=readl(base+DIV_CPU0);++returnparent_rate/EXYNOS4210_ARM_DIV1(div0)/+EXYNOS4210_ARM_DIV2(div0);+}++staticintexynos4210_armclk_pre_rate_change(structclk_notifier_data*ndata,+structexynos_cpuclk*armclk,void__iomem*base)+{+structexynos4210_armclk_data*armclk_data=armclk->data;+unsignedlongalt_prate=clk_get_rate(armclk->alt_parent);+unsignedlongalt_div,div0,div1,tdiv0,mux_reg;+unsignedlongcur_armclk_rate,timeout;+unsignedlongflags;++/* find out the divider values to use for clock data */+while(armclk_data->prate!=ndata->new_rate){+if(armclk_data->prate==0)+return-EINVAL;+armclk_data++;+}++div0=armclk_data->div0;+div1=armclk_data->div1;+if(readl(base+SRC_CPU)&EXYNOS4210_MUX_HPM_MASK){+div1=readl(base+DIV_CPU1)&EXYNOS4210_DIV1_HPM_MASK;+div1|=((armclk_data->div1)&~EXYNOS4210_DIV1_HPM_MASK);+}++/*+*ifthenewandoldparentclockspeedislessthantheclockspeed+*ofthealternateparent,thenitshouldbeensuredthatatnopoint+*thearmclkspeedismorethantheold_prateuntilthedividersare+*set.+*/+tdiv0=readl(base+DIV_CPU0);+cur_armclk_rate=ndata->old_rate/EXYNOS4210_ARM_DIV1(tdiv0)/+EXYNOS4210_ARM_DIV2(tdiv0);+if(alt_prate>cur_armclk_rate){+alt_div=_calc_div(alt_prate,cur_armclk_rate);+_exynos4210_set_armclk_div(base,alt_div);+div0|=alt_div;+}++/* select sclk_mpll as the alternate parent */+spin_lock_irqsave(armclk->lock,flags);+mux_reg=readl(base+SRC_CPU);+writel(mux_reg|(1<<16),base+SRC_CPU);++timeout=jiffies+msecs_to_jiffies(10);+while(time_before(jiffies,timeout))+if(((readl(base+STAT_CPU)>>16)&0x7)==2)+break;+spin_unlock_irqrestore(armclk->lock,flags);++if(((readl(base+STAT_CPU)>>16)&0x7)!=2)+pr_err("%s: re-parenting to sclk_mpll failed\n",__func__);++/* alternate parent is active now. set the dividers */+writel(div0,base+DIV_CPU0);+timeout=jiffies+msecs_to_jiffies(10);+while(time_before(jiffies,timeout))+if(!readl(base+DIV_STAT_CPU0))+break;++if(readl(base+DIV_STAT_CPU0))+pr_err("%s: timeout in divider0 stablization\n",__func__);++writel(div1,base+DIV_CPU1);+timeout=jiffies+msecs_to_jiffies(10);+while(time_before(jiffies,timeout))+if(!readl(base+DIV_STAT_CPU1))+break;+if(readl(base+DIV_STAT_CPU1))+pr_err("%s: timeout in divider1 stablization\n",__func__);++return0;+}++staticintexynos4210_armclk_post_rate_change(structexynos_cpuclk*armclk,+void__iomem*base)+{+unsignedlongmux_reg,flags;+unsignedlongtimeout=jiffies+msecs_to_jiffies(10);++spin_lock_irqsave(armclk->lock,flags);+mux_reg=readl(base+SRC_CPU);+writel(mux_reg&~(1<<16),base+SRC_CPU);+while(time_before(jiffies,timeout))+if(((readl(base+STAT_CPU)>>16)&0x7)==1)+break;+spin_unlock_irqrestore(armclk->lock,flags);++if(((readl(base+STAT_CPU)>>16)&0x7)!=1)+pr_err("%s: re-parenting to mout_apll failed\n",__func__);++return0;+}++/*+*Thisclocknotifieriscalledwhenthefrequencyoftheparentclock+*ofarmclkistobechanged.Thisnotifierhandlesthesettingupall+*thedividerclocks,remuxtotemporaryparentandhandlingthesafe+*frequencylevelswhenusingtemporaryparent.+*/+staticintexynos4210_armclk_notifier_cb(structnotifier_block*nb,+unsignedlongevent,void*data)+{+structclk_notifier_data*ndata=data;+structexynos_cpuclk*armclk=to_exynos_cpuclk_nb(nb);+void__iomem*base=armclk->ctrl_base+armclk->offset;+interr=0;++if(event==PRE_RATE_CHANGE)+err=exynos4210_armclk_pre_rate_change(ndata,armclk,base);+elseif(event==POST_RATE_CHANGE)+err=exynos4210_armclk_post_rate_change(armclk,base);++returnnotifier_from_errno(err);+}++staticintexynos4210_armclk_set_rate(structclk_hw*hw,unsignedlongdrate,+unsignedlongprate)+{+structexynos_cpuclk*armclk=to_exynos_cpuclk_hw(hw);+void__iomem*base=armclk->ctrl_base+armclk->offset;+unsignedlongdiv;++div=drate<prate?_calc_div(prate,drate):0;+_exynos4210_set_armclk_div(base,div);+return0;+}++staticconststructclk_opsexynos4210_armclk_clk_ops={+.recalc_rate=exynos4210_armclk_recalc_rate,+.round_rate=exynos_cpuclk_round_rate,+.set_rate=exynos4210_armclk_set_rate,+};++/*+*parsedividerconfigurationdatafromdtforallthecpuclockdomain+*clocksinexynos4210andcompatibleSoC's.+*/+staticint__initexynos4210_armclk_parser(structdevice_node*np,void**data)+{+structexynos4210_armclk_data*tdata;+u32cfg[10],num_rows,row,col;+structproperty*prop;+const__be32*ptr=NULL;+u32cells;+intret;++if(of_property_read_u32(np,"samsung,armclk-cells",&cells))+return-EINVAL;+prop=of_find_property(np,"samsung,armclk-divider-table",NULL);+if(!prop)+return-EINVAL;+if(!prop->value)+return-EINVAL;+if((prop->length/sizeof(u32))%cells)+return-EINVAL;+num_rows=(prop->length/sizeof(u32))/cells;++/* allocate a zero terminated table */+*data=kzalloc(sizeof(*tdata)*(num_rows+1),GFP_KERNEL);+if(!*data)+ret=-ENOMEM;+tdata=*data;++for(row=0;row<num_rows;row++,tdata++){+for(col=0;col<cells;col++)+ptr=of_prop_next_u32(prop,ptr,&cfg[col]);++tdata->prate=cfg[0]*1000;+tdata->div0=EXYNOS4210_DIV_CPU0(cfg[6],cfg[5],cfg[4],+cfg[3],cfg[2],cfg[1]);+tdata->div1=cells==10?+EXYNOS4210_DIV_CPU1(cfg[9],cfg[8],cfg[7]):+EXYNOS4210_DIV_CPU1(0,cfg[8],cfg[7]);+}+tdata->prate=0;+return0;+}++staticconststructexynos_cpuclk_soc_dataexynos4210_cpuclk_soc_data={+.parser=exynos4210_armclk_parser,+.ops=&exynos4210_armclk_clk_ops,+.offset=0x14200,+.clk_cb=exynos4210_armclk_notifier_cb,+};++staticconststructexynos_cpuclk_soc_dataexynos5250_cpuclk_soc_data={+.parser=exynos4210_armclk_parser,+.ops=&exynos4210_armclk_clk_ops,+.offset=0x200,+.clk_cb=exynos4210_armclk_notifier_cb,+};++staticconststructof_device_idexynos_clock_ids_armclk[]={+{.compatible="samsung,exynos4210-clock",+.data=&exynos4210_cpuclk_soc_data,},+{.compatible="samsung,exynos4412-clock",+.data=&exynos4210_cpuclk_soc_data,},+{.compatible="samsung,exynos5250-clock",+.data=&exynos5250_cpuclk_soc_data,},+{},+};++/**+*exynos_register_arm_clock:registerarmclockwithccf.+*@lookup_id:armclkclockoutputidfortheclockcontroller.+*@parent_names:nameoftheparentclockforarmclk.+*@num_parents:numberofparentsinthe@parent_namesarray.+*@base:baseaddressoftheclockcontrollerfromwhicharmclkisgenerated.+*@np:devicetreenodepointeroftheclockcontroller(optional).+*@ops:clockopsforthisclock(optional).+*@lock:registeraccesslock.+*/+int__initexynos_register_arm_clock(unsignedintlookup_id,+constchar**parent_names,unsignedintnum_parents,+void__iomem*base,structdevice_node*np,structclk_ops*ops,+spinlock_t*lock)+{+conststructof_device_id*match;+conststructexynos_cpuclk_soc_data*data=NULL;++if(!np)+return-EINVAL;++match=of_match_node(exynos_clock_ids_armclk,np);+if(!match)+return-EINVAL;++data=match->data;+returnexynos_cpuclk_register(lookup_id,"armclk",parent_names,+num_parents,base,data,np,ops,lock);+}
From: Doug Anderson <dianders@chromium.org> Date: 2014-05-15 18:18:44
Thomas,
On Tue, May 13, 2014 at 6:11 PM, Thomas Abraham [off-list ref] wrote:
From: Thomas Abraham <redacted>
+static int exynos4210_armclk_pre_rate_change(struct clk_notifier_data *ndata,
+ struct exynos_cpuclk *armclk, void __iomem *base)
+{
+ struct exynos4210_armclk_data *armclk_data = armclk->data;
+ unsigned long alt_prate = clk_get_rate(armclk->alt_parent);
+ unsigned long alt_div, div0, div1, tdiv0, mux_reg;
+ unsigned long cur_armclk_rate, timeout;
+ unsigned long flags;
+
+ /* find out the divider values to use for clock data */
+ while (armclk_data->prate != ndata->new_rate) {
+ if (armclk_data->prate == 0)
+ return -EINVAL;
+ armclk_data++;
+ }
+
+ div0 = armclk_data->div0;
+ div1 = armclk_data->div1;
+ if (readl(base + SRC_CPU) & EXYNOS4210_MUX_HPM_MASK) {
+ div1 = readl(base + DIV_CPU1) & EXYNOS4210_DIV1_HPM_MASK;
+ div1 |= ((armclk_data->div1) & ~EXYNOS4210_DIV1_HPM_MASK);
+ }
+
+ /*
+ * if the new and old parent clock speed is less than the clock speed
+ * of the alternate parent, then it should be ensured that at no point
+ * the armclk speed is more than the old_prate until the dividers are
+ * set.
+ */
+ tdiv0 = readl(base + DIV_CPU0);
+ cur_armclk_rate = ndata->old_rate / EXYNOS4210_ARM_DIV1(tdiv0) /
+ EXYNOS4210_ARM_DIV2(tdiv0);
+ if (alt_prate > cur_armclk_rate) {
+ alt_div = _calc_div(alt_prate, cur_armclk_rate);
+ _exynos4210_set_armclk_div(base, alt_div);
+ div0 |= alt_div;
Don't you need to up the voltage here, too? ...I haven't reviewed
this whole patch (so perhaps it's elsewhere in the patch or in the
series), but I stumbled upon this while trying to solve a different
problem and figured I'd check...
-Doug
Am Donnerstag, 15. Mai 2014, 11:18:44 schrieb Doug Anderson:
Thomas,
On Tue, May 13, 2014 at 6:11 PM, Thomas Abraham [off-list ref] wrote:
quoted
From: Thomas Abraham <redacted>
+static int exynos4210_armclk_pre_rate_change(struct clk_notifier_data
*ndata, + struct exynos_cpuclk *armclk, void
__iomem *base) +{
+ struct exynos4210_armclk_data *armclk_data = armclk->data;
+ unsigned long alt_prate = clk_get_rate(armclk->alt_parent);
+ unsigned long alt_div, div0, div1, tdiv0, mux_reg;
+ unsigned long cur_armclk_rate, timeout;
+ unsigned long flags;
+
+ /* find out the divider values to use for clock data */
+ while (armclk_data->prate != ndata->new_rate) {
+ if (armclk_data->prate == 0)
+ return -EINVAL;
+ armclk_data++;
+ }
+
+ div0 = armclk_data->div0;
+ div1 = armclk_data->div1;
+ if (readl(base + SRC_CPU) & EXYNOS4210_MUX_HPM_MASK) {
+ div1 = readl(base + DIV_CPU1) & EXYNOS4210_DIV1_HPM_MASK;
+ div1 |= ((armclk_data->div1) & ~EXYNOS4210_DIV1_HPM_MASK);
+ }
+
+ /*
+ * if the new and old parent clock speed is less than the clock
speed + * of the alternate parent, then it should be ensured that
at no point + * the armclk speed is more than the old_prate until
the dividers are + * set.
+ */
+ tdiv0 = readl(base + DIV_CPU0);
+ cur_armclk_rate = ndata->old_rate / EXYNOS4210_ARM_DIV1(tdiv0) /
+ EXYNOS4210_ARM_DIV2(tdiv0);
+ if (alt_prate > cur_armclk_rate) {
+ alt_div = _calc_div(alt_prate, cur_armclk_rate);
+ _exynos4210_set_armclk_div(base, alt_div);
+ div0 |= alt_div;
Don't you need to up the voltage here, too? ...I haven't reviewed
this whole patch (so perhaps it's elsewhere in the patch or in the
series), but I stumbled upon this while trying to solve a different
problem and figured I'd check...
setting the voltage should be done by the cpufreq driver like cpufreq-cpu0 -
whose usage this series intents to allow.
As I've hijacked Thomas' concept for my current rockchip clock work, I've
already seen this working nicely :-) .
Heiko
From: Doug Anderson <dianders@chromium.org> Date: 2014-05-15 19:36:45
Heiko,
On Thu, May 15, 2014 at 12:17 PM, Heiko St?bner [off-list ref] wrote:
Am Donnerstag, 15. Mai 2014, 11:18:44 schrieb Doug Anderson:
quoted
Thomas,
On Tue, May 13, 2014 at 6:11 PM, Thomas Abraham [off-list ref] wrote:
quoted
From: Thomas Abraham <redacted>
+static int exynos4210_armclk_pre_rate_change(struct clk_notifier_data
*ndata, + struct exynos_cpuclk *armclk, void
__iomem *base) +{
+ struct exynos4210_armclk_data *armclk_data = armclk->data;
+ unsigned long alt_prate = clk_get_rate(armclk->alt_parent);
+ unsigned long alt_div, div0, div1, tdiv0, mux_reg;
+ unsigned long cur_armclk_rate, timeout;
+ unsigned long flags;
+
+ /* find out the divider values to use for clock data */
+ while (armclk_data->prate != ndata->new_rate) {
+ if (armclk_data->prate == 0)
+ return -EINVAL;
+ armclk_data++;
+ }
+
+ div0 = armclk_data->div0;
+ div1 = armclk_data->div1;
+ if (readl(base + SRC_CPU) & EXYNOS4210_MUX_HPM_MASK) {
+ div1 = readl(base + DIV_CPU1) & EXYNOS4210_DIV1_HPM_MASK;
+ div1 |= ((armclk_data->div1) & ~EXYNOS4210_DIV1_HPM_MASK);
+ }
+
+ /*
+ * if the new and old parent clock speed is less than the clock
speed + * of the alternate parent, then it should be ensured that
at no point + * the armclk speed is more than the old_prate until
the dividers are + * set.
+ */
+ tdiv0 = readl(base + DIV_CPU0);
+ cur_armclk_rate = ndata->old_rate / EXYNOS4210_ARM_DIV1(tdiv0) /
+ EXYNOS4210_ARM_DIV2(tdiv0);
+ if (alt_prate > cur_armclk_rate) {
+ alt_div = _calc_div(alt_prate, cur_armclk_rate);
+ _exynos4210_set_armclk_div(base, alt_div);
+ div0 |= alt_div;
Don't you need to up the voltage here, too? ...I haven't reviewed
this whole patch (so perhaps it's elsewhere in the patch or in the
series), but I stumbled upon this while trying to solve a different
problem and figured I'd check...
setting the voltage should be done by the cpufreq driver like cpufreq-cpu0 -
whose usage this series intents to allow.
As I've hijacked Thomas' concept for my current rockchip clock work, I've
already seen this working nicely :-) .
I guess I should have been more clear. I was talking more
specifically about upping the voltage as part of the mux switch in the
case that alt_prate > cur_armclk_rate.
...if you're switching from 200MHz to 300MHz and the alt_prate is
800MHz, you need to account for that fact. The code here accounts for
the fact in setting the "armclk_div", but (I don't think) it accounts
for the fact that 800MHz will need a higher voltage.
As per a separate discussion, a clean solution might be to move the
mux switching to the core of CPU_FREQ. That would have the side
effect of also making it very easy to send notifications.
-Doug
Hi Doug,
Am Donnerstag, 15. Mai 2014, 12:36:45 schrieb Doug Anderson:
On Thu, May 15, 2014 at 12:17 PM, Heiko St?bner [off-list ref] wrote:
quoted
Am Donnerstag, 15. Mai 2014, 11:18:44 schrieb Doug Anderson:
quoted
Thomas,
On Tue, May 13, 2014 at 6:11 PM, Thomas Abraham [off-list ref]
wrote:
quoted
quoted
quoted
From: Thomas Abraham <redacted>
+static int exynos4210_armclk_pre_rate_change(struct clk_notifier_data
*ndata, + struct exynos_cpuclk *armclk, void
__iomem *base) +{
+ struct exynos4210_armclk_data *armclk_data = armclk->data;
+ unsigned long alt_prate = clk_get_rate(armclk->alt_parent);
+ unsigned long alt_div, div0, div1, tdiv0, mux_reg;
+ unsigned long cur_armclk_rate, timeout;
+ unsigned long flags;
+
+ /* find out the divider values to use for clock data */
+ while (armclk_data->prate != ndata->new_rate) {
+ if (armclk_data->prate == 0)
+ return -EINVAL;
+ armclk_data++;
+ }
+
+ div0 = armclk_data->div0;
+ div1 = armclk_data->div1;
+ if (readl(base + SRC_CPU) & EXYNOS4210_MUX_HPM_MASK) {
+ div1 = readl(base + DIV_CPU1) &
EXYNOS4210_DIV1_HPM_MASK;
+ div1 |= ((armclk_data->div1) &
~EXYNOS4210_DIV1_HPM_MASK);
+ }
+
+ /*
+ * if the new and old parent clock speed is less than the clock
speed + * of the alternate parent, then it should be ensured
that
at no point + * the armclk speed is more than the old_prate
until
the dividers are + * set.
+ */
+ tdiv0 = readl(base + DIV_CPU0);
+ cur_armclk_rate = ndata->old_rate / EXYNOS4210_ARM_DIV1(tdiv0)
/
+ EXYNOS4210_ARM_DIV2(tdiv0);
+ if (alt_prate > cur_armclk_rate) {
+ alt_div = _calc_div(alt_prate, cur_armclk_rate);
+ _exynos4210_set_armclk_div(base, alt_div);
+ div0 |= alt_div;
Don't you need to up the voltage here, too? ...I haven't reviewed
this whole patch (so perhaps it's elsewhere in the patch or in the
series), but I stumbled upon this while trying to solve a different
problem and figured I'd check...
setting the voltage should be done by the cpufreq driver like cpufreq-cpu0
- whose usage this series intents to allow.
As I've hijacked Thomas' concept for my current rockchip clock work, I've
already seen this working nicely :-) .
I guess I should have been more clear. I was talking more
specifically about upping the voltage as part of the mux switch in the
case that alt_prate > cur_armclk_rate.
from earlier discussions I remember Thomas and me talked about setting a
divider to make sure that alt_prate <= cur_armclk_rate, so the voltage can
stay@its current level. I haven't looked deeply into this revision, but the
last one did exactly this.
...if you're switching from 200MHz to 300MHz and the alt_prate is
800MHz, you need to account for that fact. The code here accounts for
the fact in setting the "armclk_div", but (I don't think) it accounts
for the fact that 800MHz will need a higher voltage.
As per a separate discussion, a clean solution might be to move the
mux switching to the core of CPU_FREQ. That would have the side
effect of also making it very easy to send notifications.
I'll just wait until you all decide what the best solution is :-), but
personally I like the concept of keeping the clock logic inside the clock
driver, especially as this is not limited to setting the mux but also adapting
tightly bound child clocks and this all may not fit into a generic
implementation of a cpufreq driver.
And this is also working really nice on my rockchip platform.
Heiko
From: Doug Anderson <dianders@chromium.org> Date: 2014-05-15 20:26:11
Heiko,
On Thu, May 15, 2014 at 1:12 PM, Heiko St?bner [off-list ref] wrote:
Hi Doug,
Am Donnerstag, 15. Mai 2014, 12:36:45 schrieb Doug Anderson:
quoted
On Thu, May 15, 2014 at 12:17 PM, Heiko St?bner [off-list ref] wrote:
quoted
Am Donnerstag, 15. Mai 2014, 11:18:44 schrieb Doug Anderson:
quoted
Thomas,
On Tue, May 13, 2014 at 6:11 PM, Thomas Abraham [off-list ref]
wrote:
quoted
quoted
quoted
quoted
From: Thomas Abraham <redacted>
+static int exynos4210_armclk_pre_rate_change(struct clk_notifier_data
*ndata, + struct exynos_cpuclk *armclk, void
__iomem *base) +{
+ struct exynos4210_armclk_data *armclk_data = armclk->data;
+ unsigned long alt_prate = clk_get_rate(armclk->alt_parent);
+ unsigned long alt_div, div0, div1, tdiv0, mux_reg;
+ unsigned long cur_armclk_rate, timeout;
+ unsigned long flags;
+
+ /* find out the divider values to use for clock data */
+ while (armclk_data->prate != ndata->new_rate) {
+ if (armclk_data->prate == 0)
+ return -EINVAL;
+ armclk_data++;
+ }
+
+ div0 = armclk_data->div0;
+ div1 = armclk_data->div1;
+ if (readl(base + SRC_CPU) & EXYNOS4210_MUX_HPM_MASK) {
+ div1 = readl(base + DIV_CPU1) &
EXYNOS4210_DIV1_HPM_MASK;
+ div1 |= ((armclk_data->div1) &
~EXYNOS4210_DIV1_HPM_MASK);
+ }
+
+ /*
+ * if the new and old parent clock speed is less than the clock
speed + * of the alternate parent, then it should be ensured
that
at no point + * the armclk speed is more than the old_prate
until
the dividers are + * set.
+ */
+ tdiv0 = readl(base + DIV_CPU0);
+ cur_armclk_rate = ndata->old_rate / EXYNOS4210_ARM_DIV1(tdiv0)
/
+ EXYNOS4210_ARM_DIV2(tdiv0);
+ if (alt_prate > cur_armclk_rate) {
+ alt_div = _calc_div(alt_prate, cur_armclk_rate);
+ _exynos4210_set_armclk_div(base, alt_div);
+ div0 |= alt_div;
Don't you need to up the voltage here, too? ...I haven't reviewed
this whole patch (so perhaps it's elsewhere in the patch or in the
series), but I stumbled upon this while trying to solve a different
problem and figured I'd check...
setting the voltage should be done by the cpufreq driver like cpufreq-cpu0
- whose usage this series intents to allow.
As I've hijacked Thomas' concept for my current rockchip clock work, I've
already seen this working nicely :-) .
I guess I should have been more clear. I was talking more
specifically about upping the voltage as part of the mux switch in the
case that alt_prate > cur_armclk_rate.
from earlier discussions I remember Thomas and me talked about setting a
divider to make sure that alt_prate <= cur_armclk_rate, so the voltage can
stay at its current level. I haven't looked deeply into this revision, but the
last one did exactly this.
Ah-ha, that's a reasonable solution to this problem. I was familiar
with the old code and know that it used to set the CPUD ratio here, so
I assumed that was what Thomas's code did. ...but you're right, his
code is setting the divider here. I didn't double-check all of his
code / calculations, but he's certainly tweaking the right bits.
Nice!
quoted
...if you're switching from 200MHz to 300MHz and the alt_prate is
800MHz, you need to account for that fact. The code here accounts for
the fact in setting the "armclk_div", but (I don't think) it accounts
for the fact that 800MHz will need a higher voltage.
As per a separate discussion, a clean solution might be to move the
mux switching to the core of CPU_FREQ. That would have the side
effect of also making it very easy to send notifications.
I'll just wait until you all decide what the best solution is :-), but
personally I like the concept of keeping the clock logic inside the clock
driver, especially as this is not limited to setting the mux but also adapting
tightly bound child clocks and this all may not fit into a generic
implementation of a cpufreq driver.
Yup, I didn't think of the solution you guys came up with. Your
solution handles things nicely.
Thanks!
-Doug
From: Thomas Abraham <hidden> Date: 2014-05-16 04:55:57
On Fri, May 16, 2014 at 1:56 AM, Doug Anderson [off-list ref] wrote:
Heiko,
On Thu, May 15, 2014 at 1:12 PM, Heiko St?bner [off-list ref] wrote:
quoted
Hi Doug,
Am Donnerstag, 15. Mai 2014, 12:36:45 schrieb Doug Anderson:
quoted
On Thu, May 15, 2014 at 12:17 PM, Heiko St?bner [off-list ref] wrote:
quoted
Am Donnerstag, 15. Mai 2014, 11:18:44 schrieb Doug Anderson:
quoted
Thomas,
On Tue, May 13, 2014 at 6:11 PM, Thomas Abraham [off-list ref]
wrote:
quoted
quoted
quoted
quoted
From: Thomas Abraham <redacted>
+static int exynos4210_armclk_pre_rate_change(struct clk_notifier_data
*ndata, + struct exynos_cpuclk *armclk, void
__iomem *base) +{
+ struct exynos4210_armclk_data *armclk_data = armclk->data;
+ unsigned long alt_prate = clk_get_rate(armclk->alt_parent);
+ unsigned long alt_div, div0, div1, tdiv0, mux_reg;
+ unsigned long cur_armclk_rate, timeout;
+ unsigned long flags;
+
+ /* find out the divider values to use for clock data */
+ while (armclk_data->prate != ndata->new_rate) {
+ if (armclk_data->prate == 0)
+ return -EINVAL;
+ armclk_data++;
+ }
+
+ div0 = armclk_data->div0;
+ div1 = armclk_data->div1;
+ if (readl(base + SRC_CPU) & EXYNOS4210_MUX_HPM_MASK) {
+ div1 = readl(base + DIV_CPU1) &
EXYNOS4210_DIV1_HPM_MASK;
+ div1 |= ((armclk_data->div1) &
~EXYNOS4210_DIV1_HPM_MASK);
+ }
+
+ /*
+ * if the new and old parent clock speed is less than the clock
speed + * of the alternate parent, then it should be ensured
that
at no point + * the armclk speed is more than the old_prate
until
the dividers are + * set.
+ */
+ tdiv0 = readl(base + DIV_CPU0);
+ cur_armclk_rate = ndata->old_rate / EXYNOS4210_ARM_DIV1(tdiv0)
/
+ EXYNOS4210_ARM_DIV2(tdiv0);
+ if (alt_prate > cur_armclk_rate) {
+ alt_div = _calc_div(alt_prate, cur_armclk_rate);
+ _exynos4210_set_armclk_div(base, alt_div);
+ div0 |= alt_div;
Don't you need to up the voltage here, too? ...I haven't reviewed
this whole patch (so perhaps it's elsewhere in the patch or in the
series), but I stumbled upon this while trying to solve a different
problem and figured I'd check...
setting the voltage should be done by the cpufreq driver like cpufreq-cpu0
- whose usage this series intents to allow.
As I've hijacked Thomas' concept for my current rockchip clock work, I've
already seen this working nicely :-) .
I guess I should have been more clear. I was talking more
specifically about upping the voltage as part of the mux switch in the
case that alt_prate > cur_armclk_rate.
from earlier discussions I remember Thomas and me talked about setting a
divider to make sure that alt_prate <= cur_armclk_rate, so the voltage can
stay at its current level. I haven't looked deeply into this revision, but the
last one did exactly this.
Ah-ha, that's a reasonable solution to this problem. I was familiar
with the old code and know that it used to set the CPUD ratio here, so
I assumed that was what Thomas's code did. ...but you're right, his
code is setting the divider here. I didn't double-check all of his
code / calculations, but he's certainly tweaking the right bits.
Nice!
quoted
quoted
...if you're switching from 200MHz to 300MHz and the alt_prate is
800MHz, you need to account for that fact. The code here accounts for
the fact in setting the "armclk_div", but (I don't think) it accounts
for the fact that 800MHz will need a higher voltage.
As per a separate discussion, a clean solution might be to move the
mux switching to the core of CPU_FREQ. That would have the side
effect of also making it very easy to send notifications.
I'll just wait until you all decide what the best solution is :-), but
personally I like the concept of keeping the clock logic inside the clock
driver, especially as this is not limited to setting the mux but also adapting
tightly bound child clocks and this all may not fit into a generic
implementation of a cpufreq driver.
+1. I also prefer to go with this approach.
Yup, I didn't think of the solution you guys came up with. Your
solution handles things nicely.
Thanks!
-Doug
Doug,
Heiko had suggested to use the divider to keep the armclk within
limits during the frequency transition. This helped to avoid the
corresponding voltage scaling step which greatly simplified the
frequency scaling for exynos. We definitely plan to use the same
suggestion on other exynos platforms as well.
Thanks,
Thomas.
From: Tomasz Figa <hidden> Date: 2014-05-16 17:17:12
Hi Thomas,
On 14.05.2014 03:11, Thomas Abraham wrote:
quoted hunk
From: Thomas Abraham <redacted>
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.
Cc: Tomasz Figa <redacted>
Signed-off-by: Thomas Abraham <redacted>
---
drivers/clk/samsung/Makefile | 2 +-
drivers/clk/samsung/clk-cpu.c | 458 +++++++++++++++++++++++++++++++++++++++++
drivers/clk/samsung/clk.h | 5 +
3 files changed, 464 insertions(+), 1 deletions(-)
create mode 100644 drivers/clk/samsung/clk-cpu.c
Macro arguments should be put into parentheses to make sure that whole
argument is subject to further arithmetic operations.
+
+#define EXYNOS4210_DIV1_HPM_MASK ((0x7 << 0) | (0x7 << 4))
+#define EXYNOS4210_MUX_HPM_MASK (1 << 20)
+
+/**
+ * struct exynos4210_armclk_data: config data to setup exynos4210 cpu clocks.
+ * @prate: frequency of the parent clock.
+ * @div0: value to be programmed in the div_cpu0 register.
+ * @div1: value to be programmed in the div_cpu1 register.
+ *
+ * This structure holds the divider configuration data for divider clocks
+ * belonging to the CMU_CPU clock domain. The parent frequency at which these
+ * divider values are vaild is specified in @prate.
s/vaild/valid/
+ */
+struct exynos4210_armclk_data {
+ unsigned long prate;
+ unsigned int div0;
+ unsigned int div1;
+};
+
+/**
+ * struct exynos_cpuclk: information about clock supplied to a CPU core.
+ * @hw: handle between ccf and cpu clock.
s/ccf/CCF/
s/cpu/CPU/
+ * @alt_parent: alternate parent clock to use when switching the speed
+ * of the primary parent clock.
+ * @ctrl_base: base address of the clock controller.
+ * @offset: offset from the ctrl_base address where the cpu clock div/mux
s/cpu/CPU/
+ * registers can be accessed.
+ * @clk_nb: clock notifier registered for changes in clock speed of the
+ * primary parent clock.
+ * @lock: register access lock.
+ * @data: optional data which the acutal instantiation of this clock
+ * can use.
s/acutal/actual/
+ */
+struct exynos_cpuclk {
+ struct clk_hw hw;
+ struct clk *alt_parent;
+ void __iomem *ctrl_base;
+ unsigned long offset;
+ struct notifier_block clk_nb;
+ spinlock_t *lock;
+ void *data;
+};
+
+#define to_exynos_cpuclk_hw(hw) container_of(hw, struct exynos_cpuclk, hw)
+#define to_exynos_cpuclk_nb(nb) container_of(nb, struct exynos_cpuclk, clk_nb)
+
+/**
+ * struct exynos_cpuclk_soc_data: soc specific data for cpu clocks.
+ * @parser: pointer to a function that can parse SoC specific data.
+ * @ops: clock operations to be used for this clock.
+ * @offset: optional offset from base of clock controller register base, to
+ * be used when accessing clock controller registers related to the
+ * cpu clock.
s/cpu/CPU/
+ * @clk_cb: the clock notifier callback to be called for changes in the
+ * clock rate of the primary parent clock.
+ *
+ * This structure provides SoC specific data for ARM clocks. Based on
+ * the compatible value of the clock controller node, the value of the
+ * fields in this structure can be populated.
+ */
+struct exynos_cpuclk_soc_data {
+ int (*parser)(struct device_node *, void **);
Here you don't have argument names, but...
+ const struct clk_ops *ops;
+ unsigned int offset;
+ int (*clk_cb)(struct notifier_block *nb, unsigned long evt, void *data);
...here you have. Please keep some consistency.
+};
+
+/* common round rate callback useable for all types of cpu clocks */
s/cpu/CPU/
+static long exynos_cpuclk_round_rate(struct clk_hw *hw,
+ unsigned long drate, unsigned long *prate)
Hmm, the long return type will overflow with *prate > INT_MAX and
best_div == 1, I wonder why it is defined so in CCF, even though it
shouldn't return error codes...
+{
+ struct clk *parent = __clk_get_parent(hw->clk);
+ unsigned long max_prate = __clk_round_rate(parent, UINT_MAX);
+ unsigned long t_prate, div = 1, best_div = 1;
+ unsigned long delta, min_delta = UINT_MAX;
By the way, shouldn't this function take into account the list of
available CPU rates and round drate to a less or equal supported one?
Otherwise, in further code you might hit cases where an unsupported rate
is requested, which is against the CCF semantics, if .round_rate()
operation is provided.
+
+ do {
+ t_prate = __clk_round_rate(parent, drate * div);
+ delta = drate - (t_prate / div);
+ if (delta < min_delta) {
+ *prate = t_prate;
+ best_div = div;
+ min_delta = delta;
+ }
+ if (!delta)
+ break;
+ div++;
+ } while ((drate * div) < max_prate && div <= MAX_DIV);
+
+ return *prate / best_div;
+}
+
+static unsigned long _calc_div(unsigned long prate, unsigned long drate)
+{
+ unsigned long div = prate / drate;
+
+ WARN_ON(div >= MAX_DIV);
+ return (!(prate % drate)) ? div-- : div;
Could you explain what is the purpose of this check and adjustment?
If my assumption that this is essentially DIV_ROUND_UP(prate, drate) - 1
is true then this probably used to obtain a divisor value to get less or
equal rate than drate. Is it right?
+}
+
+/* helper function to register a cpu clock */
+static int __init exynos_cpuclk_register(unsigned int lookup_id,
+ const char *name, const char **parents,
+ unsigned int num_parents, void __iomem *base,
The num_parents argument doesn't seem to be used in the code. Maybe
instead you should simply replace it and parents arguments with (const
char *parent) and (const char *alt_parent)?
The 0x7 could be defined as a preprocessor macro. Also for increased
readability, this could be split into separate read, modify and write.
+ while (time_before(jiffies, timeout))
+ if (!readl(base + DIV_STAT_CPU0))
+ return;
+ pr_err("%s: timeout in divider stablization\n", __func__);
+}
+
+static unsigned long exynos4210_armclk_recalc_rate(struct clk_hw *hw,
+ unsigned long parent_rate)
+{
+ struct exynos_cpuclk *armclk = to_exynos_cpuclk_hw(hw);
+ void __iomem *base = armclk->ctrl_base + armclk->offset;
+ unsigned long div0 = readl(base + DIV_CPU0);
+
+ return parent_rate / EXYNOS4210_ARM_DIV1(div0) /
+ EXYNOS4210_ARM_DIV2(div0);
+}
+
+static int exynos4210_armclk_pre_rate_change(struct clk_notifier_data *ndata,
+ struct exynos_cpuclk *armclk, void __iomem *base)
+{
+ struct exynos4210_armclk_data *armclk_data = armclk->data;
+ unsigned long alt_prate = clk_get_rate(armclk->alt_parent);
+ unsigned long alt_div, div0, div1, tdiv0, mux_reg;
+ unsigned long cur_armclk_rate, timeout;
+ unsigned long flags;
+
+ /* find out the divider values to use for clock data */
+ while (armclk_data->prate != ndata->new_rate) {
I assume this code relies on the assumption that target DIV_CORE and
DIV_CORE2 are always 0 (divide by 1)? Otherwise it should compare
armclk_data->prate with new parent rate, not new target armclk rate,
which would be parent rate divided by DIV_CORE and DIV_CORE2.
+ if (readl(base + SRC_CPU) & EXYNOS4210_MUX_HPM_MASK) {
+ div1 = readl(base + DIV_CPU1) & EXYNOS4210_DIV1_HPM_MASK;
+ div1 |= ((armclk_data->div1) & ~EXYNOS4210_DIV1_HPM_MASK);
+ }
+
+ /*
+ * if the new and old parent clock speed is less than the clock speed
+ * of the alternate parent, then it should be ensured that at no point
+ * the armclk speed is more than the old_prate until the dividers are
+ * set.
+ */
+ tdiv0 = readl(base + DIV_CPU0);
+ cur_armclk_rate = ndata->old_rate / EXYNOS4210_ARM_DIV1(tdiv0) /
+ EXYNOS4210_ARM_DIV2(tdiv0);
+ if (alt_prate > cur_armclk_rate) {
Shouldn't you compare two parent rates here, not alt parent rate with
current armclk rate?
Also, this condition compares only alt rate with current rate. Let's see:
1) old >= alt && new >= alt => alt < old X new
The voltage will be always enough to handle the switch, so no division
is needed.
2) old < alt && new >= alt => old < alt <= new
The voltage will be switched to higher or equal necessary one for alt
rate, so no division is needed.
3) old < alt && new < alt => old X new < alt
The voltage won't be enough for alt rate so division is needed.
4) old >= alt && new < alt => new < alt <= old
Current voltage is enough for alt rate and it will be lowered only after
the switching finishes, so division is not needed.
This means that division is necessary only if both new and old rates are
lower than alt and this is what the comment above says, but not what the
code does, which is slightly inefficient.
Hmm, this code is barely readable. It is not clear whether _calc_div()
is returning a value ready to be written to the register or real divisor
value. I'd make _calc_div() to simply return raw divisor value and then
use a macro that calculates required bitfield value.
Another thing is whether 8 is big enough maximum divisor. If not, both
DIV_CORE and DIV_CORE2 should be used together to form a 6-bit divisor,
which lets you divide by up to 64.
+ }
+
+ /* select sclk_mpll as the alternate parent */
+ spin_lock_irqsave(armclk->lock, flags);
Hmm, is the start of critical section really here? The big
read-modify-write section seems to begin at
if (readl(base + SRC_CPU) & EXYNOS4210_MUX_HPM_MASK) {
div1 = readl(base + DIV_CPU1) & EXYNOS4210_DIV1_HPM_MASK;
Is this really end of critical secion? More writes to registers are
happening below. Keep in mind that APLL_RATIO field of CLK_DIV_CPU0
register is used by generic divider clock - "sclk_apll".
+
+ if (((readl(base + STAT_CPU) >> 16) & 0x7) != 2)
+ pr_err("%s: re-parenting to sclk_mpll failed\n", __func__);
+
+ /* alternate parent is active now. set the dividers */
+ writel(div0, base + DIV_CPU0);
+ timeout = jiffies + msecs_to_jiffies(10);
+ while (time_before(jiffies, timeout))
+ if (!readl(base + DIV_STAT_CPU0))
+ break;
+
+ if (readl(base + DIV_STAT_CPU0))
+ pr_err("%s: timeout in divider0 stablization\n", __func__);
+
+ writel(div1, base + DIV_CPU1);
+ timeout = jiffies + msecs_to_jiffies(10);
+ while (time_before(jiffies, timeout))
+ if (!readl(base + DIV_STAT_CPU1))
+ break;
+ if (readl(base + DIV_STAT_CPU1))
+ pr_err("%s: timeout in divider1 stablization\n", __func__);
IMHO to be safe, the spin_unlock_irqrestore() should be called here.
+ while (time_before(jiffies, timeout))
+ if (((readl(base + STAT_CPU) >> 16) & 0x7) == 1)
+ break;
+ spin_unlock_irqrestore(armclk->lock, flags);
+
+ if (((readl(base + STAT_CPU) >> 16) & 0x7) != 1)
+ pr_err("%s: re-parenting to mout_apll failed\n", __func__);
+
+ return 0;
+}
+
+/*
+ * This clock notifier is called when the frequency of the parent clock
+ * of armclk is to be changed. This notifier handles the setting up all
+ * the divider clocks, remux to temporary parent and handling the safe
+ * frequency levels when using temporary parent.
+ */
+static int exynos4210_armclk_notifier_cb(struct notifier_block *nb,
+ unsigned long event, void *data)
+{
+ struct clk_notifier_data *ndata = data;
+ struct exynos_cpuclk *armclk = to_exynos_cpuclk_nb(nb);
+ void __iomem *base = armclk->ctrl_base + armclk->offset;
+ int err = 0;
+
+ if (event == PRE_RATE_CHANGE)
+ err = exynos4210_armclk_pre_rate_change(ndata, armclk, base);
+ else if (event == POST_RATE_CHANGE)
+ err = exynos4210_armclk_post_rate_change(armclk, base);
+
+ return notifier_from_errno(err);
+}
+
+static int exynos4210_armclk_set_rate(struct clk_hw *hw, unsigned long drate,
+ unsigned long prate)
+{
+ struct exynos_cpuclk *armclk = to_exynos_cpuclk_hw(hw);
+ void __iomem *base = armclk->ctrl_base + armclk->offset;
+ unsigned long div;
+
+ div = drate < prate ? _calc_div(prate, drate) : 0;
+ _exynos4210_set_armclk_div(base, div);
Hmm, the code above in pre_rate_change() assumed that both DIV_CORE and
DIV_CORE2 are 0, but here it sets DIV_CORE to a potentially non-zero
value. It doesn't look correct.
+ return 0;
+}
+
+static const struct clk_ops exynos4210_armclk_clk_ops = {
+ .recalc_rate = exynos4210_armclk_recalc_rate,
+ .round_rate = exynos_cpuclk_round_rate,
+ .set_rate = exynos4210_armclk_set_rate,
+};
+
+/*
+ * parse divider configuration data from dt for all the cpu clock domain
+ * clocks in exynos4210 and compatible SoC's.
+ */
+static int __init exynos4210_armclk_parser(struct device_node *np, void **data)
+{
+ struct exynos4210_armclk_data *tdata;
+ u32 cfg[10], num_rows, row, col;
+ struct property *prop;
+ const __be32 *ptr = NULL;
+ u32 cells;
+ int ret;
+
+ if (of_property_read_u32(np, "samsung,armclk-cells", &cells))
+ return -EINVAL;
+ prop = of_find_property(np, "samsung,armclk-divider-table", NULL);
You should rather use the *lenp argument of of_find_property(), instead
of dereferencing the struct.
+ if (!prop)
+ return -EINVAL;
+ if (!prop->value)
+ return -EINVAL;
You can skip the check above, as the calculation below will give you
num_rows equal 0 in this case.
+ if ((prop->length / sizeof(u32)) % cells)
+ return -EINVAL;
+ num_rows = (prop->length / sizeof(u32)) / cells;
+
+ /* allocate a zero terminated table */
+ *data = kzalloc(sizeof(*tdata) * (num_rows + 1), GFP_KERNEL);
+ if (!*data)
+ ret = -ENOMEM;
Shouldn't you just return -ENOMEM here?
Best regards,
Tomasz
From: Thomas Abraham <hidden> Date: 2014-05-23 14:41:41
Hi Tomasz,
On Fri, May 16, 2014 at 10:47 PM, Tomasz Figa [off-list ref] wrote:
Hi Thomas,
On 14.05.2014 03:11, Thomas Abraham wrote:
quoted
From: Thomas Abraham <redacted>
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.
Cc: Tomasz Figa <redacted>
Signed-off-by: Thomas Abraham <redacted>
---
drivers/clk/samsung/Makefile | 2 +-
drivers/clk/samsung/clk-cpu.c | 458 +++++++++++++++++++++++++++++++++++++++++
drivers/clk/samsung/clk.h | 5 +
3 files changed, 464 insertions(+), 1 deletions(-)
create mode 100644 drivers/clk/samsung/clk-cpu.c
Macro arguments should be put into parentheses to make sure that whole
argument is subject to further arithmetic operations.
quoted
+
+#define EXYNOS4210_DIV1_HPM_MASK ((0x7 << 0) | (0x7 << 4))
+#define EXYNOS4210_MUX_HPM_MASK (1 << 20)
+
+/**
+ * struct exynos4210_armclk_data: config data to setup exynos4210 cpu clocks.
+ * @prate: frequency of the parent clock.
+ * @div0: value to be programmed in the div_cpu0 register.
+ * @div1: value to be programmed in the div_cpu1 register.
+ *
+ * This structure holds the divider configuration data for divider clocks
+ * belonging to the CMU_CPU clock domain. The parent frequency at which these
+ * divider values are vaild is specified in @prate.
s/vaild/valid/
quoted
+ */
+struct exynos4210_armclk_data {
+ unsigned long prate;
+ unsigned int div0;
+ unsigned int div1;
+};
+
+/**
+ * struct exynos_cpuclk: information about clock supplied to a CPU core.
+ * @hw: handle between ccf and cpu clock.
s/ccf/CCF/
s/cpu/CPU/
quoted
+ * @alt_parent: alternate parent clock to use when switching the speed
+ * of the primary parent clock.
+ * @ctrl_base: base address of the clock controller.
+ * @offset: offset from the ctrl_base address where the cpu clock div/mux
s/cpu/CPU/
quoted
+ * registers can be accessed.
+ * @clk_nb: clock notifier registered for changes in clock speed of the
+ * primary parent clock.
+ * @lock: register access lock.
+ * @data: optional data which the acutal instantiation of this clock
+ * can use.
s/acutal/actual/
quoted
+ */
+struct exynos_cpuclk {
+ struct clk_hw hw;
+ struct clk *alt_parent;
+ void __iomem *ctrl_base;
+ unsigned long offset;
+ struct notifier_block clk_nb;
+ spinlock_t *lock;
+ void *data;
+};
+
+#define to_exynos_cpuclk_hw(hw) container_of(hw, struct exynos_cpuclk, hw)
+#define to_exynos_cpuclk_nb(nb) container_of(nb, struct exynos_cpuclk, clk_nb)
+
+/**
+ * struct exynos_cpuclk_soc_data: soc specific data for cpu clocks.
+ * @parser: pointer to a function that can parse SoC specific data.
+ * @ops: clock operations to be used for this clock.
+ * @offset: optional offset from base of clock controller register base, to
+ * be used when accessing clock controller registers related to the
+ * cpu clock.
s/cpu/CPU/
quoted
+ * @clk_cb: the clock notifier callback to be called for changes in the
+ * clock rate of the primary parent clock.
+ *
+ * This structure provides SoC specific data for ARM clocks. Based on
+ * the compatible value of the clock controller node, the value of the
+ * fields in this structure can be populated.
+ */
+struct exynos_cpuclk_soc_data {
+ int (*parser)(struct device_node *, void **);
Here you don't have argument names, but...
quoted
+ const struct clk_ops *ops;
+ unsigned int offset;
+ int (*clk_cb)(struct notifier_block *nb, unsigned long evt, void *data);
...here you have. Please keep some consistency.
quoted
+};
+
+/* common round rate callback useable for all types of cpu clocks */
s/cpu/CPU/
quoted
+static long exynos_cpuclk_round_rate(struct clk_hw *hw,
+ unsigned long drate, unsigned long *prate)
Hmm, the long return type will overflow with *prate > INT_MAX and
best_div == 1, I wonder why it is defined so in CCF, even though it
shouldn't return error codes...
quoted
+{
+ struct clk *parent = __clk_get_parent(hw->clk);
+ unsigned long max_prate = __clk_round_rate(parent, UINT_MAX);
+ unsigned long t_prate, div = 1, best_div = 1;
+ unsigned long delta, min_delta = UINT_MAX;
By the way, shouldn't this function take into account the list of
available CPU rates and round drate to a less or equal supported one?
Otherwise, in further code you might hit cases where an unsupported rate
is requested, which is against the CCF semantics, if .round_rate()
operation is provided.
quoted
+
+ do {
+ t_prate = __clk_round_rate(parent, drate * div);
+ delta = drate - (t_prate / div);
+ if (delta < min_delta) {
+ *prate = t_prate;
+ best_div = div;
+ min_delta = delta;
+ }
+ if (!delta)
+ break;
+ div++;
+ } while ((drate * div) < max_prate && div <= MAX_DIV);
+
+ return *prate / best_div;
+}
+
+static unsigned long _calc_div(unsigned long prate, unsigned long drate)
+{
+ unsigned long div = prate / drate;
+
+ WARN_ON(div >= MAX_DIV);
+ return (!(prate % drate)) ? div-- : div;
Could you explain what is the purpose of this check and adjustment?
If my assumption that this is essentially DIV_ROUND_UP(prate, drate) - 1
is true then this probably used to obtain a divisor value to get less or
equal rate than drate. Is it right?
quoted
+}
+
+/* helper function to register a cpu clock */
+static int __init exynos_cpuclk_register(unsigned int lookup_id,
+ const char *name, const char **parents,
+ unsigned int num_parents, void __iomem *base,
The num_parents argument doesn't seem to be used in the code. Maybe
instead you should simply replace it and parents arguments with (const
char *parent) and (const char *alt_parent)?
The 0x7 could be defined as a preprocessor macro. Also for increased
readability, this could be split into separate read, modify and write.
quoted
+ while (time_before(jiffies, timeout))
+ if (!readl(base + DIV_STAT_CPU0))
+ return;
+ pr_err("%s: timeout in divider stablization\n", __func__);
+}
+
+static unsigned long exynos4210_armclk_recalc_rate(struct clk_hw *hw,
+ unsigned long parent_rate)
+{
+ struct exynos_cpuclk *armclk = to_exynos_cpuclk_hw(hw);
+ void __iomem *base = armclk->ctrl_base + armclk->offset;
+ unsigned long div0 = readl(base + DIV_CPU0);
+
+ return parent_rate / EXYNOS4210_ARM_DIV1(div0) /
+ EXYNOS4210_ARM_DIV2(div0);
+}
+
+static int exynos4210_armclk_pre_rate_change(struct clk_notifier_data *ndata,
+ struct exynos_cpuclk *armclk, void __iomem *base)
+{
+ struct exynos4210_armclk_data *armclk_data = armclk->data;
+ unsigned long alt_prate = clk_get_rate(armclk->alt_parent);
+ unsigned long alt_div, div0, div1, tdiv0, mux_reg;
+ unsigned long cur_armclk_rate, timeout;
+ unsigned long flags;
+
+ /* find out the divider values to use for clock data */
+ while (armclk_data->prate != ndata->new_rate) {
I assume this code relies on the assumption that target DIV_CORE and
DIV_CORE2 are always 0 (divide by 1)? Otherwise it should compare
armclk_data->prate with new parent rate, not new target armclk rate,
which would be parent rate divided by DIV_CORE and DIV_CORE2.
+ if (readl(base + SRC_CPU) & EXYNOS4210_MUX_HPM_MASK) {
+ div1 = readl(base + DIV_CPU1) & EXYNOS4210_DIV1_HPM_MASK;
+ div1 |= ((armclk_data->div1) & ~EXYNOS4210_DIV1_HPM_MASK);
+ }
+
+ /*
+ * if the new and old parent clock speed is less than the clock speed
+ * of the alternate parent, then it should be ensured that at no point
+ * the armclk speed is more than the old_prate until the dividers are
+ * set.
+ */
+ tdiv0 = readl(base + DIV_CPU0);
+ cur_armclk_rate = ndata->old_rate / EXYNOS4210_ARM_DIV1(tdiv0) /
+ EXYNOS4210_ARM_DIV2(tdiv0);
+ if (alt_prate > cur_armclk_rate) {
Shouldn't you compare two parent rates here, not alt parent rate with
current armclk rate?
Also, this condition compares only alt rate with current rate. Let's see:
1) old >= alt && new >= alt => alt < old X new
The voltage will be always enough to handle the switch, so no division
is needed.
2) old < alt && new >= alt => old < alt <= new
The voltage will be switched to higher or equal necessary one for alt
rate, so no division is needed.
3) old < alt && new < alt => old X new < alt
The voltage won't be enough for alt rate so division is needed.
4) old >= alt && new < alt => new < alt <= old
Current voltage is enough for alt rate and it will be lowered only after
the switching finishes, so division is not needed.
This means that division is necessary only if both new and old rates are
lower than alt and this is what the comment above says, but not what the
code does, which is slightly inefficient.
Hmm, this code is barely readable. It is not clear whether _calc_div()
is returning a value ready to be written to the register or real divisor
value. I'd make _calc_div() to simply return raw divisor value and then
use a macro that calculates required bitfield value.
Another thing is whether 8 is big enough maximum divisor. If not, both
DIV_CORE and DIV_CORE2 should be used together to form a 6-bit divisor,
which lets you divide by up to 64.
quoted
+ }
+
+ /* select sclk_mpll as the alternate parent */
+ spin_lock_irqsave(armclk->lock, flags);
Hmm, is the start of critical section really here? The big
read-modify-write section seems to begin at
if (readl(base + SRC_CPU) & EXYNOS4210_MUX_HPM_MASK) {
div1 = readl(base + DIV_CPU1) & EXYNOS4210_DIV1_HPM_MASK;
Is this really end of critical secion? More writes to registers are
happening below. Keep in mind that APLL_RATIO field of CLK_DIV_CPU0
register is used by generic divider clock - "sclk_apll".
quoted
+
+ if (((readl(base + STAT_CPU) >> 16) & 0x7) != 2)
+ pr_err("%s: re-parenting to sclk_mpll failed\n", __func__);
+
+ /* alternate parent is active now. set the dividers */
+ writel(div0, base + DIV_CPU0);
+ timeout = jiffies + msecs_to_jiffies(10);
+ while (time_before(jiffies, timeout))
+ if (!readl(base + DIV_STAT_CPU0))
+ break;
+
+ if (readl(base + DIV_STAT_CPU0))
+ pr_err("%s: timeout in divider0 stablization\n", __func__);
+
+ writel(div1, base + DIV_CPU1);
+ timeout = jiffies + msecs_to_jiffies(10);
+ while (time_before(jiffies, timeout))
+ if (!readl(base + DIV_STAT_CPU1))
+ break;
+ if (readl(base + DIV_STAT_CPU1))
+ pr_err("%s: timeout in divider1 stablization\n", __func__);
IMHO to be safe, the spin_unlock_irqrestore() should be called here.
+ while (time_before(jiffies, timeout))
+ if (((readl(base + STAT_CPU) >> 16) & 0x7) == 1)
+ break;
+ spin_unlock_irqrestore(armclk->lock, flags);
+
+ if (((readl(base + STAT_CPU) >> 16) & 0x7) != 1)
+ pr_err("%s: re-parenting to mout_apll failed\n", __func__);
+
+ return 0;
+}
+
+/*
+ * This clock notifier is called when the frequency of the parent clock
+ * of armclk is to be changed. This notifier handles the setting up all
+ * the divider clocks, remux to temporary parent and handling the safe
+ * frequency levels when using temporary parent.
+ */
+static int exynos4210_armclk_notifier_cb(struct notifier_block *nb,
+ unsigned long event, void *data)
+{
+ struct clk_notifier_data *ndata = data;
+ struct exynos_cpuclk *armclk = to_exynos_cpuclk_nb(nb);
+ void __iomem *base = armclk->ctrl_base + armclk->offset;
+ int err = 0;
+
+ if (event == PRE_RATE_CHANGE)
+ err = exynos4210_armclk_pre_rate_change(ndata, armclk, base);
+ else if (event == POST_RATE_CHANGE)
+ err = exynos4210_armclk_post_rate_change(armclk, base);
+
+ return notifier_from_errno(err);
+}
+
+static int exynos4210_armclk_set_rate(struct clk_hw *hw, unsigned long drate,
+ unsigned long prate)
+{
+ struct exynos_cpuclk *armclk = to_exynos_cpuclk_hw(hw);
+ void __iomem *base = armclk->ctrl_base + armclk->offset;
+ unsigned long div;
+
+ div = drate < prate ? _calc_div(prate, drate) : 0;
+ _exynos4210_set_armclk_div(base, div);
Hmm, the code above in pre_rate_change() assumed that both DIV_CORE and
DIV_CORE2 are 0, but here it sets DIV_CORE to a potentially non-zero
value. It doesn't look correct.
quoted
+ return 0;
+}
+
+static const struct clk_ops exynos4210_armclk_clk_ops = {
+ .recalc_rate = exynos4210_armclk_recalc_rate,
+ .round_rate = exynos_cpuclk_round_rate,
+ .set_rate = exynos4210_armclk_set_rate,
+};
+
+/*
+ * parse divider configuration data from dt for all the cpu clock domain
+ * clocks in exynos4210 and compatible SoC's.
+ */
+static int __init exynos4210_armclk_parser(struct device_node *np, void **data)
+{
+ struct exynos4210_armclk_data *tdata;
+ u32 cfg[10], num_rows, row, col;
+ struct property *prop;
+ const __be32 *ptr = NULL;
+ u32 cells;
+ int ret;
+
+ if (of_property_read_u32(np, "samsung,armclk-cells", &cells))
+ return -EINVAL;
+ prop = of_find_property(np, "samsung,armclk-divider-table", NULL);
You should rather use the *lenp argument of of_find_property(), instead
of dereferencing the struct.
quoted
+ if (!prop)
+ return -EINVAL;
+ if (!prop->value)
+ return -EINVAL;
You can skip the check above, as the calculation below will give you
num_rows equal 0 in this case.
quoted
+ if ((prop->length / sizeof(u32)) % cells)
+ return -EINVAL;
+ num_rows = (prop->length / sizeof(u32)) / cells;
+
+ /* allocate a zero terminated table */
+ *data = kzalloc(sizeof(*tdata) * (num_rows + 1), GFP_KERNEL);
+ if (!*data)
+ ret = -ENOMEM;
Shouldn't you just return -ENOMEM here?
Best regards,
Tomasz
Thanks for your detailed review. I have made all the changes that you
have suggested.
Regards,
Thomas.
From: Tomasz Figa <hidden> Date: 2014-05-23 14:50:45
Hi Thomas,
On 23.05.2014 16:41, Thomas Abraham wrote:
[snip]
Thanks for your detailed review. I have made all the changes that you
have suggested.
Unfortunately it seems like you have missed quite a lot of my comments,
especially those regarding patch 4/8, which adds DT binding.
Best regards,
Tomasz
From: Thomas Abraham <hidden> Date: 2014-05-14 01:11:12
From; Thomas Abraham [off-list ref]
The clock blocks within the CMU_CPU clock domain are put together into a
new composite clock type called the cpu clock. This clock type requires
configuration data that will be atomically programmed in the multiple
clock blocks encapsulated within the cpu clock type when the parent clock
frequency is changed. This configuration data is held in the clock controller
node. Update clock binding documentation about this configuration data format
for Samsung Exynos4 and Exynos5 platforms.
Cc: Tomasz Figa <redacted>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Pawel Moll <redacted>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Ian Campbell <redacted>
Cc: Kumar Gala <redacted>
Cc: <redacted>
Signed-off-by: Thomas Abraham <redacted>
---
.../devicetree/bindings/clock/exynos4-clock.txt | 37 ++++++++++++++++++++
.../devicetree/bindings/clock/exynos5250-clock.txt | 36 +++++++++++++++++++
2 files changed, 73 insertions(+), 0 deletions(-)
@@ -15,6 +15,35 @@ Required Properties: - #clock-cells: should be 1.+- samsung,armclk-divider-table: when the frequency of the APLL is changed+ the divider clocks in CMU_CPU clock domain also need to be updated. These+ divider clocks have SoC specific divider clock output requirements for a+ specific APLL clock speeds. When APLL clock rate is changed, these divider+ clocks are reprogrammed with pre-determined values in order to maintain the+ SoC specific divider clock outputs. This property lists the divider values+ for divider clocks in the CMU_CPU block for supported APLL clock speeds.+ The format of each entry included in the arm-frequency-table should be+ as defined below++ - for Exynos4210 and Exynos4212 based platforms:+ cell #1: arm clock parent frequency+ cell #2 ~ cell 9#: value of clock divider in the following order+ corem0_ratio, corem1_ratio, periph_ratio, atb_ratio,+ pclk_dbg_ratio, apll_ratio, copy_ratio, hpm_ratio.++ - for Exynos4412 based platforms:+ cell #1: expected arm clock parent frequency+ cell #2 ~ cell #10: value of clock divider in the following order+ corem0_ratio, corem1_ratio, periph_ratio, atb_ratio,+ pclk_dbg_ratio, apll_ratio, copy_ratio, hpm_ratio, cores_ratio++- samsung,armclk-cells: defines the number of cells in+ samsung,armclk-divider-table property. The value of this property depends on+ the SoC type.++ - for Exynos4210 and Exynos4212: the value should be 9.+ - for Exynos4412: the value should be 10.+ Each clock is assigned an identifier and client nodes can use this identifier to specify the clock which they consume.
@@ -28,6 +57,14 @@ Example 1: An example of a clock controller node is listed below. compatible = "samsung,exynos4210-clock"; reg = <0x10030000 0x20000>; #clock-cells = <1>;++ samsung,armclk-cells = <9>;+ samsung,armclk-divider-table = <1200000 3 7 3 4 1 7 5 0>,+ <1000000 3 7 3 4 1 7 4 0>,+ < 800000 3 7 3 3 1 7 3 0>,+ < 500000 3 7 3 3 1 7 3 0>,+ < 400000 3 7 3 3 1 7 3 0>,+ < 200000 1 3 1 1 1 0 3 0>; }; Example 2: UART controller node that consumes the clock generated by the clock
@@ -13,6 +13,24 @@ Required Properties: - #clock-cells: should be 1.+- samsung,armclk-divider-table: when the frequency of the APLL is changed+ the divider clocks in CMU_CPU clock domain also need to be updated. These+ divider clocks have SoC specific divider clock output requirements for a+ specific APLL clock speeds. When APLL clock rate is changed, these divider+ clocks are reprogrammed with pre-determined values in order to maintain the+ SoC specific divider clock outputs. This property lists the divider values+ for divider clocks in the CMU_CPU block for supported APLL clock speeds.+ The format of each entry included in the arm-frequency-table should be+ as defined below++ cell #1: expected arm clock parent frequency+ cell #2 ~ cell #9: value of clock divider in the following order+ cpud_ratio, acp_ratio, periph_ratio, atb_ratio,+ pclk_dbg_ratio, apll_ratio, copy_ratio, hpm_ratio++- samsung,armclk-cells: defines the number of cells in+ samsung,armclk-divider-table property. The value of this property should be 9.+ Each clock is assigned an identifier and client nodes can use this identifier to specify the clock which they consume.
From: Tomasz Figa <hidden> Date: 2014-05-16 23:24:15
Hi Thomas,
Please see my comments inline.
On 14.05.2014 03:11, Thomas Abraham wrote:
quoted hunk
From; Thomas Abraham [off-list ref]
The clock blocks within the CMU_CPU clock domain are put together into a
new composite clock type called the cpu clock. This clock type requires
configuration data that will be atomically programmed in the multiple
clock blocks encapsulated within the cpu clock type when the parent clock
frequency is changed. This configuration data is held in the clock controller
node. Update clock binding documentation about this configuration data format
for Samsung Exynos4 and Exynos5 platforms.
Cc: Tomasz Figa <redacted>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Pawel Moll <redacted>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Ian Campbell <redacted>
Cc: Kumar Gala <redacted>
Cc: <redacted>
Signed-off-by: Thomas Abraham <redacted>
---
.../devicetree/bindings/clock/exynos4-clock.txt | 37 ++++++++++++++++++++
.../devicetree/bindings/clock/exynos5250-clock.txt | 36 +++++++++++++++++++
2 files changed, 73 insertions(+), 0 deletions(-)
@@ -15,6 +15,35 @@ Required Properties: - #clock-cells: should be 1.+- samsung,armclk-divider-table: when the frequency of the APLL is changed+ the divider clocks in CMU_CPU clock domain also need to be updated. These+ divider clocks have SoC specific divider clock output requirements for a+ specific APLL clock speeds. When APLL clock rate is changed, these divider+ clocks are reprogrammed with pre-determined values in order to maintain the+ SoC specific divider clock outputs. This property lists the divider values+ for divider clocks in the CMU_CPU block for supported APLL clock speeds.+ The format of each entry included in the arm-frequency-table should be+ as defined below
As far as I understand, the relation is not between the APLL frequency
and particular clocks in CPU domain, but rather between the latter and
input clock to CPU domain, which is _after_ the two dividers (called
DIV_CORE and DIV_CORE2 or ARM_DIV1 and ARM_DIV2), which is also exactly
the output frequency of ARMCLK.
+
+ - for Exynos4210 and Exynos4212 based platforms:
+ cell #1: arm clock parent frequency
Considering my comment above, this should be rather ARMCLK frequency.
+ cell #2 ~ cell 9#: value of clock divider in the following order
+ corem0_ratio, corem1_ratio, periph_ratio, atb_ratio,
+ pclk_dbg_ratio, apll_ratio, copy_ratio, hpm_ratio.
+
+ - for Exynos4412 based platforms:
+ cell #1: expected arm clock parent frequency
Ditto.
+ cell #2 ~ cell #10: value of clock divider in the following order
+ corem0_ratio, corem1_ratio, periph_ratio, atb_ratio,
+ pclk_dbg_ratio, apll_ratio, copy_ratio, hpm_ratio, cores_ratio
+
+- samsung,armclk-cells: defines the number of cells in
+ samsung,armclk-divider-table property. The value of this property depends on
+ the SoC type.
To follow conventions used by all other bindings with variable number of
cells, the property should be called "#samsung,armclk-cells". AFAIK the
"#" should be interpreted as "number of" and so accents the meaning of
the property.
quoted hunk
+
+ - for Exynos4210 and Exynos4212: the value should be 9.
+ - for Exynos4412: the value should be 10.
+
Each clock is assigned an identifier and client nodes can use this identifier
to specify the clock which they consume.
@@ -28,6 +57,14 @@ Example 1: An example of a clock controller node is listed below. compatible = "samsung,exynos4210-clock"; reg = <0x10030000 0x20000>; #clock-cells = <1>;++ samsung,armclk-cells = <9>;+ samsung,armclk-divider-table = <1200000 3 7 3 4 1 7 5 0>,+ <1000000 3 7 3 4 1 7 4 0>,+ < 800000 3 7 3 3 1 7 3 0>,+ < 500000 3 7 3 3 1 7 3 0>,+ < 400000 3 7 3 3 1 7 3 0>,+ < 200000 1 3 1 1 1 0 3 0>; }; Example 2: UART controller node that consumes the clock generated by the clock
From: Thomas Abraham <hidden> Date: 2014-05-26 06:05:08
Hi Tomasz,
Thanks for your comments. Please see inline reply.
On Sat, May 17, 2014 at 4:54 AM, Tomasz Figa [off-list ref] wrote:
Hi Thomas,
Please see my comments inline.
On 14.05.2014 03:11, Thomas Abraham wrote:
quoted
From; Thomas Abraham [off-list ref]
The clock blocks within the CMU_CPU clock domain are put together into a
new composite clock type called the cpu clock. This clock type requires
configuration data that will be atomically programmed in the multiple
clock blocks encapsulated within the cpu clock type when the parent clock
frequency is changed. This configuration data is held in the clock controller
node. Update clock binding documentation about this configuration data format
for Samsung Exynos4 and Exynos5 platforms.
Cc: Tomasz Figa <redacted>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Pawel Moll <redacted>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Ian Campbell <redacted>
Cc: Kumar Gala <redacted>
Cc: <redacted>
Signed-off-by: Thomas Abraham <redacted>
---
.../devicetree/bindings/clock/exynos4-clock.txt | 37 ++++++++++++++++++++
.../devicetree/bindings/clock/exynos5250-clock.txt | 36 +++++++++++++++++++
2 files changed, 73 insertions(+), 0 deletions(-)
@@ -15,6 +15,35 @@ Required Properties: - #clock-cells: should be 1.+- samsung,armclk-divider-table: when the frequency of the APLL is changed+ the divider clocks in CMU_CPU clock domain also need to be updated. These+ divider clocks have SoC specific divider clock output requirements for a+ specific APLL clock speeds. When APLL clock rate is changed, these divider+ clocks are reprogrammed with pre-determined values in order to maintain the+ SoC specific divider clock outputs. This property lists the divider values+ for divider clocks in the CMU_CPU block for supported APLL clock speeds.+ The format of each entry included in the arm-frequency-table should be+ as defined below
As far as I understand, the relation is not between the APLL frequency
and particular clocks in CPU domain, but rather between the latter and
input clock to CPU domain, which is _after_ the two dividers (called
DIV_CORE and DIV_CORE2 or ARM_DIV1 and ARM_DIV2), which is also exactly
the output frequency of ARMCLK.
quoted
+
+ - for Exynos4210 and Exynos4212 based platforms:
+ cell #1: arm clock parent frequency
Considering my comment above, this should be rather ARMCLK frequency.
The clocks SCLK_APLL, SCLK_HPM, ATCLK and PCLK_DBG have no relation to
the ARMCLK frequency. These clocks are directly derived from the PLL
clock and so it would not be correct to have them related to ARMCLK.
So, I see two solutions to this, first being preferred solution.
[A] Cell #1 should define PLL (parent of armclk) clock speed. Cell #2
and Cell #3 should define divider values for ARMCLK clock speed. The
hardware does support PLL frequency != ARMCLK frequency and so DT
binding should allow that (even though implementation in the linux
kernel does not use this feature). BTW, this was what was done in v2
of this series.
[B] Embedded this data with the code and don't get this from DT. The
reason for doing this is, these are SoC specific values and not board
specific. And when we are clear about what we want to put in DT, have
a provision to lookup DT first and if DT values are not found,
fallback on data embedded with the code.
Thanks,
Thomas.
quoted
+ cell #2 ~ cell 9#: value of clock divider in the following order
+ corem0_ratio, corem1_ratio, periph_ratio, atb_ratio,
+ pclk_dbg_ratio, apll_ratio, copy_ratio, hpm_ratio.
+
+ - for Exynos4412 based platforms:
+ cell #1: expected arm clock parent frequency
Ditto.
quoted
+ cell #2 ~ cell #10: value of clock divider in the following order
+ corem0_ratio, corem1_ratio, periph_ratio, atb_ratio,
+ pclk_dbg_ratio, apll_ratio, copy_ratio, hpm_ratio, cores_ratio
+
+- samsung,armclk-cells: defines the number of cells in
+ samsung,armclk-divider-table property. The value of this property depends on
+ the SoC type.
To follow conventions used by all other bindings with variable number of
cells, the property should be called "#samsung,armclk-cells". AFAIK the
"#" should be interpreted as "number of" and so accents the meaning of
the property.
quoted
+
+ - for Exynos4210 and Exynos4212: the value should be 9.
+ - for Exynos4412: the value should be 10.
+
Each clock is assigned an identifier and client nodes can use this identifier
to specify the clock which they consume.
@@ -28,6 +57,14 @@ Example 1: An example of a clock controller node is listed below. compatible = "samsung,exynos4210-clock"; reg = <0x10030000 0x20000>; #clock-cells = <1>;++ samsung,armclk-cells = <9>;+ samsung,armclk-divider-table = <1200000 3 7 3 4 1 7 5 0>,+ <1000000 3 7 3 4 1 7 4 0>,+ < 800000 3 7 3 3 1 7 3 0>,+ < 500000 3 7 3 3 1 7 3 0>,+ < 400000 3 7 3 3 1 7 3 0>,+ < 200000 1 3 1 1 1 0 3 0>; }; Example 2: UART controller node that consumes the clock generated by the clock
From: Tomasz Figa <hidden> Date: 2014-05-26 11:02:06
Thomas,
On 26.05.2014 08:05, Thomas Abraham wrote:
Hi Tomasz,
Thanks for your comments. Please see inline reply.
On Sat, May 17, 2014 at 4:54 AM, Tomasz Figa [off-list ref] wrote:
quoted
Hi Thomas,
Please see my comments inline.
On 14.05.2014 03:11, Thomas Abraham wrote:
quoted
From; Thomas Abraham [off-list ref]
The clock blocks within the CMU_CPU clock domain are put together into a
new composite clock type called the cpu clock. This clock type requires
configuration data that will be atomically programmed in the multiple
clock blocks encapsulated within the cpu clock type when the parent clock
frequency is changed. This configuration data is held in the clock controller
node. Update clock binding documentation about this configuration data format
for Samsung Exynos4 and Exynos5 platforms.
Cc: Tomasz Figa <redacted>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Pawel Moll <redacted>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Ian Campbell <redacted>
Cc: Kumar Gala <redacted>
Cc: <redacted>
Signed-off-by: Thomas Abraham <redacted>
---
.../devicetree/bindings/clock/exynos4-clock.txt | 37 ++++++++++++++++++++
.../devicetree/bindings/clock/exynos5250-clock.txt | 36 +++++++++++++++++++
2 files changed, 73 insertions(+), 0 deletions(-)
@@ -15,6 +15,35 @@ Required Properties: - #clock-cells: should be 1.+- samsung,armclk-divider-table: when the frequency of the APLL is changed+ the divider clocks in CMU_CPU clock domain also need to be updated. These+ divider clocks have SoC specific divider clock output requirements for a+ specific APLL clock speeds. When APLL clock rate is changed, these divider+ clocks are reprogrammed with pre-determined values in order to maintain the+ SoC specific divider clock outputs. This property lists the divider values+ for divider clocks in the CMU_CPU block for supported APLL clock speeds.+ The format of each entry included in the arm-frequency-table should be+ as defined below
As far as I understand, the relation is not between the APLL frequency
and particular clocks in CPU domain, but rather between the latter and
input clock to CPU domain, which is _after_ the two dividers (called
DIV_CORE and DIV_CORE2 or ARM_DIV1 and ARM_DIV2), which is also exactly
the output frequency of ARMCLK.
quoted
+
+ - for Exynos4210 and Exynos4212 based platforms:
+ cell #1: arm clock parent frequency
Considering my comment above, this should be rather ARMCLK frequency.
The clocks SCLK_APLL, SCLK_HPM, ATCLK and PCLK_DBG have no relation to
the ARMCLK frequency. These clocks are directly derived from the PLL
clock and so it would not be correct to have them related to ARMCLK.
Oh, right, the old driver was changing DIV_APLL, DIV_ATB and
DIV_PCLK_DBG as well. Somehow I was under an impression that we need to
care only about those dividers on the path after DIV_CORE and DIV_CORE2.
In this case the parent rate is the key here, although I'd call it "CPU
block parent rate (usually APLL)".
However this means that the trick with using DIV_CORE and DIV_CORE2 to
divide the rate of temporary parent clock is not enough, because DIV_ATB
is sourced directly from MOUT_CORE.
So, I see two solutions to this, first being preferred solution.
[A] Cell #1 should define PLL (parent of armclk) clock speed. Cell #2
and Cell #3 should define divider values for ARMCLK clock speed. The
hardware does support PLL frequency != ARMCLK frequency and so DT
binding should allow that (even though implementation in the linux
kernel does not use this feature). BTW, this was what was done in v2
of this series.
[B] Embedded this data with the code and don't get this from DT. The
reason for doing this is, these are SoC specific values and not board
specific. And when we are clear about what we want to put in DT, have
a provision to lookup DT first and if DT values are not found,
fallback on data embedded with the code.
Well, they are not that generic as they might appear. I've seen
different values for the same SoC in different vendor kernels, depending
on device the kernel was targeted for. Also they will likely differ
between SoC revisions.
However it might be a good idea indeed to keep the table in the code as
a first step to get the driver running without creating new DT bindings.
By the way, I'm not fully convinced if there is really a need for such
hardcoded look-up tables at all. Those divisors certainly look like they
are calculated based on some upper bounds for certain clocks and the
driver could simply find them out itself if those limits were provided.
Best regards,
Tomasz
From: Thomas Abraham <hidden> Date: 2014-05-14 01:11:13
From: Thomas Abraham <redacted>
With the addition of the new Samsung specific cpu-clock type, the
arm clock can be represented as a cpu-clock type and the independent
clock blocks that made up the arm clock can be removed.
Cc: Tomasz Figa <redacted>
Signed-off-by: Thomas Abraham <redacted>
---
drivers/clk/samsung/clk-exynos4.c | 25 +++++++++----------------
drivers/clk/samsung/clk-exynos5250.c | 12 ++++++------
include/dt-bindings/clock/exynos5250.h | 1 +
3 files changed, 16 insertions(+), 22 deletions(-)
From: Mike Turquette <hidden> Date: 2014-05-14 21:37:29
Quoting Thomas Abraham (2014-05-13 18:11:13)
From: Thomas Abraham <redacted>
With the addition of the new Samsung specific cpu-clock type, the
arm clock can be represented as a cpu-clock type and the independent
clock blocks that made up the arm clock can be removed.
<rant>
I am not a fan of this type of "clock hiding". Certainly the design of
the CCF allows for a clock provider to obfuscate it's internals; there
was never a requirement that every clock node be exposed to Linux as a
struct clk. A truly obfuscated system could only expose leaf clocks that
are consumed by Linux device drivers, and never expose any of the
intermediary clocks in between the input clock signal (if it exists) and
the leaf nodes.
However I feel that this patch is more of a workaround to the fact that
the clock framework today does not make DVFS transitions (or
coordinated, multi-clock rate change transitions) easy to control. The
generic "walk up the tree" algorithm might give you the right rate, but
perhaps using a non-validated combination of PLL frequency and
adjustable-rate dividers, or a combination that his higher jitter or is
more likely to unlock at higher temperatures, etc.
Back in the pre-CCF days lots of folks implemented this with "virtual"
clock nodes that simply called "clk_set_rate" or whatever on the
affected clocks, or even worse just banged a bunch of registers.
The cbus clock series for Tegra is also looks a little like this.
</rant>
Thomas,
Would a coordinated clock rate change method solve this problem for you
in place of the cpu-clock provider type? A poorly conceived call graph
for this might look something like:
clk_set_rate(div_arm2, 1000000);
-> if (div_arm2->coordinated == true)
clk_coordinate_rates(div_arm2, 1000000);
-> clk->ops->coordinate(div_arm2->hw, 1000000);
-> vendor_supplied_magic()
The vendor_supplied_magic() would be a callback that essentially calls
clk_set_rate() on all of the affected (coordinated) clocks. In your case
that looks like mout_core, div_core, div_core2, arm_clk and sclk_apll
for Exynos4.
The trick is that calling clk_set_rate() from any driver would initiate
this coordinated rate change, and then the exynos clock driver would set
up the coordinated clocks itself. No new API is introduced to drivers
(since it still uses clk_set_rate) and now a new clock provider doesn't
have to be invented every time we have a couple of clocks involved in a
DVFS transition.
Does this sound right to you or have I horribly misinterpreted the point
of these clock patches?
Thanks,
Mike
quoted hunk
Cc: Tomasz Figa <redacted>
Signed-off-by: Thomas Abraham <redacted>
---
drivers/clk/samsung/clk-exynos4.c | 25 +++++++++----------------
drivers/clk/samsung/clk-exynos5250.c | 12 ++++++------
include/dt-bindings/clock/exynos5250.h | 1 +
3 files changed, 16 insertions(+), 22 deletions(-)
From: Thomas Abraham <hidden> Date: 2014-05-15 07:48:24
On Thu, May 15, 2014 at 3:07 AM, Mike Turquette [off-list ref] wrote:
Quoting Thomas Abraham (2014-05-13 18:11:13)
quoted
From: Thomas Abraham <redacted>
With the addition of the new Samsung specific cpu-clock type, the
arm clock can be represented as a cpu-clock type and the independent
clock blocks that made up the arm clock can be removed.
<rant>
I am not a fan of this type of "clock hiding". Certainly the design of
the CCF allows for a clock provider to obfuscate it's internals; there
was never a requirement that every clock node be exposed to Linux as a
struct clk. A truly obfuscated system could only expose leaf clocks that
are consumed by Linux device drivers, and never expose any of the
intermediary clocks in between the input clock signal (if it exists) and
the leaf nodes.
However I feel that this patch is more of a workaround to the fact that
the clock framework today does not make DVFS transitions (or
coordinated, multi-clock rate change transitions) easy to control. The
generic "walk up the tree" algorithm might give you the right rate, but
perhaps using a non-validated combination of PLL frequency and
adjustable-rate dividers, or a combination that his higher jitter or is
more likely to unlock at higher temperatures, etc.
Back in the pre-CCF days lots of folks implemented this with "virtual"
clock nodes that simply called "clk_set_rate" or whatever on the
affected clocks, or even worse just banged a bunch of registers.
The cbus clock series for Tegra is also looks a little like this.
</rant>
Thomas,
Would a coordinated clock rate change method solve this problem for you
in place of the cpu-clock provider type? A poorly conceived call graph
for this might look something like:
clk_set_rate(div_arm2, 1000000);
-> if (div_arm2->coordinated == true)
clk_coordinate_rates(div_arm2, 1000000);
-> clk->ops->coordinate(div_arm2->hw, 1000000);
-> vendor_supplied_magic()
The vendor_supplied_magic() would be a callback that essentially calls
clk_set_rate() on all of the affected (coordinated) clocks. In your case
that looks like mout_core, div_core, div_core2, arm_clk and sclk_apll
for Exynos4.
The trick is that calling clk_set_rate() from any driver would initiate
this coordinated rate change, and then the exynos clock driver would set
up the coordinated clocks itself. No new API is introduced to drivers
(since it still uses clk_set_rate) and now a new clock provider doesn't
have to be invented every time we have a couple of clocks involved in a
DVFS transition.
Does this sound right to you or have I horribly misinterpreted the point
of these clock patches?
Mike,
Thanks for your comments. If I have understood the coordinated clock
as described above, I believe this patch series also attempts to do
almost the same thing. The advantage of using coordinated clock over
the approach in this series is that each platform need not define a
new clock type.
In this patch series, for all exynos SoC's, a new cpu clock type was
introduced which is reusable for multi-cluster exynos SoCs as well.
The new clock type is part of the clock driver and all drivers can use
standard clock API to access this clock type instance.
So probably, all the logic for the new cpu_clk clock type introduced
in this patch would go into the vendor_supplied_magic() callback. So
it sounds like the coordinated clock approach would be helpful. Is
this something that you are already working on?
Thanks,
Thomas.
Thanks,
Mike
quoted
Cc: Tomasz Figa <redacted>
Signed-off-by: Thomas Abraham <redacted>
---
drivers/clk/samsung/clk-exynos4.c | 25 +++++++++----------------
drivers/clk/samsung/clk-exynos5250.c | 12 ++++++------
include/dt-bindings/clock/exynos5250.h | 1 +
3 files changed, 16 insertions(+), 22 deletions(-)
On Thu, May 15, 2014 at 3:07 AM, Mike Turquette
[off-list ref] wrote:
quoted
Quoting Thomas Abraham (2014-05-13 18:11:13)
quoted
From: Thomas Abraham <redacted>
With the addition of the new Samsung specific cpu-clock type, the
arm clock can be represented as a cpu-clock type and the
independent clock blocks that made up the arm clock can be removed.
<rant>
I am not a fan of this type of "clock hiding". Certainly the design
of the CCF allows for a clock provider to obfuscate it's internals;
there was never a requirement that every clock node be exposed to
Linux as a struct clk. A truly obfuscated system could only expose
leaf clocks that are consumed by Linux device drivers, and never
expose any of the intermediary clocks in between the input clock
signal (if it exists) and the leaf nodes.
However I feel that this patch is more of a workaround to the fact
that the clock framework today does not make DVFS transitions (or
coordinated, multi-clock rate change transitions) easy to control.
The generic "walk up the tree" algorithm might give you the right
rate, but perhaps using a non-validated combination of PLL
frequency and adjustable-rate dividers, or a combination that his
higher jitter or is more likely to unlock at higher temperatures,
etc.
Back in the pre-CCF days lots of folks implemented this with
"virtual" clock nodes that simply called "clk_set_rate" or whatever
on the affected clocks, or even worse just banged a bunch of
registers.
The cbus clock series for Tegra is also looks a little like this.
</rant>
Thomas,
Would a coordinated clock rate change method solve this problem for
you in place of the cpu-clock provider type? A poorly conceived
call graph for this might look something like:
clk_set_rate(div_arm2, 1000000);
-> if (div_arm2->coordinated == true)
clk_coordinate_rates(div_arm2, 1000000);
-> clk->ops->coordinate(div_arm2->hw, 1000000);
-> vendor_supplied_magic()
The vendor_supplied_magic() would be a callback that essentially
calls clk_set_rate() on all of the affected (coordinated) clocks.
In your case that looks like mout_core, div_core, div_core2,
arm_clk and sclk_apll for Exynos4.
I might misinterpret the idea here, but is clk_set_rate() function
ready to handle atomic change for several clocks? Especially setting
rate of dividers located in the same register, represented as different
clocks to CCF.
As fair as I remember it was not possible to serialize operations on
clocks with calling clk_set_rate() several times. Those had to be done
instantly, otherwise platform hanged.
Am I missing something, or a major improvement had I overlooked?
quoted
The trick is that calling clk_set_rate() from any driver would
initiate this coordinated rate change, and then the exynos clock
driver would set up the coordinated clocks itself. No new API is
introduced to drivers (since it still uses clk_set_rate) and now a
new clock provider doesn't have to be invented every time we have a
couple of clocks involved in a DVFS transition.
Does this sound right to you or have I horribly misinterpreted the
point of these clock patches?
Mike,
Thanks for your comments. If I have understood the coordinated clock
as described above, I believe this patch series also attempts to do
almost the same thing. The advantage of using coordinated clock over
the approach in this series is that each platform need not define a
new clock type.
In this patch series, for all exynos SoC's, a new cpu clock type was
introduced which is reusable for multi-cluster exynos SoCs as well.
The new clock type is part of the clock driver and all drivers can use
standard clock API to access this clock type instance.
So probably, all the logic for the new cpu_clk clock type introduced
in this patch would go into the vendor_supplied_magic() callback. So
it sounds like the coordinated clock approach would be helpful. Is
this something that you are already working on?
Thanks,
Thomas.
quoted
Thanks,
Mike
quoted
Cc: Tomasz Figa <redacted>
Signed-off-by: Thomas Abraham <redacted>
---
drivers/clk/samsung/clk-exynos4.c | 25
+++++++++---------------- drivers/clk/samsung/clk-exynos5250.c
| 12 ++++++------ include/dt-bindings/clock/exynos5250.h | 1 +
3 files changed, 16 insertions(+), 22 deletions(-)
diff --git a/drivers/clk/samsung/clk-exynos4.c
b/drivers/clk/samsung/clk-exynos4.c index b4f9672..7e3bb16c 100644
From: Thomas Abraham <hidden> Date: 2014-05-15 09:59:56
On Thu, May 15, 2014 at 1:40 PM, Lukasz Majewski [off-list ref] wrote:
Hi Thomas,
quoted
On Thu, May 15, 2014 at 3:07 AM, Mike Turquette
[off-list ref] wrote:
quoted
Quoting Thomas Abraham (2014-05-13 18:11:13)
quoted
From: Thomas Abraham <redacted>
With the addition of the new Samsung specific cpu-clock type, the
arm clock can be represented as a cpu-clock type and the
independent clock blocks that made up the arm clock can be removed.
<rant>
I am not a fan of this type of "clock hiding". Certainly the design
of the CCF allows for a clock provider to obfuscate it's internals;
there was never a requirement that every clock node be exposed to
Linux as a struct clk. A truly obfuscated system could only expose
leaf clocks that are consumed by Linux device drivers, and never
expose any of the intermediary clocks in between the input clock
signal (if it exists) and the leaf nodes.
However I feel that this patch is more of a workaround to the fact
that the clock framework today does not make DVFS transitions (or
coordinated, multi-clock rate change transitions) easy to control.
The generic "walk up the tree" algorithm might give you the right
rate, but perhaps using a non-validated combination of PLL
frequency and adjustable-rate dividers, or a combination that his
higher jitter or is more likely to unlock at higher temperatures,
etc.
Back in the pre-CCF days lots of folks implemented this with
"virtual" clock nodes that simply called "clk_set_rate" or whatever
on the affected clocks, or even worse just banged a bunch of
registers.
The cbus clock series for Tegra is also looks a little like this.
</rant>
Thomas,
Would a coordinated clock rate change method solve this problem for
you in place of the cpu-clock provider type? A poorly conceived
call graph for this might look something like:
clk_set_rate(div_arm2, 1000000);
-> if (div_arm2->coordinated == true)
clk_coordinate_rates(div_arm2, 1000000);
-> clk->ops->coordinate(div_arm2->hw, 1000000);
-> vendor_supplied_magic()
The vendor_supplied_magic() would be a callback that essentially
calls clk_set_rate() on all of the affected (coordinated) clocks.
In your case that looks like mout_core, div_core, div_core2,
arm_clk and sclk_apll for Exynos4.
I might misinterpret the idea here, but is clk_set_rate() function
ready to handle atomic change for several clocks? Especially setting
rate of dividers located in the same register, represented as different
clocks to CCF.
As fair as I remember it was not possible to serialize operations on
clocks with calling clk_set_rate() several times. Those had to be done
instantly, otherwise platform hanged.
Am I missing something, or a major improvement had I overlooked?
Hi Lukasz,
Not sure what Mike would suggest here, but probably the clk_set_rate()
itself might not be called for coordinated clocks. It could be some
internal clock framework function that takes care of setting rate of
coordinated clocks. I don't have a good understanding of how the
implementation for this will look like.
Thanks,
Thomas.
quoted
quoted
The trick is that calling clk_set_rate() from any driver would
initiate this coordinated rate change, and then the exynos clock
driver would set up the coordinated clocks itself. No new API is
introduced to drivers (since it still uses clk_set_rate) and now a
new clock provider doesn't have to be invented every time we have a
couple of clocks involved in a DVFS transition.
Does this sound right to you or have I horribly misinterpreted the
point of these clock patches?
Mike,
Thanks for your comments. If I have understood the coordinated clock
as described above, I believe this patch series also attempts to do
almost the same thing. The advantage of using coordinated clock over
the approach in this series is that each platform need not define a
new clock type.
In this patch series, for all exynos SoC's, a new cpu clock type was
introduced which is reusable for multi-cluster exynos SoCs as well.
The new clock type is part of the clock driver and all drivers can use
standard clock API to access this clock type instance.
So probably, all the logic for the new cpu_clk clock type introduced
in this patch would go into the vendor_supplied_magic() callback. So
it sounds like the coordinated clock approach would be helpful. Is
this something that you are already working on?
Thanks,
Thomas.
quoted
Thanks,
Mike
quoted
Cc: Tomasz Figa <redacted>
Signed-off-by: Thomas Abraham <redacted>
---
drivers/clk/samsung/clk-exynos4.c | 25
+++++++++---------------- drivers/clk/samsung/clk-exynos5250.c
| 12 ++++++------ include/dt-bindings/clock/exynos5250.h | 1 +
3 files changed, 16 insertions(+), 22 deletions(-)
diff --git a/drivers/clk/samsung/clk-exynos4.c
b/drivers/clk/samsung/clk-exynos4.c index b4f9672..7e3bb16c 100644
From: Thomas Abraham <hidden> Date: 2014-05-16 05:14:44
On Thu, May 15, 2014 at 3:07 AM, Mike Turquette [off-list ref] wrote:
Quoting Thomas Abraham (2014-05-13 18:11:13)
quoted
From: Thomas Abraham <redacted>
With the addition of the new Samsung specific cpu-clock type, the
arm clock can be represented as a cpu-clock type and the independent
clock blocks that made up the arm clock can be removed.
<rant>
I am not a fan of this type of "clock hiding". Certainly the design of
the CCF allows for a clock provider to obfuscate it's internals; there
was never a requirement that every clock node be exposed to Linux as a
struct clk. A truly obfuscated system could only expose leaf clocks that
are consumed by Linux device drivers, and never expose any of the
intermediary clocks in between the input clock signal (if it exists) and
the leaf nodes.
However I feel that this patch is more of a workaround to the fact that
the clock framework today does not make DVFS transitions (or
coordinated, multi-clock rate change transitions) easy to control. The
generic "walk up the tree" algorithm might give you the right rate, but
perhaps using a non-validated combination of PLL frequency and
adjustable-rate dividers, or a combination that his higher jitter or is
more likely to unlock at higher temperatures, etc.
Back in the pre-CCF days lots of folks implemented this with "virtual"
clock nodes that simply called "clk_set_rate" or whatever on the
affected clocks, or even worse just banged a bunch of registers.
The cbus clock series for Tegra is also looks a little like this.
</rant>
Thomas,
Would a coordinated clock rate change method solve this problem for you
in place of the cpu-clock provider type? A poorly conceived call graph
for this might look something like:
clk_set_rate(div_arm2, 1000000);
-> if (div_arm2->coordinated == true)
clk_coordinate_rates(div_arm2, 1000000);
-> clk->ops->coordinate(div_arm2->hw, 1000000);
-> vendor_supplied_magic()
The vendor_supplied_magic() would be a callback that essentially calls
clk_set_rate() on all of the affected (coordinated) clocks. In your case
that looks like mout_core, div_core, div_core2, arm_clk and sclk_apll
for Exynos4.
The trick is that calling clk_set_rate() from any driver would initiate
this coordinated rate change, and then the exynos clock driver would set
up the coordinated clocks itself. No new API is introduced to drivers
(since it still uses clk_set_rate) and now a new clock provider doesn't
have to be invented every time we have a couple of clocks involved in a
DVFS transition.
Does this sound right to you or have I horribly misinterpreted the point
of these clock patches?
Thanks,
Mike
Hi Mike,
I have been testing this patch series on three exynos platforms for
about five days now and do not see any issues. There are some review
comments which I need to incorporate. So before that, I wanted to
check with you about the grouping of clocks approach in this patch
series. Is this approach something that we could use until the changes
for coordinated clocks are in place? This patch series also helps with
multi-platform support for exynos as well.
Tomasz, I have addressed most of your comments from v3. If you could
have a look at the v4 series as well, it would be helpful in making
this better.
Thanks,
Thomas.
quoted
Cc: Tomasz Figa <redacted>
Signed-off-by: Thomas Abraham <redacted>
---
drivers/clk/samsung/clk-exynos4.c | 25 +++++++++----------------
drivers/clk/samsung/clk-exynos5250.c | 12 ++++++------
include/dt-bindings/clock/exynos5250.h | 1 +
3 files changed, 16 insertions(+), 22 deletions(-)
From: Tomasz Figa <hidden> Date: 2014-05-16 23:57:35
Hi Thomas,
On 14.05.2014 03:11, Thomas Abraham wrote:
quoted hunk
From: Thomas Abraham <redacted>
With the addition of the new Samsung specific cpu-clock type, the
arm clock can be represented as a cpu-clock type and the independent
clock blocks that made up the arm clock can be removed.
Cc: Tomasz Figa <redacted>
Signed-off-by: Thomas Abraham <redacted>
---
drivers/clk/samsung/clk-exynos4.c | 25 +++++++++----------------
drivers/clk/samsung/clk-exynos5250.c | 12 ++++++------
include/dt-bindings/clock/exynos5250.h | 1 +
3 files changed, 16 insertions(+), 22 deletions(-)
@@ -572,8 +570,6 @@ static struct samsung_mux_clock exynos4x12_mux_clks[] __initdata = {/* list of divider clocks supported in all exynos4 soc's */staticstructsamsung_div_clockexynos4_div_clks[]__initdata={-DIV(0,"div_core","mout_core",DIV_CPU0,0,3),-DIV(0,"div_core2","div_core",DIV_CPU0,28,3),
Please don't remove these clocks, as they will be necessary to define
proper hierarchy CMU_CPU clock output block. In particular, access to
following clocks will be required:
div_corem0
div_corem1
div_cores
div_atb
div_periph
div_pclk_dbg
div_hpm
They might be implemented using normal dividers, but with
CLK_DIVIDER_READ_ONLY [1] and CLK_GET_RATE_NOCACHE flags.
[1] http://thread.gmane.org/gmane.linux.ports.arm.kernel/322977/focus=322978
This also means keeping mout_core clock defined, but with
CLK_MUX_READ_ONLY flag it should be fine.
I'd prefer name of this clock to be not changed to not cause conflicts
with other patches.
Similar comments apply to clk-exynos5250.
Best regards,
Tomasz
This is not the right value. The bottom 24-bits of MPIDR register on
Exynos4210 are 0x900 and 0x901 for CPU0 and CPU1 respectively.
Of course this won't work before fixing the GIC driver to not rely on
wrong assumptions, but I have already posted patches [1] and need to
address comments.
[1] http://thread.gmane.org/gmane.linux.kernel.samsung-soc/29573
From: Thomas Abraham <hidden> Date: 2014-05-14 01:11:15
From: Thomas Abraham <redacted>
Remove the platform device instantiation for Exynos specific cpufreq
driver and add the platform device for cpufreq-cpu0 driver.
Signed-off-by: Thomas Abraham <redacted>
---
arch/arm/mach-exynos/exynos.c | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
On Wednesday 14 May 2014 06:41:15 Thomas Abraham wrote:
quoted hunk
From: Thomas Abraham <redacted>
Remove the platform device instantiation for Exynos specific cpufreq
driver and add the platform device for cpufreq-cpu0 driver.
Signed-off-by: Thomas Abraham <redacted>
---
arch/arm/mach-exynos/exynos.c | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
Could we please come up with a way to probe this from DT in the cpufreq-cpu0
driver itself, so we don't have to add a device in every platform using it?
I realize you copied it from the other platforms using this driver, but
it still seems really wrong.
Arnd
On 14 May 2014 18:20, Arnd Bergmann [off-list ref] wrote:
Could we please come up with a way to probe this from DT in the cpufreq-cpu0
driver itself, so we don't have to add a device in every platform using it?
Its followed that way because DT Maintainers had strong objections
to creating virtual device nodes and haven't allowed creation of nodes
for cpufreq drivers.. For which there is no physical device, as CPU already
has a separate node..
Am Mittwoch, 14. Mai 2014, 18:35:29 schrieb Viresh Kumar:
On 14 May 2014 18:20, Arnd Bergmann [off-list ref] wrote:
quoted
Could we please come up with a way to probe this from DT in the
cpufreq-cpu0 driver itself, so we don't have to add a device in every
platform using it?
Its followed that way because DT Maintainers had strong objections
to creating virtual device nodes and haven't allowed creation of nodes
for cpufreq drivers.. For which there is no physical device, as CPU already
has a separate node..
as we already have the "enable-method" property for enabling/disabling cpus,
would something like a "scaling-method" be feasible?
On 14 May 2014 18:41, Heiko St?bner [off-list ref] wrote:
Am Mittwoch, 14. Mai 2014, 18:35:29 schrieb Viresh Kumar:
quoted
On 14 May 2014 18:20, Arnd Bergmann [off-list ref] wrote:
quoted
Could we please come up with a way to probe this from DT in the
cpufreq-cpu0 driver itself, so we don't have to add a device in every
platform using it?
quoted
Its followed that way because DT Maintainers had strong objections
to creating virtual device nodes and haven't allowed creation of nodes
for cpufreq drivers.. For which there is no physical device, as CPU already
has a separate node..
as we already have the "enable-method" property for enabling/disabling cpus,
would something like a "scaling-method" be feasible?
Lets see what DT maintainers have to say on this, I would rather go for a
more straight forward name: "scaling-driver" :) ..
On Wednesday 14 May 2014 18:44:46 Viresh Kumar wrote:
On 14 May 2014 18:41, Heiko St?bner [off-list ref] wrote:
quoted
Am Mittwoch, 14. Mai 2014, 18:35:29 schrieb Viresh Kumar:
quoted
On 14 May 2014 18:20, Arnd Bergmann [off-list ref] wrote:
quoted
Could we please come up with a way to probe this from DT in the
cpufreq-cpu0 driver itself, so we don't have to add a device in every
platform using it?
quoted
quoted
Its followed that way because DT Maintainers had strong objections
to creating virtual device nodes and haven't allowed creation of nodes
for cpufreq drivers.. For which there is no physical device, as CPU already
has a separate node..
as we already have the "enable-method" property for enabling/disabling cpus,
would something like a "scaling-method" be feasible?
Good idea to put it as a property into the CPU node.
Lets see what DT maintainers have to say on this, I would rather go for a
more straight forward name: "scaling-driver" ..
From: Rob Herring <hidden> Date: 2014-05-14 13:45:23
On Wed, May 14, 2014 at 8:18 AM, Arnd Bergmann [off-list ref] wrote:
On Wednesday 14 May 2014 18:44:46 Viresh Kumar wrote:
quoted
On 14 May 2014 18:41, Heiko St?bner [off-list ref] wrote:
quoted
Am Mittwoch, 14. Mai 2014, 18:35:29 schrieb Viresh Kumar:
quoted
On 14 May 2014 18:20, Arnd Bergmann [off-list ref] wrote:
quoted
Could we please come up with a way to probe this from DT in the
cpufreq-cpu0 driver itself, so we don't have to add a device in every
platform using it?
quoted
quoted
Its followed that way because DT Maintainers had strong objections
to creating virtual device nodes and haven't allowed creation of nodes
for cpufreq drivers.. For which there is no physical device, as CPU already
has a separate node..
as we already have the "enable-method" property for enabling/disabling cpus,
would something like a "scaling-method" be feasible?
Good idea to put it as a property into the CPU node.
We already have properties which indicate this driver can be used by a
platform: opp table and a clock for the cpu. If this information is
not sufficient to determine whether you can use this driver or not,
then you simply need to match against the platform. Perhaps the match
list should be a blacklist rather than a whitelist, so new platforms
work without a kernel change.
Alternatively, create a new OPP binding that addresses this and all
the other limitations in the current OPP binding.
quoted
Lets see what DT maintainers have to say on this, I would rather go for a
more straight forward name: "scaling-driver" ..
Both sound fine to me.
The fact that linux needs a way to create a platform device to enable
a certain driver is not a DT problem. I proposed a solution for how to
get this out of the platform code [1], but evidently we want people to
open code the exceptions and adding boilerplate helpers will just
encourage the exceptions.
Rob
[1] https://lkml.org/lkml/2013/10/30/30
On Wednesday 14 May 2014 08:45:23 Rob Herring wrote:
On Wed, May 14, 2014 at 8:18 AM, Arnd Bergmann [off-list ref] wrote:
quoted
On Wednesday 14 May 2014 18:44:46 Viresh Kumar wrote:
quoted
On 14 May 2014 18:41, Heiko St?bner [off-list ref] wrote:
quoted
Am Mittwoch, 14. Mai 2014, 18:35:29 schrieb Viresh Kumar:
quoted
On 14 May 2014 18:20, Arnd Bergmann [off-list ref] wrote:
quoted
Could we please come up with a way to probe this from DT in the
cpufreq-cpu0 driver itself, so we don't have to add a device in every
platform using it?
quoted
quoted
Its followed that way because DT Maintainers had strong objections
to creating virtual device nodes and haven't allowed creation of nodes
for cpufreq drivers.. For which there is no physical device, as CPU already
has a separate node..
as we already have the "enable-method" property for enabling/disabling cpus,
would something like a "scaling-method" be feasible?
Good idea to put it as a property into the CPU node.
We already have properties which indicate this driver can be used by a
platform: opp table and a clock for the cpu. If this information is
not sufficient to determine whether you can use this driver or not,
then you simply need to match against the platform. Perhaps the match
list should be a blacklist rather than a whitelist, so new platforms
work without a kernel change.
We'd not only need a blacklist, but also a way to tell whether we
want to use the cpu0 or the big/little implementation, which currently
have indistinguishable bindings.
Alternatively, create a new OPP binding that addresses this and all
the other limitations in the current OPP binding.
Yes.
quoted
quoted
Lets see what DT maintainers have to say on this, I would rather go for a
more straight forward name: "scaling-driver" ..
Both sound fine to me.
The fact that linux needs a way to create a platform device to enable
a certain driver is not a DT problem. I proposed a solution for how to
get this out of the platform code [1], but evidently we want people to
open code the exceptions and adding boilerplate helpers will just
encourage the exceptions.
I think the only benefit we have from using platform devices at all
for cpufreq (not for cpuidle, which has a similar problem) is module
autoloading. I think your patch doesn't actually help with that.
Arnd
Hi Arnd/Rob/Mike et al,
We didn't conclude anything out of this thread and so kicking it
again as we need to close bindings to support cpufreq-cpu0
better for platforms not sharing clock lines across all CPUs.
https://lkml.org/lkml/2014/7/1/358
On 14 May 2014 20:03, Arnd Bergmann [off-list ref] wrote:
On Wednesday 14 May 2014 08:45:23 Rob Herring wrote:
quoted
We already have properties which indicate this driver can be used by a
platform: opp table and a clock for the cpu. If this information is
There can be platform drivers which also depend on these properties
and picking cpufreq-cpu0 on this basis doesn't look correct.
quoted
not sufficient to determine whether you can use this driver or not,
then you simply need to match against the platform. Perhaps the match
list should be a blacklist rather than a whitelist, so new platforms
work without a kernel change.
We'd not only need a blacklist, but also a way to tell whether we
want to use the cpu0 or the big/little implementation, which currently
have indistinguishable bindings.
Correct and there can be other platform drivers which cannot use
cpufreq-cpu0 (though I am trying to force people to use cpufreq-cpu0
instead of a new driver).
Is something terribly wrong with having a property at 'cpus' node
which can point to the driver we want to use? Like:
cpus {
#address-cells = <1>;
#size-cells = <0>;
scaling-method = "cpufreq-cpu0"
cpu at 0 {
....
};
....
};
Or if we can reuse compatibility string some way.
[Copying mail from Mike]
On 15 May 2014 02:46, Mike Turquette [off-list ref] wrote:
The hardware property that matters for cpufreq-cpu0 users is that a
multi-core CPU uses a single clock input to scale frequency across all
of the cores in that cluster. So an accurate description is:
scaling-method = "clock-ganged"; //hardware-people-speak
Or,
scaling-method = "clock-shared"; //software-people-speak
Versus independently scalable CPUs in an SMP cluster:
scaling-method = "independent"; //x86, Krait, etc.
Or perhaps instead of "independent" at the parent "cpus" node we would
put the following in each cpu at N node:
scaling-method = "clock";
Or "psci" or "acpi" or whatever.
Thought exercise: for Hyperthreaded(tm) CPUs with 2 virtual cores for
every hard CPU (and multiple CPUs in a cluster):
scaling-method = "paired";
Or more simply, "hyperthreaded".
Probably we have mixed both the problems. We have two problems to
solve:
- Identifying which driver to probe for a platform, earlier explanation
I tried to gave were around that..
- Identifying if clocks are shared between CPUs? If yes which ones?
Probably Mike's suggestions were around this second problem, but
I still couldn't make out which CPUs share clock line from his
examples.
Please see if we can close this thread soon... Few platforms are waiting
to reuse cpufreq-cpu0 :)
--
viresh
From: Thomas Abraham <hidden> Date: 2014-05-14 14:03:56
On Wed, May 14, 2014 at 6:41 PM, Heiko St?bner [off-list ref] wrote:
Am Mittwoch, 14. Mai 2014, 18:35:29 schrieb Viresh Kumar:
quoted
On 14 May 2014 18:20, Arnd Bergmann [off-list ref] wrote:
quoted
Could we please come up with a way to probe this from DT in the
cpufreq-cpu0 driver itself, so we don't have to add a device in every
platform using it?
Its followed that way because DT Maintainers had strong objections
to creating virtual device nodes and haven't allowed creation of nodes
for cpufreq drivers.. For which there is no physical device, as CPU already
has a separate node..
as we already have the "enable-method" property for enabling/disabling cpus,
would something like a "scaling-method" be feasible?
"scaling-method" also sounds like a software specific property. Would
that be something that will be acceptable in dt?
On Wed, May 14, 2014 at 6:41 PM, Heiko St?bner [off-list ref] wrote:
quoted
Am Mittwoch, 14. Mai 2014, 18:35:29 schrieb Viresh Kumar:
quoted
On 14 May 2014 18:20, Arnd Bergmann [off-list ref] wrote:
quoted
Could we please come up with a way to probe this from DT in the
cpufreq-cpu0 driver itself, so we don't have to add a device in every
platform using it?
Its followed that way because DT Maintainers had strong objections
to creating virtual device nodes and haven't allowed creation of nodes
for cpufreq drivers.. For which there is no physical device, as CPU already
has a separate node..
as we already have the "enable-method" property for enabling/disabling cpus,
would something like a "scaling-method" be feasible?
"scaling-method" also sounds like a software specific property. Would
that be something that will be acceptable in dt?
How about dvfs-method ? But the value should not be based on the driver they
use, but something more generic.
Regards,
Sudeep
From: Thomas Abraham <hidden> Date: 2014-05-14 14:09:58
On Wed, May 14, 2014 at 6:20 PM, Arnd Bergmann [off-list ref] wrote:
On Wednesday 14 May 2014 06:41:15 Thomas Abraham wrote:
quoted
From: Thomas Abraham <redacted>
Remove the platform device instantiation for Exynos specific cpufreq
driver and add the platform device for cpufreq-cpu0 driver.
Signed-off-by: Thomas Abraham <redacted>
---
arch/arm/mach-exynos/exynos.c | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
Could we please come up with a way to probe this from DT in the cpufreq-cpu0
driver itself, so we don't have to add a device in every platform using it?
Okay, I don't have a solution for this as of now. Would this be
considered as a blocker for this series? I hope we could just live
with this for now.
Thanks,
Thomas.
I realize you copied it from the other platforms using this driver, but
it still seems really wrong.
Arnd
From: Tomasz Figa <hidden> Date: 2014-05-17 00:04:27
On 14.05.2014 03:11, Thomas Abraham wrote:
quoted hunk
From: Thomas Abraham <redacted>
Remove the platform device instantiation for Exynos specific cpufreq
driver and add the platform device for cpufreq-cpu0 driver.
Signed-off-by: Thomas Abraham <redacted>
---
arch/arm/mach-exynos/exynos.c | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
As a good intermediate step that can be completely replaced in future,
if necessary (as opposed to stable DT bindings...):
Acked-by: Tomasz Figa <redacted>
Best regards,
Tomasz