[PATCH 0/8] cpufreq: Introduce support for ST's cpufreq functionality

[PATCH 0/8] cpufreq: Introduce support for ST's cpufreq functionality

[Lee Jones]

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 1/8] ARM: STi: STiH407: Provide generic (safe) DVFS configuration

[Lee Jones]

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 <lee.jones@linaro.org>
---
 arch/arm/boot/dts/stih407-family.dtsi | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm/boot/dts/stih407-family.dtsi b/arch/arm/boot/dts/stih407-family.dtsi
index be201aa..2680ac7 100644
--- a/arch/arm/boot/dts/stih407-family.dtsi
+++ b/arch/arm/boot/dts/stih407-family.dtsi
@@ -22,11 +22,21 @@
 			device_type = "cpu";
 			compatible = "arm,cortex-a9";
 			reg = <0>;
+					 /* kHz     uV   */
+			operating-points = <1500000 0
+					    1200000 0
+					    800000  0
+					    500000  0>;
 		};
 		cpu@1 {
 			device_type = "cpu";
 			compatible = "arm,cortex-a9";
 			reg = <1>;
+					 /* kHz     uV   */
+			operating-points = <1500000 0
+					    1200000 0
+					    800000  0
+					    500000  0>;
 		};
 	};
 
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 2/8] ARM: STi: STiH407: Provide CPU with clocking information

[Lee Jones]

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/boot/dts/stih407-family.dtsi | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm/boot/dts/stih407-family.dtsi b/arch/arm/boot/dts/stih407-family.dtsi
index 2680ac7..fcab003 100644
--- a/arch/arm/boot/dts/stih407-family.dtsi
+++ b/arch/arm/boot/dts/stih407-family.dtsi
@@ -27,6 +27,10 @@
 					    1200000 0
 					    800000  0
 					    500000  0>;
+
+			clocks = <&clk_m_a9>;
+			clock-names = "cpu";
+			clock-latency = <100000>;
 		};
 		cpu@1 {
 			device_type = "cpu";
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 3/8] ARM: STi: STiH407: Link CPU with its voltage supply

[Lee Jones]

Used for Voltage Scaling using CPUFreq.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/boot/dts/stih407-family.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/boot/dts/stih407-family.dtsi b/arch/arm/boot/dts/stih407-family.dtsi
index fcab003..f48767e 100644
--- a/arch/arm/boot/dts/stih407-family.dtsi
+++ b/arch/arm/boot/dts/stih407-family.dtsi
@@ -31,6 +31,7 @@
 			clocks = <&clk_m_a9>;
 			clock-names = "cpu";
 			clock-latency = <100000>;
+			cpu0-supply = <&pwm_regulator>;
 		};
 		cpu@1 {
 			device_type = "cpu";
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 4/8] ARM: STi: STiH407: Provide a node for CPUFreq

[Lee Jones]

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/boot/dts/stih407-family.dtsi | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm/boot/dts/stih407-family.dtsi b/arch/arm/boot/dts/stih407-family.dtsi
index f48767e..f57fd83 100644
--- a/arch/arm/boot/dts/stih407-family.dtsi
+++ b/arch/arm/boot/dts/stih407-family.dtsi
@@ -98,6 +98,11 @@
 		ranges;
 		compatible = "simple-bus";
 
+		cpufreq {
+			compatible = "st,stih407-cpufreq";
+			st,syscfg = <&syscfg_core 0x8e0>;
+		};
+
 		restart {
 			compatible = "st,stih407-restart";
 			st,syscfg = <&syscfg_sbc_reg>;
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 4/8] ARM: STi: STiH407: Provide a node for CPUFreq

[Viresh Kumar]

[Ccing Rob]

On 22-06-15, 16:43, Lee Jones wrote:
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  arch/arm/boot/dts/stih407-family.dtsi | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/stih407-family.dtsi b/arch/arm/boot/dts/stih407-family.dtsi
> index f48767e..f57fd83 100644
> --- a/arch/arm/boot/dts/stih407-family.dtsi
> +++ b/arch/arm/boot/dts/stih407-family.dtsi
> @@ -98,6 +98,11 @@
>  		ranges;
>  		compatible = "simple-bus";
>  
> +		cpufreq {
> +			compatible = "st,stih407-cpufreq";
> +			st,syscfg = <&syscfg_core 0x8e0>;
> +		};

These virtual nodes aren't allowed in DT and you have added them
before the bindings patch :).

-- 
viresh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 4/8] ARM: STi: STiH407: Provide a node for CPUFreq

[Lee Jones]

On Tue, 23 Jun 2015, Viresh Kumar wrote:

> [Ccing Rob]

Rob is already Cc'ed.

> On 22-06-15, 16:43, Lee Jones wrote:
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> >  arch/arm/boot/dts/stih407-family.dtsi | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/arch/arm/boot/dts/stih407-family.dtsi b/arch/arm/boot/dts/stih407-family.dtsi
> > index f48767e..f57fd83 100644
> > --- a/arch/arm/boot/dts/stih407-family.dtsi
> > +++ b/arch/arm/boot/dts/stih407-family.dtsi
> > @@ -98,6 +98,11 @@
> >  		ranges;
> >  		compatible = "simple-bus";
> >  
> > +		cpufreq {
> > +			compatible = "st,stih407-cpufreq";
> > +			st,syscfg = <&syscfg_core 0x8e0>;
> > +		};
> 
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 4/8] ARM: STi: STiH407: Provide a node for CPUFreq

[Viresh Kumar]

On 23-06-15, 08:10, Lee Jones wrote:
> > 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

[PATCH 5/8] ARM: STi: STiH407: Move PWM nodes STiH407 => STiH407-family

[Lee Jones]

This also incorporates the STiH410.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/boot/dts/stih407-family.dtsi | 29 +++++++++++++++++++++++++++++
 arch/arm/boot/dts/stih407.dtsi        | 28 ----------------------------
 2 files changed, 29 insertions(+), 28 deletions(-)

diff --git a/arch/arm/boot/dts/stih407-family.dtsi b/arch/arm/boot/dts/stih407-family.dtsi
index f57fd83..a673386 100644
--- a/arch/arm/boot/dts/stih407-family.dtsi
+++ b/arch/arm/boot/dts/stih407-family.dtsi
@@ -569,5 +569,34 @@
 
 			status = "disabled";
 		};
+
+		/* COMMS PWM Module */
+		pwm0: pwm@9810000 {
+			compatible	= "st,sti-pwm";
+			status		= "okay";
+			#pwm-cells	= <2>;
+			reg		= <0x9810000 0x68>;
+			pinctrl-names	= "default";
+			pinctrl-0	= <&pinctrl_pwm0_chan0_default>;
+			clock-names	= "pwm";
+			clocks		= <&clk_sysin>;
+			st,pwm-num-chan = <1>;
+		};
+
+		/* SBC PWM Module */
+		pwm1: pwm@9510000 {
+			compatible	= "st,sti-pwm";
+			status		= "okay";
+			#pwm-cells	= <2>;
+			reg		= <0x9510000 0x68>;
+			pinctrl-names	= "default";
+			pinctrl-0	= <&pinctrl_pwm1_chan0_default
+					&pinctrl_pwm1_chan1_default
+					&pinctrl_pwm1_chan2_default
+					&pinctrl_pwm1_chan3_default>;
+			clock-names	= "pwm";
+			clocks		= <&clk_sysin>;
+			st,pwm-num-chan = <4>;
+		};
 	};
 };
diff --git a/arch/arm/boot/dts/stih407.dtsi b/arch/arm/boot/dts/stih407.dtsi
index 2c560fc3..3efa3b2 100644
--- a/arch/arm/boot/dts/stih407.dtsi
+++ b/arch/arm/boot/dts/stih407.dtsi
@@ -147,33 +147,5 @@
 				};
 			};
 		};
-
-		/* COMMS PWM Module */
-		pwm0: pwm@9810000 {
-			compatible	= "st,sti-pwm";
-			status		= "disabled";
-			#pwm-cells	= <2>;
-			reg		= <0x9810000 0x68>;
-			pinctrl-names	= "default";
-			pinctrl-0	= <&pinctrl_pwm0_chan0_default>;
-			clock-names	= "pwm";
-			clocks		= <&clk_sysin>;
-		};
-
-		/* SBC PWM Module */
-		pwm1: pwm@9510000 {
-			compatible	= "st,sti-pwm";
-			status		= "disabled";
-			#pwm-cells	= <2>;
-			reg		= <0x9510000 0x68>;
-			pinctrl-names	= "default";
-			pinctrl-0	= <&pinctrl_pwm1_chan0_default
-					&pinctrl_pwm1_chan1_default
-					&pinctrl_pwm1_chan2_default
-					&pinctrl_pwm1_chan3_default>;
-			clock-names	= "pwm";
-			clocks		= <&clk_sysin>;
-			st,pwm-num-chan = <4>;
-		};
 	};
 };
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 5/8] ARM: STi: STiH407: Move PWM nodes STiH407 => STiH407-family

[Viresh Kumar]

On 22-06-15, 16:43, Lee Jones wrote:
> This also incorporates the STiH410.

Any explanation to why this patch is part of this series ?

-- 
viresh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 5/8] ARM: STi: STiH407: Move PWM nodes STiH407 => STiH407-family

[Lee Jones]

On Tue, 23 Jun 2015, Viresh Kumar wrote:
> On 22-06-15, 16:43, Lee Jones wrote:
> > 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 5/8] ARM: STi: STiH407: Move PWM nodes STiH407 => STiH407-family

[Viresh Kumar]

On 23-06-15, 08:08, Lee Jones wrote:
> Because the CPUFreq regulator is controlled with PWM and CPUFreq will
> fail on STiH410 (my main development platform) without it.

I see, probably this should find a place in the cover-letter :)

> It's safe for you to ignore it though.

Thanks :)

-- 
viresh

[PATCH 6/8] ARM: multi_v7_defconfig: Enable support for PWM Regulators

[Lee Jones]

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/configs/multi_v7_defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/configs/multi_v7_defconfig b/arch/arm/configs/multi_v7_defconfig
index f632af0..6666973 100644
--- a/arch/arm/configs/multi_v7_defconfig
+++ b/arch/arm/configs/multi_v7_defconfig
@@ -365,6 +365,7 @@ CONFIG_REGULATOR_MAX8907=y
 CONFIG_REGULATOR_MAX8973=y
 CONFIG_REGULATOR_MAX77686=y
 CONFIG_REGULATOR_PALMAS=y
+CONFIG_REGULATOR_PWM=y
 CONFIG_REGULATOR_S2MPS11=y
 CONFIG_REGULATOR_S5M8767=y
 CONFIG_REGULATOR_TPS51632=y
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 7/8] cpufreq: st: Provide runtime initialised driver for ST's platforms

[Lee Jones]

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 <lee.jones@linaro.org>
---
 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

diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
index 4f3dbc8..1408884 100644
--- a/drivers/cpufreq/Kconfig.arm
+++ b/drivers/cpufreq/Kconfig.arm
@@ -258,6 +258,13 @@ config ARM_SPEAR_CPUFREQ
 	help
 	  This adds the CPUFreq driver support for SPEAr SOCs.
 
+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.
+
 config ARM_TEGRA_CPUFREQ
 	bool "TEGRA CPUFreq support"
 	depends on ARCH_TEGRA
diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
index cdce92a..7d3f47b 100644
--- a/drivers/cpufreq/Makefile
+++ b/drivers/cpufreq/Makefile
@@ -77,6 +77,7 @@ obj-$(CONFIG_ARM_S5PV210_CPUFREQ)	+= s5pv210-cpufreq.o
 obj-$(CONFIG_ARM_SA1100_CPUFREQ)	+= sa1100-cpufreq.o
 obj-$(CONFIG_ARM_SA1110_CPUFREQ)	+= sa1110-cpufreq.o
 obj-$(CONFIG_ARM_SPEAR_CPUFREQ)		+= spear-cpufreq.o
+obj-$(CONFIG_ARM_ST_CPUFREQ)		+= st-cpufreq.o
 obj-$(CONFIG_ARM_TEGRA_CPUFREQ)		+= tegra-cpufreq.o
 obj-$(CONFIG_ARM_VEXPRESS_SPC_CPUFREQ)	+= vexpress-spc-cpufreq.o
 
diff --git a/drivers/cpufreq/st-cpufreq.c b/drivers/cpufreq/st-cpufreq.c
new file mode 100644
index 0000000..167062f
--- /dev/null
+++ b/drivers/cpufreq/st-cpufreq.c
@@ -0,0 +1,450 @@
+/*
+ * Create CPUFreq OPP list
+ *
+ * Author: Ajit Pal Singh <ajitpal.singh@st.com>
+ *         Lee Jones <lee.jones@linaro.org>
+ *
+ * Copyright (C) 2015 STMicroelectronics (R&D) Limited
+ *
+ * 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.
+ */
+
+#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
+
+struct st_dvfs_tab {
+	unsigned int freq;
+	unsigned int avs;
+};
+
+/**
+ * ST CPUFreq Driver Data
+ *
+ * @dvfs_tab		Supported Voltage/Frequency table
+ * @dvfs_tab_count	Number of entries in the table above
+ * @pcode		Device's Process Code determines the running voltage
+ * @substrate		Device's Substrate version -- as above
+ * @regmap_eng		Engineering Syscon register map
+ *
+ */
+struct st_cpufreq_ddata {
+	struct st_dvfs_tab dvfs_tab[STI_DVFS_TAB_MAX];
+	int dvfs_tab_count;
+	int pcode;
+	int substrate;
+	struct regmap *regmap_eng;
+
+};
+
+struct sti_dvfs_cdata {
+	const struct reg_field *reg_fields;
+};
+
+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");
+	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");
+		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;
+		}
+	}
+
+	return 0;
+}
+
+static int st_cpufreq_fetch_regmap_field(struct platform_device *pdev,
+					 const struct reg_field *reg_fields,
+					 int pcode_offset, int field)
+{
+	struct st_cpufreq_ddata *ddata = platform_get_drvdata(pdev);
+	struct regmap_field *regmap_field;
+	struct reg_field reg_field = reg_fields[field];
+	unsigned int value;
+	int ret;
+
+	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");
+		return PTR_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");
+		return ret;
+	}
+
+	return value;
+}
+
+static const struct reg_field sti_stih407_dvfs_regfields[DVFS_MAX_REGFIELDS] = {
+	[PCODE]		= REG_FIELD(0, 16, 19),
+	[SUBSTRATE]	= REG_FIELD(0, 0, 2),
+};
+
+static struct of_device_id sti_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 */
+static void sti_cpufreq_get_pcode(struct platform_device *pdev)
+{
+	struct st_cpufreq_ddata *ddata = platform_get_drvdata(pdev);
+	struct device_node *np = pdev->dev.of_node;
+	const struct reg_field *reg_fields;
+	const struct of_device_id *match;
+	int pcode_offset;
+	int ret;
+
+	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");
+		goto set_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");
+		goto set_default;
+	}
+
+	match = of_match_node(sti_cpufreq_of_match, np);
+	if (!match) {
+		dev_warn(&pdev->dev, "Failed to match device\n");
+		goto set_default;
+	}
+	reg_fields = match->data;
+
+	ddata->pcode = st_cpufreq_fetch_regmap_field(pdev, reg_fields,
+						     pcode_offset,
+						     PCODE);
+	if (ddata->pcode < 0)
+		goto set_default;
+
+	ddata->substrate = st_cpufreq_fetch_regmap_field(pdev, reg_fields,
+							 pcode_offset,
+							 SUBSTRATE);
+	if (ddata->substrate < 0)
+		goto set_default;
+
+	return;
+
+set_default:
+	dev_warn(&pdev->dev,
+		 "Setting pcode to highest tolerance/voltage for safety\n");
+	ddata->pcode = 0;
+	ddata->substrate = 0;
+}
+
+static int sti_cpufreq_probe(struct platform_device *pdev)
+{
+	struct st_cpufreq_ddata *ddata;
+	int ret;
+
+	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);
+
+	return 0;
+}
+
+static struct platform_driver sti_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");
+
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 7/8] cpufreq: st: Provide runtime initialised driver for ST's platforms

[Viresh Kumar]

On 22-06-15, 16:43, Lee Jones wrote:
> +config ARM_ST_CPUFREQ
> +	bool "ST CPUFreq support"

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

> 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 ?

> +
> +	return 0;
> +}
> +
> +static int st_cpufreq_fetch_regmap_field(struct platform_device *pdev,
> +					 const struct reg_field *reg_fields,
> +					 int pcode_offset, int field)
> +{
> +	struct st_cpufreq_ddata *ddata = platform_get_drvdata(pdev);
> +	struct regmap_field *regmap_field;
> +	struct reg_field reg_field = reg_fields[field];
> +	unsigned int value;
> +	int ret;
> +
> +	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");
> +		return PTR_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");
> +		return ret;
> +	}
> +
> +	return value;
> +}
> +
> +static const struct reg_field sti_stih407_dvfs_regfields[DVFS_MAX_REGFIELDS] = {
> +	[PCODE]		= REG_FIELD(0, 16, 19),
> +	[SUBSTRATE]	= REG_FIELD(0, 0, 2),
> +};
> +
> +static struct of_device_id sti_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 */
> +static void sti_cpufreq_get_pcode(struct platform_device *pdev)
> +{
> +	struct st_cpufreq_ddata *ddata = platform_get_drvdata(pdev);
> +	struct device_node *np = pdev->dev.of_node;
> +	const struct reg_field *reg_fields;
> +	const struct of_device_id *match;
> +	int pcode_offset;
> +	int ret;
> +
> +	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");
> +		goto set_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");
> +		goto set_default;
> +	}
> +
> +	match = of_match_node(sti_cpufreq_of_match, np);

Are you planning to add more entries to this table? We are here as
probe() is called only after matching for this string.

> +	if (!match) {
> +		dev_warn(&pdev->dev, "Failed to match device\n");
> +		goto set_default;
> +	}
> +	reg_fields = match->data;
> +
> +	ddata->pcode = st_cpufreq_fetch_regmap_field(pdev, reg_fields,
> +						     pcode_offset,
> +						     PCODE);
> +	if (ddata->pcode < 0)
> +		goto set_default;
> +
> +	ddata->substrate = st_cpufreq_fetch_regmap_field(pdev, reg_fields,
> +							 pcode_offset,
> +							 SUBSTRATE);
> +	if (ddata->substrate < 0)
> +		goto set_default;

Maybe:

	if (ddata->substrate >= 0)
                return;

> +
> +	return;
> +
> +set_default:
> +	dev_warn(&pdev->dev,
> +		 "Setting pcode to highest tolerance/voltage for safety\n");
> +	ddata->pcode = 0;
> +	ddata->substrate = 0;
> +}
> +
> +static int sti_cpufreq_probe(struct platform_device *pdev)
> +{
> +	struct st_cpufreq_ddata *ddata;
> +	int ret;
> +
> +	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);
> +
> +	return 0;
> +}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 7/8] cpufreq: st: Provide runtime initialised driver for ST's platforms

[Lee Jones]

Thanks for your timely review Viresh.

On Tue, 23 Jun 2015, Viresh Kumar wrote:
> On 22-06-15, 16:43, Lee Jones wrote:
> > +config ARM_ST_CPUFREQ
> > +	bool "ST CPUFreq support"
> 
> 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.

> > +	depends on SOC_STIH407

[...]

> > +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.

[...]

> > +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.

> > +		return -EINVAL;

[...]

> > +	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.

> > +
> > +	return 0;
> > +}

[...]

> > +static const struct reg_field sti_stih407_dvfs_regfields[DVFS_MAX_REGFIELDS] = {
> > +	[PCODE]		= REG_FIELD(0, 16, 19),
> > +	[SUBSTRATE]	= REG_FIELD(0, 0, 2),
> > +};
> > +
> > +static struct of_device_id sti_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 */
> > +static void sti_cpufreq_get_pcode(struct platform_device *pdev)
> > +{
> > +	struct st_cpufreq_ddata *ddata = platform_get_drvdata(pdev);
> > +	struct device_node *np = pdev->dev.of_node;
> > +	const struct reg_field *reg_fields;
> > +	const struct of_device_id *match;
> > +	int pcode_offset;
> > +	int ret;
> > +
> > +	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");
> > +		goto set_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");
> > +		goto set_default;
> > +	}
> > +
> > +	match = of_match_node(sti_cpufreq_of_match, np);
> 
> Are you planning to add more entries to this table? We are here as
> probe() is called only after matching for this string.

Yes, when new platforms are enabled.  The information stored in .data
will be different.

> > +	if (!match) {
> > +		dev_warn(&pdev->dev, "Failed to match device\n");
> > +		goto set_default;
> > +	}
> > +	reg_fields = match->data;
> > +
> > +	ddata->pcode = st_cpufreq_fetch_regmap_field(pdev, reg_fields,
> > +						     pcode_offset,
> > +						     PCODE);
> > +	if (ddata->pcode < 0)
> > +		goto set_default;
> > +
> > +	ddata->substrate = st_cpufreq_fetch_regmap_field(pdev, reg_fields,
> > +							 pcode_offset,
> > +							 SUBSTRATE);
> > +	if (ddata->substrate < 0)
> > +		goto set_default;
> 
> Maybe:
> 
> 	if (ddata->substrate >= 0)
>                 return;

0 is a valid substrate value.

> > +
> > +	return;
> > +
> > +set_default:
> > +	dev_warn(&pdev->dev,
> > +		 "Setting pcode to highest tolerance/voltage for safety\n");
> > +	ddata->pcode = 0;
> > +	ddata->substrate = 0;
> > +}

[...]

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/

Re: [STLinux Kernel] [PATCH 7/8] cpufreq: st: Provide runtime initialised driver for ST's platforms

[Maxime Coquelin]

On 06/23/2015 09:16 AM, Lee Jones wrote:
> Thanks for your timely review Viresh.
>
> On Tue, 23 Jun 2015, Viresh Kumar wrote:
>> On 22-06-15, 16:43, Lee Jones wrote:
>>> +config ARM_ST_CPUFREQ
>>> +	bool "ST CPUFreq support"
>> 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.
I think you can use STI instead.

Regards,
Maxime
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 7/8] cpufreq: st: Provide runtime initialised driver for ST's platforms

[Viresh Kumar]

On 23-06-15, 08:16, Lee Jones wrote:
> Thanks for your timely review Viresh.

Your welcome Lee :)

> On Tue, 23 Jun 2015, Viresh Kumar wrote:
> > On 22-06-15, 16:43, Lee Jones wrote:
> > > +config ARM_ST_CPUFREQ
> > > +	bool "ST CPUFreq support"
> > 
> > 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.

> > > +	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..

> > 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..

> > > +	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

Re: [PATCH 7/8] cpufreq: st: Provide runtime initialised driver for ST's platforms

[Lee Jones]

On Tue, 23 Jun 2015, Viresh Kumar wrote:

> On 23-06-15, 08:16, Lee Jones wrote:
> > Thanks for your timely review Viresh.
> 
> Your welcome Lee :)
> 
> > On Tue, 23 Jun 2015, Viresh Kumar wrote:
> > > On 22-06-15, 16:43, Lee Jones wrote:
> > > > +config ARM_ST_CPUFREQ
> > > > +	bool "ST CPUFreq support"
> > > 
> > > 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.

> > > > +	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.

> > > 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.

> > > > +	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

Re: [PATCH 7/8] cpufreq: st: Provide runtime initialised driver for ST's platforms

[Viresh Kumar]

On 23-06-15, 09:27, Lee Jones wrote:
> 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.

Your call :)

-- 
viresh

Re: [PATCH 7/8] cpufreq: st: Provide runtime initialised driver for ST's platforms

[Lee Jones]

On Tue, 23 Jun 2015, Viresh Kumar wrote:

> On 23-06-15, 09:27, Lee Jones wrote:
> > 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.

> > Unless his is a big blocker for you, I would like to keep these
> > semantics.
> 
> No, the print is actually fine.

Perfect, thanks.

> > 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

Re: [PATCH 7/8] cpufreq: st: Provide runtime initialised driver for ST's platforms

[Paul Bolle]

On Mon, 2015-06-22 at 16:43 +0100, Lee Jones wrote:
> --- a/drivers/cpufreq/Kconfig.arm
> +++ b/drivers/cpufreq/Kconfig.arm
 
> +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.


> --- a/drivers/cpufreq/Makefile
> +++ b/drivers/cpufreq/Makefile

> +obj-$(CONFIG_ARM_ST_CPUFREQ)		+= st-cpufreq.o

> --- /dev/null
> +++ b/drivers/cpufreq/st-cpufreq.c

> + * 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.

> +#include <linux/module.h>

> +MODULE_DEVICE_TABLE(of, sti_cpufreq_of_match);
> 
> +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");


(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

Re: [PATCH 7/8] cpufreq: st: Provide runtime initialised driver for ST's platforms

[Lee Jones]

On Tue, 23 Jun 2015, Paul Bolle wrote:

> On Mon, 2015-06-22 at 16:43 +0100, Lee Jones wrote:
> > --- a/drivers/cpufreq/Kconfig.arm
> > +++ b/drivers/cpufreq/Kconfig.arm
>  
> > +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.
> 
> 
> > --- a/drivers/cpufreq/Makefile
> > +++ b/drivers/cpufreq/Makefile
> 
> > +obj-$(CONFIG_ARM_ST_CPUFREQ)		+= st-cpufreq.o
> 
> > --- /dev/null
> > +++ b/drivers/cpufreq/st-cpufreq.c
> 
> > + * 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.
> 
> > +#include <linux/module.h>
> 
> > +MODULE_DEVICE_TABLE(of, sti_cpufreq_of_match);
> > 
> > +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");
> 
> 
> (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

Re: [PATCH 7/8] cpufreq: st: Provide runtime initialised driver for ST's platforms

[Paul Bolle]

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

Re: [PATCH 7/8] cpufreq: st: Provide runtime initialised driver for ST's platforms

[Lee Jones]

On Tue, 23 Jun 2015, Paul Bolle wrote:
> 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?

Certainly does, thanks.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 8/8] dt: cpufreq: st: Provide bindings for ST's CPUFreq implementation

[Lee Jones]

Cc: devicetree@vger.kernel.org
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 .../devicetree/bindings/cpufreq/cpufreq-st.txt     | 48 ++++++++++++++++++++++
 1 file changed, 48 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-st.txt

diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-st.txt b/Documentation/devicetree/bindings/cpufreq/cpufreq-st.txt
new file mode 100644
index 0000000..cfa8952
--- /dev/null
+++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-st.txt
@@ -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@X' sub-nodes
+ - opp@{1..X} 		: Each 'opp@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]>;
+};
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 8/8] dt: cpufreq: st: Provide bindings for ST's CPUFreq implementation

