[PATCH 1/4] iMX6SL frequency table

[PATCH 1/4] iMX6SL frequency table

[John Tobias]

    Device tree for iMX6SL doesn't have an existing cpu frequency table.

Signed-off-by: John Tobias <john.tobias.ph@gmail.com>
---
 arch/arm/boot/dts/imx6sl.dtsi | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/arch/arm/boot/dts/imx6sl.dtsi b/arch/arm/boot/dts/imx6sl.dtsi
index 28558f1..0a2c73c 100644
--- a/arch/arm/boot/dts/imx6sl.dtsi
+++ b/arch/arm/boot/dts/imx6sl.dtsi
@@ -38,6 +38,20 @@
 			device_type = "cpu";
 			reg = <0x0>;
 			next-level-cache = <&L2>;
+			operating-points = <
+				/* kHz    uV */
+				996000  1250000 /* for consumer grade only */
+				792000  1150000
+				396000  1050000
+			>;
+			clock-latency = <61036>; /* two CLK32 periods */
+			clocks = <&clks IMX6SL_CLK_ARM>, <&clks IMX6SL_CLK_PLL2_PFD2>, <&clks IMX6SL_CLK_STEP>,
+				 <&clks IMX6SL_CLK_PLL1_SW>, <&clks IMX6SL_CLK_PLL1_SYS>;
+			clock-names = "arm", "pll2_pfd2_396m", "step",
+				      "pll1_sw", "pll1_sys";
+			arm-supply = <&reg_arm>;
+			pu-supply = <&reg_pu>;
+			soc-supply = <&reg_soc>;
 		};
 	};
 
-- 
1.8.3.2

[PATCH 2/4] Moving of_init_opp_table

[John Tobias]

    Moving of_init_opp_table from mach-imx6q.c to imx6q-cpufreq.c in order to avoid adding it to mach-imx6sl.c.
    Just incase the cpu frequency scaling is enabled on iMX6SL.

Signed-off-by: John Tobias <john.tobias.ph@gmail.com>
---
 drivers/cpufreq/imx6q-cpufreq.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c
index 4b3f18e..2eea3d9 100644
--- a/drivers/cpufreq/imx6q-cpufreq.c
+++ b/drivers/cpufreq/imx6q-cpufreq.c
@@ -166,6 +166,11 @@ static int imx6q_cpufreq_probe(struct platform_device *pdev)
 		return -ENOENT;
 	}
 
+	if (of_init_opp_table(cpu_dev)) {
+		dev_err(cpu_dev,"failed to init OPP table\n");
+		return -ENODEV;
+	}
+
 	arm_clk = devm_clk_get(cpu_dev, "arm");
 	pll1_sys_clk = devm_clk_get(cpu_dev, "pll1_sys");
 	pll1_sw_clk = devm_clk_get(cpu_dev, "pll1_sw");
-- 
1.8.3.2

Re: [PATCH 2/4] Moving of_init_opp_table

[Shawn Guo]

On Tue, Dec 17, 2013 at 05:17:29PM -0800, John Tobias wrote:
>     Moving of_init_opp_table from mach-imx6q.c to imx6q-cpufreq.c in order to avoid adding it to mach-imx6sl.c.
>     Just incase the cpu frequency scaling is enabled on iMX6SL.

Drop the leading spaces and wrap the lines around 72 column.

> 
> Signed-off-by: John Tobias <john.tobias.ph@gmail.com>
> ---
>  drivers/cpufreq/imx6q-cpufreq.c | 5 +++++

For this patch, a proper patch subject prefix would be 'cpufreq: imx6q: '
You can use 'git log' to find out what a proper prefix for a file/patch
should be.

>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c
> index 4b3f18e..2eea3d9 100644
> --- a/drivers/cpufreq/imx6q-cpufreq.c
> +++ b/drivers/cpufreq/imx6q-cpufreq.c
> @@ -166,6 +166,11 @@ static int imx6q_cpufreq_probe(struct platform_device *pdev)
>  		return -ENOENT;
>  	}
>  
> +	if (of_init_opp_table(cpu_dev)) {
> +		dev_err(cpu_dev,"failed to init OPP table\n");
> +		return -ENODEV;
> +	}
> +

Before doing that in the driver, you should check and make sure that
platform hasn't supply a opp table.

Shawn

>  	arm_clk = devm_clk_get(cpu_dev, "arm");
>  	pll1_sys_clk = devm_clk_get(cpu_dev, "pll1_sys");
>  	pll1_sw_clk = devm_clk_get(cpu_dev, "pll1_sw");
> -- 
> 1.8.3.2
> 

Re: [PATCH 2/4] Moving of_init_opp_table

[John Tobias]

Ahh ok I think I got what you mean here..

I should call opp_get_opp_count(cpu_dev) to check if the platform
supplied it. If not, call the of_init_opp_table.


On Tue, Dec 17, 2013 at 11:43 PM, Shawn Guo <shawn.guo@linaro.org> wrote:
> On Tue, Dec 17, 2013 at 05:17:29PM -0800, John Tobias wrote:
>>     Moving of_init_opp_table from mach-imx6q.c to imx6q-cpufreq.c in order to avoid adding it to mach-imx6sl.c.
>>     Just incase the cpu frequency scaling is enabled on iMX6SL.
>
> Drop the leading spaces and wrap the lines around 72 column.
>
>>
>> Signed-off-by: John Tobias <john.tobias.ph@gmail.com>
>> ---
>>  drivers/cpufreq/imx6q-cpufreq.c | 5 +++++
>
> For this patch, a proper patch subject prefix would be 'cpufreq: imx6q: '
> You can use 'git log' to find out what a proper prefix for a file/patch
> should be.
>
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c
>> index 4b3f18e..2eea3d9 100644
>> --- a/drivers/cpufreq/imx6q-cpufreq.c
>> +++ b/drivers/cpufreq/imx6q-cpufreq.c
>> @@ -166,6 +166,11 @@ static int imx6q_cpufreq_probe(struct platform_device *pdev)
>>               return -ENOENT;
>>       }
>>
>> +     if (of_init_opp_table(cpu_dev)) {
>> +             dev_err(cpu_dev,"failed to init OPP table\n");
>> +             return -ENODEV;
>> +     }
>> +
>
> Before doing that in the driver, you should check and make sure that
> platform hasn't supply a opp table.
>
> Shawn
>
>>       arm_clk = devm_clk_get(cpu_dev, "arm");
>>       pll1_sys_clk = devm_clk_get(cpu_dev, "pll1_sys");
>>       pll1_sw_clk = devm_clk_get(cpu_dev, "pll1_sw");
>> --
>> 1.8.3.2
>>
>

[PATCH 3/4] Removing imx6q_opp_init

[John Tobias]

    Since of_init_opp_table is in imx6q-cpufreq.c, the imx6q_opp_init is no longer needed anymore.
    The only question right now is the imx6q_opp_check_1p2ghz function.

    Is the function below are acceptible under imx6q_init_late?.

    if (!cpu_dev)
        imx6q_opp_check_1p2ghz(cpu_dev);

    If not, what is the better way to impelement because the said function is board specific?.

Signed-off-by: John Tobias <john.tobias.ph@gmail.com>
---
 arch/arm/mach-imx/mach-imx6q.c | 30 ++++--------------------------
 1 file changed, 4 insertions(+), 26 deletions(-)

diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c
index d0cfb22..f5d9e04 100644
--- a/arch/arm/mach-imx/mach-imx6q.c
+++ b/arch/arm/mach-imx/mach-imx6q.c
@@ -183,31 +183,6 @@ put_node:
 	of_node_put(np);
 }
 
-static void __init imx6q_opp_init(void)
-{
-	struct device_node *np;
-	struct device *cpu_dev = get_cpu_device(0);
-
-	if (!cpu_dev) {
-		pr_warn("failed to get cpu0 device\n");
-		return;
-	}
-	np = of_node_get(cpu_dev->of_node);
-	if (!np) {
-		pr_warn("failed to find cpu0 node\n");
-		return;
-	}
-
-	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);
-}
 
 static struct platform_device imx6q_cpufreq_pdev = {
 	.name = "imx6q-cpufreq",
@@ -215,6 +190,7 @@ static struct platform_device imx6q_cpufreq_pdev = {
 
 static void __init imx6q_init_late(void)
 {
+	struct device *cpu_dev = get_cpu_device(0);
 	/*
 	 * WAIT mode is broken on TO 1.0 and 1.1, so there is no point
 	 * to run cpuidle on them.
@@ -223,8 +199,10 @@ static void __init imx6q_init_late(void)
 		imx6q_cpuidle_init();
 
 	if (IS_ENABLED(CONFIG_ARM_IMX6Q_CPUFREQ)) {
-		imx6q_opp_init();
 		platform_device_register(&imx6q_cpufreq_pdev);
+		
+		if (!cpu_dev)
+			imx6q_opp_check_1p2ghz(cpu_dev);
 	}
 }
 
-- 
1.8.3.2

Re: [PATCH 3/4] Removing imx6q_opp_init

[Shawn Guo]

On Tue, Dec 17, 2013 at 05:17:30PM -0800, John Tobias wrote:
>     Since of_init_opp_table is in imx6q-cpufreq.c, the imx6q_opp_init is no longer needed anymore.
>     The only question right now is the imx6q_opp_check_1p2ghz function.
> 
>     Is the function below are acceptible under imx6q_init_late?.
> 
>     if (!cpu_dev)
>         imx6q_opp_check_1p2ghz(cpu_dev);
> 
>     If not, what is the better way to impelement because the said function is board specific?.
> 
> Signed-off-by: John Tobias <john.tobias.ph@gmail.com>
> ---
>  arch/arm/mach-imx/mach-imx6q.c | 30 ++++--------------------------
>  1 file changed, 4 insertions(+), 26 deletions(-)
> 
> diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c
> index d0cfb22..f5d9e04 100644
> --- a/arch/arm/mach-imx/mach-imx6q.c
> +++ b/arch/arm/mach-imx/mach-imx6q.c
> @@ -183,31 +183,6 @@ put_node:
>  	of_node_put(np);
>  }
>  
> -static void __init imx6q_opp_init(void)
> -{
> -	struct device_node *np;
> -	struct device *cpu_dev = get_cpu_device(0);
> -
> -	if (!cpu_dev) {
> -		pr_warn("failed to get cpu0 device\n");
> -		return;
> -	}
> -	np = of_node_get(cpu_dev->of_node);
> -	if (!np) {
> -		pr_warn("failed to find cpu0 node\n");
> -		return;
> -	}
> -
> -	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);
> -}
>  
>  static struct platform_device imx6q_cpufreq_pdev = {
>  	.name = "imx6q-cpufreq",
> @@ -215,6 +190,7 @@ static struct platform_device imx6q_cpufreq_pdev = {
>  
>  static void __init imx6q_init_late(void)
>  {
> +	struct device *cpu_dev = get_cpu_device(0);
>  	/*
>  	 * WAIT mode is broken on TO 1.0 and 1.1, so there is no point
>  	 * to run cpuidle on them.
> @@ -223,8 +199,10 @@ static void __init imx6q_init_late(void)
>  		imx6q_cpuidle_init();
>  
>  	if (IS_ENABLED(CONFIG_ARM_IMX6Q_CPUFREQ)) {
> -		imx6q_opp_init();
>  		platform_device_register(&imx6q_cpufreq_pdev);
> +		
> +		if (!cpu_dev)
> +			imx6q_opp_check_1p2ghz(cpu_dev);

imx6q_opp_check_1p2ghz() only works after of_init_opp_table() is called.
So you shouldn't make these changes in mach-imx6q.c, but in cpufreq
driver check if platform already supplies an opp table, and only calls
of_init_opp_table() in driver in case that platform hasn't.  Then, for
imx6q case where opp table is supplied by platform, driver will skip
of_init_opp_table() call, while for imx6sl case where platform does not
handle opp table, driver will just call of_init_opp_table().

Shawn

>  	}
>  }
>  
> -- 
> 1.8.3.2
> 

Re: [PATCH 3/4] Removing imx6q_opp_init

[John Tobias]

Got it.

thanks

john

On Tue, Dec 17, 2013 at 11:49 PM, Shawn Guo <shawn.guo@linaro.org> wrote:
> On Tue, Dec 17, 2013 at 05:17:30PM -0800, John Tobias wrote:
>>     Since of_init_opp_table is in imx6q-cpufreq.c, the imx6q_opp_init is no longer needed anymore.
>>     The only question right now is the imx6q_opp_check_1p2ghz function.
>>
>>     Is the function below are acceptible under imx6q_init_late?.
>>
>>     if (!cpu_dev)
>>         imx6q_opp_check_1p2ghz(cpu_dev);
>>
>>     If not, what is the better way to impelement because the said function is board specific?.
>>
>> Signed-off-by: John Tobias <john.tobias.ph@gmail.com>
>> ---
>>  arch/arm/mach-imx/mach-imx6q.c | 30 ++++--------------------------
>>  1 file changed, 4 insertions(+), 26 deletions(-)
>>
>> diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c
>> index d0cfb22..f5d9e04 100644
>> --- a/arch/arm/mach-imx/mach-imx6q.c
>> +++ b/arch/arm/mach-imx/mach-imx6q.c
>> @@ -183,31 +183,6 @@ put_node:
>>       of_node_put(np);
>>  }
>>
>> -static void __init imx6q_opp_init(void)
>> -{
>> -     struct device_node *np;
>> -     struct device *cpu_dev = get_cpu_device(0);
>> -
>> -     if (!cpu_dev) {
>> -             pr_warn("failed to get cpu0 device\n");
>> -             return;
>> -     }
>> -     np = of_node_get(cpu_dev->of_node);
>> -     if (!np) {
>> -             pr_warn("failed to find cpu0 node\n");
>> -             return;
>> -     }
>> -
>> -     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);
>> -}
>>
>>  static struct platform_device imx6q_cpufreq_pdev = {
>>       .name = "imx6q-cpufreq",
>> @@ -215,6 +190,7 @@ static struct platform_device imx6q_cpufreq_pdev = {
>>
>>  static void __init imx6q_init_late(void)
>>  {
>> +     struct device *cpu_dev = get_cpu_device(0);
>>       /*
>>        * WAIT mode is broken on TO 1.0 and 1.1, so there is no point
>>        * to run cpuidle on them.
>> @@ -223,8 +199,10 @@ static void __init imx6q_init_late(void)
>>               imx6q_cpuidle_init();
>>
>>       if (IS_ENABLED(CONFIG_ARM_IMX6Q_CPUFREQ)) {
>> -             imx6q_opp_init();
>>               platform_device_register(&imx6q_cpufreq_pdev);
>> +
>> +             if (!cpu_dev)
>> +                     imx6q_opp_check_1p2ghz(cpu_dev);
>
> imx6q_opp_check_1p2ghz() only works after of_init_opp_table() is called.
> So you shouldn't make these changes in mach-imx6q.c, but in cpufreq
> driver check if platform already supplies an opp table, and only calls
> of_init_opp_table() in driver in case that platform hasn't.  Then, for
> imx6q case where opp table is supplied by platform, driver will skip
> of_init_opp_table() call, while for imx6sl case where platform does not
> handle opp table, driver will just call of_init_opp_table().
>
> Shawn
>
>>       }
>>  }
>>
>> --
>> 1.8.3.2
>>
>

[PATCH 4/4] Added cpufreq support for iMX6SL

[John Tobias]

    Re-using imx6q cpufreq driver

Signed-off-by: John Tobias <john.tobias.ph@gmail.com>
---
 arch/arm/mach-imx/mach-imx6sl.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-imx/mach-imx6sl.c b/arch/arm/mach-imx/mach-imx6sl.c
index 2f952e3..a8b1543 100644
--- a/arch/arm/mach-imx/mach-imx6sl.c
+++ b/arch/arm/mach-imx/mach-imx6sl.c
@@ -8,7 +8,6 @@
  */
 
 #include <linux/irqchip.h>
-#include <linux/of.h>
 #include <linux/of_platform.h>
 #include <linux/mfd/syscon.h>
 #include <linux/mfd/syscon/imx6q-iomuxc-gpr.h>
@@ -34,6 +33,17 @@ static void __init imx6sl_fec_init(void)
 	}
 }
 
+static struct platform_device imx6q_cpufreq_pdev = {
+	.name = "imx6q-cpufreq",
+};
+
+static void __init imx6sl_init_late(void)
+{
+	if (IS_ENABLED(CONFIG_ARM_IMX6Q_CPUFREQ)) {
+		platform_device_register(&imx6q_cpufreq_pdev);
+	}
+}
+
 static void __init imx6sl_init_machine(void)
 {
 	struct device *parent;
@@ -70,6 +80,7 @@ DT_MACHINE_START(IMX6SL, "Freescale i.MX6 SoloLite (Device Tree)")
 	.map_io		= debug_ll_io_init,
 	.init_irq	= imx6sl_init_irq,
 	.init_machine	= imx6sl_init_machine,
+	.init_late      = imx6sl_init_late,
 	.dt_compat	= imx6sl_dt_compat,
 	.restart	= mxc_restart,
 MACHINE_END
-- 
1.8.3.2

Re: [PATCH 1/4] iMX6SL frequency table

[Shawn Guo]

On Tue, Dec 17, 2013 at 05:17:28PM -0800, John Tobias wrote:
>     Device tree for iMX6SL doesn't have an existing cpu frequency table.

Drop these leading spaces.

> 
> Signed-off-by: John Tobias <john.tobias.ph@gmail.com>
> ---
>  arch/arm/boot/dts/imx6sl.dtsi | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)

For such imx6sl dts changes, please put a prefix 'ARM: dts: imx6sl: ...'
on the patch subject.

> 
> diff --git a/arch/arm/boot/dts/imx6sl.dtsi b/arch/arm/boot/dts/imx6sl.dtsi
> index 28558f1..0a2c73c 100644
> --- a/arch/arm/boot/dts/imx6sl.dtsi
> +++ b/arch/arm/boot/dts/imx6sl.dtsi
> @@ -38,6 +38,20 @@
>  			device_type = "cpu";
>  			reg = <0x0>;
>  			next-level-cache = <&L2>;
> +			operating-points = <
> +				/* kHz    uV */
> +				996000  1250000 /* for consumer grade only */
> +				792000  1150000
> +				396000  1050000

Please see my reply to your previous patch.

> +			>;
> +			clock-latency = <61036>; /* two CLK32 periods */
> +			clocks = <&clks IMX6SL_CLK_ARM>, <&clks IMX6SL_CLK_PLL2_PFD2>, <&clks IMX6SL_CLK_STEP>,
> +				 <&clks IMX6SL_CLK_PLL1_SW>, <&clks IMX6SL_CLK_PLL1_SYS>;

The lines are somehow too long.  The following one might be easier to
read.

			clocks = <&clks IMX6SL_CLK_ARM>,
				 <&clks IMX6SL_CLK_PLL2_PFD2>,
				 <&clks IMX6SL_CLK_STEP>,
				 <&clks IMX6SL_CLK_PLL1_SW>,
				 <&clks IMX6SL_CLK_PLL1_SYS>;

Shawn

> +			clock-names = "arm", "pll2_pfd2_396m", "step",
> +				      "pll1_sw", "pll1_sys";
> +			arm-supply = <&reg_arm>;
> +			pu-supply = <&reg_pu>;
> +			soc-supply = <&reg_soc>;
>  		};
>  	};
>  
> -- 
> 1.8.3.2
> 

Re: [PATCH 1/4] iMX6SL frequency table

[John Tobias]

Will update it base on your previous email.

Thanks,

john

On Tue, Dec 17, 2013 at 11:37 PM, Shawn Guo <shawn.guo@linaro.org> wrote:
> On Tue, Dec 17, 2013 at 05:17:28PM -0800, John Tobias wrote:
>>     Device tree for iMX6SL doesn't have an existing cpu frequency table.
>
> Drop these leading spaces.
>
>>
>> Signed-off-by: John Tobias <john.tobias.ph@gmail.com>
>> ---
>>  arch/arm/boot/dts/imx6sl.dtsi | 14 ++++++++++++++
>>  1 file changed, 14 insertions(+)
>
> For such imx6sl dts changes, please put a prefix 'ARM: dts: imx6sl: ...'
> on the patch subject.
>
>>
>> diff --git a/arch/arm/boot/dts/imx6sl.dtsi b/arch/arm/boot/dts/imx6sl.dtsi
>> index 28558f1..0a2c73c 100644
>> --- a/arch/arm/boot/dts/imx6sl.dtsi
>> +++ b/arch/arm/boot/dts/imx6sl.dtsi
>> @@ -38,6 +38,20 @@
>>                       device_type = "cpu";
>>                       reg = <0x0>;
>>                       next-level-cache = <&L2>;
>> +                     operating-points = <
>> +                             /* kHz    uV */
>> +                             996000  1250000 /* for consumer grade only */
>> +                             792000  1150000
>> +                             396000  1050000
>
> Please see my reply to your previous patch.
>
>> +                     >;
>> +                     clock-latency = <61036>; /* two CLK32 periods */
>> +                     clocks = <&clks IMX6SL_CLK_ARM>, <&clks IMX6SL_CLK_PLL2_PFD2>, <&clks IMX6SL_CLK_STEP>,
>> +                              <&clks IMX6SL_CLK_PLL1_SW>, <&clks IMX6SL_CLK_PLL1_SYS>;
>
> The lines are somehow too long.  The following one might be easier to
> read.
>
>                         clocks = <&clks IMX6SL_CLK_ARM>,
>                                  <&clks IMX6SL_CLK_PLL2_PFD2>,
>                                  <&clks IMX6SL_CLK_STEP>,
>                                  <&clks IMX6SL_CLK_PLL1_SW>,
>                                  <&clks IMX6SL_CLK_PLL1_SYS>;
>
> Shawn
>
>> +                     clock-names = "arm", "pll2_pfd2_396m", "step",
>> +                                   "pll1_sw", "pll1_sys";
>> +                     arm-supply = <&reg_arm>;
>> +                     pu-supply = <&reg_pu>;
>> +                     soc-supply = <&reg_soc>;
>>               };
>>       };
>>
>> --
>> 1.8.3.2
>>
>