Patchwork [v2,2/2] ARM: imx: enable imx6q-cpufreq support

login
register
mail settings
Submitter Shawn Guo
Date Jan. 10, 2013, 8:34 a.m.
Message ID <1357806863-6899-3-git-send-email-shawn.guo@linaro.org>
Download mbox | patch
Permalink /patch/210951/
State New
Headers show

Comments

Shawn Guo - Jan. 10, 2013, 8:34 a.m.
Update operating-points per hardware document and add support for
1 GHz and 1.2 GHz frequencies.

400 MHz, 800 MHz and 1 GHz should be supported by all i.MX6Q chips,
while 1.2 GHz support needs to know from OTP fuse bit.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 arch/arm/boot/dts/imx6q.dtsi   |   19 ++++++++----
 arch/arm/mach-imx/mach-imx6q.c |   65 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 78 insertions(+), 6 deletions(-)
Bedia, Vaibhav - Jan. 10, 2013, 10:50 a.m.
Hi,

On Thu, Jan 10, 2013 at 14:04:23, Shawn Guo wrote:
> Update operating-points per hardware document and add support for
> 1 GHz and 1.2 GHz frequencies.
> 
> 400 MHz, 800 MHz and 1 GHz should be supported by all i.MX6Q chips,
> while 1.2 GHz support needs to know from OTP fuse bit.
> 
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> ---
>  arch/arm/boot/dts/imx6q.dtsi   |   19 ++++++++----
>  arch/arm/mach-imx/mach-imx6q.c |   65 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 78 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/imx6q.dtsi b/arch/arm/boot/dts/imx6q.dtsi
> index d6265ca..17c5618 100644
> --- a/arch/arm/boot/dts/imx6q.dtsi
> +++ b/arch/arm/boot/dts/imx6q.dtsi
> @@ -38,12 +38,18 @@
>  			next-level-cache = <&L2>;
>  			operating-points = <
>  				/* kHz    uV */
> -				792000  1100000
> +				996000  1250000
> +				792000  1150000
>  				396000  950000
> -				198000  850000
>  			>;

[...]

>  
> +#define OCOTP_CFG3			0x440
> +#define OCOTP_CFG3_SPEED_SHIFT		16
> +#define OCOTP_CFG3_SPEED_1P2GHZ		0x3
> +
> +static void __init imx6q_opp_check_1p2ghz(struct device *cpu_dev)
> +{
> +	struct device_node *np;
> +	void __iomem *base;
> +	u32 val;
> +
> +	np = of_find_compatible_node(NULL, NULL, "fsl,imx6q-ocotp");
> +	if (!np) {
> +		pr_warn("failed to find ocotp node\n");
> +		return;
> +	}
> +
> +	base = of_iomap(np, 0);
> +	if (!base) {
> +		pr_warn("failed to map ocotp\n");
> +		goto put_node;
> +	}
> +
> +	val = readl_relaxed(base + OCOTP_CFG3);
> +	val >>= OCOTP_CFG3_SPEED_SHIFT;
> +	if ((val & 0x3) == OCOTP_CFG3_SPEED_1P2GHZ)
> +		if (opp_add(cpu_dev, 1200000000, 1275000))
> +			pr_warn("failed to add 1.2 GHz operating point\n");
> +

I just happened to be thinking about the problem of enabling additional OPPs based on
some Si rev info for TI's AM335x. 

The other approach could be to register additional OPPs and mark then as disabled to begin with.
A platform specific hook could then be used to selectively enable the OPPs for based on the Si rev
info.

If this approach was already discussed sorry for the noise.

Regards,
Vaibhav
Shawn Guo - Jan. 10, 2013, 11:07 a.m.
On Thu, Jan 10, 2013 at 10:50:25AM +0000, Bedia, Vaibhav wrote:
...
> > +static void __init imx6q_opp_check_1p2ghz(struct device *cpu_dev)
> > +{
> > +	struct device_node *np;
> > +	void __iomem *base;
> > +	u32 val;
> > +
> > +	np = of_find_compatible_node(NULL, NULL, "fsl,imx6q-ocotp");
> > +	if (!np) {
> > +		pr_warn("failed to find ocotp node\n");
> > +		return;
> > +	}
> > +
> > +	base = of_iomap(np, 0);
> > +	if (!base) {
> > +		pr_warn("failed to map ocotp\n");
> > +		goto put_node;
> > +	}
> > +
> > +	val = readl_relaxed(base + OCOTP_CFG3);
> > +	val >>= OCOTP_CFG3_SPEED_SHIFT;
> > +	if ((val & 0x3) == OCOTP_CFG3_SPEED_1P2GHZ)
> > +		if (opp_add(cpu_dev, 1200000000, 1275000))
> > +			pr_warn("failed to add 1.2 GHz operating point\n");
> > +
> 
> I just happened to be thinking about the problem of enabling additional OPPs based on
> some Si rev info for TI's AM335x. 
> 
> The other approach could be to register additional OPPs and mark then as disabled to begin with.
> A platform specific hook could then be used to selectively enable the OPPs for based on the Si rev
> info.

May I ask the advantage of this approach?  I hate to create
platform_data merely for passing a platform specific hook to driver.

Shawn
Bedia, Vaibhav - Jan. 10, 2013, 1:02 p.m.
On Thu, Jan 10, 2013 at 16:37:45, Shawn Guo wrote:
> On Thu, Jan 10, 2013 at 10:50:25AM +0000, Bedia, Vaibhav wrote:
> ...
> > > +static void __init imx6q_opp_check_1p2ghz(struct device *cpu_dev)
> > > +{
> > > +	struct device_node *np;
> > > +	void __iomem *base;
> > > +	u32 val;
> > > +
> > > +	np = of_find_compatible_node(NULL, NULL, "fsl,imx6q-ocotp");
> > > +	if (!np) {
> > > +		pr_warn("failed to find ocotp node\n");
> > > +		return;
> > > +	}
> > > +
> > > +	base = of_iomap(np, 0);
> > > +	if (!base) {
> > > +		pr_warn("failed to map ocotp\n");
> > > +		goto put_node;
> > > +	}
> > > +
> > > +	val = readl_relaxed(base + OCOTP_CFG3);
> > > +	val >>= OCOTP_CFG3_SPEED_SHIFT;
> > > +	if ((val & 0x3) == OCOTP_CFG3_SPEED_1P2GHZ)
> > > +		if (opp_add(cpu_dev, 1200000000, 1275000))
> > > +			pr_warn("failed to add 1.2 GHz operating point\n");
> > > +
> > 
> > I just happened to be thinking about the problem of enabling additional OPPs based on
> > some Si rev info for TI's AM335x. 
> > 
> > The other approach could be to register additional OPPs and mark then as disabled to begin with.
> > A platform specific hook could then be used to selectively enable the OPPs for based on the Si rev
> > info.
> 
> May I ask the advantage of this approach?  I hate to create
> platform_data merely for passing a platform specific hook to driver.
> 

In the current approach the OPP data is split across DT and kernel code. If you take the
other approach all OPP entries can reside in DT and for someone just looking at that file
there's no confusion about what the kernel could potentially support. Whether a particular
an OPP should be supported is best decided at runtime.

Also, IMHO letting platforms invoke opp_add() is open to abuse and platforms should be
restricted to invoking opp_enable/disable to indicate what's possible on a particular Si rev. 

Regards,
Vaibhav
Shawn Guo - Jan. 10, 2013, 1:20 p.m.
On Thu, Jan 10, 2013 at 01:02:30PM +0000, Bedia, Vaibhav wrote:
> In the current approach the OPP data is split across DT and kernel code. If you take the
> other approach all OPP entries can reside in DT and for someone just looking at that file
> there's no confusion about what the kernel could potentially support. Whether a particular
> an OPP should be supported is best decided at runtime.
> 
Listing the OPP that some Si rev can not support in DT is also
a confusion to people who is just looking at DTS.  To me, the approach
is not really doing anything better on this aspect.

> Also, IMHO letting platforms invoke opp_add() is open to abuse and platforms should be
> restricted to invoking opp_enable/disable to indicate what's possible on a particular Si rev. 
> 
I do not see anything wrong with platform adding the OPP it can support.
Isn't omap_init_opp_table() being called by platform code?  I do not see
omap-cpufreq driver is calling a platform hook to do that.

Sorry, I'm not really a fan of platform hook, and I have spent past
couple years to minimize the use of it when converting IMX family to
device tree.

Shawn
Bedia, Vaibhav - Jan. 10, 2013, 2:02 p.m.
On Thu, Jan 10, 2013 at 18:50:55, Shawn Guo wrote:
> On Thu, Jan 10, 2013 at 01:02:30PM +0000, Bedia, Vaibhav wrote:
> > In the current approach the OPP data is split across DT and kernel code. If you take the
> > other approach all OPP entries can reside in DT and for someone just looking at that file
> > there's no confusion about what the kernel could potentially support. Whether a particular
> > an OPP should be supported is best decided at runtime.
> > 
> Listing the OPP that some Si rev can not support in DT is also
> a confusion to people who is just looking at DTS.  To me, the approach
> is not really doing anything better on this aspect.
> 

I still think putting the OPP data in a single place is better.

> > Also, IMHO letting platforms invoke opp_add() is open to abuse and platforms should be
> > restricted to invoking opp_enable/disable to indicate what's possible on a particular Si rev. 
> > 
> I do not see anything wrong with platform adding the OPP it can support.
> Isn't omap_init_opp_table() being called by platform code?  I do not see
> omap-cpufreq driver is calling a platform hook to do that.
> 
> Sorry, I'm not really a fan of platform hook, and I have spent past
> couple years to minimize the use of it when converting IMX family to
> device tree.
> 

I don't know what the OMAP plans are but with DT + generic driver adoption I would expect most
of it to go away. And adding the platform hook with the right DT data is just preparing
things for the future with 2 potential users right now.

If OPP data across Si rev starts looking very different all entries can be placed in a single
place and the platform code enables what are relevant. Unless there's a Si rev specific blob a bunch
of opp_add() calls ignoring what was passed defeats the purpose of moving the information out of the
kernel.
 
Regards,
Vaibhav

Patch

diff --git a/arch/arm/boot/dts/imx6q.dtsi b/arch/arm/boot/dts/imx6q.dtsi
index d6265ca..17c5618 100644
--- a/arch/arm/boot/dts/imx6q.dtsi
+++ b/arch/arm/boot/dts/imx6q.dtsi
@@ -38,12 +38,18 @@ 
 			next-level-cache = <&L2>;
 			operating-points = <
 				/* kHz    uV */
-				792000  1100000
+				996000  1250000
+				792000  1150000
 				396000  950000
-				198000  850000
 			>;
 			clock-latency = <61036>; /* two CLK32 periods */
-			cpu0-supply = <&reg_cpu>;
+			clocks = <&clks 104>, <&clks 6>, <&clks 16>,
+				 <&clks 17>, <&clks 170>;
+			clock-names = "arm", "pll2_pfd2_396m", "step",
+				      "pll1_sw", "pll1_sys";
+			arm-supply = <&reg_arm>;
+			pu-supply = <&reg_pu>;
+			soc-supply = <&reg_soc>;
 		};
 
 		cpu@1 {
@@ -471,7 +477,7 @@ 
 					anatop-max-voltage = <2750000>;
 				};
 
-				reg_cpu: regulator-vddcore@140 {
+				reg_arm: regulator-vddcore@140 {
 					compatible = "fsl,anatop-regulator";
 					regulator-name = "cpu";
 					regulator-min-microvolt = <725000>;
@@ -485,7 +491,7 @@ 
 					anatop-max-voltage = <1450000>;
 				};
 
-				regulator-vddpu@140 {
+				reg_pu: regulator-vddpu@140 {
 					compatible = "fsl,anatop-regulator";
 					regulator-name = "vddpu";
 					regulator-min-microvolt = <725000>;
@@ -499,7 +505,7 @@ 
 					anatop-max-voltage = <1450000>;
 				};
 
-				regulator-vddsoc@140 {
+				reg_soc: regulator-vddsoc@140 {
 					compatible = "fsl,anatop-regulator";
 					regulator-name = "vddsoc";
 					regulator-min-microvolt = <725000>;
@@ -965,6 +971,7 @@ 
 			};
 
 			ocotp@021bc000 {
+				compatible = "fsl,imx6q-ocotp";
 				reg = <0x021bc000 0x4000>;
 			};
 
diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c
index 4eb1b3a..16f9a13 100644
--- a/arch/arm/mach-imx/mach-imx6q.c
+++ b/arch/arm/mach-imx/mach-imx6q.c
@@ -12,6 +12,7 @@ 
 
 #include <linux/clk.h>
 #include <linux/clkdev.h>
+#include <linux/cpu.h>
 #include <linux/cpuidle.h>
 #include <linux/delay.h>
 #include <linux/export.h>
@@ -22,6 +23,7 @@ 
 #include <linux/of_address.h>
 #include <linux/of_irq.h>
 #include <linux/of_platform.h>
+#include <linux/opp.h>
 #include <linux/phy.h>
 #include <linux/regmap.h>
 #include <linux/micrel_phy.h>
@@ -209,9 +211,72 @@  static struct cpuidle_driver imx6q_cpuidle_driver = {
 	.state_count		= 1,
 };
 
+#define OCOTP_CFG3			0x440
+#define OCOTP_CFG3_SPEED_SHIFT		16
+#define OCOTP_CFG3_SPEED_1P2GHZ		0x3
+
+static void __init imx6q_opp_check_1p2ghz(struct device *cpu_dev)
+{
+	struct device_node *np;
+	void __iomem *base;
+	u32 val;
+
+	np = of_find_compatible_node(NULL, NULL, "fsl,imx6q-ocotp");
+	if (!np) {
+		pr_warn("failed to find ocotp node\n");
+		return;
+	}
+
+	base = of_iomap(np, 0);
+	if (!base) {
+		pr_warn("failed to map ocotp\n");
+		goto put_node;
+	}
+
+	val = readl_relaxed(base + OCOTP_CFG3);
+	val >>= OCOTP_CFG3_SPEED_SHIFT;
+	if ((val & 0x3) == OCOTP_CFG3_SPEED_1P2GHZ)
+		if (opp_add(cpu_dev, 1200000000, 1275000))
+			pr_warn("failed to add 1.2 GHz operating point\n");
+
+put_node:
+	of_node_put(np);
+}
+
+static void __init imx6q_opp_init(struct device *cpu_dev)
+{
+	struct device_node *np;
+
+	np = of_find_node_by_path("/cpus/cpu@0");
+	if (!np) {
+		pr_warn("failed to find cpu0 node\n");
+		return;
+	}
+
+	cpu_dev->of_node = np;
+	if (of_init_opp_table(cpu_dev)) {
+		pr_warn("failed to init OPP table\n");
+		goto put_node;
+	}
+
+	imx6q_opp_check_1p2ghz(cpu_dev);
+
+put_node:
+	of_node_put(np);
+}
+
+struct platform_device imx6q_cpufreq_pdev = {
+	.name = "imx6q-cpufreq",
+};
+
 static void __init imx6q_init_late(void)
 {
 	imx_cpuidle_init(&imx6q_cpuidle_driver);
+
+	if (IS_ENABLED(CONFIG_ARM_IMX6Q_CPUFREQ)) {
+		imx6q_opp_init(&imx6q_cpufreq_pdev.dev);
+		platform_device_register(&imx6q_cpufreq_pdev);
+	}
 }
 
 static void __init imx6q_map_io(void)