[Viresh Kumar]

[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.

> Cc: devicetree@vger.kernel.org
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  .../devicetree/bindings/cpufreq/cpufreq-st.txt     | 48 ++++++++++++++++++++++
>  1 file changed, 48 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-st.txt
> 
> diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-st.txt b/Documentation/devicetree/bindings/cpufreq/cpufreq-st.txt
> new file mode 100644
> index 0000000..cfa8952
> --- /dev/null
> +++ b/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:
> +-------------------
> +- compatible		: Supported values are:
> +				"st,stih407-cpufreq"

Nodes for virtual devices aren't allowed in DT.

> +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@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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 8/8] dt: cpufreq: st: Provide bindings for ST's CPUFreq implementation

[Lee Jones]

> [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.

> > Cc: devicetree@vger.kernel.org
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> >  .../devicetree/bindings/cpufreq/cpufreq-st.txt     | 48 ++++++++++++++++++++++
> >  1 file changed, 48 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-st.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-st.txt b/Documentation/devicetree/bindings/cpufreq/cpufreq-st.txt
> > new file mode 100644
> > index 0000000..cfa8952
> > --- /dev/null
> > +++ b/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:
> > +-------------------
> > +- compatible		: Supported values are:
> > +				"st,stih407-cpufreq"
> 
> Nodes for virtual devices aren't allowed in DT.

Then why do Exynos, Spear, HREF and Snowball have CPUFreq nodes?

One rule for one ... ?

> > +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@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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 8/8] dt: cpufreq: st: Provide bindings for ST's CPUFreq implementation

[Viresh Kumar]

On 23-06-15, 08:06, Lee Jones wrote:
> > [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. :)

> > 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.

I don't know, but it seems obvious to me: Bindings first and then the
code.

> > > +Required properties:
> > > +-------------------
> > > +- compatible		: Supported values are:
> > > +				"st,stih407-cpufreq"
> > 
> > Nodes for virtual devices aren't allowed in DT.
> 
> 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

Re: [PATCH 8/8] dt: cpufreq: st: Provide bindings for ST's CPUFreq implementation

[Lee Jones]

On Tue, 23 Jun 2015, Viresh Kumar wrote:
> On 23-06-15, 08:06, Lee Jones wrote:
> > > [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.

> > > 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.
> 
> 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.

> > > > +Required properties:
> > > > +-------------------
> > > > +- compatible		: Supported values are:
> > > > +				"st,stih407-cpufreq"
> > > 
> > > Nodes for virtual devices aren't allowed in DT.
> > 
> > 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

Re: [PATCH 8/8] dt: cpufreq: st: Provide bindings for ST's CPUFreq implementation

[Javier Martinez Canillas]

Hello Lee and Viresh,

On Tue, Jun 23, 2015 at 10:38 AM, Lee Jones <lee.jones@linaro.org> wrote:
> On Tue, 23 Jun 2015, Viresh Kumar wrote:
>> On 23-06-15, 08:06, Lee Jones wrote:
>> >
>> > > 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

Re: [PATCH 8/8] dt: cpufreq: st: Provide bindings for ST's CPUFreq implementation

[Lee Jones]

On Tue, 23 Jun 2015, Javier Martinez Canillas wrote:

> Hello Lee and Viresh,
> 
> On Tue, Jun 23, 2015 at 10:38 AM, Lee Jones <lee.jones@linaro.org> wrote:
> > On Tue, 23 Jun 2015, Viresh Kumar wrote:
> >> On 23-06-15, 08:06, Lee Jones wrote:
> >> >
> >> > > 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

Re: [PATCH 8/8] dt: cpufreq: st: Provide bindings for ST's CPUFreq implementation

[Viresh Kumar]

On 23-06-15, 09:38, Lee Jones wrote:
> 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