From: Lee Jones <hidden> Date: 2015-06-22 15:44:09
This driver reads very platform specific information in from Device Tree
and translates it into frequency tables. The generic drivers then use the
tables to conduct frequency and voltage scaling in the normal way.
Lee Jones (8):
ARM: STi: STiH407: Provide generic (safe) DVFS configuration
ARM: STi: STiH407: Provide CPU with clocking information
ARM: STi: STiH407: Link CPU with its voltage supply
ARM: STi: STiH407: Provide a node for CPUFreq
ARM: STi: STiH407: Move PWM nodes STiH407 => STiH407-family
ARM: multi_v7_defconfig: Enable support for PWM Regulators
cpufreq: st: Provide runtime initialised driver for ST's platforms
dt: cpufreq: st: Provide bindings for ST's CPUFreq implementation
.../devicetree/bindings/cpufreq/cpufreq-st.txt | 48 +++
arch/arm/boot/dts/stih407-family.dtsi | 49 +++
arch/arm/boot/dts/stih407.dtsi | 28 --
arch/arm/configs/multi_v7_defconfig | 1 +
drivers/cpufreq/Kconfig.arm | 7 +
drivers/cpufreq/Makefile | 1 +
drivers/cpufreq/st-cpufreq.c | 450 +++++++++++++++++++++
7 files changed, 556 insertions(+), 28 deletions(-)
create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-st.txt
create mode 100644 drivers/cpufreq/st-cpufreq.c
--
1.9.1
From: Lee Jones <hidden> Date: 2015-06-22 15:44:38
This also incorporates the STiH410.
Signed-off-by: Lee Jones <redacted>
---
arch/arm/boot/dts/stih407-family.dtsi | 29 +++++++++++++++++++++++++++++
arch/arm/boot/dts/stih407.dtsi | 28 ----------------------------
2 files changed, 29 insertions(+), 28 deletions(-)
From: Lee Jones <hidden> Date: 2015-06-22 15:44:54
Used for Voltage Scaling using CPUFreq.
Signed-off-by: Lee Jones <redacted>
---
arch/arm/boot/dts/stih407-family.dtsi | 1 +
1 file changed, 1 insertion(+)
@@ -0,0 +1,48 @@+Binding for ST's CPUFreq driver+===============================++Required properties:+-------------------+- compatible : Supported values are:+ "st,stih407-cpufreq"++Required properties [for working voltage scaling]:+-------------------------------------------------++Located in CPUFreq's node:++- st,syscfg : Phandle to Major number register+ First cell: offset to major number+- st,syscfg-eng : Phandle to Minor number and Pcode registers+ First cell: offset to process code+ Second cell: offset to minor number++Located in CPU's node:++- st,opp-list : Bootloader provided node containing one or more 'opp at X' sub-nodes+ - opp@{1..X} : Each 'opp at X' subnode will contain the following properties:+ - st,avs : List of available voltages [uV] indexed by process code+ - st,freq : CPU frequency [Hz] for this OPP+ - st,cuts : Cut version this OPP is suitable for [0xFF means ALL]+ - st,substrate : Substrate version this OPP is suitable for [0xFF means ALL]++WARNING: The st,opp-list will be provided by the bootloader. Do not attempt to+ artificially synthesise the st,opp-list node or any of its descendants.+ They are very platform specific and may damage the hardware if created+ incorrectly.++Required properties [if the voltage scaling properties are missing]:+-------------------------------------------------------------------++Located in CPU's node:++- operating-points : [See: ../power/opp.txt]++Example:+-------++cpufreq {+ compatible = "st,stih407-cpufreq";+ st,syscfg = <&syscfg [major_offset]>;+ st,syscfg-eng = <&syscfg_eng [pcode_offset] [minor_offset]>;+};
From: Lee Jones <hidden> Date: 2015-06-22 15:45:41
The bootloader is charged with the responsibility to provide platform
specific Dynamic Voltage and Frequency Scaling (DVFS) information via
Device Tree. This driver takes the supplied configuration and loads
it into the generic OPP subsystem to it can be used as part of the
CPUFreq framework.
Signed-off-by: Lee Jones <redacted>
---
drivers/cpufreq/Kconfig.arm | 7 +
drivers/cpufreq/Makefile | 1 +
drivers/cpufreq/st-cpufreq.c | 450 +++++++++++++++++++++++++++++++++++++++++++
3 files changed, 458 insertions(+)
create mode 100644 drivers/cpufreq/st-cpufreq.c
@@ -0,0 +1,450 @@+/*+*CreateCPUFreqOPPlist+*+*Author:AjitPalSingh<ajitpal.singh@st.com>+*LeeJones<lee.jones@linaro.org>+*+*Copyright(C)2015STMicroelectronics(R&D)Limited+*+*Thisprogramisfreesoftware;youcanredistributeitand/ormodify+*itunderthetermsoftheGNUGeneralPublicLicenseaspublishedby+*theFreeSoftwareFoundation;eitherversion2oftheLicense,or+*(atyouroption)anylaterversion.+*/++#include<linux/cpu.h>+#include<linux/clk.h>+#include<linux/module.h>+#include<linux/io.h>+#include<linux/pm_opp.h>+#include<linux/of.h>+#include<linux/of_platform.h>+#include<linux/mfd/syscon.h>+#include<linux/platform_device.h>+#include<linux/regmap.h>+#include<linux/regulator/consumer.h>+#include<linux/sort.h>++#include<linux/cpufreq.h>++#define VERSION_SHIFT 28+#define PCODE_INDEX 1+#define MAJOR_ID_INDEX 1+#define MINOR_ID_INDEX 2++enum{+PCODE=0,+SUBSTRATE,+DVFS_MAX_REGFIELDS,+};++#define STI_DVFS_TAB_MAX 8++structst_dvfs_tab{+unsignedintfreq;+unsignedintavs;+};++/**+*STCPUFreqDriverData+*+*@dvfs_tabSupportedVoltage/Frequencytable+*@dvfs_tab_countNumberofentriesinthetableabove+*@pcodeDevice'sProcessCodedeterminestherunningvoltage+*@substrateDevice'sSubstrateversion--asabove+*@regmap_engEngineeringSysconregistermap+*+*/+structst_cpufreq_ddata{+structst_dvfs_tabdvfs_tab[STI_DVFS_TAB_MAX];+intdvfs_tab_count;+intpcode;+intsubstrate;+structregmap*regmap_eng;++};++structsti_dvfs_cdata{+conststructreg_field*reg_fields;+};++staticintst_cpufreq_cmp(constvoid*a,constvoid*b)+{+conststructst_dvfs_tab*a_tab=a,*b_tab=b;++if(a_tab->freq>b_tab->freq)+return-1;++if(a_tab->freq<b_tab->freq)+return1;++return0;+}++staticintst_cpufreq_check_if_matches(structdevice_node*child,+constchar*prop,+unsignedintmatch)+{+unsignedintdt_major,dt_minor;+unsignedintsoc_major,soc_minor;+const__be32*tmp;+unsignedintval;+intlen=0;+inti;++tmp=of_get_property(child,prop,&len);+if(!tmp||!len)+return-EINVAL;++val=be32_to_cpup(tmp);++len/=sizeof(u32);+if(len==1&&val==0xff)+/*+*If'cuts'or'substrate'valueis0xff,itmeansthat+*theentryisvalidforALLcutsandsubstrates+*/+gotomatchfound;++/* Check if this opp node is for us */+for(i=0;i<len;i++){+if(match==val)+gotomatchfound;++if(!strcmp(prop,"st,cuts")){+dt_major=val&0xff;;+dt_minor=val>>8&0xff;+soc_major=match&0xff;+soc_minor=match>>8&0xff;++if(dt_major==soc_major&&+(!dt_minor||(dt_minor==soc_minor)))+gotomatchfound;+}+val++;+}++/* No match found */+return-EINVAL;++matchfound:+return0;+}++staticintst_cpufreq_get_version(structplatform_device*pdev,+unsignedint*minor,unsignedint*major)+{+structst_cpufreq_ddata*ddata=platform_get_drvdata(pdev);+structdevice_node*np=pdev->dev.of_node;+structregmap*syscfg_regmap;+unsignedintminor_offset,major_offset;+unsignedintsocid,minid;+intret;++/* Get Major */+syscfg_regmap=syscon_regmap_lookup_by_phandle(np,"st,syscfg");+if(IS_ERR(syscfg_regmap)){+dev_err(&pdev->dev,+"No syscfg phandle specified in %s [%li]\n",+np->full_name,PTR_ERR(syscfg_regmap));+returnPTR_ERR(syscfg_regmap);+}++ret=of_property_read_u32_index(np,"st,syscfg",+MAJOR_ID_INDEX,&major_offset);+if(ret){+dev_err(&pdev->dev,+"No minor number offset provided in %s [%d]\n",+np->full_name,ret);+returnret;+}++ret=regmap_read(syscfg_regmap,major_offset,&socid);+if(ret)+returnret;++/* Get Minor */+ret=of_property_read_u32_index(np,"st,syscfg-eng",+MINOR_ID_INDEX,&minor_offset);+if(ret){+dev_err(&pdev->dev,"No minor number offset provided %s [%d]\n",+np->full_name,ret);+returnret;+}++ret=regmap_read(ddata->regmap_eng,minor_offset,&minid);+if(ret){+dev_err(&pdev->dev,+"Failed to read the minor number from syscon [%d]\n",+ret);+returnret;+}++*major=((socid>>VERSION_SHIFT)&0xf)+1;+*minor=minid&0xf;++return0;+}++staticintst_cpufreq_get_dvfs_info(structplatform_device*pdev)+{+structst_cpufreq_ddata*ddata=platform_get_drvdata(pdev);+structst_dvfs_tab*dvfs_tab=&ddata->dvfs_tab[0];+structdevice_node*np=pdev->dev.of_node;+structdevice_node*opplist,*opp;+unsignedintminor=0,major=0;+interr,ret=0;++opplist=of_get_child_by_name(np,"opp-list");+if(!opplist){+dev_err(&pdev->dev,"opp-list node missing\n");+return-ENODATA;+}++ret=st_cpufreq_get_version(pdev,&minor,&major);+if(ret){+dev_err(&pdev->dev,"No OPP match found for this platform\n");+returnret;+}++for_each_child_of_node(opplist,opp){+if(ddata->dvfs_tab_count==STI_DVFS_TAB_MAX){+dev_err(&pdev->dev,"Too many DVFS entries found\n");+ret=-EOVERFLOW;+break;+}++/* Cut version e.g. 2.0 [major.minor] */+err=st_cpufreq_check_if_matches(opp,"st,cuts",+(minor<<8)|major);+if(err)+continue;++ret=st_cpufreq_check_if_matches(opp,"st,substrate",+ddata->substrate);+if(err)+continue;++ret=of_property_read_u32(opp,"st,freq",&dvfs_tab->freq);+if(ret){+dev_err(&pdev->dev,"Can't read frequency: %d\n",ret);+gotoout;+}+dvfs_tab->freq*=1000;++ret=of_property_read_u32_index(opp,"st,avs",+ddata->pcode,+&dvfs_tab->avs);+if(ret){+dev_err(&pdev->dev,"Can't read AVS: %d\n",ret);+gotoout;+}++dvfs_tab++;+ddata->dvfs_tab_count++;+}++sort(&ddata->dvfs_tab[0],ddata->dvfs_tab_count,+sizeof(structst_dvfs_tab),st_cpufreq_cmp,NULL);++out:+of_node_put(opplist);++if(!ddata->dvfs_tab_count){+dev_err(&pdev->dev,"No suitable AVS table found\n");+return-EINVAL;+}++returnret;+}++staticintsti_cpufreq_voltage_scaling_init(structplatform_device*pdev)+{+structst_cpufreq_ddata*ddata=platform_get_drvdata(pdev);+structst_dvfs_tab*dvfs_tab=&ddata->dvfs_tab[0];+structdevice*cpu_dev;+structdev_pm_opp*opp;+unsignedlonghighest_freq=0;+intret;+inti;++cpu_dev=get_cpu_device(0);+if(!cpu_dev){+dev_err(&pdev->dev,"Failed to get cpu0 device\n");+return-ENODEV;+}++/* Populate OPP table with default non-AVS frequency values */+of_init_opp_table(cpu_dev);++/*+*Disable,butkeepdefaultvalues--thispreventstheframeworkfrom+*erroneouslyre-addingandenablingentrieswithmissingvoltagerates+*/+while(1){+highest_freq++;++opp=dev_pm_opp_find_freq_ceil(cpu_dev,&highest_freq);+if(IS_ERR(opp))+break;++ret=dev_pm_opp_disable(cpu_dev,highest_freq);+if(ret){+dev_err(&pdev->dev,"Failed to disable freq: %li\n",+highest_freq);+returnret;+}+}++for(i=0;i<ddata->dvfs_tab_count;i++,dvfs_tab++){+unsignedintf=dvfs_tab->freq*1000;+unsignedintv=dvfs_tab->avs*1000;++opp=dev_pm_opp_find_freq_exact(cpu_dev,f,false);++/* Remove existing voltage-less OPP entry */+if(!IS_ERR(opp))+dev_pm_opp_remove(cpu_dev,f);++/* Supply new fully populated OPP entry */+ret=dev_pm_opp_add(cpu_dev,f,v);+if(ret){+dev_err(&pdev->dev,"Failed to add OPP %u\n",f);+returnret;+}+}++return0;+}++staticintst_cpufreq_fetch_regmap_field(structplatform_device*pdev,+conststructreg_field*reg_fields,+intpcode_offset,intfield)+{+structst_cpufreq_ddata*ddata=platform_get_drvdata(pdev);+structregmap_field*regmap_field;+structreg_fieldreg_field=reg_fields[field];+unsignedintvalue;+intret;++reg_field.reg=pcode_offset;+regmap_field=devm_regmap_field_alloc(&pdev->dev,+ddata->regmap_eng,+reg_field);+if(IS_ERR(regmap_field)){+dev_err(&pdev->dev,"Failed to allocate reg field\n");+returnPTR_ERR(regmap_field);+}++ret=regmap_field_read(regmap_field,&value);+if(ret){+dev_err(&pdev->dev,"Failed to read %s code\n",+field?"SUBSTRATE":"PCODE");+returnret;+}++returnvalue;+}++staticconststructreg_fieldsti_stih407_dvfs_regfields[DVFS_MAX_REGFIELDS]={+[PCODE]=REG_FIELD(0,16,19),+[SUBSTRATE]=REG_FIELD(0,0,2),+};++staticstructof_device_idsti_cpufreq_of_match[]={+{+.compatible="st,stih407-cpufreq",+.data=&sti_stih407_dvfs_regfields,+},+{}+};+MODULE_DEVICE_TABLE(of,sti_cpufreq_of_match);++/* Find process code -- calibrated per-SoC */+staticvoidsti_cpufreq_get_pcode(structplatform_device*pdev)+{+structst_cpufreq_ddata*ddata=platform_get_drvdata(pdev);+structdevice_node*np=pdev->dev.of_node;+conststructreg_field*reg_fields;+conststructof_device_id*match;+intpcode_offset;+intret;++ddata->regmap_eng=syscon_regmap_lookup_by_phandle(np,"st,syscfg-eng");+if(IS_ERR(ddata->regmap_eng)){+dev_warn(&pdev->dev,"\"st,syscfg-eng\" not supplied\n");+gotoset_default;+}++ret=of_property_read_u32_index(np,"st,syscfg-eng",+PCODE_INDEX,&pcode_offset);+if(ret){+dev_warn(&pdev->dev,"Process code offset is required\n");+gotoset_default;+}++match=of_match_node(sti_cpufreq_of_match,np);+if(!match){+dev_warn(&pdev->dev,"Failed to match device\n");+gotoset_default;+}+reg_fields=match->data;++ddata->pcode=st_cpufreq_fetch_regmap_field(pdev,reg_fields,+pcode_offset,+PCODE);+if(ddata->pcode<0)+gotoset_default;++ddata->substrate=st_cpufreq_fetch_regmap_field(pdev,reg_fields,+pcode_offset,+SUBSTRATE);+if(ddata->substrate<0)+gotoset_default;++return;++set_default:+dev_warn(&pdev->dev,+"Setting pcode to highest tolerance/voltage for safety\n");+ddata->pcode=0;+ddata->substrate=0;+}++staticintsti_cpufreq_probe(structplatform_device*pdev)+{+structst_cpufreq_ddata*ddata;+intret;++ddata=devm_kzalloc(&pdev->dev,sizeof(*ddata),GFP_KERNEL);+if(!ddata)+return-ENOMEM;++platform_set_drvdata(pdev,ddata);++sti_cpufreq_get_pcode(pdev);++ret=st_cpufreq_get_dvfs_info(pdev);+if(ret)+dev_warn(&pdev->dev,"Not doing voltage scaling [%d]\n",ret);+else+sti_cpufreq_voltage_scaling_init(pdev);++platform_device_register_simple("cpufreq-dt",-1,NULL,0);++return0;+}++staticstructplatform_driversti_cpufreq={+.driver={+.name="sti_cpufreq",+.of_match_table=sti_cpufreq_of_match,+},+.probe=sti_cpufreq_probe,+};+module_platform_driver(sti_cpufreq);++MODULE_AUTHOR("Ajitpal Singh <ajitpal.singh@st.com>");+MODULE_DESCRIPTION("Creates an OPP list for cpufreq-cpu0 at runtime");+MODULE_LICENSE("GPL v2");+
From: Lee Jones <hidden> Date: 2015-06-22 15:46:15
You'll notice that the voltage cell is populated with 0's. Voltage
information is very platform specific, even depends on 'cut' and
'substrate' versions. Thus it is left blank for a generic (safe)
implementation. If other nodes/properties are provided by the
bootloader, the ST CPUFreq driver will over-ride these generic
values.
Signed-off-by: Lee Jones <redacted>
---
arch/arm/boot/dts/stih407-family.dtsi | 10 ++++++++++
1 file changed, 10 insertions(+)
[Adding Rob]
On 22-06-15, 16:43, Lee Jones wrote:
At least some description was required here on why you need additional
bindings are what are they.
Over that, this patch should have been present before any other
patches using these bindings.
quoted hunk
Cc: devicetree at vger.kernel.org
Signed-off-by: Lee Jones <redacted>
---
.../devicetree/bindings/cpufreq/cpufreq-st.txt | 48 ++++++++++++++++++++++
1 file changed, 48 insertions(+)
create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-st.txt
@@ -0,0 +1,48 @@+Binding for ST's CPUFreq driver+===============================
OPP-v2 bindings are out now and you can probably use them to make life
simple, they are part of Rafael's recent pull request:
https://lkml.org/lkml/2015/6/22/606
+Required properties [for working voltage scaling]:
+-------------------------------------------------
+
+Located in CPUFreq's node:
+
+- st,syscfg : Phandle to Major number register
+ First cell: offset to major number
+- st,syscfg-eng : Phandle to Minor number and Pcode registers
+ First cell: offset to process code
+ Second cell: offset to minor number
+
+Located in CPU's node:
+
+- st,opp-list : Bootloader provided node containing one or more 'opp at X' sub-nodes
I can see that this will be passed in from the bootloader. But at
least an example on how exactly things would actually look would have
been good. In logs if not in this file.
--
viresh
Isn't using ST just too generic? There are multiple SoCs ST has been
involved with, I have worked on a completely different series.
Probably a more relative string is required here, like stih407 ?
+ depends on SOC_STIH407
quoted hunk
diff --git a/drivers/cpufreq/st-cpufreq.c b/drivers/cpufreq/st-cpufreq.c
+static int st_cpufreq_cmp(const void *a, const void *b)
+{
+ const struct st_dvfs_tab *a_tab = a, *b_tab = b;
+
+ if (a_tab->freq > b_tab->freq)
+ return -1;
+
+ if (a_tab->freq < b_tab->freq)
+ return 1;
+
+ return 0;
+}
+
+static int st_cpufreq_check_if_matches(struct device_node *child,
+ const char *prop,
+ unsigned int match)
+{
+ unsigned int dt_major, dt_minor;
+ unsigned int soc_major, soc_minor;
+ const __be32 *tmp;
+ unsigned int val;
+ int len = 0;
+ int i;
+
+ tmp = of_get_property(child, prop , &len);
+ if (!tmp || !len)
+ return -EINVAL;
+
+ val = be32_to_cpup(tmp);
+
+ len /= sizeof(u32);
+ if (len == 1 && val == 0xff)
+ /*
+ * If 'cuts' or 'substrate' value is 0xff, it means that
+ * the entry is valid for ALL cuts and substrates
+ */
+ goto matchfound;
+
+ /* Check if this opp node is for us */
+ for (i = 0; i < len; i++) {
+ if (match == val)
+ goto matchfound;
+
+ if (!strcmp(prop, "st,cuts")) {
+ dt_major = val & 0xff;;
+ dt_minor = val >> 8 & 0xff;
+ soc_major = match & 0xff;
+ soc_minor = match >> 8 & 0xff;
+
+ if (dt_major == soc_major &&
+ (!dt_minor || (dt_minor == soc_minor)))
+ goto matchfound;
+ }
+ val++;
+ }
+
+ /* No match found */
+ return -EINVAL;
+
+matchfound:
+ return 0;
+}
+
+static int st_cpufreq_get_version(struct platform_device *pdev,
+ unsigned int *minor, unsigned int *major)
+{
+ struct st_cpufreq_ddata *ddata = platform_get_drvdata(pdev);
+ struct device_node *np = pdev->dev.of_node;
+ struct regmap *syscfg_regmap;
+ unsigned int minor_offset, major_offset;
+ unsigned int socid, minid;
+ int ret;
+
+ /* Get Major */
+ syscfg_regmap = syscon_regmap_lookup_by_phandle(np, "st,syscfg");
+ if (IS_ERR(syscfg_regmap)) {
+ dev_err(&pdev->dev,
+ "No syscfg phandle specified in %s [%li]\n",
+ np->full_name, PTR_ERR(syscfg_regmap));
+ return PTR_ERR(syscfg_regmap);
+ }
+
+ ret = of_property_read_u32_index(np, "st,syscfg",
+ MAJOR_ID_INDEX, &major_offset);
+ if (ret) {
+ dev_err(&pdev->dev,
+ "No minor number offset provided in %s [%d]\n",
+ np->full_name, ret);
+ return ret;
+ }
+
+ ret = regmap_read(syscfg_regmap, major_offset, &socid);
+ if (ret)
+ return ret;
+
+ /* Get Minor */
+ ret = of_property_read_u32_index(np, "st,syscfg-eng",
+ MINOR_ID_INDEX, &minor_offset);
+ if (ret) {
+ dev_err(&pdev->dev, "No minor number offset provided %s [%d]\n",
+ np->full_name, ret);
+ return ret;
+ }
+
+ ret = regmap_read(ddata->regmap_eng, minor_offset, &minid);
+ if (ret) {
+ dev_err(&pdev->dev,
+ "Failed to read the minor number from syscon [%d]\n",
+ ret);
+ return ret;
+ }
+
+ *major = ((socid >> VERSION_SHIFT) & 0xf) + 1;
+ *minor = minid & 0xf;
+
+ return 0;
+}
+
+static int st_cpufreq_get_dvfs_info(struct platform_device *pdev)
+{
+ struct st_cpufreq_ddata *ddata = platform_get_drvdata(pdev);
+ struct st_dvfs_tab *dvfs_tab = &ddata->dvfs_tab[0];
+ struct device_node *np = pdev->dev.of_node;
+ struct device_node *opplist, *opp;
+ unsigned int minor = 0, major = 0;
+ int err, ret = 0;
+
+ opplist = of_get_child_by_name(np, "opp-list");
st,opp-list ?
+ if (!opplist) {
+ dev_err(&pdev->dev, "opp-list node missing\n");
+ return -ENODATA;
+ }
+
+ ret = st_cpufreq_get_version(pdev, &minor, &major);
+ if (ret) {
+ dev_err(&pdev->dev, "No OPP match found for this platform\n");
+ return ret;
+ }
+
+ for_each_child_of_node(opplist, opp) {
+ if (ddata->dvfs_tab_count == STI_DVFS_TAB_MAX) {
+ dev_err(&pdev->dev, "Too many DVFS entries found\n");
+ ret = -EOVERFLOW;
+ break;
+ }
+
+ /* Cut version e.g. 2.0 [major.minor] */
+ err = st_cpufreq_check_if_matches(opp, "st,cuts",
+ (minor << 8) | major);
+ if (err)
+ continue;
+
+ ret = st_cpufreq_check_if_matches(opp, "st,substrate",
+ ddata->substrate);
+ if (err)
+ continue;
+
+ ret = of_property_read_u32(opp, "st,freq", &dvfs_tab->freq);
+ if (ret) {
+ dev_err(&pdev->dev, "Can't read frequency: %d\n", ret);
+ goto out;
+ }
+ dvfs_tab->freq *= 1000;
+
+ ret = of_property_read_u32_index(opp, "st,avs",
+ ddata->pcode,
+ &dvfs_tab->avs);
+ if (ret) {
+ dev_err(&pdev->dev, "Can't read AVS: %d\n", ret);
+ goto out;
+ }
+
+ dvfs_tab++;
+ ddata->dvfs_tab_count++;
+ }
+
+ sort(&ddata->dvfs_tab[0], ddata->dvfs_tab_count,
+ sizeof(struct st_dvfs_tab), st_cpufreq_cmp, NULL);
+
+out:
+ of_node_put(opplist);
+
+ if (!ddata->dvfs_tab_count) {
+ dev_err(&pdev->dev, "No suitable AVS table found\n");
Why is this an error? I thought in this case you will go ahead with
the normal OPP-table.
+ return -EINVAL;
+ }
+
+ return ret;
+}
+
+static int sti_cpufreq_voltage_scaling_init(struct platform_device *pdev)
+{
+ struct st_cpufreq_ddata *ddata = platform_get_drvdata(pdev);
+ struct st_dvfs_tab *dvfs_tab = &ddata->dvfs_tab[0];
+ struct device *cpu_dev;
+ struct dev_pm_opp *opp;
+ unsigned long highest_freq = 0;
+ int ret;
+ int i;
+
+ cpu_dev = get_cpu_device(0);
+ if (!cpu_dev) {
+ dev_err(&pdev->dev, "Failed to get cpu0 device\n");
+ return -ENODEV;
+ }
+
+ /* Populate OPP table with default non-AVS frequency values */
+ of_init_opp_table(cpu_dev);
+
+ /*
+ * Disable, but keep default values -- this prevents the framework from
+ * erroneously re-adding and enabling entries with missing voltage rates
+ */
+ while (1) {
+ highest_freq++;
+
+ opp = dev_pm_opp_find_freq_ceil(cpu_dev, &highest_freq);
+ if (IS_ERR(opp))
+ break;
+
+ ret = dev_pm_opp_disable(cpu_dev, highest_freq);
+ if (ret) {
+ dev_err(&pdev->dev, "Failed to disable freq: %li\n",
+ highest_freq);
+ return ret;
+ }
+ }
+
+ for (i = 0; i < ddata->dvfs_tab_count; i++, dvfs_tab++) {
+ unsigned int f = dvfs_tab->freq * 1000;
+ unsigned int v = dvfs_tab->avs * 1000;
+
+ opp = dev_pm_opp_find_freq_exact(cpu_dev, f, false);
+
+ /* Remove existing voltage-less OPP entry */
+ if (!IS_ERR(opp))
+ dev_pm_opp_remove(cpu_dev, f);
+
+ /* Supply new fully populated OPP entry */
+ ret = dev_pm_opp_add(cpu_dev, f, v);
+ if (ret) {
+ dev_err(&pdev->dev, "Failed to add OPP %u\n", f);
+ return ret;
+ }
+ }
So you have added new OPPs here, but cpufreq-dt will try to add old
OPPs. You must be getting lots of warnings ?
From: Lee Jones <hidden> Date: 2015-06-23 07:07:03
[Adding Rob]
Rob is not the only DT Maintainer, there are many of them. The DT
list was CC'ed, which they are all part of. Adding them all
separately is not required IMO.
On 22-06-15, 16:43, Lee Jones wrote:
At least some description was required here on why you need additional
bindings are what are they.
Sure, I can do that.
Over that, this patch should have been present before any other
patches using these bindings.
I've never heard that one before, but it's easy to re-order the set.
quoted
Cc: devicetree at vger.kernel.org
Signed-off-by: Lee Jones <redacted>
---
.../devicetree/bindings/cpufreq/cpufreq-st.txt | 48 ++++++++++++++++++++++
1 file changed, 48 insertions(+)
create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-st.txt
@@ -0,0 +1,48 @@+Binding for ST's CPUFreq driver+===============================
OPP-v2 bindings are out now and you can probably use them to make life
simple, they are part of Rafael's recent pull request:
https://lkml.org/lkml/2015/6/22/606
Then why do Exynos, Spear, HREF and Snowball have CPUFreq nodes?
One rule for one ... ?
quoted
+Required properties [for working voltage scaling]:
+-------------------------------------------------
+
+Located in CPUFreq's node:
+
+- st,syscfg : Phandle to Major number register
+ First cell: offset to major number
+- st,syscfg-eng : Phandle to Minor number and Pcode registers
+ First cell: offset to process code
+ Second cell: offset to minor number
+
+Located in CPU's node:
+
+- st,opp-list : Bootloader provided node containing one or more 'opp at X' sub-nodes
I can see that this will be passed in from the bootloader. But at
least an example on how exactly things would actually look would have
been good. In logs if not in this file.
Sure.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
From: Lee Jones <hidden> Date: 2015-06-23 07:08:36
On Tue, 23 Jun 2015, Viresh Kumar wrote:
On 22-06-15, 16:43, Lee Jones wrote:
quoted
This also incorporates the STiH410.
Any explanation to why this patch is part of this series ?
Because the CPUFreq regulator is controlled with PWM and CPUFreq will
fail on STiH410 (my main development platform) without it.
It's safe for you to ignore it though.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
These virtual nodes aren't allowed in DT and you have added them
before the bindings patch :).
This isn't a virtual node, but you bring up a good point.
The compatible string should also contain "syscon".
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Isn't using ST just too generic? There are multiple SoCs ST has been
involved with, I have worked on a completely different series.
Probably a more relative string is required here, like stih407 ?
This is ST's only CPUFreq implementation and is pretty board
agnostic. This particular driver only currently supports the STiH407
family, but internally it supports some others too. I'll have a chat
and see if we can make it more specific somehow.
quoted
+ depends on SOC_STIH407
[...]
quoted
+static int st_cpufreq_get_dvfs_info(struct platform_device *pdev)
+{
+ struct st_cpufreq_ddata *ddata = platform_get_drvdata(pdev);
+ struct st_dvfs_tab *dvfs_tab = &ddata->dvfs_tab[0];
+ struct device_node *np = pdev->dev.of_node;
+ struct device_node *opplist, *opp;
+ unsigned int minor = 0, major = 0;
+ int err, ret = 0;
+
+ opplist = of_get_child_by_name(np, "opp-list");
st,opp-list ?
Sure.
[...]
quoted
+out:
+ of_node_put(opplist);
+
+ if (!ddata->dvfs_tab_count) {
+ dev_err(&pdev->dev, "No suitable AVS table found\n");
Why is this an error? I thought in this case you will go ahead with
the normal OPP-table.
I've written it so it's an error within this function, as it makes the
function fail, but is downgraded by the caller to a warning and
gracefully bypassed to still allow frequency scaling.
quoted
+ return -EINVAL;
[...]
quoted
+ for (i = 0; i < ddata->dvfs_tab_count; i++, dvfs_tab++) {
+ unsigned int f = dvfs_tab->freq * 1000;
+ unsigned int v = dvfs_tab->avs * 1000;
+
+ opp = dev_pm_opp_find_freq_exact(cpu_dev, f, false);
+
+ /* Remove existing voltage-less OPP entry */
+ if (!IS_ERR(opp))
+ dev_pm_opp_remove(cpu_dev, f);
+
+ /* Supply new fully populated OPP entry */
+ ret = dev_pm_opp_add(cpu_dev, f, v);
+ if (ret) {
+ dev_err(&pdev->dev, "Failed to add OPP %u\n", f);
+ return ret;
+ }
+ }
So you have added new OPPs here, but cpufreq-dt will try to add old
OPPs. You must be getting lots of warnings ?
Yes, we recieve the 'duplicate OPPs detected' warning, but there is
nothing we can do about that.
Isn't using ST just too generic? There are multiple SoCs ST has been
involved with, I have worked on a completely different series.
Probably a more relative string is required here, like stih407 ?
This is ST's only CPUFreq implementation and is pretty board
agnostic. This particular driver only currently supports the STiH407
family, but internally it supports some others too. I'll have a chat
and see if we can make it more specific somehow.
Rob is not the only DT Maintainer, there are many of them. The DT
list was CC'ed, which they are all part of. Adding them all
separately is not required IMO.
I didn't Cc him because you missed him, but because we have been
discussing opp-v2 bindings recently and this was somehow related to
that. :)
quoted
On 22-06-15, 16:43, Lee Jones wrote:
At least some description was required here on why you need additional
bindings are what are they.
Sure, I can do that.
quoted
Over that, this patch should have been present before any other
patches using these bindings.
I've never heard that one before, but it's easy to re-order the set.
I don't know, but it seems obvious to me: Bindings first and then the
code.
Then why do Exynos, Spear, HREF and Snowball have CPUFreq nodes?
One rule for one ... ?
Not really, but I got a bit confused now with your reply.
So, what I meant when I wrote: "Nodes for virtual devices aren't
allowed in DT", was that we aren't supposed to do something like:
cpufreq {
...
}
in DT as cpufreq isn't a device here. A CPU is a device and that can
contain whatever property we feel is reasonable.
What SPEAr and Exyons did was putting something in the cpu-node. Not a
node for cpufreq device itself. Couldn't find HREF and snowball's
bindings though..
--
viresh
These virtual nodes aren't allowed in DT and you have added them
before the bindings patch :).
This isn't a virtual node, but you bring up a good point.
Hmm, wrong choice of words. Sorry..
So, its a node for a virtual device :). The device is CPU and it
already has a node for itself. And so such nodes were discouraged
earlier by DT maintainers.
--
viresh
+config ARM_ST_CPUFREQ
+> > bool "ST CPUFreq support"
+> > depends on SOC_STIH407
+> > help
+> > OPP list for cpufreq-dt driver can be provided through DT or can be
+> > created at runtime. Select this if you want create OPP list at runtime.
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+MODULE_AUTHOR("Ajitpal Singh [off-list ref]");
+MODULE_DESCRIPTION("Creates an OPP list for cpufreq-cpu0 at runtime");
+MODULE_LICENSE("GPL v2");
(There's a mismatch between the license used in the comment at the top
of this file and the ident used in the MODULE_LICENSE() macro. See
include/linux/module.h.)
st-cpufreq.o can only be built-in. But the code contains of few lines
that are only useful if the code can be modular. Was ARM_ST_CPUFREQ
perhaps meant to be tristate?
Thanks,
Paul Bolle
Isn't using ST just too generic? There are multiple SoCs ST has been
involved with, I have worked on a completely different series.
Probably a more relative string is required here, like stih407 ?
This is ST's only CPUFreq implementation and is pretty board
agnostic. This particular driver only currently supports the STiH407
family, but internally it supports some others too. I'll have a chat
and see if we can make it more specific somehow.
So, SPEAr is also from ST. And it already have a driver for itself.
quoted
quoted
+ if (!ddata->dvfs_tab_count) {
+ dev_err(&pdev->dev, "No suitable AVS table found\n");
Why is this an error? I thought in this case you will go ahead with
the normal OPP-table.
I've written it so it's an error within this function, as it makes the
function fail, but is downgraded by the caller to a warning and
gracefully bypassed to still allow frequency scaling.
Not that, I was asking about the print. I thought we will still try to
find OPP from the CPU node and a warning or a error might not be the
right choice. You can surely add a debug print. Currently you are
doing a dev_err() here, followed by a dev_warn() I think..
quoted
So you have added new OPPs here, but cpufreq-dt will try to add old
OPPs. You must be getting lots of warnings ?
Yes, we recieve the 'duplicate OPPs detected' warning, but there is
nothing we can do about that.
:)
OPP-v2 will get that solved too..
quoted
quoted
+ if (ddata->substrate < 0)
+ goto set_default;
Maybe:
if (ddata->substrate >= 0)
return;
0 is a valid substrate value.
I had >= in the comparison. Wasn't that right?
And I was just suggesting that a single return can be used instead of
if (xyz)
goto set_default;
return;
--
viresh
Isn't using ST just too generic? There are multiple SoCs ST has been
involved with, I have worked on a completely different series.
Probably a more relative string is required here, like stih407 ?
This is ST's only CPUFreq implementation and is pretty board
agnostic. This particular driver only currently supports the STiH407
family, but internally it supports some others too. I'll have a chat
and see if we can make it more specific somehow.
So, SPEAr is also from ST. And it already have a driver for itself.
Sure. I will use STI as suggested by Maxime.
quoted
quoted
quoted
+ if (!ddata->dvfs_tab_count) {
+ dev_err(&pdev->dev, "No suitable AVS table found\n");
Why is this an error? I thought in this case you will go ahead with
the normal OPP-table.
I've written it so it's an error within this function, as it makes the
function fail, but is downgraded by the caller to a warning and
gracefully bypassed to still allow frequency scaling.
Not that, I was asking about the print. I thought we will still try to
find OPP from the CPU node and a warning or a error might not be the
right choice. You can surely add a debug print. Currently you are
doing a dev_err() here, followed by a dev_warn() I think..
Okay, but the reasoning is the same. I consider the function to have
failed, but the over-all failure culminates in just a warning that
voltage scaling has indeed failed, but we can still go on with
frequency scaling.
Unless his is a big blocker for you, I would like to keep these
semantics.
quoted
quoted
So you have added new OPPs here, but cpufreq-dt will try to add old
OPPs. You must be getting lots of warnings ?
Yes, we recieve the 'duplicate OPPs detected' warning, but there is
nothing we can do about that.
:)
OPP-v2 will get that solved too..
I'll take another look at them to see if there is anything we can
use.
quoted
quoted
quoted
+ if (ddata->substrate < 0)
+ goto set_default;
Maybe:
if (ddata->substrate >= 0)
return;
0 is a valid substrate value.
I had >= in the comparison. Wasn't that right?
Oh, you reversed the condition, I see now.
And I was just suggesting that a single return can be used instead of
So technically you are correct, but it makes the code slightly more
confusing IMHO. Yes, it's one more line of code, but it's worth it to
add clarity.
if (xyz)
goto set_default;
return;
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
+config ARM_ST_CPUFREQ
+> > bool "ST CPUFreq support"
+> > depends on SOC_STIH407
+> > help
+> > OPP list for cpufreq-dt driver can be provided through DT or can be
+> > created at runtime. Select this if you want create OPP list at runtime.
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+MODULE_AUTHOR("Ajitpal Singh [off-list ref]");
+MODULE_DESCRIPTION("Creates an OPP list for cpufreq-cpu0 at runtime");
+MODULE_LICENSE("GPL v2");
(There's a mismatch between the license used in the comment at the top
of this file and the ident used in the MODULE_LICENSE() macro. See
include/linux/module.h.)
st-cpufreq.o can only be built-in. But the code contains of few lines
that are only useful if the code can be modular. Was ARM_ST_CPUFREQ
perhaps meant to be tristate?
All noted. Will fix. Thanks Paul.
BTW, do you have a script that does this now, or are you still
hand-rolling these?
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Okay, but the reasoning is the same. I consider the function to have
failed, but the over-all failure culminates in just a warning that
voltage scaling has indeed failed, but we can still go on with
frequency scaling.
Ahh, I thought that the other opp-table will also have voltages.
Unless his is a big blocker for you, I would like to keep these
semantics.
No, the print is actually fine.
So technically you are correct, but it makes the code slightly more
confusing IMHO. Yes, it's one more line of code, but it's worth it to
add clarity.
From: Lee Jones <hidden> Date: 2015-06-23 08:39:10
On Tue, 23 Jun 2015, Viresh Kumar wrote:
On 23-06-15, 08:06, Lee Jones wrote:
quoted
quoted
[Adding Rob]
Rob is not the only DT Maintainer, there are many of them. The DT
list was CC'ed, which they are all part of. Adding them all
separately is not required IMO.
I didn't Cc him because you missed him, but because we have been
discussing opp-v2 bindings recently and this was somehow related to
that. :)
Okay, fair point.
quoted
quoted
On 22-06-15, 16:43, Lee Jones wrote:
At least some description was required here on why you need additional
bindings are what are they.
Sure, I can do that.
quoted
Over that, this patch should have been present before any other
patches using these bindings.
I've never heard that one before, but it's easy to re-order the set.
I don't know, but it seems obvious to me: Bindings first and then the
code.
Do you always write your documentation before implementing a
feature?
Surely it goes;
Requirements Gathering
Plan and Prepare
Implement
Test
Document
Deliver
;)
... but as I say, I can re-order if required. It's really not a problem.
Then why do Exynos, Spear, HREF and Snowball have CPUFreq nodes?
One rule for one ... ?
Not really, but I got a bit confused now with your reply.
So, what I meant when I wrote: "Nodes for virtual devices aren't
allowed in DT", was that we aren't supposed to do something like:
cpufreq {
...
}
in DT as cpufreq isn't a device here. A CPU is a device and that can
contain whatever property we feel is reasonable.
What SPEAr and Exyons did was putting something in the cpu-node. Not a
node for cpufreq device itself. Couldn't find HREF and snowball's
bindings though..
That's not what it looks like to me:
git grep -C20 "compatible.*cpufreq" -- arch
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
From: Javier Martinez Canillas <javier@dowhile0.org> Date: 2015-06-23 08:53:04
Hello Lee and Viresh,
On Tue, Jun 23, 2015 at 10:38 AM, Lee Jones [off-list ref] wrote:
On Tue, 23 Jun 2015, Viresh Kumar wrote:
quoted
On 23-06-15, 08:06, Lee Jones wrote:
quoted
quoted
Over that, this patch should have been present before any other
patches using these bindings.
I've never heard that one before, but it's easy to re-order the set.
I don't know, but it seems obvious to me: Bindings first and then the
code.
Do you always write your documentation before implementing a
feature?
Surely it goes;
Requirements Gathering
Plan and Prepare
Implement
Test
Document
Deliver
;)
... but as I say, I can re-order if required. It's really not a problem.
This is actually documented in
Documentation/devicetree/bindings/submitting-patches.txt:
...
3) The Documentation/ portion of the patch should come in the series before
the code implementing the binding.
....
The rationale AFAIU is that it is easier to review the implementation
of a binding after reading the DT binding doc since then you can see
if the code matches what the DT binding describes.
Best regards,
Javier
From: Lee Jones <hidden> Date: 2015-06-23 08:59:17
On Tue, 23 Jun 2015, Javier Martinez Canillas wrote:
Hello Lee and Viresh,
On Tue, Jun 23, 2015 at 10:38 AM, Lee Jones [off-list ref] wrote:
quoted
On Tue, 23 Jun 2015, Viresh Kumar wrote:
quoted
On 23-06-15, 08:06, Lee Jones wrote:
quoted
quoted
Over that, this patch should have been present before any other
patches using these bindings.
I've never heard that one before, but it's easy to re-order the set.
I don't know, but it seems obvious to me: Bindings first and then the
code.
Do you always write your documentation before implementing a
feature?
Surely it goes;
Requirements Gathering
Plan and Prepare
Implement
Test
Document
Deliver
;)
... but as I say, I can re-order if required. It's really not a problem.
This is actually documented in
Documentation/devicetree/bindings/submitting-patches.txt:
...
3) The Documentation/ portion of the patch should come in the series before
the code implementing the binding.
....
The rationale AFAIU is that it is easier to review the implementation
of a binding after reading the DT binding doc since then you can see
if the code matches what the DT binding describes.
Fair enough. Can't argue with that. :)
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Do you always write your documentation before implementing a
feature?
Surely it goes;
Requirements Gathering
Plan and Prepare
Implement
Test
Document
Deliver
;)
DT bindings aren't just simple documentation of how things are
working. I do things in the way you wrote earlier though :)
That's not what it looks like to me:
git grep -C20 "compatible.*cpufreq" -- arch
Yeah, that's wrong. They got in much earlier and the bindings weren't
well thought through.
But they are discouraged today ..
--
viresh
From: Lee Jones <hidden> Date: 2015-06-23 09:00:40
On Tue, 23 Jun 2015, Viresh Kumar wrote:
On 23-06-15, 09:27, Lee Jones wrote:
quoted
Okay, but the reasoning is the same. I consider the function to have
failed, but the over-all failure culminates in just a warning that
voltage scaling has indeed failed, but we can still go on with
frequency scaling.
Ahh, I thought that the other opp-table will also have voltages.
Nope. See patch 1.
quoted
Unless his is a big blocker for you, I would like to keep these
semantics.
No, the print is actually fine.
Perfect, thanks.
quoted
So technically you are correct, but it makes the code slightly more
confusing IMHO. Yes, it's one more line of code, but it's worth it to
add clarity.
Your call :)
Thanks again.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
From: Paul Bolle <hidden> Date: 2015-06-23 20:03:32
On Tue, 2015-06-23 at 09:28 +0100, Lee Jones wrote:
BTW, do you have a script that does this now, or are you still
hand-rolling these?
There was a script detecting Kconfig problems that is basically
retired. (The messages I sent were always manually written.) That
script's function is now performed, much better, by the undertaker
-checkpatch bot (courtesy of a group of people apparently all
affiliated with the university of Erlangen, Germany). We're in virtual
contact but that group is doing fine so I'm basically not contributing.
Anyhow, my reply to your patch was the result of a manual scan for a
small set of common (and mostly trivial) mistakes in patches that I try
to do regularly. All pretty basic stuff, otherwise I wouldn't been able
to spot these mistakes.
Does this answer your question?
Paul Bolle
From: Lee Jones <hidden> Date: 2015-06-24 07:33:23
On Tue, 23 Jun 2015, Paul Bolle wrote:
On Tue, 2015-06-23 at 09:28 +0100, Lee Jones wrote:
quoted
BTW, do you have a script that does this now, or are you still
hand-rolling these?
There was a script detecting Kconfig problems that is basically
retired. (The messages I sent were always manually written.) That
script's function is now performed, much better, by the undertaker
-checkpatch bot (courtesy of a group of people apparently all
affiliated with the university of Erlangen, Germany). We're in virtual
contact but that group is doing fine so I'm basically not contributing.
Anyhow, my reply to your patch was the result of a manual scan for a
small set of common (and mostly trivial) mistakes in patches that I try
to do regularly. All pretty basic stuff, otherwise I wouldn't been able
to spot these mistakes.
Does this answer your question?
Certainly does, thanks.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog